fix: free a column family name immediately on drop (ghost tables)#647
Open
sleekmountaincat wants to merge 1 commit into
Open
fix: free a column family name immediately on drop (ghost tables)#647sleekmountaincat wants to merge 1 commit into
sleekmountaincat wants to merge 1 commit into
Conversation
Root cause of the harper "ghost table" failure. When a column family was dropped while other DBHandles still referenced it (Harper opens every table from multiple worker threads), tryUnregisterColumnFamily left the entry in DBDescriptor::columns (use_count > 2; versions before v2.0.0 had no eviction at all). A later open-by-name in DBRegistry::OpenDB then reused the dropped descriptor and handed back a dangling column family handle: every write failed with "Invalid argument: Invalid column family specified in write batch", which poisoned the database env, wedged table creation, and made drop+recreate-same-name impossible without a process restart. Fix: unconditionally remove the by-name map entry on successful drop (DBDescriptor::unregisterColumnFamily). Instances still holding the dropped column family keep the descriptor alive through their shared_ptr and can continue reading until they close; only the by-name lookup is removed, so a reopen creates a fresh column family. Because the erase now fires on every drop, the columns map gets a dedicated columnsMutex guarding all concurrent accessors: DBRegistry::OpenDB's copy-check-insert, the columns getter, flush() (runs on libuv worker threads; now also pins the descriptors so handles cannot die mid-Flush), close()'s compact loop and clear, and the erase itself. Lock order is databasesMutex before columnsMutex; the drop path takes only columnsMutex, avoiding any interaction with registry-lock holders that wait on in-flight operations (e.g. PurgeAll at shutdown). This intentionally changes drop semantics from deferred-while-referenced to immediate: the old "drop a column family with two database instances" test encoded the deferred contract and has been replaced with a regression test for the new contract. The new test also documents that a write attempted through a still-held dropped handle fails and contaminates the env's shared write path (the poison this fix prevents callers from triggering through reopen-by-name). Removes DBDescriptor::tryUnregisterColumnFamily (its conditional refcount heuristic was the bug; Drop/DropSync were its only callers). Validated: native build clean (macOS arm64), full vitest suite 459 passed / 1 skipped / 0 failed; tsc, oxlint, and oxfmt clean (pre-commit hook bypassed only because the local pnpm shim is broken; its checks were run manually). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Contributor
📊 Benchmark Resultsget-sync.bench.tsgetSync() > random keys - small key size (100 records)
getSync() > sequential keys - small key size (100 records)
ranges.bench.tsgetRange() > small range (100 records, 50 range)
realistic-load.bench.tsRealistic write load with workers > write variable records with transaction log
transaction-log.bench.tsTransaction log > read 100 iterators while write log with 100 byte records
Transaction log > read one entry from random position from log with 1000 100 byte records
worker-put-sync.bench.tsputSync() > random keys - small key size (100 records, 10 workers)
worker-transaction-log.bench.tsTransaction log with workers > write log with 100 byte records
Results from commit c2e927b |
kriszyp
approved these changes
Jun 11, 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
2 participants
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.
Summary
Dropping a column family now unconditionally removes its by-name entry from the descriptor's registry map, so a subsequent open with the same name creates a fresh column family instead of reusing the dangling dropped handle. A new
columnsMutexonDBDescriptorguards all concurrent accessors of the columns map, andflush()/close()now snapshot-and-pin the descriptors they iterate.Purpose
Root cause of Harper's "ghost table" failures (deleted tables resurrecting after restart,
Invalid column family specified in write batchpoisoning a whole database env, wedged table creation). Harper opens every table's column family from multiple worker threads, so the oldtryUnregisterColumnFamilyrefcount gate (use_count <= 2) never fired: the dropped entry stayed registered, a same-name recreate got the dead handle, and one failed write contaminated the env's shared write path until process restart. Reproduced from scratch on a live 2-node Fabric cluster (harper-pro 5.0.27) with a plain table: drop + same-name recreate without a restart breaks the env every time.Where to focus review
test/drop.test.ts). This needs an explicit maintainer call.databasesMutexbeforecolumnsMutexeverywhere both are held (DBRegistry::OpenDB,close()underPurgeAll); the drop path takes onlycolumnsMutex, so it cannot interact with registry-lock holders that wait on in-flight operations at shutdown.flush()pinning (db_descriptor.cpp): runs on libuv worker threads; it now copies theshared_ptrs under the mutex so a concurrent drop/close cannot destroy a handle mid-Flush. Previously this was an unsynchronized iteration that the new every-drop erase would have made a routine data race.Validation
fix/ghost-table-drop-atomicity,unitTests/resources/dropTableGhost.test.js): they fail on the current binding with the incident error (Could not access column family N) and pass with this fix.Coordination
Sibling backport PR targets
v1.4.xfor a 1.4.3 patch release (harper-pro 5.0.x pins^1.4.2). The harper core PR depends on a release containing this fix.Generated by an AI agent (Claude, investigation + fix); reviewed via adversarial agent review and a cross-model (Antigravity) review whose findings (columns-map data race, flush use-after-free, lock granularity) are addressed in this diff.
🤖 Generated with Claude Code