Skip to content

fix(infra): register no-op error handler on detached gateway spawn child#101491

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

fix(infra): register no-op error handler on detached gateway spawn child#101491
chenxiaoyu209 wants to merge 1 commit into
openclaw:mainfrom
chenxiaoyu209:fix/gateway-spawn-error-listener-101458

Conversation

@chenxiaoyu209

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes #101458 — In spawnDetachedGatewayProcess, the child_process.spawn() call creates a detached child process and immediately calls child.unref() without registering an error listener. If spawn fails asynchronously (e.g., ENOMEM, missing executable), the ChildProcess emits an unhandled error event that crashes the parent Node.js process.

Why This Change Was Made

The detached spawn error listener was missing, creating a crash path on async spawn failures during gateway update restarts. The sibling restart-helper.ts already uses the same pattern: child.on("error", () => {}) before child.unref(). This fix mirrors that established pattern.

User Impact

Prevents parent process crash when the detached gateway spawn child encounters an async error (ENOMEM, missing binary, etc.). The error is silently swallowed since the child is detached and the parent cannot recover — matching the best-effort handoff semantics of the existing restart-helper.

Evidence

  • Sibling precedent: src/cli/update-cli/restart-helper.ts:417 — uses exact same child.on("error", () => {}) + child.unref() pattern
  • Node.js contract: Confirmed via direct Node v24 probe — missing executable + no error listener → process exit 1 with unhandled error; with error listener → exit 0
  • Tests: 26/26 passing, including new regression test verifying the error listener is registered and is a no-op
  • ClawSweeper: Reviewed and confirmed clawsweeper:fix-shape-clear, clawsweeper:source-repro, clawsweeper:queueable-fix

🤖 Generated with Claude Code

@clawsweeper

clawsweeper Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Close as a duplicate landing candidate: the same production fix is already carried by an open maintainer-labeled PR with slightly stronger regression coverage, and this branch does not add a meaningful unique runtime change.

Root-cause cluster
Relationship: duplicate
Canonical: #101489
Summary: This PR duplicates the open maintainer-labeled fix PR for the same linked gateway respawn crash report.

Members:

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

Canonical path: #101489 is open, mergeable, maintainer-labeled, and carries the same production line while using a shared mock child helper and asserting the listener is registered before unref.

So I’m closing this here and keeping the remaining discussion on #101489 and #101458.

Review details

Best possible solution:

Land one canonical maintainer-owned fix for the linked gateway respawn crash and close duplicate fix PRs that do not add unique runtime behavior.

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

Yes. Current main lacks the child error listener in the detached gateway respawn path, and a direct Node v24.18.0 child_process probe shows the parent exits on an unhandled child error without that listener.

Is this the best way to solve the issue?

No, not as a separate landing branch. The code change is correct, but the best path is the open maintainer-labeled canonical PR that carries the same runtime fix with stronger ordering coverage.

Security review:

Security review cleared: The diff only adds a no-op ChildProcess error listener and unit-test updates; it does not broaden secrets, dependency, workflow, or package execution surfaces.

AGENTS.md: found and applied where relevant.

What I checked:

  • Current main still has the underlying bug: On current main, spawnDetachedGatewayProcess calls spawn with detached stdio and immediately calls child.unref() without a child error listener, so the linked crash report is a valid bug but not yet fixed on main. (src/infra/process-respawn.ts:51, 24330a82b44a)
  • This PR applies the expected runtime mitigation: The PR head adds child.on("error", () => {}) before child.unref() and adds a focused test that checks listener registration and no-throw handler behavior. (src/infra/process-respawn.ts:56, 26a20695902e)
  • Canonical maintainer-labeled PR covers the same fix: fix(infra): handle detached respawn child errors #101489 is open, mergeable, maintainer-labeled, and carries the same production line while using a shared mock child helper and asserting the listener is registered before unref. (src/infra/process-respawn.ts:56, 2d9b586f603c)
  • Linked issue and duplicate search: The PR closes the same open crash report at Unhandled error on detached spawned gateway process crashes the Node process #101458, and GitHub search found several same-surface fix PRs for that issue, making one canonical landing path preferable.
  • Node child_process contract probe: A Node v24.18.0 probe with a missing executable exited 1 with an unhandled ChildProcess error when no listener was attached, and exited 0 when child.on('error', () => {}) was attached.
  • Feature history: git blame ties the current respawn function on main to commit 7b366e1 from the Windows CLI backend work, which is the relevant current-main provenance for routing. (src/infra/process-respawn.ts:41, 7b366e16b0b3)

Likely related people:

  • steipete: git blame and live PR metadata tie the current process-respawn implementation to the merged Windows CLI backend work. (role: introduced current respawn path; confidence: high; commits: 7b366e16b0b3; files: src/infra/process-respawn.ts, src/infra/process-respawn.test.ts, src/cli/gateway-cli/lifecycle.runtime.ts)
  • giodl73-repo: Authored the merged restart-helper fix that used the same detached child error-listener pattern for update restart script handoff. (role: adjacent sibling-fix contributor; confidence: medium; commits: e224c84c348c; files: src/cli/update-cli/restart-helper.ts, src/cli/update-cli/restart-helper.test.ts)
  • momothemage: Authored the open maintainer-labeled PR carrying the same production fix and stronger process-respawn regression coverage. (role: canonical follow-up PR owner; confidence: medium; commits: 2d9b586f603c; files: src/infra/process-respawn.ts, src/infra/process-respawn.test.ts)

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

@clawsweeper clawsweeper Bot added 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. P2 Normal backlog priority with limited blast radius. labels Jul 7, 2026
@vincentkoc

Copy link
Copy Markdown
Member

Closing as a duplicate of PR #101489 for issue #101458. This variant only adds a no-op listener; #101489 uses the actual ChildProcess error contract and includes focused ordering/error coverage, so it is the canonical implementation.

@vincentkoc vincentkoc closed this Jul 7, 2026
@chenxiaoyu209
chenxiaoyu209 deleted the fix/gateway-spawn-error-listener-101458 branch July 8, 2026 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unhandled error on detached spawned gateway process crashes the Node process

2 participants