From 7995d8391ba9aa1933cfdbf2fbbc4555ef017692 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Wed, 23 Sep 2026 07:38:16 -0700 Subject: [PATCH] fix(update): the update lock adopts a stale claim naming our own pid A `hermes update` killed after taking the lock leaves `.hermes-update-in-progress` naming its pid. In a container every restart starts pid numbering over, so the retry is routinely handed that same pid; the liveness probe then found the "holder" alive (it is the retry itself) and refused with "Another Hermes update is already running (process 2)" until the 20-minute age ceiling expired. No other live process can hold our pid, so `UpdateLock.acquire` now treats a marker naming `os.getpid()` as ours and adopts it verbatim, the same rule `UpdateMarkerGuard::acquire` in the desktop updater already applies. started_at is kept so repeated retries cannot hold a wedged update under the age ceiling. Co-authored-by: ZT-Toolkit <296727067+ZT-Toolkit@users.noreply.github.com> --- hermes_cli/update_lock.py | 7 ++++ tests/hermes_cli/test_update_lock.py | 63 ++++++++++++++++++++-------- 2 files changed, 52 insertions(+), 18 deletions(-) diff --git a/hermes_cli/update_lock.py b/hermes_cli/update_lock.py index 4e8c74a28c..0e38ccd1db 100644 --- a/hermes_cli/update_lock.py +++ b/hermes_cli/update_lock.py @@ -166,6 +166,13 @@ class UpdateLock: """ existing = read_live_update(path=self.path) if existing is not None: + if existing.pid == os.getpid(): + # No other live process has our pid, so this claim is ours: a desktop pre-write, or + # the marker of a killed update whose pid this retry inherited (containers restart + # pid numbering). Adopt it verbatim like UpdateMarkerGuard::acquire in update.rs — + # rewriting started_at would let retries keep a wedged update under the age ceiling. + self.acquired = True + return True if existing.pid == _handoff_pid() or _is_ancestor_pid(existing.pid): return True self.holder = existing diff --git a/tests/hermes_cli/test_update_lock.py b/tests/hermes_cli/test_update_lock.py index a71fac318f..d2962bbf5f 100644 --- a/tests/hermes_cli/test_update_lock.py +++ b/tests/hermes_cli/test_update_lock.py @@ -17,6 +17,8 @@ disk. from __future__ import annotations import os +import subprocess +import sys import time import pytest @@ -41,6 +43,19 @@ def marker(tmp_path): return tmp_path / ".hermes-update-in-progress" +@pytest.fixture +def other_pid(): + """A live process that is not us: the stand-in for another updater (our own pid is ours).""" + proc = subprocess.Popen([sys.executable, "-c", "import time; time.sleep(120)"], stdin=subprocess.DEVNULL) + yield proc.pid + proc.kill() + proc.wait() + + +def _claim(marker, pid, started_at=None): + marker.write_text(f"{pid}\n{int(time.time() if started_at is None else started_at)}\n", encoding="utf-8") + + def test_marker_path_follows_process_hermes_home(tmp_path, monkeypatch): """The lock must land where the Rust updater and Electron gate look. @@ -63,21 +78,19 @@ def test_acquire_writes_pid_and_start_time(marker): assert len(lines) == 2, "wire format is exactly pid + started_at" -def test_second_acquire_is_refused_while_the_first_is_live(marker): +def test_second_acquire_is_refused_while_the_first_is_live(marker, other_pid): """The bug: two updaters mutating one checkout at the same time.""" - first = UpdateLock(path=marker) - assert first.acquire() is True + _claim(marker, other_pid) second = UpdateLock(path=marker) assert second.acquire() is False assert second.holder is not None - assert second.holder.pid == os.getpid() + assert second.holder.pid == other_pid assert second.acquired is False -def test_refused_lock_does_not_delete_the_live_owners_marker(marker): - first = UpdateLock(path=marker) - first.acquire() +def test_refused_lock_does_not_delete_the_live_owners_marker(marker, other_pid): + _claim(marker, other_pid) second = UpdateLock(path=marker) second.acquire() @@ -85,7 +98,21 @@ def test_refused_lock_does_not_delete_the_live_owners_marker(marker): assert marker.exists(), "a refused claimant must never clear the live owner's lock" - first.release() + +def test_marker_naming_our_own_pid_is_adopted(marker): + """A killed update's marker names the pid its retry gets (containers restart pid numbering). + + No other live process can hold our pid, so the claim is ours: adopt it verbatim (like + ``UpdateMarkerGuard::acquire``) instead of refusing "another update" for up to 20 minutes. + """ + started_at = int(time.time()) - 60 + _claim(marker, os.getpid(), started_at) + + lock = UpdateLock(path=marker) + assert lock.acquire() is True + assert lock.acquired is True + assert marker.read_text(encoding="utf-8").splitlines() == [str(os.getpid()), str(started_at)] + lock.release() assert not marker.exists() @@ -186,10 +213,10 @@ class TestHandoffFromOrchestratingUpdater: 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())) + def test_child_runs_under_the_parents_live_claim(self, marker, monkeypatch, other_pid): + # other_pid stands in for the live parent updater. + _claim(marker, other_pid) + monkeypatch.setenv(HANDOFF_PID_ENV, str(other_pid)) lock = UpdateLock(path=marker) assert lock.acquire() is True @@ -197,20 +224,20 @@ class TestHandoffFromOrchestratingUpdater: 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() + assert int(marker.read_text(encoding="utf-8").splitlines()[0]) == other_pid - def test_handoff_pid_that_is_not_the_live_holder_grants_nothing(self, marker, monkeypatch): + def test_handoff_pid_that_is_not_the_live_holder_grants_nothing(self, marker, monkeypatch, other_pid): """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)) + _claim(marker, other_pid) + monkeypatch.setenv(HANDOFF_PID_ENV, str(other_pid + 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") + def test_malformed_handoff_values_fall_back_to_refusal(self, marker, monkeypatch, value, other_pid): + _claim(marker, other_pid) monkeypatch.setenv(HANDOFF_PID_ENV, value) assert UpdateLock(path=marker).acquire() is False