Skip to content

fix: bound every outbound fetch with a timeout (R1.2) - #98

Merged
stormer78 merged 2 commits into
mainfrom
fix/bound-every-outbound-fetch
Jul 19, 2026
Merged

fix: bound every outbound fetch with a timeout (R1.2)#98
stormer78 merged 2 commits into
mainfrom
fix/bound-every-outbound-fetch

Conversation

@stormer78

Copy link
Copy Markdown
Contributor

Second of the audit findings. CLAUDE.md names handleApiGet/handleApiPost as the unbounded fetch that hangs a page against a blackholed VTA. That one was fixed in #88 and is now the single compliant call site in the repo. Nine others never were.

Why they were missed

A literal grep "fetch(" finds one call site repo-wide and would report R1.2 as satisfied. This package injects fetch for testability:

const f = opts.fetch ?? fetch.bind(globalThis);

so the real calls are spelled f(...), fetchFn(...), this.fetchImpl(...) — and none of the six files containing them mentioned signal, AbortSignal, or timeout.

An unbounded fetch against a wedged peer doesn't fail slowly, it doesn't fail at all: the page waits on a promise that never settles, and in MV3 the unresolved await also pins the service worker awake and stacks later requests behind it. Nothing surfaces an error, so nothing reports it.

Sites bounded

file what
vault/transport.ts ×2 /auth/challenge + /auth/ — the bearer handshake
vta/rest-channel.ts the Trust-Task REST dispatcher
vta/client.ts PWA passkey enrolment
rp-login/step-up.ts ×2 VTA step-up start/finish
siop/login-client.ts ×2 SIOP challenge/authenticate
device/register-gateway.ts push registration

Approach

withFetchTimeout is applied at each injection point, not each call site. That is what makes coverage complete, and it matters most for getVtaBearer — it runs before every REST request and is reached only indirectly through VtaAuthInputs, so leaving it unbounded would have defeated a timeout added anywhere downstream.

A caller-supplied signal is combined with the deadline, not replaced — silently disabling a caller's cancellation would be a worse bug than the one being fixed. Where AbortSignal.any is unavailable the timeout survives: losing the combination is recoverable, losing the deadline is the defect.

Adds e.client.timeout so "the VTA never answered" is distinguishable from "the connection failed" via a stable code rather than message text (R3.7). isFetchTimeout matches DOMException.name — the platform's own machine-readable discriminant.

Verification

10 new tests: deadline fires on a blackholed peer, fast responses untouched, caller's abort still wins and isn't mistaken for a timeout, deadline survives a non-aborting caller signal, init fields preserved, global-fetch fallback bounded — plus a wiring test asserting a real exported path (registerPushChannel) actually hands a signal to its injected fetch. The helper being correct is not the same as it being applied.

Mutation-tested: with the timeout removed the suite hangs rather than failing — which is exactly the production symptom, and the clearest possible demonstration that the tests bind the behaviour.

Full suite 190/190 (was 180), clean build and lint.

Not verified: no run against a real wedged VTA in a loaded extension. The 20s default matches the extension proxy but has not been tuned against real VTA latency — if legitimate slow operations exist that exceed it, this converts a hang into a spurious failure. Worth a look before merge.

Remaining audit findings

  1. register-gateway.ts:71-75 throws on status before reading the body, on a consent-capable endpoint (R3.7)
  2. rest-channel.ts:98 consumes the body, so errorFromResponse re-reads a used stream and always loses the server's code — proven with a repro (R3.7)
  3. Persist-before-ack violated structurally in vti-didcomm-js — cross-repo (R1.6/R4.1)

Pre-merge checklist

  • All clients have finite timeouts (R1.2) — this is the fix; every injection point now wrapped
  • No lock held across a network await (R1.3) — n/a
  • No local state committed before its remote effect (R2.1) — n/a
  • Every retry bounded + backed off (R1.4) — the rest-channel 401 retry is unchanged and now bounded on both attempts
  • Accept/poll/listen loops survive transient errors (R1.5) — n/a
  • Acks/deletes only after durable handoff (R1.6) — untouched; see remaining finding 3
  • New/changed wire types (R3.*) — e.client.timeout is a client-side code, not a wire type; no consumer switches exhaustively on the union
  • Config absence = most restrictive (R5.*) — no injected fetch still yields a bounded one; the default applies unless overridden
  • Logs/status claim only what was verified (R6.*) — a timeout now reports as a timeout instead of hanging silently
  • "Process dies on the next line" answered (R2.1) — no mutation semantics changed
  • Deviations flagged with rule numbers — none

CLAUDE.md names `handleApiGet`/`handleApiPost` as the unbounded fetch that
hangs a page against a blackholed VTA. That one was fixed in #88 and is now
the one call site in the repo that complies. Nine others never were.

They hid from a naive audit: a literal `grep "fetch("` finds a single call
site repo-wide. This package injects `fetch` for testability —

    const f = opts.fetch ?? fetch.bind(globalThis);

— so the real calls are spelled `f(...)`, `fetchFn(...)`,
`this.fetchImpl(...)`, and none of the six files containing them mentioned
`signal`, `AbortSignal` or `timeout`.

An unbounded fetch against a wedged peer does not fail slowly; it does not
fail at all. The page waits on a promise that never settles, and in MV3 the
unresolved `await` also pins the service worker awake and stacks later
requests behind it. Nothing surfaces an error, so nothing reports it.

Adds `withFetchTimeout`, applied at each INJECTION point rather than each call
site — that is what makes the coverage complete, and it matters most for
`getVtaBearer`, which runs before every REST request and is reached only
indirectly through `VtaAuthInputs`. Leaving it unbounded would have defeated a
timeout added anywhere downstream.

A caller-supplied `signal` is combined with the deadline rather than replaced:
silently disabling a caller's cancellation would be a worse bug than the one
being fixed. Where `AbortSignal.any` is unavailable the timeout survives, on
the principle that losing the combination is recoverable and losing the
deadline is the defect.

Adds `e.client.timeout` so "the VTA never answered" is distinguishable from
"the connection failed" by a stable code rather than message text (R3.7).
`isFetchTimeout` matches `DOMException.name`, the platform's own
machine-readable discriminant.

Sites bounded: vault/transport.ts (x2, the bearer handshake), vta/rest-channel,
vta/client, rp-login/step-up (x2), siop/login-client (x2),
device/register-gateway.

Verified: 10 new tests — deadline fires on a blackholed peer, fast responses
untouched, caller's abort still wins, timeout survives a non-aborting caller
signal, init fields preserved, global-fetch fallback bounded, plus a wiring
test asserting a real exported call path (`registerPushChannel`) hands a
signal to its injected fetch. Mutation-tested: with the timeout removed the
suite HANGS rather than failing, which is precisely the production symptom.
Full suite 190/190, clean build and lint.

Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
The new timeout tests passed locally and failed on CI with every test in the
file reported as "Promise resolution is still pending but the event loop has
already resolved".

Node unrefs the timer behind `AbortSignal.timeout`, so a pending abort does
not hold the event loop open. The blackhole stub's promise had nothing else
queued, so on CI the process drained and exited before the deadline fired.
Locally it survived only because other work happened to keep the loop alive —
the tests were depending on an accident.

A ref'd interval, cleared when the abort lands, removes the dependency.

Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
@stormer78
stormer78 merged commit c76cf11 into main Jul 19, 2026
3 checks passed
@stormer78
stormer78 deleted the fix/bound-every-outbound-fetch branch July 19, 2026 05:04
stormer78 added a commit that referenced this pull request Jul 19, 2026
CLAUDE.md was untracked, so every contributor had their own copy or none.
Checks it in, and corrects the parts that had gone stale — its named defects
were the ones most likely to be trusted verbatim, and all three were fixed:

- R3.7's `consentRequiredFrom` example was fixed (it now matches
  `details.reason`). Replaced with the rule that actually bites now: a
  Response body reads once, so an already-parsed body must use
  `errorFromBody`, not a re-read of the spent Response (#99).
- R1.5's "one 2s retry from onClose" applied to the worker inbound path
  (fixed in #88) and then to the approver inbox (fixed in #97). Replaced with
  the invariant — cap the delay not the attempt count, re-arm on every
  failure including first-connect — and a pointer to `ReconnectScheduler`.
- R1.2's `handleApiGet`/`handleApiPost` example was fixed in #88 and is now
  the compliant reference. Replaced with the thing that actually hides these:
  fetch is injected, so `grep "fetch("` finds almost nothing and the timeout
  belongs at the injection point (#98).

Promotes the one genuinely open defect to its own section: R1.6
persist-before-ack, which is not fixable from this repo — vti-didcomm-js acks
before dispatching to `onMessage`, and the wallet persists only the message
id, so an offscreen teardown mid-prompt loses a task-consent request for good.

Adds a repo-mechanics section for the traps that cost time this week: build
`core` before typechecking dependents, lint is `tsc -b` (never `-b --noEmit`,
TS6310), cross-workspace imports need a `references` entry, what CI asserts,
stub with real `Response` objects, and Node unreffing the `AbortSignal.timeout`
timer (passes locally, fails in CI).

Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant