Skip to content

lib: fix AbortSignal.any() nested composite leak - #66617

Open
DariusHQ wants to merge 1 commit into
nodejs:mainfrom
DariusHQ:abortsignal-any-nested-leak
Open

DariusHQ wants to merge 1 commit into
nodejs:mainfrom
DariusHQ:abortsignal-any-nested-leak

Conversation

@DariusHQ

@DariusHQ DariusHQ commented Oct 8, 2026

Copy link
Copy Markdown

Fixes: #66612
Refs: #62367

AbortSignal.any() copies the source WeakRefs of a composite input into the new composite, so nested composites share them. Since #62367 those WeakRefs are also the unregister tokens of sourceSignalsCleanupRegistry. When the inner composite is collected first, its cleanup therefore also unregisters the outer composite, and an observed outer composite stays in gcPersistentSignals after all of its sources are gone.

This uses each composite's own WeakRef as the unregister token, so collecting one composite only removes its own registrations.

The new test in test-abortsignal-drop-settled-signals.mjs fails without the change and passes with it. The reproduction from the issue goes from 8/8 to 0/8 retained composites.

AbortSignal.any() copies the source WeakRefs of a composite input into
the new composite, so nested composites share them. Those WeakRefs were
also used as the unregister tokens for the registrations that prune a
composite's sources once they are collected. When an inner composite
was collected first, its cleanup therefore also unregistered the outer
composite from the same sources. An observed outer composite then
stayed in gcPersistentSignals, together with its listeners, even after
all of its sources were gone and nothing could abort it anymore.

Use each composite's own WeakRef as the unregister token instead, so
collecting one composite only removes its own registrations.

Fixes: nodejs#66612
Refs: nodejs#62367
Assisted-by: Claude Code
Signed-off-by: journaltraces <kxenk@protonmail.com>
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 8, 2026
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Welcome to Node.js, and thank you for your first contribution!

Before review, please take a moment to read:

Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal.

@codecov

codecov Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.43%. Comparing base (dd9777b) to head (3e0d631).
⚠️ Report is 42 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66617      +/-   ##
==========================================
- Coverage   92.78%   90.43%   -2.35%     
==========================================
  Files         422      791     +369     
  Lines      193692   276587   +82895     
  Branches    29881    53112   +23231     
==========================================
+ Hits       179714   250135   +70421     
- Misses      13650    16860    +3210     
- Partials      328     9592    +9264     
Files with missing lines Coverage Δ
lib/internal/abort_controller.js 95.41% <100.00%> (+0.01%) ⬆️

... and 499 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.

@atlowChemi atlowChemi added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. abortcontroller Issues and PRs related to the AbortController and AbortSignal APIs. labels Oct 11, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 11, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@DariusHQ

Copy link
Copy Markdown
Author

@atlowChemi thanks for the review! CI run 78303 failed only on node-test-commit-osx (macos15-x64). macOS arm64 and every other platform passed, including test-abortsignal-drop-settled-signals. I can't access the Jenkins logs, so I can't tell which test failed there. Could you take a look and, if it's unrelated, add resume-ci, then commit-queue once it's green? Thanks!

@gurgunday gurgunday left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@gurgunday gurgunday added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Oct 11, 2026
@inoway46 inoway46 added the resume-ci Add this label to resume the latest eligible Jenkins CI run on a PR with an approving review. label Oct 11, 2026
@github-actions github-actions Bot added resume-ci-failed Resuming CI with the resume-ci label failed and requires manual intervention. and removed resume-ci Add this label to resume the latest eligible Jenkins CI run on a PR with an approving review. labels Oct 11, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Failed to resume CI

✖ CI job 78303 is in status RUNNING, skipping resume

Full Auto Start CI output
�[36m⠋�[39m Validating Jenkins credentials
�[36m⠋�[39m Validating Jenkins credentials
✔  Jenkins credentials valid
�[36m⠙�[39m Looking for CI runs for pull request 66617
�[36m⠙�[39m Looking for CI runs for pull request 66617
�[36m⠙�[39m Getting PR from nodejs/node/pull/66617
�[36m⠙�[39m Getting reviews from nodejs/node/pull/66617
�[36m⠙�[39m Getting comments from nodejs/node/pull/66617
✔  Found PR CI job 78303
�[36m⠹�[39m Querying data for job/node-test-pull-request/78303/
�[36m⠹�[39m Querying data for job/node-test-pull-request/78303/
�[36m⠹�[39m Querying API for job/node-test-pull-request/78303/
✔  Build data downloaded
   ✖  CI job 78303 is in status RUNNING, skipping resume

View workflow run

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

abortcontroller Issues and PRs related to the AbortController and AbortSignal APIs. author ready PRs with CI started, the required approvals, and no outstanding review comments. needs-ci PRs that need a full CI run. resume-ci-failed Resuming CI with the resume-ci label failed and requires manual intervention.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AbortSignal.any() of another composite leaks: an observed outer composite outlives all of its sources

5 participants