[peter-svensson]

Range support on the proxy download route, as discussed in #263. The sendStream → pipeline fix was split out and landed as #282.

Why

@actions/cache only uses its concurrent ranged downloader for .blob.core.windows.net hosts, so stock clients still do one plain GET against this server. This helps range-capable clients use parallel downloads without object-store credentials leaving the server. Measured on one runner pod, 320 MB entry: ~15 MB/s single stream, 143 MB/s over 8 parallel ranges.

What

• Adapters return { stream, size, range }. range is set only when the backend served one, and the range is passed through to it (S3 Range, Azure download(offset, count), filesystem/GCS createReadStream({ start, end })), so nothing is sliced or buffered in the server.
• 206 with content-range and content-length when the adapter served the range.
• 200 with content-length otherwise: no Range, multi/suffix ranges, or an unmerged entry (streamed from its parts, nothing to seek into). Clients key off the status.
• 416 with content-range: bytes */<size>, the size taken from the storage location row so every backend reports it.

Download URLs are pinned to the storage location

/download/:cacheEntryId was a stable URL whose bytes changed on re-save (completeUpload repoints the entry to a fresh location). A client fetching ranges in parallel across a re-save would splice two objects together without noticing. The URL is now /download/:locationId: a location's bytes never change, and a superseded location answers 404, so the client fails loudly instead of corrupting the archive. This replaces the ETag/If-Range discussion, which couldn't protect a parallel first batch.

Testing

tests/range-download.test.ts covers closed, open-ended, clamped, unsatisfiable, malformed and unmerged cases, plus the re-save case. Green on sqlite across filesystem, S3 (MinIO), GCS and Azure (Azurite).

Refs: #263

🤖 Generated with Claude Code

[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.

[peter-svensson]

@LouisHaftmann any chance we couldmoe this forward?

[chenrui333]

The sendStream → pipeline change here is also a memory fix, not only a lease/abort fix (details in #274). On 9.7.0 with S3 storage and direct downloads off, I restored a 1.3 GiB merged entry to clients limited to ~15 MiB/s, which is the single-stream rate from #263:

slow clients	peak arrayBuffers
1	1.43 GB
2	2.79 GB
4	OOM-killed at 4 GiB (3.39 GB in socket write buffers)

The cause is h3's web-stream branch of sendStream, which calls res.write() without waiting for drain. The upload route has the same problem through getRequestWebStream; that fix is in #275.

About the test gap you mentioned (the backend read can't be observed from the test): it can be, if the route handler runs in the test process. The test below mounts the real /download/:cacheEntryId handler on an in-process h3 app, with the h3 exports stubbed in as the Nitro auto-imports. It serves a generated 512 MiB stream and pauses the client after the first chunk. Against sendStream it fails: 536,805,514 bytes are pulled from storage and 535,728,052 bytes sit in the response queue. On this branch it passes, with 852,128 and 65,545 bytes. It runs in about 0.7 s with the default sqlite + filesystem setup, and gave identical numbers over 3 runs.

tests/download-backpressure.test.ts

import type { IncomingMessage, ServerResponse } from 'node:http'
import type { AddressInfo } from 'node:net'
import { Buffer } from 'node:buffer'
import { randomUUID } from 'node:crypto'
import { once } from 'node:events'
import http from 'node:http'
import { Readable } from 'node:stream'
import { setTimeout as sleep } from 'node:timers/promises'

import * as h3 from 'h3'
import { afterAll, afterEach, beforeAll, describe, expect, test, vi } from 'vitest'
import { getStorage } from '~/lib/storage'

const MiB = 1024 * 1024
const ENTRY_BYTES = 512 * MiB
const CHUNK_BYTES = 64 * 1024
// Stream highWaterMarks are KiBs and loopback socket buffers a few MiB. Holding
// more than this means the route is queueing the body in process memory instead
// of letting TCP flow control pace the faster side.
const MAX_BUFFERED_BYTES = 64 * MiB

// The real route handlers on an in-process h3 app, so the test process is the
// server process and its memory and storage calls are observable.
const router = h3.createRouter({ preemptive: true }) // as Nitro: an `undefined` return ends the response
const server = http.createServer(h3.toNodeListener(h3.createApp().use(router)))

beforeAll(async () => {
  for (const [name, value] of Object.entries(h3)) vi.stubGlobal(name, value)
  // Not static imports: route modules call Nitro auto-imports (`defineEventHandler`,
  // …) at evaluation time, so they can only load after the stubs above.
  const { default: download } = await import('~/routes/download/[cacheEntryId]')
  router.get('/download/:cacheEntryId', download)
  server.listen(0)
  await once(server, 'listening')
})

afterEach(() => {
  vi.restoreAllMocks()
  server.closeAllConnections()
})

afterAll(async () => {
  vi.unstubAllGlobals()
  server.close()
  await once(server, 'close')
})

/** Resolves once `read()` has not changed for `quietMs`, i.e. the pipe has stalled or finished. */
async function settled(read: () => number, quietMs = 500, timeoutMs = 20_000) {
  const deadline = Date.now() + timeoutMs
  let last = read()
  let quietSince = Date.now()
  while (Date.now() < deadline && Date.now() - quietSince < quietMs) {
    await sleep(50)
    if (read() === last) continue
    last = read()
    quietSince = Date.now()
  }
  return last
}

describe('download route applies backpressure', () => {
  test('download does not buffer the entry for a client that stopped reading', async () => {
    let pulled = 0
    function* entry() {
      while (pulled < ENTRY_BYTES) {
        pulled += CHUNK_BYTES
        yield Buffer.alloc(CHUNK_BYTES) // fresh memory per chunk, like a storage read
      }
    }
    const source = Readable.from(entry(), { objectMode: false, highWaterMark: CHUNK_BYTES })
    const storage = await getStorage()
    vi.spyOn(storage, 'download').mockResolvedValue({ stream: source })

    const { port } = server.address() as AddressInfo
    const serverSide = once(server, 'request') as Promise<[IncomingMessage, ServerResponse]>
    const req = http.get(`http://localhost:${port}/download/${randomUUID()}`)
    const [res] = (await once(req, 'response')) as [IncomingMessage]
    expect(res.statusCode).toBe(200)
    let received = 0
    res.once('data', (chunk: Buffer) => {
      received += chunk.length
      res.pause() // the client stops reading but keeps the connection open
    })

    await settled(() => pulled)
    const readAhead = pulled - received
    const [, serverRes] = await serverSide
    const queued = serverRes.writableLength
    req.destroy()

    expect
      .soft(readAhead, 'bytes read from storage but not delivered')
      .toBeLessThan(MAX_BUFFERED_BYTES)
    expect.soft(queued, 'bytes queued in the response').toBeLessThan(MAX_BUFFERED_BYTES)
  }, 60_000)
})

Feel free to take it into this PR. Otherwise I'll add it to #275 once this lands.

[peter-svensson]

@chenrui333 thanks — took it in as tests/download-backpressure.test.ts (co-authored, c60d28a). Confirmed locally: passes on this branch, fails against sendStream with ~536 MB read ahead and ~535 MB queued. That closes the gap I flagged in the lease test without a test-only hook. I've added the memory angle and a link to #274 to the description, and dropped the two items it listed as rough, which were settled in the September round.

@LouisHaftmann the open question is still the ETag/If-Range direction (a/b/c above). Happy to go with (a) if you have no preference.