Add secondary (live-follower) open mode via secondaryPath + catchUpWithPrimary; report read-only open races accurately instead of as corruption - #814
Draft
kriszyp wants to merge 36 commits into
Draft
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces support for opening RocksDB databases as secondary instances (read-only followers of a live primary) via the secondaryPath option, along with catchUpWithPrimary and catchUpWithPrimarySync methods to pull in new writes. It also updates transaction log handling to ensure read-only and secondary opens do not mutate the primary's logs or run tail recovery. The review feedback highlights two main areas for improvement: a bug in the directory nesting check when the primary path ends with a slash, and a potential bypass of the workspace exclusivity check if the secondary path is not canonicalized before comparison in the registry.
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 ffe77a1 |
kriszyp
marked this pull request as ready for review
September 2, 2026 04:13
…rruption
A read-only open replays the MANIFEST and then opens each SST it names,
holding no reference on any of them; a compaction in a process actively
writing the database can unlink an input file between those steps. RocksDB
reports that either as a Corruption status blaming the MANIFEST ("The file
MANIFEST-… may be corrupted") or as a bare IOError — and the readOnly open
branch collapsed every IOError to "Database does not exist". Both reports
are false against intact data, and the corruption wording invites repair or
restore actions against a healthy database (#812).
Classify the missing-SST shape (Node-free core/open_status.cpp,
GoogleTest-covered: an IOError/Corruption status naming an .sst with a
not-found signal) and throw a distinct ERR_CONCURRENT_COMPACTION error that
explains the race, avoids asserting a cause the status cannot prove, and
preserves the original RocksDB status text. Genuine corruption (checksum
mismatch, bad MANIFEST record), a missing column family, and a genuinely
missing database keep their existing failure shapes.
DBException gains an optional JS error code forwarded by Database::Open's
catch — the same napi_throw_error code surface THROW_IF_READONLY uses.
Refs #812
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SucX4CMt9xxhDkh6r4DSnJ
…daryPath + catchUpWithPrimary A plain readOnly open is a point-in-time snapshot that loses races against a live writer (see the previous commit); RocksDB's supported follower mode is a secondary instance. Expose it (#812): - `secondaryPath` open option: presence switches the open to DB::OpenAsSecondary with that directory as the secondary's own workspace (`path` stays the primary's data dir). Implies readOnly (explicit readOnly: false throws) so every read-only guard applies; forces max_open_files = -1 — the eagerly-opened, fd-held table and blob files are what make the primary's deletions safe, so an explicit bounded value is rejected rather than silently ignored. - `db.catchUpWithPrimary()` / `db.catchUpWithPrimarySync()` bindings for TryCatchUpWithPrimary (serialized per descriptor; the async form pins the descriptor with the #672 purge-retry discipline), plus a `db.secondaryPath` getter. Non-secondary handles throw ERR_NOT_SECONDARY. - Registry identity gains the workspace: DBKey is {path, readOnly, secondaryPath}, with descriptorKey() as the single reconstruction point so the close/purge paths (CloseDB, backup, backup-stream, checkpoint destructors) cannot miss a secondary's entry and leak it. - Workspace exclusivity is enforced, not documented: RocksDB itself accepts a second secondary on the same workspace (proved by the new native test) and the instances would corrupt each other, so a kernel advisory lock on <secondaryPath>/.secondary.lock (the backup-lock utility) excludes other processes and the registry rejects in-process reuse across primaries. - The missing-file race classifier now matches .blob and .log names too: driving a real parallel writer showed the read-only open tripping on blob-GC'd .blob files and flushed WAL segments, both previously misreported as "Database does not exist". - Docs: README + TSDoc now state the readOnly live-writer hazard (including post-open lazy reads) and document secondary mode, its fd footprint, and upstream's secondary+BlobDB stance (facebook/rocksdb#13296) — the pinned build fd-holds blob files like SSTs, verified by test/native/secondary_blob_test.cc, which is the regression net for every RocksDB upgrade. Race proof (test/secondary.test.ts): a worker-thread writer drives continuous write/flush/compaction traffic while read-only opens race it — failures are classified ERR_CONCURRENT_COMPACTION (never corruption, never "does not exist") — and the same churn through secondary opens/catch-ups/reads runs clean. Refs #812 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SucX4CMt9xxhDkh6r4DSnJ
…wn-safe catch-up, destroy closes all descriptors Fixes from the pre-push cross-model review (codex + gemini + harper-domain): - A read-only or secondary open no longer mutates the primary's transaction logs: store discovery under a readOnly registration skips the retention purge and recoverTail's truncation (the log directory may belong to a live writer in another process — reader truncation is how acknowledged writes vanish, invariant 5 / the harper#2016 class). Readers tolerate the unrecovered torn tail via the CorruptFrameError/resync protocol. The inverse hazard is guarded too: a writable open is refused while read-only-loaded stores are live in-process (appends must not land past an unrecovered tail), checked before the descriptor exists so the throw cannot mis-decrement the read-only entry's refcount. - Async catchUpWithPrimary now claims operationsInFlight before queueing (checkpoint's discipline): finishClose waits on that counter unbounded, while the async-work drain it otherwise relied on is bounded and its failure ignored — a long replay could have descriptor->db reset under it. - DBRegistry::DestroyDB claims and closes EVERY descriptor for the path (read-write, read-only, each secondary) instead of closing one and erasing all: an entry erased unclosed leaks its resources, and for a secondary the workspace .secondary.lock is only released by finishClose, wedging the workspace for the life of the process. - secondaryPath must not alias the database path (canonical comparison). - The race classifier matches extensions at a filename-token boundary, so a directory named like an SST/WAL cannot ride path text into the classification; applied to the secondary open branch as a safety net — and a new test documents that OpenAsSecondary's point-in-time replay tolerates a missing file by serving the last fully-present version. Refs #812 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SucX4CMt9xxhDkh6r4DSnJ
…sting guards, teardown hardening - TransactionLogFile gains a readOnly mode threaded from the store: the fd/handle opens read-only without creating the file or its parent dir (a follower can read logs on a read-only-mounted or foreign-uid volume), an existing append-boundary marker is read but never created, repaired, or removed (the marker tree belongs to the writer, possibly live in another process), an empty file is rejected instead of header-initialized, and the Windows mapping path clamps to the physical size instead of extending the file. Round 1 had gated only recoverTail and retention. - DiscoverStores never re-loads a store already live in this process: load()'s tail recovery ran before emplace could decide the store was a duplicate, so any second open of a path re-recovered (and could truncate) the live writer's active segment (invariant 5). - The workspace guard fails closed (lexically-normalized fallback when canonicalization errors) and also rejects a workspace nested inside the primary — destroy()'s recursive delete would take a live workspace with it. - SecondaryLockGuard is declared before the RocksDB instance so a throw between OpenAsSecondary and ownership transfer destroys the instance before the workspace lock releases. - DestroyDB's multi-descriptor close is exception-safe: a throw from one finishClose no longer strands later descriptors mid-close with their entry conditions never notified (which wedged every later open of those keys). - The probabilistic race test skips (with a warning) when the environment never reproduces the race instead of failing a correct product; the secondary-side zero-failure assertions stay hard. - Docs: the secondary's transaction-log view is frozen at open (catch-up advances the database view only), catch-up calls should be serialized per database, and in-process workspace reuse wording matches the implementation (rejected for a different database; same database shares the follower). Refs #812 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SucX4CMt9xxhDkh6r4DSnJ
…d-only ResolveStore, config-shape hardening - The async catch-up worker checks only the atomic isCancelled(), never handle->opened(): opened() reads the handle's non-atomic descriptor while a concurrent close() resets it on the JS thread, and close's bounded async-work drain makes that overlap the designed case, not a corner (AGENTS invariant 9; CreateCheckpoint's discipline). The worker's own descriptor pin is what keeps the replay safe. - ResolveStore honors a read-only registration: useLog() for a store the primary created after a sole-registrant follower opened loads it read-only, and a store that does not exist is reported as absent instead of mkdir'd (with writable, header-initializing files) into the primary's tree — which also fabricated a phantom store that later failed EnsureWritableRegistrationSafe. Its config pointer is now copied under the lock like DiscoverStores'. - secondaryPath: null (the natural absent value from JSON/env-derived config) is treated as absent in TS, matching the native layer's napi_null handling — it no longer implied readOnly while native ignored it, which silently produced the point-in-time open the option exists to replace. - The workspace/path guard compares case-insensitively on Windows/macOS (case-insensitive default filesystems), so a re-cased spelling of the primary cannot nest a workspace inside it. - The probabilistic race test skips when the environment cannot reproduce the race; the blob regression net asserts its headline hazards (old blob after primary GC, fd-hold evidence on Linux) instead of printing them. Refs #812 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SucX4CMt9xxhDkh6r4DSnJ
… null-store guards The round-3 read-only ResolveStore branch introduced two defects the round-4 review caught: its nullptr return could reach an unguarded bind in Transaction::UseLog, and its lazy load-and-publish both contradicted the documented frozen-at-open log view and let a writable open that had already passed EnsureWritableRegistrationSafe adopt an unrecovered, O_RDONLY store published behind its back. - A read-only registration now reports an undiscovered store as NOT FOUND — never creates one, never lazily loads one. useLog() on a read-only or secondary handle throws 'Transaction log "<name>" not found' at creation (a handle with a null store broke in the reader on first query), and the read-only transaction shim surfaces the same error through txn.useLog. - Null-store guards at the native bind boundaries (Transaction::UseLog, TransactionLogHandle::addEntry) as defense in depth — unreachable today because read-only transactions never construct a native transaction, but the resolve contract now admits null. - The churn test tolerates (and logs) open-failure shapes other than the classified race, still asserting the acceptance bar — never a corruption claim, never "Database does not exist". - Comment accuracy: the Linux deleted-fd count is labeled supporting evidence (it includes the primary's own handles); narration trimmed. Refs #812 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SucX4CMt9xxhDkh6r4DSnJ
napi_new_instance failing because the TransactionLog constructor threw left a pending exception that NAPI_STATUS_THROWS overwrote with a second, generic throw — Node ignored the second throw, Bun surfaced it as "An exception is pending". Return with the original exception intact instead. Refs #812 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SucX4CMt9xxhDkh6r4DSnJ
…not the path-global config Register's read-only->writer upgrade of the shared entry config never un-flips (Unregister only decrements the refcount), so a process that opened a writer, then a secondary, then closed the writer kept writer semantics for the entry's life — a later secondary useLog of an undiscovered name would mkdir into what may by then be a foreign live primary's transaction_logs tree and observe post-open log state the frozen-view contract forbids. ResolveStore now takes the CALLER's readOnly mode: a read-only caller gets only stores already live in this process, regardless of what the entry config says. The entry config keeps governing discovery at open, where it is registration-scoped and correct. Refs #812 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SucX4CMt9xxhDkh6r4DSnJ
cb1kenobi
reviewed
Sep 2, 2026
kriszyp
force-pushed
the
feat/open-as-secondary
branch
from
September 2, 2026 04:43
f9f1e13 to
206c2d1
Compare
kriszyp
marked this pull request as draft
September 2, 2026 04:43
Test failures: - secondary.test.ts asserted the whole error message never says "may be corrupted", but the classified ERR_CONCURRENT_COMPACTION message deliberately preserves RocksDB's raw status in a trailing parenthetical, and that text is where the wording comes from. Assert the leading explanation instead. - readonly.test.ts asserted a writable open physically shrinks a torn log file. Windows sections are mandatory, so a live mapping (a reader's, possibly awaiting GC) blocks SetEndOfFile and the file cannot shrink. Recovery now zero-fills [validEnd, size) there instead, restoring the zero-timestamp end-of-entries marker — leaving the bytes is not benign, since appends resume at `size` and a later shorter batch would leave them reading as an entry. Covered deterministically by a new Windows GoogleTest; the JS test accepts either outcome on Windows. Review comments: - Store discovery now takes the OPENING handle's mode instead of the path-global entry config: a writer that has since closed left the entry stamped writable, so the next secondary's discovery of a newly-appeared store ran retention purge and recoverTail truncation against a live primary's logs. The entry no longer carries a mode at all, and EnsureWritableRegistrationSafe decides on each live store's own flag. - A cancelled catch-up no longer leaks its operationsInFlight claim (which would wedge finishClose forever): AsyncCatchUpState owns an exactly-once release, run by execute or by its destructor. - secondaryPath is resolved once at parse time, so two spellings of one workspace share a registry entry instead of colliding on the advisory lock; the nesting check handles a filesystem-root primary. - Documented that a secondary's log view is "stores discovered at open", and that the log leads the database view for every writer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- eraseTail() on Windows falls back to the same zero-fill as recoverTail(): the discarded bytes are whole entries, so a mapping that blocks SetEndOfFile must not leave them for the next batch's flag to swallow. - Document the resolved workspace in DBKey and drop the now-stale canonicalization comment in the nesting check. - The workspace-reuse test matches the resolved spelling (the temp dir is a symlink on macOS). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- zeroTailLocked has a commit point: it writes back to front so the zero AT the boundary lands last (a partial failure leaves the original torn tail, never a premature end marker), takes the entries end explicitly rather than re-reading `size`, and on any failure retires the segment — `size` drops to the boundary and appends are refused, so the store rotates instead of appending above zeros where no reader would ever reach it. New Windows GoogleTest injects the failure with a read-only handle. - ResolveStore refuses to hand a writer a store another handle loaded read-only. EnsureWritableRegistrationSafe only covers the open; a store can appear after a writer is already open and be discovered read-only by a later secondary, and the writer would then append past a torn tail. - isPathWithin compares filesystem identity (equivalent() over the child's ancestors) instead of folding case by platform: case sensitivity is a volume property, so the fold rejected a distinct /data/DB vs /data/db. - Docs: a secondary's log view is what this process has resident — an in-process writer's new stores and appends ARE visible; only a cross-process primary's need a reopen. Removed the stale "frozen at open" wording and the references to the deleted config mode field. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- resolveTransactionLogStore's new writer rejection (and the pre-existing mkdir failure) is a DBException, which derives from std::exception, not std::runtime_error: it escaped TransactionLog's constructor and slipped past transaction.ts's narrower catch, aborting the process instead of throwing to JS. Both entry points now catch std::exception; collectStats swallows it (stats are not worth failing over). - A segment retired by a failed zero-fill persists its boundary marker and rotates in load(), where the retirement is decided. The write path's persist-then-rotate only runs from writeEntries' catch, and writeBatch's max-age check can rotate ahead of it — losing the boundary, so a restart would pull the orphaned tail back inside the logical file. - recoverTail applies the same bookkeeping (index reset, lastFlushedSize clamp, operator warning) when the boundary moved by retirement rather than by repair; a stale index entry otherwise points into the discarded range and a query through it reads a zero and returns nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e on a transaction boundary - The retire-at-load block sat outside load()'s `!readOnly` guard, so a read-only or secondary open wrote (and fsynced) into the primary's append-boundary marker tree — the rule invariant 18 exists to state — and failed the open outright where that tree is not writable. Recovery and retirement are now both writable-load-only, and persistAppendBoundaryRetirement refuses a read-only file outright so no future call site can reintroduce the write. - A segment retired because its zero-fill failed kept whole entries of an unclosed transaction inside its logical extent. The marker is a retired segment's only eraser, so that prefix would be closed by the next segment's first flagged batch, merging two source transactions into one (invariant 14 spans rotations). Retirement now ends at the same transaction boundary a repair erases to, via unclosedTransactionBoundary shared by both paths. New Windows GoogleTest. - Regression test for the writer-adoption rejection at both useLog surfaces: it throws a DBException, whose escape from an N-API callback aborts the process rather than failing a call, so a green suite said nothing about it. - Trimmed comments that narrated review history or restated a callee's header. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015WgdbGX7qcRccULRysbKfZ
…entity - The transaction-log store registry keyed on the raw dbPath while this PR resolves secondaryPath through resolveIdentityPath for exactly this reason. Two spellings of one directory (trailing slash, relative path, a symlinked /tmp) opened two entries over one transaction_logs tree, so the read-only/writable guards looked past each other: a writer could run recoverTail()'s truncation on a segment a reader had mapped and handed to JS, whose next access is an uncatchable SIGBUS. Every entry point now resolves the key first (before taking entriesMutex, since it touches the filesystem). Regression test asserts the guard fires across spellings — it fails without the fix. - The secondary ERR_CONCURRENT_COMPACTION message carries the same hedge its read-only sibling does: a follower whose primary genuinely lost a file should not retry forever against "the database is not corrupt". - The read-only append-boundary marker read drops its redundant exists() gate, which was also swallowing the stat error the writable sibling throws on. readTransactionLogAppendBoundaryMarker already returns 0 for an absent marker and fails closed otherwise. - Docs: name the libuv pool-exhaustion risk of overlapping catch-ups rather than only saying to serialize them. - Trimmed comments that narrate the line below them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015WgdbGX7qcRccULRysbKfZ
…t open - Round 10 resolved the transaction-log registry key on every call, which is CWD-dependent: weakly_canonical/absolute consult the process CWD, and nothing absolutizes the database path (the README's own examples pass a relative one). A process.chdir() after open then remapped a live handle's key — the next useLog() would miss the entry and, for a writer, create a second store outside the database, while Unregister() at close would miss the original and leak its stores. The key is now resolved once and carried on the descriptor (logRegistryKey), the way secondaryPath already is, and the registry consumes it verbatim. - discardUnclosedTransaction treated a failed erase as "nothing happened", but the Windows retire-on-failed-zero-fill path has already dropped `size` to the boundary before returning false, so it skipped the index reset and the lastFlushedSize clamp — a timestamp query would then start past end-of-entries and return nothing for entries that still exist. It now applies the same bookkeeping recoverTail's retire branch got. New Windows GoogleTest. - Dropped the four comments that restated the two assignments below them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015WgdbGX7qcRccULRysbKfZ
…s follower test - The registry identity was resolved twice inside one open (a local for the pre-descriptor guard, then again in the constructor). They agree in ordinary cases, but a concurrent CWD change or a transient weakly_canonical failure splits them and Unregister then misses the entry it registered — leaking its stores and, if any were read-only-loaded, wedging every later writable open of that path. The local is now passed to the constructor: one resolution per open. - The secondary-workspace exclusivity scan compared raw primary path text while both workspaces are resolved, so the same database spelled two ways was rejected as "already in use by database <itself>". It compares resolved identity now. - New test: a primary in a separate PROCESS writing, flushing and compacting while a follower catches up and reads. The existing race test drives the writer from a worker thread, which shares this process's log-store registry — the configuration whose log semantics differ, so the cross-process case the mode exists for was untested. - The filename-token terminator set gained \r, \t and ';': a Windows status can end the line right after the filename, and Windows is the platform whose wording the classifier explicitly matches. New GoogleTest. - The secondaryPath nesting check runs before the workspace is created, so a rejected path no longer leaves an empty directory inside the database. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015WgdbGX7qcRccULRysbKfZ
…stries - The database registry key was still the caller's raw path spelling while the workspace scan compared resolved identity, so an alias-spelled second handle to the same database and workspace passed the scan and opened a second secondary instance on that workspace: on a normal volume the .secondary.lock rejected it (the right outcome, from the wrong layer, with a misleading message), and where that lock degrades to a no-op (FUSE/9p, invariant 7) nothing stopped it. DBKey.path is now the resolved identity, DBRegistry::OpenDB resolves it once — before the lock, not per entry inside it — and passes it to DBDescriptor::open, which carries it as identityPath for the transaction-log registry too. One resolution per open, one identity for both registries. DestroyDB resolves the same way, or it would walk past the descriptors whose files it deletes. - The cross-process test now exercises the log half of the contract, which is the half that actually differs cross-process: a primary's appends to a store the follower already holds are not visible, a store it created after the follower opened is not found, and a reopen picks up both. - The read-only branch of ensureAppendBoundaryMarker was duplicated verbatim across both platform files despite touching no fd or HANDLE; it moves to the shared file. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015WgdbGX7qcRccULRysbKfZ
…the boundary Making DBKey.path the resolved identity left the raw caller string still selecting the filesystem target: DestroyDB claimed and erased entries by identity but then ran rocksdb::DestroyDB and remove_all on the unresolved text, and DBDescriptor::open opened RocksDB at the raw path after being handed the identity. The two disagree whenever the mapping changes in between — a relative path plus a process.chdir(), or a symlinked data directory repointed while destroy is closing descriptors (which flush, wait on compactions and join threads, so that window is not microseconds). Database::Open now resolves the path once, where it enters the addon and where secondaryPath is already resolved, and everything downstream uses that one string: both registry keys, the RocksDB target, the default transaction-logs directory, and destroy's deletions. Destroy deletes what it closed, so a database reached through a symlink has its real directory removed and the link left dangling — noted in the code. Also drops the third resolution of the same path in the secondary nesting check. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015WgdbGX7qcRccULRysbKfZ
…oss its wait Both outside legs landed on the same hazard. DBRegistry::OpenDB held a reference into the registry map while parked on that entry's condition variable, and the wait releases databasesMutex. DestroyDB erases every entry for a path, so the node could be destroyed under the waiter: the reference dangles and a condition variable is destroyed with a thread on it, which is undefined behavior. Pre-existing, but this PR multiplies the entries per path (read-write, read-only, one per secondary), so "an entry for this path is erased while another thread waits" moves from rare to routine. The wait is now a re-check loop that re-finds the entry after every wake and parks on whatever condition the CURRENT entry carries — an entry erased and re-created while we waited has a new condition, and staying on the old one would miss its notify. The lock is held across DBDescriptor::open itself, so the wait was the only window. Also drops three comments that restate the lines under them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015WgdbGX7qcRccULRysbKfZ
…rk is queued An N-API failure between allocating AsyncCatchUpState and queueing its work returned without deleting it. For a secondary that is worse than the leak the repo already tolerates on this shape elsewhere: the leaked state holds a strong descriptor pin, so PurgeIfUnreferenced never sees the count drop and the follower's RocksDB and its <secondaryPath>/.secondary.lock stay held for the life of the process, leaving that workspace unopenable by anything on the host until restart. The state is now owned by a unique_ptr released only once the queue accepts the work, its async work is deleted on that failure path (the base destructor asserts it was), and a failed queue unregisters the handle's async-work count, which used to be left permanently non-zero and timed out every later close. The count is still taken BEFORE the queue: the worker can start the moment the queue accepts it, so close()'s drain has to already be counting it. The state takes the in-flight claim with it at allocation: with two owners the RAII claim and the state's destructor both decremented on a failure path, underflowing a counter finishClose() waits on unbounded — a close that never returns. Also trims three comments that restate their own header or the line below. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015WgdbGX7qcRccULRysbKfZ
…r writes on Windows - Round 16's ownership fix destroys the state on a setup failure, and ~BaseAsyncState unregisters async work unconditionally — so a failure before registration decremented a count that was never incremented, and close()'s drain would then report "nothing in flight" while a worker was still running. Registration now precedes the allocation, so every destroy is balanced, and setup deletes the resolve/reject references the destructor only drops. - Windows openFile() created the parent directory and restamped the file's DACL outside the readOnly branch, unlike POSIX: a secondary would recreate a store directory inside a live primary's tree after the primary purged it, and the exists() check races a segment the primary is creating right now. Both are writer-only now (invariant 18). - Documents the mapped-tail hazard this mode's cross-process reading inherits: a follower's MAP_SHARED overlay covers an unrecovered torn tail, so a primary restart whose recovery truncates below a mapped page can SIGBUS the follower. Serving read-only stores through positional reads is the fix and is left for its own change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015WgdbGX7qcRccULRysbKfZ
Resolving the database path at the addon boundary made every path the API hands back the resolved spelling, which CI caught on macOS: purgeLogs returned /private/var/... where the caller passed /var/... . That is a behavior change for any deployment reaching a database through a symlink, not just a test detail — a blue/green `current` link does the same. The resolved identity is what the registries key on and what RocksDB opens, which is what keeps identity and filesystem target from disagreeing; it is no longer what the caller sees. Database::Open keeps the spelling it was given, so the descriptor's path, the default transaction-logs directory and every returned path and message stay as the caller wrote them, and the secondary workspace conflict names the other database the way its own caller spelled it. Covered by a test that opens a database through a symlink and asserts the returned log path keeps the link spelling — it fails without this change, on every platform rather than only where the temp directory happens to be a symlink. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015WgdbGX7qcRccULRysbKfZ
cb1kenobi
reviewed
Sep 3, 2026
cb1kenobi
reviewed
Sep 3, 2026
Second surface of the same mistake CI caught: registryStatus() reported the key's resolved identity, so a caller matching entries against the path it opened found nothing wherever the two spell one directory differently — every macOS run, and any deployment behind a symlink or a Windows short path. It now reports the descriptor's own path. The symlink test asserts this surface too, so the next one of these fails on every platform instead of only where the temp directory happens to be aliased. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015WgdbGX7qcRccULRysbKfZ
cb1kenobi
reviewed
Sep 3, 2026
…s identity Destroy re-derived the database's identity from the caller's spelling instead of using the one the descriptor resolved at open, so a repointed symlink or a CWD change between open and destroy would name a different directory: the claim loop would match no entry, leave the caller's own descriptors open, and then delete that other directory's files. It now passes the descriptor's identity, falling back to the opened path for a handle that is already closed and has no descriptor left — dereferencing it there crashed the suite. ListColumnFamilies was still probing the caller's spelling while the open two dozen lines later used the identity; both now agree. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015WgdbGX7qcRccULRysbKfZ
…portable Both failed under Bun on CI while passing everywhere else, and neither was a product defect. The workspace-conflict assertion interpolated a database path into a RegExp. On Windows every backslash then becomes an identity escape, so the pattern could not match the literal path it was built from. It checks substrings now, which also states the point directly: the workspace is named by its resolved spelling, the database by the spelling its own caller used. The cross-process test assumed one catch-up after the writer exits sees the writer's last round. Catch-up replays what the primary has made visible, and an exited writer's final MANIFEST/WAL records can still be landing, so it now retries within a bound. The assertion is unchanged: the follower must reach the writer's last round. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015WgdbGX7qcRccULRysbKfZ
cb1kenobi
reviewed
Sep 3, 2026
…r close destroy() on a closed handle had no descriptor left and fell back to re-resolving the caller's spelling, which follows a mapping that may have moved since the open — a repointed symlink or a changed CWD would then delete a different database than the one this handle opened. The handle now copies the descriptor's identity at open and keeps it, so destroy targets what it opened whether or not the handle is still open. Also tightens the cross-process test's catch-up retry budget from 40 to 5: enough to absorb an exiting writer's last records still landing, not enough to hide a case where one catch-up is systematically insufficient. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015WgdbGX7qcRccULRysbKfZ
…s durability
Three findings from the final review round.
destroy() on a handle that was never opened passed an empty path into the
identity resolution, and libc++ implements the standard's `absolute("")` as
`current_path() / p` — the process working directory, with no error. Destroy
ends in remove_all(), so `new RocksDatabase(path); db.destroy()` would have
deleted the directory the process was running in on macOS. libstdc++ errors
instead, which is why nothing here ever showed it. Three layers refuse it now:
an empty path resolves to nothing, the registry rejects an empty destroy
target, and destroy rejects a handle with no path. The test drives it from a
child process with a throwaway working directory, so a regression cannot reach
anything real; with the guards removed and the libc++ behavior simulated it
fails with "destroy() deleted the working directory".
The Windows zero-fill claimed that writing back to front keeps the byte at the
boundary from landing first. That ordered the calls, not their durability: the
cache and the drive may persist them in any order, and a crash that persisted
the boundary first would leave a clean-looking end-of-entries marker with live
bytes behind it. Everything above the boundary is now flushed before the
boundary chunk is issued.
Adds the missing coverage for close/destroy racing an in-flight
catchUpWithPrimary(), the most intricate of the new lifecycle code, including
an assertion that the database leaves nothing in the process-global registry.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015WgdbGX7qcRccULRysbKfZ
cb1kenobi
reviewed
Sep 3, 2026
… tear down through the writable handle
Both from the last round, both correct.
The durability barrier sat at a 64 KiB offset, so it only separated the
boundary from bytes above 64 KiB. For any tail smaller than that — the common
case, one partial entry — no intervening flush ran at all and the boundary
landed in the same buffered write as the stale bytes behind it, which is the
reordering the barrier exists to prevent. The boundary write is now its own
sector: everything above it is durable before it is issued, and a sector is
the smallest unit a drive writes atomically, so a crash either lands those
zeros or leaves the torn tail.
The new teardown regression tore down through the secondary handle, which is
read-only, so destroy() stopped at the read-only guard and never reached the
registry — it asserted a throw that came from somewhere else entirely while
its comment claimed otherwise. It now tears down through the primary, which
claims and closes every descriptor for the path: destroy reaches
DestroyDB and fails on the reference the in-flight catch-up still holds
("1 reference(s) still held after closing all handles"), which is the path the
test exists to cover.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015WgdbGX7qcRccULRysbKfZ
cb1kenobi
reviewed
Sep 3, 2026
…r, cover the purge retry The boundary write started at an arbitrary entry offset and ran a fixed 512 bytes, so it straddled two sectors whenever that offset was not aligned — the reordering window was narrowed, not closed. It now runs from the boundary to the end of the sector containing it, so the drive either lands those zeros or leaves the torn tail, with no state where a clean end-of-entries marker sits above bytes that are still live. The teardown test reached destroy's own erase but never the other half of the same hazard: a plain close() while a catch-up is still running skips the purge, because the operation holds a descriptor reference, and only the async state's destructor retries it. Without that retry the entry, its RocksDB and the follower's workspace lock outlive every handle (#672). The test now drives that path too. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015WgdbGX7qcRccULRysbKfZ
…wing Rounding the boundary up to an absolute sector offset wrapped to zero for any boundary in the last sector of the uint32_t extent, which sent the chunk loop back to zero-fill the whole segment — header included — and made the boundary write read past its buffer on the way. Reachable on Windows with a log size configured near 4 GiB and truncation blocked by a live mapping: recovery meant to erase one partial entry would erase the entire segment. The length is measured as a remainder instead, so the sum cannot overflow: the boundary chunk is at most the rest of its sector, and the boundary plus the tail is the end of the entries by construction. Checked against a boundary already sector-aligned, a tail shorter than the remainder, and the last sector of the extent. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015WgdbGX7qcRccULRysbKfZ
Member
|
Cool! This is dope. |
Recheck secondary workspace ownership after every close wait, retain closing descriptors through spurious wakes, and keep destroy and transaction-log I/O pinned to their captured filesystem identities. Classify localized missing RocksDB files without mistaking genuine corruption. Co-Authored-By: OpenAI Codex GPT-5.6 <noreply@openai.com>
Retain the mode of the last successful open, wait for descriptors already closing before destruction, and cover captured destroy identity across symlink repoints. Co-Authored-By: OpenAI Codex GPT-5.6 <noreply@openai.com>
Retain requested read-only mode across failed opens and keep registry entries visible until physical deletion completes, so waiting opens cannot recreate a database underneath destroy. Co-Authored-By: OpenAI Codex GPT-5.6 <noreply@openai.com>
Make every registry key for a physical path wait behind close/destroy, and evict read-only-loaded log stores when the last reader unregisters so a live writer can reopen them safely. Co-Authored-By: OpenAI Codex GPT-5.6 <noreply@openai.com>
Do not evict an unrecovered store into the constructor path, which would append past an existing torn tail. Writers continue to require a full path reopen before adopting it. Co-Authored-By: OpenAI Codex GPT-5.6 <noreply@openai.com>
Explain that safe writer adoption requires all process-local handles to close before a writable reopen. Co-Authored-By: OpenAI Codex GPT-5.6 <noreply@openai.com>
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.
Opening a database with
readOnly: trueagainst a directory another process is actively writing could fail with RocksDB's "The file MANIFEST-… may be corrupted" wording (or a blanket "Database does not exist") when the writer's compaction, blob GC, or flush deleted a file mid-open — a false report against intact data that invites a repair or restore during an incident. This PR fixes the report and adds the mode that makes a live follower actually correct: RocksDB secondary instances.Part B — honest classification. A Node-free classifier (
isMissingSstOpenRaceincore/open_status.cpp, GoogleTest-covered) recognizes a status naming a numbered.sst,.blob, or.logat a filename-token boundary, then the open path confirms that file is absent inside the captured database directory without relying on localized OS error text. The read-only branch throws a distinctERR_CONCURRENT_COMPACTIONerror that explains the race, preserves the original RocksDB status text, and stops claiming corruption or nonexistence. All three file classes were observed empirically (a parallel writer reclaims compaction inputs, GC'd blob files, and flushed WAL segments). Genuine corruption (checksum mismatch, bad MANIFEST record), a genuinely missing column family, and a genuinely missing database keep their existing failure shapes.Part A — secondary mode. A new
secondaryPathopen option switches the open toDB::OpenAsSecondary:pathstays the primary's data directory, andsecondaryPathnames the secondary instance's own workspace (created if missing, required to be outside the primary directory). Newdb.catchUpWithPrimary()/db.catchUpWithPrimarySync()methods replay the primary's MANIFEST + WAL to advance the follower (ERR_NOT_SECONDARYon other handles), anddb.secondaryPathidentifies a follower handle. A secondary impliesreadOnly(an explicitreadOnly: falsethrows; anullsecondaryPath is absent, matching the native layer) and forcesmaxOpenFiles: -1— the eagerly opened, fd-held table and blob files are what make the primary's deletions safe, so an explicit bounded value is rejected rather than silently ignored.Three enforcement pieces are load-bearing. Registry identity:
DBKeygains the workspace, withdescriptorKey()as the single reconstruction point so every close/purge path (CloseDB and the backup/backup-stream/checkpoint purge-retry destructors) finds a secondary's entry instead of leaking it;DestroyDBnow claims and closes every descriptor for the path. The database path is resolved to filesystem identity once per open, and that one string is both registry keys, what RocksDB opens, and what destroy deletes — so two spellings of one directory cannot hold two descriptors and open one workspace twice, which is the in-process half of exclusivity that the kernel lock cannot cover where it degrades. Callers still see the spelling they passed: identity keys the registries, it is not what any API hands back. Workspace exclusivity: RocksDB itself accepts a secondOpenAsSecondaryon the same workspace (proved by a native test) even though the instances corrupt each other's state, so the binding takes a kernel advisory lock on<secondaryPath>/.secondary.lock(the backup-lock utility, with its documented FUSE/NFS degrade caveats) and the registry rejects in-process reuse across primaries. Read-only log access: a read-only or secondary open no longer mutates the primary's transaction logs — discovery skips the retention purge,recoverTailtruncation, and segment retirement (reader truncation of a live writer's active segment is how acknowledged writes vanish — invariant 5, the harper#2016 class), log files open with read-only descriptors and never create, repair, or remove append-boundary markers, and a writable open is refused while read-only-loaded stores are live in-process. That guard only meets both handles when they land on one registry entry, so the log-store registry keys on the same resolved identity: two spellings of one directory used to open two entries over one log tree and let a writer truncate a segment a reader had mapped, whose next access is an uncatchable SIGBUS.For the human reviewer
Judgment calls from the review's decision ledger, with what was chosen and why. Items 1 and 2 were consulted before implementation (needs-input, 2026-09-01) and ruled: proceed with secondary mode over BlobDB backed by the empirical regression net, and spell the API
secondaryPath.Shipping over upstream-unsupported BlobDB (ruled): upstream documents secondary+BlobDB as unsupported (facebook/rocksdb#13296) while this library enables blob files unconditionally. Chosen: ship, with
test/native/secondary_blob_test.ccas the assertion-only regression net (the pinned build fd-holds blob files exactly like SSTs — verified via/proc/self/fd) and the caveat documented in README/TSDoc/AGENTS. Alternative: disable blob files for secondaries — an on-disk format decision. A RocksDB upgrade that regresses this fails the native suite.API shape (ruled):
secondaryPath?: string, presence switches the mode; rejectedsecondary: {path}(no second sub-option exists) and amodeenum (collides with optimistic/pessimistic).A secondary's log view is what this process has resident, not a frozen snapshot:
catchUpWithPrimary()advances the RocksDB view only. A store this process never discovered stays not-found (useLog()throws rather than conjuring or lazily loading one), and a cross-process primary's new stores and appends need a reopen — but a store an in-process writer holds open is the same object, so its appends are visible. That is not unsafe state: the log write completes before the RocksDB commit for every writer, so log-leads-database is the normal direction and every consumer already tolerates it. Alternative: a uniform snapshot contract, which would mean binding each reader to a private store copy; documented in README, TSDoc and AGENTS instead, so changing it later is a docs-and-behavior break.Writable open refused while read-only-loaded stores are live in-process: the alternative (upgrade in place with recovery) truncates under live readers holding maps — the invariant-14 shape. Makes in-process open order load-bearing: open the writer before the follower, or close the follower first. The same rule is enforced per store at resolve time, so a store that appears after a writer is already open cannot be adopted by it either — that rejection reaches JS as an error at both
useLogsurfaces (a regression test covers it, because an escaping C++ exception there aborts the process rather than failing a call).A retired transaction-log segment ends on a transaction boundary. A Windows zero-fill that cannot land retires the segment instead of leaving it appendable; because a retired segment cannot erase anything, its persisted boundary is its only eraser, so it retires at the last complete transaction rather than at the framing end — otherwise the next segment's first flagged batch closes the orphaned prefix and two source transactions read as one (invariant 14 spans rotations). Erase and retire now share one predicate. Retirement is also decided at load rather than on the first write, which means an unwritable marker fails the open; a reviewer may prefer degrading to read-only there instead.
destroy()closes every descriptor for the path, secondaries included (then errors if references remain). Alternative: reject destroy while a follower is open — a behavior change for existing two-handle callers (a read-write + read-only pair already destroys today).Hard rejections over silent normalization: explicit
readOnly: falseor boundedmaxOpenFilesalongsidesecondaryPaththrow. Strictness surfaces config mistakes; the cost is that a shared options object withmaxOpenFilesset cannot be reused for secondaries verbatim.Deferred, documented: concurrent
catchUpWithPrimary()calls each hold a libuv worker for the whole replay and queue on the per-database mutex while holding it, so overlapping calls can exhaust the default four-thread pool (the shape invariant 16 describes). Coalescing waiters onto one in-flight replay removes it without a new thread and is the obvious follow-up; for now the README and TSDoc say to await one call before starting the next. The registry hazards exposed by this PR are fixed: waits re-find their entries after every wake, keep an explicit closing descriptor visible across spurious wakes, and recheck path-wide workspace ownership; destroy serializes against every registry key for the physical path, so a reader or fresh-workspace secondary cannot open mid-delete. Still left for separate issues (pre-existing): a POSIXtruncateFile()failure leaves a torn segment appendable where this PR built the symmetric Windows retirement, the registry mutex is held across slow opens, non-race read-only IOErrors still collapse to "Database does not exist", andcatchUpWithPrimary()'s replay is uncancellable, so a close that waits on it blocks until it finishes.Known limitation, documented not fixed: a cross-process follower reads a transaction-log file through a
MAP_SHAREDoverlay covering an unrecovered torn tail, so if the primary restarts and its recovery truncates below a mapped page, the follower's scan of that region takes SIGBUS — an uncatchable kill, and precisely when a follower matters most. Windows is protected by mandatory sections; POSIX is not, and invariant 14's "POSIX needs none" reasoning covered only a same-process writer. The fix is to serve read-only stores through positional reads rather than a shared mapping, which is a change to the read path that deserves its own review rather than a late addition here. Recorded in AGENTS invariant 18.destroy()does not delete a secondary's workspace directory. It closes every follower on the path and releases their locks, but the workspace is a caller-chosen directory outside the database, so removing it is not destroy's to do. A reviewer who wants destroy to clean it up should say so; the alternative is silently deleting a directory the caller named and may reuse.The cross-key open/destroy serialization is inspection-backed. The independent reviewer traced the condition-variable and mutex ordering without finding a lost wake or deadlock, but there is not yet a deterministic worker-thread regression that repeatedly opens read-only or fresh-workspace secondary handles while another thread destroys the same physical path. Adding such a stress seam/test is the strongest follow-up; rejecting this choice would mean delaying the fix while the previously demonstrated open-during-delete hole remains. Separately,
test/lock-tracker.test.ts's park test failed twice under Bun on Windows during this work and is green now; this branch touches nothing in the park, verification-table or commit path.Destroy holds the process-global database-registry mutex through RocksDB deletion and workspace removal. This is deliberate conservative serialization: it prevents a different registry key for the same physical path from opening after descriptor claims are erased but before deletion finishes. The cost is that an unusually slow or large destroy stalls unrelated registry operations process-wide; the independent reviewer found no re-entry or deadlock. Narrowing that critical section later requires a path-wide destruction sentinel that survives entry erasure.
Where to look hardest: the read-only transaction-log load paths (
transaction_log_store.cpp,transaction_log_file_posix.cpp/_windows.cpp) — they touch invariant-5 machinery, and the Windows side is untested locally (CI covers it).Verification
test/secondary.test.tsruns a follower against a primary in a separate process — writing, flushing and compacting while the follower catches up and reads — and asserts the view advances only on catch-up, never backwards, and never fails on a reclaimed file. The race test proves the defect rather than asserting it: a worker-thread writer drives continuous write/flush/compaction traffic while read-only opens race it — measured runs: ~10% of opens fail classified asERR_CONCURRENT_COMPACTION(never corruption, never "does not exist") plus stale-snapshot read failures — and the same churn through secondary opens, catch-ups, and reads runs clean. Catch-up visibility is asserted in both directions, including >2KB blob-resident values; the test skips (never false-fails) on an environment that cannot reproduce the race.test/readonly.test.tscovers the Part B classification end-to-end plus the new no-mutation guarantees: a read-only open leaves a torn transaction-log tail untouched (byte-for-byte) where a writable open truncates it, a writable open is refused while read-only-loaded stores are live, a writer cannot adopt a read-only-loaded store through eitheruseLogsurface, and the writable-open refusal holds when the two handles spell the path differently (that last one fails without the identity re-key).test/native/secondary_blob_test.cc(assertion-only) is the secondary+BlobDB regression net: blob reads through a secondary, catch-up after primary blob GC, cold reads of GC'd blobs with fd-hold evidence on Linux, and the proof that RocksDB does not reject a shared workspace — the reason the kernel lock exists.test/native/open_status_test.cccovers the classifier over observed status shapes and corruption/missing-CF/missing-DB counterexamples;test/native/transaction_log_erase_tail_test.cccovers the Windows zero-fill fallback, the retire-on-failure path, and that retirement lands on a transaction boundary.pnpm buildpassed;pnpm test(859 passed, 3 skipped),pnpm test:native(179 passed), andpnpm checkclean. The affectedtest/secondary.test.ts,test/readonly.test.ts, andtest/destroy.test.tssuites also passed after the final race-hardening changes. Cross-process workspace exclusion is verified with a spawned child process.Refs #812
Complexity: complicated
— Claude Fable 5.1
🤖 Generated with Claude Code
Review-Coverage: authored=codex; ran=claude; declined=gemini,cursor-grok,cursor-composer,domain; rounds=6 @ 276d382
Human-Review-Need: 4 @ 276d382