Card data update: release reference.db before replacing it

Windows refuses to replace a file SQLite has open, so every update after the first failed at the final swap with "Access is denied". Building and installing are now separate: the app stays usable during the build, then closes the catalog, swaps the file and reopens under the state lock. The swap retries briefly in case a virus scanner is holding the new file, and a failed one puts the previous data back in service and removes the temp file. Importing a card database shares the same swap.

crates/mtgc-core/src/bin/ingest.rs

@@ -15,9 +15,14 @@ fn main() -> ExitCode {
 
     eprintln!("Building reference db at {}", dest.display());
     let cancel = CancelToken::new();
-    match mtgc_core::ingest::build_reference_db(&dest, &cancel, |p| {
+    let built = mtgc_core::ingest::build_reference_db(&dest, &cancel, |p| {
         eprintln!("  [{}] {:.1}% {}", p.stage, p.overall_percent, p.message)
-    }) {
+    })
+    .and_then(|(tmp, stats)| {
+        mtgc_core::ingest::install_reference_db(&tmp, &dest)?;
+        Ok(stats)
+    });
+    match built {
         Ok(stats) => {
             eprintln!(
                 "Done: {} oracle cards, {} printings.",

crates/mtgc-core/src/ingest.rs

@@ -9,9 +9,10 @@
 //! memory.
 
 use std::io::{BufRead, BufReader, Read};
-use std::path::Path;
+use std::path::{Path, PathBuf};
 use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::Arc;
+use std::time::Duration;
 
 use flate2::read::GzDecoder;
 use rusqlite::{params, Connection};
@@ -139,31 +140,55 @@ fn overall_percent(stages: &[IngestStageInfo], active: &str, local_fraction: f64
     (acc * 100.0).min(100.0)
 }
 
-/// Download the latest Scryfall bulk data and (re)build `reference.db` at
-/// `dest`. Builds into a temp file alongside `dest` and only replaces it on
-/// full success, so a cancelled or failed update never destroys a working
-/// reference db.
+/// Download the latest Scryfall bulk data and build a new `reference.db` in a
+/// temp file alongside `dest`, returning its path. `dest` itself is untouched,
+/// so a cancelled or failed update never destroys a working reference db, and
+/// it can stay in use for the whole build. Finish with [`install_reference_db`].
 pub fn build_reference_db(
     dest: &Path,
     cancel: &CancelToken,
     mut progress: impl FnMut(IngestProgress),
-) -> Result<IngestStats> {
+) -> Result<(PathBuf, IngestStats)> {
     let tmp = dest.with_extension("db.tmp");
-    let result = build_into(&tmp, cancel, &mut progress);
-    match &result {
-        Ok(_) => {
-            // Rename straight over `dest`: POSIX rename replaces the
-            // destination atomically, so there is never a moment with no
-            // reference.db. (Deleting `dest` first would open a window where a
-            // process kill — far likelier on Android, which reaps backgrounded
-            // apps — leaves the user with no card data at all.)
-            std::fs::rename(&tmp, dest)?;
-        }
-        Err(_) => {
+    match build_into(&tmp, cancel, &mut progress) {
+        Ok(stats) => Ok((tmp, stats)),
+        Err(e) => {
             let _ = std::fs::remove_file(&tmp);
+            Err(e)
+        }
+    }
+}
+
+/// Times to try replacing the reference db, and the wait between tries.
+const INSTALL_ATTEMPTS: u32 = 5;
+const INSTALL_RETRY_DELAY: Duration = Duration::from_millis(200);
+
+/// Move a built reference db over `dest`, consuming `built` even on failure.
+///
+/// Anything with `dest` open must let go first: Windows refuses to replace a
+/// file SQLite has open. A virus scanner can also hold a file this size for a
+/// moment after it's written, and giving up here would throw away a download
+/// of several minutes, hence the retries.
+pub fn install_reference_db(built: &Path, dest: &Path) -> std::io::Result<()> {
+    let mut attempt = 1;
+    loop {
+        // Rename straight over `dest`: rename replaces the destination
+        // atomically, so there is never a moment with no reference.db.
+        // (Deleting `dest` first would open a window where a process kill — far
+        // likelier on Android, which reaps backgrounded apps — leaves the user
+        // with no card data at all.)
+        match std::fs::rename(built, dest) {
+            Ok(()) => return Ok(()),
+            Err(_) if attempt < INSTALL_ATTEMPTS => {
+                attempt += 1;
+                std::thread::sleep(INSTALL_RETRY_DELAY);
+            }
+            Err(e) => {
+                let _ = std::fs::remove_file(built);
+                return Err(e);
+            }
         }
     }
-    result
 }
 
 fn build_into(
@@ -1483,4 +1508,33 @@ mod tests {
         assert_eq!(fetch("p-null"), 0.0);
         assert_eq!(fetch("p-noprices"), 0.0);
     }
+
+    #[test]
+    fn installing_replaces_the_reference_db() {
+        let dir = tempfile::tempdir().unwrap();
+        let dest = dir.path().join("reference.db");
+        let built = dest.with_extension("db.tmp");
+        std::fs::write(&dest, b"old").unwrap();
+        std::fs::write(&built, b"new").unwrap();
+
+        install_reference_db(&built, &dest).unwrap();
+
+        assert_eq!(std::fs::read(&dest).unwrap(), b"new");
+        assert!(!built.exists());
+    }
+
+    #[test]
+    fn a_failed_install_cleans_up_and_keeps_what_was_there() {
+        let dir = tempfile::tempdir().unwrap();
+        // A folder can't be renamed over, on any platform.
+        let dest = dir.path().join("reference.db");
+        std::fs::create_dir(&dest).unwrap();
+        let built = dest.with_extension("db.tmp");
+        std::fs::write(&built, b"new").unwrap();
+
+        install_reference_db(&built, &dest).unwrap_err();
+
+        assert!(dest.is_dir());
+        assert!(!built.exists());
+    }
 }

src-tauri/src/commands.rs

@@ -127,15 +127,15 @@ pub async fn update_card_data(app: AppHandle, db: Db<'_>) -> Res<Status> {
     .map_err(|e| e.to_string())?;
 
     db.lock().unwrap().ingest_cancel = None;
-    let stats = crate::logged("update_card_data", result.map_err(|e| e.to_string()))?;
+    let (built, stats) = crate::logged("update_card_data", result.map_err(|e| e.to_string()))?;
     crate::mlog!(
-        "update_card_data: ok ({} oracle cards, {} printings)",
+        "update_card_data: built ({} oracle cards, {} printings)",
         stats.oracle_cards,
         stats.printings
     );
 
     let mut s = db.lock().unwrap();
-    crate::logged("update_card_data reopen", s.reopen())?;
+    crate::logged("update_card_data install", s.replace_reference(&built))?;
     Ok(status_of(&s))
 }
 
@@ -182,16 +182,7 @@ pub fn import_reference_db(app: AppHandle, db: Db, path: String) -> Res<Status>
     })?;
     crate::mlog!("import_reference_db: source holds {cards} oracle cards");
 
-    // Release the handles on the file we're about to replace.
-    s.close();
-
-    if let Err(e) = std::fs::rename(&tmp, &dest) {
-        let _ = std::fs::remove_file(&tmp);
-        let _ = s.reopen(); // put the previous db back into service
-        return Err(format!("Couldn't install that card database: {e}"));
-    }
-
-    s.reopen()?;
+    s.replace_reference(&tmp)?;
     Ok(status_of(&s))
 }
 

src-tauri/src/state.rs

@@ -434,13 +434,26 @@ impl AppState {
     }
 
     /// Drop the open catalog (and its name index), releasing the handles on both
-    /// db files. Call before replacing `reference.db` on disk so nothing is
-    /// still reading the file being swapped out.
-    pub fn close(&mut self) {
+    /// db files.
+    fn close(&mut self) {
         self.catalog = None;
         self.index = None;
     }
 
+    /// Put a newly built reference db into service, consuming `built`.
+    ///
+    /// The catalog has `reference.db` open, and Windows won't replace an open
+    /// file, so it lets go first. Holding `&mut self` keeps every command out
+    /// until the catalog is back.
+    pub fn replace_reference(&mut self, built: &Path) -> Result<(), String> {
+        self.close();
+        let installed = mtgc_core::ingest::install_reference_db(built, &self.reference_path);
+        // Either way there's a db to reopen: if the swap failed, the previous one.
+        let reopened = self.reopen();
+        installed.map_err(|e| format!("Couldn't install the new card data: {e}"))?;
+        reopened
+    }
+
     /// (Re)open the catalog + rebuild the name index. Only opens when a reference
     /// db exists — attaching a non-existent file would create an empty (tableless)
     /// db and make every query fail.
@@ -513,6 +526,51 @@ mod tests {
         }
     }
 
+    /// A reference db holding one card called `name`, at `path`.
+    fn reference_with(path: &Path, name: &str) {
+        mtgc_core::db::open_reference_for_build(path)
+            .unwrap()
+            .execute_batch(&format!(
+                "INSERT INTO oracle_cards(oracle_id, name, name_lower) VALUES ('{name}', '{name}', '{name}');"
+            ))
+            .unwrap();
+    }
+
+    #[test]
+    fn replacing_the_reference_reopens_on_the_new_file() {
+        let dir = scratch("replace-reference");
+        reference_with(&dir.join("reference.db"), "Old");
+        let mut s = AppState::new(dir.clone());
+        assert!(s.catalog.is_some());
+
+        let built = dir.join("reference.db.tmp");
+        reference_with(&built, "New");
+        s.replace_reference(&built).unwrap();
+
+        assert_eq!(s.index().unwrap().len(), 1);
+        assert_eq!(s.index().unwrap().search("New", 10).len(), 1);
+        assert!(s.catalog.is_some());
+        assert!(!built.exists());
+        drop(s);
+        std::fs::remove_dir_all(&dir).unwrap();
+    }
+
+    #[test]
+    fn a_failed_replacement_keeps_the_previous_reference_in_service() {
+        let dir = scratch("replace-reference-failed");
+        reference_with(&dir.join("reference.db"), "Old");
+        let mut s = AppState::new(dir.clone());
+
+        let missing = dir.join("reference.db.tmp");
+        let err = s.replace_reference(&missing).unwrap_err();
+
+        assert!(err.contains("Couldn't install the new card data"), "got: {err}");
+        assert_eq!(s.index().unwrap().search("Old", 10).len(), 1);
+        assert!(s.catalog.is_some());
+        drop(s);
+        std::fs::remove_dir_all(&dir).unwrap();
+    }
+
     #[test]
     fn every_setting_round_trips() {
         let dir = scratch("round-trip");