From fe8e4d93daad6a3ae24b69401946b0e3bce3df31 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Wed, 29 Jul 2026 17:45:48 -0500 Subject: [PATCH] fix(update): make the in-progress marker a real cross-process lock Three surfaces start updates against one checkout: a terminal "hermes update", the dashboard's Update button (which spawns that same command detached), and the desktop's, which hands off to the Tauri updater. Only the Tauri updater published the in-progress marker, and only Electron read it -- to gate backend startup, not to stop a second updater. So a dashboard-spawned update and an installer-driven git checkout could mutate the same tree concurrently, rewriting source under a live interpreter. Claim the same marker from cmd_update rather than adding a second mechanism: same path, same pid+started_at payload the Rust and Electron readers already parse. A marker only counts as live when its pid is alive and it is inside the shared age ceiling, so a crashed updater self-heals instead of wedging every future update. Release only removes a marker we still own, leaving a handoff partner's claim intact. Refusing exits 2, matching the existing concurrent-instance contract the Tauri updater already recognizes. --- hermes_cli/main.py | 19 +++ hermes_cli/update_lock.py | 216 +++++++++++++++++++++++++++ tests/hermes_cli/test_update_lock.py | 177 ++++++++++++++++++++++ 3 files changed, 412 insertions(+) create mode 100644 hermes_cli/update_lock.py create mode 100644 tests/hermes_cli/test_update_lock.py diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 4db17c3e872..759b0c96fd7 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -8787,9 +8787,28 @@ def cmd_update(args): # writes to a closed stdout. No-op in gateway mode. See # _install_hangup_protection for rationale. _update_io_state = _install_hangup_protection(gateway_mode=gateway_mode) + # Cross-process mutual exclusion. The dashboard's Update button spawns + # this same command detached, and the desktop hands off to the Tauri + # updater / install-mode bootstrap — all three mutate one checkout. Two of + # them running together rewrite source under a live interpreter and strand + # the tree half-updated. Share the marker the Tauri updater and Electron + # already use rather than inventing a second lock. + from hermes_cli.update_lock import ( + UPDATE_EXIT_CONCURRENT, + UpdateLock, + describe_holder, + ) + + _update_lock = UpdateLock() + if not _update_lock.acquire(): + print(describe_holder(_update_lock.holder)) + _finalize_update_output(_update_io_state) + sys.exit(UPDATE_EXIT_CONCURRENT) + try: _cmd_update_impl(args, gateway_mode=gateway_mode) finally: + _update_lock.release() _finalize_update_output(_update_io_state) diff --git a/hermes_cli/update_lock.py b/hermes_cli/update_lock.py new file mode 100644 index 00000000000..32728bab65d --- /dev/null +++ b/hermes_cli/update_lock.py @@ -0,0 +1,216 @@ +"""Cross-process mutual exclusion for in-flight Hermes updates. + +Three different surfaces can start an update of the same install tree: + +* ``hermes update`` from a terminal, +* the dashboard's Update button (``POST /api/hermes/update`` → + ``_spawn_hermes_action(["update"])``, detached), +* the desktop's Update button, which hands off to the Tauri + ``hermes-setup --update`` and, on its failure screen, to install-mode + bootstrap (``install.ps1`` / ``install.sh``). + +Until now only the Tauri updater published an "update in progress" marker +(``UpdateMarkerGuard`` in ``apps/bootstrap-installer/src-tauri/src/update.rs``), +and only the Electron desktop consumed it (``electron/update-marker.ts``, to +gate local backend startup). Nothing stopped two *updaters* from running at +once — so a dashboard-spawned ``hermes update`` and an installer-driven +``git checkout`` could mutate the same checkout concurrently, rewriting source +under a live interpreter and leaving the tree half-updated. + +This module makes that same marker the single lock for **all** update +entrypoints instead of adding a fourth mechanism. Format and location are +unchanged and remain byte-compatible with the Rust and Electron readers: + + /.hermes-update-in-progress body: "\\n" + +A marker only counts as a live update when its pid is alive AND it is younger +than :data:`UPDATE_MARKER_MAX_AGE_MS` — mirroring ``readLiveUpdateMarker`` so a +crashed updater self-heals instead of wedging every future update. A stale +marker is removed on read by whoever notices it first. +""" + +from __future__ import annotations + +import logging +import os +import time +from dataclasses import dataclass +from pathlib import Path + +logger = logging.getLogger(__name__) + +# Keep in sync with UPDATE_MARKER_MAX_AGE_MS in +# apps/desktop/electron/update-marker.ts — the same marker is read by both, and +# a shorter ceiling here would let Python steal a lock Electron still considers +# live. A full update (git pull + uv sync + desktop rebuild) is minutes. +UPDATE_MARKER_MAX_AGE_SECONDS = 20 * 60 + +MARKER_NAME = ".hermes-update-in-progress" + +# Exit code meaning "another updater/instance owns this install right now". +# Already the de-facto contract: the Windows shim + venv-holder guards in +# _cmd_update_impl exit 2, and the Tauri updater matches on it +# (UPDATE_EXIT_CONCURRENT in apps/bootstrap-installer/src-tauri/src/update.rs) +# to show "Hermes is still running" instead of a generic failure. Naming it +# here keeps the concurrent-update refusal on that same understood contract. +UPDATE_EXIT_CONCURRENT = 2 + + +def update_marker_path() -> Path: + """Path of the shared update marker. + + Uses the *process* Hermes home (never the context-local profile override): + the Rust updater resolves ``$HERMES_HOME`` or the platform default, and the + desktop pins that same value into the updater's env. A profile-scoped path + here would put the lock somewhere the other two owners never look. + """ + from hermes_constants import get_process_hermes_home + + return get_process_hermes_home() / MARKER_NAME + + +def _pid_alive(pid: int) -> bool: + """True when a process with ``pid`` currently exists. + + ``os.kill(pid, 0)`` is POSIX-only in spirit but CPython implements the + signal-0 existence probe on Windows too. ``PermissionError`` means the pid + exists but is owned by another user — still alive for our purposes. + + A value too large for the platform's ``pid_t`` raises ``OverflowError`` + (not ``OSError``), so a corrupt marker would otherwise crash every update + that reads it. Treat any unusable pid as dead: the marker is garbage and + must not wedge the lock. + """ + if pid <= 0: + return False + try: + os.kill(pid, 0) + except ProcessLookupError: + return False + except PermissionError: + return True + except (OverflowError, ValueError): + return False + except OSError: + # Unexpected errno: assume alive rather than stomping a live updater. + return True + return True + + +@dataclass(frozen=True) +class UpdateHolder: + """A confirmed-live update currently holding the lock.""" + + pid: int + age_seconds: float + + +def read_live_update(*, path: Path | None = None) -> UpdateHolder | None: + """Return the live update holding the lock, or ``None``. + + Mirrors ``readLiveUpdateMarker`` in ``electron/update-marker.ts``: absent, + unreadable, malformed, dead-pid, and past-the-ceiling all mean "no live + update", and a stale marker file is deleted so it can't strand future runs. + Never raises. + """ + marker = path or update_marker_path() + try: + raw = marker.read_text(encoding="utf-8") + except OSError: + return None # absent or unreadable => no live update + + lines = raw.splitlines() + try: + pid = int(lines[0].strip()) + except (IndexError, ValueError): + pid = -1 + try: + started_at = float(lines[1].strip()) + except (IndexError, ValueError): + started_at = float("-inf") + + age = time.time() - started_at + if not _pid_alive(pid) or age > UPDATE_MARKER_MAX_AGE_SECONDS: + try: + marker.unlink() + except OSError: + pass + return None + + return UpdateHolder(pid=pid, age_seconds=age) + + +def describe_holder(holder: UpdateHolder) -> str: + """One-line, user-facing explanation of who holds the update lock.""" + minutes, seconds = divmod(int(max(holder.age_seconds, 0)), 60) + elapsed = f"{minutes}m {seconds}s" if minutes else f"{seconds}s" + return ( + f"✗ Another Hermes update is already running (PID {holder.pid}, " + f"started {elapsed} ago).\n" + "\n" + " Two updates mutating the same checkout corrupt it: one rewrites\n" + " source while the other is mid-install. Wait for it to finish, or\n" + " close the window/dashboard tab that started it, then retry." + ) + + +class UpdateLock: + """Context manager owning the shared update marker for this process. + + ``acquired`` is False when another live update already holds it — callers + decide whether that's a hard refusal (CLI/dashboard) or a wait. Releasing + only removes the marker when *we* still own it, so a marker rewritten by a + handoff partner (the Tauri updater overwrites it with its own pid) is never + deleted out from under its new owner. + """ + + def __init__(self, *, path: Path | None = None) -> None: + self.path = path or update_marker_path() + self.acquired = False + self.holder: UpdateHolder | None = None + + def acquire(self) -> bool: + """Claim the lock. Returns False (and sets ``holder``) if it's taken.""" + existing = read_live_update(path=self.path) + if existing is not None: + self.holder = existing + return False + try: + self.path.parent.mkdir(parents=True, exist_ok=True) + self.path.write_text( + f"{os.getpid()}\n{int(time.time())}\n", encoding="utf-8" + ) + except OSError as exc: + # Best-effort, exactly like the Rust guard: an unwritable marker + # must not block the update itself (that would be a worse failure + # than the race it prevents). Degrade to the pre-lock behavior. + logger.debug("Could not write update marker %s: %s", self.path, exc) + return True + self.acquired = True + return True + + def release(self) -> None: + """Drop the marker if this process still owns it. Never raises.""" + if not self.acquired: + return + self.acquired = False + try: + raw = self.path.read_text(encoding="utf-8") + owner = int(raw.splitlines()[0].strip()) + except (OSError, IndexError, ValueError): + return + if owner != os.getpid(): + # A handoff partner took ownership (e.g. the Tauri updater wrote + # its own pid). Leave it alone — it's still a live update. + return + try: + self.path.unlink() + except OSError: + pass + + def __enter__(self) -> "UpdateLock": + self.acquire() + return self + + def __exit__(self, *_exc) -> None: + self.release() diff --git a/tests/hermes_cli/test_update_lock.py b/tests/hermes_cli/test_update_lock.py new file mode 100644 index 00000000000..93dfef71016 --- /dev/null +++ b/tests/hermes_cli/test_update_lock.py @@ -0,0 +1,177 @@ +"""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 ( + 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"