Let a database be opened into a caller-owned table graph instead of the global map - #2285
Let a database be opened into a caller-owned table graph instead of the global map#2285kriszyp wants to merge 2 commits into
Conversation
…he global map Branched databases (#643) need a database whose Table classes are reachable only through the application scope that asked for it. Registering one in the process-global `databases` map and asking every consumer to skip it does not work: that map is walked by analytics (storeDBSizeMetrics), by describe_all (schemaDescribe), and by worker teardown (closeLoadedDatabases), and Pro walks it too -- a skip list is a rule each future enumerator has to remember, and one that forgets leaks a branch into a customer's metrics or a describe response. So `initStores` takes a destination instead. With one, it builds into the caller's object, seeds that object's defined-tables bookkeeping, and emits no global updateTable event; `readRocksMetaDb` threads it through and additionally skips transaction-log replay, which would otherwise write through the global path rather than the destination. Without one every path behaves exactly as before. `openBranchDatabase(path, databaseName)` is the narrow entry point on top: it opens a RocksDB directory -- in practice a checkpoint of a base database -- under the *logical* name the application already uses, so its schema and code need no changes, and returns the private graph. Nothing enters `databases`, so enumeration leaks are impossible by construction rather than by policy. The trailing optional arguments of `initStores` became an options object on the way; there were three call sites and a fourth positional boolean was not going to read. Refs #643 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to open a RocksDB directory as a scope-private branch database via openBranchDatabase, allowing table graphs to be built into a caller-owned object rather than the global databases map. The feedback highlights two important improvements: first, ensuring that column families (such as dbisDb and auditStore) are properly closed during a partial initialization failure to prevent native resource leaks; second, calling getDatabases() at the start of openBranchDatabase to guarantee that global databases are loaded and collision checks do not silently pass.
| } catch (error) { | ||
| // readRocksMetaDb registers the store before building tables, so a failure part-way leaves it | ||
| // in the env map with no handle able to close it. | ||
| const stranded = rocksdbDatabaseEnvs.get(path); | ||
| rocksdbDatabaseEnvs.delete(path); | ||
| try { | ||
| stranded?.close(); | ||
| } catch (closeError) { | ||
| logger.warn?.(`Error closing partially opened branch database at ${path}`, closeError); | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
When cleaning up a partially opened branch database on failure, only calling stranded?.close() leaves behind any opened column families (such as dbisDb and auditStore) that were successfully initialized during initStores before the failure occurred. As noted in the close() method, closing only the root store leaves these handles behind, causing a native resource leak.
We should explicitly close these column families in the catch block as well.
} catch (error) {
// readRocksMetaDb registers the store before building tables, so a failure part-way leaves it
// in the env map with no handle able to close it.
const stranded = rocksdbDatabaseEnvs.get(path);
rocksdbDatabaseEnvs.delete(path);
if (stranded) {
for (const store of [(stranded as any).dbisDb, (stranded as any).auditStore]) {
try {
store?.close?.();
} catch (closeError) {
logger.warn?.(`Error closing branch column family at ${path}`, closeError);
}
}
try {
stranded.close();
} catch (closeError) {
logger.warn?.(`Error closing partially opened branch database at ${path}`, closeError);
}
}
throw error;
}| export function openBranchDatabase(path: string, databaseName: string, storeName: string): BranchDatabase { | ||
| if (!existsSync(path)) throw new Error(`Cannot open branch database: no directory at ${path}`); | ||
| // The guards below compare against keys in the env map, so two spellings of one directory must | ||
| // not read as two directories. | ||
| path = realpathSync(path); | ||
| // A second open would hand back a rival table graph over one shared root store, and the two | ||
| // callers would disagree about who may close it. | ||
| if (openBranches.has(path)) throw new Error(`Branch database at ${path} is already open`); | ||
| // A globally-loaded database's store is shared and closed by `closeLoadedDatabases`; adopting it | ||
| // here would mean this handle's `close()` tears down a live database. | ||
| if (rocksdbDatabaseEnvs.has(path)) throw new Error(`Cannot branch ${path}: it is already open as a database`); | ||
| // `storeName` picks the blob roots. Letting it name a real database would point the branch's blob | ||
| // writes at that database's directory, which is the collision the separate identity exists to stop. | ||
| if (databases[storeName] || definedDatabases?.has(storeName)) { | ||
| throw new Error(`Cannot use '${storeName}' as a branch store identity: a database of that name exists`); | ||
| } |
There was a problem hiding this comment.
If openBranchDatabase is called before the global databases are loaded (e.g., in certain test setups, scripts, or early initialization phases), definedDatabases will be undefined and databases will be empty. As a result, the collision check databases[storeName] || definedDatabases?.has(storeName) will silently pass, potentially allowing a branch to be opened with a storeName that conflicts with an existing database.
Calling getDatabases() at the start of openBranchDatabase ensures that all global databases are loaded and definedDatabases is fully initialized before performing the collision check.
| export function openBranchDatabase(path: string, databaseName: string, storeName: string): BranchDatabase { | |
| if (!existsSync(path)) throw new Error(`Cannot open branch database: no directory at ${path}`); | |
| // The guards below compare against keys in the env map, so two spellings of one directory must | |
| // not read as two directories. | |
| path = realpathSync(path); | |
| // A second open would hand back a rival table graph over one shared root store, and the two | |
| // callers would disagree about who may close it. | |
| if (openBranches.has(path)) throw new Error(`Branch database at ${path} is already open`); | |
| // A globally-loaded database's store is shared and closed by `closeLoadedDatabases`; adopting it | |
| // here would mean this handle's `close()` tears down a live database. | |
| if (rocksdbDatabaseEnvs.has(path)) throw new Error(`Cannot branch ${path}: it is already open as a database`); | |
| // `storeName` picks the blob roots. Letting it name a real database would point the branch's blob | |
| // writes at that database's directory, which is the collision the separate identity exists to stop. | |
| if (databases[storeName] || definedDatabases?.has(storeName)) { | |
| throw new Error(`Cannot use '${storeName}' as a branch store identity: a database of that name exists`); | |
| } | |
| export function openBranchDatabase(path: string, databaseName: string, storeName: string): BranchDatabase { | |
| getDatabases(); | |
| if (!existsSync(path)) throw new Error(`Cannot open branch database: no directory at ${path}`); | |
| // The guards below compare against keys in the env map, so two spellings of one directory must | |
| // not read as two directories. | |
| path = realpathSync(path); | |
| // A second open would hand back a rival table graph over one shared root store, and the two | |
| // callers would disagree about who may close it. | |
| if (openBranches.has(path)) throw new Error(`Branch database at ${path} is already open`); | |
| // A globally-loaded database's store is shared and closed by `closeLoadedDatabases`; adopting it | |
| // here would mean this handle's `close()` tears down a live database. | |
| if (rocksdbDatabaseEnvs.has(path)) throw new Error(`Cannot branch ${path}: it is already open as a database`); | |
| // `storeName` picks the blob roots. Letting it name a real database would point the branch's blob | |
| // writes at that database's directory, which is the collision the separate identity exists to stop. | |
| if (databases[storeName] || definedDatabases.has(storeName)) { | |
| throw new Error(`Cannot use '${storeName}' as a branch store identity: a database of that name exists`); | |
| } |
| // The column families opened for the tables and the audit store hold their own handles, and | ||
| // closing only the root leaves them behind. | ||
| for (const store of [(rootStore as any).dbisDb, (rootStore as any).auditStore]) { | ||
| try { | ||
| store?.close?.(); | ||
| } catch (error) { | ||
| logger.warn?.(`Error closing branch column family at ${path}`, error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Blocker: close() leaks per-table RocksDB column-family handles.
What: close() only closes rootStore.dbisDb and rootStore.auditStore (plus the root store itself). It never closes the primaryStore or indices handles initStores opened for each table loaded into destination (tables) — those are separate openRocksDatabase(...) / rootStore.openDB(...) handles, exactly like the ones closeDatabase() (this file, ~line 1418-1426) explicitly walks and closes for a real database.
Why it matters: closeLoadedDatabases's own docstring a few lines above explains why closeDatabase() bothers closing every table's primaryStore/indices individually: rocksdb-js's native handle registry is process-global, and a handle that's never closed just leaks (the refcount never drops). openBranchDatabase's docstring promises this handle "closes what nothing else can," but every branch table's column families are left open on every close() call — a leak per open/close cycle once a caller exists (#643). The identical gap exists in the failure-path cleanup a few lines up (the catch block, ~line 1074-1085): it only closes the stranded root store, so any table already loaded into tables before a later table's init failed also leaks its column families.
Suggested fix: Iterate Object.values(tables) here (and in the catch block) and close each table's primaryStore and each entry of .indices, the same way closeDatabase() does, before closing dbisDb/auditStore/rootStore.
|
Blockers found. This push only adds a RocksDB-only test guard; |
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Branched databases (#642) need a database whose Table classes are reachable only through the application scope that asked for it. Registering one in the process-global
databasesmap and asking every consumer to skip it does not work: that map is walked by analytics (storeDBSizeMetrics), bydescribe_all(schemaDescribe), and by worker teardown (closeLoadedDatabases), and Pro walks it too. A skip list is a rule each future enumerator has to remember, and the one that forgets leaks a branch into a customer's metrics or a describe response.So
initStorestakes a destination instead. With one it builds into the caller's object, seeds that object's defined-tables bookkeeping, emits no globalupdateTable, and replays the transaction log into that same graph. Without one, every existing path behaves exactly as before.openBranchDatabase(path, databaseName, storeName)is the narrow entry point on top, returning a closeable handle.This is the storage-side opener only — nothing calls it yet. The config, scope wiring, and lifecycle that expose a branch to application code are the rest of #643.
Refs #643.
For the human reviewer
logical-vs-physical-name-split). Table classes keep the base's logical name so an application's schema and code resolve unchanged; only the root store carriesstoreName, which is whatgetRootBlobPathsForDBreads. The alternative — a fully distinct database name remapped at the scope boundary — avoids the split but means the app's own@tablenames stop matching. The cost is real: every subsystem keyed offdatabaseNamenow sees the base's name for a branch, which is exactly what makes item 3 necessary.blob-roots-diverge-from-base). That follows fromstoreName, and it means a checkpointed record's existing blobs are not readable through a branch until Branched databases: blob store sharing, allocator, and GC safety #644 lands — Branched databases: blob store sharing, allocator, and GC safety #644 hard-links the base's blob tree into those roots. Reviewing this PR standalone, that gap is sequencing, not oversight. The alternative considered was read-through to the base's roots with copy-on-write, which is a storage-layout decision rather than a local edit.ship-with-schema-mutation-unsafe). Because branch Table classes carry the base's logical name, adropTable()through one resolves against the global schema and would delete the live base Table class. Row reads and writes are unaffected. Kris's call was to gate it in the scope-wiring PR before any caller exists rather than widen this one intoTable; it is recorded in the docstring and as acceptance criteria on Branched databases: app config, scope proxy, lifecycle wiring #643. If you would rather see it enforced here (a branch flag rejecting DDL), that is the alternative and it is cheaper now than after callers exist.rocksdbDatabaseEnvsbehind guards (process-global-env-map-reuse). A separate branch-only map would remove the adoption window by construction instead of by guard. I kept the shared map becausereadRocksMetaDband the close path already key off it and splitting them duplicates lifecycle logic — but the guards are the load-bearing part, so look hardest there.no-api-for-branch-writes-yet). Unit tests only, because there is no caller. You are judging a mechanism against a described intent; the integration proof arrives with the scope wiring.One finding I rejected rather than fixed. The review flagged that branch tables "register into a process-global registry that
close()never unregisters." I could not substantiate it:Table.tshas no module-levelMap/Set,makeTableperforms no registration in its body, andsetTablewrites only into the object it is handed — the private destination for a branch. The one global side effect that did exist, theupdateTableevent, is suppressed. If you know of a registry I did not find, that changes the isolation claim and I want to hear it.Verification
Nine new cases: reads a row through the branch, the base's entry in the global map is untouched and gains no key, the store carries the branch identity, a second open of the same directory is refused, a directory already open as a real database is refused, a
storeNamenaming a real database is refused,close()is idempotent, and a missing path throws without registering anything.Discrimination checked rather than assumed: disabling only the
destination ?? ensureDB(...)line — leaving the new function in place — fails two of them, so they test the mechanism and not the function's existence.End-to-end route: not observable end-to-end in this PR — nothing calls the opener, so the integration proof lands with the scope wiring in #643.
Suites:
databases.test.js,closeLoadedDatabases.test.js,caching-rocks-database.test.js— 29 passing.tsc --noEmit,lint:required, prettier clean. Not run locally:test:unit:main/test:unit:resourcesfull gates (known shared-lock contention on this machine) — relying on CI.One caveat recorded honestly: I saw a single unreproducible failure in
databases.test.jsduring one run and could not reproduce it across five subsequent runs, including both orderings combined withcloseLoadedDatabases.test.js.Complexity: medium
Review-Coverage: authored=claude; ran=gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-composer; rounds=4 @ 51676f1
Human-Review-Need: 4 (decisions: logical-vs-physical-name-split, blob-roots-diverge-from-base, ship-with-schema-mutation-unsafe, process-global-env-map-reuse, no-api-for-branch-writes-yet) @ 51676f1