fix(songs): detect added/removed files on Reload Songs/Courses#494
Draft
adstep wants to merge 1 commit into
Draft
fix(songs): detect added/removed files on Reload Songs/Courses#494adstep wants to merge 1 commit into
adstep wants to merge 1 commit into
Conversation
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 pnn64#493
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
With the default
FastLoad=1, the song scan trusts the on-disk cache without verifying the song folder's directory hash. If a song folder is missing its music file at first scan, it gets cached withmusic_path: None, and the cache's path-existence check treatsNoneas "present". So when the user later adds the music file and runs Reload Songs/Courses, the change is never detected. The song still won't preview or play until a folder is renamed (which changes the cache key) or the cache is wiped.Approach
An explicit reload should always pick up on-disk changes, regardless of the
FastLoadstartup-speed toggle. So reload now forces cache freshness verification while startup stays fast:process_songtakes aforce_freshflag and computesverify_freshness = force_fresh || !fastload. Cache reads stay fast; they are just verified against the directory hash.load_pack_scans,scan_and_load_songs_impl, andreload_song_dirs_impl.reload_all_songs_with_progress_counts(force-fresh) backs both full-reload call sites (options/reload.rsandselect_music.rs); startup (init.rs) keeps the fast path unchanged.The directory-hash check is symmetric, so this also catches file removals and delete-and-re-add with the same folder name.
Performance
This was measured, not just estimated. Microbenchmark over 2000 synthetic song folders (5 files each, warm cache, single-threaded, Windows):
The extra cost is one
canonicalize+read_dir+ per-entrymetadataper song. These are single-threaded numbers; the real reload parses in parallel, so wall-clock overhead is roughlydelta / threads(~14 ms per 1000 songs on a 32-thread box). No extra chart parsing happens for unchanged songs, and startup is untouched. This is an acceptable trade for an occasional, progress-bar-driven action.Notes for reviewers
simfile/cache.rscover the missing-then-added music case and a file-removal case.Closes #493