Skip to content

fix: flush stdout before forced exit to avoid 64KB pipe truncation#215

Merged
steipete merged 9 commits into
openclaw:mainfrom
badmoo:fix/stdout-pipe-truncation-on-forced-exit
Jun 26, 2026
Merged

fix: flush stdout before forced exit to avoid 64KB pipe truncation#215
steipete merged 9 commits into
openclaw:mainfrom
badmoo:fix/stdout-pipe-truncation-on-forced-exit

Conversation

@badmoo

@badmoo badmoo commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

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 forced process.exit() ~50ms later (force-exit is on by default), without waiting for stdout to drain. On POSIX, writes to a pipe are asynchronousconsole.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 forced process.exit() then discards them. File writes are synchronous, which is why > file was unaffected.

The existing FORCE_EXIT_GRACE_MS = 50 (from #145 / #151) only delays the exit by a fixed window; it does not wait for the buffer to actually drain, so outputs larger than the pipe buffer were still truncated.

Fix

Flush stdout before 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.

const STDOUT_FLUSH_TIMEOUT_MS = 2000;

const scheduleForcedExit = () => {
  if (shouldForceExit) {
    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);
  }
};

Behavior proof

A large-output stdio MCP server returns a ~200KB text result. Running through a delayed pipe reader (| (sleep 0.5; cat) | wc -c) vs a file redirect:

=== BEFORE FIX (src/cli.ts @ origin/main) ===
  > file (redirect)        : 200074 bytes
  | (sleep 0.5; cat)|wc -c :  65536 bytes   <- TRUNCATED at the pipe buffer

=== AFTER FIX ===
  > file (redirect)        : 200074 bytes
  | (sleep 0.5; cat)|wc -c : 200074 bytes   <- COMPLETE (matches file)

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 with describe.skipIf(process.platform === 'win32') since it relies on sh/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 at 65536).
  • 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 --check all pass; existing tests/cli-force-exit-behavior.integration.test.ts still passes.

@badmoo badmoo force-pushed the fix/stdout-pipe-truncation-on-forced-exit branch from f8134ec to c9e7cf1 Compare June 23, 2026 03:26
@clawsweeper

clawsweeper Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed June 24, 2026, 2:23 PM ET / 18:23 UTC.

Summary
The PR changes CLI forced-exit shutdown to flush stdout and stderr with a bounded timeout and adds POSIX integration tests for large pipe output, stalled readers, and early-closing consumers.

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.

  • Changed Surface: 2 files affected, +235/-3. The diff is narrow, but it touches CLI shutdown behavior that affects runtime-backed commands.
  • Shutdown Cap: 50 ms grace plus 2000 ms drain fallback. Maintainers need to accept the new bounded wait because it changes worst-case forced-exit timing for non-draining pipes.
  • Open Peer Candidate: 1 overlapping open PR. The same canonical bug is also targeted by fix: flush CLI stdio before forced exit #216, so maintainers need one landing path.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #214
Summary: This PR is a candidate fix for the open large-piped-output truncation bug; #216 is an overlapping candidate for the same root cause.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • none.

Risk before merge

  • [P1] fix: flush CLI stdio before forced exit #216 is an overlapping open candidate for the same canonical bug, so maintainers should choose one branch or combine the stronger pieces before either lands.
  • [P2] The 2s drain fallback changes the forced-exit availability tradeoff for stalled stdout or stderr consumers; the PR covers the intended cases, but downstream pipe timing remains a maintainer-owned compatibility choice.
  • [P1] The PR changes cleanup-time broken-pipe behavior by swallowing stdout/stderr stream errors during forced shutdown, which is likely desirable for shell pipelines but still needs maintainer acceptance.

Maintainer options:

  1. Choose One Landing Branch (recommended)
    Compare this PR with fix: flush CLI stdio before forced exit #216 and land one final forced-exit drain implementation rather than both overlapping branches.
  2. Accept The 2s Drain Cap
    Maintainers can accept this PR as-is if the longer drain window and EPIPE-tolerant shutdown semantics are the desired CLI pipeline behavior.
  3. Close After Preserving Stronger Coverage
    If maintainers prefer the peer branch, close this PR only after preserving any delayed-pipe, stalled-reader, or EPIPE coverage they want from it.

Next step before merge

  • [P2] The remaining action is maintainer selection between this contributor branch and the overlapping bot branch, plus acceptance of the 2s drain cap; there is no narrow automated repair left.

Security
Cleared: The diff changes CLI shutdown behavior and integration tests only; I found no concrete security or supply-chain concern.

Review details

Best 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 changes

Label justifications:

  • P2: This is a normal-priority CLI correctness fix for silently truncated large machine-readable output in piped workflows.
  • merge-risk: 🚨 compatibility: The PR changes observable shell and child-process behavior by waiting for stdio drains and tolerating cleanup-time broken-pipe errors.
  • merge-risk: 🚨 availability: The PR extends the forced-exit path with a 2s fallback, affecting how quickly the CLI terminates when stdio cannot drain.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body and follow-up comments include before/after terminal output for delayed pipe, file redirect, stalled-reader fallback, and early-close EPIPE behavior.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body and follow-up comments include before/after terminal output for delayed pipe, file redirect, stalled-reader fallback, and early-close EPIPE behavior.
Evidence reviewed

What I checked:

  • AGENTS.md policy read: Read the full 136-line AGENTS.md; its conservative one-ticket, verified bug-fix, and maintainer-choice guidance affected this PR review. (AGENTS.md:1, 2a9b353b2142)
  • Current main forced-exit path: Current main still schedules process.exit(process.exitCode ?? 0) after FORCE_EXIT_GRACE_MS without waiting for stdout or stderr to drain. (src/cli.ts:354, 2a9b353b2142)
  • Large stdout emission path: Large JSON output is emitted with console.log(JSON.stringify(value, null, 2)), matching the source-level pipe-buffer truncation theory when the process exits before stdout drains. (src/cli/output-utils.ts:126, 2a9b353b2142)
  • PR implementation and tests: The PR adds flushWriteStreamForExit(), flushStdioThenForceExit(), shutdown-time stdout/stderr error listeners, and POSIX regression coverage for delayed readers, stalled readers, and EPIPE. (src/cli.ts:17, d083a017a187)
  • PR metadata: GitHub reports this cross-repository PR open and mergeable with 2 changed files, +235/-3, head d083a01, proof and risk labels already present, and no exact-head checks returned in statusCheckRollup. (d083a017a187)
  • History provenance: git blame attributes FORCE_EXIT_GRACE_MS, the current direct forced-exit block, and the JSON output helper to 023314c; git log shows 7491ed5 as recent adjacent CLI work. (src/cli.ts:348, 023314cf31cd)

Likely related people:

  • steipete: git blame attributes the current forced-exit block, FORCE_EXIT_GRACE_MS constant, JSON output helper, and existing forced-exit test history to the v0.12.0 release commit. (role: introduced behavior and prior area owner; confidence: high; commits: 023314cf31cd; files: src/cli.ts, src/cli/output-utils.ts, tests/cli-force-exit-behavior.integration.test.ts)
  • vincentkoc: git log shows recent current-main CLI hardening work touching src/cli.ts and output-adjacent behavior shortly before this PR. (role: recent adjacent contributor; confidence: medium; commits: 7491ed5a857e; files: src/cli.ts, src/cli/output-utils.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Jun 23, 2026
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
@badmoo badmoo force-pushed the fix/stdout-pipe-truncation-on-forced-exit branch from c9e7cf1 to 04699ad Compare June 23, 2026 03:37
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.
@badmoo

badmoo commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the review feedback:

  • @clawsweeper (no regression coverage / needs behavior proof): Added tests/cli-stdout-pipe-truncation.integration.test.ts. It spawns the CLI with a >64KB result and a delayed pipe reader (| (sleep 0.5; cat)) so the kernel pipe buffer fills before draining, then asserts the piped output matches the complete file output byte-for-byte.
    • Proof (macOS): without the fix the piped output is clamped to exactly 65536 bytes (test fails); with the fix it equals the full ~200KB output (test passes).
  • @codex P1 (gate the POSIX shell-pipe test on Windows): The test relies on sh/sleep/cat and POSIX async pipe semantics, so it is now gated with describe.skipIf(process.platform === 'win32'), consistent with other POSIX-only integration tests.

Also note: the current FORCE_EXIT_GRACE_MS = 50 (introduced via #145 / #151) only delays the exit by a fixed window without waiting for the buffer to drain, which is why outputs larger than the pipe buffer were still truncated. This PR waits for the actual drain.

Green gate locally: build, typecheck, lint:oxlint, oxfmt --check all pass.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 23, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/cli.ts Outdated
Comment on lines +355 to +357
process.stdout.write('', () => {
process.exit(process.exitCode ?? 0);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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.
@badmoo

badmoo commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the P2 (src/cli.ts — keep forced exit bounded when stdout cannot drain):

  • Wrapped the drain wait with an unref'd fallback deadline (STDOUT_FLUSH_TIMEOUT_MS = 2000). A shared exited guard ensures process.exit runs at most once, whichever fires first.
    • Normal/slow readers: the empty-chunk write callback fires once output drains → complete output, exits immediately.
    • Consumer that keeps stdout open but never reads: the fallback deadline fires after ~2s → the force-exit guarantee is preserved, no indefinite hang.
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 (still force-exits when stdout is piped to a consumer that never reads) that spawns the CLI with >64KB of output and a paused (non-reading) stdout consumer, then asserts the process still exits (code 0) well within the timeout instead of hanging. Locally it terminates in ~2.25s.

Green gate: build, typecheck, lint:oxlint, oxfmt --check all pass; both pipe-truncation tests pass.

@codex review

@badmoo

badmoo commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

Added real behavior proof (the remaining blocker from @clawsweeper) and updated the PR body.

A large-output stdio MCP server returns a ~200KB text result. Running it through a delayed pipe reader vs a file redirect, before and after the fix:

=== BEFORE FIX (src/cli.ts @ origin/main) ===
  > file (redirect)        : 200074 bytes
  | (sleep 0.5; cat)|wc -c :  65536 bytes   <- TRUNCATED at the pipe buffer

=== AFTER FIX ===
  > file (redirect)        : 200074 bytes
  | (sleep 0.5; cat)|wc -c : 200074 bytes   <- COMPLETE (matches file)

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 still force-exits when stdout is piped to a consumer that never reads.

Both behaviors now have focused regression coverage in tests/cli-stdout-pipe-truncation.integration.test.ts (gated process.platform === 'win32' per the earlier @codex P1).

Green gate locally: build, typecheck, lint:oxlint, oxfmt --check all pass.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 23, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +45 to +46
'|',
'(sleep 0.5; cat)',

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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.
@badmoo

badmoo commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the latest @codex P2 (capture the CLI status in the pipe proof):

runCliThroughPipe() previously used sh -c "cli ... | (sleep 0.5; cat) > file", whose exit status reflects the trailing cat, not mcporter. Switched it to bash -c with set -o pipefail, so a non-zero exit from the CLI (the first pipeline stage) now propagates and a piped-only failure can't be masked. (The test is already win32-gated; bash is available on the macOS/Ubuntu CI legs.)

execFile('bash', ['-c', `set -o pipefail; ${command}`], ...);

Both tests still pass locally; lint:oxlint and oxfmt --check are clean.

@codex review
@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 23, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Chef's kiss.

Reviewed commit: f332593943

ℹ️ 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".

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 23, 2026
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.
@badmoo

badmoo commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

Strengthened the fix to address the @clawsweeper needs-changes findings:

  • [P1] cleanup-time stream errors / broken pipe (EPIPE): Now flush both stdout and stderr before the bounded exit, and attach error listeners during shutdown so a consumer that closes a stream early can no longer surface an unhandled EPIPE and crash the process. The 2s fallback deadline (anti-hang guarantee) is preserved.
  • [P2] EPIPE regression test: Added does not crash when a piped consumer closes stdout early (EPIPE) — pipes a >64KB result to head -c 100 (which closes the read end early) and asserts the CLI exits 0 with no EPIPE/unhandled-error output. Verified it crashed before this change and passes after.

Verified locally (| head -c 100, 3/3 runs): exit code 0, empty stderr.

All tests pass: the truncation, stalled-reader, and new EPIPE cases, plus the existing cli-force-exit-behavior suite. typecheck, lint:oxlint, oxfmt --check are clean.

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
@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 23, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jun 23, 2026
@badmoo

badmoo commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

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:

  1. CI hasn't run — this is a fork PR, so the workflows are pending approval. Could you approve the workflow run? Locally the full gate is green: build, typecheck, lint:oxlint, oxfmt --check, plus the new/existing integration tests.

  2. Overlap with fix: flush CLI stdio before forced exit #216 (opened by the ClawSweeper bot for the same issue). Quick diff so you can pick one or combine:

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.

@steipete steipete merged commit 74d992a into openclaw:main Jun 26, 2026
3 checks passed
@steipete

Copy link
Copy Markdown
Collaborator

Landed in 74d992a.

Maintainer verification:

  • pnpm check && pnpm test && pnpm build: 128 test files passed, 1 skipped; 803 tests passed, 3 skipped; formatting, lint, typecheck, and build clean.
  • Focused integration suite: 6/6 passed.
  • Live local stdio MCP E2E covered a 200 KB response through a delayed real OS pipe, a stalled reader with bounded exit, an early-closing consumer, and asynchronous EPIPE before runtime cleanup. The asynchronous regression exited 1 before the early selective handler and 0 after it.
  • CI green on Ubuntu, macOS, and Windows.

Thanks, @badmoo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]:CLI truncates large output at 64KB when stdout is a pipe (forced process.exit() before stdout drains)

2 participants