Repository navigation
Defer column-family reclamation behind generation-distinct table stores - #2604
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
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.
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
force-pushed
the
fix/deferred-cf-reclamation
branch
from
September 22, 2026 18:09
89be763 to
afcdd4c
Compare
kriszyp
marked this pull request as ready for review
September 23, 2026 11:21
This was referenced Oct 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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-js2.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
mainnow 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.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.columnFamily.pendingReclaimsis 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.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.tsowns that ordering;serverHandlers.jsunloads matching generations on receiving workers and ignores a delayed event aimed at an older generation.RocksDB table retirement
Durable recovery and blob ownership
What remains recoverable when a create or drop stops between durable lifecycle steps?
databases.tsrecordscreatingandretiredgenerations 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.tsexposes 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).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.dropTableGeneration,dropTableCrossWorkerWrite,Resource-get-context): 22 passing, 2 expected pending.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