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.

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.

Complexity: complicated

_{Review-Coverage: authored=codex; ran=claude; blocked=gemini(timeout); declined=cursor-grok,cursor-composer,domain; rounds=4 @ 459b253}

_{Human-Review-Need: 3 @ 459b253}

[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]

Suggestion (non-blocking): recoverInterruptedDrop's default remove here is a synchronous rmSync(..., { recursive: true }). It only runs when an interrupted drop marker is actually found, and only on the one thread that wins tryFileLock — but that thread blocks its own event loop for the full duration of the removal, on a rescan path that (per the comment at resources/databases.ts:870-872) runs on every worker for every schema event. The online drop path already moved this same job to the async, iterative removeSteadily/removeDroppedDatabaseFiles, so this crash-recovery path is now the one place still doing a blocking recursive delete on a worker's event loop.

I see the PR description calls this out as deferred because getDatabases() requires a synchronous return — that's a reasonable scope call. But the trade-off isn't recorded anywhere in this file. A short comment on recoverInterruptedDrop (or right here) explaining why it's sync despite its async sibling would save whoever revisits getDatabases() from having to rediscover the constraint first.

[claude]

Reviewed; no blockers found — left one non-blocking suggestion inline.