Skip to content

Add storage.randomAccessFields config to disable typed structures (v5.0) - #1169

Merged
kriszyp merged 2 commits into
v5.0from
kris/v5.0-randomaccessfields
Jun 8, 2026
Merged

Add storage.randomAccessFields config to disable typed structures (v5.0)#1169
kriszyp merged 2 commits into
v5.0from
kris/v5.0-randomaccessfields

Conversation

@kriszyp

@kriszyp kriszyp commented Jun 8, 2026

Copy link
Copy Markdown
Member

Summary

Adds a storage.randomAccessFields config (default true) that lets an operator turn typed random-access structures off on v5.0. When false, RecordEncoder writes classic shared structures instead of typed structs, while randomAccessStructure stays 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

  • Uses msgpackr 1.12.0's new 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 — DBI open — so it picks up env/CLI overrides). An explicit readOnlyStructures option 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):

  • 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. (This is the MQTT "publish non-JSON → null" class of bug.)
  • 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 internal nodes are mutated in place and depend on struct encoding.

Where to focus review

  • RecordEncoder reading global config directly (the coupling): chosen because lmdb-js drops non-whitelisted encoder options and nulls the encoder config 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.
  • Read-compatibility guarantee: verified empirically (typed + classic round-trip on a shared structures store with a randomAccessStructure reader) and covered by unitTests/resources/recordEncoder.test.js (9 cases). The key real-world case — an opt-out reader decoding pre-opt-out typed data — is tested directly.
  • HNSW exemption: object-store indexes are opened as isPrimary and would otherwise inherit readOnlyStructures from the global config; IndexRecordEncoder forces structs back on for them.

Testing notes

  • New unit tests (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).
  • The local test:unit:main and test:unit:resources suites 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: clean origin/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 msgpackr 1.11.14 for the #186 save-failure rebuild; this PR keeps 1.12.0, which already contains #186 (verified: pack.js has the structures.uninitialized rebuild path) and adds readOnlyStructures on top — so 1.12.0 supersedes 1.11.14 and does not regress the saveStructures fix. Both fixes are present post-merge.

…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>
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@kriszyp
kriszyp marked this pull request as ready for review June 8, 2026 20:52
@kriszyp
kriszyp requested a review from a team as a code owner June 8, 2026 20:52
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@cb1kenobi cb1kenobi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code reviewed and smoke tested. LGTM!

@socket-security

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Updatedmsgpackr@​1.11.14 ⏵ 1.12.0100 +2100100 +1100 +7100

View full report

@kriszyp
kriszyp merged commit 68aa9b8 into v5.0 Jun 8, 2026
17 of 22 checks passed
@kriszyp
kriszyp deleted the kris/v5.0-randomaccessfields branch June 8, 2026 21:41
@heskew

heskew commented Jun 8, 2026

Copy link
Copy Markdown
Member

Codex review

Blocking conflict-resolution note: this branch is behind current v5.0 and conflicts in resources/RecordEncoder.ts. When resolving, please keep the base saveStructures fix from 15a5e6a6 / 38a96c4b and layer the new readOnlyStructures config on top. The PR branch currently still has the older RocksDB path that returns transactionSync(...) directly and sets structureUpdate inside the callback; taking that side would reintroduce the path where a failed/aborted structure save can look successful and a later record can reference a structure that was never durably saved.

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');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 heskew left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@heskew

heskew commented Jun 8, 2026

Copy link
Copy Markdown
Member

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 storage.randomAccessFields=false: a declared-schema table is perfectly clean — bounded memory, all records readable, 0 decode errors. But the same run on a table with high-cardinality undeclared dynamic attributes leaves records unreadable — inserts succeed, reads return empty, and the log fills with Record id is not defined for N. It reproduces even at low shape counts and persists despite the #1157 / msgpackr #186 save-race fix. Controlled A/B (same image + data, only the schema declared vs not): declared = clean, undeclared = broken.

My read (unconfirmed — didn't trace the exact path): readOnlyStructures writes references to classic structures it never durably saves for the dynamic-attribute shapes, so a fresh reader can't resolve them. Likely outside this opt-out's intended declared-schema use — and CDI's schema is declared, so it's unaffected — but since the config is pitched for "wide/variably-typed schemas," a dynamic-attribute table could land here and silently lose readability.

Repro is in the fabric-lab struct rig (RAF=false on the undeclared Doc table). Expected, or worth a guard?

🤖 Posted by Claude on Nathan's behalf

@heskew

heskew commented Jun 8, 2026

Copy link
Copy Markdown
Member

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, readOnlyStructures: true, 20k high-diversity records) is clean: dictionary bounded to ~32, 0 decode errors — it inlines the unknown shapes. So the option itself handles highly-variable shapes fine in-process.

It breaks in Harper's integration. Same data shape on 5.0.29 (rocks) + randomAccessFields=false:

  • declared schema → clean, all records readable, 0 errors
  • only the PK declared + high-cardinality dynamic attributes → records unreadable, logs full of Record id is not defined for 5312

The sharp clue: under readOnlyStructures the dictionary should stay tiny (~32, per the microbench), yet records are referencing structure id 5312 that isn't in the durable store. So for dynamic shapes the encoder is still assigning/writing high structure-id references that never get durably persisted — so a fresh reader/worker can't resolve them. With a declared schema the structures are established up front and persist, so no dangling refs. Unconfirmed read: the rocks structure-save path under the opt-out doesn't persist the dynamically-minted structures → exactly your "highly variable record shapes that don't persist."

Repro is in the fabric-lab struct rig: RAF=false on a table with only the PK declared + high-cardinality dynamic attributes. Happy to dig the RecordEncoder save/getStructures path against rocks for a pinpoint if that'd help.

🤖 Posted by Claude on Nathan's behalf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants