mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-31 19:16:29 +00:00
fix(update): let the GUI updater's hermes update child pass the lock it already holds
The cross-process update lock (fe8e4d93d) made the in-progress marker
mutually exclusive across every update entrypoint — but the Tauri
updater holds that marker for its WHOLE run and then spawns
hermes update as a child stage. The child read the marker, found its
own parent's live pid, refused with exit 2, and the GUI mapped that to
"Hermes is still running. Close all Hermes windows and try the update
again." Retry spawns a fresh updater that deadlocks against itself the
same way, so every GUI-driven update dead-ends on the failure screen
with no winnable retry (observed: three consecutive self-refusals in
bootstrap-installer.log within 90 seconds).
Hand the claim off explicitly: update_child_env exports
HERMES_UPDATE_HANDOFF_PID naming the updater's own pid, and
UpdateLock.acquire treats a live holder matching that pid as the lock
we are already running under — run without claiming, and release
leaves the parent's marker untouched. 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, and a dashboard-spawned
hermes update (no handoff env) is still refused exactly as before.
This commit is contained in:
parent
98e43be8e0
commit
8c76fe19f8
3 changed files with 112 additions and 1 deletions
|
|
@ -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");
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue