diff --git a/crates/llm/src/model_manager.rs b/crates/llm/src/model_manager.rs index a59d9b6..16ba50d 100644 --- a/crates/llm/src/model_manager.rs +++ b/crates/llm/src/model_manager.rs @@ -364,6 +364,18 @@ where .await .map_err(|e| DownloadError::Http(e.to_string()))?; if resume_from > 0 && response.status() != reqwest::StatusCode::PARTIAL_CONTENT { + // Server downgraded from Range-aware to full-body 200 (typically a + // mirror / CDN that advertises `Accept-Ranges` but doesn't honour a + // mid-stream resume). The existing `.part` bytes are stale — they + // cannot be stitched onto a fresh 200 stream. Unlink them BEFORE + // returning so the next `download_model()` call starts from + // `resume_from = 0` and succeeds. Without this unlink the user is + // wedged: every retry sends the same Range header, the server + // returns 200 again, and `ResumeUnsupported` fires forever until + // the user manually calls `delete_model()`. That is itself a + // reversibility kill in the same family as Rev-1 (atomiser + // 2026-05-12); fixed in Phase B.3 audit. + tokio::fs::remove_file(&tmp).await.ok(); return Err(DownloadError::ResumeUnsupported); } if !response.status().is_success() && response.status() != reqwest::StatusCode::PARTIAL_CONTENT @@ -512,6 +524,76 @@ mod tests { server_task.await.unwrap(); } + /// Phase B.3 audit residual (2026-05-14). The original Rev-1 fix + /// stopped the pre-emptive unlink of `dest` on SHA mismatch, but it + /// did NOT clean up `.part` when `download_impl` returned + /// `ResumeUnsupported`. That meant a transient mirror downgrade + /// (server returns 200 to a Range request) left a stale `.part` on + /// disk that every subsequent retry kept feeding back into the same + /// failing Range request — wedged until the user manually called + /// `delete_model()`. Same reversibility-kill family as Rev-1. + /// + /// We spin a server that ignores the Range header and returns 200 + /// with full body. With a pre-existing `.part` the call must fail + /// with `ResumeUnsupported` AND the stale `.part` must be gone, so + /// a follow-up call would compute `resume_from = 0` and start + /// fresh. + #[tokio::test] + async fn resume_unsupported_unlinks_part_so_retry_starts_fresh() { + let body = b"fresh full body returned by server ignoring Range header".to_vec(); + let expected_sha = format!("{:x}", Sha256::digest(&body)); + + let server = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = server.local_addr().unwrap(); + let content = body.clone(); + + let server_task = tokio::spawn(async move { + let (mut socket, _) = server.accept().await.unwrap(); + let mut request = vec![0u8; 2048]; + let _ = socket.read(&mut request).await.unwrap(); + // Deliberately ignore Range header and return 200 with the + // full body — the case the downloader must recover from + // without leaving a stuck `.part`. + let response = format!( + "HTTP/1.1 200 OK\r\nContent-Length: {}\r\n\r\n", + content.len() + ); + socket.write_all(response.as_bytes()).await.unwrap(); + socket.write_all(&content).await.unwrap(); + }); + + let dir = tempdir().unwrap(); + let dest = dir.path().join("fixture.gguf"); + let part = dest.with_extension("gguf.part"); + // Pretend a previous interrupted attempt left 10 stale bytes. + tokio::fs::write(&part, b"STALEBYTES").await.unwrap(); + assert!(part.exists()); + + let err = download_impl( + &format!("http://{addr}/fixture.gguf"), + &expected_sha, + &dest, + |_, _| {}, + ) + .await + .expect_err("server ignoring Range must surface ResumeUnsupported"); + + assert!( + matches!(err, DownloadError::ResumeUnsupported), + "expected ResumeUnsupported, got: {err:?}" + ); + assert!( + !part.exists(), + "ResumeUnsupported must unlink .part so the next attempt starts fresh" + ); + assert!( + !dest.exists(), + "dest must not have been written — only the unlink should run" + ); + + server_task.await.unwrap(); + } + /// Rev-1 regression (atomiser 2026-05-12). Before the fix the /// SHA-mismatch path in `download_model` deleted the existing /// file BEFORE the network call. A failing download then left