feat(download): serve HTTP Range on the proxy route

[peter-svensson]

Range support on /download/:id, as discussed in #263.

Why the proxy route needs this

@actions/cache picks its download strategy from the URL's hostname. A .blob.core.windows.net host gets the concurrent, ranged downloader; everything else gets a single httpClient.get() with no Range and no keep-alive (cacheHttpClient.ts). A self-hosted server can never match that hostname, so its clients are pinned to one stream no matter how much bandwidth is available.

Serving Range here is what lets a range-capable client use that bandwidth without handing object-store credentials to the job — the credentials stay in this process, which is the point of proxying rather than presigning.

Measured on one runner pod against one 320 MB object: ~15 MB/s as shipped, 143 MB/s over 8 parallel ranges.

Two commits, kept separate as requested.

fix(download): destroy the backend stream when the client hangs up

Independent of Range. The route handed the storage stream to h3's sendStream, whose web-stream path neither applies backpressure nor notices the client going away, so an aborted download kept draining the adapter read into a socket nobody was reading — the backend GET ran to completion in the background, and the reader lease attached to that stream was held until it expired instead of being released on close. One leaked read per abort, multiplied by a client that fans out in parallel and retries.

stream.pipeline destroys the source when the destination closes, which releases the lease through the existing close handler. ERR_STREAM_PREMATURE_CLOSE is the expected outcome of an abort and is logged at debug; other post-headers failures keep the previous behaviour of logging rather than becoming an HTTP error.

tests/download-abort.test.ts aborts mid-body against a paced, unending source and asserts the source is destroyed. It fails against the sendStream-style web-stream pipe and passes with pipeline — verified both ways.

feat(download): serve HTTP Range on the proxy route

Adapters return { stream, size, range } instead of a bare Readable. range is set only when the adapter actually served one, clamped to the object; size is the whole object's size when known. The range is passed through to the backend (S3 Range, createReadStream start/end on the filesystem and GCS) rather than sliced in the server, so a partial request never pulls the whole object into memory, and an open-ended bytes=N- stays open-ended so the backend resolves the end itself — no HEAD needed.

The route answers:

• 206 with content-range: bytes <start>-<end>/<size> and content-length when the adapter served the range;
• 200 with content-length otherwise — no Range, an unparseable one, or an unmerged entry, which is concatenated from its Parts as it is read and has nothing to seek into. Clients key off the status, not accept-ranges;
• 416 with content-range: bytes */<size> when the range starts at or past the end, header omitted when the backend did not report a size.

Only the single bytes=start-end and bytes=start- forms are parsed; anything else falls through to a normal 200 with the whole object, which is always correct.

An S3 206 whose Content-Range does not parse destroys the stream and throws rather than serving a slice under a 200 — the one case where status and body would disagree and a client could not tell.

The reader lease stays bound to the stream on every path, including the 416 throw, the merge-tee path and the !merge fallback.

Not included

• Eager merge on finalize — landed as feat(storage): opt-in eager merge so the first restore can be a direct download #267 while this was in progress, so it is dropped here.
• Presigned URL expiry — already fixed on dev in 8d9d11e; my fork predated it.

Testing

tests/range-download.test.ts covers closed, open-ended, clamped, unsatisfiable, malformed and unmerged cases against the running server, with body bytes compared to the upload.

Full suite green locally on sqlite across all three storage drivers: filesystem (51 passed), gcs (52 passed), s3 via MinIO (53 passed). I have not run the postgres or mysql legs.

Two things I know are left rough, both driver-dependent and neither a regression:

• A zero-byte object answers bytes=0- with 416 on filesystem and GCS (clampRange treats start >= size as unsatisfiable) but 200 on S3, which returns the empty body rather than an error. Happy to normalize whichever way you prefer.
• On a 416 from S3, the total size comes from the error's ActualObjectSize, which is not a modeled field on the SDK's InvalidRange shape. When it is absent the content-range header is simply omitted, so it degrades safely, but a client gets a 416 it cannot plan against. Recovering it would need a HeadObject on that path, which I left out rather than reintroduce the HEAD this design avoids.

End to end on our deployment, same 320 MB entry, sha256-verified: 5.6 s → 3.5 s, ~140 MB/s over 9 requests.

Refs: #263

[LouisHaftmann]

There are a lot of comments in this PR, and most are too long. Many just say again what the code or types already show, like the doc comments on DownloadStream, RangeRequest and ByteRange, the block inside the S3 GetObjectCommand, and the notes above sendStream and accept-ranges. The one above parseRange has benchmark numbers and a toolkit source link, which belong in the PR description because they'll go stale in the code.

Could you go through them and keep only the ones that explain something the code can't, like why unmerged entries ignore Range or why _handled is set? Those should be one line each. Remove the rest. The diff will be about half the size and a lot easier to read.

[LouisHaftmann]

I think there's a race here. When someone saves the same key and version again, the entry keeps its id and completeUpload just points it at the new location. So if a client is pulling 8 ranges and a save finishes halfway through, some ranges come from the old object and some from the new one. They're all 206s with the same size, so the client has no way to notice and ends up with a broken archive. With one stream that couldn't happen.

Could we send an ETag on every response and support If-Range? The storage location id would work as the tag. If the tag doesn't match, send the whole object as a 200.

[peter-svensson]

Good catch — this is real, and I confirmed the mechanism: completeUpload inserts a new storage_locations row with a fresh UUID and repoints the existing cache_entries.locationId at it, keeping the entry id. So /download/:id is a stable URL whose bytes change on re-save.

I started on ETag + If-Range as you suggested and hit two things that change the shape of the fix, so I've written them up in a top-level comment rather than bury them here. Short version:

1. The storage location id alone isn't a safe tag — the same id can serve either the merged object or the concatenated parts, and I reproduced a merged → unmerged flip on a stable id. With a plain location-id tag, If-Range then matches and the route sends a full 200 body mid-pull, which is the same corruption through a different door. Suffixing the tag with the representation served fixes it cheaply.
2. If-Range only helps a client that already holds an ETag, so it doesn't cover a parallel first batch — which is the case in your comment and the motivation for the feature.

I don't want to claim this closes the race when it doesn't close the parallel case, so I've asked in the top-level comment which direction you'd prefer before I write it.

[LouisHaftmann]

This test spins up its own server and calls pipeline itself, so it only shows that pipeline works. It never hits the route or checks the lease, even though the commit message says it does. Could you change it to abort a real /download/:id request and wait for the lease row to go away?

[peter-svensson]

You're right, and thanks — it was worse than the PR claimed.

I rewrote it against the real route first, and that still didn't test the right thing. Two reasons: the server runs as its own process (tests/setup.ts spawns the built server with execa), so the adapter's read can't be observed or mocked from the test — I confirmed a vi.spyOn on the storage adapter never fires, SPY CALLS: 0. And protectDownloadStream releases the lease on end as well as close, so any finite body clears the lease whether or not the abort destroyed anything.

I checked whether the test could tell the fix from the bug by reverting the route to sendStream: it still passed. So the original test passed against the exact bug it was said to catch.

Renamed it to tests/download-leases.test.ts and cut the claim back to what it actually verifies — the reader lease is released after an abort and after a completed download. The docblock now says plainly that the sendStream leak is not covered here and why.

Catching the real leak needs a body that never ends, which means a test-only hook in the server process. Happy to add one if you want that; it seemed like more test-only surface than you'd want me adding uninvited.

[peter-svensson]

Thanks — you're right about the race, and I've confirmed it in the code. completeUpload inserts a new storage_locations row with a fresh UUID and then repoints the existing cache_entries.locationId at it, keeping the entry id, so /download/:id is a stable URL whose bytes change on re-save.

I pushed the three smaller items already:

• benchmark numbers and the toolkit link moved to the PR description, comments cut back throughout (docs: commit)
• _handled now carries a note that it's an h3 v1 internal
• empty objects: the 416 path returns an empty 200 when size === 0, so S3 and filesystem/GCS agree

I also renamed download-abort.test.ts to download-leases.test.ts and corrected what it claims. You were right that it didn't test the route — but the rewrite didn't fix it either. The server runs as its own process, so the backend read isn't observable from the test, and protectDownloadStream releases the lease on end as well as close, so a finite body clears it whether or not the abort destroyed anything. I verified by reverting the route to sendStream: the test still passed against the bug it was supposed to catch. It now asserts only lease lifecycle, and the docblock says what isn't covered. Catching the original leak needs a body that never ends, which means a test-only hook in the server — happy to add one if you want it, but it seemed like more test surface than you'd want without asking.

On ETag + If-Range — I went to implement it as you suggested and hit two things I'd rather check with you before writing code, because they change what the fix should be.

1. A location id alone isn't a safe tag, because the same id can serve two different representations.

A location can go merged → unmerged without the id changing. startMerge commits mergedAt at storage.ts:301-305, and the .catch at storage.ts:311-314 clears it back to null. That rollback is guarded only by the merge lease token, not by mergedAt IS NULL, so it will happily overwrite a mergedAt that was already durably committed.

That needs the commit to succeed but the caller to still see a throw — a lost commit ack or a connection dropped right after COMMIT. (A commit that genuinely fails is harmless: the rollback writes NULL over a row that is already NULL.) I reproduced it by letting a real merge run to completion and then throwing from the transaction wrapper after the commit landed:

COMMITTED mergedAt=1789451144761
FINAL {"mergedAt":null,"mergeStartedAt":null} mergedExists=true

The merged object is on disk, the row says unmerged, and the location id never changed. Worth saying this is a narrow window and I had to force it — I'm not claiming it's common, only that the id alone can't distinguish the two representations.

The consequence for If-Range is bad: the tag matches, so the route proceeds, but downloadFromCacheEntryLocation now takes the unmerged branch and returns no size and no range. The 206 condition fails and the route sends the whole object under a 200 in the middle of what the client thinks is a ranged pull. A client writing range N at offset N writes the entire archive there. If-Range matched, so it gets no warning — the same corruption, through a different door.

Cheap fix: suffix the tag with the representation actually served, "<locationId>-m" when serving merged and "<locationId>-p" when serving from parts. The flip becomes a mismatch, If-Range fails, and the client gets a 200 with a status it can see. Both variants are byte-identical today (the merge tee feeds the merged object from the same streamParts sequence, and the compose paths concatenate the same immutable parts), so this costs nothing — it just stops the design depending on that identity holding forever.

2. If-Range doesn't cover the parallel case, which is the one that motivates the feature.

It only helps a client that already holds an ETag. A client issuing 8 ranges concurrently — the case in your comment, and the reason to serve Range at all — has no tag for any of them, so all 8 go out with no If-Range and a completeUpload landing mid-flight still splices two objects silently.

So If-Range is detection for sequential and resumed reads, not a guarantee for parallel ones. It's a real improvement and I'm happy to build it, but I don't want to describe it as closing the race when it doesn't close the motivating case. Fully closing it server-side needs something that pins a client to one location across requests — a version in the URL, or a short-lived token — which is a bigger change than this PR and probably your call on direction.

Which would you prefer?

• a) ETag + If-Range with the variant suffix, documented as partial: protects sequential and resumed reads, not a parallel first batch.
• b) Same, plus requiring a range-capable client to learn the ETag from a probe request before fanning out, documented as a client contract.
• c) Something that closes it properly (location id in the download URL, say), if you'd rather not ship a partial mitigation.

I'd lean (a) for this PR with the limitation stated plainly, and a follow-up issue for the durable fix — but it's your call, and (c) is a design decision I don't want to make unilaterally in someone else's codebase.

One implementation note if we go with (a) or (b): If-Range can't be evaluated before download(), since the location id isn't known until it resolves, and download() already pushes the range down into the adapter. I'd accept one wasted ranged backend GET on the mismatch path and re-issue unranged, rather than split download() into resolve-then-open — the split would widen the gap between taking the reader lease and opening the stream, which is what currently makes the within-request case safe.

Also happy to add the tests for both: a re-save between two ranged GETs, and the merged/unmerged flip.