diff --git a/src/main/providers/local-pty-provider.test.ts b/src/main/providers/local-pty-provider.test.ts index a7b675e78da..3f99d7db886 100644 --- a/src/main/providers/local-pty-provider.test.ts +++ b/src/main/providers/local-pty-provider.test.ts @@ -112,9 +112,24 @@ import { LOCAL_PTY_PHYSICAL_EXIT_TIMEOUT_MS, LocalPtyProvider } from './local-pty-provider' +import type { NodePtyModule } from './node-pty-loader' import { isRootLikePath } from './pty-path-safety' import { POWERLEVEL10K_WIZARD_DISABLE_ENV } from '../pty/powerlevel10k-wizard-env' +function createHeldNodePtyLoad() { + let finish!: (module: NodePtyModule) => void + const load = vi.fn( + () => + new Promise((resolve) => { + finish = resolve + }) + ) + return { + load, + finish: () => finish({ spawn: spawnMock } as unknown as NodePtyModule) + } +} + describe('LocalPtyProvider', () => { let provider: LocalPtyProvider let mockProc: { @@ -400,6 +415,66 @@ describe('LocalPtyProvider', () => { expect(spawnMock).toHaveBeenCalledOnce() }) + it('does not spawn when shutdown wins during the native module load', async () => { + const heldLoad = createHeldNodePtyLoad() + provider = new LocalPtyProvider({ loadNodePty: heldLoad.load }) + spawnMock.mockClear() + + const spawn = provider.spawn({ + cols: 80, + rows: 24, + sessionId: 'native-load-shutdown' + }) + const canceledSpawn = expect(spawn).rejects.toThrow( + 'PTY spawn canceled: native-load-shutdown' + ) + await vi.waitFor(() => expect(heldLoad.load).toHaveBeenCalledOnce()) + + await provider.shutdown('native-load-shutdown', { immediate: true }) + heldLoad.finish() + + await canceledSpawn + expect(spawnMock).not.toHaveBeenCalled() + }) + + it('does not spawn into a stale renderer generation after the native module load', async () => { + const heldLoad = createHeldNodePtyLoad() + provider = new LocalPtyProvider({ loadNodePty: heldLoad.load }) + spawnMock.mockClear() + + const spawn = provider.spawn({ cols: 80, rows: 24, sessionId: 'stale-load' }) + const canceledSpawn = expect(spawn).rejects.toThrow('PTY spawn canceled: stale-load') + await vi.waitFor(() => expect(heldLoad.load).toHaveBeenCalledOnce()) + + provider.advanceGeneration() + heldLoad.finish() + + await canceledSpawn + expect(spawnMock).not.toHaveBeenCalled() + }) + + it('revalidates client cancellation after the native module load', async () => { + const heldLoad = createHeldNodePtyLoad() + const abortController = new AbortController() + provider = new LocalPtyProvider({ loadNodePty: heldLoad.load }) + spawnMock.mockClear() + + const spawn = provider.spawn({ + cols: 80, + rows: 24, + sessionId: 'aborted-load', + signal: abortController.signal + }) + const canceledSpawn = expect(spawn).rejects.toThrow('client_disconnected') + await vi.waitFor(() => expect(heldLoad.load).toHaveBeenCalledOnce()) + + abortController.abort() + heldLoad.finish() + + await canceledSpawn + expect(spawnMock).not.toHaveBeenCalled() + }) + it('coalesces a concurrent same-session-id spawn before launching a redundant shell (F3)', async () => { spawnMock.mockClear() const procA = { ...mockProc, pid: 1001 } diff --git a/src/main/providers/local-pty-provider.ts b/src/main/providers/local-pty-provider.ts index d1ba44e5f2e..1935893c853 100644 --- a/src/main/providers/local-pty-provider.ts +++ b/src/main/providers/local-pty-provider.ts @@ -10,7 +10,7 @@ import { import { buildWindowsPowerShellSpawnAttempts } from './windows-shell-fallback-chain' import { resolveProcessCwd } from './process-cwd' import { existsSync } from 'node:fs' -import * as pty from 'node-pty' +import type * as pty from 'node-pty' import { getDefaultWslDistro, parseWslPath, isWslAvailableAsync } from '../wsl' import { splitWorktreeIdForFilesystem } from '../../shared/worktree-id' import { @@ -78,6 +78,7 @@ import { expandWindowsEnvironmentVariables, expandWindowsPathEnvironmentVariables } from '../../shared/windows-environment-expansion' +import { loadNodePty, type NodePtyModule } from './node-pty-loader' const PANE_IDENTITY_ENV_KEYS = [ 'ORCA_PANE_KEY', @@ -341,8 +342,13 @@ function allocatePtyId(sessionId: string | undefined): string { return id } -async function prepareLocalPtySpawn(id: string): Promise { +async function prepareLocalPtySpawn( + id: string, + signal: AbortSignal | undefined, + load: () => Promise +): Promise<{ nodePty: NodePtyModule; generation: number }> { const pendingSpawn: PendingLocalPtySpawn = { canceled: false } + const generation = loadGeneration const pending = pendingLocalPtySpawns.get(id) ?? new Set() pending.add(pendingSpawn) pendingLocalPtySpawns.set(id, pending) @@ -352,6 +358,17 @@ async function prepareLocalPtySpawn(id: string): Promise { if (pendingSpawn.canceled) { throw new Error(`PTY spawn canceled: ${id}`) } + if (signal?.aborted) { + throw new Error('client_disconnected') + } + const nodePty = await load() + if (pendingSpawn.canceled || generation !== loadGeneration) { + throw new Error(`PTY spawn canceled: ${id}`) + } + if (signal?.aborted) { + throw new Error('client_disconnected') + } + return { nodePty, generation } } finally { pending.delete(pendingSpawn) if (pending.size === 0) { @@ -489,6 +506,8 @@ function requestPtyTermination(id: string, proc: pty.IPty): void { } export type LocalPtyProviderOptions = { + /** Test seam for holding the native load across lifecycle races. */ + loadNodePty?: () => Promise /** Why: `ctx.command` (pi/omp/claude) must drive overlay source-dir selection — a disk-presence fallback shadows the other agent's extensions. */ buildSpawnEnv?: ( id: string, @@ -838,10 +857,11 @@ export class LocalPtyProvider implements IPtyProvider { logHistoryInjection(worktreeId, historyResult) } - await prepareLocalPtySpawn(id) - if (args.signal?.aborted) { - throw new Error('client_disconnected') - } + const prepared = await prepareLocalPtySpawn( + id, + args.signal, + this.opts.loadNodePty ?? loadNodePty + ) // Why: another same-id request can win while this one awaits preflight; attach before launching a redundant shell. const concurrentWinner = reattachId ? reattachLocalPty(id, args.cols, args.rows) : null if (concurrentWinner) { @@ -855,7 +875,7 @@ export class LocalPtyProvider implements IPtyProvider { cwd: effectiveCwd, env: finalEnv, termName: finalEnv.TERM, - ptySpawn: pty.spawn, + ptySpawn: prepared.nodePty.spawn, getShellReadyConfig: getFallbackShellReadyConfig, // Why: on zsh→bash fallback HISTFILE still points to zsh_history; update before spawn so the child inherits it (design doc §8). onBeforeFallbackSpawn: historyResult?.histFile @@ -906,7 +926,7 @@ export class LocalPtyProvider implements IPtyProvider { id, getAgentForegroundContextPaths({ cwd: args.cwd, worktreeId: args.worktreeId }) ) - ptyLoadGeneration.set(id, loadGeneration) + ptyLoadGeneration.set(id, prepared.generation) ptyIncarnations.set(id, incarnationId) this.opts.onSpawned?.(id, incarnationId) diff --git a/src/main/providers/node-pty-loader.test.ts b/src/main/providers/node-pty-loader.test.ts new file mode 100644 index 00000000000..f5e78bf2d6d --- /dev/null +++ b/src/main/providers/node-pty-loader.test.ts @@ -0,0 +1,47 @@ +import { describe, expect, it, vi } from 'vitest' +import type { NodePtyModule } from './node-pty-loader' +import { createNodePtyLoader } from './node-pty-loader' + +function deferred(): { + promise: Promise + resolve: (value: T) => void +} { + let resolve!: (value: T) => void + const promise = new Promise((res) => { + resolve = res + }) + return { promise, resolve } +} + +describe('createNodePtyLoader', () => { + it('single-flights concurrent loads and caches success', async () => { + const pending = deferred() + const module = { spawn: vi.fn() } as unknown as NodePtyModule + const importer = vi.fn(() => pending.promise) + const load = createNodePtyLoader(importer) + + const first = load() + const second = load() + expect(importer).toHaveBeenCalledOnce() + + pending.resolve(module) + await expect(Promise.all([first, second])).resolves.toEqual([module, module]) + await expect(load()).resolves.toBe(module) + expect(importer).toHaveBeenCalledOnce() + }) + + it('preserves an actionable failure until process restart', async () => { + const nativeFailure = new Error('dlopen: incompatible architecture') + const importer = vi.fn(() => Promise.reject(nativeFailure)) + const load = createNodePtyLoader(importer) + + const firstFailure = (await load().catch((error: unknown) => error)) as Error + const secondFailure = (await load().catch((error: unknown) => error)) as Error + + expect(firstFailure).toBe(secondFailure) + expect(firstFailure).toMatchObject({ cause: nativeFailure }) + expect(firstFailure.message).toContain('dlopen: incompatible architecture') + expect(firstFailure.message).toContain('Restart Orca') + expect(importer).toHaveBeenCalledOnce() + }) +}) diff --git a/src/main/providers/node-pty-loader.ts b/src/main/providers/node-pty-loader.ts new file mode 100644 index 00000000000..fa13025fa2a --- /dev/null +++ b/src/main/providers/node-pty-loader.ts @@ -0,0 +1,43 @@ +import type * as NodePty from 'node-pty' + +export type NodePtyModule = typeof NodePty +export type NodePtyImporter = () => Promise + +function nodePtyLoadError(cause: unknown): Error { + const detail = cause instanceof Error ? cause.message : String(cause) + return new Error( + `Local terminals could not load node-pty (${detail}). Restart Orca after repairing or reinstalling its native dependencies.`, + { cause } + ) +} + +/** A failed native load is terminal for this process; restart is the only retry boundary. */ +export function createNodePtyLoader(importer: NodePtyImporter): () => Promise { + let loaded: NodePtyModule | undefined + let inFlight: Promise | undefined + let failure: Error | undefined + + return (): Promise => { + if (loaded) { + return Promise.resolve(loaded) + } + if (failure) { + return Promise.reject(failure) + } + if (!inFlight) { + inFlight = importer().then( + (module) => { + loaded = module + return module + }, + (cause: unknown) => { + failure = nodePtyLoadError(cause) + throw failure + } + ) + } + return inFlight + } +} + +export const loadNodePty = createNodePtyLoader(() => import('node-pty'))