fix(test): test delete Windows registry accidently by baka-gourd · Pull Request #4918 · rust-lang/rustup

baka-gourd · 2026-06-21T13:17:39Z

This PR is intended to fix the issue mentioned in the ticket regarding the accidental deletion of existing registry entries. However, while investigating the problem, we also discovered a concurrency issue in RegistryGuard, so the scope of the fix is quite extensive.

Add per-test Windows registry isolation using a unique test UUID.
Route Windows registry helpers through test-specific registry subkeys when a UUID is present.
Update RegistryGuard to support multiple registry values and clean up the per-test registry subtree.
Forward the registry UUID through the test Config and CliTestContext.
Update Windows unit and integration tests to use UUID-scoped registry guards.
Keep PATH registry checks and Programs entry checks isolated in tests.
Treat missing registry keys or values as non-fatal during registry cleanup.
No new UUID dependency is introduced; UUIDs are generated from rand.

Cloud0310 · 2026-06-30T12:19:40Z

-static REGISTRY_LOCK: Mutex<()> = Mutex::new(());
+impl Drop for RegistryGuard {
+    fn drop(&mut self) {
+        for (id, prev) in self.saved.iter().rev() {


I guess since we're trying to remove the whole test subtree all together, no need for settting them empty again here?

View changes since the review

Cloud0310 · 2026-06-30T12:22:44Z

Another thing here is, can we make code format coming along with commits instead of doing formatting in the end commit here.

rustbot · 2026-08-05T11:20:18Z

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 · 2026-08-05T12:50:12Z

        }
        Ok(()) if !no_modify_path => {
            info!("removing cargo bin directory `{cargo_bin_display}` from $PATH");
            do_remove_from_path(process)?;


Moving do_remove_from_programs() out of this match{} is a separate change and has nothing to do with this refactoring commit.

Consider spitting this into a refactoring commit (that puts do_remove_from_programs() directly below this line) first, and a breaking change commit (moving the above call out of the match{} block) after that.

View changes since the review

rami3l · 2026-08-05T12:59:01Z

        }

+        #[cfg(windows)]
+        do_add_to_programs(cfg.process)?;


Same problem as 7ae30ea#r3720674968.

View changes since the review

rami3l · 2026-08-05T13:04:21Z

-            id,
-            prev: id.get()?,
-        })
+fn registry_sub_key_path(sub_key: &str, process: &Process) -> String {


Nit: This should be converted into a Windows-only method of the Process type.

View changes since the review

rami3l · 2026-08-05T13:31:52Z


+#[cfg(not(any(test, feature = "test")))]
+fn registry_sub_key_path(sub_key: &str, _process: &Process) -> String {
+    sub_key.to_owned()


There doesn't seem to be a need for this function.

View changes since the review

rami3l · 2026-08-05T13:35:00Z

 impl RegistryValueId {
-    pub fn get(&self) -> Result<Option<Value>> {
-        let sub_key = CURRENT_USER.create(self.sub_key)?;
+    fn resolved_sub_key(&self, uuid: Option<&str>) -> windows_registry::Result<Key> {


Suggest inlining this function.

View changes since the review

rami3l · 2026-08-05T13:52:10Z

 impl RegistryValueId {
-    pub fn get(&self) -> Result<Option<Value>> {
-        let sub_key = CURRENT_USER.create(self.sub_key)?;
+    fn resolved_sub_key(&self, uuid: Option<&str>) -> windows_registry::Result<Key> {


It seems that in every test case involving registry access, a UUID is needed anyways, if that is indeed true, I don't see a reason to pass Option<> here.

View changes since the review

rami3l · 2026-08-05T13:52:47Z

+pub struct RegistryGuard {
+    uuid: String,
+    saved: Vec<(&'static RegistryValueId, Option<Value>)>,
+}


If the analysis in d303f39#r3721131249 is correct, then we don't need a registry guard at all.

View changes since the review

rami3l · 2026-08-05T13:55:06Z

-            id,
-            prev: id.get()?,
-        })
+fn registry_sub_key_path(sub_key: &str, process: &Process) -> String {


When we are running tests, new registry keys and all its children should be created as volatile, so that we won't leave unintended garbage entries in the developer's machine (thanks, @Cloud0310).

View changes since the review

rami3l · 2026-08-05T13:56:09Z

    test_root_dir: PathBuf,
+    /// Per-test Windows registry UUID.
+    #[cfg(windows)]
+    pub test_registry_uuid: Option<String>,


It feels like this (01182b7) should be squashed into one of the previous commits; your goal should be that each commit would still be able to pass all the tests.

View changes since the review

rami3l

@baka-gourd I think this looks quite interesting but would require more work before it can be merged.

Three general principles of our committing style:

Each commit should do one single thing, whether that is refactoring, introducing a breaking change, or adding a new feature.
Each commit should ensure that all test cases are passing. If running all tests is not practical in your local environment, you should have run at least all related tests and have made sure that they are passing.
When a commit indicates a behavioral change that might cause some tests to fail, you should update those influenced tests in the same commit so that they pass again.

View changes since this review

…e_from_path

…o_path

… changes

baka-gourd force-pushed the windows-registry-per-test-uuid branch from a758fde to c4635ae Compare June 21, 2026 13:53

Cloud0310 reviewed Jun 30, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

baka-gourd force-pushed the windows-registry-per-test-uuid branch from 9be4399 to 360e048 Compare July 2, 2026 12:21

This comment has been minimized.

Sign in to view

baka-gourd requested a review from Cloud0310 July 2, 2026 12:54

rami3l self-assigned this Jul 3, 2026

This comment has been minimized.

Sign in to view

rami3l requested review from rami3l and removed request for Cloud0310 August 3, 2026 14:31

Cloud0310 reviewed Aug 4, 2026

View reviewed changes

Comment thread src/cli/self_update/windows.rs Outdated

baka-gourd force-pushed the windows-registry-per-test-uuid branch from 360e048 to dcd1d78 Compare August 5, 2026 11:20

rami3l reviewed Aug 5, 2026

View reviewed changes

baka-gourd added 4 commits August 5, 2026 22:38

refactor(self-update): extract Windows registry cleanup from do_remov…

593e14a

…e_from_path

refactor(self-update): extract Windows registry updates from do_add_t…

393cadc

…o_path

refactor(self-update)!: update Windows registry independently of PATH…

a79e280

… changes

feat(test): isolate Windows registry state per test

78a2fde

baka-gourd force-pushed the windows-registry-per-test-uuid branch from 6366e7f to 78a2fde Compare August 5, 2026 15:06

Uh oh!

Uh oh!

Conversation

baka-gourd commented Jun 21, 2026

Uh oh!

Cloud0310 Jun 30, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cloud0310 commented Jun 30, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

rustbot commented Aug 5, 2026

Uh oh!

rami3l Aug 5, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rami3l Aug 5, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rami3l Aug 5, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rami3l Aug 5, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rami3l Aug 5, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rami3l Aug 5, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rami3l Aug 5, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rami3l Aug 5, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rami3l Aug 5, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rami3l left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Cloud0310 Jun 30, 2026 •

edited by rustbot

Loading

rami3l Aug 5, 2026 •

edited by rustbot

Loading

rami3l Aug 5, 2026 •

edited by rustbot

Loading

rami3l Aug 5, 2026 •

edited by rustbot

Loading

rami3l Aug 5, 2026 •

edited by rustbot

Loading

rami3l Aug 5, 2026 •

edited by rustbot

Loading

rami3l Aug 5, 2026 •

edited by rustbot

Loading

rami3l Aug 5, 2026 •

edited by rustbot

Loading

rami3l Aug 5, 2026 •

edited by rustbot

Loading

rami3l Aug 5, 2026 •

edited by rustbot

Loading

rami3l left a comment •

edited by rustbot

Loading