test_runner: do not reuse a worker ID held by a running file - #65739
vserpokryl wants to merge 1 commit into
Conversation
|
Review requested:
|
35a00d4 to
5416f6a
Compare
Codecov Report❌ Patch coverage is
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
🚀 New features to boost your workflow:
|
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>
620faf8 to
ee9bc89
Compare
MoLow
left a comment
There was a problem hiding this comment.
Change LGTM, I am debating if this is a breaking change or a bugfix though
|
@MoLow I'd call it a bugfix: the docs say |
|
Thanks for the reviews @MoLow @jasnell @pmarchini! This has 3 approvals, no requested changes and no conflicts, but 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 |
WorkerIdPoolhanded 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=2and three files, where the first one outlives thesecond:
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
finallyblock 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