diff --git a/config/scripts/windows-pty-table-stress.cjs b/config/scripts/windows-pty-table-stress.cjs index f6623f22eeb..36d6bf20af8 100644 --- a/config/scripts/windows-pty-table-stress.cjs +++ b/config/scripts/windows-pty-table-stress.cjs @@ -43,7 +43,6 @@ async function exerciseTable() { useConptyDll: true }) const record = { proc, output: '', exited: false, closed: false } - const marker = `ORCA_PTY_READY_${spawned.length}` let resolveReady record.ready = new Promise((resolve) => { resolveReady = resolve @@ -55,16 +54,14 @@ async function exerciseTable() { resolveExit(event) }) }) + // First output is node-pty's own readiness, after which kill/resize/clear stop deferring. + // Not typed input: ConPTY rarely starts without an input reader, which the daemon respawns. proc.onData((chunk) => { record.output = (record.output + chunk).slice(-2048) - if (record.output.includes(marker)) { - resolveReady(true) - } + resolveReady(true) }) observer.watch(record, { round, slot }) spawned.push(record) - // Escaping one letter keeps echoed input from satisfying the output marker. - proc.write(`echo ${marker.replace('READY', 'REA^DY')}\r`) return record } diff --git a/src/main/daemon/pty-subprocess-foreground-degraded-scan.test.ts b/src/main/daemon/pty-subprocess-foreground-degraded-scan.test.ts index 559b36b503b..cee9a07f157 100644 --- a/src/main/daemon/pty-subprocess-foreground-degraded-scan.test.ts +++ b/src/main/daemon/pty-subprocess-foreground-degraded-scan.test.ts @@ -44,17 +44,22 @@ vi.mock('../providers/windows-pty-job-membership', () => ({ })) import { createPtySubprocess } from './pty-subprocess' +import { createConptyPreludeSender } from './pty-subprocess-test-harness' const BASE_TIME_MS = 1_000_000 function mockPtyProcess(processName: string, pid = 12345) { + const sendConptyPrelude = createConptyPreludeSender() return { pid, write: vi.fn(), resize: vi.fn(), kill: vi.fn(), process: processName, - onData: vi.fn(() => ({ dispose: vi.fn() })), + onData: vi.fn((cb: (data: string) => void) => { + sendConptyPrelude(cb) + return { dispose: vi.fn() } + }), onExit: vi.fn(() => ({ dispose: vi.fn() })) } } diff --git a/src/main/daemon/pty-subprocess-foreground-scan-cadence.test.ts b/src/main/daemon/pty-subprocess-foreground-scan-cadence.test.ts index 57acdcd8ad1..96193097e37 100644 --- a/src/main/daemon/pty-subprocess-foreground-scan-cadence.test.ts +++ b/src/main/daemon/pty-subprocess-foreground-scan-cadence.test.ts @@ -50,12 +50,14 @@ vi.mock('../providers/agent-foreground-process', () => ({ })) import { createPtySubprocess } from './pty-subprocess' +import { createConptyPreludeSender } from './pty-subprocess-test-harness' // Well past any hot window or retry throttle so t=0 reads behave as idle. const BASE_TIME_MS = 1_000_000 function mockPtyProcess(processName: string, pid = 12345) { const onDataListeners: ((data: string) => void)[] = [] + const sendConptyPrelude = createConptyPreludeSender() return { pid, write: vi.fn(), @@ -64,6 +66,7 @@ function mockPtyProcess(processName: string, pid = 12345) { process: processName, onData: vi.fn((cb: (data: string) => void) => { onDataListeners.push(cb) + sendConptyPrelude(cb) return { dispose: vi.fn() } }), onExit: vi.fn(() => ({ dispose: vi.fn() })), @@ -126,7 +129,10 @@ describe('daemon pty foreground scan cadence', () => { Object.defineProperty(process, 'platform', { configurable: true, value: targetPlatform }) const proc = mockPtyProcess(shellProcessName) spawnMock.mockReturnValue(proc) + // Startup output (ConPTY DA1 query) lands before the 10s hot window these idle cases measure. + vi.setSystemTime(BASE_TIME_MS - 11_000) const handle = await createPtySubprocess({ sessionId: 'test', cols: 80, rows: 24 }) + vi.setSystemTime(BASE_TIME_MS) return { proc, handle } } diff --git a/src/main/daemon/pty-subprocess-handle-lifecycle.test.ts b/src/main/daemon/pty-subprocess-handle-lifecycle.test.ts index ae22a58140e..75b2ec56d37 100644 --- a/src/main/daemon/pty-subprocess-handle-lifecycle.test.ts +++ b/src/main/daemon/pty-subprocess-handle-lifecycle.test.ts @@ -69,7 +69,11 @@ vi.mock('../providers/windows-pty-job-membership', () => ({ })) import { createPtySubprocess } from './pty-subprocess' -import { mockPtyProcess, useDaemonPtySubprocessEnv } from './pty-subprocess-test-harness' +import { + mockConptyStartupOutput, + mockPtyProcess, + useDaemonPtySubprocessEnv +} from './pty-subprocess-test-harness' import { __setConptyJobNativeForTests } from '../windows/windows-pty-job' describe('createPtySubprocess', () => { @@ -210,7 +214,7 @@ describe('createPtySubprocess', () => { handle.onData((d) => data.push(d)) proc._simulateData('hello') - expect(data).toEqual(['hello']) + expect(data).toEqual([...mockConptyStartupOutput(), 'hello']) }) it('replays data emitted before the Session registers onData', async () => { @@ -222,7 +226,7 @@ describe('createPtySubprocess', () => { const data: string[] = [] handle.onData((d) => data.push(d)) - expect(data).toEqual(['early setup output\r\n']) + expect(data).toEqual([...mockConptyStartupOutput(), 'early setup output\r\n']) }) it('routes onExit events', async () => { @@ -249,7 +253,7 @@ describe('createPtySubprocess', () => { handle.onData((d) => data.push(d)) handle.onExit((code) => codes.push(code)) - expect(data).toEqual(['last output\r\n']) + expect(data).toEqual([...mockConptyStartupOutput(), 'last output\r\n']) expect(codes).toEqual([7]) }) @@ -264,7 +268,11 @@ describe('createPtySubprocess', () => { handle.onExit((code) => events.push(`exit:${code}`)) handle.onData((data) => events.push(`data:${data}`)) - expect(events).toEqual(['exit:7', 'data:last output\r\n']) + expect(events).toEqual([ + 'exit:7', + ...mockConptyStartupOutput().map((data) => `data:${data}`), + 'data:last output\r\n' + ]) }) it('sends signal via process.kill', async () => { diff --git a/src/main/daemon/pty-subprocess-test-harness.ts b/src/main/daemon/pty-subprocess-test-harness.ts index 1f4e038c498..9b8b70ca1aa 100644 --- a/src/main/daemon/pty-subprocess-test-harness.ts +++ b/src/main/daemon/pty-subprocess-test-harness.ts @@ -47,9 +47,28 @@ export type MockPtyProcess = { _simulateExit: (code: number) => void } +const CONPTY_MOCK_PRELUDE = '\x1b[c' + +/** Windows spawns wait for the DA1 query bundled ConPTY writes once it reads input. */ +export function createConptyPreludeSender(): (cb: (data: string) => void) => void { + let sent = false + return (cb) => { + if (process.platform === 'win32' && !sent) { + sent = true + queueMicrotask(() => cb(CONPTY_MOCK_PRELUDE)) + } + } +} + +/** What a mocked pane printed before the test's own output; the replayed prelude on Windows. */ +export function mockConptyStartupOutput(): string[] { + return process.platform === 'win32' ? [CONPTY_MOCK_PRELUDE] : [] +} + export function mockPtyProcess(pid = 12345): MockPtyProcess { const onDataListeners: ((data: string) => void)[] = [] const onExitListeners: ((e: { exitCode: number }) => void)[] = [] + const sendConptyPrelude = createConptyPreludeSender() return { pid, write: vi.fn(), @@ -58,6 +77,7 @@ export function mockPtyProcess(pid = 12345): MockPtyProcess { process: 'zsh', onData: vi.fn((cb: (data: string) => void) => { onDataListeners.push(cb) + sendConptyPrelude(cb) return { dispose: vi.fn() } }), onExit: vi.fn((cb: (e: { exitCode: number }) => void) => { diff --git a/src/main/daemon/pty-subprocess-windows-shell-launch.test.ts b/src/main/daemon/pty-subprocess-windows-shell-launch.test.ts index e9a334a7c09..bbf2437274b 100644 --- a/src/main/daemon/pty-subprocess-windows-shell-launch.test.ts +++ b/src/main/daemon/pty-subprocess-windows-shell-launch.test.ts @@ -82,6 +82,62 @@ describe('createPtySubprocess', () => { validateWorkingDirectoryMock }) + it('delivers output and exit that land while ConPTY startup is still being confirmed', async () => { + const proc = mockPtyProcess() + // The shell dies before ConPTY announces its input reader, so nothing has subscribed yet. + // Each event is emitted once, to whoever listens at that moment, as node-pty does. + proc.onData.mockImplementationOnce((cb: (data: string) => void) => { + queueMicrotask(() => cb('\x1b[1t')) + return { dispose: vi.fn() } + }) + proc.onExit.mockImplementationOnce((cb: (e: { exitCode: number }) => void) => { + queueMicrotask(() => queueMicrotask(() => cb({ exitCode: 7 }))) + return { dispose: vi.fn() } + }) + spawnMock.mockReturnValue(proc) + const platform = Object.getOwnPropertyDescriptor(process, 'platform') + Object.defineProperty(process, 'platform', { value: 'win32' }) + try { + const handle = await createPtySubprocess({ sessionId: 'test', cols: 80, rows: 24 }) + const data: string[] = [] + const exitCode = new Promise((resolve) => handle.onExit((code) => resolve(code))) + handle.onData((chunk) => data.push(chunk)) + expect(await exitCode).toBe(7) + expect(data[0]).toBe('\x1b[1t') + expect(spawnMock).toHaveBeenCalledOnce() + } finally { + if (platform) { + Object.defineProperty(process, 'platform', platform) + } + } + }) + + it('never respawns a dead ConPTY pane whose user shell args may run a command', async () => { + const proc = mockPtyProcess() + proc.onData.mockImplementationOnce((cb: (data: string) => void) => { + queueMicrotask(() => cb('Microsoft Windows\r\n')) + return { dispose: vi.fn() } + }) + spawnMock.mockReturnValue(proc) + const platform = Object.getOwnPropertyDescriptor(process, 'platform') + Object.defineProperty(process, 'platform', { value: 'win32' }) + try { + await createPtySubprocess({ + sessionId: 'test', + cols: 80, + rows: 24, + env: { COMSPEC: CMD_ABS }, + terminalShellArgs: ['/c', 'deploy.cmd'] + }) + } finally { + if (platform) { + Object.defineProperty(process, 'platform', platform) + } + } + expect(spawnMock).toHaveBeenCalledOnce() + expect(proc.kill).not.toHaveBeenCalled() + }) + it('keeps powershell.exe when the inbox PowerShell implementation is selected on Windows', async () => { const proc = mockPtyProcess() spawnMock.mockReturnValue(proc) diff --git a/src/main/daemon/pty-subprocess.ts b/src/main/daemon/pty-subprocess.ts index c67511027b5..1594fda4713 100644 --- a/src/main/daemon/pty-subprocess.ts +++ b/src/main/daemon/pty-subprocess.ts @@ -85,9 +85,17 @@ export async function createPtySubprocess(opts: PtySubprocessOptions): Promise | 'exited' | 'undecided' + +// A console shell connects, prompting the announcement, within milliseconds; under heavy CI load +// within ~4s. Past this the pane is handed out unverified rather than stalling the spawn reply. +export const CONPTY_STARTUP_DECISION_LIMIT_MS = 5_000 + +export const CONPTY_NO_INPUT_READER_NOTICE = + '\r\n\x1b[31m[Orca] Windows could not start keyboard input for this terminal. Close it and open a new one.\x1b[0m\r\n' + +export function classifyConptyStartupOutput(output: string): ConptyStartupVerdict { + let rest = output + for (;;) { + const report = WINDOW_VISIBILITY_REPORTS.find((candidate) => rest.startsWith(candidate)) + if (!report) { + break + } + rest = rest.slice(report.length) + } + if (INPUT_READER_ANNOUNCEMENTS.some((announcement) => rest.startsWith(announcement))) { + return 'input-reader' + } + // A chunk can end mid-sequence; only output that rules out every expected prefix decides. + const expected = [...INPUT_READER_ANNOUNCEMENTS, ...WINDOW_VISIBILITY_REPORTS] + return expected.some((sequence) => sequence.startsWith(rest)) ? 'pending' : 'no-input-reader' +} + +export type ConptyStartupEvents = { + data: string[] + exit: { exitCode: number; signal?: number } | null +} + +export type ConptyStartupWatch = { + /** Settles on the first output that decides, or on exit before any did. */ + verdict: Promise + /** Hands buffered output and exit to the caller and stops buffering. */ + claim(): ConptyStartupEvents +} + +/** Buffers a fresh ConPTY's output until its owner claims it, so no startup byte is lost. */ +export function watchConptyStartup(proc: pty.IPty): ConptyStartupWatch { + const events: ConptyStartupEvents = { data: [], exit: null } + let output = '' + let settle: (outcome: ConptyStartupOutcome) => void = () => {} + const verdict = new Promise((resolve) => { + settle = resolve + }) + let decided = false + const dataSubscription = proc.onData((data) => { + events.data.push(data) + if (decided) { + return + } + output += data + const classified = classifyConptyStartupOutput(output) + if (classified !== 'pending') { + decided = true + settle(classified) + } + }) + const exitSubscription = proc.onExit(({ exitCode, signal }) => { + events.exit = { exitCode, ...(signal !== undefined ? { signal } : {}) } + decided = true + settle('exited') + }) + return { + verdict, + claim: () => { + dataSubscription.dispose() + exitSubscription.dispose() + return events + } + } +} + +async function decideConptyStartup( + proc: pty.IPty, + signal: AbortSignal | undefined +): Promise<{ outcome: ConptyStartupOutcome; claim: () => ConptyStartupEvents }> { + const watch = watchConptyStartup(proc) + let limit: ReturnType | undefined + let onAbort = (): void => {} + try { + signal?.throwIfAborted() + const outcome = await new Promise((resolve, reject) => { + onAbort = () => reject(signal?.reason) + signal?.addEventListener('abort', onAbort, { once: true }) + limit = setTimeout(() => resolve('undecided'), CONPTY_STARTUP_DECISION_LIMIT_MS) + limit.unref?.() + void watch.verdict.then(resolve) + }) + return { outcome, claim: watch.claim } + } catch (error) { + // A canceled spawn must not leave the half-started shell running. + discardConpty(proc, watch) + throw error + } finally { + clearTimeout(limit) + signal?.removeEventListener('abort', onAbort) + } +} + +function discardConpty(proc: pty.IPty, watch: Pick): void { + watch.claim() + // Handle-based only: a pid may already be recycled if the shell exited during startup. + terminatePtyJob(proc) + killConptyNow(proc) + proc.kill() +} + +// Why process-wide: the announcement is a fact of the bundled ConPTY build. Until one pane shows +// it, a missing one may mean a newer build changed its startup, not that input is dead. +let announcementObserved = false +let announcementUntrusted = false +// Logged with each respawn so a misfiring check shows up as a climbing count in the daemon log. +let respawnCount = 0 + +/** Test seam: reset the process-wide trust in the announcement. */ +export function _resetConptyStartupTrustForTest(): void { + announcementObserved = false + announcementUntrusted = false + respawnCount = 0 +} + +function withNotice(claim: () => ConptyStartupEvents): () => ConptyStartupEvents { + return () => { + const events = claim() + return { ...events, data: [...events.data, CONPTY_NO_INPUT_READER_NOTICE] } + } +} + +/** + * Spawns a Windows ConPTY pane and confirms its console host reads input before anyone can write. + * Nothing holds the pane until this returns, so a respawn never repeats a command typed into it. + */ +export async function spawnConptyWithInputReader( + spawn: () => Promise, + opts: { + label: string + signal?: AbortSignal + /** Respawn only shells started with Orca's own interactive argv: any other argv may run a + * command (`pwsh -Command`, `cmd /c`, a startup command), and a respawn would run it twice. */ + respawnSafe: boolean + } +): Promise<{ proc: pty.IPty; claimStartupEvents: () => ConptyStartupEvents }> { + const first = await spawn() + if (announcementUntrusted) { + return { proc: first, claimStartupEvents: () => ({ data: [], exit: null }) } + } + const firstStartup = await decideConptyStartup(first, opts.signal) + if (firstStartup.outcome === 'input-reader') { + announcementObserved = true + } + if (firstStartup.outcome !== 'no-input-reader') { + return { proc: first, claimStartupEvents: firstStartup.claim } + } + if (!opts.respawnSafe) { + console.warn(`[daemon/pty] ConPTY for "${opts.label}" started without an input reader`) + const claim = firstStartup.claim + return { proc: first, claimStartupEvents: announcementObserved ? withNotice(claim) : claim } + } + respawnCount += 1 + console.warn( + `[daemon/pty] ConPTY for "${opts.label}" started without an input reader; respawning (${respawnCount})` + ) + discardConpty(first, firstStartup) + const retry = await spawn() + const retryStartup = await decideConptyStartup(retry, opts.signal) + if (retryStartup.outcome === 'input-reader') { + announcementObserved = true + } + if (retryStartup.outcome !== 'no-input-reader') { + return { proc: retry, claimStartupEvents: retryStartup.claim } + } + if (!announcementObserved) { + // Two misses and no pane ever announced: distrust the check rather than respawn every pane. + announcementUntrusted = true + console.warn('[daemon/pty] ConPTY never announced an input reader; disabling the startup check') + return { proc: retry, claimStartupEvents: retryStartup.claim } + } + console.warn(`[daemon/pty] ConPTY for "${opts.label}" again started without an input reader`) + // After one respawn, show the failure on screen instead of silently ignoring keystrokes. + return { proc: retry, claimStartupEvents: withNotice(retryStartup.claim) } +} diff --git a/src/main/daemon/pty-subprocess/native-pty-spawn-windows.test.ts b/src/main/daemon/pty-subprocess/native-pty-spawn-windows.test.ts index a63ea778b88..ac2d021b270 100644 --- a/src/main/daemon/pty-subprocess/native-pty-spawn-windows.test.ts +++ b/src/main/daemon/pty-subprocess/native-pty-spawn-windows.test.ts @@ -2,9 +2,19 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const nodePty = vi.hoisted(() => ({ spawn: vi.fn() })) vi.mock('node-pty', () => nodePty) -vi.mock('../../windows/windows-pty-job', () => ({ assignHostProcessToKillOnCloseJob: vi.fn() })) +const ptyJob = vi.hoisted(() => ({ + assignHostProcessToKillOnCloseJob: vi.fn(), + terminatePtyJob: vi.fn(), + killConptyNow: vi.fn() +})) +vi.mock('../../windows/windows-pty-job', () => ptyJob) import { spawnNativeDaemonPty } from './native-pty-spawn' +import { + _resetConptyStartupTrustForTest, + CONPTY_NO_INPUT_READER_NOTICE, + CONPTY_STARTUP_DECISION_LIMIT_MS +} from './conpty-startup-input-reader' const attempts = ['pwsh.exe', 'powershell.exe', 'cmd.exe'].map((shellPath) => ({ shellPath, @@ -14,6 +24,7 @@ const attempts = ['pwsh.exe', 'powershell.exe', 'cmd.exe'].map((shellPath) => ({ startupCommandDeliveredInShellArgs: true })) const args = { + primaryArgsRespawnSafe: true, shellPath: attempts[0]!.shellPath, shellArgs: attempts[0]!.shellArgs, spawnCwd: 'C:\\work', @@ -23,12 +34,52 @@ const args = { windowsFallbackAttempts: attempts } +const CONPTY_PRELUDE = '\x1b[1t\x1b[c\x1b[?1004h\x1b[?9001h' + +/** A pane that writes `chunks` on later ticks, as ConPTY's output socket does. */ +function fakePane(pid: number, chunks: string[], exitCode?: number) { + const dataListeners = new Set<(data: string) => void>() + const exitListeners = new Set<(event: { exitCode: number }) => void>() + const pane = { + pid, + kill: vi.fn(), + onData: (listener: (data: string) => void) => { + dataListeners.add(listener) + return { dispose: () => dataListeners.delete(listener) } + }, + onExit: (listener: (event: { exitCode: number }) => void) => { + exitListeners.add(listener) + return { dispose: () => exitListeners.delete(listener) } + } + } + void (async () => { + for (const chunk of chunks) { + await new Promise((resolve) => setImmediate(resolve)) + for (const listener of dataListeners) { + listener(chunk) + } + } + if (exitCode !== undefined) { + await new Promise((resolve) => setImmediate(resolve)) + for (const listener of exitListeners) { + listener({ exitCode }) + } + } + })() + return pane +} + +const healthyPane = (pid: number) => fakePane(pid, [CONPTY_PRELUDE]) +const deadPane = (pid: number) => fakePane(pid, ['Microsoft Windows\r\n']) + describe('Windows shell fallback over node-pty ConPTY', () => { const platform = Object.getOwnPropertyDescriptor(process, 'platform')! beforeEach(() => { Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) vi.spyOn(console, 'warn').mockImplementation(() => {}) nodePty.spawn.mockReset() + ptyJob.terminatePtyJob.mockReset() + _resetConptyStartupTrustForTest() }) afterEach(() => { Object.defineProperty(process, 'platform', platform) @@ -36,7 +87,7 @@ describe('Windows shell fallback over node-pty ConPTY', () => { }) it('walks both fallback shells when ConPTY rejects the primary ones', async () => { - const proc = { pid: 9876 } + const proc = fakePane(9876, [CONPTY_PRELUDE]) nodePty.spawn.mockImplementation((file: string) => { if (file !== 'cmd.exe') { throw new Error(`spawn ${file} EACCES`) @@ -55,6 +106,104 @@ describe('Windows shell fallback over node-pty ConPTY', () => { expect(result.startupCommandDeliveredInShellArgs).toBe(true) }) + it('respawns a pane whose ConPTY never started reading input', async () => { + // Measured on CI: such a pane prints its banner and prompt, then ignores every keystroke. + const broken = fakePane(990001, ['Microsoft Windows [Version 10.0.20348]\r\n']) + const healthy = fakePane(990002, ['\x1b[1t', '\x1b[c\x1b[?1004h', 'Microsoft Windows']) + nodePty.spawn.mockReturnValueOnce(broken).mockReturnValueOnce(healthy) + + const result = await spawnNativeDaemonPty(args) + + expect(nodePty.spawn.mock.calls.map(([file]) => file)).toEqual(['pwsh.exe', 'pwsh.exe']) + expect(ptyJob.terminatePtyJob).toHaveBeenCalledWith(broken) + expect(broken.kill).toHaveBeenCalledOnce() + expect(result.process).toBe(healthy) + expect(result.shellPath).toBe('pwsh.exe') + expect(result.claimStartupEvents?.().data).toEqual(['\x1b[1t', '\x1b[c\x1b[?1004h']) + }) + + it('respawns only once, then says so in the pane', async () => { + nodePty.spawn.mockReturnValueOnce(healthyPane(990001)) + await spawnNativeDaemonPty(args) + const first = deadPane(990002) + const second = deadPane(990003) + nodePty.spawn.mockReturnValueOnce(first).mockReturnValueOnce(second) + + const result = await spawnNativeDaemonPty(args) + + expect(nodePty.spawn).toHaveBeenCalledTimes(3) + expect(second.kill).not.toHaveBeenCalled() + expect(result.process).toBe(second) + expect(result.claimStartupEvents?.().data).toEqual([ + 'Microsoft Windows\r\n', + CONPTY_NO_INPUT_READER_NOTICE + ]) + }) + + it('never respawns a shell whose argv may carry a command', async () => { + nodePty.spawn.mockReturnValueOnce(healthyPane(990001)) + await spawnNativeDaemonPty(args) + const dead = deadPane(990002) + nodePty.spawn.mockReturnValueOnce(dead) + + const result = await spawnNativeDaemonPty({ ...args, primaryArgsRespawnSafe: false }) + + expect(nodePty.spawn).toHaveBeenCalledTimes(2) + expect(dead.kill).not.toHaveBeenCalled() + expect(result.claimStartupEvents?.().data.at(-1)).toBe(CONPTY_NO_INPUT_READER_NOTICE) + }) + + it('stops checking when no pane ever announces, as a changed ConPTY build would', async () => { + nodePty.spawn.mockReturnValueOnce(deadPane(990001)).mockReturnValueOnce(deadPane(990002)) + const result = await spawnNativeDaemonPty(args) + expect(result.claimStartupEvents?.().data).not.toContain(CONPTY_NO_INPUT_READER_NOTICE) + + const later = deadPane(990003) + nodePty.spawn.mockReturnValueOnce(later) + expect((await spawnNativeDaemonPty(args)).process).toBe(later) + expect(nodePty.spawn).toHaveBeenCalledTimes(3) + }) + + it('does not respawn a shell that exited before ConPTY announced anything', async () => { + const exited = fakePane(990001, [], 3) + nodePty.spawn.mockReturnValueOnce(exited) + + const result = await spawnNativeDaemonPty(args) + + expect(nodePty.spawn).toHaveBeenCalledOnce() + expect(result.claimStartupEvents?.().exit).toEqual({ exitCode: 3 }) + }) + + it('kills the pane tree when the spawn is canceled while startup is awaited', async () => { + const controller = new AbortController() + const silent = fakePane(990001, []) + nodePty.spawn.mockReturnValueOnce(silent) + const spawned = spawnNativeDaemonPty({ ...args, signal: controller.signal }) + await new Promise((resolve) => setImmediate(resolve)) + controller.abort(new Error('spawn canceled')) + + await expect(spawned).rejects.toThrow('spawn canceled') + expect(ptyJob.terminatePtyJob).toHaveBeenCalledWith(silent) + // node-pty defers kill() until first output, which a silent pane never produces. + expect(ptyJob.killConptyNow).toHaveBeenCalledWith(silent) + expect(silent.kill).toHaveBeenCalledOnce() + expect(nodePty.spawn).toHaveBeenCalledOnce() + }) + + it('hands out a pane that stays silent instead of stalling the spawn', async () => { + vi.useFakeTimers() + const silent = fakePane(990001, []) + nodePty.spawn.mockReturnValueOnce(silent) + const spawned = spawnNativeDaemonPty(args) + await vi.advanceTimersByTimeAsync(CONPTY_STARTUP_DECISION_LIMIT_MS) + const result = await spawned + vi.useRealTimers() + + expect(nodePty.spawn).toHaveBeenCalledOnce() + expect(silent.kill).not.toHaveBeenCalled() + expect(result.process).toBe(silent) + }) + it('starts no fallback shell once the spawn is canceled', async () => { const controller = new AbortController() nodePty.spawn.mockImplementation(() => { diff --git a/src/main/daemon/pty-subprocess/native-pty-spawn.ts b/src/main/daemon/pty-subprocess/native-pty-spawn.ts index 47b0a9289e7..22c6edd779d 100644 --- a/src/main/daemon/pty-subprocess/native-pty-spawn.ts +++ b/src/main/daemon/pty-subprocess/native-pty-spawn.ts @@ -6,6 +6,7 @@ import { import type { WindowsShellSpawnAttempt } from '../../providers/windows-shell-fallback-chain' import { assignHostProcessToKillOnCloseJob } from '../../windows/windows-pty-job' import { wrapPtySpawnInTerminalScope } from '../terminal-scopes-runtime' +import { spawnConptyWithInputReader, type ConptyStartupEvents } from './conpty-startup-input-reader' async function loadNodePty(): Promise { return import('node-pty') @@ -17,8 +18,12 @@ export type SpawnedDaemonPty = { startupCommandDeliveredInShellArgs?: boolean /** False when a wrapper owns the reported status, so no exit code may be read from it. */ reportsChildExitStatus: boolean + /** Windows: output and exit seen while confirming ConPTY reads input; claim before subscribing. */ + claimStartupEvents?: () => ConptyStartupEvents } +type SpawnedNativePty = { proc: pty.IPty; claimStartupEvents?: () => ConptyStartupEvents } + export async function spawnNativeDaemonPty(args: { shellPath: string shellArgs: string[] @@ -27,11 +32,13 @@ export async function spawnNativeDaemonPty(args: { cols: number rows: number windowsFallbackAttempts: WindowsShellSpawnAttempt[] + /** The primary shell's argv is Orca's interactive default, so a respawn cannot repeat a command. */ + primaryArgsRespawnSafe?: boolean signal?: AbortSignal onMacosTccSpawnStrategy?: (strategy: 'wrapped' | 'direct') => void }): Promise { let reportsChildExitStatus = true - const spawnAt = async ( + const spawnOnce = async ( shellPath: string, shellArgs: string[], cwd: string @@ -57,14 +64,35 @@ export async function spawnNativeDaemonPty(args: { args.onMacosTccSpawnStrategy?.(tcc.file === shellPath ? 'direct' : 'wrapped') return proc } + const spawnAt = async ( + shellPath: string, + shellArgs: string[], + cwd: string, + respawnSafe: boolean + ): Promise => { + if (process.platform !== 'win32') { + return { proc: await spawnOnce(shellPath, shellArgs, cwd) } + } + return spawnConptyWithInputReader(() => spawnOnce(shellPath, shellArgs, cwd), { + label: shellPath, + respawnSafe, + ...(args.signal ? { signal: args.signal } : {}) + }) + } try { - const process_ = await spawnAt(args.shellPath, args.shellArgs, args.spawnCwd) + const spawned = await spawnAt( + args.shellPath, + args.shellArgs, + args.spawnCwd, + args.primaryArgsRespawnSafe === true + ) return { - process: process_, + process: spawned.proc, shellPath: args.shellPath, spawnCwd: args.spawnCwd, - reportsChildExitStatus + reportsChildExitStatus, + ...(spawned.claimStartupEvents ? { claimStartupEvents: spawned.claimStartupEvents } : {}) } } catch (primaryErr) { args.signal?.throwIfAborted() @@ -73,17 +101,23 @@ export async function spawnNativeDaemonPty(args: { } for (const attempt of args.windowsFallbackAttempts.slice(1)) { try { - const process = await spawnAt(attempt.shellPath, attempt.shellArgs, attempt.effectiveCwd) + const spawned = await spawnAt( + attempt.shellPath, + attempt.shellArgs, + attempt.effectiveCwd, + args.primaryArgsRespawnSafe === true && !attempt.startupCommandDeliveredInShellArgs + ) const message = primaryErr instanceof Error ? primaryErr.message : String(primaryErr) console.warn( `[daemon/pty] Primary shell "${args.shellPath}" failed (${message}), fell back to "${attempt.shellPath}"` ) return { - process, + process: spawned.proc, shellPath: attempt.shellPath, spawnCwd: attempt.effectiveCwd, startupCommandDeliveredInShellArgs: attempt.startupCommandDeliveredInShellArgs, - reportsChildExitStatus + reportsChildExitStatus, + ...(spawned.claimStartupEvents ? { claimStartupEvents: spawned.claimStartupEvents } : {}) } } catch { args.signal?.throwIfAborted() diff --git a/src/main/daemon/pty-subprocess/subprocess-handle.ts b/src/main/daemon/pty-subprocess/subprocess-handle.ts index e03ed1cafd9..18c5f4ed194 100644 --- a/src/main/daemon/pty-subprocess/subprocess-handle.ts +++ b/src/main/daemon/pty-subprocess/subprocess-handle.ts @@ -11,6 +11,7 @@ import { createPtyForegroundProcessTracker } from './foreground-process-tracker' import { PtyPreListenerEvents } from './pre-listener-events' import { ptyProcessNameIsSpawnFile } from './spawn-file-foreground-process' import { inspectSpawnFileWindowsChildProcesses } from './spawn-file-child-processes' +import type { ConptyStartupEvents } from './conpty-startup-input-reader' type DisposableNativePty = pty.IPty & { destroy?: () => void @@ -34,6 +35,8 @@ export function createDaemonPtySubprocessHandle(args: { requestedCwd?: string sessionId: string startupAgentRecognition: RecognizedAgentProcess | null + /** Output and exit buffered before this handle subscribed; replayed ahead of live events. */ + startupEvents?: ConptyStartupEvents }): SubprocessHandle { const reportsChildExitStatus = args.reportsChildExitStatus const shellPath = args.shellPath @@ -77,10 +80,12 @@ export function createDaemonPtySubprocessHandle(args: { isDead: () => dead }) - proc.onData((data) => { + const acceptData = (data: string): void => { foreground.recordOutput(data) events.acceptData(data) - }) + } + args.startupEvents?.data.forEach(acceptData) + proc.onData(acceptData) const retireMaster = (): void => { masterRetired = true if (dead || masterRetiredTimer) { @@ -100,7 +105,11 @@ export function createDaemonPtySubprocessHandle(args: { } }) nativeProc.on?.('close', retireMaster) - proc.onExit(({ exitCode, signal }) => { + // node-pty may have retired its master while Windows startup was confirmed, before these listeners. + if (args.startupEvents && nativeProc._writable === false) { + retireMaster() + } + const acceptExit = ({ exitCode, signal }: { exitCode: number; signal?: number }): void => { // Exit listeners may re-enter cleanup; retire signal authority before notifying them. dead = true clearMasterRetiredTimer() @@ -114,7 +123,12 @@ export function createDaemonPtySubprocessHandle(args: { signal, hostReportsChildExitStatus: reportsChildExitStatus }) - }) + } + if (args.startupEvents?.exit) { + acceptExit(args.startupEvents.exit) + } else { + proc.onExit(acceptExit) + } const slavePath = readPtySlavePath(proc) return { diff --git a/src/main/providers/agent-foreground-process-git-bash.win32.test.ts b/src/main/providers/agent-foreground-process-git-bash.win32.test.ts index 4c774ce0d92..4321f480925 100644 --- a/src/main/providers/agent-foreground-process-git-bash.win32.test.ts +++ b/src/main/providers/agent-foreground-process-git-bash.win32.test.ts @@ -30,7 +30,7 @@ async function proveIdlePromptCycle( expectedJobSize: number ): Promise { const proc = spawned.process - let output = '' + let output = spawned.claimStartupEvents?.().data.join('') ?? '' let dead = false proc.onData((chunk) => { output += chunk @@ -164,7 +164,7 @@ describeOnWindows("Git Bash launcher shell proof with Orca's real launch", () => const env = createDaemonPtyEnvironment(opts) const spawned = await spawnPlannedPane(createPtyShellLaunchPlan(opts, env), env, opts) const proc = spawned.process - let output = '' + let output = spawned.claimStartupEvents?.().data.join('') ?? '' proc.onData((chunk) => { output += chunk }) diff --git a/src/main/windows/windows-pty-job.ts b/src/main/windows/windows-pty-job.ts index 8f08ea6d914..8c7b3db91a2 100644 --- a/src/main/windows/windows-pty-job.ts +++ b/src/main/windows/windows-pty-job.ts @@ -255,3 +255,19 @@ export function __setConptyJobNativeForTests(loader?: () => ConptyNative | null) cachedNative = undefined hostJobAssigned = null } + +type UnreadyWindowsPty = IPty & { _agent?: { kill?: () => void } } + +/** + * Tears a ConPTY pane down now. node-pty defers its own kill() until the pane's first output, so + * a pane discarded before printing would keep its console host, input pipe and output worker. + * The agent's kill closes all three; node-pty's later deferred kill finds nothing left to close. + */ +export function killConptyNow(proc: IPty): void { + const pane: UnreadyWindowsPty = proc + try { + pane._agent?.kill?.() + } catch { + // Best effort; the host job still reaps the console host with the daemon. + } +} diff --git a/src/main/windows/windows-pty-job.win32.test.ts b/src/main/windows/windows-pty-job.win32.test.ts index 514f0a1ba2e..f04fb0a5047 100644 --- a/src/main/windows/windows-pty-job.win32.test.ts +++ b/src/main/windows/windows-pty-job.win32.test.ts @@ -6,9 +6,11 @@ import type { IPty } from 'node-pty' import { runProcess } from '@orca/process-host' import { isPtyJobOwnershipAvailable, + killConptyNow, listPtyJobProcessIds, terminatePtyJob } from './windows-pty-job' +import { watchConptyStartup } from '../daemon/pty-subprocess/conpty-startup-input-reader' /** * The unit tests pin the contract; this pins the thing the contract is for. @@ -101,6 +103,36 @@ describeOnWindows('ConPTY job ownership', () => { return { proc, grandchildPid } } + it('announces an input reader the daemon recognizes', async () => { + // A ConPTY upgrade that changes its startup must fail here, not respawn every pane. + const nodePty = await import('node-pty') + const proc = nodePty.spawn('cmd.exe', ['/d', '/q'], { + cols: 80, + rows: 24, + cwd: process.cwd(), + useConptyDll: true + }) + spawned.push(proc) + const startup = watchConptyStartup(proc) + expect(await startup.verdict).toBe('input-reader') + startup.claim() + }, 30_000) + + it('tears down a pane discarded before its first output', async () => { + // node-pty's own kill() would wait for output that a discarded pane never sends. + const nodePty = await import('node-pty') + const proc = nodePty.spawn('cmd.exe', ['/d', '/q'], { + cols: 80, + rows: 24, + cwd: process.cwd(), + useConptyDll: true + }) + spawned.push(proc) + killConptyNow(proc) + await waitUntilDead(proc.pid) + expect(isAlive(proc.pid)).toBe(false) + }, 30_000) + it('reports this build as able to own pty trees', () => { // A node-pty rebuilt from unpatched sources would silently fall back to the // old probe, so every assertion below would pass vacuously. diff --git a/tests/e2e/terminal-foreground-confirmation.unit.test.ts b/tests/e2e/terminal-foreground-confirmation.unit.test.ts index 562bfda1546..0882cf07340 100644 --- a/tests/e2e/terminal-foreground-confirmation.unit.test.ts +++ b/tests/e2e/terminal-foreground-confirmation.unit.test.ts @@ -25,18 +25,23 @@ vi.mock('../../src/main/providers/agent-foreground-process', () => ({ })) import { createPtySubprocess } from '../../src/main/daemon/pty-subprocess' +import { createConptyPreludeSender } from '../../src/main/daemon/pty-subprocess-test-harness' import { createPaneForegroundAgentTracker } from '../../src/renderer/src/components/terminal-pane/pane-foreground-agent-tracker' import type { PaneForegroundAgentEntry } from '../../src/renderer/src/store/slices/pane-foreground-agent' function mockWindowsPty() { const exitListeners: ((event: { exitCode: number }) => void)[] = [] + const sendConptyPrelude = createConptyPreludeSender() return { pid: 12345, process: 'powershell.exe', write: vi.fn(), resize: vi.fn(), kill: vi.fn(), - onData: vi.fn(() => ({ dispose: vi.fn() })), + onData: vi.fn((callback: (data: string) => void) => { + sendConptyPrelude(callback) + return { dispose: vi.fn() } + }), onExit: vi.fn((callback: (event: { exitCode: number }) => void) => { exitListeners.push(callback) return { dispose: vi.fn() }