fix(migration): preserve records with file-backed blobs during v4→v5 migration#870
Conversation
![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 addresses an issue during the LMDB to RocksDB migration where records containing file-backed blobs were silently dropped because the primary store's encoder lacked a reference to the root store. The fix explicitly assigns rootStore to the primary store's encoder during migration. A regression test has been added to verify this behavior. The review feedback highlights that createBlob is used in the new test file without being imported, which will cause a runtime error, and notes that redundant await keywords are used on synchronous createBlob calls.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… migration copyDbToRocks opened the source LMDB primary dbi with a RecordEncoder but never set its rootStore. Decoding any record holding a file-backed blob reference then threw "No store specified, cannot load blob from storage"; the error was swallowed (record decoded to null) and the record was silently skipped, dropping every record with a file-backed blob on upgrade (#857). Set sourceDbi.encoder.rootStore = sourceRootStore so blob references resolve against the source store during migration. Adds a regression test and documents the requirement in DESIGN.md alongside the existing source-dbi compression note. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Reviewed; no blockers found. Prior finding (regression test not wired into CI) addressed by commit |
Pre-existing repo-wide prettier drift (a long require line on main, from d28a16f) that fails the PR-only Format Check on every new PR. `npm run format:write` produces exactly this; unrelated to the migration fix. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
kriszyp
force-pushed
the
kris/857-blob-migration-rootstore
branch
from
5536dce to
e84cb1a
Compare
…xecutes The blob-migration regression test (and copyDB.test.js) require an LMDB source and self-skip unless HARPER_STORAGE_ENGINE=lmdb. test:unit:lmdb only covered resources + apitests, so the bin suite never ran under lmdb and the new test silently no-opped in CI. Append the bin suite to test:unit:lmdb (50 bin tests pass under lmdb locally). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Patch cherry-pick: conflictCherry-pick onto The conflict markers are committed on branch |
1 participant
Summary
During the v4 (LMDB) → v5 (RocksDB) startup migration,
copyDbToRocksopened each source LMDB primary dbi with aRecordEncoderbut never set itsrootStore. Decoding any record holding a file-backed blob reference then ran with no store context, so theBlobmsgpackr extension threwNo store specified, cannot load blob from storage.RecordEncoder.decodeswallows that error and returnsnull, andcopyDbToRocksskipsnullvalues — so every record containing a file-backed blob was silently dropped on upgrade (small/inlined blobs and blob-less records were unaffected).Fix: set
sourceDbi.encoder.rootStore = sourceRootStorefor the primary source dbi so blob references resolve against the source store during migration (the runtime read path gets this viahandleLocalTimeForGets; the migration opens the source dbi raw).Resolves #857.
Changes
bin/copyDb.ts: setrootStoreon the source primary encoder; exportcopyDbToRocksso the regression test can drive it directly.unitTests/bin/blobMigration.test.js: new regression test (LMDB engine) — migrates a file-backed blob, an inlined blob, and a no-blob record, asserts all survive and the blob content is intact.DESIGN.md: documents therootStorerequirement next to the existing source-dbicompressionnote.Where to look
if (isPrimary && sourceDbi.encoder) sourceDbi.encoder.rootStore = sourceRootStore;. ConfirmrootStoreis the correct store for resolving source blob references (it mirrors whathandleLocalTimeForGetsassigns at runtime).copyDbcompaction copies raw bytes (decoder/encodernulled), so neither needs this. The fix is intentionally guarded toisPrimary.Notes
<hdbBasePath>/blobs/<databaseName>path and the samefileIdis re-emitted). It does not decode the migrated record back through a v5 RocksDB store to confirm the reference resolves end-to-end — that full read-through belongs in theintegrationTests/upgradepath. Flagging in case you'd prefer a stronger unit assertion.