Write the version/metadata prefix on LMDB→RocksDB migrated records (v5.1 backport of #2014) - #2020
Write the version/metadata prefix on LMDB→RocksDB migrated records (v5.1 backport of #2014)#2020kriszyp wants to merge 4 commits into
Conversation
…rated records (v5.1 backport of #2014) Backport of #2014 (harper#2012) to the v5.1 line, which is affected from v5.1.2 (#1307 shipped the same day): every migrateOnStart run wrote prefix-less records — versions silently dropped. On v5.1 the handleLocalTimeForGets wrapper still repairs prototypes on point reads, so migrated instances look healthy until they upgrade to 5.2 and hit the prototype-loss symptom; the version loss is live on v5.1 today. Variant differences from main: no PrimaryRocksDatabase on this branch, so the read-side repair changes and primaryRocksMetadataRepair.test.js are dropped, and the migration tests verify via RecordEncoder.decode metadata wrappers instead of PrimaryRocksDatabase point reads. Write-side fix, staged+verified promotion (staging dir, atomic rename, close-all-handles, exact-header tripwire, key-exempted sweep), verifyMigratedDatabase export, and discovery exclusion are ported unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces restart-safe promotion for the LMDB to RocksDB migration by staging the database copy in a ".migrating" directory and atomically renaming it upon successful verification. It also fixes a critical issue where versioned records were incorrectly stripped of their metadata prefix during migration due to an implicit falsy check on useVersions. The review feedback correctly identifies a potential resource leak in verifyMigratedDatabase if an error occurs during the initialization of the second database handle before entering the try-finally block.
|
Reviewed; no blockers found. The latest commit (a8e59e2) closes each per-table |
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…tial-open failure Track every open in a handles array closed in finally (the copyDbToRocks pattern): a throw opening the second store leaked the first, holding the RocksDB lock on the diagnostic path operators use after a broken migration. Flagged by both review bots. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…iew on #2022) Keeps the v5.1 backport identical to main: only rootStore/dbisDb need the leak-safety array; per-table CF handles close immediately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tial-open failure Track every open in a handles array closed in finally (the copyDbToRocks pattern): a throw opening the second store leaked the first, holding the RocksDB lock on the diagnostic path operators use after a broken migration. Caught by the review bots on the v5.1 backport (#2020) of the change that introduced it (#2014). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Manual v5.1 backport of #2014 (fixes the v5.1 exposure of #2012). The v5.1 line is affected from v5.1.2 — #1307 shipped in it the same day it merged — so every
storage.migrateOnStartrun on v5.1.2–v5.1.23 wrote records without the[8-byte version][flags word]prefix, silently dropping record versions. Empirically confirmed by @heskew on 5.1.23 and by ancestry check (details).Why v5.1 instances look healthy: this branch has no
PrimaryRocksDatabase; thehandleLocalTimeForGetswrapper repairs record prototypes unconditionally on point reads, so affected instances read correctly and only lose versions (cache admission,ifVersion/CAS, replication version comparisons). The invisible-records symptom fires when they upgrade to 5.2 — so shipping this in 5.1 stops new damage, andverifyMigratedDatabaseidentifies instances needing the rewrite pass before any 5.2 upgrade.Variant differences from #2014 (main)
resources/PrimaryRocksDatabase.tsread-side changes andunitTests/resources/primaryRocksMetadataRepair.test.js— the file doesn't exist on v5.1 and the wrapper already repairs unconditionally.RecordEncoder.decodemetadata wrappers ({ value, version }) instead ofPrimaryRocksDatabasepoint reads.copyDb.tswholesale (byte-identical base pre-fix): explicituseVersions === falseopt-out in the RecordEncoder encode hook, staged migration (<db>.migrating+ atomic rename + close-all-handles + Windows rename retry), exact-header tripwire, key-exempted full verification sweep,verifyMigratedDatabaseexport, discovery exclusion of*.migratingdirs,MIGRATING_DIR_SUFFIXin hdbTerms.Tests
unitTests/bin/migration*.test.js: 7 passing underHARPER_STORAGE_ENGINE=lmdb, 7 pending (visible skip) under rocks; full lmdb bin suite 69 passing locally. Note: local run used a newer rocksdb-js than the branch lockfile (shared node_modules) — CI arbitrates against the real pin.All review findings from #2014 (Codex tripwire ambiguity, kriszyp staging restart-safety, heskew verification gaps, cb1kenobi sweep byte-sniffing) are incorporated since this ports the final state.
🤖 Generated with Claude Code (Claude Fable 5)