From c527e5d6f7d599272f1cb357f3834964ac765bd4 Mon Sep 17 00:00:00 2001 From: Garrett Mills Date: Fri, 30 Oct 2026 20:15:34 -0500 Subject: [PATCH] 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 | 9 ++- crates/mtgc-core/src/ingest.rs | 90 ++++++++++++++++++++++++------ src-tauri/src/commands.rs | 17 ++---- src-tauri/src/state.rs | 64 ++++++++++++++++++++- 4 files changed, 144 insertions(+), 36 deletions(-) diff --git a/crates/mtgc-core/src/bin/ingest.rs b/crates/mtgc-core/src/bin/ingest.rs index aa5ddee..4994e04 100644 --- a/crates/mtgc-core/src/bin/ingest.rs +++ b/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.", diff --git a/crates/mtgc-core/src/ingest.rs b/crates/mtgc-core/src/ingest.rs index 38a994b..a8adf8e 100644 --- a/crates/mtgc-core/src/ingest.rs +++ b/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 { +) -> 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()); + } } diff --git a/src-tauri/src/commands.rs b/src-tauri/src/commands.rs index e86d61f..8bb5a44 100644 --- a/src-tauri/src/commands.rs +++ b/src-tauri/src/commands.rs @@ -127,15 +127,15 @@ pub async fn update_card_data(app: AppHandle, db: Db<'_>) -> Res { .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 })?; 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)) } diff --git a/src-tauri/src/state.rs b/src-tauri/src/state.rs index fd66f28..33c6a64 100644 --- a/src-tauri/src/state.rs +++ b/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");