Originally created by: Akarsh-Hegde
download_and_stage failure as retryable or permanent using a small DownloadError enum.RUNTIME_DOWNLOAD_TIMEOUT so the overall budget stays bounded.mlx-download-progress event.auto_upgrade_runtime uses the same retry helper.| Failure | Verdict |
|---|---|
| Network error, HTTP 5xx / 429 | retryable |
| Disk write error, stream error | retryable |
| Checksum mismatch (truncated stream) | retryable |
| Wrong arch, unsupported macOS | permanent |
| HTTP 4xx (except 429) | permanent |
tar missing / extract failed |
permanent |
The wizard's runtime download failed on the first network hiccup — one dropped connection, one 502, one truncated stream and the user had to hit Retry. The background auto-upgrade did the same but silently gave up. Neither is honest about how flaky first-attempt downloads actually are.
cargo check --workspace + cargo clippy --workspace -- -D warnings clean.cargo test --lib on tray/src-tauri/ — 42 tests pass including two new ones (http_retryable_classification, retry_backoff_covers_all_gaps).🤖 Generated with Claude Code
Originally posted by: coderabbitai[bot]
✨ Finishing Touches
🧪 Generate unit tests (beta)
- [ ] Create PR with unit tests - [ ] Commit unit tests in branch `feat/runtime-download-retry`Comment
@coderabbitai helpto get the list of available commands.Originally posted by: adityaharishch
Reviewed the diff (
tray/src-tauri/src/mlx_server.rs, +199/-39). The retry/backoff structure, retryable-vs-permanent classification, and graceful degradation (staging dir wiped, overallRUNTIME_DOWNLOAD_TIMEOUTstill wraps the whole retry loop) are sound overall. A few concrete issues to fix before merging:Correctness
mlx_server.rs:559-565(DOWNLOAD_RETRY_BACKOFF) — the array has 5 entries (2s, 8s, 20s, 45s, 90s) but only 4 gaps exist between 5 attempts (attempt == DOWNLOAD_MAX_ATTEMPTSshort-circuits before the 5th sleep). The loop only ever indexesbackoff[0..=3], so the90sentry is dead code and the real worst-case cumulative wait is 75s (2+8+20+45), not the "~2m45s" claimed in the PR description and in the doc comment above the const ("attempts 1→2, 2→3, 3→4, 4→5" is 4 transitions but 5 values are listed). Either trim the array to 4 entries or bumpDOWNLOAD_MAX_ATTEMPTSto 6 so the 90s value is actually used — whichever matches the intended budget.mlx_server.rs:1266(retry_backoff_covers_all_gapstest) — assertsDOWNLOAD_RETRY_BACKOFF.len() >= DOWNLOAD_MAX_ATTEMPTS - 1, which is too loose to catch the off-by-one above (5 >= 4 passes even though the 5th entry is unreachable). Should assert exact equality (== DOWNLOAD_MAX_ATTEMPTS - 1) so a future attempt/backoff mismatch fails the test.Design / robustness (non-blocking, worth a note)
mlx_server.rs:559-565).auto_upgrade_runtimeruns on many machines against the same manifest/CDN host; a shared outage (5xx blip) means every client retries on the exact same 2s/8s/20s/45s cadence — a small thundering-herd risk. Not critical at current scale, but worth a+ rand jitterif this runs fleet-wide.transient(mlx_server.rs:508-515), on the assumption it's a truncated stream. If the server-side artifact itself is corrupted (not just a network truncation), this burns up to 5 full multi-hundred-MB re-downloads before giving up. Probably fine given the stated reasoning, but flagging since it's the one "retryable" case that isn't obviously network-transient.Style
Result<_, String>rather thananyhow::Result+.context(...), which conflicts with CLAUDE.md's Rust conventions — but this matches the file's pre-existing pattern throughout (not introduced by this PR), so not a blocker, just noting for awareness if this file is ever migrated toanyhow.No file-header, unwrap(), tracing-interpolation, or clippy 7-arg violations found — those all look clean.
Overall: solid feature, but please fix the backoff/attempts mismatch (and tighten the test) before merging — right now the shipped behavior doesn't match what the PR description promises.
Originally posted by: Akarsh-Hegde
Thanks — the backoff/attempts off-by-one is fixed in 18d9363d.
Correctness (fixed)
DOWNLOAD_RETRY_BACKOFFto exactly 4 entries (3s → 12s → 30s → 75s, ~2m total) so there's one wait per inter-attempt gap and no unreachable trailing value. You were right — the old 90s entry was dead and the true worst-case was 75s. Doc comment rewritten to the real schedule.retry_backoff_covers_all_gapsfrom>=toassert_eq!(DOWNLOAD_RETRY_BACKOFF.len(), DOWNLOAD_MAX_ATTEMPTS - 1), so a future attempt/backoff drift (or a re-introduced trailing entry) fails the test instead of passing.Non-blocking, acknowledged:
auto_upgrade_runtimepath (every machine on a shared ~6h cadence hitting the same CDN). I left it out of this fix to avoid pulling a randomness dep into a targeted correctness change; tracking it as a follow-up (cheap to add jitter derived from the machine's uid/hostname hash, norandneeded).transient: agreed it's the one non-network-obvious retryable case. Kept as-is per the truncated-stream reasoning, but noted — if we ever see a genuinely corrupt server-side artifact in the wild, capping checksum retries at 1–2 (vs the full 5) is the right escalation.Result<_, String>vsanyhow: matches the file's existing pattern; leaving consistent rather than half-migrating one function.Re the PR description's "~2m45s" — now corrected to ~2m in the code; I'll fix the PR body too.
Ticket changed by: Akarsh-Hegde