feat(download): @stitchapi/download — batch downloader, plus the core fixes it surfaced - #639
Open
rejifald wants to merge 12 commits into
Open
feat(download): @stitchapi/download — batch downloader, plus the core fixes it surfaced#639rejifald wants to merge 12 commits into
rejifald wants to merge 12 commits into
Conversation
The `download` surface's `interpret` accepted any sub-400 status as a complete Blob, so a stray `206 Partial Content` (a range-serving proxy/CDN answering a request that never sent a Range) was handed back as if it were the whole file. `interpret` now accepts only a complete body (200/204) and rejects everything else. Adds a controllable download test rig (extending the zero-dep node:http mock server) that reproduces real-world server/network behavior and pins the download surface against it: - byte accounting: stray-206 rejection; Content-Length-mismatch truncation (undici hangs on a clean FIN, cured by `timeout`) - network faults: mid-body RST, idle stall, slow TTFB, steady throttle - redirects: cross-origin credential strip proven end-to-end through download() (the signed-URL -> CDN pattern), same-origin no-over-strip, redirect-loop -> interpret-reject, retry x redirect No engine change; the rig is test-only. Two findings recorded in the specs for the future @stitchapi/download package: the engine has no idle/forward-progress timeout (a healthy-but-slow download dies like a stall), and the transport `.cause` (UND_ERR_SOCKET) is stripped before callers see it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ost-pooling
Milestone 4 of the download test rig — pin the substrate the future
@stitchapi/download batch downloader stands on: per-request concurrency,
host-pooled budgets, partial-failure isolation, multi-host fairness, and
stall isolation. All test-only; no src/ change.
- MockServer concurrency probe: maxOpen/openNow/arrivals(path?) — the count of
requests SIMULTANEOUSLY open on the wire (high-water mark), marked at handler
entry and released on res 'close', so a test asserts ACTUAL wire overlap.
- 5 download-concurrency-*.spec.ts, all asserting overlap COUNT (a hard limiter
invariant), never a timing gap:
ceiling — <= k requests open at once under a shared host throttle
pool — pool:'host' shares one budget vs pool:'stitch' independent
partial-failure — allSettled: 200 + 404 + RST each settle on their own
multihost — two hosts = two budgets; a saturated host can't stall the other
stall-isolation — one stalled item times out alone; siblings' bytes intact
- Root-cause + fix the flaky throttle-host-pooling spec: it measured a
server-arrival wall-clock gap (conflated the 500ms rate spacing with first-fetch
cold-start / event-loop lag). Rewritten on mockAdapter + a shared manualClock,
proving pooling by virtual-time gating — deterministic, zero wall-clock.
Deferred to the unbuilt batch orchestrator (each noted in a // Deferred: comment):
FIFO admission order, cancel one/all, aggregate progress/ETA, same-URL dedupe.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ing, size, range (M5)
Completes the download test rig beyond M4: the connection-level faults, status/retry
semantics, content-encoding, size boundaries, and Range scaffolding a downloader must
survive. All test-only; no src/ change.
Infra:
- test/support/hostile-net.ts — raw net.Server escape hatch (unusedPort, startRawServer)
for faults node:http normalizes away (ECONNREFUSED, immediate FIN, accept-then-silence,
broken TLS handshake).
- mock-server.ts — contentEncoding (gzip/br via node:zlib), etag/lastModified/acceptRanges
validators, forceStatusOnRange (416 / 200-ignore-Range). Plus a fix: reset() now destroys
ONLY sockets with an in-flight response, leaving idle keep-alive sockets alive — destroying
them made undici reuse a dead connection on the next test's first request (the N12
stale-keep-alive "fetch failed"), a latent flake across every multi-test spec.
Specs (real-timer, loose bounds; manualClock for the retry-after delta):
- download-conn-{dns,refused,hangup,no-response,tls} — N7–N11: DNS surfaces generically with
no host leak; ECONNREFUSED is prompt; a FIN / no-response is cut by the timeout; a TLS
handshake failure rejects cleanly.
- download-retry-{transient,after,policy} — X9–X11: a 503 is retried and the WHOLE file
re-downloaded (no Range — buffered surface has no resume); a 429 Retry-After waits the exact
delta (manualClock); 500 and 408 are NOT retried by default, opt-in via retry.on restores it.
- download-{content-encoding,size-boundaries,html-error-page,range-416} — B7/B8 gzip+br
"progress lies" (loaded > compressed total, body intact); Z1–Z3 + 204 boundaries; Z4 a 200
HTML error page returns as a Blob (caller must check); R4 a 416 is rejected.
Deferred (noted per spec) to the future @stitchapi/download package / resume feature:
client-side Range/If-Range resume, plus the M4 batch-orchestrator properties.
Verified: check:types 0, full core suite 1248 green, download+throttle 5x green
(28 files / 35 tests), build+size unchanged (24.58/19.80 — subpath-only), eslint+prettier clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…sees (#450) When a transport call fails (undici `TypeError: fetch failed` whose `.cause` is a `SocketError { code: 'UND_ERR_SOCKET' }`, a DNS/connect failure, an AbortError, …), the engine reduced it to its top-level MESSAGE on the error event and rebuilt a `StitchError('fetch failed', …)` with NO `.cause` — so a caller (both the awaited and `.safe()` paths) saw only the generic message and could not tell a socket reset from any other transport failure (`err.cause === undefined`). Surfaced by the download rig's download-reset-midbody spec. Fix, entirely on the existing non-enumerable ERROR_SOURCE channel: - engine.ts `errEvt`: pin the live error for a bare transport/internal error too (not just RateLimitError / HTTP `.response` failures). A thrown `StitchError` stays excluded, so it keeps being rebuilt from the event (unchanged behaviour). - stitch.ts `rebuildError`: re-surface RateLimitError / contract-violation StitchError UNCHANGED (as before), and otherwise build a StitchError carrying the pinned transport error as `cause`. `cause` is non-enumerable, so it never leaks into a trace sink — only the enumerable `status`/`message` do. This is what lets @stitchapi/download's classifier read `err.cause`/`.code` to distinguish an RST from a generic "fetch failed". download-reset-midbody.spec.ts turns from pinning the gap to pinning the fix: it now asserts `err.cause` is defined and `UND_ERR_SOCKET` is reachable through the cause chain, on both the throwing and `.safe()` paths. Verified: core typecheck 0; full core suite 1248/1248 green; bundle 24.61/19.84 within the 24.80/20.00 budget. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
… ETA, idle-timeout, dedupe) (#449) * feat(download): @stitchapi/download — batch downloader (FIFO, cancel, ETA, idle-timeout, dedupe) A new zero-runtime-dep, browser-first workspace package that sequences a LIST of downloads on top of core's buffered `download()` surface — the niceties a single `download()` call can't express, which the download rig (PRs #447/#448) pinned as DEFERRED. `downloadAll(items, opts)` (one-shot, awaitable + controllable) and the stateful `DownloadManager` it's built on. Its own FIFO concurrency scheduler over `download()` adds, and proves against the rig: - FIFO admission order — queued items start in enqueue order as slots free (P6/P14). - Per-item settling — the batch promise NEVER rejects; one 404/RST never fails a sibling (P4). - Cancellation — cancel-one (slot returns to the queue), cancel-queued (frees no slot, no skip), cancel-all (in-flight abort, queue drains, pool:'host' left clean) (P10-P13). - Aggregate progress + ETA across N concurrent streams under a known byte schedule, driven by the injected Clock (manualClock) so the math is deterministic; a stalled sibling never distorts it (P8/P9/P17). - Same-URL dedupe — independent fetches by default, opt-in `dedupe:true` collapses onto one in-flight request (P18). Solves the two findings the rig surfaced, in the package (core untouched): - Idle / forward-progress timeout that resets on each onProgress chunk — a slow-but-alive stream survives while a dead stall is cut (retryable IDLE_TIMEOUT). The engine's wall-clock timeout cannot make this distinction. - Error classification — captures the RAW transport error via a per-item hooks.onError seam (the engine drops the transport .cause before the caller sees it) and reports retryable + a machine code (UND_ERR_SOCKET / HTTP_404 / IDLE_TIMEOUT). Proven by 7 test/gaps/download-batch-*.spec.ts (15 tests) that re-export the core rig (mock-server + hostile-net) and mirror the existing download-* patterns exactly: assert on-wire overlap via the server's maxOpen probe, manualClock for ETA math, real timers with loose bounds for socket tests. Zero runtime deps; own bundle-size gate (2.46 KB gzip / 2.65 KB budget); core bundle unchanged at 24.58/19.80. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(download): register @stitchapi/download in the README package tables (fix drift) The `drift-executable` CI job (yakir `readme-packages` + `core-integrations` tethers) flagged the new package: `gen-readme-metrics.mjs` auto-discovers every non-private workspace package, so adding `@stitchapi/download` changed the generator's emit while the committed README regions stayed stale — a two-site conflict. Register the package (a group + a curated one-liner, per the generator's own convention) and regenerate: - scripts/gen-readme-metrics.mjs: add `download` to GROUP_BY_DIR ('surface', alongside shell) and TABLE_DESCRIPTIONS. - README.md + packages/core/README.md: regenerated regions now list @stitchapi/download under "Surfaces". Verified: `yakir check --tier executable` AND `--tier token` both exit 0 (5 ok / 3 ok, 0 conflict); the 3 changed files are prettier-clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
… (unblocks lint)
The repo's `no-restricted-syntax` rule forbids `...(x !== undefined ? { k: x } : {})`
in favour of `compact({ ...obj, k: x })` — my cause-carry change tripped it at
stitch.ts, failing core `check:lint` and blocking the `verify` job.
- core rebuildError: `compact({ status, attempts, cause })`.
- @stitchapi/download classify.ts: `compact({ retryable, code })` (compact is re-exported
from `stitchapi`) — same pattern, kept consistent even though the package isn't linted.
Behavior-identical (compact drops undefined keys). Verified: core check:lint 0,
check:types 0, full core suite 1248/1248, @stitchapi/download 15/15, prettier clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@rc install The consolidation PR's `verify` job surfaced two ratchets #447 never reached (it died earlier at check:lint, now fixed): - check:contract (R2/P17): duration fields must not carry an `Ms` suffix — ms is the house unit (cf. core's `waited`/`elapsed`). Renamed `BatchProgress.etaMs` → `eta` and `DownloadIdleTimeoutError.idleMs` → `idle` (+ usages, README, spec). - check:release: every publishable package ships a LICENSE, and an rc package's README install command must target `@rc`. Added packages/download/LICENSE (Apache-2.0) and changed the install line to `@stitchapi/download@rc stitchapi@rc`. Verified locally: check:contract 0, check:release 0, package check:types 0, 15/15 tests, prettier clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`claude/download-consolidate` was cut 2026-07-06 with a 2026-07-23 merge base. Main has since landed the pre-GA breaking sweep, so replaying the seven commits was only half the work — the other half is what the ratchets then demanded. Core - `download`'s `interpret` composes `verdictOf(res, cfg)` (ADR 0022 Decision 4) and yields `data`, not `value`. The M1 complete-body rule sits AFTER the composed verdict, as the surface's own, and now reads `verdict.accept`: `classifyStatus` consults it only at `>= 400`, so without that read a caller who declared `206` NORMAL would still be rejected — the surface overriding an explicit declaration rather than ruling where none was made. Second test in download-stray-206.spec.ts pins it. - `rebuildError`'s cause-carry lands on top of main's body/url arm instead of replacing it. Test rig + specs - `throttle-host-pooling` keeps the deterministic manual-clock rewrite (the wall-clock flake fix) minus its `scope`-alias half — #490 removed the alias. ADR 0023 Decision 1 ratified exactly the 500ms in-process spacing it asserts. - Rig duration fields drop the `Ms` suffix (D3/P17), matching main's own `stream.chunkDelay`: `ttfbDelayMs` → `ttfbDelay`, `chunkDelayMs` → `chunkDelay`. The rig keeps main's `retryAfterSeconds` (the header's native unit) over the branch's `retryAfter`; four existing specs already depend on it. - Spec config: auth moved to `stitchapi/auth` (ADR 0021) and `apiKey` takes `secret` (#580); `retry.baseMs`/`baseDelay` fold into `retry.backoff.base` (#513); `respectRetryAfter` → `retry.respect` (#608); `AdapterProgress.phase` → `direction`. @stitchapi/download - `DownloadBatch.cancel`/`cancelAll` fold into `cancel(id?)` — R8 landed after this branch was cut and flags the shared prefix (P24). For a verb the optional parameter IS the collapse; an envelope would be the same pair one level deeper. - `hooks` is built unannotated so `onError` infers REQUIRED: the slot is `AtLeastOne<Hooks>` now (P20, #507), which an all-optional annotation cannot satisfy. - Version + vitest to the rc.7 lockstep; lockfile regenerated from main's. - classify.ts and the README no longer claim the engine drops the transport cause — this branch's own core fix carries it through. The hook seam stays: it is strictly wider (it also catches a thrown non-`Error`). Verify: 40/40 workspace projects green (core 1460 passed, download 15/15); check:lint / types / types-d / exports / exports:companions / contract / unknown-keys / release / docs-links / format all clean; core size 23.45 gzip (budget 23.50) and 20.87 for `import { stitch }` (20.90), @stitchapi/download 2.45/2.65 KB; `yakir check` both tiers 0 drift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of #639 flagged that this PR adds a second copy of the `instanceof StitchError || instanceof RateLimitError` predicate six lines below the one already in the function — the same question asked negatively (flatten a foreign error carrying `.response`) and then positively (pass an engine-minted error through). Bind it once. Behaviour is identical; the tests are unchanged and still green. The duplication is a symptom: the two classes are siblings, so every dispatch site has to name both. #662 makes `RateLimitError` extend `StitchError`, after which this collapses to a bare `source instanceof StitchError` — noted in a comment so the follow-up is obvious at the site. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
Reviewed the On this branch. The PR added a second copy of the The underlying issue → #662. The duplication is a symptom: the two classes were siblings, so every dispatch site had to name both. Worse, #662 makes Unrelated but still true from the PR description: this branch is behind |
rejifald
added a commit
that referenced
this pull request
Aug 5, 2026
The two error classes were siblings, and CONTRACT.md P10 kept them in parity by having `RateLimitError` re-declare `status`/`attempts`/`body`/`url` by hand — a written rule enforcing exactly what `extends` gives for free. Root the taxonomy at `StitchError` instead. The duplication had a cost beyond tidiness. `SafeResult.error` is typed `StitchError`, so `.safe()` coerced a delegate-backoff `RateLimitError` into a bare one: the instance moved to `.cause`, the `instanceof` test stopped working, and `error.body` came back `undefined` — dropping the payload an outer gate reads to pace itself. The mode whose whole point is handing back-pressure outward lost its signal on the path the docs otherwise recommend. `.safe()` now returns the same instance `await` throws. (Independently found from the consumer side by the scenario pass in #638, `body-verdict-footguns.md` item 2.) Every dispatch site also carried a two-arm `instanceof StitchError` / `instanceof RateLimitError` check; #639 was adding a second copy of it inside one function. One test now covers both. The one hazard the subclassing introduces is arm ordering — a leading generic `StitchError` arm swallows the delegate signal. `engine.ts`'s `errEvt` already had the order right (a RateLimitError also carries `.response`); it is now commented as load-bearing, P10 requires it, and the docs and error catalog say so where they branch. Also fixes the test stub, which flattened a stubbed `RateLimitError` and never stamped `retryAfter` on the streamed `error` event the way the engine does. Breaking for consumers that branch on both classes. Pre-GA (1.0.0-rc.7). Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
`errEvt` grew a third branch whose only distinguishing content was the same `Object.defineProperty(evt, ERROR_SOURCE, …)` the other two already ran — four sites in the file once `contractViolationEvt` is counted, held apart only by their branches being mutually exclusive. `defineProperty` defaults to non-configurable, so a second pin on one event throws; "written once" was an accident of the control flow rather than a property of the code. Collapse the decision into `ridesThrough(err)` — the mirror of `rebuildError`'s `isOurs` in stitch.ts, which asks the same question from the reading side — and route every producer through a single `pinSource` writer. The `.response` arm stays FIRST and untyped, so a BYO adapter throwing a non-Error bag that carries one is pinned exactly as before. No behaviour change: the branches were already disjoint, and the `retryAfter` stamp is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The rule reads `!== 200 && !== 204 && !accepts(status)`, and the comment above it only justified the 206 arm. Two things a reviewer has to reconstruct are now written down: - The `3xx` arm is load-bearing, not collateral. `followRedirects` hands the last 3xx back when the chain hits the hop cap or carries no `Location`, and download-redirect-loop.spec.ts depends on this rule to reject it — a narrower `status === 206` check would let an exhausted chain through as an empty Blob. - `203`/`226` do carry a complete body and are rejected anyway. That is a decision (strict reading, `accept` as the way back in), so it says so instead of looking like an oversight. Also states why `accept`'s position in the conjunction is not a semantic question: it only ever WIDENS, so a match makes the whole expression false wherever it sits. Testing it last is evaluation cost — no predicate closure allocated on the hot `200` path — not precedence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`BatchOptions.idleTimeout?: number` broke the naming contract twice: a
two-word field for one concept (P1), and a consumer-authored duration that
took raw ms only (P17 — "if it accepts a duration at all, it MUST also accept
a string"). It is now `idle?: number | string`, parsed at construction by
core's shared `parseDuration`.
`idle`, not a `timeout` envelope: a stitch's `timeout` is wall-clock —
`{ total, perAttempt }` fire on elapsed time regardless of progress — and it is
configurable in the same call, under `defaults`. Two different clocks sharing
one word is exactly what P2 forbids, so the batch's window is named for what it
measures rather than for what it does. A single-member `timeout: { idle }`
envelope would also nest for zero added configurability (P24 (b)).
That rename collides with `DownloadManager.idle()`, the queue-drain awaitable —
the same P2 hit one level over — so it becomes `drained()`, which is what its
own doc line already called it. Internals follow: `Active.idleTimer` → `timer`
(the only timer it holds), `#idledOut` → `#stalled`, `#idleWaiters` →
`#drainWaiters`. The `DownloadIdleTimeoutError` class and the `IDLE_TIMEOUT`
result code are unchanged — an error taxonomy name and a machine code, neither
an authored field.
Pre-GA and unreleased, so no alias (P19). The idle-timeout spec now passes the
window as `'120ms'` to pin the string arm.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds
@stitchapi/download, a batch downloader (FIFO queue, cancel, ETA, idle-timeout, request dedupe), together with thepackages/corefixes and test rig the work surfaced along the way.Commits
0dc61f5fix(core): reject partial (206) download responses; add a test riga938df6test(core): download batch/concurrency rig (M4) + de-flake throttle-host-poolingdf7e8d2test(core): finish the download rig — connection faults, retry, encoding, size, range (M5)9e5dfdffix(core): carry the transport cause through to the error the caller sees (#450)7fbfb4ffeat(download): the batch downloader itself (#449)0f216d6fix(core,download): usecompact()over the conditional-spread pattern (unblocks lint)3a67614fix(download): satisfy verify — drop Ms-suffix fields (P17) + LICENSE +@rcinstallfa92962fix(download,core): adopt ADR 0022, the pre-GA name sweep, and P24/R8Shape
60 files, +4,706/−61 — 34 in
packages/core, 23 in the newpackages/download, plusREADME.md,pnpm-lock.yaml, andscripts/gen-readme-metrics.mjs.Two things worth a reviewer's attention:
packages/corecarries real behavior changes, not just the new package: 206 partial responses are now rejected rather than silently accepted, and transport errors carry their cause through to the caller (fix(core): carry the transport cause through to the error the caller sees #450). Those are the parts to read closely.Note for review
This branch is behind
mainand intentionally not rebased — it lives in its own worktree that may be in active use, so rewriting its history wasn't something to do unprompted. Given it touchespackages/core, an Update branch (or a rebase — say the word) before merge is worth doing so CI runs against currentmain.🤖 Generated with Claude Code