diff --git a/src/main/daemon/daemon-health.test.ts b/src/main/daemon/daemon-health.test.ts index 093081ebe5c..b6833d8bcc7 100644 --- a/src/main/daemon/daemon-health.test.ts +++ b/src/main/daemon/daemon-health.test.ts @@ -22,6 +22,16 @@ import { startTimesWithinTolerance } from './daemon-health' import type { SubprocessHandle } from './session' +import type { DaemonPtyOwnership } from './daemon-live-pty-evidence' + +// Why: the veto's production default is otherwise never exercised — every other test injects it. +const { inspectDaemonPtyOwnershipMock } = vi.hoisted(() => ({ + inspectDaemonPtyOwnershipMock: vi.fn(async (_pid: number) => 'unknown' as DaemonPtyOwnership) +})) + +vi.mock('./daemon-live-pty-evidence', () => ({ + inspectDaemonPtyOwnership: inspectDaemonPtyOwnershipMock +})) function createMockSubprocess(): SubprocessHandle { return { @@ -697,6 +707,26 @@ describe.skipIf(process.platform === 'win32')('killStaleDaemon live PTY ownershi } }) + it('routes the opt-in through the real ownership inspector when no hook is injected', async () => { + // Why: production passes no hook, so a default that never sees live PTYs would ship a + // veto that silently never fires — indistinguishable from having no veto at all. + inspectDaemonPtyOwnershipMock.mockResolvedValueOnce('owns-live-ptys') + const killSpy = vi.spyOn(process, 'kill') + + try { + await expect( + killStaleDaemon(dir, socketPath, tokenPath, undefined, { + probeEndpoint: async () => 'missing', + preserveWhenOwningLivePtys: true + }) + ).resolves.toEqual({ killed: false, liveOwnerSurvived: true }) + expect(inspectDaemonPtyOwnershipMock).toHaveBeenCalledWith(stubPid) + expect(terminationSignals(killSpy)).toEqual([]) + } finally { + killSpy.mockRestore() + } + }) + it('never consults PTY ownership without the opt-in, so restarts stay killable', async () => { const inspectPtyOwnership = vi.fn(async () => 'owns-live-ptys' as const) const killSpy = vi.spyOn(process, 'kill') diff --git a/src/main/daemon/daemon-init.test.ts b/src/main/daemon/daemon-init.test.ts index b8d5c8ddb35..67ef30db7c1 100644 --- a/src/main/daemon/daemon-init.test.ts +++ b/src/main/daemon/daemon-init.test.ts @@ -1196,6 +1196,50 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { } }) + it('falls back to killStaleDaemon without the live-PTY veto, so an explicit restart always wins', async () => { + // Why: this is the documented escape hatch (Settings → Manage Sessions → Restart). Opting + // into the veto here would report liveOwnerSurvived and throw, leaving the user no daemon. + probeSocketExistsMock.mockReturnValue(true) + netConnectMock.mockImplementation(() => { + const handlers: Record void)[]> = { connect: [], error: [] } + return { + on(event: string, cb: () => void) { + handlers[event]?.push(cb) + if (event === 'connect') { + queueMicrotask(() => cb()) + } + return this + }, + removeListener(event: string, cb: () => void) { + handlers[event] = handlers[event]?.filter((handler) => handler !== cb) ?? [] + return this + }, + destroy() {} + } + }) + const mod = await importFresh() + daemonClientMock.mockImplementationOnce(function MockWedgedDaemonClient() { + return { + ensureConnected: vi.fn(async () => { + throw new Error('Hello response timed out') + }), + request: vi.fn(), + disconnect: vi.fn() + } + }) + + await expect( + mod.cleanupDaemonForProtocol('/fake/daemon', PROTOCOL_VERSION) + ).resolves.toMatchObject({ cleaned: true }) + + expect(killStaleDaemonMock).toHaveBeenCalledWith( + '/fake/daemon', + `/fake/daemon/daemon-v${PROTOCOL_VERSION}.sock`, + `/fake/daemon/daemon-v${PROTOCOL_VERSION}.token`, + PROTOCOL_VERSION + ) + }) + it('coalesces concurrent restartDaemon() calls so the 7-step sequence runs exactly once', async () => { const mod = await importFresh() await mod.initDaemonPtyProvider() @@ -2884,7 +2928,10 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { expect(forkMock).not.toHaveBeenCalled() }) - it('replaces a health-check-failing daemon when live sessions cannot be verified and the pipe is dead', async () => { + // Note: only the adoption client is wedged here; the session probe answers, so this is the + // verified-zero case and the live-PTY veto stays off. The unverifiable path is covered by + // the grace-retry tests below. + it('replaces a health-check-failing daemon that reports no sessions when the pipe is dead', async () => { const mod = await importFresh() await mod.initDaemonPtyProvider() diff --git a/src/main/daemon/daemon-live-pty-evidence.test.ts b/src/main/daemon/daemon-live-pty-evidence.test.ts index d30c857e4a6..ecd84b5d4d6 100644 --- a/src/main/daemon/daemon-live-pty-evidence.test.ts +++ b/src/main/daemon/daemon-live-pty-evidence.test.ts @@ -1,8 +1,20 @@ -import { describe, expect, it, vi } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest' import { inspectDaemonPtyOwnership } from './daemon-live-pty-evidence' import type { ProcessTableRow } from '../../shared/process-table-snapshot' import type { WindowsProcessCandidate } from '../providers/windows-foreground-process-rows' +const { readFreshProcessTable, readCachedProcessTable } = vi.hoisted(() => ({ + readFreshProcessTable: vi.fn(async () => [] as ProcessTableRow[]), + readCachedProcessTable: vi.fn(async () => [] as ProcessTableRow[]) +})) + +// Spread the original: the Windows enumerator builds its reader from this module too. +vi.mock('../../shared/process-table-snapshot', async (importOriginal) => ({ + ...(await importOriginal>()), + getFreshProcessTableSnapshot: readFreshProcessTable, + getProcessTableSnapshot: readCachedProcessTable +})) + const DAEMON_PID = 4242 function row(pid: number, ppid: number, overrides: Partial = {}): ProcessTableRow { @@ -103,13 +115,20 @@ describe('inspectDaemonPtyOwnership on POSIX', () => { ).resolves.toBe('unknown') }) - it('tolerates a ppid cycle without hanging', async () => { + it('tolerates a ppid cycle reachable from the daemon without hanging', async () => { + // Why this shape: `ps` is not atomic, so a re-parented process can appear twice and + // close a loop. The cycle must be reachable from the root or the walk never enters it. await expect( inspectDaemonPtyOwnership(DAEMON_PID, { platform: 'darwin', - readPosixProcessTable: posixTable([daemonRow, row(101, 102), row(102, 101)]) + readPosixProcessTable: posixTable([ + daemonRow, + row(101, DAEMON_PID), + row(102, 101), + row(101, 102) + ]) }) - ).resolves.toBe('no-live-ptys') + ).resolves.toBe('owns-live-ptys') }) it('never consults the Windows enumerator on POSIX', async () => { @@ -166,6 +185,21 @@ describe('inspectDaemonPtyOwnership on win32', () => { ).resolves.toBe('unknown') }) + it('gives up as unknown when enumeration blows its deadline', async () => { + // Why bounded: the CIM query has no budget of its own and can queue behind an in-flight + // scan, so an unbounded wait would stall the launch path. Blind is safe here; hanging is not. + const queryWindowsDescendants = vi.fn(() => new Promise(() => {})) + + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'win32', + windowsDeadlineMs: 5, + queryWindowsDescendants + }) + ).resolves.toBe('unknown') + expect(queryWindowsDescendants).toHaveBeenCalled() + }) + it('never consults the POSIX table on win32', async () => { const readPosixProcessTable = vi.fn(async () => [daemonRow]) @@ -238,3 +272,21 @@ describe('inspectDaemonPtyOwnership sampling', () => { expect(readPosixProcessTable.mock.calls.length).toBeGreaterThan(1) }) }) + +describe('inspectDaemonPtyOwnership POSIX process-table source', () => { + beforeEach(() => { + readFreshProcessTable.mockReset() + readCachedProcessTable.mockReset() + readFreshProcessTable.mockResolvedValue([daemonRow, row(101, DAEMON_PID)]) + readCachedProcessTable.mockResolvedValue([]) + }) + + it('reads an uncached table, since the cached one can predate the PTYs it protects', async () => { + // The 500ms TTL would also hand both samples the same array, collapsing the confirmation. + await expect(inspectDaemonPtyOwnership(DAEMON_PID, { platform: 'darwin' })).resolves.toBe( + 'owns-live-ptys' + ) + expect(readFreshProcessTable).toHaveBeenCalled() + expect(readCachedProcessTable).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/daemon/daemon-live-pty-evidence.ts b/src/main/daemon/daemon-live-pty-evidence.ts index e19565be057..8688b839a6e 100644 --- a/src/main/daemon/daemon-live-pty-evidence.ts +++ b/src/main/daemon/daemon-live-pty-evidence.ts @@ -21,17 +21,42 @@ export type DaemonPtyOwnershipDeps = { platform?: NodeJS.Platform readPosixProcessTable?: () => Promise queryWindowsDescendants?: typeof queryWindowsProcessDescendants + windowsDeadlineMs?: number } /** - * Why generous, and why sampled twice: the load that wedges the daemon is the same - * load that makes `ps` miss a tight budget, so the evidence would go blind exactly - * when it matters most. A positive is confirmed by a second sample so a short-lived - * child — a resolver probe, a health-check shell — cannot masquerade as an agent. + * Why sampled twice: the load that wedges the daemon is the same load that can blind + * the process-table read, so a single blind sample would lose the evidence exactly when + * it matters most. A second sample also lets a short-lived child — a resolver probe, a + * health-check shell — be recognized as transient rather than mistaken for an agent. * This runs only on the replace path, after ~60s of grace is already spent. */ const PTY_OWNERSHIP_PROBE_ATTEMPTS = 2 +/** + * Windows enumeration has no budget of its own: each scan is a powershell CIM query that + * may fall back to wmic, and the shared reader can queue behind an in-flight scan — worst + * case tens of seconds on the launch path. Blind is a safe answer here; hanging is not. + */ +const WINDOWS_OWNERSHIP_PROBE_DEADLINE_MS = 6_000 + +function withDeadline(work: Promise, deadlineMs: number, onDeadline: T): Promise { + return new Promise((resolve) => { + const timer = setTimeout(() => resolve(onDeadline), deadlineMs) + timer.unref?.() + void work.then( + (value) => { + clearTimeout(timer) + resolve(value) + }, + () => { + clearTimeout(timer) + resolve(onDeadline) + } + ) + }) +} + // A wedged daemon cannot reap, so its exited PTYs linger as zombies. Counting them // would read "every agent already exited" as "agents still running" — and that // false positive is systematically correlated with the wedge we are diagnosing. @@ -77,11 +102,12 @@ async function probeOnce( platform: NodeJS.Platform ): Promise { if (platform === 'win32') { - const descendants = await (deps.queryWindowsDescendants ?? queryWindowsProcessDescendants)( - daemonPid, - { fresh: true } + const descendants = await withDeadline( + (deps.queryWindowsDescendants ?? queryWindowsProcessDescendants)(daemonPid, { fresh: true }), + deps.windowsDeadlineMs ?? WINDOWS_OWNERSHIP_PROBE_DEADLINE_MS, + null ) - // null covers both enumeration failure and a root absent from the table. + // null covers enumeration failure, a root absent from the table, and our own deadline. if (descendants === null) { return 'unknown' } @@ -97,10 +123,14 @@ async function probeOnce( } /** - * PTY children are the daemon's only long-lived descendants, so a descendant that - * survives two samples is positive proof that killing it would destroy running work. - * Descendants rather than direct children: macOS wraps every shell in login(1) for - * TCC attribution, so the agent is a grandchild at best. + * PTY children are the daemon's only long-lived descendants, so a live descendant is + * positive proof that killing it would destroy running work. Descendants rather than + * direct children: macOS wraps every shell in login(1) for TCC attribution, so the + * agent is a grandchild at best. + * + * Resolution between disagreeing samples: an observed-empty root wins (it is conclusive, + * and it is how a transient child is recognized), and otherwise a single sighting is + * enough to preserve — a blind read is not evidence against a live one. */ export async function inspectDaemonPtyOwnership( daemonPid: number,