Repository navigation
structuredClone: keep the serializer and deserializer reference pools in sync - #32796
Conversation
|
Updated 9:12 PM PT - Jun 26th, 2026
❌ @robobun, your commit 62f8986 has some failures in 🧪 To try this PR locally: bunx bun-pr 32796That installs a local version of the PR into your bun-32796 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughStructured-clone serialization and deserialization now pool terminal objects from version 14 on both write and read paths. Tests add in-process, cross-process, and version-13 coverage for identity and back-reference behavior. ChangesStructured clone terminal pooling
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/bindings/webcore/SerializedScriptValue.cpp (1)
2075-2119: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSerialize KeyObject payload before adding it to the object pool. Mirror the X509 path: build and validate the PEM bytes first, then
recordObjectand write the tag/payload once serialization has succeeded. The current order can advance the object pool and leave partially written state if OpenSSL fails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jsc/bindings/webcore/SerializedScriptValue.cpp` around lines 2075 - 2119, The KeyObject serialization path in the serialized value writer currently records the object before the PEM payload is fully built, which can leave the object pool and output stream in a partial state if OpenSSL serialization fails. Update the Bun::JSKeyObject handling in SerializedScriptValue::write to first build and validate the PEM bytes for Secret/Public/Private keys, then call recordObject/startObjectInternal and write the tag plus payload only after serialization succeeds, matching the safer X509 flow.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/bindings/webcore/SerializedScriptValue.cpp`:
- Around line 571-579: Shorten the newly added versioning comments around
SerializedScriptValue’s terminal object pooling so they fit the repo’s 3-line
comment limit while preserving the invariant. Update the explanatory block near
CurrentVersion and FirstVersionWithPooledTerminals to a more concise form, and
apply the same tightening to the other marked comment blocks in this file,
keeping the key facts about pooled terminal types and the version >= 14
deserializer behavior.
In `@test/js/web/workers/structured-clone.test.ts`:
- Line 29: Strengthen the structured-clone identity check for the File case in
the worker test by asserting the cloned value is a File, not only a Blob. Update
the identity matrix entry in structured-clone.test.ts so the expected
constructor matches File and the assertion verifies File-specific identity,
ensuring a regression to a plain Blob is caught.
---
Outside diff comments:
In `@src/jsc/bindings/webcore/SerializedScriptValue.cpp`:
- Around line 2075-2119: The KeyObject serialization path in the serialized
value writer currently records the object before the PEM payload is fully built,
which can leave the object pool and output stream in a partial state if OpenSSL
serialization fails. Update the Bun::JSKeyObject handling in
SerializedScriptValue::write to first build and validate the PEM bytes for
Secret/Public/Private keys, then call recordObject/startObjectInternal and write
the tag plus payload only after serialization succeeds, matching the safer X509
flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d5e7318f-413a-41f2-9a72-1e143c06731f
📒 Files selected for processing (2)
src/jsc/bindings/webcore/SerializedScriptValue.cpptest/js/web/workers/structured-clone.test.ts
![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 issues, but this touches the core structured-clone serializer across many type branches and bumps the wire format version, so it's worth a human pass to confirm the serializer/deserializer pool entries stay 1:1 on every path.
Extended reasoning...
Overview
This PR fixes object-identity preservation and back-reference corruption in SerializedScriptValue.cpp, the shared implementation behind structuredClone, postMessage, and bun:jsc serialize/deserialize. On the serializer side it adds startObjectInternal calls for Date, RegExp, ErrorInstance, DOMException, CryptoKey, KeyObject, X509Certificate, and the Bun cloneable types so duplicates emit ObjectReferenceTag. On the deserializer side it removes three stray m_gcBuffer appends in readBigInt() and adds matching, version-gated recordTerminalObject calls for each new type. CurrentVersion is bumped 13 → 14, and ~180 lines of new tests cover identity, back-reference alignment (including the >255-entry index-widening edge), and version-13 fixture compatibility.
Security risks
None identified. The change does not alter parsing of untrusted input beyond appending already-constructed objects to an internal GC buffer; bounds checks and tag parsing are unchanged. The X509Certificate reorder actually improves robustness by fully encoding the DER before any output is written or any pool entry is recorded.
Level of scrutiny
High. This is core serialization machinery used by IPC, workers, and persisted bun:jsc payloads. Correctness hinges on a subtle invariant — the serializer's m_objectPool and the deserializer's m_gcBuffer must stay index-aligned on every code path — and the PR touches ~10 separate type branches plus a wire-format version bump. A mismatch on any single path silently corrupts later back-references rather than failing loudly. The version gate on recordTerminalObject is also load-bearing for backward compatibility with stored data.
Other factors
The PR is unusually well-argued and the test additions are thorough (including frozen v13 payload fixtures that prove the version gate matters). The bug-hunting system found nothing. I spot-checked startObjectInternal and the early-return semantics in dumpDOMException / the transfer-incompatible Bun-cloneable fallback and they look consistent. Still, given the breadth of branches modified and the format-version decision, a maintainer familiar with this file should confirm nothing was missed (e.g., any remaining terminal type whose serializer/deserializer pooling is now asymmetric).
… and other terminal objects
6a6f599 to
a51a272
Compare
|
Rebased onto main after #32791 merged and rescoped this PR to the remaining half of the bug. #32791 already fixed the back-reference corruption caused by one-sided pool entries ( What remains here is object identity for On the three CodeRabbit findings:
Verification after the rebase: against main (which includes #32791) the 37 new identity tests fail and the 165 existing tests in the file pass; with this diff all 202 pass. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/web/workers/structured-clone.test.ts`:
- Around line 3-4: The test setup is importing bunExe from a different helper
source than bunEnv, which breaks the expected harness pattern. Update the
structured-clone test to use the harness-provided bunExe alongside bunEnv,
keeping both subprocess helpers sourced consistently from harness. Locate the
imports at the top of structured-clone.test.ts and adjust the existing bunExe
reference used by the worker/subprocess code so it comes from the same harness
module as bunEnv.
- Around line 491-495: Shorten the explanatory comment in the structured-clone
test to fit the 3-line limit while keeping the key point about object-pool
alignment and back-reference desync. Update the nearby comment text in
structured-clone serialization/deserialization tests so it remains clear without
the extra detail about BigInt, CryptoKey, X509Certificate, and index byte width.
Keep the reference focused on the serializer/deserializer object pool behavior
and the risk of mismatched entries shifting later references.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d939b4a6-3514-4cd6-8503-1e472dab1280
📒 Files selected for processing (2)
src/jsc/bindings/webcore/SerializedScriptValue.cpptest/js/web/workers/structured-clone.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/web/workers/structured-clone.test.ts`:
- Around line 3-4: The test setup is importing bunExe from a different helper
source than bunEnv, which breaks the expected harness pattern. Update the
structured-clone test to use the harness-provided bunExe alongside bunEnv,
keeping both subprocess helpers sourced consistently from harness. Locate the
imports at the top of structured-clone.test.ts and adjust the existing bunExe
reference used by the worker/subprocess code so it comes from the same harness
module as bunEnv.
- Around line 491-495: Shorten the explanatory comment in the structured-clone
test to fit the 3-line limit while keeping the key point about object-pool
alignment and back-reference desync. Update the nearby comment text in
structured-clone serialization/deserialization tests so it remains clear without
the extra detail about BigInt, CryptoKey, X509Certificate, and index byte width.
Keep the reference focused on the serializer/deserializer object pool behavior
and the risk of mismatched entries shifting later references.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d939b4a6-3514-4cd6-8503-1e472dab1280
📒 Files selected for processing (2)
src/jsc/bindings/webcore/SerializedScriptValue.cpptest/js/web/workers/structured-clone.test.ts
🛑 Comments failed to post (2)
test/js/web/workers/structured-clone.test.ts (2)
3-4: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Import
bunExefromharness.The subprocess helper should use the same harness source for both
bunExeandbunEnv.Proposed fix
-import { bunEnv, tls } from "harness"; -import { bunExe } from "js/bun/shell/test_builder"; +import { bunEnv, bunExe, tls } from "harness";As per coding guidelines, “When spawning Bun processes, use
bunExeandbunEnvfromharness.”📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.import { bunEnv, bunExe, tls } from "harness";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/js/web/workers/structured-clone.test.ts` around lines 3 - 4, The test setup is importing bunExe from a different helper source than bunEnv, which breaks the expected harness pattern. Update the structured-clone test to use the harness-provided bunExe alongside bunEnv, keeping both subprocess helpers sourced consistently from harness. Locate the imports at the top of structured-clone.test.ts and adjust the existing bunExe reference used by the worker/subprocess code so it comes from the same harness module as bunEnv.Source: Coding guidelines
491-495: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Shorten this comment to the 3-line limit.
Proposed tightening
-// A repeated object is serialized as an ObjectReferenceTag holding an index into the -// serializer's object pool. The deserializer must rebuild that pool entry-for-entry: -// any value it appends that the serializer did not record (BigInt primitives, -// CryptoKey, X509Certificate) shifts every later back-reference, and the index byte -// width depends on the pool size, so a big enough mismatch desyncs the whole stream. +// ObjectReferenceTag indexes into the serializer's object pool, so the +// deserializer must rebuild it entry-for-entry; extra/missing entries shift +// back-references and can desync the variable-width index stream.As per coding guidelines, “Keep code comments to 3 lines max.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// ObjectReferenceTag indexes into the serializer's object pool, so the // deserializer must rebuild it entry-for-entry; extra/missing entries shift // back-references and can desync the variable-width index stream.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/js/web/workers/structured-clone.test.ts` around lines 491 - 495, Shorten the explanatory comment in the structured-clone test to fit the 3-line limit while keeping the key point about object-pool alignment and back-reference desync. Update the nearby comment text in structured-clone serialization/deserialization tests so it remains clear without the extra detail about BigInt, CryptoKey, X509Certificate, and index byte width. Keep the reference focused on the serializer/deserializer object pool behavior and the risk of mismatched entries shifting later references.Source: Coding guidelines
|
Addressed the two new CodeRabbit findings:
Both findings are on pre-existing lines for the same reason; the first was a one-line mechanical fix in an import block this PR already touches, so I took it. |
|
This is ready for review. Summary of where things stand: VerificationAgainst current main (which includes #32791), the 37 new identity tests fail without this The red CI lanes are not from this diffThis PR touches only
The only other annotations are a retried-and-recovered warning on Merge stateThe PR is mergeable against current main, including #32799 (another I am not pushing further retriggers; the remaining red is the darwin artifact infra plus the already-fixed sourcemap lanes, and another run would just re-roll the same dice. Happy to rebase onto current main if a reviewer prefers a fully green branch build over the mergeable state. |
There was a problem hiding this comment.
I didn't find any bugs (and my earlier nit was addressed), but this bumps the on-disk serialization format version and touches the object-pool invariant across ~10 type branches in SerializedScriptValue.cpp, so it's worth a human pass.
Extended reasoning...
Overview
This PR makes structuredClone / postMessage / bun:jsc serialize preserve object identity for terminal types (Date, RegExp, Error and subclasses, DOMException, CryptoKey, KeyObject, X509Certificate, Blob/File/BlockList). It does this by recording each of these in the serializer's object pool via startObjectInternal (or the checkForDuplicate/recordObject split for X509) and adding a matching, version-gated addTerminalToObjectPool call at every corresponding deserializer site. CurrentVersion goes 13 → 14, with a FirstVersionWithPooledTerminals gate so version-13 payloads keep deserializing with their old pool indices. Tests cover every affected type via structuredClone, in-process bun:jsc round-trip, one cross-process round-trip, and hard-coded version-13 payload fixtures.
Security risks
None apparent. This is structured-clone object-identity bookkeeping; no new untrusted input parsing surface, no auth/crypto semantics change (KeyObject/X509/CryptoKey serialization payloads are unchanged — only the pool entry is added). The version gate prevents older payloads from being misread.
Level of scrutiny
Medium-high. SerializedScriptValue.cpp is the single shared serializer for workers, postMessage, and bun:jsc's persisted format. The core invariant — serializer and deserializer pool indices must stay 1:1 or every later ObjectReferenceTag resolves wrong and the variable-width index encoding desyncs — is exactly what this PR is editing across many independent type branches. It also bumps the persisted wire format version, which is a one-way door for forward compat. The change is mechanical and the pattern is consistent (each serializer startObjectInternal is paired with a deserializer addTerminalToObjectPool), but a missed pairing or a pool entry on a failure path would be a subtle data-corruption bug, so a maintainer who knows this file should confirm the pairing is complete.
Other factors
- The bug hunting system found nothing. My one prior inline nit (redundant DER encode on duplicate X509 references) was addressed in 6f62dd7 with the
checkForDuplicate/recordObjectsplit. - All CodeRabbit findings are resolved.
- Test coverage is thorough: per-type identity matrix, mixed-container reachability, control test for already-pooled types, and version-13 fixture payloads proving the version gate is load-bearing.
- The robobun CI comment shows musl build failures on 6f62dd7; a
ci: retriggercommit followed. Worth confirming green CI before merge. - Not simple/mechanical enough for bot approval given the format-version bump and the breadth of pool-invariant edits.
…ned Bun (#297) * fix(ingestion): fail a file whose processor message is lost A message the file processor cannot deserialize now fails the file with PROCESSOR_MESSAGE_FAILED, replaces the processor, and logs one line with direction, backend, file and phase. Workers handle messageerror on both sides (Bun 1.4.0+); for the child backend Bun closes the IPC channel on both ends, so a clean child exit is reported as a lost message. Task: fn-213-resident-crash-on-old-bun-and-mcp.1 Claude-Session: https://claude.ai/code/session_01Str2FrDjp49xZyocXAVLbA * feat: require Bun 1.4.1 and warn on older runtimes engines.bun moves to >=1.4.1, which carries the structured-clone fixes (oven-sh/bun#32796, #40272) in the worker message path. serve, daemon and mcp still start on an older Bun but print one warning line. Status reports the range from package.json, and the CI watcher matrix tests the floor instead of 1.3.11. Task: fn-213-resident-crash-on-old-bun-and-mcp.1 Claude-Session: https://claude.ai/code/session_01Str2FrDjp49xZyocXAVLbA * fix(mcp): write version-independent Bun paths and repair on --force gno mcp install names a version manager's version-independent link (mise installs/bun/latest, Homebrew opt/bun) instead of the versioned install directory when that link resolves to the running Bun or GNO package, and notes when no such link exists. A compiled executable is left alone. --force over an existing GNO entry now rewrites only the launch path and keeps its arguments, environment and other keys in every format, applying only the options passed explicitly. Task: fn-213-resident-crash-on-old-bun-and-mcp.1 Claude-Session: https://claude.ai/code/session_01Str2FrDjp49xZyocXAVLbA * feat: report registrations and residents on an old Bun gno doctor (mcp-runtime, resident-runtime) and gno status (runtimeIssues, 'Bun runtime:') flag MCP registrations whose Bun or entrypoint is missing or below the supported minimum, or on another Bun, with the --force repair command, and running residents on an old Bun. A resident started by another GNO version is read through its /api/status and never signalled; the advice names the PID because --stop refuses it. New pid-files record bun_version. Task: fn-213-resident-crash-on-old-bun-and-mcp.1 Claude-Session: https://claude.ai/code/session_01Str2FrDjp49xZyocXAVLbA * docs: changelog for fn-213 Task: fn-213-resident-crash-on-old-bun-and-mcp.1 Claude-Session: https://claude.ai/code/session_01Str2FrDjp49xZyocXAVLbA * chore(egress): register the Bun runtime probes The registration --version probe and the loopback /api/status read in runtime-health send no collection data. Task: fn-213-resident-crash-on-old-bun-and-mcp.1 Claude-Session: https://claude.ai/code/session_01Str2FrDjp49xZyocXAVLbA * chore(flow): task receipt fn-213-resident-crash-on-old-bun-and-mcp.1 Claude-Session: https://claude.ai/code/session_01Str2FrDjp49xZyocXAVLbA * chore(flow): qa verdict fn-213-resident-crash-on-old-bun-and-mcp * chore(flow): close fn-213-resident-crash-on-old-bun-and-mcp
2 participants
Summary
structuredClone,postMessage, andbun:jsc'sserialize/deserializeshare one serializer (SerializedScriptValue.cpp). It loses object identity for several serializable types: the sameDatereferenced twice comes back as two distinctDates. HTML's StructuredSerializeInternal requires the memory map to cover every serialized object, and Node preserves all of these.The same applies to
RegExp, everyErrorsubclass,DOMException,CryptoKey, node'sKeyObject,X509Certificate, andBlob/File.Cause
A repeated object is serialized as
ObjectReferenceTag <index>into the serializer's object pool, and the deserializer rebuilds that pool entry for entry (#32791 gave it a dedicatedm_objectPoolfor exactly this). None of the types above were ever entered into the pool on either side, so a repeat reference is simply serialized again as a fresh copy.Fix
Record them on both sides, 1:1, and bump the serialization format version.
Serializer (
CloneSerializer::dumpIfTerminal):startObjectInternalforDateInstance,RegExpObject,ErrorInstance,DOMException,CryptoKey,KeyObject,X509Certificate, and the Bun cloneable types (Blob,BlockList). For the Bun cloneables this generalizes the change #32791 made on the for-transfer/for-storage placeholder path to the compatible path as well. TheX509Certificatebranch now encodes the DER before touching the output, so ani2d_X509failure no longer leaves a partially written tag behind.Deserializer: a matching
addTerminalToObjectPoolcall at each of those tag readers, gated onm_version >= 14. Version 13 writers did not count these types, so the reader must not pool them for version 13 payloads or the pool indices stop matching.CurrentVersiongoes from 13 to 14. Version 13 payloads (for example those persisted withbun:jscserialize) keep deserializing correctly, and version 14 payloads are rejected by older readers through the existingversion > CurrentVersioncheck instead of being silently misread.Out of scope:
WebAssembly.ModuleandWebAssembly.Memoryalso lose identity, but they are tracked in a separate side table on both sides (so there is no corruption), upstream WebKit behaves the same way, and fixing them is an unrelated mechanism.Builds on #32791, which fixed the other half of this bug class: back-references corrupted by pool entries the deserializer recorded but the serializer never did (
BigInt,CryptoKey,X509Certificate).Tests
Added to
test/js/web/workers/structured-clone.test.ts:structuredCloneand the in-processbun:jscround trip, plus one cross-process round trip covering the whole matrix,DateandTypeErrorthrough object, array,Map, andSetpaths,bun:jscdata still deserializes correctly and that the version gate is load-bearing,Without the fix, 37 of the new tests fail and the 165 existing tests in the file (including the ones added by #32791) pass; with it, all 202 pass.