Skip to content

fix: serialize parallel test results without mutating user objects#6056

Draft
GrahamCampbell wants to merge 2 commits into
mochajs:mainfrom
GrahamCampbell:fix/parallel-serializer-mutation-12x
Draft

fix: serialize parallel test results without mutating user objects#6056
GrahamCampbell wants to merge 2 commits into
mochajs:mainfrom
GrahamCampbell:fix/parallel-serializer-mutation-12x

Conversation

@GrahamCampbell

Copy link
Copy Markdown
Contributor

PR Checklist

Overview

In parallel mode, SerializableEvent#serialize prepares events for IPC by walking the event's data and error graphs in place: function-valued properties are deleted from the original objects. When a failing test's error references shared state (the #6053 repro reaches require('crypto')), shared modules are permanently stripped of their functions for later files run by the reused worker, and when the walk reaches a non-configurable property (e.g. the crypto.getRandomValues accessor) the strict-mode delete throws, aborting SerializableWorkerResult#serialize — the whole file's result batch is lost and the original failure is never reported.

This PR replaces the in-place walk with one that builds a fresh, JSON-safe copy and never mutates its inputs:

  • Functions are omitted from the copy instead of deleted from originals, so non-configurable and frozen properties can no longer throw.
  • Cycle handling moves into the copy walk (object-identity based, writing the same [Circular] marker into the copy). The breakCircularDeps(result.error) call is removed: it mutated original error graphs, and it crashes outright on ESM module namespace objects (TypeError: Cannot assign to read only property ... of '[object Module]' — namespace exports report writable: true but reject [[Set]]), the import * twin of the reported bug. The exported util itself is untouched.
  • The old dedupe keyed on property name — which skipped same-named properties on different objects (including anything literally named data or error, pre-seeded by the top-level keys) and doubled as the walk's only termination guard — is replaced by identity-based cycle detection plus a node budget and depth cap ([Truncated] placeholder). Cyclic event data previously survived the walk and killed the batch in process.send; lazily-infinite getter chains could hang the worker.
  • Array contents are now actually walked. The old array branch built [element, index] pairs (element as the parent), so e.g. the extra errors in err.multiple crossed the wire as {} with their messages lost. Function elements become null, which is what the JSON IPC channel already did to leaked functions.
  • BigInt values are stringified (previously a guaranteed process.send TypeError losing the batch) and throwing getters are omitted rather than fatal.
  • Constructor-form error.cause (own but non-enumerable, previously dropped by the Object.assign copy) is carried, so cause chains render in parallel mode as they do in serial mode.
  • SerializableWorkerResult#serialize now substitutes a same-eventName stand-in when an event still cannot be serialized, so a single pathological event cannot erase a file's results.

Wire compatibility: all existing serializer unit tests pass unchanged (two test titles reworded, assertions untouched), and the existing parallel integration fixtures for circular references and getters pass as-is. An own enumerable toJSON is still not honored while inherited implementations (Date, Buffer) still are, matching the previous end-to-end JSON behavior. The deliberate, visible differences are strict improvements over data destruction: nested errors arrive with message/stack instead of {}, data cycles serialize as [Circular] instead of crashing the send, and pathological graphs truncate instead of hanging or overflowing.

Verification: 60 serializer unit tests (40 existing + 20 new, one per hazard), the parallel integration specs, and an end-to-end run of the #6053 repro (the original failure is reported and crypto stays intact in the reused worker) all pass on this branch.

Note: the first commit here is the same change as #6055 — on current main the serializer spec cannot load (#6054), so this branch needs it to run its own tests; happy to rebase once #6055 or #6015 lands. The four node-unit failures visible on this branch (buffered-worker-pool, parallel-buffered-runner, reporters/parallel-buffered, mocha.spec) predate it — current main cannot run the node-unit suite at all — and are addressed by #6015. A v11.x backport (the serializer there is identical apart from module wrappers) follows separately.

@github-actions

Copy link
Copy Markdown
Contributor

👋 Hi @GrahamCampbell, thanks for the pull request! A scan flagged a concern with it. Could you please take a look?

[pr-task-completion] This PR's body is missing [x] checks on the following tasks from the PR template.

Repositories often provide a set of tasks that pull request authors are expected to complete. Those tasks should be marked as completed with a [x] in the pull request description. Please complete those tasks and mark the checks as [x] completed.

🗺️ This message was posted automatically by OctoGuide: a bot for GitHub repository best practices.

@mark-wiemer

Copy link
Copy Markdown
Member

Marking as draft until the backing issue it out of triage :)

@mark-wiemer
mark-wiemer marked this pull request as draft June 11, 2026 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants