test_runner: do not crash on stdout that mimics a v8 frame

[faizanu94]

Summary

node --test can crash the parent runner when a test writes bytes to stdout that look like a v8 report frame. A single console.log of the wrong bytes aborts the whole run with an internal error that does not point at any test:

Error: Unable to deserialize cloned data.
    at #processRawBuffer (node:internal/test_runner/runner:...)

Fixes: #66164

Root cause

The child test process multiplexes framed report messages and raw user stdout on one pipe. A frame starts with the two magic bytes FF 0F followed by a 4 byte size. #processRawBuffer scans for that magic, reads the size and hands the payload to the v8 deserializer. User output can contain those same bytes, so stray stdout with a plausible size reaches the deserializer, which throws. The call had no error handling, so the exception aborted the entire run. The earlier >>> 0 fix in #64706 hardened the size read only. This case has a valid looking size and fails one step later at the deserialize.

Fix

The reporter writes each frame as a v8 header, a 4 byte size, then a second v8 header, then the serialized value. So a real frame always repeats the FF 0F header at the start of its payload. #processRawBuffer now uses that structure instead of a blanket catch:

1. Validate before deserializing. A candidate is handed to the deserializer only when its payload is long enough and begins with the inner v8 header. Stray stdout that only mimics a frame fails this check and is emitted as stdout. A genuine frame with a valid inner header that still fails to deserialize is left to throw, so a real regression in the report protocol surfaces instead of being hidden.
2. Resync live. When the bytes only mimic a frame, one byte is emitted as stdout and the header search restarts right away, so a real frame that follows stray bytes is reported immediately instead of waiting for shutdown.
3. Single loop. #processRawBuffer is now one loop that inspects the head of the buffer on each pass and handles exactly one case: leading stdout, an incomplete frame, a frame that only mimics the header, or a real frame. This replaces the earlier nested loops and the mimicsFrame flag.

This turns a fatal crash into recoverable stdout, keeps any real frames that follow the stray bytes and surfaces genuine deserialize failures.

Tests

Added cases to test/parallel/test-runner-v8-deserializer.mjs:

• stdout that mimics a frame with a plausible size becomes stdout and does not crash
• that false frame followed by a real message: the parser resyncs live and reports the real event before drain()
• a real message, then the false frame, then another real message: both real messages survive
• several stray frames in a row are peeled off as stdout and a real message after them is still reported
• the false frame split across two chunks still recovers as stdout
• a frame whose payload is shorter than the inner header is treated as stdout
• a genuinely corrupt frame with a valid inner header is left to throw instead of being hidden

Known limitation (possible follow-up, out of scope here)

For stray bytes to be misread now they would have to reproduce the full frame shape, the outer header, a matching size, the inner header, then a valid serialized payload. This is pre-existing and extremely unlikely. Fully removing the ambiguity needs an escape mechanism or a separate channel for user stdout, which is a larger change. This PR removes the crash, which is the reported bug.

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[github-actions]

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

Before review, please take a moment to read:

• the guide for first-time contributors
• the contribution and automation policies
• the pull request guide
• the AI use policy
• the Code of Conduct

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.

[inoway46]

This seems reasonable as a mitigation, but I'm concerned that swallowing deserialization errors could hide real regressions in the internal report protocol.

Could we make the frame validation stricter before deserializing instead? For example, checking for the expected inner V8 header after the length field may let us reject false positives as stdout without hiding genuine deserialization failures.

Separating the report stream from user stdout still seems like the cleanest long-term fix, but stricter validation might be a smaller alternative.

[codecov]

Codecov Report

❌ Patch coverage is 96.92308% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.36%. Comparing base (6a6e09e) to head (d249668).
⚠️ Report is 19 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/test_runner/runner.js	96.92%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #66273   +/-   ##
=======================================
  Coverage   90.36%   90.36%           
=======================================
  Files         790      790           
  Lines      274787   274813   +26     
  Branches    52631    52629    -2     
=======================================
+ Hits       248323   248347   +24     
- Misses      16932    16942   +10     
+ Partials     9532     9524    -8     

Files with missing lines	Coverage Δ
lib/internal/test_runner/runner.js	95.12% <96.92%> (-0.07%)	⬇️

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

[faizanu94]

Thanks for the sharp review. You are right that a blanket catch could hide a real regression in the report protocol. I pushed a commit that validates the inner v8 header before deserializing, so stray stdout that only mimics a frame is rejected as output, while a genuine frame that fails to deserialize is left to surface instead of being swallowed. I also added a test for that second case. I agree that separating the report stream from user stdout is the cleanest long-term fix.

[inoway46]

Thanks for the update! My previous concern is resolved. I left one remaining comment.

Also, #66307 has landed in main, so could you rebase onto the latest main to pick up the fix for the CI failure?

[inoway46]

Does this actually verify resync before drain()?

collectReported() calls fileTest.drain() after feeding all chunks, so this would still pass if the real frame stayed buffered until the child exited.

Could we assert before drain() instead, e.g.:

fileTest.parseMessage(plausibleSizeFalseHeader);
fileTest.parseMessage(chunks[0]);

assert.deepStrictEqual(reported.at(-1), reportedDiagnosticEvent);

If the inner-header check already tells us this is not a valid frame, perhaps we could consume one byte as stdout and immediately restart the header search instead of breaking.

[faizanu94]

Thanks for the review. I have addressed the remaining comment.

Recovery is now live inside #processRawBuffer. When the payload does not start with the inner v8 header it emits one byte as stdout and restarts the header search right away instead of breaking, so a real frame that follows a stray one is reported immediately.

The test now asserts before drain(). It feeds the false frame then a real message and never calls drain(). It then checks that the diagnostic is already reported. One small note: the reporter is a stream, so I flush it with end() and finished() before asserting rather than reading it synchronously. That keeps the assertion deterministic.

I also rebased onto the latest main to pick up #66307.

[inoway46]

Thanks for the update. The behavior looks better now, but #processRawBuffer() has become difficult to reason about with the nested loops and mimicsFrame state.

Could we simplify it into a single loop that re-checks the buffer head after each step?

1. If the buffer does not start with a frame header, emit stdout and continue.
2. If the frame is incomplete, stop and wait for more data.
3. If it only mimics a frame, consume enough to make progress and continue.
4. Otherwise deserialize the real frame and continue from the remaining buffer.

I think this would make the resync behavior and edge cases much easier to reason about.

[faizanu94]

Thanks, that reads much better. I rewrote #processRawBuffer as a single loop that re-checks the head on each pass and handles one case at a time: leading stdout, an incomplete frame, a frame that only mimics the header, or a real frame. The nested loops and the mimicsFrame flag are gone. The existing tests cover each case.

[MoLow]

this might break every time we upgrade v8.
also, Id expect to see some regression tests

[inoway46]

Thanks for addressing. LGTM!

[inoway46]

@faizanu94 It looks like the PR description is a bit outdated. Could you update it to reflect the current implementation?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/78004/

[faizanu94]

@inoway46 Done. I have updated the description to match the current implementation.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/78007/

[inoway46]

@MoLow Would you mind landing this manually and updating the squashed commit message to reflect the current implementation? Rewriting the first commit now would require another approval and full CI run.