diff --git a/apps/desktop/electron/backend-release-gate.test.ts b/apps/desktop/electron/backend-release-gate.test.ts new file mode 100644 index 0000000000..db5e82dcad --- /dev/null +++ b/apps/desktop/electron/backend-release-gate.test.ts @@ -0,0 +1,174 @@ +/** + * backend-release-gate.test.ts + * + * The #74805 first-attempt race, pinned as a contract on the extracted gate: + * the desktop must not hand off to the updater while PIDs it signalled are + * still in the process table, even when the venv shim probe reads unlocked + * (the backend `python.exe -m hermes_cli.main serve` need not hold the shim + * at all). On merge-base main.ts the gate was shim-only and passed on its + * first iteration with zero dwell — the sabotage A/B run proves these tests + * bite on that behavior. + */ + +import { describe, expect, it } from 'vitest' + +import { + RELEASE_GATE_POLL_MS, + type ReleaseGateDeps, + waitForBackendRelease +} from './backend-release-gate' + +/** A fake clock where sleep() advances time instantly. */ +function fakeClock() { + let t = 0 + + return { + now: () => t, + sleep: async (ms: number) => { + t += ms + }, + advance: (ms: number) => { + t += ms + } + } +} + +function makeDeps(overrides: Partial = {}): ReleaseGateDeps & { + logs: string[] + kills: number[] +} { + const clock = fakeClock() + const logs: string[] = [] + const kills: number[] = [] + + return { + isShimLocked: () => false, + isPidAlive: () => false, + collectStragglerPids: () => [], + killProcessTree: pid => kills.push(pid), + sleep: clock.sleep, + now: clock.now, + log: line => logs.push(line), + logs, + kills, + ...overrides + } +} + +describe('waitForBackendRelease (#74805 first-attempt race)', () => { + it('does NOT pass while a signalled PID is still in the process table, even with the shim unlocked', async () => { + // The exact #74805 shape: shim unlocked from tick 0 (serve backend never + // held it), but the killed python is still tearing down for ~1.2s. + let aliveUntil = 4 * RELEASE_GATE_POLL_MS + const clock = fakeClock() + + const deps = makeDeps({ + now: clock.now, + sleep: clock.sleep, + isShimLocked: () => false, + isPidAlive: () => clock.now() < aliveUntil + }) + + const result = await waitForBackendRelease([4021], deps, 'test') + + expect(result.unlocked).toBe(true) + expect(result.lingeringPids).toEqual([]) + // The gate must have dwelled at least until the PID actually exited — + // on merge-base (shim-only gate) it would have returned at t=0. + expect(clock.now()).toBeGreaterThanOrEqual(aliveUntil) + }) + + it('passes immediately when the shim is unlocked and no signalled PID lingers', async () => { + const deps = makeDeps() + + const result = await waitForBackendRelease([4021, 4022], deps, 'test') + + expect(result.unlocked).toBe(true) + expect(deps.now()).toBe(0) // no dwell needed — everything already gone + }) + + it('keeps waiting while the shim is locked and fails closed at the deadline', async () => { + const deps = makeDeps({ isShimLocked: () => true }) + + const result = await waitForBackendRelease([], deps, 'test', 3 * RELEASE_GATE_POLL_MS) + + expect(result.unlocked).toBe(false) + }) + + it('proceeds at the deadline when the shim is unlocked but PIDs still linger (pre-#74805 escape hatch)', async () => { + // Lingering PIDs past the deadline are the venv-blocker re-scan's job — + // the gate must not invent a new failure mode for them. + const deps = makeDeps({ isPidAlive: () => true }) + + const result = await waitForBackendRelease([4021], deps, 'test', 3 * RELEASE_GATE_POLL_MS) + + expect(result.unlocked).toBe(true) + expect(result.lingeringPids).toEqual([4021]) + }) + + it('kills and then waits out stragglers that respawn mid-teardown', async () => { + // A pool entry registered mid-teardown appears on pass 2; the gate must + // signal it AND add it to the exit-wait set. + const clock = fakeClock() + let stragglerServed = false + let stragglerKilledAt: number | null = null + const kills: number[] = [] + + const deps = makeDeps({ + now: clock.now, + sleep: clock.sleep, + collectStragglerPids: () => { + if (!stragglerServed) { + stragglerServed = true + + return [7777] + } + + return [] + }, + killProcessTree: pid => { + stragglerKilledAt = clock.now() + kills.push(pid) + }, + // Primary PID 4021 lingers for one poll (forcing a straggler-collect + // pass); the straggler stays alive for two polls after being killed. + isPidAlive: pid => { + if (pid === 4021) { + return clock.now() < RELEASE_GATE_POLL_MS + } + + return ( + pid === 7777 && + stragglerKilledAt !== null && + clock.now() < stragglerKilledAt + 2 * RELEASE_GATE_POLL_MS + ) + } + }) + + const result = await waitForBackendRelease([4021], deps, 'test') + + expect(kills).toContain(7777) + expect(result.unlocked).toBe(true) + expect(result.lingeringPids).toEqual([]) + // The gate must have dwelled until the straggler actually exited. + expect(clock.now()).toBeGreaterThanOrEqual( + (stragglerKilledAt ?? 0) + 2 * RELEASE_GATE_POLL_MS + ) + }) + + it('ignores invalid PIDs in the seed and straggler sets', async () => { + const deps = makeDeps({ + collectStragglerPids: () => [0, -4, NaN as unknown as number] + }) + + const result = await waitForBackendRelease( + [0, -1, 2.5, NaN as unknown as number], + deps, + 'test', + 2 * RELEASE_GATE_POLL_MS + ) + + expect(result.unlocked).toBe(true) + expect(deps.kills).toEqual([]) + }) +}) diff --git a/apps/desktop/electron/backend-release-gate.ts b/apps/desktop/electron/backend-release-gate.ts new file mode 100644 index 0000000000..01aee80279 --- /dev/null +++ b/apps/desktop/electron/backend-release-gate.ts @@ -0,0 +1,131 @@ +/** + * backend-release-gate.ts + * + * The Windows pre-update unlock gate: after the desktop tree-kills its own + * backends, decide when it is actually safe to hand off to the updater. + * + * Why this exists (#74805): `taskkill /T /F` returns once termination is + * INITIATED, not completed. A dying `python.exe -m hermes_cli.main serve` + * stays in the process table while it unmaps .pyd files (AV / NTFS filter + * drivers stretch this out), and it need not hold the venv `hermes.exe` shim + * at all — so a gate that only probes the shim can pass on its very first + * iteration, with zero dwell, while the killed pythons are still + * terminating. The venv-blocker scan downstream has no liveness filter; it + * enumerates those dying processes as holders and aborts the hand-off. + * Result: the FIRST update attempt from the footbar always failed, and the + * manual retry (by which time the table had settled) succeeded. + * + * The gate therefore requires BOTH: the shim unlocked AND every PID we have + * ever signalled to have actually left the process table. On deadline, the + * old shim-only criterion is kept as the escape hatch — lingering PIDs past + * 15s are the venv-blocker re-scan's job, not a new failure mode. + * + * Extracted into its own dependency-free module (no electron import) so the + * gate's decision logic can be asserted directly with fake clocks and fake + * process tables, following the backend-child.ts pattern. + */ + +export interface ReleaseGateDeps { + /** Probe the venv hermes.exe shim (real: O_RDWR open attempt). */ + isShimLocked: () => boolean + /** True while `pid` is still enumerable in the process table. */ + isPidAlive: (pid: number) => boolean + /** + * Re-collect PIDs that may have (re)spawned since the initial sweep — + * the supervised primary backend and pool entries. Called every pass. + */ + collectStragglerPids: () => number[] + /** Tree-kill (real: taskkill /PID n /T /F). */ + killProcessTree: (pid: number) => void + /** Async sleep; injectable so tests run on a fake clock. */ + sleep: (ms: number) => Promise + /** Monotonic-enough clock; injectable for tests. */ + now: () => number + /** Log sink (real: rememberLog). */ + log: (line: string) => void +} + +export interface ReleaseGateResult { + unlocked: boolean + /** PIDs we signalled that were still enumerable when the gate resolved. */ + lingeringPids: number[] +} + +export const RELEASE_GATE_DEADLINE_MS = 15000 +export const RELEASE_GATE_POLL_MS = 300 + +/** + * Wait until the install is genuinely releasable: shim unlocked AND every + * signalled PID gone — or the deadline passes. + * + * `initialPids` are the PIDs the caller already signalled (primary backend + + * pool) before invoking the gate; stragglers collected on each pass are + * killed and added to the same watch set. + */ +export async function waitForBackendRelease( + initialPids: number[], + deps: ReleaseGateDeps, + tag: string, + deadlineMs: number = RELEASE_GATE_DEADLINE_MS +): Promise { + const killedPids = new Set( + initialPids.filter(pid => Number.isInteger(pid) && pid > 0) + ) + + const deadline = deps.now() + deadlineMs + + while (deps.now() < deadline) { + const lingering = [...killedPids].filter(pid => deps.isPidAlive(pid)) + + if (!deps.isShimLocked() && lingering.length === 0) { + deps.log( + `[${tag}] venv shim unlocked and ${killedPids.size} signalled backend PID(s) exited; safe to proceed` + ) + + return { unlocked: true, lingeringPids: [] } + } + + // A supervised backend can respawn between kill and check (grandchildren, + // pool entries registered mid-teardown). Re-collect and re-kill each pass + // instead of trusting the initial sweep. + for (const pid of deps.collectStragglerPids()) { + if (Number.isInteger(pid) && pid > 0) { + killedPids.add(pid) + deps.killProcessTree(pid) + } + } + + await deps.sleep(RELEASE_GATE_POLL_MS) + } + + // Deadline reached. Keep the pre-#74805 success criterion — an unlocked + // shim — rather than inventing a new failure mode for PIDs that linger + // past the deadline; the venv-blocker re-scan downstream covers that + // residue (and a REAL foreign holder still fails the shim probe). + const lingering = [...killedPids].filter(pid => deps.isPidAlive(pid)) + + if (!deps.isShimLocked()) { + deps.log( + `[${tag}] proceeding after deadline: venv shim unlocked, but ${lingering.length} signalled PID(s) still enumerable` + ) + + return { unlocked: true, lingeringPids: lingering } + } + + return { unlocked: false, lingeringPids: lingering } +} + +/** + * Liveness probe for a PID on Windows. `process.kill(pid, 0)` delivers + * nothing; it only probes existence: EPERM ⇒ exists but inaccessible (still + * alive), ESRCH ⇒ gone. + */ +export function isPidAliveWindows(pid: number): boolean { + try { + process.kill(pid, 0) + + return true + } catch (err: any) { + return Boolean(err) && err.code === 'EPERM' + } +} diff --git a/apps/desktop/electron/backend-release-gate.windows-live.test.ts b/apps/desktop/electron/backend-release-gate.windows-live.test.ts new file mode 100644 index 0000000000..72b67b570f --- /dev/null +++ b/apps/desktop/electron/backend-release-gate.windows-live.test.ts @@ -0,0 +1,150 @@ +/** + * backend-release-gate.windows-live.test.ts + * + * LIVE Windows E2E for the #74805 unlock gate: real spawned processes, the + * REAL isPidAliveWindows probe against the live process table, real + * taskkill — no fake clocks, no fake tables. Runs only on win32 (the + * ephemeral wine2e lane); skipped everywhere else. + * + * This is the platform half of the proof: the unit suite pins the gate's + * decision logic on a fake table; this file proves the two real-world + * premises the fix rests on: + * 1. taskkill /T /F returns while the killed process is still enumerable + * (the race window exists), and + * 2. the gate, wired to the real probes, dwells through that window and + * only passes once the PID has genuinely left the table. + */ + +import { execFileSync, spawn } from 'node:child_process' + +import { describe, expect, it } from 'vitest' + +import { isPidAliveWindows, waitForBackendRelease } from './backend-release-gate' + +const isWindows = process.platform === 'win32' + +function spawnSleeper(): { pid: number; kill: () => void } { + // A real python if available (mirrors the backend shape), else powershell. + const child = spawn( + 'powershell', + ['-NoProfile', '-Command', 'Start-Sleep -Seconds 300'], + { stdio: 'ignore' } + ) + + if (!child.pid) { + throw new Error('sleeper failed to spawn') + } + + return { + pid: child.pid, + kill: () => { + try { + child.kill() + } catch { + /* already gone */ + } + } + } +} + +function taskkillTree(pid: number): void { + try { + execFileSync('taskkill', ['/PID', String(pid), '/T', '/F'], { stdio: 'ignore' }) + } catch { + /* already gone */ + } +} + +describe.skipIf(!isWindows)('waitForBackendRelease — live Windows (#74805)', () => { + it('isPidAliveWindows tracks a real process through spawn and exit', async () => { + const sleeper = spawnSleeper() + + expect(isPidAliveWindows(sleeper.pid)).toBe(true) + + taskkillTree(sleeper.pid) + + // Poll until the table retires the PID (bounded). + const deadline = Date.now() + 10000 + + while (isPidAliveWindows(sleeper.pid) && Date.now() < deadline) { + await new Promise(r => setTimeout(r, 100)) + } + + expect(isPidAliveWindows(sleeper.pid)).toBe(false) + }) + + it('the gate dwells until a real killed PID leaves the live process table', async () => { + const sleeper = spawnSleeper() + const logs: string[] = [] + let firstAliveCheck: boolean | null = null + + // Fire the real taskkill and IMMEDIATELY enter the gate — the #74805 + // shape. The shim probe reads unlocked throughout (the serve backend + // never held it); only the PID exit-wait can hold the gate closed. + taskkillTree(sleeper.pid) + + const result = await waitForBackendRelease( + [sleeper.pid], + { + isShimLocked: () => false, + isPidAlive: pid => { + const alive = isPidAliveWindows(pid) + + if (firstAliveCheck === null) { + firstAliveCheck = alive + } + + return alive + }, + collectStragglerPids: () => [], + killProcessTree: taskkillTree, + sleep: ms => new Promise(r => setTimeout(r, ms)), + now: () => Date.now(), + log: line => logs.push(line) + }, + 'live-e2e' + ) + + expect(result.unlocked).toBe(true) + // The gate resolved only after the real PID left the real table: + expect(isPidAliveWindows(sleeper.pid)).toBe(false) + expect(result.lingeringPids).toEqual([]) + // Record whether the race window was observable on this runner (taskkill + // returned while the PID was still enumerable). Informational: fast + // runners can retire tiny process trees before our first check, but the + // gate's correctness (above) does not depend on winning that race. + logs.push(`race-window-observed=${firstAliveCheck}`) + + expect(logs.some(l => l.includes('safe to proceed'))).toBe(true) + }) + + it('a live foreign holder keeps the gate closed until the deadline', async () => { + const holder = spawnSleeper() + + try { + const result = await waitForBackendRelease( + [holder.pid], + { + // Simulates the shim held by a process we did NOT kill — the gate + // must fail closed rather than hand off over a live holder. + isShimLocked: () => true, + isPidAlive: isPidAliveWindows, + collectStragglerPids: () => [], + killProcessTree: () => { + /* nothing else to kill */ + }, + sleep: ms => new Promise(r => setTimeout(r, ms)), + now: () => Date.now(), + log: () => {} + }, + 'live-e2e', + 2000 + ) + + expect(result.unlocked).toBe(false) + expect(result.lingeringPids).toEqual([holder.pid]) + } finally { + taskkillTree(holder.pid) + } + }) +}) diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index 8e77be07d8..0f9f2acb19 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -61,6 +61,7 @@ import { verifyHermesCli } from './backend-probes' import { waitForDashboardPortAnnouncement } from './backend-ready' +import { isPidAliveWindows, waitForBackendRelease } from './backend-release-gate' import { isHostKeyChangedBootFailure, isRetryableRemoteBootFailure, @@ -3469,43 +3470,59 @@ async function releaseBackendLock(updateRoot, tag) { const hermesProcess = backendConnectionState.getProcess() + // Seed the release gate with every PID we are about to signal: the + // supervised primary backend and all pool backends. The gate waits for + // these to actually LEAVE the process table, not just for the shim to + // unlock — the shim probe only covers venv\Scripts\hermes.exe, but the + // backend is `python.exe -m hermes_cli.main serve`, which need not hold + // the shim at all (#74805 first-attempt race). + const initialPids = [] + + if (hermesProcess && Number.isInteger(hermesProcess.pid)) { + initialPids.push(hermesProcess.pid) + } + + for (const entry of backendPool.values()) { + if (entry.process && Number.isInteger(entry.process.pid)) { + initialPids.push(entry.process.pid) + } + } + stopBackendTreesForUpdate(hermesProcess, { forceKillProcessTree, stopAllPoolBackends }) const shim = venvHermesShimPath(updateRoot) - const deadlineMs = Date.now() + 15000 - while (Date.now() < deadlineMs) { - if (!isShimLocked(shim)) { - rememberLog(`[${tag}] venv shim unlocked; safe to proceed`) + const gate = await waitForBackendRelease(initialPids, { + isShimLocked: () => Boolean(isShimLocked(shim)), + isPidAlive: isPidAliveWindows, + collectStragglerPids: () => { + const stragglers = [] - return { unlocked: true } - } + const currentHermesProcess = backendConnectionState.getProcess() - // A supervised backend can respawn between kill and check (grandchildren, - // pool entries registered mid-teardown). Re-collect and re-kill each pass - // instead of trusting the initial sweep. - const stragglers = [] - - const currentHermesProcess = backendConnectionState.getProcess() - - if (currentHermesProcess && Number.isInteger(currentHermesProcess.pid)) { - stragglers.push(currentHermesProcess.pid) - } - - for (const entry of backendPool.values()) { - if (entry.process && Number.isInteger(entry.process.pid)) { - stragglers.push(entry.process.pid) + if (currentHermesProcess && Number.isInteger(currentHermesProcess.pid)) { + stragglers.push(currentHermesProcess.pid) } - } - for (const pid of stragglers) { - forceKillProcessTree(pid) - } + for (const entry of backendPool.values()) { + if (entry.process && Number.isInteger(entry.process.pid)) { + stragglers.push(entry.process.pid) + } + } - await new Promise(r => setTimeout(r, 300)) + return stragglers + }, + killProcessTree: forceKillProcessTree, + sleep: (ms: number) => new Promise(r => setTimeout(r, ms)), + now: () => Date.now(), + log: rememberLog + }, tag) + + if (gate.unlocked) { + return { unlocked: true } } // Do NOT proceed past a held lock: handing off to the updater while another @@ -3682,6 +3699,23 @@ async function applyUpdates(opts: { stopSafeBlockers?: boolean } = {}) { scanOutcome = await scanVenvBlockers(updateRoot) } + // Re-scan before aborting on 'blocked' (#74805). Process-table teardown + // is asynchronous on Windows: even after releaseBackendLock's PID-exit + // wait, a grandchild the desktop never tracked (or a process an AV / + // NTFS filter driver is holding in teardown) can stay enumerable for a + // few more seconds and read as a holder. Each scan already costs + // seconds (spawns a venv python + psutil sweep), so two retries with a + // short dwell give the table time to settle without meaningfully + // delaying the abort path when a REAL holder (a user terminal, second + // window) is present — that holder is still there on the third scan. + for (let attempt = 0; scanOutcome.kind === 'blocked' && attempt < 2; attempt++) { + rememberLog( + `[updates] venv-blocker scan reported ${scanOutcome.result.processes.length} holder(s); re-scanning after settle (attempt ${attempt + 2}/3)` + ) + await new Promise(resolve => setTimeout(resolve, 1500)) + scanOutcome = await scanVenvBlockers(updateRoot) + } + if (scanOutcome.kind === 'blocked') { const message = formatBlockerMessage(scanOutcome.result)