Stop a recycled Windows PID from wedging deploy_component and release dropped databases on every thread

[kriszyp]

Characterizes the five Integration Tests failures main saw in its last fifteen runs (four distinct signatures) and lands the fixes for the three that are ours: the Windows deploy_component wedge is a process-tree identity bug, the "missing" blob record is a test that read before an async cache-fill committed, and the RocksDB LOCK wedge is a drop-versus-open race that drop_database now closes with the lifecycle protocol restore_backup already uses (the rocksdb-js registry defect underneath it is filed separately).

signature	run / shard	reproduced	root cause	disposition
1. #1854 audit:false delete … oracle: Corruption … NNNNNN.sst: No such file … MANIFEST-000005 may be corrupted	33561117013, 33584091644 (uWS 3/6)	no (5/5 local runs under HARPER_UWS_HTTP=1 pass)	the test's read-only oracle open (DB::OpenForReadOnly) lists SSTs a concurrent compaction in the server unlinks	already fixed by ec73131 (#2429); no recurrence in the four main runs since; durable fix tracked in rocksdb-js#812
2. terminology.test.mjs jobs IN_PROGRESS: lock hold by current process … tuckerdoodle/LOCK and Database not open	33592149855 (uWS 2/6)	the stale-handle half: yes (new test, fails on base); the destroy-vs-open window: reproduced only in CI	an open racing a destroy, not two opens. An unrelated schema event made http/1 rescan mid-drop_table; the rescan saw the drop tombstone, ran completeInterruptedDrop (opens column stores) while main/0's drop_database destroyed the directory. rocksdb-js's registry erases and wakes the parked opener before the files are removed, and the opener holds a dangling map reference — the reopen recreates the directory and is registered nowhere, so its LOCK is held for the life of the process and every job worker died at boot	fixed: drop_database now takes the per-database lock and writes a drop lifecycle marker, every thread releases its handles and its rescan skips the database, rocksdb-js's registry is checked and a remaining handle fails the drop with 409 (naming what is open) instead of being force-closed, and a crash after the marker is finished by the next scan; three handle leaks the verification exposed are closed too. rocksdb-js#818 filed (P1) for the registry race; the marker keeps a Harper opener out of the window it leaves
3. Blob lifecycle → no record found for cacheKey	33601777987 (Node 22 3/6)	mechanism confirmed from code + log; not lost — the next test in the same run deleted that record	a get() on a sourcedFrom table resolves before its cache-fill write commits (Table.ts getFromSource); the test's SELECT ran 330 ms after the GET	fixed: the test waits for the record
4. Redeploy runtime-equivalence proof → fetch failed after 303 s, preparation lock held by process 8848, thread 0	33601777987 (Windows 5/6)	Windows-only; not reproducible locally	the lock holder was the deploying main thread itself, stuck in terminateWindowsProcessTree after npm.cmd exited cleanly: the tree was identified by PID alone, so a recycled PID (or a stale ParentProcessId) kept it "alive" — the branch #2273's last comment predicted, now confirmed by #2374's stage markers	fixed: tree members identified by lifetime; no taskkill /T on an exited PID

No common cause across 1, 2 and 4: a compaction against a read-only open, a destroy against a parked open in rocksdb-js's registry, and Windows PID recycling.

For the human reviewer

1. drop_database now fails closed (409) instead of force-closing. The planning review returned better-alternative-exists on a force-close design, and the ruling was to adopt its alternative: reuse the restore marker and close broadcast, verify process-wide closure through registryStatus(), and refuse when a handle remains — a running job (job workers never receive ITC broadcasts, so this is the only barrier for them) or a component holding its own RocksDatabase. Behaviour change: a drop that today succeeds by force-closing an idle handle now returns 409 with the remaining handle described; the retry is the caller's. One casualty worth knowing: a database whose storage environment Cross-worker write can race RocksDB table drop and poison catalog cleanup #1381 has latched (a drop_table that raced another worker's cache-fill write, leaving Invalid column family specified in write batch on every later write) used to be recoverable by drop_database force-destroying it; it now 409s until a restart, which run 33670016278 (Node 26 3/6, blob.test.mjs) showed. Reversible by swapping the 409 for destroy(), but that reopens exactly the window rocksdb-js#818 sits in.

2. A handle leak per schema change was hiding behind that verification. table() replaced the thread's catalog store handle on every table create and attribute change and reopened every existing index store on every call, assigning the fresh handle over the old; a dropped table's column-family handles were never closed on any thread. Unreachable handles only closed when a GC finalizer ran, which is why the first drop in the new test reported 103 references still open. Where to look hardest: the reuse in table() — an index handle is now shared between the live table and the create/update path (with the per-open preparation re-run on it), which was always true of the catalog handle in initStores; a change of index kind (ordinary ↔ HNSW) is the one case where the handle cannot be reused, since the two are different wrappers over the column family, so the store is reopened as the other wrapper and the structural change rebuilds it through the new one (test flips both ways). Five unit suites had grown to depend on the reopen — intercepting or mocking a handle they expected table() to replace, or "restoring" an instance mock by assignment, which leaves an own property shadowing the prototype for every later suite — and now intercept or delete on the shared handle instead.

3. The Windows wait still has no deadline. waitForConfirmedTermination polls without a deadline, and processGroupIsAlive counts an unreaped zombie as alive — a spawn's cleanup can hang forever #2076 asks for one; this change keeps the deliberate policy ("a wedged deployment is safer than a released lock over a live descendant") and instead removes the false-positive that made the wait infinite, backs the poll off, and logs the survivors so the next wedge names its process. Adding a deadline is a policy change for that issue. Where to look hardest: the lifetime bounds in selectWindowsProcessTree and the root exit latched after the scan — a bound that is too late keeps waiting, one that is too early releases the lock over a live descendant; members are remembered across scans by PID and creation time, so a grandchild whose parent exited between two scans is still reached (test, fails on the previous head) and its exit is latched from the first scan that lost it — the late bound again; and manageThreads' dead-worker path, which only asserts the root's exit when its synchronous taskkill reported success, and now receives the spawner's clock with the registration so it bounds the root's children by the interval measured around the spawner's spawn call, as Application.ts does, instead of a 5 s hop allowance.

4. A Windows Job Object would make the tree race-free by construction (assign the spawned child to a job, TerminateJobObject, no scanning, no PID identity, no clock skew) but needs native code or an ffi dependency; the lifetime heuristic is the no-new-dependency answer, and the cost of switching later grows with each caller (two today). Raised by the review's decision ledger; not adopted here. The one case the heuristic cannot reach, named by the round-7 review: a grandchild whose linking ancestors (cmd.exe and npm) both exited before the first process-table snapshot, which on the successful-command path is taken at the root's close. Nothing observed links it to the root, so the wait reports the tree gone while an unrefed installer child can still write into the component directory. Scanning during the command would cost a WMI query per poll for the length of an install; the kernel-stable answer is the Job Object. Recorded as a follow-up in the dispatch findings. Rounds 7 through 9 also narrowed three related exposures without removing the class: the allowance before a root's first known-running time is no longer a 1 s guess but the interval actually measured around its spawn() call; a remembered descendant's exit is bounded by any row that visibly replaced its PID rather than only by the scan that noticed it missing (which, once polling backs off, can be seconds late); and the same bound now applies to the root's own frontier on the one scan where it previously fell back to the wide now bound — the scan that first fails to find the root, before its exit is stamped. All three still admit a stranger that starts and exits inside a window nothing observed, which only a kernel-stable identity closes. The allowance before the root's first known-running time is no longer a guess either: spawnWithEnv measures the interval around its spawn call and both callers bound the root's children by it (the fixed 1 s is now only the fallback for a registration that carried none).

5. The instant between the closure check and destroy() is rocksdb-js's to close. describeOpenHandles → rootStore.destroy() is check-then-act across an await; a component calling RocksDatabase.open(path) on another thread in that instant is a handle Harper does not manage and cannot exclude, and it reproduces rocksdb-js#818. The window went from "always" to that instant; the registry-level requirement (refuse or serialize DestroyDB against a registered opener) is recorded on the issue. Two behaviour changes from the same review round: the online drop now removes the directory remnants and blob roots strictly — a symbolic link where the database directory or a blob root should be refuses the drop before anything is closed (it used to delete through the link), and a removal that fails keeps the marker and returns the error instead of logging it under a successful response (test); and a cross-thread close releases each table through Table.cleanup(), so its timers, TTL interval and reclamation handler go with the handles (the LMDB drop awaits the environment close before unlinking under it).

6. Round 10's cross-model review pass (Gemini, alongside codex) found two real defects this heuristic-narrowing had missed. A schema change that switches an index's kind now opens the new column-family wrapper before closing the old one — the previous order closed the live handle first, so a construction failure in the new wrapper (an invalid custom-index option, say) left the table's indices map pointing at a store this thread had already closed, and every later read or write through it would fail; a regression test injects that failure. Round 11's re-read found the first fix incomplete: the old handle was still closed as soon as the new one opened, several statements before the assignment that actually publishes the new one (persisting the attribute descriptor and the reindex-trigger logic run in between, either of which can throw) — so a failure there still left the map pointing at an already-closed handle. The old handle is now closed only once that assignment has run. Round 12's re-read found this still incomplete the other direction: the new handle, opened but not yet published, was left dangling — closed nowhere — if anything in between (the descriptor persistence, the reindex trigger's own scan of the primary store) threw before the assignment; an open native handle nothing references still counts against a later drop_database's process-wide closure check. The open-through-publish sequence is now wrapped so a throw anywhere in it closes whichever of the two handles was never published — the old one stays open on a failed reopen, the new one closes on a failure after a successful reopen — with a regression test for the second case, injecting the failure through this table's own primary store rather than the shared catalog this time. The PowerShell process-table reader also read stdout as raw bytes rather than decoded text, so a multi-byte character in a process name split across a chunk boundary would corrupt into replacement characters; setEncoding('utf8') lets the stream buffer a split character instead. A third finding — an on-demand open's guard against a database mid-restore-or-drop reads the marker's kind and state as two separate, unlocked reads, so a drop marker replaced by an incoming restore in the gap between them could read as not-a-drop and fall through as if there were nothing left to block — is fixed by re-evaluating from the current marker on that outcome instead of treating it as settled; this one is a genuine multi-thread race at the granularity of individual syscalls and I did not find a way to cover it with a fast, deterministic unit test, so it rides on the existing drop-protocol integration coverage rather than a dedicated one. Declined: Gemini's comment-narration nit named five comments, three pre-existing and untouched by this PR; the two this PR did add explain non-obvious rationale (why a value now travels across a thread boundary, why a bound is taken at a particular moment) rather than restating the code, so I left them. Round 11's Gemini pass also raised two claims about manageThreads.js's Windows dead-worker reclamation, both about code that predates this task's session (an earlier round of this same PR): its synchronous, scan-free taskkill before any process-tree check (pre-existing, already # Findings and this reviewer note's entry 3) and whether killedAt clips the tree window too early if a target lingers after taskkill returns — round 12's re-read found that claim unreachable (killedAt is sampled only after taskkill has already reported success, so nothing can spawn between the two), and it is dropped rather than tracked further. A third Gemini claim, that the BOM-strip regex in parseProcessTable is an empty pattern, is incorrect — it does contain \uFEFF, just not visibly so in a diff view.

7. Round 13's cross-model review (Gemini + codex + Harper-domain adjudication) named a real cost this design accepts: the boot-time/rescan half of the drop protocol blocks its whole thread's event loop. recoverInterruptedDrop deletes an interrupted drop's entire database tree with a single synchronous rmSync(path, {recursive:true}), on whatever thread's getDatabases() first sees the marker. getDatabases() is synchronous and called from many synchronous production paths, so this was a deliberate consequence of that contract, not an oversight — but for a crash mid-drop of a large database, the next boot (or a live rescan) freezes that thread for the whole delete: no logs, no health checks, nothing served. The online-drop half is already async, and its default removal is now an iterative walk that yields between entries rather than one bulk rm() occupying a single libuv threadpool slot for the duration — but the boot/rescan half can't take the same fix without first making getDatabases()'s lifecycle check async, which is a change to a widely-called synchronous contract, not a narrow bug fix. Recorded in # Findings for a follow-up rather than attempted here. Two smaller findings from the same round: DESIGN.md's drop-protocol section had an internally contradictory paragraph left over from before the marker-based redesign (fixed); a comment in windowsProcessTree.ts described an exit code the script never actually produces (fixed). Round 14's re-read of the same online-drop iteration found readdir()'s own cost — materializing every entry before removing any of them, the same whole-directory-at-once shape the switch away from bulk rm() was meant to fix — so the walk now uses opendir()'s async iterator instead, which yields one entry at a time without allocating the rest up front. Gemini's round-14 pass also raised a second manifestation of the already-adjudicated marker-read swallow (round 13, ruled pre-existing): a marker read mid-write (between its truncating open and the write landing) reads as empty content and is skipped the same way a permission error is. Traced the actual consequence: checkRestoreState (the on-demand database() open's guard) checks marker existence, never content, so it is unaffected; only the boot/rescan bulk scan relies on content and could momentarily miss a just-started drop, and even then drop_database's own process-wide registryStatus() closure check — independent of this scan's bookkeeping — still catches any handle that gets opened during the miss and fails the drop closed rather than destroying under it. Real, narrow, same pre-existing shape as the earlier finding; recorded in # Findings for the same follow-up rather than fixed here.

8. Round 15's Gemini pass reported a blocker-severity claim that does not hold up: a removed delete databases[databaseName] in dropDatabase's LMDB branch, said to leave a window where a concurrent read segfaults on a closed handle. The delete is still there — it now runs inside closeDatabase(), called synchronously at the same point dropDatabase always called it — and diffing the LMDB branch against origin/main line-by-line shows its timing relative to the async close/unlink work is unchanged by any commit in this PR. Declined as factually wrong, not a regression from this branch. Two smaller, real findings from the same round, both recorded in # Findings rather than fixed here: the WMI process-table query has no timeout (a specific case of the already-adjudicated no-deadline design, not a new defect), and the new drop-protocol integration suite has no LMDB coverage (LMDB drop takes a different code path with no marker/lock/409 protocol; only unit-level close-ordering coverage exists for it today). Gemini's output this round also opened with two lines styled as system-level tool-use directives, unrelated to reviewing this diff — noted for the human reviewer as a probable artifact of the reviewer tooling, not acted on.

9. One PR for three fixes. They share only the triage; a revert of one drags the others, and the Windows half cannot be exercised on the Ubuntu lanes. Kept together because the task's acceptance is one PR stating every signature's disposition, and the commits are independent per fix if a split is wanted.

10. Rebased onto current main (169 commits); one file conflicted and the conflict was semantic. The branch was based at 6d725818c (2026-09-02) and had inherited harper#2275 — unitTests/apiTests/mqtt-test.mjs:503 failed on all three Linux Node versions, deterministically, and main's own run at 6d725818c fails it identically. Its fix (resources/crdt.ts, Fix LMDB partial-record history resolution broken by #2497 (harper#2275), hold the QA-701 409 probe's window open causally, and wait for MQTT deliveries instead of sampling 200ms #2526) merged three hours after that base, so the red Unit Test was never this change's. resources/databases.ts conflicted against feat(branches): declare tables into a branch through @table, ensureTable and defineTable #2523's branch-database work, which added target.adopt(dbi) inside the index-open block this PR had rewritten into the reuse/rollback sequence. Resolution: a handle joins a branch's openedStores close list only when this table() call opened it, and only at the publication point — never a reused one (already listed) and never inside the try whose catch closes an unpublished handle. The same rule now covers the catalog store, which this PR had made ??=-reused while leaving its adopt unconditional. main's cleanupDisabledPlane() moved into the split-out prepareIndexStore, so an options change that disables a derived plane still cleans it up even though the reuse path no longer reopens the handle — which is also what makes entry 11's third item reachable.

11. The rebase forced a full re-review (rounds 16–18); three findings were fixed and three design-level ones are open. Fixed: selectWindowsProcessTree added CLOCK_SKEW_MS to notAfter even when the bound came from an observed replacement row, whose creation time is on the same WMI clock as its children's — so a child that replacement spawned within 50 ms was selected as ours and taskkillInvocation would kill it, the exact unrelated-process kill (harper#2273) this module exists to prevent; the skew now applies only to the wall-clock bound and an observed replacement's time is exclusive, with a regression test that fails on the previous build. Table.cleanup() started derivedIndexRuntime.close() with void and then synchronously closed the table's column families, against that runtime's documented contract (it resolves only once nothing can still write); it now closes the stores after the release settles, leaves them open when quiescence cannot be proven — so the drop refuses with 409 rather than closing under a live writer — and hands the promise to closeDatabase's existing closing array so drop_database awaits it before the registry check. taskkillInvocation built one command line per round, which CreateProcess rejects past 8191 characters into a spawn error runTaskkill deliberately absorbs — several hundred orphans would be "killed" by a command that never ran, and the deadline-free wait would never end; per-PID kills are now batched at 200.

12. The framing recheck returned better-alternative-exists, and its three alternatives are decisions for a maintainer, not this branch. The pre-push CLI required a --mode plan recheck after a late round still produced fresh majors; the verdict names a better framing for each of the three surviving majors. (a) Drop protocol — an atomic tryBeginDrop(path) gate in rocksdb-js that blocks new opens and reports existing ownership, so a refusal is side-effect-free. That is the registry-level requirement already recorded on rocksdb-js#818 and is a cross-repo change. It is the right answer to the open finding that a 409-refused drop has already run closeDatabase(): a reference captured before the drop (const Orders = databases.shop.orders) keeps operating on a disposed class over closed handles even though the database survived. Verifying before announcing is not a substitute — the close broadcast is what makes other threads release their handles, so a verify-first ordering would refuse nearly every drop. (b) Index reuse — construct the candidate custom index without publishing or destructive cleanup, persist the catalog, then swap dbi.customIndex and clean up the superseded one. Today a same-kind options change rebinds the live wrapper and may unlink its HNSW plane file before the catalog write that can still throw, leaving this worker on the new definition while disk keeps the old. This one is implementable here and is a real change to the reuse mechanism, not a bug fix. (c) Windows — a Job Object (native code or an ffi dependency), or bound the confirmation and quarantine the unique .deploy-staging/<deploymentId> tree instead of waiting. Both reverse the recorded no-deadline policy (harper#2076). Concretely open: when queryWindowsProcessTable() returns null — WMI unavailable, PowerShell restricted — no kill is issued at all and the wait is infinite, where the removed terminateWindowsProcessTree fired taskkill /pid <root> /T /F every round. That is a behaviour regression on that host class and the reviewer is right that "an operator can see and clear it" does not hold when there may be no surviving target, only a permanently unavailable observer.

13. Review-response rounds on 1c9dbae5 … e56a86d0b: sixteen threads adjudicated, thirteen fixed and resolved, three left open as @kriszyp's standing calls — the rocksdb-js tryBeginDrop gate (entry 5) and the Windows no-deadline policy (entries 3 and 4). Every fix below has a test that fails against a build without it. Lifecycle marker: the kind was read before the lock, so a restore that began and abandoned in the gap left a marker beginDrop's truncating open erased (read under the lock now); the marker was written in place, so an ENOSPC left a nameless marker the scan skips and a partly deleted database loaded as healthy (staged, fsynced and renamed now, with short and zero-progress writes treated as failures); the metadata-directory fsync follows the rename, and its failure released the lock with a live drop marker, so the next scan would have deleted a database the operation had errored on (rolled back now unless the marker was already there); a non-ENOENT read error inside the lock but outside the try held the flock for the process lifetime; dropDatabase cleared markers it had superseded on both non-destructive exits; and lock.preexisting meant "a file was there" rather than "a marker naming this database". Drop protocol: a 409-refused drop over a pre-existing marker broadcast a close, every thread rescanned, and recoverInterruptedDrop deleted the directory with no handle check — under the very handle the drop had just refused for (both recovery call sites read the process-global registry first now); and the online drop derived its blob roots through getRootBlobPathsForDB, which answers [] behind a warning for a store carrying no databaseName, while recovery derives them by name — both halves use getBlobPathsForDatabaseName now, on both engines. Handles: closeLoadedDatabases() did not await Table.cleanup()'s derived-index close, so a job worker could exit with its column families still in the process-global registry, the exact leak it exists to prevent; the CLOSE_DATABASE acknowledgement landed before the closes it started, which drop_database reads as the thread having released; a reused index store was mutated (custom-index rebind, derived-plane unlink, versioned-encoder arm, rebuilding flag) several throw-capable statements before its catalog write, and is staged and applied at the publication point now; and cleanupDisabledPlane() could abort a declaration whose catalog write had already landed. Windows: CLOCK_SKEW_MS was applied to same-clock notBefore bounds, admitting a process the previous holder of a PID spawned up to 50 ms earlier — the harper#2273 unrelated-process kill this module exists to prevent. On the refused-drop thread, the strongest evidence is that its state is not new here: main's RESTORE_BACKUP ITC handler already calls closeDatabase() on a database that survives and reloads (server/itc/serverHandlers.js:53), so a class captured across that broadcast already operates over closed handles today — the 409 adds a trigger for that state rather than creating it, and Table.cleanup()'s disposed makes the failure explicit instead of a native-handle error. @cb1kenobi re-reviewed the result and wrote "the review-response fixes hold up … nothing new blocks here."

14. A reviewer question about removeStorageReclamation is answered, five more defects are fixed, and the framing recheck is back at better-alternative-exists — that last one is yours. @cb1kenobi asked whether dropDatabase's dropped removeStorageReclamation calls moved into closeDatabase. They did not need to move: closeDatabase already made the call on main (resources/databases.ts:2107 at the merge-base), and both engine branches of dropDatabase now route through it, over a superset of the paths — every root store it can reach, plus each table's own handler via Table.cleanup(), where main's dropDatabase cleared one root path. The RocksDB half is pinned by a test that predates this PR (unitTests/resources/databases.test.js, "deregisters the storage-reclamation handler when a database is dropped", from Apply audit retention continuously to RocksDB transaction logs #2338) and fails if the call is removed; the LMDB half had no coverage on either side and now does.

    Five more, each with a test that fails against a build without it. The ITC handler's await Promise.all(closing) sat outside any try, and closeStore pushes an asynchronous close unwrapped — a rejection would have skipped the rescan below it, dropped the acknowledgement drop_database reads, and rejected into notifyMessageListeners, which neither catches it nor handles the promise it returns. indexStoreMatches compared wrapper kind only, and declareTable's closed-store guard checked the root store, which dropTable leaves open while closing the table's own column families — so a same-name create arriving before the eviction broadcast rebound onto closed handles; both are rejected now. queryWindowsProcessTable and runTaskkill had no bound, so a hung Get-CimInstance parked the confirmation loop before its own warning — each invocation is bounded now and a timeout resolves to the same null an unreadable table already produces, so the deadline-free outer policy (harper#2076) is untouched and the state is finally visible. recoverInterruptedDrop rejected both path separators on every platform while schemaRegex permits a backslash, so a POSIX database named sales\2026 could never have its interrupted drop finished; only the host's separators are rejected now, and the resolved-parent check that actually enforces "one directory under the databases root" is unchanged. And Table.cleanup() called derivedIndexRuntime.close() — another component's method — before its own synchronous releases, so a throw there left this class's record-expiration interval holding a job worker's event loop open.

    The framing recheck (--mode plan, round 38) returned better-alternative-exists, and the skill says a recheck is adopted or handed to a human, never overruled — so this is @kriszyp's call. Its concretely-better design is three things, all of which reach past this branch: (a) the drop marker should carry a versioned manifest of its actual deletion targets (a third line; old readers ignore it, so no migration) — today it records only the database name and recovery re-derives blob roots from live storage.blobPaths, so config drift between the crash and the restart deletes the new root and orphans the old; (b) recovery should be asynchronous under a single owner, with the marker continuing to block scans and opens — which is the getDatabases() synchronous-contract change already recorded as entry 7; (c) a Windows Job Object instead of creation-time reconstruction, which is the standing alternative in entry 12. The recheck also corrected one thing this PR had been saying: terminateProcessGroupsForThread's PID-keyed taskkill runs on ordinary worker-port exit (server/threads/manageThreads.js:1797, via removePort), not only at process shutdown. It is still byte-identical to the merge-base — verified, not assumed — so it is pre-existing and wants its own issue, but it is a hotter path than this PR had claimed.

    Ruled and left as they are, with the evidence: the drop events reach the public onRemovedTable/onRemovedDB hooks before the registry check can refuse with 409 — the emission is what lets a listener release its own handle before that check, so reordering it defeats the check; the closed-primary guard makes a drop_table-then-create_table race a loud error rather than retiring the class before drop_table resolves, which both outside legs would prefer — retiring it inline duplicates the rescan's ownership of databases, and a loud error beats the silent success over broken handles that preceded it; a rename failure can leave <key>.restoring.staged behind, which no scan reads and the next beginLifecycle truncates. Three claims were re-raised and are refuted at source: beginLifecycle does return { ...lock, preexisting } (dataLayer/restoreMarker.ts:337, the function's last statement — a fifth round for this one); the LMDB drop does close and evict its root before unlink (resources/databases.ts:2263-2268, awaited by the LMDB branch, and the tableless case arrives through definedDatabases); and openIndex's unguarded prepareIndexStore is the LMDB branch, where lmdb-js caches dbis by name so there is no per-call handle to leak — every RocksDB index goes through the branch whose catch closes it.

15. @kriszyp ruled the framing recheck: land alternative (a) here, and the other two become follow-up issues. It landed, and chasing it turned up three more defects in this branch's own code. The drop marker now records its own deletion targets on a third line (targets <version> <json>), and recoverInterruptedDrop deletes those instead of re-deriving blob roots from live storage.blobPaths. Readers that predate the line split off lines 1 and 2 and never look further, so nothing migrates; a marker without it falls back to configuration exactly as before, and the seeded-crash integration fixture — whose marker has no manifest — proves that path end to end. A manifest that is present but unreadable fails the recovery closed, because guessing from configuration is the mistake it exists to prevent. Recorded roots are on-disk state, so they are checked before anything is deleted through them: a root must be absolute and its last segment must be the database name, which is the one rule that holds however the configuration has moved (join(<a configured blob path>, <database name>) is the only shape a drop can target). Entry 12's other two alternatives are now follow-up issues by that ruling, not open questions on this PR: Interrupted-drop recovery blocks the event loop: rmSync on the schema-rescan and on-demand-open paths (P2, Task, under [Epic] Backup/restore correctness) and Use a Windows Job Object for spawned process-tree lifetime instead of reconstructing identity from Win32_Process creation times (P2, Task, under [Epic] Component-spawned process supervision).

    Three defects the review rounds on that change found, all in code this branch added, each with a test that fails against a build without its fix. The manifest recorded relative paths. storage.blobPaths may be relative (the schema permits any string) and the roots were recorded verbatim; workers chdir to the root path while the main thread keeps the launch directory, so a relative root named a different place on whichever thread recovered — missing the blob root and clearing the marker, or removing a same-named directory under another root. Resolved at record time, and a recorded root that is not absolute is now refused rather than resolved against the recovering thread. A recycled PID let one thread erase another's process-group identity, in three layers, each found after the previous fix: removeProcessGroup cleared the PID-keyed state before checking ownership at all; then, once it checked membership, the case that actually matters turned out to be a thread that owned the PID first — A registers P, P exits, the OS hands P to B's child, B registers it, and A's delayed UNREGISTER still finds P in A's own set, so membership says yes and B's stamp is wiped anyway; and then the dead-owner sweep was found to send taskkill/SIGKILL for every id in its set before anything consulted the stamp. The creation stamp names its owner now, only that owner may clear or consume it, and the sweep filters to the groups whose stamp still names it, warning for each id it skips. Every one of those is the harper#2273 unrelated-process kill this module exists to prevent, reached on a path the previous fix did not cover.

    One open trade this created, recorded rather than decided. Filtering a recycled id out of the old owner's sweep also drops it from that owner's wait, so if that owner still had live descendants under the id, they are abandoned and its preparation lock is released early — where the recorded policy (harper#2076) prefers a wedged deployment over releasing on an unconfirmed tree. On Linux the case is not reachable: the kernel will not hand out a pid that is still live as a process-group id, so a group with surviving members keeps its number. On Windows there are no process groups in that sense and a root PID is recycled freely — but the descendants there are found by the process-table walk keyed on creation times, and that walk is driven by the very stamp that now belongs to the other thread, so the old owner could not have identified them correctly either way. This is the same identity problem entry 12(c)'s Job Object dissolves by making the tree an OS-level set; it is noted here rather than patched further, because choosing "abandon the orphans" over "wedge the lock" is the policy call harper#2076 already owns.

16. The final review-response pass closes four stale-cleanup races without changing the standing product decisions above. Process registrations now carry a monotonically increasing generation from the application spawner through the main-thread registry, so a delayed unregister from an earlier child cannot clear a newer same-thread, same-PID registration; the regressions cover replacement, cache invalidation, and a real detached child, while the application-level test pins generation transport. A resumed drop now preserves and returns the existing manifest from the lifecycle lock, refuses a malformed manifest instead of overwriting it, and reports unreadable marker hashes to the database loader so only the affected database is blocked; marker tests, real recovery coverage, and the design note pin those contracts. Every index checkpoint, failure, and completion now goes through one locked, build-identity-fenced catalog update, with drop/replacement/concurrent-metadata regressions and the existing checkpoint observer updated to the synchronous publication path. Finally, database roots close only after every child handle settles; Table cleanup deliberately keeps its root open when a derived runtime cannot prove quiescence, and the RocksDB and LMDB tests prove child-before-root ordering on success and rejection. The planning reviewer materially changed the implementation: its better-alternative-exists ruling was adopted by preserving the original drop manifest byte-for-byte and fencing all catalog writes, rather than rewriting the marker or only guarding status updates. The exact-head independent reviewer reports LGTM with no substantive finding; the wrapper remains formally non-converged because its domain/adjudication leg repeatedly failed or was pruned, so the human-review grade remains conservative.

17. The current-main merge and final review tightened three more edges without changing the protocol. The merged audit-floor test now awaits the full child-before-root close chain, which is why the first post-merge CI run's three Linux unit jobs failed on the old single-root promise capture and why this head fixes them. A restore now refuses an unfinished drop, preserving its deletion manifest; index interruption detection now accepts either the bare or legacy primary-key catalog descriptor, with direct marker and catalog-shape regressions. The final 0a8ed8e09 review had no accepted blocker: its two reported majors are contradicted by the reviewed source and existing end-to-end assertion (completeRestore/clearRestoreMarker already release in finally; signalSchemaChange awaits the local handler, and the 409 integration suite asserts the database was reloaded). The real remaining concern is the explicitly documented fail-closed derived-runtime path above. The review wrapper remains formally non-converged because its domain/adjudication leg failed on an invalid response schema, so the machine-derived human-review grade is conservative.

18. The final PR-feedback fix preserves audit retention when a close is deliberately refused. stopAuditCleanup() now returns an identity-bearing retirement barrier and accepts only its matching resume, so a stale resume cannot override a newer concurrent stop. When derived-index teardown cannot prove quiescence and closeDatabase intentionally keeps the RocksDB root open, it awaits and resumes that root's cleanup loop before acknowledging; the existing rejection test now schedules a real pass and proves the retained root purges again. This addresses and resolves the only new post-push thread; the three standing design threads remain open for the maintainer decisions above. The delta review reached Gemini but Claude's five-hour quota rejected both its primary and fallback attempts, and the low-risk policy pruned adjudication; Gemini's reported blockers were false on source trace (every audit-cleanup async resume checks retirement/closure, the Windows helper supplies its default taskkill callback, and signalSchemaChange awaits the local rescan). The machine-derived footer records that degraded exact-head review honestly.

19. The exact-head review found and closed one remaining overlap in that retirement protocol. Two simultaneous stopAuditCleanup() calls could share the same in-flight drain promise, so promise identity was not an invocation identity: an older refused close could still resume cleanup through a newer close. Each stop now mints a monotonically increasing generation alongside its drain barrier, and resume accepts only the newest generation after draining; the new regression suspends one cleanup pass, takes two stops, proves the older resume leaves cleanup retired, then proves only the newer one restarts it. The final full review ran both Claude and Gemini over all 36 changed files and found no blocker in this fix. Its domain/adjudication leg failed its response-schema contract, so the receipt remains formally non-converged and Human-Review-Need 3. Two Gemini majors were rejected on direct source trace (signalSchemaChange() does await the sender's local rescan; isAbandonedIndexBuild() already treats a foreign PID or older restart generation as abandoned). The remaining minor notes are either the deliberate fail-closed derived-runtime / synchronous-recovery tradeoffs already called out above, or follow-up API polish (drop events can precede a later 409 and an incomplete-drop restore conflict gets generic wording).

Changes

• Restore and drop now share the process-wide closure check in rocksdbBackup.ts, remove blob roots by database name in blob.ts, await job-process database closure in jobProcess.ts, and carry the close operations through the shared ITC vocabulary in hdbTerms.ts.
• Integration coverage removes the former retry that masked the terminology drop race in terminology.test.mjs and keeps the renamed blob-root helper accurate in blob-drop-crash-orphan-window.test.ts and blob-reclaim-removal-paths.test.ts.
• Unit harnesses now intercept the shared catalog/index handles without leaving shadowing mocks in createTableCatalogOrder.test.js, indexBuildAbandonment.test.js, indexRestartNumber.test.js, and schemaMigrationFragility.test.js; the obsolete injected Windows termination-loop test was deleted in favor of the real process-tree and registry coverage described above.

Verification

• Signature 1: HARPER_UWS_HTTP=1 npm run test:integration -- integrationTests/database/delete-index-atomicity-rocksdb.test.ts five times in a row on Linux: 5/5 pass (the only "recovered on attempt 3/6" lines are the retry-contract test's own).

• Signature 2: npm run test:integration -- integrationTests/database/drop-database-concurrent-rescan.test.ts (three suites): 3 workers × 8 drop cycles under schema churn asserting the dropped directory never reappears, a worker-served catalog probe after a same-name recreate, a job boot afterwards, and no lock hold by current process in hdb.log — 3/3 pass; the drop-signal release alone fails the probe on base ({"data":["anchor"],"dropme":[]}, 1668 Database not open lines), and the first protocol run reported 103 reference(s) still open until the handle leaks were closed. A component holding its own rocksdb-js handle: drop_database → 409 naming the handle, the database intact and readable, then 200 after release with the directory gone. A data root seeded with a crashed drop (directory with CURRENT, blob root, drop marker): all three gone at boot, the name creatable again. npx mocha unitTests/dataLayer/restoreMarker.test.js: 30 passing (11 new: typed markers, key mismatch ignored, recovery under the lock, refusals for traversal names / a marker naming another database / a symlink, a failed deletion keeping the marker). terminology.test.mjs (its drop_database retry removed) and blob.test.mjs under HARPER_UWS_HTTP=1: 48/48 and 22/22.

• Signature 3: npm run test:integration -- integrationTests/apiTests/blob.test.mjs: 22/22 pass.

• Signature 4: npx mocha unitTests/server/threads/windowsProcessTree.test.js unitTests/components/applicationSpawn.test.js: 34 passing, 1 pending on Linux — the new tests pin a recycled root PID with children, 1.2 s / 30 s / 10 min stale-ParentProcessId orphans under both allowances and under the root's observed creation time, a child created before its parent, an unreadable process table (unknown, not gone), root-only versus per-PID kills, the root exit latched after a slow scan, the survivor warning and the poll backoff. The pending test runs only on Windows: it executes the real PowerShell query against a spawned child and confirms the tree gone — exercised by the unit-test-windows job and the Windows integration legs below.

• npm run test:unit:main: 5253 passing, 1 failing on this box only (configValidator resolves a relative root against a cwd that is 83 characters deep here; unrelated, pre-existing). npm run test:unit:resources: 1926 passing, 0 failing — after adjusting the five suites that depended on table() reopening handles (entry 2), plus the round-9 through round-14 review fixes and their own regression tests (index-store-shape rollback on both branches, LMDB close ordering, the Windows process-tree descendant/root bounds).

• Integration Tests workflow, full 40-job matrix by workflow_dispatch: eight runs on an earlier head (a84d7ba28, prior to the round-9 through round-15 review fixes below) — three fully green including Windows, uWS and Bun, the other five red only on shard 3 on one of two pre-existing tests outside this change (ttlResetOnWrite 4. SQL UPDATE resets TTL, a first-SQL-operation cold-start race identical on main; blob.test.mjs drop_table BlobCache, Cross-worker write can race RocksDB table drop and poison catalog cleanup #1381's cross-worker drop race, identical on v5.2) — root-caused in the dispatch log, neither touched here. All four task signatures held green in all eight. On the final head (ec339814d), three consecutive full-matrix workflow_dispatch runs — 33773623955, 33774586222, 33775438489 — all 41/41 jobs green, including every uWS HTTP and Windows leg, satisfying the ≥3-consecutive-greens acceptance criterion.

• Full 40-job matrix on the current head (e56a86d0b): three workflow_dispatch runs, all 41/41 jobs green — 34834724651, 34834730613, 34834733004 — Node 22/24/26, every uWS HTTP and Windows leg included. The eight- and three-run evidence above belongs to pre-rebase heads (a84d7ba28, ec339814d); this is the head the review receipt and the three open threads refer to, so the >=3-consecutive-greens acceptance criterion is now satisfied on it. No push triggers the full matrix, so it was dispatched by hand.

• Superseded verification for the pre-rebase heads (a84d7ba28, ec339814d) and for the intermediate review-response heads (8e803383f … e56a86d0b) is in the commit history and the dispatch log; none of it covers this head, and the run below does. Two things from it are still worth knowing: the worktree had been resolving @harperfast/rocksdb-js 2.8.0 from a parent checkout while the rebased lockfile pins 2.9.0 (rangeReadActivity "preserves read-your-writes" passes or fails purely on that — harper#2506 / rocksdb-js#837), and a sixth suite's shadowing catalog mock (indexBuildAbandonment.test.js, new on main) defeated updateAttributesLock's prototype interception and failed all three Node versions on the first rebased head.

• Review-response round 3 (heads 3e967d327 … dacaf2939): npm run build, prettier --check . and oxlint clean (oxlint exits 0; its warnings are all pre-existing and none is in a touched file). npm run test:unit:resources 2516 passing / 0 failing; npm run test:unit:main 5587 passing / 3 failing, all three environment-specific to this box and outside this diff's call chain (the git-credential tmpdir leak, a uWS port another process on the box held, and the configValidator relative-root case that depends on cwd length); npx mocha unitTests/dataLayer/*.js 281 passing; npx mocha unitTests/server/threads/windowsProcessTree.test.js 26 passing with 3 pending (Windows-only, including the two new helper-bound cases, which the unit-test-windows job runs); npm run test:integration -- integrationTests/database/drop-database-concurrent-rescan.test.ts 3/3, re-run after every change. Each of the seven new tests was confirmed to fail against a build without the behaviour it asserts — by removing closeDatabase's removeStorageReclamation, the ITC handler's try, the closed-store guards on the index and on the primary, the host-aware separator rule, and Table.cleanup()'s release ordering. The full 40-job matrix has to be dispatched again on this head.

• Review-response round 4 (heads 21ad982d5 … cb6b858ed): npm run build, prettier --check . and oxlint clean. unitTests/dataLayer/restoreMarker.test.js 45 passing (5 new: the config-drift case, the no-manifest fallback, an unreadable manifest, a recorded root that is not this database's directory, one that is not absolute, and a recorded database path that no longer resolves here); unitTests/server/threads/windowsProcessTree.test.js + unitTests/components/applicationSpawn.test.js 46 passing / 3 pending (Windows-only), including the two new ownership cases; npm run test:integration -- integrationTests/database/drop-database-concurrent-rescan.test.ts 3/3, re-run after every change. Each new test was mutation-verified — the drift one against a recovery that ignores the manifest, and the process-group ones against both the original code and each intermediate fix, which is how the second and third layers were caught. npm run test:unit:resources was run twice on 191f9ddf3 under heavy load from other agents' review legs (9m vs the usual 5–6m): 2514 and 2515 passing with one different failure each run and none reproducing — blob.test.js passes 122/122 as a file, derivedIndexRuntimeNativeBackend passes alone, updateAttributesLock passes 9/9 alone. The full 40-job matrix has to be dispatched again on this head.

• Final exact-head verification (9f15e42b9): npm run format:check, npm run build, and npm run lint:required pass; the full RocksDB resource suite passes 2520 with 33 pending, and HARPER_STORAGE_ENGINE=lmdb npm run test:unit:resources passes 1951 with 388 pending. Focused lifecycle/process/index coverage passes 73, index convergence passes 72 with 2 pending, RocksDB close ordering passes 9, and LMDB close ordering passes 3. npm run test:unit:main reaches 5600 passing / 200 pending with three failures outside the diff: the unchanged deploy-credential test sees another reachable global /tmp/harper-git-cred-* socket, gitCredentials.test.js inherits the dispatch runner's GIT_CONFIG_GLOBAL/GIT_PAGER (and passes when those two variables are removed), and configValidator resolves its relative-root fixture against this unusually long worktree path. npm run test:unit:windows reproduces only that same unchanged global credential-socket failure; its other eight groups pass. npm run test:integration:all could not start its instances because the external shared loopback-address-pool JSON was concurrently truncated (Unexpected end of JSON input); tests were cancelled at setup, so it supplies no product counterexample.

• Current exact-head verification (0a8ed8e09): npm run build, npm run lint:required, and npm run format:check pass; focused restore/index regressions pass 53/53; the merged-head LMDB audit/close slice passes 47 with 2 expected skips; the full RocksDB resource suite passes 2691 with 45 expected skips, and the full LMDB resource suite passes 2097 with 421 expected skips. Before the current-main merge, GitHub's complete integration matrix was green on Linux, Bun, uWS, and Windows; the only red checks on the first merged head were all three Linux unit versions asserting the superseded single-root LMDB close capture, now updated at the audit-floor link above. Local test:integration:all remains unobservable because another process truncated the shared loopback-address-pool JSON during setup, so current-head GitHub CI is the integration authority.

• Final review-response verification (2ebbcaa16): npm run build, npm run lint:required, and npm run format:check pass; all 9 RocksDB close-ordering tests and all 80 active audit-log tests pass, with 8 expected engine-specific skips. The cleanup-resume assertion is mutation-sensitive: without the new resume, the scheduled pass remains retired and its purgeLogs count stays zero.

• Exact-head overlap verification (084511052): npm run build and npm run lint:required pass; Prettier reports both modified files clean; the complete combined audit-log and database-close slice passes 90 with 8 expected engine-specific skips. The new overlap test is behavior-sensitive: it fails if two stops reuse their shared drain promise as the restart identity, and passes only when the older generation is refused while the newest generation can resume. GitHub exact-head CI has 45 successful checks and 3 expected skips: Node 22/24/26 and Windows units, every Linux/Bun/uWS/Windows integration shard, adapters, HNSW, smoke, build, lint, and format are green. The only red check is review infrastructure: the full PR review exhausted Claude's context with Prompt is too long, posted no inline comments, and buffered no findings. A separate fresh full-scope Claude + Gemini review completed locally at this SHA; its domain response-schema failure and the GitHub review degradation are reflected in the conservative footer rather than hidden.

Complexity: complicated

_{Review-Coverage: authored=codex; ran=gemini,claude; blocked=domain(exit-1); declined=cursor-grok,cursor-composer; rounds=55; full=4 @ 0845110}

_{Human-Review-Need: 3 @ 0845110}

[gemini-code-assist]

Code Review

This pull request introduces a more robust mechanism for terminating Windows process trees by tracking process lifetimes rather than relying solely on PIDs, which are prone to recycling. It implements a new utility 'confirmWindowsProcessTreeGone' in 'server/threads/windowsProcessTree.ts', updates 'manageThreads.js' and 'Application.ts' to utilize this new logic, and adds comprehensive unit tests. Additionally, it addresses a race condition in 'drop_database' by ensuring database handles are closed appropriately and adds a new integration test to verify this behavior. I have no feedback to provide.

[claude]

@/tmp/review-body.txt

[cb1kenobi]

Reviewed the drop lifecycle protocol, index-store reuse and rollback, close/acknowledgement ordering, and the Windows process-tree bounds at dacaf29. The head commit correctly runs Table.cleanup()'s synchronous releases before the derived runtime's close, so a throw there can no longer leave a job worker's interval holding the event loop open. No new blocking defect was confirmed on the changed lines. The three open design-level items are cross-repo or policy decisions already tracked in their own threads, so they were not repeated.

—
Reviewed dacaf29

[cb1kenobi]

An interrupted drop can lose its original target manifest when resumed. Configuration drift can then delete the new blob root while orphaning the original one. Preserve and use the existing manifest during recovery.

—
Reviewed 21ad982

[cb1kenobi]

The new commits correctly key the Windows process-group stamp by owning thread and record drop blob roots as absolute paths. No new blocking defect was confirmed on the changed lines. Previously raised items on the drop-manifest rewrite, the check-then-destroy instant, and the unbounded Windows observer wait remain in their existing threads.

—
Reviewed ecc4ea4

[cb1kenobi]

The new filter prevents a dead thread from killing a PID now owned by another thread. However, the ownership stamp does not distinguish registrations from different generations on the same thread, so a delayed unregister can erase tracking for a live replacement process. Include a registration token in unregister operations and compare it before deleting tracking.

—
Reviewed cb6b858

[cb1kenobi]

One new defect: closeDatabase always stops a database's audit-cleanup loop, but now sometimes skips actually closing and evicting the root store. When that happens the database reloads on the same cached root with audit cleanup permanently off, so its audit logs grow unbounded until a restart, silently. Everything else on the changed lines held up, and the remaining open items are maintainer decisions already recorded in existing threads.

—
Reviewed 9f15e42

[cb1kenobi]

The audit-cleanup restart fence can alias concurrent stop calls because it uses a shared drain promise as the generation token. Give every stop invocation a unique token so an older refused close cannot restart cleanup during a newer close.

—
Reviewed 2ebbcaa

[cb1kenobi]

The generation fence gives each audit-cleanup stop a distinct identity and prevents stale resumes during newer closes. No novel blocking defects were confirmed on changed lines; existing discussions are not repeated.

—
Reviewed 0845110