Conversation
|
@robobun adopt |
|
✅ Adopted & validated — ready for a maintainer. Built debug (ASAN) and ran the full suites —
Confirmed fail-before on a build without the fix (cause comes back CI (#59701): every build lane green (incl. ASAN, musl, baseline, android, freebsd, windows-cross) and every linux/windows/alpine test lane green (incl. Commits on top of the original: |
|
Actionable comments posted: 0 |
|
Warning Review limit reached
More reviews will be available in 17 minutes and 32 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis PR extends structured-clone support for Error instances by adding serialization and deserialization of the ChangesError cause structured-clone support
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
…erialization The cause stashed in m_pendingErrorCause is descended into by the state machine one iteration later. Append it to m_gcBuffer when stashing it, mirroring the map-value path (MapDataStartVisitEntry), so the codec roots its own pending value rather than relying on the cause staying reachable as an own property of the error being serialized.
|
Actionable comments posted: 0 |
The two "event listeners doesn't close event loop" tests killed the spawned subprocess after a fixed setTimeout(1000) and reported a timeout. Under the ASAN debug build the spawn + ~50-message worker exchange exceeds 1s, so the watchdog fired before the child could print "done" and exit 0, failing the tests even though the behavior under test was correct. Drop the timer and just await x.stdout + x.exited (await using handles cleanup), matching the repo's no-setTimeout-in-tests guidance.
9b31b63 to
881b097
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs and the test coverage is thorough, but this threads new control flow through the structured-clone state machine (with a wire-format version bump and new Error identity-pooling), so it's worth a human pass on the C++ side.
Extended reasoning...
Overview
This PR extends SerializedScriptValue.cpp to preserve Error.prototype.cause across structuredClone, postMessage, and node:v8 serialize/deserialize. It bumps CurrentVersion 13→14, adds a new ErrorEndVisitCause walker state, introduces m_pendingErrorCause / m_pendingErrorWithCause member slots, and weaves cause serialization into the existing iterative state machine rather than recursing natively. It also newly calls startObjectInternal() on ErrorInstances and appends them to m_gcBuffer on the deserializer side, which adds identity preservation (the same Error appearing twice now round-trips as a single object via ObjectReferenceTag). Two test files gain ~130 lines of coverage for primitive/object/nested/cyclic/shared causes plus a captured v13 blob for back-compat, and two flaky worker tests are rewritten to await subprocess exit instead of racing a 1s timeout.
Security risks
The deserializer reads one extra byte (hasCause) gated on m_version >= 14, and the cause value is read through the existing recursive-value path with the same maximumFilterRecursion depth check applied via outputObjectStack.size(). I don't see new untrusted-input parsing surface beyond what the existing object/array/map paths already expose, and the v13 back-compat gate means older persisted blobs don't hit the new read. No auth/crypto/permissions code is touched.
Level of scrutiny
This warrants careful human review. The change is well-reasoned and the tests are excellent (cycles, shared identity, deep chains, version back-compat, cross-process, cross-thread), but the implementation relies on subtle invariants: dumpIfTerminal is re-entered from StateUnknown after the array/object member visitors already called it once, and the new top-of-function guard on m_pendingErrorCause is what prevents double-emission on that second call. Likewise the deserializer's m_pendingErrorWithCause guard suppresses byte consumption on re-entry. These are correct as far as I traced, but the goto-driven state machine has several dumpIfTerminal / readTerminal call sites and the interaction with the new object-pool registration for errors (which changes both serialization output — ObjectReferenceTag for repeated errors — and m_gcBuffer indexing on deserialize) is the kind of thing a maintainer familiar with this file should sanity-check.
Other factors
robobun built debug+ASAN and confirmed both test suites pass with fail-before/pass-after on the cause round-trip; the only CI failures are unrelated musl LTO link errors. The wire-format bump is one-way (v14 blobs won't deserialize on older Bun), which is expected but worth a maintainer ack. No outstanding reviewer comments.
|
Let's use |
|
Added On byte-exact: no, and it shouldn't be. Bun's |
Extend error-cause-node-parity so every case runs through both shared entry points (structuredClone and the node:v8 round-trip): descriptor shape (writable, non-enumerable, configurable), no-cause => no own property, explicit-undefined cause, number/object causes, nested Error type, chains, cyclic self-reference identity, shared identity, and identity inside containers. Rename to .test.mts so Node parses it as ESM without the MODULE_TYPELESS_PACKAGE_JSON warning; bun test still auto-discovers it. structured-clone.test.ts now spawns `node --test` on the file, so the Node.js half of the parity claim is enforced in CI rather than only being runnable by hand. Serialization is intentionally not byte-compatible across runtimes (JSC SerializedScriptValue vs V8 serializer); behavior is what must match.
No — and it can't be. Bun's Done in 90d254e, building on your
|
The Terminal production listed every tag except ErrorInstanceTag (a gap since v13 introduced it). Document the full layout including the v14 hasCause byte and optional recursive cause value.
Mirrors main's noUnify entry from 63d5cd4: foreach_target.h has a TU-wide include guard, so when xxhash3.cpp shares a release-sized (32-file) bundle with highway_strings.cpp it only expands the baseline ISA namespace and HWY_EXPORT(HashLong) fails to resolve the N_SSE4/N_AVX2/N_AVX3 variants. Carrying the exclusion here keeps this branch building cleanly against trees that contain the xxhash3 TU; the hunk is verbatim from main so it merges as a no-op.
This branch has not been deployed
2 participants
structuredClone(err),Worker.postMessage(err), andnode:v8.serialize/deserialize(err)silently dropped an Error'scause— the cloned error hadcause === undefined. Node preserves it. TheSerializedScriptValueErrorInstance codec serialized only{type, message, line, column, sourceURL, stack}, nevercause.Serialize
causeas a full value through the existing clone state machine (the same path Array/Object/Map children use), so it keeps its type — an Error cause stays an Error, an object stays an object, primitives by value, nested chains preserved — matching Node/V8 (not upstream WebKit's stringify-the-cause). The error is recorded in the object pool before its cause, so cycles (e.cause = e) and shared identity resolve correctly, and 5000-deep cause chains serialize via the work-stack with no native recursion.Bumps
CurrentVersion13→14 and gates the cause read on the version, so already-persisted v13v8.serializeblobs still deserialize (verified with a captured v13 blob). An error with no owncauseround-trips unchanged;causeis reconstructed non-enumerable, matchingnew Error(msg, { cause }).Matches Node exactly (string / number / object / nested-Error / cyclic / undefined / no-cause) across all three entry points; v14 also gains Error identity preservation. Adds tests across structuredClone,
node:v8, andWorker.postMessage. Not a port regression — the C++ codec dropped cause in 1.3.14 too; Node is the reference.