From 6ced06a1b337cdf403b1e53407cac9cb156b856b Mon Sep 17 00:00:00 2001 From: r-vdp Date: Wed, 20 May 2026 19:46:27 +0200 Subject: [PATCH] switch-to-configuration-ng: rework user-unit migration candidate selection The previous "FragmentPath not under /etc" deny-list swept up units that were never managed by a per-user manager (e.g. dbus-broker, whose FragmentPath systemd reports under /run/current-system/sw/share via systemd.packages) and missed nothing it should have caught, but for the wrong reason. Make the intent explicit. A unit is a migration candidate iff it is active, the new generation defines it in /etc/systemd/user, and either * its FragmentPath is under $XDG_CONFIG_HOME/systemd/user (the home-manager case; ~/.config shadows /etc, so we must wait for sd-switch to remove the copy), or * its FragmentPath is anywhere else outside /etc and the previous generation did not have it in /etc (package-shipped units found via $XDG_DATA_HOME / $XDG_DATA_DIRS, e.g. ~/.nix-profile/share; /etc outranks these so it wins on daemon-reload). The "previous generation did not have it" guard keeps units that have always been in /etc, but whose FragmentPath systemd reports elsewhere, out of the candidate set, and the existing now_etc check verifies /etc actually won before acting. Compare FragmentPath by parent directory instead of string prefix while here. Covered by a new switch-test case that seeds a unit in ~/.local/share/systemd/user. --- nixos/tests/switch-test.nix | 39 +++++++++ .../sw/switch-to-configuration-ng/src/main.rs | 85 +++++++++++++++---- 2 files changed, 108 insertions(+), 16 deletions(-) diff --git a/nixos/tests/switch-test.nix b/nixos/tests/switch-test.nix index e83e43430fae..4126645f59e0 100644 --- a/nixos/tests/switch-test.nix +++ b/nixos/tests/switch-test.nix @@ -826,6 +826,15 @@ in RemainAfterExit=true ExecStart=${pkgs.runtimeShell} -c 'echo home > %t/migrated-owner' ''; + + # Unit file placed in ~/.local/share/systemd/user (lower priority than + # /etc) to simulate a package-shipped unit. + dataMigratedUnit = pkgs.writeText "migrated.service" '' + [Service] + Type=oneshot + RemainAfterExit=true + ExecStart=${pkgs.runtimeShell} -c 'echo data > %t/migrated-owner' + ''; in # python '' @@ -1857,6 +1866,36 @@ in out = machine.succeed(f"sudo -u usertest {user_env} cat /run/user/1001/migrated-owner") assert_contains(out, "home") + # Migration from a lower-priority search-path entry ($XDG_DATA_HOME + # here, standing in for ~/.nix-profile/share etc.). /etc outranks + # these, so pass 2 must restart onto the /etc definition. + switch_to_specialisation("${machine}", "") + machine.fail(f"sudo -u usertest {user_env} systemctl --user is-active migrated.service") + machine.succeed( + "sudo -u usertest mkdir -p ~usertest/.local/share/systemd/user", + "sudo -u usertest cp ${dataMigratedUnit} ~usertest/.local/share/systemd/user/migrated.service", + ) + user_systemctl("daemon-reload") + user_systemctl("start migrated.service") + user_systemctl("is-active migrated.service") + out = machine.succeed(f"sudo -u usertest {user_env} cat /run/user/1001/migrated-owner") + assert_contains(out, "data") + out = user_systemctl("show -p FragmentPath migrated.service") + assert_contains(out, "/.local/share/systemd/user/migrated.service") + out = switch_to_specialisation("${machine}", "userServiceMigratedShadowed") + assert_contains(out, "restarting (post-activation) the following user units: migrated.service") + user_systemctl("is-active migrated.service") + out = user_systemctl("show -p FragmentPath migrated.service") + assert_contains(out, "/etc/systemd/user/migrated.service") + out = machine.succeed(f"sudo -u usertest {user_env} cat /run/user/1001/migrated-owner") + assert_contains(out, "nixos") + # Switching again must NOT touch it: /etc already had it, so it is + # not a candidate even though the lower-priority copy is still there. + out = switch_to_specialisation("${machine}", "userServiceMigratedShadowed") + assert_lacks(out, "migrated.service") + machine.succeed("sudo -u usertest rm -rf ~usertest/.local/share/systemd") + user_systemctl("daemon-reload") + # Units that remain shadowed by ~/.config must be left alone in both # passes even though /etc now also defines them. switch_to_specialisation("${machine}", "") diff --git a/pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs b/pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs index 6046d3437ae0..bebf3c46a4bf 100644 --- a/pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs +++ b/pkgs/by-name/sw/switch-to-configuration-ng/src/main.rs @@ -1395,26 +1395,79 @@ fn do_user_switch(parent_exe: String) -> anyhow::Result<()> { let current_active_units = get_active_units(&systemd)?; + let old_unit_dir = old_toplevel.join(scope.etc_dir()); let new_unit_dir = toplevel.join(scope.etc_dir()); - let fragment_prefix = scope - .current_dir() - .to_str() - .expect("scope dir is valid UTF-8"); + let fragment_dir = scope.current_dir(); - // Units that are currently running from a non-/etc location (typically - // ~/.config/systemd/user, i.e. home-manager) but that the new NixOS - // configuration also defines. Pass 1 will skip these because of the - // FragmentPath filter; if the per-user activation (sd-switch) later drops - // its copy, we need a second pass to bring the NixOS-owned definition up. + // Determine $XDG_CONFIG_HOME/systemd/user from the user manager's own + // environment (we are spawned with env_clear()). + let user_config_unit_dir: Option = match systemd.environment() { + Err(err) => { + log::debug!("Failed to read user manager environment: {err}"); + None + } + Ok(env) => { + let lookup = |key: &str| { + env.iter().find_map(|kv| { + kv.strip_prefix(key) + .and_then(|rest| rest.strip_prefix('=')) + .filter(|v| Path::new(v).is_absolute()) + .map(PathBuf::from) + }) + }; + let config_home = + lookup("XDG_CONFIG_HOME").or_else(|| lookup("HOME").map(|h| h.join(".config"))); + if config_home.is_none() { + log::debug!( + "Neither $XDG_CONFIG_HOME nor $HOME is set in the user manager's environment" + ); + } + config_home.map(|config_home| config_home.join("systemd/user")) + } + }; + + if user_config_unit_dir.is_none() { + log::debug!( + "Could not determine $XDG_CONFIG_HOME/systemd/user; \ + units shadowed by ~/.config will not be considered for migration" + ); + } + + // Units active from a non-/etc location that the new generation defines + // in /etc/systemd/user. Pass 1 skips these (FragmentPath filter); pass 2 + // brings the /etc definition into effect once /etc has won. Two cases: + // * ~/.config/systemd/user (home-manager): shadows /etc, so wait for + // the per-user activation (sd-switch) to remove its copy. + // * anywhere else outside /etc ($XDG_DATA_HOME, $XDG_DATA_DIRS, ...): + // /etc outranks these, so only act when /etc is gaining the unit; + // if the previous generation already had it, leave it alone. + // Pass 2's `now_etc` check verifies /etc actually won before acting. let migration_candidates: Vec = current_active_units .iter() .filter(|(unit, _)| new_unit_dir.join(unit).exists()) - .filter(|(_, unit_state)| { - !unit_state + .filter(|(unit, unit_state)| { + let Ok(fragment_path) = unit_state .proxy - .get("org.freedesktop.systemd1.Unit", "FragmentPath") - .map(|p: String| p.starts_with(fragment_prefix)) - .unwrap_or(false) + .get::("org.freedesktop.systemd1.Unit", "FragmentPath") + else { + return false; + }; + let fragment_parent = Path::new(&fragment_path).parent(); + + // Already in /etc: handled by pass 1. + if fragment_parent == Some(fragment_dir) { + return false; + } + + // Loaded from ~/.config/systemd/user, which shadows /etc. + if let Some(dir) = &user_config_unit_dir { + if fragment_parent == Some(dir.as_path()) { + return true; + } + } + + // Elsewhere: only act if /etc is gaining the unit this switch. + !old_unit_dir.join(unit).exists() }) .map(|(unit, _)| unit.clone()) .collect(); @@ -1422,7 +1475,7 @@ fn do_user_switch(parent_exe: String) -> anyhow::Result<()> { collect_unit_changes( &toplevel, scope, - &old_toplevel.join(scope.etc_dir()), + &old_unit_dir, &new_unit_dir, ¤t_active_units, &mut units_to_stop, @@ -1577,7 +1630,7 @@ fn do_user_switch(parent_exe: String) -> anyhow::Result<()> { let now_etc = unit_state .proxy .get("org.freedesktop.systemd1.Unit", "FragmentPath") - .map(|p: String| p.starts_with(fragment_prefix)) + .map(|p: String| Path::new(&p).parent() == Some(fragment_dir)) .unwrap_or(false); if !now_etc { // Still shadowed (or read error); leave it alone.