diff --git a/.dev/features/fetch-hard-deadline/GRILL.md b/.dev/features/fetch-hard-deadline/GRILL.md new file mode 100644 index 0000000..6af8364 --- /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 0000000..5931b85 --- /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 0000000..54450f7 --- /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 0000000..27a0037 --- /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 0000000..bcb5669 --- /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 0000000..d181233 --- /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 0000000..af47021 --- /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 0000000..54b18b1 --- /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 a3d1e84..623222d 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 0000000..78956b9 --- /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 30e946f..fbecf90 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 60c7013..30eaeb9 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 0000000..7678dab --- /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 daba92d..29877d9 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 7accc1b..b953ae8 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 fd55a9f..ab71a5d 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. //