diff --git a/.fallowrc.json b/.fallowrc.json index dbbdc5eb7..fa99e2b27 100644 --- a/.fallowrc.json +++ b/.fallowrc.json @@ -23,6 +23,11 @@ "scripts/vitest-runner-timeout-setup.ts", "test/contention-retry-fixtures/vitest.fixture.config.ts", "test/contention-retry-fixtures/timeout-provenance.fixture.ts", + // #1596 regression fixtures: run as real `node --experimental-strip-types` + // subprocesses (test/integration/daemon-replace-exit-flush.test.ts), so + // dependency analysis cannot follow the runCmdSync string path to either. + "test/integration/support/exit-naive.ts", + "test/integration/support/exit-after-flush.ts", "src/utils/update-check-entry.ts", "examples/sdk/client-session.ts", "examples/sdk/metro-runtime.ts", diff --git a/src/__tests__/cli-exit-paths.test.ts b/src/__tests__/cli-exit-paths.test.ts new file mode 100644 index 000000000..4c97cf77e --- /dev/null +++ b/src/__tests__/cli-exit-paths.test.ts @@ -0,0 +1,224 @@ +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { afterEach, test, vi } from 'vitest'; +import assert from 'node:assert/strict'; + +vi.mock('../cli/commands/web.ts', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, runWebCommand: vi.fn(async () => 0) }; +}); + +import { runCli } from '../cli.ts'; +import { runWebCommand } from '../cli/commands/web.ts'; +import { installIsolatedCliTestEnv } from './cli-test-env.ts'; +import { resolveDaemonPaths } from '../daemon/config.ts'; +import type { DaemonResponse } from '../daemon/client/daemon-client.ts'; + +afterEach(() => { + vi.clearAllMocks(); +}); + +function installExitSpy(): { calls: number[]; restore: () => void } { + const originalExit = process.exit; + const calls: number[] = []; + (process as any).exit = ((code?: number) => { + calls.push(code ?? 0); + }) as typeof process.exit; + return { calls, restore: () => (process.exit = originalExit) }; +} + +class ProcessExitSentinel extends Error { + code: number; + constructor(code: number) { + super(`process.exit(${code})`); + this.code = code; + } +} + +// `parseCliInputOrExit`'s early-exit branches run before `runCli`'s own +// try/catch, so — unlike `installExitSpy` above — the mock here must behave +// like a real `process.exit()` and actually stop execution (by throwing), +// or the function falls through past its `return exitAfterFlush(...)` into +// code that assumes a command was parsed. +function installTerminatingExitSpy(): { restore: () => void } { + const originalExit = process.exit; + (process as any).exit = ((code?: number) => { + throw new ProcessExitSentinel(code ?? 0); + }) as typeof process.exit; + return { restore: () => (process.exit = originalExit) }; +} + +async function runCliExpectingExit( + argv: string[], + deps: { sendToDaemon: (...args: any[]) => Promise }, +): Promise { + try { + await runCli(argv, deps as any); + } catch (error) { + if (error instanceof ProcessExitSentinel) return error.code; + throw error; + } + throw new Error('expected runCli to exit'); +} + +function captureStdout(): { read: () => string; restore: () => void } { + const originalWrite = process.stdout.write.bind(process.stdout); + let captured = ''; + (process.stdout as { write: typeof process.stdout.write }).write = ((chunk: unknown) => { + captured += String(chunk); + return true; + }) as typeof process.stdout.write; + return { + read: () => captured, + restore: () => { + process.stdout.write = originalWrite; + }, + }; +} + +function captureStderr(): { read: () => string; restore: () => void } { + const originalWrite = process.stderr.write.bind(process.stderr); + let captured = ''; + (process.stderr as { write: typeof process.stderr.write }).write = ((chunk: unknown) => { + captured += String(chunk); + return true; + }) as typeof process.stderr.write; + return { + read: () => captured, + restore: () => { + process.stderr.write = originalWrite; + }, + }; +} + +test('--version exits 0 and prints the version, without touching the daemon', async () => { + const restoreEnv = installIsolatedCliTestEnv(); + const exitSpy = installTerminatingExitSpy(); + const stdout = captureStdout(); + const sendToDaemon = async (): Promise => { + throw new Error('sendToDaemon should not be called for --version'); + }; + + let exitCode: number; + try { + exitCode = await runCliExpectingExit(['--version'], { sendToDaemon }); + } finally { + stdout.restore(); + exitSpy.restore(); + restoreEnv(); + } + + assert.equal(exitCode, 0); + assert.ok(stdout.read().trim().length > 0); +}); + +test('bare `help` with no target exits 0 and prints usage', async () => { + const restoreEnv = installIsolatedCliTestEnv(); + const exitSpy = installTerminatingExitSpy(); + const stdout = captureStdout(); + const sendToDaemon = async (): Promise => { + throw new Error('sendToDaemon should not be called for help'); + }; + + let exitCode: number; + try { + exitCode = await runCliExpectingExit(['help'], { sendToDaemon }); + } finally { + stdout.restore(); + exitSpy.restore(); + restoreEnv(); + } + + assert.equal(exitCode, 0); + assert.ok(stdout.read().includes('agent-device')); +}); + +test('no command exits 1 and prints usage', async () => { + const restoreEnv = installIsolatedCliTestEnv(); + const exitSpy = installTerminatingExitSpy(); + const stdout = captureStdout(); + const sendToDaemon = async (): Promise => { + throw new Error('sendToDaemon should not be called with no command'); + }; + + let exitCode: number; + try { + exitCode = await runCliExpectingExit([], { sendToDaemon }); + } finally { + stdout.restore(); + exitSpy.restore(); + restoreEnv(); + } + + assert.equal(exitCode, 1); + assert.ok(stdout.read().length > 0); +}); + +test("web command exits with runWebCommand's status code", async () => { + const restoreEnv = installIsolatedCliTestEnv(); + const exitSpy = installExitSpy(); + const sendToDaemon = async (): Promise => { + throw new Error('sendToDaemon should not be called for web'); + }; + vi.mocked(runWebCommand).mockResolvedValueOnce(0); + + try { + await runCli(['web', 'status'], { sendToDaemon }); + } finally { + exitSpy.restore(); + restoreEnv(); + } + + assert.deepEqual(exitSpy.calls, [0]); + assert.equal(vi.mocked(runWebCommand).mock.calls.length, 1); + assert.deepEqual(vi.mocked(runWebCommand).mock.calls[0]?.[0], ['status']); +}); + +// #1596: printDaemonLogTailOnError's --debug dump must stay bounded even when +// the daemon log itself is large — otherwise the dump risks the same +// process.exit()-truncates-a-pipe-write failure exitAfterFlush exists to fix. +test('a --debug failure caps the daemon-log-tail dump instead of printing it unbounded', async () => { + const tempRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-cli-log-tail-')); + const stateDir = path.join(tempRoot, 'state'); + fs.mkdirSync(stateDir, { recursive: true }); + const { logPath } = resolveDaemonPaths(stateDir); + + // 200 lines * 500 bytes = 100,000 bytes: within the existing 200-line cap, + // but past the 64,000-byte cap this change adds — so only the byte cap can + // explain the head line being absent from the captured output below. + const headMarker = 'HEAD_OF_SEEDED_LOG_LINE_0000'; + const tailMarker = 'TAIL_OF_SEEDED_LOG_LINE_0199'; + const lines: string[] = []; + for (let i = 0; i < 200; i += 1) { + const marker = i === 0 ? headMarker : i === 199 ? tailMarker : `line-${i}`; + lines.push(`${marker}-${'x'.repeat(500 - marker.length - 1)}`); + } + fs.writeFileSync(logPath, `${lines.join('\n')}\n`); + + const restoreEnv = installIsolatedCliTestEnv(); + const exitSpy = installExitSpy(); + const stderr = captureStderr(); + const sendToDaemon = async (): Promise => ({ + ok: false, + error: { code: 'SESSION_NOT_FOUND', message: 'No active session' }, + }); + + try { + await runCli(['session', 'list', '--state-dir', stateDir, '--debug'], { sendToDaemon }); + } finally { + stderr.restore(); + exitSpy.restore(); + restoreEnv(); + fs.rmSync(tempRoot, { recursive: true, force: true }); + } + + assert.deepEqual(exitSpy.calls, [1]); + const output = stderr.read(); + assert.ok(output.includes('[daemon log]'), 'expected the daemon-log-tail block to be printed'); + assert.ok(output.includes(tailMarker), 'expected the most recent line to survive the byte cap'); + assert.ok( + !output.includes(headMarker), + 'expected the byte cap to drop the oldest lines, not just the 200-line cap', + ); +}); diff --git a/src/bin.ts b/src/bin.ts index b8fc85e00..18b0a2006 100644 --- a/src/bin.ts +++ b/src/bin.ts @@ -33,9 +33,12 @@ function runVersionFastPath(argv: string[]): boolean { function runNoCommandFastPath(argv: string[]): boolean { if (argv.length !== 0) return false; import('./cli/parser/cli-help.ts') - .then(({ buildUsageText }) => { + .then(async ({ buildUsageText }) => { process.stdout.write(`${buildUsageText()}\n`); - process.exit(1); + // #1596: exitAfterFlush (not a bare process.exit) so the full usage + // text reaches a piped caller before the process terminates. + const { exitAfterFlush } = await import('./utils/process-exit.ts'); + await exitAfterFlush(1); }) .catch(handleStartupError); return true; @@ -118,5 +121,8 @@ function runCli(argv: string[]): void { function handleStartupError(error: unknown): void { process.stderr.write(`${error instanceof Error ? error.message : String(error)}\n`); - process.exit(1); + // #1596: exitAfterFlush so the message above isn't dropped on a piped stderr. + import('./utils/process-exit.ts') + .then(({ exitAfterFlush }) => exitAfterFlush(1)) + .catch(() => process.exit(1)); } diff --git a/src/cli.ts b/src/cli.ts index 6f5ae2617..f26298b0b 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -7,6 +7,7 @@ import { throwDaemonError, } from '@agent-device/kernel/errors'; import { printHumanError, printJson } from './utils/output.ts'; +import { exitAfterFlush } from './utils/process-exit.ts'; import { readVersion } from './utils/version.ts'; import { pathToFileURL } from 'node:url'; import { sendToDaemon } from './daemon/client/daemon-client.ts'; @@ -112,7 +113,7 @@ export async function runCli(argv: string[], deps: CliDeps = DEFAULT_CLI_DEPS): debugEnabled, }); const debugOutputEnabled = isParsedDebugRequested(command, parsed.providedFlags); - const ctx = resolveRunContextOrExit(parsed, { + const ctx = await resolveRunContextOrExit(parsed, { command, positionals, requestId, @@ -122,11 +123,11 @@ export async function runCli(argv: string[], deps: CliDeps = DEFAULT_CLI_DEPS): let logTailStopper: (() => void) | null = null; try { if (command === 'react-devtools') { - process.exit(await runReactDevtoolsCli(ctx, deps)); + await exitAfterFlush(await runReactDevtoolsCli(ctx, deps)); return; } if (command === 'web') { - process.exit( + await exitAfterFlush( await runWebCommand(positionals, { flags: ctx.effectiveFlags, stateDir: ctx.daemonPaths.baseDir, @@ -143,7 +144,7 @@ export async function runCli(argv: string[], deps: CliDeps = DEFAULT_CLI_DEPS): await resolveRemoteContext(ctx, deps); registerDaemonAuthDiagnosticValue(ctx.effectiveFlags); if (command === 'cdp') { - process.exit( + await exitAfterFlush( await runAgentCdpCommand(positionals, { flags: ctx.effectiveFlags, runtime: ctx.resolvedRuntime, @@ -165,7 +166,7 @@ export async function runCli(argv: string[], deps: CliDeps = DEFAULT_CLI_DEPS): }); await dispatchCliCommand(ctx, client, replayTestReporterRuntime); } catch (err) { - handleRunCliFailure(err, ctx, logTailStopper); + await handleRunCliFailure(err, ctx, logTailStopper); } finally { if (logTailStopper) logTailStopper(); } @@ -207,7 +208,7 @@ async function parseCliInputOrExit( } else { printHumanError(normalized, { showDetails: options.debugEnabled }); } - process.exit(1); + return exitAfterFlush(1); } for (const warning of parsed.warnings) { @@ -216,7 +217,7 @@ async function parseCliInputOrExit( if (parsed.flags.version) { process.stdout.write(`${options.version}\n`); - process.exit(0); + return exitAfterFlush(0); } const isHelpAlias = parsed.command === 'help'; @@ -224,26 +225,26 @@ async function parseCliInputOrExit( if (isHelpAlias || isHelpFlag) { if (isHelpAlias && parsed.positionals.length > 1) { printHumanError(new AppError('INVALID_ARGS', 'help accepts at most one command.')); - process.exit(1); + return exitAfterFlush(1); } const helpTarget = isHelpAlias ? parsed.positionals[0] : parsed.command; if (!helpTarget) { process.stdout.write(`${await usage()}\n`); - process.exit(0); + return exitAfterFlush(0); } const commandHelp = await usageForCommand(helpTarget); if (commandHelp) { process.stdout.write(commandHelp); - process.exit(0); + return exitAfterFlush(0); } printHumanError(new AppError('INVALID_ARGS', formatUnknownHelpTargetMessage(helpTarget))); process.stdout.write(`${await usage()}\n`); - process.exit(1); + return exitAfterFlush(1); } if (!parsed.command) { process.stdout.write(`${await usage()}\n`); - process.exit(1); + return exitAfterFlush(1); } return { parsed, command: parsed.command, positionals: parsed.positionals }; @@ -270,10 +271,10 @@ type CliRunContext = { parsedBatchSteps: BatchStep[] | undefined; }; -function resolveRunContextOrExit( +async function resolveRunContextOrExit( parsed: ReturnType, base: { command: string; positionals: string[]; requestId: string; debugOutputEnabled: boolean }, -): CliRunContext { +): Promise { const explicitFlagKeys = new Set(parsed.providedFlags.map((entry) => entry.key)); try { const binding = resolveBindingSettings({ @@ -325,7 +326,7 @@ function resolveRunContextOrExit( } else { printHumanError(normalized, { showDetails: base.debugOutputEnabled }); } - process.exit(1); + return exitAfterFlush(1); } } @@ -536,11 +537,11 @@ async function dispatchCliCommand( throw new AppError('INVALID_ARGS', formatUnhandledCommandMessage(command)); } -function handleRunCliFailure( +async function handleRunCliFailure( err: unknown, ctx: CliRunContext, logTailStopper: (() => void) | null, -): void { +): Promise { const appErr = asAppError(err); const normalized = normalizeError(appErr, { diagnosticId: getDiagnosticsMeta().diagnosticId, @@ -564,15 +565,24 @@ function handleRunCliFailure( } } if (logTailStopper) logTailStopper(); - process.exit(1); + // #1596: a bare `process.exit()` right after these writes can drop them — + // Node flushes stdout/stderr synchronously only to a file or TTY, and this + // CLI is commonly piped by whatever is driving it. `exitAfterFlush` waits + // for the writes above to actually reach the pipe first. + await exitAfterFlush(1); } +const DAEMON_LOG_TAIL_MAX_BYTES = 64_000; + function printDaemonLogTailOnError(logPath: string): void { try { if (fs.existsSync(logPath)) { const content = fs.readFileSync(logPath, 'utf8'); const lines = content.split('\n'); - const tail = lines.slice(Math.max(0, lines.length - 200)).join('\n'); + let tail = lines.slice(Math.max(0, lines.length - 200)).join('\n'); + if (tail.length > DAEMON_LOG_TAIL_MAX_BYTES) { + tail = tail.slice(tail.length - DAEMON_LOG_TAIL_MAX_BYTES); + } if (tail.trim().length > 0) { process.stderr.write(`\n[daemon log]\n${tail}\n`); } @@ -801,10 +811,10 @@ function guessSessionFromArgv(argv: string[]): string | null { const isDirectRun = pathToFileURL(process.argv[1] ?? '').href === import.meta.url; if (isDirectRun) { - runCli(process.argv.slice(2)).catch((err) => { + runCli(process.argv.slice(2)).catch(async (err) => { const appErr = asAppError(err); printHumanError(normalizeError(appErr), { showDetails: true }); - process.exit(1); + await exitAfterFlush(1); }); } diff --git a/src/utils/__tests__/process-exit.test.ts b/src/utils/__tests__/process-exit.test.ts new file mode 100644 index 000000000..ff9d4c575 --- /dev/null +++ b/src/utils/__tests__/process-exit.test.ts @@ -0,0 +1,115 @@ +import assert from 'node:assert/strict'; +import { afterEach, test, vi } from 'vitest'; + +vi.mock('../timeouts.ts', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, sleep: vi.fn(async () => {}) }; +}); + +import { exitAfterFlush } from '../process-exit.ts'; +import { sleep } from '../timeouts.ts'; + +type FakeWriteStream = { + writableLength: number; + once: ReturnType; +}; + +function installFakeStdio(streams: { + stdout: FakeWriteStream; + stderr: FakeWriteStream; +}): () => void { + const originalStdout = Object.getOwnPropertyDescriptor(process, 'stdout'); + const originalStderr = Object.getOwnPropertyDescriptor(process, 'stderr'); + Object.defineProperty(process, 'stdout', { value: streams.stdout, configurable: true }); + Object.defineProperty(process, 'stderr', { value: streams.stderr, configurable: true }); + return () => { + if (originalStdout) Object.defineProperty(process, 'stdout', originalStdout); + if (originalStderr) Object.defineProperty(process, 'stderr', originalStderr); + }; +} + +function installExitSpy(): { calls: number[]; restore: () => void } { + const originalExit = process.exit; + const calls: number[] = []; + (process as any).exit = ((code?: number) => { + calls.push(code ?? 0); + }) as typeof process.exit; + return { calls, restore: () => (process.exit = originalExit) }; +} + +function drainedStream(): FakeWriteStream { + return { writableLength: 0, once: vi.fn() }; +} + +afterEach(() => { + vi.clearAllMocks(); +}); + +test('exitAfterFlush exits immediately when both streams are already drained', async () => { + const restoreStdio = installFakeStdio({ stdout: drainedStream(), stderr: drainedStream() }); + const exitSpy = installExitSpy(); + + try { + await exitAfterFlush(1); + } finally { + restoreStdio(); + exitSpy.restore(); + } + + assert.deepEqual(exitSpy.calls, [1]); +}); + +// #1596: proves the queued-write branch — a stream with buffered data must be +// let drain before the process exits, not raced against immediately. +test('exitAfterFlush waits for a backlogged stream to drain before exiting', async () => { + // The FLUSH_TIMEOUT_MS race's other arm is `sleep`, mocked module-wide to + // resolve immediately; override it here to a promise that never resolves so + // this test can only pass via the drain callback, not the timeout racing + // ahead of it. + vi.mocked(sleep).mockImplementationOnce(() => new Promise(() => {})); + let drainCallback: (() => void) | undefined; + const backloggedStderr: FakeWriteStream = { + writableLength: 1024, + once: vi.fn((event: string, cb: () => void) => { + if (event === 'drain') drainCallback = cb; + }), + }; + const restoreStdio = installFakeStdio({ stdout: drainedStream(), stderr: backloggedStderr }); + const exitSpy = installExitSpy(); + + try { + const pending = exitAfterFlush(2); + // Give the microtask queue a turn: exit must not have fired yet, since the + // backlogged stream hasn't drained. + await Promise.resolve(); + await Promise.resolve(); + assert.deepEqual(exitSpy.calls, [], 'exit fired before the backlogged stream drained'); + assert.equal(typeof drainCallback, 'function'); + + drainCallback?.(); + await pending; + } finally { + restoreStdio(); + exitSpy.restore(); + } + + assert.deepEqual(exitSpy.calls, [2]); +}); + +// A stream that never emits 'drain' (a stalled or broken pipe) must not hang +// the process — the FLUSH_TIMEOUT_MS race still exits. `sleep` is mocked +// above so this resolves instantly instead of waiting the real timeout. +test('exitAfterFlush still exits when a stream never drains', async () => { + const neverDrains: FakeWriteStream = { writableLength: 1024, once: vi.fn() }; + const restoreStdio = installFakeStdio({ stdout: drainedStream(), stderr: neverDrains }); + const exitSpy = installExitSpy(); + + try { + await exitAfterFlush(3); + } finally { + restoreStdio(); + exitSpy.restore(); + } + + assert.deepEqual(exitSpy.calls, [3]); +}); diff --git a/src/utils/process-exit.ts b/src/utils/process-exit.ts new file mode 100644 index 000000000..97f47b971 --- /dev/null +++ b/src/utils/process-exit.ts @@ -0,0 +1,29 @@ +import { sleep } from './timeouts.ts'; + +const FLUSH_TIMEOUT_MS = 2000; + +/** + * `process.exit()` truncates output still queued on `process.stdout`/`stderr`: + * Node writes to those streams synchronously only when they're a file or TTY — + * when the CLI runs as a piped subprocess (the common case for an agent + * driving it), writes are queued asynchronously, and `process.exit()` tears + * the process down before a queued write reaches the pipe. A caller then sees + * a truncated or empty final message instead of the structured error. + * `FLUSH_TIMEOUT_MS` bounds the wait so a stalled/broken pipe still exits + * rather than hanging the process. + */ +export async function exitAfterFlush(code: number): Promise { + await Promise.race([drainStdio(), sleep(FLUSH_TIMEOUT_MS)]); + process.exit(code); +} + +async function drainStdio(): Promise { + await Promise.all([drainStream(process.stdout), drainStream(process.stderr)]); +} + +function drainStream(stream: NodeJS.WriteStream): Promise { + if (stream.writableLength === 0) return Promise.resolve(); + return new Promise((resolve) => { + stream.once('drain', resolve); + }); +} diff --git a/test/integration/daemon-replace-exit-flush.test.ts b/test/integration/daemon-replace-exit-flush.test.ts new file mode 100644 index 000000000..273403f81 --- /dev/null +++ b/test/integration/daemon-replace-exit-flush.test.ts @@ -0,0 +1,146 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { skipWhenLoopbackUnavailable } from '../../src/__tests__/test-utils/loopback.ts'; +import { runCmdSync } from '../../src/utils/exec.ts'; +import { stopProcessForTakeover } from '../../src/daemon/daemon-process.ts'; +import { isProcessAlive } from '../../src/utils/host-process.ts'; +import { runCliJson } from './test-helpers.ts'; +import { PAYLOAD_MARKER } from './support/exit-payload.ts'; + +// #1596: a CLI command that finds its recorded daemon unreachable replaces it +// (`Replacing daemon (pid N, vX) in : unreachable`) and retries +// against a fresh one. Three field runs died with zero further agent actions +// immediately after that replace plus a SESSION_NOT_FOUND (the fresh daemon +// has no sessions yet, which is expected). This file locks down that a +// replace-mid-command always ends in a normal, fully-delivered structured +// error rather than a truncated or hung process. + +const SUPPORT_DIR = path.join(import.meta.dirname, 'support'); +const FIXTURE_TIMEOUT_MS = 10_000; + +type DaemonInfo = { + pid: number; + processStartTime?: string; +}; + +test('daemon replace mid-command returns a structured, parseable error and exits normally', async (t) => { + if (await skipWhenLoopbackUnavailable(t)) { + return; + } + + const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-replace-exit-flush-')); + let info: DaemonInfo | null = null; + try { + // A real daemon, started by this codebase, so its recorded version/code + // signature legitimately match — the only way to reach the "unreachable" + // takeover reason (as opposed to a version/signature mismatch takeover). + const started = runCliJson(['session', 'list', '--json', '--state-dir', stateDir]); + assert.equal(started.status, 0, `${started.stderr}\n${started.stdout}`); + + info = readDaemonInfo(stateDir); + assert.equal(isProcessAlive(info.pid), true, 'expected the started daemon to be alive'); + + // Kill it out from under its own metadata: daemon.json stays put and + // still points at a pid that is now unreachable, reproducing the crash + // the field transcripts observed. + process.kill(info.pid, 'SIGKILL'); + await waitForProcessDeath(info.pid); + + const result = runCliJson(['close', '--json', '--state-dir', stateDir]); + + assert.equal(result.status, 1, formatUnexpected('exit code', result)); + assert.ok( + result.stderr.includes('Replacing daemon') && result.stderr.includes('unreachable'), + formatUnexpected('takeover notice on stderr', result), + ); + assert.ok(result.json, formatUnexpected('parseable JSON stdout', result)); + assert.equal(result.json.success, false, formatUnexpected('success:false', result)); + assert.equal( + result.json.error?.code, + 'SESSION_NOT_FOUND', + formatUnexpected('SESSION_NOT_FOUND', result), + ); + // #1596 requirement: a hint pointing at `open` is always present, not + // just "fresh daemon, good luck" — this is the daemon.json truthfully + // having no sessions, which is expected; only the error's shape/delivery + // was ever in question. + assert.match( + result.json.error?.hint ?? '', + /open/i, + formatUnexpected('an `open` hint', result), + ); + + info = readDaemonInfo(stateDir); + } finally { + if (info) { + await stopProcessForTakeover(info.pid, { + termTimeoutMs: 1_500, + killTimeoutMs: 1_500, + expectedStartTime: info.processStartTime, + }); + } + fs.rmSync(stateDir, { recursive: true, force: true }); + } +}); + +// Isolates the exact mechanism from the end-to-end test above: Node flushes +// stdout/stderr synchronously only to a file or TTY, so `process.exit()` +// called right after a write can drop that write when the stream is a pipe +// (this CLI's normal condition, driven as a subprocess). Runs the write+exit +// sequence directly as a real piped child process, independent of any +// daemon/device setup, so the mechanism itself is proven deterministically. +test('a bare process.exit() after a large write truncates it on a piped stream', () => { + const { exitCode, stderr } = runFixture('exit-naive.ts'); + assert.equal(exitCode, 1); + assert.ok( + !stderr.includes(PAYLOAD_MARKER), + 'expected the naive exit to truncate before the trailing marker; the pipe-buffer ' + + 'reproduction this test depends on may not hold on this platform', + ); +}); + +test('exitAfterFlush (the #1596 fix) delivers the full write before the process exits', () => { + const { exitCode, stderr } = runFixture('exit-after-flush.ts'); + assert.equal(exitCode, 1); + assert.ok( + stderr.includes(PAYLOAD_MARKER), + 'expected the full payload, including its trailing marker', + ); +}); + +function runFixture(name: string): { exitCode: number; stderr: string } { + const result = runCmdSync( + process.execPath, + ['--experimental-strip-types', path.join(SUPPORT_DIR, name)], + { allowFailure: true, timeoutMs: FIXTURE_TIMEOUT_MS }, + ); + return { exitCode: result.exitCode, stderr: result.stderr }; +} + +async function waitForProcessDeath(pid: number): Promise { + const deadline = Date.now() + 5_000; + while (Date.now() < deadline) { + if (!isProcessAlive(pid)) return; + await new Promise((resolve) => setTimeout(resolve, 25)); + } + assert.fail(`daemon pid ${pid} did not die after SIGKILL`); +} + +function readDaemonInfo(stateDir: string): DaemonInfo { + return JSON.parse(fs.readFileSync(path.join(stateDir, 'daemon.json'), 'utf8')) as DaemonInfo; +} + +function formatUnexpected( + expected: string, + result: { status: number; stdout: string; stderr: string }, +): string { + return [ + `expected ${expected}`, + `status: ${result.status}`, + `stdout: ${result.stdout || '(empty)'}`, + `stderr: ${result.stderr || '(empty)'}`, + ].join('\n'); +} diff --git a/test/integration/support/exit-after-flush.ts b/test/integration/support/exit-after-flush.ts new file mode 100644 index 000000000..29a0b4b5b --- /dev/null +++ b/test/integration/support/exit-after-flush.ts @@ -0,0 +1,7 @@ +// Counterpart to exit-naive.ts using the #1596 fix: same oversized write, +// exited through `exitAfterFlush` instead of a bare `process.exit()`. +import { exitAfterFlush } from '../../../src/utils/process-exit.ts'; +import { buildPayload } from './exit-payload.ts'; + +process.stderr.write(buildPayload()); +await exitAfterFlush(1); diff --git a/test/integration/support/exit-naive.ts b/test/integration/support/exit-naive.ts new file mode 100644 index 000000000..dfd3ec0a5 --- /dev/null +++ b/test/integration/support/exit-naive.ts @@ -0,0 +1,9 @@ +// Regression fixture for #1596: writes a payload larger than a pipe's kernel +// buffer to stderr, then exits the way `src/cli.ts` used to (a bare +// `process.exit()` right after the write). Run as a real subprocess by +// process-exit.test.ts — Node flushes stdout/stderr synchronously only to a +// file or TTY, so a piped parent reliably observes the write truncated. +import { buildPayload } from './exit-payload.ts'; + +process.stderr.write(buildPayload()); +process.exit(1); diff --git a/test/integration/support/exit-payload.ts b/test/integration/support/exit-payload.ts new file mode 100644 index 000000000..c3b60e8c5 --- /dev/null +++ b/test/integration/support/exit-payload.ts @@ -0,0 +1,10 @@ +// Shared payload for the #1596 exit-flush fixtures: sized past a pipe's +// kernel buffer (64 KiB on macOS/Linux) so a truncated write is observable, +// and ends with a marker that only survives the write if it wasn't cut off. +export const PAYLOAD_MARKER = 'EXIT_PAYLOAD_END_MARKER'; +const PAYLOAD_BYTES = 200_000; + +export function buildPayload(): string { + const body = 'x'.repeat(PAYLOAD_BYTES - PAYLOAD_MARKER.length - 1); + return `${body}${PAYLOAD_MARKER}\n`; +}