From 93a7165dac530c662a7f35c1c62d742758964560 Mon Sep 17 00:00:00 2001
From: Jake <jake@corbel.consulting>
Date: Wed, 22 Apr 2026 09:09:09 +0100
Subject: [PATCH] fix(cr-2026-04-22): reject HTTP 4xx/5xx on non-resume
 download path
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 crates/transcription/src/model_manager.rs | 70 +++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/crates/transcription/src/model_manager.rs b/crates/transcription/src/model_manager.rs
index aa21d09..0c339c5 100644
--- a/crates/transcription/src/model_manager.rs
+++ b/crates/transcription/src/model_manager.rs
@@ -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();