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 due to invalid or unsupported version.
    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

Two small changes in #processRawBuffer:

1. Advance the buffer past a frame only after it has been read successfully, so a failed read cannot drop real frames that follow.
2. Wrap readHeader() and readValue() in try/catch. On failure, leave the buffer untouched and stop parsing frames. The existing #drainRawBuffer no progress path then emits the stray byte as stdout and rescans for the next real header.

This turns a fatal crash into recoverable stdout, keeps any real frames that follow the stray bytes and matches how the runner already treats false headers with an oversized size.

Tests

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

• stdout that mimics a frame with a plausible size becomes stdout and does not crash
• the same false frame followed by a real message: the parser resyncs and still reports the real event
• a real message, then the false frame, then another real message: both real messages survive
• the false frame split across two chunks still recovers as stdout

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

If stray bytes ever form a valid v8 payload, the deserialize would succeed and report a bogus item. 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

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.34%. Comparing base (a3bb551) to head (ff40068).
⚠️ Report is 50 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #66273      +/-   ##
==========================================
- Coverage   90.35%   90.34%   -0.01%     
==========================================
  Files         789      789              
  Lines      273493   273506      +13     
  Branches    52281    52280       -1     
==========================================
- Hits       247101   247091      -10     
- Misses      16853    16891      +38     
+ Partials     9539     9524      -15     

Files with missing lines	Coverage Δ
lib/internal/test_runner/runner.js	95.24% <100.00%> (+0.05%)	⬆️

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