diff --git a/src/main/daemon/daemon-pty-adapter.test.ts b/src/main/daemon/daemon-pty-adapter.test.ts index 120706921f3..aea7fb55df0 100644 --- a/src/main/daemon/daemon-pty-adapter.test.ts +++ b/src/main/daemon/daemon-pty-adapter.test.ts @@ -1255,6 +1255,44 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => { 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) + + 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) + + flaky.dispose() + }) }) describe('getBufferSnapshot', () => { diff --git a/src/main/daemon/daemon-pty-adapter.ts b/src/main/daemon/daemon-pty-adapter.ts index a73363c3ebb..06579ac9279 100644 --- a/src/main/daemon/daemon-pty-adapter.ts +++ b/src/main/daemon/daemon-pty-adapter.ts @@ -203,6 +203,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() @@ -899,23 +901,31 @@ export class DaemonPtyAdapter implements IPtyProvider { async probePtyLiveness(id: string): Promise { try { - if (this.protocolVersion < GET_SIZE_PROTOCOL_VERSION) { - // Why: a pre-v18 daemon rejects `getSize` as unknown, so it would 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 already routes by, and - // is requested directly (not via listProcesses, which swallows errors into an empty list) - // so a dead socket still rejects rather than reading as absence. - const { sessions } = await this.client.request( - 'listSessions', - undefined - ) - return sessions.some((session) => session.sessionId === id && session.isAlive) + 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 } + ) + 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 + } } - const result = await this.client.request<{ size: { cols: number; rows: number } | null }>( - 'getSize', - { sessionId: id } - ) - return result.size !== null + // 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 inventory legacy discovery already routes by, requested directly + // (not via listProcesses, which swallows errors into an empty list) so a dead socket + // still rejects rather than reading as absence. + const { sessions } = await this.client.request('listSessions', undefined) + return sessions.some((session) => session.sessionId === id && session.isAlive) } catch { return null } @@ -2422,6 +2432,16 @@ function notifyAuditListeners(listeners: readonly ((value: T) => void)[], val // 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. +/** + * 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') +} + function isDaemonGoneError(err: unknown): boolean { if (!(err instanceof Error)) { return false