Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a flock to avoid concurrent initialization of project environments #11259

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Feb 5, 2025

Summary

If you uv run from the same directory via multiple processes at the same time, some of them will fail as they'll see an "incomplete" virtual environment.

Closes #11219.

@charliermarsh charliermarsh added the bug Something isn't working label Feb 5, 2025
@charliermarsh charliermarsh marked this pull request as ready for review February 5, 2025 19:53
/// Grab a file lock for the environment to prevent concurrent writes across processes.
pub(crate) async fn lock(workspace: &Workspace) -> Result<LockedFile, std::io::Error> {
LockedFile::acquire(
std::env::temp_dir().join(format!(
Copy link
Member

@zanieb zanieb Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this naughty? Aren't you supposed to grab some uv-specific tmp dir location?

(I thought we removed these intentionally in the past, the only remaining reference is in the trampolines)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh and

env::temp_dir().join(format!("uv-{}.lock", cache_digest(&self.0.root))),

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we do this elsewhere. I guess we could create a directory within env::temp_dir called uv?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I kind of think we should do this everywhere rather than creating .lock files in virtualenvs, etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought there was a very intentional reason we used co-located temporary files or the cache instead of /tmp

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if it's relevant here but OS tempdirs are popular sources of Different Filesystem, and moving a file across filesystems does not benefit from the same atomicity guarantees (it's like cp instead of mv).

Copy link
Contributor

@Gankra Gankra Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This is broadly possible with basically any random Other Directory, hence "do atomic moves/renames only in the ~same directory" is often advisable)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think this is where we landed in Discord — co-location is critical for any atomic rename operations.

@charliermarsh charliermarsh merged commit 311a96b into main Feb 5, 2025
73 checks passed
@charliermarsh charliermarsh deleted the charlie/synchronize-virtualenv branch February 5, 2025 20:19
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Feb 6, 2025
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.5.27` -> `0.5.29` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>astral-sh/uv (astral-sh/uv)</summary>

### [`v0.5.29`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0529)

[Compare Source](astral-sh/uv@0.5.28...0.5.29)

##### Enhancements

-   Add `--bare` option to `uv init` ([#&#8203;11192](astral-sh/uv#11192))
-   Add support for respecting `VIRTUAL_ENV` in project commands via `--active` ([#&#8203;11189](astral-sh/uv#11189))
-   Allow the project `VIRTUAL_ENV` warning to be silenced with `--no-active` ([#&#8203;11251](astral-sh/uv#11251))

##### Python

The managed Python distributions have been updated, including:

-   CPython 3.12.9
-   CPython 3.13.2
-   pkg-config files are now relocatable

See the [`python-build-standalone` release notes](https://github.com/astral-sh/python-build-standalone/releases/tag/20250205) for more details.

##### Bug fixes

-   Always use base Python discovery logic for cached environments ([#&#8203;11254](astral-sh/uv#11254))
-   Use a flock to avoid concurrent initialization of project environments ([#&#8203;11259](astral-sh/uv#11259))
-   Fix handling of `--all-groups` and `--no-default-groups` flags ([#&#8203;11224](astral-sh/uv#11224))

##### Documentation

-   Minor touchups to the Docker provenance docs ([#&#8203;11252](astral-sh/uv#11252))
-   Move content from the `mkdocs.public.yml` into the template ([#&#8203;11246](astral-sh/uv#11246))

### [`v0.5.28`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0528)

[Compare Source](astral-sh/uv@0.5.27...0.5.28)

##### Bug fixes

-   Allow discovering virtual environments from the first interpreter found on the `PATH` ([#&#8203;11218](astral-sh/uv#11218))
-   Clear ephemeral overlays when running tools ([#&#8203;11141](astral-sh/uv#11141))
-   Disable SSL in Git commands for `--allow-insecure-host` ([#&#8203;11210](astral-sh/uv#11210))
-   Fix hardlinks in tar unpacking ([#&#8203;11221](astral-sh/uv#11221))
-   Set base executable when returning virtual environment ([#&#8203;11209](astral-sh/uv#11209))
-   Use base Python for cached environments ([#&#8203;11208](astral-sh/uv#11208))

##### Documentation

-   Add documentation on verifying Docker image attestations ([#&#8203;11140](astral-sh/uv#11140))
-   Add `last updated` to documentation ([#&#8203;11164](astral-sh/uv#11164))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS4xNTguMSIsInVwZGF0ZWRJblZlciI6IjM5LjE1OC4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Project virtual environment creation is not concurrency-safe
3 participants