Skip to content

fix(hooks): clean up abort listener in error handler#2841

Merged
DennisYu07 merged 1 commit intoQwenLM:mainfrom
chinesepowered:hook-runner-error
Apr 3, 2026
Merged

fix(hooks): clean up abort listener in error handler#2841
DennisYu07 merged 1 commit intoQwenLM:mainfrom
chinesepowered:hook-runner-error

Conversation

@chinesepowered
Copy link
Copy Markdown
Contributor

Fix abort listener leak in hook runner error handler.

TLDR

Adds missing signal.removeEventListener('abort', abortHandler) cleanup to the error event handler in hookRunner.ts. Without this, when a child process emits an error event (e.g., executable not found, spawn failure), the abort listener remains registered on the signal, leaking memory if the same signal is reused across multiple hook executions.

Screenshots / Video Demo

N/A — no user-facing UI change. The fix ensures proper cleanup of event listeners on process error.

Dive Deeper

The close handler (line 358-364) correctly cleans up both the timeout and the abort listener:

child.on('close', (exitCode) => {
  clearTimeout(timeoutHandle);
  if (signal) {
    signal.removeEventListener('abort', abortHandler);
  }
  // ...
});

But the error handler (line 440-453) only cleared the timeout:

child.on('error', (error) => {
  clearTimeout(timeoutHandle);
  // abort listener NOT removed
  // ...
});

Per Node.js docs, when spawn fails (e.g., executable not found), the error event fires but close is NOT guaranteed. So the abort listener remains registered on the signal. If the same AbortSignal is used for multiple hook executions, stale handlers accumulate.

Modified file:

  • packages/core/src/hooks/hookRunner.ts — Added abort listener cleanup to error handler (4 insertions)

Reviewer Test Plan

  1. Trigger a hook with an invalid executable path — verify no listener leak
  2. Run hook tests: npx vitest run src/hooks/ (all 274 pass)
  3. Run type check: tsc --noEmit (clean)

Testing Matrix

macOS Windows Linux
npm run ? pass ?
npx ? ? ?
Docker ? ? ?
Podman ? - -
Seatbelt ? - -

The error handler in hookRunner cleared the timeout but did not remove
the abort signal listener, unlike the close handler. When spawn fails
(e.g. executable not found), only the error event fires — the close
event is not guaranteed — so the abort listener leaked on the signal.
Copy link
Copy Markdown
Collaborator

@DennisYu07 DennisYu07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@DennisYu07 DennisYu07 merged commit 76e52a5 into QwenLM:main Apr 3, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants