Skip to content

test_runner: avoid hanging on incomplete v8 frames#62704

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
thisalihassan:fix/test-runner-hangs
May 18, 2026
Merged

test_runner: avoid hanging on incomplete v8 frames#62704
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
thisalihassan:fix/test-runner-hangs

Conversation

@thisalihassan
Copy link
Copy Markdown
Contributor

@thisalihassan thisalihassan commented Apr 12, 2026

The test runner could hang while draining child stdout when the buffered data started with bytes that matched the V8 serializer header (0xff 0x0f) but did not contain a complete serialized frame.

In that case #processRawBuffer() made no progress, and #drainRawBuffer() kept calling it in a loop forever.

This change makes the deserializer recover from that end-of-stream case by flushing the buffered bytes as stdout and continuing to resync. It also preserves the ordering of a trailing partial V8 header split across chunks.

Closes #62693

Assisted-by: Claude Opus

@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 Apr 12, 2026
// size, causing #processRawBuffer to make no progress and
// #drainRawBuffer to loop forever without the no-progress guard.
const reported = await collectReported([oversizedLengthHeader]);
assert(reported.every((event) => event.type === 'test:stdout'));
Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Apr 12, 2026

Choose a reason for hiding this comment

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

We can get a much more useful error output by using partialDeepStrictEqual here

Suggested change
assert(reported.every((event) => event.type === 'test:stdout'));
assert.partialDeepStrictEqual(reported, Array.from({ length: reported.length }, () => ({ type: 'test:stdout' }));

Comment thread lib/internal/test_runner/runner.js Outdated
data: {
__proto__: null,
file: this.name,
message: TypedArrayPrototypeSubarray(bufferHead, 0, 1).toString('utf-8'),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we're dealing with a single-byte UTF-8 char, we don't need the intermediate view I think

Suggested change
message: TypedArrayPrototypeSubarray(bufferHead, 0, 1).toString('utf-8'),
message: StringFromCharCode(bufferHead[0]),

Copy link
Copy Markdown
Contributor Author

@thisalihassan thisalihassan Apr 12, 2026

Choose a reason for hiding this comment

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

it changes the output semantics here StringFromCharCode(0xff) gives "ÿ" but Buffer.from([0xff]).toString('utf8') gives "�" which matches the current stdout path so I’m keeping the UTF-8 decode

Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Apr 12, 2026

Choose a reason for hiding this comment

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

String.fromCodePoint then – but I'm surprised we would get non-ASCII chars here

Copy link
Copy Markdown
Contributor Author

@thisalihassan thisalihassan Apr 12, 2026

Choose a reason for hiding this comment

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

Right, switched to StringFromCharCode the byte here is always the V8 version tag (0xFF)

Signed-off-by: Ali Hassan <ali-hassan27@outlook.com>
@thisalihassan thisalihassan force-pushed the fix/test-runner-hangs branch from 631f70a to 825b3e3 Compare April 12, 2026 11:20
@thisalihassan thisalihassan requested a review from aduh95 April 12, 2026 11:21
@avivkeller
Copy link
Copy Markdown
Member

Can you add a proper PR description?

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 12, 2026

Codecov Report

❌ Patch coverage is 95.65217% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.80%. Comparing base (0fea430) to head (e07cdd4).
⚠️ Report is 355 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/test_runner/runner.js 95.65% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #62704      +/-   ##
==========================================
- Coverage   89.80%   89.80%   -0.01%     
==========================================
  Files         699      699              
  Lines      216363   216417      +54     
  Branches    41370    41377       +7     
==========================================
+ Hits       194309   194356      +47     
- Misses      14140    14141       +1     
- Partials     7914     7920       +6     
Files with missing lines Coverage Δ
lib/internal/test_runner/runner.js 93.68% <95.65%> (+0.03%) ⬆️

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

…-byte fallback path

Signed-off-by: Ali Hassan <ali-hassan27@outlook.com>
@thisalihassan
Copy link
Copy Markdown
Contributor Author

@aduh95 can you please take a relook at this again?

@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label May 17, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 17, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@atlowChemi atlowChemi added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels May 18, 2026
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 18, 2026
@nodejs-github-bot nodejs-github-bot merged commit debe2ed into nodejs:main May 18, 2026
71 checks passed
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Landed in debe2ed

aduh95 pushed a commit that referenced this pull request May 19, 2026
Signed-off-by: Ali Hassan <ali-hassan27@outlook.com>
PR-URL: #62704
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
aduh95 pushed a commit that referenced this pull request May 19, 2026
Signed-off-by: Ali Hassan <ali-hassan27@outlook.com>
PR-URL: #62704
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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.

test_runner: infinite loop in FileTest#drainRawBuffer when child stdout contains FF 0F followed by large size bytes

7 participants