From 37631ac87bc13abc81a6af6b5a881806099e7376 Mon Sep 17 00:00:00 2001 From: adstep Date: Sat, 6 Jun 2026 13:46:07 -0700 Subject: [PATCH] fix(songs): detect added/removed files on Reload Songs/Courses Default FastLoad=1 made the song scan trust the cache without verifying the song folder's directory hash. A song missing its music file is cached with music_path: None, which the path-existence check treats as present, so adding the file later was never detected on reload. It only recovered after renaming a folder (new cache key) or wiping the cache. An explicit Reload Songs/Courses now always verifies cache freshness, independent of the FastLoad startup toggle: process_song takes a force_fresh flag (verify_freshness = force_fresh || !fastload), threaded through load_pack_scans / scan_and_load_songs_impl / reload_song_dirs_impl. A new reload_all_songs_with_progress_counts (force-fresh) backs both full-reload call sites; startup stays on the fast path. The directory-hash check is symmetric, so additions and removals (including delete-and-re-add) are caught. Closes #493 --- src/game/parsing/simfile.rs | 10 ++++++-- src/game/parsing/simfile/cache.rs | 42 +++++++++++++++++++++++++++++++ src/game/parsing/simfile/scan.rs | 38 +++++++++++++++++++++------- src/screens/options/reload.rs | 2 +- src/screens/select_music.rs | 2 +- 5 files changed, 81 insertions(+), 13 deletions(-) diff --git a/src/game/parsing/simfile.rs b/src/game/parsing/simfile.rs index 5c1edb610..46f97e186 100644 --- a/src/game/parsing/simfile.rs +++ b/src/game/parsing/simfile.rs @@ -30,7 +30,8 @@ mod scan; pub(crate) use scan::collect_song_scan_roots; pub use scan::{ - reload_song_dirs_with_progress_counts, scan_and_load_songs, scan_and_load_songs_with_progress, + reload_all_songs_with_progress_counts, reload_song_dirs_with_progress_counts, + scan_and_load_songs, scan_and_load_songs_with_progress, scan_and_load_songs_with_progress_counts, }; @@ -1291,6 +1292,7 @@ fn process_song( simfile_path: PathBuf, fastload: bool, cachesongs: bool, + force_fresh: bool, global_offset_seconds: f32, ) -> Result<(SongData, bool), String> { let cache_path = if fastload || cachesongs { @@ -1299,10 +1301,14 @@ fn process_song( None }; + // Explicit reloads force freshness verification so on-disk changes (e.g. a music + // file that was missing at first scan and later added) are detected, even when the + // fastload startup shortcut would otherwise trust the cache without checking. + let verify_freshness = force_fresh || !fastload; let allow_cache_read = fastload || cachesongs; if allow_cache_read && let Some(cp) = cache_path.as_deref() - && let Some(song_data) = cache::load_song_from_cache(&simfile_path, cp, !fastload) + && let Some(song_data) = cache::load_song_from_cache(&simfile_path, cp, verify_freshness) { return Ok((song_data, true)); } diff --git a/src/game/parsing/simfile/cache.rs b/src/game/parsing/simfile/cache.rs index d9fa16109..7ca5435e1 100644 --- a/src/game/parsing/simfile/cache.rs +++ b/src/game/parsing/simfile/cache.rs @@ -471,4 +471,46 @@ mod tests { assert!(load_cached_song_for_gameplay(&simfile, &cache_path, false).is_some()); let _ = fs::remove_dir_all(root); } + + // Regression for #493: a song folder that was missing its music file is cached with + // `music_path: None`, which the path-existence check reports as "present". Only the + // directory hash notices the music file once it is added, so an explicit reload (which + // verifies freshness) must invalidate the stale cache, while the fastload startup path + // intentionally keeps trusting it. + #[test] + fn reload_detects_added_music_file_when_verifying() { + let root = test_dir("reload-added-music-verify"); + let simfile = root.join("song.ssc"); + let cache_path = root.join("cache.bin"); + fs::write(&simfile, b"#TITLE:Old;#MUSIC:song.ogg;").unwrap(); + write_song_cache(&cache_path, &cached_song(&simfile), 0.0); + + // Music file was missing at scan time; now the user adds it. + fs::write(root.join("song.ogg"), b"audio bytes").unwrap(); + + // Explicit reload verifies freshness -> stale cache is rejected and the song reparses. + assert!(load_song_from_cache(&simfile, &cache_path, true).is_none()); + // Fastload startup keeps trusting the cache (fast launch, detection deferred to reload). + assert!(load_song_from_cache(&simfile, &cache_path, false).is_some()); + let _ = fs::remove_dir_all(root); + } + + // The directory-hash check is symmetric: removing a file from a cached folder must also + // invalidate the cache when verifying freshness (e.g. delete-and-re-add workflows). + #[test] + fn reload_detects_removed_file_when_verifying() { + let root = test_dir("reload-removed-file-verify"); + let simfile = root.join("song.ssc"); + let music = root.join("song.ogg"); + let cache_path = root.join("cache.bin"); + fs::write(&simfile, b"#TITLE:Old;").unwrap(); + fs::write(&music, b"audio bytes").unwrap(); + write_song_cache(&cache_path, &cached_song(&simfile), 0.0); + + fs::remove_file(&music).unwrap(); + + assert!(load_song_from_cache(&simfile, &cache_path, true).is_none()); + assert!(load_song_from_cache(&simfile, &cache_path, false).is_some()); + let _ = fs::remove_dir_all(root); + } } diff --git a/src/game/parsing/simfile/scan.rs b/src/game/parsing/simfile/scan.rs index de647a1a0..db7dd4b82 100644 --- a/src/game/parsing/simfile/scan.rs +++ b/src/game/parsing/simfile/scan.rs @@ -11,7 +11,7 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; pub fn scan_and_load_songs(root_path: &Path) { - scan_and_load_songs_impl::(root_path, None); + scan_and_load_songs_impl::(root_path, None, false); } pub fn scan_and_load_songs_with_progress(root_path: &Path, progress: &mut F) @@ -19,14 +19,26 @@ where F: FnMut(&str, &str), { let mut with_counts = |_: usize, _: usize, pack: &str, song: &str| progress(pack, song); - scan_and_load_songs_impl(root_path, Some(&mut with_counts)); + scan_and_load_songs_impl(root_path, Some(&mut with_counts), false); } pub fn scan_and_load_songs_with_progress_counts(root_path: &Path, progress: &mut F) where F: FnMut(usize, usize, &str, &str), { - scan_and_load_songs_impl(root_path, Some(progress)); + scan_and_load_songs_impl(root_path, Some(progress), false); +} + +/// Full rescan triggered by the user's explicit "Reload Songs/Courses" action. +/// +/// Unlike [`scan_and_load_songs_with_progress_counts`] (used at startup, which honours the +/// fastload shortcut), this forces cache freshness verification so on-disk changes such as a +/// newly added or removed music file are detected without renaming folders or wiping the cache. +pub fn reload_all_songs_with_progress_counts(root_path: &Path, progress: &mut F) +where + F: FnMut(usize, usize, &str, &str), +{ + scan_and_load_songs_impl(root_path, Some(progress), true); } pub fn reload_song_dirs_with_progress_counts( @@ -36,7 +48,7 @@ pub fn reload_song_dirs_with_progress_counts( ) where F: FnMut(usize, usize, &str, &str), { - reload_song_dirs_impl(root_path, dirs, Some(progress)); + reload_song_dirs_impl(root_path, dirs, Some(progress), true); } fn path_key(path: &Path) -> String { @@ -472,6 +484,7 @@ fn reap_song_parse( fn load_pack_scans( packs: Vec, mut progress: Option<&mut F>, + force_fresh: bool, ) -> (Vec, SongLoadStats) where F: FnMut(usize, usize, &str, &str), @@ -564,6 +577,7 @@ where simfile_path.clone(), fastload, cachesongs, + force_fresh, global_offset_seconds, ) { Ok((song_data, is_hit)) => { @@ -598,6 +612,7 @@ where simfile_path_owned.clone(), fastload, cachesongs, + force_fresh, global_offset_seconds, ) .map(|(data, is_hit)| (Arc::new(data), is_hit)) @@ -611,6 +626,7 @@ where simfile_path.clone(), fastload, cachesongs, + force_fresh, global_offset_seconds, ) { Ok((song_data, is_hit)) => { @@ -661,7 +677,7 @@ where (loaded_packs, stats) } -fn scan_and_load_songs_impl(root_path: &Path, progress: Option<&mut F>) +fn scan_and_load_songs_impl(root_path: &Path, progress: Option<&mut F>, force_fresh: bool) where F: FnMut(usize, usize, &str, &str), { @@ -681,7 +697,7 @@ where } let packs = scan_song_roots(&song_roots); - let (loaded_packs, stats) = load_pack_scans(packs, progress); + let (loaded_packs, stats) = load_pack_scans(packs, progress, force_fresh); let songs_loaded = count_loaded_songs(&loaded_packs); info!( "Finished scan. Found {} packs / {} songs (parsed {}, cache hits {}, failed {}) in {}.", @@ -695,8 +711,12 @@ where set_song_cache(loaded_packs); } -fn reload_song_dirs_impl(root_path: &Path, pack_dirs: &[PathBuf], progress: Option<&mut F>) -where +fn reload_song_dirs_impl( + root_path: &Path, + pack_dirs: &[PathBuf], + progress: Option<&mut F>, + force_fresh: bool, +) where F: FnMut(usize, usize, &str, &str), { ensure_song_cache_dir(); @@ -719,7 +739,7 @@ where ); let started = std::time::Instant::now(); let packs = scan_pack_dirs(&scan_dirs); - let (reloaded_packs, stats) = load_pack_scans(packs, progress); + let (reloaded_packs, stats) = load_pack_scans(packs, progress, force_fresh); let reloaded_pack_count = reloaded_packs.len(); let reloaded_song_count = count_loaded_songs(&reloaded_packs); diff --git a/src/screens/options/reload.rs b/src/screens/options/reload.rs index 127ffe2bc..7de32a573 100644 --- a/src/screens/options/reload.rs +++ b/src/screens/options/reload.rs @@ -76,7 +76,7 @@ pub(super) fn start_reload_songs_and_courses(state: &mut State) { song: song.to_owned(), }); }; - song_loading::scan_and_load_songs_with_progress_counts( + song_loading::reload_all_songs_with_progress_counts( &dirs::app_dirs().songs_dir(), &mut on_song, ); diff --git a/src/screens/select_music.rs b/src/screens/select_music.rs index aa3d1a283..2456fb688 100644 --- a/src/screens/select_music.rs +++ b/src/screens/select_music.rs @@ -4855,7 +4855,7 @@ fn start_reload_songs_and_courses(state: &mut State) { song: song.to_owned(), }); }; - song_loading::scan_and_load_songs_with_progress_counts( + song_loading::reload_all_songs_with_progress_counts( &dirs::app_dirs().songs_dir(), &mut on_song, );