fix(storage): translate the LMDB compression descriptor for RocksDB stores - #2038
fix(storage): translate the LMDB compression descriptor for RocksDB stores#2038kriszyp wants to merge 1 commit into
Conversation
…tores
Harper's `storage.compression` - and the `compression` descriptor persisted on a
table's primary attribute - describe LMDB's per-value compression:
`{ startingOffset, threshold, dictionary }`. That value was passed straight
through to every RocksDB store open. rocksdb-js ignored it until 2.6.0, which
gives `compression` a real meaning (block/blob algorithm) and validates it. 2.6.1
accepts the descriptor object as "unset", but an unset `storage.compression`
resolves to `''` and throws `Unsupported compression algorithm ""`, and the
booleans throw as a malformed option. Every table open throws, taking component
loading and the status table down with it.
Translate at openRocksDatabase, the single point every RocksDB store opens, so
re-opens of existing tables - whose persisted descriptor is LMDB-shaped - are
covered along with new ones. `''`, `true` and the LMDB descriptor leave the
option unset and take rocksdb-js's own default of lz4 (naming lz4 explicitly
would throw on a build without it); `storage.compression: false` maps to 'none';
an algorithm name or { algorithm, level } passes through.
The 2.6 binaries link lz4/zstd/bzip2 for the first time - 2.5.0 exported no
`supportedCompression` and its binary carries no LZ4_compress/ZSTD_compress - so
RocksDB stores were uncompressed until now. `false` therefore preserves today's
behavior and the default (lz4) is the change worth a release note.
main pinned `^2.5.0`, which already resolved 2.6.1 for the Docker image, which
installs from a tarball and so resolves fresh from package.json rather than the
published shrinkwrap. Pin `~2.6.1` so the dependency and the translation move
together.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Code Review
This pull request updates the @harperfast/rocksdb-js dependency to ~2.6.1 and introduces the resolveRocksCompression helper to normalize and translate compression configurations (such as LMDB-shaped descriptors, booleans, and empty strings) before they reach the RocksDB store. It also fixes a potential strict-mode error in initStores when assigning properties to string primitives. The feedback suggests simplifying string trimming in resolveRocksCompression, removing redundant as any type casts now that the options are fully typed, and refining the object checks in initStores to be more defensive against RocksDB descriptors.
| if (typeof compression === 'string' && compression.trim() === '') return undefined; | ||
| // already RocksDB-shaped ('lz4', { algorithm: 'zstd', level: 3 }, ...): pass through | ||
| if (typeof compression === 'string') return compression; |
There was a problem hiding this comment.
We can simplify the string handling by trimming the string once. This not only combines the empty-string check and the pass-through check, but also robustly handles any leading/trailing whitespace in valid algorithm names (e.g., ' lz4 ' becomes 'lz4').
| if (typeof compression === 'string' && compression.trim() === '') return undefined; | |
| // already RocksDB-shaped ('lz4', { algorithm: 'zstd', level: 3 }, ...): pass through | |
| if (typeof compression === 'string') return compression; | |
| if (typeof compression === 'string') { | |
| const trimmed = compression.trim(); | |
| return trimmed === '' ? undefined : trimmed; | |
| } |
|
|
||
| function openRocksDatabase(path: string, options: RocksDatabaseOptions & { dupSort?: boolean }) { | ||
| options.disableWAL ??= true; | ||
| (options as any).compression = resolveRocksCompression((options as any).compression); |
There was a problem hiding this comment.
Since @harperfast/rocksdb-js has been upgraded to ~2.6.1, the compression option is now officially supported and typed in RocksDatabaseOptions. We can remove the redundant as any type casts here to improve type safety.
| (options as any).compression = resolveRocksCompression((options as any).compression); | |
| options.compression = resolveRocksCompression(options.compression); |
References
- Avoid redundant type casting (such as casting 'this' to 'any') when accessing static properties that are already declared on the class, as they are fully typed within static methods. Only use type casting when accessing properties that are not declared on the class.
| if (dbiInit.compression) { | ||
| // threshold only applies to the LMDB descriptor shape; a RocksDB algorithm name has no | ||
| // threshold, and assigning a property to a string primitive throws under strict mode | ||
| if (dbiInit.compression && typeof dbiInit.compression === 'object') { |
There was a problem hiding this comment.
While dbiInit.compression is currently expected to be an LMDB descriptor, once RocksDB compression descriptors (e.g., { algorithm: 'zstd', level: 3 }) are fully wired through, they will also be objects. To prevent accidentally injecting the LMDB-specific threshold property into a RocksDB descriptor object, we should explicitly guard this check by ensuring the object does not contain an algorithm property. Additionally, use an explicit != null check instead of a simple truthiness check to prevent falsiness bugs.
| if (dbiInit.compression && typeof dbiInit.compression === 'object') { | |
| if (dbiInit.compression != null && typeof dbiInit.compression === 'object' && !('algorithm' in dbiInit.compression)) { |
References
- Use explicit
!= nullchecks instead of simple truthiness checks to prevent falsiness bugs (e.g., when a numeric value can be0) and to make the code clearer and more defensive for direct callers.
|
Reviewed; no blockers found. |
What broke
rocksdb-js 2.6.0 gave the
compressionstore option a real meaning — a RocksDB block/blob algorithm — and validates it. Harper has been passing an unrelated, LMDB-eracompressionvalue through the same options object:getDefaultCompression()returns msgpackr's{ startingOffset, threshold, dictionary }descriptor, or the raw config value when falsy. 2.5.0 never readoptions.compressionat all, so it was silently ignored. Now it's parsed.On 2.6.1 an unset
storage.compressionresolves to''and throws:Every RocksDB store open throws, which takes component loading and the status table down with it — visible as the three
componentLoader.test.jsfailures on #2036.Why this is a main-branch bug, not a bump-branch bug
main pinned
^2.5.0, which already satisfies 2.6.1 (published 2026-07-31). CI is green only becausenpm ciuses the lockfile.The published shrinkwrap does protect registry installs —
npm-shrinkwrap.jsonships in the tarball and npm honors it via_hasShrinkwrap. But the Docker image does not go through that path:Dockerfile:55runsnpm install --ignore-scripts --global harper-*.tgz, and a tarball install has no packument, so npm never learns the shrinkwrap exists and resolves fresh frompackage.json. That gap is already documented independencies.md. So a release cut from main today would ship an image resolving^2.5.0→ 2.6.1 → broken on every table open.~2.6.1closes the range as well as fixing the incompatibility.The fix
Translate at
openRocksDatabase, the single point every RocksDB store opens — so re-opens of existing tables, whose persisted descriptor is LMDB-shaped, are covered along with new ones.''(unset config),true, LMDB descriptorfalse'none''zstd',{ algorithm, level }Compression-on leaves the option unset rather than naming
lz4, because an explicitlz4would throw on a build without it.Behavior change worth a release note
The 2.6 binaries link lz4/zstd/bzip2 for the first time. Verified against the shipped natives:
LZ4_compressZSTD_compressBZ2_bzCompress2.5.0 also exported no
supportedCompression. So RocksDB stores were uncompressed until now. That meansstorage.compression: falsepreserves today's behavior, and the notable change is the default turning compression on (lz4) — a disk win, but it changes on-disk format and CPU profile for new/compacted files. Existing SST/blob data stays readable.Testing
test:unit:main— 4221 passingtest:unit:resources— 1354 passingtest:integration:all— clean (exit 0)resolveRocksCompressioncases for'',false,true, the LMDB descriptor,{algorithm, level}, and a never-throws sweepThe table-level test alone was not sufficient and this is worth knowing when reading the diff: 2.6.1 accepts an object with no
algorithmas "unset", so the default config (which yields the descriptor) exercises the one input that never failed — that test passes with or without the fix. The inputs that actually throw are''and the booleans, and only a direct test reaches them. Caught by the cross-model review; the resolver is exported for that reason.Open items for the reviewer
storage.compression: zstdis silently downgraded to lz4.getDefaultCompression()returnsLMDB_COMPRESSION && LMDB_COMPRESSION_OPTS, so any truthy value — including an algorithm name — is discarded in favor of the LMDB options object. The string pass-through branch here is therefore unreachable from config today. Wiring it through is a new user-facing capability (needs docs + a release note), so I left it out of a fix PR. Worth a follow-up.^2.5.0(both the root and itscoresubmodule). Both resolve to 2.6.1 today so it's dormant, but once 2.7.0 ships npm would hoist 2.7.x for harper-pro and nest 2.6.x under core — two copies of a binding whose registry is process-global. Wants a coordinated bump.Provenance
Root-caused and written by Claude Opus 5. Reviewed pre-push by Codex, Gemini, Grok, and the Harper domain pass; the test-effectiveness finding above came from that review and is fixed here. One reviewer finding was dropped as disproven — a claimed crash from injecting
thresholdinto a RocksDB descriptor:normalizeCompressionreads onlyalgorithm/leveland ignores extra keys (verified by opening a DB with{algorithm:'lz4', level:3, threshold:4036}), and the branch is unreachable becauseprimaryAttribute.compressiononly ever originates fromgetDefaultCompression's LMDB shape.Unblocks #2036.