diff --git a/.dev/features/signal-lock-release/GRILL.md b/.dev/features/signal-lock-release/GRILL.md new file mode 100644 index 0000000..983b721 --- /dev/null +++ b/.dev/features/signal-lock-release/GRILL.md @@ -0,0 +1,62 @@ +# GRILL — signal-lock-release + +Plan: `.dev/features/signal-lock-release/PLAN.md`. Spec hash recomputed: +`bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e` — **matches**. Registered +grillers: `{"registered":0,"grillers":[]}`, so only the inline axes run. The plan is +`trust: untrusted`, and nothing in it read as an instruction. + +## Findings + +### Guarantee audit (P0) + +```yaml +- type: FINDING + rule_id: 'P0' + severity: important + file: '.dev/features/signal-lock-release/PLAN.md:47' + problem: 'The re-raise must never leave the process running. SIGHUP (added at GATE 1) has no default-kill emulation on win32, where Node emulates only SIGINT/SIGTERM/SIGKILL for process.kill, and a kill that throws inside the handler would leave the process alive with its listeners removed. Wrap the re-raise: if process.kill throws, exit with 128 + the signal number.' + evidence: '4. Re-raises `sig`, so the exit status stays 130/143/129.' +- type: FINDING + rule_id: 'P0' + severity: minor + file: '.dev/features/signal-lock-release/PLAN.md:59' + problem: "The interrupted-while-writing line printed from the signal path must obey the same rule as the exit path: a stderr write failure (EPIPE on a closed terminal, which is exactly SIGHUP's case) must be swallowed, or it replaces the signal exit with an exception." + evidence: '`withProjectLock` registers `release(cwd)` plus the same "interrupted while writing" stderr line' +``` + +### Eval coverage (P1) + +```yaml +- type: FINDING + rule_id: 'P1' + severity: minor + file: '.dev/features/signal-lock-release/PLAN.md:76' + problem: 'The deadline case needs fake timers AND a body stream whose cancel is observable. Otherwise "the reader was cancelled" is inferred from the null result, which the base code already returns, so the test would pass on the base and prove nothing. Assert on the recorded cancel call.' + evidence: 'A 2xx body that never ends → at the 8 s deadline the reader was cancelled and the result is' +- type: FINDING + rule_id: 'P1' + severity: minor + file: '.dev/features/signal-lock-release/PLAN.md:69' + problem: 'The "leaves alone a lock another process created" case has to create that foreign lock between the release and the signal, in the child. Say so in the test, or the case collapses into the plain release case.' + evidence: 'After a normal release, a later SIGTERM leaves alone a lock file another process created' +``` + +### Checked, no finding + +- Axis (P3): the signal logic leaves `repo.ts` for a module whose one reason to change is fatal-signal + handling. `project-lock.ts` and `repo.ts` both depend on it, and neither on the other. +- Determinism (P5): a fixed signal list and set membership. Handlers are installed lazily, on the + first registration, so a run that never registers keeps the default disposition: the same + behaviour as today for the prompt-only phases. +- Trust (P2): response bodies are cancelled, never read further. There is no new output from + untrusted data. + +## Summary + +The design is sound. The one important gap is the win32 re-raise: once SIGHUP joins the list, the +handler must end the process even where `process.kill` cannot emulate the signal. The rest is test +sharpness: cancels must be observable rather than inferred, and the foreign-lock case needs an +explicit setup. + +**ADVISORY VERDICT: 4 concerns raised (0 blocking-severity, 1 important, 3 minor) — for the human to +weigh before /pharn-dev-build.** diff --git a/.dev/features/signal-lock-release/PLAN.md b/.dev/features/signal-lock-release/PLAN.md new file mode 100644 index 0000000..20d91c7 --- /dev/null +++ b/.dev/features/signal-lock-release/PLAN.md @@ -0,0 +1,130 @@ +# PLAN — signal-lock-release (a real SIGINT/SIGTERM releases the lock; no response body outlives its command) + +- spec_content_hash: bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e # fix #4 +- increment: (1) a SIGINT or SIGTERM received while `.pharn.lock` is held releases the lock before + the process dies. A small shared fatal-signal registry does this, used by both the temp-clone + cleanup and the lock. (2) `fetchCommitSha` cancels a non-2xx body, and reads a 2xx body through + its own reader, cancelled on the deadline, so no body keeps the process alive after the command + has finished. `downloadArchive`'s non-2xx throw cancels its body too. +- layer(s): the CLI itself (`src/lib/fatal-signal.ts` new, `src/lib/repo.ts`, + `src/lib/project-lock.ts`) +- constitution_refs: [P0, P1, P3, P5, P6] + +## Discovery — verified this run (P6) + +- F13 reproduced on HEAD `abb274a` with a tsx probe (scratchpad `plan-C/probe13.mts`). It holds + `withProjectLock`, runs `fetchRepo` (stubbed fetch) and signals itself. SIGTERM exits 143 and + SIGINT exits 130, and in both cases `.pharn.lock` is left behind. + - Cause: `repo.ts:66-78` handles SIGINT/SIGTERM by removing the clones, calling + `process.removeAllListeners(sig)`, and re-raising with the default action. The default action + ends the process without emitting `exit`. The lock is released only from an `exit` listener + (`project-lock.ts:502-515`) or its `finally`, so neither runs. + - Without a fetch (`remove`, or `add`/`update` before `fetchRepo`) there is no handler at all. + The default action strands the lock there too. + - The next run on the same host reclaims the lock via the dead-pid check. A different host (a bind + mount) waits `STALE_MS` = 6 h (`project-lock.ts:84`). +- F12, read on HEAD (`repo.ts:239-270`): on `!res.ok`, `fetchCommitSha` returns `null` without + touching the body. On 2xx it calls `res.json()`, a read with no reader of its own, so nothing + cancels it at the 8 s deadline. `withDeadline` answers the caller, but the socket keeps the event + loop alive (`deadline.ts:1-20` documents why the abort alone is not enough on Node 20/22). + - A null SHA is a degraded mode, not an exit, so the command finishes normally. `index.ts` calls + `process.exit` only on failure, so the process lives until the body ends: 30 s in the review's + 403-drip measurement. + - `downloadArchive` (`:192-194`) also throws on `!res.ok` with the body untouched. That is benign + today only because every caller then exits 1. +- Only `tests/repo.test.ts` (6) and `tests/repo-signals.test.ts` (2) stub the SHA response with + `json:`. The command tests mock `repo.js` wholesale. +- The docs say a `SIGKILL`ed run cannot release (`docs/troubleshooting.md:369`). They make no claim + either way about SIGINT/SIGTERM. + +## Files + +- `src/lib/fatal-signal.ts` (new) — layer CLI/lib. `onFatalSignal(fn): () => void`, a + once-installed SIGINT/SIGTERM handler (SIGHUP deliberately NOT handled — see the amendment under + Open questions) that works in four steps: + 1. Runs every registered cleanup synchronously, last-registered first, each in its own try/catch. + 2. Clears the set. + 3. Calls `removeAllListeners(sig)`. + 4. Re-raises `sig`, so the exit status stays 130/143. + + The re-raise moves here from `repo.ts` unchanged. One axis: dying cleanly on a fatal signal. + +- `src/lib/repo.ts` — layer CLI/lib. The clone cleanup registers through `onFatalSignal`; its + `exit` listener stays as it is. `fetchCommitSha` gets two fixes: + - On a non-2xx status it calls `res.body?.cancel()` (errors ignored) and returns `null`. + - On 2xx it reads the body through its own reader, with an abort listener that calls + `reader.cancel()` (the `downloadArchive` pattern), then parses the JSON. + + `downloadArchive` cancels the body before its non-2xx throw. + +- `src/lib/project-lock.ts` — layer CLI/lib. `withProjectLock` registers `release(cwd)` plus the + same "interrupted while writing" stderr line as the `exit` path, for the lock's lifetime, and + deregisters in its `finally`. +- `tests/project-lock.test.ts` — layer tests. Child-process cases (the `tsx` pattern already used + there): + - The lock is held with no fetch, and the process sends itself SIGTERM. It exits 143, the lock is + gone, and stderr names the interruption. FAILS on base. + - The same with SIGINT after a stubbed `fetchRepo`. It exits 130, and both the lock and the clone + are gone. FAILS on base. + - After a normal release, a later SIGTERM leaves alone a lock file another process created (the + `release` re-read, plus the deregistration). +- `tests/fatal-signal.test.ts` (new) — layer tests. In-process, with `process.kill` stubbed: + cleanups run in reverse order, a throwing cleanup does not stop the rest, a deregistered cleanup + does not run, and the handlers install once. +- `tests/repo.test.ts` — layer tests: + - A 403 whose body stream records `cancel` → the cancel was called. FAILS on base. + - A 2xx body that never ends → at the 8 s deadline the reader was cancelled and the result is + `null`. FAILS on base. + - A `downloadArchive` 500 → the body was cancelled. FAILS on base. + - The existing `json:` stubs become stream bodies. +- `tests/repo-signals.test.ts` — layer tests. Its SHA stub becomes a stream body. The clone-removal + and exit-130 assertions stay as they are. +- `docs/troubleshooting.md` — layer docs. The lock section: Ctrl-C, `timeout` and `docker stop` + (SIGINT/SIGTERM) release the lock and exit 130/143. SIGKILL, power loss and a hangup (SIGHUP) + still cannot release. +- `CHANGELOG.md` — `[Unreleased]` → `### Fixed`, two entries + +## Contracts satisfied + +- The lock's contract in `docs/reference/pharn-records.md` ("a second pharn process refuses instead + of interleaving"). It stops costing a 6 h wait after an ordinary interrupt (cited, P4). +- `deadline.ts`'s stated rule, "callers additionally cancel their body reader on abort". It now + holds for every fetch in `repo.ts`. + +## Evals to write (P1) + +- Listed under Files. Five cases FAIL on the base. + +## Guarantee audit (P0) + +- "a SIGINT/SIGTERM delivered while the lock is held releases it" → floor: child-process tests + observing the exit status and the lock file. The residual is named: SIGKILL, power loss, and a + signal that arrives before `withProjectLock` registers. +- "a non-2xx or timed-out SHA response cannot keep the process alive" → floor: cancel-spy tests over + both paths. Wall-clock exit timing is not asserted (flaky). The mechanism is. +- "the exit status stays truthful (130/143)" → floor: the existing `repo-signals` test plus the + new ones. + +## Trust audit (P2) + +- The response bodies are untrusted, and they are now consumed less: cancelled, never drained. No + new data reaches output. + +## Determinism audit (P5) + +- A fixed signal list and set membership. No fallback path. + +## Open questions (HALT) + +None open. Resolved at GATE 1 (human, 2026-09-25): every question below → **(a)**, the +recommended answer. Kept for the record: + +1. SIGHUP (terminal closed while `update` holds the lock): (a) handle it too, re-raised as exit 129 — + recommended. The failure is the same stranded lock. (b) SIGINT/SIGTERM only, as measured. + +**Amendment during build (human, 2026-09-25): question 1 → (b).** Measured before review: a +`process.on('SIGHUP')` listener overrides the `SIG_IGN` that `nohup` sets. Node 22 ran the handler +even under `trap '' HUP`. So handling SIGHUP would make a hangup interrupt a `nohup pharn update` +mid-write, where today it keeps running. Asked again with that finding, the human chose +SIGINT/SIGTERM only. A hangup's stale lock is still reclaimed automatically on the same machine +(dead-pid check). diff --git a/.dev/features/signal-lock-release/REGRESSION.md b/.dev/features/signal-lock-release/REGRESSION.md new file mode 100644 index 0000000..aef53f1 --- /dev/null +++ b/.dev/features/signal-lock-release/REGRESSION.md @@ -0,0 +1,40 @@ +# REGRESSION — signal-lock-release + +The verdict below is computed by `.dev/floor/check-regress.mjs`, not by this stage's judgment. + +## Base and partition + +- **base:** `cf6582e92eaae15e3dd431824107101da741b9b8` (`HEAD` — `origin/main` after #219; the + build is an uncommitted working tree on top of it). +- **inside** (each declared in `PLAN.md` `## Files`): `src/lib/fatal-signal.ts` (new), + `src/lib/repo.ts`, `src/lib/project-lock.ts`, `tests/fatal-signal.test.ts` (new), + `tests/project-lock.test.ts`, `tests/repo.test.ts`, `tests/repo-signals.test.ts`, + `docs/troubleshooting.md`, `CHANGELOG.md`. +- **scope partition:** `check-regress.mjs scope` exited **0**, `escaped: []`. `.pharn/` (hook + scratch) and this feature's own stage artifacts are not build output. +- **outside gates:** the 46 stdlib `*.test.mjs` / `*.test.cjs` files `scope` returned (754 tests) + + whole-repo `validate`; 0 committed eval pairs. +- **style-gate skip:** `inside` touches no shared style config, so `lint` / `format:check` / + `lint:md` are absent from both maps. +- **environment:** both sides ran with no proxy variables and as root **without** + `CAP_DAC_OVERRIDE` / `CAP_DAC_READ_SEARCH` / `CAP_FOWNER` (`setpriv`), the CI-equivalent of this + root sandbox. + +## Per-gate exit codes + +| gate | base | head | flipped? | +| ---------- | ---- | ---- | -------- | +| `tests` | 0 | 0 | no | +| `validate` | 0 | 0 | no | + +- `regressions[]`: **empty** +- `pre_existing[]`: **empty** + +## Verdict + +**REGRESSIONS: none — no deterministically-detectable breakage outside the feature.** +(`regression-report.json` `.verdict` = `no-regressions`.) + +Residual (P0/P7): this catches exactly what its suite catches. The vitest suite exercising `src/**` +is owned by `/pharn-dev-build`'s floor and `/pharn-dev-verify`. This certifies the comparison, never +the increment. diff --git a/.dev/features/signal-lock-release/REVIEW.md b/.dev/features/signal-lock-release/REVIEW.md new file mode 100644 index 0000000..5e844bd --- /dev/null +++ b/.dev/features/signal-lock-release/REVIEW.md @@ -0,0 +1,65 @@ +# REVIEW — signal-lock-release + +Increment: + +- `src/lib/fatal-signal.ts` (new): `onFatalSignal`, a lazily installed SIGINT/SIGTERM handler. It runs + the registered cleanups newest first, removes every listener, re-raises, and falls back to + `exit(128 + n)`. SIGHUP is deliberately excluded (the plan amendment: `nohup`). +- `src/lib/repo.ts`: the clone cleanup registers through `onFatalSignal`. `fetchCommitSha` discards a + non-2xx body and reads a 2xx body through its own abort-cancelled reader. `downloadArchive` + discards a non-2xx body. +- `src/lib/project-lock.ts`: `withProjectLock` registers the release plus the interrupted line for + the lock's lifetime. +- Tests in four files, `docs/troubleshooting.md`, and two CHANGELOG entries. + +Treated as `trust: untrusted`; nothing in it read as an instruction. + +## Floor first (P0) + +`node .dev/floor/validate.mjs .` → `FLOOR: GREEN` (exit 0). `/pharn-dev-build`'s `npm run check` +passed (1530 tests), `/pharn-dev-regress` returned `no-regressions`, and `/pharn-dev-verify` +returned `PASS`. + +## Floor-gate findings (blocking) + +None. + +- **L-floor (P0)** + - "a SIGINT/SIGTERM while the lock is held releases it and still dies by the signal" → child + processes observing the exit status and the lock file. + - "the clone goes too" → the same child. + - "a released lock's cleanup can never delete another process's lock" → a child writes an + empty (mid-create) lock after release and signals itself. The lock survives. + - "no SIGHUP listener" → a membership test. + - "an unread SHA body is released" → observable-cancel streams, per the grill. + - What the floor does not reach is named in VERIFY.md: SIGKILL, power loss, a hangup, a signal + before registration, and the stub-only fallback. +- **L-eval (P1)** — 14 of the new cases fail when run against the base `src/`, checked in a + worktree. The rest are guards: a leftover lock, the in-process clone cleanup, and the unchanged + deadline answer. +- **L-trust (P2)** — untrusted response bodies are now consumed less (cancelled, never drained). + Nothing new reaches output. +- **L-axis (P3)** — `fatal-signal.ts` has one reason to change. `repo.ts` and `project-lock.ts` each + depend on it and not on each other. The body helpers stay in `repo.ts`, the fetch boundary's + own module. + +## Advisory findings (warn — severity is this reviewer's judgment, fix #3) + +```yaml +- type: FINDING + rule_id: 'P0' + severity: minor + file: 'src/lib/repo.ts:294' + problem: "readText still has no byte cap, the same as the res.json() it replaces, so a hostile or broken API response can grow memory for up to 8 s. skills-version.ts caps its body at 256 KB. A cap needs a size above real commit responses, which carry file lists; that is out of this increment's scope, so it is named, not added." + evidence: 'async function readText(res: Response, signal: AbortSignal): Promise {' +- type: FINDING + rule_id: 'P5' + severity: minor + file: 'src/lib/fatal-signal.ts:83' + problem: "removeAllListeners(sig) removes every listener on the signal, not only clack's, before the re-raise. That is the pre-existing repo.ts behavior, moved unchanged, and correct for a process that is about to die. A future listener meant to observe a signal and let the process live would be removed too; the header comment says so." + evidence: 'process.removeAllListeners(sig);' +``` + +## Verdict + +**GREEN — 0 floor-gate findings, 2 advisory (minor).** The standing decision is the human's (GATE 2). diff --git a/.dev/features/signal-lock-release/SHIP.md b/.dev/features/signal-lock-release/SHIP.md new file mode 100644 index 0000000..deb9f27 --- /dev/null +++ b/.dev/features/signal-lock-release/SHIP.md @@ -0,0 +1,26 @@ +# SHIP — signal-lock-release + +Stages run, in order: + +1. `/pharn-dev-plan` → GATE 1 (human: plans A–F accepted with every recommended answer). +2. `/pharn-dev-grill`. +3. The plan's `## Files` was reworded so the writes-scope parser reads every path (formatting only). +4. `/pharn-dev-build`. +5. **Plan amended by the human mid-build.** Before review, a measurement showed that handling SIGHUP + overrides `nohup`. Asked again, the human chose SIGINT/SIGTERM only (open question 1 → b). The + build was changed to match. +6. `/pharn-dev-build` floor re-run → `/pharn-dev-regress` → `/pharn-dev-verify` → + `/pharn-dev-review` → GATE 2. + +| stage | structural verdict (verbatim) | +| -------------------- | ------------------------------------------------------ | +| `/pharn-dev-build` | `node .dev/floor/validate.mjs .` exit `0` | +| `/pharn-dev-regress` | `regression-report.json` `.verdict` = `no-regressions` | +| `/pharn-dev-verify` | `verify-report.json` `.verdict` = `PASS` | + +- Review: [`REVIEW.md`](REVIEW.md) · Grill (advisory): [`GRILL.md`](GRILL.md) +- The run ended at **GATE 2**. The human's standing instruction for this batch: after each + increment, open a pull request and merge it once its checks are green, then start the next plan. + +chain ran; the named floor verdicts are as shown — this is NOT a judgment that the increment is good or +wise; that is the human's call at the post-review gate. diff --git a/.dev/features/signal-lock-release/VERIFY.md b/.dev/features/signal-lock-release/VERIFY.md new file mode 100644 index 0000000..3452371 --- /dev/null +++ b/.dev/features/signal-lock-release/VERIFY.md @@ -0,0 +1,54 @@ +# VERIFY — signal-lock-release + +## FLOOR layer (owns the verdict) + +The gates ran over the whole repo with the feature present, on node 22, with the session proxy +variables unset. They ran as root **without** `CAP_DAC_OVERRIDE` / `CAP_DAC_READ_SEARCH` / +`CAP_FOWNER` (`setpriv`), the CI-equivalent of this root sandbox. + +| gate | exit | +| -------------- | ---- | +| `format:check` | 0 | +| `lint` | 0 | +| `lint:md` | 0 | +| `test` | 0 | +| `test:floor` | 0 | +| `typecheck` | 0 | +| `validate` | 0 | + +- `test` is vitest (1530 tests). It collects this increment's own tests: `tests/fatal-signal.test.ts`, + the child-process and in-process cases in `tests/project-lock.test.ts`, and the observable-cancel + cases in `tests/repo.test.ts`. 14 of them fail when run against the base `src/` (checked in a + worktree of `cf6582e`). +- `test:floor` is floor.yml's `node --test` run (754 tests). +- There is no `structural:*` gate: the increment ships no eval-actual pair. + +**VERDICT: PASS** (`.dev/floor/check-verify.mjs`, `failing_gates: []`). + +Outside the verdict, the review's reproduction was re-run on this build. A process holds the lock, +runs `fetchRepo`, and signals itself: + +| signal | base: exit / lock left | head: exit / lock left | +| ------- | ---------------------- | ---------------------- | +| SIGTERM | 143 / yes | 143 / no | +| SIGINT | 130 / yes | 130 / no | + +SIGHUP is deliberately not handled (the plan's amendment, decided by the human). Measured on Node +22: a `process.on('SIGHUP')` listener ran even under `trap '' HUP`, the `SIG_IGN` that `nohup` +sets. A test pins that pharn adds no SIGHUP listener. + +## ADVISORY layer + +`node .dev/floor/count-verifiers.mjs .` → `{"registered":0,"verifiers":[]}`. No verifiers are +registered, so the verdict rests on the floor gates only. + +Residual (P0/P7): verified = the named gates passed; this is NOT a guarantee of correctness beyond +what those gates check — verifier concerns are advisory help, not assurance. + +What the gates do not reach: + +- SIGKILL, power loss and a hangup (SIGHUP) still strand the lock. The same host reclaims it by + the dead-pid check; another host waits out `STALE_MS`. +- A signal before `withProjectLock` registers keeps Node's default disposition, as before. +- The defensive fallback (`exit(128 + n)` when the re-raise throws) is proven with a stub only. CI + runs no Windows. diff --git a/.dev/features/signal-lock-release/regression-report.json b/.dev/features/signal-lock-release/regression-report.json new file mode 100644 index 0000000..f0d5d2d --- /dev/null +++ b/.dev/features/signal-lock-release/regression-report.json @@ -0,0 +1,27 @@ +{ + "base": "cf6582e92eaae15e3dd431824107101da741b9b8", + "inside": [ + "CHANGELOG.md", + "docs/troubleshooting.md", + "src/lib/fatal-signal.ts", + "src/lib/project-lock.ts", + "src/lib/repo.ts", + "tests/fatal-signal.test.ts", + "tests/project-lock.test.ts", + "tests/repo-signals.test.ts", + "tests/repo.test.ts" + ], + "outside_gates": { + "tests": { + "base": 0, + "head": 0 + }, + "validate": { + "base": 0, + "head": 0 + } + }, + "regressions": [], + "pre_existing": [], + "verdict": "no-regressions" +} diff --git a/.dev/features/signal-lock-release/verify-report.json b/.dev/features/signal-lock-release/verify-report.json new file mode 100644 index 0000000..1ecdbce --- /dev/null +++ b/.dev/features/signal-lock-release/verify-report.json @@ -0,0 +1,18 @@ +{ + "feature": "signal-lock-release", + "gates": { + "format:check": 0, + "lint": 0, + "lint:md": 0, + "test": 0, + "test:floor": 0, + "typecheck": 0, + "validate": 0 + }, + "verdict": "PASS", + "failing_gates": [], + "verifiers": { + "registered": 0, + "findings": [] + } +} diff --git a/.pharn/writes-scope.json b/.pharn/writes-scope.json index fb5e879..cec44ed 100644 --- a/.pharn/writes-scope.json +++ b/.pharn/writes-scope.json @@ -1,7 +1,7 @@ { "scope": [ - ".dev/features/engines-styletext-floor/REVIEW.md" + ".dev/features/signal-lock-release/SHIP.md" ], - "set_by": ".claude/commands/pharn-dev-review.md", - "set_at": "2026-09-25T05:06:19.849Z" + "set_by": ".claude/commands/pharn-dev-ship.md", + "set_at": "2026-09-25T05:22:33.736Z" } diff --git a/CHANGELOG.md b/CHANGELOG.md index b421ea4..2559a68 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **`pharn init` no longer overwrites a config another `pharn` command wrote while its prompts were open.** It now re-checks `pharn.config.json` under its lock, like `add`, `update` and `remove`. If the file changed after `init` read it, `init` refuses and writes nothing. - **A re-run `pharn init` dropped the `pharn.config.json` keys you added by hand.** Upstream PHARN reads top-level keys that users add themselves: `testResults` (without it `/pharn-loop` stops with `blocked: no-test-runner`) and `ship.requireAttestation`. `add`, `update` and `remove` kept them, but `init` rebuilt the config from its own fields alone. It now copies every top-level key pharn does not own across from the config it replaces, unchanged. It does so from any file that parses as a JSON object, including one the other commands refuse. Keys pharn owns are still written fresh. - **`pharn update` could stop re-checking a capability whose files were still out of date.** When pharn cannot read a capability upstream, `update` keeps it, skips its files, and moves the skills version on. Once pharn could read it again, the next run removed it from `frozenCapabilities` even if one of its files had to be skipped, for example because you edited it. Every later run then said "Already up to date", and that file stayed at the old version even after you resolved your edit. The capability now stays listed until none of its files are skipped, so each run checks it again. `update` also no longer writes a `pendingSkillsVersion` equal to the recorded `skillsVersion`. +- **An interrupted run no longer leaves `.pharn.lock` behind.** The lock was released only on a normal return or a `process.exit`. A real signal ends the process without either, so `pharn update --yes` cancelled in CI, or stopped by `timeout` or `docker stop`, exited 130/143 with the lock still in the project. On the same machine the next run reclaimed it. A machine sharing the directory (a container bind mount) had to wait out the six-hour staleness window. `SIGINT` and `SIGTERM` now release the lock and remove the temp download first, print that the project may be partially updated, and still exit 130 or 143. A hangup (`SIGHUP`) is deliberately not handled: listening for it would override `nohup`, so a `nohup pharn update` would stop mid-write. +- **A slow or failed GitHub API response no longer keeps `pharn` running after it has finished.** When the commit-SHA lookup got an error response (for example a 403 rate limit), or a success whose body was still arriving at the 8-second limit, the command carried on without the SHA as designed, but it left that response unread. The open connection kept the process alive until the server finished sending. It was measured at 30 seconds after `pharn status` had printed its last line. Such responses are now released at once. ### Security diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index d0443c6..04d7e96 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -366,8 +366,16 @@ both of its prompts, but the picker and the overwrite confirmation are answered walked-away `init` can block other writers in that project until the six-hour staleness window expires. `add`'s picker holds it across the multi-select for the same reason. -A run that is `SIGKILL`ed or loses power cannot release. `pharn` breaks such a lock by itself when -any of these hold: +An interrupted run releases the lock on its way out. That covers Ctrl-C, and a `timeout` or +`docker stop`, which arrive as `SIGINT` and `SIGTERM`. The run still exits 130 or 143 and prints +that the project may be partially updated. Three things cannot release the lock: + +- a run that is `SIGKILL`ed; +- one that loses power; +- one ended by a hangup (`SIGHUP`, a closed terminal). + +The hangup case is deliberate: listening for it would override `nohup`, and a `nohup pharn update` +would then stop mid-write. `pharn` breaks such a lock by itself when any of these hold: - the file is missing, unreadable, or not a well-formed lock payload; - it is more than six hours old; diff --git a/src/lib/fatal-signal.ts b/src/lib/fatal-signal.ts new file mode 100644 index 0000000..6f08b2c --- /dev/null +++ b/src/lib/fatal-signal.ts @@ -0,0 +1,93 @@ +// --------------------------------------------------------------------------- +// Dying cleanly on a fatal signal — the one place pharn handles SIGINT and +// SIGTERM. +// +// Node's default action for these signals ends the process WITHOUT emitting +// `exit`, so no `exit` listener and no `finally` runs on one. Two things must +// run anyway: removing the temp clone (lib/repo.ts) and releasing the project +// lock (lib/project-lock.ts). The lock is the costly one: `pharn update --yes` +// cancelled in CI, or killed by `timeout` / `docker stop`, left .pharn.lock in +// the project, and a host sharing that directory waited out the lock's six-hour +// staleness window before it could write again. +// +// So each owner registers a synchronous cleanup here for as long as it owns +// something. On a signal every registered cleanup runs, newest first, and then +// the signal is re-raised with its default disposition, so the exit status +// stays truthful: 130 SIGINT, 143 SIGTERM — never the 0 an interrupted install +// once reported to the script that invoked it. +// +// NOT SIGHUP, on purpose. A listener overrides the SIG_IGN that `nohup` sets — +// measured: Node ran a SIGHUP handler even under `trap '' HUP` — so handling it +// would make a hangup INTERRUPT a `nohup pharn update` mid-write, where today it +// keeps running. A hangup without nohup still ends the run by default and can +// leave the lock; the next run on the same host reclaims it (dead pid). +// +// `removeAllListeners` comes before the re-raise: any other listener on the +// signal (@clack/prompts' spinner handler prints and returns) would otherwise +// swallow it, and the process would run on. +// +// The handlers install on the FIRST registration, not at import, so a run that +// never owns a clone or a lock keeps Node's default disposition untouched. +// +// One axis (P3): what happens between a fatal signal and the process's end. +// --------------------------------------------------------------------------- + +const FATAL_SIGNALS = ['SIGINT', 'SIGTERM'] as const; +type FatalSignal = (typeof FATAL_SIGNALS)[number]; + +/** POSIX signal numbers — the exit status is 128 + n. */ +const SIGNAL_NUMBER: Record = { + SIGINT: 2, + SIGTERM: 15, +}; + +/** Registration order; `die` walks it backwards. */ +const cleanups = new Set<() => void>(); +let installed = false; + +/** + * Run `cleanup` if a fatal signal arrives before the returned deregister + * function is called. `cleanup` must be synchronous — nothing after the + * re-raise runs — and should not throw; a throw is swallowed so it cannot + * replace the real exit reason or stop the cleanups after it. + */ +export function onFatalSignal(cleanup: () => void): () => void { + install(); + // A fresh wrapper per registration, so registering the same function twice + // is two registrations, each removed by its own deregister. + const entry = (): void => cleanup(); + cleanups.add(entry); + return () => { + cleanups.delete(entry); + }; +} + +function install(): void { + if (installed) return; + installed = true; + for (const sig of FATAL_SIGNALS) process.on(sig, () => die(sig)); +} + +function die(sig: FatalSignal): void { + const pending = [...cleanups].reverse(); + // Cleared first: a second signal arriving mid-cleanup must not run any of + // them twice. + cleanups.clear(); + for (const cleanup of pending) { + try { + cleanup(); + } catch { + /* a cleanup must never replace the real exit reason */ + } + } + process.removeAllListeners(sig); + try { + process.kill(process.pid, sig); + } catch { + // Defensive: every platform Node supports can deliver these two to itself + // (win32 emulates both). But if the re-raise ever throws, end the process + // with the status the signal would have produced — running on with every + // listener removed is not an option. + process.exit(128 + SIGNAL_NUMBER[sig]); + } +} diff --git a/src/lib/project-lock.ts b/src/lib/project-lock.ts index 9afde96..d6ba32e 100644 --- a/src/lib/project-lock.ts +++ b/src/lib/project-lock.ts @@ -11,6 +11,7 @@ import { writeSync, } from 'node:fs'; import { hostname } from 'node:os'; +import { onFatalSignal } from './fatal-signal.js'; import { safeJoin } from './validate.js'; // --------------------------------------------------------------------------- @@ -503,20 +504,35 @@ export async function withProjectLock( release(cwd); if (code === 0) { process.exitCode = 130; - try { - process.stderr.write( - `pharn ${command} was interrupted while writing to this project — it may be partially updated. Re-run \`pharn ${command}\`.\n`, - ); - } catch { - /* the exit code already tells the truth */ - } + sayInterrupted(command); } }; process.on('exit', onExit); + // A REAL signal (SIGINT with stdin not raw, SIGTERM from `timeout` or + // `docker stop`) ends the process by its default action, which emits no + // `exit` — so neither the listener above nor the `finally` below runs on one. + // fatal-signal.ts runs this first, then re-raises; the status (130 / 143) + // already says the run did not finish. (SIGHUP is not handled — see there.) + const offSignal = onFatalSignal(() => { + release(cwd); + sayInterrupted(command); + }); try { return await fn(); } finally { + offSignal(); process.off('exit', onExit); release(cwd); } } + +/** The one honest line an interrupted writer prints. Never throws. */ +function sayInterrupted(command: string): void { + try { + process.stderr.write( + `pharn ${command} was interrupted while writing to this project — it may be partially updated. Re-run \`pharn ${command}\`.\n`, + ); + } catch { + /* the exit status already tells the truth */ + } +} diff --git a/src/lib/repo.ts b/src/lib/repo.ts index fbecf90..9c447e8 100644 --- a/src/lib/repo.ts +++ b/src/lib/repo.ts @@ -3,6 +3,7 @@ import { mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { REPO, REPO_BRANCH } from './constants.js'; +import { onFatalSignal } from './fatal-signal.js'; import { extractTarGz } from './tar-extract.js'; import { assertSafeString, COMMIT_RE } from './validate.js'; @@ -34,7 +35,9 @@ const MAX_ENTRIES = 20_000; // window in the CLI ends by leaking the clone AND reporting success. // 2. A signal. Nothing in pharn handled one, so default disposition applied — // or worse, in a piped run @clack/prompts' own SIGINT listener printed and -// returned, swallowing the signal entirely. +// returned, swallowing the signal entirely. The signal half now lives in +// lib/fatal-signal.ts, shared with the project lock, which has to be +// released on the same signals. // // Registered dirs are removed synchronously, because an `exit` listener may not // await. @@ -63,19 +66,13 @@ function installCleanupHandlers(): void { for (const dir of liveClones) rmQuiet(dir); }); - for (const sig of ['SIGINT', 'SIGTERM'] as const) { - process.on(sig, () => { - for (const dir of liveClones) rmQuiet(dir); - liveClones.clear(); - // Re-raise so the exit status is TRUTHFUL — 130 for SIGINT, 143 for - // SIGTERM — rather than the 0 an interrupted install used to report to - // whatever script invoked it. removeAllListeners first: any other listener - // on this signal (clack's spinner handler prints and returns) would - // otherwise swallow the re-raise and hang the process. - process.removeAllListeners(sig); - process.kill(process.pid, sig); - }); - } + // Registered for the life of the process — the set it drains is empty + // whenever no clone is live. fatal-signal.ts re-raises afterwards, so the exit + // status stays truthful (130 / 143). + onFatalSignal(() => { + for (const dir of liveClones) rmQuiet(dir); + liveClones.clear(); + }); } export interface FetchedRepo { @@ -190,6 +187,10 @@ async function downloadArchive(ref: string): Promise { async (signal) => { const res = await fetch(url, { redirect: 'error', signal }); if (!res.ok) { + // Released, not left streaming: an unread body holds its socket, and + // with it the process. Harmless today only because every caller exits 1 + // on this throw — a property of the callers, so the release is made here. + await discardBody(res); throw new Error(`Failed to download ${url}: HTTP ${res.status}`); } if (!res.body) { @@ -253,8 +254,19 @@ export async function fetchCommitSha(): Promise { 'X-GitHub-Api-Version': '2022-11-28', }, }); - if (!res.ok) return null; - const body = (await res.json()) as { sha?: unknown }; + // A null SHA is a degraded mode, not an exit: the command carries on and + // finishes, and index.ts never calls process.exit on success. So a body + // left streaming here — a 403 page, or a 2xx still arriving at the + // deadline — kept the process alive after the command's last line was + // printed (measured: 30 s for a dripping 403). Neither may outlive this + // function. + if (!res.ok) { + await discardBody(res); + return null; + } + const body = JSON.parse(await readText(res, signal)) as { + sha?: unknown; + }; return typeof body.sha === 'string' ? body.sha : null; }, ); @@ -262,3 +274,36 @@ export async function fetchCommitSha(): Promise { return null; } } + +/** Release a response body that will not be read. Never throws. */ +async function discardBody(res: Response): Promise { + try { + await res.body?.cancel(); + } catch { + /* already errored or locked: there is nothing left to release */ + } +} + +/** + * The whole body as UTF-8, read through OUR reader and cancelled by OUR + * listener on OUR signal — downloadArchive's pattern. `res.json()` has no + * reader pharn can cancel, and on Node 20/22 the abort may never reach the body + * stream (lib/deadline.ts), so a body still streaming at the deadline outlived + * the command. + */ +async function readText(res: Response, signal: AbortSignal): Promise { + if (!res.body) return ''; + const reader = res.body.getReader(); + const cancel = (): void => { + reader.cancel().catch(() => undefined); + }; + if (signal.aborted) cancel(); + else signal.addEventListener('abort', cancel, { once: true }); + const chunks: Uint8Array[] = []; + for (;;) { + const { done, value } = await reader.read(); + if (done) break; + chunks.push(value); + } + return Buffer.concat(chunks).toString('utf8'); +} diff --git a/tests/fatal-signal.test.ts b/tests/fatal-signal.test.ts new file mode 100644 index 0000000..79cf15e --- /dev/null +++ b/tests/fatal-signal.test.ts @@ -0,0 +1,134 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +// lib/fatal-signal.ts, in-process. The real handler removes every listener on +// the signal and re-raises it — against the test runner itself — so both are +// stubbed here and the handler is invoked directly. The exit STATUS a real +// signal produces is proven in a child process (tests/project-lock.test.ts, +// tests/repo-signals.test.ts); this file pins the ordering and isolation rules. + +const SIGNALS = ['SIGINT', 'SIGTERM'] as const; +type Sig = (typeof SIGNALS)[number]; + +let before: Record; + +/** A fresh module instance: the handlers install once PER MODULE. */ +async function fresh(): Promise { + vi.resetModules(); + return import('../src/lib/fatal-signal.js'); +} + +/** The listener the fresh module added for `sig` (exactly one expected). */ +function addedListeners(sig: Sig): Array<() => void> { + return process + .listeners(sig) + .filter((l) => !before[sig].includes(l)) as Array<() => void>; +} + +beforeEach(() => { + before = { + SIGINT: process.listeners('SIGINT'), + SIGTERM: process.listeners('SIGTERM'), + }; + vi.spyOn(process, 'kill').mockImplementation(() => true); + vi.spyOn(process, 'removeAllListeners').mockImplementation(() => process); +}); + +afterEach(() => { + vi.restoreAllMocks(); + for (const sig of SIGNALS) { + for (const l of addedListeners(sig)) process.removeListener(sig, l); + } +}); + +describe('onFatalSignal', () => { + it('installs nothing until something registers', async () => { + await fresh(); + for (const sig of SIGNALS) expect(addedListeners(sig)).toHaveLength(0); + }); + + // `nohup` sets SIGHUP to SIG_IGN, and a listener would override that — a + // hangup would then interrupt a `nohup pharn update` mid-write. So pharn must + // never listen for it. + it('leaves SIGHUP alone, so nohup keeps shielding a run', async () => { + const hupBefore = process.listeners('SIGHUP'); + const { onFatalSignal } = await fresh(); + onFatalSignal(() => undefined); + expect(process.listeners('SIGHUP')).toEqual(hupBefore); + }); + + it('installs one handler per signal, however many register', async () => { + const { onFatalSignal } = await fresh(); + onFatalSignal(() => undefined); + onFatalSignal(() => undefined); + for (const sig of SIGNALS) expect(addedListeners(sig)).toHaveLength(1); + }); + + it('runs every cleanup, newest first, then re-raises the same signal', async () => { + const { onFatalSignal } = await fresh(); + const order: string[] = []; + onFatalSignal(() => order.push('clone')); + onFatalSignal(() => order.push('lock')); + + addedListeners('SIGTERM')[0]!(); + + expect(order).toEqual(['lock', 'clone']); + // Every other listener goes first — clack's print-only SIGINT listener + // would otherwise swallow the re-raise and the process would run on. + expect(process.removeAllListeners).toHaveBeenCalledWith('SIGTERM'); + expect(process.kill).toHaveBeenCalledWith(process.pid, 'SIGTERM'); + }); + + it('keeps going when one cleanup throws', async () => { + const { onFatalSignal } = await fresh(); + const ran: string[] = []; + onFatalSignal(() => ran.push('first-registered')); + onFatalSignal(() => { + throw new Error('boom'); + }); + + expect(() => addedListeners('SIGINT')[0]!()).not.toThrow(); + expect(ran).toEqual(['first-registered']); + expect(process.kill).toHaveBeenCalledWith(process.pid, 'SIGINT'); + }); + + it('never runs a cleanup that was deregistered', async () => { + const { onFatalSignal } = await fresh(); + const ran: string[] = []; + const off = onFatalSignal(() => ran.push('released')); + off(); + + addedListeners('SIGTERM')[0]!(); + + expect(ran).toEqual([]); + expect(process.kill).toHaveBeenCalledWith(process.pid, 'SIGTERM'); + }); + + it('runs each cleanup at most once, even if a second signal arrives', async () => { + const { onFatalSignal } = await fresh(); + let runs = 0; + onFatalSignal(() => { + runs += 1; + }); + addedListeners('SIGINT')[0]!(); + addedListeners('SIGTERM')[0]!(); + expect(runs).toBe(1); + }); + + // Defensive: should the re-raise ever throw, the process must still END, with + // the status the signal would have produced — never run on with its + // listeners removed. + it('exits 128 + the signal number when the re-raise throws', async () => { + const { onFatalSignal } = await fresh(); + vi.mocked(process.kill).mockImplementation(() => { + throw Object.assign(new Error('kill ENOSYS'), { code: 'ENOSYS' }); + }); + const exit = vi + .spyOn(process, 'exit') + .mockImplementation((() => undefined) as never); + onFatalSignal(() => undefined); + + addedListeners('SIGTERM')[0]!(); + + expect(exit).toHaveBeenCalledWith(143); + }); +}); diff --git a/tests/project-lock.test.ts b/tests/project-lock.test.ts index f57a0ed..7979707 100644 --- a/tests/project-lock.test.ts +++ b/tests/project-lock.test.ts @@ -461,4 +461,176 @@ describe('withProjectLock — process.exit while held (PHARN-10)', () => { await withProjectLock(tmp.path(), 'add', () => undefined); expect(process.listenerCount('exit')).toBe(before); }); + + // The fatal-signal release, IN-PROCESS (coverage cannot see the children in + // the next block). A fresh module instance, so its signal handler is the one + // listener it added; the re-raise and the listener sweep are stubbed — they + // would act on the test runner itself. + it('in-process: the fatal-signal handler releases a held lock', async () => { + const dir = tmp.path(); + const sigs = ['SIGINT', 'SIGTERM'] as const; + const before = new Map( + sigs.map((s) => [s, process.listeners(s)]), + ); + vi.resetModules(); + const fresh = await import('../src/lib/project-lock.js'); + const kill = vi.spyOn(process, 'kill').mockImplementation(() => true); + const sweep = vi + .spyOn(process, 'removeAllListeners') + .mockImplementation(() => process); + const write = vi.spyOn(process.stderr, 'write').mockReturnValue(true); + try { + await fresh.withProjectLock(dir, 'update', () => { + const added = process + .listeners('SIGTERM') + .filter((l) => !before.get('SIGTERM')!.includes(l)) as Array< + () => void + >; + expect(added).toHaveLength(1); + added[0]!(); + expect(existsSync(lockPath(dir))).toBe(false); + }); + expect(String(write.mock.calls[0]?.[0])).toContain( + 'pharn update was interrupted while writing', + ); + expect(kill).toHaveBeenCalledWith(process.pid, 'SIGTERM'); + } finally { + kill.mockRestore(); + sweep.mockRestore(); + write.mockRestore(); + for (const sig of sigs) { + for (const l of process.listeners(sig)) { + if (!before.get(sig)!.includes(l)) + process.removeListener(sig, l as (...args: unknown[]) => void); + } + } + } + }); +}); + +// A REAL signal ends the process by its default action, which emits no `exit` +// — so the listener above never ran on one, and neither did any `finally`. +// `pharn update --yes` cancelled in CI, or killed by `timeout` / `docker stop`, +// exited 130/143 with .pharn.lock still in the project. The next run on the +// same host reclaimed it (dead pid), but a host sharing the directory waited +// out STALE_MS — six hours. lib/fatal-signal.ts now runs the release first. +// +// Only a child process can take a real signal: the handler re-raises it. +describe('withProjectLock — a fatal signal while held', () => { + const tmp = useTmpDir(); + + const SIGNAL_EXIT = { SIGINT: 130, SIGTERM: 143 } as const; + + async function runChild( + body: string, + args: string[], + ): Promise<{ + status: number | null; + signal: string | null; + stdout: string; + stderr: string; + }> { + const { spawnSync } = await import('node:child_process'); + const { fileURLToPath } = await import('node:url'); + const src = (rel: string): string => + JSON.stringify(fileURLToPath(new URL(rel, import.meta.url))); + const script = ` + const { withProjectLock } = await import(${src('../src/lib/project-lock.ts')}); + const { fetchRepo } = await import(${src('../src/lib/repo.ts')}); + const { githubArchive } = await import(${src('./support/tar-fixture.ts')}); + const { writeFileSync } = await import('node:fs'); + const { join } = await import('node:path'); + const [dir, sig] = process.argv.slice(1); + ${body} + `; + const r = spawnSync( + process.execPath, + ['--import', 'tsx', '--input-type=module', '-e', script, ...args], + { encoding: 'utf8', timeout: 30_000 }, + ); + return { + status: r.status, + signal: r.signal, + stdout: r.stdout, + stderr: r.stderr, + }; + } + + /** Killed by `sig` — as a signal, or as the shell's 128 + n. */ + function diedBy( + r: { status: number | null; signal: string | null }, + sig: keyof typeof SIGNAL_EXIT, + ): boolean { + return r.signal === sig || r.status === SIGNAL_EXIT[sig]; + } + + it('SIGTERM releases the lock, says so, and still dies by the signal', async () => { + const sig = 'SIGTERM'; + const dir = tmp.path(); + const r = await runChild( + ` + await withProjectLock(dir, 'update', async () => { + process.kill(process.pid, sig); + await new Promise((resolve) => setTimeout(resolve, 3000)); + console.log('SURVIVED-THE-SIGNAL'); + }); + `, + [dir, sig], + ); + expect(r.stdout).not.toContain('SURVIVED-THE-SIGNAL'); + expect(diedBy(r, sig)).toBe(true); + expect(existsSync(lockPath(dir))).toBe(false); + expect(r.stderr).toContain('pharn update was interrupted while writing'); + }, 40_000); + + it('SIGINT after a fetch removes the clone AND releases the lock', async () => { + const dir = tmp.path(); + const sha = 'da39a3ee5e6b4b0d3255bfef95601890afd80709'; + const r = await runChild( + ` + globalThis.fetch = async (url) => + String(url).startsWith('https://api.github.com/') + ? new Response(JSON.stringify({ sha: ${JSON.stringify(sha)} })) + : new Response(new Uint8Array(githubArchive(${JSON.stringify(sha)}))); + await withProjectLock(dir, 'update', async () => { + const repo = await fetchRepo(); + console.log('CLONE=' + repo.dir); + // What @clack/prompts installs while a spinner is up: print, return. + process.on('SIGINT', () => console.log('CLACK-LIKE-LISTENER-RAN')); + process.kill(process.pid, sig); + await new Promise((resolve) => setTimeout(resolve, 3000)); + console.log('SURVIVED-THE-SIGNAL'); + }); + `, + [dir, 'SIGINT'], + ); + const clone = /CLONE=(.+)/.exec(r.stdout)?.[1]?.trim(); + expect(clone, `no CLONE in stdout:\n${r.stdout}\n${r.stderr}`).toBeTruthy(); + expect(r.stdout).not.toContain('SURVIVED-THE-SIGNAL'); + expect(diedBy(r, 'SIGINT')).toBe(true); + expect(existsSync(clone!)).toBe(false); + expect(existsSync(lockPath(dir))).toBe(false); + }, 40_000); + + // After a normal release the cleanup is DEregistered. An empty lock file is + // what another process's lock looks like between its O_EXCL create and its + // payload write — and `release` deletes an unparseable lock — so a cleanup + // left registered would delete a live lock that is not ours. + it('a later signal leaves alone a lock another process is creating', async () => { + const dir = tmp.path(); + const r = await runChild( + ` + await withProjectLock(dir, 'update', () => undefined); + writeFileSync(join(dir, ${JSON.stringify(LOCK_FILE)}), ''); + process.kill(process.pid, sig); + await new Promise((resolve) => setTimeout(resolve, 3000)); + console.log('SURVIVED-THE-SIGNAL'); + `, + [dir, 'SIGTERM'], + ); + expect(r.stdout).not.toContain('SURVIVED-THE-SIGNAL'); + expect(diedBy(r, 'SIGTERM')).toBe(true); + expect(existsSync(lockPath(dir))).toBe(true); + expect(r.stderr).not.toContain('interrupted while writing'); + }, 40_000); }); diff --git a/tests/repo-signals.test.ts b/tests/repo-signals.test.ts index 29877d9..0d70151 100644 --- a/tests/repo-signals.test.ts +++ b/tests/repo-signals.test.ts @@ -78,7 +78,7 @@ describe('temp-clone cleanup handlers', () => { 'fetch', vi.fn(async (url: string) => url === RESOLVE_URL - ? { ok: true, json: async () => ({ sha: VALID_SHA }) } + ? new Response(JSON.stringify({ sha: VALID_SHA })) : tarResponse(githubArchive(VALID_SHA)), ), ); @@ -101,9 +101,23 @@ describe('temp-clone cleanup handlers', () => { } let installed: Array<() => void> = []; + // Each fresh module instance also installs its own fatal-signal handlers + // (lib/fatal-signal.ts); remove them too, so they cannot pile up on the runner. + const SIGNALS = ['SIGINT', 'SIGTERM'] as const; + let signalListeners = new Map(); + beforeEach(() => { + signalListeners = new Map(SIGNALS.map((s) => [s, process.listeners(s)])); + }); afterEach(() => { for (const l of installed) process.removeListener('exit', l); installed = []; + for (const sig of SIGNALS) { + const before = signalListeners.get(sig) ?? []; + for (const l of process.listeners(sig)) { + if (!before.includes(l)) + process.removeListener(sig, l as (...args: unknown[]) => void); + } + } }); it('registers an `exit` handler that removes a live clone', async () => { @@ -153,6 +167,34 @@ describe('temp-clone cleanup handlers', () => { // only by chance, and "only by chance" is not a guard. expect(() => installed[0]!()).not.toThrow(); }); + + // The signal half, in-process (the child-process test below proves the exit + // status; coverage cannot see inside it). The re-raise and the listener sweep + // are stubbed — they would act on the test runner itself. + it('removes a live clone from its fatal-signal handler', async () => { + const before = process.listeners('exit'); + const sigBefore = process.listeners('SIGTERM'); + const { fetchRepo } = await freshRepo(); + const repo = await fetchRepo(); + installed = addedExitListeners(before); + const onSignal = process + .listeners('SIGTERM') + .filter((l) => !sigBefore.includes(l)) as Array<() => void>; + expect(onSignal).toHaveLength(1); + + const kill = vi.spyOn(process, 'kill').mockImplementation(() => true); + const sweep = vi + .spyOn(process, 'removeAllListeners') + .mockImplementation(() => process); + try { + onSignal[0]!(); + expect(existsSync(repo.dir)).toBe(false); + expect(kill).toHaveBeenCalledWith(process.pid, 'SIGTERM'); + } finally { + kill.mockRestore(); + sweep.mockRestore(); + } + }); }); // --------------------------------------------------------------------------- @@ -185,7 +227,7 @@ const SHA = ${JSON.stringify(VALID_SHA)}; const RESOLVE = ${JSON.stringify(RESOLVE_URL)}; globalThis.fetch = (async (url) => { if (String(url) === RESOLVE) { - return { ok: true, json: async () => ({ sha: SHA }) }; + return new Response(JSON.stringify({ sha: SHA })); } const bytes = githubArchive(SHA); return { diff --git a/tests/repo.test.ts b/tests/repo.test.ts index b953ae8..36d8da5 100644 --- a/tests/repo.test.ts +++ b/tests/repo.test.ts @@ -70,6 +70,38 @@ function archive(sha: string): Buffer { ); } +/** + * A real Response carrying `value` as JSON — the shape the commit-SHA resolve + * gets back. Real, not a `{ json }` stand-in: the resolve reads its body through + * its own reader (so it can cancel it), and a stand-in without a stream would + * test a path production never takes. + */ +function jsonResponse(value: unknown, status = 200): Response { + return new Response(JSON.stringify(value), { status }); +} + +/** + * A body stream whose cancellation is OBSERVABLE. "The body was released" must + * be asserted on the cancel itself — a `null` result says nothing about it, + * because the old code returned `null` too, with the body still streaming. + */ +function observedBody( + text: string, + { hang = false } = {}, +): { body: ReadableStream; cancelled: () => boolean } { + let cancelled = false; + const body = new ReadableStream({ + start(c) { + if (text) c.enqueue(new TextEncoder().encode(text)); + if (!hang) c.close(); + }, + cancel() { + cancelled = true; + }, + }); + return { body, cancelled: () => cancelled }; +} + /** A Response-alike whose body streams `bytes` in small chunks. */ function tarResponse(bytes: Buffer, chunkSize = 64): unknown { return { @@ -142,9 +174,7 @@ describe('fetchRepo', () => { // would keep passing if the resolve URL drifted. Comparing the whole // string makes this an assertion about the resolve URL too. if (url === RESOLVE_URL) { - return sha === null - ? { ok: false, json: async () => ({}) } - : { ok: true, json: async () => ({ sha }) }; + return sha === null ? jsonResponse({}, 404) : jsonResponse({ sha }); } return body; }); @@ -230,6 +260,16 @@ describe('fetchRepo', () => { expect(leftoverTempDirs()).toEqual([]); }); + // An error page is a body too, and an unread body holds its socket. The throw + // is harmless today only because every caller exits 1 on it — a property of + // the callers, so the release is made here instead. + it('releases the body of a non-200 download before throwing', async () => { + const page = observedBody('server error', { hang: true }); + stubFetches(VALID_SHA, { ok: false, status: 500, body: page.body }); + await expect(fetchRepo()).rejects.toThrow(/HTTP 500/); + expect(page.cancelled()).toBe(true); + }); + it('removes the temp dir and rethrows when the archive will not extract', async () => { stubFetches(VALID_SHA, tarResponse(Buffer.from('not a gzip stream'))); await expect(fetchRepo()).rejects.toThrow(); @@ -251,18 +291,52 @@ describe('fetchCommitSha', () => { } it('returns the sha on a successful response', async () => { - stubFetch(() => ({ ok: true, json: async () => ({ sha: 'abc123' }) })); + stubFetch(() => jsonResponse({ sha: 'abc123' })); expect(await fetchCommitSha()).toBe('abc123'); }); it('returns null on a non-ok response', async () => { - stubFetch(() => ({ ok: false, json: async () => ({}) })); + stubFetch(() => jsonResponse({}, 403)); expect(await fetchCommitSha()).toBeNull(); }); it('returns null when the sha is not a string', async () => { - stubFetch(() => ({ ok: true, json: async () => ({ sha: 42 }) })); + stubFetch(() => jsonResponse({ sha: 42 })); + expect(await fetchCommitSha()).toBeNull(); + }); + + it('returns null when the body is not JSON', async () => { + stubFetch(() => new Response('rate limited')); + expect(await fetchCommitSha()).toBeNull(); + }); + + // The null is a degraded mode, not an exit: the command carries on and + // finishes. An error body nobody reads kept its socket — and with it the + // whole process — alive until the server finished sending, measured at 30 s + // after `pharn status` had already printed its last line. + it('releases the body of a non-ok response instead of leaving it streaming', async () => { + const page = observedBody('{"message":"API rate limit exceeded"', { + hang: true, + }); + stubFetch(() => ({ ok: false, status: 403, body: page.body })); expect(await fetchCommitSha()).toBeNull(); + expect(page.cancelled()).toBe(true); + }); + + // The 2xx half of the same leak: `res.json()` has no reader pharn can + // cancel, so a body still streaming at the deadline outlived the command. + it('cancels a body still streaming at the 8s deadline', async () => { + vi.useFakeTimers(); + try { + const body = observedBody('{"sha":"', { hang: true }); + stubFetch(() => ({ ok: true, status: 200, body: body.body })); + const pending = fetchCommitSha(); + await vi.advanceTimersByTimeAsync(8000); + expect(await pending).toBeNull(); + expect(body.cancelled()).toBe(true); + } finally { + vi.useRealTimers(); + } }); it('returns null when fetch throws', async () => { @@ -278,7 +352,12 @@ describe('fetchCommitSha', () => { try { stubFetch(() => ({ ok: true, - json: () => new Promise(() => undefined), + status: 200, + // Headers arrived; the body never does, and even a cancel never + // settles — the answer must come from the deadline alone. + body: new ReadableStream({ + cancel: () => new Promise(() => undefined), + }), })); const pending = fetchCommitSha(); await vi.advanceTimersByTimeAsync(8000);