fix(imessage): handle stdout/stderr stream errors in the RPC client child#101084
Merged
steipete merged 3 commits intoJul 7, 2026
Merged
Conversation
…hild A dead imsg RPC helper can emit an async error on any of its stdio streams. On a raw stream an unhandled 'error' event throws and surfaces as an uncaughtException, crashing the gateway. openclaw#75438 added this guard for stdin but left stdout/stderr — on the same long-lived child — unguarded. Route stdout/stderr stream errors through the existing failAll path via a shared failFromStreamError helper, mirroring the stdin handler. Add a regression test that emits 'error' on each stream and asserts the child does not throw and in-flight requests reject cleanly.
Contributor
|
Codex review: needs real behavior proof before merge. Reviewed July 6, 2026, 10:11 PM ET / 02:11 UTC. Summary PR surface: Source +17, Tests +115. Total +132 across 2 files. Reproducibility: yes. source-reproducible with high confidence: current main has stdout/stderr data handlers but no stdout/stderr error listeners, and a Node EventEmitter probe confirms unhandled error events throw. I did not run a full live imsg gateway reproduction in this read-only review. Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the narrow iMessage RPC hardening after redacted real behavior proof is attached or a maintainer explicitly overrides the proof gate; keep one-shot iMessage CLI stream hardening as separate follow-up work. Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible with high confidence: current main has stdout/stderr data handlers but no stdout/stderr error listeners, and a Node EventEmitter probe confirms unhandled error events throw. I did not run a full live imsg gateway reproduction in this read-only review. Is this the best way to solve the issue? Yes for the long-lived RPC client: routing process, stdin, stdout, and stderr errors through one terminal finish path is the narrow owner-boundary fix. The remaining gap is proof quality, not an alternate implementation path. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 63d71f4ab143. Label changesLabel justifications:
Evidence reviewedPR surface: Source +17, Tests +115. Total +132 across 2 files. View PR surface stats
What 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
Review history (1 earlier review cycle)
|
Contributor
|
Land-ready proof for exact head
Known proof gap: the real process/stream lifecycle is covered, but I did not deliberately fault a live Messages transport session. |
Contributor
|
Merged via squash.
|
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
Jul 7, 2026
…hild (openclaw#101084) * fix(imessage): handle stdout/stderr stream errors in the RPC client child A dead imsg RPC helper can emit an async error on any of its stdio streams. On a raw stream an unhandled 'error' event throws and surfaces as an uncaughtException, crashing the gateway. openclaw#75438 added this guard for stdin but left stdout/stderr — on the same long-lived child — unguarded. Route stdout/stderr stream errors through the existing failAll path via a shared failFromStreamError helper, mirroring the stdin handler. Add a regression test that emits 'error' on each stream and asserts the child does not throw and in-flight requests reject cleanly. * fix(imessage): terminate failed RPC transports * test(imessage): exercise real stream failure --------- Co-authored-by: Peter Steinberger <steipete@gmail.com>
giodl73-repo
pushed a commit
to giodl73-repo/openclaw
that referenced
this pull request
Jul 8, 2026
…hild (openclaw#101084) * fix(imessage): handle stdout/stderr stream errors in the RPC client child A dead imsg RPC helper can emit an async error on any of its stdio streams. On a raw stream an unhandled 'error' event throws and surfaces as an uncaughtException, crashing the gateway. openclaw#75438 added this guard for stdin but left stdout/stderr — on the same long-lived child — unguarded. Route stdout/stderr stream errors through the existing failAll path via a shared failFromStreamError helper, mirroring the stdin handler. Add a regression test that emits 'error' on each stream and asserts the child does not throw and in-flight requests reject cleanly. * fix(imessage): terminate failed RPC transports * test(imessage): exercise real stream failure --------- Co-authored-by: Peter Steinberger <steipete@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Problem This Solves
The long-lived
imsgmonitor client handled errors from the child process itself, but not from its stdin/stdout/stderr streams. Node treats an unhandlederrorevent as fatal, and a terminal stream failure could also leave pending RPC requests andwaitForClose()unresolved.Why This Change Was Made
Route child-process and stdio errors through one idempotent terminal path. The path rejects every pending request, resolves the close promise exactly once, and terminates the unusable child with
SIGTERM. This preserves the monitor provider's existing cleanup contract: it awaitswaitForClose()before stopping the active client.The original contributor patch identified the missing listeners. The maintainer refactor makes those listeners participate in the client's lifecycle instead of only suppressing an uncaught event.
User Impact
An
imsgmonitor whose child pipe fails now shuts down predictably instead of crashing the host process or hanging its monitor loop indefinitely.Evidence
waitForClose()resolves, and the child is terminated.errorevents throw and terminate the process; stream failures emiterrorbeforeclose: https://nodejs.org/api/events.html#error-events and https://nodejs.org/api/stream.html#event-errorKnown gap: this uses the real Node process/stream lifecycle but does not deliberately fault a live Messages transport session.
Thanks @masatohoshino for finding and fixing this failure mode.