From ea355fda17bebf24d226579f4d46fd02fca3e1a0 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Mon, 5 Oct 2026 16:29:31 -0700 Subject: [PATCH] fix: one install's completed restart never erases another install's host restart debt Two Hermes installations of one OS user (a second checkout, a ZIP install) shared the single host-update-restart.json in the host state dir. Install B's arm for its SHA replaced install A's record, and B's discharge then unlinked it: A's owed restart was gone. The record is now keyed by the checkout the module runs from (host-update-restart-.json): every profile of one install still shares one record, each install owes and clears its own. The key comes from __file__ and hashlib only, so the N-1 updater that lazily imports this module after its pull still arms it, and write_host_obligation still never raises. A debt an older release armed in the unkeyed file is still read and cleared on discharge; all records keep one mutex sidecar (review S3). --- hermes_cli/update_host_obligation.py | 69 +++++++++++++------ tests/hermes_cli/test_old_updater_takeover.py | 4 +- .../hermes_cli/test_update_host_obligation.py | 36 ++++++++++ website/docs/getting-started/updating.md | 2 +- 4 files changed, 86 insertions(+), 25 deletions(-) diff --git a/hermes_cli/update_host_obligation.py b/hermes_cli/update_host_obligation.py index 781ad17807..b992f0581d 100644 --- a/hermes_cli/update_host_obligation.py +++ b/hermes_cli/update_host_obligation.py @@ -10,7 +10,8 @@ SHARED process, and every other profile's CLI could neither see nor discharge th The record therefore lives beside the host rendezvous record, in :func:`gateway.host_rendezvous.host_state_dir` (``$HERMES_GATEWAY_LOCK_DIR`` else ``$XDG_STATE_HOME/hermes/gateway-locks``) — the one cross-profile, per-OS-user state root the -tree already has. It is written once per host, read by every profile's CLI, and cleared once. +tree already has. It is written once per host and installation (keyed by the checkout), read by +every profile's CLI, and cleared once. The same "one host process, not one per profile" identity is what :func:`collapse_units_to_host_processes` applies to enumerated systemd units: leftover @@ -22,6 +23,7 @@ from __future__ import annotations import base64 import contextlib +import hashlib import json import logging import os @@ -32,20 +34,36 @@ from typing import Any, Callable, Iterable, Optional logger = logging.getLogger("hermes_cli.update_cmd") -#: One file per OS user, beside ``host-gateway.json`` / ``host-serve.json``. +#: The record before it carried an install key, beside ``host-gateway.json`` / ``host-serve.json``. +#: Still read, and cleared on discharge, so a debt an older release armed survives the upgrade; it +#: names no install, so it keeps its old meaning: owed by whichever install reads it. HOST_OBLIGATION_NAME = "host-update-restart.json" _RECORD_VERSION = 1 def host_obligation_path() -> Path: - """Path of the host obligation record.""" + """This installation's host obligation record. + + Keyed by the checkout this module runs from: every profile of one install arms, reads and + clears the same record, while a second install of the same OS user (another checkout, a ZIP + install) owes its own, so its completed restart never erases this install's debt (review S3). + """ # Not ``gateway.host_rendezvous.host_state_dir``: ``gateway.status`` imports ``utils`` -> ruamel, # absent from the historical interpreter an old updater's takeover arms this record in; the # recovery module's stdlib copy of the rule is drift-tested against the gateway resolver. from hermes_cli.update_restart_recovery import _host_state_dir - return Path(_host_state_dir()) / HOST_OBLIGATION_NAME + install = os.path.normcase(str(Path(__file__).resolve().parents[1])) + key = hashlib.sha256(install.encode("utf-8")).hexdigest()[:16] + return Path(_host_state_dir()) / f"{Path(HOST_OBLIGATION_NAME).stem}-{key}.json" + + +def _owed_path() -> Path: + """The record a reader honours: this install's, else an older release's unkeyed one.""" + path = host_obligation_path() + legacy = path.with_name(HOST_OBLIGATION_NAME) + return legacy if not os.path.exists(path) and os.path.exists(legacy) else path # never raises def _write_record(path: Path, record: dict) -> None: @@ -66,8 +84,12 @@ def _write_record(path: Path, record: dict) -> None: def read_host_obligation() -> Optional[dict]: """The published obligation record, or ``None`` when absent/corrupt/foreign-versioned.""" + return _read_record(_owed_path()) + + +def _read_record(path: Path) -> Optional[dict]: try: - payload = json.loads(host_obligation_path().read_text(encoding="utf-8-sig")) + payload = json.loads(path.read_text(encoding="utf-8-sig")) except (OSError, UnicodeDecodeError, ValueError): return None if not isinstance(payload, dict) or payload.get("version") != _RECORD_VERSION: @@ -82,7 +104,7 @@ def host_obligation_present() -> bool: one — the restart is still owed and the reader falls back to "no recorded inventory". """ try: - return host_obligation_path().is_file() + return _owed_path().is_file() except OSError: return False @@ -101,7 +123,7 @@ def write_host_obligation( ``restarted`` proof) instead of resetting it: the host owes one restart, not one per profile. ``owner`` (an update run's commit-point token) joins the record's ``owners``: a run that fails before its move hands back only its own stake (``release_host_obligation``), never - another install's debt for the same SHA. The first owner also stores what it found there + another run's debt for the same SHA. The first owner also stores what it found there (``found``), which the last owner to leave puts back; an owner retargeting to another SHA keeps that baseline (``_baseline_for``). """ @@ -115,7 +137,7 @@ def write_host_obligation( def _write_locked(path: Path, *, expected_sha: str, runtimes: Optional[list], profile: str, owner: str) -> bool: - existing = read_host_obligation() + existing = _read_record(path) try: found = _baseline_for(owner, existing, path) if owner else None except OSError as exc: # unreadable is not absent: a guessed ``found`` would delete it later @@ -170,7 +192,8 @@ def _record_mutex(path: Path): The sidecar sits in a subdirectory: the state dir is the gateway lock dir, whose ``--replace`` cleanup unlinks every top-level ``*.lock``, and a deleted sidecar splits the lock in two. """ - guard = path.parent / f".{path.stem}.mutex" + # One guard for every install's record (and the unkeyed one an older release still locks). + guard = path.parent / f".{Path(HOST_OBLIGATION_NAME).stem}.mutex" guard.mkdir(parents=True, exist_ok=True) with _marker_mutex()(guard / "record"): yield @@ -234,23 +257,23 @@ def release_host_obligation(owner: str) -> None: """Hand back ``owner``'s stake in the record. When no other run still owes through it, the record goes back to what its first owner found (``found``; absent = unlinked). - A record without ``owner`` was rewritten since (another install's newer pull): it is theirs and + A record without ``owner`` was rewritten since (another run's newer pull): it is theirs and stays. Raises OSError when the record cannot be rewritten (the caller keeps the debt armed). Judged and rewritten under ``_record_mutex``, which every arm takes too, so no arm can land between the look and the write and be undone by it (kshitijk4poor F22/N05). """ - if not owner or owner not in _owners(read_host_obligation() or {}): - return # nothing of ours to hand back: no lock taken, no state dir created path = host_obligation_path() + if not owner or owner not in _owners(_read_record(path) or {}): + return # nothing of ours to hand back: no lock taken, no state dir created with _record_mutex(path): _release_locked(path, owner) def _release_locked(path: Path, owner: str) -> None: - record = read_host_obligation() + record = _read_record(path) owners = _owners(record) if record is not None else [] if owner not in owners: - return # rewritten since the unlocked look (another install's arm): theirs now + return # rewritten since the unlocked look (another run's arm): theirs now remaining = [o for o in owners if o != owner] if remaining: _write_record(path, {**record, "owners": remaining}) @@ -282,11 +305,13 @@ def replace_bytes(path: Path, data: bytes) -> None: def clear_host_obligation() -> None: - """Discharge the obligation for the whole host. Never raises.""" - try: - host_obligation_path().unlink(missing_ok=True) - except OSError as exc: - logger.debug("Could not clear host update-restart obligation: %s", exc) + """Discharge this install's obligation (and an older release's unkeyed one). Never raises.""" + path = host_obligation_path() + for owed in (path, path.with_name(HOST_OBLIGATION_NAME)): + try: + owed.unlink(missing_ok=True) + except OSError as exc: + logger.debug("Could not clear host update-restart obligation: %s", exc) def obligation_fields() -> Optional[dict[str, str]]: @@ -316,12 +341,12 @@ def mark_host_restart_completed(sha: str) -> None: def _update_record(change: Callable[[dict], None], what: str) -> None: """Read, ``change`` and rewrite an armed record under ``_record_mutex`` (an arm's new owner stake is never overwritten by a stale copy). Never raises; no record, no write.""" - if read_host_obligation() is None: + path = _owed_path() + if _read_record(path) is None: return - path = host_obligation_path() try: with _record_mutex(path): - record = read_host_obligation() + record = _read_record(path) if record is None: return change(record) diff --git a/tests/hermes_cli/test_old_updater_takeover.py b/tests/hermes_cli/test_old_updater_takeover.py index 836ae657d3..460dd82560 100644 --- a/tests/hermes_cli/test_old_updater_takeover.py +++ b/tests/hermes_cli/test_old_updater_takeover.py @@ -535,8 +535,8 @@ def test_takeover_arms_the_host_record_without_application_dependencies(tmp_path stdin=subprocess.DEVNULL, capture_output=True, text=True, encoding="utf-8", timeout=60, ) assert child.returncode == 0, child.stdout + child.stderr - record = json.loads((lock_dir / "host-update-restart.json").read_text(encoding="utf-8-sig")) - assert record["expected_sha"] == head + [armed] = lock_dir.glob("host-update-restart-*.json") # the install-keyed record (review S3) + assert json.loads(armed.read_text(encoding="utf-8-sig"))["expected_sha"] == head def test_a_finish_child_that_cannot_start_after_the_commit_is_owed_not_failed(tmp_path, monkeypatch, capsys): diff --git a/tests/hermes_cli/test_update_host_obligation.py b/tests/hermes_cli/test_update_host_obligation.py index a005cb15b0..118bbf8696 100644 --- a/tests/hermes_cli/test_update_host_obligation.py +++ b/tests/hermes_cli/test_update_host_obligation.py @@ -219,6 +219,42 @@ def test_unwritable_host_state_dir_never_hides_the_debt_from_another_profile(two lock_dir.chmod(0o700) +def test_one_installs_completed_restart_never_erases_another_installs_debt(tmp_path): + """Two installations of one OS user share the host state dir. Install B arming its own pull and + then completing its restart must leave install A's debt standing; each install arms, reads and + clears from its own process, the way two checkouts' ``hermes update`` runs do (review S3).""" + import shutil + import subprocess + import sys + from pathlib import Path + + source = Path(__file__).resolve().parents[2] + files = ("hermes_constants.py", "hermes_cli/__init__.py", "hermes_cli/update_host_obligation.py", + "hermes_cli/update_restart_recovery.py", "hermes_cli/update_lock.py") + roots = {} + for name in ("install-a", "install-b"): + roots[name] = tmp_path / name + for rel in files: + (roots[name] / rel).parent.mkdir(parents=True, exist_ok=True) + shutil.copy2(source / rel, roots[name] / rel) + env = {**os.environ, "HERMES_GATEWAY_LOCK_DIR": str(tmp_path / "gateway-locks")} + + def run(install: str, body: str) -> str: + code = (f"import sys\nsys.path.insert(0, {str(roots[install])!r})\n" + "from hermes_cli.update_host_obligation import *\n" + body) + child = subprocess.run([sys.executable, "-I", "-S", "-B", "-c", code], env=env, capture_output=True, + text=True, encoding="utf-8", stdin=subprocess.DEVNULL, timeout=60) + assert child.returncode == 0, child.stdout + child.stderr + return child.stdout.strip() + + run("install-a", f"assert write_host_obligation(expected_sha={'a' * 40!r}, owner='run-a')") + run("install-b", f"assert write_host_obligation(expected_sha={'b' * 40!r}, owner='run-b')\n" + f"mark_host_restart_completed({'b' * 40!r})\nclear_host_obligation()") + + assert run("install-a", "print((read_host_obligation() or {}).get('expected_sha'))") == "a" * 40 + assert run("install-b", "print(host_obligation_present())") == "False" + + def test_unreadable_host_record_is_never_discharged_by_the_legacy_marker(two_profiles, no_live_fleet, monkeypatch, tmp_path): """A record whose terms are UNKNOWN cannot be settled by another record's terms. diff --git a/website/docs/getting-started/updating.md b/website/docs/getting-started/updating.md index 7fb59be1e7..3247731f60 100644 --- a/website/docs/getting-started/updating.md +++ b/website/docs/getting-started/updating.md @@ -288,7 +288,7 @@ The same inventory is embedded in every real update's receipt (`~/.hermes/logs/u Every `hermes update` run writes a machine-readable receipt to `~/.hermes/logs/update_receipts/` (last 20 kept, `latest.json` always points at the most recent): the pre-update fleet plan, each step taken, anything skipped and why, the gateway restart outcome, and the final fleet version matrix. The SQLite runtime repair is one of those steps (`sqlite_runtime_repair`): a failed repair records the actual reason (for example the `uv sync` error) and the SQLite version pair, a deferred or not-applicable repair lands in the skips with its reason. After the restart phase the updater compares each live gateway's running code against the freshly updated checkout and prints a per-profile matrix — a gateway still serving pre-update code is reported loudly with the exact restart command and recorded as a `gateway_restart` follow-up in the receipt; the code is already in place, so the update still exits 0, and every later command warns until the fleet serves the new code, so a mixed-version fleet is never silently healthy. Both `--plan` and the fleet check ask each running gateway directly over its local control socket (`gateway.sock` in the profile's data directory, a named pipe on Windows) when available, so version and supervisor information comes from the gateway itself; gateways from older versions are still discovered through their state files as before. -A multiplexed default gateway is one process serving several profiles, so it appears once in the matrix and vouches for every profile in its `served_profiles` record. The same coverage clears the "A previous `hermes update` pulled new code but did not restart running gateways" hint: once that gateway (or, after a manual `git pull`, every gateway an update restarted) runs the current code, `hermes gateway restart` is enough — the hint no longer waits for the next `hermes update` to write a fresh receipt. The same is true of the restart obligation left by an update that died before recording which gateways it owed (or by an older updater that never recorded them): once every live gateway runs the current checkout, the obligation is retired and the hint stops. On a host that runs no gateway at all (the Desktop app alone), it is retired once no profile has a gateway that went away without a clean stop and every running backend is restarted by its own supervisor or has its own reminder. That obligation is recorded once per HOST, in the cross-profile rendezvous directory (`$HERMES_GATEWAY_LOCK_DIR`, else `$XDG_STATE_HOME/hermes/gateway-locks`) as `host-update-restart.json`, so every profile's CLI sees the same one: `hermes -p coder update` and `hermes -p writer update` restart the shared multiplexed gateway once between them, not once each. An obligation left behind by an older per-profile updater (`fleet_restart_pending` in one profile's Hermes home) is still read and cleared. An update whose pre-update plan found no gateway at all owes nothing and leaves no breadcrumb. A backend supervised by Desktop, systemd or launchd is restarted by its supervisor and never blocks this settlement; only a manual backend whose reminder could not be saved keeps the obligation open. +A multiplexed default gateway is one process serving several profiles, so it appears once in the matrix and vouches for every profile in its `served_profiles` record. The same coverage clears the "A previous `hermes update` pulled new code but did not restart running gateways" hint: once that gateway (or, after a manual `git pull`, every gateway an update restarted) runs the current code, `hermes gateway restart` is enough — the hint no longer waits for the next `hermes update` to write a fresh receipt. The same is true of the restart obligation left by an update that died before recording which gateways it owed (or by an older updater that never recorded them): once every live gateway runs the current checkout, the obligation is retired and the hint stops. On a host that runs no gateway at all (the Desktop app alone), it is retired once no profile has a gateway that went away without a clean stop and every running backend is restarted by its own supervisor or has its own reminder. That obligation is recorded once per HOST, in the cross-profile rendezvous directory (`$HERMES_GATEWAY_LOCK_DIR`, else `$XDG_STATE_HOME/hermes/gateway-locks`) as `host-update-restart-.json` (one per installation, so a second Hermes checkout for the same user keeps its own), so every profile's CLI sees the same one: `hermes -p coder update` and `hermes -p writer update` restart the shared multiplexed gateway once between them, not once each. An obligation left behind by an older per-profile updater (`fleet_restart_pending` in one profile's Hermes home) is still read and cleared. An update whose pre-update plan found no gateway at all owes nothing and leaves no breadcrumb. A backend supervised by Desktop, systemd or launchd is restarted by its supervisor and never blocks this settlement; only a manual backend whose reminder could not be saved keeps the obligation open. ### Manual backend restart reminders