diff --git a/hermes_cli/dashboard_procs.py b/hermes_cli/dashboard_procs.py index d5b27a081b..70a8564ced 100644 --- a/hermes_cli/dashboard_procs.py +++ b/hermes_cli/dashboard_procs.py @@ -525,12 +525,147 @@ def _exclude_pids_from_env() -> set[int]: return out +# --- SSH remote-backend lock ownership ------------------------------------- +# +# ``backend.lock.json`` is the ownership record the Desktop SSH runtime writes +# on the *remote* host for every ``hermes serve`` backend it spawns over SSH +# (see apps/desktop/electron/remote-lifecycle.ts). A backend started from +# another client/machine — e.g. a MacBook driving a ``hermes serve`` on a Mac +# Mini over SSH — is a *legitimate, lock-owned* backend even though it has no +# parent on this host (sshd has long since exited, reparenting it to pid 1). +# +# The orphan reap must NEVER kill a PID that a valid ``backend.lock.json`` +# claims as its owner. Doing so murdered a real production SSH remote backend +# on a Mac Mini the first time the local Desktop app rebooted. The lock file is +# the source of truth for "is this serve legitimately owned by some client", +# regardless of which machine started it. + +# Mirror the schema constants in remote-lifecycle.ts (the writer). Bumping one +# side without the other makes the lock unreadable on purpose, which is the +# safe failure mode for reuse — but for the reap we only ever *spare*, so a +# mismatched-schema record is simply ignored (never used to kill). +_LOCKFILE_SCHEMA_VERSION = 2 +_PROTOCOL_VERSION = 1 +_REMOTE_LOCK_SUBDIR = "desktop-ssh" +_HEX32 = set("0123456789abcdef") +_HEX16 = _HEX32 + + +def _hermes_home_dir() -> Path: + """Resolved Hermes home (HERMES_HOME override or ~/.hermes).""" + override = os.environ.get("HERMES_HOME", "").strip() + if override: + return Path(override).expanduser() + return Path.home() / ".hermes" + + +def _valid_lockfile_payload(parsed: object, ownership_id: str) -> bool: + """Validate a parsed ``backend.lock.json`` body, mirroring readLockfile(). + + Returns True only when every structural field the SSH runtime writes is + present and well-formed. A lock that fails validation is ignored (treated + as "no ownership claim"), which never causes a kill — the reap only ever + *adds* lock-owned PIDs to its spare-set. + """ + if not isinstance(parsed, dict): + return False + if parsed.get("schemaVersion") != _LOCKFILE_SCHEMA_VERSION: + return False + if parsed.get("protocolVersion") != _PROTOCOL_VERSION: + return False + if parsed.get("ownershipId") != ownership_id: + return False + spawn_nonce = parsed.get("spawnNonce") + if not isinstance(spawn_nonce, str) or len(spawn_nonce) != 16: + return False + if set(spawn_nonce) - _HEX16: + return False + token_fp = parsed.get("tokenFingerprint") + if not isinstance(token_fp, str) or len(token_fp) != 32 or set(token_fp) - _HEX32: + return False + pid = parsed.get("pid") + if not isinstance(pid, int) or pid <= 0 or pid > 4194304: + return False + port = parsed.get("port") + if not isinstance(port, int) or port < 0 or port > 65535: + return False + # String fields must be present and bounded (the writer enforces <=1024). + for field in ("profile", "hermesPath", "hermesHome", "logPath", "startedAt"): + value = parsed.get(field) + if not isinstance(value, str) or len(value) > 1024: + return False + # logPath is written as ``{lock_root}/{ownershipId}/{spawnNonce}.log``. We + # only check the suffix so a relocated HERMES_HOME (different leading path) + # doesn't falsely reject a legitimate remote-owned backend — a false reject + # here would re-introduce the exact kill we're fixing. + log_path = parsed["logPath"] + if not log_path.endswith(f"/{ownership_id}/{spawn_nonce}.log"): + return False + return True + + +def _lock_owned_serve_pids(base_dir: Path | None = None) -> set[int]: + """PIDs claimed as owners by valid ``backend.lock.json`` records on this host. + + Scans ``{hermes_home}/desktop-ssh//backend.lock.json`` (the + same directory the Desktop SSH runtime writes to). Any PID a valid lock + names is a legitimately-owned backend — including backends another client + or machine started over SSH — and must be spared by the orphan reap. + + Best-effort: any read/parse/IO error for a single record is swallowed and + that record contributes no PID. Never raises. + """ + import json + + root = base_dir if base_dir is not None else ( + _hermes_home_dir() / _REMOTE_LOCK_SUBDIR + ) + owned: set[int] = set() + if not root.is_dir(): + return owned + try: + entries = list(root.iterdir()) + except OSError: + return owned + for entry in entries: + try: + if not entry.is_dir(): + continue + except OSError: + continue + ownership_id = entry.name + # Mirror validateOwnershipId(): exactly 32 lowercase hex chars. + if len(ownership_id) != 32 or set(ownership_id) - _HEX32: + continue + lock_path = entry / "backend.lock.json" + try: + if not lock_path.is_file(): + continue + with open(lock_path, "rb") as handle: + data = handle.read() + except OSError: + continue + if len(data) > 65536: + continue + try: + parsed = json.loads(data) + except (UnicodeDecodeError, ValueError): + continue + if _valid_lockfile_payload(parsed, ownership_id): + try: + owned.add(int(parsed["pid"])) + except (TypeError, ValueError): + continue + return owned + + def _reap_orphaned_desktop_local_serves( *, reason: str = "orphaned desktop-local hermes serve", signal_term=None, signal_kill=None, sleep_fn=None, + lock_owned_pids_fn=None, ) -> dict[str, list]: """Kill leftover Desktop-local ``hermes serve`` backends with no parent. @@ -548,6 +683,10 @@ def _reap_orphaned_desktop_local_serves( - only the Desktop-local spawn shape (loopback + ``--port 0``) - only processes whose current ppid is 1 (or 0 on some supervisors) - never self / never HERMES_DESKTOP_CHILD_PID entries + - never a PID a valid ``backend.lock.json`` claims as its owner — that is + a legitimately lock-owned backend, *including SSH remote backends started + by another client/machine* which legitimately sit at ppid 1 after sshd + exits. Killing those is a production incident, not cleanup. - never fixed-port remote serves (e.g. ``--port 9119``) - best-effort; failures never raise to the caller """ @@ -560,6 +699,8 @@ def _reap_orphaned_desktop_local_serves( signal_kill = getattr(_signal, "SIGKILL", _signal.SIGTERM) if sleep_fn is None: sleep_fn = _time.sleep + if lock_owned_pids_fn is None: + lock_owned_pids_fn = _lock_owned_serve_pids if sys.platform == "win32": # Windows desktop uses taskkill tree teardown; orphan scan here is POSIX. @@ -572,16 +713,34 @@ def _reap_orphaned_desktop_local_serves( exclude.add(os.getppid()) except Exception: pass + # Spare every PID a valid backend.lock.json owns — SSH remote backends + # started by other clients/machines are legitimate, lock-owned owners even + # though they are orphaned (ppid 1) on this host. (#78872 regression) + try: + exclude |= set(lock_owned_pids_fn()) + except Exception: + # Best-effort: never let lock scanning block or widen the reap. + pass try: scanned = _scan_dashboard_processes(exclude_pids=exclude) except Exception: return {"matched": [], "killed": [], "failed": []} + # Re-read lock ownership defensively: the scan above already filtered + # exclude PIDs, but a lock file may have been written between the scan and + # now. Defense in depth — never kill a freshly-claimed owner. + try: + owned_now = set(lock_owned_pids_fn()) + except Exception: + owned_now = set() + targets: list[tuple[int, str]] = [] for pid, cmd in scanned: if not _is_desktop_local_serve_cmdline(cmd): continue + if pid in owned_now: + continue ppid = _process_ppid(pid) if ppid is None: continue diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index 5260a288bc..bcec243812 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -1052,6 +1052,10 @@ _CATEGORY_MERGE: Dict[str, str] = { # `doctor.live_probe_timeout` is the only schema-surfaced doctor field — # fold it into general rather than spawning a one-field orphan category. "doctor": "general", + # `runtime.nofile_soft_limit` (#78873) is the only schema-surfaced runtime + # field — fold it into the agent tab rather than spawning a one-field + # orphan category. + "runtime": "agent", } # Display order for tabs — unlisted categories sort alphabetically after these. diff --git a/tests/hermes_cli/test_orphan_desktop_serve_reap.py b/tests/hermes_cli/test_orphan_desktop_serve_reap.py index 01c38790b0..ab038d1f93 100644 --- a/tests/hermes_cli/test_orphan_desktop_serve_reap.py +++ b/tests/hermes_cli/test_orphan_desktop_serve_reap.py @@ -108,3 +108,180 @@ def test_reap_passes_child_pid_exclude_to_scan(): exclude = scan.call_args.kwargs["exclude_pids"] assert 111 in exclude assert 999 in exclude + + +# --------------------------------------------------------------------------- +# Regression: a legitimately lock-owned SSH remote backend (started by another +# client/machine) must survive the boot reap even though it matches the +# Desktop-local serve shape and is orphaned at ppid 1. Production incident: +# the local Desktop app's reboot reap killed a Mac Mini backend that a MacBook +# had launched over SSH — its PID was the recorded owner in backend.lock.json. +# --------------------------------------------------------------------------- + +import json +from hermes_cli.dashboard_procs import ( + _lock_owned_serve_pids, + _valid_lockfile_payload, +) + + +def _valid_lock_payload(pid: int, ownership_id: str, spawn_nonce: str) -> dict: + """A minimally-valid backend.lock.json body matching remote-lifecycle.ts.""" + return { + "schemaVersion": 2, + "protocolVersion": 1, + "ownershipId": ownership_id, + "spawnNonce": spawn_nonce, + "tokenFingerprint": "a" * 32, + "pid": pid, + "port": 0, + "profile": "default", + "hermesPath": "/opt/hermes/bin/hermes", + "hermesHome": "~/.hermes", + "logPath": f"~/.hermes/desktop-ssh/{ownership_id}/{spawn_nonce}.log", + "startedAt": "2026-08-04T20:00:00Z", + } + + +def test_lock_owned_serve_pids_reads_valid_backend_lock(tmp_path): + oid = "f" * 32 + nonce = "d" * 16 + lock_root = tmp_path / "desktop-ssh" + (lock_root / oid).mkdir(parents=True) + (lock_root / oid / "backend.lock.json").write_text( + json.dumps(_valid_lock_payload(7777, oid, nonce)) + ) + # A second, malformed lock (bad schemaVersion) must contribute nothing. + other_oid = "e" * 32 + (lock_root / other_oid).mkdir(parents=True) + (lock_root / other_oid / "backend.lock.json").write_text( + json.dumps({**_valid_lock_payload(8888, other_oid, nonce), "schemaVersion": 99}) + ) + assert _lock_owned_serve_pids(base_dir=lock_root) == {7777} + + +def test_valid_lockfile_payload_rejects_wrong_owner_and_shape(): + oid = "f" * 32 + nonce = "d" * 16 + good = _valid_lockfile_payload(_valid_lock_payload(1, oid, nonce), oid) + assert good is True + # ownershipId must match the directory it lives in. + assert _valid_lockfile_payload( + _valid_lock_payload(1, "e" * 32, nonce), oid + ) is False + # pid out of range. + bad_pid = _valid_lock_payload(1, oid, nonce) + bad_pid["pid"] = 0 + assert _valid_lockfile_payload(bad_pid, oid) is False + # port out of range. + bad_port = _valid_lock_payload(1, oid, nonce) + bad_port["port"] = 70000 + assert _valid_lockfile_payload(bad_port, oid) is False + # spawnNonce wrong length. + bad_nonce = _valid_lock_payload(1, oid, nonce) + bad_nonce["spawnNonce"] = "z" * 15 + assert _valid_lockfile_payload(bad_nonce, oid) is False + # logPath not ending in /.log. + bad_log = _valid_lock_payload(1, oid, nonce) + bad_log["logPath"] = "~/.hermes/desktop-ssh/{oid}/other.log".format(oid=oid) + assert _valid_lockfile_payload(bad_log, oid) is False + + +def test_reap_spare_lock_owned_ssh_remote_backend_of_foreign_client(): + """The exact production-incident shape: a foreign-client SSH remote backend + matches the Desktop-local serve shape and is orphaned at ppid 1, but a valid + backend.lock.json owns its PID. The reap must NOT kill it.""" + scanned = [ + (555, "hermes serve --host 127.0.0.1 --port 0"), # lock-owned remote + (666, "hermes serve --host 127.0.0.1 --port 0"), # genuine orphan + ] + ppids = {555: 1, 666: 1} + terms: list[int] = [] + live = {555, 666} + + def fake_kill(pid, sig): + if sig == 0: + if pid in live: + return None + raise ProcessLookupError() + if sig == 15: + terms.append(pid) + live.discard(pid) + return None + if sig == 9: + live.discard(pid) + return None + return None + + # 555 is claimed by a valid backend.lock.json; 666 is not. + lock_owned = {555} + + with ( + patch( + "hermes_cli.dashboard_procs._scan_dashboard_processes", + return_value=scanned, + ), + patch( + "hermes_cli.dashboard_procs._process_ppid", + side_effect=lambda pid: ppids.get(pid), + ), + patch("os.kill", side_effect=fake_kill), + patch("sys.platform", "darwin"), + ): + os.environ.pop("HERMES_DESKTOP_CHILD_PID", None) + result = _reap_orphaned_desktop_local_serves( + sleep_fn=lambda _s: None, + signal_term=15, + signal_kill=9, + lock_owned_pids_fn=lambda: lock_owned, + ) + + # Only the genuine orphan (666) is reaped; the lock-owned remote (555) lives. + assert set(result["matched"]) == {666} + assert set(terms) == {666} + assert 555 not in terms + assert set(result["killed"]) == {666} + + +def test_reap_spare_lock_owned_backend_even_without_exclude_match(tmp_path): + """End-to-end through the real lock scanner: a backend.lock.json on disk + spares a matching orphaned serve even when HERMES_DESKTOP_CHILD_PID is + unset (foreign-client backend, not ours by env either).""" + oid = "a" * 32 + nonce = "b" * 16 + lock_root = tmp_path / "desktop-ssh" + (lock_root / oid).mkdir(parents=True) + (lock_root / oid / "backend.lock.json").write_text( + json.dumps(_valid_lock_payload(4242, oid, nonce)) + ) + + scanned = [(4242, "hermes serve --host 127.0.0.1 --port 0")] + terms: list[int] = [] + + def fake_kill(pid, sig): + if sig == 15: + terms.append(pid) + return None + + with ( + patch( + "hermes_cli.dashboard_procs._scan_dashboard_processes", + return_value=scanned, + ), + patch( + "hermes_cli.dashboard_procs._process_ppid", + return_value=1, + ), + patch("os.kill", side_effect=fake_kill), + patch("sys.platform", "darwin"), + ): + os.environ.pop("HERMES_DESKTOP_CHILD_PID", None) + result = _reap_orphaned_desktop_local_serves( + sleep_fn=lambda _s: None, + signal_term=15, + signal_kill=9, + lock_owned_pids_fn=lambda: _lock_owned_serve_pids(base_dir=lock_root), + ) + + assert terms == [] + assert result["matched"] == [] diff --git a/tests/test_resource_limits.py b/tests/test_resource_limits.py index a31b9ef186..6f24ce0e51 100644 --- a/tests/test_resource_limits.py +++ b/tests/test_resource_limits.py @@ -206,6 +206,13 @@ def test_serve_startup_applies_limit_before_web_server(monkeypatch): import hermes_cli.plugins import hermes_cli.web_server + # cmd_dashboard(headless_backend=True) exports HERMES_SERVE_HEADLESS=1 into + # this process's environment (main.py serve path). Touch the key through + # monkeypatch FIRST so teardown restores the pre-test state — otherwise the + # leaked flag flips later web-server tests (mount_spa) into the headless + # 404 path. + monkeypatch.setenv("HERMES_SERVE_HEADLESS", "0") + calls: list[str] = [] monkeypatch.setattr( resource_limits,