Add storage.randomAccessFields config to disable typed structures (v5.0) - #1169
Conversation
…es on v5.0 Typed random-access structures remain on by default, but a wide or variably-typed schema can grow the per-encoder structure dictionary unbounded (OOM) or hit struct-encoding edge cases. This adds a storage.randomAccessFields config (default true) so operators can opt a deployment out: when false, RecordEncoder writes classic shared structures instead of typed structs while randomAccessStructure stays on for reads, so existing typed-struct data is still readable and only new writes change. Uses msgpackr 1.12.0's readOnlyStructures option (write-disable, read-compatible). lmdb-js does not forward non-whitelisted encoder options, so the flag is derived from the global config in the RecordEncoder constructor (cold path, picks up env/CLI overrides); an explicit option still wins. Opting out exposes classic-structure code paths that typed structs masked, so this also ports the fixes that make classic-off work correctly: - noMetadata: a classic record whose structure-id byte is 66 (0x42) collides with the rocksdb local-timestamp marker; the audit store's getValue now skips the prefix heuristic for values it knows carry no timestamp. - copy-on-mutate: decoded records are frozen, so the save and source-resolve write paths copy before stamping created/updated times and the primary key. - IndexRecordEncoder: custom-index object stores (HNSW) keep writing typed structs regardless of the opt-out, since their nodes are mutated in place. 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! |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
cb1kenobi
left a comment
There was a problem hiding this comment.
Code reviewed and smoke tested. LGTM!
…sfields # Conflicts: # package.json
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Codex review Blocking conflict-resolution note: this branch is behind current Suggested shape for the RocksDB branch: this.saveStructures = function (structures, isCompatible): boolean | undefined {
if (this.isRocksDB) {
const committed = this.rootStore.transactionSync(
(txn) => {
const sharedStructuresKey = [Symbol.for('structures'), this.name];
const existingStructuresBuffer = txn.getBinarySync(sharedStructuresKey);
const existingStructures = existingStructuresBuffer ? this.decode(existingStructuresBuffer) : undefined;
if (typeof isCompatible == 'function') {
if (!isCompatible(existingStructures)) {
return false;
}
} else if (existingStructures && existingStructures.length !== isCompatible) {
return false;
}
txn.putSync(sharedStructuresKey, structures);
return true;
},
{ retryOnBusy: true }
);
if (committed === true) {
this.structureUpdate = structures;
return true;
}
return false;
} else {
const result = superSaveStructures.call(this, structures, isCompatible);
this.structureUpdate = structures;
return result;
}
}; |
| @@ -0,0 +1,179 @@ | |||
| require('../testUtils'); | |||
| const assert = require('assert'); | |||
There was a problem hiding this comment.
Small repo-standard nit: new tests should import Node's built-in assert module with the node: prefix. Either node:assert or node:assert/strict works; the important part is avoiding the bare assert package name.\n\nsuggestion\nconst assert = require('node:assert/strict');\n\n\nCodex review
heskew
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Independently confirmed the mechanism with a pure-msgpackr 1.12.0 microbench (built into the fabric-lab structure-explosion rig). Under high shape-diversity data (variable field presence — the "mostly empty" shape), 20k records, 75 fields:
| mode | structures minted | decode errors | RSS |
|---|---|---|---|
typed (default randomAccessStructure) |
~20k (1:1) | 0 | 1250 MB — the explosion |
maxOwnStructures=256 alone |
256, rest inline | 19,712 | 340 MB |
readOnlyStructures (this PR) |
~32 (recycled) | 0 | 357 MB |
So the opt-out bounds the presence case, not just width, and is cleanly better than the bare cap — which bounds memory but leaves ~19.7k records undecodable (the Record id is not defined failure). Confirms the approach end-to-end.
One thing worth a sanity check (not a blocker): the byte-66 / local-timestamp collision fix (noMetadata) is wired on the audit-store decode path — is that the only no-prefix decode path that can hit a classic record beginning with 0x42 on rocks? I couldn't find another in the diff, but it's the class of thing that bit once. (Also: branch shows CONFLICTING against v5.0 — needs a rebase before merge.)
🤖 Posted by Claude on Nathan's behalf
|
Follow-up from verifying this end-to-end — a reproducible edge, flagging in case it warrants a guard or it's expected. On 5.0.29 with My read (unconfirmed — didn't trace the exact path): Repro is in the fabric-lab struct rig ( 🤖 Posted by Claude on Nathan's behalf |
|
Following up — and agreed, this looks like a real fix, not by-design. It's not msgpackr. A pure-library microbench (msgpackr 1.12.0, It breaks in Harper's integration. Same data shape on 5.0.29 (rocks) +
The sharp clue: under Repro is in the fabric-lab struct rig: 🤖 Posted by Claude on Nathan's behalf |
Summary
Adds a
storage.randomAccessFieldsconfig (default true) that lets an operator turn typed random-access structures off on v5.0. Whenfalse,RecordEncoderwrites classic shared structures instead of typed structs, whilerandomAccessStructurestays on for reads — so existing typed-struct data remains readable and only new writes change.Purpose
Typed structures are on by default, but a wide or variably-typed schema can grow the per-encoder structure dictionary unbounded (OOM) or hit struct-encoding edge cases (the same class of issue that motivates the default-off change on main/5.1, #1152). v5.0 is released with typed structs on, so we can't flip the default — but operators hitting these problems need a documented escape hatch. This is config-only on v5.0 (no per-table directive; that's 5.1).
How it works
readOnlyStructuresoption (write-disable, read-compatible).RecordEncoderconstructor (cold path — DBI open — so it picks up env/CLI overrides). An explicitreadOnlyStructuresoption still wins (covers rocksdb-js's option spread and tests).Opting out exposes classic-structure code paths that typed structs previously masked, so this also ports the fixes that make classic-off correct (these are the v5.0 equivalents of fixes in #1152):
0x42) collides with the rocksdb local-timestamp marker; the audit store'sgetValuenow skips the prefix heuristic for values it knows carry no timestamp. (This is the MQTT "publish non-JSON → null" class of bug.)IndexRecordEncoder— custom-index object stores (HNSW) keep writing typed structs regardless of the opt-out, since their internal nodes are mutated in place and depend on struct encoding.Where to focus review
RecordEncoderreading global config directly (the coupling): chosen because lmdb-js drops non-whitelisted encoder options and nulls theencoderconfig object, so neither a store option nor the encoder config reaches the encoder on lmdb. This is engine-agnostic and correct for v5.0's global-only semantics, but it's the main design call worth a look.randomAccessStructurereader) and covered byunitTests/resources/recordEncoder.test.js(9 cases). The key real-world case — an opt-out reader decoding pre-opt-out typed data — is tested directly.isPrimaryand would otherwise inheritreadOnlyStructuresfrom the global config;IndexRecordEncoderforces structs back on for them.Testing notes
recordEncoder.test.js) pass: write-toggle (typed vs classic), opt-out read-compat, typed+classic mixing, the HNSW exemption, the 0x42/noMetadata regression, and config-driven defaulting (config false→classic, true→typed, explicit option overrides).test:unit:mainandtest:unit:resourcessuites could not be run in this worktree — they crash at setup decoding a pre-existing shared data dir (Data read, but end of buffer not reached). Confirmed this crash is environmental, not a regression: cleanorigin/v5.0(msgpackr 1.11.13, zero changes) fails identically. Relying on CI for the full suites.🤖 Generated by Claude (Opus 4.7).
Cross-model review: Codex reviewed the diff — no discrete regressions identified ("consistent with the intended opt-out behavior, preserves decoding compatibility, targeted tests for the new encoder paths"). Gemini hit its daily quota and could not review.
Merged
origin/v5.0(the harper#1154 / msgpackr#186 "Record id is not defined" fix). Conflict note for reviewers: v5.0 pins msgpackr1.11.14for the #186 save-failure rebuild; this PR keeps1.12.0, which already contains #186 (verified:pack.jshas thestructures.uninitializedrebuild path) and addsreadOnlyStructureson top — so 1.12.0 supersedes 1.11.14 and does not regress the saveStructures fix. Both fixes are present post-merge.