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:
@@ -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', () => {
|
test('close/stop surfaces a taskkill failure and does not clear a lock a live holder still owns', () => {
|
||||||
const killed: number[] = []
|
const killed: number[] = []
|
||||||
const cleared: string[] = []
|
const cleared: string[] = []
|
||||||
|
|
||||||
const locks: RuntimeLock[] = [
|
const locks: RuntimeLock[] = [
|
||||||
{ path: 'C:\\Users\\me\\.hermes\\gateway.lock', holderPids: [4242], held: true },
|
{ path: 'C:\\Users\\me\\.hermes\\gateway.lock', holderPids: [4242], held: true },
|
||||||
{ path: 'C:\\Users\\me\\.hermes\\profiles\\other\\gateway.lock', holderPids: [], held: false }
|
{ 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())
|
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', () => {
|
test('a taskkill error is not discarded when the owned PID is already gone', () => {
|
||||||
const result = finishWindowsCloseStop([4242], [{ path: 'stale.lock', holderPids: [4242] }], {
|
const result = finishWindowsCloseStop([4242], [{ path: 'stale.lock', holderPids: [4242] }], {
|
||||||
killTree: killThatThrows('The process "4242" not found'),
|
killTree: killThatThrows('The process "4242" not found'),
|
||||||
|
|||||||
@@ -36,7 +36,9 @@ export interface CloseStopKillResult {
|
|||||||
/** Owned PIDs still enumerable after the tree kill. */
|
/** Owned PIDs still enumerable after the tree kill. */
|
||||||
remainingPids: number[]
|
remainingPids: number[]
|
||||||
clearedLocks: string[]
|
clearedLocks: string[]
|
||||||
|
/** Held locks, plus unheld ones whose removal failed (see lockErrors). */
|
||||||
retainedLocks: string[]
|
retainedLocks: string[]
|
||||||
|
lockErrors: { path: string; error: string }[]
|
||||||
/**
|
/**
|
||||||
* True when a taskkill failure left an owned PID alive, or any owned PID
|
* 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
|
* 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 alive = new Set(remainingPids)
|
||||||
const clearedLocks: string[] = []
|
const clearedLocks: string[] = []
|
||||||
const retainedLocks: string[] = []
|
const retainedLocks: string[] = []
|
||||||
|
const lockErrors: { path: string; error: string }[] = []
|
||||||
|
|
||||||
for (const lock of locks) {
|
for (const lock of locks) {
|
||||||
if (lockIsHeld(lock, pid => alive.has(pid) || deps.isPidAlive(pid))) {
|
if (lockIsHeld(lock, pid => alive.has(pid) || deps.isPidAlive(pid))) {
|
||||||
retainedLocks.push(lock.path)
|
retainedLocks.push(lock.path)
|
||||||
|
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
|
||||||
deps.clearLock(lock.path)
|
// An open handle (msvcrt byte lock) makes the delete fail: keep the lock
|
||||||
clearedLocks.push(lock.path)
|
// 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))
|
const failedWhileAlive = new Set(taskkillFailures.map(failure => failure.pid))
|
||||||
@@ -100,6 +111,7 @@ export function finishWindowsCloseStop(
|
|||||||
remainingPids,
|
remainingPids,
|
||||||
clearedLocks,
|
clearedLocks,
|
||||||
retainedLocks,
|
retainedLocks,
|
||||||
|
lockErrors,
|
||||||
liveFailure
|
liveFailure
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -3685,51 +3685,56 @@ function collectCloseStopLocks(): RuntimeLock[] {
|
|||||||
return locks
|
return locks
|
||||||
}
|
}
|
||||||
|
|
||||||
function collectOwnedBackendPids(): number[] {
|
// Captured before teardown drops the handles. Node keeps each process handle
|
||||||
const pids: number[] = []
|
// open until exit is observed, so a PID read from a still-running child here
|
||||||
const primary = backendConnectionState.getProcess()
|
// 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) {
|
return children.filter(
|
||||||
pids.push(primary.pid)
|
(child): child is ChildProcess => Boolean(child) && Number.isInteger(child.pid) && child.pid > 0
|
||||||
}
|
)
|
||||||
|
|
||||||
for (const entry of backendPool.values()) {
|
|
||||||
const pid = entry?.process?.pid
|
|
||||||
|
|
||||||
if (Number.isInteger(pid) && pid > 0) {
|
|
||||||
pids.push(pid)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
return pids
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// Close/stop: same tree-kill as forceKillProcessTree, then inventory the
|
// Close/stop, after the graceful teardown, pool stop and straggler reap: the
|
||||||
// owned PIDs and clear only locks no live holder still owns. A taskkill
|
// same tree-kill for any owned child that is still running, an inventory of
|
||||||
// failure is logged and, when an owned PID is still alive, thrown.
|
// those PIDs, and a clear of only the locks no live holder owns. Never throws;
|
||||||
function windowsCloseStopOwnedBackends() {
|
// returns the failure for the caller to surface once cleanup is done.
|
||||||
|
function windowsCloseStopOwnedBackends(children: ChildProcess[]): Error | null {
|
||||||
if (!IS_WINDOWS) {
|
if (!IS_WINDOWS) {
|
||||||
return
|
return null
|
||||||
}
|
}
|
||||||
|
|
||||||
const result = finishWindowsCloseStop(collectOwnedBackendPids(), collectCloseStopLocks(), {
|
try {
|
||||||
killTree: forceKillProcessTree,
|
const running = children.filter(child => child.exitCode === null && child.signalCode === null)
|
||||||
isPidAlive: isPidAliveWindows,
|
|
||||||
clearLock: lockPath => {
|
const result = finishWindowsCloseStop(
|
||||||
fs.rmSync(lockPath, { force: true })
|
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) {
|
for (const failure of result.lockErrors) {
|
||||||
rememberLog(`[close-stop] taskkill PID ${failure.pid} failed: ${failure.error}`)
|
rememberLog(`[close-stop] could not clear unheld lock ${failure.path}: ${failure.error}`)
|
||||||
}
|
}
|
||||||
|
|
||||||
if (result.clearedLocks.length) {
|
if (result.clearedLocks.length) {
|
||||||
rememberLog(`[close-stop] cleared unheld lock(s): ${result.clearedLocks.join(', ')}`)
|
rememberLog(`[close-stop] cleared unheld lock(s): ${result.clearedLocks.join(', ')}`)
|
||||||
}
|
}
|
||||||
|
|
||||||
if (result.liveFailure) {
|
return result.liveFailure ? new Error(closeStopFailureMessage(result)) : null
|
||||||
throw new Error(closeStopFailureMessage(result))
|
} 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> => {
|
const backendShutdown = createBackendShutdownCoordinator(async (): Promise<void> => {
|
||||||
// Before teardown drops the process handles: inventory those owned PIDs
|
const ownedChildren = IS_WINDOWS ? collectOwnedBackendChildren() : []
|
||||||
// after the same tree-kill, and refuse to pretend close succeeded.
|
|
||||||
windowsCloseStopOwnedBackends()
|
|
||||||
|
|
||||||
const localShutdown = localBackendLifecycle.shutdown()
|
const localShutdown = localBackendLifecycle.shutdown()
|
||||||
const primary = backendConnectionState.getProcess()
|
const primary = backendConnectionState.getProcess()
|
||||||
const primaryStop = teardownPrimaryBackendAndWait()
|
const primaryStop = teardownPrimaryBackendAndWait()
|
||||||
@@ -12109,6 +12111,15 @@ const backendShutdown = createBackendShutdownCoordinator(async (): Promise<void>
|
|||||||
await waitForTeardown([localShutdown, primaryStop, pooledStops], 7_000)
|
await waitForTeardown([localShutdown, primaryStop, pooledStops], 7_000)
|
||||||
|
|
||||||
reapInstallRootedStragglers(Number.isInteger(primary?.pid) ? [primary.pid] : [])
|
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())
|
const quitTeardown = createQuitTeardownCoordinator(() => app.quit())
|
||||||
@@ -12134,7 +12145,12 @@ async function teardownSshForQuit(): Promise<void> {
|
|||||||
}
|
}
|
||||||
|
|
||||||
async function exitAfterBackendShutdown(code) {
|
async function exitAfterBackendShutdown(code) {
|
||||||
await backendShutdown.run()
|
try {
|
||||||
|
await backendShutdown.run()
|
||||||
|
} catch {
|
||||||
|
// Already logged by backendShutdown; the exit must still happen.
|
||||||
|
}
|
||||||
|
|
||||||
app.exit(code)
|
app.exit(code)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user