From 36005604334a173f3eae1afa8c7e087bb0a578cb Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 2 Sep 2026 20:35:48 -0700 Subject: [PATCH] refactor(hermes_cli/update): compact update_lock docstrings and marker parsing --- hermes_cli/update_lock.py | 129 ++++++++++++++------------------------ 1 file changed, 47 insertions(+), 82 deletions(-) diff --git a/hermes_cli/update_lock.py b/hermes_cli/update_lock.py index fbc43063a0..36afab7a12 100644 --- a/hermes_cli/update_lock.py +++ b/hermes_cli/update_lock.py @@ -1,12 +1,9 @@ """Cross-process mutual exclusion for in-flight Hermes updates. -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). - -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: +The marker file the Tauri updater writes (``UpdateMarkerGuard`` in +``apps/bootstrap-installer/src-tauri/src/update.rs``) and the Electron desktop reads +(``electron/update-marker.ts``) is the single lock for **all** update entrypoints. +Format and location are byte-compatible with both readers. """ from __future__ import annotations @@ -14,42 +11,37 @@ from __future__ import annotations import logging import os import time +from contextlib import suppress 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. +# Keep in sync with UPDATE_MARKER_MAX_AGE_MS in apps/desktop/electron/update-marker.ts: +# 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" -# Set by an orchestrating updater (the Tauri `hermes-setup --update` flow) to -# its own pid before spawning `hermes update` as a child stage. The parent -# holds the marker for its whole run, so without this the child refuses its -# own parent's lock and the GUI update can never complete. See update_child_env -# in apps/bootstrap-installer/src-tauri/src/update.rs — keep the name in sync. +# Set by an orchestrating updater (Tauri `hermes-setup --update`) to its own pid before +# spawning `hermes update` as a child stage; the parent holds the marker for its whole run, +# so without this the child would refuse its own parent's lock. Keep in sync with +# update_child_env in apps/bootstrap-installer/src-tauri/src/update.rs. HANDOFF_PID_ENV = "HERMES_UPDATE_HANDOFF_PID" -# 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. +# Exit code meaning "another updater/instance owns this install right now" — the same +# contract as the Windows shim / venv-holder guards in _cmd_update_impl, matched by the +# Tauri updater (UPDATE_EXIT_CONCURRENT in update.rs) to show "Hermes is still running". 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. + 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, so a profile-scoped path would be one the other owners never look at. """ from hermes_constants import get_process_hermes_home @@ -59,12 +51,10 @@ def update_marker_path() -> Path: def _pid_alive(pid: int) -> bool: """True when a process with ``pid`` currently exists. - Delegates to :func:`gateway.status._pid_exists`, the project's existing no-kill probe. Do NOT - hand-roll this with ``os.kill(pid, 0)``: on Windows that is not a no-op — CPython routes - ``sig=0`` to ``GenerateConsoleCtrlEvent``, which Ctrl+C's the target's whole console process - group (bpo-14484). - - Any pid we cannot evaluate counts as dead: a corrupt marker must not wedge the lock forever. + Delegates to :func:`gateway.status._pid_exists`. Do NOT hand-roll ``os.kill(pid, 0)``: on + Windows CPython routes ``sig=0`` to ``GenerateConsoleCtrlEvent``, which Ctrl+C's the + target's whole console process group (bpo-14484). Any pid we cannot evaluate counts as + dead so a corrupt marker never wedges the lock. """ if pid <= 0: return False @@ -73,37 +63,26 @@ def _pid_alive(pid: int) -> bool: return bool(_pid_exists(pid)) except Exception as exc: - # Import failure or an unusable pid (e.g. larger than the platform's - # pid_t). Treat the marker as stale rather than blocking updates. logger.debug("Could not probe pid %s: %s", pid, exc) return False def _handoff_pid() -> int | None: - """Pid of the orchestrating updater that spawned us, if any. - - Read from :data:`HANDOFF_PID_ENV`. Malformed values count as absent — a broken handoff must fall - back to the normal refusal, never crash. - """ - raw = os.environ.get(HANDOFF_PID_ENV, "").strip() - if not raw: - return None + """Pid of the orchestrating updater that spawned us (:data:`HANDOFF_PID_ENV`); malformed + values count as absent so a broken handoff falls back to the normal refusal.""" try: - pid = int(raw) + pid = int(os.environ.get(HANDOFF_PID_ENV, "").strip()) except ValueError: return None return pid if pid > 0 else None def _is_ancestor_pid(pid: int) -> bool: - """True when ``pid`` is a live ancestor (parent chain) of this process. + """True when ``pid`` is a live ancestor of this process. - The orchestrating updater spawns ``hermes update`` as a (grand)child, so a live marker owned by - one of our ancestors can only be the claim we are already running under — an unrelated - concurrent updater is never in our parent chain. - - Never includes our own pid, and any failure counts as "not an ancestor": an unprovable ancestry - must fall back to the normal refusal. + The orchestrating updater spawns ``hermes update`` as a (grand)child, so a live marker + owned by an ancestor can only be the claim we already run under — an unrelated concurrent + updater is never in our parent chain. Never our own pid; any failure is "not an ancestor". """ if pid <= 0: return False @@ -128,16 +107,14 @@ 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. + 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") + lines = marker.read_text(encoding="utf-8").splitlines() except OSError: - return None # absent or unreadable => no live update - - lines = raw.splitlines() + return None try: pid = int(lines[0].strip()) except (IndexError, ValueError): @@ -149,12 +126,9 @@ def read_live_update(*, path: Path | None = None) -> UpdateHolder | None: age = time.time() - started_at if not _pid_alive(pid) or age > UPDATE_MARKER_MAX_AGE_SECONDS: - try: + with suppress(OSError): marker.unlink() - except OSError: - pass return None - return UpdateHolder(pid=pid, age_seconds=age) @@ -175,10 +149,10 @@ def describe_holder(holder: UpdateHolder) -> str: class UpdateLock: """Context manager owning the shared update marker for this process. - ``acquired`` is False when another live update holds it; callers decide between hard refusal - (CLI/dashboard) and waiting. Release only removes the marker when *we* still own it, so a marker - rewritten by a handoff partner (the Tauri updater writes its own pid) is never deleted from - under its new owner. + ``acquired`` is False when another live update holds it; callers decide between hard + refusal (CLI/dashboard) and waiting. Release only removes the marker when *we* still own + it, so a marker rewritten by a handoff partner (the Tauri updater writes its own pid) is + never deleted from under its new owner. """ def __init__(self, *, path: Path | None = None) -> None: @@ -189,10 +163,9 @@ class UpdateLock: def acquire(self) -> bool: """Claim the lock. Returns False (and sets ``holder``) if it's taken. - A live holder whose pid matches :data:`HANDOFF_PID_ENV` — or is an ancestor of ours — is our - own orchestrating parent (e.g. the Tauri updater staging ``hermes update``): run under ITS - claim and leave its marker untouched on release. The ancestry path covers staged updaters - older than the env-var export. + A live holder whose pid matches :data:`HANDOFF_PID_ENV` — or is an ancestor of ours — + is our own orchestrating parent: run under ITS claim and leave its marker untouched on + 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: @@ -202,13 +175,10 @@ class UpdateLock: 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" - ) + 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. + # Best-effort, like the Rust guard: an unwritable marker must not block the + # update itself (worse than the race it prevents). Degrade to pre-lock behavior. logger.debug("Could not write update marker %s: %s", self.path, exc) return True self.acquired = True @@ -220,18 +190,13 @@ class UpdateLock: return self.acquired = False try: - raw = self.path.read_text(encoding="utf-8") - owner = int(raw.splitlines()[0].strip()) + owner = int(self.path.read_text(encoding="utf-8").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: + return # a handoff partner took ownership — still a live update + with suppress(OSError): self.path.unlink() - except OSError: - pass def __enter__(self) -> "UpdateLock": self.acquire()