From 4cb013c0a9251275fa3d20ea33b45429e07aa6be Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 28 Aug 2026 15:57:57 -0700 Subject: [PATCH] Never let a non-owning provider answer a PTY presence question false during the daemon swap window (#16953) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(pty): answer unverifiable, not false, for presence questions during the daemon swap window During cold start the installed local provider is still the plain in-process LocalPtyProvider until daemon-init swaps in the daemon router. It does not own restored daemon PTY ids, but pty:hasPty and the runtime controller's sync hasPty still let it answer — and its "not in my table" false read as an observed absence: the renderer dead-session reconciler tears panes down on exactly that false, remount recovery refuses on it, and terminal.list records an observed absence instead of an unverifiable verdict. - pty:hasPty now waits for the local-provider startup barrier before choosing an answering provider (the same #7742 guard pty:kill uses), so the post-swap owner answers. - hasPtyFromRuntimeController is sync and cannot wait; while the startup barrier is unsettled it answers null (unverifiable), and it inherits the async probe's remote-handle guard: no locally routed provider may answer for a paired runtime handle. - SSH-owned ids keep answering from their own provider without waiting, and a registration without a startup barrier (headless/orcad) keeps the in-process provider's false authoritative (#12393). * test(pty): isolate the remote-handle guard from the swap-window gate * refactor(pty): arm the swap-window settle watcher once per startup promise * Gate pty:inspectProcess on the daemon-swap startup barrier During the cold-start swap window the routed local provider is still the pre-swap LocalPtyProvider, which does not own restored daemon ids; its answer about one is fabricated, and today reads as unavailable only because the inspection funnel happens to consult hasPty before the provider's own inspection. Completion-sensitive inspection must not ride on that internal ordering: defer until the swap lands, exactly like pty:kill (#7742) and pty:hasPty. SSH-owned ids and no-barrier (headless/orcad sole-owner, #12393) registrations keep answering immediately. The thrice-repeated barrier idiom is now one helper. --- .../pty-startup-swap-window-presence.test.ts | 268 ++++++++++++++++++ src/main/ipc/pty/ipc/inspect.ts | 28 +- src/main/ipc/pty/runtime/controller.ts | 2 +- src/main/ipc/pty/runtime/operations.ts | 29 +- 4 files changed, 319 insertions(+), 8 deletions(-) create mode 100644 src/main/ipc/pty-startup-swap-window-presence.test.ts diff --git a/src/main/ipc/pty-startup-swap-window-presence.test.ts b/src/main/ipc/pty-startup-swap-window-presence.test.ts new file mode 100644 index 00000000000..d0a511e86cd --- /dev/null +++ b/src/main/ipc/pty-startup-swap-window-presence.test.ts @@ -0,0 +1,268 @@ +import { describe, expect, it, vi } from 'vitest' +import { makeDeferred } from './pty-ipc-test-constants' +import { setupPtyIpcSuite } from './pty-ipc-test-harness' +import { registerPtyHandlers, registerSshPtyProvider } from './pty' +import { ptyOwnership } from './pty/provider/ownership-state' + +vi.mock('electron', () => import('./pty-ipc-mock-registry').then((m) => m.electronModuleMock())) +vi.mock('fs', () => import('./pty-ipc-mock-registry').then((m) => m.fsModuleMock())) +vi.mock('node-pty', () => import('./pty-ipc-mock-registry').then((m) => m.nodePtyModuleMock())) +vi.mock('node:child_process', async (importOriginal) => + (await import('./pty-ipc-mock-registry')).childProcessModuleMock(await importOriginal()) +) +vi.mock('../opencode/hook-service', () => + import('./pty-ipc-mock-registry').then((m) => m.openCodeHookServiceModuleMock()) +) +vi.mock('../mimo/hook-service', () => + import('./pty-ipc-mock-registry').then((m) => m.mimoHookServiceModuleMock()) +) +vi.mock('../agent-hooks/server', () => + import('./pty-ipc-mock-registry').then((m) => m.agentHookServerModuleMock()) +) +vi.mock('../pi/titlebar-extension-service', () => + import('./pty-ipc-mock-registry').then((m) => m.piTitlebarExtensionModuleMock()) +) +vi.mock('../pwsh', () => import('./pty-ipc-mock-registry').then((m) => m.pwshModuleMock())) +vi.mock('../wsl', async (importOriginal) => + (await import('./pty-ipc-mock-registry')).wslModuleMock(await importOriginal()) +) +vi.mock('../telemetry/client', () => + import('./pty-ipc-mock-registry').then((m) => m.telemetryClientModuleMock()) +) +vi.mock('../telemetry/classify-error', () => + import('./pty-ipc-mock-registry').then((m) => m.classifyErrorModuleMock()) +) +vi.mock('../cli/linux-terminal-orca-cli-shim', () => + import('./pty-ipc-mock-registry').then((m) => m.linuxCliShimModuleMock()) +) +vi.mock('../memory/pty-registry', () => + import('./pty-ipc-mock-registry').then((m) => m.ptyRegistryModuleMock()) +) +vi.mock('../agent-hooks/migration-unsupported-pty-state', () => + import('./pty-ipc-mock-registry').then((m) => m.migrationUnsupportedPtyModuleMock()) +) +vi.mock('../codex/codex-pane-account-registry', () => + import('./pty-ipc-mock-registry').then((m) => m.codexPaneAccountRegistryModuleMock()) +) +vi.mock('../codex/codex-state-db-backfill-recovery', () => + import('./pty-ipc-mock-registry').then((m) => m.codexBackfillRecoveryModuleMock()) +) + +// During the cold-start daemon swap the installed local provider is still the plain +// in-process LocalPtyProvider; it does not own daemon-restored PTY ids, so its +// "no PTY" is fabricated, not observed. A confident false here tears down live +// panes (shouldReconcileMissingSession reconciles ONLY on false) and blocks +// input-undeliverable remount recovery. These suites pin: while the swap is in +// flight the presence answer is deferred (IPC) or unverifiable-null (sync), and +// the post-swap owner's answer is the one that lands. +describe('registerPtyHandlers daemon-swap-window presence', () => { + const { handlers, mainWindow, installDaemonTestProvider } = setupPtyIpcSuite() + + const registerWithStartupBarrier = ( + barrier: Promise, + runtime?: Record + ): void => { + registerPtyHandlers( + mainWindow as never, + runtime as never, + undefined, + undefined, + undefined, + undefined, + { awaitLocalPtyProviderStartup: () => barrier } + ) + } + + const installRuntimeControllerWithBarrier = ( + barrier: Promise + ): { hasPty: (ptyId: string) => boolean | null } => { + let controller: { hasPty: (ptyId: string) => boolean | null } | undefined + registerWithStartupBarrier(barrier, { + setPtyController: vi.fn((next) => { + controller = next + }), + registerPty: vi.fn(), + onPtySpawned: vi.fn(), + onPtyExit: vi.fn(), + onPtyData: vi.fn() + }) + if (!controller) { + throw new Error('runtime controller was not installed') + } + return controller + } + + it('pty:hasPty defers a restored daemon id until the provider swap lands instead of answering a pre-swap false', async () => { + const barrier = makeDeferred() + registerWithStartupBarrier(barrier.promise) + + const pending = Promise.resolve( + handlers.get('pty:hasPty')!(null, { id: 'daemon-restored-pty' }) + ) as Promise + let settled = false + void pending.then(() => { + settled = true + }) + + await Promise.resolve() + await Promise.resolve() + // Pre-fix this has already resolved false — the pre-swap LocalPtyProvider + // answered for a PTY it does not own, and the renderer reconciler treats + // exactly that false as authority to tear the pane down. + expect(settled).toBe(false) + + installDaemonTestProvider({ hasPty: (id: string) => id === 'daemon-restored-pty' }) + barrier.resolve() + + await expect(pending).resolves.toBe(true) + }) + + it('pty:hasPty answers SSH-owned ids from their provider without waiting on the local swap', async () => { + const barrier = makeDeferred() + const sshHasPty = vi.fn((id: string) => id === 'ssh:ssh-1@@pty-2') + registerSshPtyProvider('ssh-1', { hasPty: sshHasPty } as never) + registerWithStartupBarrier(barrier.promise) + + await expect(handlers.get('pty:hasPty')!(null, { id: 'ssh:ssh-1@@pty-2' })).resolves.toBe(true) + expect(sshHasPty).toHaveBeenCalledWith('ssh:ssh-1@@pty-2') + }) + + it('runtime controller hasPty answers null, not false, while the local provider swap is in flight', async () => { + const barrier = makeDeferred() + const controller = installRuntimeControllerWithBarrier(barrier.promise) + + // Pre-fix: the pre-swap LocalPtyProvider's ptyProcesses.has() answers a + // confident false for a daemon-owned id. terminal.list then records an + // observed absence (verdict forgotten) instead of unverifiable. + expect(controller.hasPty('daemon-restored-pty')).toBe(null) + + installDaemonTestProvider({ hasPty: (id: string) => id === 'daemon-restored-pty' }) + barrier.resolve() + + await vi.waitFor(() => { + expect(controller.hasPty('daemon-restored-pty')).toBe(true) + }) + }) + + it('runtime controller hasPty never answers a paired-runtime handle from the local registry', () => { + // No startup barrier: the remote-handle guard must hold on its own, not + // ride on the swap-window gate. Same routing hazard the async probe and + // pty:hasPty already guard — no locally routed provider can + // authoritatively answer for a remote host's PTY, so remote-scoped ids + // stay unknown, never absent. + let controller: { hasPty: (ptyId: string) => boolean | null } | undefined + registerPtyHandlers( + mainWindow as never, + { + setPtyController: vi.fn((next) => { + controller = next + }), + registerPty: vi.fn(), + onPtySpawned: vi.fn(), + onPtyExit: vi.fn(), + onPtyData: vi.fn() + } as never + ) + + expect(controller?.hasPty('remote:environment@@pty-1')).toBe(null) + }) + + it('runtime controller hasPty answers SSH-owned ids without waiting on the local swap', () => { + const barrier = makeDeferred() + const sshHasPty = vi.fn((id: string) => id === 'ssh-live-pty') + registerSshPtyProvider('ssh-1', { hasPty: sshHasPty } as never) + ptyOwnership.set('ssh-live-pty', 'ssh-1') + try { + const controller = installRuntimeControllerWithBarrier(barrier.promise) + + expect(controller.hasPty('ssh-live-pty')).toBe(true) + expect(sshHasPty).toHaveBeenCalledWith('ssh-live-pty') + } finally { + ptyOwnership.delete('ssh-live-pty') + } + }) + + it('pty:inspectProcess defers a restored daemon id until the provider swap lands instead of answering from the non-owning provider', async () => { + const barrier = makeDeferred() + registerWithStartupBarrier(barrier.promise) + + const pending = Promise.resolve( + handlers.get('pty:inspectProcess')!(null, { id: 'daemon-restored-pty' }) + ) + let settled = false + void pending.then(() => { + settled = true + }) + + await Promise.resolve() + await Promise.resolve() + await Promise.resolve() + // Pre-fix this has already resolved — the pre-swap LocalPtyProvider was + // consulted about a PTY it does not own. Its non-ownership happens to read + // as unavailable today only because the inspection funnel consults hasPty + // before the provider's own inspection; completion-sensitive evidence must + // come from the post-swap owner, not from that internal ordering. + expect(settled).toBe(false) + + installDaemonTestProvider({ + hasPty: (id: string) => id === 'daemon-restored-pty', + inspectProcess: vi.fn(async () => ({ + foregroundProcess: 'codex', + hasChildProcesses: true + })) + }) + barrier.resolve() + + await expect(pending).resolves.toEqual({ + foregroundProcess: 'codex', + hasChildProcesses: true + }) + }) + + it('pty:inspectProcess answers SSH-owned ids from their provider without waiting on the local swap', async () => { + const barrier = makeDeferred() + const sshInspect = vi.fn(async () => ({ + foregroundProcess: 'ssh-codex', + hasChildProcesses: true + })) + registerSshPtyProvider('ssh-1', { + hasPty: (id: string) => id === 'ssh:ssh-1@@pty-2', + inspectProcess: sshInspect + } as never) + registerWithStartupBarrier(barrier.promise) + + await expect( + handlers.get('pty:inspectProcess')!(null, { id: 'ssh:ssh-1@@pty-2' }) + ).resolves.toEqual({ foregroundProcess: 'ssh-codex', hasChildProcesses: true }) + expect(sshInspect).toHaveBeenCalledWith('ssh:ssh-1@@pty-2') + }) + + it('keeps the in-process provider authoritative when no startup barrier is configured', async () => { + // Headless/orcad installs the daemon before registerPtyHandlers and passes + // no barrier; the installed provider is then the sole owner (#12393) and + // its false stays an observed absence. + let controller: { hasPty: (ptyId: string) => boolean | null } | undefined + registerPtyHandlers( + mainWindow as never, + { + setPtyController: vi.fn((next) => { + controller = next + }), + registerPty: vi.fn(), + onPtySpawned: vi.fn(), + onPtyExit: vi.fn(), + onPtyData: vi.fn() + } as never + ) + + expect(controller?.hasPty('never-spawned-pty')).toBe(false) + await expect(handlers.get('pty:hasPty')!(null, { id: 'never-spawned-pty' })).resolves.toBe( + false + ) + // The sole owner's inspection answer stays immediate too: with no swap in + // flight there is no window in which its word could be fabricated. + await expect( + handlers.get('pty:inspectProcess')!(null, { id: 'never-spawned-pty' }) + ).resolves.toEqual({ foregroundProcess: null, hasChildProcesses: false, unavailable: true }) + }) +}) diff --git a/src/main/ipc/pty/ipc/inspect.ts b/src/main/ipc/pty/ipc/inspect.ts index 96c84b45c74..9f99d1e099c 100644 --- a/src/main/ipc/pty/ipc/inspect.ts +++ b/src/main/ipc/pty/ipc/inspect.ts @@ -29,6 +29,16 @@ export function installPtyInspectIpcHandlers(deps: { const ipcMain = getPtyIpc() const { getLocalPtyProviderStartupPromise } = deps + // Why: wait for daemon startup before selecting the local provider for an id + // the swap may re-own (#7742); ids owned by an SSH connection never wait. + // renderer-kill.ts inlines this — pty:kill's listener teardown is + // ordering-sensitive and must not gain even a no-barrier microtask. + const awaitSwapWindow = async (id: string): Promise => { + await getLocalPtyProviderStartupPromise( + ptyOwnership.get(id) ?? parseAppSshPtyId(id)?.connectionId + ) + } + ipcMain.handle('pty:listSessions', async (): Promise => { const deduped = new Map() const admission = new PtyProcessListAdmission() @@ -117,6 +127,10 @@ export function installPtyInspectIpcHandlers(deps: { // authoritative dead. That is a fabricated answer about another host's PTY. return null } + // Why: the pre-swap LocalPtyProvider does not own restored daemon ids, and + // its "no PTY" is exactly the false the renderer reconciler is allowed to + // close panes on. + await awaitSwapWindow(args.id) const ownedConnectionId = ptyOwnership.get(args.id) const parsedSshId = ownedConnectionId === undefined ? parseAppSshPtyId(args.id) : null const provider = parsedSshId @@ -155,12 +169,14 @@ export function installPtyInspectIpcHandlers(deps: { ipcMain.handle('pty:inspectProcess', async (_event, args: { id: string }) => { // Why: same routing hazard as pty:hasPty — an unroutable id must read as unavailable, not as a local-provider answer or a raised IPC error. - if ( - typeof args?.id !== 'string' || - !args.id || - args.id.startsWith('remote:') || - !hasPtyProviderForInspection(args.id) - ) { + if (typeof args?.id !== 'string' || !args.id || args.id.startsWith('remote:')) { + return { foregroundProcess: null, hasChildProcesses: false, unavailable: true as const } + } + // Why: the pre-swap LocalPtyProvider does not own restored daemon ids, so + // nothing it reports about one is an observation; the post-swap owner must + // answer completion-sensitive inspection. + await awaitSwapWindow(args.id) + if (!hasPtyProviderForInspection(args.id)) { return { foregroundProcess: null, hasChildProcesses: false, unavailable: true as const } } return inspectPtyProviderProcessForRenderer(getProviderForPty(args.id), args.id) diff --git a/src/main/ipc/pty/runtime/controller.ts b/src/main/ipc/pty/runtime/controller.ts index ff5a0e5ccd0..19adaec77de 100644 --- a/src/main/ipc/pty/runtime/controller.ts +++ b/src/main/ipc/pty/runtime/controller.ts @@ -60,7 +60,7 @@ export function installPtyRuntimeController(deps: PtyRuntimeControllerDeps): voi getCwd: (ptyId) => getCwdFromRuntimeController(ptyId), hasChildProcesses: (ptyId) => hasChildProcessesFromRuntimeController(ptyId), clearBuffer: (ptyId) => clearBufferFromRuntimeController(deps, ptyId), - hasPty: (ptyId) => hasPtyFromRuntimeController(ptyId), + hasPty: (ptyId) => hasPtyFromRuntimeController(deps, ptyId), listProcesses: (connectionId, opts) => listProcessesFromRuntimeController(deps, connectionId, opts), listProcessesWithHostScope: (opts) => diff --git a/src/main/ipc/pty/runtime/operations.ts b/src/main/ipc/pty/runtime/operations.ts index bf3ca74af25..bb8032f9cfb 100644 --- a/src/main/ipc/pty/runtime/operations.ts +++ b/src/main/ipc/pty/runtime/operations.ts @@ -152,8 +152,35 @@ export async function clearBufferFromRuntimeController( } } -export function hasPtyFromRuntimeController(ptyId: string): boolean | null { +const settledLocalPtyProviderStartups = new WeakSet>() +const watchedLocalPtyProviderStartups = new WeakSet>() + +export function hasPtyFromRuntimeController( + deps: PtyRuntimeControllerDeps, + ptyId: string +): boolean | null { try { + // Why: no locally routed provider can authoritatively answer for a + // remote host's PTY, so remote-scoped ids stay unknown, never absent. + if (ptyId.startsWith('remote:')) { + return null + } + const connectionId = ptyOwnership.get(ptyId) ?? parseAppSshPtyId(ptyId)?.connectionId + const startupPromise = deps.getLocalPtyProviderStartupPromise(connectionId) + if (startupPromise && !settledLocalPtyProviderStartups.has(startupPromise)) { + // Why: a sync probe cannot wait out the cold-start daemon swap the way + // probePtyLiveness does, and the pre-swap provider's "no PTY" for a + // daemon-restored id is fabricated — answer unverifiable until the swap + // settles (docs/reference/ssh-execution-boundary.md rule 2). + if (!watchedLocalPtyProviderStartups.has(startupPromise)) { + watchedLocalPtyProviderStartups.add(startupPromise) + const markSettled = (): void => { + settledLocalPtyProviderStartups.add(startupPromise) + } + startupPromise.then(markSettled, markSettled) + } + return null + } return getProviderForPty(ptyId).hasPty?.(ptyId) ?? null } catch { return null