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}