child_process: release the abort listener when a spawn fails by hktitof · Pull Request #66189 · nodejs/node · GitHub
Skip to content

child_process: release the abort listener when a spawn fails - #66189

Open
hktitof wants to merge 1 commit into
nodejs:mainfrom
hktitof:fix/spawn-signal-listener-dispose
Open

hktitof wants to merge 1 commit into
nodejs:mainfrom
hktitof:fix/spawn-signal-listener-dispose

Conversation

@hktitof

@hktitof hktitof commented Sep 21, 2026 •

Copy link
Copy Markdown

Context: sibling to #65506 (which moved the spawn timeout cleanup from 'exit' to 'close' so it also fires on spawn-time failures). This applies the same rule to the AbortSignal cleanup a few lines below in the same spawn() function

Description

A failed spawn emits 'error' and 'close' but never 'exit'. The timeout cleanup got fixed by #65506 for exactly this, but the AbortSignal dispose still sits only on 'exit', so after a failed spawn the abort listener stays attached to the user's signal

// lib/child_process.js, spawn()
const disposable = addAbortListener(signal, onAbortListener);
child.once('exit', disposable[SymbolDispose]); // never fires when the spawn itself failed

probed on a pristine main build (67a4416): after the 'error' of a failed spawn, the signal still carries the abort listener, and 'exit' never fires while 'close' does

this keeps the dispose on 'exit' (so the timing the existing test in test-child-process-spawn-timeout-kill-signal.js pins is untouched) and adds a second dispose on 'close', which always fires. Removing an already removed listener is a no-op, so the success path behaves exactly as before

Validation

the test joins test-child-process-spawn-timeout-clear-on-error.js, the file #65506 added

raw output on a pristine main build (the new assertion fails bcs the listener is still attached):

> out/Release/node test/parallel/test-child-process-spawn-timeout-clear-on-error.js
AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

1 !== 0

    at ChildProcess.<anonymous> (test/parallel/test-child-process-spawn-timeout-clear-on-error.js:34:12)
    at ChildProcess.emit (node:events:514:20)
    ... (truncated, full assertion block in the run log)

Node.js v27.0.0-pre

with the fix (exit code 0, no output), plus the spawn and abort signal files around the change:

test-child-process-spawn-timeout-clear-on-error.js: exit=0
test-child-process-spawn-timeout-kill-signal.js: exit=0
test-child-process-spawn-error.js: exit=0
test-child-process-fork-abort-signal.js: exit=0
test-child-process-exec-abortcontroller-promisified.js: exit=0
test-child-process-execFile-promisified-abortController.js: exit=0
test-child-process-spawnsync-kill-signal.js: exit=0
test-child-process-spawn-controller.js: exit=0

Note: Just to let you knmow that I have used AI assistance to help check the sibling pattern from #65506, but tested and verified all changes locally myself on main build 67a4416

@nodejs-github-bot nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run. labels Sep 21, 2026
@github-actions

Copy link
Copy Markdown
Contributor

@hktitof

hktitof commented Sep 22, 2026

Copy link
Copy Markdown
Author

Hi, saw the agentscan note, just wanted to clarify i tested this locally against main build 67a4416 after seeing the timeout cleanup in #65506. happy to adjust or answer any questions on the abort listener behavior!

@hktitof
hktitof force-pushed the fix/spawn-signal-listener-dispose branch 3 times, most recently from 292b75f to a155af9 Compare September 22, 2026 15:37
@codecov

codecov Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.39%. Comparing base (cede7e6) to head (24374f1).
⚠️ Report is 32 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #66189   +/-   ##
=======================================
  Coverage   90.39%   90.39%           
=======================================
  Files         792      792           
  Lines      275697   275700    +3     
  Branches    52868    52858   -10     
=======================================
+ Hits       249208   249228   +20     
+ Misses      16892    16888    -4     
+ Partials     9597     9584   -13     
Files with missing lines Coverage Δ
lib/child_process.js 93.02% <100.00%> (+0.01%) ⬆️

... and 23 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hktitof
hktitof force-pushed the fix/spawn-signal-listener-dispose branch from a155af9 to 24374f1 Compare October 1, 2026 09:22
A failed spawn emits 'error' and 'close' but never 'exit'. The
AbortSignal cleanup in spawn() was registered on 'exit', so the abort
listener stayed attached to the user's signal after a failed spawn.

Register the cleanup on 'close' as well, matching the timeout cleanup.

Signed-off-by: Abdel <hktitof@gmail.com>
@hktitof
hktitof force-pushed the fix/spawn-signal-listener-dispose branch from 24374f1 to 4376701 Compare October 8, 2026 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agentscan:mixed-signals child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants