diff --git a/src/main/daemon/daemon-health-socket-cleanup.test.ts b/src/main/daemon/daemon-health-socket-cleanup.test.ts index 8d2f8e9ad..a6404ed02 100644 --- a/src/main/daemon/daemon-health-socket-cleanup.test.ts +++ b/src/main/daemon/daemon-health-socket-cleanup.test.ts @@ -42,12 +42,7 @@ describe('daemon health socket listener cleanup', () => { const result = healthCheckDaemon(socketPath, tokenPath) socket.emit('connect') - socket.emit( - 'data', - Buffer.from( - '{"type":"hello","ok":true}\n{"id":"health-1","ok":true}\n{"id":"health-2","ok":true}\n' - ) - ) + socket.emit('data', Buffer.from('{"type":"hello","ok":true}\n{"id":"health-1","ok":true}\n')) await expect(result).resolves.toBe(true) expect(socket.listenerCount('connect')).toBe(0) diff --git a/src/main/daemon/daemon-health.test.ts b/src/main/daemon/daemon-health.test.ts index 6bf71a37f..92220f8d5 100644 --- a/src/main/daemon/daemon-health.test.ts +++ b/src/main/daemon/daemon-health.test.ts @@ -74,36 +74,15 @@ describe('daemon health', () => { }) it('passes when a daemon answers ping', async () => { - const ptySpawnHealthCheck = vi.fn(async () => {}) const server = new DaemonServer({ socketPath, tokenPath, - ptySpawnHealthCheck, spawnSubprocess: () => createMockSubprocess() }) await server.start() try { await expect(healthCheckDaemon(socketPath, tokenPath)).resolves.toBe(true) - expect(ptySpawnHealthCheck).toHaveBeenCalledOnce() - } finally { - await server.shutdown() - } - }) - - it('fails when a protocol-healthy daemon cannot spawn PTYs', async () => { - const server = new DaemonServer({ - socketPath, - tokenPath, - ptySpawnHealthCheck: vi.fn(async () => { - throw new Error('stale node-pty helper') - }), - spawnSubprocess: () => createMockSubprocess() - }) - await server.start() - - try { - await expect(healthCheckDaemon(socketPath, tokenPath)).resolves.toBe(false) } finally { await server.shutdown() } diff --git a/src/main/daemon/daemon-health.ts b/src/main/daemon/daemon-health.ts index 2f13e1502..a4e95ce72 100644 --- a/src/main/daemon/daemon-health.ts +++ b/src/main/daemon/daemon-health.ts @@ -21,8 +21,6 @@ const KILL_WAIT_MS = 3_000 const KILL_POLL_MS = 100 const START_TIME_TOLERANCE_MS = 1_500 -export type DaemonHealthCheckResult = 'healthy' | 'unhealthy' | 'pty-spawn-unhealthy' - type ParsedDaemonPid = { pid: number startedAtMs: number | null @@ -67,13 +65,10 @@ function canConnectSocket(socketPath: string): Promise { }) } -export function checkDaemonHealth( - socketPath: string, - tokenPath: string -): Promise { +export function healthCheckDaemon(socketPath: string, tokenPath: string): Promise { return new Promise((resolve) => { if (process.platform !== 'win32' && !existsSync(socketPath)) { - resolve('unhealthy') + resolve(false) return } @@ -81,13 +76,13 @@ export function checkDaemonHealth( try { token = readFileSync(tokenPath, 'utf8').trim() } catch { - resolve('unhealthy') + resolve(false) return } let settled = false let sock: Socket | null = null - const settle = (result: DaemonHealthCheckResult): void => { + const settle = (result: boolean): void => { if (settled) { return } @@ -102,7 +97,7 @@ export function checkDaemonHealth( sock?.off('connect', onConnect) sock?.off('data', onData) } - const onError = (): void => settle('unhealthy') + const onError = (): void => settle(false) const onConnect = (): void => { const hello: HelloMessage = { type: 'hello', @@ -133,13 +128,13 @@ export function checkDaemonHealth( try { message = JSON.parse(line) as Record } catch { - settle('unhealthy') + settle(false) return } if (message.type === 'hello') { if (!(message as HelloResponse).ok) { - settle('unhealthy') + settle(false) return } sock?.write(encodeNdjson({ id: 'health-1', type: 'ping' })) @@ -147,24 +142,12 @@ export function checkDaemonHealth( } if (message.id === 'health-1') { - if (!message.ok) { - settle('unhealthy') - return - } - // Why: protocol ping only proves the socket loop is alive. New - // terminals also depend on node-pty's native helper state inside - // the daemon process, which can go stale after dev rebuilds. - sock?.write(encodeNdjson({ id: 'health-2', type: 'ptySpawnHealth' })) - continue - } - - if (message.id === 'health-2') { - settle(message.ok ? 'healthy' : 'pty-spawn-unhealthy') + settle(message.ok === true) return } } } - const timer = setTimeout(() => settle('unhealthy'), HEALTH_CHECK_TIMEOUT_MS) + const timer = setTimeout(() => settle(false), HEALTH_CHECK_TIMEOUT_MS) sock = connect({ path: socketPath }) sock.on('error', onError) @@ -175,10 +158,6 @@ export function checkDaemonHealth( }) } -export async function healthCheckDaemon(socketPath: string, tokenPath: string): Promise { - return (await checkDaemonHealth(socketPath, tokenPath)) === 'healthy' -} - function isSystemResolverHealth(value: unknown): value is SystemResolverHealth { return value === 'healthy' || value === 'unhealthy' || value === 'unknown' } diff --git a/src/main/daemon/daemon-init.test.ts b/src/main/daemon/daemon-init.test.ts index e320609ef..14df83d33 100644 --- a/src/main/daemon/daemon-init.test.ts +++ b/src/main/daemon/daemon-init.test.ts @@ -20,7 +20,6 @@ const { writeFileSyncMock, netConnectMock, forkMock, - checkDaemonHealthMock, healthCheckDaemonMock, getMacDaemonSystemResolverHealthMock, getDaemonLaunchIdentityMock, @@ -67,7 +66,6 @@ const { } }) - const checkDaemonHealthMock = vi.fn(async () => 'healthy') const healthCheckDaemonMock = vi.fn(async () => true) const getMacDaemonSystemResolverHealthMock = vi.fn(() => 'healthy') const getDaemonLaunchIdentityMock = vi.fn(() => 'match') @@ -103,7 +101,6 @@ const { writeFileSyncMock, netConnectMock, forkMock, - checkDaemonHealthMock, healthCheckDaemonMock, getMacDaemonSystemResolverHealthMock, getDaemonLaunchIdentityMock, @@ -175,7 +172,6 @@ vi.mock('child_process', () => ({ fork: forkMock })) vi.mock('net', () => ({ connect: netConnectMock })) vi.mock('./daemon-health', () => ({ - checkDaemonHealth: checkDaemonHealthMock, getDaemonLaunchIdentity: getDaemonLaunchIdentityMock, getMacDaemonSystemResolverHealth: getMacDaemonSystemResolverHealthMock, healthCheckDaemon: healthCheckDaemonMock, @@ -272,9 +268,8 @@ async function importFresh() { setLocalPtyProviderMock.mockClear() unbindLocalProviderListenersMock.mockClear() rebindLocalProviderListenersMock.mockClear() - checkDaemonHealthMock.mockClear() - checkDaemonHealthMock.mockResolvedValue('healthy') healthCheckDaemonMock.mockClear() + healthCheckDaemonMock.mockResolvedValue(true) getMacDaemonSystemResolverHealthMock.mockReset() getMacDaemonSystemResolverHealthMock.mockReturnValue('healthy') getDaemonLaunchIdentityMock.mockClear() @@ -1031,7 +1026,7 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { probeSocketExistsMock.mockImplementation( (p?: string) => p === '/fake/app/out/main/daemon-entry.js' ) - checkDaemonHealthMock.mockResolvedValueOnce('unhealthy') + healthCheckDaemonMock.mockResolvedValueOnce(false) const mod = await importFresh() getAppPathMock.mockReturnValue('/fake/app/out/main') await mod.initDaemonPtyProvider() @@ -1074,7 +1069,7 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { }) it('removes detached daemon startup listeners after readiness', async () => { - checkDaemonHealthMock.mockResolvedValueOnce('unhealthy') + healthCheckDaemonMock.mockResolvedValueOnce(false) const mod = await importFresh() await mod.initDaemonPtyProvider() @@ -1129,7 +1124,7 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { }) it('removes detached daemon startup listeners after startup error', async () => { - checkDaemonHealthMock.mockResolvedValueOnce('unhealthy') + healthCheckDaemonMock.mockResolvedValueOnce(false) const mod = await importFresh() await mod.initDaemonPtyProvider() @@ -1173,7 +1168,7 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { expect(child.unref).not.toHaveBeenCalled() }) - it('preserves a spawn-unhealthy daemon when it owns live sessions', async () => { + it('preserves a health-check-failing daemon when it owns live sessions', async () => { const mod = await importFresh() await mod.initDaemonPtyProvider() @@ -1198,7 +1193,7 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { socketPath: string, tokenPath: string ) => Promise<{ shutdown(): Promise }> - checkDaemonHealthMock.mockResolvedValueOnce('pty-spawn-unhealthy') + healthCheckDaemonMock.mockResolvedValueOnce(false) await launcher('/fake/socket', '/fake/token') @@ -1208,7 +1203,51 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { expect(forkMock).not.toHaveBeenCalled() }) - it('replaces a spawn-unhealthy daemon when no live sessions would be lost', async () => { + it('replaces a health-check-failing daemon when live sessions cannot be verified', async () => { + const mod = await importFresh() + await mod.initDaemonPtyProvider() + + daemonClientMock.mockImplementationOnce(function MockDaemonClient() { + return { + ensureConnected: vi.fn(async () => { + throw new Error('daemon is wedged') + }), + request: vi.fn(), + disconnect: vi.fn() + } + }) + + const launcher = spawnerInstances[0].launcher as ( + socketPath: string, + tokenPath: string + ) => Promise<{ shutdown(): Promise }> + healthCheckDaemonMock.mockResolvedValueOnce(false) + 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/userData/daemon', + '/fake/socket', + '/fake/token' + ) + expect(forkMock).toHaveBeenCalled() + }) + + it('replaces a health-check-failing daemon when no live sessions would be lost', async () => { const mod = await importFresh() await mod.initDaemonPtyProvider() @@ -1216,7 +1255,7 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { socketPath: string, tokenPath: string ) => Promise<{ shutdown(): Promise }> - checkDaemonHealthMock.mockResolvedValueOnce('pty-spawn-unhealthy') + healthCheckDaemonMock.mockResolvedValueOnce(false) forkMock.mockImplementationOnce(() => { const handlers: Record void)[]> = { message: [], diff --git a/src/main/daemon/daemon-init.ts b/src/main/daemon/daemon-init.ts index ff1795cfb..4ff09169c 100644 --- a/src/main/daemon/daemon-init.ts +++ b/src/main/daemon/daemon-init.ts @@ -28,10 +28,10 @@ import { type ListSessionsResult } from './types' import { - checkDaemonHealth, getMacDaemonSystemResolverHealth, getDaemonLaunchIdentity, getProcessStartedAtMs, + healthCheckDaemon, isDaemonStaleForCurrentBundle, killStaleDaemon } from './daemon-health' @@ -174,77 +174,65 @@ async function shouldPreserveDaemonWithLiveSessions( function createOutOfProcessLauncher(runtimeDir: string): DaemonLauncher { return async (socketPath, tokenPath) => { const entryPath = getDaemonEntryPath() - const health = await checkDaemonHealth(socketPath, tokenPath) - if (health !== 'unhealthy') { - if (health === 'pty-spawn-unhealthy') { - if ( - await shouldPreserveDaemonWithLiveSessions( - socketPath, - tokenPath, - 'that cannot spawn new PTYs' + const healthy = await healthCheckDaemon(socketPath, tokenPath) + if (healthy) { + const resolverHealth = await getMacDaemonSystemResolverHealth(socketPath, tokenPath) + if (resolverHealth === 'unhealthy') { + const liveSessionCount = await getAliveDaemonSessionCount(socketPath, tokenPath) + if (liveSessionCount !== 0) { + console.warn( + liveSessionCount === null + ? '[daemon] Preserving daemon with unavailable macOS system resolver because live session state could not be verified' + : `[daemon] Preserving daemon with unavailable macOS system resolver because it owns ${liveSessionCount} live session${liveSessionCount === 1 ? '' : 's'}` ) - ) { return createPreservedDaemonHandle(runtimeDir) } - console.warn('[daemon] Replacing daemon that cannot spawn new PTYs') + console.warn('[daemon] Replacing daemon with unavailable macOS system resolver') await cleanupDaemonForProtocol(runtimeDir, PROTOCOL_VERSION) } else { - const resolverHealth = await getMacDaemonSystemResolverHealth(socketPath, tokenPath) - if (resolverHealth === 'unhealthy') { - const liveSessionCount = await getAliveDaemonSessionCount(socketPath, tokenPath) - if (liveSessionCount !== 0) { - console.warn( - liveSessionCount === null - ? '[daemon] Preserving daemon with unavailable macOS system resolver because live session state could not be verified' - : `[daemon] Preserving daemon with unavailable macOS system resolver because it owns ${liveSessionCount} live session${liveSessionCount === 1 ? '' : 's'}` - ) + // Why: a protocol-healthy daemon can outlive the app bundle that + // launched it. In dev this happens after deleting/rebuilding a + // worktree; in packaged apps it happens when the stable + // /Applications/Orca.app path is replaced during update. + const identity = await getDaemonLaunchIdentity(runtimeDir, socketPath, tokenPath, entryPath) + const stalePackagedBundle = + app.isPackaged && + (await isDaemonStaleForCurrentBundle(runtimeDir, socketPath, tokenPath, app.getVersion())) + if (identity === 'mismatch' || stalePackagedBundle) { + // Why: replacing a healthy daemon kills its child PTYs; defer code + // freshness until no live terminal sessions would be lost. + const replacementLabel = stalePackagedBundle + ? 'launched before the current app bundle was installed' + : 'launched from a different app path' + if (await shouldPreserveDaemonWithLiveSessions(socketPath, tokenPath, replacementLabel)) { return createPreservedDaemonHandle(runtimeDir) } - console.warn('[daemon] Replacing daemon with unavailable macOS system resolver') + console.warn( + stalePackagedBundle + ? '[daemon] Replacing daemon launched before the current app bundle was installed' + : '[daemon] Replacing daemon launched from a different app path' + ) await cleanupDaemonForProtocol(runtimeDir, PROTOCOL_VERSION) } else { - // Why: a protocol-healthy daemon can outlive the app bundle that - // launched it. In dev this happens after deleting/rebuilding a - // worktree; in packaged apps it happens when the stable - // /Applications/Orca.app path is replaced during update. - const identity = await getDaemonLaunchIdentity( - runtimeDir, - socketPath, - tokenPath, - entryPath - ) - const stalePackagedBundle = - app.isPackaged && - (await isDaemonStaleForCurrentBundle( - runtimeDir, - socketPath, - tokenPath, - app.getVersion() - )) - if (identity === 'mismatch' || stalePackagedBundle) { - // Why: replacing a healthy daemon kills its child PTYs; defer code - // freshness until no live terminal sessions would be lost. - const replacementLabel = stalePackagedBundle - ? 'launched before the current app bundle was installed' - : 'launched from a different app path' - if ( - await shouldPreserveDaemonWithLiveSessions(socketPath, tokenPath, replacementLabel) - ) { - return createPreservedDaemonHandle(runtimeDir) - } - console.warn( - stalePackagedBundle - ? '[daemon] Replacing daemon launched before the current app bundle was installed' - : '[daemon] Replacing daemon launched from a different app path' - ) - await cleanupDaemonForProtocol(runtimeDir, PROTOCOL_VERSION) - } else { - // Why: daemon is already running from a previous app session and - // responded to a protocol-level ping. Safe to reuse. - return createPreservedDaemonHandle(runtimeDir) - } + // Why: daemon is already running from a previous app session and + // responded to a protocol-level ping. Safe to reuse. + return createPreservedDaemonHandle(runtimeDir) } } + } else { + // 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. + const liveSessionCount = await getAliveDaemonSessionCount(socketPath, tokenPath) + if (liveSessionCount !== null && liveSessionCount > 0) { + console.warn( + `[daemon] Preserving daemon that failed the health check because it owns ${liveSessionCount} live session${liveSessionCount === 1 ? '' : 's'}` + ) + return createPreservedDaemonHandle(runtimeDir) + } } // Why: a raw socket can outlive a broken or wedged daemon. Kill by PID diff --git a/src/main/daemon/daemon-server.test.ts b/src/main/daemon/daemon-server.test.ts index 7d10ce1f1..b7fbca39e 100644 --- a/src/main/daemon/daemon-server.test.ts +++ b/src/main/daemon/daemon-server.test.ts @@ -198,19 +198,19 @@ describe('DaemonServer', () => { expect(result).toEqual({ pong: true }) }) - it('handles ptySpawnHealth through the daemon process', async () => { - const ptySpawnHealthCheck = vi.fn(async () => {}) - server = new DaemonServer({ - socketPath, - tokenPath, - ptySpawnHealthCheck, - spawnSubprocess: () => createMockSubprocess() - }) - await server.start() + it('replies with an error to unknown request types and keeps serving', async () => { + await startServer() const c = await connectClient() - await expect(c.request('ptySpawnHealth', undefined)).resolves.toEqual({ healthy: true }) - expect(ptySpawnHealthCheck).toHaveBeenCalledOnce() + // Why: older app builds still send the removed ptySpawnHealth probe. + // The daemon must reject it gracefully so a downgraded client lands on + // its session-preserving branch instead of crashing the daemon. + await expect(c.request('ptySpawnHealth', undefined)).rejects.toThrow( + 'Unknown request type: ptySpawnHealth' + ) + await expect(c.request<{ pong: boolean }>('ping', undefined)).resolves.toEqual({ + pong: true + }) }) it('handles systemResolverHealth', async () => { diff --git a/src/main/daemon/daemon-server.ts b/src/main/daemon/daemon-server.ts index 7d9940470..558a41b53 100644 --- a/src/main/daemon/daemon-server.ts +++ b/src/main/daemon/daemon-server.ts @@ -11,7 +11,6 @@ import { TerminalHost } from './terminal-host' import { DaemonStreamDataBatcher } from './daemon-stream-data-batcher' import { readCurrentProcessMacSystemResolverHealth } from '../network/macos-system-resolver-health' import type { SubprocessHandle } from './session' -import { checkPtySpawnHealth } from './pty-subprocess' import { PROTOCOL_VERSION, NOTIFY_PREFIX, @@ -23,7 +22,6 @@ import { export type DaemonServerOptions = { socketPath: string tokenPath: string - ptySpawnHealthCheck?: () => Promise spawnSubprocess: (opts: { sessionId: string cols: number @@ -47,7 +45,6 @@ export class DaemonServer { private host: TerminalHost private socketPath: string private tokenPath: string - private ptySpawnHealthCheck: () => Promise private clients = new Map() private streamDataBatcher = new DaemonStreamDataBatcher((clientId) => this.clients.get(clientId)) @@ -65,7 +62,6 @@ export class DaemonServer { this.tokenPath = opts.tokenPath this.token = randomUUID() this.host = new TerminalHost({ spawnSubprocess: opts.spawnSubprocess }) - this.ptySpawnHealthCheck = opts.ptySpawnHealthCheck ?? checkPtySpawnHealth } async start(): Promise { @@ -379,10 +375,6 @@ export class DaemonServer { case 'systemResolverHealth': return { health: await readCurrentProcessMacSystemResolverHealth() } - case 'ptySpawnHealth': - await this.ptySpawnHealthCheck() - return { healthy: true } - case 'shutdown': if (request.payload.killSessions) { this.host.dispose() diff --git a/src/main/daemon/pty-subprocess.test.ts b/src/main/daemon/pty-subprocess.test.ts index ad62e6272..a9a5fef32 100644 --- a/src/main/daemon/pty-subprocess.test.ts +++ b/src/main/daemon/pty-subprocess.test.ts @@ -1,19 +1,13 @@ /* oxlint-disable max-lines -- Why: exercises full PTY subprocess surface (spawn setup, signal routing, data events, platform-specific shell configs, and Windows PowerShell implementations) with co-located test scenarios to prevent fixture drift. */ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -import { mkdtempSync, realpathSync, rmSync, writeFileSync } from 'fs' +import { mkdtempSync, realpathSync, rmSync } from 'fs' import { tmpdir } from 'os' import { join } from 'path' import type * as LocalPtyUtils from '../providers/local-pty-utils' -const { - spawnMock, - isPwshAvailableMock, - validateWorkingDirectoryMock, - getNodePtySpawnHelperCandidatesMock -} = vi.hoisted(() => ({ +const { spawnMock, isPwshAvailableMock, validateWorkingDirectoryMock } = vi.hoisted(() => ({ spawnMock: vi.fn(), isPwshAvailableMock: vi.fn(), - getNodePtySpawnHelperCandidatesMock: vi.fn(), validateWorkingDirectoryMock: vi.fn((cwd: string) => { if (cwd.includes('definitely-missing')) { throw new Error( @@ -35,12 +29,11 @@ vi.mock('../providers/local-pty-utils', async (importOriginal) => { const actual = await importOriginal() return { ...actual, - getNodePtySpawnHelperCandidates: getNodePtySpawnHelperCandidatesMock, validateWorkingDirectory: validateWorkingDirectoryMock } }) -import { checkPtySpawnHealth, createPtySubprocess } from './pty-subprocess' +import { createPtySubprocess } from './pty-subprocess' const ORCA_SHELL_WRAPPER_ENV = [ 'ORCA_ATTRIBUTION_SHIM_DIR', @@ -86,10 +79,6 @@ describe('createPtySubprocess', () => { isPwshAvailableMock.mockReturnValue(false) previousUserDataPath = process.env.ORCA_USER_DATA_PATH userDataPath = mkdtempSync(join(tmpdir(), 'daemon-pty-subprocess-test-')) - const spawnHelperPath = join(userDataPath, 'spawn-helper') - writeFileSync(spawnHelperPath, '') - getNodePtySpawnHelperCandidatesMock.mockReset() - getNodePtySpawnHelperCandidatesMock.mockReturnValue([spawnHelperPath]) process.env.ORCA_USER_DATA_PATH = userDataPath for (const key of ORCA_SHELL_WRAPPER_ENV) { savedWrapperEnv[key] = process.env[key] @@ -180,54 +169,6 @@ describe('createPtySubprocess', () => { ) }) - it('checks macOS PTY spawn health with a short-lived shell', async () => { - const proc = mockPtyProcess() - spawnMock.mockReturnValue(proc) - const platform = Object.getOwnPropertyDescriptor(process, 'platform') - Object.defineProperty(process, 'platform', { value: 'darwin' }) - - try { - const result = checkPtySpawnHealth() - proc._simulateExit(0) - await expect(result).resolves.toBeUndefined() - expect(proc.kill).not.toHaveBeenCalled() - } finally { - if (platform) { - Object.defineProperty(process, 'platform', platform) - } - } - - expect(spawnMock).toHaveBeenCalledWith( - '/bin/sh', - ['-c', 'exit 0'], - expect.objectContaining({ - cols: 2, - rows: 1, - cwd: userDataPath, - name: 'xterm-256color' - }) - ) - }) - - it('surfaces stale node-pty helper failures during macOS PTY spawn health', async () => { - spawnMock.mockImplementation(() => { - throw new Error( - "node-pty: posix_spawn failed: ENOENT (errno 2, No such file or directory) - helper='/tmp/deleted/spawn-helper'" - ) - }) - const platform = Object.getOwnPropertyDescriptor(process, 'platform') - Object.defineProperty(process, 'platform', { value: 'darwin' }) - - try { - await expect(checkPtySpawnHealth()).rejects.toThrow('Daemon failed to spawn shell "/bin/sh"') - await expect(checkPtySpawnHealth()).rejects.toThrow('posix_spawn failed: ENOENT') - } finally { - if (platform) { - Object.defineProperty(process, 'platform', platform) - } - } - }) - it('returns a SubprocessHandle with correct pid', () => { const proc = mockPtyProcess(42) spawnMock.mockReturnValue(proc) diff --git a/src/main/daemon/pty-subprocess.ts b/src/main/daemon/pty-subprocess.ts index 53287122a..8f6fb96a3 100644 --- a/src/main/daemon/pty-subprocess.ts +++ b/src/main/daemon/pty-subprocess.ts @@ -29,7 +29,6 @@ import { isWindowsGitBashShellPath, resolveWindowsGitBashShellPath } from '../gi import { WINDOWS_GIT_BASH_SHELL } from '../../shared/windows-terminal-shell' const PANE_IDENTITY_ENV_KEYS = ['ORCA_PANE_KEY', 'ORCA_TAB_ID', 'ORCA_WORKTREE_ID'] as const -const PTY_SPAWN_HEALTH_TIMEOUT_MS = 2_000 export type PtySubprocessOptions = { sessionId: string @@ -232,75 +231,6 @@ function formatPtySpawnError(err: unknown, shellPath: string, spawnCwd: string): return formatted } -export async function checkPtySpawnHealth(): Promise { - if (process.platform !== 'darwin') { - return - } - - ensureNodePtySpawnHelperExecutable() - preflightMacNodePtySpawnEnvironment() - - const cwd = isExistingDirectory(process.env.ORCA_USER_DATA_PATH) - ? process.env.ORCA_USER_DATA_PATH - : getDefaultCwd() - - let proc: pty.IPty - try { - proc = pty.spawn('/bin/sh', ['-c', 'exit 0'], { - name: 'xterm-256color', - cols: 2, - rows: 1, - cwd, - env: { - ...process.env, - TERM: 'xterm-256color' - } - }) - } catch (err) { - throw formatPtySpawnError(err, '/bin/sh', cwd) - } - - await new Promise((resolve, reject) => { - let settled = false - let exitDisposable: { dispose(): void } | undefined - const finish = (error?: Error, opts?: { kill?: boolean }): void => { - if (settled) { - return - } - settled = true - clearTimeout(timer) - exitDisposable?.dispose() - if (opts?.kill) { - try { - proc.kill() - } catch { - // Best-effort cleanup for a short-lived health probe. - } - } - if (error) { - reject(error) - return - } - resolve() - } - const timer = setTimeout(() => { - finish(new Error(`PTY spawn health check timed out after ${PTY_SPAWN_HEALTH_TIMEOUT_MS}ms`), { - kill: true - }) - }, PTY_SPAWN_HEALTH_TIMEOUT_MS) - - // Why: ping only proves the daemon protocol is alive. A real short-lived - // PTY spawn catches stale node-pty helper paths captured by this process. - exitDisposable = proc.onExit(({ exitCode }) => { - if (exitCode === 0) { - finish() - return - } - finish(new Error(`PTY spawn health check exited with code ${exitCode}`)) - }) - }) -} - function normalizeForegroundProcessName(processName: string | null | undefined): string | null { const trimmed = processName?.trim().replace(/^["']|["']$/g, '') ?? '' if (!trimmed || trimmed === 'xterm-256color') { diff --git a/src/main/daemon/slow-daemon-session-verification.test.ts b/src/main/daemon/slow-daemon-session-verification.test.ts new file mode 100644 index 000000000..8c3ad71c0 --- /dev/null +++ b/src/main/daemon/slow-daemon-session-verification.test.ts @@ -0,0 +1,126 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { connect, createServer, type Server, type Socket } from 'net' +import { tmpdir } from 'os' +import { join } from 'path' +import { mkdtempSync, rmSync } from 'fs' +import { DaemonServer } from './daemon-server' +import { DaemonClient } from './client' +import { healthCheckDaemon } from './daemon-health' +import type { ListSessionsResult } from './types' +import type { SubprocessHandle } from './session' + +// Why: terminals were lost after app updates because a busy machine could +// time out the 3s startup health check against a daemon that was alive and +// owning sessions, and the unhealthy path killed it. The fix re-verifies +// with listSessions, which has far larger budgets (5s hello, 30s request). +// This test reproduces the production asymmetry against a REAL daemon by +// inserting a response delay that exceeds the health-check budget but fits +// the verification budgets, and asserts the guard's two inputs disagree the +// way the fix depends on. +const RESPONSE_DELAY_MS = 3_500 + +function createMockSubprocess(): SubprocessHandle { + return { + pid: 55555, + getForegroundProcess: vi.fn(() => null), + write: vi.fn(), + resize: vi.fn(), + kill: vi.fn(), + forceKill: vi.fn(), + signal: vi.fn(), + onData: vi.fn(), + onExit: vi.fn(), + dispose: vi.fn() + } +} + +/** Forwards client bytes to the daemon immediately, but delays every daemon + * response so each round-trip looks like a daemon under heavy load. */ +function startDelayProxy(listenPath: string, upstreamPath: string): Server { + const proxy = createServer((clientSocket: Socket) => { + const upstream = connect(upstreamPath) + clientSocket.on('data', (chunk) => upstream.write(chunk)) + upstream.on('data', (chunk) => { + setTimeout(() => { + if (!clientSocket.destroyed) { + clientSocket.write(chunk) + } + }, RESPONSE_DELAY_MS) + }) + const teardown = (): void => { + clientSocket.destroy() + upstream.destroy() + } + clientSocket.on('close', teardown) + clientSocket.on('error', teardown) + upstream.on('close', () => { + setTimeout(teardown, RESPONSE_DELAY_MS) + }) + upstream.on('error', teardown) + }) + proxy.listen(listenPath) + return proxy +} + +describe('slow daemon session verification', () => { + let dir: string + let daemonSocketPath: string + let proxySocketPath: string + let tokenPath: string + let server: DaemonServer + let proxy: Server + const clients: DaemonClient[] = [] + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'daemon-slow-verification-test-')) + daemonSocketPath = join(dir, 'daemon.sock') + proxySocketPath = join(dir, 'proxy.sock') + tokenPath = join(dir, 'daemon.token') + }) + + afterEach(async () => { + for (const client of clients.splice(0)) { + client.disconnect() + } + await new Promise((resolve) => proxy?.close(() => resolve())) + await server?.shutdown() + rmSync(dir, { recursive: true, force: true }) + }) + + it( + 'fails the 3s health check against a slow daemon while listSessions still verifies its live session', + { timeout: 60_000 }, + async () => { + server = new DaemonServer({ + socketPath: daemonSocketPath, + tokenPath, + spawnSubprocess: () => createMockSubprocess() + }) + await server.start() + + const directClient = new DaemonClient({ socketPath: daemonSocketPath, tokenPath }) + clients.push(directClient) + await directClient.ensureConnected() + await directClient.request('createOrAttach', { + sessionId: 'wt-1@@live-session', + cols: 80, + rows: 24 + }) + + proxy = startDelayProxy(proxySocketPath, daemonSocketPath) + + // The exact pre-fix kill trigger: the daemon is alive but too slow for + // the health-check budget. + await expect(healthCheckDaemon(proxySocketPath, tokenPath)).resolves.toBe(false) + + // The fix's re-verification against the SAME slow daemon: the larger + // client budgets absorb the latency and prove the session is alive. + const verificationClient = new DaemonClient({ socketPath: proxySocketPath, tokenPath }) + clients.push(verificationClient) + await verificationClient.ensureConnected() + const result = await verificationClient.request('listSessions', undefined) + const liveSessionCount = result.sessions.filter((session) => session.isAlive).length + expect(liveSessionCount).toBe(1) + } + ) +}) diff --git a/src/main/daemon/types.ts b/src/main/daemon/types.ts index abac7f20d..924e322f8 100644 --- a/src/main/daemon/types.ts +++ b/src/main/daemon/types.ts @@ -182,11 +182,6 @@ export type SystemResolverHealthRequest = { type: 'systemResolverHealth' } -export type PtySpawnHealthRequest = { - id: string - type: 'ptySpawnHealth' -} - export type GetSnapshotRequest = { id: string type: 'getSnapshot' @@ -210,7 +205,6 @@ export type DaemonRequest = | ShutdownRequest | PingRequest | SystemResolverHealthRequest - | PtySpawnHealthRequest | GetSnapshotRequest // ─── RPC Responses (Daemon → Client, on control socket) ──────────── diff --git a/tests/e2e/daemon-slow-health-check-preservation.spec.ts b/tests/e2e/daemon-slow-health-check-preservation.spec.ts new file mode 100644 index 000000000..8a825c85c --- /dev/null +++ b/tests/e2e/daemon-slow-health-check-preservation.spec.ts @@ -0,0 +1,135 @@ +import { existsSync, readFileSync } from 'fs' +import path from 'path' +import type { ElectronApplication } from '@stablyai/playwright-test' +import { test, expect } from './helpers/orca-app' +import { TEST_REPO_PATH_FILE } from './global-setup' +import { + discoverActivePtyId, + execInTerminal, + getTerminalContent, + waitForActiveTerminalManager, + waitForPaneCount, + waitForTerminalOutput +} from './helpers/terminal' +import { ensureTerminalVisible, waitForActiveWorktree, waitForSessionReady } from './helpers/store' +import { attachRepoAndOpenTerminal, createRestartSession } from './helpers/orca-restart' +import { PROTOCOL_VERSION } from '../../src/main/daemon/types' +import { PTY_SESSION_ID_SEPARATOR } from '../../src/shared/pty-session-id-format' + +// Why: must land after the relaunched app's 3s daemon health check has timed +// out (so the unhealthy guard runs) but before the guard's 5s client hello +// budget expires. Daemon init starts within the first ~2s of main startup. +const RESUME_DAEMON_AFTER_MS = 6_500 + +function readDaemonPid(userDataDir: string): number { + const raw = readFileSync( + path.join(userDataDir, 'daemon', `daemon-v${PROTOCOL_VERSION}.pid`), + 'utf8' + ) + const parsed = JSON.parse(raw) as { pid?: unknown } + if (typeof parsed.pid !== 'number') { + throw new Error(`Daemon pid file did not contain a numeric pid: ${raw}`) + } + return parsed.pid +} + +test.describe.configure({ mode: 'serial' }) + +test('preserves a live daemon PTY when the daemon is too slow for the startup health check', async (// oxlint-disable-next-line no-empty-pattern -- Playwright's second fixture arg is testInfo; the first must be an object destructure to opt out of the default fixture set. +{}, testInfo) => { + const repoPath = readFileSync(TEST_REPO_PATH_FILE, 'utf-8').trim() + if (!repoPath || !existsSync(repoPath)) { + test.skip(true, 'Global setup did not produce a seeded test repo') + return + } + test.skip(process.platform === 'win32', 'SIGSTOP/SIGCONT are POSIX-only') + + const session = createRestartSession(testInfo) + let firstApp: ElectronApplication | null = null + let secondApp: ElectronApplication | null = null + let daemonPid: number | null = null + + try { + const firstLaunch = await session.launch() + firstApp = firstLaunch.app + const page = await firstApp.firstWindow() + const worktreeId = await attachRepoAndOpenTerminal(page, repoPath) + await waitForSessionReady(page) + await waitForActiveWorktree(page) + await ensureTerminalVisible(page) + await waitForActiveTerminalManager(page, 30_000) + await waitForPaneCount(page, 1, 30_000) + const ptyId = await discoverActivePtyId(page) + expect(ptyId).toContain(PTY_SESSION_ID_SEPARATOR) + + const marker = `DAEMON_SLOW_HEALTH_PRESERVE_${Date.now()}` + await execInTerminal(firstLaunch.page, ptyId, `echo ${marker}`) + await waitForTerminalOutput(firstLaunch.page, marker) + + daemonPid = readDaemonPid(session.userDataDir) + + await session.close(firstApp) + firstApp = null + + // Why: a stopped daemon still accepts socket connections at the kernel + // level but answers nothing — the same observable behavior as a daemon + // that is too busy to respond within the health-check budget. + process.kill(daemonPid, 'SIGSTOP') + + const stderrLines: string[] = [] + const resumeTimer = setTimeout(() => { + if (daemonPid !== null) { + process.kill(daemonPid, 'SIGCONT') + } + }, RESUME_DAEMON_AFTER_MS) + try { + const secondLaunch = await session.launch() + secondApp = secondLaunch.app + secondApp.process().stderr?.on('data', (chunk: Buffer) => { + stderrLines.push(chunk.toString()) + }) + + await waitForSessionReady(secondLaunch.page) + await expect + .poll( + async () => secondLaunch.page.evaluate(() => window.__store?.getState().activeWorktreeId), + { timeout: 15_000 } + ) + .toBe(worktreeId) + await ensureTerminalVisible(secondLaunch.page) + await waitForActiveTerminalManager(secondLaunch.page, 30_000) + await waitForPaneCount(secondLaunch.page, 1, 30_000) + await waitForTerminalOutput(secondLaunch.page, marker, 20_000) + + // The guard path must actually have run: the daemon failed the health + // check and was preserved because its live session was verified. + await expect + .poll(() => stderrLines.join(''), { timeout: 10_000 }) + .toContain('Preserving daemon that failed the health check') + expect(readDaemonPid(session.userDataDir)).toBe(daemonPid) + // Why: a killed daemon cold-restores scrollback from history, so the + // marker text alone cannot distinguish a live session from a dead one. + // The restore banner only appears for cold-restored (dead) sessions. + expect(await getTerminalContent(secondLaunch.page)).not.toContain('--- session restored ---') + } finally { + clearTimeout(resumeTimer) + } + } finally { + if (daemonPid !== null) { + try { + // Idempotent: ensures the daemon is resumable for harness cleanup even + // if the test failed before the resume timer fired. + process.kill(daemonPid, 'SIGCONT') + } catch { + // Daemon already gone + } + } + if (secondApp) { + await session.close(secondApp) + } + if (firstApp) { + await session.close(firstApp) + } + await session.dispose() + } +})