Skip to content

Let a database be opened into a caller-owned table graph instead of the global map - #2285

Draft
kriszyp wants to merge 2 commits into
mainfrom
kris/643-branch-database-opener
Draft

Let a database be opened into a caller-owned table graph instead of the global map#2285
kriszyp wants to merge 2 commits into
mainfrom
kris/643-branch-database-opener

Conversation

@kriszyp

@kriszyp kriszyp commented Aug 22, 2026

Copy link
Copy Markdown
Member

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 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 the 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, emits no global updateTable, 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

  1. Logical name on the tables, physical identity on the store (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 carries storeName, which is what getRootBlobPathsForDB reads. The alternative — a fully distinct database name remapped at the scope boundary — avoids the split but means the app's own @table names stop matching. The cost is real: every subsystem keyed off databaseName now sees the base's name for a branch, which is exactly what makes item 3 necessary.
  2. A branch gets its own, initially empty, blob roots (blob-roots-diverge-from-base). That follows from storeName, 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 landsBranched 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.
  3. Ships with schema mutation unsafe, gated in the next PR (ship-with-schema-mutation-unsafe). Because branch Table classes carry the base's logical name, a dropTable() 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 into Table; 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.
  4. Branch stores live in the shared rocksdbDatabaseEnvs behind 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 because readRocksMetaDb and 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.
  5. No end-to-end proof yet (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.ts has no module-level Map/Set, makeTable performs no registration in its body, and setTable writes only into the object it is handed — the private destination for a branch. The one global side effect that did exist, the updateTable event, 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 storeName naming 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:resources full gates (known shared-lock contention on this machine) — relying on CI.

One caveat recorded honestly: I saw a single unreproducible failure in databases.test.js during one run and could not reproduce it across five subsequent runs, including both orderings combined with closeLoadedDatabases.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

…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>

@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 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.

Comment thread resources/databases.ts
Comment on lines +1074 to +1085
} 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;
}

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.

high

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;
	}

Comment thread resources/databases.ts
Comment on lines +1053 to +1068
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`);
}

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.

medium

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.

Suggested change
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`);
}

Comment thread resources/databases.ts
Comment on lines +1092 to +1100
// 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);
}
}

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.

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.

@claude

claude Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Blockers found. This push only adds a RocksDB-only test guard; resources/databases.ts is unchanged since the prior review, so the three open threads still stand: close() (and its failure-path catch block) leaks per-table primaryStore/indices column families, and the storeName collision check can silently pass before getDatabases() has run.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
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