From a415020bad6bc1d194873f18bf77ef2f8fa064a1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 08:51:54 +0000 Subject: [PATCH] fix(fetch): a hard deadline that holds even when undici drops the abort (PHARN-09) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every fetch used an abort-only timer: setTimeout -> controller.abort(). On Node 20/22 (undici 6) the caller's signal is held through a WeakRef, and after one full GC the abort no longer reaches the response body: with a server dripping a byte every 250 ms, a 3 s-capped read was still running at 10 s (reproduced on 22.22.2 and 20.20.2; Node 24 unaffected). undici's bodyTimeout measures the gap between chunks, so init/add/update/status could hang indefinitely — add/update while holding the project lock. New lib/deadline.ts `withDeadline` races the whole fetch-and-read against a timer that aborts AND rejects on its own; downloadArchive, fetchCommitSha and fetchRemoteSkillsVersion use it, and each body reader is cancelled from our own abort listener so the socket is released too. With the same GC repro the new shape rejects at 3.0 s. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01TvcuVhk8hTeDskp5pAJhnc --- .dev/features/fetch-hard-deadline/GRILL.md | 23 ++++ .dev/features/fetch-hard-deadline/PLAN.md | 67 +++++++++++ .../fetch-hard-deadline/REGRESSION.md | 30 +++++ .dev/features/fetch-hard-deadline/REVIEW.md | 40 +++++++ .dev/features/fetch-hard-deadline/SHIP.md | 17 +++ .dev/features/fetch-hard-deadline/VERIFY.md | 27 +++++ .../regression-report.json | 25 +++++ .../fetch-hard-deadline/verify-report.json | 17 +++ .pharn/writes-scope.json | 4 +- src/lib/deadline.ts | 50 +++++++++ src/lib/repo.ts | 105 +++++++++++------- src/lib/skills-version.ts | 87 ++++++++------- tests/deadline.test.ts | 75 +++++++++++++ tests/repo-signals.test.ts | 13 ++- tests/repo.test.ts | 54 ++++++++- tests/skills-version.test.ts | 20 ++++ 16 files changed, 563 insertions(+), 91 deletions(-) create mode 100644 .dev/features/fetch-hard-deadline/GRILL.md create mode 100644 .dev/features/fetch-hard-deadline/PLAN.md create mode 100644 .dev/features/fetch-hard-deadline/REGRESSION.md create mode 100644 .dev/features/fetch-hard-deadline/REVIEW.md create mode 100644 .dev/features/fetch-hard-deadline/SHIP.md create mode 100644 .dev/features/fetch-hard-deadline/VERIFY.md create mode 100644 .dev/features/fetch-hard-deadline/regression-report.json create mode 100644 .dev/features/fetch-hard-deadline/verify-report.json create mode 100644 src/lib/deadline.ts create mode 100644 tests/deadline.test.ts diff --git a/.dev/features/fetch-hard-deadline/GRILL.md b/.dev/features/fetch-hard-deadline/GRILL.md new file mode 100644 index 00000000..6af8364b --- /dev/null +++ b/.dev/features/fetch-hard-deadline/GRILL.md @@ -0,0 +1,23 @@ +# GRILL — fetch-hard-deadline + +Plan: `.dev/features/fetch-hard-deadline/PLAN.md` · spec-hash `bca940a5…d729d3c4e` matches live +`ARCHITECTURE.md`. Registered grillers: `{"registered":0,"grillers":[]}` → inline axes only. + +## Findings + +```yaml +- type: FINDING + rule_id: 'P1' + severity: important + file: '.dev/features/fetch-hard-deadline/PLAN.md:44' + problem: "The real trigger (undici 6 + forced GC on Node 20/22) is not reproduced in-suite; the tests model it as 'the body ignores the abort signal'. That is the right invariant to pin, but the GC reproduction should be re-run out-of-suite on Node 22 as evidence." + evidence: 'a download whose body stream IGNORES the abort signal (the post-GC undici shape)' +- type: FINDING + rule_id: 'P5' + severity: minor + file: '.dev/features/fetch-hard-deadline/PLAN.md:25' + problem: "Existing error messages/tests for the 8 s abort (`/abort/i`, 'Could not reach') must keep matching, or users lose the host name in the timeout error." + evidence: 'the timeout error keeps the existing "Could not reach " shape' +``` + +ADVISORY VERDICT: 2 concerns raised (0 blocking-severity, 2 advisory) — for the human to weigh before /pharn-dev-build. diff --git a/.dev/features/fetch-hard-deadline/PLAN.md b/.dev/features/fetch-hard-deadline/PLAN.md new file mode 100644 index 00000000..5931b85f --- /dev/null +++ b/.dev/features/fetch-hard-deadline/PLAN.md @@ -0,0 +1,67 @@ +# PLAN — fetch-hard-deadline (PHARN-09: the network timeouts must hold even when undici drops the abort) + +- spec_content_hash: bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e # fix #4 +- increment: a small shared `withDeadline(ms, message, work)` in `src/lib/deadline.ts` races the whole + fetch-and-read against a timer that (a) aborts the request's controller and (b) REJECTS on its own, so + the caller is released at the deadline whether or not the abort reaches the body stream; the three + fetch sites (`downloadArchive`, `fetchCommitSha`, `fetchRemoteSkillsVersion`) use it, and the body + readers cancel their stream on abort so the socket is released too. +- layer(s): the CLI itself (`src/lib/deadline.ts` new, `src/lib/repo.ts`, `src/lib/skills-version.ts`) +- constitution_refs: [P0, P1, P2, P3] + +## Discovery — verified this run (P6) + +Reproduced in the review: minimal `downloadArchive` equivalent (`for await` over `res.body`, +`AbortController`, 3 s timer, server dripping 1 byte / 250 ms): no GC → `AbortError` at 3.0 s; one +forced `gc()` → "NOT ABORTED after 10 s" on Node 22.22.2 and 20.20.2 (undici 6.x holds the caller's +signal via a `WeakRef`); Node 24 unaffected. The helper agent reproduced it on the real `fetchRepo` +(still downloading at 75 s with a 60 s cap) and `fetchCommitSha` (no answer after 20 s). Timing- +dependent in the wild → Low. `SECURITY.md` / `THREAT-MODEL.md` promise the timeout covers a streamed +body. The engines floor is `>=20.12.0`, so Node 20/22 are supported targets. + +## Files + +- `src/lib/deadline.ts` — `withDeadline(ms, onTimeout: () => Error, work: (signal) => Promise)`: + one `AbortController`, one timer that aborts AND rejects, `Promise.race`, timer cleared in `finally`, + the losing `work` promise's rejection swallowed (no unhandled rejection) — layer CLI/lib +- `src/lib/repo.ts` — `downloadArchive` and `fetchCommitSha` run under `withDeadline`; the archive body + is read through an explicit reader that is cancelled on abort; `fetchCommitSha` still returns `null` + on timeout (best-effort) — layer CLI/lib +- `src/lib/skills-version.ts` — `fetchRemoteSkillsVersion` under `withDeadline`; the timeout error keeps + the existing "Could not reach " shape — layer CLI/lib +- `tests/deadline.test.ts` — work that never settles is rejected at the deadline with the given error + and the signal is aborted; work that settles first wins and the timer is cleared; a late rejection of + the losing work is not unhandled +- `tests/repo.test.ts` — a download whose body stream IGNORES the abort signal (the post-GC undici + shape) still rejects at `CLONE_TIMEOUT_MS`; `fetchCommitSha` with a never-settling body → `null` +- `tests/repo-signals.test.ts` — its fake download bodies become real web `ReadableStream`s (the shape + `fetch` returns; the download now reads through `getReader()`) +- `tests/skills-version.test.ts` — same "abort not wired to the body" case → rejects at 8 s naming the URL + +## Contracts satisfied + +- `SECURITY.md` / `THREAT-MODEL.md` (8 s / 60 s timeouts cover the streamed body) and CLAUDE.md "Remote + fetches use `redirect: 'error'`, an 8s timeout, and a 256KB body cap" — now independent of whether + the runtime delivers the abort to the stream. + +## Evals to write (P1) + +- listed above; the "abort ignored by the body" cases hang on the base source (vitest timeout = fail). + +## Guarantee audit (P0) + +- "a fetch returns control to pharn within its timeout" → floor: the timer's own `reject` in a + `Promise.race` — it does not depend on undici honouring the signal. +- "the socket is released at the deadline" → best effort (reader.cancel + controller.abort); advisory. + +## Trust audit (P2) + +- No change to what remote bytes are accepted; only when a read is abandoned. + +## Determinism audit (P5) + +- Timer-based; tests use fake timers. + +## Open questions (HALT) + +- none diff --git a/.dev/features/fetch-hard-deadline/REGRESSION.md b/.dev/features/fetch-hard-deadline/REGRESSION.md new file mode 100644 index 00000000..54450f79 --- /dev/null +++ b/.dev/features/fetch-hard-deadline/REGRESSION.md @@ -0,0 +1,30 @@ +# REGRESSION — fetch-hard-deadline + +The verdict below is computed by `.dev/floor/check-regress.mjs`, not by this stage's judgment. + +## Base and partition + +- **base:** `dd165c837e536d8321d4013b5006e95a7de7dbf9` (`origin/main` at build time; the build is an uncommitted working tree on top of it). +- **inside** (each declared in `PLAN.md` `## Files`): `src/lib/deadline.ts`, `src/lib/repo.ts`, `src/lib/skills-version.ts`, `tests/deadline.test.ts`, `tests/repo-signals.test.ts`, `tests/repo.test.ts`, `tests/skills-version.test.ts`. +- **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 stdlib `*.test.mjs` / `*.test.cjs` files + 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. + +## 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/fetch-hard-deadline/REVIEW.md b/.dev/features/fetch-hard-deadline/REVIEW.md new file mode 100644 index 00000000..27a0037d --- /dev/null +++ b/.dev/features/fetch-hard-deadline/REVIEW.md @@ -0,0 +1,40 @@ +# REVIEW — fetch-hard-deadline + +Floor first: `node .dev/floor/validate.mjs .` → exit 0 (GREEN). Everything below is **advisory**. + +## Floor-gate findings (blocking) + +None. + +- **L-floor (P0):** "a fetch returns control within its timeout" reduces to the timer's own `reject` in + a `Promise.race` — independent of whether the runtime delivers the abort to the body stream. +- **L-eval (P1):** `tests/deadline.test.ts` (4 cases) + one "abort never reaches the body" case per + call site (download, commit-SHA resolve, SKILLS_VERSION) — all three HANG on the base source (vitest + 5 s timeout). `repo.test.ts` / `repo-signals.test.ts` fakes now return real web `ReadableStream`s. +- **Out-of-suite evidence (grill #1):** the review's original repro (chunked server dripping every + 250 ms, 3 s cap, `gc()` at 1 s): old shape "NOT ABORTED after 10 s" on Node 22.22.2 and 20.20.2; + the `withDeadline` shape rejects at 3.0 s on both. +- **L-trust (P2):** no change to what remote bytes are accepted; the byte caps are unchanged. +- **L-axis (P3):** `lib/deadline.ts` has one axis (bounding a network operation's duration); the two + fetch modules consume it. + +## Advisory findings + +```yaml +- type: FINDING + rule_id: 'P5' + severity: minor + file: 'src/lib/deadline.ts' + problem: "After the deadline the losing work may keep a socket open until undici's own timeouts if the cancel does not propagate; the CLI's callers exit or continue regardless, so this is a resource note, not a hang." + evidence: 'running.catch(() => undefined);' +- type: FINDING + rule_id: 'P4' + severity: minor + file: 'CHANGELOG.md:8' + problem: "No CHANGELOG `[Unreleased]` entry (not in the plan's `## Files`)." + evidence: '## [Unreleased]' +``` + +## Verdict + +**GREEN** — 0 floor-gate findings, 2 advisory findings. No lesson proposed for canon. diff --git a/.dev/features/fetch-hard-deadline/SHIP.md b/.dev/features/fetch-hard-deadline/SHIP.md new file mode 100644 index 00000000..bcb56694 --- /dev/null +++ b/.dev/features/fetch-hard-deadline/SHIP.md @@ -0,0 +1,17 @@ +# SHIP — fetch-hard-deadline + +Stages run, in order: `/pharn-dev-plan` → GATE 1 (human: **Approve as written**) → `/pharn-dev-grill` → +`/pharn-dev-build` → `/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) +- Run ended at **GATE 2**. The human's standing instruction for this batch: open a PR and merge it only if + its CI checks are green. + +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/fetch-hard-deadline/VERIFY.md b/.dev/features/fetch-hard-deadline/VERIFY.md new file mode 100644 index 00000000..d1812337 --- /dev/null +++ b/.dev/features/fetch-hard-deadline/VERIFY.md @@ -0,0 +1,27 @@ +# VERIFY — fetch-hard-deadline + +## FLOOR layer (owns the verdict) + +Gates run over the whole repo with the feature present, as a non-root user on node 22 with the session +proxy variables unset (as root with the proxy set, 5 pre-existing tests in `init.test.ts` / +`update.test.ts` fail for environmental reasons, identically at the baseline). + +| gate | exit | +| -------------- | ---- | +| `format:check` | 0 | +| `lint` | 0 | +| `lint:md` | 0 | +| `test` | 0 | +| `typecheck` | 0 | +| `validate` | 0 | + +No `structural:*` gate — the increment ships no eval-actual pair. + +**VERDICT: PASS** (`.dev/floor/check-verify.mjs`, `failing_gates: []`). + +## ADVISORY layer + +`node .dev/floor/count-verifiers.mjs .` → `{"registered":0,"verifiers":[]}` — floor gates only. + +Residual (P0/P7): "verified" means the named gates passed — not that the feature is correct in any sense +the suite does not encode. diff --git a/.dev/features/fetch-hard-deadline/regression-report.json b/.dev/features/fetch-hard-deadline/regression-report.json new file mode 100644 index 00000000..af470215 --- /dev/null +++ b/.dev/features/fetch-hard-deadline/regression-report.json @@ -0,0 +1,25 @@ +{ + "base": "dd165c837e536d8321d4013b5006e95a7de7dbf9", + "inside": [ + "src/lib/deadline.ts", + "src/lib/repo.ts", + "src/lib/skills-version.ts", + "tests/deadline.test.ts", + "tests/repo-signals.test.ts", + "tests/repo.test.ts", + "tests/skills-version.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/fetch-hard-deadline/verify-report.json b/.dev/features/fetch-hard-deadline/verify-report.json new file mode 100644 index 00000000..54b18b19 --- /dev/null +++ b/.dev/features/fetch-hard-deadline/verify-report.json @@ -0,0 +1,17 @@ +{ + "feature": "fetch-hard-deadline", + "gates": { + "format:check": 0, + "lint": 0, + "lint:md": 0, + "test": 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 a3d1e848..623222d1 100644 --- a/.pharn/writes-scope.json +++ b/.pharn/writes-scope.json @@ -1,7 +1,7 @@ { "scope": [ - ".dev/features/capability-index-nonfile-md/SHIP.md" + ".dev/features/fetch-hard-deadline/SHIP.md" ], "set_by": ".claude/commands/pharn-dev-ship.md", - "set_at": "2026-09-24T08:43:14.331Z" + "set_at": "2026-09-24T08:51:54.189Z" } diff --git a/src/lib/deadline.ts b/src/lib/deadline.ts new file mode 100644 index 00000000..78956b9f --- /dev/null +++ b/src/lib/deadline.ts @@ -0,0 +1,50 @@ +// --------------------------------------------------------------------------- +// A hard deadline for a network operation — one that does not depend on the +// runtime delivering an abort. +// +// Every fetch here used the same shape: an AbortController, a setTimeout that +// calls `abort()`, and a `finally` that clears it. That shape relies on undici +// forwarding the abort into the RESPONSE BODY stream. On Node 20/22 (undici 6) +// it holds the caller's signal through a WeakRef; measured, after one full GC a +// server dripping one byte every 250 ms kept a 3 s-capped read alive past 10 s +// (Node 24 is unaffected). undici's own bodyTimeout (300 s) measures the gap +// BETWEEN chunks, so a slow drip never trips it: `init`/`add`/`update`/`status` +// could hang indefinitely — `add`/`update` while holding the project lock. +// +// So the timer here does two things: it aborts the controller (the polite path, +// which releases the socket whenever undici honours it) AND rejects on its own +// through a Promise.race, which returns control to pharn at the deadline +// regardless. Callers additionally cancel their body reader on abort (their +// own listener on their own signal — no WeakRef involved). +// +// One axis (P3): bounding how long a network operation may take. +// --------------------------------------------------------------------------- + +/** + * Run `work` with a signal that is aborted after `ms`, and settle no later than + * that: at the deadline the returned promise rejects with `onTimeout()` even if + * `work` never settles. A `work` rejection after the deadline is swallowed — the + * caller has already been answered. + */ +export async function withDeadline( + ms: number, + onTimeout: () => Error, + work: (signal: AbortSignal) => Promise, +): Promise { + const controller = new AbortController(); + let timer: ReturnType | undefined; + const expired = new Promise((_, reject) => { + timer = setTimeout(() => { + controller.abort(); + reject(onTimeout()); + }, ms); + }); + const running = work(controller.signal); + // The loser of the race must never surface as an unhandled rejection. + running.catch(() => undefined); + try { + return await Promise.race([running, expired]); + } finally { + clearTimeout(timer); + } +} diff --git a/src/lib/repo.ts b/src/lib/repo.ts index 30e946f8..fbecf908 100644 --- a/src/lib/repo.ts +++ b/src/lib/repo.ts @@ -1,3 +1,4 @@ +import { withDeadline } from './deadline.js'; import { mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -178,37 +179,53 @@ export async function fetchRepo(): Promise { */ async function downloadArchive(ref: string): Promise { const url = `${CODELOAD}/${REPO}/tar.gz/${ref}`; - const controller = new AbortController(); - const timer = setTimeout(() => controller.abort(), CLONE_TIMEOUT_MS); - try { - const res = await fetch(url, { - redirect: 'error', - signal: controller.signal, - }); - if (!res.ok) { - throw new Error(`Failed to download ${url}: HTTP ${res.status}`); - } - if (!res.body) { - throw new Error(`Failed to download ${url}: empty response body`); - } - // codeload sends no content-length, so the cap is a running count over the - // stream rather than a header check. - const chunks: Buffer[] = []; - let total = 0; - for await (const chunk of res.body) { - const buf = Buffer.from(chunk as Uint8Array); - total += buf.byteLength; - if (total > MAX_ARCHIVE_BYTES) { + // withDeadline, not a bare abort timer: on Node 20/22 the abort may never + // reach the body stream (lib/deadline.ts), and this read is multi-megabyte. + return withDeadline( + CLONE_TIMEOUT_MS, + () => + new Error( + `Timed out downloading ${url} after ${CLONE_TIMEOUT_MS / 1000}s.`, + ), + async (signal) => { + const res = await fetch(url, { redirect: 'error', signal }); + if (!res.ok) { + throw new Error(`Failed to download ${url}: HTTP ${res.status}`); + } + if (!res.body) { + throw new Error(`Failed to download ${url}: empty response body`); + } + // An explicit reader, cancelled by OUR listener on OUR signal, so the + // socket is released at the deadline even when undici drops the abort. + const reader = res.body.getReader(); + signal.addEventListener('abort', () => { + reader.cancel().catch(() => undefined); + }); + // codeload sends no content-length, so the cap is a running count over + // the stream rather than a header check. + const chunks: Buffer[] = []; + let total = 0; + for (;;) { + const { done, value } = await reader.read(); + if (done) break; + const buf = Buffer.from(value); + total += buf.byteLength; + if (total > MAX_ARCHIVE_BYTES) { + await reader.cancel().catch(() => undefined); + throw new Error( + `Refusing ${url}: archive exceeds ${MAX_ARCHIVE_BYTES} bytes.`, + ); + } + chunks.push(buf); + } + if (signal.aborted) { throw new Error( - `Refusing ${url}: archive exceeds ${MAX_ARCHIVE_BYTES} bytes.`, + `Timed out downloading ${url} after ${CLONE_TIMEOUT_MS / 1000}s.`, ); } - chunks.push(buf); - } - return Buffer.concat(chunks); - } finally { - clearTimeout(timer); - } + return Buffer.concat(chunks); + }, + ); } /** @@ -221,23 +238,27 @@ async function downloadArchive(ref: string): Promise { */ export async function fetchCommitSha(): Promise { const url = `${API}/repos/${REPO}/commits/${REPO_BRANCH}`; - const controller = new AbortController(); - const timer = setTimeout(() => controller.abort(), FETCH_TIMEOUT_MS); try { - const res = await fetch(url, { - redirect: 'error', - signal: controller.signal, - headers: { - Accept: 'application/vnd.github+json', - 'X-GitHub-Api-Version': '2022-11-28', + // Same hard deadline as the download (lib/deadline.ts); a timeout is just + // one more failure that degrades to `null`. + return await withDeadline( + FETCH_TIMEOUT_MS, + () => new Error(`Timed out resolving ${url}`), + async (signal) => { + const res = await fetch(url, { + redirect: 'error', + signal, + headers: { + Accept: 'application/vnd.github+json', + 'X-GitHub-Api-Version': '2022-11-28', + }, + }); + if (!res.ok) return null; + const body = (await res.json()) as { sha?: unknown }; + return typeof body.sha === 'string' ? body.sha : null; }, - }); - if (!res.ok) return null; - const body = (await res.json()) as { sha?: unknown }; - return typeof body.sha === 'string' ? body.sha : null; + ); } catch { return null; - } finally { - clearTimeout(timer); } } diff --git a/src/lib/skills-version.ts b/src/lib/skills-version.ts index 60c70135..30eaeb97 100644 --- a/src/lib/skills-version.ts +++ b/src/lib/skills-version.ts @@ -1,3 +1,4 @@ +import { withDeadline } from './deadline.js'; import { closeSync, existsSync, @@ -193,46 +194,50 @@ function rethrowUnreachable(url: string): (err: unknown) => never { export async function fetchRemoteSkillsVersion(): Promise { const url = `${RAW}/${REPO}/${REPO_BRANCH}/${SKILLS_VERSION_FILE}`; const unreachable = rethrowUnreachable(url); - const controller = new AbortController(); - const timer = setTimeout(() => controller.abort(), FETCH_TIMEOUT_MS); - // ONE try around the fetch AND the body read — the timer shape `fetchCommitSha` - // already uses (`lib/repo.ts`). `fetch()` resolves as soon as HEADERS arrive, so - // a `finally` that closes before the body is consumed disarms the abort exactly - // where it is needed: a server that dribbles bytes then runs until undici's - // 300s inter-chunk bodyTimeout with no pharn timer armed at all. + // ONE deadline around the fetch AND the body read. `fetch()` resolves as soon + // as HEADERS arrive, so a timer that stops before the body is consumed is + // disarmed exactly where it is needed. withDeadline (lib/deadline.ts) also + // rejects on its own at the deadline, because on Node 20/22 the abort may + // never reach the body stream after a GC. The timeout keeps the + // "Could not reach " shape every other transport failure here has. // - // Only the SHAPE is mirrored. `fetchCommitSha` swallows every failure to `null` - // (best-effort provenance, LIMITS.md §1b/§3b); this function must keep throwing. - try { - // `.catch` on the EXPRESSION, not a `try` around the block: the wrap must - // cover this rejection and nothing thrown after it resolves, or the three - // deliberate throws below (non-ok status, the two cap refusals, - // assertSafeString) get re-labelled as transport failures. - const res = await fetch(url, { - redirect: 'error', - signal: controller.signal, - }).catch(unreachable); - if (!res.ok) { - throw new Error( - `SKILLS_VERSION fetch failed (${res.status}) from ${url}`, - ); - } - // ADVISORY (P0), and backstopped below — never the guard itself. - // `content-length` is the remote's own claim: a chunked response omits it - // entirely (`Number(null)` is `0`, which sails through this compare), and a - // hostile server is free to declare a small lie. It buys exactly one thing — - // an HONESTLY declared oversize is refused before a single byte is read. - const declared = Number(res.headers.get('content-length')); - if (Number.isFinite(declared) && declared > MAX_BODY_BYTES) { - throw new Error( - `SKILLS_VERSION too large (${declared} bytes) from ${url}`, + // Only the SHAPE is shared with `fetchCommitSha` (lib/repo.ts), which swallows + // every failure to `null` (best-effort provenance, LIMITS.md §1b/§3b); this + // function must keep throwing. + return withDeadline( + FETCH_TIMEOUT_MS, + () => + new Error( + `Could not reach ${url}: This operation was aborted (timed out after ${FETCH_TIMEOUT_MS / 1000}s)`, + ), + async (signal) => { + // `.catch` on the EXPRESSION, not a `try` around the block: the wrap must + // cover this rejection and nothing thrown after it resolves, or the three + // deliberate throws below (non-ok status, the two cap refusals, + // assertSafeString) get re-labelled as transport failures. + const res = await fetch(url, { redirect: 'error', signal }).catch( + unreachable, ); - } - const text = await readCappedBody(res, url, unreachable); - return assertSafeString(text.trim(), SKILLS_VERSION_FILE, VERSION_RE); - } finally { - clearTimeout(timer); - } + if (!res.ok) { + throw new Error( + `SKILLS_VERSION fetch failed (${res.status}) from ${url}`, + ); + } + // ADVISORY (P0), and backstopped below — never the guard itself. + // `content-length` is the remote's own claim: a chunked response omits it + // entirely (`Number(null)` is `0`, which sails through this compare), and + // a hostile server is free to declare a small lie. It buys exactly one + // thing — an HONESTLY declared oversize is refused before a byte is read. + const declared = Number(res.headers.get('content-length')); + if (Number.isFinite(declared) && declared > MAX_BODY_BYTES) { + throw new Error( + `SKILLS_VERSION too large (${declared} bytes) from ${url}`, + ); + } + const text = await readCappedBody(res, url, unreachable, signal); + return assertSafeString(text.trim(), SKILLS_VERSION_FILE, VERSION_RE); + }, + ); } /** @@ -252,11 +257,17 @@ async function readCappedBody( res: Response, url: string, unreachable: (err: unknown) => never, + signal: AbortSignal, ): Promise { // A bodyless response (a 204, or `new Response(null)`) reads as empty text and // then fails VERSION_RE downstream — the same outcome `res.text()` produced. if (!res.body) return ''; const reader = res.body.getReader(); + // Our own listener on our own signal: at the deadline the stream is cancelled + // (socket released) even when undici does not forward the abort. + signal.addEventListener('abort', () => { + reader.cancel().catch(() => undefined); + }); const chunks: Uint8Array[] = []; let total = 0; // FABLE 4.6: the wrap covers this read, not just the fetch call — since the diff --git a/tests/deadline.test.ts b/tests/deadline.test.ts new file mode 100644 index 00000000..7678dabd --- /dev/null +++ b/tests/deadline.test.ts @@ -0,0 +1,75 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { withDeadline } from '../src/lib/deadline.js'; + +// PHARN-09: on Node 20/22 undici can lose the abort after a GC, so a timeout +// that only CALLS abort() never fires into the body read. withDeadline must +// answer at the deadline on its own. +describe('withDeadline', () => { + beforeEach(() => vi.useFakeTimers()); + afterEach(() => vi.useRealTimers()); + + it('rejects at the deadline even when the work never settles, and aborts its signal', async () => { + let seen: AbortSignal | undefined; + const pending = withDeadline( + 1000, + () => new Error('too slow'), + (signal) => { + seen = signal; + return new Promise(() => undefined); // ignores the abort entirely + }, + ); + const rejects = expect(pending).rejects.toThrow('too slow'); + await vi.advanceTimersByTimeAsync(999); + expect(seen!.aborted).toBe(false); + await vi.advanceTimersByTimeAsync(1); + await rejects; + expect(seen!.aborted).toBe(true); + }); + + it('returns the work result when it settles first, and clears the timer', async () => { + const value = await withDeadline( + 1000, + () => new Error('too slow'), + async () => 42, + ); + expect(value).toBe(42); + expect(vi.getTimerCount()).toBe(0); + }); + + it('propagates the work rejection when it settles first', async () => { + await expect( + withDeadline( + 1000, + () => new Error('too slow'), + async () => { + throw new Error('boom'); + }, + ), + ).rejects.toThrow('boom'); + }); + + it('a late rejection of the losing work is not an unhandled rejection', async () => { + const unhandled = vi.fn(); + process.on('unhandledRejection', unhandled); + try { + let fail!: (e: Error) => void; + const pending = withDeadline( + 10, + () => new Error('too slow'), + () => + new Promise((_, reject) => { + fail = reject; + }), + ); + const rejects = expect(pending).rejects.toThrow('too slow'); + await vi.advanceTimersByTimeAsync(10); + await rejects; + fail(new Error('late')); + await vi.advanceTimersByTimeAsync(0); + await Promise.resolve(); + expect(unhandled).not.toHaveBeenCalled(); + } finally { + process.off('unhandledRejection', unhandled); + } + }); +}); diff --git a/tests/repo-signals.test.ts b/tests/repo-signals.test.ts index daba92d6..29877d9c 100644 --- a/tests/repo-signals.test.ts +++ b/tests/repo-signals.test.ts @@ -50,9 +50,14 @@ function tarResponse(bytes: Buffer): unknown { return { ok: true, status: 200, - body: (async function* () { - yield bytes; - })(), + // A web ReadableStream — the shape `fetch` returns; the download reads it + // through getReader(). + body: new ReadableStream({ + start(c) { + c.enqueue(new Uint8Array(bytes)); + c.close(); + }, + }), }; } @@ -186,7 +191,7 @@ globalThis.fetch = (async (url) => { return { ok: true, status: 200, - body: (async function* () { yield bytes; })(), + body: new ReadableStream({ start(c) { c.enqueue(new Uint8Array(bytes)); c.close(); } }), }; }) as unknown as typeof fetch; diff --git a/tests/repo.test.ts b/tests/repo.test.ts index 7accc1ba..b953ae85 100644 --- a/tests/repo.test.ts +++ b/tests/repo.test.ts @@ -75,11 +75,15 @@ function tarResponse(bytes: Buffer, chunkSize = 64): unknown { return { ok: true, status: 200, - body: (async function* () { - for (let i = 0; i < bytes.length; i += chunkSize) { - yield bytes.subarray(i, i + chunkSize); - } - })(), + // A web ReadableStream, the shape `fetch` really returns — the download is + // read through `getReader()` so it can be cancelled on its own signal. + body: ReadableStream.from( + (async function* () { + for (let i = 0; i < bytes.length; i += chunkSize) { + yield new Uint8Array(bytes.subarray(i, i + chunkSize)); + } + })(), + ), }; } @@ -148,6 +152,30 @@ describe('fetchRepo', () => { return mock; } + // PHARN-09: a body stream the abort NEVER reaches — the shape undici 6 (Node + // 20/22) leaves after a GC drops its WeakRef to the caller's signal. The old + // abort-only timer then left the download hanging indefinitely. + it('gives up at CLONE_TIMEOUT_MS even when the abort never reaches the body', async () => { + vi.useFakeTimers(); + try { + stubFetches(VALID_SHA, { + ok: true, + status: 200, + body: new ReadableStream({ + start(c) { + c.enqueue(new Uint8Array([0x1f])); // one byte, then silence + }, + }), + }); + const pending = fetchRepo(); + const rejects = expect(pending).rejects.toThrow(/Timed out downloading/); + await vi.advanceTimersByTimeAsync(60_000); + await rejects; + } finally { + vi.useRealTimers(); + } + }); + it('downloads the tarball at the resolved SHA and records it (recorded == fetched)', async () => { const mock = stubFetches(VALID_SHA); const repo = await fetchRepo(); @@ -243,4 +271,20 @@ describe('fetchCommitSha', () => { }); expect(await fetchCommitSha()).toBeNull(); }); + + // PHARN-09: headers arrived, the JSON body never does and ignores the abort. + it('returns null at the 8s deadline even when the abort never reaches the body', async () => { + vi.useFakeTimers(); + try { + stubFetch(() => ({ + ok: true, + json: () => new Promise(() => undefined), + })); + const pending = fetchCommitSha(); + await vi.advanceTimersByTimeAsync(8000); + expect(await pending).toBeNull(); + } finally { + vi.useRealTimers(); + } + }); }); diff --git a/tests/skills-version.test.ts b/tests/skills-version.test.ts index fd55a9f3..ab71a5dd 100644 --- a/tests/skills-version.test.ts +++ b/tests/skills-version.test.ts @@ -269,6 +269,26 @@ describe('fetchRemoteSkillsVersion', () => { } }); + // PHARN-09: the same read, but the abort is NOT wired into the body — what + // undici 6 (Node 20/22) leaves after a GC drops its WeakRef to the signal. + // The deadline must still answer, naming the host. + it('rejects at 8s even when the abort never reaches the body stream', async () => { + vi.useFakeTimers(); + try { + vi.spyOn(globalThis, 'fetch').mockImplementation( + async () => new Response(new ReadableStream({})), + ); + const pending = fetchRemoteSkillsVersion(); + const rejects = expect(pending).rejects.toThrow( + /Could not reach .*SKILLS_VERSION.*abort/i, + ); + await vi.advanceTimersByTimeAsync(8000); + await rejects; + } finally { + vi.useRealTimers(); + } + }); + // ------------------------------------------------------------------------- // FABLE 4.6 - transport failures name the host they could not reach. //