ITADN

🐛 Bug: parallel mode serializer mutates objects reachable from a test's error in place, corrupting shared modules for the reused worker and losing the file's results

#6053OpenGrahamCampbell 创建于 2026-06-10
type: bugstatus: in triagemajor: v11
G
GrahamCampbellcommented
### Bug Report Checklist - [x] This is NOT a [security, `npm audit`, or GitHub Advisory issue](https://mochajs.org/explainers/security-vulnerability-reports). - [x] I have read and agree to Mocha's [Code of Conduct](https://github.com/mochajs/mocha/blob/main/.github/CODE_OF_CONDUCT.md) and [Contributing Guidelines](https://github.com/mochajs/mocha/blob/main/.github/CONTRIBUTING.md) - [x] I have searched for [related issues](https://github.com/mochajs/mocha/issues?q=is%3Aissue) and [issues with the `faq` label](https://github.com/mochajs/mocha/issues?utf8=%E2%9C%93&q=is%3Aissue%20label%3Afaq%20), but none matched my issue. - [x] I have 'smoke tested' the code to be tested by running it outside the real test suite to get a better sense of whether the problem is in the code under test, my usage of Mocha, or Mocha itself. - [ ] I want to provide a PR to resolve this ### Expected When a test fails with an error that has extra properties attached, parallel mode reports the failure the same way serial mode does: the failure appears in the output with its message and stack, objects reachable from the error are left unmodified, and subsequent test files running in the same worker are unaffected. ### Actual When a failed test's error carries a property whose object graph reaches an object with function-valued properties — in our case a built-in module's exports — three things go wrong at once. The worker-side serialization deletes every function it encounters from the original objects rather than from a copy, so `require('crypto')` permanently lost `createHash`, `randomBytes`, and every other function for the remainder of the worker's life, and because workers are reused across files, unrelated test files subsequently failed with errors like `crypto.randomBytes is not a function`. The walk then reached `crypto.getRandomValues`, which is a non-configurable accessor whose getter returns a function, so the strict-mode `delete` threw, aborting the entire result batch for the file. That batch loss means the original test failure is never reported at all; the only trace is an `Uncaught error outside test suite` whose stack, after Mocha filters its own internal frames, consists of the single line `at Array.forEach (<anonymous>)`. The combination is very hard to debug: the triggering failure erases itself, the uncaught error points nowhere, and the visible failures land on innocent tests that merely shared a worker. Output from the reproduction below: ``` 1) uses crypto.randomBytes from the same worker (3) 2) Uncaught error outside test suite 3 passing (82ms) 2 failing 1) uses crypto.randomBytes from the same worker (3): AssertionError [ERR_ASSERTION]: crypto module was mutated: randomBytes is undefined, createHash is undefined ... 2) Uncaught error outside test suite: Uncaught TypeError: Cannot delete property 'getRandomValues' of #<Object> at Array.forEach (<anonymous>) ``` Note that the test that actually threw (`boom`, from the first file) is absent from the report entirely. Running the same files without `--parallel` reports the `boom` failure correctly and leaves `crypto` intact. ### Minimal, Complete and Verifiable Example Create `test/a-failing.spec.js` containing a failing test whose error references an object graph that reaches `node:crypto`. Attaching application context to errors is a common pattern, and reaching a built-in module through it is realistic: aws-sdk v2, for example, exposes `AWS.util.crypto.lib = require('crypto')`, so any error referencing an SDK-using application instance has such a path. ```js 'use strict'; it('fails with an error that references application context', () => { const error = new Error('boom'); error.applicationState = { lib: require('crypto') }; throw error; }); ``` Create four identical victim files, `test/b-victim-1.spec.js` through `test/b-victim-4.spec.js`, so that at least one runs on the reused worker after the failing file: ```js 'use strict'; const assert = require('assert'); it('uses crypto.randomBytes from the same worker', () => { const crypto = require('crypto'); assert.strictEqual( typeof crypto.randomBytes, 'function', `crypto module was mutated: randomBytes is ${typeof crypto.randomBytes}, ` + `createHash is ${typeof crypto.createHash}` ); }); ``` Run `npx mocha --parallel --jobs 2 'test/*.spec.js'`. Note that `--jobs` must be at least 2: `--parallel --jobs 1` runs the files in the main process without workers, so no IPC serialization occurs and the bug does not trigger. The core defect can also be demonstrated in six lines with no test runner involved, which is how we smoke-tested it: ```js const { SerializableEvent } = require('mocha/lib/nodejs/serializer'); const crypto = require('crypto'); const err = new Error('boom'); err.applicationState = { lib: crypto }; SerializableEvent.create('fail', { title: 'x' }, err).serialize(); // throws: TypeError: Cannot delete property 'getRandomValues' of #<Object> // and afterwards: typeof crypto.randomBytes === 'undefined' ``` One reproduction caveat: the serializer's walk deduplicates visited properties by key name alone, so if the property attached to the error happens to share a name with a key Mocha has already walked in the same event (for example `context`), the walk skips it and the bug does not trigger. The property name in the reproduction is chosen to avoid such collisions. ### Versions mocha 12.0.0-beta-9.2 is where we hit and reproduced this, with node v22 on windows-latest in CI and node v24.14.1 on macOS locally, plain CommonJS specs, the default spec reporter, and no transpilers. The relevant code in `lib/nodejs/serializer.js` is identical in 11.7.6 (current stable), 12.0.0-beta-9.5 (where the file became `serializer.mjs` but the logic is unchanged line for line), and 12.0.0-beta-10, so this is not a beta regression; the code dates back to the original parallel-mode implementation. ### Additional Info The root cause is `SerializableEvent#serialize()` in `lib/nodejs/serializer.js`, whose own doc comment says it "Modifies this object *in place* (for theoretical memory consumption & performance reasons)" and that "If this quickly becomes unmaintainable, we will want to move towards immutable objects post-haste". The `_serialize` walk visits every own-enumerable property of the event's `data` and `error` graphs, and for function-valued properties executes `delete parent[key]` (lines 227–231, "for now, just zap it") where `parent` is the original object in the worker. The throw escapes through `SerializableWorkerResult#serialize()`'s `this.events.forEach(...)` at line 76, which is the one frame that survives Mocha's stack filtering since it is native code with no file path. Two sibling hazards live in the same path and may warrant their own issues; happy to split them out if preferred. First, `breakCircularDeps` (`lib/utils.js`), added for #4552, also mutates the original objects: it replaces cyclic references with the string `'[Circular]'` inside whatever it walks, so shared singletons reachable from an error can have object-valued properties silently replaced by strings. Second, the walk's dedupe set keys on the property name alone (`seenPairs.has(pair[1])`), so when two different objects in the graph share a key name, the second occurrence is silently skipped — which both masks this bug for colliding names and means same-named properties are inconsistently processed in serialized results. For fixes, the robust direction is to serialize into fresh structures instead of mutating the walked objects, which resolves all three hazards at once. A minimal hardening alternative would be to consult `Object.getOwnPropertyDescriptor` rather than reading property values (the current code invokes getters, itself a side-effect hazard, and is what classifies `getRandomValues` as a deletable function), skip non-configurable properties, and wrap the `delete` in try/catch so a single bad property cannot destroy a file's entire results. Related prior art on serializer handling of error graphs: #4552, #5209, #5170. We hit this in oss-serverless CI, where the error attached a framework instance whose graph reached `node:crypto`; combined with `--bail`, the poisoned worker's misattributed failures stopped the run halfway and the self-erased original failure made this take days to trace.
3 条评论