fix(node): release stdout listeners when the stream is already closed

[SebTardif]

What Problem This Solves

attachLocalStdio keeps its stdout error and close listeners after a pending write fails on a stream that is already closed. Node reports ERR_STREAM_DESTROYED on the write callback and does not emit error or close again. The listeners, and the attach closure they capture, stay on that stream for the rest of the process.

Why This Change Was Made

The listener has to stay in place when a write callback fails before the stream emits error or close. That is the case #91 fixed. It does not apply when close already happened before the write settled. The fix records that closed state at the start of cleanup and drops the listeners when the write promise rejects.

User Impact

A stdio attach whose stdout pipe has already closed still rejects with the write error. It no longer pins the stdout stream and the terminal session afterward. A broken pipe on a stream that is still open is unchanged: the error stays inside the attach instead of killing the process.

Evidence

Terminal output from Node v26.10.0, calling attachLocalStdio in dist/node.js with the stdin of a Node process that had already exited.

Before this patch:

$ node /tmp/libterminal-proof-stdio.mjs /Users/sebastientardif/libterminal/dist
{"caught":"ERR_STREAM_DESTROYED","errorListeners":1,"closeListeners":1,"destroyed":true}

After this patch:

$ node /tmp/libterminal-proof-stdio.mjs /tmp/libterminal-f006/dist
{"caught":"ERR_STREAM_DESTROYED","errorListeners":0,"closeListeners":0,"destroyed":true}

Real behavior proof

• Behavior or issue addressed: attachLocalStdio left stdout error and close listeners attached after a write to an already closed stream rejected with ERR_STREAM_DESTROYED.
• Real environment tested: macOS, Node v26.10.0, patched dist/node.js from fix/stdio-closed-listeners.
• Exact steps or command run after this patch: Spawn node -e 'process.exit(0)', wait until its stdin emits close, then call attachLocalStdio with that stdin as stdout and one output byte. Print listenerCount for error and close.
• Evidence after fix: terminal output from the same node command against the patched bundle:

$ node /tmp/libterminal-proof-stdio.mjs /tmp/libterminal-f006/dist
{"caught":"ERR_STREAM_DESTROYED","errorListeners":0,"closeListeners":0,"destroyed":true}

• Observed result after fix: The attach still rejects with ERR_STREAM_DESTROYED, and both listener counts are 0. The unpatched bundle left both counts at 1.
• What was not tested: A Windows console stdout handle.

[clawsweeper]

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

ClawSweeper review

What this changes

The PR releases stdout listeners after a failed write to an already-closed stream, preserves protection during pending destruction, and adds regression coverage and a changelog entry.

Merge readiness

✅ Ready for maintainer review

The prior blocking finding is resolved, and no actionable defect remains in the repaired patch. This PR remains necessary because current main and v0.3.8 still retain listeners after writes to an already-closed stream fail.

Priority: P2
Reviewed head: 34e74f9415aa0d9dd56f7f9c17133e084dd301c0

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	A focused repair with relevant real-pipe evidence, resolved prior feedback, and complementary lifecycle coverage.
Proof confidence	🦞 diamond lobster (5/6)	Sufficient (terminal): The supplied macOS/Node terminal trace exercises the built stdio adapter with a real closed child-process pipe and shows listener counts fall from one each to zero while ERR_STREAM_DESTROYED remains observable. The repaired condition preserves that scenario; the delayed-destruction regression supplements it. No stored-data contract changes.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (terminal): The supplied macOS/Node terminal trace exercises the built stdio adapter with a real closed child-process pipe and shows listener counts fall from one each to zero while ERR_STREAM_DESTROYED remains observable. The repaired condition preserves that scenario; the delayed-destruction regression supplements it. No stored-data contract changes.
Evidence reviewed	6 items	Introduced cleanup change: The pinned main-to-head diff changes only listener cleanup, two regression tests, and the Unreleased changelog. Cleanup now checks stdout.closed rather than stdout.destroyed, preserving protection while destruction is pending. Prior finding resolved: The added real Writable regression establishes destroyed=true and closed=false, rejects the attach with ERR_STREAM_DESTROYED, verifies the retained error listener, then completes delayed destruction and verifies cleanup. The MEMBER repair comment reports the prior-head failure and successful focused tests, pnpm check, and autoreview: #94 (comment). Those executions were reported by the contributor, not performed during this read-only review. Real completed-close proof: The supplied full PR body records macOS/Node v26.10.0 execution of the built Node adapter against the stdin pipe of an exited real child process. After ERR_STREAM_DESTROYED, the unpatched bundle retained one error and one close listener; the patched bundle retained zero of each. The repaired closed-only condition preserves this exercised scenario.
Findings	None	None.
Security	None	None.

How this fits together

The Node adapter connects a terminal session’s byte stream to local stdin and stdout. Its cleanup must release session references while keeping delayed stream errors from escaping into the host process.

flowchart TD
 A[Terminal output bytes] --> B[Node stdio adapter]
 B --> C[Write to stdout]
 C --> D[Write settles]
 D --> E{Stream already closed?}
 E -->|Yes| F[Release listeners]
 E -->|No and write failed| G[Retain error protection]
 G -->|Error or close arrives| F

Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production and test delta	production +2/-1, tests +75/-0	The small cleanup change is supported by separate completed-close and delayed-destruction regressions.

Technical review

Best possible solution:

Keep completed-close cleanup narrow while retaining error protection until an in-progress destruction emits its error or close event.

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

Yes: writing terminal output to a child-process pipe whose close event already fired exposes the missing cleanup on main; the supplied before/after transcript corroborates that source path. This reviewer did not execute the reproduction.

Is this the best way to solve the issue?

Yes: checking completed closure is a focused repair that avoids confusing destroyed streams with streams whose asynchronous destruction has finished.

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

• add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
• add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The supplied macOS/Node terminal trace exercises the built stdio adapter with a real closed child-process pipe and shows listener counts fall from one each to zero while ERR_STREAM_DESTROYED remains observable. The repaired condition preserves that scenario; the delayed-destruction regression supplements it. No stored-data contract changes.
• remove rating: 🦐 gold shrimp: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.
• remove status: ⏳ waiting on author: Current PR status label is status: 👀 ready for maintainer look.
• remove merge-risk: 🚨 availability: Current PR review selected no merge-risk labels.

Label justifications:

• P2: This fixes a bounded Node stdio listener leak while preserving existing delayed-error handling.
• 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 supplied macOS/Node terminal trace exercises the built stdio adapter with a real closed child-process pipe and shows listener counts fall from one each to zero while ERR_STREAM_DESTROYED remains observable. The repaired condition preserves that scenario; the delayed-destruction regression supplements it. No stored-data contract changes.
• proof: sufficient: Contributor real behavior proof is sufficient. The supplied macOS/Node terminal trace exercises the built stdio adapter with a real closed child-process pipe and shows listener counts fall from one each to zero while ERR_STREAM_DESTROYED remains observable. The repaired condition preserves that scenario; the delayed-destruction regression supplements it. No stored-data contract changes.

Evidence

What I checked:

• Introduced cleanup change: The pinned main-to-head diff changes only listener cleanup, two regression tests, and the Unreleased changelog. Cleanup now checks stdout.closed rather than stdout.destroyed, preserving protection while destruction is pending. (src/node.ts:261, 34e74f9415aa)
• Prior finding resolved: The added real Writable regression establishes destroyed=true and closed=false, rejects the attach with ERR_STREAM_DESTROYED, verifies the retained error listener, then completes delayed destruction and verifies cleanup. The MEMBER repair comment reports the prior-head failure and successful focused tests, pnpm check, and autoreview: fix(node): release stdout listeners when the stream is already closed #94 (comment). Those executions were reported by the contributor, not performed during this read-only review. (test/node.test.ts:706, 34e74f9415aa)
• Real completed-close proof: The supplied full PR body records macOS/Node v26.10.0 execution of the built Node adapter against the stdin pipe of an exited real child process. After ERR_STREAM_DESTROYED, the unpatched bundle retained one error and one close listener; the patched bundle retained zero of each. The repaired closed-only condition preserves this exercised scenario.
• Still absent from main and latest release: Both fetched main and v0.3.8 retain listeners on rejection unless stdoutErrorSeen is true; neither checks completed closure. GitHub confirms this PR remains open and unmerged at the pinned head, and the latest published release is v0.3.8. (src/node.ts:263, b0b6d29e8c07)
• Related merged work and routing history: fix(node): keep stdout errors while a failed attach still has output pending #91 broadened pending-write error retention to non-abort failures; it did not implement completed-close cleanup. GitHub commit metadata identifies SebTardif for that merged change and steipete for the earlier stdio cancellation refactor. Local path history also records repeated Node lifecycle work by Vincent Koc. Some older blobs were unavailable locally, preventing a complete blame comparison; GitHub commit patches supplied the relevant historical changes. (src/node.ts:258, c23544438bf9)
• Repository policy and validation: The full root AGENTS.md and contribution guide were read; no scoped policy applies to the changed source or test paths. The patch preserves the Node runtime boundary and includes behavior tests and the required Unreleased entry. Supplied checks pass across Node 22/24 on Linux, macOS, and Windows, plus Node 26 on Linux. No tests or bundled review helpers were run during this review, and the checkout remains clean. (AGENTS.md:20, 34e74f9415aa)

Likely related people:

• SebTardif: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
• steipete: 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 (1 earlier review cycle)

• reviewed 2026-10-01T17:35:55.278Z sha c8706bd :: blocked before merge. :: [P1] Preserve error protection while stdout destruction is pending

[vincentkoc]

Maintainer repair pushed at 34e74f9415aa0d9dd56f7f9c17133e084dd301c0.

• Reproduction on the prior head: the real Writable test failed with expected 0 to be 1 after destroyed=true, closed=false, and ERR_STREAM_DESTROYED settled.
• The repair releases retained listeners early only when stdout.closed is true.
• Destruction in progress retains the error listener until the delayed _destroy callback emits its error and close.
• Completed-close cleanup remains covered.
• The final tree is byte-identical to the independently tested current-main merge result.
• Local proof: pnpm test test/node.test.ts (31 tests), pnpm check (140 tests), and high-reasoning autoreview with no actionable findings.

@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/37099244285
• Updated: 2026-10-03T05:15:40.626Z