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
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.
80f6958 to
38048e7
Compare
This comment has been minimized.
This comment has been minimized.
@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.
| } | ||
|
|
||
| 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().
There was a problem hiding this comment.
I will withhold it until #5077 (comment) is settled, since that may reshape these methods anyway.
| 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.
There was a problem hiding this comment.
The lock only says whether an update is running right now. After the parent exits and before the replacer takes the lock, cleanup sees a free lock and an updater on disk and can’t tell the difference between being finished or queued to run. Deleting it in the second case is the bug.
The marker is the “done” signal. Without it the only safe option is age, and it doesn't seem conducive to then let every updated sit around for a day.
why do we have two of them?
I guess cleanup treats them the same. failed only helps the test fail fast and leaves a trace on Windows.
| pub(super) fn prepare_updater(self, process: &Process) -> anyhow::Result<PreparedUpdater> { | ||
| let path = process | ||
| .cargo_home()? | ||
| .join(format!("bin/rustup-init{EXE_SUFFIX}")); | ||
| utils::ensure_file_removed("self-updater", &path)?; | ||
| Ok(PreparedUpdater { path, _lock: self }) | ||
| } |
There was a problem hiding this comment.
So I think this is effectively a type state thing where we're taking the SelfUpdateLock and wrapping the contained lock into a PreparedUpdater, right? I think it might be simpler to redefine SelfUpdateLock as a type with a generic type parameter somewhat like Mutex, with a Deref impl to replace PreparedUpdater::path().
There was a problem hiding this comment.
My hesitation with this is the payload is only ever a PathBuf, so it’d be one type parameter for one call site, and Deref to a path hides what’s being handed around.
| /// Unlike [`remove_file`], a busy file is left alone instead of retried, since | ||
| /// callers use this for cleanup that another process may legitimately still be | ||
| /// using. Returns whether `path` is gone afterwards. | ||
| pub(crate) fn remove_file_best_effort(name: &str, path: &Path) -> bool { |
There was a problem hiding this comment.
Unless/until this is used for other callers, suggest keeping it where it is used.
There was a problem hiding this comment.
I agreed with what @ChrisDenton said here #5077 (comment).
wdyt?
38048e7 to
6bd5c62
Compare
@djc Re: a separate PR; I suppose that is fine...ab0d89a and 1ca3033 can be by themselves, though what should I do with fc00f5f? The whole point of that test is that it fails without the fix. |
6bd5c62 to
f5c76de
Compare
This comment has been minimized.
This comment has been minimized.
@cachebag Sorry for the late reply! As per our current contribution guidelines, each commit is supposed to pass the test suite on its own, so we encourage adding a test upfront demonstrating the current behavior (even the current behavior is an error being thrown) upfront, and when the fix arrives, the test case is updated accordingly. |
f5c76de to
1aba886
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. |
A process parked at a checkpoint could only be killed by the test driver. Poll for the checkpoint marker instead of sleeping, and add `ParkedChild::resume()`, which removes the marker and waits for the process to finish. This lets a test race another command against a paused operation and then observe how the paused one completes.
Park the replacer right before it installs the new binaries and run a proxy in the meantime. The proxy's startup cleanup deletes `$CARGO_HOME/bin/rustup-init`, which is the replacer's own executable, so the replacer unlinks the installed rustup and then fails to copy itself over it, leaving no rustup behind (rust-lang#5076, rust-lang#1864). The test asserts this current behaviour so that it passes on its own; the fix later in this series updates it to assert that the replacement succeeds.
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. The proxy cleanup race test now asserts that the replacement succeeds and the installed rustup is updated.
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.
1aba886 to
ecfbdd4
Compare
|
@rami3l understood, thank you for the feedback that makes a lot of sense. this is how i've decided to split it out then:
i ran the race test at each of those commits locally to confirm it passes at every step. no rush of course: but i've left some threads open with questions/comments on your review(s). |
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