diff --git a/crates/llm/src/model_manager.rs b/crates/llm/src/model_manager.rs index 9cabe17..a59d9b6 100644 --- a/crates/llm/src/model_manager.rs +++ b/crates/llm/src/model_manager.rs @@ -276,16 +276,44 @@ where let _reservation = DownloadReservation::acquire(id)?; let dest = model_path(id); tokio::fs::create_dir_all(model_dir()).await?; + download_to(id.hf_url(), id.sha256(), &dest, on_progress).await +} +/// Inner driver split out of `download_model` so the +/// existing-file / SHA-mismatch / new-download decision can be +/// exercised by tests without hitting the hardcoded Hugging Face URLs +/// on `LlmModelId`. Behaviour: +/// 1. If `dest` already exists and its SHA matches — done, no network. +/// 2. If `dest` exists but the SHA mismatches — DO NOT delete; fall +/// through to `download_impl` which writes via `.part` and renames +/// atomically on success. Rev-1 reversibility kill (atomiser +/// 2026-05-12): the previous implementation called +/// `remove_file(&dest)` here before the network round-trip. A +/// network blip / power loss / disk-full between the unlink and +/// the eventual `rename` left users with neither the old +/// (corrupted-but-readable) model nor the new one — a 1.5–20 GB +/// redownload from scratch with no fallback. +/// 3. If `dest` doesn't exist — straight to `download_impl`. +async fn download_to( + url: &str, + expected_sha: &str, + dest: &Path, + on_progress: F, +) -> Result<(), DownloadError> +where + F: FnMut(u64, u64) + Send + 'static, +{ if dest.exists() { - let actual = sha256_file(&dest).await?; - if actual == id.sha256() { + let actual = sha256_file(dest).await?; + if actual == expected_sha { return Ok(()); } - tokio::fs::remove_file(&dest).await?; + // SHA mismatch: do NOT unlink. `download_impl` writes to a + // `.part` sibling and atomically renames over `dest` once the + // new payload verifies. On failure the user keeps the old + // file (even if "corrupt") rather than ending up with nothing. } - - download_impl(id.hf_url(), id.sha256(), &dest, on_progress).await + download_impl(url, expected_sha, dest, on_progress).await } async fn sha256_file(path: &Path) -> Result { @@ -483,4 +511,64 @@ mod tests { 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 + /// the user with neither the old nor the new model. + /// + /// We exercise `download_to` (the testable inner driver) with + /// an existing sentinel file at `dest` whose SHA does NOT match + /// the expected one, against a server that returns HTTP 500. + /// The function must fail; the destination must still exist + /// with its original contents. + #[tokio::test] + async fn download_failure_preserves_existing_file() { + let server = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = server.local_addr().unwrap(); + + let server_task = tokio::spawn(async move { + let (mut socket, _) = server.accept().await.unwrap(); + let mut buf = vec![0u8; 2048]; + let _ = socket.read(&mut buf).await.unwrap(); + let body = b"upstream blew up"; + let response = format!( + "HTTP/1.1 500 Internal Server Error\r\nContent-Length: {}\r\n\r\n", + body.len() + ); + socket.write_all(response.as_bytes()).await.unwrap(); + socket.write_all(body).await.unwrap(); + }); + + let dir = tempdir().unwrap(); + let dest = dir.path().join("fixture.gguf"); + // Sentinel "old model" file the user already had on disk. + tokio::fs::write(&dest, b"OLD").await.unwrap(); + + // Expect-sha is deliberately something the OLD file does NOT + // hash to, so the existing-file branch falls through to + // download_impl (the exact case the atomiser flagged). + let expected_sha = "0".repeat(64); + + download_to( + &format!("http://{addr}/fixture.gguf"), + &expected_sha, + &dest, + |_, _| {}, + ) + .await + .expect_err("500 response must fail the download"); + + assert!( + dest.exists(), + "download failure must leave the existing dest in place" + ); + let preserved = tokio::fs::read(&dest).await.unwrap(); + assert_eq!( + preserved, b"OLD", + "existing file contents must be untouched on failed download" + ); + + server_task.await.unwrap(); + } } diff --git a/crates/transcription/src/model_manager.rs b/crates/transcription/src/model_manager.rs index 51a2843..e8f7e4c 100644 --- a/crates/transcription/src/model_manager.rs +++ b/crates/transcription/src/model_manager.rs @@ -95,7 +95,21 @@ pub async fn download( match sha256_of_file(&dest) { Ok(actual) if actual.eq_ignore_ascii_case(file.sha256) => continue, Ok(_actual) => { - let _ = std::fs::remove_file(&dest); + // Rev-5 reversibility kill (atomiser 2026-05-12): + // the previous implementation called + // `remove_file(&dest)` here before the network + // round-trip. A network blip / power loss between + // the unlink and `rename(tmp, dest)` in + // `download_file` left users with neither the old + // (corrupt) model file nor a fresh one — Whisper + // models are 1.5–3 GB and the user is offline by + // hypothesis. + // + // We now leave the existing file in place. + // `download_file` writes to a `.part` sibling and + // only `rename`s over `dest` once the SHA matches. + // The bad file is atomically replaced on success; + // on failure the user keeps what they had. } Err(e) => { return Err(Error::DownloadFailed(format!( @@ -149,10 +163,33 @@ fn write_verified_manifest( let size = std::fs::metadata(dir.join(file.filename))?.len(); lines.push(format!("{}\t{}\t{}", file.filename, file.sha256, size)); } - std::fs::write( - verified_manifest_path(dir), - format!("{}\n", lines.join("\n")), - ) + let final_path = verified_manifest_path(dir); + let body = format!("{}\n", lines.join("\n")); + + // Rev-5 atomicity (atomiser 2026-05-12): the previous + // implementation called `fs::write(final_path, body)` directly, + // which truncates-then-writes. A power loss or kill mid-write + // leaves the manifest empty or partial; on next boot + // `verified_manifest_matches` reports `false` and the loader + // re-downloads the model even though all the GB-sized files are + // intact on disk. + // + // Write to a sibling `.tmp` first, fsync the data, then `rename` + // — POSIX `rename` is atomic within a directory, so a reader sees + // either the old manifest or the new one, never a partial. + let tmp_path = final_path.with_extension("tmp"); + { + use std::io::Write; + let mut tmp = std::fs::File::create(&tmp_path)?; + tmp.write_all(body.as_bytes())?; + tmp.flush()?; + // Best-effort fsync. If the platform/filesystem can't sync + // (e.g. some test runners on tmpfs), the rename is still + // atomic at the directory entry level, which is the property + // we actually care about for "no torn manifest". + let _ = tmp.sync_all(); + } + std::fs::rename(&tmp_path, &final_path) } /// Non-streaming SHA256 of a file on disk. Used by `download()` to @@ -616,4 +653,127 @@ mod tests { let part = dest.with_extension("bin.part"); assert!(!part.exists(), "failed hash must clean up the .part file"); } + + /// Rev-5 regression (atomiser 2026-05-12). The previous `download()` + /// loop called `remove_file(&dest)` on SHA mismatch BEFORE the + /// network call, so a failing redownload left the user with + /// neither the old (corrupt-but-readable) Whisper model nor a + /// fresh one — 1.5–3 GB redownload over an already-flaky network. + /// + /// `download_file` is the per-file primitive that gets invoked + /// after the outer-loop decides to redownload. It already writes + /// via `.part` and atomically renames. This test asserts the + /// invariant the fix relies on: when `download_file` fails (5xx + /// or SHA mismatch), an existing sentinel file at `dest` is left + /// in place. The outer-loop no longer deletes pre-emptively, so + /// this proves end-to-end: a failed redownload preserves the old + /// file. + #[tokio::test] + async fn download_file_failure_preserves_existing_dest_file() { + let addr = spawn_500_server().await; + + let dir = tempdir().unwrap(); + let dest = dir.path().join("fixture.bin"); + // Existing "old model" file the user already had on disk. + std::fs::write(&dest, b"OLD").unwrap(); + + let file = ModelFile { + filename: leak(dest.file_name().unwrap().to_string_lossy().into_owned()), + url: leak(format!("http://{addr}/fixture.bin")), + size: lumotia_core::types::Megabytes(0), + sha256: leak("0".repeat(64)), + }; + let id = ModelId::new("test-fixture"); + + let _ = download_file(&file, &dest, &id, &|_| ()) + .await + .expect_err("5xx must fail"); + + assert!( + dest.exists(), + "download failure must leave the existing dest in place" + ); + let preserved = std::fs::read(&dest).unwrap(); + assert_eq!( + preserved, b"OLD", + "existing file contents must be untouched on failed download" + ); + } + + /// Rev-5 atomic-manifest regression (atomiser 2026-05-12). The + /// previous `write_verified_manifest` called `fs::write` directly, + /// which truncates-then-writes. A power loss mid-write left an + /// empty or torn manifest; on next boot the loader couldn't match + /// the existing GB-sized model files and redownloaded everything. + /// + /// The fix writes to a sibling `.tmp` first then `rename`s — POSIX + /// `rename` is atomic. This test exercises that path: write a + /// valid manifest, then prove a stale `.tmp` left behind from a + /// crashed previous attempt does NOT survive a fresh write (the + /// rename replaces it), and that the final manifest matches. + #[test] + fn manifest_write_is_atomic() { + use lumotia_core::model_registry::{ + AccuracyTier, Engine, LanguageSupport, ModelEntry, ModelFile, SpeedTier, + }; + use lumotia_core::types::Megabytes; + use sha2::Digest; + + let dir = tempdir().unwrap(); + let file_path = dir.path().join("model.bin"); + std::fs::write(&file_path, b"payload").unwrap(); + + let entry = ModelEntry { + id: ModelId::new("test-manifest"), + engine: Engine::Whisper, + display_name: "test", + disk_size: Megabytes(0), + ram_required: Megabytes(0), + speed_tier: SpeedTier::Instant, + accuracy_tier: AccuracyTier::Great, + languages: LanguageSupport::EnglishOnly, + files: vec![ModelFile { + filename: leak( + file_path + .file_name() + .unwrap() + .to_string_lossy() + .into_owned(), + ), + url: "http://example.invalid/model.bin", + size: Megabytes(0), + sha256: leak(format!("{:x}", sha2::Sha256::digest(b"payload"))), + }], + description: "", + }; + + let manifest_path = verified_manifest_path(dir.path()); + let tmp_path = manifest_path.with_extension("tmp"); + + // Simulate a crashed previous write leaving a stale .tmp. + std::fs::write(&tmp_path, b"PARTIAL_GARBAGE").unwrap(); + assert!(tmp_path.exists()); + + write_verified_manifest(&entry, dir.path()).expect("write manifest"); + + assert!( + manifest_path.exists(), + "final manifest must exist after atomic write" + ); + assert!( + !tmp_path.exists(), + "stale .tmp must be removed by rename" + ); + + let body = std::fs::read_to_string(&manifest_path).unwrap(); + assert!(body.starts_with("version\t1")); + assert!( + body.ends_with('\n'), + "manifest must be flushed completely with terminating newline" + ); + assert!( + verified_manifest_matches(&entry, dir.path()), + "manifest written via tmp+rename must round-trip verify" + ); + } }