Prevent concurrent component installs from corrupting dependencies - #1991
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a file-system-based locking mechanism (withComponentPreparationLock) to serialize component preparation across worker threads, and implements process group tracking to ensure spawned child processes are terminated on timeout or worker exit. The feedback highlights two key improvements: snapshotting the keys of processGroupsByThread before iterating to prevent issues when mutating the Map during iteration, and properly catching and logging errors from prepareApplication during boot to avoid silent failures and potential crashes when accessing untrusted error properties.
|
Reviewed; no blockers found. |
kriszyp
left a comment
There was a problem hiding this comment.
Final lock audit findings addressed in 59d4cc3: immutable bakery tickets remove stale-owner CAS races, process-start identity handles PID reuse, and Windows distinguishes taskkill failure from an independently confirmed dead tree.
- Application.ts: terminateProcessTree now probes the process group itself instead of trusting the direct child's exitCode/signalCode, so an unref'd descendant that outlives its parent is still terminated. - manageThreads.js: a dead worker's lock claim is no longer reclaimable until its tracked process groups are confirmed terminated (SIGKILL and taskkill only queue termination). isThreadRunning now waits on that confirmation before reporting an owner gone, with the wait routed through the main thread for non-main contenders. - manageThreads.js: fixes a hang the above introduces — a process group spawned by a worker thread's own event loop is never reaped by another thread once that loop is gone, so it becomes a permanent zombie that kill(pid, 0) reports alive forever. Confirmation now also checks /proc/<pid>/stat for zombie state on Linux and treats a zombie as terminated, since it can no longer touch the filesystem. - DESIGN.md: fixes stale "hard link" language (now rename) and documents the reclamation-confirmation and zombie-handling invariants above. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
Approving — the bakery-lock serialization is solid and fully supported by tests, and everything stays on the cold install path. Leaving a few non-blocking findings inline: two edge-case gaps around stale-owner/orphan handling worth a follow-up, and two smaller cleanups.
Also: the deferred deploy-transaction ordering issue raised in-thread is real and well-characterized now — please file a tracking issue before this merges so it doesn't evaporate.
sent with Claude Fable 5
Three-lens review (Codex + Gemini + Grok) of the previous commit found real correctness gaps in the lock-reclamation hardening; this addresses the ones within scope of this change: - componentPreparationLock.ts: scanLiveClaims could silently drop a live contender when its choosing claim was read (ENOENT) exactly after it finished publishing a ticket and removed the choosing flag — a genuine TOCTOU race that could let two contenders both believe they'd won. Recovered via the token embedded in the choosing filename to look up the now-published ticket instead of discarding the claim as stale. - Application.ts: spawnWithEnv's successful 'close' path released the component lock without checking whether the process group was actually empty, mirroring the same gap already fixed for the timeout path. Now reuses terminateProcessTree to confirm/terminate the group before a successful install can be treated as done. - manageThreads.js: THREAD_INFO and PROCESS_GROUP_TERMINATION_CONFIRMED replies were only consumed by an ad-hoc one-shot listener, so every reply also hit addPort's permanent dispatcher as an "unregistered" type, warning and queuing forever during any lock wait. Registered both as intentionally-handled-elsewhere, matching the existing convention. - manageThreads.js: the Windows branch of dead-worker process-group cleanup ignored taskkill's result and confirmed termination unconditionally. Now retries taskkill and confirms via a process-table query (mirroring Application.ts's Windows tree-termination logic) before reclamation proceeds. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Re-running the independent review against the previous fix commit surfaced two more corroborated (2-3 lens) correctness gaps in the same code paths: - Application.ts: on a timed-out command, the 'close'/'error' handlers were untracking the process group immediately, even though the timeout path's own terminateProcessTree() call might still be mid-grace-period. If the owning worker exited in that window, manageThreads would have already forgotten the group and could let a replacement preparation start before the old tree was actually gone. Untracking now happens only once termination is confirmed — from whichever path (timeout or close/error) is actually driving it. - Application.ts + manageThreads.js: both Windows tree-termination loops (waitForWindowsTreeTermination, waitUntilConfirmedGone) treated a successful taskkill invocation as proof the whole tree had exited. taskkill succeeding only means the request was accepted — Windows termination is asynchronous — so both now always confirm via the independent process-table query and return only on an explicit "gone", never on taskkill's exit code alone. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two non-blocking review findings on componentPreparationLock.ts pointed at the same gap: the wait-deadline renewal treated ambiguous liveness as license to keep waiting, contradicting DESIGN.md's stated invariant that the bounded wait is a backstop specifically for when owner liveness cannot be established. - A foreign-PID owner's liveness was a bare kill(pid, 0), which only proves some process holds that PID, not that it's the original owner. After a hard crash the OS can recycle the PID to an unrelated long-lived process, silently renewing the deadline forever. - A rejected isOwnerAlive() call was treated as "alive" at the deadline site, for the same conservative reason scanLiveClaims treats it that way for claim removal — but renewal has the opposite failure mode: claim removal must not delete a possibly-live claim, while renewal on unconfirmed liveness makes the bound unbounded. Added ownerLivenessConfirmed, used only at the deadline site: renewal now requires a same-process, same-instance owner that the caller's own liveness check positively confirms alive. Everything else falls through to the bounded timeout, which only fails the waiting request and never steals the lock from a genuinely live owner. Also sweeps `.publishing` staging files left behind by a crash between publishClaim's writeFile and rename (a third, cosmetic finding on the same file) — age-gated well beyond that two-syscall window so a sweep can never race a genuinely in-flight publish. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… CIM ambiguity
Codex review of the previous commit surfaced two correctness gaps within
scope of this change:
- Application.ts: installApplications only rewrote harper-application-lock.json
once, after every component's Promise.allSettled had finished. A crash
between recordApplicationPreparation removing a stale success entry (in
memory) and that final write left the on-disk file still claiming success
for a config whose reinstall a partial directory could satisfy on the next
boot, permanently skipping the required reinstall. recordApplicationPreparation
now takes a `persist` callback and awaits it durably both before destructive
preparation starts and after it succeeds; persistApplicationLock does an
atomic temp-file + rename write, serialized per path so concurrent
components' writes can't race the same temp filename or clobber each
other's just-persisted state.
- Application.ts + manageThreads.js: the duplicated Windows CIM process-tree
query treated any non-zero, non-1 taskkill/PowerShell exit as "unknown"
but collapsed a genuine query failure (e.g. Get-CimInstance denied or WMI
unavailable) into the same exit code (1) as "positively confirmed not
found" — both reached the script's `else { exit 1 }` under PowerShell's
default non-terminating error handling. The script now sets
ErrorActionPreference=Stop and wraps the query in try/catch, exiting 2 on
failure so it's read as unknown (keep retrying) rather than gone (release
the lock).
Two further "major" findings from the same pass — a fire-and-forget
worker-to-main process-group registration that could in principle race a
worker's own forced termination, and non-Linux (non-/proc) zombie detection
hanging isThreadRunning — point at the same deeper fix (routing spawns
through the main thread so it durably owns registration and reaping) and
are recorded as follow-up work rather than attempted here under review-cycle
time pressure; see the dispatch findings.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
🤖 Note on 3 missing review threads: while clearing pending review state on this PR I deleted a pending (unsubmitted) self-review that, on closer inspection, actually contained 3 live inline comments — not stale draft text as its summary body suggested. Recreating them here since GitHub doesn't let me restore the original threads:
All three were already fixed before I found the deletion mistake; only the GitHub thread objects themselves are unrecoverable, not the underlying findings or their resolution. |
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Filed #1996 to track the cross-node deploy-transaction ordering gap (config write → install → replication not serialized across concurrent same-component deploys) per @Ethan-Arrowood's request — scoped separately from this PR's node_modules corruption fix (#1973). All other inline review threads are resolved (including the 3 recreated ones from the earlier pending-review mishap). No other open review feedback found. 🤖 KrAIs (Claude) |
Serializes component extraction and dependency installation across Harper worker threads, including the deprecated direct npm-install path. Installer process trees are now terminated on timeout, worker exit, and Harper shutdown; failed boot preparations remain retryable instead of being written to
harper-application-lock.json.Reviewer attention: filesystem lock ownership/recovery in
componentPreparationLock.tsand process-tree lifecycle registration inmanageThreads.js. Generated by Codex; the standard pre-PR review completed with Claude and Gemini, while the post-fix final-artifact Claude check ended with an internal execution error.Closes #1973