From 840d3277d1d040ad059e442063f9d47972f25b6a Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 14 Jul 2026 01:50:56 -0700 Subject: [PATCH] fix(daemon): replace a permanently wedged daemon instead of preserving it forever (#8689) (#8697) * fix(daemon): replace a permanently wedged daemon instead of preserving it forever (#8689) A daemon whose socket accepts connections but whose event loop never answers the 'hello' handshake was adopted by the launcher unconditionally and never re-evaluated, so every terminal spawn failed with 'DaemonProtocolError: Hello response timed out' with no recovery. - daemon-init.ts: bound the launcher's 'preserve any unresponsive-but-connectable daemon' with a grace window. A transient wedge (Windows update-relaunch AV/disk pressure) drains within ~20s and is preserved WITH its live sessions; a permanent wedge exhausts the grace and is replaced. Stays well under the 60s local-PTY fail-open cap. - daemon-pty-adapter.ts: isDaemonGoneError now treats 'Hello response timed out' as daemon-gone, so a runtime wedge triggers withDaemonRetry's respawn (re-entering the same grace-bounded launcher) instead of failing every spawn until app restart. Tests pin the grace magnitude so it cannot be silently shrunk. Co-authored-by: Orca * fix(daemon): widen wedged-daemon grace window to ~60s Bump WEDGED_DAEMON_GRACE_RETRIES 3 -> 11 (~20s -> ~60s) to keep live-session loss on the transient-wedge (Windows update-relaunch) path as close to zero as possible. A transient wedge drains early and stays under the 60s fail-open cap; only a permanent wedge runs the full window. Export the constant and pin its floor in tests so it can't be silently shrunk. Co-authored-by: Orca --------- Co-authored-by: Orca --- src/main/daemon/daemon-init.test.ts | 194 +++++++++++++++--- src/main/daemon/daemon-init.ts | 55 +++-- src/main/daemon/daemon-pty-adapter.test.ts | 29 +++ src/main/daemon/daemon-pty-adapter.ts | 10 +- ...6814-daemon-failure-classification.test.ts | 26 ++- 5 files changed, 271 insertions(+), 43 deletions(-) diff --git a/src/main/daemon/daemon-init.test.ts b/src/main/daemon/daemon-init.test.ts index 06344c0759f..89493f675bf 100644 --- a/src/main/daemon/daemon-init.test.ts +++ b/src/main/daemon/daemon-init.test.ts @@ -7,6 +7,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { join } from 'node:path' import { PROTOCOL_VERSION } from './types' +import { WEDGED_DAEMON_GRACE_RETRIES } from './daemon-init' const FAKE_USER_DATA_PATH = '/fake/userData' const FAKE_RUNTIME_DIR = join(FAKE_USER_DATA_PATH, 'daemon') @@ -1622,15 +1623,36 @@ 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. + // Why: a net.connect stub whose 'connect' fires — makes probeSocket() report + // the wedged daemon's pipe as alive on every grace re-check. + function stubAliveSocketConnect() { + 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() {} + } + } + + it('adopts a transiently wedged daemon that drains and reports live sessions within the grace window', async () => { + // Why: the Windows update-relaunch case — post-install disk/AV load wedges + // the daemon past the first hello budget, but it drains within seconds and + // still owns live sessions. The launcher must give it a bounded grace and + // ADOPT it rather than killing its live sessions. const mod = await importFresh() await mod.initDaemonPtyProvider() + // First probe times out (still draining); the retry within the grace + // window succeeds and reports a live session. daemonClientMock.mockImplementationOnce(function MockDaemonClient() { return { ensureConnected: vi.fn(async () => { @@ -1640,36 +1662,156 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { disconnect: vi.fn() } }) + daemonClientMock.mockImplementationOnce(function MockDaemonClient() { + return { + ensureConnected: vi.fn(async () => {}), + request: vi.fn(async () => ({ sessions: [{ sessionId: 'wt-1@@live', isAlive: true }] })), + disconnect: vi.fn() + } + }) + + const launcher = spawnerInstances[0].launcher as ( + socketPath: string, + tokenPath: string + ) => Promise<{ shutdown(): Promise }> + checkDaemonHealthMock.mockResolvedValueOnce('unreachable') + probeSocketExistsMock.mockReturnValue(true) + netConnectMock.mockImplementation(stubAliveSocketConnect) + + await launcher('/fake/socket', '/fake/token') + + expect(killStaleDaemonMock).not.toHaveBeenCalled() + expect(forkMock).not.toHaveBeenCalled() + }) + + it('replaces a permanently wedged daemon after the grace window is exhausted (#8689)', async () => { + // Why: a daemon whose socket keeps accepting connections but whose event + // loop never answers hello would, under the old code, be preserved forever + // — every terminal spawn then failed with "Hello response timed out" with + // no recovery. After the bounded grace it must be replaced so the app gets + // working terminals again. + const mod = await importFresh() + await mod.initDaemonPtyProvider() + + const answeringDefault = function MockDaemonClient() { + return { + ensureConnected: vi.fn(async () => {}), + request: vi.fn(async () => ({ sessions: [] })), + disconnect: vi.fn() + } + } + // Every probe across the whole grace window times out (permanent wedge). + daemonClientMock.mockImplementation(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: [] } + netConnectMock.mockImplementation(stubAliveSocketConnect) + 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() + })) + + // Count only the launcher's own session-count probes. + daemonClientMock.mockClear() + + try { + await launcher('/fake/socket', '/fake/token') + + expect(killStaleDaemonMock).toHaveBeenCalledWith( + FAKE_RUNTIME_DIR, + '/fake/socket', + '/fake/token' + ) + expect(forkMock).toHaveBeenCalled() + // The launcher probes the full grace budget before giving up: 1 initial + // probe + WEDGED_DAEMON_GRACE_RETRIES retries. + expect(daemonClientMock).toHaveBeenCalledTimes(1 + WEDGED_DAEMON_GRACE_RETRIES) + } finally { + // Restore the answering default so the persistent throwing impl above + // does not leak into later tests (clearAllMocks clears calls, not impls). + daemonClientMock.mockImplementation(answeringDefault) + } + }) + + it('grace budget is generous enough to ride out a ~60s transient wedge', () => { + // Why: pins the magnitude. Each probe waits out the client's 5s hello + // timeout, so 1 + 11 probes ≈ 60s of drain grace. Shrinking this narrows + // the window in which a transiently wedged daemon's live sessions are + // preserved instead of replaced — don't cut it without field telemetry. + expect(WEDGED_DAEMON_GRACE_RETRIES).toBeGreaterThanOrEqual(11) + }) + + it('preserves a daemon that stays wedged until the LAST allowed grace retry', async () => { + // Why: exercises the full grace loop end-to-end. The daemon throws on every + // probe except the final allowed one (1 + WEDGED_DAEMON_GRACE_RETRIES), on + // which it drains and reports a live session — so it must be preserved, not + // replaced. Cutting the retry budget below the drain point would replace a + // still-live daemon, which this test catches. + const mod = await importFresh() + await mod.initDaemonPtyProvider() + + let probe = 0 + const answeringDefault = function MockDaemonClient() { return { - on(event: string, cb: () => void) { - handlers[event]?.push(cb) - if (event === 'connect') { - queueMicrotask(() => cb()) + ensureConnected: vi.fn(async () => {}), + request: vi.fn(async () => ({ sessions: [] })), + disconnect: vi.fn() + } + } + daemonClientMock.mockImplementation(function MockDaemonClient() { + probe += 1 + const drainsNow = probe >= 1 + WEDGED_DAEMON_GRACE_RETRIES + return { + ensureConnected: vi.fn(async () => { + if (!drainsNow) { + throw new Error('Hello response timed out') } - return this - }, - removeListener(event: string, cb: () => void) { - handlers[event] = handlers[event]?.filter((handler) => handler !== cb) ?? [] - return this - }, - destroy() {} + }), + request: vi.fn(async () => ({ + sessions: drainsNow ? [{ sessionId: 'wt-1@@live', isAlive: true }] : [] + })), + disconnect: vi.fn() } }) - await launcher('/fake/socket', '/fake/token') + const launcher = spawnerInstances[0].launcher as ( + socketPath: string, + tokenPath: string + ) => Promise<{ shutdown(): Promise }> + checkDaemonHealthMock.mockResolvedValueOnce('unreachable') + probeSocketExistsMock.mockReturnValue(true) + netConnectMock.mockImplementation(stubAliveSocketConnect) - expect(killStaleDaemonMock).not.toHaveBeenCalled() - expect(forkMock).not.toHaveBeenCalled() + try { + await launcher('/fake/socket', '/fake/token') + + expect(killStaleDaemonMock).not.toHaveBeenCalled() + expect(forkMock).not.toHaveBeenCalled() + } finally { + daemonClientMock.mockImplementation(answeringDefault) + } }) it('replaces a hello-rejected daemon even though its pipe accepts connections', async () => { @@ -1695,6 +1837,7 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { ) => Promise<{ shutdown(): Promise }> checkDaemonHealthMock.mockResolvedValueOnce('rejected') probeSocketExistsMock.mockReturnValue(true) + netConnectMock.mockImplementation(stubAliveSocketConnect) forkMock.mockImplementationOnce(() => ({ pid: 12345, on(event: string, cb: (arg?: unknown) => void) { @@ -1709,6 +1852,7 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { disconnect: vi.fn(), unref: vi.fn() })) + daemonClientMock.mockClear() await launcher('/fake/socket', '/fake/token') @@ -1718,6 +1862,10 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { '/fake/token' ) expect(forkMock).toHaveBeenCalled() + // Pins the 'rejected' fast-path: a daemon that actively refuses the + // handshake is never worth a grace window, so it is probed exactly once + // (no retries) before replacement. + expect(daemonClientMock).toHaveBeenCalledTimes(1) }) it('adopts a healthy daemon whose pid-file identity cannot be verified (null startedAtMs metadata)', async () => { diff --git a/src/main/daemon/daemon-init.ts b/src/main/daemon/daemon-init.ts index ad0227bae86..1bbc2cad9c3 100644 --- a/src/main/daemon/daemon-init.ts +++ b/src/main/daemon/daemon-init.ts @@ -65,6 +65,24 @@ function logDaemonMilestone(event: string, details: Record = {} } } +// Why: how many extra hello+listSessions probes to make against a wedged-but- +// connectable daemon before replacing it. Each probe waits out the client's 5s +// hello timeout, so this spaces re-checks ~5s apart: 1 initial + 11 retries ≈ +// 60s of grace for a transiently wedged daemon (Windows update-relaunch drain) +// to answer and be preserved WITH its live sessions, before a permanent wedge +// (#8689) is replaced. Deliberately generous to keep live-session loss on the +// transient path as close to zero as possible. +// +// A transient wedge drains early (well under the 60s local-PTY fail-open cap), +// so its startup is short. Only a *permanent* wedge runs the full window; it can +// then approach/exceed the fail-open cap, at which point restored panes fail +// open to the in-process provider for the session and adopt the freshly forked +// daemon on the next launch — a rare path that still recovers, versus the old +// forever-broken behavior. Trade-off: a transient wedge owning live sessions +// that takes longer than ~60s to drain is replaced (live processes lost, though +// scrollback cold-restores). Raise this only alongside the fail-open cap. +export const WEDGED_DAEMON_GRACE_RETRIES = 11 + let spawner: DaemonSpawner | null = null type DaemonProvider = DaemonPtyRouter | DaemonPtyAdapter | DegradedDaemonPtyProvider @@ -257,7 +275,28 @@ function createOutOfProcessLauncher(runtimeDir: string): DaemonLauncher { // 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. - const liveSessionCount = await getAliveDaemonSessionCount(socketPath, tokenPath) + let liveSessionCount = await getAliveDaemonSessionCount(socketPath, tokenPath) + // Why: on a Windows update relaunch the daemon can be transiently wedged + // past every RPC budget (final checkpoint flush + installer/AV disk + // pressure) while its sessions are still alive — replacing it here is what + // killed those sessions. A pipe that still accepts connections proves a + // live daemon, so give a wedged-but-connectable daemon a bounded grace to + // drain and answer before deciding. A PERMANENTLY wedged daemon (accepts + // connections but its event loop never answers hello — #8689) exhausts the + // grace and falls through to replacement below, instead of being preserved + // forever, which strands the app with zero working terminals. 'rejected' + // means the daemon answered and refused the handshake — it can never be + // adopted, so it skips the grace and replacement stays the only recovery. + let graceRetry = 0 + while ( + liveSessionCount === null && + health !== 'rejected' && + graceRetry < WEDGED_DAEMON_GRACE_RETRIES && + (await probeSocket(socketPath)) + ) { + liveSessionCount = await getAliveDaemonSessionCount(socketPath, tokenPath) + graceRetry++ + } if (liveSessionCount !== null && liveSessionCount > 0) { if (health === 'pty-spawn-unhealthy') { console.warn( @@ -274,20 +313,6 @@ 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 diff --git a/src/main/daemon/daemon-pty-adapter.test.ts b/src/main/daemon/daemon-pty-adapter.test.ts index 06a77789f4b..37896ab8336 100644 --- a/src/main/daemon/daemon-pty-adapter.test.ts +++ b/src/main/daemon/daemon-pty-adapter.test.ts @@ -4,6 +4,7 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import { mkdtempSync, mkdirSync, rmSync, existsSync, readFileSync, writeFileSync } from 'node:fs' import { DaemonClient } from './client' +import { DaemonProtocolError } from './daemon-errors' import { DaemonPtyAdapter } from './daemon-pty-adapter' import { DaemonServer } from './daemon-server' import { HeadlessEmulator } from './headless-emulator' @@ -1945,6 +1946,34 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => { noRespawnAdapter.dispose() }) + it('treats a hello handshake timeout as daemon-gone and respawns (#8689)', async () => { + // Why: a wedged daemon accepts the socket connection but never answers + // hello, so ensureConnected() rejects with "Hello response timed out". + // That must be classified as daemon-gone so withDaemonRetry respawns and + // retries — otherwise every terminal spawn fails against the wedge forever. + const realEnsureConnected = DaemonClient.prototype.ensureConnected + const ensureConnectedSpy = vi + .spyOn(DaemonClient.prototype, 'ensureConnected') + .mockImplementationOnce(async () => { + // The exact error type + message the real client raises on a wedge. + throw new DaemonProtocolError('Hello response timed out') + }) + .mockImplementation(function (this: DaemonClient) { + return realEnsureConnected.call(this) + }) + const respawnFn = vi.fn(async () => {}) + const respawnAdapter = new DaemonPtyAdapter({ socketPath, tokenPath, respawn: respawnFn }) + + try { + const result = await respawnAdapter.spawn({ cols: 80, rows: 24 }) + expect(result.id).toBeDefined() + expect(respawnFn).toHaveBeenCalledOnce() + } finally { + ensureConnectedSpy.mockRestore() + respawnAdapter.dispose() + } + }) + it('coalesces concurrent respawns so only one daemon is forked', async () => { let respawnServer: DaemonServer | undefined const respawnFn = vi.fn(async () => { diff --git a/src/main/daemon/daemon-pty-adapter.ts b/src/main/daemon/daemon-pty-adapter.ts index e0d5ec598ca..25a9be9754a 100644 --- a/src/main/daemon/daemon-pty-adapter.ts +++ b/src/main/daemon/daemon-pty-adapter.ts @@ -1354,8 +1354,12 @@ export class DaemonPtyAdapter implements IPtyProvider { // unreachable (daemon died). Checking syscall avoids false positives from // token-file ENOENT (readFileSync), which has no syscall or syscall='open'. // "Connection lost" / "Not connected" mean the daemon died while we had an -// active or stale connection. All indicate the daemon is gone and a respawn -// should be attempted. +// active or stale connection. "Hello response timed out" means we reconnected +// to a daemon whose socket accepts connections but whose event loop never +// answers the handshake (a wedged daemon, #8689) — respawning re-enters the +// grace-bounded launcher, which drains a transient wedge or replaces a +// permanent one instead of failing every terminal forever. All indicate the +// daemon is unusable and a respawn should be attempted. function isDaemonGoneError(err: unknown): boolean { if (!(err instanceof Error)) { return false @@ -1365,5 +1369,5 @@ function isDaemonGoneError(err: unknown): boolean { return true } const msg = err.message - return msg === 'Connection lost' || msg === 'Not connected' + return msg === 'Connection lost' || msg === 'Not connected' || msg === 'Hello response timed out' } diff --git a/src/main/daemon/issue-6814-daemon-failure-classification.test.ts b/src/main/daemon/issue-6814-daemon-failure-classification.test.ts index e6ec68548c6..e1523a325b8 100644 --- a/src/main/daemon/issue-6814-daemon-failure-classification.test.ts +++ b/src/main/daemon/issue-6814-daemon-failure-classification.test.ts @@ -10,7 +10,8 @@ // still answers protocol, so a regression that widened it to wedged daemons // would silently strand fresh terminals on a daemon that cannot serve them. import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -import { mkdtempSync, rmSync } from 'node:fs' +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { createServer } from 'node:net' import { tmpdir } from 'node:os' import { basename, join } from 'node:path' import { DaemonServer } from './daemon-server' @@ -107,13 +108,34 @@ describe('issue #6814 repro: daemon failure-mode classification', () => { const health = await checkDaemonHealth(socketPath, tokenPath) // This is the key finding: wedged != degraded. #6830's degraded fallback // does NOT engage here; recovery still depends on the unreachable-path - // (preserve-if-live-else-replace) logic, not the degraded provider. + // (grace-bounded preserve-if-live-else-replace) logic, not the degraded + // provider. expect(health).toBe('unreachable') } finally { await server.shutdown() } }, 15000) + // #8689: a daemon whose socket accepts connections but never answers the + // hello handshake (event loop wedged before the protocol reply) also + // classifies as 'unreachable' — the launcher's grace-bounded path must + // eventually replace it rather than preserve it forever. + it('WEDGED-HELLO: a daemon that accepts connections but never answers hello classifies as unreachable (#8689)', async () => { + const wedged = createServer((sock) => { + // Swallow the hello bytes but never write a response — the exact wedge. + sock.on('data', () => {}) + }) + await new Promise((resolve) => wedged.listen(socketPath, () => resolve())) + // The health check reads the token before connecting, so it must exist. + writeFileSync(tokenPath, 'wedged-token') + try { + const health = await checkDaemonHealth(socketPath, tokenPath) + expect(health).toBe('unreachable') + } finally { + await new Promise((resolve) => wedged.close(() => resolve())) + } + }, 15000) + // No daemon at all (or token missing) -> unreachable. it('UNREACHABLE: no daemon listening classifies as unreachable', async () => { await expect(checkDaemonHealth(socketPath, tokenPath)).resolves.toBe('unreachable')