Skip to content

test_runner: do not reuse a worker ID held by a running file - #65739

Open
vserpokryl wants to merge 1 commit into
nodejs:mainfrom
vserpokryl:test-runner-exclusive-worker-id
Open

vserpokryl wants to merge 1 commit into
nodejs:mainfrom
vserpokryl:test-runner-exclusive-worker-id

Conversation

@vserpokryl

Copy link
Copy Markdown

WorkerIdPool handed out worker IDs round-robin ((nextId++ % maxConcurrency) + 1)
and never released them, so an ID was only unique among the first N test files.
As soon as files finished out of order, a file that started later was given an ID
still held by a live process, which defeats the purpose of context.workerId:
allocating a database, port or directory per worker.

With --test-concurrency=2 and three files, where the first one outlives the
second:

start slow 1
start fast 2
end fast 2
start last 1   <-- same ID as the still-running 'slow'
end last 1
end slow 1

Track the IDs that are actually in use and hand out the lowest free one, then
release it once the child process is gone. The release happens in a finally
block so an aborted run or a failed spawn does not leak the ID out of the pool.

The pool no longer needs to be told the concurrency level. That also removes a
mismatch: the pool was sized from globalOptions.concurrency ?? concurrency,
which can differ from the concurrency the root test actually enforces. Since IDs
are handed out lowest-first, the highest ID in use is now bounded by how many
files really run at once.

Refs: #61394

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Sep 2, 2026
@vserpokryl
vserpokryl force-pushed the test-runner-exclusive-worker-id branch from 35a00d4 to 5416f6a Compare September 2, 2026 12:50
@codecov

codecov Bot commented Sep 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.96%. Comparing base (5ed55ba) to head (ee9bc89).
⚠️ Report is 37 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/test_runner/runner.js 97.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65739      +/-   ##
==========================================
- Coverage   90.05%   89.96%   -0.10%     
==========================================
  Files         754      757       +3     
  Lines      256518   258078    +1560     
  Branches    48534    48929     +395     
==========================================
+ Hits       231007   232167    +1160     
- Misses      16609    16972     +363     
- Partials     8902     8939      +37     
Files with missing lines Coverage Δ
lib/internal/test_runner/runner.js 95.19% <97.00%> (+0.24%) ⬆️

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

Worker IDs were handed out round-robin and never released, so a file
that started after another finished could get an ID still held by a
live process. Track the IDs in use, hand out the lowest free one, and
release it in a finally block once the child process exits.

Refs: nodejs#61394
Signed-off-by: Vasiliy Serpokryl <vasiliy.serpokryl@mail.ru>
@vserpokryl
vserpokryl force-pushed the test-runner-exclusive-worker-id branch from 620faf8 to ee9bc89 Compare September 3, 2026 07:09

@MoLow MoLow 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.

Change LGTM, I am debating if this is a breaking change or a bugfix though

@vserpokryl

Copy link
Copy Markdown
Author

@MoLow I'd call it a bugfix: the docs say workerId is "useful for splitting resources (like database connections or server ports) across concurrent test files" (https://nodejs.org/api/test.html#contextworkerid), and that only holds if an ID isn't shared by two running files. Nothing documented changes either: the ID range only gets tighter, and which file gets which ID was never deterministic anyway.

@vserpokryl

Copy link
Copy Markdown
Author

Thanks for the reviews @MoLow @jasnell @pmarchini!

This has 3 approvals, no requested changes and no conflicts, but needs-ci is still on it. request-ci was never added.

Could someone start CI and add a semver label? Per my comment above I'd read this as semver-patch, since the documented guarantee never actually held

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

Labels

needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants