Skip to content

Close the RocksDB handles a table redeclare replaces, so online restore is no longer refused - #3201

Open
cb1kenobi wants to merge 11 commits into
mainfrom
fix/rocksdb-redeclare-handle-leak-3120
Open

cb1kenobi wants to merge 11 commits into
mainfrom
fix/rocksdb-redeclare-handle-leak-3120

Conversation

@cb1kenobi

@cb1kenobi cb1kenobi commented Oct 14, 2026 •

Copy link
Copy Markdown
Member

⊙ Problem

On RocksDB, redeclaring a table at runtime (an insert that adds an attribute, addAttributes, a cluster definition) opened a new __dbis__ handle and a new handle for every index, and never closed the ones they replaced (#3120: RocksDB table redeclare leaks superseded __dbis__ and index handles, so online restore_backup is always refused). closeDatabase walks only the current table graph, so the replaced handles stayed open: handle count grew with every schema change, and an online restore_backup of such a database was refused as "held open by a loaded component". #3001, a duplicate, measured it.

❓ Your call: replaced custom-index handles stay open until the database closes

I close a replaced HNSW handle when the database closes, not on the redeclare, because a query that took it earlier may still read through it; so each HNSW redeclare holds one extra handle until then. Closing it eagerly would break that query; reversing this is a one-line close in indexFor.

💡 Solution

A redeclare reuses the root's __dbis__ handle and any plain index whose store has not changed. Every handle a load or a declaration opens is recorded per database path, so closeDatabase closes the replaced ones too. A handle whose close fails is held until a later close succeeds.

🔧 Changes

✅ Verification

End-to-end route: restore-backup-online.test.ts (new) runs an online restore_backup of a database whose schema changed at runtime; on main it fails with the 409.

🤖 Generated by Anthropic Claude (Opus); posted via @cb1kenobi.

Release target: v5.4 — the issue's milestone; not a backport

Related PRs: #1371 overlaps (both edit index opening in resources/databases.ts), #2155 overlaps (both edit resources/databases.ts close paths and closeLoadedDatabases.test.js), #3121 independent, #3119 independent

Review-Coverage: authored=claude; ran=gemini,cursor-grok,codex,cursor-muse; adjudicated=domain; declined=cursor-composer,cursor-kimi; rounds=6; full=2 @ 8150baa

Review-Attention: deep ~30m (critical: databases.ts; decisions: weak-path-registry, drain-after-root-close, no-eager-close-on-displace, do-less-alternative) @ 8150baa

cb1kenobi and others added 10 commits October 9, 2026 15:45
Fails on main: a redeclare orphans __dbis__ and index wrappers (harper#3120).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A runtime redeclare (an insert that adds an attribute, addAttributes, a
cluster definition) and creating a second table in an existing database
opened new __dbis__ and index wrappers and overwrote the ones the table
graph pointed at, without closing them. closeDatabase only walks the
current graph, so the replaced wrappers kept rocksdb-js's process-global
registry count up, and every online restore_backup of such a database was
refused as held open by a loaded component.

declareTable now reuses the root's one __dbis__ wrapper. Every RocksDB
wrapper a thread opens for a global root, whether initStores loads it or
declareTable adopts it (an index right at the native open), is owned per
root path until its close succeeds; closeDatabaseStores closes that list
before the root and keeps failed closes for the retry. Replaced indexes
stay open until the database closes, since a lazy range can still be
reading one.

Fixes #3120

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Owning every replaced wrapper until the database closes made handle counts
grow with the square of a dynamic-schema table's attributes: an operations
API table indexes every attribute, and each insert that adds one redeclared
the table and reopened all of them. A redeclare now keeps a plain index's
open wrapper when its store is unchanged; a custom index is still reopened,
since openIndex builds its state from the current options.

The owned list now compacts closed wrappers at each doubling, and a close
drains it by swapping it out, so a declaration during the close's awaits
lands in a fresh list the same drain picks up.

Tests that hooked catalog writes by patching the prototype relied on a fresh
__dbis__ wrapper per declaration (each wrapper's own put snapshots putSync);
they now route the reused wrapper's put through the patched putSync.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
rocksdb-js releases an unreachable wrapper's native handle when it is
collected, so the replaced wrappers were only ever open until a collection
ran; restore's 3 s closure check is what kept losing that race. Holding
them strongly until the database closes would have kept every reopened
HNSW wrapper, with its index state and cache, alive for the life of a
long-running server. The owned record now holds WeakRefs: a close still
reaches every live wrapper, and an unreachable one is collected as before.

A redeclare no longer reuses a wrapper that carries a custom index for a
plain declaration, which would have kept writes and searches going
through the HNSW index state.

Tests that stubbed a prototype method on a catalog wrapper restored it by
assignment, leaving an own copy that shadowed later prototype patches now
that the wrapper is reused; they restore through a stubMethod helper. Two
indexing tests left a put/clear mock on an index wrapper across their
simulated restart and now restore it first.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ade during a close

A failed close is retried later, but a weakly held wrapper can be
collected without releasing its handle, so one whose close failed is
held strongly until a retry closes it. A declaration that resolves the
root while closeDatabaseStores is awaiting its closes records new
wrappers after the first drain, so the owned list is drained again
after the root closes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e tests

The second owned-list drain also retries a wrapper whose close failed,
so the failed-close tests need one more simulated failure to leave the
wrapper open. A new test declares a table from inside the root's close,
the window only that drain covers.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eld displaced wrapper

A custom index whose setup threw was adopted weakly before setup, so if
its cleanup close also failed, collection could drop the only record of
a handle still open. It is now adopted only after setup succeeds, and
held strongly when the cleanup close fails.

The lazy-reader test read a plain index, which a redeclare reuses, so it
never exercised a displaced wrapper. It now holds an HNSW index across
a redeclare that replaces it and opens a range through it afterward,
which fails if a redeclare closes the wrapper eagerly. The design note
names that reason, scopes the recorded wrappers to loads and
declarations, and links the teardown note instead of counting sections.
Two tests restore a patched put with stubMethod, so a reused wrapper is
left without an own copy of the prototype method.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s owner

Adopting only after setup left a branch's custom index, whose setup and
cleanup close both failed, in the global map instead of the branch's own
list, so closing the branch never reached it. The wrapper is adopted
before setup again, and a failed cleanup close asks its owner to retain
it: the global owner holds it strongly, and a branch's list already
does. TableTarget exposes that owner instead of an adopt callback.

The owned list now compacts only after doubling from its size after the
last compaction, which keeps the scans amortized as the comment claims.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…wo narrating test comments

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cb1kenobi cb1kenobi added this to the v5.4 milestone Oct 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 addresses an issue where RocksDB databases could not be restored online because replaced column-family wrappers from runtime schema redeclarations kept the database held open. It introduces tracking of opened column-family wrappers via a WeakRef-based map (ownedRocksStores) keyed by root path, ensuring that database teardown closes all wrappers. Additionally, declareTable is updated to reuse the root's dbis wrapper and plain index wrappers on unchanged stores during redeclarations. New integration and unit tests are added to verify these behaviors, and existing tests are updated to use new stubbing utilities (stubMethod and routePutThroughPutSync) to prevent prototype pollution on reused wrappers. There are no review comments, and I have no feedback to provide.

@cb1kenobi
cb1kenobi marked this pull request as ready for review October 14, 2026 22:04
@github-actions
github-actions Bot requested a review from kriszyp October 14, 2026 22:04

This branch has not been deployed

No deployments
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