Skip to content

fix(daemon,serve): handle log WriteStream and HTTP body stream errors - #280

Merged
steipete merged 3 commits into
openclaw:mainfrom
SebTardif:fix/daemon-log-serve-stream-errors
Aug 8, 2026
Merged

fix(daemon,serve): handle log WriteStream and HTTP body stream errors#280
steipete merged 3 commits into
openclaw:mainfrom
SebTardif:fix/daemon-log-serve-stream-errors

Conversation

@SebTardif

@SebTardif SebTardif commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Two long-lived Node stream paths in mcporter can take down or leak work under ordinary I/O failures:

  1. Daemon log file (createLogContext): the append WriteStream had no error listener until dispose. Disk-full / EIO on that stream becomes an uncaughtException and kills the daemon process.
  2. HTTP serve body (handleNodeRequest): only body.on('error') destroyed the response. Client abort / response-side errors did not destroy the body stream, so partial writes and open readers could leak until GC.

Summary

  • Attach an error listener immediately after opening the daemon log WriteStream; warn, drop the writer, and keep logging to console.
  • Route serve response bodies through pipeline() (exported as pipeHttpResponseBody) so body and HTTP response clean each other up on either side error.

Evidence

Live Node against a build of this branch (722f57c), not a test runner. Three boundaries:

  1. createLogContext ENOSPC on the real WriteStream
  2. Real HTTP server using production pipeHttpResponseBody with a live client (full stream success + mid-stream abort)
  3. Production serveHttp /mcp endpoint completing initialize + tools/list
$ node -v
v26.5.1

$ node --input-type=module proof.mjs
=== daemon log WriteStream (createLogContext) ===
writer error listeners at open: 1
uncaught after ENOSPC emit: 0
writer cleared after error: true
[daemon] Log file write error (.../daemon.log): ENOSPC: no space left on device

=== real HTTP success path (pipeHttpResponseBody + live client) ===
listen: 127.0.0.1:50157
response chunks received: 5
response body: "chunk-0\nchunk-1\nchunk-2\nchunk-3\nchunk-4\n"
completed full stream (chunk-4 present): true

=== real HTTP abort path (client destroy mid-stream) ===
listen: 127.0.0.1:50159
client chunks before destroy: 2
pipeHttpResponseBody settled: true
server-owned body destroyed after client abort: true

=== serveHttp production endpoint (ordinary completion) ===
listen: 127.0.0.1:50161/mcp
initialize status: 200
tools/list status: 200
tools/list body (truncated): event: message data: {"result":{"tools":[{"name":"alpha__ping","description":"[alpha] ping","inputSchema":{"type":"object"}}]},"jsonrpc":"2.0","id":2}
tools/list includes ping: true

Without the log fix, listenerCount('error') is 0 and ENOSPC becomes uncaught. Without pipeline cleanup, client abort leaves the server-owned body stream alive.

Real behavior proof

  • Behavior or issue addressed: Daemon log WriteStream errors no longer crash the process; HTTP response body streams are destroyed on client abort while ordinary streamed and serveHttp responses still complete.

  • Real environment tested: macOS, Node v26.5.1, mcporter built from branch tip 722f57c under /tmp/oc-mcporter-280.

  • Exact steps or command run after this patch: Built the branch, then ran a live Node script that (1) opened createLogContext and emitted ENOSPC on the real WriteStream, (2) started a real http.createServer that pipes a multi-chunk Readable through production pipeHttpResponseBody and completed a full client GET, (3) repeated with the client calling req.destroy() after 2 chunks, and (4) started production serveHttp on 127.0.0.1 and completed initialize + tools/list against /mcp.

  • Evidence after fix: terminal output from the patched build:

    writer error listeners at open: 1
    uncaught after ENOSPC emit: 0
    writer cleared after error: true
    [daemon] Log file write error (.../daemon.log): ENOSPC: no space left on device
    response chunks received: 5
    completed full stream (chunk-4 present): true
    client chunks before destroy: 2
    pipeHttpResponseBody settled: true
    server-owned body destroyed after client abort: true
    initialize status: 200
    tools/list status: 200
    tools/list includes ping: true
  • Observed result after fix: ENOSPC is warned and the writer is cleared with zero uncaught exceptions. A live HTTP client can fully drain a streamed body. Aborting that client mid-stream settles pipeHttpResponseBody and destroys the server-owned body. Production serveHttp still returns successful initialize and tools/list over the real /mcp endpoint.

  • What was not tested: Multi-hour daemon uptime under sustained disk pressure; TLS-terminated reverse proxies in front of serve.

@clawsweeper clawsweeper Bot added 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. P1 Urgent regression or broken agent/channel workflow affecting real users now. labels Aug 6, 2026
@clawsweeper

clawsweeper Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed August 8, 2026, 5:16 PM ET / 21:16 UTC.

ClawSweeper review

What this changes

The PR installs immediate daemon log-writer error handling and uses bidirectional HTTP stream piping so file I/O failures and client disconnects clean up safely.

Merge readiness

⚠️ Ready for maintainer review - 1 item remains

Keep open: current main still lacks both stream-safety behaviors, while the updated PR supplies a narrow implementation, focused tests, and live runtime evidence.

Priority: P1
Reviewed head: 592ece0856c50d27aa6b04018e900889b13b2270

Review scores

Measure Result What it means
Overall readiness 🦞 diamond lobster (5/6) Strong live proof and focused, conventional Node stream handling support a high-confidence merge review.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The PR body provides after-fix live terminal evidence for the real writer, direct HTTP streaming and abort cleanup, and the production MCP endpoint.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The PR body provides after-fix live terminal evidence for the real writer, direct HTTP streaming and abort cleanup, and the production MCP endpoint.
Evidence reviewed 6 items Current main remains exposed: Current main creates the daemon WriteStream without an immediate error listener, so asynchronous writer errors are not handled at creation time.
Current main uses one-way response piping: The current serve path converts the web body and pipes it directly to the response; the PR replaces this with pipeline-based bidirectional teardown.
Narrow production repair: The PR attaches the WriteStream listener before assigning the writer and clears it after an error; its HTTP helper awaits pipeline and deliberately avoids a second response write after terminal stream errors.
Findings None None.
Security None None.

How this fits together

Daemon logging writes optional operational events to an append-only file while continuing to print to the console. The HTTP serve bridge converts MCP web response bodies into Node HTTP responses for connected clients.

flowchart LR
  A[Daemon log configuration] --> B[Log file writer]
  B --> C[Writer error handling]
  C --> D[Console logging fallback]
  E[MCP web response body] --> F[HTTP stream bridge]
  G[Client disconnect or body error] --> F
  F --> H[HTTP client response]
Loading

Before merge

  • Resolve merge risk (P1) - The live proof covers direct Node-client aborts, but not disconnect propagation through a TLS-terminating reverse proxy.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and regression delta production +26/-3, tests +128, changelog +2 The small runtime change is accompanied by focused coverage for both log-writer and response-stream failure directions.

Merge-risk options

Maintainer options:

  1. Merge with the scoped stream proof (recommended)
    Accept the remaining proxy-specific coverage gap because direct client abort, full streaming, daemon writer failure, and production endpoint behavior are all demonstrated.
  2. Add proxy abort smoke proof
    Run a TLS or reverse-proxy client-abort smoke test before merge if that deployment boundary is release-critical.

Technical review

Best possible solution:

Retain the immediate log-writer listener and pipeline-based response teardown, with the focused regressions guarding both failure directions.

Do we have a high-confidence way to reproduce the issue?

Yes. Current main's missing early WriteStream listener and one-way body piping establish the relevant failure paths from source, and the PR includes a live after-fix Node run for both boundaries.

Is this the best way to solve the issue?

Yes. Attaching the listener at writer creation and delegating paired stream teardown to Node's pipeline API is the narrowest maintainable repair for the established behavior.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 4e8e37df2004.

Labels

Label changes:

  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🦞 diamond lobster, so this older rating label is no longer current.

Label justifications:

  • P1: Unhandled daemon log errors can terminate a running daemon, and response-stream cleanup affects active HTTP serving.
  • merge-risk: 🚨 availability: The PR changes terminal behavior for daemon file writers and live HTTP response streams, where an integration mistake could affect process or request availability.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body provides after-fix live terminal evidence for the real writer, direct HTTP streaming and abort cleanup, and the production MCP endpoint.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides after-fix live terminal evidence for the real writer, direct HTTP streaming and abort cleanup, and the production MCP endpoint.

Evidence

What I checked:

  • Current main remains exposed: Current main creates the daemon WriteStream without an immediate error listener, so asynchronous writer errors are not handled at creation time. (src/daemon/log-context.ts:26, 4e8e37df2004)
  • Current main uses one-way response piping: The current serve path converts the web body and pipes it directly to the response; the PR replaces this with pipeline-based bidirectional teardown. (src/serve.ts:235, 4e8e37df2004)
  • Narrow production repair: The PR attaches the WriteStream listener before assigning the writer and clears it after an error; its HTTP helper awaits pipeline and deliberately avoids a second response write after terminal stream errors. (src/serve.ts:244, 592ece0856c5)
  • Regression coverage and prior-review continuity: The updated head removes the earlier process-wide uncaught-exception handler pattern and instead verifies direct writer error handling plus both pipe error directions. (tests/daemon-log-context.test.ts:1, 592ece0856c5)
  • Feature provenance: The daemon logging context dates to the host-support extraction, while the HTTP bridge was introduced by the serve feature; the current PR head was finalized by Peter Steinberger. (src/daemon/log-context.ts:26, 592ece0856c5)
  • Real behavior proof: The PR body records a built-branch live Node run covering ENOSPC handling, a complete streamed response, a mid-stream client abort with body destruction, and initialize/tools-list through the production endpoint. (722f57c)

Likely related people:

  • steipete: Introduced the current daemon logging context in commit 41b0cbc and authored the current PR-head cleanup commit. (role: daemon-path introducer and recent stream-cleanup contributor; confidence: high; commits: 41b0cbcc9cc5, 592ece0856c5; files: src/daemon/log-context.ts, src/serve.ts)
  • zm2231: Introduced the HTTP serve bridge and later per-server endpoint work that owns the response path. (role: serve bridge introducer; confidence: high; commits: 6879a69f49e7, 2c04671b92f9; files: src/serve.ts)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

History

Review history (22 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-07T22:56:52.846Z sha 839fc6a :: needs changes before merge. :: [P3] Always remove the temporary uncaught-exception handler
  • reviewed 2026-08-08T01:14:02.420Z sha 839fc6a :: needs changes before merge. :: [P3] Always remove the temporary uncaught-exception handler
  • reviewed 2026-08-08T05:43:55.828Z sha 839fc6a :: needs changes before merge. :: [P3] Guarantee cleanup of the temporary uncaught-exception handler
  • reviewed 2026-08-08T11:52:41.507Z sha 839fc6a :: needs changes before merge. :: [P3] Guarantee cleanup of the temporary uncaught-exception handler
  • reviewed 2026-08-08T14:05:30.801Z sha 839fc6a :: needs changes before merge. :: [P3] Guarantee cleanup of the temporary uncaught-exception handler
  • reviewed 2026-08-08T16:54:42.499Z sha 839fc6a :: needs changes before merge. :: [P3] Always clean up the test's process-level handler
  • reviewed 2026-08-08T19:02:57.903Z sha 839fc6a :: needs changes before merge. :: [P3] Guarantee cleanup of the process-level handler
  • reviewed 2026-08-08T20:15:00.815Z sha 839fc6a :: needs changes before merge. :: [P3] Always clean up the process-wide test handler

@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Aug 6, 2026
@SebTardif

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Updated proof with live HTTP server evidence on the production pipeHttpResponseBody path: full stream completion and mid-stream client abort (server-owned body destroyed), plus serveHttp /mcp initialize + tools/list success and the createLogContext ENOSPC path.

@clawsweeper

clawsweeper Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@clawsweeper clawsweeper Bot added 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. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels Aug 7, 2026
Attach an error listener when opening the daemon log file so ENOSPC/EIO
cannot become an uncaughtException. Pipe serve response bodies through
pipeline() so client abort and body failure destroy both sides.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
Co-authored-by: Sebastien Tardif <sebtardif@ncf.ca>
@steipete
steipete force-pushed the fix/daemon-log-serve-stream-errors branch from 839fc6a to 592ece0 Compare August 8, 2026 21:13
@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Aug 8, 2026
@steipete

steipete commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

Exact-head maintainer proof for 592ece0856c50d27aa6b04018e900889b13b2270:

  • Rebased onto current main and retained Sebastien Tardif / @SebTardif as author of both contributor commits. The maintainer follow-up carries his co-author trailer and adds separate 0.13.1 CLI/Daemon changelog credits.
  • Root cause/provenance: the original daemon log context opened a long-lived WriteStream without asynchronous error ownership, while the original HTTP bridge used one-way body.pipe(response). The first can surface ENOSPC/EIO as an uncaught process error; the second does not make response-side aborts own source teardown.
  • Fix: attach the daemon writer error listener immediately before publishing the writer; use stream/promises pipeline() as the sole bidirectional HTTP body/response teardown owner; treat disconnect/body failures as terminal instead of attempting a second 500 write.
  • Review blocker resolved: tests/daemon-log-context.test.ts no longer installs a process-wide uncaughtException listener, retains the original writer, asserts its error directly, and closes/destroys it deterministically in finally.
  • Diff: 5 files, +156/-3. Production +26/-3, tests +128, changelog +2. Risk is limited to daemon log availability and HTTP response teardown; no protocol, config, or dependency change.
  • Focused proof: pnpm exec vitest run tests/daemon-log-context.test.ts tests/serve-stream-errors.test.ts tests/serve.test.ts tests/daemon-host.test.ts tests/cli-serve-runtime.test.ts — 5 files, 52 tests passed.
  • Repository gates: pnpm docs:list, pnpm check, pnpm docs:site, pnpm test, and git diff --check — all clean. Full suite: 185 files passed / 4 skipped; 1,393 tests passed / 26 skipped.
  • Direct built-artifact Node proof: the real daemon WriteStream had one error listener at open; synthetic ENOSPC emission returned without throwing, warned, cleared the writer, and execution continued. A live HTTP client received all five numbered chunks on success. Destroying the client after two chunks settled production pipeHttpResponseBody and destroyed the server-owned body. A real client connected to production serveHttp and listed alpha__ping.
  • Structured Codex-backed autoreview: final integrated branch run clean with no accepted/actionable findings; secret scan clean.
  • Exact diff/public-proof model-identifier audit: PASS.
  • Exact-head CI: run 31278793554 passed Ubuntu, macOS, and Windows. PR state is mergeable/CLEAN.

Gaps: no sustained real disk exhaustion, kernel-level EIO injection, TLS reverse proxy, or multi-hour daemon soak. Those are not required to exercise these in-process ownership boundaries.

Thanks @SebTardif for the focused production fix, regression coverage, and direct live Node evidence.

@steipete
steipete merged commit c2747e2 into openclaw:main Aug 8, 2026
7 checks passed
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. P1 Urgent regression or broken agent/channel workflow affecting real users now. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor 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.

2 participants