From da3a0a852fd82041ce69e8170bf60bb747782080 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sat, 8 Aug 2026 19:53:01 -0700 Subject: [PATCH] fix(update): make orphan-backend reap tree-aware + drain Desktop update trees without pre-signalling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #82179 addressing helix4u's review comment (#82179 issuecomment-5229441571). Three parts: 1. Desktop teardown (salvaged from #77436, @4adwentures): the update hand-off's releaseBackendLock() sent SIGTERM to the primary backend BEFORE taskkill /T. If the launcher exits first, Windows can no longer enumerate its descendants and they survive holding the venv — the Electron path that creates the orphan #82179 then has to repair. New stopBackendTreesForUpdate() tree-kills the live root first, with the behavioral vitest from #77436. The scanner half of #77436 is deliberately NOT taken (superseded by #82158's full-cmdline scan). 2. Tree-aware orphan classification: _orphaned_desktop_backend_pids() previously refused the whole holder set when any holder had a live parent. But the scanner legitimately returns an orphaned serve root AND its descendants (the venv trampoline's uv-managed interpreter worker — which carries the same backend argv — plus .hermes-runtime children). Those have a live parent: the orphan root itself. Now holders inside an accepted orphan root's tree fold into that root (only roots are returned; taskkill /T reaps descendants), and live-parent backends defer to the ancestry check instead of refusing outright. Anything outside an orphan tree still refuses. 3. Tests for the mixed shapes: root+managed-runtime child, grandchild depth, non-descendant stray alongside an orphan root (still refuses), descendant exited mid-classify. E2E on a real Windows box: spawned a detached backend-shaped orphan that itself spawned children (3 python descendants); the scanner-shaped mixed holder set classified to [root], taskkill /T reaped root and all descendants. The live Desktop backend on the box still classified None (refusal preserved). The first E2E attempt caught exactly the trampoline/worker case the mocks missed — the live worker re-execs with the same backend argv and a live parent — which is what part 2 fixes. Co-Authored-By: 4adwentures <296413879+4adwentures@users.noreply.github.com> --- apps/desktop/electron/backend-child.ts | 32 +++++++++- apps/desktop/electron/main.ts | 32 ++-------- .../electron/windows-child-options.test.ts | 21 ++++++- hermes_cli/update_cmd.py | 62 +++++++++++++++---- .../test_update_orphan_backend_reap.py | 55 ++++++++++++++++ 5 files changed, 161 insertions(+), 41 deletions(-) diff --git a/apps/desktop/electron/backend-child.ts b/apps/desktop/electron/backend-child.ts index fc44d49b0b..874419b14d 100644 --- a/apps/desktop/electron/backend-child.ts +++ b/apps/desktop/electron/backend-child.ts @@ -24,8 +24,18 @@ export interface StopBackendChildDeps { forceKillProcessTree: (pid: number) => void } -export interface KillableChild { +export interface StopBackendTreesForUpdateDeps { + /** Synchronous Windows taskkill /T /F implementation. */ + forceKillProcessTree: (pid: number) => void + /** Clears and stops the desktop's pooled backends. */ + stopAllPoolBackends: () => void +} + +export interface BackendProcessRoot { pid?: number | null +} + +export interface KillableChild extends BackendProcessRoot { killed?: boolean kill: (signal: string) => void } @@ -53,3 +63,23 @@ export function stopBackendChild(child: KillableChild | null | undefined, deps: // Already gone. } } + +/** + * Stop every backend tree owned by a Windows Desktop update hand-off. + * + * Tree-kill the primary root while its PID is still live, then delegate pool + * teardown to the existing routine that tree-kills each pooled root exactly + * once before mutating its registry. In particular, do not signal the primary + * first: if that root exits before taskkill /T runs, Windows can no longer + * enumerate its MCP grandchildren and they survive with the venv locked. + */ +export function stopBackendTreesForUpdate( + primary: BackendProcessRoot | null | undefined, + deps: StopBackendTreesForUpdateDeps +): void { + if (primary && Number.isInteger(primary.pid)) { + deps.forceKillProcessTree(primary.pid as number) + } + + deps.stopAllPoolBackends() +} diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index 94d45ce544..aba35f09a1 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -32,7 +32,7 @@ import { import nodePty from 'node-pty' import { classifyActiveRuntime } from './active-runtime-state' -import { stopBackendChild as stopBackendChildImpl } from './backend-child' +import { stopBackendChild as stopBackendChildImpl, stopBackendTreesForUpdate } from './backend-child' import { dashboardFallbackArgs, sourceDeclaresServe } from './backend-command' import { createBackendConnectionState } from './backend-connection-state' import { buildDesktopBackendEnv, hermesManagedNodePathEntries, normalizeHermesHomeRoot } from './backend-env' @@ -2756,34 +2756,12 @@ async function releaseBackendLock(updateRoot, tag) { return { unlocked: true } } - // Collect every backend PID the desktop owns: primary window backend + pool. - const pids = [] const hermesProcess = backendConnectionState.getProcess() - if (hermesProcess && Number.isInteger(hermesProcess.pid)) { - pids.push(hermesProcess.pid) - } - - for (const entry of backendPool.values()) { - if (entry.process && Number.isInteger(entry.process.pid)) { - pids.push(entry.process.pid) - } - } - - // Graceful first (lets Python flush), then tree-kill to catch grandchildren. - if (hermesProcess && !hermesProcess.killed) { - try { - hermesProcess.kill('SIGTERM') - } catch { - void 0 - } - } - - stopAllPoolBackends() - - for (const pid of pids) { - forceKillProcessTree(pid) - } + stopBackendTreesForUpdate(hermesProcess, { + forceKillProcessTree, + stopAllPoolBackends + }) const shim = venvHermesShimPath(updateRoot) const deadlineMs = Date.now() + 15000 diff --git a/apps/desktop/electron/windows-child-options.test.ts b/apps/desktop/electron/windows-child-options.test.ts index 33fcac3577..3e6d7daccd 100644 --- a/apps/desktop/electron/windows-child-options.test.ts +++ b/apps/desktop/electron/windows-child-options.test.ts @@ -2,7 +2,7 @@ import assert from 'node:assert/strict' import { test } from 'vitest' -import { stopBackendChild } from './backend-child' +import { stopBackendChild, stopBackendTreesForUpdate } from './backend-child' import { hiddenWindowsChildOptions } from './windows-child-options' test('hiddenWindowsChildOptions adds windowsHide:true on Windows when unset', () => { @@ -130,3 +130,22 @@ test('stopBackendChild swallows errors thrown by the kill strategy', () => { }) }) }) + +test('Windows update tree-kills captured roots without pre-signalling the primary backend', () => { + const primary = makeChild({ pid: 101 }) + const pooled = makeChild({ pid: 202 }) + const events: string[] = [] + + stopBackendTreesForUpdate(primary.child, { + forceKillProcessTree: pid => events.push(`tree:${pid}`), + stopAllPoolBackends: () => { + events.push('pool-stop') + // Production stopAllPoolBackends() already tree-kills every pool root. + events.push(`tree:${pooled.child.pid}`) + } + }) + + assert.deepEqual(events, ['tree:101', 'pool-stop', 'tree:202']) + assert.deepEqual(primary.calls, [], 'the primary root must not be signalled before taskkill /T sees it') + assert.deepEqual(pooled.calls, []) +}) diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index 88fd658c7c..8ff04e150e 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -3101,17 +3101,33 @@ def _orphaned_desktop_backend_pids( - its supervising parent is demonstrably gone: the parent PID no longer exists, or the PID was reused (parent created *after* the child). - A backend whose parent is alive (the Desktop is still open) disqualifies - the whole set — the guard must keep refusing exactly as before. Returns - ``None`` in that case, or when any holder is not a backend, or when - psutil is unavailable (can't prove orphanhood → refuse). Never raises. + Tree-aware: the scanner can return an orphaned backend AND one of its + managed-runtime descendants (the ``.hermes-runtime`` interpreter child) + in the same holder set. That descendant has a live parent — the orphaned + backend itself — and isn't a ``serve`` cmdline, so per-process rules + would refuse a set that is entirely safe to reap. Holders that sit + inside an accepted orphan root's tree are therefore folded into that + root (only roots are returned; ``taskkill /T`` reaps the descendants). + + Any other live-parent backend (the Desktop is still open), non-backend + holder outside an orphan tree, or unprovable case disqualifies the whole + set — the guard must keep refusing exactly as before. Returns ``None`` + in that case, or when psutil is unavailable (can't prove orphanhood → + refuse). Never raises. """ try: import psutil # type: ignore except Exception: return None - pids: list[int] = [] + def _is_backend(argv_low: str) -> bool: + return "hermes_cli.main" in argv_low and ( + " serve" in argv_low or " dashboard" in argv_low + ) + + # Pass 1: find orphaned backend ROOTS among the holders. + roots: list[int] = [] + remaining: list[tuple[int, str]] = [] # (pid, argv_low) still to justify for pid, _name, cmdline in matches: argv = cmdline try: @@ -3123,10 +3139,9 @@ def _orphaned_desktop_backend_pids( except Exception: pass low = argv.lower() - if "hermes_cli.main" not in low or not ( - " serve" in low or " dashboard" in low - ): - return None + if not _is_backend(low): + remaining.append((int(pid), low)) + continue try: proc = psutil.Process(int(pid)) ppid = proc.ppid() @@ -3135,13 +3150,36 @@ def _orphaned_desktop_backend_pids( # PID-reuse check: a "parent" created after its child is a # recycled PID, not the real (dead) supervisor. if parent.create_time() <= proc.create_time(): - return None + # Live parent — NOT a root. But it may still be a + # descendant of an orphan root: the venv python.exe is + # a trampoline that re-execs the uv-managed interpreter + # with the SAME backend argv, so the worker half of the + # two-process chain lands here. Defer to pass 2 instead + # of refusing outright. + remaining.append((int(pid), low)) + continue except psutil.NoSuchProcess: pass # parent gone → orphan except Exception: return None - pids.append(int(pid)) - return pids + roots.append(int(pid)) + + # Pass 2: every non-backend holder must be a descendant of an accepted + # orphan root — then it dies with the root's tree reap. Anything else + # (operator REPL, stray script) keeps the refusal. + root_set = set(roots) + for pid, _low in remaining: + if not root_set: + return None + try: + ancestors = {int(a.pid) for a in psutil.Process(pid).parents()} + except psutil.NoSuchProcess: + continue # exited already + except Exception: + return None + if not (ancestors & root_set): + return None + return roots def _stop_process_trees(pids: list[int]) -> None: diff --git a/tests/hermes_cli/test_update_orphan_backend_reap.py b/tests/hermes_cli/test_update_orphan_backend_reap.py index c03e9cb362..baf2399d5c 100644 --- a/tests/hermes_cli/test_update_orphan_backend_reap.py +++ b/tests/hermes_cli/test_update_orphan_backend_reap.py @@ -49,6 +49,7 @@ def _proc( *, ppid: int = 0, create_time: float = 100.0, + parents: list[MagicMock] | None = None, ): proc = MagicMock() proc.pid = pid @@ -56,6 +57,7 @@ def _proc( proc.ppid.return_value = ppid proc.create_time.return_value = create_time proc.is_running.return_value = True + proc.parents.return_value = parents or [] return proc @@ -120,6 +122,59 @@ def test_mixed_holders_keep_refusal(): assert cli_main._orphaned_desktop_backend_pids(holders) is None +def test_orphan_root_plus_managed_runtime_descendant_qualifies(): + # helix4u's review case (#82179): the scanner returns BOTH the orphaned + # serve root and its .hermes-runtime interpreter child. The child's live + # parent IS the orphan root, so the set is safe — only the root is + # returned (taskkill /T reaps the descendant with it). + backend = _proc(200, _SERVE_ARGV, ppid=999) + child_argv = [ + "C:\\hermes\\.hermes-runtime\\python\\generation-1\\python.exe", + "worker.py", + ] + child = _proc(210, child_argv, ppid=200, parents=[backend]) + fake = _fake_psutil({200: backend, 210: child}) + with patch.dict(sys.modules, {"psutil": fake}): + holders = _holders() + [(210, "python.exe", " ".join(child_argv))] + assert cli_main._orphaned_desktop_backend_pids(holders) == [200] + + +def test_descendant_of_grandchild_depth_qualifies(): + # Descendant two hops below the orphan root (root → child → grandchild): + # psutil.parents() walks the full chain, so ancestry still matches. + backend = _proc(200, _SERVE_ARGV, ppid=999) + mid = _proc(210, ["python.exe", "mid.py"], ppid=200, parents=[backend]) + grand = _proc( + 220, ["python.exe", "leaf.py"], ppid=210, parents=[mid, backend] + ) + fake = _fake_psutil({200: backend, 210: mid, 220: grand}) + with patch.dict(sys.modules, {"psutil": fake}): + holders = _holders() + [(220, "python.exe", "python.exe leaf.py")] + assert cli_main._orphaned_desktop_backend_pids(holders) == [200] + + +def test_non_descendant_alongside_orphan_root_keeps_refusal(): + # A stray process that is NOT under the orphan root disqualifies the set + # even though an orphan root exists. + backend = _proc(200, _SERVE_ARGV, ppid=999) + unrelated_parent = _proc(50, ["explorer.exe"]) + stray = _proc( + 300, ["python.exe", "stray.py"], ppid=50, parents=[unrelated_parent] + ) + fake = _fake_psutil({50: unrelated_parent, 200: backend, 300: stray}) + with patch.dict(sys.modules, {"psutil": fake}): + holders = _holders() + [(300, "python.exe", "python.exe stray.py")] + assert cli_main._orphaned_desktop_backend_pids(holders) is None + + +def test_descendant_exited_between_scan_and_classify_is_skipped(): + backend = _proc(200, _SERVE_ARGV, ppid=999) + fake = _fake_psutil({200: backend}) # descendant 210 already gone + with patch.dict(sys.modules, {"psutil": fake}): + holders = _holders() + [(210, "python.exe", "python.exe worker.py")] + assert cli_main._orphaned_desktop_backend_pids(holders) == [200] + + def test_holder_gone_between_scan_and_classify_is_skipped(): fake = _fake_psutil({}) # PID vanished entirely with patch.dict(sys.modules, {"psutil": fake}):