fix(lock,fetch): a real SIGINT/SIGTERM releases the lock; no response body outlives its command - #220
Merged
Conversation
… body outlives its command The project lock was released only from an `exit` listener or its `finally`, but a real signal's default action ends the process without emitting `exit`. `pharn update --yes` cancelled in CI or killed by `timeout` / `docker stop` exited 130/143 with .pharn.lock left behind; a host sharing the directory waited out the 6 h staleness window. - New lib/fatal-signal.ts: a lazily installed SIGINT/SIGTERM handler that runs registered cleanups newest first, removes every listener, and re-raises (exit(128+n) if the re-raise ever throws). The temp-clone cleanup (repo.ts) and withProjectLock both register through it; the lock deregisters in its finally. - SIGHUP is deliberately NOT handled: a listener overrides the SIG_IGN `nohup` sets (measured), so a hangup would interrupt a `nohup pharn update` mid-write. Decided by the human after the measurement. - fetchCommitSha releases a non-2xx body and reads a 2xx body through its own reader cancelled at the deadline, so neither keeps the process alive after the command finished (measured 30 s for a dripping 403). downloadArchive releases its non-2xx body too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0199owRmYfskqYQVQrVP679o
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
Plan C of the second batch from the PHARN-01..18 review (findings F12, F13), shipped through
/pharn-dev-ship.F13: a stranded lock.
.pharn.lockwas released only from anexitlistener or afinally. A real signal's default action ends the process without either. Sopharn update --yescancelled in CI, or killed bytimeout/docker stop, exited 130/143 and left the lock behind. A host sharing the directory then waited out the 6 h staleness window. Reproduced onmain, then re-run on this build:src/lib/fatal-signal.tsprovidesonFatalSignal(cleanup). It is installed lazily on the first registration. On a signal it:exit(128 + n)if the re-raise ever throws).repo.ts) andwithProjectLockboth register through it. The lock deregisters in itsfinally, so a later signal can never delete another process's lock.process.on('SIGHUP')listener overrides theSIG_IGNthatnohupsets, so a hangup would interrupt anohup pharn updatemid-write. Decided by the maintainer after that finding; it is documented in troubleshooting.F12: an unread body kept the process alive.
fetchCommitShanow releases a non-2xx body. It reads a 2xx body through its own reader, cancelled at the 8 s deadline, instead ofres.json(), which has nothing to cancel. The SHA is best-effort, so the command finishes normally, and a dripping 403 used to keep the process alive for 30 s after its last line.downloadArchivenow releases its non-2xx body too.Type of change
feat— new stack option, wizard step, or command capabilityfix— bug fixdocs— docs-only changechore/refactor— tooling or internal restructure, no behavior changeArea(s) touched
src/lib/fatal-signal.ts(new) ·src/lib/repo.ts·src/lib/project-lock.ts· tests ·docs/troubleshooting.md· CHANGELOG ·.dev/features/signal-lock-release/Checklist
.js-extension import convention.src/(checked in a worktree). They cover real-signal child processes, observable body cancels, and the registry rules.docs/page.redirect: 'error', the deadlines and the existing caps are unchanged; untrusted bodies are now cancelled rather than drained.Quality gates
npm run checkpasses locally (format:check+lint+typecheck+test) — 1530 tests.npm run buildsucceeds.npm run test:coveragepasses (coverage thresholds met).Notes for the reviewer
validateexit 0, regressno-regressions, verifyPASS, review GREEN with 2 minor advisory findings (.dev/features/signal-lock-release/REVIEW.md).🤖 Generated with Claude Code
https://claude.ai/code/session_0199owRmYfskqYQVQrVP679o
Generated by Claude Code