Skip to content

Defer column-family reclamation behind generation-distinct table stores - #2604

Merged
kriszyp merged 19 commits into
mainfrom
fix/deferred-cf-reclamation
Sep 24, 2026
Merged

kriszyp merged 19 commits into
mainfrom
fix/deferred-cf-reclamation

Conversation

@kriszyp

@kriszyp kriszyp commented Sep 14, 2026 •

Copy link
Copy Markdown
Member

Dropping a RocksDB table is now a logical removal backed by generation-distinct column-family names and the deferred-drop contract published in @harperfast/rocksdb-js 2.10.0. A durable lifecycle journal and bounded asynchronous blob sweeps let interrupted creates and drops finish without blocking schema reconciliation.

For the human reviewer

  • This PR is designed specifically for rocksdb-js#850. main now pins 2.10.0, whose contract admits commits before retirement, rejects later writes, keeps retained handles readable, and does not let those handles pin physical reclamation.
  • Generation-suffixed RocksDB family names are a one-way storage-format change. Older Harper releases would open bare names and see empty stores; the existing data-version downgrade guard remains the release boundary.
  • The live drop still calls dropSync() before its readable-handle blob sweep. A process crash in that interval can orphan files; this pre-existing durable-obligation gap remains tracked by cleanup_orphan_blobs always throws on zero-table databases but returns 200 "cleanup started" — recovery sweeper silently never runs #2390 because the binding exposes no per-family quiescence callback and Harper does not persist write-path blob identities.
  • Recovery preserves the normal two-second blob retention window, bounds queued unlink verification to 4,096 files, retries a failed sweep three times, then drops the retired family and requires operator cleanup for any remaining files.
  • columnFamily.pendingReclaims is database-wide, so an unrelated reclaim can delay a drop for up to ten seconds. A per-family completion signal would require a binding change.
  • The pre-existing LMDB same-name stale-drop race, fire-and-forget cross-worker routing window, and per-record blob-sweep failure behavior are unchanged and out of scope for this RocksDB change. The new table identity tag prevents a delayed event from deleting a replacement once that event reaches the worker.

Product and architecture tour

Logical drop and generation ownership

How does a drop stop new work without waiting for every already-admitted commit?

The RocksDB path writes a tombstone, unloads the table, awaits the cross-worker DROP_TABLE broadcast, and then retires the exact physical families under the schema lock. Table.ts owns that ordering; serverHandlers.js unloads matching generations on receiving workers and ignores a delayed event aimed at an older generation.

RocksDB table retirement

  1. Persist identity — Stamp a create-time UUID on the primary catalog row and every physical family name.
  2. Stop admission — Tombstone and unload the table, then broadcast the generation being dropped.
  3. Retire exact stores — Merge the catalog's names into the durable generation row and logically drop every registered member.
  4. Finish asynchronously — Sweep blobs through the retained readable handle and remove the journal row after physical reclaims settle.

A same-name recreate always opens a new generation; a delayed old-generation event cannot dispose it.
New calls through a retained dropped Table class fail with 404, while work admitted before retirement is left to the native atomic-commit contract.

Durable recovery and blob ownership

What remains recoverable when a create or drop stops between durable lifecycle steps?

databases.ts records creating and retired generations under a reserved catalog prefix. Restart recovery opens a surviving retired primary family, scans it in bounded batches outside the schema lock, awaits verified unlinks, and only then drops the family. blob.ts exposes completion-aware deletion while preserving cancellation when a blob is referenced again. The live path uses a bounded physical-settlement wait and a second background scan when late admitted commits may still have landed (Table.ts).

The journal row is the single durable recovery obligation. It is removed only after the exact generation is no longer registered and database-wide pending reclamation reaches zero.

Derived storage and regression coverage

Which consumers must follow the generation-stamped physical name?

HNSW plane files inherit the generation from the column-family name and are closed before destructive work (hnswDerivedIndex.ts). The raw RocksDB integration oracle resolves the stamped checkpoint family instead of assuming the catalog key (delete-index-atomicity-rocksdb.test.ts).

Regression coverage exercises the real same-thread source-fill contract (Resource-get-context.test.js), a cross-worker admitted commit (dropTableCrossWorkerWrite.test.js), and generation stamping, same-name recreation, interrupted recovery, blob reclamation, HNSW isolation, redundant drops, retained classes, and delayed stale events (dropTableGeneration.test.js).

Verification

  • npm ls @harperfast/rocksdb-js --depth=0: 2.10.0.
  • npm run build: passed.
  • Focused RocksDB suites (dropTableGeneration, dropTableCrossWorkerWrite, Resource-get-context): 22 passing, 2 expected pending.
  • Targeted oxlint on the four files changed in the final review response: passed. Repository-wide lint reports 15 pre-existing warnings outside this PR's files.
  • Resource core suite on the rebased implementation: 2,524 passing, 43 pending; four loopback-only failures passed 4/4 outside the restricted sandbox, and one unrelated Node 26 import-cycle child test fails because JSON import attributes are missing.
  • Full resource suite earlier in this maintenance run: 2,983 passing, 46 pending, with the same five environment/runtime failures above.
  • LMDB generation suite: 5 passing, 10 pending; the RocksDB-only cases skip as intended.
  • Final external review: Claude, Gemini, Cursor Muse, and the Harper domain adjudicator; Cursor Grok explicitly excluded.

Refs #1381, #2390

Complexity: complicated

Review-Coverage: authored=codex; ran=gemini,cursor-composer,codex,cursor-muse,claude; adjudicated=domain; declined=cursor-grok,cursor-kimi; rounds=20; full=3 @ afcdd4c

Human-Review-Need: 4 (decisions: generation-qualified-storage, blob-sweep-ordering, reclaim-failure-cutoff, lmdb-drop-visibility, sweep-finalizer-locking, untagged-drop-compatibility) @ afcdd4c

@kriszyp kriszyp added this to the v5.3 milestone Sep 14, 2026

@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 generation-distinct store names for RocksDB tables to prevent same-name recreates from binding to dropped column families still on disk. It replaces the previous mechanism of draining in-flight source-populated cache writes with a generation-based retirement and reclamation system, tracked via a lifecycle journal. The review feedback highlights two high-severity issues where the code mutates database keys or column families while iterating over them directly (in Table.ts and databases.ts), which can lead to skipped keys or undefined behavior. It is recommended to wrap these iterables in Array.from() to create a stable snapshot before performing deletions.

Comment thread resources/Table.ts
Comment thread resources/databases.ts Outdated
kriszyp and others added 19 commits September 22, 2026 06:06
dropTable on RocksDB now removes the table from every worker's schema first
(the awaited DROP_TABLE broadcast is the barrier: a worker acks after it has
unloaded the table and awaited its derived-index shutdown), then retires the
column families under the update-attributes lock and lets rocksdb-js drop the
bytes behind commits already admitted. The pendingSourceCommits drain and its
10 s fail-closed timeout are gone: nothing on the write path has to quiesce.

Physical store names carry a generation minted at create (`Foo/@<uuid>`,
`Foo/str@<uuid>`), persisted on the primary catalog row, so a same-name
recreate never binds to a retired family; legacy un-stamped families keep
their names. A lifecycle journal row (`/generation/<uuid>`, phase creating or
retired) makes a create that died before publishing and a drop interrupted
before its physical reclamation both recoverable at the next load, which
reclaims by exact generation rather than a table-name prefix and removes the
row only once no family of that generation is registered and no physical
drop is pending. A retained Table class refuses new operations after the drop.

Refs #1381

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…al on the binding

Round-1 review fixes: the retirement journal row is written before any
family is retired (also from completeInterruptedDrop) and names the stores
explicitly, so legacy un-stamped generations and a drop that fails midway
are reclaimable at the next load; an index this worker never loaded goes
through dropColumnFamily so its plane file is unlinked; a concurrent second
drop reads the generations from the existing tombstone instead of retiring
by bare names; settlePhysicalDrops warns when its bound passes; the two
race tests fail rather than skip on a binding without deferred reclamation,
so a CI run on the un-bumped pin cannot be green; the old drain-contract
test now asserts the new contract; blob-sweep assertions cover both the
plain drop and the racing source-fill write.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…afe unloads

A journal row already present keeps every store it names, so a redundant
concurrent drop that reaches the lock after the first removed the catalog
rows cannot narrow the list; the sweep reclaims by explicit name and by
generation suffix, covering an index family whose catalog row never
committed; a tombstone from before dropGeneration existed still gets a
retirement row. The DROP_TABLE handler marks the drop in progress before
awaiting derived-index shutdown and unloads the table even when that
shutdown rejects, so no rescan during the await completes the drop and no
worker acks while still routing to the table.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…awaiting derived shutdown

A rescan completes an interrupted drop only when it can take the
update-attributes lock without waiting, so a worker that already acked a
drop cannot race the dropper's own locked section from a later broadcast;
a contended attempt is not counted against the retry budget. The
DROP_TABLE handler removes the table from the schema and runs cleanup()
before awaiting derived-index shutdown, so a stalled shutdown cannot keep
the table routable. The concurrent-drop test asserts the retirement row is
present rather than tolerating its absence.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A drop that yielded on its broadcast can resume after another thread
completed it, a same-name create wrote fresh rows, and a later drop
tombstoned those; the removal guard now checks the tombstone's
dropGeneration as well as its presence. A tombstone from before
dropGeneration existed gets one written durably so retries share a single
journal row, and a rejecting derived-index shutdown no longer aborts the
drop before its tombstone.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…test

The oracle opens a checkpoint's index family by catalog key; the physical
family now carries the table's create-time generation, so it is found by
`columns` and the over-time log line is matched with an optional suffix.

Co-Authored-By: Claude Fable 5.1 <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>
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>
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>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
@kriszyp
kriszyp force-pushed the fix/deferred-cf-reclamation branch from 89be763 to afcdd4c Compare September 22, 2026 18:09
@kriszyp
kriszyp marked this pull request as ready for review September 23, 2026 11:21
@kriszyp
kriszyp requested a review from cb1kenobi September 23, 2026 11:21
@kriszyp
kriszyp merged commit 07b2c90 into main Sep 24, 2026
56 of 59 checks passed
@kriszyp
kriszyp deleted the fix/deferred-cf-reclamation branch September 24, 2026 16:49
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.

1 participant