From 06fda780eae5861e9fa9f38bb597eb70f5c1ede2 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 2 Sep 2026 14:52:13 -0700 Subject: [PATCH] fix(gh): reap the whole gh/glab process tree at the deadline on POSIX MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gh` and `glab` on PATH are routinely shims — mise, asdf, volta, or a hand-written wrapper — so a timed-out invocation has a chain to stop, not one process. `execFileCapture`'s POSIX kill path signals only the direct child; the descendants are orphaned to init and keep running. #18234 is exactly that shape: `bash ~/.local/bin/gh` -> `mise x gh` -> `gh`, where the reporter found the tail reparented to `systemd --user` and still at 100% CPU nearly two hours later. The 15s deadline #18239 added bounds Orca's semaphore slot and its promise; it does not bound the CPU burn. Route both CLIs through `execFileCaptureToTermination`, the primitive git's barrier path already uses: POSIX children spawn `detached`, the deadline signals `-pgid` and escalates to SIGKILL, and the promise waits for verified termination. Windows behaviour is unchanged (`taskkill /t` either way). Switching primitives also swapped execFile's hard maxBuffer failure for `runProcess`'s silent clipping, which would have turned an oversized gh response into a shorter valid-looking one. `ProcessResult` now reports truncation and the capture rejects on it, restoring the old contract and closing the same latent gap on git's barrier path. --- .../git/command-runner/exec-file-capture.ts | 22 +- .../gh-exec-file-deadline.test.ts | 102 ++-- src/main/git/command-runner/gh-exec-file.ts | 30 +- src/main/git/command-runner/glab-exec-file.ts | 25 +- src/main/git/runner-command-exec.test.ts | 160 ++++-- .../git/runner-gh-rate-limit-breaker.test.ts | 26 +- src/main/git/runner-wsl-gh-fallback.test.ts | 461 ++++++------------ ...tlab-known-host-probe-wsl-fallback.test.ts | 33 +- .../__fixtures__/fake-spawned-child.ts | 75 +++ .../child-process/bounded-output-sink.ts | 7 +- src/shared/child-process/process-spec.ts | 2 + src/shared/child-process/run-process.test.ts | 21 + src/shared/child-process/run-process.ts | 12 +- 13 files changed, 508 insertions(+), 468 deletions(-) create mode 100644 src/shared/child-process/__fixtures__/fake-spawned-child.ts diff --git a/src/main/git/command-runner/exec-file-capture.ts b/src/main/git/command-runner/exec-file-capture.ts index b1b9c664176..e9ae815ff34 100644 --- a/src/main/git/command-runner/exec-file-capture.ts +++ b/src/main/git/command-runner/exec-file-capture.ts @@ -25,7 +25,11 @@ export async function execFileCaptureToTermination( options: ExecFileCaptureOptions, termination?: WslProcessGroupTermination ): Promise<{ stdout: string | Buffer; stderr: string | Buffer }> { - const result = await runProcess({ + // Why measured here: runProcess spawns inside its promise executor, which runs + // synchronously, so this brackets exactly the main-thread block execFileCapture + // reports for its own spawns. + const spawnStartedAt = performance.now() + const pending = runProcess({ program: command, args, cwd: typeof options.cwd === 'string' ? options.cwd : undefined, @@ -37,10 +41,17 @@ export async function execFileCaptureToTermination( onChildTerminated: options.onChildTerminated, ...(options.stdin === undefined ? {} : { input: options.stdin }) }) + recordSubprocessSpawn(command, args, performance.now() - spawnStartedAt) + const result = await pending const stdout = options.encoding === 'buffer' ? Buffer.from(result.stdout) : result.stdout const cleanStderr = termination?.stripControlOutput(result.stderr) ?? result.stderr const stderr = options.encoding === 'buffer' ? Buffer.from(cleanStderr) : cleanStderr - if (result.code === 0 && !result.timedOut && !options.signal?.aborted) { + if ( + result.code === 0 && + !result.timedOut && + !result.outputTruncated && + !options.signal?.aborted + ) { return { stdout, stderr } } const error = result.timedOut @@ -48,7 +59,12 @@ export async function execFileCaptureToTermination( : new Error( options.signal?.aborted ? 'The operation was aborted.' - : cleanStderr.trim() || `${command} exited with ${result.code}.` + : result.outputTruncated + ? // Why fail instead of returning the clipped text: callers parse this + // as JSON or JSONL, where a clipped answer reads as a shorter valid + // one. execFile's own maxBuffer overrun errored for the same reason. + `${command} produced more than ${options.maxBuffer ?? DEFAULT_GIT_MAX_BUFFER} bytes of output.` + : cleanStderr.trim() || `${command} exited with ${result.code}.` ) if (options.signal?.aborted) { error.name = 'AbortError' diff --git a/src/main/git/command-runner/gh-exec-file-deadline.test.ts b/src/main/git/command-runner/gh-exec-file-deadline.test.ts index 3775b67a7ed..fa07dec32be 100644 --- a/src/main/git/command-runner/gh-exec-file-deadline.test.ts +++ b/src/main/git/command-runner/gh-exec-file-deadline.test.ts @@ -2,20 +2,15 @@ import { EventEmitter } from 'node:events' import type { ChildProcess } from 'node:child_process' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -const { execFileMock, spawnMock, killSpawnedCommandTreeMock } = vi.hoisted(() => ({ - execFileMock: vi.fn(), +const { spawnMock, processKillMock } = vi.hoisted(() => ({ spawnMock: vi.fn(), - killSpawnedCommandTreeMock: vi.fn().mockResolvedValue(undefined) + processKillMock: vi.fn() })) vi.mock('node:child_process', async (importOriginal) => ({ ...(await importOriginal()), - execFile: execFileMock, spawn: spawnMock })) -vi.mock('./spawned-command-tree-kill', () => ({ - killSpawnedCommandTree: killSpawnedCommandTreeMock -})) import { ghExecFileAsync } from './gh-exec-file' @@ -29,66 +24,87 @@ function mockChild(pid = 4321): ChildProcess { return child as unknown as ChildProcess } +function settleChild(child: ChildProcess, stdout: string): void { + child.stdout?.emit('data', Buffer.from(stdout)) + child.emit('exit', 0, null) + child.emit('close', 0, null) +} + /** * The contract the star check depends on after #18234: a `gh` that never exits - * is killed at the deadline, tree and all, rather than running forever. + * is killed at the deadline, and the kill reaches the whole chain. On the + * reporter's box `gh` was a shell wrapper calling `mise x gh`, so signalling + * only the direct child left the rest of the chain running under init. */ describe('gh exec deadline', () => { beforeEach(() => { vi.useFakeTimers() - execFileMock.mockReset() spawnMock.mockReset() - killSpawnedCommandTreeMock.mockClear() + processKillMock.mockReset() + vi.spyOn(process, 'kill').mockImplementation(processKillMock as unknown as typeof process.kill) }) afterEach(() => { vi.useRealTimers() + vi.restoreAllMocks() }) - it('kills the process tree and rejects when gh never exits', async () => { - const child = mockChild() - // Why never invoking the callback: this is exactly the stuck child from - // #18234 — spawned, spinning, and never reporting an exit. - execFileMock.mockReturnValue(child) + it.runIf(process.platform !== 'win32')( + 'signals the whole process group, not just the child, when gh never exits', + async () => { + const child = mockChild() + // Why never emitting exit: this is exactly the stuck child from #18234 — + // spawned, spinning, and never reporting an exit. + spawnMock.mockReturnValue(child) - const pending = ghExecFileAsync(['api', '--include', 'user/starred/stablyai/orca'], { - timeout: 15_000 - }) - const rejection = expect(pending).rejects.toThrow('timed out') - await vi.waitFor(() => expect(execFileMock).toHaveBeenCalledOnce()) + const pending = ghExecFileAsync(['api', '--include', 'user/starred/stablyai/orca'], { + timeout: 15_000 + }) + const rejection = expect(pending).rejects.toThrow('timed out') + await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledOnce()) - // Not yet: the deadline has not elapsed. - expect(killSpawnedCommandTreeMock).not.toHaveBeenCalled() + // The child must be its own group leader, or the signal below would go to + // whatever group it inherited — Orca's own. + expect(spawnMock.mock.calls[0][2].detached).toBe(true) + expect(processKillMock).not.toHaveBeenCalled() - await vi.advanceTimersByTimeAsync(15_000) - await rejection + await vi.advanceTimersByTimeAsync(15_000) + await vi.advanceTimersByTimeAsync(15_000) + await rejection - expect(killSpawnedCommandTreeMock).toHaveBeenCalledWith(child) - }) + expect(processKillMock).toHaveBeenCalledWith(-4321, undefined) + } + ) it('spawns with hidden console and captured stdio, never an inherited or shell stdio', async () => { const child = mockChild() - execFileMock.mockImplementation( - ( - _command: string, - _args: string[], - _options: unknown, - callback: (error: Error | null, stdout: string, stderr: string) => void - ) => { - callback(null, 'HTTP/2.0 204 No Content\r\n', '') - return child - } - ) + spawnMock.mockImplementation(() => { + queueMicrotask(() => settleChild(child, 'HTTP/2.0 204 No Content\r\n')) + return child + }) - await ghExecFileAsync(['api', '--include', 'user/starred/stablyai/orca'], { timeout: 15_000 }) + const result = await ghExecFileAsync(['api', '--include', 'user/starred/stablyai/orca'], { + timeout: 15_000 + }) - const [command, args, options] = execFileMock.mock.calls[0] + expect(result.stdout).toContain('204 No Content') + const [command, args, options] = spawnMock.mock.calls[0] expect(command).toBe('gh') expect(args).toEqual(['api', '--include', 'user/starred/stablyai/orca']) - // `execFile` captures stdout/stderr over pipes and never inherits Orca's; - // `shell` is never set, and the console stays hidden on Windows. expect(options.windowsHide).toBe(true) - expect(options.stdio).toBeUndefined() - expect(options.shell).toBeUndefined() + expect(options.stdio).toEqual(['pipe', 'pipe', 'pipe']) + expect(options.shell).toBe(false) + }) + + it('fails rather than returning a clipped answer when gh overruns maxBuffer', async () => { + const child = mockChild() + spawnMock.mockImplementation(() => { + queueMicrotask(() => settleChild(child, '['.padEnd(64, 'x'))) + return child + }) + + await expect( + ghExecFileAsync(['api', 'repos/stablyai/orca/issues'], { timeout: 15_000, maxBuffer: 8 }) + ).rejects.toThrow('more than 8 bytes') }) }) diff --git a/src/main/git/command-runner/gh-exec-file.ts b/src/main/git/command-runner/gh-exec-file.ts index b8f13be5d5e..e8308a8e4b4 100644 --- a/src/main/git/command-runner/gh-exec-file.ts +++ b/src/main/git/command-runner/gh-exec-file.ts @@ -19,7 +19,7 @@ import { isHostCommandMissing, resolveHostGitHubCli } from './github-cli-host-fallback' -import { execFileCapture } from './exec-file-capture' +import { execFileCaptureToTermination } from './exec-file-capture' import type { GitExecOptions } from './git-exec-options' import { argsLookIdempotent } from './gh-idempotency' import { applyGhHostToArgs, explicitGhHostname, explicitGhRepoHostname } from './gh-host-args' @@ -115,15 +115,25 @@ export async function ghExecFileAsync( let attemptedDefaultWslFallback = false for (let attempt = 0; attempt <= GH_RETRY_DELAYS_MS.length; attempt++) { try { - const { stdout, stderr } = await execFileCapture(resolved.binary, resolved.args, { - cwd: resolved.cwd, - encoding: (options.encoding ?? 'utf-8') as BufferEncoding, - maxBuffer: options.maxBuffer, - // Why: bound gh so one stuck child fails visibly instead of wedging the IPC lane. - timeout: options.timeout ?? defaultGhExecTimeoutMs(options.env), - env: nonInteractiveGhEnv(options.env), - signal: options.signal - }) + // Why to-termination and not execFileCapture: `gh` on PATH is routinely a + // shim (mise, asdf, volta, a hand-written wrapper), so the deadline below + // has a chain to reap, not one process. execFileCapture's POSIX kill only + // signals the direct child, which orphans the rest to init — a wedged + // helper then outlives the timeout that was supposed to bound it (#18234). + const { stdout, stderr } = await execFileCaptureToTermination( + resolved.binary, + resolved.args, + { + cwd: resolved.cwd, + encoding: (options.encoding ?? 'utf-8') as BufferEncoding, + maxBuffer: options.maxBuffer, + // Why: bound gh so one stuck child fails visibly instead of wedging the IPC lane. + timeout: options.timeout ?? defaultGhExecTimeoutMs(options.env), + env: nonInteractiveGhEnv(options.env), + signal: options.signal + }, + resolved.termination + ) return { stdout: stdout as string, stderr: stderr as string } } catch (err) { lastError = err diff --git a/src/main/git/command-runner/glab-exec-file.ts b/src/main/git/command-runner/glab-exec-file.ts index 3a4b4467ba9..3257dd9e818 100644 --- a/src/main/git/command-runner/glab-exec-file.ts +++ b/src/main/git/command-runner/glab-exec-file.ts @@ -2,7 +2,7 @@ import { addWslEnvKeys } from '../../wsl-env' import { extractExecError, parseRetryAfterMs } from '../exec-error' import { resolveCommand, resolveDefaultWslCli } from './wsl-command-resolution' import { isHostCommandMissing } from './github-cli-host-fallback' -import { execFileCapture } from './exec-file-capture' +import { execFileCaptureToTermination } from './exec-file-capture' import type { GitExecOptions } from './git-exec-options' import { argsLookIdempotent } from './gh-idempotency' import { @@ -63,14 +63,21 @@ export async function glabExecFileAsync( let attemptedDefaultWslFallback = false for (let attempt = 0; attempt <= GH_RETRY_DELAYS_MS.length; attempt++) { try { - const { stdout, stderr } = await execFileCapture(resolved.binary, resolved.args, { - cwd: resolved.cwd, - encoding: (options.encoding ?? 'utf-8') as BufferEncoding, - maxBuffer: options.maxBuffer, - timeout: options.timeout ?? DEFAULT_GLAB_EXEC_TIMEOUT_MS, - env: options.env, - signal: options.signal - }) + // Why to-termination: same shim chain as gh — the deadline has to reap the + // whole tree, not just the wrapper that spawned it (#18234). + const { stdout, stderr } = await execFileCaptureToTermination( + resolved.binary, + resolved.args, + { + cwd: resolved.cwd, + encoding: (options.encoding ?? 'utf-8') as BufferEncoding, + maxBuffer: options.maxBuffer, + timeout: options.timeout ?? DEFAULT_GLAB_EXEC_TIMEOUT_MS, + env: options.env, + signal: options.signal + }, + resolved.termination + ) return { stdout: stdout as string, stderr: stderr as string } } catch (err) { lastError = err diff --git a/src/main/git/runner-command-exec.test.ts b/src/main/git/runner-command-exec.test.ts index 27d7a72c747..89bcc4dca15 100644 --- a/src/main/git/runner-command-exec.test.ts +++ b/src/main/git/runner-command-exec.test.ts @@ -46,6 +46,31 @@ function createMockChildProcess(pid: number): MockChildProcess { return child } +/** + * Spawn stand-in for the gh/glab deadline tests: the CLI hangs, while the `ps` + * quiescence probe the tree termination runs answers immediately. + */ +function mockWedgedCliSpawn(child: MockChildProcess): void { + spawnMock.mockImplementation((program: string) => { + if (program !== 'ps') { + return child + } + const probe = createMockChildProcess(9100) + queueMicrotask(() => probe.emit('close', 0, null)) + return probe + }) +} + +/** Signals succeed; the existence probe reports the group already gone. */ +function mockProcessGroupSignals(): ReturnType { + return vi.spyOn(process, 'kill').mockImplementation(((_pid: number, signal?: unknown) => { + if (signal === 0) { + throw Object.assign(new Error('ESRCH'), { code: 'ESRCH' }) + } + return true + }) as typeof process.kill) +} + function createMockTaskkillProcess(): MockChildProcess { const child = createMockChildProcess(9000) child.unref = vi.fn() @@ -271,32 +296,46 @@ describe('runner execFile timeout handling', () => { } ) - it('rejects gh executions that never call back using the default timeout', async () => { + // Why the group and not the child (#18234): `gh` and `glab` on PATH are often + // shims, so the deadline has a chain to reap. Signalling only the direct child + // leaves the rest of it running under init long after the deadline passed. + it('signals the whole gh process group when gh never calls back', async () => { const child = createMockChildProcess(1234) - execFileMock.mockReturnValue(child) + mockWedgedCliSpawn(child) + const processKill = mockProcessGroupSignals() + try { + const promise = ghExecFileAsync(['api', 'repos/stablyai/orca/issues/5388'], { + cwd: '/repo' + }) + const rejection = expect(promise).rejects.toThrow('gh timed out.') + await vi.advanceTimersByTimeAsync(30_000) + expect(spawnMock.mock.calls[0][2].detached).toBe(true) + await vi.advanceTimersByTimeAsync(2_000) - const promise = ghExecFileAsync(['api', 'repos/stablyai/orca/issues/5388'], { - cwd: '/repo' - }) - const rejection = expect(promise).rejects.toThrow('gh timed out.') - await vi.advanceTimersByTimeAsync(30_000) - - await rejection - expect(child.kill).toHaveBeenCalled() + await rejection + expect(processKill).toHaveBeenCalledWith(-1234, undefined) + } finally { + processKill.mockRestore() + } }) - it('rejects glab executions that never call back using the default timeout', async () => { + it('signals the whole glab process group when glab never calls back', async () => { const child = createMockChildProcess(1234) - execFileMock.mockReturnValue(child) + mockWedgedCliSpawn(child) + const processKill = mockProcessGroupSignals() + try { + const promise = glabExecFileAsync(['api', 'projects/stablyai%2Forca/issues'], { + cwd: '/repo' + }) + const rejection = expect(promise).rejects.toThrow('glab timed out.') + await vi.advanceTimersByTimeAsync(30_000) + await vi.advanceTimersByTimeAsync(2_000) - const promise = glabExecFileAsync(['api', 'projects/stablyai%2Forca/issues'], { - cwd: '/repo' - }) - const rejection = expect(promise).rejects.toThrow('glab timed out.') - await vi.advanceTimersByTimeAsync(30_000) - - await rejection - expect(child.kill).toHaveBeenCalled() + await rejection + expect(processKill).toHaveBeenCalledWith(-1234, undefined) + } finally { + processKill.mockRestore() + } }) it('aborts glab retry backoff instead of starting another attempt', async () => { @@ -304,9 +343,14 @@ describe('runner execFile timeout handling', () => { const transient = Object.assign(new Error('glab failed'), { stderr: 'HTTP 503 Service Unavailable' }) - execFileMock.mockImplementationOnce((_command, _args, _options, callback) => { - callback(transient) - return createMockChildProcess(1234) + spawnMock.mockImplementationOnce(() => { + const child = createMockChildProcess(1234) + queueMicrotask(() => { + child.stderr.emit('data', Buffer.from(transient.stderr)) + child.emit('exit', 1, null) + child.emit('close', 1, null) + }) + return child }) const promise = glabExecFileAsync(['api', 'projects'], { @@ -314,52 +358,68 @@ describe('runner execFile timeout handling', () => { signal: controller.signal }) const rejection = expect(promise).rejects.toMatchObject({ name: 'AbortError' }) - await vi.waitFor(() => expect(execFileMock).toHaveBeenCalledTimes(1)) + await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(1)) controller.abort() await rejection - expect(execFileMock).toHaveBeenCalledTimes(1) + expect(spawnMock).toHaveBeenCalledTimes(1) }) it('kills an active gh execution when its caller aborts', async () => { const child = createMockChildProcess(1234) - execFileMock.mockReturnValue(child) - const controller = new AbortController() - const promise = ghExecFileAsync(['api', 'repos/stablyai/orca/issues/5388'], { - cwd: '/repo', - signal: controller.signal - }) - const rejection = expect(promise).rejects.toMatchObject({ name: 'AbortError' }) + mockWedgedCliSpawn(child) + const processKill = mockProcessGroupSignals() + try { + const controller = new AbortController() + const promise = ghExecFileAsync(['api', 'repos/stablyai/orca/issues/5388'], { + cwd: '/repo', + signal: controller.signal + }) + const rejection = expect(promise).rejects.toMatchObject({ name: 'AbortError' }) - controller.abort() + await vi.waitFor(() => expect(spawnMock).toHaveBeenCalled()) + controller.abort() + await vi.advanceTimersByTimeAsync(2_000) - await rejection - expect(child.kill).toHaveBeenCalled() + await rejection + expect(processKill).toHaveBeenCalledWith(-1234, undefined) + } finally { + processKill.mockRestore() + } }) it('honors explicit gh timeouts', async () => { const child = createMockChildProcess(1234) - execFileMock.mockReturnValue(child) + mockWedgedCliSpawn(child) + const processKill = mockProcessGroupSignals() + try { + const promise = ghExecFileAsync(['api', 'repos/stablyai/orca/issues/5388'], { + cwd: '/repo', + timeout: 1234 + }) + const rejection = expect(promise).rejects.toThrow('gh timed out.') + await vi.advanceTimersByTimeAsync(1233) + expect(processKill).not.toHaveBeenCalled() + await vi.advanceTimersByTimeAsync(1) + await vi.advanceTimersByTimeAsync(2_000) - const promise = ghExecFileAsync(['api', 'repos/stablyai/orca/issues/5388'], { - cwd: '/repo', - timeout: 1234 - }) - const rejection = expect(promise).rejects.toThrow('gh timed out.') - await vi.advanceTimersByTimeAsync(1233) - expect(child.kill).not.toHaveBeenCalled() - await vi.advanceTimersByTimeAsync(1) - - await rejection - expect(child.kill).toHaveBeenCalled() + await rejection + expect(processKill).toHaveBeenCalledWith(-1234, undefined) + } finally { + processKill.mockRestore() + } }) it('runs gh non-interactively while preserving explicit env', async () => { - const child = createMockChildProcess(1234) let capturedEnv: NodeJS.ProcessEnv | undefined - execFileMock.mockImplementation((_cmd, _args, opts, cb) => { + spawnMock.mockImplementation((_cmd, _args, opts) => { capturedEnv = opts.env - cb(null, 'ok', '') + const child = createMockChildProcess(1234) + queueMicrotask(() => { + child.stdout.emit('data', Buffer.from('ok')) + child.emit('exit', 0, null) + child.emit('close', 0, null) + }) return child }) diff --git a/src/main/git/runner-gh-rate-limit-breaker.test.ts b/src/main/git/runner-gh-rate-limit-breaker.test.ts index 17e14b8e7d6..e450afd7939 100644 --- a/src/main/git/runner-gh-rate-limit-breaker.test.ts +++ b/src/main/git/runner-gh-rate-limit-breaker.test.ts @@ -12,6 +12,7 @@ vi.mock('child_process', () => ({ spawn: spawnMock })) +import { fakeSpawnReturning } from '../../shared/child-process/__fixtures__/fake-spawned-child' import { ghExecFileAsync } from './runner' import { _resetGhRateLimitBreaker, @@ -23,25 +24,16 @@ const PRIMARY_RATE_LIMIT_STDERR = 'gh: API rate limit exceeded for user ID 1775218. Please wait. (HTTP 403)' function mockGhFailure(stderr: string): void { - execFileMock.mockImplementation((_binary, _args, options, callback) => { - const done = typeof options === 'function' ? options : callback - queueMicrotask(() => - done(Object.assign(new Error(`Command failed: gh\n${stderr}`), { stderr }), '', stderr) - ) - return { once: vi.fn() } - }) + spawnMock.mockImplementation(fakeSpawnReturning({ stderr, code: 1 })) } function mockGhSuccess(stdout: string): void { - execFileMock.mockImplementation((_binary, _args, options, callback) => { - const done = typeof options === 'function' ? options : callback - queueMicrotask(() => done(null, stdout, '')) - return { once: vi.fn() } - }) + spawnMock.mockImplementation(fakeSpawnReturning({ stdout })) } beforeEach(() => { execFileMock.mockReset() + spawnMock.mockReset() }) afterEach(() => { @@ -54,14 +46,14 @@ describe('ghExecFileAsync rate-limit breaker', () => { await expect( ghExecFileAsync(['api', '--cache', '120s', 'search/issues?q=repo:a/b&per_page=1']) ).rejects.toThrow('rate limit') - expect(execFileMock).toHaveBeenCalledTimes(1) + expect(spawnMock).toHaveBeenCalledTimes(1) // The 90-repo storm case: every further search-bucket call must fail fast // without a subprocess. await expect( ghExecFileAsync(['api', '--cache', '120s', 'search/issues?q=repo:c/d&per_page=1']) ).rejects.toMatchObject({ ghRateLimitBlocked: true }) - expect(execFileMock).toHaveBeenCalledTimes(1) + expect(spawnMock).toHaveBeenCalledTimes(1) }) it('keeps other buckets working while one bucket is blocked', async () => { @@ -72,7 +64,7 @@ describe('ghExecFileAsync rate-limit breaker', () => { stdout: '[]', stderr: '' }) - expect(execFileMock).toHaveBeenCalledTimes(2) + expect(spawnMock).toHaveBeenCalledTimes(2) }) it('keeps other GitHub hosts and WSL runtimes working when github.com is blocked', async () => { @@ -105,7 +97,7 @@ describe('ghExecFileAsync rate-limit breaker', () => { value: originalPlatform }) } - expect(execFileMock).toHaveBeenCalledTimes(5) + expect(spawnMock).toHaveBeenCalledTimes(5) }) it.each([ @@ -195,7 +187,7 @@ describe('ghExecFileAsync rate-limit breaker', () => { ).resolves.toMatchObject({ stdout: '{"resources":{}}' }) - expect(execFileMock).toHaveBeenCalledTimes(2) + expect(spawnMock).toHaveBeenCalledTimes(2) }) it('does not trip the breaker on secondary rate limits', async () => { diff --git a/src/main/git/runner-wsl-gh-fallback.test.ts b/src/main/git/runner-wsl-gh-fallback.test.ts index 5f933c60620..9f4566f56ae 100644 --- a/src/main/git/runner-wsl-gh-fallback.test.ts +++ b/src/main/git/runner-wsl-gh-fallback.test.ts @@ -1,16 +1,19 @@ -import { EventEmitter } from 'node:events' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + createFakeSpawnedChild, + fakeSpawnDispatch, + fakeSpawnReturning +} from '../../shared/child-process/__fixtures__/fake-spawned-child' import type * as WslModule from '../wsl' -const { execFileMock, execFileSyncMock, spawnMock, getDefaultWslDistroMock } = vi.hoisted(() => ({ - execFileMock: vi.fn(), +const { execFileSyncMock, spawnMock, getDefaultWslDistroMock } = vi.hoisted(() => ({ execFileSyncMock: vi.fn(), spawnMock: vi.fn(), getDefaultWslDistroMock: vi.fn() })) vi.mock('child_process', () => ({ - execFile: execFileMock, + execFile: vi.fn(), execFileSync: execFileSyncMock, spawn: spawnMock })) @@ -26,25 +29,18 @@ import { _resetGhRateLimitBreaker } from './gh-rate-limit-breaker' const PRIMARY_RATE_LIMIT_STDERR = 'gh: API rate limit exceeded for user ID 1775218. Please wait. (HTTP 403)' -type MockChildProcess = EventEmitter & { - pid: number - kill: ReturnType - unref: ReturnType -} +// What the distro prints when the CLI is absent inside WSL but present on the host. +const WSL_GH_MISSING = 'bash: line 1: gh: command not found\n' +const TRANSIENT_502 = 'HTTP 502 Bad Gateway' -function createMockChildProcess(pid: number): MockChildProcess { - const child = new EventEmitter() as MockChildProcess - child.pid = pid - child.kill = vi.fn() - child.unref = vi.fn() - return child +function spawnEnoent(command: string): { spawnError: Error } { + return { spawnError: Object.assign(new Error(`spawn ${command} ENOENT`), { code: 'ENOENT' }) } } describe('ghExecFileAsync WSL fallback', () => { const originalPlatform = process.platform beforeEach(() => { - execFileMock.mockReset() spawnMock.mockReset() getDefaultWslDistroMock.mockReset() getDefaultWslDistroMock.mockReturnValue(null) @@ -66,21 +62,11 @@ describe('ghExecFileAsync WSL fallback', () => { }) it('falls back to host gh for explicit-repo WSL calls when gh is missing in the distro', async () => { - execFileMock.mockImplementation((binary, _args, options, callback) => { - if (typeof options === 'function') { - callback = options - } - if (binary === 'wsl.exe') { - callback( - Object.assign(new Error('Command failed: wsl.exe'), { - stdout: '', - stderr: 'bash: line 1: gh: command not found\n' - }) - ) - return - } - callback(null, { stdout: '[]', stderr: '' }) - }) + spawnMock.mockImplementation( + fakeSpawnDispatch((program) => + program === 'wsl.exe' ? { stderr: WSL_GH_MISSING, code: 1 } : { stdout: '[]' } + ) + ) await expect( ghExecFileAsync(['issue', 'list', '--repo', 'stablyhq/noqa', '--json', 'number,title'], { @@ -88,7 +74,7 @@ describe('ghExecFileAsync WSL fallback', () => { }) ).resolves.toEqual({ stdout: '[]', stderr: '' }) - expect(execFileMock).toHaveBeenNthCalledWith( + expect(spawnMock).toHaveBeenNthCalledWith( 1, 'wsl.exe', [ @@ -102,53 +88,34 @@ describe('ghExecFileAsync WSL fallback', () => { // Why a concrete directory (#16463): `undefined` makes CreateProcessW inherit // Orca's own cwd, a deletable WSL UNC path when it was launched from a // worktree. The Linux directory still rides inside the command. - expect.objectContaining({ cwd: expect.any(String) }), - expect.any(Function) + expect.objectContaining({ cwd: expect.any(String) }) ) - expect(execFileMock).toHaveBeenNthCalledWith( + expect(spawnMock).toHaveBeenNthCalledWith( 2, 'gh', ['issue', 'list', '--repo', 'stablyhq/noqa', '--json', 'number,title'], - expect.objectContaining({ cwd: undefined }), - expect.any(Function) + expect.objectContaining({ cwd: undefined }) ) }) it('does not fall back for repo-context gh calls without explicit repo context', async () => { - execFileMock.mockImplementation((_binary, _args, _options, callback) => { - callback( - Object.assign(new Error('Command failed: wsl.exe'), { - stdout: '', - stderr: 'bash: line 1: gh: command not found\n' - }) - ) - }) + spawnMock.mockImplementation(fakeSpawnReturning({ stderr: WSL_GH_MISSING, code: 1 })) await expect( ghExecFileAsync(['issue', 'list'], { cwd: String.raw`\\wsl.localhost\Ubuntu\home\jinwoo\stably\noqa` }) - ).rejects.toThrow('Command failed: wsl.exe') + ).rejects.toThrow('gh: command not found') - expect(execFileMock).toHaveBeenCalledTimes(1) + expect(spawnMock).toHaveBeenCalledTimes(1) }) it('falls back for short-form explicit repo flags used by gh', async () => { - execFileMock.mockImplementation((binary, _args, options, callback) => { - if (typeof options === 'function') { - callback = options - } - if (binary === 'wsl.exe') { - callback( - Object.assign(new Error('Command failed: wsl.exe'), { - stdout: '', - stderr: 'bash: line 1: gh: command not found\n' - }) - ) - return - } - callback(null, { stdout: '[]', stderr: '' }) - }) + spawnMock.mockImplementation( + fakeSpawnDispatch((program) => + program === 'wsl.exe' ? { stderr: WSL_GH_MISSING, code: 1 } : { stdout: '[]' } + ) + ) await expect( ghExecFileAsync(['issue', 'list', '-R', 'stablyhq/noqa'], { @@ -156,31 +123,20 @@ describe('ghExecFileAsync WSL fallback', () => { }) ).resolves.toEqual({ stdout: '[]', stderr: '' }) - expect(execFileMock).toHaveBeenNthCalledWith( + expect(spawnMock).toHaveBeenNthCalledWith( 2, 'gh', ['issue', 'list', '-R', 'stablyhq/noqa'], - expect.objectContaining({ cwd: undefined }), - expect.any(Function) + expect.objectContaining({ cwd: undefined }) ) }) it('falls back for compact short-form repo flags used by gh', async () => { - execFileMock.mockImplementation((binary, _args, options, callback) => { - if (typeof options === 'function') { - callback = options - } - if (binary === 'wsl.exe') { - callback( - Object.assign(new Error('Command failed: wsl.exe'), { - stdout: '', - stderr: 'bash: line 1: gh: command not found\n' - }) - ) - return - } - callback(null, { stdout: '[]', stderr: '' }) - }) + spawnMock.mockImplementation( + fakeSpawnDispatch((program) => + program === 'wsl.exe' ? { stderr: WSL_GH_MISSING, code: 1 } : { stdout: '[]' } + ) + ) await expect( ghExecFileAsync(['issue', 'list', '-Rstablyhq/noqa'], { @@ -188,31 +144,20 @@ describe('ghExecFileAsync WSL fallback', () => { }) ).resolves.toEqual({ stdout: '[]', stderr: '' }) - expect(execFileMock).toHaveBeenNthCalledWith( + expect(spawnMock).toHaveBeenNthCalledWith( 2, 'gh', ['issue', 'list', '-Rstablyhq/noqa'], - expect.objectContaining({ cwd: undefined }), - expect.any(Function) + expect.objectContaining({ cwd: undefined }) ) }) it('falls back for repo view with an explicit positional repository', async () => { - execFileMock.mockImplementation((binary, _args, options, callback) => { - if (typeof options === 'function') { - callback = options - } - if (binary === 'wsl.exe') { - callback( - Object.assign(new Error('Command failed: wsl.exe'), { - stdout: '', - stderr: 'bash: line 1: gh: command not found\n' - }) - ) - return - } - callback(null, { stdout: '{"isFork":false}', stderr: '' }) - }) + spawnMock.mockImplementation( + fakeSpawnDispatch((program) => + program === 'wsl.exe' ? { stderr: WSL_GH_MISSING, code: 1 } : { stdout: '{"isFork":false}' } + ) + ) await expect( ghExecFileAsync( @@ -224,68 +169,42 @@ describe('ghExecFileAsync WSL fallback', () => { ) ).resolves.toEqual({ stdout: '{"isFork":false}', stderr: '' }) - expect(execFileMock).toHaveBeenNthCalledWith( + expect(spawnMock).toHaveBeenNthCalledWith( 2, 'gh', ['repo', 'view', 'github.acme-corp.com/stablyhq/noqa', '--json', 'isFork,parent'], - expect.objectContaining({ cwd: undefined }), - expect.any(Function) + expect.objectContaining({ cwd: undefined }) ) }) it('does not fall back for gh api calls that depend on repo-context placeholders', async () => { - execFileMock.mockImplementation((_binary, _args, _options, callback) => { - callback( - Object.assign(new Error('Command failed: wsl.exe'), { - stdout: '', - stderr: 'bash: line 1: gh: command not found\n' - }) - ) - }) + spawnMock.mockImplementation(fakeSpawnReturning({ stderr: WSL_GH_MISSING, code: 1 })) await expect( ghExecFileAsync(['api', 'repos/stablyhq/noqa/branches/{branch}'], { cwd: String.raw`\\wsl.localhost\Ubuntu\home\jinwoo\stably\noqa` }) - ).rejects.toThrow('Command failed: wsl.exe') + ).rejects.toThrow('gh: command not found') - expect(execFileMock).toHaveBeenCalledTimes(1) + expect(spawnMock).toHaveBeenCalledTimes(1) }) it('retries idempotent gh GraphQL query transient failures', async () => { - execFileMock - .mockImplementationOnce((_binary, _args, _options, callback) => { - callback( - Object.assign(new Error('HTTP 502 Bad Gateway'), { - stdout: '', - stderr: 'HTTP 502 Bad Gateway' - }) - ) - }) - .mockImplementationOnce((_binary, _args, _options, callback) => { - callback(null, { stdout: '{"data":{}}', stderr: '' }) - }) + spawnMock + .mockImplementationOnce(fakeSpawnReturning({ stderr: TRANSIENT_502, code: 1 })) + .mockImplementationOnce(fakeSpawnReturning({ stdout: '{"data":{}}' })) await expect( ghExecFileAsync(['api', 'graphql', '-f', 'query=query { viewer { login } }']) ).resolves.toEqual({ stdout: '{"data":{}}', stderr: '' }) - expect(execFileMock).toHaveBeenCalledTimes(2) + expect(spawnMock).toHaveBeenCalledTimes(2) }) it('retries a host-pinned idempotent gh GraphQL query after host injection', async () => { - execFileMock - .mockImplementationOnce((_binary, _args, _options, callback) => { - callback( - Object.assign(new Error('HTTP 502 Bad Gateway'), { - stdout: '', - stderr: 'HTTP 502 Bad Gateway' - }) - ) - }) - .mockImplementationOnce((_binary, _args, _options, callback) => { - callback(null, { stdout: '{"data":{}}', stderr: '' }) - }) + spawnMock + .mockImplementationOnce(fakeSpawnReturning({ stderr: TRANSIENT_502, code: 1 })) + .mockImplementationOnce(fakeSpawnReturning({ stdout: '{"data":{}}' })) await expect( ghExecFileAsync(['api', 'graphql', '-f', 'query=query { viewer { login } }'], { @@ -293,8 +212,8 @@ describe('ghExecFileAsync WSL fallback', () => { }) ).resolves.toEqual({ stdout: '{"data":{}}', stderr: '' }) - expect(execFileMock).toHaveBeenCalledTimes(2) - expect(execFileMock).toHaveBeenNthCalledWith( + expect(spawnMock).toHaveBeenCalledTimes(2) + expect(spawnMock).toHaveBeenNthCalledWith( 1, 'gh', [ @@ -305,37 +224,22 @@ describe('ghExecFileAsync WSL fallback', () => { '-f', 'query=query { viewer { login } }' ], - expect.any(Object), - expect.any(Function) + expect.any(Object) ) }) it('does not retry non-idempotent gh API transient failures', async () => { - execFileMock.mockImplementation((_binary, _args, _options, callback) => { - callback( - Object.assign(new Error('HTTP 502 Bad Gateway'), { - stdout: '', - stderr: 'HTTP 502 Bad Gateway' - }) - ) - }) + spawnMock.mockImplementation(fakeSpawnReturning({ stderr: TRANSIENT_502, code: 1 })) await expect( ghExecFileAsync(['api', '-X', 'POST', 'repos/stablyai/orca/issues']) ).rejects.toThrow('HTTP 502 Bad Gateway') - expect(execFileMock).toHaveBeenCalledTimes(1) + expect(spawnMock).toHaveBeenCalledTimes(1) }) it('does not retry gh GraphQL mutation transient failures', async () => { - execFileMock.mockImplementation((_binary, _args, _options, callback) => { - callback( - Object.assign(new Error('HTTP 502 Bad Gateway'), { - stdout: '', - stderr: 'HTTP 502 Bad Gateway' - }) - ) - }) + spawnMock.mockImplementation(fakeSpawnReturning({ stderr: TRANSIENT_502, code: 1 })) await expect( ghExecFileAsync([ @@ -346,42 +250,31 @@ describe('ghExecFileAsync WSL fallback', () => { ]) ).rejects.toThrow('HTTP 502 Bad Gateway') - expect(execFileMock).toHaveBeenCalledTimes(1) + expect(spawnMock).toHaveBeenCalledTimes(1) }) it('does not retry high-level gh edit transient failures', async () => { - execFileMock.mockImplementation((_binary, _args, _options, callback) => { - callback( - Object.assign(new Error('HTTP 502 Bad Gateway'), { - stdout: '', - stderr: 'HTTP 502 Bad Gateway' - }) - ) - }) + spawnMock.mockImplementation(fakeSpawnReturning({ stderr: TRANSIENT_502, code: 1 })) await expect( ghExecFileAsync(['issue', 'edit', '5', '--repo', 'stablyai/orca']) ).rejects.toThrow('HTTP 502 Bad Gateway') - expect(execFileMock).toHaveBeenCalledTimes(1) + expect(spawnMock).toHaveBeenCalledTimes(1) }) it('retries cwd-less gh calls through the default WSL distro when host gh is missing', async () => { getDefaultWslDistroMock.mockReturnValue('Ubuntu') - execFileMock - .mockImplementationOnce((_binary, _args, _options, callback) => { - callback(Object.assign(new Error('spawn gh ENOENT'), { code: 'ENOENT' })) - }) - .mockImplementationOnce((_binary, _args, _options, callback) => { - callback(null, { stdout: '{"resources":{}}', stderr: '' }) - }) + spawnMock + .mockImplementationOnce(fakeSpawnReturning(spawnEnoent('gh'))) + .mockImplementationOnce(fakeSpawnReturning({ stdout: '{"resources":{}}' })) await expect(ghExecFileAsync(['api', 'rate_limit'])).resolves.toEqual({ stdout: '{"resources":{}}', stderr: '' }) - expect(execFileMock).toHaveBeenNthCalledWith( + expect(spawnMock).toHaveBeenNthCalledWith( 2, 'wsl.exe', ['-d', 'Ubuntu', '--exec', 'bash', '-c', "'gh' 'api' 'rate_limit'"], @@ -389,53 +282,35 @@ describe('ghExecFileAsync WSL fallback', () => { // Orca's own cwd, a deletable WSL UNC path when it was launched from a // worktree. This global call has no repo directory at all, so nothing about // where it runs changes. - expect.objectContaining({ cwd: expect.any(String) }), - expect.any(Function) + expect.objectContaining({ cwd: expect.any(String) }) ) }) it('checks a blocked WSL scope before repeating a native-to-WSL fallback', async () => { getDefaultWslDistroMock.mockReturnValue('Ubuntu') - execFileMock.mockImplementation((binary, _args, _options, callback) => { - if (binary === 'gh') { - callback(Object.assign(new Error('spawn gh ENOENT'), { code: 'ENOENT', stderr: '' })) - return - } - callback( - Object.assign(new Error(PRIMARY_RATE_LIMIT_STDERR), { - stdout: '', - stderr: PRIMARY_RATE_LIMIT_STDERR - }) + spawnMock.mockImplementation( + fakeSpawnDispatch((program) => + program === 'gh' ? spawnEnoent('gh') : { stderr: PRIMARY_RATE_LIMIT_STDERR, code: 1 } ) - }) + ) await expect(ghExecFileAsync(['api', 'repos/acme/widgets/pulls'])).rejects.toThrow('rate limit') await expect(ghExecFileAsync(['api', 'repos/acme/widgets/pulls'])).rejects.toMatchObject({ ghRateLimitBlocked: true }) - expect(execFileMock).toHaveBeenCalledTimes(3) - expect(execFileMock.mock.calls.map(([binary]) => binary)).toEqual(['gh', 'wsl.exe', 'gh']) + expect(spawnMock).toHaveBeenCalledTimes(3) + expect(spawnMock.mock.calls.map(([binary]) => binary)).toEqual(['gh', 'wsl.exe', 'gh']) }) it('checks a blocked native scope before repeating a WSL-to-native fallback', async () => { - execFileMock.mockImplementation((binary, _args, _options, callback) => { - if (binary === 'wsl.exe') { - callback( - Object.assign(new Error('Command failed: wsl.exe'), { - stdout: '', - stderr: 'bash: line 1: gh: command not found\n' - }) - ) - return - } - callback( - Object.assign(new Error(PRIMARY_RATE_LIMIT_STDERR), { - stdout: '', - stderr: PRIMARY_RATE_LIMIT_STDERR - }) + spawnMock.mockImplementation( + fakeSpawnDispatch((program) => + program === 'wsl.exe' + ? { stderr: WSL_GH_MISSING, code: 1 } + : { stderr: PRIMARY_RATE_LIMIT_STDERR, code: 1 } ) - }) + ) const options = { cwd: String.raw`\\wsl.localhost\Ubuntu\home\jinwoo\stably\noqa` @@ -447,19 +322,12 @@ describe('ghExecFileAsync WSL fallback', () => { ghExecFileAsync(['api', 'repos/acme/widgets/pulls'], options) ).rejects.toMatchObject({ ghRateLimitBlocked: true }) - expect(execFileMock).toHaveBeenCalledTimes(3) - expect(execFileMock.mock.calls.map(([binary]) => binary)).toEqual(['wsl.exe', 'gh', 'wsl.exe']) + expect(spawnMock).toHaveBeenCalledTimes(3) + expect(spawnMock.mock.calls.map(([binary]) => binary)).toEqual(['wsl.exe', 'gh', 'wsl.exe']) }) it('does not retry non-idempotent glab transient failures', async () => { - execFileMock.mockImplementation((_binary, _args, _options, callback) => { - callback( - Object.assign(new Error('HTTP 502 Bad Gateway'), { - stdout: '', - stderr: 'HTTP 502 Bad Gateway' - }) - ) - }) + spawnMock.mockImplementation(fakeSpawnReturning({ stderr: TRANSIENT_502, code: 1 })) await expect( glabExecFileAsync(['api', '-X', 'POST', 'projects/stablyai%2Forca/issues/5/notes'], { @@ -467,18 +335,11 @@ describe('ghExecFileAsync WSL fallback', () => { }) ).rejects.toThrow('HTTP 502 Bad Gateway') - expect(execFileMock).toHaveBeenCalledTimes(1) + expect(spawnMock).toHaveBeenCalledTimes(1) }) it('does not retry high-level glab update transient failures', async () => { - execFileMock.mockImplementation((_binary, _args, _options, callback) => { - callback( - Object.assign(new Error('HTTP 502 Bad Gateway'), { - stdout: '', - stderr: 'HTTP 502 Bad Gateway' - }) - ) - }) + spawnMock.mockImplementation(fakeSpawnReturning({ stderr: TRANSIENT_502, code: 1 })) await expect( glabExecFileAsync(['issue', 'update', '5', '-R', 'stablyai/orca'], { @@ -486,25 +347,21 @@ describe('ghExecFileAsync WSL fallback', () => { }) ).rejects.toThrow('HTTP 502 Bad Gateway') - expect(execFileMock).toHaveBeenCalledTimes(1) + expect(spawnMock).toHaveBeenCalledTimes(1) }) it('retries cwd-less glab calls through the default WSL distro when host glab is missing', async () => { getDefaultWslDistroMock.mockReturnValue('Ubuntu') - execFileMock - .mockImplementationOnce((_binary, _args, _options, callback) => { - callback(Object.assign(new Error('spawn glab ENOENT'), { code: 'ENOENT' })) - }) - .mockImplementationOnce((_binary, _args, _options, callback) => { - callback(null, { stdout: '[]', stderr: '' }) - }) + spawnMock + .mockImplementationOnce(fakeSpawnReturning(spawnEnoent('glab'))) + .mockImplementationOnce(fakeSpawnReturning({ stdout: '[]' })) await expect(glabExecFileAsync(['api', 'projects'])).resolves.toEqual({ stdout: '[]', stderr: '' }) - expect(execFileMock).toHaveBeenNthCalledWith( + expect(spawnMock).toHaveBeenNthCalledWith( 2, 'wsl.exe', ['-d', 'Ubuntu', '--exec', 'bash', '-c', "'glab' 'api' 'projects'"], @@ -512,24 +369,21 @@ describe('ghExecFileAsync WSL fallback', () => { // Orca's own cwd, a deletable WSL UNC path when it was launched from a // worktree. This global call has no repo directory at all, so nothing about // where it runs changes. - expect.objectContaining({ cwd: expect.any(String) }), - expect.any(Function) + expect.objectContaining({ cwd: expect.any(String) }) ) }) it('times out the default-WSL glab fallback and waits for full tree cleanup', async () => { vi.useFakeTimers() getDefaultWslDistroMock.mockReturnValue('Ubuntu') - const nativeChild = createMockChildProcess(1200) - const wslChild = createMockChildProcess(2400) - const taskkill = createMockChildProcess(3600) - execFileMock - .mockImplementationOnce((_binary, _args, _options, callback) => { - callback(Object.assign(new Error('spawn glab ENOENT'), { code: 'ENOENT' })) - return nativeChild - }) - .mockReturnValueOnce(wslChild) - spawnMock.mockReturnValue(taskkill) + const wslChild = createFakeSpawnedChild(2400) + const taskkill = createFakeSpawnedChild(3600) + spawnMock + .mockImplementationOnce(fakeSpawnReturning(spawnEnoent('glab'))) + // Why a child that never exits: this is the wedged WSL helper the deadline + // has to reap, so nothing must settle the promise before taskkill reports. + .mockImplementationOnce(() => wslChild) + .mockImplementation(() => taskkill) const promise = glabExecFileAsync(['auth', 'status'], { timeout: 1000 }) const rejection = expect(promise).rejects.toThrow('wsl.exe timed out.') @@ -539,7 +393,7 @@ describe('ghExecFileAsync WSL fallback', () => { }) await vi.advanceTimersByTimeAsync(999) - expect(spawnMock).not.toHaveBeenCalled() + expect(spawnMock).toHaveBeenCalledTimes(2) await vi.advanceTimersByTimeAsync(1) expect(spawnMock).toHaveBeenCalledWith( 'taskkill', @@ -556,29 +410,24 @@ describe('ghExecFileAsync WSL fallback', () => { it('aborts the default-WSL glab fallback with full process-tree cleanup', async () => { getDefaultWslDistroMock.mockReturnValue('Ubuntu') - const nativeChild = createMockChildProcess(1200) - const wslChild = createMockChildProcess(2400) - const taskkill = createMockChildProcess(3600) - execFileMock - .mockImplementationOnce((_binary, _args, _options, callback) => { - callback(Object.assign(new Error('spawn glab ENOENT'), { code: 'ENOENT' })) - return nativeChild - }) - .mockReturnValueOnce(wslChild) - spawnMock.mockReturnValue(taskkill) + const wslChild = createFakeSpawnedChild(2400) + const taskkill = createFakeSpawnedChild(3600) + spawnMock + .mockImplementationOnce(fakeSpawnReturning(spawnEnoent('glab'))) + .mockImplementationOnce(() => wslChild) + .mockImplementation(() => taskkill) const controller = new AbortController() const promise = glabExecFileAsync(['auth', 'status'], { signal: controller.signal }) const rejection = expect(promise).rejects.toMatchObject({ name: 'AbortError' }) - await vi.waitFor(() => expect(execFileMock).toHaveBeenCalledTimes(2)) + await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(2)) controller.abort() - expect(execFileMock).toHaveBeenNthCalledWith( + expect(spawnMock).toHaveBeenNthCalledWith( 2, 'wsl.exe', ['-d', 'Ubuntu', '--exec', 'bash', '-c', "'glab' 'auth' 'status'"], - expect.not.objectContaining({ signal: controller.signal }), - expect.any(Function) + expect.not.objectContaining({ signal: controller.signal }) ) expect(spawnMock).toHaveBeenCalledWith( 'taskkill', @@ -593,40 +442,26 @@ describe('ghExecFileAsync WSL fallback', () => { it('does not wake the default WSL distro for host-only GitLab diagnostics', async () => { getDefaultWslDistroMock.mockReturnValue('Ubuntu') - execFileMock - .mockImplementationOnce((_binary, _args, _options, callback) => { - callback(Object.assign(new Error('spawn glab ENOENT'), { code: 'ENOENT' })) - }) - .mockImplementationOnce((_binary, _args, _options, callback) => { - callback(null, { stdout: 'Logged in to gitlab.com', stderr: '' }) - }) + spawnMock + .mockImplementationOnce(fakeSpawnReturning(spawnEnoent('glab'))) + .mockImplementationOnce(fakeSpawnReturning({ stdout: 'Logged in to gitlab.com' })) await expect( glabExecFileAsync(['auth', 'status'], { allowDefaultWslFallback: false }) ).rejects.toThrow('spawn glab ENOENT') - expect(execFileMock).toHaveBeenCalledTimes(1) - expect(execFileMock).toHaveBeenCalledWith( + expect(spawnMock).toHaveBeenCalledTimes(1) + expect(spawnMock).toHaveBeenCalledWith( 'glab', ['auth', 'status'], - expect.objectContaining({ cwd: undefined }), - expect.any(Function) + expect.objectContaining({ cwd: undefined }) ) }) it('still retries idempotent glab transient failures', async () => { - execFileMock - .mockImplementationOnce((_binary, _args, _options, callback) => { - callback( - Object.assign(new Error('HTTP 502 Bad Gateway'), { - stdout: '', - stderr: 'HTTP 502 Bad Gateway' - }) - ) - }) - .mockImplementationOnce((_binary, _args, _options, callback) => { - callback(null, { stdout: '[]', stderr: '' }) - }) + spawnMock + .mockImplementationOnce(fakeSpawnReturning({ stderr: TRANSIENT_502, code: 1 })) + .mockImplementationOnce(fakeSpawnReturning({ stdout: '[]' })) await expect( glabExecFileAsync(['api', 'projects/stablyai%2Forca/issues'], { @@ -634,7 +469,7 @@ describe('ghExecFileAsync WSL fallback', () => { }) ).resolves.toEqual({ stdout: '[]', stderr: '' }) - expect(execFileMock).toHaveBeenCalledTimes(2) + expect(spawnMock).toHaveBeenCalledTimes(2) }) it('resolves fallback to the overridden distro if configured, and falls back to default WSL distro otherwise', async () => { @@ -642,60 +477,54 @@ describe('ghExecFileAsync WSL fallback', () => { setDefaultWslDistroOverride('Debian') getDefaultWslDistroMock.mockReturnValue('Ubuntu') - execFileMock - .mockImplementationOnce((_binary, _args, _options, callback) => { - callback(Object.assign(new Error('spawn gh ENOENT'), { code: 'ENOENT' })) - }) - .mockImplementationOnce((binary, args, _options, callback) => { - if (binary === 'wsl.exe' && args.includes('Debian')) { - callback(null, { stdout: 'Logged in to github.com as override', stderr: '' }) - return - } - callback(new Error('Wrong distro fallback')) - }) + spawnMock + .mockImplementationOnce(fakeSpawnReturning(spawnEnoent('gh'))) + .mockImplementationOnce( + fakeSpawnDispatch((program, args) => + program === 'wsl.exe' && args.includes('Debian') + ? { stdout: 'Logged in to github.com as override' } + : { stderr: 'Wrong distro fallback', code: 1 } + ) + ) await expect(ghExecFileAsync(['auth', 'status'])).resolves.toEqual({ stdout: 'Logged in to github.com as override', stderr: '' }) - expect(execFileMock).toHaveBeenCalledTimes(2) - expect(execFileMock).toHaveBeenNthCalledWith( + expect(spawnMock).toHaveBeenCalledTimes(2) + expect(spawnMock).toHaveBeenNthCalledWith( 2, 'wsl.exe', ['-d', 'Debian', '--exec', 'bash', '-c', "'gh' 'auth' 'status'"], - expect.any(Object), - expect.any(Function) + expect.any(Object) ) // 2) Test without override (should use default 'Ubuntu') - execFileMock.mockClear() + spawnMock.mockClear() setDefaultWslDistroOverride(null) - execFileMock - .mockImplementationOnce((_binary, _args, _options, callback) => { - callback(Object.assign(new Error('spawn gh ENOENT'), { code: 'ENOENT' })) - }) - .mockImplementationOnce((binary, args, _options, callback) => { - if (binary === 'wsl.exe' && args.includes('Ubuntu')) { - callback(null, { stdout: 'Logged in to github.com as default', stderr: '' }) - return - } - callback(new Error('Wrong distro fallback')) - }) + spawnMock + .mockImplementationOnce(fakeSpawnReturning(spawnEnoent('gh'))) + .mockImplementationOnce( + fakeSpawnDispatch((program, args) => + program === 'wsl.exe' && args.includes('Ubuntu') + ? { stdout: 'Logged in to github.com as default' } + : { stderr: 'Wrong distro fallback', code: 1 } + ) + ) await expect(ghExecFileAsync(['auth', 'status'])).resolves.toEqual({ stdout: 'Logged in to github.com as default', stderr: '' }) - expect(execFileMock).toHaveBeenCalledTimes(2) - expect(execFileMock).toHaveBeenNthCalledWith( + expect(spawnMock).toHaveBeenCalledTimes(2) + expect(spawnMock).toHaveBeenNthCalledWith( 2, 'wsl.exe', ['-d', 'Ubuntu', '--exec', 'bash', '-c', "'gh' 'auth' 'status'"], - expect.any(Object), - expect.any(Function) + expect.any(Object) ) }) }) diff --git a/src/main/gitlab/gitlab-known-host-probe-wsl-fallback.test.ts b/src/main/gitlab/gitlab-known-host-probe-wsl-fallback.test.ts index 40da88f0c91..f9f3c573629 100644 --- a/src/main/gitlab/gitlab-known-host-probe-wsl-fallback.test.ts +++ b/src/main/gitlab/gitlab-known-host-probe-wsl-fallback.test.ts @@ -1,15 +1,15 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { fakeSpawnDispatch } from '../../shared/child-process/__fixtures__/fake-spawned-child' import type * as WslModule from '../wsl' -const { execFileMock, execFileSyncMock, spawnMock, getDefaultWslDistroMock } = vi.hoisted(() => ({ - execFileMock: vi.fn(), +const { execFileSyncMock, spawnMock, getDefaultWslDistroMock } = vi.hoisted(() => ({ execFileSyncMock: vi.fn(), spawnMock: vi.fn(), getDefaultWslDistroMock: vi.fn() })) vi.mock('child_process', () => ({ - execFile: execFileMock, + execFile: vi.fn(), execFileSync: execFileSyncMock, spawn: spawnMock })) @@ -26,17 +26,16 @@ describe('glab known-hosts probe on Windows', () => { const originalPlatform = process.platform const hostGlabMissingWslLoggedIn = (): void => { - execFileMock.mockImplementation((binary, _args, _options, callback) => { - if (binary === 'wsl.exe') { - callback(null, { stdout: 'Logged in to gitlab.wsl.test as user', stderr: '' }) - return - } - callback(Object.assign(new Error('spawn glab ENOENT'), { code: 'ENOENT' })) - }) + spawnMock.mockImplementation( + fakeSpawnDispatch((program) => + program === 'wsl.exe' + ? { stdout: 'Logged in to gitlab.wsl.test as user' } + : { spawnError: Object.assign(new Error('spawn glab ENOENT'), { code: 'ENOENT' }) } + ) + ) } beforeEach(() => { - execFileMock.mockReset() spawnMock.mockReset() getDefaultWslDistroMock.mockReset() getDefaultWslDistroMock.mockReturnValue('Ubuntu') @@ -56,12 +55,11 @@ describe('glab known-hosts probe on Windows', () => { await expect(getGlabKnownHosts()).resolves.toEqual(['gitlab.com']) - expect(execFileMock).toHaveBeenCalledTimes(1) - expect(execFileMock).toHaveBeenCalledWith( + expect(spawnMock).toHaveBeenCalledTimes(1) + expect(spawnMock).toHaveBeenCalledWith( 'glab', ['auth', 'status'], - expect.objectContaining({ cwd: undefined }), - expect.any(Function) + expect.objectContaining({ cwd: undefined }) ) }) @@ -73,15 +71,14 @@ describe('glab known-hosts probe on Windows', () => { await expect(getGlabKnownHosts('conn-1')).resolves.toEqual(['gitlab.com', 'gitlab.wsl.test']) - expect(execFileMock).toHaveBeenCalledWith( + expect(spawnMock).toHaveBeenCalledWith( 'wsl.exe', ['-d', 'Ubuntu', '--exec', 'bash', '-c', "'glab' 'auth' 'status'"], // Why a concrete directory (#16463): `undefined` makes CreateProcessW inherit // Orca's own cwd, a deletable WSL UNC path when it was launched from a // worktree. This probe has no repo directory at all, so nothing about where // it runs changes. The native `glab` assertion above keeps `undefined`. - expect.objectContaining({ cwd: expect.any(String) }), - expect.any(Function) + expect.objectContaining({ cwd: expect.any(String) }) ) }) }) diff --git a/src/shared/child-process/__fixtures__/fake-spawned-child.ts b/src/shared/child-process/__fixtures__/fake-spawned-child.ts new file mode 100644 index 00000000000..7183bc12e07 --- /dev/null +++ b/src/shared/child-process/__fixtures__/fake-spawned-child.ts @@ -0,0 +1,75 @@ +import { EventEmitter } from 'node:events' +import type { ChildProcess } from 'node:child_process' +import { vi } from 'vitest' + +/** + * A `child_process.spawn` stand-in for suites that drive gh/glab/git runners. + * + * Those runners capture output through `runProcess`, which reads the streams + * and waits for `close`, so a bare EventEmitter is not enough — a test child + * has to carry stdio and report an exit or the promise never settles. + */ +export function createFakeSpawnedChild(pid = 4321): ChildProcess { + const child = new EventEmitter() as EventEmitter & Record + child.pid = pid + child.kill = vi.fn(() => true) + child.stdin = Object.assign(new EventEmitter(), { end: vi.fn() }) + child.stdout = new EventEmitter() + child.stderr = new EventEmitter() + return child as unknown as ChildProcess +} + +/** Emit output and a clean exit, the way a CLI that answered would. */ +export function completeFakeSpawn( + child: ChildProcess, + result: { stdout?: string; stderr?: string; code?: number } = {} +): void { + if (result.stdout) { + child.stdout?.emit('data', Buffer.from(result.stdout)) + } + if (result.stderr) { + child.stderr?.emit('data', Buffer.from(result.stderr)) + } + const code = result.code ?? 0 + child.emit('exit', code, null) + child.emit('close', code, null) +} + +/** What a faked spawn should do: answer, or fail to start at all. */ +export type FakeSpawnOutcome = + | { stdout?: string; stderr?: string; code?: number } + | { spawnError: Error } + +function settleFakeSpawn(child: ChildProcess, outcome: FakeSpawnOutcome): void { + if ('spawnError' in outcome) { + // Why an event and not a throw: an unresolvable program fails asynchronously + // in libuv, which is what makes ENOENT reach callers as a rejection. + child.emit('error', outcome.spawnError) + return + } + completeFakeSpawn(child, outcome) +} + +/** + * Build a `spawn` implementation that answers every call the same way. + * + * Why a fresh child per call: the runners retry and fall back, and a shared + * emitter would replay the first call's exit into the second's listeners. + */ +export function fakeSpawnReturning( + outcome: FakeSpawnOutcome = {} +): (program: string, args: readonly string[]) => ChildProcess { + return fakeSpawnDispatch(() => outcome) +} + +/** Build a `spawn` implementation that answers per invoked program and argv. */ +export function fakeSpawnDispatch( + resolve: (program: string, args: readonly string[]) => FakeSpawnOutcome +): (program: string, args: readonly string[]) => ChildProcess { + return (program, args) => { + const child = createFakeSpawnedChild() + const outcome = resolve(program, args) + queueMicrotask(() => settleFakeSpawn(child, outcome)) + return child + } +} diff --git a/src/shared/child-process/bounded-output-sink.ts b/src/shared/child-process/bounded-output-sink.ts index c231234ef1a..195e466fdf6 100644 --- a/src/shared/child-process/bounded-output-sink.ts +++ b/src/shared/child-process/bounded-output-sink.ts @@ -10,6 +10,7 @@ import { Buffer } from 'node:buffer' export function createOutputSink(maxBytes: number): { write: (chunk: Buffer | string) => void text: () => string + truncated: () => boolean } { const chunks: Buffer[] = [] let bytes = 0 @@ -18,11 +19,15 @@ export function createOutputSink(maxBytes: number): { const chunk = Buffer.isBuffer(raw) ? raw : Buffer.from(raw) const remaining = maxBytes - bytes if (remaining <= 0) { + bytes += chunk.length return } chunks.push(chunk.length > remaining ? chunk.subarray(0, remaining) : chunk) bytes += chunk.length }, - text: () => Buffer.concat(chunks).toString('utf8') + text: () => Buffer.concat(chunks).toString('utf8'), + // Why: callers that parse the output need to tell a short answer from a + // clipped one -- truncated JSON or JSONL parses as a smaller valid result. + truncated: () => bytes > maxBytes } } diff --git a/src/shared/child-process/process-spec.ts b/src/shared/child-process/process-spec.ts index ac705974efe..2acfe82d61d 100644 --- a/src/shared/child-process/process-spec.ts +++ b/src/shared/child-process/process-spec.ts @@ -65,6 +65,8 @@ export type ProcessResult = { stderr: string /** True when the process was killed by `timeoutMs` rather than exiting. */ timedOut: boolean + /** True when stdout or stderr exceeded `maxOutputBytes` and was clipped. */ + outputTruncated?: boolean } export const DEFAULT_PROCESS_TIMEOUT_MS = 30_000 diff --git a/src/shared/child-process/run-process.test.ts b/src/shared/child-process/run-process.test.ts index 5fad9b5dc70..d36de3c688a 100644 --- a/src/shared/child-process/run-process.test.ts +++ b/src/shared/child-process/run-process.test.ts @@ -99,6 +99,27 @@ describe('runProcessSync', () => { }) }) +describe('bounded output', () => { + it('reports a clipped answer instead of passing it off as the whole one', async () => { + const result = await runProcess({ + program: process.execPath, + args: ['-e', 'process.stdout.write("x".repeat(64))'], + maxOutputBytes: 8 + }) + expect(result.stdout).toBe('xxxxxxxx') + expect(result.outputTruncated).toBe(true) + }) + + it('does not call output that exactly fills the cap truncated', async () => { + const result = await runProcess({ + program: process.execPath, + args: ['-e', 'process.stdout.write("x".repeat(8))'], + maxOutputBytes: 8 + }) + expect(result.outputTruncated).toBe(false) + }) +}) + describe('unkillable children', () => { it('settles after the grace period rather than outliving its own deadline', async () => { // `close` only fires once the child is gone, so a child that ignores the diff --git a/src/shared/child-process/run-process.ts b/src/shared/child-process/run-process.ts index ec83c5beed0..67bd48f5b81 100644 --- a/src/shared/child-process/run-process.ts +++ b/src/shared/child-process/run-process.ts @@ -186,7 +186,14 @@ export function runProcess(spec: ProcessSpec): Promise { const resolveFromClose = (code: number | null, signal: NodeJS.Signals | null): void => settle(() => - resolve({ code, signal, stdout: stdout.text(), stderr: stderr.text(), timedOut }) + resolve({ + code, + signal, + stdout: stdout.text(), + stderr: stderr.text(), + timedOut, + outputTruncated: stdout.truncated() || stderr.truncated() + }) ) const settleBarrierOutcome = (): void => { @@ -381,6 +388,9 @@ export function runProcessSync(spec: ProcessSpec): ProcessResult { signal: result.signal, stdout: result.stdout?.toString('utf8') ?? '', stderr: result.stderr?.toString('utf8') ?? '', + // Why always false: spawnSync reports an overrun as an ENOBUFS error, and + // the guard above rethrows it, so no truncated result reaches this point. + outputTruncated: false, // Why ETIMEDOUT and not the signal: a timeout kills with SIGTERM, but so // does anything else that terminates the child, and only a timeout also // sets this error. Reading the signal alone reports a deliberately