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>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user