From c06081345bf0df07fd68055f1dae29c95330c5ab Mon Sep 17 00:00:00 2001 From: Job Date: Thu, 16 Jul 2026 03:41:45 +0200 Subject: [PATCH] Revert "perf(scan): skip full library re-stat when the tree is unchanged (#979)" --- lib/scan.py | 142 ++------------------------------ server.py | 7 +- tests/test_feedpak_extension.py | 75 ----------------- 3 files changed, 7 insertions(+), 217 deletions(-) diff --git a/lib/scan.py b/lib/scan.py index 6780b04f..820cb50e 100644 --- a/lib/scan.py +++ b/lib/scan.py @@ -51,97 +51,6 @@ log = logging.getLogger("feedBack.scan") -import json - - -# ── Directory-signature fast path ───────────────────────────────────────────── -# -# A startup scan globs the whole library twice (*.feedpak, *.wem) and stats every -# file to detect what changed. On a 50k-song library that lives on a slow mount -# (an NTFS-3G FUSE volume here) it is ~100k filesystem round trips every launch — -# the "big drive churns on every startup" report. -# -# But adds / removes / renames of songs all bump the mtime of the DIRECTORY that -# holds them (verified on the target NTFS-3G mount), and so does the addition of -# a subdirectory (a new entry in its parent). So after a scan we record every -# library directory and its mtime; on the next scan we re-stat ONLY those -# directories (a handful, vs 100k file ops). If none changed, the file set is -# unchanged and the whole listing/stat pass is skipped. -# -# The one thing this cannot see is a file edited IN PLACE under the same name — -# that bumps the file's mtime but not its directory's. That is rare for a song -# library (you add and remove packs, you don't rewrite them under the same name), -# and the manual Refresh forces a full scan (force=True) for exactly that case. -def _dir_signature_file() -> Path: - return appstate.config_dir / "scan_dir_signature.json" - - -def _load_dir_signature() -> dict | None: - try: - data = json.loads(_dir_signature_file().read_text(encoding="utf-8")) - if isinstance(data, dict) and isinstance(data.get("dirs"), dict): - return data - except (OSError, ValueError): - pass - return None - - -def _save_dir_signature(dlc: Path, dirs: dict[str, int]) -> None: - # Keyed by the DLC path so switching libraries never matches a stale - # signature. Best-effort: a failed write just means the next scan is a full - # one, never a wrong one. - try: - _dir_signature_file().write_text( - json.dumps({"dlc": str(dlc), "dirs": dirs}), encoding="utf-8") - except OSError as e: - log.debug("scan: could not persist dir signature: %s", e) - - -def _library_dirs(all_songs, dlc: Path) -> set[str]: - """Every directory whose mtime reflects an add/remove of a library song: - each song's containing directory and all of its ancestors up to the DLC - root (the root itself always included, as "."). Derived from the already- - listed songs — no extra filesystem walk. The builtin carve-outs - (tutorials-builtin / minigames-builtin) are absent because the caller - already excluded them from `all_songs`, so a minigame writing a drill there - never invalidates the fast path. - - Directory-form songs (loose-song folders, directory sloppak bundles) also - record their OWN directory: a file added/removed/replaced INSIDE the folder - bumps that folder's mtime but not its parent's, so tracking only the parent - would miss an in-place change to such a song. File-form sloppaks (a single - .feedpak zip) aren't dirs, so they add nothing here — the flat file library - stays at a handful of dir stats.""" - rels = {"."} - for f in all_songs: - rel = Path(_relpath(f, dlc)) - if f.is_dir(): - rels.add(rel.as_posix()) - parent = rel.parent - rels.add(parent.as_posix()) - for anc in parent.parents: - rels.add(anc.as_posix()) - return rels - - -def _record_dir_signature(all_songs, dlc: Path) -> None: - sig = _stat_dirs(dlc, _library_dirs(all_songs, dlc)) - if sig is not None: # a dir vanished mid-scan → skip; next scan is full - _save_dir_signature(dlc, sig) - - -def _stat_dirs(dlc: Path, rels) -> dict[str, int] | None: - """{reldir: mtime_ns} for the given library dirs, or None if any is gone or - unreadable — a vanished recorded dir means the tree changed, so fail to a - full scan rather than a false match.""" - out: dict[str, int] = {} - for rel in rels: - try: - out[rel] = (dlc if rel == "." else dlc / rel).stat().st_mtime_ns - except OSError: - return None - return out - _SCAN_STATUS_INIT = {"running": False, "stage": "idle", "total": 0, "done": 0, "current": "", "error": None, "is_first_scan": False, "added": 0, "removed": 0} @@ -190,12 +99,9 @@ def _make_scan_executor(): ) -def background_scan(force: bool = False): +def background_scan(): """Scan the library and cache song metadata on startup. Uses a process pool to bypass the GIL for CPU-bound metadata parsing. - `force` skips the directory-signature fast path and always does the full - listing/stat pass — the manual Refresh sets it (see _dir_signature_file). - Never sets `_scan_status["running"] = False` — ownership of that flag lives in `_scan_runner` so a `kick_scan()` racing this function's terminal write cannot observe a stale False and start a second runner. @@ -215,22 +121,6 @@ def background_scan(force: bool = False): builtin_content.seed_builtin_diagnostic_sloppaks(appstate.server_root, dlc) builtin_content.seed_builtin_starter_content(appstate.server_root, dlc) - # Fast path: if every library directory recorded by the last scan still has - # the same mtime, nothing was added, removed, or renamed, so the whole - # glob-and-stat pass below can be skipped (see the signature comment above). - # `force` (manual Refresh) always does the full pass. Seeding above is - # idempotent — it only writes when a builtin is missing — so it does not - # perturb the mtimes on a settled library. - if not force: - stored = _load_dir_signature() - if stored is not None and stored.get("dlc") == str(dlc): - current = _stat_dirs(dlc, stored["dirs"].keys()) - if current is not None and current == stored["dirs"]: - _scan_status = {**_SCAN_STATUS_INIT, "running": True, "stage": "complete"} - log.info("Scan: library tree unchanged (%d dirs) — skipped the full listing/stat pass", - len(current)) - return - # Listing can fail on macOS without Full Disk Access, or on Docker if the # path isn't shared. Report the failure explicitly rather than silently # appearing to scan nothing. @@ -333,9 +223,6 @@ def _is_excluded_from_library(p: Path) -> bool: to_scan.append((f, mtime, size, dlc)) if not to_scan: - # Full pass completed with the DB already up to date — record the tree - # signature so the next startup can take the fast path. - _record_dir_signature(all_songs, dlc) _scan_status = {**_SCAN_STATUS_INIT, "running": True, "stage": "complete", "added": added, "removed": removed} log.info("Scan: nothing new to scan (%d songs, all cached)", len(all_songs)) return @@ -360,9 +247,6 @@ def _is_excluded_from_library(p: Path) -> bool: _scan_status["done"] += 1 _scan_status["current"] = fname - # Record the tree signature after a completed full pass so the next startup - # can skip it when nothing has changed. - _record_dir_signature(all_songs, dlc) log.info("Scan complete: %d songs cached", len(to_scan)) _scan_status = {**_SCAN_STATUS_INIT, "running": True, "stage": "complete", "added": added, "removed": removed} @@ -371,9 +255,6 @@ def _is_excluded_from_library(p: Path) -> bool: _scan_rescan_pending = False -# Set by kick_scan(force=True); consumed by _scan_runner for the next pass so a -# manual Refresh bypasses the directory-signature fast path. -_scan_force_next = False # Handles to the running scan / enrichment worker threads. Both use the shared @@ -384,15 +265,9 @@ def _is_excluded_from_library(p: Path) -> bool: _scan_thread: threading.Thread | None = None -def kick_scan(force: bool = False) -> bool: +def kick_scan() -> bool: """Request a library rescan, single-flight + coalescing. - `force` skips the directory-signature fast path for the resulting pass (the - manual Refresh uses it so an in-place same-name edit — the one thing the - fast path can't see — is always picked up). A forced request that coalesces - onto a running or queued scan keeps the force intent: the pass is forced if - ANY pending request asked for it. - Returns True if a new scan thread was started, False if one was already running. In the latter case a follow-up pass is queued and runs as soon as the current scan finishes so files landing mid-scan (e.g. an upload @@ -400,10 +275,8 @@ def kick_scan(force: bool = False) -> bool: until the next periodic pass. Multiple late-arriving requests coalesce into a single follow-up. """ - global _scan_rescan_pending, _scan_thread, _scan_force_next + global _scan_rescan_pending, _scan_thread with _scan_kick_lock: - if force: - _scan_force_next = True if _scan_status["running"]: _scan_rescan_pending = True return False @@ -417,15 +290,10 @@ def kick_scan(force: bool = False) -> bool: def _scan_runner(): """Run _background_scan, then re-run if requests arrived mid-scan.""" - global _scan_rescan_pending, _scan_force_next + global _scan_rescan_pending while True: - # Consume the force flag for THIS pass; a forced request queued mid-scan - # sets it again for the follow-up. - with _scan_kick_lock: - forced = _scan_force_next - _scan_force_next = False try: - background_scan(force=forced) + background_scan() except Exception: log.exception("background scan failed unexpectedly") diff --git a/server.py b/server.py index ecb68637..ee13aa98 100644 --- a/server.py +++ b/server.py @@ -1115,10 +1115,7 @@ async def _gen(): @app.post("/api/rescan") def trigger_rescan(): """Manually trigger a library rescan.""" - # force=True: a manual Refresh must skip the directory-signature fast path — - # it is the escape hatch for the one change dir mtimes can't see (a pack - # rewritten in place under the same name). - if not scan.kick_scan(force=True): + if not scan.kick_scan(): return {"message": "Scan already in progress"} return {"message": "Rescan started"} @@ -1136,7 +1133,7 @@ def trigger_full_rescan(): # delete_missing() prunes anything genuinely gone at the end. meta_db.conn.execute("UPDATE songs SET mtime = -1") meta_db.conn.commit() - if not scan.kick_scan(force=True): + if not scan.kick_scan(): return {"message": "Scan already in progress"} return {"message": "Full rescan started"} diff --git a/tests/test_feedpak_extension.py b/tests/test_feedpak_extension.py index 2f556fed..f8aeb3d8 100644 --- a/tests/test_feedpak_extension.py +++ b/tests/test_feedpak_extension.py @@ -137,81 +137,6 @@ def mock_extract(f, dlc_dir): assert "ignore.zip" not in seen -# ── 2b. directory-signature fast path (skip the full re-stat) ──────────────── - -def test_dir_signature_fast_path_skips_unchanged_tree(tmp_path, scan_server): - """After a full scan records the library-dir signature, a second scan with - an unchanged tree takes the fast path and does NOT re-glob/extract — but a - forced scan (manual Refresh) always does the full pass, and a new song - (which bumps the dir mtime) reverts to a full pass on its own.""" - import unittest.mock as mock - - dlc = tmp_path / "dlc" - dlc.mkdir() - (dlc / "a.feedpak").write_bytes(b"") - (tmp_path / "config.json").write_text('{"dlc_dir": "%s"}' % dlc) - - scan = importlib.import_module("scan") - seen: list[str] = [] - - def mock_extract(f, dlc_dir): - seen.append(f.name) - return {"title": f.name, "artist": "", "album": ""} - - with mock.patch("scan_worker._extract_meta_for_file", new=mock_extract): - # 1) first pass: full scan, extracts a.feedpak (+ seeded builtins), - # records the signature - scan.background_scan() - assert "a.feedpak" in seen - assert scan._dir_signature_file().exists() - - # 2) unchanged tree: fast path — no glob, no extraction at all - seen.clear() - scan.background_scan() - assert seen == [] - assert scan.status()["stage"] == "complete" - - # 3) a new song bumps the dlc mtime → signature mismatch → full pass - # picks it up on its own (no manual Refresh needed for adds) - (dlc / "b.feedpak").write_bytes(b"") - seen.clear() - scan.background_scan() - assert "b.feedpak" in seen - - # 4) force=True (Refresh) bypasses the fast path even on a settled tree: - # with the signature now current, a plain scan skips, a forced one lists - seen.clear() - scan.background_scan() # fast path - assert seen == [] - forced_listed = [] - real_delete_missing = scan.appstate.meta_db.delete_missing - def _spy(files): - forced_listed.append(set(files)) - return real_delete_missing(files) - with mock.patch.object(scan.appstate.meta_db, "delete_missing", new=_spy): - scan.background_scan(force=True) - assert forced_listed, "force=True must run the full listing pass" - - -def test_dir_signature_tracks_directory_form_song_own_dir(tmp_path): - """A directory-form song (loose folder / directory bundle) records its OWN - directory in the signature, so an in-place file change inside it — which - bumps that folder's mtime but not its parent's — invalidates the fast path. - A file-form sloppak (a plain .feedpak zip) is not a dir and adds nothing.""" - scan = importlib.import_module("scan") - dlc = tmp_path / "dlc" - (dlc / "packs").mkdir(parents=True) - loose = dlc / "packs" / "my_loose_song" # directory-form song - loose.mkdir() - zipped = dlc / "packs" / "zipped.feedpak" # file-form song - zipped.write_bytes(b"") - - rels = scan._library_dirs([loose, zipped], dlc) - assert "packs/my_loose_song" in rels, "directory-form song must track its own dir" - assert "packs" in rels and "." in rels - assert "packs/zipped.feedpak" not in rels, "a file-form sloppak is not a tracked dir" - - # ── 3. POST /api/songs/upload gate (endpoint) ──────────────────────────────── @pytest.fixture()