fix: flush stdout before forced exit to avoid 64KB pipe truncation#215
Conversation
f8134ec to
c9e7cf1
Compare
|
Codex review: needs maintainer review before merge. Reviewed June 24, 2026, 2:23 PM ET / 18:23 UTC. Summary Reproducibility: yes. from source inspection and contributor proof: current main emits large JSON with console.log and then force-exits after 50 ms without a stdio drain wait. I did not run the live pipe test in this read-only review. Review metrics: 3 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land one verified forced-exit stdout/stderr drain fix for #214 with focused regression coverage and a maintainer-accepted bounded timeout. Do we have a high-confidence way to reproduce the issue? Yes from source inspection and contributor proof: current main emits large JSON with console.log and then force-exits after 50 ms without a stdio drain wait. I did not run the live pipe test in this read-only review. Is this the best way to solve the issue? Yes, pending maintainer choice on branch and timeout value: flushing stdout/stderr before the forced exit with a bounded fallback is the narrow fix that preserves the anti-hang behavior. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 2a9b353b2142. Label changesLabel justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8134ecd99
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| shellQuote(outFile), | ||
| ].join(' '); | ||
| return new Promise((resolve) => { | ||
| execFile('sh', ['-c', command], { cwd: process.cwd(), env: process.env }, (error) => { |
There was a problem hiding this comment.
Gate the POSIX shell pipe test on Windows
The CI workflow runs pnpm test on windows-latest (.github/workflows/ci.yml), but this new integration test is unconditional while invoking sh -c and depending on POSIX pipeline utilities/pipe semantics (sleep, cat, delayed reader). On the Windows leg this exercises shell/path compatibility instead of the stdout-flush behavior and can fail the whole suite; gate the suite with process.platform !== 'win32' like the other POSIX-only integration tests.
Useful? React with 👍 / 👎.
closeRuntimeAfterCommand() called process.exit() ~50ms after printing without waiting for stdout to drain. On POSIX, pipe writes are async, so bytes still queued in libuv were discarded on exit, truncating large piped output at the kernel pipe buffer size (~64KB). File output (synchronous writes) was unaffected. Wait for stdout to flush via an empty-chunk write callback before the forced exit. Closes openclaw#214
c9e7cf1 to
04699ad
Compare
Spawns the CLI with a >64KB result and a delayed pipe reader (sleep+cat) so the kernel pipe buffer fills before draining, then asserts the piped output matches the complete file output byte-for-byte. Without the fix the piped output is clamped to 65536 bytes. POSIX-only; skipped on win32 since it relies on sh/sleep/cat and POSIX async pipe semantics.
|
Addressed the review feedback:
Also note: the current Green gate locally: @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4802f381cd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| process.stdout.write('', () => { | ||
| process.exit(process.exitCode ?? 0); | ||
| }); |
There was a problem hiding this comment.
Keep forced exit bounded when stdout cannot drain
Because this callback is now the only path to process.exit, a command with more than a pipe buffer of stdout piped to a consumer that keeps the fd open but stops reading can never invoke the callback, so the default force-exit path hangs indefinitely instead of terminating after the grace period. Please keep a secondary fallback deadline around the drain wait so normal slow readers still get complete output without removing the force-exit guarantee when stdout cannot drain.
Useful? React with 👍 / 👎.
The stdout-flush callback was the only path to process.exit, so a consumer that keeps stdout open but stops reading (with more than a pipe buffer of output) could block the forced exit forever. Add a fallback timeout (unref'd) around the drain wait so normal slow readers still get complete output while the force-exit guarantee is preserved when stdout cannot drain. Adds a regression test asserting the CLI still exits when stdout is piped to a non-reading consumer.
|
Addressed the P2 (
setTimeout(() => {
let exited = false;
const exit = () => {
if (exited) return;
exited = true;
process.exit(process.exitCode ?? 0);
};
const fallback = setTimeout(exit, STDOUT_FLUSH_TIMEOUT_MS);
fallback.unref?.();
process.stdout.write('', () => {
clearTimeout(fallback);
exit();
});
}, FORCE_EXIT_GRACE_MS);Added a regression test ( Green gate: @codex review |
|
Added real behavior proof (the remaining blocker from @clawsweeper) and updated the PR body. A Anti-hang / bounded-fallback proof (P2): with stdout piped to a consumer that keeps the fd open but never reads, the CLI still force-exits via the fallback deadline (code 0) in ~2.25s rather than hanging. This is covered by the new test Both behaviors now have focused regression coverage in Green gate locally: @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35e9f08b4c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| '|', | ||
| '(sleep 0.5; cat)', |
There was a problem hiding this comment.
Capture the CLI status in the pipe proof
In this sh -c command the exit status is that of the last pipeline stage (cat), not the mcporter process. If a regression makes the CLI exit non-zero only when stdout is piped while cat still drains the bytes, runCliThroughPipe() resolves 0 and expect(pipeCode).toBe(0) passes, so the new regression proof misses a piped-only failure; use pipefail/bash or capture the first pipeline status explicitly.
Useful? React with 👍 / 👎.
The piped runner used 'sh -c "cli ... | (sleep; cat) > file"', whose exit status reflects the trailing cat, not the CLI. Switch to 'bash -c' with 'set -o pipefail' so a non-zero CLI exit (first pipeline stage) propagates and a piped-only failure cannot be masked.
|
Addressed the latest @codex P2 (capture the CLI status in the pipe proof):
execFile('bash', ['-c', `set -o pipefail; ${command}`], ...);Both tests still pass locally; @codex review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Flush both stdout and stderr before the forced exit, and attach error listeners during shutdown so a consumer that closes a stream early (EPIPE) cannot crash the process before the bounded exit runs. The 2s fallback deadline is preserved. Adds a regression test asserting the CLI exits cleanly (no EPIPE crash) when a piped consumer closes stdout early.
|
Strengthened the fix to address the @clawsweeper
Verified locally ( All tests pass: the truncation, stalled-reader, and new EPIPE cases, plus the existing Re: overlap with #216 — that PR was opened by the ClawSweeper bot for the same issue. This PR (#215) is the original contributor fix for #214; it now covers stdout+stderr flushing, broken-pipe tolerance, a generous 2s drain cap, and focused regression tests for the truncation, stalled-reader, and EPIPE paths. Happy to defer to maintainers on which to land. @codex review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Maintainer check-in 🙏 (cc @steipete @vincentkoc) This PR fixes #214 and ClawSweeper now rates it ready for maintainer review (platinum hermit / diamond lobster proof, no contributor-side blockers left). A couple of things that need a maintainer call:
Behavior proof (before/after) and details are in the PR body. Happy to adjust the 2s cap or rebase/squash however you prefer — just let me know which branch you'd like to land. |
|
Landed in Maintainer verification:
Thanks, @badmoo! |
Summary
Fixes #214.
Large command output piped to another process was silently truncated at the kernel pipe buffer size (~64KB). Writing the same output to a file (
> out.json) produced the complete result.Root cause
After a command completes,
closeRuntimeAfterCommand()(src/cli.ts) schedules a forcedprocess.exit()~50ms later (force-exit is on by default), without waiting forstdoutto drain. On POSIX, writes to a pipe are asynchronous —console.log(JSON.stringify(...))only enqueues the data in libuv's write buffer. Once the kernel pipe buffer (~64KB) is full, the remaining bytes stay queued in libuv; the forcedprocess.exit()then discards them. File writes are synchronous, which is why> filewas unaffected.Fix
Flush
stdoutbefore the forced exit, bounded by a fallback deadline so a consumer that keeps stdout open but never reads cannot block the exit indefinitely (preserving the anti-hang guarantee). No-op for TTY/file stdout, where writes are synchronous.Behavior proof
A
large-outputstdio MCP server returns a ~200KB text result. Running through a delayed pipe reader (| (sleep 0.5; cat) | wc -c) vs a file redirect:Stalled-reader (anti-hang) check: with stdout piped to a consumer that keeps the fd open but never reads, the CLI still force-exits via the fallback deadline in ~2.25s (code 0) instead of hanging.
Tests
Adds
tests/cli-stdout-pipe-truncation.integration.test.ts(POSIX-only, gated withdescribe.skipIf(process.platform === 'win32')since it relies onsh/sleep/cat):does not truncate large output when stdout is a pipe— asserts the piped output matches the complete file output byte-for-byte (fails without the fix at65536).still force-exits when stdout is piped to a consumer that never reads— spawns the CLI with a paused (non-reading) stdout consumer and asserts the process still exits well within the timeout.Green gate:
build,typecheck,lint:oxlint,oxfmt --checkall pass; existingtests/cli-force-exit-behavior.integration.test.tsstill passes.