Conversation
e266280 to
a60db3a
Compare
cachebag
changed the title
|
@rami3l Re: #3937; this PR is intentionally limited to replacing rustup’s own executable and does not modify toolchain transactions, but it introduces a self-update-specific staging directory, lock, and cleanup lifecycle. Does that overlap with the primitives you are developing for the process-safety goal, or do you think keeping self-update isolated here is reasonable? |
@cachebag Actually I think it can be done quite separately from the transactional semantics. As I added at the final minute comment when trying to merge the project goal, I specifically said that it would be only about concurrency problems of the Rust installations rather than those of the rustup installation (which this PR falls under): rust-lang/goals#731 (comment) |
2cdfb67 to
3b4cc2b
Compare
cachebag
changed the title
|
CI seems to have crapped out on us due to a GitHub issue. @rami3l does it let you re-run the failed jobs? |
bc36468 to
af1b039
Compare
This comment has been minimized.
This comment has been minimized.
af1b039 to
c7ebab5
Compare
This comment has been minimized.
This comment has been minimized.
c7ebab5 to
c1d76d8
Compare
dcef181 to
1dcce29
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
In terms of the end result this looks good. However as @djc has mentioned in #5077 (review), I have the feeling that the two main commits here have been way too juicy to review properly.
Also, this is introducing an API surface that is way too wide without a clear reason. For example, helpers like is_finished() have introduced no clear benefits and has slowed down the review process since the reviewer has to find the function and do the jump.
My perspective as the reviewer regarding atomic commits is that every should be as dumb as possible: the less juicy the meat of the feature actually is (and the fewer jumps I have to make when reading the diff file), the better.
Most commits in your PR should contain mostly tasteless changes with a clear single focus: renaming, extraction, inlining actions etc., that sandwich the actual feature changes, and when a PR contains too many such changes upfront, you should consider splitting some of them into one preparation PR or more.
A more concrete example here: WRT the staged updater, the cleanup of the stale file can be completely separated from the main feature, so probably you can split the change into two.
I totally understand that you might feel uncomfortable "inventing" intermediate states of your history, but as with every paper or code publication there is, some polishing to the "how we got there" is required for easier digestion.
7bb25fd to
80f6958
Compare
Import sibling items through `super` in the Windows module and move `DEFAULT_UPDATE_ROOT` below its users, as the coding standards prefer. No functional change.
Two concurrent `rustup self update` invocations shared one updater path and could overwrite each other's download or replacement (rust-lang#1864). `prepare_update` now takes a global self-update lock before downloading and hands it to `run_update` inside a `PreparedUpdater`, which releases it only once the replacer has been spawned. The replacer takes the same lock before replacing rustup, so `install_bins` becomes a method on the lock and can only run while it is held. The lock file lives under `$RUSTUP_HOME/self-update/` and is released by the OS when the owning process exits, so a crash can never leave it held.
…come Every rustup or proxy invocation deleted `$CARGO_HOME/bin/rustup-init` during startup cleanup. A proxy starting while the updater was still replacing rustup could therefore delete the updater out from under it (rust-lang#5076). The updater now lives at `$RUSTUP_HOME/self-update/rustup-init`, and the replacer records a `complete` or `failed` marker next to it once it is done. Startup cleanup removes the managed updater only when such a marker exists and the self-update lock is free, so an update in progress is never touched. The legacy path is still cleaned as before.
An updater whose replacer never ran, or crashed before recording an outcome, has no marker and was left behind forever. A legacy `$CARGO_HOME/bin/rustup-init` may still belong to an older rustup that is running it, so deleting it on sight is the very race being fixed. Both are now removed only after they have gone untouched for a day.
Replacement used to unlink the installed rustup and then copy the updater over the freed path. Any failure in between, such as the updater having been deleted meanwhile, left `$CARGO_HOME/bin` without a rustup at all. The new binary is now copied to a `.rustup-pending-*` sibling, synced to disk, and then renamed over the installed rustup. `std::fs::rename` replaces an existing destination in one step on every platform, so a failure before publication leaves the existing rustup untouched.
A crash between staging and publishing leaves a `.rustup-pending-*` file in `$CARGO_HOME/bin`. Startup cleanup now removes such files once they have gone untouched for a day, the same threshold used for abandoned updaters.
After spawning the replacer, the parent ran the updater a second time with `--version` and wrote the result to the uninstall registry entry. The registry could therefore claim a version that was never installed if the replacer went on to fail. The replacer is the new rustup and knows its own version, so it now updates `DisplayVersion` right after installing the binaries, under the same self-update lock. The test waits for the completion marker because the registry is now written after `rustup self update` has returned.
80f6958 to
38048e7
Compare
|
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. |
@rami3l @djc What do you think of the new structure? Let me know if there are any changes you'd want me to make. |
There was a problem hiding this comment.
@cachebag Thanks for the update! I think it is indeed looking a bit better now.
I really want to get this right before merging, so I'd like to invite the rest of @rust-lang/rustup to review as well just in case I've missed anything and/or they have some opinions regarding this patch.
| } | ||
|
|
||
| /// Tell the upgrader to replace the rustup bins, then delete | ||
| /// itself. |
There was a problem hiding this comment.
Nit: I think this docstring still applies?
| let status = prepared_updater.spawn_replacer()?.wait().context(format!( | ||
| "unable to wait for updater ({})", | ||
| setup_path.display() | ||
| ))?; |
There was a problem hiding this comment.
Nit: Pre-existing, but possibly we can use .with_context() instead of .context() to make the evaluation a bit lazier?
| // Get download path | ||
| let download_url = utils::parse_url(&url)?; | ||
| let prepared_updater = self_update_lock.prepare_updater(dl_cfg.process)?; | ||
|
|
There was a problem hiding this comment.
Nit: Adding let setup_path = prepared_updater.path(); here could save some space in the diff.
| } | ||
| } | ||
|
|
||
| pub(super) fn mark_result(process: &Process, succeeded: bool) { |
There was a problem hiding this comment.
Nit: According to our style guide, you should place the most interesting param upfront, so succeeded in mark_result() first, and then process.
| pub(super) fn cleanup(process: &Process) -> anyhow::Result<()> { | ||
| if let Some(lock) = SelfUpdateLock::try_acquire(process)? { | ||
| let updater = lock.updater_path(); | ||
| // A finished update has recorded its outcome; an abandoned one never will. |
There was a problem hiding this comment.
Is this claim a bit inaccurate?
If I have understood it correctly, it looks like "having markers" implies that an update has finished, but not having markers may or may not indicate a completed update, because the updater could have crashed exactly when trying to write the markers?
| } | ||
|
|
||
| impl SelfUpdateLock { | ||
| pub(super) fn acquire(process: &Process) -> anyhow::Result<Self> { |
There was a problem hiding this comment.
Nit: Following the naming convention, this series of methods are better named .lock() and .try_lock().
| let markers = [Marker::Complete, Marker::Failed]; | ||
| let finished = markers | ||
| .iter() | ||
| .any(|marker| marker.path(&lock.directory).is_file()); | ||
| if finished && utils::remove_file_best_effort("self-updater", &updater) { | ||
| for marker in markers { | ||
| utils::remove_file_best_effort( | ||
| "self-update status marker", | ||
| &marker.path(&lock.directory), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Question: Would you mind elaborating why the marker is required in addition to the lock and, if it is indeed needed, why do we have two of them?
The rationale here is not very clear to me at least through your docstrings and/or commit messages.
5 participants
Self-update currently stages every updater at the shared
$CARGO_HOME/bin/rustup-initpath. Any rustup proxy started while replacement is in progress can unlink that path during startup cleanup. On Unix, the replacement process then removes the installedrustup, fails to reopen its own deleted path, and leaves rustup missing. This appears to explain the failures reported in #1864, #4648, and #4777.The first commit adds a deterministic test for that race. It fails on
demo/self-update-race-beforeand the identical test passes with this branch.This PR uses a single managed updater under
$RUSTUP_HOME, guarded by an OS-released global self-update lock. It fully prepares and syncs a sibling binary before publishing it with rename on Unix or ReplaceFileW on Windows. Completed, failed, and abandoned artifacts are cleaned without racing an active update.The core result of this fix is that now, a failure before publication will leave the existing rustup untouched.
Fixes #5076
Fixes #1864