mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-31 19:16:29 +00:00
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.
226 lines
8.3 KiB
Python
226 lines
8.3 KiB
Python
"""Cross-process update mutual exclusion (``hermes_cli.update_lock``).
|
|
|
|
Three surfaces can start an update of one install tree: a terminal ``hermes
|
|
update``, the dashboard's Update button (which spawns that same command
|
|
detached), and the desktop's Update button (Tauri updater → install-mode
|
|
bootstrap on its failure screen). Before the shared lock, two of them could run
|
|
concurrently and rewrite source under a live interpreter — observed in the wild
|
|
as an installer ``git checkout`` rewinding the checkout ~9k commits while a
|
|
dashboard-spawned ``hermes update`` was mid-``npm install``, which then failed
|
|
against the rewound tree's manifests.
|
|
|
|
These exercise the real marker file against a temp home — no mocks — because
|
|
the contract that matters is what the Rust updater and the Electron gate see on
|
|
disk.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import os
|
|
import time
|
|
|
|
import pytest
|
|
|
|
from hermes_cli.update_lock import (
|
|
HANDOFF_PID_ENV,
|
|
UPDATE_MARKER_MAX_AGE_SECONDS,
|
|
UpdateLock,
|
|
describe_holder,
|
|
read_live_update,
|
|
update_marker_path,
|
|
)
|
|
|
|
# A pid no live process owns. os.kill(pid, 0) must report it dead so a crashed
|
|
# updater can never wedge every future update. Deliberately larger than any
|
|
# platform's pid_t so it also covers the corrupt-marker path (OverflowError).
|
|
DEAD_PID = 4294967294
|
|
|
|
|
|
@pytest.fixture
|
|
def marker(tmp_path):
|
|
return tmp_path / ".hermes-update-in-progress"
|
|
|
|
|
|
def test_marker_path_follows_process_hermes_home(tmp_path, monkeypatch):
|
|
"""The lock must land where the Rust updater and Electron gate look.
|
|
|
|
All three resolve the *process* HERMES_HOME; a profile-scoped path would
|
|
put the lock somewhere the other two owners never read.
|
|
"""
|
|
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
|
assert update_marker_path() == tmp_path / ".hermes-update-in-progress"
|
|
|
|
|
|
def test_acquire_writes_pid_and_start_time(marker):
|
|
lock = UpdateLock(path=marker)
|
|
|
|
assert lock.acquire() is True
|
|
assert lock.acquired is True
|
|
|
|
lines = marker.read_text(encoding="utf-8").splitlines()
|
|
assert int(lines[0]) == os.getpid(), "the Electron gate probes this pid for liveness"
|
|
assert int(lines[1]) == pytest.approx(time.time(), abs=5)
|
|
assert len(lines) == 2, "wire format is exactly pid + started_at"
|
|
|
|
|
|
def test_second_acquire_is_refused_while_the_first_is_live(marker):
|
|
"""The bug: two updaters mutating one checkout at the same time."""
|
|
first = UpdateLock(path=marker)
|
|
assert first.acquire() is True
|
|
|
|
second = UpdateLock(path=marker)
|
|
assert second.acquire() is False
|
|
assert second.holder is not None
|
|
assert second.holder.pid == os.getpid()
|
|
assert second.acquired is False
|
|
|
|
|
|
def test_refused_lock_does_not_delete_the_live_owners_marker(marker):
|
|
first = UpdateLock(path=marker)
|
|
first.acquire()
|
|
|
|
second = UpdateLock(path=marker)
|
|
second.acquire()
|
|
second.release()
|
|
|
|
assert marker.exists(), "a refused claimant must never clear the live owner's lock"
|
|
|
|
first.release()
|
|
assert not marker.exists()
|
|
|
|
|
|
def test_release_leaves_a_marker_a_handoff_partner_now_owns(marker):
|
|
"""The desktop writes the marker, then the Tauri updater takes ownership.
|
|
|
|
Releasing must not delete a marker whose pid is no longer ours — that would
|
|
reopen the gate while the partner is still mid-update.
|
|
"""
|
|
lock = UpdateLock(path=marker)
|
|
lock.acquire()
|
|
|
|
marker.write_text(f"{DEAD_PID}\n{int(time.time())}\n", encoding="utf-8")
|
|
lock.release()
|
|
|
|
assert marker.exists(), "the partner's marker is not ours to remove"
|
|
|
|
|
|
def test_dead_owner_is_reclaimed_not_honored(marker):
|
|
marker.write_text(f"{DEAD_PID}\n{int(time.time())}\n", encoding="utf-8")
|
|
|
|
lock = UpdateLock(path=marker)
|
|
assert lock.acquire() is True
|
|
assert int(marker.read_text(encoding="utf-8").splitlines()[0]) == os.getpid()
|
|
|
|
|
|
def test_owner_past_the_age_ceiling_is_reclaimed(marker):
|
|
"""A live-but-wedged updater must not hold the lock forever."""
|
|
long_ago = int(time.time()) - UPDATE_MARKER_MAX_AGE_SECONDS - 60
|
|
marker.write_text(f"{os.getpid()}\n{long_ago}\n", encoding="utf-8")
|
|
|
|
lock = UpdateLock(path=marker)
|
|
assert lock.acquire() is True
|
|
|
|
|
|
@pytest.mark.parametrize(
|
|
"body",
|
|
["", "not-a-pid\n123\n", "\n\n", "12345"],
|
|
ids=["empty", "garbage-pid", "blank-lines", "no-start-time"],
|
|
)
|
|
def test_malformed_markers_never_block_an_update(marker, body):
|
|
marker.write_text(body, encoding="utf-8")
|
|
|
|
assert read_live_update(path=marker) is None
|
|
assert UpdateLock(path=marker).acquire() is True
|
|
|
|
|
|
def test_stale_marker_is_removed_on_read(marker):
|
|
marker.write_text(f"{DEAD_PID}\n{int(time.time())}\n", encoding="utf-8")
|
|
|
|
assert read_live_update(path=marker) is None
|
|
assert not marker.exists(), "whoever notices a stale marker clears it"
|
|
|
|
|
|
def test_absent_marker_reports_no_live_update(marker):
|
|
assert read_live_update(path=marker) is None
|
|
|
|
|
|
def test_context_manager_releases_even_on_exception(marker):
|
|
with pytest.raises(RuntimeError):
|
|
with UpdateLock(path=marker) as lock:
|
|
assert lock.acquired is True
|
|
raise RuntimeError("update blew up mid-flight")
|
|
|
|
assert not marker.exists(), "a crashed update must not strand the lock"
|
|
|
|
|
|
def test_describe_holder_names_the_pid_and_elapsed_time(marker):
|
|
lock = UpdateLock(path=marker)
|
|
lock.acquire()
|
|
|
|
holder = read_live_update(path=marker)
|
|
assert holder is not None
|
|
message = describe_holder(holder)
|
|
|
|
assert str(os.getpid()) in message, "the user needs the pid to find the other update"
|
|
assert "already running" in message
|
|
|
|
|
|
def test_unwritable_marker_location_does_not_block_the_update(tmp_path):
|
|
"""Degrade to pre-lock behavior rather than refusing to update at all.
|
|
|
|
An unwritable marker path is a worse reason to block an update than the
|
|
race the lock prevents.
|
|
"""
|
|
lock = UpdateLock(path=tmp_path / "nonexistent-file" / "marker")
|
|
(tmp_path / "nonexistent-file").write_text("i am a file, not a dir", encoding="utf-8")
|
|
|
|
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()
|