Skip to content

fix: Tolerate legacy storage.compression values when opening RocksDB databases - #2037

Merged
kriszyp merged 2 commits into
mainfrom
fix/rocksdb-261-compression-compat
Aug 1, 2026
Merged

fix: Tolerate legacy storage.compression values when opening RocksDB databases#2037
kriszyp merged 2 commits into
mainfrom
fix/rocksdb-261-compression-compat

Conversation

@cb1kenobi

Copy link
Copy Markdown
Member

Why

#2036 (chore(deps): Update rocksdb-js to 2.6.1) fails unit tests on all Node versions with Unsupported compression algorithm "". Main is green — the failures are caused by the interaction of the dep bump with two latent Harper issues, fixed here so #2036 can land.

Root cause

rocksdb-js 2.5.0 ignored the compression option entirely; 2.6.x validates it strictly and treats unset as "use the build default (lz4)". Harper passes LMDB-era values through to RocksDB opens:

  • getDefaultCompression() returns LMDB_COMPRESSION && OPTS, i.e. whatever falsy value the config resolves to ('', false, null) when compression is disabled — and that value is persisted into table metadata at table creation, then replayed on every open.
  • storage.compression: false is a validated config value (configValidator.ts), so on 2.6.1 any instance with compression disabled would fail to open its tables — a real user-facing break, not just a test issue.
  • The unit-test trigger: two component test stubs returned '' for every unknown config key, so compression: '' reached the RocksDB open.

Fix

  • toRocksCompression() maps legacy values at the single chokepoint every RocksDB open flows through (openRocksDatabase): defined-falsy → 'none', true → unset, everything else passes through (2.6.1 already treats an { threshold, ... } object without algorithm as unset). Internal DBIs (compression never set) keep the build default, matching current behavior.
  • getDefaultCompression() now returns false (not raw config garbage) when disabled, so nothing unnormalizable is persisted going forward.
  • Test stubs return undefined for unknown keys.
  • Unit tests for the mapping.

Semantics note (reviewer attention)

On 2.5.0 the option was ignored, so storage.compression: false instances have been writing lz4 anyway. This fix makes false actually mean no compression for new writes once 2.6.x lands. Existing lz4 blocks stay readable (RocksDB records compression per block; mixed SSTs are supported). If parity-with-current-behavior is preferred over honoring the config, the 'none' mapping can become unset — flagging for review.

Verification

  • Build + the affected suites pass against both rocksdb-js 2.5.0 (current lockfile) and 2.6.1 (componentLoader, startOnMainThreadOnce, new rocksCompression tests: 16 passing on each).
  • Cross-model review: Codex (gpt-5) reviewed the diff — confirmed no missed call sites, no LMDB-path regression (falsy still means disabled there), and the mixed-compression safety reasoning; it caught that an earlier draft didn't compile against 2.5.0 typings (fixed by isolating the legacy value behind a cast).

Note: #2036 also shows a failing Windows integration shard (EPERM: rename ... harper-config.yaml.tmp) — that's an unrelated Windows file-lock flake.

Generated by Claude (Fable 5).

🤖 Generated with Claude Code

rocksdb-js >= 2.6 validates the compression option strictly: '' and booleans
throw ("Unsupported compression algorithm"), and unset means the build default
(lz4). Harper's persisted table metadata carries LMDB-era values where defined
falsy (false, '') means explicitly disabled, so map those to 'none' and true to
unset at the single chokepoint every RocksDB open flows through. Also normalize
getDefaultCompression's disabled return to false so raw config garbage ('') is
never persisted, and fix two component test stubs that returned '' for every
unknown config key - which is how CI on the rocksdb-js 2.6.1 bump first hit
this.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@cb1kenobi
cb1kenobi requested a review from kriszyp July 31, 2026 23:42
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a normalization utility, toRocksCompression, to map legacy LMDB-era compression configuration values to formats compatible with rocksdb-js >= 2.6. Specifically, it maps explicitly disabled values (such as false or empty strings) to 'none', and maps true to undefined to allow the build default to apply. It also updates mock configurations in tests to return undefined instead of empty strings for unknown keys to prevent validation errors, and adds a suite of unit tests to verify these mappings. There are no review comments, and we have no feedback to provide.

@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

@cb1kenobi
cb1kenobi marked this pull request as ready for review July 31, 2026 23:55
@kriszyp
kriszyp merged commit 8dc2fa6 into main Aug 1, 2026
44 checks passed
@kriszyp
kriszyp deleted the fix/rocksdb-261-compression-compat branch August 1, 2026 00:12
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.

2 participants