Merge pull request #74630 from NousResearch/bb/update-restart-race

fix(update): GUI update self-deadlocks against its own lock — every retry fails with "Hermes is still running"

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");

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:

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()