From 7a61f1d247b6cbea6e5b2ba737b9752e18d81bd8 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Wed, 23 Sep 2026 09:56:59 -0700 Subject: [PATCH] fix(update): an adopted own-pid claim is refreshed, not kept near expiry Adopting a killed update's marker verbatim kept its started_at. When that claim was close to the 20-minute ceiling, the retry's own lock expired seconds into its run and a second updater could start over the same checkout: live, a detached `hermes update` acquired the lock 6s after the retry adopted a claim 3s from expiry. A marker naming our pid can only be a dead predecessor's (nothing pre-writes one for `hermes update`, which always runs under a parent's claim), so it is a new attempt and is claimed fresh like any dead holder's. Only one acquire runs per process, so this cannot renew a wedged update. --- hermes_cli/update_lock.py | 14 ++++++-------- tests/hermes_cli/test_update_lock.py | 16 ++++++++++------ 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/hermes_cli/update_lock.py b/hermes_cli/update_lock.py index 0e38ccd1db..c793a05d49 100644 --- a/hermes_cli/update_lock.py +++ b/hermes_cli/update_lock.py @@ -165,14 +165,12 @@ class UpdateLock: release. The ancestry path covers staged updaters older than the env-var export. """ 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 + # A live claim naming our own pid is a killed update's marker whose pid this retry + # inherited (containers restart pid numbering): no other live process has our pid, and + # nothing pre-writes a marker for `hermes update` (it always runs under a parent's claim). + # It is a new attempt, so it is claimed fresh like a dead holder's. Keeping the old + # started_at would let the ceiling expire mid-run and admit a second updater. + if existing is not None and existing.pid != os.getpid(): 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 d2962bbf5f..9ab4378167 100644 --- a/tests/hermes_cli/test_update_lock.py +++ b/tests/hermes_cli/test_update_lock.py @@ -99,19 +99,23 @@ def test_refused_lock_does_not_delete_the_live_owners_marker(marker, other_pid): assert marker.exists(), "a refused claimant must never clear the live owner's lock" -def test_marker_naming_our_own_pid_is_adopted(marker): +def test_marker_naming_our_own_pid_is_adopted(marker, monkeypatch): """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. + No other live process can hold our pid, so the claim is ours: take it instead of refusing + "another update" for up to 20 minutes. It is a new attempt, so it is claimed fresh: a + nearly-expired claim must still block a second updater for our whole run. """ - started_at = int(time.time()) - 60 - _claim(marker, os.getpid(), started_at) + _claim(marker, os.getpid(), time.time() - UPDATE_MARKER_MAX_AGE_SECONDS + 5) 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)] + + real_time = time.time + monkeypatch.setattr(time, "time", lambda: real_time() + 60) + holder = read_live_update(path=marker) + assert holder is not None and holder.pid == os.getpid(), "a second updater would start mid-run" lock.release() assert not marker.exists()