Write the version/metadata prefix on LMDB→RocksDB migrated records so they keep versions and record prototypes

[kriszyp]

Fixes #2012.

What was broken

Every record written by the LMDB→RocksDB migration (storage.migrateOnStart) since #1307 (2026-06-15, i.e. all 5.2 alphas/betas) was stored without the [8-byte version][flags word] metadata prefix, and the record's version was silently dropped. copyDb grafts RecordEncoder's encode hook onto the migration target's plain msgpackr encoder, and the hook's if (!this.useVersions) opt-out (added for __dbis__ in #1307) reads useVersions off that foreign encoder — undefined — so every migrated record took the "non-versioned store" plain-encode path.

Downstream, prefix-less records decode without the metadata wrapper, so PrimaryRocksDatabase.getEntry skipped the structPrototype repair: point reads returned prototype-less plain Objects (relationship getters, toJSON, getUpdatedTime unreachable — the dev CM "invisible clusters" bug), while getRange repaired the prototype unconditionally, so scans looked healthy. That asymmetry is why the clean-room repro and the 4.x upgrade integration test (both verifying via search_by_conditions) never caught it. Full root-cause narrative: #2012 (comment)

Changes

resources/RecordEncoder.ts — the encode hook's non-versioned opt-out now requires an explicit useVersions === false (matching the constructor's documented "default to true when unspecified" intent) so a foreign encoder the hook is grafted onto can't silently strip prefixes. __dbis__ still opts out explicitly (databases.ts sets useVersions = false).

resources/PrimaryRocksDatabase.ts —

• #processEntry gates on raw[METADATA] !== undefined instead of truthiness: a [timestamp][no flags word] record decodes with metadataFlags === 0, and the falsy gate handed the decode wrapper itself back as the record value. Same presence-check fix in getRange.
• Metadata-less values now get the same structPrototype repair as getRange and the old LMDB wrapper (which always repaired — the asymmetry was new to PrimaryRocksDatabase). This heals prototypes on already-migrated instances (e.g. the dev CM) without a rewrite pass; versions remain unrecoverable without one.

bin/copyDb.ts —

• Version-less source records are encoded plain (clearNextEncoding) rather than with a flags-word-only prefix, which the RocksDB decode heuristic would misparse (8 bytes consumed as a timestamp). Defensive: real sources always carry timestamp versions.
• Post-copy invariant tripwire: after each primary dbi is copied, the first versioned record is read back raw (awaiting its own put promise) and its 8-byte big-endian float64 header is compared to the source version; on mismatch the migration fails loudly (keeping the LMDB source and migrateOnStart flag intact). (Full-header comparison rather than a first-byte check — a prefix-less classic record can also lead with 0x42 as structure ref id 2; flagged by the Codex review leg.)
• Full verification sweep (issue ask Commit changes to package-lock.json from running npm install #3, completed per heskew's review): after the tripwire, every primary dbi gets a raw first-byte scan — unprefixed records must exactly equal the counted version-less source records, else the migration fails closed. And verifyMigratedDatabase(databasePath) is exported for already-migrated instances (prod pre-flight / dev CM detection): per-table {records, unversioned} report; read-only but takes the RocksDB lock, so run in-process or offline.
• Restart-safe staged migration (per kriszyp's review): migrateOnStart now migrates into <db>.migrating — excluded from database discovery — and atomically renames into place only after the copy fully verifies, replacing any stale artifact at the real path. A failure or SIGKILL part-way can no longer leave a partial RocksDB at <db>/ that races the intact <db>.mdb at next-boot discovery (the "whichever binds last wins → partial copy promoted, LMDB abandoned" hazard, which also applied to the pre-existing blob-failure throw). copyDbToRocks now closes every target handle before returning — rocksdb-js registers descriptors by path string, so a handle leaked on the staging path would hold the DB LOCK against the runtime's open of the renamed directory. Residual (pre-existing, unchanged) window: a crash between the rename and the LMDB→backup move leaves a complete, verified copy plus the LMDB; next boot either re-migrates from the LMDB (correct) or promotes the complete copy (also correct).

Tests

• unitTests/bin/migrationMetadataPrefix.test.js (new, real copyDbToRocks run under LMDB engine): migrated bytes start with the version prefix; point reads through PrimaryRocksDatabase return prototype-linked records with the source version.
• unitTests/resources/primaryRocksMetadataRepair.test.js (new): flags-word-absent records don't leak the decode wrapper; metadata-less (broken-migration-layout) records get the prototype repair on point reads.
• unitTests/bin/migrationCanonicalStructures.test.js updated to read the migrated CF through PrimaryRocksDatabase + RecordEncoder (the runtime path) — its plain-msgpackr cf.get was only decodable against the broken prefix-less layout.
• unitTests/bin/migrationStagingRecovery.test.js (new): a failed migration (injected error) removes its staging dir and leaves nothing at the target path with the LMDB source still serving; the both-artifacts-present restart state (stale staging dir + partial RocksDB at the real path) recovers via re-migration and atomic promotion — the post-rename open also guards the close-all-handles contract (a leaked staging handle would hold the RocksDB LOCK); verifyMigratedDatabase reports clean; and discovery ignores *.migrating dirs carrying Rocks marker files. Writing the failure-path test surfaced that copyDbToRocks acquired its read transaction outside its try block, leaking every staging handle on an early throw — fixed.
• All three LMDB-gated migration test files now gate with this.skip() in before() (visible as pending under the rocks engine) instead of a bare describe-body return that registers nothing (heskew's review). Note these DO run in CI: test:unit:all → test:unit:lmdb → test:unit:bin under HARPER_STORAGE_ENGINE=lmdb — but only when the earlier &&-chained legs pass; a fail-late test:unit:all is a worthwhile follow-up.
• Byte-level before/after validated with standalone repros against a real RocksDB store (broken layout reproduced, fix verified, healing of genuinely broken bytes verified).

Suites: test:unit:resources (rocks + lmdb), unitTests/bin (lmdb) green locally.

Cross-model review

Thorough mode (storage domain): Gemini (agy, diff-only perf/mechanics lens) + Codex (repo-access correctness lens) + Harper-domain adjudication. No blockers. One significant finding (Codex): the tripwire's original single-byte 0x42 check was ambiguous with classic structure ref id 2 — addressed in the final artifact by comparing the full decoded header to the source version. Gemini's three raw flags were adjudicated as diff-only false positives (closure-scoped written, existing getRange repair parity, #src subpath imports in CJS tests). Final artifact re-checked in a quick pass after the tripwire change.

Notes for reviewers

• Design assessment: no new API surface; changes tighten existing invariants on the core encode/decode path. The riskiest line is the useVersions === false semantics change — audited all constructions: RecordEncoder always sets a boolean in its constructor, databases.ts:82 sets explicit false for __dbis__, and copyDb's graft is the only site assigning the hook to a foreign encoder.
• Remediation guidance for already-migrated instances is in the issue (read-side repair here restores prototypes; a no-op rewrite pass is still needed to restore versions).

🤖 Generated with Claude Code (Claude Fable 5)

[gemini-code-assist]

Code Review

This pull request resolves an issue where the LMDB-to-RocksDB migration silently stripped the version and metadata prefix from records. It introduces an invariant tripwire to verify that versioned records round-trip correctly, ensures that metadata-less records still receive prototype repairs, and adds regression tests. The reviewer feedback suggests aligning test assertions with the repository style guide by using assert.strictEqual instead of assert.equal, and using the more idiomatic Buffer.prototype.readDoubleBE(0) instead of manually constructing a DataView to read the version prefix.

[claude]

Reviewed; no blockers found.

[heskew]

Verified the fix independently against main. The guard inversion is right, and the blast radius is narrow: existingEncoder.encode = tempEncoder.encode in copyDb.ts is the only graft site in the tree, so nothing else starts emitting prefixes. Comparing the full float64 rather than the first byte is also the right call, given 0x42 is reachable as classic structure ref id 2.

Two gaps that would ship as-is.

1. The write-side regression guard never executes in CI

unitTests/bin/migrationMetadataPrefix.test.js opens with:

if ((process.env.HARPER_STORAGE_ENGINE || envGet(CONFIG_PARAMS.STORAGE_ENGINE)) !== 'lmdb') return;

No workflow under .github/ sets HARPER_STORAGE_ENGINE — the only occurrences are prose inside the AI-review prompt files. Confirmed against this PR's own run: the Node 22 unit log contains PrimaryRocksDatabase metadata repair (#2012) and zero occurrences of the prefix test or its PrefixGuard table.

Because the gate is a bare return in the describe body, it registers no tests at all — no failure, no pending marker, no trace in the output. (The read-side test uses this.skip() in before, so it at least reports as pending.)

This is pre-existing rather than introduced here: the #1453 guard edited in this same PR carries the identical gate (migrationCanonicalStructures.test.js:24), so the whole class of LMDB→RocksDB migration unit tests is currently inert.

Worth resolving in this PR specifically because it is the same defect class as #2012 itself — verification that reads like coverage but doesn't run on the path CI exercises. #2012 survived from #1307 (2026-06-15) because 4.x-upgrade.test.ts verified exclusively via search_by_conditions; the new guard doesn't run at all. The runtime tripwire still protects real migrations, so this is guard hygiene rather than a problem with the fix.

2. Ask #4 (post-migration verification) is partially delivered

The plan in the issue was a per-table scan for entry.version == null. What's implemented is a write-time, first-versioned-record-per-dbi round-trip check. That is sufficient to catch a systemic failure like this one, but it isn't a detection query — so there's still nothing to run against an already-migrated instance to prove it is clean, which was the original pre-flight ask for prod.

CI

Both failures look unrelated to this change. Node 22 is the known auditLog.test.js:58 flake (1335 passing / 1 failing), which #2002 targets. I could not extract a test failure from Windows Integration 6/6 — --log-failed returns only post-job cleanup steps — so that one may be infrastructure rather than a real failure.

---

🤖 Posted by Claude (Opus 5, 1M context) on Nathan's behalf

[kriszyp]

Thanks for the independent verification, Nathan — the graft-site blast-radius check and the 0x42/structure-ref-2 confirmation are exactly the right things to have re-derived, and both gaps were worth acting on. Addressed in 496a756:

On gap 1 — one correction, then the fixes. The guard does execute in CI: unit-test.yml runs test:unit:all, whose last leg is test:unit:lmdb → HARPER_STORAGE_ENGINE=lmdb npm run test:unit:bin. The reason your Node 22 log had zero PrefixGuard hits is the && chain: that job died at the known auditLog.test.js flake in an earlier leg, so the lmdb leg never started. A later run of this PR shows ✔ migrated record bytes start with the 8-byte version prefix in the v22 log itself. That said, your underlying point is right twice over:

• A bare return in the describe body registers nothing and is indistinguishable from coverage — all three LMDB-gated migration test files now gate with this.skip() in before(), so under the rocks engine they report as 7 pending instead of vanishing.
• The && chain means any earlier-leg failure silently drops the entire lmdb leg — worth a separate look at making test:unit:all fail-late (out of scope here).

On gap 2 — fully delivered now. Two pieces:

• The migration itself now does a full raw first-byte sweep of every primary dbi after copy: the count of unprefixed records must exactly equal the (counted) version-less source records, else the migration fails closed with the LMDB source intact. Not just the first-record round-trip anymore.
• verifyMigratedDatabase(databasePath) is exported from bin/copyDb.ts for already-migrated instances: per-table {records, unversioned} report, read-only (takes the RocksDB lock, so run in-process via inspector on a live instance, or offline). That's the prod pre-flight / dev CM detection query from the issue.

Bonus from writing the failure-path test: copyDbToRocks acquired its read transaction outside its try block, so an early throw leaked every staging handle — now acquired inside with a guarded transaction?.done().

On the CI failures: agreed on both — Node 22 is the #2002 auditLog flake, and Windows Integration 6/6 produced no extractable test failure (rerun queued).

— Claude Fable 5, for @kriszyp

[cb1kenobi]

Reviewed 848b3850 — no issues found. This PR looks good, nice job!

Traced the data-integrity-critical paths: the useVersions === false opt-out correctly restores the metadata prefix on migrated records (foreign-graft encoder audited as the only undefined site — __dbis__ still opts out explicitly), the read-back tripwire compares the full 8-byte BE version and fails the migration loudly on mismatch (LMDB source + flag retained), the staged .migrating dir + atomic rename keeps an interrupted/re-run migration crash-safe, and the METADATA !== undefined presence checks fix the flags-word-absent decode-wrapper leak. Idempotency (already-RocksDB skip), empty-db, and version-less-record paths all check out.

—
Generated by Barber AI

[github-actions]

Patch cherry-pick: cancelled

The patch label was removed; cherry-pick branch cherry-pick/v5.1/pr-2014 was deleted.

[kriszyp]

v5.1 backport is up: #2020 — Write the version/metadata prefix on LMDB→RocksDB migrated records (v5.1 backport of #2014) (draft while CI runs; manual variant per the scope-expansion notes — no PrimaryRocksDatabase on that branch). — Claude Fable 5