diff --git a/config/scripts/daemon-replacement-live-agent-pty-preservation-repro.mjs b/config/scripts/daemon-replacement-live-agent-pty-preservation-repro.mjs index 275a531b75e..8d99c0e44df 100644 --- a/config/scripts/daemon-replacement-live-agent-pty-preservation-repro.mjs +++ b/config/scripts/daemon-replacement-live-agent-pty-preservation-repro.mjs @@ -595,11 +595,11 @@ function checkLauncherHoldsOccupiedDaemon() { // A daemon that did complete a hello is adoptable, so it must not be routed to a mode that // never adopts. const unverifiableGuard = occupiedBlock.text.match( - /if\s*\(\s*health\s*===\s*'unreachable'\s*&&\s*occupancy\.liveSessions\s*===\s*null\s*\)\s*\{/ + /if\s*\(\s*health\s*===\s*'rejected'\s*\|\|\s*occupancy\.liveSessions\s*===\s*null\s*\)\s*\{/ ) assert( unverifiableGuard !== null, - `${relativePath}:${occupiedLine} does not gate the hold on an unreachable daemon with an unverifiable (null) session count` + `${relativePath}:${occupiedLine} does not gate the hold on a daemon that cannot be adopted (rejected, or an unverifiable session count)` ) const unverifiableBlock = extractBlock( occupiedBlock.text, diff --git a/src/main/daemon/daemon-init.test.ts b/src/main/daemon/daemon-init.test.ts index 78b48330dc6..70580c339a3 100644 --- a/src/main/daemon/daemon-init.test.ts +++ b/src/main/daemon/daemon-init.test.ts @@ -2,7 +2,8 @@ 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' +import { WEDGED_DAEMON_GRACE_BUDGET_MS, WEDGED_DAEMON_GRACE_RETRIES } from './daemon-init' +import { LOCAL_PTY_STARTUP_FAIL_OPEN_TIMEOUT_MS } from '../startup/first-window-startup-services' const FAKE_USER_DATA_PATH = '/fake/userData' const FAKE_RUNTIME_DIR = join(FAKE_USER_DATA_PATH, 'daemon') @@ -3244,6 +3245,54 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { } }) + it('adopts a PTY-spawn-unhealthy daemon in degraded mode instead of merely holding it', async () => { + // Why not 'held': 'pty-spawn-unhealthy' is only reachable after a successful hello, so this + // daemon CAN be adopted — it just cannot open new PTYs. Holding it (which never adopts) + // because the occupancy count happened to come from the process table strands its sessions. + const mod = await importFresh() + await mod.initDaemonPtyProvider() + + const answeringDefault = function MockDaemonClient() { + return { + ensureConnected: vi.fn(async () => {}), + request: vi.fn(async () => ({ sessions: [] })), + disconnect: vi.fn() + } + } + // Hello succeeds; only listSessions is unanswerable, so occupancy falls back to the table. + daemonClientMock.mockImplementation(function MockPtySpawnUnhealthyClient() { + return { + ensureConnected: vi.fn(async () => {}), + request: vi.fn(async () => { + throw new Error('listSessions timed out') + }), + disconnect: vi.fn() + } + }) + readVerifiedDaemonPidMock.mockResolvedValue({ pid: 4242 }) + inspectDaemonPtyOwnershipMock.mockResolvedValue('owns-live-ptys') + + const launcher = spawnerInstances[0].launcher as ( + socketPath: string, + tokenPath: string + ) => Promise<{ shutdown(): Promise; mode?: string }> + checkDaemonHealthMock.mockResolvedValueOnce('pty-spawn-unhealthy') + probeSocketExistsMock.mockReturnValue(true) + netConnectMock.mockImplementation(stubAliveSocketConnect) + + try { + const handle = await launcher('/fake/socket', '/fake/token') + + expect(handle.mode).toBe('degraded-new-pty-fallback') + expect(killStaleDaemonMock).not.toHaveBeenCalled() + expect(forkMock).not.toHaveBeenCalled() + } finally { + daemonClientMock.mockImplementation(answeringDefault) + readVerifiedDaemonPidMock.mockResolvedValue(null) + inspectDaemonPtyOwnershipMock.mockResolvedValue('unknown') + } + }) + it('keeps a daemon handle when the preserved daemon is too wedged to be adopted', async () => { // Why: adoption needs a hello, which is exactly what a daemon wedged enough to be // preserved cannot answer. Throwing here would abort initDaemonPtyProvider, leaving no @@ -3291,11 +3340,128 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { } }) + it('holds a hello-rejected daemon that owns live terminals rather than adopting it', async () => { + // Why: 'rejected' means it answered and refused the handshake, so adoption can never + // succeed. Falling through to preserveDaemon() would throw and cost the app its daemon + // entirely — and killing it would end agents that are still running. + const mod = await importFresh() + await mod.initDaemonPtyProvider() + + const answeringDefault = function MockDaemonClient() { + return { + ensureConnected: vi.fn(async () => {}), + request: vi.fn(async () => ({ sessions: [] })), + disconnect: vi.fn() + } + } + daemonClientMock.mockImplementation(function MockRejectingDaemonClient() { + return { + ensureConnected: vi.fn(async () => { + throw new Error('hello refused') + }), + getDaemonIdentity: vi.fn(readLaunchedDaemonIdentity), + request: vi.fn(), + disconnect: vi.fn() + } + }) + readVerifiedDaemonPidMock.mockResolvedValue({ pid: 4242 }) + inspectDaemonPtyOwnershipMock.mockResolvedValue('owns-live-ptys') + + const launcher = spawnerInstances[0].launcher as ( + socketPath: string, + tokenPath: string + ) => Promise<{ shutdown(): Promise; mode?: string }> + checkDaemonHealthMock.mockResolvedValueOnce('rejected') + probeSocketExistsMock.mockReturnValue(true) + netConnectMock.mockImplementation(stubAliveSocketConnect) + + try { + const handle = await launcher('/fake/socket', '/fake/token') + + expect(handle.mode).toBe('held') + expect(killStaleDaemonMock).not.toHaveBeenCalled() + expect(forkMock).not.toHaveBeenCalled() + } finally { + daemonClientMock.mockImplementation(answeringDefault) + readVerifiedDaemonPidMock.mockResolvedValue(null) + inspectDaemonPtyOwnershipMock.mockResolvedValue('unknown') + } + }) + it('grace budget is generous enough to ride out a ~60s transient wedge', () => { // Why: each probe waits the client's 5s hello timeout, so 1 + 11 probes ≈ 60s of drain grace; don't cut without telemetry. expect(WEDGED_DAEMON_GRACE_RETRIES).toBeGreaterThanOrEqual(11) }) + it('leaves headroom under the startup fail-open cap', () => { + // Why a wall-clock budget at all: the retry count does not bound the grace window, because + // each probe waits the client's hello timeout. Startup abandons the daemon provider entirely + // at the fail-open cap, so overrunning it trades a wedged daemon for no daemon at all. + expect(WEDGED_DAEMON_GRACE_BUDGET_MS).toBeLessThan(LOCAL_PTY_STARTUP_FAIL_OPEN_TIMEOUT_MS) + }) + + it('stops grace-retrying when the wall-clock budget runs out, with retries still left', async () => { + const mod = await importFresh() + await mod.initDaemonPtyProvider() + + const answeringDefault = function MockDaemonClient() { + return { + ensureConnected: vi.fn(async () => {}), + request: vi.fn(async () => ({ sessions: [] })), + disconnect: vi.fn() + } + } + // Permanently wedged: without the budget this drains all WEDGED_DAEMON_GRACE_RETRIES probes. + daemonClientMock.mockImplementation(function MockWedgedDaemonClient() { + 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') + probeSocketExistsMock.mockReturnValue(true) + netConnectMock.mockImplementation(stubAliveSocketConnect) + forkMock.mockImplementationOnce(() => { + throw new Error('stop after replacement decision') + }) + + // Stand in for the hello timeout each probe really waits out: mocked probes return instantly, + // so only a moving clock can exercise the budget without a slow (or flaky) real wait. + const startMs = Date.now() + let elapsedMs = 0 + const nowSpy = vi.spyOn(Date, 'now').mockImplementation(() => { + const value = startMs + elapsedMs + elapsedMs += WEDGED_DAEMON_GRACE_BUDGET_MS / 3 + return value + }) + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + try { + await expect(launcher('/fake/socket', '/fake/token')).rejects.toThrow( + 'stop after replacement decision' + ) + + const verdict = warnSpy.mock.calls + .map((call) => String(call[0])) + .find((message) => message.includes('graceRetries=')) + expect(verdict).toBeDefined() + const graceRetries = Number(/graceRetries=(\d+)/.exec(verdict as string)?.[1]) + expect(graceRetries).toBeLessThan(WEDGED_DAEMON_GRACE_RETRIES) + } finally { + nowSpy.mockRestore() + warnSpy.mockRestore() + daemonClientMock.mockImplementation(answeringDefault) + } + }) + it('preserves a daemon that stays wedged until the LAST allowed grace retry', async () => { // Why: daemon drains only on the last allowed probe (1 + WEDGED_DAEMON_GRACE_RETRIES) — must be preserved, not replaced. const mod = await importFresh() diff --git a/src/main/daemon/daemon-init.ts b/src/main/daemon/daemon-init.ts index d4399b75a14..e0c210963de 100644 --- a/src/main/daemon/daemon-init.ts +++ b/src/main/daemon/daemon-init.ts @@ -633,12 +633,14 @@ function createOutOfProcessLauncher( ) return preserveDaemon('degraded-new-pty-fallback') } - // Why hold rather than adopt: no hello ever completed and only the process table - // could answer, so adoption would fail on the same silence. Holding keeps its - // terminals alive and routes fresh ones locally until it recovers or is restarted. - if (health === 'unreachable' && occupancy.liveSessions === null) { + // Why hold rather than adopt: adoption opens a hello, and neither of these daemons + // can complete one — 'rejected' answered and refused, and a count only the process + // table could supply means nothing answered at all. Attempting it would throw and + // cost the app its daemon entirely. Holding keeps their terminals alive and routes + // fresh ones locally until they recover or the user restarts them. + if (health === 'rejected' || occupancy.liveSessions === null) { console.warn( - '[daemon] DEGRADED MODE: holding a daemon that cannot report its sessions but still owns live terminal processes. Killing it would end them; fresh terminals run on the local provider WITHOUT daemon persistence until it recovers or you restart it (Manage Sessions → Restart).' + `[daemon] DEGRADED MODE: holding a daemon that cannot be adopted (health=${health}) but still owns ${owned}. Killing it would end them; fresh terminals run on the local provider WITHOUT daemon persistence until it recovers or you restart it (Manage Sessions → Restart).` ) return holdIncumbentDaemon() } diff --git a/src/main/daemon/daemon-live-pty-evidence.test.ts b/src/main/daemon/daemon-live-pty-evidence.test.ts index 31bc40e4e28..14b21095474 100644 --- a/src/main/daemon/daemon-live-pty-evidence.test.ts +++ b/src/main/daemon/daemon-live-pty-evidence.test.ts @@ -109,6 +109,47 @@ describe('inspectDaemonPtyOwnership on POSIX', () => { ).resolves.toBe('no-live-ptys') }) + it("excludes the daemon's own PTY-spawn probe, which forkpty also makes a session leader", async () => { + // Why the stat flag is not enough: the daemon opens this PTY itself, so a daemon hosting + // zero user terminals would be held forever on the strength of its own stuck health check. + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'darwin', + readPosixProcessTable: posixTable([ + daemonRow, + row(101, DAEMON_PID, { stat: 'Ss', command: '/bin/sh -c exit 0' }) + ]) + }) + ).resolves.toBe('no-live-ptys') + }) + + it("still counts a real terminal sitting beside the daemon's own probe", async () => { + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'darwin', + readPosixProcessTable: posixTable([ + daemonRow, + row(101, DAEMON_PID, { stat: 'Ss', command: '/bin/sh -c exit 0' }), + row(202, DAEMON_PID, { stat: 'Ss+', command: 'claude' }) + ]) + }) + ).resolves.toBe('owns-live-ptys') + }) + + it('does not exclude an agent whose command merely contains a probe command', async () => { + // Exact match, not prefix or substring: `sh -c` payloads are user-supplied, and treating one + // as the daemon's own probe discards proof that killing the daemon would end real work. + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'darwin', + readPosixProcessTable: posixTable([ + daemonRow, + row(101, DAEMON_PID, { stat: 'Ss', command: '/bin/sh -c exit 0 && claude' }) + ]) + }) + ).resolves.toBe('owns-live-ptys') + }) + it('counts a session leader reached through a non-session-leader hop', async () => { await expect( inspectDaemonPtyOwnership(DAEMON_PID, { @@ -194,6 +235,43 @@ describe('inspectDaemonPtyOwnership on win32', () => { ).resolves.toBe('no-live-ptys') }) + it('ignores the conpty warmup processes the daemon spawns for itself', async () => { + // Casing and padding vary with how the OS recorded the command line, and this branch counts + // *any* descendant — so without the match a daemon hosting nothing would look occupied. + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'win32', + queryWindowsDescendants: async () => [ + { ...windowsCandidate(101), command: ' CMD.EXE /C EXIT ' }, + { ...windowsCandidate(102), command: 'Cmd /C Exit' } + ] + }) + ).resolves.toBe('no-live-ptys') + }) + + it('still counts a hosted terminal sitting beside the conpty warmup', async () => { + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'win32', + queryWindowsDescendants: async () => [ + { ...windowsCandidate(101), command: 'cmd.exe /c exit' }, + { ...windowsCandidate(202), command: 'claude' } + ] + }) + ).resolves.toBe('owns-live-ptys') + }) + + it('does not exclude a command that merely starts with the warmup command', async () => { + await expect( + inspectDaemonPtyOwnership(DAEMON_PID, { + platform: 'win32', + queryWindowsDescendants: async () => [ + { ...windowsCandidate(101), command: 'cmd.exe /c exit && npm run dev' } + ] + }) + ).resolves.toBe('owns-live-ptys') + }) + it('reports unknown when enumeration failed or the root was absent', async () => { await expect( inspectDaemonPtyOwnership(DAEMON_PID, { diff --git a/src/main/daemon/daemon-occupancy.test.ts b/src/main/daemon/daemon-occupancy.test.ts index 4040885c16d..27d1b22feca 100644 --- a/src/main/daemon/daemon-occupancy.test.ts +++ b/src/main/daemon/daemon-occupancy.test.ts @@ -1,5 +1,10 @@ import { describe, expect, it, vi } from 'vitest' -import { resolveDaemonOccupancy, type DaemonOccupancyDeps } from './daemon-occupancy' +import { + raiseOccupancyWithProcessEvidence, + resolveDaemonOccupancy, + type DaemonOccupancy, + type DaemonOccupancyDeps +} from './daemon-occupancy' import type { inspectDaemonPtyOwnership } from './daemon-live-pty-evidence' const SOCKET_PATH = '/tmp/orca-daemon.sock' @@ -108,6 +113,83 @@ describe('resolveDaemonOccupancy when the daemon could not answer', () => { }) }) +describe('raiseOccupancyWithProcessEvidence', () => { + const UNKNOWN: DaemonOccupancy = { state: 'unknown', liveSessions: null } + + it('raises an unanswered verdict to occupied, with no count to report', async () => { + const inspectPtyOwnership = ownershipIs('owns-live-ptys') + + await expect( + raiseOccupancyWithProcessEvidence(UNKNOWN, DAEMON_PID, { inspectPtyOwnership }) + ).resolves.toEqual({ state: 'occupied', liveSessions: null }) + expect(inspectPtyOwnership).toHaveBeenCalledWith(DAEMON_PID) + }) + + it('leaves an unanswered verdict unknown — never empty — when the table shows no live PTYs', async () => { + // The whole point of the raise-only contract: ps can miss PTYs it never observed, so an + // empty-looking table is not permission to kill a daemon that could not answer for itself. + await expect( + raiseOccupancyWithProcessEvidence(UNKNOWN, DAEMON_PID, { + inspectPtyOwnership: ownershipIs('no-live-ptys') + }) + ).resolves.toEqual(UNKNOWN) + }) + + it('leaves an unanswered verdict unchanged when the table could not be read', async () => { + await expect( + raiseOccupancyWithProcessEvidence(UNKNOWN, DAEMON_PID, { + inspectPtyOwnership: ownershipIs('unknown') + }) + ).resolves.toEqual(UNKNOWN) + }) + + it('does not inspect an unverified pid', async () => { + // A pid we could not tie back to this daemon may have been recycled; its children are + // evidence about the wrong process tree. + const inspectPtyOwnership = ownershipIs('owns-live-ptys') + + await expect( + raiseOccupancyWithProcessEvidence(UNKNOWN, null, { inspectPtyOwnership }) + ).resolves.toEqual(UNKNOWN) + expect(inspectPtyOwnership).not.toHaveBeenCalled() + }) + + it('returns an empty verdict untouched, without consulting the process table', async () => { + // 'empty' came from the daemon itself and is the one state that licenses a kill. Re-asking + // the table could only lower it back to 'unknown', discarding an IPC-proven answer. + const inspectPtyOwnership = ownershipIs('owns-live-ptys') + + await expect( + raiseOccupancyWithProcessEvidence({ state: 'empty', liveSessions: 0 }, DAEMON_PID, { + inspectPtyOwnership + }) + ).resolves.toEqual({ state: 'empty', liveSessions: 0 }) + expect(inspectPtyOwnership).not.toHaveBeenCalled() + }) + + it('keeps the counted sessions of an already-occupied verdict', async () => { + // Raising an answered count to the countless 'occupied' would lose what the daemon reported. + const inspectPtyOwnership = ownershipIs('no-live-ptys') + + await expect( + raiseOccupancyWithProcessEvidence({ state: 'occupied', liveSessions: 3 }, DAEMON_PID, { + inspectPtyOwnership + }) + ).resolves.toEqual({ state: 'occupied', liveSessions: 3 }) + expect(inspectPtyOwnership).not.toHaveBeenCalled() + }) + + it('leaves the verdict unchanged when the inspector throws', async () => { + await expect( + raiseOccupancyWithProcessEvidence(UNKNOWN, DAEMON_PID, { + inspectPtyOwnership: vi.fn(async () => { + throw new Error('process table read exploded') + }) + }) + ).resolves.toEqual(UNKNOWN) + }) +}) + describe('resolveDaemonOccupancy when an injected dep throws', () => { it('degrades an inspector rejection to unknown', async () => { // Why: a question that could not be asked is exactly what the residual is for. Letting