fix(desktop): verify close/stop after the backend teardown, not before it

The Windows tree-kill check ran at the top of backendShutdown and threw
before the graceful teardown, pool stop and straggler reap. It now takes
the owned child handles up front, runs after the reap on the children that
are still running, and surfaces a failure only once cleanup is done. A lock
whose delete fails is kept and logged instead of throwing out of close, and
exitAfterBackendShutdown still exits when shutdown reports a failure.
This commit is contained in:
Hermes Agent
2026-09-24 23:42:33 -05:00
committed by brooklyn!
parent 658e6c885d
commit 4b2429624a
3 changed files with 101 additions and 42 deletions

View File

@@ -13,6 +13,7 @@ function killThatThrows(message: string) {
test('close/stop surfaces a taskkill failure and does not clear a lock a live holder still owns', () => {
const killed: number[] = []
const cleared: string[] = []
const locks: RuntimeLock[] = [
{ path: 'C:\\Users\\me\\.hermes\\gateway.lock', holderPids: [4242], held: true },
{ path: 'C:\\Users\\me\\.hermes\\profiles\\other\\gateway.lock', holderPids: [], held: false }
@@ -72,6 +73,36 @@ test('close/stop inventories owned PIDs after the tree kill and clears only unhe
assert.deepEqual(result.retainedLocks.sort(), ['foreign-held.lock', 'still-held.lock'].sort())
})
test('a lock whose delete fails is kept and reported, and the remaining locks still clear', () => {
const cleared: string[] = []
const result = finishWindowsCloseStop(
[],
[
{ path: 'open.lock', holderPids: [] },
{ path: 'stale.lock', holderPids: [] }
],
{
killTree: () => {},
isPidAlive: () => false,
clearLock: path => {
if (path === 'open.lock') {
throw new Error('EBUSY: resource busy or locked')
}
cleared.push(path)
}
}
)
assert.deepEqual(cleared, ['stale.lock'])
assert.deepEqual(result.clearedLocks, ['stale.lock'])
assert.deepEqual(result.retainedLocks, ['open.lock'])
assert.equal(result.lockErrors.length, 1)
assert.match(result.lockErrors[0].error, /EBUSY/)
assert.equal(result.liveFailure, false)
})
test('a taskkill error is not discarded when the owned PID is already gone', () => {
const result = finishWindowsCloseStop([4242], [{ path: 'stale.lock', holderPids: [4242] }], {
killTree: killThatThrows('The process "4242" not found'),

View File

@@ -36,7 +36,9 @@ export interface CloseStopKillResult {
/** Owned PIDs still enumerable after the tree kill. */
remainingPids: number[]
clearedLocks: string[]
/** Held locks, plus unheld ones whose removal failed (see lockErrors). */
retainedLocks: string[]
lockErrors: { path: string; error: string }[]
/**
* True when a taskkill failure left an owned PID alive, or any owned PID
* is still enumerable. An already-gone taskkill error is recorded but is
@@ -81,15 +83,24 @@ export function finishWindowsCloseStop(
const alive = new Set(remainingPids)
const clearedLocks: string[] = []
const retainedLocks: string[] = []
const lockErrors: { path: string; error: string }[] = []
for (const lock of locks) {
if (lockIsHeld(lock, pid => alive.has(pid) || deps.isPidAlive(pid))) {
retainedLocks.push(lock.path)
continue
}
deps.clearLock(lock.path)
clearedLocks.push(lock.path)
// An open handle (msvcrt byte lock) makes the delete fail: keep the lock
// and report it rather than aborting the rest of close/stop.
try {
deps.clearLock(lock.path)
clearedLocks.push(lock.path)
} catch (error) {
retainedLocks.push(lock.path)
lockErrors.push({ path: lock.path, error: errorText(error) })
}
}
const failedWhileAlive = new Set(taskkillFailures.map(failure => failure.pid))
@@ -100,6 +111,7 @@ export function finishWindowsCloseStop(
remainingPids,
clearedLocks,
retainedLocks,
lockErrors,
liveFailure
}
}

View File

@@ -3685,51 +3685,56 @@ function collectCloseStopLocks(): RuntimeLock[] {
return locks
}
function collectOwnedBackendPids(): number[] {
const pids: number[] = []
const primary = backendConnectionState.getProcess()
// Captured before teardown drops the handles. Node keeps each process handle
// open until exit is observed, so a PID read from a still-running child here
// cannot have been reused by an unrelated process.
function collectOwnedBackendChildren(): ChildProcess[] {
const children = [backendConnectionState.getProcess(), ...[...backendPool.values()].map(entry => entry?.process)]
if (primary && Number.isInteger(primary.pid) && primary.pid > 0) {
pids.push(primary.pid)
}
for (const entry of backendPool.values()) {
const pid = entry?.process?.pid
if (Number.isInteger(pid) && pid > 0) {
pids.push(pid)
}
}
return pids
return children.filter(
(child): child is ChildProcess => Boolean(child) && Number.isInteger(child.pid) && child.pid > 0
)
}
// Close/stop: same tree-kill as forceKillProcessTree, then inventory the
// owned PIDs and clear only locks no live holder still owns. A taskkill
// failure is logged and, when an owned PID is still alive, thrown.
function windowsCloseStopOwnedBackends() {
// Close/stop, after the graceful teardown, pool stop and straggler reap: the
// same tree-kill for any owned child that is still running, an inventory of
// those PIDs, and a clear of only the locks no live holder owns. Never throws;
// returns the failure for the caller to surface once cleanup is done.
function windowsCloseStopOwnedBackends(children: ChildProcess[]): Error | null {
if (!IS_WINDOWS) {
return
return null
}
const result = finishWindowsCloseStop(collectOwnedBackendPids(), collectCloseStopLocks(), {
killTree: forceKillProcessTree,
isPidAlive: isPidAliveWindows,
clearLock: lockPath => {
fs.rmSync(lockPath, { force: true })
try {
const running = children.filter(child => child.exitCode === null && child.signalCode === null)
const result = finishWindowsCloseStop(
running.map(child => child.pid as number),
collectCloseStopLocks(),
{
killTree: forceKillProcessTree,
isPidAlive: isPidAliveWindows,
clearLock: lockPath => {
fs.rmSync(lockPath, { force: true })
}
}
)
for (const failure of result.taskkillFailures) {
rememberLog(`[close-stop] taskkill PID ${failure.pid} failed: ${failure.error}`)
}
})
for (const failure of result.taskkillFailures) {
rememberLog(`[close-stop] taskkill PID ${failure.pid} failed: ${failure.error}`)
}
for (const failure of result.lockErrors) {
rememberLog(`[close-stop] could not clear unheld lock ${failure.path}: ${failure.error}`)
}
if (result.clearedLocks.length) {
rememberLog(`[close-stop] cleared unheld lock(s): ${result.clearedLocks.join(', ')}`)
}
if (result.clearedLocks.length) {
rememberLog(`[close-stop] cleared unheld lock(s): ${result.clearedLocks.join(', ')}`)
}
if (result.liveFailure) {
throw new Error(closeStopFailureMessage(result))
return result.liveFailure ? new Error(closeStopFailureMessage(result)) : null
} catch (error) {
return error instanceof Error ? error : new Error(String(error))
}
}
@@ -12092,10 +12097,7 @@ function reapInstallRootedStragglers(excludePids: number[]): void {
}
const backendShutdown = createBackendShutdownCoordinator(async (): Promise<void> => {
// Before teardown drops the process handles: inventory those owned PIDs
// after the same tree-kill, and refuse to pretend close succeeded.
windowsCloseStopOwnedBackends()
const ownedChildren = IS_WINDOWS ? collectOwnedBackendChildren() : []
const localShutdown = localBackendLifecycle.shutdown()
const primary = backendConnectionState.getProcess()
const primaryStop = teardownPrimaryBackendAndWait()
@@ -12109,6 +12111,15 @@ const backendShutdown = createBackendShutdownCoordinator(async (): Promise<void>
await waitForTeardown([localShutdown, primaryStop, pooledStops], 7_000)
reapInstallRootedStragglers(Number.isInteger(primary?.pid) ? [primary.pid] : [])
// Verify last, so a surviving child cannot skip the teardown above.
const closeStopFailure = windowsCloseStopOwnedBackends(ownedChildren)
if (closeStopFailure) {
rememberLog(`[close-stop] ${closeStopFailure.message}`)
throw closeStopFailure
}
})
const quitTeardown = createQuitTeardownCoordinator(() => app.quit())
@@ -12134,7 +12145,12 @@ async function teardownSshForQuit(): Promise<void> {
}
async function exitAfterBackendShutdown(code) {
await backendShutdown.run()
try {
await backendShutdown.run()
} catch {
// Already logged by backendShutdown; the exit must still happen.
}
app.exit(code)
}