Skip to content

cargo metadata call alters lockfile despite --frozen r-a cargo configuration #19729

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

Open
obi1kenobi opened this issue May 1, 2025 · 5 comments
Labels
A-cargo cargo related issues Broken Window Bugs / technical debt to be addressed immediately C-bug Category: bug

Comments

@obi1kenobi
Copy link
Member

obi1kenobi commented May 1, 2025

rust-analyzer version: 0.3.2441-standalone (d8887c0758 2025-04-26)

rustc version: `rustc 1.85.1 (4eb161250 2025-03-15)

editor or extension: Cursor v0.49.6 based on VSCode 1.96.2 + official rust-lang.rust-analyzer extension version 0.3.2441

relevant settings: VSCode workspace settings file containing

"rust-analyzer.cargo.extraArgs": ["--frozen"],
"rust-analyzer.cargo.extraEnv": { "CARGO_NET_OFFLINE": "true" },
"rust-analyzer.cargo.targetDir": true,

What I expected to happen: r-a cannot cause Cargo.lock changes, due to "rust-analyzer.cargo.extraArgs": ["--frozen"]

What actually happens: r-a occasionally causes Cargo.lock changes, due to a cargo metadata call that doesn't supply --frozen

How I verified this is from r-a and not something else:

  • replaced cargo with a shim that logs full invocations
  • this recorded an r-a invocation of cargo metadata --format-version 1 --manifest-path /path/to/Cargo.toml --filter-platform <my-platform> — note this did not include the configured --frozen
  • git then reported changes in Cargo.lock

How this happens in the real world

In the real-world this happens due to a "torn read" race condition versus git checkout. I'll describe the real-world situation first for intuition-building purposes, then offer a way to simulate the outcome of the race for easier debugging.

Consider a large repo where a git checkout may take a noticeable amount of time. Say we move from a newer branch to an older one, where the newer branch had updated some dependencies and picked up new ones.

  • git checkout <older-branch>
  • mid-checkout, imagine Cargo.toml becomes updated but the matching Cargo.lock change isn't applied yet — git is busy updating other files and hasn't gotten to it yet
  • at this instant, the VSCode extension runs r-a
  • r-a runs cargo metadata without --frozen, despite the configuration (as mentioned above)
  • (not positive about this, but possible) "rust-analyzer.cargo.targetDir": true means the lockfile isn't locked during this process
  • cargo metadata reads Cargo.toml and Cargo.lock, and continues running (A)
  • while cargo metadata is in flight, the git checkout updates Cargo.lock (B)
  • cargo notices that it's getting a different resolution: some of the dependencies present in the Cargo.lock it read at (A) are superfluous, so it rewrites Cargo.lock to exclude them — but it leaves the updated versions of other still-used dependencies (C)
  • (C) happens after (B), so git's checkout of Cargo.lock at (B) is overwritten by (C)

At this point, both the checkout and the r-a run are complete. A subsequent git status shows Cargo.lock has newer versions of some dependencies — a hybrid lockfile between the newer and older branch.

Simulating the torn read

The easiest way to simulate the race is:

git checkout -b older-branch
git checkout -b newer-branch
cargo update  # to cause existing dependencies' versions to be updated to newer versions
cargo add <some-new-dependency>
git commit -m "lockfile bump"

# switch to the older branch but check out the newer branch's lockfile
git checkout older-branch
git checkout newer-branch Cargo.lock

# at this point, we've set up the "torn read" condition manually;
# cargo commands like `cargo check` or `cargo metadata` will update
# `Cargo.lock` to remove `<some-new-dependency>` from the lockfile
# but will *preserve* the `cargo update` outcome otherwise,
# creating a new `Cargo.lock` that is unlike either branch's contents

r-a origin of the cargo metadata call

AFAICT the cargo metadata invocation in question is coming from this block, which explicitly passes locked: false. There's a similar block above it, for the rustc_dir case when there's a custom sysroot, which might also be similarly affected — I'm not sure.

Happy to provide any more info or help however I can. Thanks for looking into this!

@obi1kenobi obi1kenobi added the C-bug Category: bug label May 1, 2025
@Veykril
Copy link
Member

Veykril commented May 1, 2025

The cargo metadata call by rust-analyzer is currently not configurable at all.

Iirc the main reason why we don't pass --locked by default is that it, well, will cause cargo metadata to fail if it does need to update which in turn means no package graph output and as such no working rust-analyzer. So we are unfortunately forced to not pass the flag by default. This once again goes back to cargo metadata not being a read-only operation despite looking like one.

@Veykril Veykril added the A-cargo cargo related issues label May 1, 2025
@obi1kenobi
Copy link
Member Author

Any advice on how to avoid the torn read race condition? It's really vexing (and raises security teams' eyebrows) when a user's git checkout command with an editor open results in a "dirty" git state with a Cargo.lock change.

Would it be possible, say, to have r-a run cargo metadata locked, backing off on failure a few times, and only then run unlocked — or optionally per a config setting, fail hard with an error?

@Veykril
Copy link
Member

Veykril commented May 2, 2025

Back off seems problematic to me (implementation/complexity wise). So a config would be an option. Alternatively I wonder if the error message cargo gives for when the invocation fails due to --locked is recognizable enough that we can identify it and ask the user if updating the locked file is okay (in which case we re-run the command on button press without the locked flag).

@obi1kenobi
Copy link
Member Author

I think those are good options. Just checking my intuition: let's say in either way we make r-a not update the lockfile. Then:

  • Cargo.toml changes, r-a sees the torn state and bails because of the non-matching lockfile
  • git eventually also updates Cargo.lock, r-a sees that too and runs successfully this time
  • from the user's perspective, nothing weird happened and everything is normal, right?

In this case, my slight preference from a workflow UX perspective would be for not popping up a notification, because the issue fixed itself already. So I'd personally prefer the config option.

But like I said, both options you mentioned are strongly preferable over the status quo. I'd be happy with either.

Thanks again!

@Veykril
Copy link
Member

Veykril commented May 17, 2025

So I was talking a bit with people at RustWeek, turns out there should be a fairly simple fix to this. We should just debounce the metadata call by 50-100ms after the last file change notification. That way we should be deferring things until git is done.

@Veykril Veykril added the Broken Window Bugs / technical debt to be addressed immediately label May 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cargo cargo related issues Broken Window Bugs / technical debt to be addressed immediately C-bug Category: bug
Projects
None yet
Development

No branches or pull requests

2 participants