From c4177110ab012cbfcf8df3e6e685e9fe563b81c9 Mon Sep 17 00:00:00 2001 From: Rahul Joshi <186129212+crypticsaiyan@users.noreply.github.com> Date: Wed, 21 Oct 2026 22:13:26 +0530 Subject: [PATCH] fix(cli): keep a failing exit code when stdout's reader closes early --- src/lib/interrupt.test.ts | 82 ++++++++++++++++++++++++++++++++++++--- src/lib/interrupt.ts | 33 ++++++++++++---- 2 files changed, 103 insertions(+), 12 deletions(-) diff --git a/src/lib/interrupt.test.ts b/src/lib/interrupt.test.ts index 74dbd34e..046692a3 100644 --- a/src/lib/interrupt.test.ts +++ b/src/lib/interrupt.test.ts @@ -3,6 +3,7 @@ import { writeSync } from 'node:fs'; import { describe, expect, it, vi } from 'vitest'; import { InterruptError } from './errors.js'; import { + INTERRUPT_EXIT_BACKSTOP_MS, SIGINT_EXIT_CODE, ShutdownController, TERMINATION_EXIT_CODES, @@ -321,16 +322,60 @@ describe('installBrokenPipeGuard', () => { return Object.assign(new Error('write EPIPE'), { code: 'EPIPE' }); } - it('exits 0 on stdout EPIPE (clean SIGPIPE-equivalent for `| head`)', () => { + function installDisarmed(exitCode: () => number | string | undefined) { const stdout = new EventEmitter(); const stderr = new EventEmitter(); const exit = vi.fn(); const shutdown = new ShutdownController(); - installBrokenPipeGuard({ stdout, stderr, exit, shutdown }); + let fire: (() => void) | undefined; + const unref = vi.fn(); + const setTimer = vi.fn((callback: () => void, _delayMs: number) => { + fire = callback; + return { unref }; + }); + installBrokenPipeGuard({ stdout, stderr, exit, shutdown, setTimer, exitCode }); + return { stdout, exit, shutdown, setTimer, unref, fire: () => fire?.() }; + } - stdout.emit('error', makeEpipe()); - expect(exit).toHaveBeenCalledWith(0); - expect(shutdown.signal.aborted).toBe(false); + it('exits 0 on stdout EPIPE (clean SIGPIPE-equivalent for `| head`)', () => { + const guard = installDisarmed(() => undefined); + + guard.stdout.emit('error', makeEpipe()); + guard.fire(); + expect(guard.exit).toHaveBeenCalledWith(0); + expect(guard.shutdown.signal.aborted).toBe(false); + }); + + it('disarmed: a failing verdict set after EPIPE keeps its exit code', () => { + // index.ts sets the verdict's code only after an async telemetry flush, + // so the EPIPE lands first. The guard must not exit 0 in between. + const verdict: { code?: number } = {}; + const guard = installDisarmed(() => verdict.code); + + guard.stdout.emit('error', makeEpipe()); + expect(guard.exit).not.toHaveBeenCalled(); + verdict.code = 1; + guard.fire(); + expect(guard.exit).toHaveBeenCalledExactlyOnceWith(1); + }); + + it('disarmed: arms one unref-ed backstop so the event loop can drain first', () => { + const guard = installDisarmed(() => undefined); + + guard.stdout.emit('error', makeEpipe()); + guard.stdout.emit('error', makeEpipe()); + expect(guard.setTimer).toHaveBeenCalledOnce(); + expect(guard.setTimer).toHaveBeenCalledWith(expect.any(Function), INTERRUPT_EXIT_BACKSTOP_MS); + expect(guard.unref).toHaveBeenCalledOnce(); + expect(guard.exit).not.toHaveBeenCalled(); + }); + + it('disarmed: a non-numeric exit code falls back to 0', () => { + const guard = installDisarmed(() => 'oops'); + + guard.stdout.emit('error', makeEpipe()); + guard.fire(); + expect(guard.exit).toHaveBeenCalledWith(0); }); it('armed scope: stdout EPIPE requests graceful cleanup instead of exiting immediately', async () => { @@ -367,6 +412,33 @@ describe('installBrokenPipeGuard', () => { expect(shutdown.isArmed).toBe(false); }); + it('armed scope: a failing verdict after EPIPE is never overridden by the guard', async () => { + const stdout = new EventEmitter(); + const stderr = new EventEmitter(); + const exit = vi.fn(); + const setTimer = vi.fn(() => ({ unref: vi.fn() })); + const shutdown = new ShutdownController(); + const disarm = shutdown.arm(); + const verdict: { code?: number } = {}; + installBrokenPipeGuard({ + stdout, + stderr, + exit, + shutdown, + setTimer, + exitCode: () => verdict.code, + }); + + stdout.emit('error', makeEpipe()); + // The tunnel lifetime's teardown runs, then the run's verdict propagates. + disarm(); + verdict.code = 1; + await Promise.resolve(); + expect(exit).not.toHaveBeenCalled(); + expect(setTimer).not.toHaveBeenCalled(); + expect(shutdown.received).toBe('EPIPE'); + }); + it('re-throws a non-EPIPE stdout error instead of silently swallowing it', () => { const stdout = new EventEmitter(); const stderr = new EventEmitter(); diff --git a/src/lib/interrupt.ts b/src/lib/interrupt.ts index 939a2fd5..81b010e9 100644 --- a/src/lib/interrupt.ts +++ b/src/lib/interrupt.ts @@ -12,10 +12,11 @@ * Broken pipe: when output is piped to a reader that closes early * (`testsprite ... | head`), the kernel raises `EPIPE` on the next stdout write. * Node turns an `'error'` with no listener into an uncaughtException and dumps a - * raw `write EPIPE` stack (exit 1). Outside an armed lifetime the guard exits - * 0 immediately, the conventional SIGPIPE-equivalent result. While armed it - * first requests coordinated shutdown so tunnel cleanup can finish, then the - * same synthetic interrupt reason preserves exit 0. + * raw `write EPIPE` stack (exit 1). Outside an armed lifetime the guard lets + * the command settle so its own exit code (a failing verdict included) stands, + * with a backstop exit for a process left idle; a clean result still exits 0. + * While armed it first requests coordinated shutdown so tunnel cleanup can + * finish, then the same synthetic interrupt reason preserves exit 0. * * `process` and the streams are injectable so the wiring is unit-testable * without spawning a subprocess or sending a real signal. @@ -339,20 +340,28 @@ export interface BrokenPipeDeps { exit?: (code: number) => void; /** Shutdown coordinator. Defaults to {@link globalShutdown}. */ shutdown?: ShutdownController; + /** Backstop timer. Defaults to `setTimeout`. */ + setTimer?: (callback: () => void, delayMs: number) => Pick; + /** The exit code set so far. Defaults to reading `process.exitCode`. */ + exitCode?: () => number | string | null | undefined; } /** * Guard against `EPIPE` on stdout/stderr so piping to a reader that closes * early (`testsprite ... | head`) exits cleanly instead of crashing with an * unhandled `write EPIPE` stack. An armed stdout path requests graceful - * shutdown before exiting 0; a disarmed one exits 0 immediately. Only `EPIPE` - * is swallowed; any other stream error is left to surface normally. + * shutdown before exiting 0; a disarmed one leaves the exit code to the + * command and only arms a backstop. Only `EPIPE` is swallowed; any other + * stream error is left to surface normally. */ export function installBrokenPipeGuard(deps: BrokenPipeDeps = {}): void { const stdout = deps.stdout ?? process.stdout; const stderr = deps.stderr ?? process.stderr; const exit = deps.exit ?? ((code: number) => process.exit(code)); const shutdown = deps.shutdown ?? globalShutdown; + const setTimer = deps.setTimer ?? setTimeout; + const exitCode = deps.exitCode ?? (() => process.exitCode); + let backstopArmed = false; stdout.on('error', (error: NodeJS.ErrnoException) => { // Reader went away (`| head`, `| less` then q): exit cleanly like SIGPIPE @@ -366,7 +375,17 @@ export function installBrokenPipeGuard(deps: BrokenPipeDeps = {}): void { shutdown.brokenPipe(); return; } - exit(0); + // Exiting here would race index.ts, which sets a failing verdict's exit + // code only after an async telemetry flush, and turn it into 0. Let the + // command settle; once the event loop drains the process exits with the + // code it set. The unref'd backstop only fires when something keeps the + // process alive (an idle prompt, a stdout `drain` that never comes). + if (backstopArmed) return; + backstopArmed = true; + setTimer(() => { + const code = Number(exitCode() ?? 0); + exit(Number.isInteger(code) ? code : 0); + }, INTERRUPT_EXIT_BACKSTOP_MS).unref(); return; } throw error;