fix(cr-2026-04-22): reject HTTP 4xx/5xx on non-resume download path

MAJOR from the 2026-04-22 review (model_manager.rs:161-262): reqwest
does not return Err on 4xx/5xx by default. The resume branch
validated 206/200 and errored on anything else, but the non-resume
branch skipped the status check entirely — a 404 or 500 body was
streamed into .part and atomically renamed over the destination as
if it were the model file. For models without a sha256 declared,
this failure mode was silent and catastrophic (the engine would
crash loading an HTML error page as GGML on next launch).

Adds an is_success() check in the non-resume branch: any non-2xx
returns KonError::DownloadFailed with the HTTP status in the
message, and (importantly) we return before File::create so no
.part file is left behind.

Test: spawn_500_server that responds 500 to any request; a fresh
(no .part, no sha256) download must Err with "HTTP 500" and leave
neither .part nor dest on disk.
This commit is contained in:
2026-04-22 09:09:09 +01:00
parent b665754560
commit 93a7165dac

View File

@@ -168,6 +168,12 @@ async fn download_file(
// full file on top of our partial bytes (which would produce a
// corrupt result), restart cleanly. This mirrors the kon-llm
// ResumeUnsupported branch — item #8 of the brief.
//
// For the non-resume path, we still have to validate the status:
// reqwest does not error on 4xx/5xx by default, so without this
// check a 404 or 500 would be streamed into `.part` and renamed
// over the destination as if the download succeeded
// (2026-04-22 review MAJOR).
let actually_resuming = if resuming {
match response.status().as_u16() {
206 => true,
@@ -183,6 +189,13 @@ async fn download_file(
}
}
} else {
if !response.status().is_success() {
return Err(KonError::DownloadFailed(format!(
"download returned HTTP {} for {}",
response.status(),
file.filename
)));
}
false
};
@@ -466,6 +479,63 @@ mod tests {
assert!(!part.exists(), ".part → dest rename must run after restart");
}
/// Always returns HTTP 500 with a short error body. Used to verify
/// the non-resume download path validates status codes rather than
/// writing error bodies into `.part` and renaming them over the
/// destination.
async fn spawn_500_server() -> std::net::SocketAddr {
let listener = TcpListener::bind("127.0.0.1:0").await.unwrap();
let addr = listener.local_addr().unwrap();
tokio::spawn(async move {
let (mut socket, _) = listener.accept().await.unwrap();
let mut buf = vec![0u8; 2048];
let _ = socket.read(&mut buf).await.unwrap();
let body = b"internal error";
let response = format!(
"HTTP/1.1 500 Internal Server Error\r\n\
Content-Length: {}\r\n\r\n",
body.len()
);
socket.write_all(response.as_bytes()).await.unwrap();
socket.write_all(body).await.unwrap();
});
addr
}
#[tokio::test]
async fn download_file_rejects_5xx_on_non_resume_path() {
// Regression for the 2026-04-22 review: reqwest does not
// auto-error on 4xx/5xx, and the non-resume branch previously
// streamed any status' body into `.part` and renamed it over
// the destination.
let addr = spawn_500_server().await;
let dir = tempdir().unwrap();
let dest = dir.path().join("fixture.bin");
let part = dest.with_extension("bin.part");
let file = ModelFile {
filename: leak(dest.file_name().unwrap().to_string_lossy().into_owned()),
url: leak(format!("http://{addr}/fixture.bin")),
size: kon_core::types::Megabytes(0),
sha256: None,
};
let id = ModelId::new("test-fixture");
let err = download_file(&file, &dest, &id, &|_| ())
.await
.expect_err("5xx must fail");
let msg = err.to_string();
assert!(
msg.contains("HTTP 500"),
"error should name the HTTP status, got: {msg}"
);
assert!(!dest.exists(), "5xx must not leave a destination file");
assert!(!part.exists(), "5xx must not leave a .part file");
}
#[tokio::test]
async fn download_file_fails_on_sha_mismatch_and_cleans_part_file() {
let body = b"speech-to-text fixture body".to_vec();