Skip to content

Add storage tiering: SST paths and blob file placement across volumes - #767

Open
kriszyp wants to merge 43 commits into
mainfrom
kris/tiered-storage
Open

Add storage tiering: SST paths and blob file placement across volumes#767
kriszyp wants to merge 43 commits into
mainfrom
kris/tiered-storage

Conversation

@kriszyp

@kriszyp kriszyp commented Aug 8, 2026

Copy link
Copy Markdown
Member

Adds database-wide SST placement through paths, per-column-family blob placement through blobs.dir, and a separate process-wide blob cache for large values. The final validation pass fails closed around persisted blob directories and derives directory creation permission from the same relocation decision, allows a harmless leftover acknowledgement on a read-only no-op, and makes warm paths reuse distinguish omission from an explicit empty list: omission inherits and [] rejects a tiered live database, while the same [] matches an already-untiered one.

Directory isolation remains caller-managed by prior ruling: two databases must never share a paths or blobs.dir directory. The branch keeps the conflict-free origin/main merge requested by @kriszyp; no history was rewritten.

A successful non-default column-family drop now removes that family from every live and retained destroy layout before unregistering its name. This prevents a later destroy() through an old handle from deleting a replacement database that reused the dropped family's blob directory, while preserving same-name recreation and directories still shared by another family; five regressions cover synchronous, asynchronous, live-descriptor, stale-handle, and shared-directory cases.

That drop is also atomic against DBRegistry::OpenDB. It holds databasesMutex across DB::DropColumnFamily itself, both layout erasures, and the columns erase — the same mutex a warm open takes before columnsMutex — so an open sees the family wholly present or wholly gone rather than being handed a ColumnFamilyDescriptor for a family RocksDB has already dropped. The verification-table sweep deliberately stays outside that section: the VT writer mutex is process-global and must never be taken under the registry mutex. A test-only two-phase latch parks the drop mid-section until a worker's warm open has reached the registry mutex, keyed by database path because the counters are process-global and Vitest's thread pool shares them, so the regression asserts the interleaving happened rather than hoping for it.

The retained destroy layout is now canonical rather than last-writer-wins. Registry entries are keyed by path AND read-only, so one database can have two live descriptors, and a read-only handle opens with a paths shorter than the writer's — or none — whenever the files it needs still sit at path index 0. db_paths is serialized nowhere, so recording that shorter list erased the only trace of the writer's other volumes and destroy() reported success with their SST files still on disk (reported by @cb1kenobi). RecordLayout now takes a new list only when it EXTENDS the retained one, and DestroyDB applies the record first and the live descriptor second, so the entry the registry loop happens to pick can add volumes but never remove them. Both halves of that rule are load-bearing in opposite directions, and invariant 18 carries the contract: a shorter list must not shorten the record, and a divergent one must not extend it either — destroy() deletes every SST it finds in each recorded directory, so one mistyped paths naming a neighbouring database's volume would otherwise take that database's files down with this one. Three regressions cover the reader that omits paths, the reader that passes a shorter list and stays live across destroy(), and a reader naming a neighbour's volume — each mutation-checked against the rule it exists for.

Keeping that record is only half of it, and @cb1kenobi found the other half: a writable open the record refused still succeeded, warned about in the info LOG and otherwise ignored. RocksDB then used the requested list while destroy() used the retained one — compaction writing SSTs to a volume destroy() never sweeps, and destroy() still sweeping one this open disowned, which another database may since have been given. Neither is repairable afterwards, so such an open now fails, and it fails before DB::Open — past that point a single compaction placing a single SST is already the divergence. RecordLayout throws on the same condition rather than returning a bool nobody acted on, now purely as the assertion that the record did not move underneath an open database.

The dangerous shape is the quiet one: while every file still sits at path index 0, RocksDB opens happily under a shorter or divergent list, so the regression uses exactly that setup and asserts that no new OPTIONS file appears — RocksDB writes one on every open, so an unchanged set is what proves the refusal preceded the open rather than being unwound after one. Removing the pre-open call makes that assertion fail. One consequence: the check preempts explainOpenFailure's removed/reordered guidance whenever the same process opened the database earlier, so coverage for that message moved to a spawned child, where the record is gone and RocksDB reports the MANIFEST as corrupt — which is what a restart actually produces.

For the human reviewer

  1. Require patched-build coverage before merge. The selected policy is to wait for a prebuild carrying Add RocksDB blob_dir patch so blob files can live on a separate volume rocksdb-prebuilds#13, then run the ROCKSDB_JS_REQUIRE_BLOB_DIR=1 suite and specifically confirm external-blob cleanup through DestroyDB; the alternative is merging native storage code that is compiled out by every current prebuild. Waiting is reversible as soon as the prebuild exists, while saying no accepts an unexecuted data-integrity path. One change on this branch sits entirely inside that gap: destroy() now prefers the retained record's blob directory over a live descriptor's, so a read-only handle opened before a relocation can no longer make the sweep visit the directory the blob files left. It is argued from the ordering rather than demonstrated, because every case that would exercise it is capability-skipped.
  2. Keep allowDirChange database-scoped even though it is nested under per-family blobs. A relocation moves every family sharing the source directory, so the decision deliberately reaches those families together; the alternative is splitting or renaming the acknowledgement before release. Changing this later is breaking, while saying no now costs an API redesign and its migration tests.
  3. Retain tier layouts in the process registry, not a persistent marker. This makes close() followed by destroy() work because the registry keeps a path-keyed layout copy, but a fresh process cannot recover db_paths and may orphan external SSTs. A rocksdb-js-owned marker would cover fresh-process destroy and directory identity at the cost of a new durable format; adding one later is possible but cannot retroactively describe existing tiered databases. Independent review raised the same gap from its other end this round — a process whose first open is read-only seeds an EMPTY path record, so close() + destroy() there reports success with external SSTs still on disk. Declined as the same open question rather than a separate defect: the empty record suppresses nothing, because the fallback it bypasses is LoadLatestOptions, and db_paths sits in RocksDB's "not yet supported" serialization block, so that path recovers exactly as little. Trusting a reader's list instead is the cross-database deletion the retained-record rule exists to prevent. Only a durable marker closes it.
  4. Leave the read-only descriptor's column entry registered on a drop. Independent review raised this three rounds running as a leak: a warm read-only descriptor keeps serving the dropped family. It is a separate DB::OpenForReadOnly instance whose handles a writable drop does not invalidate and which by design never observes later changes, so unregistering there would make the next read-only open throw "cannot create column family in read-only mode" for a family its own frozen manifest view still contains; the rationale is now recorded at the call site. The alternative is treating a read-only view as invalidated by a writable drop, which is a broader semantic change than this PR should make.
  5. Keep omitted blobs.dir strict on warm opens. Omitting it means “alongside the SSTs,” so a plain second open of a family already using an external directory rejects even though the live handle cannot move. The alternative is an explicitness latch limited to warm reuse; loosening this later is compatible, while saying no now costs that additional state and test surface.

Verification

  • pnpm build — production TypeScript bundle and native binding built successfully.
  • pnpm check — type-check, lint, and formatting checks passed for 191 files.
  • pnpm test — 864 passed, 31 skipped, 0 failed on the pushed head; pnpm check (type-check, lint, format over 192 files) passed.
  • pnpm test:native — 197/197 passed; the focused BlobRelocation/BlobDirScanner run passed 31/31, including destination-creation ownership and no-op acknowledgement cases.
  • The three destroy-layout regressions were mutation-checked one rule at a time: without the RecordLayout guard the omitted-paths reader orphans the writer's second volume, without the DestroyDB merge the shorter-list reader does, and with either the size-only rule or the previous live-first ordering the neighbour's SST files are deleted.
  • The drop/warm-open regression was mutation-checked: restoring the two-lock split makes the worker receive the dropped family and its write is silently discarded, and a second latch use in the same process still works from the baselined counters.
  • nm -C -u build/Release/rocksdb-js.node | rg 'rocksdb_js::' and git diff --check produced no output.
  • On the origin/main merge (one conflict, the include block in db_descriptor.cpp, both sides kept): pnpm build, pnpm test (875 passed / 32 skipped, 65 files), pnpm test:native (209/209), pnpm check, and nm -C -u clean of undefined rocksdb_js:: symbols — the check that caught the last rebase silently dropping main's ParkTimeoutRegistry hunks.
  • The pre-open refusal is mutation-checked in the direction that matters: disabling only AssertDbPathsExtendRetained leaves the post-open throw in place, the message assertion still passes, and the test fails on the OPTIONS-file assertion (OPTIONS-000007, OPTIONS-000015 vs OPTIONS-000007) — i.e. the open had happened.
  • The new spawned fixture was run under the repo's actual Deno command (deno run --allow-all --sloppy-imports on Deno 2.7.5): both storage-path regressions pass. Independent review flagged it as needing its own permission flags; measured against pnpm test:deno rather than a bare deno run, the child inherits, as the existing blob-cache fixture already relies on.
  • The 28-case blobs.dir integration suite remains capability-skipped because the current RocksDB prebuild lacks the downstream patch; this includes the new read-only relocation/no-op distinction and dropped-layout regressions.

Complexity: complicated

Review-Coverage: authored=codex; ran=gemini,claude; blocked=domain(timeout); declined=cursor-grok,cursor-composer; rounds=11 @ a2e39b2

Human-Review-Need: 4 @ a2e39b2

@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 tiered storage support for RocksDB, allowing SST files to be distributed across multiple storage paths and large values to be decoupled into separate blob files. The changes span C++ bindings, TypeScript definitions, documentation, and tests. The review feedback correctly identifies a cross-platform path handling issue on Windows, where using std::filesystem in the C++ layer can corrupt non-ASCII paths due to ANSI code page round-tripping. It is recommended to resolve and normalize all paths in the JavaScript layer using node:path before passing them to the native binding.

Comment thread src/binding/database/database.cpp Outdated
Comment thread src/binding/database/database.cpp Outdated
Comment thread src/store.ts
Comment thread src/store.ts
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

📊 Benchmark Results

get-sync.bench.ts

getSync() > random keys - small key size (100 records)

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 lmdb 1 24.38K ops/sec 41.01 39.55 1,949.715 0.133 121,915
🥈 rocksdb 2 11.19K ops/sec 89.34 86.00 24,156.369 0.976 55,965

getSync() > sequential keys - small key size (100 records)

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 lmdb 1 28.31K ops/sec 35.32 34.20 520.156 0.099 141,545
🥈 rocksdb 2 11.14K ops/sec 89.78 87.33 2,434.336 0.105 55,692

ranges.bench.ts

getRange() > small range (100 records, 50 range)

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 lmdb 1 24.56K ops/sec 40.71 36.51 1,643.372 0.292 122,825
🥈 rocksdb 2 16.27K ops/sec 61.45 54.00 1,070.446 0.119 81,371

realistic-load.bench.ts

Realistic write load with workers > write variable records with transaction log

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 rocksdb 1 344.94 ops/sec 2,899.072 74.76 70,486.212 16.59 690
🥈 lmdb 2 26.85 ops/sec 37,246.872 432.678 1,185,040.898 136.306 64.00

transaction-log.bench.ts

Transaction log > read 100 iterators while write log with 100 byte records

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 rocksdb 1 39.48K ops/sec 25.33 11.93 20,914.439 0.849 197,383
🥈 lmdb 2 446.62 ops/sec 2,239.016 179.01 16,705.78 1.29 2,234

Transaction log > read one entry from random position from log with 1000 100 byte records

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 rocksdb 1 784.98K ops/sec 1.27 1.10 4,474.274 0.198 3,924,902
🥈 lmdb 2 440.31K ops/sec 2.27 1.10 5,428.855 0.801 2,201,546

worker-put-sync.bench.ts

putSync() > random keys - small key size (100 records, 10 workers)

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 rocksdb 1 831.56 ops/sec 1,202.563 1,020.423 3,725.859 0.477 1,664
🥈 lmdb 2 1.15 ops/sec 871,047.528 854,308.75 898,816.079 1.15 10.00

worker-transaction-log.bench.ts

Transaction log with workers > write log with 100 byte records

Implementation Rank Operations/sec Mean (ms) Min (ms) Max (ms) RME (%) Samples
🥇 rocksdb 1 23.56K ops/sec 42.45 29.76 20,537.305 2.09 47,115
🥈 lmdb 2 848.63 ops/sec 1,178.371 195.787 18,582.525 5.37 1,698

Results from commit 8de4e3a

@kriszyp
kriszyp force-pushed the kris/tiered-storage branch from 2a9d78e to 03d58b4 Compare August 12, 2026 21:03
@kriszyp
kriszyp marked this pull request as ready for review August 24, 2026 20:51
@kriszyp
kriszyp force-pushed the kris/tiered-storage branch from 06e2d76 to 942f905 Compare August 24, 2026 21:42
Comment thread src/binding/database/db_registry.cpp Outdated
Comment thread src/binding/database/db_descriptor.cpp Outdated
Comment thread src/binding/database/db_descriptor.cpp
Comment thread src/binding/database/database.cpp Outdated
Comment thread src/binding/database/db_settings.cpp Outdated
Comment thread test/tiered-storage.test.ts
kriszyp pushed a commit that referenced this pull request Aug 25, 2026
…guard

Review feedback on #767.

- Blob settings are per-column-family, and RocksDB restores none of them on
  open, so a cold open of one family restamped every other family with the
  opener's settings. In Harper every table is a named column family, so
  whichever table opened the database first decided min_blob_size /
  enable_blob_files / GC for all of them, and it flipped on restart. BlobOptions
  is now a struct of std::optional: each family's persisted values are restored
  from its OPTIONS file and only the fields the caller supplied are applied, to
  the target family alone — the same rule compression already followed. The
  blobs.dir mismatch check is scoped to the target family for the same reason,
  and the warm-reopen check in DBRegistry::OpenDB now covers every explicitly
  requested blob setting rather than just the directory.

- destroy() was handed a default rocksdb::Options, which describes "everything
  under the database directory" — it silently orphaned every tiered SST and
  every blob file. It now gets the live database's db_paths and per-CF blob_dir.
  db_paths is not written to the OPTIONS file, so the layout has to be captured
  before the close.

- Adding paths to a database that does not have it bricked the database: its
  existing files sit at path index 0, which the new list redefines, and RocksDB
  reports the MANIFEST as corrupt — sending an operator to backup restore rather
  than to the config line they changed. assertStoragePathsUsable rejects it with
  the real cause and the supported form (list the database directory as
  paths[0]). Decided by file existence rather than string comparison, so an
  already-tiered database and a differently-spelled same directory both pass.

- std::filesystem is gone from the option parsing. It could throw out of an
  N-API callback (std::terminate rather than a JS error), and on Windows it is
  wchar_t-based, so round-tripping a UTF-8 path through it corrupts non-ASCII
  names. Paths are resolved in store.ts with node:path and the native layer
  rejects a relative one.

- config({ blobCacheSize }) now latches: once a caller has stated a budget, a
  later block-cache-only call no longer discards it. blobCacheSize: undefined is
  treated as omitted instead of throwing, and the whole call is validated before
  either cache is resized. README/docs say plainly that the derived default
  raises the process memory ceiling by 10%.

- Tiered-storage tests open through a helper the afterEach hook always closes,
  so a failed assertion no longer deletes a live database's files out from under
  it. Blob-cache tests moved to their own file; the derivation case runs in a
  child process because the "set explicitly" flag is process-global.

Refs #767

Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp pushed a commit that referenced this pull request Aug 25, 2026
Pre-push review (codex + gemini + harper-domain) on ce4d04d:

- major: creating a database through a named family also creates `default` on
  the way, and it was built from the caller's blob request — so opening table t1
  with a blobs.dir persisted that directory on `default` too, and a later plain
  RocksDatabase.open(path) could not open the database at all. `default` is now
  created from the blob creation defaults when it is not the target, mirroring
  the compression gate beside it.

- The paths migration guard returned as soon as one SST in the database
  directory was reachable under paths[0]. A half-finished copy, or a colliding
  file number under a paths[0] shared with another database, let one file vouch
  for the rest. It now checks every SST.

- The blob-cache "set explicitly" latch was read outside blobCacheMutex, so a
  worker thread's config({ blockCacheSize }) could overwrite another thread's
  explicit budget with the derived 10%. Read and written under the lock now.

- noBlockCache means "this database does not use the process-wide caches", but
  the blob cache was attached regardless, letting a scratch database evict the
  serving database's blob values.

- prepopulateCache was the one blob field missing from the warm-reopen conflict
  list, so an explicit request was silently ignored on a live column family.

- destroy() captured the layout only when it won the close claim; losing to a
  concurrent close silently destroyed with default options and orphaned the
  external files. Capture no longer depends on the claim, and falls back to the
  persisted per-CF blob_dir when nothing live is available.

- An unpatched build opening a database written by a patched one read its blobs
  from the wrong directory with no error at open. LoadLatestOptions drops the
  field it cannot parse, so the OPTIONS file is scanned as text and the open is
  refused.

- docs: "restore it flat, then open it without blobs.dir" is rejected by the
  mismatch guard — the flat open needs allowDirChange per affected family.
  createCheckpoint() carries the same flat-layout hazard as backup.

- Trimmed comments that narrate signatures or duplicate the public JSDoc.

Refs #767

Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp pushed a commit that referenced this pull request Aug 25, 2026
Pre-push review round 2 (codex + gemini + harper-domain) on 2b3b5d6:

- blocker: the destroy layout capture I added in round 1 read `descriptor->db`
  and iterated `columns` whether or not it won the close claim, so the thread
  that lost raced `finishClose()` resetting `db` and clearing `columns` — the
  freed-descriptor class AGENTS.md invariant 6 describes, now on the drop path.
  The descriptor records its own layout (db_paths + per-CF blob_dir) at open and
  when a family is created; destroy reads that snapshot under its own mutex and
  never touches the live DB.

- major: the round-1 prepopulateCache warm-reopen check rejected a second open
  with IDENTICAL options whenever no blob cache is attached. The cold path only
  honors the request when there is a cache to prepopulate, so the live family
  stays disabled and the check saw false -> true. That is every process which
  never called config({ blockCacheSize }), and every noBlockCache database, so
  it was a startup failure on an unchanged config. The comparison is now gated
  on a cache actually being attached.

- Writing an end-to-end backup test turned up something the docs had wrong:
  `paths` does not make backups come out flat, it makes them impossible.
  GetLiveFilesStorageInfo — which BackupEngine and Checkpoint both go through —
  returns NotSupported for any non-empty db_paths, including the one-entry
  `[{ path: <database directory> }]` form the migration section recommends. Both
  db.backup() and db.createCheckpoint() fail. Documented in README, the guide,
  and AGENTS.md, and asserted by a test.

- blobs.dir is created if missing. Nothing created it, and a missing directory
  does not fail the open: writes are acknowledged and the first flush fails,
  flipping the database read-only on a background error.

- ROCKSDB_JS_REQUIRE_BLOB_DIR=1 makes the blobs.dir suite fail rather than skip,
  so a release job can require the packaged prebuild to carry the patch.

- Tests: flat-restore recovery for a blobs.dir database via allowDirChange, and
  a cold-reopen round trip for the GC ratio fields, which are hand-copied
  through three sites and otherwise only proven at the parse boundary.

- Docs: noBlockCache now disables the blob cache too, and `dir` is the one blob
  field that does not inherit on a plain reopen — both were stated wrongly.

Refs #767

Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp added a commit that referenced this pull request Aug 25, 2026
Pre-push review round 3 (codex + gemini + harper-domain) on 6332824:

- major: `blobs.allowDirChange` acknowledged a relocation for the open's target
  column family only. Every other family kept the `blob_dir` in the OPTIONS
  file — which for a restored copy is the SOURCE database's live blob
  directory, so the two databases mint colliding `NNNNNN.blob` numbers there
  and each one's obsolete-file scan deletes the other's live files. It also
  made the documented multi-table migration unreachable: one family could be
  relocated per cold open, and the second family's open in the same process is
  a warm one, which cannot move a live family's directory. Since a relocation
  (and a flat restore) moves the whole closed database's blob files at once,
  `dir` is now applied to every family under `allowDirChange`. Only `dir` is
  widened; the rest of `blobs.*` stays per-family.

  What is still uncaught: a restored copy opened WITHOUT the acknowledgement
  whose target family has no external blob directory of its own (`default`,
  typically). Nothing in the OPTIONS file distinguishes a copy from the
  original, so that one is a documented procedure, not a check.

- major: a column family created on an already-open database never goes
  through `DB::Open`, so nothing created its `blobs.dir`. Writes were
  acknowledged and the first background flush flipped the whole database
  read-only. Harper reaches this on the normal path — a plain open at startup,
  then a table opened with its own directory. Directory creation is now a
  shared `ensureBlobDirExists` both the cold and warm paths call. The existing
  tests missed it because they pre-created the directory.

- A persisted `blob_dir` that no longer exists now fails the open naming the
  family and the directory, instead of surfacing as a read-only database at
  the first flush.

- `prepopulate_blob_cache` was restored (and applied) only when this process
  had a blob cache attached. RocksDB rewrites the OPTIONS file on every open,
  so a cache-less opener — a CLI tool, a `noBlockCache` script — persisted
  kDisable over the serving process's request and turned it off permanently.
  Restored unconditionally; the warm conflict check no longer needs its cache
  gate either, because the live family now reflects the request.

- `parseStoragePaths` reserved against the JS array length. A sparse
  `new Array(2**32 - 1)` costs nothing to make and the reserve throws
  `std::bad_alloc` out of the N-API callback, which is `std::terminate`. The
  count is bounded first.

- Docs: `allowDirChange` is database-wide; open a restored copy with it before
  anything else touches it; removing `paths` produces the same misleading
  MANIFEST-corruption message as adding it, and is equally unguarded.

- Trimmed comments that restate a signature rather than record an invariant.

Refs #767

Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp added a commit that referenced this pull request Aug 25, 2026
Round-4 review found a regression in round 3's fix. Applying `blobs.dir` to
every column family under `allowDirChange` strands the families whose blob
files did NOT move — and the normal Harper layout is exactly that shape:
`default` keeps its blobs beside the SSTs (applyBlobCreationDefaults
guarantees it for a database created through a named family) while named
tables have their own volume. Relocating table1 re-pointed `default` at a
directory its blobs were never in, and nothing caught it: the mismatch guard
is target-only and the new existence check passes because the directory does
exist.

The acknowledgement now reaches exactly as far as the move went. Blob files
sitting in one directory move together, so the families that shared the
target's persisted directory are re-pointed with it; a family whose blobs were
somewhere else keeps its own. An omitted `dir` still re-points every family,
because that is the statement "the whole database was flattened into its own
directory" — which is what a backup restore produces and what makes restoring
beside a live source safe.

One open therefore describes one move; a database with several distinct blob
directories needs one open per directory. Documented in the guide, the README,
the public type, and AGENTS.md, and covered by a mixed-layout test that fails
on the unconditional form.

Also trims the code comment narrating why the previous shape was wrong;
AGENTS.md is where that belongs.

Refs #767

Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp added a commit that referenced this pull request Aug 25, 2026
Round-5 review. The round-4 narrowing landed on the explicit-`dir` branch and
left the flatten branch (`allowDirChange` with no `dir`) unbounded.

`{ blobs: { allowDirChange: true } }` is character-for-character what
docs/tiered-storage.md tells operators to type against a restored copy, so a
wrong path argument aims it at the live original — where clearing every
family's `blob_dir` strands each tiered family's live blob files, with the
target-side guard skipped by the acknowledgement and the existence check
skipped by the empty directory. It was also carried onto a family that does
not exist yet, where nothing could have moved.

The claim is now checked rather than trusted: it is checkable. If a family's
recorded directory still holds `.blob` files, the database was not flattened —
either nothing moved, or those files belong to the source database a copy was
restored from and sharing the directory would corrupt both. The open is
refused naming that family. The genuine cases are unaffected: after a real
flatten the old directory is empty, and a restore onto a host without the
source volume cannot reach it.

Also:

- `assertNoPersistedBlobDir`'s OPTIONS-file scan is extracted to Node-free
  `core/options_file.cpp` and covered by GoogleTest. It is compiled only into
  an UNPATCHED build, where no test can produce a database carrying a
  `blob_dir` to trip it — so nothing executed it, and a regression in the
  key-boundary scan would ship silently as exactly the failure it prevents.
  The extracted scan also handles CRLF, which the inline version did not.

- `paths: [null]` reported N-API's "Cannot convert null to object": reading a
  property off a non-object leaves that pending and it swallows the specific
  message. The entry type is checked first.

- Docs: spell one blob directory identically across families (grouping is by
  persisted string, so a symlinked alias is a second group), and the flatten
  claim is verified.

Refs #767

Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp added a commit that referenced this pull request Aug 25, 2026
Round-6 review. The claim check added last round sat inside the `!isTarget`
branch, so naming the tiered family bypassed it — `open(dbPath, { name:
'table1', blobs: { allowDirChange: true } })` against a live original cleared
table1's directory while its `.blob` files were still in it. The target-side
mismatch guard is skipped by the acknowledgement and the missing-directory
check is skipped because `blob_dir` is empty by then, so nothing caught it.
That is the arbitrary half of the same guard: the sibling direction was
refused and the direction the operator actually typed was not.

The check is hoisted above the branch and evaluated against each family's
PERSISTED directory, because the target's own request has already been applied
to `cfo` by then.

- `findPersistedBlobDir` treated `# blob_dir=/mnt/old` as a live value: the
  whole-key test only looked at the preceding character. It now requires
  whitespace back to the start of the line. RocksDB never writes comments, but
  a hand-edited file would make an unpatched build refuse an untiered database
  over a directory nothing was configured with.

- Trimmed comments restating the exception message beside them.

Refs #767

Co-Authored-By: Claude Opus <noreply@anthropic.com>
Comment thread src/binding/database/db_registry.cpp Outdated
Comment thread src/binding/database/db_descriptor.cpp
Comment thread src/binding/database/db_descriptor.cpp Outdated
Comment thread src/binding/database/db_settings.cpp
Comment thread test/tiered-storage.test.ts
kriszyp added a commit that referenced this pull request Aug 25, 2026
Resolves the invariant numbering: main added "A dropped transaction must
release itself" as 12, so this branch's 13 and 14 become 14 and 15, and the
two `(invariant 14)` references in db_descriptor.cpp follow. Left as-is the
merge produced duplicate list numbers, which oxfmt renumbers — the formatting
check failed on the merge result while passing on either side alone.

Also fixes a pre-existing Windows failure the merge run surfaced: the two
relative-path tests derive their relative spelling from `os.tmpdir()`, which
can be on a different drive than the process working directory, where
`path.relative` returns an ABSOLUTE path — so the case under test ("a relative
path is resolved against the process working directory") could not be
expressed at all and the test's own precondition failed. Those two now use a
directory under the working directory.

Tests: pnpm test 839 passed / 22 skipped; pnpm test:native 164 passed;
pnpm check clean.

Refs #767

Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp added a commit that referenced this pull request Aug 25, 2026
Round-8 review: `allowDirChange` with no `dir` was verified against the files
on disk; `allowDirChange` WITH a `dir` — the form the docs tell operators to
type after an `mv` — was taken on faith. Running the open before the `mv`
finishes, or against the wrong destination, persists the new directory and
every value at or above `min_blob_size` reads as missing until a compaction
turns it into a background error. The destination existing is no signal:
`ensureBlobDirExists` creates it.

Both forms are claims about files on disk, so both are now checked against
them. A move is refused when the recorded directory still holds every `.blob`
file and the destination holds none. A partially-completed move (both hold
files) is left alone rather than guessed at.

Refs #767

Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp added a commit that referenced this pull request Aug 25, 2026
Round-9 review. The relocate check accepted a partial move: it required the
destination to hold NO blob files, so an interrupted `rsync` that copied ten
of sixty passed, the new directory was persisted, and every value still behind
was stranded. The destination holding some files is not evidence the move
completed, and the destination being empty is not evidence either — the open
creates it.

Both forms now share one rule: a family whose recorded directory still holds
`.blob` files has not moved, and the open is refused naming that family. It is
skipped when nothing is actually changing, so an `allowDirChange` left behind
in a config file does not start refusing every open.

Refs #767

Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp added a commit that referenced this pull request Aug 25, 2026
Test follow-up to the previous commit: `allowDirChange` on a family whose
recorded directory still holds `.blob` files is refused whichever form the
request takes, so the two flatten cases assert the one shared message, and the
restore cases assert the refusal FIRST — with the source database's blob
directory still populated, which is exactly the state a mistyped restore is in —
before removing it and opening the copy on its own.

Refs #767

Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp added a commit that referenced this pull request Aug 25, 2026
… leak

Three defects from @cb1kenobi's review, each reproduced before the fix and
mutation-checked after it.

`close()` then `destroy()` orphaned every SST file on a `paths` volume.
`destroy()` accepts a closed handle (test/destroy.test.ts covers it), and
closing the last handle purges the registry entry the layout was read from —
so the destroy ran with an empty `db_paths`, removed the database directory,
left the tiered files behind and reported success. `db_paths` is not written to
the OPTIONS file, so nothing on disk could put it back. `DBHandle::close` now
copies a `DBFileLayout` off the descriptor before releasing it and
`Database::Destroy` hands it to `DBRegistry::DestroyDB`.

`prepopulate_blob_cache` leaked from the database's first opener into every
column family created warm afterwards. It was the one blob field assigned only
in the `true` direction, and the warm path builds on the descriptor's own
retained options — so a table that never asked for prepopulation warmed the
shared blob cache on every flush, persisted, across restarts. Assigned in both
directions now, as is `blob_cache`, which inherited the same way past a
`noBlockCache` request.

The per-family regression test passed with the restore removed: `t2` was
created after `t1`'s cold open, so there was nothing on disk to restamp. It now
seeds `t2` first and asserts against the OPTIONS file — a plain reopen asks for
the default anyway and writes the correct value back, which is what hid the
restamp from a behavioral assertion.

Removing or reordering `paths` still cannot be detected before the open, but it
no longer reports only `MANIFEST may be corrupted`: `explainOpenFailure` appends
what to do about it. Appended and conditional, because real corruption gives the
same status.

Refs #767

Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp added a commit that referenced this pull request Aug 25, 2026
Round-10 pre-push review.

`allowDirChange` skipped its own check for a family whose persisted `blob_dir`
is empty. Empty means "in the database directory", not "no directory", so
comparing the recorded string against the request exempted exactly the
flat-to-external move — the only migration an untiered database can make, and
the one the docs lead with. An operator running the open before the `mv`, or
against the wrong destination, had the new directory persisted for them and
every value at or above `min_blob_size` read as missing. Both sides now resolve
an empty directory to the database directory before comparing, which leaves the
flatten form alone: there both sides resolve to the same place and the existing
short-circuit still applies.

The layout `destroy()` needs is now remembered per DATABASE PATH in the registry
rather than per handle. A handle-scoped copy is stale the moment another open
appends a storage path, so `close()` → reopen with `[A, B]` → close →
`destroy()` on the first handle deleted only `A`'s files. Keyed by path, the
record is always the latest open's. It is erased when the path is destroyed.

The warm-path `paths` conflict now names what actually happened. RocksDB
sanitizes an untiered database's `db_paths` to `[{dbname, UINT64_MAX}]` rather
than leaving it empty, so a plain open followed by a table open carrying the
zero-to-one migration reported a mismatch between two requested lists instead of
"this database is already open untiered in this process; the change needs a cold
open". That branch had no test in either direction.

Also drops three comments that restated the code below them.

Refs #767

Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp added a commit that referenced this pull request Aug 25, 2026
… PurgeAll

Round-11 review. The stale-layout regression asserted that `fast` OR `slow` held
an SST before the destroy, which `fast` alone satisfies — and the manual
`compactSync()` it used targets path 0, as does flush output, so `slow` could
legitimately be empty and the per-handle-prefix bug it exists to pin would still
have passed. It now waits for automatic compaction to reach `slow` and asserts
that directly, which is how the neighbouring spill test does it.

`PurgeAll` clears `knownLayouts` too, so the process-global map is torn down
with the rest of the registry's state rather than only on `destroy()`.

Also drops comments that restated AGENTS.md invariants 14/15 next to the code.

Refs #767

Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp added a commit that referenced this pull request Aug 25, 2026
Round-12 review. Clearing `knownLayouts` in `PurgeAll` was a regression I
introduced taking round 11's cleanup nit: `PurgeAll` is reached from the public
`shutdown()`, and a handle retained across that can still be destroyed — which
would then orphan every tiered SST file, the exact failure the cache exists to
prevent. Erased on `destroy()` only.

`findPersistedBlobDir` returned whitespace as a directory name, so an OPTIONS
file with `blob_dir=` padded by spaces made an unpatched build refuse an
untiered database, naming a directory nothing was ever configured with. The
value is trimmed, with GoogleTest cases both ways.

Refs #767

Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp added a commit that referenced this pull request Aug 25, 2026
Refs #767

Co-Authored-By: Claude Opus <noreply@anthropic.com>
@kriszyp

kriszyp commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

Thanks — four of the five were real and are fixed at d725c32; the fifth is in the PR body for a ruling.

  • destroy() after close() — fixed, and the root cause turned out to be wider than a missing capture: any per-handle snapshot goes stale the moment another open appends a storage path. The process now remembers each database's layout by path (DBRegistry::knownLayouts), so close() → reopen with [A, B] → close → destroy() on the first handle deletes both volumes. Covered by two cases, both mutation-checked.
  • prepopulate_blob_cache leak — fixed as suggested; blob_cache was inheriting the same way past a noBlockCache request and got the same treatment.
  • The regression test passing with the fix removed — confirmed by mutation, and seeding t2 first was not enough on its own: the later plain reopen asks for the 2 KB default anyway and writes the correct value back, so it still passed. It now asserts against the OPTIONS file, which is where the restamp actually lands.
  • Removing/reordering pathsexplainOpenFailure now appends your suggested guidance to any Corruption naming a .sst. Appended and conditional rather than substituted, since genuine corruption gives the same status.
  • The derived blob cache — left for @kriszyp with the two alternatives (carve it out of blockCacheSize, or default to 0 and require opt-in) written up in the PR body.

Chasing the flat-family case in your third comment also turned up a bigger one: allowDirChange skipped its own "checked, not trusted" guard whenever the persisted blob_dir was empty — which is every family of an untiered database, so the flat-to-external move, the only migration those databases can make, was the one form taken on faith. Both sides now resolve an empty directory to the database directory before comparing.

— Claude Opus

@cb1kenobi

Copy link
Copy Markdown
Member

Re-reviewed d725c32ab9882d7665039f183b336543dd9b0068 (round 12). Every prior finding was re-measured on a local build rather than read for the absence of the old code — 0 Critical, 0 High, 1 Medium open.

Confirmed fixed, by measurement

High — close() then destroy() orphaned every tiered SST. All three sequences now clean up, where the middle one was the bug:

sequence round 11 d725c32a
destroy-while-open 1 → 0 1 → 0
close-then-destroy 1 → 1 1 → 0
second-handle-open 1 → 0 1 → 0

Proved load-bearing rather than incidental: disabling only the knownLayouts fallback in DestroyDB and rebuilding reproduces the old result exactly (close-then-destroy back to 1 → 1, the other two unaffected). The cache lifetime holds up too — it survives shutdown() (1 → 0), and reopening a path with a different list overwrites it, so it cannot go stale. Reverting the PurgeAll clear in a5fbf362 was the right call; the measurement agrees with the reasoning in that commit message.

Medium — prepopulate_blob_cache leaking into warm-created families. Fixed, and the new test at tiered-storage.test.ts:620 is load-bearing: restoring the one-sided assignment fails it with expected 'kFlushOnly' to be 'kDisable', i.e. the leak persisted into the OPTIONS file, reproduced exactly.

Low — the regression test that passed with its own fix removed. Now proves its claim. Making RecordLayout first-writer-wins (a per-handle layout) fails it with 8 orphaned SSTs on the appended volume.

Medium — removing or reordering paths. Removal and reorder still fail the open, which is inherent since db_paths is never persisted, but the corruption status now carries the explanation and the recovery, and restoring the list does recover the database. Verified all four cases directly. Appending the note rather than substituting it, and phrasing it conditionally, is the right call given a genuinely corrupt database produces the same status.

Still open

Medium — the derived blob cache is capacity on top of the block cache (db_settings.cpp:240, already commented inline). Re-measured: config({ blockCacheSize: 100 MB }) with blobCacheSize never set gives a 100 MB block cache plus a separate 10 MB blob cache — total ceiling 110 MB, +10% on upgrade with no code change. The blobCacheSizeExplicit latch is correct (an explicit blobCacheSize: 0 leaves no blob cache attached), so only the never-set case is affected — which is every existing caller. load-binding.ts:631-638 now documents this accurately, but the thread was resolved without a reply, so it reads as unanswered rather than decided. If the +10% is intended it is a release-note item and can be closed as accepted — worth saying so explicitly rather than leaving a silent resolve to stand in for the decision.

Coverage is still the headline risk

Local run is green — 62 files, 846 passed / 22 skipped / 0 failed, plus native 163 passed / 3 skipped (15 OptionsFile cases). But skips have grown 9 → 19 → 21, and all 21 are the blobs.dir block gated on the blob_dir patch. Of the seven tests this round adds, five run and two do not:

  • skipped: should refuse to relocate before the blob files have moved
  • skipped: should refuse to relocate a flat family before its blob files have moved

So the relocation guards added by 648678d2 / 1f17124a / 9d94879e / cdcea6be are static-only on every build without the patch, which is every build until rocksdb-prebuilds#13 — Add RocksDB blob_dir patch so blob files can live on a separate volume lands. That PR is still open and is now waiting on @cb1kenobi. The ROCKSDB_JS_REQUIRE_BLOB_DIR=1 gate is the right guard so a release cannot silently ship without the feature, but it does not make these tests run in the meantime — green CI is not evidence for the blobs.dir half.


Generated by Barber AI

@kriszyp

kriszyp commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

Addressed the round-12 executable-coverage gap in ccdbcc3. The patched-only relocation decision now lives in Node-free core/blob_relocation.cpp, with its filesystem predicates injected, and the unpatched native test binary executes 18 cases covering the two skipped guards plus the full relocation blast radius, flattening, stale acknowledgements, and missing-directory failures. No relocation behavior changed. Local verification: pnpm build; pnpm test:native (184/184, including all 18 new cases); pnpm test (844 passed / 24 skipped); pnpm check.

kriszyp pushed a commit that referenced this pull request Aug 25, 2026
…guard

Review feedback on #767.

- Blob settings are per-column-family, and RocksDB restores none of them on
  open, so a cold open of one family restamped every other family with the
  opener's settings. In Harper every table is a named column family, so
  whichever table opened the database first decided min_blob_size /
  enable_blob_files / GC for all of them, and it flipped on restart. BlobOptions
  is now a struct of std::optional: each family's persisted values are restored
  from its OPTIONS file and only the fields the caller supplied are applied, to
  the target family alone — the same rule compression already followed. The
  blobs.dir mismatch check is scoped to the target family for the same reason,
  and the warm-reopen check in DBRegistry::OpenDB now covers every explicitly
  requested blob setting rather than just the directory.

- destroy() was handed a default rocksdb::Options, which describes "everything
  under the database directory" — it silently orphaned every tiered SST and
  every blob file. It now gets the live database's db_paths and per-CF blob_dir.
  db_paths is not written to the OPTIONS file, so the layout has to be captured
  before the close.

- Adding paths to a database that does not have it bricked the database: its
  existing files sit at path index 0, which the new list redefines, and RocksDB
  reports the MANIFEST as corrupt — sending an operator to backup restore rather
  than to the config line they changed. assertStoragePathsUsable rejects it with
  the real cause and the supported form (list the database directory as
  paths[0]). Decided by file existence rather than string comparison, so an
  already-tiered database and a differently-spelled same directory both pass.

- std::filesystem is gone from the option parsing. It could throw out of an
  N-API callback (std::terminate rather than a JS error), and on Windows it is
  wchar_t-based, so round-tripping a UTF-8 path through it corrupts non-ASCII
  names. Paths are resolved in store.ts with node:path and the native layer
  rejects a relative one.

- config({ blobCacheSize }) now latches: once a caller has stated a budget, a
  later block-cache-only call no longer discards it. blobCacheSize: undefined is
  treated as omitted instead of throwing, and the whole call is validated before
  either cache is resized. README/docs say plainly that the derived default
  raises the process memory ceiling by 10%.

- Tiered-storage tests open through a helper the afterEach hook always closes,
  so a failed assertion no longer deletes a live database's files out from under
  it. Blob-cache tests moved to their own file; the derivation case runs in a
  child process because the "set explicitly" flag is process-global.

Refs #767

Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp pushed a commit that referenced this pull request Aug 25, 2026
Pre-push review (codex + gemini + harper-domain) on ce4d04d:

- major: creating a database through a named family also creates `default` on
  the way, and it was built from the caller's blob request — so opening table t1
  with a blobs.dir persisted that directory on `default` too, and a later plain
  RocksDatabase.open(path) could not open the database at all. `default` is now
  created from the blob creation defaults when it is not the target, mirroring
  the compression gate beside it.

- The paths migration guard returned as soon as one SST in the database
  directory was reachable under paths[0]. A half-finished copy, or a colliding
  file number under a paths[0] shared with another database, let one file vouch
  for the rest. It now checks every SST.

- The blob-cache "set explicitly" latch was read outside blobCacheMutex, so a
  worker thread's config({ blockCacheSize }) could overwrite another thread's
  explicit budget with the derived 10%. Read and written under the lock now.

- noBlockCache means "this database does not use the process-wide caches", but
  the blob cache was attached regardless, letting a scratch database evict the
  serving database's blob values.

- prepopulateCache was the one blob field missing from the warm-reopen conflict
  list, so an explicit request was silently ignored on a live column family.

- destroy() captured the layout only when it won the close claim; losing to a
  concurrent close silently destroyed with default options and orphaned the
  external files. Capture no longer depends on the claim, and falls back to the
  persisted per-CF blob_dir when nothing live is available.

- An unpatched build opening a database written by a patched one read its blobs
  from the wrong directory with no error at open. LoadLatestOptions drops the
  field it cannot parse, so the OPTIONS file is scanned as text and the open is
  refused.

- docs: "restore it flat, then open it without blobs.dir" is rejected by the
  mismatch guard — the flat open needs allowDirChange per affected family.
  createCheckpoint() carries the same flat-layout hazard as backup.

- Trimmed comments that narrate signatures or duplicate the public JSDoc.

Refs #767

Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp and others added 29 commits September 3, 2026 17:24
Round-10 pre-push review.

`allowDirChange` skipped its own check for a family whose persisted `blob_dir`
is empty. Empty means "in the database directory", not "no directory", so
comparing the recorded string against the request exempted exactly the
flat-to-external move — the only migration an untiered database can make, and
the one the docs lead with. An operator running the open before the `mv`, or
against the wrong destination, had the new directory persisted for them and
every value at or above `min_blob_size` read as missing. Both sides now resolve
an empty directory to the database directory before comparing, which leaves the
flatten form alone: there both sides resolve to the same place and the existing
short-circuit still applies.

The layout `destroy()` needs is now remembered per DATABASE PATH in the registry
rather than per handle. A handle-scoped copy is stale the moment another open
appends a storage path, so `close()` → reopen with `[A, B]` → close →
`destroy()` on the first handle deleted only `A`'s files. Keyed by path, the
record is always the latest open's. It is erased when the path is destroyed.

The warm-path `paths` conflict now names what actually happened. RocksDB
sanitizes an untiered database's `db_paths` to `[{dbname, UINT64_MAX}]` rather
than leaving it empty, so a plain open followed by a table open carrying the
zero-to-one migration reported a mismatch between two requested lists instead of
"this database is already open untiered in this process; the change needs a cold
open". That branch had no test in either direction.

Also drops three comments that restated the code below them.

Refs #767

Co-Authored-By: Claude Opus <noreply@anthropic.com>
… PurgeAll

Round-11 review. The stale-layout regression asserted that `fast` OR `slow` held
an SST before the destroy, which `fast` alone satisfies — and the manual
`compactSync()` it used targets path 0, as does flush output, so `slow` could
legitimately be empty and the per-handle-prefix bug it exists to pin would still
have passed. It now waits for automatic compaction to reach `slow` and asserts
that directly, which is how the neighbouring spill test does it.

`PurgeAll` clears `knownLayouts` too, so the process-global map is torn down
with the rest of the registry's state rather than only on `destroy()`.

Also drops comments that restated AGENTS.md invariants 14/15 next to the code.

Refs #767

Co-Authored-By: Claude Opus <noreply@anthropic.com>
Round-12 review. Clearing `knownLayouts` in `PurgeAll` was a regression I
introduced taking round 11's cleanup nit: `PurgeAll` is reached from the public
`shutdown()`, and a handle retained across that can still be destroyed — which
would then orphan every tiered SST file, the exact failure the cache exists to
prevent. Erased on `destroy()` only.

`findPersistedBlobDir` returned whitespace as a directory name, so an OPTIONS
file with `blob_dir=` padded by spaces made an unpatched build refuse an
untiered database, naming a directory nothing was ever configured with. The
value is trimmed, with GoogleTest cases both ways.

Refs #767

Co-Authored-By: Claude Opus <noreply@anthropic.com>
Refs #767

Co-Authored-By: Claude Opus <noreply@anthropic.com>
The relocation guards compile only into a build carrying the downstream
blob_dir patch, and no prebuild carries it yet — so every integration test
of them skips on every build that exists, and the review's round-12 read was
that they are static-only today. Both of the bounds they encode (how far
allowDirChange reaches, and that an empty blob_dir means the database
directory) were got wrong once each, which is exactly the code that should
not be covered only by reading it.

The decision is moved into Node-free core/blob_relocation.cpp with the two
filesystem questions injected, so a GoogleTest executes it regardless of the
macro — the same reasoning that put the unpatched-build OPTIONS scan in
core/options_file.cpp. No behavior change: the call site builds the same
inputs and throws the same messages.

18 cases, each mutation-checked. Reintroducing the raw-string comparison,
either wrong blast radius for allowDirChange, a trusted acknowledgement or a
dropped existence check fails the case named for it.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Rebasing this branch onto main resolved the db_descriptor.cpp conflict by
taking the branch side wholesale, which deleted every hunk main's #744
(fd4e001) had added to that file while leaving the class declaration in
db_descriptor.h and its seven uses in transaction/transaction.cpp. The
binding then had four unresolved symbols: dlopen fails outright on
Linux/Windows (every Linux, Windows, Bun, Deno, benchmark and stress job
on the last CI run), and macOS links under -undefined dynamic_lookup and
crashes on close() instead.

Re-applies main's four hunks: the <system_error> include, the
ParkTimeoutRegistry method definitions, both parkTimeouts->shutdown()
calls (the destructor safety net and the finishClose() one that exists
because a park can sit on a foreign-dbId tracker cancelForDB() will not
wake), and the comment recording why finishClose()'s flush() keeps the
waiting default. No tiered-storage change is touched; flush(bool
allowWriteStall) was already restored by a2965c0.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
An unset blob_dir does not mean the database directory: RocksDB derives
every blob file path from cf_paths.front(), which falls back to
db_paths.front(), and the downstream blob_dir patch keeps that as the
empty-value behavior. So on a database that also sets `paths`, the flat
blob files sit on paths[0].

decideBlobRelocation resolved an empty directory to the database
directory unconditionally, which made the allowDirChange guard read a
directory the files were never in on a tiered database: it finds no
.blob files, accepts an acknowledgement nobody honored, and opens a
database whose every value at or above min_blob_size is unreadable —
the exact failure that check exists to prevent. It also refused an
acknowledgement across paths[0] and an omitted dir, which name the same
directory.

BlobRelocationInput carries the resolved default (empty still means the
database directory, i.e. an untiered database), the cold open supplies it
from db_paths, and six native cases cover it; three of them fail against
the old resolution.

Also short-circuits assertStoragePathsUsable when paths[0] is spelled
exactly as the database directory — the supported form of the zero-to-one
migration, where it otherwise stats every SST file of the database at the
directory it just listed them from, on every open.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Preserve whether the caller supplied a paths array so an explicit empty list cannot silently inherit a live tiered layout. Cover both omission inheritance and the empty-list conflict.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
A drop retired its registry state in two steps: DBRegistry::RemoveColumnFamilyLayout
took databasesMutex for the layout erasures and released it before
unregisterColumnFamily took columnsMutex. OpenDB's warm path takes those two
locks in that order to decide whether to reuse a family or create a fresh one,
so an open could land in the gap, still find the name in columns, and be handed
a ColumnFamilyDescriptor for a family RocksDB had already dropped — the open
succeeds and every later write fails with "Invalid column family specified in
write batch".

DBRegistry::DropColumnFamily now holds databasesMutex across DB::DropColumnFamily,
both layout erasures, and the columns erase, so an open observes the family
either wholly present or wholly gone. Putting the RocksDB call inside the same
section closes the window that existed before this branch too, between the drop
returning and the unregister starting. The verification-table sweep stays
outside the lock: the VT writer mutex is process-global and must never be taken
under the registry mutex.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
Adds the deterministic regression the atomic-drop change was missing: a
test-only latch holds DBRegistry::DropColumnFamily's critical section open
after the RocksDB drop, and a worker thread — which waits for the drop to be
provably inside that section rather than guessing with a timer — proves its
warm open is serialized behind the whole section and yields a fresh, writable
family. Both assertions catch the split-section regression independently:
mutating the section back into two lock acquisitions makes the open return in
0ms with the dropped family, whose write is silently discarded.

The latch is armed from JS rather than an env var because Vitest's threads pool
runs tests in worker_threads, whose process.env writes never reach ::getenv —
the same reason forceTryAgainForTesting exists.

Also condenses the drop commentary, which narrated the same control flow across
the registry and both callers, and drops an inaccurate claim that a mid-purge
descriptor has left the registry map: PurgeIfUnreferenced deliberately leaves a
closing descriptor there until finishClose() returns, so the loop over the
path's entries already covers it.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
Round-2 review findings on the regression itself:

- The worker's readiness promise had no reject path, so a Bun/Deno/Windows
  loader failure during worker bootstrap turned into a test timeout instead of
  the actual error. Worker error and an exit without a report now reject it.
- The latch counter is process-wide and monotonic, so a rerun in the same
  process started above zero and the worker skipped its wait entirely. It now
  waits for an increment past a baseline the test reads before arming.
- The open-duration assertion depended on the worker getting CPU during the
  first half of the latch. It now asserts the open did not RETURN before
  `dropStartedAt + delayMs`, which the critical section guarantees and which a
  starved worker can only satisfy more easily — scheduling can weaken the test,
  never fail it. Mutating the section back into two lock acquisitions still
  fails it, as does the unchanged write-visibility assertion.

Also condenses the seam commentary to one explanation in core/test_seam.h.

Declined: unregistering the dropped family from the read-only descriptor too.
A read-only descriptor is a separate `DB::OpenForReadOnly` instance whose column
family handles the writable drop does not invalidate, and which by design does
not see later changes; unregistering the name there would only make a
subsequent read-only open throw "cannot create column family in read-only
mode". Layouts are removed from every descriptor because deleted files are
path-global; column registration is per-DB-instance.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
The regression's timestamp assertion could not fail for a starved worker, but
for the same reason it could PASS the split-section regression: a worker not
scheduled until after the latch expired would open after cleanup, get a fresh
family, and satisfy every assertion even with databasesMutex released
mid-section.

The latch is now two-phase. DBRegistry::OpenDB increments a counter immediately
before it acquires databasesMutex (only while the latch is armed, so production
pays one relaxed load per open), and the parked drop waits for that increment
before holding on any further. `dropColumnFamilyLatchStatsForTesting()` reports
both `entered` and `observedOpen`, so the test asserts the two threads actually
met instead of hoping they did — a starved worker now fails the test rather
than quietly weakening it. Anchoring the hold to the opener's arrival also cut
the latch from 1000ms to 250ms.

Mutating the section back into two lock acquisitions fails it deterministically:
the worker receives the dropped family and its write is discarded.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
The opener-attempt counter was process-global and unkeyed, and Vitest's thread
pool shares this native singleton across test files: an unrelated test opening
any database while the drop was parked would be credited as the regression's
opener, restoring the same false-pass window the handshake was added to close.

An armed drop now publishes the path it is parked on, and OpenDB increments the
counter only for an open of that path. The path guard is released before OpenDB
takes databasesMutex, so it cannot invert against the parked drop, which holds
databasesMutex while publishing.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
Three review rounds raised this as a leak, which is a sign the code did not
explain itself. A read-only entry for the same path is a separate
DB::OpenForReadOnly instance whose handles a writable drop does not invalidate,
and which by design never observes later changes; unregistering the name there
would make the next read-only open throw "cannot create column family in
read-only mode" for a family its own frozen manifest view still contains.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
Registry entries are keyed by path AND read-only, so one database can have two
live descriptors. A read-only handle that omits `paths` succeeds whenever the
files it needs still sit at path index 0 — the normal shape when index 0 is the
database directory — and its descriptor's layout names no volumes. Recording
that layout last erased the retained tiered record, which is the only place
`db_paths` survives (RocksDB serializes it in its "not yet supported" block).
If the writer then compacted files onto the other volume, closing both handles
and calling destroy() reported success and left every one of those SSTs behind.

`RecordLayout` now never shortens the recorded `db_paths` — it is append-only
(invariant 18) and persisted nowhere, so the longest list this process has seen
is the canonical one — and `DestroyDB` falls back to the retained record
whenever the live descriptor it captured described no volumes. `applyLayout`
merges rather than overwrites now that it can run twice, so a column family is
named once across both calls.

Regression: a read-only open without `paths` between the first flush and the
compaction that distributes onto the second volume. Mutation-checked — dropping
the RecordLayout guard leaves 8 orphaned SST files after a successful destroy().

Reported by @cb1kenobi on #767.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
The round-25 review found the previous rule still losing volumes two ways.
`DestroyDB` consulted the retained record only when the live descriptor it
captured named none, so a read-only descriptor left open with an explicit
one-path list won and the writer's second volume was swept from nothing; and
`RecordLayout`'s size-only guard let an equal-length or longer ALTERNATE list
overwrite the real paths.

Both disappear once the record accumulates rather than choosing between lists.
`RecordLayout` unions each open's list into the retained one (`mergeDbPaths`)
and `DestroyDB` merges the live descriptor's layout with the retained record
unconditionally. A union needs no rule about which list is authoritative and
loses nothing: the record feeds destroy() alone, where `rocksdb::DestroyDB`
collects the paths into a set, so their order carries no meaning.

Second regression case, alongside the reader that omits `paths`: one opened
with just the database directory and left live across destroy(). Both are
mutation-checked — each leaves SST files on the writer's second volume after a
destroy() that reported success, the first without the `RecordLayout` merge and
the second without the `DestroyDB` one.

Also trims the retained-layout narration the same round flagged as repeated
across AGENTS.md, three comments and the test.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
The round-26 review pointed out that the union has the opposite failure of the
rule it replaced: destroy() deletes every SST and blob file it finds in each
recorded directory, so accumulating whatever any open supplied lets one
mistyped `paths` — a directory belonging to a different database — be recorded
permanently and then swept.

`db_paths` is append-only (invariant 18), so the record has a chain to follow
instead of a choice to make. `extendsDbPaths` takes a new list only when the
retained one is a prefix of it, and both halves of that carry weight: a shorter
list cannot shorten the record (the original finding), and a divergent list
cannot extend it (this one), including the equal-length or longer alternates
the round before that flagged.

Same round, minor: the destroy merge kept the first non-empty blob directory,
so a read-only descriptor opened before a relocation beat the retained current
one and destroy() swept the directory the blob files had left. The retained
record now wins for blob directories — OPTIONS re-derives them on every open,
while a live descriptor's copy is frozen at the open that created it. Left
untested on purpose: `blobs.dir` needs a RocksDB carrying the downstream
blob_dir patch, and the pinned 11.8.1 prebuild does not, so all 28 `blobs.dir`
cases skip here and in CI alike.

New regression: a read-only open naming a neighbouring database's tier volume,
asserting destroy() leaves that database's SST files alone. Mutation-checked
against the size-only rule, which deletes them.

Also trims the retained-layout narration again, per the same round's nit.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
Round 27 fixed `RecordLayout` against a divergent list but left `DestroyDB`
seeding `db_paths` from whichever live descriptor the registry loop picked. A
divergent read-only descriptor still open when the writer closes therefore
seeded the neighbour's volume, and the canonical retained list — which does not
extend that seed — was then rejected, so the neighbour's SST files were swept
after all.

The record now goes first. A live layout adds only what extends it, and only
blob directories for families the record does not name, which is also the
simpler shape: the `retainedRecord` parameter that expressed blob precedence is
gone, since the record is simply applied first.

The regression now leaves the divergent reader live across `destroy()`, so it
covers this path as well as the recording one. Mutation-checked against the
previous ordering.

Also trims the two long local comments per the same round's repeated nit.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
Comments only, per the nit the last three rounds repeated: the retained-layout
failure story lived in AGENTS.md, the helper docstring, the destroy merge
comment, the RecordLayout docstring and the regression preamble at once. The
durable contract stays in AGENTS.md; each site now states only the rule it
enforces, and the regression leans on its name and setup.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
Retain authoritative default placement, let only writable cold opens extend storage paths, and stop live read-only snapshots from expanding destroy targets. Cover the state transition in native tests and both deletion scenarios through tiered-storage integration coverage.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Make refused writable path changes visible in the RocksDB log, bound the drop test latch, and cover the drop-then-warm-create layout regression.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
The retained `db_paths` record is what `destroy()` deletes from, and it only
ever grows. A writable open with a shorter or divergent list was accepted
anyway — warned about in the info LOG and otherwise ignored — which left
RocksDB and `destroy()` disagreeing about where the files are: compaction
writes SSTs to a volume `destroy()` never sweeps, and `destroy()` keeps
sweeping one this open disowned, which another database may since have been
given. Neither is repairable afterwards, and the dangerous shape is the quiet
one: while every file still sits at path index 0, RocksDB opens happily under
any of those lists.

`DBRegistry::AssertDbPathsExtendRetained` now rejects it, before `DB::Open` —
past that point one compaction is already the divergence. `RecordLayout` throws
on the same condition rather than returning a bool nobody could act on, now as
the assertion that the record did not move underneath an open database.

The check preempts `explainOpenFailure`'s guidance whenever the same process
opened the database earlier, so the coverage for that message moves to a
spawned child, where the record is gone and RocksDB reports the MANIFEST as
corrupt — which is what a restart actually produces.

Refs #767

Co-Authored-By: Claude Opus <noreply@anthropic.com>
The rebase moved file-placement rules to invariant 17 and per-column-family creation rules to invariant 18. Keep the local references aligned so maintenance guidance still names the governing invariant.

Co-Authored-By: Claude Opus <noreply@anthropic.com>

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
@kriszyp
kriszyp force-pushed the kris/tiered-storage branch from 03bdc51 to a2e39b2 Compare September 4, 2026 01:00
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