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

test_runner: do not reuse a worker ID held by a running file#65739
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
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 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.99%. Comparing base (5ed55ba) to head (5416f6a).
⚠️ Report is 25 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.99%   -0.06%     
==========================================
  Files         754      757       +3     
  Lines      256518   257374     +856     
  Branches    48534    48795     +261     
==========================================
+ Hits       231007   231627     +620     
- Misses      16609    16838     +229     
- Partials     8902     8909       +7     
Files with missing lines Coverage Δ
lib/internal/test_runner/runner.js 95.19% <97.00%> (+0.24%) ⬆️

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

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.

2 participants