diff --git a/hermes_cli/gateway_migrate.py b/hermes_cli/gateway_migrate.py index bd1c7ae860..6dca0ba71b 100644 --- a/hermes_cli/gateway_migrate.py +++ b/hermes_cli/gateway_migrate.py @@ -88,8 +88,10 @@ class MigrationPlan: profiles: list[ProfileGateway] multiplex_flag_on: bool live_served: Optional[list[str]] # served_profiles the live default gateway recorded, if any - # A manifest with the flag on and no live default gateway: an earlier apply died between flipping - # the flag and bringing the multiplexer up (#110850). Not "already multiplexed" — resumable. + # A manifest with the flag on and no LIVE default gateway: an earlier apply died between flipping + # the flag and the multiplexer confirming it is up (#110850). An installed unit is not proof of + # anything — `systemd_install` writes the unit before the start that can still fail or be killed. + # Not "already multiplexed" — resumable. interrupted: bool = False blockers: list[str] = field(default_factory=list) notices: list[str] = field(default_factory=list) @@ -449,7 +451,7 @@ def build_migration_plan() -> MigrationPlan: multiplex_flag_on=_read_multiplex_flag(default_home), live_served=recorded_served_profiles(default_home), ) - plan.interrupted = plan.multiplex_flag_on and not plan.default.has_gateway and _read_manifest(default_home) is not None + plan.interrupted = plan.multiplex_flag_on and _manifest_not_yet_served(_read_manifest(default_home), plan.live_served) if len(plan.profiles) < 2: plan.notices.append("Only one profile exists: nothing to multiplex.") return plan @@ -487,7 +489,7 @@ def format_plan(plan: MigrationPlan, *, dry_run: bool) -> list[str]: return lines if plan.interrupted: lines.append(f" ↻ An earlier migration was interrupted before the default gateway came up " - f"(flag on, no gateway; manifest {plan.default_home / MANIFEST_NAME}); this run resumes it.") + f"(flag on, no live multiplexer; manifest {plan.default_home / MANIFEST_NAME}); this run resumes it.") steps = [] for p in plan.standalone_secondaries: what = " + ".join(x for x in (f"stop pid {p.pid}" if p.pid else "", f"uninstall {p.service_label()}" if p.services else "") if x) @@ -615,6 +617,19 @@ def _read_manifest(default_home: Path) -> Optional[dict]: return data if isinstance(data, dict) else None +def _manifest_not_yet_served(manifest: Optional[dict], live_served: Optional[list[str]]) -> bool: + """The postcondition ``apply_migration`` waits for, re-derived from live state: a LIVE default that + recorded serving every profile the manifest migrated. Anything less — no live gateway, an + installed-but-dead unit (``systemd_install`` writes the unit before the start that can still fail), + a standalone default never restarted — is a half-applied migration, not "already multiplexed". + Profiles created after the migration are not in the manifest, so they cannot flag it as interrupted.""" + if manifest is None: + return False + recs = [r for r in (manifest.get("default"), *(_manifest_secondaries(manifest) or [])) if isinstance(r, dict)] + migrated = {str(r.get("profile") or "default") for r in recs} | {"default"} + return not migrated <= set(live_served or []) + + def _write_manifest(default_home: Path, data: dict) -> None: from utils import atomic_json_write @@ -691,14 +706,48 @@ def _remove_secondary_gateways(plan: MigrationPlan) -> None: print(f" ✓ {p.name}: stopped standalone gateway (pid {p.pid})") +def _preflight_apply(plan: MigrationPlan, target: Optional[tuple[str, bool]], run_as_user: Optional[str]) -> Optional[str]: + """A failure of the destructive phase that is knowable from the plan alone, refused BEFORE any + working per-profile gateway is stopped: rollback is the fallback for surprises, not the plan. + Mirrors the checks ``systemd_install``/``_service_call`` make on a system unit (root, resolvable + ``User=``) and the config write's read-guard.""" + from hermes_cli import gateway as gw + from hermes_cli.config import require_readable_config_before_write + try: + require_readable_config_before_write(plan.default_home / "config.yaml") + except Exception as exc: + return f"default: config.yaml cannot be updated ({exc})" + touches_system_unit = target == ("systemd", True) or any(p.has_system_unit for p in plan.standalone_secondaries) + if touches_system_unit: + try: + gw._require_root_for_system_service("migration") + except Exception as exc: + return str(exc) + if plan.default.service is None and target == ("systemd", True): + if run_as_user is None: + try: + gw._system_service_identity() # the #110850 refusal (implicit root), before anything is removed + except ValueError as exc: + return f"default: {exc}" + else: + import pwd + try: + pwd.getpwnam(run_as_user) + except KeyError: + return f"default: the recorded service user '{run_as_user}' does not exist on this host" + return None + + def apply_migration(plan: MigrationPlan, *, served_wait: float = _SERVED_WAIT_SECONDS) -> bool: - """Stop/uninstall every secondary gateway, flip the flag, bring up the multiplexer, verify. + """Flip the flag, stop/uninstall every secondary gateway, bring up the multiplexer, verify. Returns True when the multiplexer verifiably serves every profile. - Bringing the default up is the one step that can fail after the destructive ones (a system unit - that needs ``--run-as-user``, an unreachable user bus). It runs inside a rollback: on failure the - manifest written before the first destructive step restores the flag and every recorded per-profile - gateway (#110850), so the fleet never ends with the flag on and no gateway at all.""" + Every step after the manifest write is fallible (a config write, a secondary's stop or its + unit's daemon-reload, a system unit that needs ``--run-as-user``, an unreachable user bus) and + runs inside ONE compensating boundary: on failure the manifest written before the first + destructive step restores the flag and every recorded per-profile gateway (#110850), so the + fleet never ends half-migrated. The flag goes on first so an apply killed anywhere after it is + resumable from the manifest (flag on + manifest + no live multiplexer = interrupted).""" if plan.blocked: _print(["✗ Migration refused:", *[f" • {b}" for b in plan.blockers]]) return False @@ -707,18 +756,22 @@ def apply_migration(plan: MigrationPlan, *, served_wait: float = _SERVED_WAIT_SE return True target, run_as_user = plan.target_service_kind(), plan.target_run_as_user() if plan.interrupted: - # An earlier apply removed the secondaries and flipped the flag but never brought the default - # up; the manifest is the only record of the units that existed. Finish from it, don't rewrite it. + # An earlier apply flipped the flag (and removed some or all secondaries) but the default never + # came up; the manifest is the only record of the units that existed. Finish from it, don't rewrite it. manifest = _read_manifest(plan.default_home) or {} target, run_as_user = _target_from_manifest(manifest) print(f" ↻ resuming an interrupted migration recorded in {_manifest_path(plan.default_home)}") elif _read_manifest(plan.default_home) is not None: - # Flag off + manifest present = a rollback that did not finish. Overwriting the manifest would - # discard the only record of the units that rollback still has to restore. + # Flag off + manifest present = a rollback (or an apply killed before its flag write) that did + # not finish. Overwriting the manifest would discard the only record of the units to restore. _print([f"✗ A previous migration's manifest is still at {_manifest_path(plan.default_home)} (its rollback did not finish).", " Finish it with: hermes gateway migrate --standalone (or delete the manifest to start over)"]) return False else: + blocker = _preflight_apply(plan, target, run_as_user) + if blocker is not None: + _print(["✗ Migration refused before changing anything:", f" • {blocker}"]) + return False manifest = { "version": 1, "migrated_at": time.strftime("%Y-%m-%dT%H:%M:%S%z"), "flag_was": plan.multiplex_flag_on, @@ -728,13 +781,13 @@ def apply_migration(plan: MigrationPlan, *, served_wait: float = _SERVED_WAIT_SE # Recovery metadata must exist before the first destructive operation; the manifest never # changes afterwards, so this is the only write it needs. _write_manifest(plan.default_home, manifest) - _remove_secondary_gateways(plan) - _write_multiplex_flag(plan.default_home, True) - print(f" ✓ default: gateway.multiplex_profiles: true ({plan.default_home / 'config.yaml'})") try: + _write_multiplex_flag(plan.default_home, True) + print(f" ✓ default: gateway.multiplex_profiles: true ({plan.default_home / 'config.yaml'})") + _remove_secondary_gateways(plan) # on resume: whatever an apply killed mid-removal left installed print(f" ✓ {_restart_default(plan.default, target, plan.default_home, run_as_user=run_as_user)}") except Exception as exc: - _print([f" ✗ default: could not bring up the multiplexed gateway ({exc})", + _print([f" ✗ migration failed ({exc})", " ↩ Rolling back to per-profile gateways so no profile is left without one..."]) rolled_back = rollback_migration(plan.default_home) if not rolled_back: diff --git a/hermes_cli/gateway_migrate_guards.py b/hermes_cli/gateway_migrate_guards.py index a7a04b3d99..cadbb14ac7 100644 --- a/hermes_cli/gateway_migrate_guards.py +++ b/hermes_cli/gateway_migrate_guards.py @@ -105,6 +105,11 @@ def _guard_service_domain(plan: MigrationPlan, profile: ProfileGateway) -> Optio def _guard_unix_user(plan: MigrationPlan, profile: ProfileGateway) -> Optional[str]: default_uid = plan.default.uid + if default_uid is None and plan.default.has_system_unit: + # Consolidating INTO a principal this host cannot identify is the same unknown boundary + # from the other side: the default's system unit names an account NSS does not resolve. + return ("The default gateway runs a system unit whose User= cannot be resolved on this host: " + "an unknown service principal is not folded into automatically.") if profile.uid is None and profile.has_system_unit: # Unknown principal is not "same user": the unit names an account this host cannot resolve. return (f"Profile '{profile.name}' runs a system unit whose User= cannot be resolved on this host: " @@ -134,12 +139,13 @@ _AUTO_MIGRATION_GUARDS: tuple[Callable[[MigrationPlan, ProfileGateway], Optional def auto_migration_blockers(plan: MigrationPlan) -> list[str]: """Every boundary a standalone secondary sits behind; empty when the fleet is one user, one service domain, one profiles/ tree — the only shape ``hermes update`` may fold on its own.""" - return [ + findings = [ finding for profile in plan.standalone_secondaries for guard in _AUTO_MIGRATION_GUARDS if (finding := guard(plan, profile)) is not None ] + return list(dict.fromkeys(findings)) # a default-side finding repeats per secondary # --------------------------------------------------------------------------- opt-out diff --git a/tests/hermes_cli/test_gateway_migrate_multiplex.py b/tests/hermes_cli/test_gateway_migrate_multiplex.py index d1f1fe008f..8952fd28ba 100644 --- a/tests/hermes_cli/test_gateway_migrate_multiplex.py +++ b/tests/hermes_cli/test_gateway_migrate_multiplex.py @@ -78,6 +78,8 @@ def fleet(tmp_path, monkeypatch): monkeypatch.setattr(gm, "_service_op", _service_op) monkeypatch.setattr(gm, "_stop_gateway_process", lambda home: state.pids.pop(_name(home), None)) monkeypatch.setattr(gm, "_host_supports_migration", lambda: None) + # Part of the faked service layer: the real check asks systemd's questions (root, NSS user). + monkeypatch.setattr(gm, "_preflight_apply", lambda plan, target, run_as_user: None) state.root = root return state @@ -92,6 +94,9 @@ def _units(recorded) -> list: return list(recorded) if isinstance(recorded, list) else [recorded] +_real_preflight = gm._preflight_apply + + def _config_flag(root: Path): import yaml raw = yaml.safe_load((root / "config.yaml").read_text(encoding="utf-8")) or {} @@ -564,3 +569,111 @@ def test_opt_out_reads_effective_config_managed_false_wins_and_string_false_is_f assert auto_migration_opted_out(fleet.root) is True gm.maybe_auto_migrate_after_update() assert fleet.ops == [] and _config_flag(fleet.root) is None + + +@pytest.mark.parametrize("default_unit_preinstalled", [False, True]) +def test_interruption_after_the_default_unit_exists_is_still_interrupted_not_already_multiplexed( + fleet, monkeypatch, capsys, default_unit_preinstalled, +): + """``systemd_install`` writes the unit before the start that can be killed; an existing stopped + default unit can be interrupted mid-restart. Either way the flag is on and a default unit exists + but nothing serves anyone: that is an interrupted migration to resume, not a completed one. Only + a LIVE multiplexer that recorded ``served_profiles`` counts as already multiplexed.""" + if default_unit_preinstalled: + fleet.services["default"] = ("systemd", False) + real_op = gm._service_op + + def _killed_at_start(kind, system, verb, home, *, run_as_user=None): + if verb in ("start", "restart") and _name(home) == "default": + raise KeyboardInterrupt() + real_op(kind, system, verb, home, run_as_user=run_as_user) + + with pytest.MonkeyPatch.context() as dying: + dying.setattr(gm, "_service_op", _killed_at_start) + with pytest.raises(KeyboardInterrupt): + gm.apply_migration(gm.build_migration_plan(), served_wait=0.1) + assert _config_flag(fleet.root) is True and fleet.services == {"default": ("systemd", False)} + assert (fleet.root / gm.MANIFEST_NAME).exists() + + plan = gm.build_migration_plan() + assert plan.interrupted and not plan.already_multiplexed + fleet.ops.clear() + assert gm.apply_migration(plan, served_wait=5.0) is True + assert fleet.ops[-1] == ("default", "restart") and "serves 3 profiles" in capsys.readouterr().out + # Postcondition met: the next plan sees the live multiplexer and stops. + assert gm.build_migration_plan().already_multiplexed + + +@pytest.mark.parametrize("failing_op", [("ops", "stop"), ("coder", "uninstall"), ("default", "flag")]) +def test_failure_anywhere_in_the_destructive_phase_restores_the_removed_secondaries(fleet, monkeypatch, capsys, failing_op): + """The compensation boundary covers the whole destructive phase, not only the default bring-up: + a later secondary's stop, a unit unlink/daemon-reload, or the flag write failing after an earlier + secondary was removed must put that secondary back (the manifest alone is not a restored fleet).""" + name, verb = failing_op + real_op = gm._service_op + + def _failing(kind, system, verb_, home, *, run_as_user=None): + if (_name(home), verb_) == (name, verb): + raise RuntimeError(f"{name} {verb} failed") + real_op(kind, system, verb_, home, run_as_user=run_as_user) + + monkeypatch.setattr(gm, "_service_op", _failing) + if verb == "flag": + real_flag = gm._write_multiplex_flag + monkeypatch.setattr(gm, "_write_multiplex_flag", + lambda home, value: (_ for _ in ()).throw(OSError("read-only config")) if value else real_flag(home, value)) + assert gm.apply_migration(gm.build_migration_plan(), served_wait=0.1) is False + out = capsys.readouterr().out + assert "Rolling back" in out and "Rolled back" in out + assert _config_flag(fleet.root) is not True + assert fleet.services == {"coder": ("systemd", False), "ops": ("systemd", False)} + assert not (fleet.root / gm.MANIFEST_NAME).exists() + assert gm.build_migration_plan().eligible_for_migration() + + +def test_known_bringup_refusal_is_rejected_before_any_secondary_is_touched(fleet, monkeypatch, capsys): + """A system-unit fleet with no recorded User= run by root is the #110850 refusal: known from the plan, + so it is refused before a working gateway is stopped rather than discovered and rolled back.""" + from hermes_cli import gateway as gw + fleet.services.update({"coder": ("systemd", True), "ops": ("systemd", True)}) + monkeypatch.setattr(gm, "_systemd_service_user", lambda home, services: None) + monkeypatch.setattr(gm, "_preflight_apply", _real_preflight) + monkeypatch.setattr(gw, "_require_root_for_system_service", lambda action: None) # we are "root" + for var in ("SUDO_USER", "USER", "LOGNAME"): + monkeypatch.setenv(var, "root") + assert gm.apply_migration(gm.build_migration_plan(), served_wait=0.1) is False + out = capsys.readouterr().out + assert "before changing anything" in out and "--run-as-user root" in out + assert fleet.ops == [] and _config_flag(fleet.root) is None and not (fleet.root / gm.MANIFEST_NAME).exists() + + +def test_unknown_default_system_principal_blocks_the_update_hook(fleet, tmp_path, monkeypatch, capsys): + """Mirror of the unknown-secondary case: the default's system unit names a User= this host cannot + resolve while both secondaries are known root system units. Folding INTO an unidentifiable + principal is the same boundary; known-same uid still folds, known-different still refuses.""" + from hermes_cli import gateway as gw + from hermes_cli.gateway_migrate_guards import auto_migration_blockers, gateway_identity + unit_dir = tmp_path / "system"; unit_dir.mkdir() + monkeypatch.setattr(gw, "_SYSTEM_UNIT_DIR", unit_dir) + with gm._home_env(fleet.root): + gw.get_systemd_unit_path(system=True).write_text("[Service]\nUser=no-such-pr111062-user\n", encoding="utf-8") + for name in ("coder", "ops"): + with gm._home_env(fleet.root / "profiles" / name): + gw.get_systemd_unit_path(system=True).write_text("[Service]\nUser=root\n", encoding="utf-8") + fleet.services.update({"default": ("systemd", True), "coder": ("systemd", True), "ops": ("systemd", True)}) + fleet.pids.clear() # stopped units everywhere: identity comes from User=, resolved for real + plan = gm.build_migration_plan() + assert plan.default.uid is None and {p.uid for p in plan.standalone_secondaries} == {0} + assert gateway_identity(fleet.root, None, [("systemd", True)])[0] is None + blockers = auto_migration_blockers(plan) + assert len(blockers) == 1 and "default gateway" in blockers[0] and "cannot be resolved" in blockers[0] + gm.maybe_auto_migrate_after_update() + out = capsys.readouterr().out + assert "cannot be resolved" in out and gm.MIGRATE_COMMAND in out + assert fleet.ops == [] and _config_flag(fleet.root) is None + + # Controls: same known uid folds; a different known uid refuses. + monkeypatch.setattr(gm, "_gateway_identity", lambda home, pid, services: (0, home)) + assert auto_migration_blockers(gm.build_migration_plan()) == [] + monkeypatch.setattr(gm, "_gateway_identity", lambda home, pid, services: (0 if _name(home) == "default" else 1000, home)) + assert any("UNIX privilege boundary" in b for b in auto_migration_blockers(gm.build_migration_plan())) diff --git a/website/docs/user-guide/multi-profile-gateways.md b/website/docs/user-guide/multi-profile-gateways.md index ecb4333e3d..2398580b58 100644 --- a/website/docs/user-guide/multi-profile-gateways.md +++ b/website/docs/user-guide/multi-profile-gateways.md @@ -854,7 +854,7 @@ A standalone secondary behind any of these boundaries stops the automatic path: |---|---| | different service manager or scope | default on user systemd, a secondary on **system** systemd (or launchd), or the default detached with a service-managed secondary | | more than one installed unit on a profile | a user **and** a system unit for the same profile (the explicit command removes both) | -| different UNIX user | a system unit with its own `User=`, or a live gateway owned by another uid; a system unit whose `User=` this host cannot resolve counts as unknown, never as "same user" | +| different UNIX user | a system unit with its own `User=`, or a live gateway owned by another uid; a system unit whose `User=` this host cannot resolve — on the secondary **or** on the default — counts as unknown, never as "same user" | | `HERMES_HOME` outside `/profiles/` | a unit pinning `HERMES_HOME=/opt/hermes/profiles/emma` | In that case `hermes update` prints the boundary it found plus @@ -957,13 +957,17 @@ previous value, restarts the default gateway, and reinstalls/starts every recorded per-profile service (a system unit comes back with the `User=` it had). The manifest is removed once everything is back. -The forward migration is transactional in the same way: if bringing the default -gateway up fails after the per-profile gateways were removed (for example a -system unit that has to run as root), `--multiplex` rolls back through the -manifest on the spot so no profile is left without a gateway. Should the -process die between flipping the flag and starting the default, the next -`hermes gateway migrate --multiplex` sees the manifest with no live gateway and -resumes from it instead of reporting "already multiplexed". +The forward migration is transactional in the same way. Failures it can see +coming from the plan (a system unit that would have to run as root without a +recorded `User=`, a config file it cannot rewrite) are refused before any +per-profile gateway is stopped. Anything that fails after the manifest is +written — the flag write, a later secondary's stop or unit removal, the +default's install or start — rolls back through the manifest on the spot, so no +profile is left without a gateway. Should the process die anywhere in that +window, the next `hermes gateway migrate --multiplex` sees the flag on, the +manifest, and no live multiplexer serving the migrated profiles (an installed +but stopped default unit does not count) and resumes from the manifest instead +of reporting "already multiplexed". If no manifest exists (you enabled multiplexing by hand), leave multiplex mode with `hermes config set gateway.multiplex_profiles false && hermes gateway restart` and reinstall the per-profile services you want.