Skip to content

fix: lock state and settings file updates#4946

Merged
rami3l merged 1 commit into
rust-lang:mainfrom
ychampion:fix-state-settings-file-locks
Jul 10, 2026
Merged

fix: lock state and settings file updates#4946
rami3l merged 1 commit into
rust-lang:mainfrom
ychampion:fix-state-settings-file-locks

Conversation

@ychampion

Copy link
Copy Markdown
Contributor

Summary

  • Add locked read/write helpers for file contents.
  • Use them for state.toml and settings.toml reads and writes.
  • Cover locked write truncation with a unit test.

Why

Fixes #4942. These files can be touched by concurrent rustup processes, so writes now take an exclusive lock before truncating and reads take a shared lock.

Validation

  • cargo test --features test locked_file_round_trip_truncates_previous_contents — passed
  • cargo test --features test settings::tests — passed
  • cargo test --features test --test test_bonanza notify_release_hint — passed
  • cargo check --all-targets --features test — passed
  • cargo clippy --all-targets --features test -- -D warnings — passed
  • cargo fmt --all -- --check — passed
  • git diff --check — passed

Comment thread src/utils/raw.rs

@rami3l rami3l left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks! Please squash the two commits into one and I'll merge it shortly after.

View changes since this review

@ychampion ychampion force-pushed the fix-state-settings-file-locks branch from 6c613d6 to 8ba412f Compare July 9, 2026 13:36
@ychampion

Copy link
Copy Markdown
Contributor Author

Squashed into one commit now. I reran the locked-file regression plus fmt/diff checks after the rewrite.

@rami3l

rami3l commented Jul 9, 2026

Copy link
Copy Markdown
Member

@ychampion The CI is failing but it's probably not your fault; it might be due to an upstream update of Rust.

Since we are at it, would you mind fixing the clippy warnings in a separate commit? Many thanks in advance 🙏

@djc djc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Linting issues might be from Rust 1.97 release that just dropped.

Personally I don't like splitting this code between utils/mod.rs and utils/raw.rs; maybe just keep the "raw" variants just below the utils functions?

View changes since this review

@rami3l

rami3l commented Jul 9, 2026

Copy link
Copy Markdown
Member

Personally I don't like splitting this code between utils/mod.rs and utils/raw.rs; maybe just keep the "raw" variants just below the utils functions?

@djc I see your point but refactoring PRs can always follow this one... I would prefer not putting too much pressure on first-time contributors in fixing our pre-existing issues WRT code organization, if you agree 🙏

@djc

djc commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

@djc I see your point but refactoring PRs can always follow this one... I would prefer not putting too much pressure on first-time contributors in fixing our pre-existing issues WRT code organization, if you agree 🙏

I might agree but I guess I put less value on seemingly drive-by contributions that seem clearly LLM-assisted.

@ychampion

Copy link
Copy Markdown
Contributor Author

Done in 42a5b3e as a separate commit.

Validation:

  • cargo clippy --all-targets --all-features -- -D warnings on Rust 1.97.0
  • cargo fmt --all -- --check
  • cargo test --features test --test test_bonanza add_target -- --nocapture

@ychampion

Copy link
Copy Markdown
Contributor Author

@djc That's fair. I can move the raw helpers back beside the existing utils functions if you'd prefer that in this PR; I kept the latest push to the requested Rust 1.97 Clippy-only cleanup.

@djc

djc commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

@djc That's fair. I can move the raw helpers back beside the existing utils functions if you'd prefer that in this PR; I kept the latest push to the requested Rust 1.97 Clippy-only cleanup.

Okay, let's keep it as is for now. Would be nice if you can clean up single caller raw variants in a next PR.

Please rebase, since the Clippy fixes are already on main.

Constraint: rustup state and settings files can be touched by concurrent rustup processes.
Rejected: truncating on open | The exclusive lock must be held first.
Confidence: high
Scope-risk: narrow
Directive: Keep locked-file read/write changes focused on state/settings persistence semantics.
Tested: cargo test --features test locked_file_round_trip_truncates_previous_contents; cargo test --features test settings::tests; cargo test --features test --test test_bonanza notify_release_hint; cargo check --all-targets --features test; cargo clippy --all-targets --features test -- -D warnings; cargo fmt --all -- --check; git diff --check
Not-tested: full test_bonanza suite
@ychampion ychampion force-pushed the fix-state-settings-file-locks branch from 42a5b3e to 702fcb5 Compare July 10, 2026 12:56
@rustbot

rustbot commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@ychampion

Copy link
Copy Markdown
Contributor Author

Rebased onto current main. The separate Clippy cleanup dropped out because it is already present there; the locking change itself is unchanged.

@rami3l rami3l enabled auto-merge July 10, 2026 13:05
@rami3l rami3l added this pull request to the merge queue Jul 10, 2026
Merged via the queue into rust-lang:main with commit cc40f2b Jul 10, 2026
31 checks passed
@rami3l

rami3l commented Jul 12, 2026

Copy link
Copy Markdown
Member

That's fair. I can move the raw helpers back beside the existing utils functions if you'd prefer that in this PR; I kept the latest push to the requested Rust 1.97 Clippy-only cleanup.

Okay, let's keep it as is for now. Would be nice if you can clean up single caller raw variants in a next PR.

@ychampion Would you mind evaluating the proposed clean-up of the raw module and, if feasible, make a follow-up PR? Many thanks in advance 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use File::lock*() when reading/writing state and settings files

4 participants