From f2850e6fc97ea98c2761a6c265813bf8469a1ef2 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 31 Jul 2026 00:58:26 -0700 Subject: [PATCH] fix(daemon): require authoritative recovery liveness --- .../daemon-spawn-route-validation.test.ts | 28 +++++++++++++++++++ .../daemon/degraded-daemon-session-routing.ts | 1 + .../degraded-fallback-session-routes.ts | 17 +++++++++-- src/main/ipc/pty.test.ts | 26 +++++++++++++---- src/main/ipc/pty.ts | 7 +++-- 5 files changed, 69 insertions(+), 10 deletions(-) diff --git a/src/main/daemon/daemon-spawn-route-validation.test.ts b/src/main/daemon/daemon-spawn-route-validation.test.ts index a597e409af4..6e50e926626 100644 --- a/src/main/daemon/daemon-spawn-route-validation.test.ts +++ b/src/main/daemon/daemon-spawn-route-validation.test.ts @@ -75,3 +75,31 @@ it('rejects a degraded existing spawn after its selected daemon is replaced', as await expect(spawning).rejects.toThrow('daemon_session_routing_unavailable') }) + +it('rejects an existing fallback spawn when daemon ownership appears before its reply', async () => { + const sessionId = 'fallback-owner-race' + const current = createDaemonAdapter('daemon') + const fallback = createProvider('fallback') + const provider = new DegradedDaemonPtyProvider({ current, legacy: [], fallback }) + await provider.spawn({ sessionId, isNewSession: true, cols: 80, rows: 24 }) + let resolveSpawn!: (result: PtySpawnResult) => void + let markSpawnStarted!: () => void + const spawnStarted = new Promise((resolve) => { + markSpawnStarted = resolve + }) + vi.mocked(fallback.spawn).mockImplementationOnce( + () => + new Promise((resolve) => { + markSpawnStarted() + resolveSpawn = resolve + }) + ) + + const spawning = provider.spawn({ sessionId, cols: 80, rows: 24 }) + await spawnStarted + current.emitData(sessionId, 'daemon ownership') + resolveSpawn({ id: sessionId }) + + await expect(spawning).rejects.toThrow('daemon_session_routing_unavailable') + expect(() => provider.write(sessionId, 'blocked')).toThrow('daemon_session_routing_unavailable') +}) diff --git a/src/main/daemon/degraded-daemon-session-routing.ts b/src/main/daemon/degraded-daemon-session-routing.ts index ee17e3f0835..9afe7efc6a9 100644 --- a/src/main/daemon/degraded-daemon-session-routing.ts +++ b/src/main/daemon/degraded-daemon-session-routing.ts @@ -75,6 +75,7 @@ export class DegradedDaemonSessionRouting { if (!this.fallbackSpawns.hasExited(result.id)) { this.fallbackRoutes.recordSpawnedSession(result.id, target) } + this.fallbackRoutes.assertExistingSpawnAvailable(opts, result.id) } else { this.fallbackRoutes.recordDaemonSpawn(result.id, opts.sessionId, target as DaemonPtyAdapter) } diff --git a/src/main/daemon/degraded-fallback-session-routes.ts b/src/main/daemon/degraded-fallback-session-routes.ts index a3758e08fda..39a412229d2 100644 --- a/src/main/daemon/degraded-fallback-session-routes.ts +++ b/src/main/daemon/degraded-fallback-session-routes.ts @@ -1,6 +1,6 @@ -import type { IPtyProvider } from '../providers/types' +import type { IPtyProvider, PtySpawnOptions } from '../providers/types' import type { DaemonPtyAdapter } from './daemon-pty-adapter' -import { sameDaemonIncarnation } from './daemon-session-route' +import { DaemonSessionOwnerUnknownError, sameDaemonIncarnation } from './daemon-session-route' import type { DaemonSessionRouteTable } from './daemon-session-route-table' import { recordValidatedDaemonSpawn } from './daemon-spawn-route-validation' import type { DegradedFallbackSpawnRoutes } from './degraded-fallback-spawn-routes' @@ -44,6 +44,19 @@ export class DegradedFallbackSessionRoutes { } } + assertExistingSpawnAvailable(opts: PtySpawnOptions, resultSessionId: string): void { + if (!opts.sessionId || opts.isNewSession) { + return + } + const hasDaemonOwner = [opts.sessionId, resultSessionId].some((sessionId) => { + const route = this.daemonRoutes.get(sessionId) + return route !== undefined && route.state !== 'unavailable' + }) + if (hasDaemonOwner) { + throw new DaemonSessionOwnerUnknownError(opts.sessionId) + } + } + recordDaemonSpawn( resultSessionId: string, requestedSessionId: string | undefined, diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index f074f5f00a9..ba353cd9b24 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -6299,11 +6299,14 @@ describe('registerPtyHandlers', () => { }) it('checks single-PTY liveness without listing every session', async () => { - const hasPty = vi.fn((id: string) => id === 'live-pty') + const probePtyLiveness = vi.fn(async (id: string) => (id === 'live-pty' ? true : null)) + const hasPty = vi.fn((_id: string): boolean => { + throw new Error('cached ownership is not authoritative') + }) const listProcesses = vi.fn(async () => { throw new Error('listProcesses should not be called') }) - setLocalPtyProvider({ + const provider = { spawn: vi.fn(), write: vi.fn(), resize: vi.fn(), @@ -6323,16 +6326,27 @@ describe('registerPtyHandlers', () => { listProcesses, attach: vi.fn(), hasPty, + probePtyLiveness, getDefaultShell: vi.fn(), getProfiles: vi.fn() - } as never) + } + setLocalPtyProvider(provider as never) registerPtyHandlers(mainWindow as never) await expect(handlers.get('pty:hasPty')!(null, { id: 'live-pty' })).resolves.toBe(true) - await expect(handlers.get('pty:hasPty')!(null, { id: 'dead-pty' })).resolves.toBe(false) + await expect(handlers.get('pty:hasPty')!(null, { id: 'uncertain-pty' })).resolves.toBe(null) - expect(hasPty).toHaveBeenCalledWith('live-pty') - expect(hasPty).toHaveBeenCalledWith('dead-pty') + expect(probePtyLiveness).toHaveBeenCalledTimes(2) + expect(probePtyLiveness).toHaveBeenNthCalledWith(1, 'live-pty') + expect(probePtyLiveness).toHaveBeenNthCalledWith(2, 'uncertain-pty') + expect(hasPty).not.toHaveBeenCalled() + expect(listProcesses).not.toHaveBeenCalled() + + provider.probePtyLiveness = undefined as never + hasPty.mockImplementation((id: string) => id === 'cached-live-pty') + await expect(handlers.get('pty:hasPty')!(null, { id: 'cached-pty' })).resolves.toBe(false) + + expect(hasPty).toHaveBeenCalledExactlyOnceWith('cached-pty') expect(listProcesses).not.toHaveBeenCalled() }) diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 75fc48112a7..247b842363b 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -6163,11 +6163,14 @@ export function registerPtyHandlers( const provider = parsedSshId ? sshProviders.get(parsedSshId.connectionId) : tryGetProviderForPty(args.id) - if (!provider?.hasPty) { + if (!provider) { return null } try { - return provider.hasPty(args.id) + const live = provider.probePtyLiveness + ? await provider.probePtyLiveness(args.id) + : provider.hasPty?.(args.id) + return typeof live === 'boolean' ? live : null } catch { // Why: liveness is only allowed to close panes on an authoritative false. return null