agent: code-atomiser-fix — atomic model download + manifest (Rev-1, Rev-5)

Two reversibility kills in the model-download path both followed the
same pattern: SHA mismatch on an existing file triggered
`remove_file(&dest)` BEFORE the network round-trip. A network blip /
power loss between the unlink and the eventual `rename(.part, dest)`
left users with neither the old (corrupt-but-readable) model nor a
fresh one — 1.5-20 GB redownload from scratch with no fallback.

Rev-1 (crates/llm/src/model_manager.rs):
  - Extract the existing-file decision into `download_to`, drop the
    pre-emptive unlink. `download_impl` already writes via `.part`
    and atomically renames; the rename overwrites on success and
    leaves dest untouched on failure.
  - Regression test `download_failure_preserves_existing_file` plants
    a sentinel "OLD" file at dest, points at a 500-returning server,
    and asserts dest still exists with original contents after the
    failed download.

Rev-5 (crates/transcription/src/model_manager.rs):
  - Drop the pre-emptive unlink in the outer `download()` SHA-mismatch
    branch. Same atomic rename via `download_file`.
  - Make `write_verified_manifest` atomic: write to `.tmp`, fsync,
    rename. Previous direct `fs::write` truncates-then-writes, so a
    crash mid-write left an empty/torn manifest and triggered a full
    GB-sized redownload on next boot.
  - `download_file_failure_preserves_existing_dest_file` and
    `manifest_write_is_atomic` regression tests added.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

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<F>(
+    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<String, io::Error> {
@@ -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();
+    }
 }

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"
+        );
+    }
 }