fix(update): make orphan-backend reap tree-aware + drain Desktop update trees without pre-signalling
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>
This commit is contained in:
@@ -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()
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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, [])
|
||||
})
|
||||
|
||||
+50
-12
@@ -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:
|
||||
|
||||
@@ -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}):
|
||||
|
||||
Reference in New Issue
Block a user