mirror of
https://github.com/stablyai/orca.git
synced 2026-09-24 00:02:24 +00:00
Never let a non-owning provider answer a PTY presence question false during the daemon swap window (#16953)
* 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.
This commit is contained in:
@@ -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<void>,
|
||||
runtime?: Record<string, unknown>
|
||||
): void => {
|
||||
registerPtyHandlers(
|
||||
mainWindow as never,
|
||||
runtime as never,
|
||||
undefined,
|
||||
undefined,
|
||||
undefined,
|
||||
undefined,
|
||||
{ awaitLocalPtyProviderStartup: () => barrier }
|
||||
)
|
||||
}
|
||||
|
||||
const installRuntimeControllerWithBarrier = (
|
||||
barrier: Promise<void>
|
||||
): { 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<boolean | null>
|
||||
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 })
|
||||
})
|
||||
})
|
||||
@@ -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<void> => {
|
||||
await getLocalPtyProviderStartupPromise(
|
||||
ptyOwnership.get(id) ?? parseAppSshPtyId(id)?.connectionId
|
||||
)
|
||||
}
|
||||
|
||||
ipcMain.handle('pty:listSessions', async (): Promise<PtyListedSession[]> => {
|
||||
const deduped = new Map<string, PtyListedSession>()
|
||||
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)
|
||||
|
||||
@@ -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) =>
|
||||
|
||||
@@ -152,8 +152,35 @@ export async function clearBufferFromRuntimeController(
|
||||
}
|
||||
}
|
||||
|
||||
export function hasPtyFromRuntimeController(ptyId: string): boolean | null {
|
||||
const settledLocalPtyProviderStartups = new WeakSet<Promise<void>>()
|
||||
const watchedLocalPtyProviderStartups = new WeakSet<Promise<void>>()
|
||||
|
||||
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
|
||||
|
||||
Reference in New Issue
Block a user