Close one database alias without tearing down the store its other aliases share

[kriszyp]

Closing one name of a database that another configured name also resolves to (an alias of the same path) tore down the shared root store, and under LMDB the second name's close then ran mdb_dbi_close against the freed environment — the SIGSEGV (exit 139) in the "Unit tests: lmdb" step on the Node.js v22 and v26 Unit Test legs after #2683's alias suite landed (run 35646369634, run 35648838471). closeDatabase(name) is now the release of one name, and the root store closes with the last name that references it; a new closeDatabaseWithAliases(name) is the physical close of every name of a store, which the restore_backup ITC handler now uses so an aliased database can actually be restored online. Design note with the trace and alternatives: resources/DESIGN.md (moved there from docs/design/ mid-review — see entry 7).

This branch was rebased onto main after a ~140-commit gap that included a RocksDB drop/reclamation rework and a repo-wide DESIGN.md restructuring; only one textual conflict (an import line in serverHandlers.js, merged), and one non-textual conflict handled below (entry 7). Two review findings are declined rather than fixed: entry 3 (alias discovery by identity, not path) and entry 9 (retiring the alias holding an HNSW backend can strand the survivor without one).

For the human reviewer

1. Requirement. The task asked for the segfault to be root-caused and fixed without weakening what the alias suite verifies, and to fix the defect rather than the test if it is real. It is real and predates Keep blob paths stable for aliased databases #2683: the only production caller of closeDatabase, the restore_backup ITC broadcast, runs exactly this two-name sequence on every worker of a deployment with a configured alias. The scope grew past a segfault fix on the planning review's verdict (better-alternative-exists, adopted): a name-only release with no physical close would have left an aliased online restore refused by verifyDatabaseClosed under RocksDB, which base already does today by a different route. The fix as shipped covers both engines' bookkeeping; a "no" here means shipping a narrower close that keeps that restore gap.
2. closeDatabase changes meaning: logical release, not physical close. Chosen over keeping closeDatabase physical and adding a name-only variant, because every existing caller (the ITC handler, worker-exit teardown, tests) wants the last-name semantics and none wants a shared store torn down under another live name. harper-pro has no callers of closeDatabase outside core (grepped). Reversal is a rename.
3. Alias discovery by root-store object identity, not resolved path — declined review finding. If the target name is not loaded on this thread while its alias is, closeDatabaseWithAliases returns false and the alias keeps its handles; restore then fails verifyDatabaseClosed with a 409, not a purge under open handles (that gate polls the rocksdb-js registry by path, checked again on rebase against dataLayer/rocksdbBackup.ts's current line numbers). Declined because names load per thread all at once from one configuration scan, and the only production close now closes every name together, so a half-loaded pair is not constructible in production; identity is also what keeps two databases at neighbouring paths from being confused. Reversal: local to closeDatabaseWithAliases.
4. The physical close proceeds even when a derived-index stop fails to prove its work quiescent. Every affected runtime's stop is awaited before any store closes, but a rejection is logged and the close proceeds rather than blocking the restore — the HNSW backend's own non-dropping close() already settles-and-logs before this point, so this is fail-open by construction at both layers, not a decision this PR alone could flip. Failing closed would need the runtime's stop contract changed too. A "no" here blocks restore on any derived-index backend that cannot prove it stopped in time.
5. closeDatabase stays synchronous; only the physical close awaits the derived-index stop barrier. Table.cleanup() starts the runtime stop without awaiting it, so a flush in flight could land after the stores closed — for a restore, after the files were replaced. closeDatabaseWithAliases awaits it (entry 4); worker-exit teardown (closeLoadedDatabases) cannot await and keeps the base behaviour, which never retired the runtime at all — an improvement, not a regression, and out of this PR's scope as pre-existing. A "no" means making closeDatabase async through closeLoadedDatabases and its test callers.
6. Fix in harper, not lmdb-js. An environment-liveness guard in lmdb-js's DbiWrap::close would stop the crash for every embedder but needs a native registry and a dependency release, and would still leave harper closing a store another name reads from (the RocksDB probe showed the same broken state with no native fault). Harper created the sharing (lmdbDatabaseEnvs), so harper enforces the invariant; the native guard can be added later as defence in depth.
7. Design note relocated from docs/design/ into resources/DESIGN.md mid-review. While this PR was open, main partitioned the root DESIGN.md into per-directory notes and removed docs/ entirely (HarperFast/harper@76178b7f8). The rebase moved this PR's note into a new resources/DESIGN.md section, condensed to the invariant and mechanism per that file's own rule that review/process history belongs in the PR; npm run check:design-docs passes.
8. LMDB table handles are never closed individually. Where to look hardest: the RocksDB-only handle close is the line that encodes the two engines' ownership models. mdb_dbi_close is optional by LMDB's contract and invalidates the environment-wide slot for every wrapper of it, so the environment close at the last name releases them. Cost: nothing is leaked, but a future path that closes an environment late keeps those handles alive until then.
9. Retiring the alias that currently owns a native-HNSW backend can strand the surviving alias — declined review finding. A second alias loading a native-HNSW table replaces the first alias's backend; table.cleanup() on that alias then settles and deletes the only registered backend, so writes through the surviving alias keep committing to the primary store but nothing updates the HNSW index. Declined because both in-tree production callers (closeDatabaseWithAliases, closeLoadedDatabases) close every alias together; only a direct caller of the exported closeDatabase on a multi-alias HNSW table would hit it, and none exists today. A "no" here needs the surviving alias's backend re-registered inside closeDatabase, which reaches into HNSW backend lifecycle this PR does not otherwise touch.
10. Child process under MALLOC_PERTURB_ in the resources suite. It turns the allocator-luck crash into a deterministic red assertion on glibc (dies with free(): invalid pointer on base); on macOS and Windows the variable is ignored and the test passes without testing anything. Cost: two short process spawns per run.

Changes

resources/databases.ts — closeDatabase collects the name's root stores, marks each one still referenced by another loaded name as shared, retires every table's process-wide registrations through Table.cleanup() (guarded, so a throw cannot stop the teardown), closes only RocksDB column-family handles per name, and runs the root-store teardown (audit-cleanup stop, storage-reclamation unregistration, dbisDb and root close, env-cache entries) only for unshared roots; closeStore also attaches a rejection handler to a promise-returning close. closeDatabaseWithAliases finds every loaded name sharing a root store, awaits their derived-index runtime stops, then closes them all. collectRootStores (tables plus the defined-database entry of a tableless name) and isRootStoreReferencedElsewhere compare by object identity.

server/itc/serverHandlers.js — the restore_backup ITC handler imports and awaits closeDatabaseWithAliases instead of closeDatabase; unrelated to this PR, the rebase carried forward main's own DROP_TABLE handling in the same file, which this branch does not touch.

resources/DESIGN.md — the new section states the invariant and mechanism (entry 7). DESIGN.md — one new index line links it in.

Verification

Fails-on-base, with origin/main's databases.ts and serverHandlers.js built into dist: the two close-order tests and the physical-close test fail on both engines (cleanup count 0; root closed/closing after the first close), and the child-process test fails under LMDB with free(): invalid pointer. All pass with the fix, on Node 26.2.0 (the CI leg that failed).

unitTests/resources/databaseAliasIdentity.test.js (both engines unless noted): closing either alias first retires the removed name's Table.cleanup() once, leaves the shared root open, lets the surviving name read a record written through the other, and the last close leaves the root closed with no refCount > 0 entry in rocksdb-js's registry (RocksDB); closeDatabaseWithAliases on one name removes both and closes the store; closeLoadedDatabases with two names releases the store (RocksDB, as that teardown is RocksDB-only by design); a child process (unitTests/resources/databaseAliasIdentity-close.js, which first reads through each name because that is what binds the handle the other name's close invalidates) closes two names in either order under MALLOC_PERTURB_=165 and exits 0. The existing identity tests now create the store with a secondary index and a record; waitForClosed uses the shared unitTests/waitFor.js helper instead of a hand-rolled poll.

Gates on 840d14f26 (the rebased head), Node 26.2.0, after npm ci against main's bumped dependencies (@harperfast/rocksdb-js 2.9.1→2.10.0, @harperfast/hnsw 0.3.0→0.4.0): npm run test:unit:resources (RocksDB) 3034 passing, 0 failing; HARPER_STORAGE_ENGINE=lmdb npm run test:unit:resources 2328 passing, 0 failing; npm run test:unit:main 5851 passing, 5 failing — all 5 are box artifacts unrelated to this diff: gitCredentials.test.js:395 fails on this machine's own GIT_CONFIG_GLOBAL/GIT_EDITOR environment regardless of branch, and 4 in deployStageActivate.test.js (a file this PR does not touch) passed 41/41 re-run alone once the box quieted. npm run lint:required, prettier --check on the changed files, git diff --check, and npm run check:design-docs all clean.

End to end: not observable in the integration suite — the production trigger is the restore_backup ITC broadcast on a deployment with a configured alias, which no integration fixture configures; the unit test checks the same registry predicate verifyDatabaseClosed polls. An aliased online-restore integration fixture is recorded as a follow-up.

Refs #2683

Complexity: complicated

🤖 Generated with Claude Code

https://claude.ai/code/session_01DbUogt8UDDeJB5YMCQ2bvU

Origin — the dispatch brief this PR was written from

Close one database alias without tearing down the store its other aliases share

LIVE CONVERSATION about #2721.

You are answering a person, in a thread, one turn at a time. Every turn:

1. Read the whole thread in this dispatch file's # Log — it is the conversation so far, and
   each of your previous turns is in it. Read the PR/issue and the code as needed.
2. Answer the LAST message. Append your answer to # Log as your turn. Prose, not a report:
   they are talking to you, and a status template is not an answer.
3. Set status: needs-input and stop. The thread stays open; their next message resumes it.

Each turn arrives as ASK (answer it, change nothing) or PERFORM (do it, then say what you did) —
the person chose which when they sent it, and the run's own prompt tells you which one this is.
Never infer it from the wording: an unrequested commit in the middle of a discussion and a polite
description of work that was supposed to happen are the two failures this exists to prevent.

Never mark a PR ready and never merge from this conversation.

_{Dispatch: task chat-pr-harper-2721-kriszyp · queued by unknown · ran by claude/opus/xhigh · worker kzyp-xps-1}

_{Review-Coverage: authored=claude; ran=codex,gemini; adjudicated=domain; declined=cursor-grok,cursor-composer,cursor-kimi,cursor-muse; rounds=6; full=2 @ 840d14f}

_{Human-Review-Need: 3 (decisions: fail-open-on-unquiesced-stop, logical-vs-physical-close-split, lmdb-env-only-handle-release, per-thread-alias-discovery) @ 840d14f}

[gemini-code-assist]

Code Review

This pull request introduces a mechanism to close a database alias without tearing down the shared root store if other aliases still reference it. It updates closeDatabase to perform a logical release, adds closeDatabaseWithAliases to handle physical closure of all shared aliases (especially for backup restoration), and adds corresponding unit tests to prevent regression and SIGSEGV crashes under LMDB. The reviewer provided feedback on closeDatabaseWithAliases pointing out that a synchronous exception in runtime.close() would bypass Promise.resolve and reject the outer promise, suggesting the use of the new Promise executor pattern to safely catch both synchronous and asynchronous errors.

[kriszyp]

Closing as superseded by #2766, which fixes the LMDB segfault this PR targeted (closes the shared
LMDB environment once instead of closing dbis after another alias freed it) with a smaller,
more minimal diff.

This PR also carried behavior #2766 deliberately leaves out:

• keeping a surviving alias usable when another alias closes (this PR: logical release per name;
  Close a shared LMDB environment once instead of closing its dbis after another alias freed it #2766: closes every LMDB alias together),
• closing RocksDB handles under every alias before an online restore, and
• Table.cleanup() / derived-index runtime teardown in the restore close path.

Filed as follow-up rather than carried forward here:

• restore_backup 409s when a RocksDB database has another loaded alias — closeDatabase only closes handles under the restored name #2802 — the RocksDB-restore alias gap
• a comment on Table background work (expiration timer, delete/cleanup callbacks) not disposed when a database's stores are closed #1811 — the Table.cleanup()/runtime-teardown gap, plus an HNSW alias-backend
  strand risk this PR's prototype surfaced for whoever wires that fix in

No further action on this branch.

🤖 Generated with Claude Code