Skip to content

fix(matrix): handle stdout/stderr stream errors in dependency commands#101597

Merged
vincentkoc merged 4 commits into
openclaw:mainfrom
Alix-007:alix/matrix-deps-stream-errors
Jul 8, 2026
Merged

fix(matrix): handle stdout/stderr stream errors in dependency commands#101597
vincentkoc merged 4 commits into
openclaw:mainfrom
Alix-007:alix/matrix-deps-stream-errors

Conversation

@Alix-007

@Alix-007 Alix-007 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add stdout/stderr error handlers to Matrix dependency bootstrap commands
  • align the stream-error path with the established src/node-host/invoke.ts lifecycle pattern: record the first stream error, send one SIGTERM, keep the process-exit cleanup registered, escalate to SIGKILL if the child does not exit, and settle only from the child close event
  • remove the production spawn override that an earlier revision of this branch added for testing; the runtime command path is fixed and takes no injection seam
  • cover the stream-error lifecycle with a real spawned node child over real stdio pipes in a focused regression test
  • collision check: no open PR found for the Matrix dependency command stream-error issue or extensions/matrix/src/matrix/deps.ts; the only fuzzy search hit was an unrelated Telegram PR

Verification

  • node scripts/run-vitest.mjs extensions/matrix/src/matrix/deps.test.ts — passed; 1 file, 10 tests passed
  • ./node_modules/.bin/oxfmt --check --threads=1 extensions/matrix/src/matrix/deps.ts extensions/matrix/src/matrix/deps.test.ts — all files correctly formatted
  • ./node_modules/.bin/oxlint --tsconfig config/tsconfig/oxlint.extensions.json extensions/matrix/src/matrix/deps.ts extensions/matrix/src/matrix/deps.test.ts — no findings
  • mutation check: changing the stream-error guard to allow duplicate stream errors made the focused test fail with expected "kill" to be called 1 times, but got 2 times; restoring the guard made the same test pass

Real behavior proof

Behavior addressed: Matrix dependency command stdout/stderr stream errors no longer settle the command result before child close, no longer remove the process-exit cleanup early, and duplicate stream errors cannot trigger teardown twice.
Real environment tested: local Linux Node 22.22 source checkout; the regression test drives the real exported runFixedCommandWithTimeout helper with a real spawned node child process and real pipe-backed stdout/stderr.
Exact steps or command run after this patch: the test destroys the parent-side stdout/stderr Readable with an error while the real child is alive, then observes SIGTERM, SIGKILL escalation, and settlement from the child close event; command: node scripts/run-vitest.mjs extensions/matrix/src/matrix/deps.test.ts.
Evidence after fix: the real-child regression asserts no settlement before close, exactly one SIGTERM for duplicate stream errors, retained process-exit listener during shutdown, SIGKILL escalation, child close { code: null, signal: "SIGKILL" }, and exit-listener cleanup after settlement.
Observed result after fix: Test Files 1 passed (1); Tests 10 passed (10).
What was not tested: a live Matrix bootstrap download. The test injects the Readable error on the parent side because a real download-lib.js child cannot deterministically produce a parent read-side error: normal child exit, fd close, and SIGKILL surface as EOF/close on the parent read side, and EPIPE is a write-side failure.

@openclaw-barnacle openclaw-barnacle Bot added channel: matrix Channel integration: matrix size: S labels Jul 7, 2026
@clawsweeper

clawsweeper Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Codex review: stale review; fresh review needed.

Summary
The latest durable ClawSweeper review was for head 6ae4180eea824a45da52cfb7bf6c5c363666d6c3, but the PR head is now f9903bfa58bb0ee1c4e8a0baa8b36bfd58ee825b. Its old verdict and PR readiness labels are no longer current.

Next step
Run or wait for a fresh ClawSweeper review on the current PR head.

Review history (5 earlier review cycles)
  • reviewed 2026-07-07T11:49:19.959Z sha 2471231 :: needs real behavior proof before merge. :: [P2] Give the mock process a typed kill implementation
  • reviewed 2026-07-07T16:53:22.438Z sha 0155a1e :: needs real behavior proof before merge. :: [P1] Keep spawn injection out of bootstrap runtime
  • reviewed 2026-07-08T00:25:22.864Z sha b4f31b4 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-08T00:33:48.183Z sha b4f31b4 :: needs changes before merge. :: [P2] Use a concrete spy type | [P2] Pass defined spawn options to actual.spawn
  • reviewed 2026-07-08T04:15:00.194Z sha 8c20c42 :: needs maintainer review before merge. :: none

@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 backlog priority with limited blast radius. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. proof: sufficient ClawSweeper judged the real behavior proof convincing. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. 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. labels Jul 7, 2026
@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: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jul 8, 2026
@vincentkoc vincentkoc self-assigned this Jul 8, 2026
@vincentkoc
vincentkoc requested review from a team as code owners July 8, 2026 04:36
@vincentkoc
vincentkoc force-pushed the alix/matrix-deps-stream-errors branch from 0ef9cd8 to 6ae4180 Compare July 8, 2026 04:36
@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation channel: discord Channel integration: discord channel: imessage Channel integration: imessage channel: line Channel integration: line channel: msteams Channel integration: msteams channel: nextcloud-talk Channel integration: nextcloud-talk labels Jul 8, 2026
@vincentkoc

Copy link
Copy Markdown
Member

Land-ready maintainer pass complete on final head f9903bfa58bb0ee1c4e8a0baa8b36bfd58ee825b.

  • Source review: the Matrix dependency command helper owns the child and stdio lifecycle. This patch records only the first stream error, sends one SIGTERM, escalates to SIGKILL after a bounded grace period, retains process-exit cleanup, and settles only from child close. It matches the established node-host lifecycle pattern and keeps test injection out of production.
  • Sanitized AWS Crabbox lease cbx_a369782859a3, Linux Node 24.15.0:
    • exact-final-head run run_a6768dcfbd19: 10/10 Matrix dependency tests passed, followed by clean targeted oxfmt and extension oxlint.
    • patch-identical stress run run_f785e18fa042: the same 10-test suite passed three consecutive times.
  • Fresh final-head gpt-5.5 high-thinking AutoReview: no accepted or actionable findings.
  • Hosted CI run 28917845974 on the patch-identical immediately preceding rebase: 63 passing checks, 35 expected skips, zero failures. Native scripts/pr prepare-run 101597 accepted the recent-rebase evidence and verified that the remote and reviewed trees match.
  • Native review artifact validation and prepare gates passed.

Known proof boundary: the regression uses a real spawned child and real pipes but injects the parent-side readable error, because a live Matrix dependency download cannot deterministically produce that failure.

@vincentkoc

Copy link
Copy Markdown
Member

Merged via squash.

@Alix-007

Alix-007 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @vincentkoc, really appreciate the detailed maintainer pass here.

The lifecycle notes around first stream error handling, bounded termination, and settling only from child close are very helpful. I’ll use that node-host pattern for similar stdio/child-process fixes going forward.

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

Labels

channel: matrix Channel integration: matrix P2 Normal backlog priority with limited blast radius. size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants