fix(download): bound a transfer by whether it is moving, not by a flat total (B13 of #110)

[mobileskyfi]

B13 of #110. Both download paths bounded a transfer by total duration, with a number nothing had measured — and they did not agree on even that:

• images.ts:50 — AbortSignal.timeout(120_000) per attempt, 3 attempts with backoff;
• packages.ts — downloadPackages() had no deadline and no retries; its only bound was whatever the calling test imposed.

A total-duration deadline cannot tell slow from stuck. It fires on a healthy transfer whose only sin is being large, and the retry path then re-downloads from zero — one slow transfer becomes three.

The deadline was inside the healthy variance band, which is why it was intermittent

I set out to reproduce the abort locally and got something more useful. Same 52.2 MB all_packages-arm64-7.22.1.zip, same link, same client, four consecutive attempts against download.mikrotik.com:

attempt	elapsed	throughput	under the old flat 120 s?
1	118.4 s	0.421 MB/s	yes, by 1.6 s
2	94.2 s	0.528 MB/s	yes
3	123.5 s	0.403 MB/s	no — aborted
4	82.2 s	0.606 MB/s	yes

All four completed. One in four exceeded the deadline, and a fifth attempt through the new streaming path measured 129.4 s (0.385 MB/s) — so the range on one ordinary link is 82–129 s against a 120 s bound. The old deadline was inside the distribution, so an unchanged healthy download passed or failed on link jitter alone. That is a sharper statement of the defect than a clean abort would have been, and it explains why CI saw "both images needed two retries" sometimes rather than always.

CI's cold ~0.35 MB/s (→ ~149 s) sits below that entire local range, which is why a hosted runner hit it far more reliably than this laptop does.

Two corrections I made to myself mid-PR, both recorded in the commits: an early draft claimed the old shape "aborts that same transfer mid-flight" on the strength of one 129.4 s measurement — the next attempt completed in 101.5 s and disproved it as a general claim. The replacement four-point set then turned out to mix two client implementations, which is a confound in a measurement about link variance, so it was re-taken with one client. The table above is the homogeneous set.

Two deadlines, not one

The stall-only design in #116's body has a hole — a transfer trickling at one byte per second resets its stall deadline forever. So a transfer is bounded by both, and the failure says which fired:

1. Stall (30 s) — reset on every received chunk. Bounds silence, so a moving transfer is never aborted for being slow.
2. Transfer budget — 30 s base plus content-length ÷ 120 000 B/s, a floor throughput ≈ ⅓ of the slowest cold transfer on record. The 52.2 MB zip gets 465 s instead of 120 s.

Verified the size-derived budget is the production path, not the fallback: download.mikrotik.com sends content-length on both image and package artifacts (52216933 and 44980299 respectively). When a server does not, the stated 15-minute DOWNLOAD_NO_LENGTH_BUDGET_MS applies and the outcome says so.

Retry policy follows from the split

• DOWNLOAD_STALLED — retriable. A wedged socket usually moves on the next attempt.
• DOWNLOAD_TOO_SLOW — terminal on purpose. The budget is already ~3× the slowest throughput on record, so retrying re-downloads from zero on a link measured slower than the floor. That is the "one slow transfer becomes three" behavior this bite removes; it is a signal that the floor is wrong (or the link genuinely is), not something to paper over with another attempt.

Both carry bytes/expected/elapsed/throughput and the budget, so a CI log answers "slow, stalled, or refused?" without a re-run:

Download stalled: no data for 30s — 12.0 MB of 49.8 MB in 149.0s (0.081 MB/s, budget 465s) from https://…

One home for the floor constant

COLD_DOWNLOAD_FLOOR_BYTES_PER_S moves from test/integration/timeouts.ts into src/lib/download.ts, and the test module re-exports it. coldDownloadTestTimeout() is now derived from transferBudgetMs() rather than recomputing from the same floor, which makes one of #106's partial orders structural instead of coincidental:

download stall + transfer deadline  <  test timeout

A test can no longer be given less time than the download it waits on — #91/#116 in its general form. The old arrangement satisfied it by 30 s, by accident.

Partial transfers cannot poison the cache

Downloads stream to <dest>.part and are renamed (not copied) only after a length-verified transfer. Both callers gate on existsSync(zipPath), so a truncated file at the destination would have been served as a complete cached artifact forever. The old arrayBuffer() path was accidentally safe here; a streaming one is not unless you handle it.

Testing note worth keeping

Bun.serve drops an explicit content-length when the body is a ReadableStream and frames the response transfer-encoding: chunked. A stub built on it cannot exercise the size-derived budget at all — every response looks unknown-length, and my first cut of these tests passed for that wrong reason. The anchor tests use a raw Bun.listen HTTP stub instead. Also measured: when a server declares a length and closes early, Bun's fetch raises ECONNRESET before the explicit length check is reached, so that check is defense in depth rather than the primary guard.

Verification

• bun run check green; unit 889 pass / 0 fail (26 new in test/unit/download.test.ts, all against the local stub — no test depends on download.mikrotik.com being slow).

• Real cold download through the library: 52.2 MB in 129.4 s at 0.385 MB/s, content-length verified, byte-for-byte match on disk.

• Cold-cache CI dispatch: run 30645898475 — linux-arm64 · 7.22.1 · license.test.ts, green, pinned to a version the cache does not hold (it holds only 7.23.2). The image download was genuinely cold and classified with no retries:

  Cache READER for linux-arm64 @ 7.22.1 — test-filter is set; readers never write (#104)
  Cache restored from key: chr-images-v3-linux-arm64-7.23.2
  Downloading CHR 7.22.1 (arm64)...
    Transferring 18.1 MB (budget 188s)
    Saved (18.1 MB of 18.1 MB in 4.4s (4.071 MB/s, budget 188s))
  

  Two honest limits on that dispatch. The packages came back Using cached packages: 7.22.1 — the restore-keys fallback to the 7.23.2 entry carried 7.22.1's zips, which is exactly the union growth B3 flagged in CI: per-run cache rotation writes ~5 GB per full dispatch and exceeds the 10 GB quota #104, so the 52.2 MB artifact was not re-downloaded there. And the runner pulled at 4.07 MB/s, ~10× the 0.35 MB/s CI measured on 2026-07-30 — so this run confirms the classification works on a real cold download, it does not re-measure the slow-link condition. The slow path is covered by the local table above and by the stub tests.

Review round (CodeRabbit, 8a45dc0)

Five findings, all valid and all fixed with a regression test each. Two were more than their grade suggested:

• Shared .part path (Major). Both call sites guard only with existsSync(destPath), so two concurrent downloads of one artifact interleaved writes and renameSync then published the corrupt result to the cache path — the exact failure the .part mechanism was added to prevent. Now <dest>.<pid>-<uuid>.part. Collision removed; coalescing deliberately not attempted (needs a lock at both call sites).
• Stall message reported the constant, not the effective deadline. Graded Minor, but the message is the deliverable of Download deadlines are flat totals, so a healthy large transfer fails as a timeout instead of classifying #116 — and it was live in my own suite, which runs at stallMs: 300 while every failure claimed "no data for 30s".

Plus the timer-overflow clamp, content-length parse hardening, and cancelling the response body on a bad status.

A sixth, found while probing the fifth. Bun's fetch passes a malformed content-length through to response.headers but delivers a 0-byte body (measured: 4096.5 and -5; a blank value is stripped to null, so the reported Number("") path is unreachable through this transport). Since that leaves expected undefined there was no length to verify against — so an empty response would have been renamed into the cache and served as a complete artifact forever. downloadToFile now rejects a zero-byte body outright, whatever the framing. No artifact quickchr downloads is empty.

Out of scope

Cache keys and ownership (#104, landed). Boot envelopes (#106). Range-request resume would be strictly better than either deadline — MikroTik sends accept-ranges: bytes — but this bite bounds and classifies a transfer, it does not change the transport.

Closes #116.

🤖 Generated with Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR adds shared streaming download handling for images and packages. It enforces stall and size-based transfer deadlines, classifies failures, retries eligible errors, verifies content length, atomically finalizes files, and aligns integration timeouts with the shared budget.

Changes

Download lifecycle

Layer / File(s)	Summary
Download contracts and error classifications src/lib/download.ts, src/lib/types.ts	Defines transfer budgets, download options, transfer outcomes, deadline errors, diagnostic formatting, and DOWNLOAD_STALLED and DOWNLOAD_TOO_SLOW error codes.
Streaming, retry, and atomic finalization src/lib/download.ts	Adds downloadToFile with resettable stall detection, size-based transfer budgets, retries, streaming writes, length verification, partial-file cleanup, and atomic .part-file renaming.
Image and package integration src/lib/images.ts, src/lib/packages.ts	Routes both download paths through downloadToFile and removes their local retry, response validation, buffering, and file-writing logic.
Budget wiring and behavioral validation test/integration/timeouts.ts, test/unit/download.test.ts, test/unit/timeout-scaling.test.ts, .github/instructions/ci.instructions.md, DESIGN.md, CHANGELOG.md	Aligns test timeouts with transferBudgetMs and covers slow, stalled, truncated, retryable, non-retryable, and unknown-length transfers. Documentation records the new classifications and deadlines.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant downloadImage
  participant downloadPackages
  participant downloadToFile
  participant HTTPResponse
  participant PartFile
  downloadImage->>downloadToFile: request image download
  downloadPackages->>downloadToFile: request package download
  downloadToFile->>HTTPResponse: stream response
  HTTPResponse->>PartFile: write chunks
  downloadToFile->>downloadToFile: enforce stall and transfer-budget deadlines
  downloadToFile->>PartFile: verify length and rename completed file

Loading

Possibly related PRs

• tikoci/quickchr#117: Both PRs modify download timeout budgeting in test/integration/timeouts.ts.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The implementation meets issue #116 by adding shared bounded downloads, distinct classifications, retries, diagnostics, atomic files, and targeted tests.
Out of Scope Changes check	✅ Passed	The code, documentation, and tests directly support the download deadline, classification, retry, and timeout objectives in issue #116.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes replacing flat download timeouts with movement-based transfer bounds, which is the main change.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/116-download-deadlines

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[mobileskyfi]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/lib/download.ts`:
- Around line 157-167: Update DownloadDeadlineError to accept the effective
stall deadline instead of always using DOWNLOAD_STALL_MS, and use that value in
the stall message. At the throw site around the transfer-deadline handling, pass
opts.stallMs ?? DOWNLOAD_STALL_MS so overridden deadlines are reported
accurately while non-stall messages remain unchanged.
- Around line 315-318: Update the content-length parsing near expected and
budgetMs so empty or whitespace-only header values produce undefined rather than
zero. Only convert a nonblank header to a finite numeric expected value,
preserving the existing fallback and transferBudgetMs behavior for unknown
sizes.
- Around line 116-121: Update transferBudgetMs to clamp the calculated budget to
the maximum delay supported by setTimeout (2_147_483_647 ms), while preserving
the existing fallback for undefined or invalid byte counts and normal
calculations below the limit.
- Around line 303-313: Update the non-OK response handling in the download flow
to cancel the response body before either error is thrown. Ensure this
cancellation occurs for both the non-retriable QuickCHRError path and the
retriable Error path, including every retry attempt, while preserving the
existing status handling and messages.
- Around line 215-230: Make the temporary part path unique for each
downloadToFile invocation instead of deriving it solely from destPath. Update
the partPath construction near the retry loop to include a per-call unique
suffix, and ensure attemptDownload, renameSync, and removeIfPresent all use that
same path so concurrent downloads cannot overwrite or delete one another’s
temporary files.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b9956c59-4b4d-49b5-9d3b-cb30f0a2c12b

📥 Commits

Reviewing files that changed from the base of the PR and between de7e231 and 56b6ce3.

📒 Files selected for processing (10)

• .github/instructions/ci.instructions.md
• CHANGELOG.md
• DESIGN.md
• src/lib/download.ts
• src/lib/images.ts
• src/lib/packages.ts
• src/lib/types.ts
• test/integration/timeouts.ts
• test/unit/download.test.ts
• test/unit/timeout-scaling.test.ts

📜 Review details

🧰 Additional context used

📓 Path-based instructions (7)

src/**

📄 CodeRabbit inference engine (CLAUDE.md)

Follow the rules in general.instructions.md for files under src/**, including layer boundaries, error-code usage, port layout, and the RouterOS “expired admin” caveat.

Files:

• src/lib/images.ts
• src/lib/packages.ts
• src/lib/types.ts
• src/lib/download.ts

**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

In Bun-based TypeScript code, use Bun.spawn(), Bun.write(), Bun.sleep(), bun:test, and ESM imports with .ts extensions.

Files:

• src/lib/images.ts
• src/lib/packages.ts
• test/unit/timeout-scaling.test.ts
• src/lib/types.ts
• test/unit/download.test.ts
• src/lib/download.ts
• test/integration/timeouts.ts

src/lib/**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

Keep src/lib/ as pure library code: do not import from src/cli/ and do not call process.exit() there.

Keep src/lib/ as pure library modules: do not add CLI dependencies or call process.exit().

Files:

• src/lib/images.ts
• src/lib/packages.ts
• src/lib/types.ts
• src/lib/download.ts

**/*.ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.ts: Use Bun APIs and tooling rather than Node.js equivalents: Bun.spawn(), Bun.write(), Bun.sleep(), bun test, and bun:test. Use ESM with .ts extensions in imports; do not use CommonJS.
For ARM64 virt machines, never use if=virtio for drives; use an explicit -device virtio-blk-pci,drive=drive0 configuration.
When using HVF acceleration, use -cpu host, not cortex-a710.
For arm64 guests on macOS, automatically select TCG with -cpu cortex-a710; HVF cannot run the CHR image's 32-bit ARM userspace on Apple Silicon. --accel and QUICKCHR_ACCEL must override this selection for testing.
UEFI pflash code and vars units must be identical in size.
QGA is x86-only; do not assume the guest agent starts for arm64 CHR.
Use tabs for indentation.
Do not add unnecessary comments to obvious code.
Errors must be thrown as QuickCHRError(code, message, installHint?).
Preserve the documented public API types and behavior: QuickCHR.start(opts) returns ChrInstance; ChrInstance provides stop(), remove(), rest(), monitor(), serial(), and qga(); and MachineState represents persisted machine.json state.

Files:

• src/lib/images.ts
• src/lib/packages.ts
• test/unit/timeout-scaling.test.ts
• src/lib/types.ts
• test/unit/download.test.ts
• src/lib/download.ts
• test/integration/timeouts.ts

test/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Do not turn a red integration test green by broadening timeouts, skipping it, or platform-gating it before reproducing and root-causing the failure.

Files:

• test/unit/timeout-scaling.test.ts
• test/unit/download.test.ts
• test/integration/timeouts.ts

test/unit/**/*.ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Unit tests must be fast and must not require QEMU.

Files:

• test/unit/timeout-scaling.test.ts
• test/unit/download.test.ts

test/integration/**/*.ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Integration tests may require QEMU and must be guarded by QUICKCHR_INTEGRATION=1.

Files:

• test/integration/timeouts.ts

🧠 Learnings (1)

📚 Learning: 2026-07-31T11:56:40.455Z

Learnt from: mobileskyfi
Repo: tikoci/quickchr PR: 117
File: scripts/ci-cache-key.ts:68-68
Timestamp: 2026-07-31T11:56:40.455Z
Learning: In Bun TypeScript files, do not flag use of `node:fs.appendFileSync` when append semantics are required and `Bun.write()` cannot provide them. This applies to cases such as appending multiple entries to `$GITHUB_OUTPUT` or writing to the boot log; use append-capable file operations rather than overwriting existing content.

Applied to files:

• src/lib/images.ts
• src/lib/packages.ts
• test/unit/timeout-scaling.test.ts
• src/lib/types.ts
• test/unit/download.test.ts
• src/lib/download.ts
• test/integration/timeouts.ts

🪛 LanguageTool

DESIGN.md

[style] ~82-~82: ‘on the strength of’ might be wordy. Consider a shorter alternative.
Context: ... An earlier draft of this entry said so on the strength of a single 129.4 s measurement; the next ...

(EN_WORDINESS_PREMIUM_ON_THE_STRENGTH_OF)

🔇 Additional comments (12)

src/lib/download.ts (2)

247-255: LGTM!

---

335-352: 🩺 Stability & Availability

No buffering concern remains. Bun.file(...).writer() flushes automatically at its highWaterMark, so periodic flush() or an explicit highWaterMark are not required here.

src/lib/types.ts (1)

791-800: LGTM!

src/lib/images.ts (1)

10-10: LGTM!

Also applies to: 45-46

src/lib/packages.ts (1)

9-9: LGTM!

Also applies to: 37-45

test/integration/timeouts.ts (1)

30-30: LGTM!

Also applies to: 55-65, 100-102

test/unit/timeout-scaling.test.ts (1)

12-12: LGTM!

Also applies to: 90-128

test/unit/download.test.ts (2)

139-417: LGTM!

---

20-35: 🎯 Functional Correctness

No issue: src/lib/download.ts exports the required test symbols.

DESIGN.md (1)

65-97: LGTM!

CHANGELOG.md (1)

102-125: LGTM!

.github/instructions/ci.instructions.md (1)

463-465: LGTM!