Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions apps/bootstrap-installer/src-tauri/src/update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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");
Expand Down
42 changes: 41 additions & 1 deletion hermes_cli/update_lock.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,15 @@
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
Expand All @@ -47,6 +56,13 @@

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
Expand Down Expand Up @@ -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."""
Expand Down Expand Up @@ -168,9 +200,17 @@ def __init__(self, *, path: Path | None = None) -> None:
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:
Expand Down
49 changes: 49 additions & 0 deletions tests/hermes_cli/test_update_lock.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import pytest

from hermes_cli.update_lock import (
HANDOFF_PID_ENV,
UPDATE_MARKER_MAX_AGE_SECONDS,
UpdateLock,
describe_holder,
Expand Down Expand Up @@ -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()
Loading