Skip to content

fix(infra): add error listener to detached spawned gateway process to prevent crashes#101482

Closed
krissding wants to merge 2 commits into
openclaw:mainfrom
krissding:fix/detached-spawn-error-listener
Closed

fix(infra): add error listener to detached spawned gateway process to prevent crashes#101482
krissding wants to merge 2 commits into
openclaw:mainfrom
krissding:fix/detached-spawn-error-listener

Conversation

@krissding

@krissding krissding commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

spawnDetachedGatewayProcess() in src/infra/process-respawn.ts calls child_process.spawn() with detached: true but doesn't attach an error listener before calling child.unref(). When spawn() fails asynchronously (e.g., ENOENT, ENOMEM), the ChildProcess emits an unhandled error event that crashes the parent Node.js process.

Fixes #101458

Why This Change Was Made

The fix adds a child.on("error", () => {}) handler between spawn() and unref() — the same pattern used in src/process/exec.ts and packages/agent-core/src/harness/env/kill-tree.ts for async spawn error handling. This prevents the crashing unhandled 'error' event while preserving all existing behavior.

Evidence

Before Fix — Unhandled crash

spawn('/nonexistent/binary', [], {detached: true, stdio: 'ignore'}) crashes the process:

node:events:486
      throw er; // Unhandled 'error' event
      ^

Error: spawn /nonexistent ENOENT
    at ChildProcess._handle.onexit (node:internal/child_process:286:19)
Emitted 'error' event on ChildProcess instance at:
    at ChildProcess._handle.onexit (node:internal/child_process:292:12)

Exit code: 1 (CRASH)

After Fix — Graceful handling

With child.on('error', () => {}) added, the same spawn failure is caught:

Caught async error: ENOENT — process does NOT crash
Process still alive after 500ms — no crash
Exit code: 0 (SAFE)

Regression Test

Added test "attaches an error listener to the detached child before unref to prevent crashes" in src/infra/process-respawn.test.ts — verifies that .on("error", ...) is called on the spawned child before returning from respawnGatewayProcessForUpdate().

Production Source

File: src/infra/process-respawn.ts

function spawnDetachedGatewayProcess(opts: GatewayRespawnOptions = {}): {
  child: ChildProcess;
  pid?: number;
} {
  const [entryArg, ...entryArgs] = process.argv.slice(1);
  const args = [
    ...process.execArgv,
    ...(entryArg ? [rewritePnpmVersionedOpenClawEntryPath(entryArg)] : []),
    ...entryArgs,
  ];
  const child = spawn(process.execPath, args, {
    env: opts.env ? { ...process.env, ...opts.env } : process.env,
    detached: true,
    stdio: "inherit",
  });
  child.on("error", () => {});  // ← prevents crash on async spawn failure
  child.unref();
  return { child, pid: child.pid ?? undefined };
}

Merge Risk

Low — single line change in process cleanup path. The empty error handler matches the established pattern in kill-tree.ts:runTaskkill() which already uses child.once("error", () => {}) for detached spawn error swallowing.

🤖 Generated with Claude Code

@krissding @claude
fix(infa): add error listener to detached spawned gateway process to …
96404cb 
…prevent crashes

When spawn() fails asynchronously (e.g., ENOENT, ENOMEM), the ChildProcess
emits an 'error' event. Without a listener attached, Node.js treats this as
an unhandled exception and crashes the parent process.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
@clawsweeper

clawsweeper Bot commented Jul 7, 2026 •
edited
Loading

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 superseded: this PR is a focused and likely correct fix, but the maintainer-labeled PR at #101489 owns the same production change with stronger listener-before-unref regression coverage, so keeping both open creates duplicate landing work.

Canonical path: Use #101489 as the canonical landing path for #101458 and close parallel duplicate PR branches.

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

Review details

Best possible solution:

Use #101489 as the canonical landing path for #101458 and close parallel duplicate PR branches.

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

Yes, source-level reproduction is high confidence: current main lacks the ChildProcess error listener, and a Node v24.18.0 probe shows a missing detached executable exits with an unhandled error unless the listener is attached. I did not run a full OpenClaw gateway update respawn flow.

Is this the best way to solve the issue?

The one-line listener before unref is the right fix shape, matching sibling restart-helper behavior. This PR is no longer the best landing vehicle because the maintainer-labeled canonical PR carries the same production fix with stronger ordering coverage.

Security review:

Security review cleared: The diff only adds a ChildProcess error listener and test mocks; it does not introduce new commands, permissions, dependency sources, secrets handling, or supply-chain changes.

AGENTS.md: found and applied where relevant.

What I checked:

  • Repository policy read: Root AGENTS.md was read fully; no scoped AGENTS.md owns src/infra, so the root high-confidence PR review, duplicate cleanup, and process/security review guidance applies. (AGENTS.md:1, 6799b5b3d444)
  • Current main still has the bug shape: Current main spawns the detached gateway child and immediately calls child.unref() without a ChildProcess error listener. (src/infra/process-respawn.ts:51, 6799b5b3d444)
  • Current PR patch: This PR adds child.on("error", () => {}) before child.unref() and adds a regression test that checks the listener is registered. (src/infra/process-respawn.ts:53, e53738ec8771)
  • Node child_process contract probe: Node v24.18.0 exits with an unhandled ENOENT ChildProcess error when a detached missing executable has no error listener, and stays alive when the listener is attached.
  • Sibling implementation precedent: runRestartScript already attaches child.on("error", () => {}) before child.unref() for detached restart-script handoff. (src/cli/update-cli/restart-helper.ts:411, 6799b5b3d444)
  • Canonical maintainer PR: fix(infra): handle detached respawn child errors #101489 is open with the maintainer label, closes the same issue, adds the same production listener, and includes stronger test coverage asserting the listener call happens before unref. (src/infra/process-respawn.test.ts:395, 2d9b586f603c)

Likely related people:

  • shbernal: PR history for fix: include pnpm 11 bins in gateway PATH #85238 and blame on src/infra/process-respawn.ts tie the current detached respawn helper to Santiago's gateway PATH work. (role: introduced current respawn path; confidence: high; commits: 80bb0cd24ed1; files: src/infra/process-respawn.ts, src/infra/process-respawn.test.ts, src/cli/gateway-cli/run-loop.ts)
  • vincentkoc: GitHub PR metadata shows vincentkoc merged fix: include pnpm 11 bins in gateway PATH #85238 and contributed follow-up commits in the same gateway PATH branch. (role: merger and adjacent contributor; confidence: medium; commits: 80bb0cd24ed1, 16cfff03540a; files: src/infra/process-respawn.ts, src/cli/gateway-cli/run-loop.ts)
  • giodl73-repo: Merged PR fix(update): ignore restart script spawn failures #85761 fixed the same detached spawn error-listener pattern in src/cli/update-cli/restart-helper.ts. (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: The open maintainer-labeled PR at fix(infra): handle detached respawn child errors #101489 owns the same remaining fix with stronger regression coverage. (role: canonical PR owner; confidence: high; commits: 2d9b586f603c; files: src/infra/process-respawn.ts, src/infra/process-respawn.test.ts)

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

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. 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. P2 Normal backlog priority with limited blast radius. labels Jul 7, 2026
@clawsweeper clawsweeper Bot closed this Jul 7, 2026
@clawsweeper

clawsweeper Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

🦞✅
ClawSweeper autoclose is complete.

Reason: structured ClawSweeper close marker: close-required (sha=e53738ec8771767a81b16499eaf1f7e16bc872e2)

Closed:

This was referenced Jul 7, 2026
Closed
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. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: XS status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

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

1 participant

@krissding