diff --git a/src/main/daemon/daemon-entry.ts b/src/main/daemon/daemon-entry.ts index aeffb7aa2a5..184037455ea 100644 --- a/src/main/daemon/daemon-entry.ts +++ b/src/main/daemon/daemon-entry.ts @@ -83,7 +83,9 @@ async function main(): Promise { // Signal readiness to parent via IPC (if available) if (process.send) { - process.send({ type: 'ready' }) + // Why: Windows has no cheap OS query for a child's start time, so the + // daemon self-reports it here for the pid file's pid-recycling guard. + process.send({ type: 'ready', startedAtMs: Date.now() - process.uptime() * 1000 }) } warmWindowsConptyOnce() diff --git a/src/main/daemon/daemon-health.test.ts b/src/main/daemon/daemon-health.test.ts index 94d0564d09b..defbc8fd09a 100644 --- a/src/main/daemon/daemon-health.test.ts +++ b/src/main/daemon/daemon-health.test.ts @@ -13,7 +13,9 @@ import { parseLinuxBootTimeSeconds, parseLinuxProcStartTicks, parseDaemonPidFile, - startTimeMatches + parseWindowsProcessIdentityJson, + startTimeMatches, + startTimesWithinTolerance } from './daemon-health' import type { SubprocessHandle } from './session' @@ -125,6 +127,26 @@ describe('daemon health', () => { await expect(healthCheckDaemon(socketPath, tokenPath)).resolves.toBe(false) }) + it('classifies a hello-rejected daemon as rejected, not unreachable', async () => { + // Why: 'rejected' means the daemon answered and refused adoption — the + // launcher may replace it. 'unreachable' also covers a wedged-but-live + // daemon, which must never be replaced while its pipe accepts connections. + const server = new DaemonServer({ + socketPath, + tokenPath, + spawnSubprocess: () => createMockSubprocess() + }) + await server.start() + + try { + writeFileSync(tokenPath, 'not-the-daemon-token', { mode: 0o600 }) + await expect(checkDaemonHealth(socketPath, tokenPath)).resolves.toBe('rejected') + await expect(healthCheckDaemon(socketPath, tokenPath)).resolves.toBe(false) + } finally { + await server.shutdown() + } + }) + it('does not unlink a live socket when the pid file does not match this daemon', async () => { if (process.platform === 'win32') { return @@ -286,6 +308,42 @@ describe('startTimeMatches', () => { }) }) +describe('parseWindowsProcessIdentityJson', () => { + it('parses command line and start time from the CIM query output', () => { + expect( + parseWindowsProcessIdentityJson( + '{"cmd":"Orca.exe daemon-entry.js","start":1700000000000}\r\n' + ) + ).toEqual({ commandLine: 'Orca.exe daemon-entry.js', startedAtMs: 1_700_000_000_000 }) + }) + + it('returns a null start time when CreationDate was unavailable', () => { + expect( + parseWindowsProcessIdentityJson('{"cmd":"Orca.exe daemon-entry.js","start":null}') + ).toEqual({ commandLine: 'Orca.exe daemon-entry.js', startedAtMs: null }) + }) + + it('returns null for a missing process or inaccessible command line', () => { + expect(parseWindowsProcessIdentityJson('')).toBeNull() + expect(parseWindowsProcessIdentityJson(' \r\n')).toBeNull() + expect(parseWindowsProcessIdentityJson('{"cmd":null,"start":123}')).toBeNull() + expect(parseWindowsProcessIdentityJson('not-json')).toBeNull() + }) +}) + +describe('startTimesWithinTolerance', () => { + it('fails open when either side is null', () => { + expect(startTimesWithinTolerance(null, 1_700_000_000_000, 1_500)).toBe(true) + expect(startTimesWithinTolerance(1_700_000_000_000, null, 1_500)).toBe(true) + expect(startTimesWithinTolerance(null, null, 1_500)).toBe(true) + }) + + it('matches within tolerance and rejects outside it', () => { + expect(startTimesWithinTolerance(1_700_000_001_000, 1_700_000_000_000, 1_500)).toBe(true) + expect(startTimesWithinTolerance(1_700_000_005_000, 1_700_000_000_000, 1_500)).toBe(false) + }) +}) + describe('killStaleDaemon pid identity guards', () => { let dir: string let socketPath: string diff --git a/src/main/daemon/daemon-health.ts b/src/main/daemon/daemon-health.ts index 96d4a885dea..50e7226f73c 100644 --- a/src/main/daemon/daemon-health.ts +++ b/src/main/daemon/daemon-health.ts @@ -24,8 +24,17 @@ const RESOLVER_HEALTH_CHECK_TIMEOUT_MS = 3_000 const KILL_WAIT_MS = 3_000 const KILL_POLL_MS = 100 const START_TIME_TOLERANCE_MS = 1_500 +// Why: on Windows the pid file's startedAtMs is the daemon's self-reported +// Node start time, while verification reads the OS process creation time — +// the gap between them is the exe bootstrap, which AV/disk pressure can +// stretch to seconds. Pid recycling differs by minutes-to-days, so a wide +// tolerance keeps the guard effective without false mismatches. +const WIN32_START_TIME_TOLERANCE_MS = 10_000 -export type DaemonHealth = 'healthy' | 'unreachable' | 'pty-spawn-unhealthy' +// 'rejected' means the daemon answered and refused the handshake (bad token, +// foreign protocol) — it can never be adopted, unlike 'unreachable', which +// also covers a live-but-wedged daemon that simply missed the RPC budget. +export type DaemonHealth = 'healthy' | 'unreachable' | 'rejected' | 'pty-spawn-unhealthy' type ParsedDaemonPid = { pid: number @@ -134,13 +143,13 @@ export function checkDaemonHealth(socketPath: string, tokenPath: string): Promis try { message = JSON.parse(line) as Record } catch { - settle('unreachable') + settle('rejected') return } if (message.type === 'hello') { if (!(message as HelloResponse).ok) { - settle('unreachable') + settle('rejected') return } // Why: a protocol-live daemon with a stale cwd or node-pty helper @@ -371,6 +380,10 @@ export function getProcessStartedAtMs(pid: number): number | null { } if (process.platform === 'win32') { + // Why: the only OS source is a CIM query costing a powershell spawn — + // too slow for this sync path. Windows pid files instead carry the + // daemon's self-reported start time from its ready message, and + // isDaemonProcess verifies it against CIM CreationDate asynchronously. return null } @@ -387,27 +400,61 @@ export function getProcessStartedAtMs(pid: number): number | null { } export function startTimeMatches(pid: number, expectedStartedAtMs: number | null): boolean { - if (expectedStartedAtMs === null) { + return startTimesWithinTolerance( + getProcessStartedAtMs(pid), + expectedStartedAtMs, + START_TIME_TOLERANCE_MS + ) +} + +// Why: fail open on null — a pid file or OS query without a start time must +// not veto an otherwise-matching daemon (adoption safety beats recycle safety). +export function startTimesWithinTolerance( + actualStartedAtMs: number | null, + expectedStartedAtMs: number | null, + toleranceMs: number +): boolean { + if (expectedStartedAtMs === null || actualStartedAtMs === null) { return true } - - const actualStartedAtMs = getProcessStartedAtMs(pid) - if (actualStartedAtMs === null) { - return true - } - - return Math.abs(actualStartedAtMs - expectedStartedAtMs) <= START_TIME_TOLERANCE_MS + return Math.abs(actualStartedAtMs - expectedStartedAtMs) <= toleranceMs } const execFileAsync = promisify(execFile) +export type WindowsProcessIdentity = { + commandLine: string + startedAtMs: number | null +} + +export function parseWindowsProcessIdentityJson(stdout: string): WindowsProcessIdentity | null { + const trimmed = stdout.trim() + if (!trimmed) { + return null + } + try { + const parsed = JSON.parse(trimmed) as { cmd?: unknown; start?: unknown } + if (typeof parsed.cmd !== 'string' || !parsed.cmd) { + return null + } + return { + commandLine: parsed.cmd, + startedAtMs: + typeof parsed.start === 'number' && Number.isFinite(parsed.start) ? parsed.start : null + } + } catch { + return null + } +} + // Why: the only reliable command-line source on Windows is a CIM query, which // costs a full powershell.exe spawn (300-800ms cold, worse under Defender). // Async because the sync version measurably froze the Electron main thread at // startup for the whole spawn (benchmark: ~0.5s warm, 3s timeout cap cold). -// Timed under ORCA_STARTUP_DIAGNOSTICS so the cold-start benchmark can -// attribute startup cost to these checks. -async function queryWindowsProcessCommandLine(pid: number): Promise { +// CreationDate rides along in the same spawn so start-time verification adds +// zero extra process launches. Timed under ORCA_STARTUP_DIAGNOSTICS so the +// cold-start benchmark can attribute startup cost to these checks. +async function queryWindowsProcessIdentity(pid: number): Promise { const startedAt = performance.now() try { const { stdout } = await execFileAsync( @@ -416,14 +463,17 @@ async function queryWindowsProcessCommandLine(pid: number): Promise { if (process.platform === 'win32') { - return queryWindowsProcessCommandLine(pid) + return (await queryWindowsProcessIdentity(pid))?.commandLine ?? null } try { diff --git a/src/main/daemon/daemon-init.test.ts b/src/main/daemon/daemon-init.test.ts index 349cd1fbec7..ad2e906b585 100644 --- a/src/main/daemon/daemon-init.test.ts +++ b/src/main/daemon/daemon-init.test.ts @@ -25,6 +25,8 @@ const { isPackagedMock, probeSocketExistsMock, writeFileSyncMock, + readFileSyncMock, + unlinkSyncMock, netConnectMock, forkMock, checkDaemonHealthMock, @@ -34,6 +36,7 @@ const { isDaemonStaleForCurrentBundleMock, killStaleDaemonMock, getProcessStartedAtMsMock, + parseDaemonPidFileMock, daemonClientMock, spawnerInstances, ensureRunningOverrides, @@ -50,6 +53,13 @@ const { const probeSocketExistsMock = vi.fn((_path?: string) => false) const writeFileSyncMock = vi.fn() + // Why: readFileSync throws by default so legacyDaemonProcessMayBeAlive's + // catch treats every legacy pid file as unreadable — matching the pre-fix + // cleanup behavior every existing test was written against. + const readFileSyncMock = vi.fn((): string => { + throw new Error('ENOENT') + }) + const unlinkSyncMock = vi.fn() const forkMock = vi.fn() const netConnectMock = vi.fn(() => { // Why: the real probeSocket() in daemon-init connects to the socket and @@ -82,7 +92,10 @@ const { const getDaemonLaunchIdentityMock = vi.fn(() => 'match') const isDaemonStaleForCurrentBundleMock = vi.fn(() => false) const killStaleDaemonMock = vi.fn(async () => true) - const getProcessStartedAtMsMock = vi.fn(() => 1_000_000) + const getProcessStartedAtMsMock = vi.fn((): number | null => 1_000_000) + const parseDaemonPidFileMock = vi.fn( + (): { pid: number; startedAtMs: number | null } | null => null + ) const daemonClientMock = vi.fn().mockImplementation(function MockDaemonClient() { return { @@ -141,6 +154,8 @@ const { isPackagedMock, probeSocketExistsMock, writeFileSyncMock, + readFileSyncMock, + unlinkSyncMock, netConnectMock, forkMock, checkDaemonHealthMock, @@ -150,6 +165,7 @@ const { isDaemonStaleForCurrentBundleMock, killStaleDaemonMock, getProcessStartedAtMsMock, + parseDaemonPidFileMock, daemonClientMock, spawnerInstances, ensureRunningOverrides, @@ -209,7 +225,8 @@ vi.mock('electron', () => ({ vi.mock('fs', () => ({ mkdirSync: vi.fn(), existsSync: (p: string) => probeSocketExistsMock(p) || p.includes('.pid'), - unlinkSync: vi.fn(), + unlinkSync: unlinkSyncMock, + readFileSync: readFileSyncMock, writeFileSync: writeFileSyncMock })) @@ -224,7 +241,8 @@ vi.mock('./daemon-health', () => ({ healthCheckDaemon: healthCheckDaemonMock, isDaemonStaleForCurrentBundle: isDaemonStaleForCurrentBundleMock, killStaleDaemon: killStaleDaemonMock, - getProcessStartedAtMs: getProcessStartedAtMsMock + getProcessStartedAtMs: getProcessStartedAtMsMock, + parseDaemonPidFile: parseDaemonPidFileMock })) vi.mock('./client', () => ({ DaemonClient: daemonClientMock })) @@ -352,6 +370,15 @@ async function importFresh() { daemonClientMock.mockClear() probeSocketExistsMock.mockClear() writeFileSyncMock.mockClear() + readFileSyncMock.mockReset() + readFileSyncMock.mockImplementation(() => { + throw new Error('ENOENT') + }) + unlinkSyncMock.mockClear() + parseDaemonPidFileMock.mockReset() + parseDaemonPidFileMock.mockReturnValue(null) + getProcessStartedAtMsMock.mockReset() + getProcessStartedAtMsMock.mockReturnValue(1_000_000) // Why: importing daemon-init *after* resetModules means the module-level // `spawner`/`adapter`/`restartInFlight` start fresh for every test, which is // the only way to reliably exercise the "first-time init" path and the @@ -1385,7 +1412,7 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { expect(forkMock).not.toHaveBeenCalled() }) - it('replaces a health-check-failing daemon when live sessions cannot be verified', async () => { + it('replaces a health-check-failing daemon when live sessions cannot be verified and the pipe is dead', async () => { const mod = await importFresh() await mod.initDaemonPtyProvider() @@ -1429,6 +1456,203 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { expect(forkMock).toHaveBeenCalled() }) + it('adopts an unresponsive daemon whose pipe still accepts connections (update-relaunch wedge)', async () => { + // Why: the Windows update-relaunch regression — post-install disk/AV load + // wedges the daemon past the 3s health budget AND the 5s hello budget of + // the session-list re-verification, while its sessions are still alive. + // The old fail-closed path killed the daemon here. A pipe that accepts a + // raw connection proves the daemon is alive, so the launcher must adopt. + const mod = await importFresh() + await mod.initDaemonPtyProvider() + + daemonClientMock.mockImplementationOnce(function MockDaemonClient() { + return { + ensureConnected: vi.fn(async () => { + throw new Error('Hello response timed out') + }), + request: vi.fn(), + disconnect: vi.fn() + } + }) + + const launcher = spawnerInstances[0].launcher as ( + socketPath: string, + tokenPath: string + ) => Promise<{ shutdown(): Promise }> + checkDaemonHealthMock.mockResolvedValueOnce('unreachable') + // The raw pipe probe succeeds even though every RPC timed out. + probeSocketExistsMock.mockReturnValue(true) + netConnectMock.mockImplementationOnce(() => { + 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() {} + } + }) + + await launcher('/fake/socket', '/fake/token') + + expect(killStaleDaemonMock).not.toHaveBeenCalled() + expect(forkMock).not.toHaveBeenCalled() + }) + + it('replaces a hello-rejected daemon even though its pipe accepts connections', async () => { + // Why: 'rejected' means the daemon answered and refused the handshake — + // it can never be adopted, so keeping it alive would strand the app with + // no terminals forever. Replacement stays the only recovery. + const mod = await importFresh() + await mod.initDaemonPtyProvider() + + daemonClientMock.mockImplementationOnce(function MockDaemonClient() { + return { + ensureConnected: vi.fn(async () => { + throw new Error('Hello rejected') + }), + request: vi.fn(), + disconnect: vi.fn() + } + }) + + const launcher = spawnerInstances[0].launcher as ( + socketPath: string, + tokenPath: string + ) => Promise<{ shutdown(): Promise }> + checkDaemonHealthMock.mockResolvedValueOnce('rejected') + probeSocketExistsMock.mockReturnValue(true) + forkMock.mockImplementationOnce(() => ({ + pid: 12345, + on(event: string, cb: (arg?: unknown) => void) { + if (event === 'message') { + queueMicrotask(() => cb({ type: 'ready' })) + } + return this + }, + off() { + return this + }, + disconnect: vi.fn(), + unref: vi.fn() + })) + + await launcher('/fake/socket', '/fake/token') + + expect(killStaleDaemonMock).toHaveBeenCalledWith( + FAKE_RUNTIME_DIR, + '/fake/socket', + '/fake/token' + ) + expect(forkMock).toHaveBeenCalled() + }) + + it('adopts a healthy daemon whose pid-file identity cannot be verified (null startedAtMs metadata)', async () => { + // Why: the regression contract — a pid file with startedAtMs null (all + // pre-fix Windows pid files) resolves launch identity to 'unknown'. With + // a live daemon answering on the pipe, that must ADOPT, never replace. + const mod = await importFresh() + await mod.initDaemonPtyProvider() + + const launcher = spawnerInstances[0].launcher as ( + socketPath: string, + tokenPath: string + ) => Promise<{ shutdown(): Promise }> + getDaemonLaunchIdentityMock.mockReturnValueOnce('unknown') + isPackagedMock.mockReturnValue(true) + + await launcher('/fake/socket', '/fake/token') + + expect(killStaleDaemonMock).not.toHaveBeenCalled() + expect(forkMock).not.toHaveBeenCalled() + }) + + it('writes the daemon self-reported start time to the pid file when the OS query returns null', async () => { + // Why: getProcessStartedAtMs has no cheap Windows implementation, so the + // pid file's pid-recycling guard depends on the ready-message fallback. + const mod = await importFresh() + await mod.initDaemonPtyProvider() + + const launcher = spawnerInstances[0].launcher as ( + socketPath: string, + tokenPath: string + ) => Promise<{ shutdown(): Promise }> + checkDaemonHealthMock.mockResolvedValueOnce('unreachable') + getProcessStartedAtMsMock.mockReturnValue(null) + forkMock.mockImplementationOnce(() => ({ + pid: 12345, + on(event: string, cb: (arg?: unknown) => void) { + if (event === 'message') { + queueMicrotask(() => cb({ type: 'ready', startedAtMs: 1_700_000_123_456 })) + } + return this + }, + off() { + return this + }, + disconnect: vi.fn(), + unref: vi.fn() + })) + + await launcher('/fake/socket', '/fake/token') + + expect(writeFileSyncMock).toHaveBeenCalledWith( + `/fake/daemon/daemon-v${PROTOCOL_VERSION}.pid`, + JSON.stringify({ + pid: 12345, + startedAtMs: 1_700_000_123_456, + entryPath: FAKE_DAEMON_ENTRY_PATH, + appVersion: '1.2.3' + }), + { mode: 0o600 } + ) + }) + + it('keeps legacy daemon pid/token files when the probe fails but the pid-file process is alive', async () => { + // Why: deleting a live-but-wedged legacy daemon's token file makes its + // sessions permanently unadoptable — no future launch could authenticate. + const mod = await importFresh() + readFileSyncMock.mockReturnValue('{"pid":123}') + // process.pid is guaranteed alive, so the liveness probe succeeds. + parseDaemonPidFileMock.mockReturnValue({ pid: process.pid, startedAtMs: null }) + + await mod.initDaemonPtyProvider() + + const legacyUnlinks = unlinkSyncMock.mock.calls.filter( + ([p]) => typeof p === 'string' && (p.includes('.token') || p.includes('.pid')) + ) + expect(legacyUnlinks).toEqual([]) + }) + + it('cleans up legacy daemon pid/token files when the probe fails and the process is gone', async () => { + const mod = await importFresh() + readFileSyncMock.mockReturnValue('{"pid":123}') + // Why: spy on process.kill so the liveness probe deterministically reports + // "no such process" without depending on an unallocated real pid. + const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => { + throw new Error('ESRCH') + }) + parseDaemonPidFileMock.mockReturnValue({ pid: 999_999, startedAtMs: null }) + + try { + await mod.initDaemonPtyProvider() + } finally { + killSpy.mockRestore() + } + + const tokenUnlinks = unlinkSyncMock.mock.calls.filter( + ([p]) => typeof p === 'string' && p.includes('.token') + ) + expect(tokenUnlinks.length).toBeGreaterThan(0) + }) + it('replaces a health-check-failing daemon when no live sessions would be lost', async () => { const mod = await importFresh() await mod.initDaemonPtyProvider() diff --git a/src/main/daemon/daemon-init.ts b/src/main/daemon/daemon-init.ts index ff18ecc3397..5317d7aebef 100644 --- a/src/main/daemon/daemon-init.ts +++ b/src/main/daemon/daemon-init.ts @@ -8,7 +8,7 @@ module-level spawner/adapter singletons must stay co-located so a future change cannot leave them drifting out of sync. */ import { join } from 'node:path' import { app } from 'electron' -import { mkdirSync, existsSync, unlinkSync, writeFileSync } from 'node:fs' +import { mkdirSync, existsSync, readFileSync, unlinkSync, writeFileSync } from 'node:fs' import { fork } from 'node:child_process' import { connect } from 'node:net' import { @@ -34,7 +34,8 @@ import { getProcessStartedAtMs, checkDaemonHealth, isDaemonStaleForCurrentBundle, - killStaleDaemon + killStaleDaemon, + parseDaemonPidFile } from './daemon-health' import { DegradedDaemonPtyProvider } from './degraded-daemon-pty-provider' import { @@ -233,9 +234,7 @@ function createOutOfProcessLauncher(runtimeDir: string): DaemonLauncher { // Why: a busy machine (e.g. right after an update) can time out the // health check while the daemon is alive and owning terminals. Killing // it would destroy every live session, so re-verify with a session list - // first. Only a verified non-empty list preserves: a daemon that cannot - // even list sessions cannot serve terminals, and replacing it is the - // only recovery. + // first. const liveSessionCount = await getAliveDaemonSessionCount(socketPath, tokenPath) if (liveSessionCount !== null && liveSessionCount > 0) { if (health === 'pty-spawn-unhealthy') { @@ -253,6 +252,20 @@ function createOutOfProcessLauncher(runtimeDir: string): DaemonLauncher { ) return createPreservedDaemonHandle(runtimeDir) } + // Why: on a Windows update relaunch the daemon can be wedged past every + // RPC budget (final checkpoint flush + installer/AV disk pressure), so + // both the health check AND the session list time out while sessions + // are still alive — failing closed here is what killed those sessions. + // A pipe that still accepts connections proves a live daemon: adopt it + // and let the adapter reconnect once the daemon drains. 'rejected' + // means the daemon answered and refused the handshake — it can never be + // adopted, so replacement stays the only recovery. + if (liveSessionCount === null && health !== 'rejected' && (await probeSocket(socketPath))) { + console.warn( + '[daemon] Preserving unresponsive daemon because its socket still accepts connections' + ) + return createPreservedDaemonHandle(runtimeDir) + } } // Why: a raw socket can outlive a broken or wedged daemon. Kill by PID @@ -323,11 +336,19 @@ function createOutOfProcessLauncher(runtimeDir: string): DaemonLauncher { // killStaleDaemon() can verify the pid still belongs to the daemon // we forked before SIGTERMing it. Prevents pid-recycling hazard // where the OS hands the daemon's old pid to an unrelated process. + // Why the ready-message fallback: Windows has no cheap OS query + // for start time, so the daemon self-reports it — without this the + // recycling guard was permanently inert on win32. + const selfReported = (msg as { startedAtMs?: unknown }).startedAtMs writeFileSync( getDaemonPidPath(runtimeDir), serializeDaemonPidFile({ pid: child.pid, - startedAtMs: getProcessStartedAtMs(child.pid), + startedAtMs: + getProcessStartedAtMs(child.pid) ?? + (typeof selfReported === 'number' && Number.isFinite(selfReported) + ? selfReported + : null), entryPath, appVersion: app.getVersion() }), @@ -708,6 +729,21 @@ export async function cleanupDaemonForProtocol( return { cleaned: didRequestShutdown || didKillStaleDaemon, killedCount } } +function legacyDaemonProcessMayBeAlive(runtimeDir: string, protocolVersion: number): boolean { + try { + const parsed = parseDaemonPidFile( + readFileSync(getDaemonPidPath(runtimeDir, protocolVersion), 'utf8') + ) + if (!parsed) { + return false + } + process.kill(parsed.pid, 0) + return true + } catch { + return false + } +} + async function createLegacyDaemonAdapters(runtimeDir: string): Promise { const adapters: DaemonPtyAdapter[] = [] for (const protocolVersion of PREVIOUS_DAEMON_PROTOCOL_VERSIONS) { @@ -717,23 +753,27 @@ async function createLegacyDaemonAdapters(runtimeDir: string): Promise window.removeEventListener('beforeunload', captureAndFlush) }, []) + // Why: beforeunload never fires on a hard kill (crash, forced update + // install, TerminateProcess), so agents alive at that moment would leave no + // resume record. This periodic capture stores only agent session ids — not + // scrollback, see the no-periodic-scrollback note below — and the store + // action skips unchanged records, so idle ticks write nothing; real changes + // flow through the debounced session-write subscriber. + useEffect(() => { + const timer = window.setInterval(() => { + if (!shouldPersistWorkspaceSession(useAppStore.getState())) { + return + } + useAppStore.getState().captureAllSleepingAgentSessions() + }, SLEEPING_AGENT_RESUME_CAPTURE_INTERVAL_MS) + return () => window.clearInterval(timer) + }, []) + // Own the single window-close-request subscription at the always-mounted App // root. Why: the rich confirmation flow lives in Terminal, which is not // mounted on the no-workspace landing page (and is lazy-loaded elsewhere), so diff --git a/src/renderer/src/store/slices/agent-status-quit-capture.test.ts b/src/renderer/src/store/slices/agent-status-quit-capture.test.ts index 271fc9495b9..8896d40bb75 100644 --- a/src/renderer/src/store/slices/agent-status-quit-capture.test.ts +++ b/src/renderer/src/store/slices/agent-status-quit-capture.test.ts @@ -214,6 +214,52 @@ describe('captureAllSleepingAgentSessions', () => { }) }) + it('skips rewriting an unchanged resume record on repeated capture', () => { + const store = createTestStore() + store.setState({ + tabsByWorktree: { + 'wt-1': [makeTab({ id: 'tab-1', worktreeId: 'wt-1' })] + } + } as Partial) + const providerSession = { key: 'session_id' as const, id: 'codex-session-1' } + store + .getState() + .setAgentStatus( + 'tab-1:leaf-1', + { state: 'working', prompt: 'first task', agentType: 'codex' }, + 'Codex', + { updatedAt: 10, stateStartedAt: 10 }, + { tabId: 'tab-1', worktreeId: 'wt-1' }, + { providerSession } + ) + + store.getState().captureAllSleepingAgentSessions() + const first = store.getState().sleepingAgentSessionsByPaneKey['tab-1:leaf-1'] + expect(first).toMatchObject({ origin: 'quit' }) + + // Why: the periodic resume-record capture re-runs this action every + // minute; an unchanged agent must not dirty the store (and the debounced + // session write) with a record differing only by capturedAt. + store.getState().captureAllSleepingAgentSessions() + expect(store.getState().sleepingAgentSessionsByPaneKey['tab-1:leaf-1']).toBe(first) + + // A real status change must still refresh the record. + store + .getState() + .setAgentStatus( + 'tab-1:leaf-1', + { state: 'waiting', prompt: 'first task', agentType: 'codex' }, + 'Codex', + { updatedAt: 20, stateStartedAt: 20 }, + { tabId: 'tab-1', worktreeId: 'wt-1' }, + { providerSession } + ) + store.getState().captureAllSleepingAgentSessions() + const refreshed = store.getState().sleepingAgentSessionsByPaneKey['tab-1:leaf-1'] + expect(refreshed).not.toBe(first) + expect(refreshed).toMatchObject({ state: 'waiting', origin: 'quit' }) + }) + it('preserves hydrated launch config during live recapture without a registry entry', () => { const store = createTestStore() store.setState({ diff --git a/src/renderer/src/store/slices/agent-status.ts b/src/renderer/src/store/slices/agent-status.ts index 855790fe43b..bde89b4b0ce 100644 --- a/src/renderer/src/store/slices/agent-status.ts +++ b/src/renderer/src/store/slices/agent-status.ts @@ -542,6 +542,34 @@ export function collectHibernatedCompletionEvidenceForWorktree( return retained } +// Why: the periodic resume-record capture re-runs on an interval; comparing +// everything except capturedAt lets an unchanged agent skip the store write +// entirely, so idle ticks never dirty the session persistence pipeline. +function sleepingRecordsEquivalentIgnoringCaptureTime( + existing: SleepingAgentSessionRecord | undefined, + next: SleepingAgentSessionRecord +): boolean { + if (!existing) { + return false + } + return ( + existing.paneKey === next.paneKey && + existing.tabId === next.tabId && + existing.worktreeId === next.worktreeId && + existing.agent === next.agent && + existing.providerSession.key === next.providerSession.key && + existing.providerSession.id === next.providerSession.id && + existing.prompt === next.prompt && + existing.state === next.state && + existing.updatedAt === next.updatedAt && + existing.terminalTitle === next.terminalTitle && + existing.lastAssistantMessage === next.lastAssistantMessage && + existing.interrupted === next.interrupted && + existing.origin === next.origin && + launchConfigsEqual(existing.launchConfig, next.launchConfig) + ) +} + function recoveryRecordMatches( existing: SleepingAgentSessionRecord | undefined, next: SleepingAgentSessionRecord @@ -2009,7 +2037,10 @@ export const createAgentStatusSlice: StateCreator