fix(stream): report one batch sink failure

[SebTardif]

What Problem This Solves

BatchPublisher can start a second flush while the first sink call is still running. Both flushes share one promise chain. When the sink rejects once, onError runs twice.

Why This Change Was Made

Each threshold write calls flush() and reports a rejection through onError. The second call does not start a second sink invocation; it waits on the same failure. The publisher now reports that failure once.

User Impact

A caller that disconnects or retries from onError sees one notification for one sink failure, including when another full batch was queued behind it. Later writes are still dropped after that failure.

Evidence

Terminal output from Node v26.10.0, constructing BatchPublisher from dist/stream.js with maxBatchBytes: 1. Two one-byte writes ran before the sink rejected.

Before this patch:

$ node /tmp/libterminal-proof-batch.mjs /Users/sebastientardif/libterminal/dist
{"onErrorCalls":2}

After this patch:

$ node /tmp/libterminal-proof-batch.mjs /tmp/libterminal-f008/dist
{"onErrorCalls":1}

Real behavior proof

• Behavior or issue addressed: One BatchPublisher sink rejection invoked onError twice when a second full batch was queued behind it.
• Real environment tested: macOS, Node v26.10.0, patched dist/stream.js from fix/batch-one-sink-error.
• Exact steps or command run after this patch: Create a publisher whose sink stays pending, write two one-byte chunks above maxBatchBytes 1, reject the sink once, and count onError calls.
• Evidence after fix: terminal output from that node command:

$ node /tmp/libterminal-proof-batch.mjs /tmp/libterminal-f008/dist
{"onErrorCalls":1}

• Observed result after fix: onError ran once. The unpatched bundle ran it twice for the same rejection.
• What was not tested: A timer flush overlapping an in-flight sink call.

[clawsweeper]

Codex review: needs maintainer review before merge. Reviewed October 3, 2026, 1:08 AM ET / 05:08 UTC (Revision 3).

ClawSweeper review

What this changes

The PR makes terminal batch publishing report a sink failure once across overlapping flushes, adds regression coverage, and records the fix in the changelog.

Merge readiness

✅ Ready for maintainer review

The PR remains necessary: current main and v0.3.8 still permit duplicate failure notifications. The resolved head has no actionable findings, and the supplied production-class terminal proof is sufficient.

Priority: P2
Reviewed head: 3e74478c432a55ea67db05d8f29ae3233ec6be9e

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	A focused repair with direct before/after runtime evidence, regression coverage, and no blocking findings.
Proof confidence	🐚 platinum hermit (4/6)	Sufficient (terminal): The macOS Node trace exercises compiled BatchPublisher through two writes and one injected sink rejection, observing one callback after the fix versus two before. The production code is unchanged at the resolved head; no persisted-data model changes apply.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (terminal): The macOS Node trace exercises compiled BatchPublisher through two writes and one injected sink rejection, observing one callback after the fix versus two before. The production code is unchanged at the resolved head; no persisted-data model changes apply.
Evidence reviewed	5 items	Pinned introduction: The introduced delta adds one notification guard and routes threshold, timer, and abort rejection handlers through it; sink ordering and public flush rejection remain unchanged. The regression queues two full batches behind one pending sink. Still necessary on main and latest release: Inspected main and v0.3.8 stream blobs: threshold and timer flushes still invoke onError independently for a shared rejected chain. Neither contains failureReported. The merged earlier fix #55 stops buffering after failure but does not deduplicate overlapping notifications. Real production-class proof: The captured PR body reports macOS Node v26.10.0 running compiled dist/stream.js: two threshold writes precede one injected sink rejection; onErrorCalls changes from 2 before to 1 after. This directly exercises the changed BatchPublisher boundary. Timer overlap was expressly untested; all automatic reporting paths use the same guard. No stored-data contract changes.
Findings	None	None.
Security	None	None.

How this fits together

The shared streaming module accepts terminal bytes and batches them into a caller-supplied asynchronous sink. Its error callback lets consuming applications react when output delivery fails.

flowchart LR
  A[Terminal bytes] --> B[Batch buffer]
  B --> C[Threshold or timer flush]
  C --> D[Ordered sink calls]
  D --> E[Sink failure]
  E --> F[Single error notification]
  E --> G[Stop and discard buffered output]

Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production and test delta	production +12/-3 lines; tests +27/-0 lines	The production growth implements one shared notification guard with focused overlapping-flush regression coverage.

Technical review

Best possible solution:

Keep one error notification for the terminally failed publisher while preserving ordered delivery, discarded later writes, and the original rejection for explicit flush callers.

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

Yes: two threshold writes queued before one pending sink rejects reach separate error handlers on current main. Source inspection establishes the path, and the contributor supplies before/after compiled-class output; this review did not execute it.

Is this the best way to solve the issue?

Yes: a shared guard fits the publisher's existing terminal-failure lifecycle and covers all automatic error-reporting paths without changing exports or delivery ordering.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against ae63783b98a9.

Labels

Label changes:

No label changes.

Label justifications:

• P2: Duplicate sink-failure callbacks are a bounded streaming correctness defect affecting consumer teardown or retry handling.
• rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit 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 macOS Node trace exercises compiled BatchPublisher through two writes and one injected sink rejection, observing one callback after the fix versus two before. The production code is unchanged at the resolved head; no persisted-data model changes apply.
• proof: sufficient: Contributor real behavior proof is sufficient. The macOS Node trace exercises compiled BatchPublisher through two writes and one injected sink rejection, observing one callback after the fix versus two before. The production code is unchanged at the resolved head; no persisted-data model changes apply.

Evidence

What I checked:

• Pinned introduction: The introduced delta adds one notification guard and routes threshold, timer, and abort rejection handlers through it; sink ordering and public flush rejection remain unchanged. The regression queues two full batches behind one pending sink. (src/stream.ts:269, 3e74478c432a)
• Still necessary on main and latest release: Inspected main and v0.3.8 stream blobs: threshold and timer flushes still invoke onError independently for a shared rejected chain. Neither contains failureReported. The merged earlier fix fix: stop BatchPublisher writes after the sink fails #55 stops buffering after failure but does not deduplicate overlapping notifications. (src/stream.ts:223, ae63783b98a9)
• Real production-class proof: The captured PR body reports macOS Node v26.10.0 running compiled dist/stream.js: two threshold writes precede one injected sink rejection; onErrorCalls changes from 2 before to 1 after. This directly exercises the changed BatchPublisher boundary. Timer overlap was expressly untested; all automatic reporting paths use the same guard. No stored-data contract changes. (src/stream.ts:224, 693960c6effa)
• Re-review continuity and resolved merge: The behavior implementation and test have no delta from the previously reviewed head. Raw test-merge parents match pinned main followed by pinned head, and the test-merge tree matches the resolved head. The member comment fix(stream): report one batch sink failure #96 (comment) reports 18 focused tests and 138 full-check tests passing; these were not executed by this read-only review. (test/stream.test.ts:236, 3e74478c432a)
• Area history and routing: Main history records SebTardif's merged sink-failure cleanup and vincentkoc's abort-listener maintenance. GitHub commit metadata and patches independently confirm these relevant contributions. Older local blame/patch traversal encountered unavailable promised objects with HTTP 403; no introduction attribution is inferred from those failed reads. (src/stream.ts, ae63783b98a9)

Likely related people:

• SebTardif: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
• vincentkoc: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

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 (2 earlier review cycles)

• reviewed 2026-10-01T17:34:44.556Z sha 693960c :: needs maintainer review before merge. :: none
• reviewed 2026-10-02T09:32:49.358Z sha 693960c :: needs maintainer review before merge. :: none

[vincentkoc]

Maintainer conflict resolution pushed at 3e74478c432a55ea67db05d8f29ae3233ec6be9e.

• The only manual resolution keeps both 0.3.9 changelog bullets.
• The resolved tree is byte-identical to the independently tested current-main merge result.
• Local proof: pnpm test test/stream.test.ts (18 tests) and pnpm check (138 tests).
• High-reasoning autoreview reported no actionable findings for the behavior patch.

@clawsweeper re-review

[clawsweeper]

🦞👀
Exact review queued.

Re-review progress:

• State: Complete
• Detail: The durable review result and its route handoff completed.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/37098892861
• Updated: 2026-10-03T05:09:21.480Z