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..63a5bfe8d71e1 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,17 +209,32 @@ 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 }) + } + + /// 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 completed update marker"); + } + } } } impl Drop for UpdateMarkerGuard { fn drop(&mut self) { - 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"); - } - } + self.complete(); } } @@ -152,7 +248,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")) @@ -453,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 { @@ -477,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. @@ -1102,7 +1253,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 +1279,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 +1290,97 @@ 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 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!( diff --git a/hermes_cli/main.py b/hermes_cli/main.py index e7859bd351bd5..30c3104316e6f 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -8983,9 +8983,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..cfd6a4ea697e1 --- /dev/null +++ b/hermes_cli/update_lock.py @@ -0,0 +1,214 @@ +"""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. + + 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. + + Any pid we cannot evaluate counts as dead: a corrupt marker must not wedge + the lock forever. + """ + if pid <= 0: + return False + try: + 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 + + +@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/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/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" 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