From cf7e06c4187cbccf277a0e3e1fa742dfa83a5429 Mon Sep 17 00:00:00 2001 From: OrcaWin <293788423+OrcaWin@users.noreply.github.com> Date: Tue, 4 Aug 2026 01:41:12 -0400 Subject: [PATCH] fix(daemon): detect getSize support instead of inferring it from the protocol number MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The version gate was not a sound boundary. `getSize` landed in 22b00a7cd2 while PROTOCOL_VERSION was already 18 and that commit did not bump it, so version 18 means two different things: daemons built before it lack the request, daemons built after it have it. Both report 18. A daemon from that window therefore passed the `>= 18` gate, was sent `getSize`, error-replied "Unknown request type", and answered `null` — and one `null` makes the owner fan-out permanently unprovable, so a genuinely dead pane could never retire. That is the same wedge the gate was added to close, just narrowed to one build window. Ask the daemon what it supports rather than trusting its number: attempt `getSize`, and fall back to `listSessions` only when the daemon itself replies that the request type is unknown. The predicate is deliberately narrow so a transient failure stays unproven instead of being mistaken for a missing capability, and the rejection is remembered so later probes skip a round trip that cannot work. This mirrors the capability-probe rule AGENTS.md already requires for Git, for the same reason: version numbers overstate capabilities. Unreachable still never means absent. --- src/main/daemon/daemon-pty-adapter.test.ts | 38 ++++++++++++++++ src/main/daemon/daemon-pty-adapter.ts | 52 +++++++++++++++------- 2 files changed, 74 insertions(+), 16 deletions(-) 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