Skip to content

fix(infra): attach error listener to detached spawn to prevent unhandled rejection crash#101487

Closed
tzy-17 wants to merge 1 commit into
openclaw:mainfrom
tzy-17:fix/101458-spawn-error-listener
Closed

fix(infra): attach error listener to detached spawn to prevent unhandled rejection crash#101487
tzy-17 wants to merge 1 commit into
openclaw:mainfrom
tzy-17:fix/101458-spawn-error-listener

Conversation

@tzy-17

@tzy-17 tzy-17 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes #101458: spawnDetachedGatewayProcess calls child.unref() without attaching an error listener. If spawn fails asynchronously (ENOMEM, missing executable), Node.js treats the unhandled error event as an uncaught exception and crashes the parent process.

Why This Change Was Made

Added a guard-checked noop error listener (child.on("error", () => {})) before child.unref(). The typeof child.on === "function" guard handles test mocks that return plain objects without EventEmitter methods.

User Impact

  • Detached gateway spawn failures no longer crash the parent Node process
  • The spawn failure is silently ignored (detached process is best-effort)

Evidence

  • pnpm test src/infra/process-respawn.test.ts — 25 tests pass
  • oxlint passes

🤖 Generated with Claude Code

@clawsweeper

clawsweeper Bot commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 7, 2026, 4:36 AM ET / 08:36 UTC.

Summary
The PR adds a guarded no-op ChildProcess error listener before unref() in spawnDetachedGatewayProcess.

PR surface: Source +6. Total +6 across 1 file.

Reproducibility: yes. Current main lacks a ChildProcess error listener in the detached update respawn path, and a direct Node probe shows a missing executable can terminate the parent unless a listener is attached.

Review metrics: 1 noteworthy metric.

  • Regression coverage: 0 tests added in this PR. This is a crash-path bug fix, and the submitted branch does not add a test for the new child error-listener invariant.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #101458
Summary: This PR and several sibling PRs are candidate fixes for the same open detached-spawn ChildProcess error issue.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦐 gold shrimp
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add redacted terminal output, logs, or a short recording showing the parent process survives the async ChildProcess error after the fix.
  • [P1] Add a focused src/infra/process-respawn.test.ts regression for listener registration before unref().
  • [P1] Coordinate with the linked issue and competing same-root PRs so maintainers land one canonical fix.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body lists unit/lint evidence only; before merge, add redacted terminal/live output showing an async child-process spawn failure no longer terminates the parent, then update the PR body to trigger re-review.

Risk before merge

  • [P1] The branch only lists unit/lint evidence and does not provide a redacted live or terminal proof that the parent survives an async ChildProcess error after the fix.
  • [P1] The production change is not backed by a focused regression test in this branch, so the exact listener-before-unref invariant could regress without failing CI.
  • [P1] Several same-issue PRs are open; maintainers should avoid landing multiple competing fixes for the same spawnDetachedGatewayProcess behavior.

Maintainer options:

  1. Decide the mitigation before merge
    Land one canonical fix that attaches the listener before unref(), adds focused async-child-error regression coverage, and includes redacted real-behavior proof for the parent-survival path.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] The PR needs contributor proof and a test update, and maintainers should choose one canonical fix among the same-root open PRs rather than dispatching an automated repair now.

Security
Cleared: The diff only adds a no-op listener to an existing child process and does not introduce new commands, dependencies, permissions, or secret handling.

Review findings

  • [P2] Add coverage for the async child error listener — src/infra/process-respawn.ts:59-60
Review details

Best possible solution:

Land one canonical fix that attaches the listener before unref(), adds focused async-child-error regression coverage, and includes redacted real-behavior proof for the parent-survival path.

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

Yes. Current main lacks a ChildProcess error listener in the detached update respawn path, and a direct Node probe shows a missing executable can terminate the parent unless a listener is attached.

Is this the best way to solve the issue?

Not as submitted. The production fix is the right narrow layer, but the branch should add focused regression coverage and real-behavior proof before it is the best merge candidate.

Full review comments:

  • [P2] Add coverage for the async child error listener — src/infra/process-respawn.ts:59-60
    This branch adds the production listener but keeps the existing process-respawn tests unchanged by guarding for mocks without .on(). That means CI would still pass if this listener were removed, so please add a focused test that returns a child with on/unref and asserts the error listener is registered before unref().
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add P2: This is a normal-priority process availability bug fix with a narrow blast radius and no evidence of widespread active outages.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists unit/lint evidence only; before merge, add redacted terminal/live output showing an async child-process spawn failure no longer terminates the parent, then update the PR body to trigger re-review.

Label justifications:

  • P2: This is a normal-priority process availability bug fix with a narrow blast radius and no evidence of widespread active outages.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists unit/lint evidence only; before merge, add redacted terminal/live output showing an async child-process spawn failure no longer terminates the parent, then update the PR body to trigger re-review.
Evidence reviewed

PR surface:

Source +6. Total +6 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 1 6 0 +6
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 6 0 +6

What I checked:

  • Current main still has the reported crash shape: On current main, spawnDetachedGatewayProcess calls spawn(...), then immediately child.unref() and returns the child without attaching an error listener. (src/infra/process-respawn.ts:51, cc2c6a87dc35)
  • PR branch adds the listener: The PR branch adds a no-op error listener before child.unref(), which matches the necessary ordering for async spawn errors. (src/infra/process-respawn.ts:56, a723a2183894)
  • PR branch does not add regression coverage: The PR branch's process-respawn.test.ts is unchanged around the update-respawn tests, so it does not assert that a child error listener is registered or registered before unref(). (src/infra/process-respawn.test.ts:284, a723a2183894)
  • Caller fallback path depends on safe child handling: The gateway run-loop treats update respawn as spawned, waits for the child to become healthy, and falls back to in-process restart if it does not, so preventing parent crash is important before that fallback can run. (src/cli/gateway-cli/run-loop.ts:210, cc2c6a87dc35)
  • Sibling implementation already uses this hardening pattern: runRestartScript attaches child.on("error", () => {}) before child.unref() for another detached restart helper. (src/cli/update-cli/restart-helper.ts:411, cc2c6a87dc35)
  • Node behavior probe: A direct Node v24.18.0 probe with a missing executable exited with an unhandled ChildProcess error when no listener was attached, and the same probe printed survived with a no-op listener.

Likely related people:

  • IAMSamuelRodda: Authored the merged update-restart work that made respawnGatewayProcessForUpdate a central gateway update path and touched the same tests. (role: feature owner; confidence: medium; commits: 6c252cc54c3c; files: src/infra/process-respawn.ts, src/infra/process-respawn.test.ts, src/cli/gateway-cli/run-loop.test.ts)
  • ZOOWH: Recently changed the managed update handoff and process respawn tests around the same update restart lifecycle. (role: recent area contributor; confidence: medium; commits: b8b946e43db8; files: src/infra/process-respawn.ts, src/infra/process-respawn.test.ts, src/cli/gateway-cli/run-loop.ts)
  • liuhao1024: Recently changed spawnDetachedGatewayProcess directly for pnpm versioned entry rewriting, so they are tied to the current helper shape. (role: recent area contributor; confidence: medium; commits: 0278b59d0ea3; files: src/infra/process-respawn.ts, src/infra/process-respawn.test.ts)
  • giodl73-repo: Authored the merged sibling fix that added the same no-op error-listener pattern to runRestartScript. (role: adjacent sibling-fix contributor; confidence: medium; commits: 566d8cdf3995; files: src/cli/update-cli/restart-helper.ts, src/cli/update-cli/restart-helper.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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

@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. labels Jul 7, 2026
@tzy-17
tzy-17 force-pushed the fix/101458-spawn-error-listener branch from a723a21 to 7eed5df Compare July 7, 2026 08:39
@clawsweeper clawsweeper Bot mentioned this pull request Jul 7, 2026
Closed
@tzy-17
fix(infra): attach error listener to detached spawn to prevent unhand…
d16b965 
…led rejection crash

spawnDetachedGatewayProcess calls child.unref() without attaching
an error listener. If spawn fails asynchronously (ENOMEM, missing
executable), the ChildProcess emits an unhandled 'error' event
that crashes the parent Node process.

Add a guard-checked noop error listener before unref() to prevent
the unhandled-event crash.

Fixes openclaw#101458
@tzy-17
tzy-17 force-pushed the fix/101458-spawn-error-listener branch from 7eed5df to d16b965 Compare July 7, 2026 08:48
@vincentkoc

vincentkoc commented Jul 7, 2026

Copy link
Copy Markdown
Member

Closing as a duplicate of PR #101489 for issue #101458. The defensive typeof guard is unnecessary for a real ChildProcess and this branch has no regression coverage; #101489 is the focused tested implementation.

@vincentkoc vincentkoc closed this Jul 7, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 7, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unhandled error on detached spawned gateway process crashes the Node process

2 participants

@tzy-17 @vincentkoc