diff --git a/src/main/daemon/daemon-protocol-version.ts b/src/main/daemon/daemon-protocol-version.ts index 61cabf2fc15..fee06b235f2 100644 --- a/src/main/daemon/daemon-protocol-version.ts +++ b/src/main/daemon/daemon-protocol-version.ts @@ -6,6 +6,8 @@ export const STABLE_PANE_ATTACH_ONLY_DAEMON_PROTOCOL_VERSION = 31 export const HISTORY_SEED_TRANSFER_PROTOCOL_VERSION = 30 export const COMPLETION_PROCESS_INSPECTION_PROTOCOL_VERSION = 27 export const GET_FOREGROUND_PROCESS_PROTOCOL_VERSION = 11 +// Why: `getSize` landed in v18; older daemons reject it as an unknown request type. +export const GET_SIZE_PROTOCOL_VERSION = 18 export const PTY_STARTUP_INGRESS_PROTOCOL_VERSION = 25 export const AGENT_SESSION_CLAIM_DAEMON_PROTOCOL_VERSION = 26 export const AGENT_SESSION_CREATE_OPERATION_DAEMON_PROTOCOL_VERSION = 26 diff --git a/src/main/daemon/daemon-pty-adapter.test.ts b/src/main/daemon/daemon-pty-adapter.test.ts index 5dc807459be..84147ddb890 100644 --- a/src/main/daemon/daemon-pty-adapter.test.ts +++ b/src/main/daemon/daemon-pty-adapter.test.ts @@ -5,10 +5,11 @@ import { join } from 'node:path' import { existsSync, mkdtempSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { DaemonClient } from './client' import { DaemonProtocolError } from './daemon-errors' -import { DaemonPtyAdapter } from './daemon-pty-adapter' +import { DaemonPtyAdapter, LIVENESS_PROBE_TIMEOUT_MS } from './daemon-pty-adapter' import { COMPLETION_PROCESS_INSPECTION_PROTOCOL_VERSION, GET_FOREGROUND_PROCESS_PROTOCOL_VERSION, + GET_SIZE_PROTOCOL_VERSION, PROTOCOL_VERSION } from './daemon-protocol-version' import { DaemonServer } from './daemon-server' @@ -1204,6 +1205,103 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => { await expect(adapter.probePtyLiveness('session')).resolves.toBeNull() }) + + function createProbeAdapter( + protocolVersion: number, + request: ReturnType + ): DaemonPtyAdapter { + const probeAdapter = new DaemonPtyAdapter({ socketPath, tokenPath, protocolVersion }) + ;( + probeAdapter as unknown as { + client: { request: ReturnType; disconnect: ReturnType } + } + ).client = { request, disconnect: vi.fn() } + return probeAdapter + } + + // Why: a pre-v18 daemon rejects `getSize` as an unknown request type, so it would answer + // `null` forever — and one `null` makes the owner fan-out permanently unprovable, which + // would leave a genuinely dead pane unable to ever retire and respawn. + it('answers a pre-getSize daemon from its session inventory', async () => { + // Faithful pre-v18 daemon: routeRequest falls through its switch and error-replies. + const request = vi.fn(async (type: string) => { + if (type !== 'listSessions') { + throw new Error(`Unknown request type: ${type}`) + } + return { + sessions: [ + { sessionId: 'legacy-live', isAlive: true }, + { sessionId: 'legacy-exited', isAlive: false } + ] + } + }) + const legacy = createProbeAdapter(GET_SIZE_PROTOCOL_VERSION - 1, request) + + await expect(legacy.probePtyLiveness('legacy-live')).resolves.toBe(true) + await expect(legacy.probePtyLiveness('legacy-exited')).resolves.toBe(false) + await expect(legacy.probePtyLiveness('never-existed')).resolves.toBe(false) + expect(request).toHaveBeenCalledWith('listSessions', undefined, LIVENESS_PROBE_TIMEOUT_MS) + expect(request).not.toHaveBeenCalledWith('getSize', expect.anything()) + + legacy.dispose() + }) + + // The P0 boundary: an owner that cannot be reached must never read as one that answered "absent". + it('still answers unknown when a pre-getSize daemon cannot be reached', async () => { + const request = vi.fn(async () => { + throw new Error('Not connected') + }) + const legacy = createProbeAdapter(GET_SIZE_PROTOCOL_VERSION - 1, request) + + await expect(legacy.probePtyLiveness('legacy-live')).resolves.toBeNull() + + legacy.dispose() + }) + + // Why: `getSize` shipped into an already-released protocol without a version bump, so a + // daemon can report a version that implies support and still reject the request. Gating on + // the number alone left those daemons permanently unprovable — the same wedge, narrowed. + it('falls back when a daemon rejects getSize despite reporting a version that has it', async () => { + const request = vi.fn(async (type: string) => { + if (type === 'getSize') { + throw new Error(`Unknown request type: ${type}`) + } + return { sessions: [{ sessionId: 'ambiguous-live', isAlive: true }] } + }) + const ambiguous = createProbeAdapter(GET_SIZE_PROTOCOL_VERSION, request) + + await expect(ambiguous.probePtyLiveness('ambiguous-live')).resolves.toBe(true) + await expect(ambiguous.probePtyLiveness('never-existed')).resolves.toBe(false) + + // The rejection is remembered, so later probes skip the round trip that cannot work. + expect(request.mock.calls.filter(([type]) => type === 'getSize')).toHaveLength(1) + // Why pinned here: a wedged daemon holds its socket open, so an unbounded getSize would + // stall a pane mount for the client's 30s default instead of answering "unknown" in 2s. + expect(request).toHaveBeenCalledWith( + 'getSize', + { sessionId: 'ambiguous-live' }, + LIVENESS_PROBE_TIMEOUT_MS + ) + + ambiguous.dispose() + }) + + // The safety direction: only the daemon's own "I do not implement that" may switch strategy. + // A transient failure must stay unproven rather than be retried as a capability question. + it('keeps a transient getSize failure unproven instead of treating it as unsupported', async () => { + const request = vi.fn(async (type: string) => { + if (type === 'getSize') { + throw new Error('Connection lost') + } + return { sessions: [] } + }) + const flaky = createProbeAdapter(GET_SIZE_PROTOCOL_VERSION, request) + + await expect(flaky.probePtyLiveness('live-elsewhere')).resolves.toBeNull() + expect(request).not.toHaveBeenCalledWith('listSessions', undefined, LIVENESS_PROBE_TIMEOUT_MS) + + flaky.dispose() + }) }) describe('getBufferSnapshot', () => { diff --git a/src/main/daemon/daemon-pty-adapter.ts b/src/main/daemon/daemon-pty-adapter.ts index 2bba3c467dc..b9473c3de32 100644 --- a/src/main/daemon/daemon-pty-adapter.ts +++ b/src/main/daemon/daemon-pty-adapter.ts @@ -37,6 +37,7 @@ import { type TakePendingOutputResult } from './types' import { + GET_SIZE_PROTOCOL_VERSION, HISTORY_SEED_TRANSFER_PROTOCOL_VERSION, SNAPSHOT_SERIALIZER_FIDELITY_DAEMON_PROTOCOL_VERSION, STABLE_PANE_ATTACH_ONLY_DAEMON_PROTOCOL_VERSION @@ -139,6 +140,12 @@ export type DaemonIdentityChangeEvent = { const MAX_TOMBSTONES = 1000 const MAX_CONCURRENT_CHECKPOINTS = 4 +// Why far below the client's 30s default: a wedged daemon holds its socket open, so an unbounded +// probe stalls a pane mount for the full request timeout — and the owner fan-out waits on every +// adapter, so one hung daemon stalls each restoring pane. Answering "unknown" quickly is strictly +// better here: unknown never authorizes retirement, it only defers it. +export const LIVENESS_PROBE_TIMEOUT_MS = 2_000 + // Why: providers take an absolute teardown deadline, but the client RPC takes a // relative timeout — convert only here, at the request itself, so sequential RPCs // naturally share the remaining budget (undefined keeps the client's 30s default). @@ -203,6 +210,8 @@ export class DaemonPtyAdapter implements IPtyProvider { this.sleepRestoreSessionIds.delete(sessionId) }) private activeSessionIds = new Set() + // Set only once this daemon has rejected `getSize` as unknown; its protocol number cannot prove it. + private getSizeUnsupported = false // A replacement daemon has none of the old PTYs; only createOrAttach can make their bindings writable again. private sessionsAwaitingDaemonRecovery = new Set() private sessionIncarnations = new Map() @@ -900,11 +909,37 @@ export class DaemonPtyAdapter implements IPtyProvider { async probePtyLiveness(id: string): Promise { try { - const result = await this.client.request<{ size: { cols: number; rows: number } | null }>( - 'getSize', - { sessionId: id } + if (!this.getSizeUnsupported && this.protocolVersion >= GET_SIZE_PROTOCOL_VERSION) { + try { + const result = await this.client.request<{ size: { cols: number; rows: number } | null }>( + 'getSize', + { sessionId: id }, + LIVENESS_PROBE_TIMEOUT_MS + ) + return result.size !== null + } catch (error) { + // Why the capability probe rather than the version alone: `getSize` shipped into an + // already-released protocol without a bump, so a daemon can report a version that + // implies support and still reject the request. Ask what it can do, not what its + // number implies — and remember the answer so later probes skip the dead round trip. + if (!isUnknownRequestTypeError(error)) { + throw error + } + this.getSizeUnsupported = true + } + } + // Why: a daemon without `getSize` would otherwise answer `null` forever, and one `null` + // makes the whole owner fan-out unprovable — a dead pane could then never be retired. + // `listSessions` is the same inventory legacy discovery routes by, and has existed since + // the first daemon protocol. Requested directly rather than through `listProcesses` so a + // liveness probe does not publish inventory audit observations as a side effect; both + // rethrow on failure, so either way a dead socket stays `null` instead of reading absent. + const { sessions } = await this.client.request( + 'listSessions', + undefined, + LIVENESS_PROBE_TIMEOUT_MS ) - return result.size !== null + return sessions.some((session) => session.sessionId === id && session.isAlive) } catch { return null } @@ -2409,6 +2444,16 @@ function notifyAuditListeners(listeners: readonly ((value: T) => void)[], val } } +/** + * Narrow on purpose: only the daemon's own reply for a request type it does not implement. + * A transient failure must stay unproven rather than be mistaken for a missing capability. + * The server throws `Unknown request type: `; the client rejects with that text, which + * `addNodePtyRecoveryHint` only ever prepends to. + */ +function isUnknownRequestTypeError(err: unknown): boolean { + return err instanceof Error && err.message.includes('Unknown request type') +} + // Why: syscall='connect' distinguishes a dead-socket ENOENT/ECONNREFUSED from token-file ENOENT (no syscall); // message strings incl. wedged-daemon "Hello response timed out" (#8689) also warrant a respawn. function isDaemonGoneError(err: unknown): boolean { diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index 257250a9858..797cfa4411f 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -9108,6 +9108,259 @@ describe('registerPtyHandlers', () => { } ) + it.each([ + { label: 'another owner reports it alive', liveness: true }, + { label: 'no owner could answer', liveness: null } + ])('keeps a persisted owner whose absence is unproven ($label)', async ({ liveness }) => { + const worktreeId = 'repo-1::/tmp/unproven-owner' + const cwd = '/tmp/unproven-owner' + const tabId = 'tab-unproven-owner' + const leafId = '56565656-5656-4656-8656-565656565656' + const paneKey = makePaneKey(tabId, leafId) + // Why: a degraded router answers unmapped ids from the local fallback, which never + // owned this daemon session — the same "Session not found" a truly dead PTY yields. + const providerSpawn = vi.fn( + async (options: { attachOnly?: boolean; command?: string; sessionId?: string }) => { + if (options.attachOnly) { + throw new Error('Session not found: pty-unproven-owner') + } + return { id: 'pty-fresh-unproven', incarnationId: 'inc-fresh-unproven' } + } + ) + const probePtyLiveness = vi.fn(async () => liveness) + setLocalPtyProvider({ + spawn: providerSpawn, + probePtyLiveness, + write: vi.fn(), + resize: vi.fn(), + kill: vi.fn(), + shutdown: vi.fn(), + sendSignal: vi.fn(), + getCwd: vi.fn(), + getInitialCwd: vi.fn(), + clearBuffer: vi.fn(), + acknowledgeDataEvent: vi.fn(), + hasChildProcesses: vi.fn(), + getForegroundProcess: vi.fn(), + serialize: vi.fn(), + revive: vi.fn(), + onData: vi.fn(() => () => {}), + onReplay: vi.fn(() => () => {}), + onExit: vi.fn(() => () => {}), + listProcesses: vi.fn(async () => []), + attach: vi.fn(), + getDefaultShell: vi.fn(), + getProfiles: vi.fn() + } as never) + let session = { + tabsByWorktree: { + [worktreeId]: [{ id: tabId, worktreeId, ptyId: 'pty-unproven-owner' }] + }, + terminalLayoutsByTabId: { + [tabId]: { + root: { type: 'leaf' as const, leafId }, + activeLeafId: leafId, + expandedLeafId: null, + ptyIdsByLeafId: { [leafId]: 'pty-unproven-owner' } + } + }, + terminalPtyIncarnationsByPaneKey: { [paneKey]: 'inc-unproven-owner' } + } + const store = { + getWorkspaceSession: vi.fn(() => session), + setWorkspaceSession: vi.fn((next) => { + session = next + }), + flushOrThrow: vi.fn(), + persistPtyBinding: vi.fn(), + getFolderWorkspace: vi.fn(() => undefined), + getFolderWorkspaces: vi.fn(() => []), + getProjectGroups: vi.fn(() => []), + getRepos: vi.fn(() => []) + } + const runtime = { + setPtyController: vi.fn(), + resolveTerminalPane: vi.fn(() => { + throw new Error('terminal_not_found') + }), + createPreAllocatedTerminalHandle: vi.fn(() => 'term-unproven'), + preAllocateHandleForPty: vi.fn(() => 'term-unproven'), + registerPreAllocatedHandleForPty: vi.fn(), + beginPtyRegistration: vi.fn(), + cancelPendingPtyRegistration: vi.fn(), + assertPtyRegistrationAllowed: vi.fn(), + registerPty: vi.fn(), + noteTerminalSpawnCommand: vi.fn(), + seedHeadlessTerminal: vi.fn(), + onPtySpawned: vi.fn(), + onPtyExit: vi.fn(), + onPtyData: vi.fn() + } + + registerPtyHandlers( + mainWindow as never, + runtime as never, + undefined, + undefined, + undefined, + store as never + ) + + await expect( + handlers.get('pty:spawn')!(null, { + cols: 80, + rows: 24, + cwd, + command: 'codex resume unproven-owner-session', + worktreeId, + tabId, + leafId, + env: { + ORCA_PANE_KEY: paneKey, + ORCA_TAB_ID: tabId, + ORCA_WORKTREE_ID: worktreeId + } + }) + ).rejects.toThrow('terminal_pane_owner_unverified') + + expect(probePtyLiveness).toHaveBeenCalledWith('pty-unproven-owner') + // The live PTY keeps its pane binding, gets no synthetic exit, and is not duplicated. + expect(providerSpawn).toHaveBeenCalledOnce() + expect(providerSpawn.mock.calls[0]?.[0]).toMatchObject({ attachOnly: true }) + expect(runtime.onPtyExit).not.toHaveBeenCalled() + expect(store.setWorkspaceSession).not.toHaveBeenCalled() + expect(store.flushOrThrow).not.toHaveBeenCalled() + expect(session.tabsByWorktree[worktreeId]).toHaveLength(1) + }) + + // Why the positive direction needs its own case: the sibling retire tests use providers with + // no `probePtyLiveness`, so they skip this guard entirely. Without this, the guard could be + // strengthened into a permanent veto — no daemon-backed pane could ever recover from a dead + // owner — and every suite would stay green. + it('still retires and respawns when a provider proves the owner is absent', async () => { + const worktreeId = 'repo-1::/tmp/proven-absent-owner' + const cwd = '/tmp/proven-absent-owner' + const tabId = 'tab-proven-absent-owner' + const leafId = '78787878-7878-4878-8878-787878787878' + const paneKey = makePaneKey(tabId, leafId) + const providerSpawn = vi.fn( + async (options: { attachOnly?: boolean; command?: string; sessionId?: string }) => { + if (options.attachOnly) { + throw new Error('Session not found: pty-proven-absent-owner') + } + return { id: 'pty-fresh-proven', incarnationId: 'inc-fresh-proven' } + } + ) + const probePtyLiveness = vi.fn(async () => false) + setLocalPtyProvider({ + spawn: providerSpawn, + probePtyLiveness, + write: vi.fn(), + resize: vi.fn(), + kill: vi.fn(), + shutdown: vi.fn(), + sendSignal: vi.fn(), + getCwd: vi.fn(), + getInitialCwd: vi.fn(), + clearBuffer: vi.fn(), + acknowledgeDataEvent: vi.fn(), + hasChildProcesses: vi.fn(), + getForegroundProcess: vi.fn(), + serialize: vi.fn(), + revive: vi.fn(), + onData: vi.fn(() => () => {}), + onReplay: vi.fn(() => () => {}), + onExit: vi.fn(() => () => {}), + listProcesses: vi.fn(async () => []), + attach: vi.fn(), + getDefaultShell: vi.fn(), + getProfiles: vi.fn() + } as never) + let session = { + tabsByWorktree: { + [worktreeId]: [{ id: tabId, worktreeId, ptyId: 'pty-proven-absent-owner' }] + }, + terminalLayoutsByTabId: { + [tabId]: { + root: { type: 'leaf' as const, leafId }, + activeLeafId: leafId, + expandedLeafId: null, + ptyIdsByLeafId: { [leafId]: 'pty-proven-absent-owner' } + } + }, + terminalPtyIncarnationsByPaneKey: { [paneKey]: 'inc-proven-absent-owner' } + } + const store = { + getWorkspaceSession: vi.fn(() => session), + setWorkspaceSession: vi.fn((next) => { + session = next + }), + flushOrThrow: vi.fn(), + persistPtyBinding: vi.fn(), + getFolderWorkspace: vi.fn(() => undefined), + getFolderWorkspaces: vi.fn(() => []), + getProjectGroups: vi.fn(() => []), + getRepos: vi.fn(() => []) + } + const runtime = { + setPtyController: vi.fn(), + resolveTerminalPane: vi.fn(() => { + throw new Error('terminal_not_found') + }), + createPreAllocatedTerminalHandle: vi.fn(() => 'term-proven-absent'), + preAllocateHandleForPty: vi.fn(() => 'term-proven-absent'), + registerPreAllocatedHandleForPty: vi.fn(), + beginPtyRegistration: vi.fn(), + cancelPendingPtyRegistration: vi.fn(), + assertPtyRegistrationAllowed: vi.fn(), + registerPty: vi.fn(), + noteTerminalSpawnCommand: vi.fn(), + seedHeadlessTerminal: vi.fn(), + onPtySpawned: vi.fn(), + onPtyExit: vi.fn(), + onPtyData: vi.fn() + } + + registerPtyHandlers( + mainWindow as never, + runtime as never, + undefined, + undefined, + undefined, + store as never + ) + + const mounted = await handlers.get('pty:spawn')!(null, { + cols: 80, + rows: 24, + cwd, + command: 'codex resume proven-absent-session', + worktreeId, + tabId, + leafId, + env: { + ORCA_PANE_KEY: paneKey, + ORCA_TAB_ID: tabId, + ORCA_WORKTREE_ID: worktreeId + } + }) + + expect(probePtyLiveness).toHaveBeenCalledWith('pty-proven-absent-owner') + // Proven absence is the one answer that authorizes retirement, so recovery must proceed. + expect(mounted).toMatchObject({ id: 'pty-fresh-proven' }) + expect(providerSpawn).toHaveBeenCalledTimes(2) + expect(providerSpawn.mock.calls[1]?.[0]).toMatchObject({ + command: 'codex resume proven-absent-session' + }) + expect(runtime.onPtyExit).toHaveBeenCalledWith( + 'pty-proven-absent-owner', + 0, + 'inc-proven-absent-owner' + ) + expect(store.setWorkspaceSession).toHaveBeenCalledOnce() + expect(store.flushOrThrow).toHaveBeenCalledOnce() + }) + it('retires a dead owner from the exact SSH host session before fresh recovery', async () => { const connectionId = 'ssh-dead-stable-pane' const hostId = `ssh:${connectionId}` diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index cd01a985243..a59f70e8ac2 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -734,6 +734,15 @@ async function attachStablePaneOwner( if (!isPtyAlreadyGoneError(error)) { throw error } + // Why: "Session not found" only proves the provider we asked has no such PTY — and a + // degraded router answers unmapped ids from the local fallback, which never owned a + // daemon session. Retiring on that would signal exit and delete a live agent's pane + // binding. Absence must be proven across every possible owner first; `null` (nobody + // could answer) is not absence. Providers without a probe are their own sole owner, + // so their refusal stays authoritative. + if (provider.probePtyLiveness && (await provider.probePtyLiveness(owner.ptyId)) !== false) { + throw new Error('terminal_pane_owner_unverified') + } const ownerBeforeRetire = args.resolveOwner?.() if ( ownerBeforeRetire &&