fix(cli): follow every redirect a host may answer with

[miguel-heygen]

What

Makes downloadFile follow every redirect a host may reasonably answer with, and resolve the Location header against the URL that sent it.

Why

The old code handled 301 and 302 only, and passed the Location header straight back as a request target.

Neither assumption holds. 303, 307 and 308 are all reachable, and hosts answer with relative locations (/api/resolve-cache/...) far more often than the original assumed. A CDN handoff therefore failed on a string that was never a URL, with no redirect loop protection either.

How

redirectTarget resolves the header against the URL that sent it, and is split out because it is the part that was wrong and the part that can be checked without a socket. REDIRECT_CODES covers all five. A hop cap ends a redirect loop rather than recursing.

The timeout, DownloadOptions and the partial-file cleanup are untouched context here; they were already on main and this PR does not change them.

Test plan

• Unit tests added/updated
• Manual testing performed
• Documentation updated (if applicable)

6 tests: main's existing one that drives downloadFile with an idle response and asserts the request timeout fires and the partial file is removed, plus 5 covering the code set, relative and absolute locations, and sibling-relative paths.

Correcting an earlier version of this PR. It replaced download.test.ts wholesale with redirect-only tests, which deleted that idle-response test and left downloadFile with no test that calls it at all. The description also claimed to be restoring a timeout that had been lost; against main nothing was lost, and that claim was wrong. The test is back and the subject now says what the diff does.

Not covered. No test asserts the full redirect chain end to end, since that needs a server that actually answers 307. The hop cap and target resolution are covered as pure functions.

[vanceingalls]

Review — APPROVE

Clean, focused fix that does exactly what the title says. The old code answered only 301/302 and shoved the raw Location header back into httpsGet, so HuggingFace's 307 + relative /api/resolve-cache/... handoff fell through the "not 200" branch and hung. This PR fixes both halves at once and adds a redirect-loop bound to boot.

What's well-done

• Extraction for testability. REDIRECT_CODES and redirectTarget() are pulled out precisely because they're the parts that don't need a socket to test — matches how the failure decomposed.
• new URL(location, from) is the right primitive: absolute URLs pass through unchanged, relative paths resolve against the request URL, query strings are preserved. Tests cover all three shapes.
• MAX_REDIRECTS = 10 bound with cleanup (res.resume() + removePartialFile(tmp) + reject) before recursion — this is the exact hole in the pre-fix code (an infinite 302 chain would have blown the stack silently).
• res.resume() before the recursive follow(...) drains each redirect response, so a long chain does not accumulate live streams.
• Backwards-compatible. Three callers in the CLI (tts/manager.ts, background-removal/manager.ts, whisper/manager.ts) all call downloadFile(url, dest) positionally with no options — no signature change, no ambient setup lost in extraction.
• Docstrings explain the why — the block above downloadFile is the kind of comment that would have prevented this bug in the first place (per-hop timeout budget documented as intentional).

Notes (non-blocking)

• Test coverage is helper-scoped, not end-to-end. The five new tests exercise REDIRECT_CODES.has(code) and redirectTarget(...) as pure functions, which is fine, but no test drives a 307 response through downloadFile itself and asserts it terminates on a 200. That would have caught, e.g., a future refactor that forgot to feed redirectTarget's output back into follow(...), or that dropped the hops+1 increment. Follow-up nit; existing coverage is enough to unblock.
• HTTPS→HTTP downgrade behavior is inherited: httpsGet throws Protocol "http:" not supported on a bad-protocol target. That's a synchronous throw from inside the response callback (unchanged from the pre-fix code path — old code passed the raw location and would hit the same edge), so not new to this PR. Worth an eventual audit but not this one.
• Per-hop timeout budget (10 × 30 s worst case = ~5 min) is documented as intentional in the docstring. Fine.

CI on the diff is green (the CANCELLED windows-render / smoke / windows-tests jobs look infra-side, unrelated).

Ship it.

— Via

[james-russo-rames-d-jusso]

Reviewed at 9cc110902.

Right diagnosis and right shape for the class. Widens REDIRECT_CODES from {301, 302} to {301, 302, 303, 307, 308} at packages/cli/src/utils/download.ts:14 — the 307 hop HuggingFace answers for tokenizer URLs (named in the test comment at download.test.ts:93-95) was the one falling into the not-200 branch and hanging. Splits redirectTarget(location, from) at download.ts:25-27 into a pure new URL(location, from).toString() that resolves relative Location headers against the URL that sent them — exactly what was needed for hosts answering /api/resolve-cache/... — and covers the three shapes (absolute, root-relative, sibling-relative) in tests at download.test.ts:112-131. MAX_REDIRECTS = 10 at download.ts:29 with the docstring "Enough hops for a CDN handoff, few enough that a redirect loop still ends" is a good default. Hop-counter threading through follow(u, hops = 0) is clean; on cap-exceeded the code correctly res.resume() → removePartialFile(tmp) → reject.

Two small notes below; not blockers.

Concerns

Malformed Location header throws through the httpsGet response callback with no catch. download.ts:65 — follow(redirectTarget(location, u), hops + 1). redirectTarget calls new URL(location, from) which throws TypeError on malformed input. The callback is (res) => { ... } on httpsGet; a synchronous throw inside a response handler bubbles as an unhandled exception on the request emitter — Node's default behavior on error with no listener is to crash. A misbehaving server (or a corrupted redirect chain) with a Location like ://bad-scheme would take the process down instead of failing this one download. Cheap fix: wrap the redirect branch in try { ... } catch (err) { res.resume(); removePartialFile(tmp); reject(err); return; } so the malformed Location fails the download rather than the process.

Per-request timeout re-arms per hop; total budget is MAX_REDIRECTS × DEFAULT_DOWNLOAD_TIMEOUT_MS. Named in the docblock at download.ts:44-46 as intentional — "a stalled socket is what it exists to catch, and without it the CLI waits forever." Fine as-is; naming here so a future reader knows the total ceiling isn't the per-request value.

Nits

• No test for the MAX_REDIRECTS cap (11-hop chain → reject with the expected message). Cheap to add for symmetry with the other five redirect tests. download.test.ts:89-131 covers the code-set, relative resolution, and edge cases; the loop-guard is uncovered.
• redirectTarget is exported but the internal call site at :65 uses it via the module-local reference. Export is for the test — that's fine, just naming.

What lands cleanly

• Widened set at download.ts:14 with a docstring naming why the old set was too narrow.
• redirectTarget pure and testable — extracted for exactly the reason Miga names (download.ts:16-24): "this is the part that was wrong, and it is the part that can be checked without a socket."
• Hop-counter + max-hops guard that closes the redirect-loop class.
• Direct contract-lock tests at download.test.ts:112-131 for all three Location shapes.
• The full-file docblock at download.ts:42-52 names the per-hop timeout choice explicitly, which is the kind of context a next-toucher will want.

LGTM from my side; the malformed-Location catch is the one thing worth naming before landing.

— Review by Rames D Jusso

[vanceingalls]

R2 delta-verify at 4db0a34e8 (base 9cc11090, +1 commit, 2 files, +132/-33).

All R1 P3 findings resolved.

P3-1 · integration test for follow(...) chain — FIXED

New test at packages/cli/src/utils/download.test.ts:141-155 drives a full 307 through downloadFile end-to-end:

mockGet
  .mockImplementationOnce(httpsResponse(307, { location: "http://cdn.example.test/model.onnx" }))
  .mockImplementationOnce(() => { throw new TypeError('Protocol "http:" not supported'); });
await expect(downloadFile("https://example.test/model.onnx", ...)).rejects.toThrow('Protocol "http:" not supported');

That is exactly the code path R1 asked for: response callback → REDIRECT_CODES branch → recursive follow(redirectTarget(...), hops+1) → sync throw from httpsGet → outer try/catch → clean reject. Concurrent-download test at :53-64 and byte-cap test at :66-78 add two more full-chain integration cases.

P3-2 · sync-throw wrap · tmp isolation · byte caps — FIXED

• Sync throw wrap (packages/cli/src/utils/download.ts:83-133): httpsGet(u, (res) => {...}) is now inside a try/catch. On sync throw (protocol mismatch, malformed URL) the catch at :129-133 calls removePartialFile(tmp); reject(error); return; before ever reaching the request.setTimeout line. The recursive follow at :98-103 is separately wrapped so a new URL() throw from redirectTarget() also lands as a clean reject.
• Tmp isolation (download.ts:74): baseline was ${dest}.tmp (single shared path); now ${dest}.${process.pid}.${randomUUID()}.tmp — unique per call across process + intra-process. Test at :53-64 races two downloadFile calls to the same dest and asserts one of the two payloads lands cleanly with no cross-contamination.
• Byte caps (download.ts:43-54, 76, 115-118): new enforceByteLimit(maxBytes) Transform accumulates received per chunk and fires the pipeline callback's error slot once received > maxBytes. Wired conditionally: maxBytes === undefined ? pipeline(res, file) : pipeline(res, enforceByteLimit(maxBytes), file) — opt-in per call. Test at :66-78 asserts both the error message ("Download exceeded 3 bytes") AND readdirSync(dir).toEqual([]) — confirms tmp is cleaned, not just that dest is absent.

Adversarial pass, no blockers

• maxBytes is opt-in per call. The primitive is in place with the right API shape; whether existing call sites now pass a cap is a separate audit, not a delta-verify concern.
• No happy-path integration test for a 307 → 200 chain that fully succeeds. The sync-throw test covers the R1 concern (error propagation through the follow chain) and redirectTarget is unit-tested across absolute/path-relative/sibling shapes, so this is a nit-of-a-nit at most.
• The outer request's setTimeout stays armed after a redirect resumes the response. If the outer socket idles long enough post-redirect while the recursive follow is still downloading, a spurious late "error" event would double-reject the promise (harmless — settled) and could removePartialFile(tmp) on the file the inner follow is writing (harmful — leads to a rename ENOENT). Not a regression from this commit (timeout wiring is pre-existing at the same shape), and the timing window is narrow. Worth a follow-up: disarm request.setTimeout on the outer hop after res.resume() on the redirect branch.

Positives

• Cleanup discipline is thorough — every reject path calls removePartialFile(tmp), and the byte-cap test verifies the directory is empty rather than just that the destination is absent (catches leaked .tmp files).
• Tmp path uses randomUUID() rather than Date.now(), which handles same-ms concurrent calls in the same process — a subtle bug that the weaker approach would have retained.
• enforceByteLimit uses the Transform callback's error slot so the pipeline sees the failure at the offending chunk, not via a post-hoc emit('error') race.
• Idle-response test at :80-137 even exercises the retry semantics of removePartialFile around a locked file (EBUSY on first attempt, success on second), confirming the "missing/locked partial files are handled by the next atomic download" claim in the comment.

APPROVE.

{
  "pr": 3148,
  "old_head": "9cc11090",
  "new_head": "4db0a34e8",
  "verdict": "APPROVE",
  "deltas_by_finding": [
    {"r1_finding": "P3-1 integration test for follow(...) chain", "outcome": "FIXED", "evidence": "download.test.ts:141-155 — 307 → sync-throw second call → rejects with 'Protocol \"http:\" not supported'; plus concurrent-download test :53-64 and byte-cap test :66-78"},
    {"r1_finding": "P3-2 sync throw + tmp isolation + byte caps", "outcome": "FIXED", "evidence": "download.ts:83-133 wraps httpsGet call + :98-103 wraps recursive follow; :74 tmp = ${dest}.${pid}.${uuid}.tmp; :43-54 + :115-118 enforceByteLimit Transform"}
  ],
  "new_findings": [
    {"severity": "nit-followup", "file": "packages/cli/src/utils/download.ts", "lines": "97-104, 134-136", "issue": "Outer request.setTimeout stays armed after redirect res.resume(); late idle-timeout could double-reject and remove tmp that recursive follow is writing. Pre-existing shape, not a regression. Consider disarming setTimeout on the outer hop when following a redirect."}
  ],
  "positives": [
    "Sync-throw wrap covers both httpsGet call and recursive follow's URL parsing",
    "Tmp isolation uses pid + randomUUID (not Date.now), safe for same-ms concurrent",
    "Byte cap uses Transform's error-slot callback (no emit-after-write race)",
    "Cleanup asserts readdirSync empty, not just dest absent",
    "Idle-response test exercises removePartialFile retry semantics"
  ]
}

— Via

[james-russo-rames-d-jusso]

Reviewed at 4db0a34e8 (delta from 9cc110902).

Delta clean. My R1 concern — malformed Location throwing through the httpsGet response callback with no catch — is closed at download.ts:88-94: the follow(redirectTarget(...)) call is wrapped in try/catch that removePartialFile(tmp) and rejects rather than bubbling as an uncaught throw on the request emitter. The httpsGet call itself is also guarded at :83, :117-121, so a synchronous TypeError at construction is caught symmetrically. Direct regression coverage at download.test.ts:141-155 — mocked 307 → second httpsGet throws Protocol "http:" not supported → download rejects with that message. Locked.

Two bonus fixes beyond what R1 named:

• Concurrent temp-file isolation at download.ts:74 — ${dest}.${process.pid}.${randomUUID()}.tmp replaces the shared ${dest}.tmp, so two downloadFile calls to the same dest no longer race on the same partial. Test at download.test.ts:36-49 locks it: two concurrent downloads to the same path, both succeed, dest contains one of the two payloads (last-writer wins on renameSync, both are complete).
• Per-response byte cap at download.ts:43-55 via a Transform that fails the pipeline once received > maxBytes, plumbed as DownloadOptions.maxBytes at :14. Test at download.test.ts:51-63 locks the reject, and specifically asserts readdirSync(dir) empty afterwards — the partial file cleanup pathway runs.

The per-request timeout budgeting is still per-hop (documented at download.ts:65-68), unchanged from R1 as intended.

Nits

• Redirect-loop guard (11 hops → reject) still doesn't have a direct test; the code-set / relative-resolution / synchronous-throw paths are covered. Follow-up.

Clean; ready from where I sit — stamp routing per standing rule.

— Review by Rames D Jusso