diff --git a/apps/bootstrap-installer/src-tauri/src/update.rs b/apps/bootstrap-installer/src-tauri/src/update.rs index 63a5bfe8d71..3fada182d02 100644 --- a/apps/bootstrap-installer/src-tauri/src/update.rs +++ b/apps/bootstrap-installer/src-tauri/src/update.rs @@ -895,6 +895,17 @@ fn update_child_env(install_root: &Path) -> Vec<(String, OsString)> { // a frozen stage, and users cancel a healthy update. Force line-by-line // output instead. envs.push(("PYTHONUNBUFFERED".to_string(), OsString::from("1"))); + // We hold the update-in-progress marker for this whole run, and the + // `hermes update` child claims that SAME lock (hermes_cli/update_lock.py). + // Name our pid so the child recognizes the live holder as its own + // orchestrator and runs under our claim — without this every GUI update + // refuses its parent's marker with exit 2 ("Hermes is still running") + // and no number of retries can ever succeed. Keep the variable name in + // sync with HANDOFF_PID_ENV in hermes_cli/update_lock.py. + envs.push(( + "HERMES_UPDATE_HANDOFF_PID".to_string(), + OsString::from(std::process::id().to_string()), + )); if let Some(path) = path_with_prepended_entries(&[ hermes_home.join("node").join("bin"), venv_bin_dir(install_root), @@ -1218,6 +1229,17 @@ mod tests { ); } + #[test] + fn update_child_env_names_our_pid_for_the_lock_handoff() { + let envs = update_child_env(Path::new("/x/hermes-agent")); + assert!( + envs.iter().any(|(k, v)| k == "HERMES_UPDATE_HANDOFF_PID" + && v.to_str() == Some(std::process::id().to_string().as_str())), + "the hermes update child claims the same marker we hold; without our pid \ + it refuses its own parent's lock and every GUI update dead-ends on exit 2" + ); + } + #[test] fn lock_probe_paths_include_desktop_app_payload() { let root = Path::new("/x/hermes-agent"); diff --git a/hermes_cli/update_lock.py b/hermes_cli/update_lock.py index cfd6a4ea697..2d8b2aaf8ac 100644 --- a/hermes_cli/update_lock.py +++ b/hermes_cli/update_lock.py @@ -27,6 +27,15 @@ A marker only counts as a live update when its pid is alive AND it is younger than :data:`UPDATE_MARKER_MAX_AGE_MS` — mirroring ``readLiveUpdateMarker`` so a crashed updater self-heals instead of wedging every future update. A stale marker is removed on read by whoever notices it first. + +One layering wrinkle: the Tauri updater holds this marker for its WHOLE run and +then spawns ``hermes update`` as a child stage. Without a handoff the child +sees its own parent's live marker and refuses — the GUI update deadlocks +against itself on every attempt ("Hermes is still running", retry forever). +The updater therefore exports :data:`HANDOFF_PID_ENV` naming its own pid, and +``acquire`` treats a live holder matching that pid as the lock we are already +running under. The env var alone grants nothing: the pid must also be the +live marker owner, so a stale or forged value cannot bypass the lock. """ from __future__ import annotations @@ -47,6 +56,13 @@ UPDATE_MARKER_MAX_AGE_SECONDS = 20 * 60 MARKER_NAME = ".hermes-update-in-progress" +# Set by an orchestrating updater (the Tauri `hermes-setup --update` flow) to +# its own pid before spawning `hermes update` as a child stage. The parent +# holds the marker for its whole run, so without this the child refuses its +# own parent's lock and the GUI update can never complete. See update_child_env +# in apps/bootstrap-installer/src-tauri/src/update.rs — keep the name in sync. +HANDOFF_PID_ENV = "HERMES_UPDATE_HANDOFF_PID" + # Exit code meaning "another updater/instance owns this install right now". # Already the de-facto contract: the Windows shim + venv-holder guards in # _cmd_update_impl exit 2, and the Tauri updater matches on it @@ -95,6 +111,22 @@ def _pid_alive(pid: int) -> bool: return False +def _handoff_pid() -> int | None: + """Pid of the orchestrating updater that spawned us, if any. + + Read from :data:`HANDOFF_PID_ENV`. Malformed values count as absent — + a broken handoff must fall back to the normal refusal, never crash. + """ + raw = os.environ.get(HANDOFF_PID_ENV, "").strip() + if not raw: + return None + try: + pid = int(raw) + except ValueError: + return None + return pid if pid > 0 else None + + @dataclass(frozen=True) class UpdateHolder: """A confirmed-live update currently holding the lock.""" @@ -168,9 +200,17 @@ class UpdateLock: self.holder: UpdateHolder | None = None def acquire(self) -> bool: - """Claim the lock. Returns False (and sets ``holder``) if it's taken.""" + """Claim the lock. Returns False (and sets ``holder``) if it's taken. + + A live holder whose pid matches :data:`HANDOFF_PID_ENV` is our own + orchestrating parent (the Tauri updater spawning `hermes update` as a + stage): we run under ITS claim rather than refusing or re-writing the + marker, and ``release`` leaves the parent's marker untouched. + """ existing = read_live_update(path=self.path) if existing is not None: + if existing.pid == _handoff_pid(): + return True self.holder = existing return False try: diff --git a/tests/hermes_cli/test_update_lock.py b/tests/hermes_cli/test_update_lock.py index 93dfef71016..290d94c7c04 100644 --- a/tests/hermes_cli/test_update_lock.py +++ b/tests/hermes_cli/test_update_lock.py @@ -22,6 +22,7 @@ import time import pytest from hermes_cli.update_lock import ( + HANDOFF_PID_ENV, UPDATE_MARKER_MAX_AGE_SECONDS, UpdateLock, describe_holder, @@ -175,3 +176,51 @@ def test_unwritable_marker_location_does_not_block_the_update(tmp_path): assert lock.acquire() is True assert lock.acquired is False, "nothing was written, so there is nothing to release" + + +class TestHandoffFromOrchestratingUpdater: + """The Tauri updater holds the marker, then spawns ``hermes update``. + + The regression: the child saw its own parent's live marker and exited 2, + so every GUI update failed with "Hermes is still running" and retrying + just re-ran the same self-deadlock. The parent names its pid in + HANDOFF_PID_ENV; a live holder matching it is our own orchestrator. + """ + + def test_child_runs_under_the_parents_live_claim(self, marker, monkeypatch): + # Stand in for the parent updater with our own (live) pid. + marker.write_text(f"{os.getpid()}\n{int(time.time())}\n", encoding="utf-8") + monkeypatch.setenv(HANDOFF_PID_ENV, str(os.getpid())) + + lock = UpdateLock(path=marker) + assert lock.acquire() is True + assert lock.acquired is False, "the parent's claim is not ours to own" + + lock.release() + assert marker.exists(), "the parent still needs its marker after our stage ends" + assert int(marker.read_text(encoding="utf-8").splitlines()[0]) == os.getpid() + + def test_handoff_pid_that_is_not_the_live_holder_grants_nothing(self, marker, monkeypatch): + """The env var alone must not bypass the lock.""" + marker.write_text(f"{os.getpid()}\n{int(time.time())}\n", encoding="utf-8") + monkeypatch.setenv(HANDOFF_PID_ENV, str(os.getpid() + 1)) + + lock = UpdateLock(path=marker) + assert lock.acquire() is False + assert lock.holder is not None + + @pytest.mark.parametrize("value", ["", "not-a-pid", "-1", "0"], ids=["empty", "garbage", "negative", "zero"]) + def test_malformed_handoff_values_fall_back_to_refusal(self, marker, monkeypatch, value): + marker.write_text(f"{os.getpid()}\n{int(time.time())}\n", encoding="utf-8") + monkeypatch.setenv(HANDOFF_PID_ENV, value) + + assert UpdateLock(path=marker).acquire() is False + + def test_handoff_env_with_no_marker_claims_normally(self, marker, monkeypatch): + """A handoff pid must not stop us writing our own claim when unlocked.""" + monkeypatch.setenv(HANDOFF_PID_ENV, str(os.getpid())) + + lock = UpdateLock(path=marker) + assert lock.acquire() is True + assert lock.acquired is True + assert int(marker.read_text(encoding="utf-8").splitlines()[0]) == os.getpid()