From db1ac051f2e9169e172d5f320536c3b4513d331f Mon Sep 17 00:00:00 2001 From: vicrodh <103399283+vicrodh@users.noreply.github.com> Date: Tue, 1 Sep 2026 20:12:36 -0600 Subject: [PATCH] fix(local-catalog): prune obsolete generations cx --- crates/Cargo.lock | 1 + crates/qbz-local-catalog/Cargo.toml | 1 + crates/qbz-local-catalog/src/bootstrap.rs | 136 +++++++++++++++++- .../qbz-local-catalog/src/bootstrap_tests.rs | 46 ++++++ crates/qbz-local-catalog/src/legacy.rs | 3 + crates/qbz-local-catalog/src/lib.rs | 4 +- 6 files changed, 188 insertions(+), 3 deletions(-) diff --git a/crates/Cargo.lock b/crates/Cargo.lock index 2912ee492..b2abfd156 100644 --- a/crates/Cargo.lock +++ b/crates/Cargo.lock @@ -4277,6 +4277,7 @@ dependencies = [ name = "qbz-local-catalog" version = "0.1.0" dependencies = [ + "log", "rusqlite", "rustix 1.1.4", "serde", diff --git a/crates/qbz-local-catalog/Cargo.toml b/crates/qbz-local-catalog/Cargo.toml index 8768529d5..4f0ee1c77 100644 --- a/crates/qbz-local-catalog/Cargo.toml +++ b/crates/qbz-local-catalog/Cargo.toml @@ -6,6 +6,7 @@ license.workspace = true repository.workspace = true [dependencies] +log.workspace = true rusqlite = { version = "0.40", features = ["backup", "bundled"] } serde.workspace = true serde_json.workspace = true diff --git a/crates/qbz-local-catalog/src/bootstrap.rs b/crates/qbz-local-catalog/src/bootstrap.rs index 60f0c8a44..7f9601981 100644 --- a/crates/qbz-local-catalog/src/bootstrap.rs +++ b/crates/qbz-local-catalog/src/bootstrap.rs @@ -1,4 +1,4 @@ -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; use std::fs::{self, File, OpenOptions}; use std::io::Write; use std::path::{Path, PathBuf}; @@ -106,6 +106,13 @@ pub struct BootstrapManifest { pub activated_at_unix_ms: u64, } +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct GenerationPruneReport { + pub removed_generations: usize, + pub reclaimed_bytes: u64, + pub failed_generations: usize, +} + #[derive(Debug, Clone, PartialEq, Eq)] pub enum FallbackReason { NoManifest, @@ -331,9 +338,136 @@ impl BootstrapLayout { } fs::rename(&temporary, &path)?; sync_directory(&self.data_dir)?; + self.prune_obsolete_generations(manifest); Ok(()) } + /// Retain only the active catalog and its immediate rollback generation. + /// + /// The manifest is the source of truth and is always published before this + /// maintenance runs. Cleanup is deliberately best-effort: Windows may + /// still have an older generation open briefly, and a failed unlink must + /// never make a successfully activated catalog look like a failed one. + /// A later startup or activation will retry any generation left behind. + pub fn prune_obsolete_generations( + &self, + manifest: &BootstrapManifest, + ) -> GenerationPruneReport { + let entries = match fs::read_dir(&self.data_dir) { + Ok(entries) => entries, + Err(error) => { + log::warn!( + "[local-catalog] generation cleanup could not scan {}: {error}", + self.data_dir.display() + ); + return GenerationPruneReport { + failed_generations: 1, + ..GenerationPruneReport::default() + }; + } + }; + let mut generations = BTreeSet::new(); + for entry in entries.flatten() { + let name = entry.file_name(); + let name = name.to_string_lossy(); + let generation = [".db", ".db-wal", ".db-shm"] + .into_iter() + .find_map(|suffix| parse_generation_name(&name, suffix)); + if let Some(generation) = generation { + generations.insert(generation); + } + } + // A schema rebuild may not be able to validate the previous manifest, + // leaving `previous_generation` empty even though the immediately + // preceding finalized database is still the safest rollback artifact. + let previous_generation = manifest.previous_generation.or_else(|| { + generations + .range(..manifest.active_generation) + .next_back() + .copied() + }); + let keep = [Some(manifest.active_generation), previous_generation] + .into_iter() + .flatten() + .collect::>(); + let stale = generations + .into_iter() + .filter(|generation| !keep.contains(generation)) + .collect::>(); + + let mut report = GenerationPruneReport::default(); + for generation in stale { + let database = self.generation_path(generation); + let mut failed = false; + let mut removed_any = false; + let database_bytes = fs::metadata(&database) + .map(|metadata| metadata.len()) + .unwrap_or(0); + let database_gone = match fs::remove_file(&database) { + Ok(()) => { + removed_any = true; + report.reclaimed_bytes = report.reclaimed_bytes.saturating_add(database_bytes); + true + } + Err(error) if error.kind() == std::io::ErrorKind::NotFound => true, + Err(error) => { + failed = true; + log::warn!( + "[local-catalog] generation cleanup could not remove {}: {error}", + database.display() + ); + false + } + }; + // If Windows still has the database open, leave its WAL/SHM pair + // untouched as well. The next maintenance pass retries the trio. + for path in database_gone + .then(|| { + [ + PathBuf::from(format!("{}-wal", database.display())), + PathBuf::from(format!("{}-shm", database.display())), + ] + }) + .into_iter() + .flatten() + { + let bytes = fs::metadata(&path) + .map(|metadata| metadata.len()) + .unwrap_or(0); + match fs::remove_file(&path) { + Ok(()) => { + removed_any = true; + report.reclaimed_bytes = report.reclaimed_bytes.saturating_add(bytes); + } + Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} + Err(error) => { + failed = true; + log::warn!( + "[local-catalog] generation cleanup could not remove {}: {error}", + path.display() + ); + } + } + } + if failed { + report.failed_generations += 1; + } else if removed_any { + report.removed_generations += 1; + } + } + if report.removed_generations > 0 || report.failed_generations > 0 { + log::info!( + "[local-catalog] generation cleanup: removed={} reclaimed_bytes={} failed={} retained_active={} retained_previous={:?}", + report.removed_generations, + report.reclaimed_bytes, + report.failed_generations, + manifest.active_generation, + previous_generation + ); + } + report + } + pub(crate) fn next_building_generation(&self, active_generation: Option) -> Result { let mut maximum_final = active_generation.unwrap_or(0); let mut building = Vec::new(); diff --git a/crates/qbz-local-catalog/src/bootstrap_tests.rs b/crates/qbz-local-catalog/src/bootstrap_tests.rs index ffa31c0a6..7f58d2f3d 100644 --- a/crates/qbz-local-catalog/src/bootstrap_tests.rs +++ b/crates/qbz-local-catalog/src/bootstrap_tests.rs @@ -1,5 +1,6 @@ use std::collections::{HashMap, HashSet}; use std::fs; +use std::path::PathBuf; use std::sync::atomic::AtomicBool; use std::time::Instant; @@ -182,6 +183,51 @@ fn building_file_is_never_active_and_previous_generation_survives() { assert_eq!(layout.read_manifest().unwrap().active_generation, 1); } +#[test] +fn activation_prunes_generations_older_than_the_rollback_generation() { + let temp = tempdir().unwrap(); + let layout = BootstrapLayout::new(temp.path()); + let key = source(SourceKind::Local, "library"); + + for generation in 1..=3 { + let snapshot = format!("v{generation}"); + let current_probe = probe(&temp.path().join("library.db"), &key, &snapshot, generation); + let (mut session, _) = layout + .prepare(&[current_probe.clone()], Some(u64::MAX)) + .unwrap(); + apply_range(&mut session, &key, &snapshot, 0, generation, generation); + session.activate(&[current_probe]).unwrap(); + + if generation == 2 { + fs::write( + PathBuf::from(format!("{}-wal", layout.generation_path(1).display())), + b"stale wal", + ) + .unwrap(); + fs::write( + PathBuf::from(format!("{}-shm", layout.generation_path(1).display())), + b"stale shm", + ) + .unwrap(); + fs::write( + temp.path().join("local_catalog-v1-g1.db.backup"), + b"keep me", + ) + .unwrap(); + } + } + + let manifest = layout.read_manifest().unwrap(); + assert_eq!(manifest.active_generation, 3); + assert_eq!(manifest.previous_generation, Some(2)); + assert!(!layout.generation_path(1).exists()); + assert!(!PathBuf::from(format!("{}-wal", layout.generation_path(1).display())).exists()); + assert!(!PathBuf::from(format!("{}-shm", layout.generation_path(1).display())).exists()); + assert!(layout.generation_path(2).is_file()); + assert!(layout.generation_path(3).is_file()); + assert!(temp.path().join("local_catalog-v1-g1.db.backup").is_file()); +} + #[test] fn interrupted_bootstrap_resumes_at_ten_fifty_and_ninety_nine_percent() { const TOTAL: u64 = 1_001; diff --git a/crates/qbz-local-catalog/src/legacy.rs b/crates/qbz-local-catalog/src/legacy.rs index 2bc125a7b..175c80d13 100644 --- a/crates/qbz-local-catalog/src/legacy.rs +++ b/crates/qbz-local-catalog/src/legacy.rs @@ -92,6 +92,9 @@ pub fn bootstrap_legacy_caches_at_with_progress( ) -> Result { let layout = BootstrapLayout::new(&locations.catalog_dir); if let ActiveCatalog::Ready { catalog, manifest } = layout.open_active() { + // Also performs the one-time migration for profiles created by builds + // that published generations but never removed obsolete ones. + layout.prune_obsolete_generations(&manifest); return Ok(BootstrapOutcome::Activated { generation: manifest.active_generation, track_count: catalog.stats()?.track_count, diff --git a/crates/qbz-local-catalog/src/lib.rs b/crates/qbz-local-catalog/src/lib.rs index bd9d7fc71..3ac01edc2 100644 --- a/crates/qbz-local-catalog/src/lib.rs +++ b/crates/qbz-local-catalog/src/lib.rs @@ -13,8 +13,8 @@ mod schema; pub use bootstrap::{ ActiveCatalog, BootstrapBatch, BootstrapLayout, BootstrapManifest, BootstrapOutcome, - BootstrapProgress, BootstrapSession, FallbackReason, PreflightReport, SourceCheckpoint, - SourceProbe, BOOTSTRAP_BATCH_ROWS, + BootstrapProgress, BootstrapSession, FallbackReason, GenerationPruneReport, PreflightReport, + SourceCheckpoint, SourceProbe, BOOTSTRAP_BATCH_ROWS, }; pub use catalog::{ normalize_artist_key, normalize_sort_key, Catalog, CatalogStats, IntegrityReport, QueryMetrics,