From b3daf1609c6ca4c3feb176599d26a950774b84cd Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Wed, 29 Jul 2026 17:35:16 -0500 Subject: [PATCH 1/5] fix(installer): never let a stale --commit pin roll an install backwards hermes-setup.exe bakes its build-time commit into the binary (BUILD_PIN_COMMIT) and passes it as -Commit on every install-mode run, including the retry the desktop's "Update didn't finish" screen kicks off. The repository stage checked that SHA out unconditionally, so an installer built months earlier rewound a current managed checkout to its build commit -- 9,160 commits in the reported case -- leaving ancient source against a current venv. npm then failed on workspaces that did not exist yet at that commit, and every later update ran against the wrong tree. Skip the pin when its target is already an ancestor of HEAD. Fresh clones have no such ancestry so reproducible/CI pinning is unchanged, and --force-commit / -ForceCommit still rolls back on purpose. --- scripts/install.ps1 | 34 +++++- scripts/install.sh | 31 ++++- tests/test_install_commit_pin_rollback.py | 138 ++++++++++++++++++++++ 3 files changed, 199 insertions(+), 4 deletions(-) create mode 100644 tests/test_install_commit_pin_rollback.py diff --git a/scripts/install.ps1 b/scripts/install.ps1 index aa434eed47f04..b39464f42c26e 100644 --- a/scripts/install.ps1 +++ b/scripts/install.ps1 @@ -22,6 +22,12 @@ param( # cloning the full default-branch history) and then `git checkout`s the # exact ref. Precedence: Commit > Tag > Branch. [string]$Commit = "", + # Apply -Commit even when it would roll an existing install BACKWARDS. + # Without this the repository stage skips a pin that is already an ancestor + # of HEAD, so a stale baked-in BUILD_PIN_COMMIT can't downgrade a current + # checkout. Reproducible/CI installs that genuinely want an older SHA on an + # existing tree pass -ForceCommit. + [switch]$ForceCommit, [string]$Tag = "", [string]$HermesHome = $(if ($env:HERMES_HOME) { $env:HERMES_HOME } else { "$env:LOCALAPPDATA\hermes" }), [string]$InstallDir = $(if ($env:HERMES_HOME) { "$env:HERMES_HOME\hermes-agent" } else { "$env:LOCALAPPDATA\hermes\hermes-agent" }), @@ -1505,8 +1511,32 @@ function Install-Repository { # Make sure we have the commit locally (a tag-less commit # SHA isn't always reachable from any one branch fetch). git -c windows.appendAtomically=false fetch origin $Commit - git -c windows.appendAtomically=false checkout --detach $Commit - if ($LASTEXITCODE -ne 0) { throw "git checkout $Commit failed (exit $LASTEXITCODE)" } + # A commit pin must never move an existing install + # BACKWARDS. hermes-setup.exe bakes its build-time commit + # into the binary (BUILD_PIN_COMMIT) and passes it as + # -Commit on every install-mode run -- including the retry + # the desktop's "Update didn't finish" screen kicks off. An + # installer built months ago would otherwise rewind a + # current checkout to its build commit, leaving ancient + # code against a current venv (npm workspaces and Python + # deps that no longer match: the #74xxx report). Skip the + # pin when the target is already an ancestor of HEAD; a + # fresh clone has no such ancestry and pins normally. + $skipRollback = $false + if (-not $ForceCommit) { + git -c windows.appendAtomically=false merge-base --is-ancestor $Commit HEAD 2>$null + $isAncestor = ($LASTEXITCODE -eq 0) + $pinnedSha = (& git -c windows.appendAtomically=false rev-parse "$Commit^{commit}" 2>$null) + $headSha = (& git -c windows.appendAtomically=false rev-parse HEAD 2>$null) + $skipRollback = $isAncestor -and ($pinnedSha -ne $headSha) + } + if ($skipRollback) { + Write-Warn "Ignoring -Commit $Commit`: the checkout is already newer." + Write-Warn "Pinning to it would roll this install back. Pass -ForceCommit to override." + } else { + git -c windows.appendAtomically=false checkout --detach $Commit + if ($LASTEXITCODE -ne 0) { throw "git checkout $Commit failed (exit $LASTEXITCODE)" } + } } elseif ($Tag) { git -c windows.appendAtomically=false fetch origin "refs/tags/${Tag}:refs/tags/${Tag}" git -c windows.appendAtomically=false checkout --detach "refs/tags/$Tag" diff --git a/scripts/install.sh b/scripts/install.sh index 119e99dc2298d..493b53e412cce 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -73,6 +73,7 @@ SKIP_BROWSER=false NO_SKILLS=false BRANCH="main" INSTALL_COMMIT="" +FORCE_COMMIT=false ENSURE_DEPS="" MANIFEST_MODE=false @@ -117,6 +118,10 @@ while [[ $# -gt 0 ]]; do INSTALL_COMMIT="$2" shift 2 ;; + --force-commit|-ForceCommit) + FORCE_COMMIT=true + shift + ;; --manifest|-Manifest) MANIFEST_MODE=true shift @@ -165,6 +170,8 @@ while [[ $# -gt 0 ]]; do echo " 'hermes update' runs never inject bundled skills either" echo " --branch NAME Git branch to install (default: main)" echo " --commit SHA Pin checkout to a specific commit after clone/update" + echo " (ignored when it would roll an existing install back)" + echo " --force-commit Apply --commit even if it rolls the install backwards" echo " --manifest Print desktop bootstrap stage manifest as JSON" echo " --stage NAME Run one desktop bootstrap stage" echo " --json Print a JSON result frame for --stage" @@ -1330,11 +1337,31 @@ EOF cd "$INSTALL_DIR" if [ -n "$INSTALL_COMMIT" ]; then - log_info "Pinning checkout to commit $INSTALL_COMMIT..." + # A commit pin must never move an existing install BACKWARDS. The + # bootstrap installer bakes its build-time commit into the binary + # (BUILD_PIN_COMMIT) and passes it as --commit on every install-mode + # run -- including the one the desktop's failure screen retries. An + # installer built months ago would otherwise rewind a current checkout + # to its build commit, stranding the user on ancient code with a + # current venv. Only pin when the target is not already an ancestor of + # HEAD; a fresh clone has no such ancestry and pins normally. if ! git cat-file -e "$INSTALL_COMMIT^{commit}" 2>/dev/null; then git fetch origin "$INSTALL_COMMIT" || true fi - git checkout --detach "$INSTALL_COMMIT" + if git rev-parse --verify --quiet HEAD >/dev/null 2>&1 \ + && git merge-base --is-ancestor "$INSTALL_COMMIT" HEAD 2>/dev/null \ + && [ "$(git rev-parse "$INSTALL_COMMIT^{commit}" 2>/dev/null)" != "$(git rev-parse HEAD)" ]; then + if [ "$FORCE_COMMIT" = true ]; then + log_warn "--force-commit: rolling this install back to $INSTALL_COMMIT." + git checkout --detach "$INSTALL_COMMIT" + else + log_warn "Ignoring --commit $INSTALL_COMMIT: the checkout is already newer." + log_warn "Pinning to it would roll this install back. Pass --force-commit to override." + fi + else + log_info "Pinning checkout to commit $INSTALL_COMMIT..." + git checkout --detach "$INSTALL_COMMIT" + fi fi log_success "Repository ready" diff --git a/tests/test_install_commit_pin_rollback.py b/tests/test_install_commit_pin_rollback.py new file mode 100644 index 0000000000000..4d6de7986cd9f --- /dev/null +++ b/tests/test_install_commit_pin_rollback.py @@ -0,0 +1,138 @@ +"""Regression: a stale ``--commit`` pin must not roll an install backwards. + +``hermes-setup.exe`` bakes its build-time commit into the binary +(``BUILD_PIN_COMMIT``) and passes it as ``-Commit`` / ``--commit`` on every +install-mode run — including the retry the desktop's "Update didn't finish" +screen kicks off. The repository stage used to ``git checkout --detach`` that +SHA unconditionally, so an installer built months earlier rewound a current +managed checkout to its build commit (observed: ~9k commits back), leaving +ancient source against a current venv — npm workspaces and Python deps that no +longer match, and every subsequent update failing against the wrong tree. + +The pin is skipped when its target is already an ancestor of HEAD, unless the +caller explicitly passes ``--force-commit`` / ``-ForceCommit``. A fresh clone +has no such ancestry, so reproducible/CI pinning is unaffected. + +``install.ps1`` carries the same guard (that is the path the Windows report +hit), but there is no PowerShell host in CI to execute it against a real repo, +and asserting on the script's *source text* would test its shape rather than +its behavior. These run the bash implementation of the same logic for real. +""" + +from __future__ import annotations + +import re +import shutil +import subprocess +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parent.parent +INSTALL_SH = REPO_ROOT / "scripts" / "install.sh" + +pytestmark = pytest.mark.skipif( + shutil.which("git") is None or shutil.which("bash") is None, + reason="needs git and bash", +) + + +def _git(cwd: Path, *args: str) -> str: + return subprocess.run( + ["git", "-c", "user.email=t@t", "-c", "user.name=t", *args], + cwd=cwd, + check=True, + capture_output=True, + text=True, + ).stdout.strip() + + +def _extract_pin_block() -> str: + """Pull the commit-pin block out of install.sh's update_repo().""" + text = INSTALL_SH.read_text() + match = re.search( + r'if \[ -n "\$INSTALL_COMMIT" \]; then.*?\n fi\n', + text, + re.DOTALL, + ) + assert match is not None, "commit-pin block not found in install.sh" + return match.group(0) + + +@pytest.fixture +def repo(tmp_path): + """A checkout with three commits, HEAD at the newest.""" + origin = tmp_path / "origin" + origin.mkdir() + _git(origin, "init", "-q", "-b", "main") + shas = [] + for n in range(3): + (origin / "f.txt").write_text(f"rev{n}\n") + _git(origin, "add", "f.txt") + _git(origin, "commit", "-qm", f"rev{n}") + shas.append(_git(origin, "rev-parse", "HEAD")) + return origin, shas + + +def _run_pin_block(repo_dir: Path, commit: str, *, force: bool = False) -> str: + """Execute install.sh's pin block standalone against ``repo_dir``.""" + script = "\n".join( + [ + "set -e", + "log_info() { echo \"INFO $*\"; }", + "log_warn() { echo \"WARN $*\"; }", + f'INSTALL_COMMIT="{commit}"', + f'FORCE_COMMIT={"true" if force else "false"}', + f'cd "{repo_dir}"', + _extract_pin_block(), + ] + ) + return subprocess.run( + ["bash", "-c", script], + capture_output=True, + text=True, + check=True, + ).stdout + + +def test_stale_pin_does_not_rewind_a_newer_checkout(repo): + """The reported failure: an old baked-in pin downgrading a current tree.""" + repo_dir, shas = repo + head_before = _git(repo_dir, "rev-parse", "HEAD") + + out = _run_pin_block(repo_dir, shas[0]) + + assert _git(repo_dir, "rev-parse", "HEAD") == head_before, ( + "a pin older than HEAD must leave the checkout where it is" + ) + assert "already newer" in out + + +def test_force_commit_still_rolls_back(repo): + """Reproducible/CI installs that genuinely want an older SHA keep working.""" + repo_dir, shas = repo + + _run_pin_block(repo_dir, shas[0], force=True) + + assert _git(repo_dir, "rev-parse", "HEAD") == shas[0] + + +def test_pin_to_current_head_is_applied(repo): + """Pinning to HEAD itself is a no-op checkout, not a skipped one.""" + repo_dir, shas = repo + + out = _run_pin_block(repo_dir, shas[2]) + + assert _git(repo_dir, "rev-parse", "HEAD") == shas[2] + assert "already newer" not in out + + +def test_pin_to_a_newer_commit_is_applied(repo): + """Rolling FORWARD to a newer pin is the legitimate case — never blocked.""" + repo_dir, shas = repo + _git(repo_dir, "checkout", "-q", "--detach", shas[0]) + + out = _run_pin_block(repo_dir, shas[2]) + + assert _git(repo_dir, "rev-parse", "HEAD") == shas[2] + assert "already newer" not in out From fe8e4d93daad6a3ae24b69401946b0e3bce3df31 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Wed, 29 Jul 2026 17:45:48 -0500 Subject: [PATCH 2/5] 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 4db17c3e872d8..759b0c96fd7ca 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 0000000000000..32728bab65ddc --- /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 0000000000000..93dfef7101652 --- /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" From 38d5f44df1895fc33847dbdeb61afb29cdeca86d Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Wed, 29 Jul 2026 17:46:00 -0500 Subject: [PATCH 3/5] fix(installer): refuse a second updater instead of clobbering the marker UpdateMarkerGuard::acquire overwrote the in-progress marker unconditionally, so a Tauri update launched while a dashboard-spawned "hermes update" was mid-flight simply took the marker and ran a second updater over the same checkout. That is the race behind the reported Windows failure: install-mode bootstrap rewound the tree while the dashboard's updater was still running npm install against it. acquire now returns Result and refuses when a live foreign owner holds the marker, and Drop no longer deletes a marker this process does not own. Liveness matches the Python and Electron readers of the same file: dead pid or past the shared age ceiling means stale and reclaimable, so a crashed updater cannot wedge future updates. Adds a cfg(unix) libc dependency for the signal-0 liveness probe; the Windows path uses OpenProcess/GetExitCodeProcess. --- apps/bootstrap-installer/src-tauri/Cargo.toml | 4 + .../src-tauri/src/update.rs | 208 +++++++++++++++++- 2 files changed, 203 insertions(+), 9 deletions(-) diff --git a/apps/bootstrap-installer/src-tauri/Cargo.toml b/apps/bootstrap-installer/src-tauri/Cargo.toml index fe65ff9aa7be3..d6b012aed6745 100644 --- a/apps/bootstrap-installer/src-tauri/Cargo.toml +++ b/apps/bootstrap-installer/src-tauri/Cargo.toml @@ -66,6 +66,10 @@ windows-sys = { version = "0.59", features = [ "Win32_UI_WindowsAndMessaging", ] } +# Signal-0 liveness probe for the update-lock marker owner (update.rs). +[target.'cfg(unix)'.dependencies] +libc = "0.2" + [profile.release] # A 5-10MB signed installer is the goal. LTO + size-opt + single codegen unit. panic = "abort" diff --git a/apps/bootstrap-installer/src-tauri/src/update.rs b/apps/bootstrap-installer/src-tauri/src/update.rs index be4884ab2634b..f689bb6f0c758 100644 --- a/apps/bootstrap-installer/src-tauri/src/update.rs +++ b/apps/bootstrap-installer/src-tauri/src/update.rs @@ -107,16 +107,97 @@ pub async fn start_update(app: AppHandle) -> Result<(), String> { /// future desktop launches. The marker payload is `{pid}\n{started_at_unix}` /// so the desktop's launch gate can detect a stale marker (dead PID / past a /// hard ceiling) and self-heal rather than wait forever. +/// +/// The marker is also the cross-process update lock: `hermes update` claims +/// the same file (see `hermes_cli/update_lock.py`) so a dashboard-spawned +/// update and this updater can't mutate one checkout at the same time. +/// `acquire` therefore REFUSES when a live foreign owner holds it rather than +/// overwriting — the pre-fix clobber is what let a dashboard `hermes update` +/// keep running while install-mode bootstrap rewrote the tree underneath it. struct UpdateMarkerGuard { path: PathBuf, + /// False when a live foreign updater already owns the marker: we hold no + /// claim, so `Drop` must not delete their marker. + owned: bool, +} + +/// Never treat a marker older than this as a live update. Mirrors +/// UPDATE_MARKER_MAX_AGE_MS in apps/desktop/electron/update-marker.ts and +/// UPDATE_MARKER_MAX_AGE_SECONDS in hermes_cli/update_lock.py — all three read +/// this one file, so a shorter ceiling in any of them would steal a lock the +/// others still consider live. +const UPDATE_MARKER_MAX_AGE_SECS: u64 = 20 * 60; + +/// The pid + age of a confirmed-live update holding the marker. +struct MarkerOwner { + pid: u32, + age_secs: u64, +} + +/// Read the marker and report a live owner, if any. `None` for every "no live +/// update" case — absent, unreadable, malformed, dead pid, past the ceiling — +/// matching `readLiveUpdateMarker` in the Electron gate. Never panics. +fn live_marker_owner(path: &Path) -> Option { + let raw = std::fs::read_to_string(path).ok()?; + let mut lines = raw.lines(); + let pid: u32 = lines.next()?.trim().parse().ok()?; + let started_at: u64 = lines.next().unwrap_or("").trim().parse().unwrap_or(0); + let now = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_secs()) + .unwrap_or(0); + let age_secs = now.saturating_sub(started_at); + if age_secs > UPDATE_MARKER_MAX_AGE_SECS || !pid_is_alive(pid) { + return None; + } + Some(MarkerOwner { pid, age_secs }) +} + +/// True when a process with `pid` currently exists. +#[cfg(windows)] +fn pid_is_alive(pid: u32) -> bool { + use windows_sys::Win32::Foundation::{CloseHandle, STILL_ACTIVE}; + use windows_sys::Win32::System::Threading::{ + GetExitCodeProcess, OpenProcess, PROCESS_QUERY_LIMITED_INFORMATION, + }; + + unsafe { + let handle = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, 0, pid); + if handle.is_null() { + // Either the pid is gone or we lack rights to open it. A pid we + // can't inspect is treated as dead so an unopenable straggler + // can't wedge every future update. + return false; + } + let mut code: u32 = 0; + let ok = GetExitCodeProcess(handle, &mut code); + CloseHandle(handle); + ok != 0 && code == STILL_ACTIVE as u32 + } +} + +#[cfg(not(windows))] +fn pid_is_alive(pid: u32) -> bool { + // signal 0 delivers nothing; it only probes existence/permission. + // ESRCH => dead. EPERM => alive but owned by another user. + let rc = unsafe { libc::kill(pid as libc::pid_t, 0) }; + if rc == 0 { + return true; + } + std::io::Error::last_os_error().raw_os_error() == Some(libc::EPERM) } impl UpdateMarkerGuard { - /// Write the marker. Best-effort: a write failure must NOT abort the - /// update (the gate degrades to "no marker => proceed", i.e. exactly the - /// pre-fix behavior), so we log and carry on with a guard that still - /// attempts cleanup of whatever may exist at the path. - fn acquire(path: PathBuf) -> Self { + /// Claim the marker, or report the live updater that already owns it. + /// + /// Writing is best-effort: a write failure must NOT abort the update (the + /// gate degrades to "no marker => proceed", i.e. exactly the pre-marker + /// behavior), so we log and carry on with a guard that still attempts + /// cleanup of whatever may exist at the path. + fn acquire(path: PathBuf) -> Result { + if let Some(owner) = live_marker_owner(&path) { + return Err(owner); + } let pid = std::process::id(); let started_at = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) @@ -128,12 +209,15 @@ impl UpdateMarkerGuard { if let Err(err) = std::fs::write(&path, format!("{pid}\n{started_at}")) { tracing::warn!(?path, %err, "could not write update-in-progress marker"); } - Self { path } + Ok(Self { path, owned: true }) } } impl Drop for UpdateMarkerGuard { fn drop(&mut self) { + if !self.owned { + return; + } if let Err(err) = std::fs::remove_file(&self.path) { if err.kind() != std::io::ErrorKind::NotFound { tracing::warn!(path = ?self.path, %err, "could not remove update-in-progress marker"); @@ -152,7 +236,39 @@ async fn run_update(app: AppHandle) -> Result<()> { // it, that backend re-locks the venv shim, our `force_kill_other_hermes` // straggler-cleanup kills it, and the relaunch/kill cycle loops. The guard // removes the marker on every exit path (incl. early returns / panics). - let _update_marker = UpdateMarkerGuard::acquire(crate::paths::update_in_progress_marker()); + // + // The same marker is the cross-process update lock (hermes_cli/ + // update_lock.py claims it too), so a live foreign owner means another + // updater — most often a dashboard-spawned `hermes update` — is already + // mutating this checkout. Refuse instead of running a second one over it. + let _update_marker = match UpdateMarkerGuard::acquire( + crate::paths::update_in_progress_marker(), + ) { + Ok(guard) => guard, + Err(owner) => { + let mins = owner.age_secs / 60; + let secs = owner.age_secs % 60; + let elapsed = if mins > 0 { + format!("{mins}m {secs}s") + } else { + format!("{secs}s") + }; + let msg = format!( + "Another Hermes update is already running (PID {}, started {} ago). \ + Wait for it to finish, or close the window or dashboard tab that \ + started it, then try again.", + owner.pid, elapsed + ); + emit( + &app, + BootstrapEvent::Failed { + stage: None, + error: msg.clone(), + }, + ); + return Err(anyhow!(msg)); + } + }; let update_branch = update_branch_from_args(std::env::args().skip(1)) .or_else(|| option_env_string("BUILD_PIN_BRANCH")) @@ -1102,7 +1218,8 @@ mod tests { let marker = dir.join(".hermes-update-in-progress"); { - let _g = UpdateMarkerGuard::acquire(marker.clone()); + let _g = UpdateMarkerGuard::acquire(marker.clone()) + .unwrap_or_else(|_| panic!("no live owner => acquire must succeed")); assert!(marker.exists(), "marker must exist while the guard is held"); let body = std::fs::read_to_string(&marker).unwrap(); let pid_line = body.lines().next().unwrap(); @@ -1127,7 +1244,8 @@ mod tests { std::fs::create_dir_all(&dir).unwrap(); let marker = dir.join(".hermes-update-in-progress"); - let guard = UpdateMarkerGuard::acquire(marker.clone()); + let guard = UpdateMarkerGuard::acquire(marker.clone()) + .unwrap_or_else(|_| panic!("no live owner => acquire must succeed")); // Simulate an external cleanup (e.g. the desktop pruned a marker it // judged stale) before our guard drops — Drop must not panic. std::fs::remove_file(&marker).unwrap(); @@ -1137,6 +1255,78 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + #[test] + fn acquire_refuses_while_a_live_updater_owns_the_marker() { + let dir = unique_tmp_dir("marker-contended"); + std::fs::create_dir_all(&dir).unwrap(); + let marker = dir.join(".hermes-update-in-progress"); + + // A live updater (us) holds it. A second updater must NOT clobber the + // marker and run concurrently over the same checkout — that race is + // what let a dashboard `hermes update` and install-mode bootstrap + // mutate one tree at once. + let held = UpdateMarkerGuard::acquire(marker.clone()) + .unwrap_or_else(|_| panic!("first acquire must succeed")); + let owner = UpdateMarkerGuard::acquire(marker.clone()) + .err() + .expect("second acquire must be refused while the first is live"); + assert_eq!(owner.pid, std::process::id()); + + // The refused guard must not delete the live owner's marker. + assert!(marker.exists(), "refused acquire must leave the marker intact"); + drop(held); + assert!(!marker.exists(), "the real owner still cleans up on drop"); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn acquire_reclaims_a_marker_owned_by_a_dead_pid() { + let dir = unique_tmp_dir("marker-dead-pid"); + std::fs::create_dir_all(&dir).unwrap(); + let marker = dir.join(".hermes-update-in-progress"); + + // pid 1 exists everywhere, so fabricate a dead one: a very large pid + // that no live process owns. A crashed updater must never wedge every + // future update. + let started_at = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_secs()) + .unwrap_or(0); + std::fs::write(&marker, format!("4294967294\n{started_at}")).unwrap(); + + let guard = UpdateMarkerGuard::acquire(marker.clone()) + .unwrap_or_else(|_| panic!("a dead owner must not block acquisition")); + let body = std::fs::read_to_string(&marker).unwrap(); + assert_eq!( + body.lines().next().unwrap().trim().parse::().unwrap(), + std::process::id(), + "reclaiming rewrites the marker with our pid" + ); + drop(guard); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn acquire_reclaims_a_marker_past_the_age_ceiling() { + let dir = unique_tmp_dir("marker-stale-age"); + std::fs::create_dir_all(&dir).unwrap(); + let marker = dir.join(".hermes-update-in-progress"); + + // Our own (live) pid, but started well past the ceiling: a wedged + // updater must not hold the lock forever. + let long_ago = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_secs()) + .unwrap_or(0) + .saturating_sub(UPDATE_MARKER_MAX_AGE_SECS + 60); + std::fs::write(&marker, format!("{}\n{long_ago}", std::process::id())).unwrap(); + + let guard = UpdateMarkerGuard::acquire(marker.clone()) + .unwrap_or_else(|_| panic!("a marker past the ceiling must be reclaimable")); + drop(guard); + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn parses_update_branch_from_space_or_equals_args() { assert_eq!( From 6a444ebec7562e177aa748b83d6e6ad7000f70f3 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Wed, 29 Jul 2026 17:48:09 -0500 Subject: [PATCH 4/5] fix(installer): release the update marker on success, not just on drop The updater keeps a Tauri/Cocoa event loop alive while it relaunches the desktop, and that loop can outlive app.exit(0). Relying on Drop alone left a *successful* update looking active -- a live pid holding a fresh marker -- which blocked desktop startup and, now that the marker is also the cross-process update lock, every subsequent updater until the age ceiling expired. Release explicitly once all install-tree mutations are done, before the relaunch. complete() is idempotent so Drop still covers the failure and panic paths. Arms a process-exit fallback so a wedged event loop cannot leave a finished updater lingering as a live pid. Co-authored-by: nateEc --- .../src-tauri/src/update.rs | 62 +++++++++++++++++-- 1 file changed, 58 insertions(+), 4 deletions(-) diff --git a/apps/bootstrap-installer/src-tauri/src/update.rs b/apps/bootstrap-installer/src-tauri/src/update.rs index f689bb6f0c758..63a5bfe8d71e1 100644 --- a/apps/bootstrap-installer/src-tauri/src/update.rs +++ b/apps/bootstrap-installer/src-tauri/src/update.rs @@ -211,21 +211,33 @@ impl UpdateMarkerGuard { } Ok(Self { path, owned: true }) } -} -impl Drop for UpdateMarkerGuard { - fn drop(&mut self) { + /// Release the marker as soon as every mutating stage has completed. + /// + /// The updater still owns a Tauri/Cocoa event loop while it relaunches the + /// desktop, and that loop can outlive `app.exit(0)`. Relying on `Drop` + /// alone therefore leaves a *successful* update looking active — a live + /// pid holding a fresh marker — which blocks desktop startup and every + /// other updater for the full age ceiling. Idempotent: `Drop` still runs + /// and tolerates an already-removed marker. + fn complete(&self) { if !self.owned { return; } if let Err(err) = std::fs::remove_file(&self.path) { if err.kind() != std::io::ErrorKind::NotFound { - tracing::warn!(path = ?self.path, %err, "could not remove update-in-progress marker"); + tracing::warn!(path = ?self.path, %err, "could not remove completed update marker"); } } } } +impl Drop for UpdateMarkerGuard { + fn drop(&mut self) { + self.complete(); + } +} + async fn run_update(app: AppHandle) -> Result<()> { let hermes_home = crate::paths::hermes_home(); let install_root = hermes_home.join("hermes-agent"); @@ -569,6 +581,12 @@ async fn run_update(app: AppHandle) -> Result<()> { marker: None, }, ); + // Every install-tree mutation is finished. Release the lock BEFORE the + // relaunch: this process can stay wedged in its native event loop even + // after a successful app.exit(), and a live pid on a fresh marker would + // make a completed update look active — blocking desktop startup and + // every other updater until the age ceiling expires. + _update_marker.complete(); if let Some(target_app) = launch_target { if let Err(err) = launch_macos_app_and_exit(&app, &target_app).await { @@ -593,9 +611,26 @@ async fn run_update(app: AppHandle) -> Result<()> { ); } + // The launch helpers normally request exit themselves, but their failure + // paths must still close a successful updater. A native event loop can + // ignore that graceful request, so arm a process-exit fallback now that + // all update state and the marker have been settled. + exit_after_success(&app); Ok(()) } +/// Ask the app to exit, with a hard `process::exit` fallback for a native +/// event loop that ignores the graceful request. Without it a finished updater +/// can linger as a live pid forever. +fn exit_after_success(app: &AppHandle) { + std::thread::spawn(|| { + std::thread::sleep(std::time::Duration::from_secs(3)); + tracing::warn!("graceful updater exit timed out; forcing process exit"); + std::process::exit(0); + }); + app.exit(0); +} + /// Poll until the venv shim AND packaged desktop app bundle are no longer locked /// (Windows) or a bounded timeout elapses. On non-Windows this is a short fixed /// grace since file locking isn't the failure mode there. @@ -1327,6 +1362,25 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + #[test] + fn completed_update_releases_marker_before_guard_drop() { + let dir = unique_tmp_dir("marker-complete"); + std::fs::create_dir_all(&dir).unwrap(); + let marker = dir.join(".hermes-update-in-progress"); + + let guard = UpdateMarkerGuard::acquire(marker.clone()) + .unwrap_or_else(|_| panic!("no live owner => acquire must succeed")); + guard.complete(); + + assert!( + !marker.exists(), + "a successful update must unblock desktop startup before relaunch/exit" + ); + drop(guard); + assert!(!marker.exists(), "Drop stays idempotent after completion"); + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn parses_update_branch_from_space_or_equals_args() { assert_eq!( From 37519b4eebc50b70baa388dcb721f1b3a0eb1240 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Wed, 29 Jul 2026 17:52:06 -0500 Subject: [PATCH 5/5] fix(update): use the no-kill pid probe, not os.kill(pid, 0) The Windows-footgun linter caught a real bug in the new update lock. On Windows os.kill(pid, 0) is not a no-op: CPython routes sig=0 to GenerateConsoleCtrlEvent, which sends Ctrl+C to the target's entire console process group (bpo-14484). The liveness probe would have killed the very updater it was asking about -- and any sibling sharing that console. Delegate to gateway.status._pid_exists, the project's existing no-kill probe, which uses psutil (OpenProcess/GetExitCodeProcess on Windows) and also reports zombies as dead. Any pid we cannot evaluate still counts as dead so a corrupt marker cannot wedge the lock. --- hermes_cli/update_lock.py | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/hermes_cli/update_lock.py b/hermes_cli/update_lock.py index 32728bab65ddc..cfd6a4ea697e1 100644 --- a/hermes_cli/update_lock.py +++ b/hermes_cli/update_lock.py @@ -72,29 +72,27 @@ def update_marker_path() -> Path: 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. + 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). A liveness check that killed the updater it was + asking about would be a spectacular way to fix a concurrency bug. - 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. + Any pid we cannot evaluate counts as dead: a corrupt marker must not wedge + the lock forever. """ if pid <= 0: return False try: - os.kill(pid, 0) - except ProcessLookupError: + from gateway.status import _pid_exists + + 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 - 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)