From a6d4a4fea5986c39364ff03df0090e25b84861ec Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 24 Oct 2026 23:59:37 -0700 Subject: [PATCH] fix(windows): respawn a ConPTY pane that started without keyboard input (#33423) * fix(windows): respawn a ConPTY pane that started without an input reader Bundled ConPTY occasionally starts a pane whose console host never reads its input pipe: the shell prints its banner and prompt, but every keystroke and the typed launch command are dropped. Such a pane is identifiable from its first output, because a healthy one always queries DA1 before relaying anything else. The daemon now waits for that first output, respawns once when it is missing, and replays the buffered startup output so no byte is lost. The table stress probe now waits for node-pty's own readiness rather than typed input. * fix(windows): bound the ConPTY startup check and never re-run a command - Respawn only when no command rode in the shell's argv; otherwise keep the pane and say on screen that it cannot take input. - Accept the pane unverified after 5s of silence instead of stalling the spawn. - Distrust the check after two misses with no pane ever announcing, so a changed ConPTY build cannot respawn every pane. - Kill a discarded pane through its job before closing the pseudoconsole. - Report a cancel during the wait as an attach cancel, not a spawn failure. - Assert on real Windows ConPTY that a fresh pane announces its input reader. * fix(windows): respawn only Orca's own shell argv and close an unready pane now - Treat user shell args like an argv startup command: never respawn, show the notice instead, so a command such as pwsh -Command never runs twice. - Close a discarded pane's pseudoconsole directly; node-pty defers kill() until first output, which a silent pane never produces. Kill the shell by pid when no job owns it. - Reconcile a master node-pty retired during the startup wait. - Drop the test-only respawn counter; the count rides in the daemon log. * fix(windows): tear a discarded unready pane down through node-pty's agent node-pty defers kill() until first output, so closing only the native pseudoconsole left the input pipe and output worker of a silent discarded pane alive until daemon exit. The agent's kill closes all three. * fix(windows): discard startup panes by handle only; account for the mock prelude A pid-addressed kill could hit a recycled pid when the shell exited during startup; the job and the agent's native kill both act on handles. Lifecycle assertions now expect the replayed DA1 query when the suite runs on Windows. * test(e2e): give the mocked Windows pane its ConPTY prelude --- config/scripts/windows-pty-table-stress.cjs | 9 +- ...ubprocess-foreground-degraded-scan.test.ts | 7 +- ...subprocess-foreground-scan-cadence.test.ts | 6 + .../pty-subprocess-handle-lifecycle.test.ts | 18 +- .../daemon/pty-subprocess-test-harness.ts | 20 ++ ...ty-subprocess-windows-shell-launch.test.ts | 56 +++++ src/main/daemon/pty-subprocess.ts | 14 +- .../conpty-startup-input-reader.ts | 193 ++++++++++++++++++ .../native-pty-spawn-windows.test.ts | 153 +++++++++++++- .../daemon/pty-subprocess/native-pty-spawn.ts | 48 ++++- .../pty-subprocess/subprocess-handle.ts | 22 +- ...-foreground-process-git-bash.win32.test.ts | 4 +- src/main/windows/windows-pty-job.ts | 16 ++ .../windows/windows-pty-job.win32.test.ts | 32 +++ ...minal-foreground-confirmation.unit.test.ts | 7 +- 15 files changed, 575 insertions(+), 30 deletions(-) create mode 100644 src/main/daemon/pty-subprocess/conpty-startup-input-reader.ts 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() }