From df7259e2ae341a7a0aacdf2a37fb3c4245b1fbbb Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 18 Oct 2026 23:02:26 -0700 Subject: [PATCH] fix(terminal): reattach a restored terminal while an older daemon cannot answer (#30486) (#30499) * fix(terminal): reattach a restored terminal while an older daemon cannot answer (#30486) A window save erased the PTY incarnation main persisted for each pane, so after a restart every reattach lacked the generation proof the owner resolver needs. With any older daemon generation that could not answer, the inventory stayed incomplete and every restored terminal was refused as unverifiable, on every Retry. - keep each pane's incarnation across a window save that still binds it - accept an exact incarnation match from a reachable daemon even when another daemon's inventory fails (incarnation ids are unique per spawn) - drop a provably exited older daemon from the reattach inventory - offer Open new terminal beside Retry on the unverifiable toast * test(daemon): build the unanswering daemon endpoint with the platform socket path * fix(session): keep each pane's incarnation when a save carries only some of them * fix(daemon): judge a replacement legacy daemon's exit by its own pid * test(daemon): router fixture reports a pid that exists, so exit proof never sees a dead daemon * test(daemon): the wedged older daemon names a live pid instead of the shared fixture's arbitrary one * fix(daemon): a spawn treats an older daemon as exited only when its socket and pid are both gone --- src/main/daemon/daemon-legacy-exit-proof.ts | 6 +- .../daemon-pty-router-legacy-gone.test.ts | 72 +++++++++++++++ .../daemon-pty-router-legacy-owner.test.ts | 43 +++++++++ ...emon-pty-router-unanswering-legacy.test.ts | 91 +++++++++++++++++++ src/main/daemon/daemon-pty-router.ts | 31 +++---- .../daemon/daemon-router-exited-legacy.ts | 40 ++++++++ src/main/daemon/daemon-router-legacy-owner.ts | 28 +++--- .../daemon/daemon-session-owner-choice.ts | 25 +++++ .../daemon-session-owner-resolution.test.ts | 66 +++++++++++++- .../daemon/daemon-session-owner-resolution.ts | 65 +++++++------ .../pty-incarnation-renderer-save.test.ts | 69 ++++++++++++++ ...ntime-authored-workspace-session-fields.ts | 35 +++++++ .../terminal-pane/TerminalErrorToast.test.ts | 20 ++++ .../terminal-pane/TerminalErrorToast.tsx | 14 ++- .../terminal-pane/TerminalPaneSurface.tsx | 4 + src/renderer/src/i18n/locales/en.json | 1 + src/renderer/src/i18n/locales/es.json | 1 + src/renderer/src/i18n/locales/fr.json | 1 + src/renderer/src/i18n/locales/ja.json | 1 + src/renderer/src/i18n/locales/ko.json | 1 + src/renderer/src/i18n/locales/zh.json | 1 + 21 files changed, 553 insertions(+), 62 deletions(-) create mode 100644 src/main/daemon/daemon-pty-router-unanswering-legacy.test.ts create mode 100644 src/main/daemon/daemon-router-exited-legacy.ts create mode 100644 src/main/daemon/daemon-session-owner-choice.ts create mode 100644 src/main/persistence/loading-store/pty-incarnation-renderer-save.test.ts diff --git a/src/main/daemon/daemon-legacy-exit-proof.ts b/src/main/daemon/daemon-legacy-exit-proof.ts index bca241f5f20..ea7e177da93 100644 --- a/src/main/daemon/daemon-legacy-exit-proof.ts +++ b/src/main/daemon/daemon-legacy-exit-proof.ts @@ -18,7 +18,11 @@ export class LegacyDaemonExitProof { // Why: a refused or missing socket is loss of contact, not death; only a vanished pid proves the // daemon exited. Windows is excluded like probeDaemonProcessIdentity, so it keeps failing closed. hasExited(adapter: DaemonPtyAdapter): boolean { - const pid = this.pids.get(adapter) + // Why the last authenticated pid first: a replacement daemon must not be judged by its predecessor's pid. + const pid = + (typeof adapter.getLastAuthenticatedDaemonIdentity === 'function' + ? adapter.getLastAuthenticatedDaemonIdentity()?.pid + : undefined) ?? this.pids.get(adapter) return ( process.platform !== 'win32' && pid !== undefined && inspectProcessSignal(pid) === 'missing' ) diff --git a/src/main/daemon/daemon-pty-router-legacy-gone.test.ts b/src/main/daemon/daemon-pty-router-legacy-gone.test.ts index 61790feb980..6588e3c4498 100644 --- a/src/main/daemon/daemon-pty-router-legacy-gone.test.ts +++ b/src/main/daemon/daemon-pty-router-legacy-gone.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it, vi } from 'vitest' import type { DaemonPtyAdapter } from './daemon-pty-adapter' import { DaemonPtyRouter } from './daemon-pty-router' +import { SessionNotFoundError, TerminalSessionOwnerUnverifiedError } from './daemon-errors' const LIVE_PID = 4242 const EXITED_PID = 4343 @@ -62,6 +63,77 @@ describe('DaemonPtyRouter legacy socket that can never answer', () => { } ) + // #30486: the reattach inventory kept the dead daemon, so every unrouted pane stayed unverifiable. + it.skipIf(process.platform === 'win32')( + 'lets a reattach prove absence once a legacy daemon has exited', + async () => { + const current = adapter('current', ['current-session']) + const exited = adapter('exited') + vi.mocked(exited.listProcesses).mockRejectedValue(connectError('ECONNREFUSED', 'connect')) + const router = new DaemonPtyRouter({ current, legacy: [exited] }) + const attach = { cols: 80, rows: 24, sessionId: 'gone-session', attachOnly: true } + + await expect(router.spawn(attach)).rejects.toBeInstanceOf(SessionNotFoundError) + // The current daemon is now the only owner, so it answers the next attach directly. + Object.assign(current, { + spawn: vi.fn(async () => { + throw new SessionNotFoundError('gone-session') + }) + }) + await expect(router.spawn(attach)).rejects.toBeInstanceOf(SessionNotFoundError) + expect(current.spawn).toHaveBeenCalledOnce() + + expect(exited.listProcesses).toHaveBeenCalledOnce() + expect(exited.disconnectOnly).toHaveBeenCalledOnce() + expect(router.getLegacyAdapters()).toEqual([]) + } + ) + + it('keeps a reattach unverifiable while the legacy socket answers, even with its pid gone', async () => { + const current = adapter('current') + const silent = adapter('silent', [], EXITED_PID) + vi.mocked(silent.listProcesses).mockRejectedValue(new Error('wedged')) + const router = new DaemonPtyRouter({ current, legacy: [silent] }) + + await expect( + router.spawn({ cols: 80, rows: 24, sessionId: 'session', attachOnly: true }) + ).rejects.toBeInstanceOf(TerminalSessionOwnerUnverifiedError) + expect(router.getLegacyAdapters()).toEqual([silent]) + expect(silent.disconnectOnly).not.toHaveBeenCalled() + }) + + it('judges a replacement legacy daemon by its own pid, not its predecessor', async () => { + const current = adapter('current') + // Recorded at construction with the exited predecessor's pid. + const replaced = adapter('replaced', [], EXITED_PID) + const router = new DaemonPtyRouter({ current, legacy: [replaced] }) + Object.assign(replaced, { + getLastAuthenticatedDaemonIdentity: vi.fn(() => ({ + pid: LIVE_PID, + startedAtMs: 2, + launchNonce: 'replacement' + })) + }) + vi.mocked(replaced.listProcesses).mockRejectedValue(connectError('ECONNREFUSED', 'connect')) + + await expect( + router.spawn({ cols: 80, rows: 24, sessionId: 'session', attachOnly: true }) + ).rejects.toBeInstanceOf(TerminalSessionOwnerUnverifiedError) + expect(router.getLegacyAdapters()).toEqual([replaced]) + }) + + it('keeps a reattach unverifiable while a gone legacy daemon process is alive', async () => { + const current = adapter('current') + const alive = adapter('alive', [], LIVE_PID) + vi.mocked(alive.listProcesses).mockRejectedValue(connectError('ECONNREFUSED', 'connect')) + const router = new DaemonPtyRouter({ current, legacy: [alive] }) + + await expect( + router.spawn({ cols: 80, rows: 24, sessionId: 'session', attachOnly: true }) + ).rejects.toBeInstanceOf(TerminalSessionOwnerUnverifiedError) + expect(router.getLegacyAdapters()).toEqual([alive]) + }) + it('keeps a gone legacy socket whose process is alive or unproven', async () => { const current = adapter('current', ['current-session']) const alive = adapter('alive', [], LIVE_PID) diff --git a/src/main/daemon/daemon-pty-router-legacy-owner.test.ts b/src/main/daemon/daemon-pty-router-legacy-owner.test.ts index 2becdce72bc..a47386f4d64 100644 --- a/src/main/daemon/daemon-pty-router-legacy-owner.test.ts +++ b/src/main/daemon/daemon-pty-router-legacy-owner.test.ts @@ -4,6 +4,8 @@ import { DaemonPtyRouter } from './daemon-pty-router' import { TerminalSessionOwnerUnverifiedError } from './daemon-errors' const SPAWN = { cols: 80, rows: 24 } +// Above any real pid ceiling, so the exit probe always finds it missing. +const MISSING_PID = 2_147_483_000 // #26296: after a daemon update, the old daemon keeps the sessions it runs; the new one takes only new ids. describe('DaemonPtyRouter keeps live sessions on their older daemon', () => { @@ -94,4 +96,45 @@ describe('DaemonPtyRouter keeps live sessions on their older daemon', () => { expect(legacy.spawn).not.toHaveBeenCalled() expect(legacy.probePtyLiveness).toHaveBeenCalledOnce() }) + + // A daemon still on its socket is not proven exited by a missing pid alone. + it('refuses while the older daemon socket answers even if its pid is gone', async () => { + const current = createAdapter('current') + const legacy = createAdapter('legacy', ['claude-session']) + vi.mocked(legacy.listProcesses).mockRejectedValue(new Error('wedged')) + vi.mocked(legacy.probePtyLiveness).mockResolvedValue(null) + vi.mocked(legacy.getLastAuthenticatedDaemonIdentity).mockReturnValue({ + pid: MISSING_PID, + startedAtMs: 0, + launchNonce: 'legacy' + }) + const router = new DaemonPtyRouter({ current, legacy: [legacy] }) + + await expect(router.spawn({ ...SPAWN, sessionId: 'claude-session' })).rejects.toBeInstanceOf( + TerminalSessionOwnerUnverifiedError + ) + expect(current.spawn).not.toHaveBeenCalled() + }) + + it.skipIf(process.platform === 'win32')( + 'creates on the current daemon once the older daemon socket and pid are both gone', + async () => { + const current = createAdapter('current') + const legacy = createAdapter('legacy', ['claude-session']) + vi.mocked(legacy.listProcesses).mockRejectedValue( + Object.assign(new Error('connect ENOENT'), { code: 'ENOENT', syscall: 'connect' }) + ) + vi.mocked(legacy.probePtyLiveness).mockResolvedValue(null) + vi.mocked(legacy.getLastAuthenticatedDaemonIdentity).mockReturnValue({ + pid: MISSING_PID, + startedAtMs: 0, + launchNonce: 'legacy' + }) + const router = new DaemonPtyRouter({ current, legacy: [legacy] }) + + await router.spawn({ ...SPAWN, sessionId: 'claude-session' }) + + expect(current.spawn).toHaveBeenCalledOnce() + } + ) }) diff --git a/src/main/daemon/daemon-pty-router-unanswering-legacy.test.ts b/src/main/daemon/daemon-pty-router-unanswering-legacy.test.ts new file mode 100644 index 00000000000..6e7046ddc52 --- /dev/null +++ b/src/main/daemon/daemon-pty-router-unanswering-legacy.test.ts @@ -0,0 +1,91 @@ +import { createServer, type Socket } from 'node:net' +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it } from 'vitest' +import { DaemonPtyAdapter } from './daemon-pty-adapter' +import { DaemonPtyRouter } from './daemon-pty-router' +import { TerminalSessionOwnerUnverifiedError } from './daemon-errors' +import { createMockSubprocess, startDaemonAdapterHarness } from './daemon-pty-adapter-test-harness' +import { getDaemonSocketPath } from './daemon-spawner' + +const cleanups: (() => Promise | void)[] = [] + +afterEach(async () => { + for (const cleanup of cleanups.splice(0).toReversed()) { + await cleanup() + } +}) + +/** An older daemon endpoint that accepts connections and never answers, like a wedged daemon. */ +async function startUnansweringDaemon(): Promise<{ socketPath: string; tokenPath: string }> { + const dir = mkdtempSync(join(tmpdir(), 'unanswering-daemon-')) + // Why: Windows needs a named pipe, which the spawner's path helper builds per platform. + const socketPath = getDaemonSocketPath(dir, 47) + const tokenPath = join(dir, 'daemon.token') + writeFileSync(tokenPath, 'token') + const sockets = new Set() + const server = createServer((socket) => { + sockets.add(socket) + socket.on('close', () => sockets.delete(socket)) + }) + await new Promise((resolve) => server.listen(socketPath, resolve)) + cleanups.push(async () => { + for (const socket of sockets) { + socket.destroy() + } + await new Promise((resolve) => server.close(() => resolve())) + rmSync(dir, { recursive: true, force: true }) + }) + return { socketPath, tokenPath } +} + +// #30486: after an update, one older daemon that could not answer stranded every restored pane. +describe('DaemonPtyRouter reattach beside an unanswering older daemon', () => { + it('reattaches the exact restored session the current daemon still runs', async () => { + const current = await startDaemonAdapterHarness(() => createMockSubprocess()) + cleanups.push(async () => { + await current.server.shutdown() + rmSync(current.dir, { recursive: true, force: true }) + }) + const spawned = await current.adapter.spawn({ + cols: 80, + rows: 24, + sessionId: 'wt@@pane-1', + isNewSession: true + }) + expect(spawned.incarnationId).toEqual(expect.any(String)) + // App restart: the old app's connection goes away; the daemon keeps the session. + await current.adapter.disconnectOnly() + + const unanswering = await startUnansweringDaemon() + const router = new DaemonPtyRouter({ + current: new DaemonPtyAdapter({ + socketPath: current.socketPath, + tokenPath: current.tokenPath + }), + legacy: [new DaemonPtyAdapter({ ...unanswering, protocolVersion: 47 })] + }) + cleanups.push(() => router.disconnectOnly()) + await router.discoverLegacySessions() + + const attach = { + cols: 80, + rows: 24, + sessionId: 'wt@@pane-1', + attachOnly: true, + expectedIncarnationId: spawned.incarnationId + } + for (let attempt = 0; attempt < 2; attempt++) { + await expect(router.spawn(attach)).resolves.toMatchObject({ + id: 'wt@@pane-1', + incarnationId: spawned.incarnationId, + isReattach: true + }) + } + // Without generation evidence the older daemon could still own the id, so that stays refused. + await expect( + router.spawn({ cols: 80, rows: 24, sessionId: 'wt@@unproven', attachOnly: true }) + ).rejects.toBeInstanceOf(TerminalSessionOwnerUnverifiedError) + }, 30_000) +}) diff --git a/src/main/daemon/daemon-pty-router.ts b/src/main/daemon/daemon-pty-router.ts index 3aba591324c..069dab3a731 100644 --- a/src/main/daemon/daemon-pty-router.ts +++ b/src/main/daemon/daemon-pty-router.ts @@ -10,7 +10,6 @@ import type { PtySpawnResult } from '../providers/pty-provider-contract' import type { PtyProcessInspection } from '../providers/pty-process-inspection' -import { isDaemonEndpointGoneError } from './daemon-errors' import { LegacyDaemonExitProof } from './daemon-legacy-exit-proof' import { shouldHandoffDaemonHistory } from './daemon-history-handoff' import type { DaemonPtyRouterDataEvent, DaemonPtyRouterExitEvent } from './daemon-pty-router-events' @@ -19,6 +18,7 @@ import type { DaemonIdleRetirementResult } from './daemon-pty-runtime-state' import { DaemonRouterRetirement } from './daemon-router-retirement' import { routeSpawnOrHold } from './daemon-router-newer-daemon-hold' import { findLegacySessionOwner } from './daemon-router-legacy-owner' +import { retireExitedLegacyAdapter, type ExitedLegacyVerdict } from './daemon-router-exited-legacy' import type { WriteSettlement } from '../../shared/pty-write-settlement' import type { TerminalOscColorQueryReplyColors } from '../../shared/terminal-osc-color-reply' @@ -34,7 +34,11 @@ export class DaemonPtyRouter implements IPtyProvider { constructor(opts: { current: DaemonPtyAdapter; legacy: DaemonPtyAdapter[] }) { this.current = opts.current this.legacy = opts.legacy - this.ownerResolver = new DaemonSessionOwnerResolver(this.allAdapters(), this.sessionAdapters) + this.ownerResolver = new DaemonSessionOwnerResolver( + this.allAdapters(), + this.sessionAdapters, + (adapter, error) => this.retireExitedLegacy(adapter, error) !== 'not-exited' + ) this.subscriptions = new DaemonPtyAdapterSubscriptionFanout( this.allAdapters(), // Why owner-only: another generation's exit for a reused id must not unroute a live legacy session. @@ -253,25 +257,13 @@ export class DaemonPtyRouter implements IPtyProvider { this.legacyExitProof.record(this.legacy) return processes } catch (error) { - if ( - adapter === this.current || - !isDaemonEndpointGoneError(error) || - !this.legacyExitProof.hasExited(adapter) - ) { + const verdict = this.retireExitedLegacy(adapter, error) + if (verdict === 'not-exited') { throw error } - const index = this.legacy.indexOf(adapter) - if (index !== -1) { - this.legacy.splice(index, 1) - // Why: the resolver and fanout were built from a copy of allAdapters(). - // Splicing this.legacy alone leaves them querying the dead socket. + if (verdict === 'retired') { this.ownerResolver.dropProvider(adapter) - this.subscriptions.dropAdapter(adapter) } - // Why: the socket is already gone; a failed disconnect must not fail the list. - void Promise.resolve() - .then(() => adapter.disconnectOnly()) - .catch(() => undefined) return [] } }) @@ -279,6 +271,11 @@ export class DaemonPtyRouter implements IPtyProvider { return results.flat() } + private retireExitedLegacy(adapter: DaemonPtyAdapter, error: unknown): ExitedLegacyVerdict { + const { current, legacy, legacyExitProof: exitProof, subscriptions } = this + return retireExitedLegacyAdapter({ adapter, error, current, legacy, exitProof, subscriptions }) + } + async getDefaultShell(): Promise { return this.current.getDefaultShell() } diff --git a/src/main/daemon/daemon-router-exited-legacy.ts b/src/main/daemon/daemon-router-exited-legacy.ts new file mode 100644 index 00000000000..b1a1004bcb9 --- /dev/null +++ b/src/main/daemon/daemon-router-exited-legacy.ts @@ -0,0 +1,40 @@ +import { isDaemonEndpointGoneError } from './daemon-errors' +import type { LegacyDaemonExitProof } from './daemon-legacy-exit-proof' +import type { DaemonPtyAdapter } from './daemon-pty-adapter' +import type { DaemonPtyAdapterSubscriptionFanout } from './daemon-pty-adapter-subscription-fanout' + +export type ExitedLegacyVerdict = 'retired' | 'already-retired' | 'not-exited' + +/** + * Unlists a legacy daemon whose socket is gone and whose process has exited. Why both: a refused + * socket alone is loss of contact, and only a vanished pid proves the daemon owns nothing. + */ +export function retireExitedLegacyAdapter(args: { + adapter: DaemonPtyAdapter + error: unknown + current: DaemonPtyAdapter + legacy: DaemonPtyAdapter[] + exitProof: LegacyDaemonExitProof + subscriptions: DaemonPtyAdapterSubscriptionFanout +}): ExitedLegacyVerdict { + const { adapter } = args + if ( + adapter === args.current || + !isDaemonEndpointGoneError(args.error) || + !args.exitProof.hasExited(adapter) + ) { + return 'not-exited' + } + const index = args.legacy.indexOf(adapter) + if (index === -1) { + return 'already-retired' + } + args.legacy.splice(index, 1) + // Why: the fanout was built from a copy of the adapters, so splicing the legacy list alone leaves it on the dead socket. + args.subscriptions.dropAdapter(adapter) + // Why: the socket is already gone; a failed disconnect must not fail the caller. + void Promise.resolve() + .then(() => adapter.disconnectOnly()) + .catch(() => undefined) + return 'retired' +} diff --git a/src/main/daemon/daemon-router-legacy-owner.ts b/src/main/daemon/daemon-router-legacy-owner.ts index fed94f3be14..08f92cacb5c 100644 --- a/src/main/daemon/daemon-router-legacy-owner.ts +++ b/src/main/daemon/daemon-router-legacy-owner.ts @@ -1,5 +1,5 @@ import type { PtySpawnOptions } from '../providers/pty-provider-contract' -import { TerminalSessionOwnerUnverifiedError } from './daemon-errors' +import { isDaemonEndpointGoneError, TerminalSessionOwnerUnverifiedError } from './daemon-errors' import type { LegacyDaemonExitProof } from './daemon-legacy-exit-proof' import type { DaemonPtyAdapter } from './daemon-pty-adapter' @@ -20,21 +20,25 @@ export async function findLegacySessionOwner( return undefined } // Why first: an unreachable legacy daemon must not block reattaching the current daemon's own session. - if ((await probeOwnership(current, sessionId)) === true) { + if ((await probeOwnership(current, sessionId)).live === true) { return current } const verdicts = await Promise.all( - legacy.map(async (adapter) => ({ - adapter, - live: await probeOwnership(adapter, sessionId) - })) + legacy.map(async (adapter) => ({ adapter, ...(await probeOwnership(adapter, sessionId)) })) ) exitProof.record(legacy) const owner = verdicts.find(({ live }) => live === true) if (owner) { return owner.adapter } - if (verdicts.some(({ adapter, live }) => live === null && !exitProof.hasExited(adapter))) { + // Why both: a missing pid alone could be stale evidence about a daemon still on the socket. + const provenExited = (adapter: DaemonPtyAdapter, endpointGone: boolean): boolean => + endpointGone && exitProof.hasExited(adapter) + if ( + verdicts.some( + ({ adapter, live, endpointGone }) => live === null && !provenExited(adapter, endpointGone) + ) + ) { throw new TerminalSessionOwnerUnverifiedError(sessionId) } return undefined @@ -45,18 +49,18 @@ const LEGACY_OWNER_INVENTORY_MS = 2_000 async function probeOwnership( adapter: DaemonPtyAdapter, sessionId: string -): Promise { +): Promise<{ live: boolean | null; endpointGone: boolean }> { const live = await adapter.probePtyLiveness(sessionId).catch(() => null) if (live !== null) { - return live + return { live, endpointGone: false } } // Why: the probe never reconnects, so a daemon whose startup connect failed would refuse forever. try { const processes = await adapter.listProcesses({ deadlineMs: Date.now() + LEGACY_OWNER_INVENTORY_MS }) - return processes.some((process) => process.id === sessionId) - } catch { - return null + return { live: processes.some((process) => process.id === sessionId), endpointGone: false } + } catch (error) { + return { live: null, endpointGone: isDaemonEndpointGoneError(error) } } } diff --git a/src/main/daemon/daemon-session-owner-choice.ts b/src/main/daemon/daemon-session-owner-choice.ts new file mode 100644 index 00000000000..eaaf88a0cf5 --- /dev/null +++ b/src/main/daemon/daemon-session-owner-choice.ts @@ -0,0 +1,25 @@ +import type { PtyProcessInfo } from '../providers/pty-provider-contract' + +export type OwnerCandidate = { provider: T; process: PtyProcessInfo } + +/** The one provider an inventory proves owns a session id, or undefined when none is proven. */ +export function chooseSessionOwner(args: { + candidates: readonly OwnerCandidate[] + inventoryComplete: boolean + expectedIncarnationId?: string + expectedIncarnationIsAuthoritative: boolean +}): OwnerCandidate | undefined { + const { candidates, expectedIncarnationId } = args + const exact = expectedIncarnationId + ? candidates.filter(({ process }) => process.incarnationId === expectedIncarnationId) + : [] + // Why exact wins over an incomplete inventory: incarnation ids are unique per spawn, so an + // unreachable daemon cannot own this generation, and refusing strands every restored pane (#30486). + if (new Set(exact.map(({ provider }) => provider)).size === 1) { + return exact[0] + } + const providers = new Set(candidates.map(({ provider }) => provider)) + return !args.expectedIncarnationIsAuthoritative && args.inventoryComplete && providers.size === 1 + ? candidates[0] + : undefined +} diff --git a/src/main/daemon/daemon-session-owner-resolution.test.ts b/src/main/daemon/daemon-session-owner-resolution.test.ts index 05a9c9491bc..f64372da1c9 100644 --- a/src/main/daemon/daemon-session-owner-resolution.test.ts +++ b/src/main/daemon/daemon-session-owner-resolution.test.ts @@ -348,11 +348,32 @@ describe('DaemonSessionOwnerResolver', () => { }) }) - it('does not trust persisted incarnation proof from an incomplete inventory', async () => { + // #30486: one unanswering older daemon stranded every restored pane the current daemon still ran. + it('accepts the exact persisted incarnation while another inventory fails', async () => { + const exactOwner = provider(async () => [ + { id: 'session', incarnationId: 'persisted', cwd: '', title: 'candidate' } + ]) + const resolver = new DaemonSessionOwnerResolver( + [ + exactOwner, + provider(async () => { + throw new Error('offline') + }) + ], + new Map() + ) + + await expect(resolver.resolve('session', 'persisted')).resolves.toMatchObject({ + kind: 'owner', + provider: exactOwner + }) + }) + + it('does not trust another incarnation of the id from an incomplete inventory', async () => { const resolver = new DaemonSessionOwnerResolver( [ provider(async () => [ - { id: 'session', incarnationId: 'persisted', cwd: '', title: 'candidate' } + { id: 'session', incarnationId: 'other', cwd: '', title: 'candidate' } ]), provider(async () => { throw new Error('offline') @@ -362,6 +383,47 @@ describe('DaemonSessionOwnerResolver', () => { ) await expect(resolver.resolve('session', 'persisted')).resolves.toEqual({ kind: 'unknown' }) + await expect(resolver.resolve('session')).resolves.toEqual({ kind: 'unknown' }) + }) + + it('proves absence once a failed provider is proven gone, and stops asking it', async () => { + const reachable = provider( + async () => [], + async () => { + throw new SessionNotFoundError('missing') + } + ) + const gone = provider(async () => { + throw new Error('connect ENOENT') + }) + const isProvenGone = vi.fn((candidate: IPtyProvider) => candidate === gone) + const resolver = new DaemonSessionOwnerResolver([reachable, gone], new Map(), isProvenGone) + const attach = { sessionId: 'missing', attachOnly: true, cols: 80, rows: 24 } + + await expect(resolver.spawnAttachOnly(attach)).rejects.toBeInstanceOf(SessionNotFoundError) + expect(reachable.spawn).not.toHaveBeenCalled() + // The sole remaining provider now answers directly. + await expect(resolver.spawnAttachOnly(attach)).rejects.toBeInstanceOf(SessionNotFoundError) + expect(reachable.spawn).toHaveBeenCalledOnce() + expect(gone.listProcesses).toHaveBeenCalledOnce() + expect(isProvenGone).toHaveBeenCalledOnce() + }) + + it('keeps absence unverified when a failed provider is not proven gone', async () => { + const resolver = new DaemonSessionOwnerResolver( + [ + provider(async () => []), + provider(async () => { + throw new Error('wedged') + }) + ], + new Map(), + () => false + ) + + await expect( + resolver.spawnAttachOnly({ sessionId: 'missing', attachOnly: true, cols: 80, rows: 24 }) + ).rejects.toBeInstanceOf(TerminalSessionOwnerUnverifiedError) }) it('accepts positive liveness proof from an incomplete inventory', async () => { diff --git a/src/main/daemon/daemon-session-owner-resolution.ts b/src/main/daemon/daemon-session-owner-resolution.ts index 29d3d47c7d4..d0bc7326106 100644 --- a/src/main/daemon/daemon-session-owner-resolution.ts +++ b/src/main/daemon/daemon-session-owner-resolution.ts @@ -5,6 +5,7 @@ import type { PtySpawnResult } from '../providers/pty-provider-contract' import { SessionNotFoundError, TerminalSessionOwnerUnverifiedError } from './daemon-errors' +import { chooseSessionOwner } from './daemon-session-owner-choice' export type DaemonSessionOwnerResolution = | { kind: 'owner'; provider: T } @@ -40,31 +41,43 @@ export class DaemonSessionOwnerResolver { constructor( providers: readonly T[], - private readonly routes: Map + private readonly routes: Map, + private readonly isProviderProvenGone?: (provider: T, error: unknown) => boolean ) { this.providers = [...providers] } /** Removes a provider that can no longer be contacted, including from later inventories. */ dropProvider(provider: T): void { + this.forgetProvider(provider) + this.invalidateProvider(provider) + } + + // Why no epoch bump on its own: a proven-gone provider lists nothing, so in-flight inventories stay valid. + private forgetProvider(provider: T): void { const index = this.providers.indexOf(provider) if (index !== -1) { this.providers.splice(index, 1) } - this.invalidateProvider(provider) + this.failedProviderCooldowns.delete(provider) + this.forgetRoutesOf(provider) } - invalidateProvider(provider: T): void { - this.epoch += 1 - this.inventoryInFlight = null - this.cachedInventory = null - this.failedProviderCooldowns.clear() + private forgetRoutesOf(provider: T): void { for (const [sessionId, routed] of this.routes) { if (routed === provider) { this.routes.delete(sessionId) this.routeIncarnations.delete(sessionId) } } + } + + invalidateProvider(provider: T): void { + this.epoch += 1 + this.inventoryInFlight = null + this.cachedInventory = null + this.forgetRoutesOf(provider) + this.failedProviderCooldowns.clear() // Another resolver may already have removed this provider's shared routes. for (const sessionId of this.routeIncarnations.keys()) { if (!this.routes.has(sessionId)) { @@ -221,28 +234,17 @@ export class DaemonSessionOwnerResolver { expectedIncarnationIsAuthoritative = false ): DaemonSessionOwnerResolution { const candidates = inventory.candidatesBySessionId.get(sessionId) ?? [] - const providers = new Set(candidates.map(({ provider }) => provider)) - const exactProviders = new Set( - candidates - .filter(({ process }) => process.incarnationId === expectedIncarnationId) - .map(({ provider }) => provider) - ) - const exactProvider = - expectedIncarnationId && exactProviders.size === 1 - ? exactProviders.values().next().value - : undefined - const soleProvider = providers.size === 1 ? providers.values().next().value : undefined - const provider = - (exactProvider && (inventory.complete || expectedIncarnationIsAuthoritative) - ? exactProvider - : undefined) ?? - (!expectedIncarnationIsAuthoritative && inventory.complete ? soleProvider : undefined) - if (provider) { - const process = candidates.find((candidate) => candidate.provider === provider)?.process - this.recordRoute(sessionId, provider, process?.incarnationId) - return { kind: 'owner', provider } + const owner = chooseSessionOwner({ + candidates, + inventoryComplete: inventory.complete, + expectedIncarnationId, + expectedIncarnationIsAuthoritative + }) + if (owner) { + this.recordRoute(sessionId, owner.provider, owner.process.incarnationId) + return { kind: 'owner', provider: owner.provider } } - if (inventory.complete && providers.size === 0 && this.providers.length > 0) { + if (inventory.complete && candidates.length === 0 && this.providers.length > 0) { return { kind: 'absent' } } return { kind: 'unknown' } @@ -270,7 +272,12 @@ export class DaemonSessionOwnerResolver { this.failedProviderCooldowns.delete(provider) try { return { provider, processes: await provider.listProcesses({ deadlineMs }) } - } catch { + } catch (error) { + // A provider proven gone owns nothing; leaving it would keep every attach unverifiable. + if (this.isProviderProvenGone?.(provider, error)) { + this.forgetProvider(provider) + return { provider, processes: [] } + } if (epoch === this.epoch) { this.failedProviderCooldowns.set(provider, Date.now() + FAILED_PROVIDER_COOLDOWN_MS) } diff --git a/src/main/persistence/loading-store/pty-incarnation-renderer-save.test.ts b/src/main/persistence/loading-store/pty-incarnation-renderer-save.test.ts new file mode 100644 index 00000000000..e9dc8547115 --- /dev/null +++ b/src/main/persistence/loading-store/pty-incarnation-renderer-save.test.ts @@ -0,0 +1,69 @@ +import { expect, it, vi } from 'vitest' +import { fixture } from './profile-state-delayed-authority-fixture' +import { resolvePersistedStablePaneOwner } from '../../ipc/pty/pane/stable-owner' +import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' + +vi.mock('../../telemetry/client', () => ({ track: vi.fn() })) +vi.mock('../../telemetry/cohort-classifier', () => ({ + getCohortAtEmit: () => ({ nth_repo_added: 2 }) +})) +vi.mock('../../ssh/config/ssh-config-parser', () => ({ + loadUserSshConfig: () => ({ hosts: [] }), + sshConfigHostsToTargets: () => [] +})) + +const binding = { + worktreeId: 'repo-local::/fixture/local', + tabId: 'aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa', + leafId: 'bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb', + ptyId: 'repo-local::/fixture/local@@pane0001', + incarnationId: 'cccccccc-cccc-4ccc-8ccc-cccccccccccc' +} +const paneKey = `${binding.tabId}:${binding.leafId}` + +/** What the window saves: its own view of the session, which never carries incarnations. */ +function rendererSave(session: WorkspaceSessionState): WorkspaceSessionState { + const { terminalPtyIncarnationsByPaneKey: _omitted, ...rest } = session + return structuredClone(rest) +} + +// #30486: a renderer save erased the incarnation, so a restart reattached with no generation proof. +it('keeps the PTY incarnation across a window save that still binds the pane', async () => { + const { store } = await fixture() + await store.persistPtyBinding(binding) + + store.setWorkspaceSession(rendererSave(store.getWorkspaceSession())) + + expect(store.getWorkspaceSession().terminalPtyIncarnationsByPaneKey).toEqual({ + [paneKey]: binding.incarnationId + }) + expect(resolvePersistedStablePaneOwner(store, paneKey, binding.worktreeId, null)).toMatchObject({ + ptyId: binding.ptyId, + incarnationId: binding.incarnationId + }) +}) + +it('keeps other panes incarnations when a save carries only some of them', async () => { + const { store } = await fixture() + await store.persistPtyBinding(binding) + const saved = rendererSave(store.getWorkspaceSession()) + saved.terminalPtyIncarnationsByPaneKey = { 'other-tab:dddddddd-dddd-4ddd-8ddd-dddddddddddd': 'x' } + + store.setWorkspaceSession(saved) + + expect(store.getWorkspaceSession().terminalPtyIncarnationsByPaneKey?.[paneKey]).toBe( + binding.incarnationId + ) +}) + +it('drops the incarnation once a window save binds the pane to another PTY', async () => { + const { store } = await fixture() + await store.persistPtyBinding(binding) + const saved = rendererSave(store.getWorkspaceSession()) + saved.terminalLayoutsByTabId![binding.tabId]!.ptyIdsByLeafId![binding.leafId] = + 'repo-local::/fixture/local@@pane0002' + + store.setWorkspaceSession(saved) + + expect(store.getWorkspaceSession().terminalPtyIncarnationsByPaneKey?.[paneKey]).toBeUndefined() +}) diff --git a/src/main/persistence/runtime-authored-workspace-session-fields.ts b/src/main/persistence/runtime-authored-workspace-session-fields.ts index 80a9d30b532..7fbccd0d9c6 100644 --- a/src/main/persistence/runtime-authored-workspace-session-fields.ts +++ b/src/main/persistence/runtime-authored-workspace-session-fields.ts @@ -1,4 +1,5 @@ import type { WorkspaceSessionState } from '../../shared/workspace-session-state-types' +import { parsePaneKey } from '../../shared/stable-pane-id' /** * Keeps session fields the renderer persist snapshot does not author across a full write. @@ -30,6 +31,39 @@ function unionWriteOnceDefaultTerminalTabsApplied( return { ...next, defaultTerminalTabsAppliedByWorktreeId: merged } } +/** + * Keeps main's PTY incarnation for each pane the write still binds to the same PTY. Why: the + * renderer never carries this map, so its save erased it and every restart reattached without the + * generation proof that disambiguates an older daemon that cannot answer (#30486). + */ +function keepPtyIncarnationsForUnchangedBindings( + next: WorkspaceSessionState, + prior: WorkspaceSessionState | null | undefined +): WorkspaceSessionState { + const priorIncarnations = prior?.terminalPtyIncarnationsByPaneKey + if (!prior || !priorIncarnations) { + return next + } + const incoming = next.terminalPtyIncarnationsByPaneKey + // Per pane: a write may carry only some panes' incarnations. + const kept: Record = {} + for (const [paneKey, incarnationId] of Object.entries(priorIncarnations)) { + const parsed = parsePaneKey(paneKey) + if (!parsed || (incoming && Object.hasOwn(incoming, paneKey))) { + continue + } + const bindingOf = (session: WorkspaceSessionState): string | undefined => + session.terminalLayoutsByTabId?.[parsed.tabId]?.ptyIdsByLeafId?.[parsed.leafId] + const priorPtyId = bindingOf(prior) + if (priorPtyId !== undefined && bindingOf(next) === priorPtyId) { + kept[paneKey] = incarnationId + } + } + return Object.keys(kept).length > 0 + ? { ...next, terminalPtyIncarnationsByPaneKey: { ...kept, ...incoming } } + : next +} + export function preserveRuntimeAuthoredWorkspaceSessionFields( next: WorkspaceSessionState, prior: WorkspaceSessionState | null | undefined @@ -56,5 +90,6 @@ export function preserveRuntimeAuthoredWorkspaceSessionFields( } // Why union: persist snapshots omit this write-once map (empty Zustand slice, omit-when-empty // payload). Treating omission as "never applied" re-spawns default terminals on every attach. + result = keepPtyIncarnationsForUnchangedBindings(result, prior) return unionWriteOnceDefaultTerminalTabsApplied(result, prior) } diff --git a/src/renderer/src/components/terminal-pane/TerminalErrorToast.test.ts b/src/renderer/src/components/terminal-pane/TerminalErrorToast.test.ts index 6fc7607354a..3b0b8781412 100644 --- a/src/renderer/src/components/terminal-pane/TerminalErrorToast.test.ts +++ b/src/renderer/src/components/terminal-pane/TerminalErrorToast.test.ts @@ -434,6 +434,26 @@ describe('TerminalErrorToast environment footer', () => { expect(view.container.textContent).toContain('Retry could not reconnect yet') ) }) + + // #30486: Retry was the only action, so a pane whose owner stays unverifiable looped on it. + it('offers a new terminal beside Retry and leaves this pane alone', () => { + const onRetry = vi.fn().mockResolvedValue(false) + const onOpenNewTerminal = vi.fn() + const onDismiss = vi.fn() + const view = render( + React.createElement(TerminalErrorToast, { + error: 'terminal_pane_owner_unverified', + onDismiss, + onRetry, + onOpenNewTerminal + }) + ) + + fireEvent.click(view.getByRole('button', { name: 'Open new terminal' })) + expect(onOpenNewTerminal).toHaveBeenCalledOnce() + expect(onRetry).not.toHaveBeenCalled() + expect(onDismiss).not.toHaveBeenCalled() + }) }) // The shape a missing folder workspace reaches the toast in when its terminal spawn is rejected. diff --git a/src/renderer/src/components/terminal-pane/TerminalErrorToast.tsx b/src/renderer/src/components/terminal-pane/TerminalErrorToast.tsx index 546f5ddd99e..5d0a5f313b5 100644 --- a/src/renderer/src/components/terminal-pane/TerminalErrorToast.tsx +++ b/src/renderer/src/components/terminal-pane/TerminalErrorToast.tsx @@ -203,7 +203,8 @@ export function TerminalErrorToast({ paneOnClient = true, onDismiss, onRestartDaemon, - onRetry + onRetry, + onOpenNewTerminal }: { error: string /** False for a pane on an SSH or remote host, or one whose host is not yet known. */ @@ -211,6 +212,8 @@ export function TerminalErrorToast({ onDismiss: () => void onRestartDaemon?: () => void onRetry?: () => Promise + /** Leaves this pane and its saved session untouched. */ + onOpenNewTerminal?: () => void }): React.JSX.Element { const ssh = isSshHostStateTerminalError(error) // Why: the client's OS and shell describe neither the host nor its shell, and the renderer knows neither. @@ -373,6 +376,15 @@ export function TerminalErrorToast({ : translate('auto.components.terminal.pane.TerminalErrorToast.retry', 'Retry')} ) : null} + {/* Why: an owner can stay unverifiable, so Retry alone can loop forever. */} + {paneOwnerUnverified && onOpenNewTerminal ? ( + + ) : null}