From 31e3f5a09950449433f9a82ca76f9f0f604ebe82 Mon Sep 17 00:00:00 2001 From: Jake Date: Thu, 14 May 2026 19:23:24 +0100 Subject: [PATCH] =?UTF-8?q?agent:=20lumotia=20=E2=80=94=20Phase=20B.3=20un?= =?UTF-8?q?link=20.part=20on=20ResumeUnsupported=20so=20retry=20can=20reco?= =?UTF-8?q?ver?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase B.3 audit of commit 9f67ab2 (atomic model download + manifest — Rev-1, Rev-5). Existing coverage is solid: the transcription-side download_file has fixture tests for resume-and-verify, restart-on-200, SHA-mismatch cleanup, 5xx rejection, Rev-1 preserve-existing-file, and the Rev-5 manifest tmp+rename atomicity. The llm-side download_impl has resume-and-verify and the Rev-1 preserve-existing-file regression. One real residual found in crates/llm/src/model_manager.rs that the original commit did not close. When a stale .part exists (resume_from > 0) and the server returns a 200 full-body response to a Range request, download_impl returns DownloadError::ResumeUnsupported without unlinking the .part. Every subsequent download_model() call computes the same resume_from > 0, sends the same Range request, gets the same 200, and fails the same way — the download is wedged until the user manually invokes delete_model(). That is itself a reversibility kill in the same family as Rev-1: stale partial state stuck on disk, no automatic recovery, the user has to discover an out-of-band command to escape. The transcription-side download_file handles this case by treating 200-on-resume as a fresh-start (line 268: "Server ignored our Range header — treat as fresh start"). The llm-side does not have an analogous restart code path, but the simpler fix is sufficient: unlink the .part before returning ResumeUnsupported. The next call sees resume_from = 0, sends no Range header, the server returns 200, and download_impl writes the new payload into a fresh .part and renames atomically over dest. Single retry recovers. Fix: * crates/llm/src/model_manager.rs: - download_impl: tokio::fs::remove_file(&tmp).await.ok() before returning ResumeUnsupported, with a comment that names this as a Phase B.3 audit residual and explains the wedge scenario. - New test resume_unsupported_unlinks_part_so_retry_starts_fresh — spins a server that ignores Range and returns 200, plants a sentinel .part, asserts ResumeUnsupported AND .part removed AND dest not written. Verification: * cargo test -p lumotia-llm --lib model_manager → 5/5 pass including the new test. * cargo fmt --check → clean. * cargo clippy -p lumotia-llm --all-targets -- -D warnings → clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/llm/src/model_manager.rs | 82 +++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) 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