Fix two-byte record corruption: cap maxOwnStructures at 32 (v5.0) - #1176
Closed
kriszyp wants to merge 1 commit into
Closed
Fix two-byte record corruption: cap maxOwnStructures at 32 (v5.0)#1176kriszyp wants to merge 1 commit into
kriszyp wants to merge 1 commit into
Conversation
RecordEncoder set maxOwnStructures=256 (the OOM cap). With msgpackr's default
maxSharedStructures of 32, that pushes maxOwn+maxShared past 64, flipping on
msgpackr's two-byte record-id encoding. That path mis-serializes over-cap "own"
structures when a shared-structures store is present (every primary store has
one): it writes an out-of-range record-id reference instead of inlining the
structure, so once a table exceeds the shared cap, new records become
undecodable ("Record id is not defined for N") — on BOTH the typed default and
the randomAccessFields=false opt-out. Confirmed via a high-cardinality repro
(2900+/3000 records undecodable at maxOwn=256; 0 at maxOwn<=32).
Lowering to 32 keeps msgpackr on the one-byte path, which inlines over-cap
shapes correctly and bounds memory at least as tightly. Adds a regression test
that writes 500 distinct shapes through a shared store and asserts every record
decodes, on both the typed and readOnlyStructures paths. The deeper msgpackr
two-byte fix (to make >32 shared structures correct) is tracked separately.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Member
Author
|
Superseding this with #1179. The cap→32 here was a write-side stop-gap for the unfixed msgpackr two-byte path. Now that the root cause is fixed upstream (msgpackr 1.12.1, read-path only — kriszyp/msgpackr#189), #1179 just bumps the dep and keeps
The regression test from here is carried over to #1179. Closing in favor of #1179. — Claude |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RecordEncodersetmaxOwnStructures = 256. With msgpackr's defaultmaxSharedStructures = 32,maxOwn + maxShared > 64flips on msgpackr's two-byte record-id encoding, which mis-serializes over-cap "own" structures when a shared-structures store is present (every primary store has one): it writes an out-of-range record-id reference instead of inlining the structure. Once a table exceeds the shared cap, new records become undecodable —Record id is not defined for N. This fix lowers the cap to 32, keeping msgpackr on the correct one-byte path (which inlines over-cap shapes and bounds memory at least as tightly).Why this matters (data integrity, shipped in v5.0.29)
The broken
maxOwnStructures=256is the OOM cap from #1146, shipped in v5.0.29. The corruption hits both configs:randomAccessFields=true)randomAccessFields=false, Add storage.randomAccessFields config to disable typed structures (v5.0) #1169)i.e. any high-cardinality table (e.g. CDI's
RaceEntrywith undeclared dynamic attributes) writing records past the ~32-shape shared cap produces undecodable records. This is theRecord id is not defined for Nflood reported on dev/CDI (Slack thread, and Nathan Heskew's verification on #1169) — root-caused here to the two-byte path, not the #1157/#186 save-race.Patch candidate for v5.0.30.
Repro (high confidence)
Pure-msgpackr matrix with a shared store + 3–5k distinct shapes:
maxOwn=256→ ~2900/3000 records undecodablemaxOwn=33(just over the one-byte boundary) → also brokenmaxOwn=32/ default → 0 errors, dictionary boundedWhere to look
resources/RecordEncoder.ts— the one-line cap change (256 → 32) + a comment explaining the one-byte invariant (maxOwn + maxShared ≤ 64).unitTests/resources/recordEncoder.test.js— new regression block: writes 500 distinct shapes through a shared store and asserts every record decodes, on both the typed andreadOnlyStructurespaths. (Fails at 256, passes at 32.)Open / follow-up
recordEncoder.test.jsconfig/byte-range tests flake under full-file ordering (config-state isolation), and the resources suite has the environmental "Data read" setup crash. The new regression tests pass in isolation and are robust to ambient config (they assert decodability, not byte ranges).🤖 Generated by Claude (Opus 4.7). Codex review: clean — no actionable correctness issues introduced. (Gemini was daily-quota-limited on the related PRs.)