Make typed random-access structures opt-in (default off) in 5.1 - #1152
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request modifies RecordEncoder.ts to disable writes for typed random-access structures by default unless explicitly opted in via options.randomAccessStructure. This is achieved by overriding _writeStruct with a no-op function () => 0 to prevent potential OOM issues and decoding failures across replicas, while still preserving read capabilities. There are no review comments, and I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
kriszyp
force-pushed
the
kris/typed-structs-default-off
branch
from
June 8, 2026 12:10
25fdb40 to
8880de3
Compare
kriszyp
force-pushed
the
kris/typed-structs-default-off
branch
2 times, most recently
from
June 8, 2026 16:23
d24cc8d to
29a6c97
Compare
… directive) main already gates struct-mode writes to primary DBIs (a2d0faf, for v4-downgrade compat). This makes the primary-DBI default itself controllable: - storage.randomAccessFields config (default off) — global default for primary stores - @table(randomAccessFields: true) directive — per-table override, persisted at creation (like sealed/compression) - OpenDBIObject: primary randomAccessStructure now follows the config (isPrimary && RANDOM_ACCESS_FIELDS); non-primary stays off (v4-downgrade compat unchanged) Typed structures key on per-field value WIDTH, so wide/variably-typed schemas mint an unbounded per-encoder dictionary (OOM) and diverge across replicas (decode failures); defaulting them off for primary stores is the conservative choice, with opt-in where safe. Also keep custom-index object stores (e.g. HNSW vector graphs) in struct mode regardless of the config — their internal node shapes (numeric-keyed per-level connection arrays and quantized bins) are fixed and rely on struct encoding, so the table-level default-off would corrupt the graph. Adds unit coverage for the config + directive wiring (databases.test.js, randomAccessFieldsDirective.test.js). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
kriszyp
force-pushed
the
kris/typed-structs-default-off
branch
from
June 8, 2026 16:54
29a6c97 to
65fad22
Compare
…te paths
With typed structures disabled (storage.randomAccessFields off), decoded records
are classic-encoded and frozen by freezeData. Several write paths stamped
created/updated times and the primary key onto the record in place, which throws
("Cannot assign to read only property") when the record is frozen — e.g. when a
record decoded during transaction-log replay is re-saved. This surfaced broadly in
integration tests under the new default-off.
Records are intentionally immutable (5.2 record caching relies on it), so the fix
is copy-on-mutate: shallow-copy a frozen record before stamping it — in the table
save validate callback and the source/caching resolve write.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
kriszyp
force-pushed
the
kris/typed-structs-default-off
branch
from
June 8, 2026 19:22
513db15 to
e9786f6
Compare
… collision) When typed random-access structures are off, records use classic shared structures whose first byte can be 66 (0x42 = structure-id #2). RecordEncoder's rocksdb metadata heuristic treats a leading 66 as a local-timestamp prefix and strips 8 bytes, corrupting the record (decoded as null). The audit store's getValue decodes a value that carries no on-disk timestamp prefix, so it now passes { noMetadata: true } to skip the heuristic entirely. Surfaced by the MQTT "can publish non-JSON" path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The directive-based tests stamp randomAccessStructure in databases.ts before the store opens, bypassing the OpenDBIObject constructor's global-config branch. Add a focused test that opens a primary/non-primary DBI without a directive and asserts the constructor reads storage.randomAccessFields correctly (true -> on, false -> off, non-primary -> off). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The first version of this test set storage.randomAccessFields via envMngr.setProperty, which passed in isolation but failed in the full unit suite (CI) — config state isn't reliably isolated across the suite, so the constructor read a stale false. Stub envMngr.get per-test (restored in afterEach) so the assertion is deterministic regardless of suite ordering; callThrough keeps other config reads real and asserts the correct key is used. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Another test in the full unit suite leaves envMngr.get wrapped by sinon and
never restores it, which broke both prior versions of this test in CI: the
setProperty version read the leaked stub's value instead of the set value, and
the sinon.stub version threw "already wrapped". Override the getter via a
defineProperty save/replace/restore that delegates to whatever get currently is
(real or another test's stub) and restores the exact prior descriptor — proven
against a simulated leak locally. Adds a non-boolean ("true" string) case to
guard the strict === true comparison.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
kriszyp
marked this pull request as ready for review
June 8, 2026 23:18
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
cb1kenobi
approved these changes
Jun 9, 2026
This was referenced Jun 9, 2026
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
2 participants
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
Builds on main's
a2d0fafa7(which gates struct-mode writes to primary DBIs for v4-downgrade compat) to make the primary-DBI default itself controllable, so typed random-access structures can be turned off by default and opted into where safe:storage.randomAccessFieldsconfig (default false) — global default for primary stores.@table(randomAccessFields: true)GraphQL directive — per-table override, persisted at table creation (likesealed/compression).OpenDBIObject: primaryrandomAccessStructurenow follows the config (isPrimary && RANDOM_ACCESS_FIELDS); non-primary stays off (a2d0faf's v4-downgrade behavior unchanged).Why
Typed structures key the per-encoder dictionary on per-field value width, which on CDI produced (1) unbounded structure explosion → OOM on wide/variably-typed schemas, and (2) cross-replica dictionary divergence → decode failures. Classic shared structures are bounded and width-agnostic, so defaulting primary stores off is the conservative choice.
What to look at
databases.ts— two pieces: (a) the per-table override flowsrandomAccessFieldsfrom the table definition →primaryKeyAttribute(persisted) →dbiInit.randomAccessStructureat both primary-store open paths; (b)openIndexkeeps custom-index object stores (HNSW vector graphs) in struct mode regardless of the config — their internal node shapes (numeric-keyed per-level connection arrays, quantized int8 bins) rely on struct encoding, so the table-level default-off would corrupt the graph. This is the key correctness point: verified that without it, both float and int8 vector-index tests fail.graphql.ts— Boolean coercion handling a real booleanfalse, the string"false", and absence.OpenDBIObject.ts— theisPrimary && RANDOM_ACCESS_FIELDSreconciliation.No change to
RecordEncoder.ts(main's a2d0faf already has the_writeStructbail) and no change to the HNSW source.Tests
unitTests/resources/models/randomAccessFieldsDirective.test.js— directive parse → encoder wiring (true / false / absent).unitTests/resources/databases.test.js—table()directive wiring.recordEncoder.test.js(gating) and the fullvectorIndex.test.jssuite (incl. int8) pass on this branch. Fulltest:unit:resources: 698 passing, 0 failing.Review
🤖 Generated by Claude (Opus 4.7). Rebased onto current main; scope narrowed to config + directive + the object-store-index struct-mode guard.