7a61f1d247
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.
295 lines
11 KiB
Python
295 lines
11 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 subprocess
|
|
import sys
|
|
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"
|
|
|
|
|
|
@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.
|
|
|
|
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, other_pid):
|
|
"""The bug: two updaters mutating one checkout at the same time."""
|
|
_claim(marker, other_pid)
|
|
|
|
second = UpdateLock(path=marker)
|
|
assert second.acquire() is False
|
|
assert second.holder is not None
|
|
assert second.holder.pid == other_pid
|
|
assert second.acquired is False
|
|
|
|
|
|
def test_refused_lock_does_not_delete_the_live_owners_marker(marker, other_pid):
|
|
_claim(marker, other_pid)
|
|
|
|
second = UpdateLock(path=marker)
|
|
second.acquire()
|
|
second.release()
|
|
|
|
assert marker.exists(), "a refused claimant must never clear the live owner's lock"
|
|
|
|
|
|
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: 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.
|
|
"""
|
|
_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
|
|
|
|
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()
|
|
|
|
|
|
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"
|
|
|
|
|
|
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, 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
|
|
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]) == other_pid
|
|
|
|
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."""
|
|
_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, other_pid):
|
|
_claim(marker, other_pid)
|
|
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()
|
|
|
|
|
|
class TestAncestryHandoff:
|
|
"""Staged updaters older than the HANDOFF_PID_ENV export never send it.
|
|
|
|
``hermes-setup`` under ``~/.hermes`` is only refreshed by a full installer
|
|
run, so an updated checkout (new lock) driven by a pre-handoff staged
|
|
updater (old parent) deadlocks on exit 2 forever unless the child also
|
|
recognizes a live holder that is its own process ancestor.
|
|
|
|
``_pid_alive`` is pinned True here because the hermetic conftest guards
|
|
``os.kill`` probes of pids outside the test subtree (our ppid included);
|
|
liveness has its own coverage above — ancestry is what's under test.
|
|
"""
|
|
|
|
@pytest.fixture(autouse=True)
|
|
def _liveness_pinned_true(self, monkeypatch):
|
|
monkeypatch.setattr("hermes_cli.update_lock._pid_alive", lambda pid: True)
|
|
|
|
def test_marker_owned_by_our_parent_process_is_our_orchestrator(self, marker):
|
|
marker.write_text(f"{os.getppid()}\n{int(time.time())}\n", encoding="utf-8")
|
|
|
|
lock = UpdateLock(path=marker)
|
|
assert lock.acquire() is True, "a live ancestor's claim is the one we run under"
|
|
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.getppid()
|
|
|
|
def test_live_non_ancestor_holder_is_still_refused(self, marker):
|
|
"""Ancestry must not open the lock to unrelated concurrent updaters."""
|
|
marker.write_text(f"{DEAD_PID}\n{int(time.time())}\n", encoding="utf-8")
|
|
|
|
lock = UpdateLock(path=marker)
|
|
assert lock.acquire() is False
|
|
assert lock.holder is not None
|
|
assert lock.holder.pid == DEAD_PID
|