Bound the typed-structure dictionary with maxOwnStructures - #4
Merged
Conversation
typedStructs is append-only and pinned on the long-lived encoder, and the encoder branches per-field on value width (num8/num32/num64, float32/float64). A wide, sparsely/variably-populated schema therefore mints a distinct structure for every (key-set x width-combination), growing the dictionary + transition trie without limit — a latent unbounded-memory path. maxOwnStructures freezes dictionary growth once the cap is reached: novel shapes return 0/null from the write hooks and fall back to plain encoding, while existing structures (and any persisted on-disk) stay decodable. Applied symmetrically to both the fast (writeStructInPlace) and standalone (_encode) paths. Default is uncapped — no behavior change unless a host opts in. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Address two issues found in review of the maxOwnStructures cap: 1. onLoadedStructures rebuilds the transition trie via createTypeTransition, which returns undefined while the module-level freeze flag is set. A reader loading previously-persisted structures after any capped encoder hit its cap in the same process would throw. Clear the flag on load — replaying saved structures is never subject to the cap. 2. Re-check the cap at the record-id mint point (not just the entry-time flag): nested encodes via pack()/encodeNested() can append structures after entry, so a record could overshoot the cap by its nesting depth. The live re-check keeps typedStructs.length a hard bound. Adds regression tests for both, on the fast and standalone paths. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two more issues from review: 1. [fast path] When frozen, a previously-learned key later carrying a non-null object reached the cap bailout only AFTER pack() had encoded the nested value and advanced the shared encoder position. Returning 0 then made msgpackr's plain-object fallback start at the wrong offset and emit garbage bytes. Bail before pack() so the fallback sees an untouched position. 2. new Structon(null) threw: the base Packr accepts null options as "use defaults", but reading options.maxOwnStructures dereferenced null. Use optional chaining. Adds regression tests (both paths): nested round-trips under cap, a known key later seen as an object, and null-options construction. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The earlier bail-before-pack only covered the first queued reference. With multiple queued object fields, an earlier ref can pack() (advancing msgpackr's shared write position) before a later field misses its transition — bailing there still corrupts the plain-object fallback. Track whether any ref has been packed: while none has, a frozen miss bails cleanly (return 0); once a ref is packed we can no longer bail, so finish the encode via an unfrozen forceTypeTransition (bounded overshoot of a handful of structures for that one record). The cap is still enforced up front, before the first pack(). Standalone path is unaffected (it returns fresh buffers, no shared position to corrupt). Adds a multi-queued-ref regression test on both paths. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The bail-after-pack avoidance (force-mint once a ref is packed) prevented corruption but defeated the cap for nested-object shape streams: each variant packed its first ref then force-minted the rest, growing typedStructs without bound. Add a pre-pack preflight on the fast path: walk the queued reference chain through existing transitions first, and if the cap is reached and any field would require a new structure, fall back to plain encoding (return 0) before any pack() advances the shared position. Past the preflight the chain is known, so the queued loop completes without minting (the unfrozen forceTypeTransition only covers the rare >0xff00 offset divergence — a bounded, self-converging case). Adds a nested-object-variant-stream regression test (both paths) asserting typedStructs stays within the cap. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The remaining gap: a second packed ref whose ref-section offset crosses 0xff00 needs an object32 structure variant; if it didn't exist, the post-pack force-mint appended it past the cap (converged at cap+1 per shape, but still over the cap). A single packed ref is always at offset 0 (object16) and cannot diverge, so the divergence requires >= 2 packed refs. Under the cap, the preflight now falls records with >= 2 packed (non-null object) refs back to plain encoding before any pack(). typedStructs is now a strict hard bound: it never exceeds maxOwnStructures. Adds a wide-ref (>0xff00) regression test on both paths. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two more fast-path edge cases: 1. Inline strings share the ref section, so even a SINGLE object ref can land past 0xff00 and need an object32 variant — my "only >=2 refs diverge" assumption was wrong. 2. The layout-retry (fixed section overflows the ref-start estimate) re-runs the encode after attempt 1 already packed+minted. If attempt 1 was unfrozen but the mint pushed length to the cap, the retry bailed under the now-frozen state after refs were packed → corrupt fallback. Simplify to a robust rule: under the cap, any record with a packing (non-null object/Date) ref falls back to plain encoding in the preflight, before any pack() — offsets can't be predicted pre-pack and we can't bail post-pack. Pass structureKnown=true on the retry so it re-encodes the already-minted structure instead of re-applying the cap. typedStructs stays a strict hard bound; flat records (the RaceEntry case) are unaffected. Adds regression tests: single object ref past 0xff00 via inline strings, and a layout-retry record with nested refs. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
On the standalone path, a capped miss falls back to superEncode (plain base
encoding). For bases that persist named structures (e.g. cbor-x records), that
fallback can call the user's saveStructures with only the base named array,
overwriting the combined {named, typed} payload — stranding previously written
struct data so a fresh reader can't decode it.
Re-save the combined structures after the capped fallback so the typed
structures survive (this.structures also carries any base record added). The
fast path is unaffected (its hook-based persistence already combines them).
Adds a regression test on both paths.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The freeze flag was module-scoped, so a re-entrant encode on another Structon instance (e.g. an enumerable getter that encodes with an uncapped sibling during iteration) could flip it and let a capped encoder mint past its cap — even maxOwnStructures:0 could produce typed structs. Derive the freeze state from the encoding instance's own typedStructs.length: a local `frozen` in writeStructInPlace/_encode (passed explicitly to createTypeTransition), recomputed in the standalone path after each encodeNested (which can self-mint). No shared mutable state, so cross-instance re-entrancy can't lift the cap. onLoadedStructures rebuilds with frozen=false (replaying persisted structures is never capped). Adds per-instance regression tests on both paths. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…e-read Two more enumerable-getter edge cases: 1. A getter that mints on the SAME encoder grows typedStructs after the entry-time freeze state was captured; the queued-ref preflight used that stale value and let the record mint past the cap. The preflight now uses a fresh typedStructs.length read (it runs after values, hence getters, are read). 2. A capped miss on a new key read the property value (invoking the getter) during the failed struct attempt, then the plain fallback read it again — double-running a side-effecting accessor. Resolve the key transition and bail on a frozen miss BEFORE reading the value, on both paths. Adds regression tests: accessor single-read, and a same-encoder getter mint staying within the cap. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Member
Author
|
Open items from the final (10th) Codex pass — surfaced for self-review, not yet fixed (both are edge cases that don't affect Harper's intended use: a fixed
Neither is a corruption/round-trip issue — the cap remains a strict hard bound for flat records and round-trips hold throughout the 94-test suite. — Claude (Opus 4.7) |
kriszyp
marked this pull request as ready for review
June 5, 2026 02:05
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
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
1 participant
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
Adds an opt-in
maxOwnStructurescap that bounds the per-encoder typed-structure dictionary (typedStructs) and its transition trie. Once the cap is reached, novel record shapes fall back to plain msgpack/cbor encoding instead of minting new structures; the read path is untouched, so existing and previously-persisted structs stay decodable. Default is uncapped — no behavior change unless a host opts in. Applied symmetrically to the fast (writeStructInPlace) and standalone (_encode) paths.Why
A Harper Fabric cluster (CDI) OOM-crash-looped during replication catch-up of a bulk-loaded, wide/sparse table. Root cause: the typed-struct encoder branches per field on the value's numeric width (num8/num32/num64, float32/float64) and string kind, so a structurally homogeneous table (~177 distinct key-sets) exploded into ~15.7K distinct structures — and the dictionary is pinned on the long-lived per-table encoder/decoder, growing without bound and never evicted. Multiply that ~0.6 GB/worker across one V8 isolate per worker thread (cgroup-summed) and it OOMs.
maxOwnStructuresbounds each dictionary, collapsing the whole product. Harper will set it (planned: 256); msgpackr stays uncapped by default.What to look at
struct.js,writeStructInPlace): the cap interacts subtly with msgpackr's shared write position. Once a queued nested ref ispack()ed the position is advanced and areturn 0would corrupt the plain-object fallback — so the cap is enforced before anypack()via a preflight (records that would mint under the cap fall back to plain), never after. Worth a careful read of the preflight + the layout-retry interaction (structureKnown).typedStructs.lengthand passed explicitly tocreateTypeTransition— not a shared module global — so a re-entrant encode on another instance can't lift the cap.{named, typed}structures so the base class can't overwrite them with just its named array (index.js).typedStructscan exceed the cap by a single record's worth before the next record's preflight catches it. It converges and is negligible at a cap like 256; flat records (the motivating case) are a strict hard bound.Review notes
pack()-after-bail corruption, layout-retry corruption, inline-string offset miscount, persistence overwrite, cross-instance freeze clobber, and accessor double-read. A 10th Codex pass was in flight at push time — any finding will be folded into this draft. Gemini review could not run (CLI/MCP auth failure in my environment) — a human cross-check there would be welcome.🤖 Generated by Claude (Opus 4.7). Please review the fast-path cap logic and the bounded-overshoot tradeoff in particular.