From b4901551c8a13f36990efc9d9d21a86f33024c91 Mon Sep 17 00:00:00 2001 From: Sam Dornan Date: Mon, 28 Sep 2026 08:54:50 -0400 Subject: [PATCH] fix(scan): stop rescans inflating live platform game counts (#4867) --- backend/endpoints/sockets/scan.py | 32 +---- backend/handler/scan_handler.py | 40 +++--- backend/tests/endpoints/sockets/test_scan.py | 43 +++++- backend/tests/handler/test_fastapi.py | 31 +++++ backend/tests/utils/test_archives.py | 4 +- .../useScanLifecycle/index.test.ts | 128 ++++++++++++++++++ .../v2/composables/useScanLifecycle/index.ts | 73 ++++++---- 7 files changed, 276 insertions(+), 75 deletions(-) diff --git a/backend/endpoints/sockets/scan.py b/backend/endpoints/sockets/scan.py index abe3076c1..40d37383b 100644 --- a/backend/endpoints/sockets/scan.py +++ b/backend/endpoints/sockets/scan.py @@ -20,7 +20,6 @@ from config import DEV_MODE, SCAN_TIMEOUT, SCAN_WORKERS, TASK_RESULT_TTL from config.config_manager import MetadataMediaType from config.config_manager import config_manager as cm from endpoints.responses.platform import PlatformSchema -from endpoints.responses.rom import SimpleRomSchema from endpoints.sockets.activity import get_authenticated_user from exceptions.fs_exceptions import ( FOLDER_STRUCT_MSG, @@ -68,6 +67,7 @@ from handler.scan_handler import ( ScanType, build_hashless_fs_rom, download_rom_resources, + emit_scanning_rom, persist_soundtrack_cover, scan_firmware, scan_platform, @@ -362,28 +362,6 @@ async def _identify_firmware( return 1 if not firmware else 0 -# `files` is left out so a scan does not ship every file row of every rom. -SCANNING_ROM_EXCLUDE: Final = { - "created_at", - "updated_at", - "rom_user", - "last_modified", - "files", - "sibling_roms", -} - - -async def _emit_scanning_rom( - socket_manager: socketio.AsyncRedisManager, rom: Rom -) -> None: - await socket_manager.emit( - "scan:scanning_rom", - SimpleRomSchema.from_orm_with_factory(rom).model_dump( - exclude=SCANNING_ROM_EXCLUDE - ), - ) - - def should_scan_rom( scan_type: ScanType, rom: Rom | None, @@ -628,7 +606,7 @@ async def _identify_rom( ) hydrated_rom = db_rom_handler.get_rom_simple(rom.id) if hydrated_rom is not None: - await _emit_scanning_rom(socket_manager, hydrated_rom) + await emit_scanning_rom(socket_manager, hydrated_rom, is_new=False) return # Update properties that don't require metadata @@ -811,7 +789,7 @@ async def _identify_rom( scanned_rom_ids.add(_added_rom.id) if _added_rom.is_identified: - await _emit_scanning_rom(socket_manager, _added_rom) + await emit_scanning_rom(socket_manager, _added_rom, is_new=newly_added) if should_update_files: # Reconcile against the existing rows instead of replacing them, so file @@ -835,7 +813,7 @@ async def _identify_rom( metadata_sources=metadata_sources, ) - await _emit_scanning_rom(socket_manager, _added_rom) + await emit_scanning_rom(socket_manager, _added_rom, is_new=newly_added) async def _scan_selected_roms( @@ -1099,7 +1077,7 @@ async def _identify_platform( if hydrated_rom is None: continue - await _emit_scanning_rom(socket_manager, hydrated_rom) + await emit_scanning_rom(socket_manager, hydrated_rom, is_new=False) # Process only ROMs that actually need scanning scan_tasks = [ diff --git a/backend/handler/scan_handler.py b/backend/handler/scan_handler.py index 5a1109327..48ba7abee 100644 --- a/backend/handler/scan_handler.py +++ b/backend/handler/scan_handler.py @@ -2,7 +2,7 @@ import asyncio import enum import functools from collections.abc import Callable -from typing import Any, NotRequired, TypedDict +from typing import Any, Final, NotRequired, TypedDict import pydash import socketio @@ -537,6 +537,28 @@ async def resolve_steam_rom( return await meta_steam_handler.get_rom(fs_name, platform_slug) +# `files` is left out so a scan does not ship every file row of every rom. +_SCANNING_ROM_EXCLUDE: Final = { + "created_at", + "updated_at", + "rom_user", + "last_modified", + "files", + "sibling_roms", +} + + +async def emit_scanning_rom( + socket_manager: socketio.AsyncRedisManager, rom: Rom, *, is_new: bool +) -> None: + """Report a scanned rom; `is_new` lets clients count only roms the scan added.""" + payload = SimpleRomSchema.from_orm_with_factory(rom).model_dump( + exclude=_SCANNING_ROM_EXCLUDE + ) + payload["is_new"] = is_new + await socket_manager.emit("scan:scanning_rom", payload) + + async def scan_rom( scan_type: ScanType, platform: Platform, @@ -737,21 +759,7 @@ async def scan_rom( _added_rom.is_identifying = True if socket_manager: - await socket_manager.emit( - "scan:scanning_rom", - { - **SimpleRomSchema.from_orm_with_factory(_added_rom).model_dump( - exclude={ - "created_at", - "updated_at", - "rom_user", - "last_modified", - "files", - "sibling_roms", - } - ), - }, - ) + await emit_scanning_rom(socket_manager, _added_rom, is_new=newly_added) # Run hash fetches concurrently ( diff --git a/backend/tests/endpoints/sockets/test_scan.py b/backend/tests/endpoints/sockets/test_scan.py index 3ec455ea5..c2785fb83 100644 --- a/backend/tests/endpoints/sockets/test_scan.py +++ b/backend/tests/endpoints/sockets/test_scan.py @@ -40,6 +40,7 @@ from endpoints.sockets.scan import ( from exceptions.fs_exceptions import FolderStructureNotMatchException from exceptions.socket_exceptions import ScanStoppedException from handler import notification_handler +from handler import scan_handler as scan_handler_module from handler.audit_handler import SYSTEM_ACTOR from handler.auth.constants import Scope from handler.database import db_collection_handler, db_platform_handler, db_rom_handler @@ -833,7 +834,7 @@ class TestIdentifyRomTagReparse: # A COMPLETE scan runs past the point a HASHES scan returns at, into the # resource downloads and the closing emit, none of which is under test. mocker.patch.object(scan_module, "download_rom_resources", new=AsyncMock()) - mocker.patch.object(scan_module, "SimpleRomSchema", MagicMock()) + mocker.patch.object(scan_handler_module, "SimpleRomSchema", MagicMock()) db = mocker.patch.object(scan_module, "db_rom_handler") db.add_rom.return_value = MagicMock( @@ -2717,7 +2718,7 @@ def identify_harness(mocker): mocker.patch.object(scan_module, "fs_resource_handler", new=AsyncMock()) mocker.patch.object(scan_module, "download_rom_resources", new=AsyncMock()) - mocker.patch.object(scan_module, "SimpleRomSchema", MagicMock()) + mocker.patch.object(scan_handler_module, "SimpleRomSchema", MagicMock()) db = mocker.patch.object(scan_module, "db_rom_handler") db.add_rom.return_value = MagicMock( @@ -2841,6 +2842,44 @@ class TestIdentifyRomFiles: identify_harness.db.add_rom.assert_called() +class TestIdentifyRomEmitsNewFlag: + """Clients bump live game counts off `is_new`, so a rescan of an existing + rom must not report it as new.""" + + @pytest.mark.parametrize( + "case, is_new", + [("new", True), ("existing", False), ("reassociated", False)], + ) + async def test_scanning_rom_payload_flags_new_roms( + self, mocker, identify_harness, case, is_new + ): + dumped = Mock(model_dump=Mock(side_effect=lambda **_: {"id": 1})) + mocker.patch.object( + scan_handler_module, + "SimpleRomSchema", + Mock(from_orm_with_factory=Mock(return_value=dumped)), + ) + # Identified, so both the post-scan and the post-download emits fire. + identify_harness.db.add_rom.return_value.is_identified = True + if case == "reassociated": + missing = identify_harness.existing_rom() + identify_harness.db.get_matching_missing_rom.return_value = missing + identify_harness.db.update_rom.return_value = missing + socket_manager = AsyncMock() + rom = identify_harness.existing_rom() if case == "existing" else None + + await identify_harness.run( + rom, ScanType.COMPLETE, [], socket_manager=socket_manager + ) + + payloads = [ + call.args[1] + for call in socket_manager.emit.await_args_list + if call.args[0] == "scan:scanning_rom" + ] + assert [payload["is_new"] for payload in payloads] == [is_new, is_new] + + class TestIdentifyRomIncrementalHashing: """Selected-rom metadata scans hand the stored rows to the file rebuild so unchanged files keep their hashes; full rescans read every file.""" diff --git a/backend/tests/handler/test_fastapi.py b/backend/tests/handler/test_fastapi.py index cae5ed27d..67b4e587e 100644 --- a/backend/tests/handler/test_fastapi.py +++ b/backend/tests/handler/test_fastapi.py @@ -32,6 +32,7 @@ from handler.metadata.ss_handler import ( from handler.scan_handler import ( MetadataSource, ScanType, + build_hashless_fs_rom, scan_platform, scan_rom, ) @@ -1711,3 +1712,33 @@ async def test_scan_rom_games_still_use_fuzzy_catalog_covers( assert result.igdb_id == 3340 assert result.name == "Paper Mario" assert result.url_cover == "https://images.igdb.com/paper-mario.jpg" + + +@pytest.mark.parametrize("newly_added", [True, False]) +async def test_scan_rom_emit_flags_new_roms(newly_added: bool): + """Clients count a ROM toward its platform's games off `is_new`.""" + platform = db_platform_handler.add_platform( + Platform(id=1, slug="n64", fs_slug="n64", name="Nintendo 64") + ) + rom = db_rom_handler.add_rom( + Rom(platform_id=platform.id, fs_name="Game.z64", fs_path="n64", tags=[]) + ) + socket_manager = AsyncMock() + + async with initialize_context(): + await scan_rom( + platform=platform, + scan_type=ScanType.QUICK, + rom=rom, + fs_rom=build_hashless_fs_rom(rom.fs_name, rom.fs_path, flat=True), + metadata_sources=[], + newly_added=newly_added, + socket_manager=socket_manager, + ) + + payloads = [ + call.args[1] + for call in socket_manager.emit.await_args_list + if call.args[0] == "scan:scanning_rom" + ] + assert [payload["is_new"] for payload in payloads] == [newly_added] diff --git a/backend/tests/utils/test_archives.py b/backend/tests/utils/test_archives.py index c494b43c5..13bb0b18f 100644 --- a/backend/tests/utils/test_archives.py +++ b/backend/tests/utils/test_archives.py @@ -189,9 +189,7 @@ class TestStalledExtractorIsKilled: @pytest.mark.usefixtures("stalling_7zz") def test_largest_member_hashing_fails(self): start = time.monotonic() - assert not archives._process_largest_7z_member( - Path("/fake/game.7z"), MagicMock() - ) + assert not archives.hash_largest_7z_member(Path("/fake/game.7z"), MagicMock()) assert time.monotonic() - start < 10 @pytest.mark.usefixtures("stalling_7zz") diff --git a/frontend/src/v2/composables/useScanLifecycle/index.test.ts b/frontend/src/v2/composables/useScanLifecycle/index.test.ts index da7d8dadd..c5edfc538 100644 --- a/frontend/src/v2/composables/useScanLifecycle/index.test.ts +++ b/frontend/src/v2/composables/useScanLifecycle/index.test.ts @@ -4,8 +4,11 @@ import { createPinia, setActivePinia } from "pinia"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { defineComponent, reactive } from "vue"; import type { ScanStats } from "@/__generated__"; +import platformApi from "@/services/api/platform"; import taskApi from "@/services/api/task"; import storeCollections from "@/stores/collections"; +import storePlatforms, { type Platform } from "@/stores/platforms"; +import storeRoms, { type SimpleRom } from "@/stores/roms"; import storeScanning from "@/stores/scanning"; import type { Events } from "@/types/emitter"; import { installScanLifecycle } from "./index"; @@ -55,6 +58,51 @@ const getTaskStatus = vi.mocked(taskApi.getTaskStatus); /** Drain pending microtasks so the reconcile's promise chain has settled. */ const flushPromises = () => new Promise((resolve) => setTimeout(resolve, 0)); +/** Outlast the handler's 100ms batching debounce for `scan:scanning_rom`. */ +const drainRomBatch = () => new Promise((resolve) => setTimeout(resolve, 150)); + +function platform(overrides: Partial = {}): Platform { + return { + id: 1, + slug: "n64", + fs_slug: "n64", + rom_count: 2, + name: "Nintendo 64", + igdb_slug: null, + moby_slug: null, + hltb_slug: null, + libretro_slug: null, + created_at: "", + updated_at: "", + fs_size_bytes: 0, + is_unidentified: false, + is_identified: true, + missing_from_fs: false, + display_name: "Nintendo 64", + firmware_count: 0, + ...overrides, + }; +} + +function simpleRom(overrides: Partial = {}): SimpleRom { + return { id: 1, name: "Game", platform_id: 1, ...overrides } as SimpleRom; +} + +function scanningRom( + id: number, + isNew: boolean, + extra: Partial = {}, +) { + return { + id, + platform_id: 1, + platform_fs_slug: "n64", + platform_display_name: "Nintendo 64", + is_new: isNew, + ...extra, + }; +} + function makeStats(overrides: Partial = {}): ScanStats { return { total_platforms: 0, @@ -170,6 +218,86 @@ describe("installScanLifecycle", () => { expect(refresh).toHaveBeenCalledTimes(1); }); + it("counts only the ROMs a scan added toward the platform's games", async () => { + install(); + const platforms = storePlatforms(); + platforms.set([platform()]); + + fire("scan:scanning_rom", scanningRom(1, false)); + fire("scan:scanning_rom", scanningRom(2, false)); + fire("scan:scanning_rom", scanningRom(3, true)); + fire("scan:scanning_rom", scanningRom(3, true)); + await drainRomBatch(); + + expect(platforms.get(1)?.rom_count).toBe(3); + }); + + it("counts a new ROM once when a platform event rebuilds the live log", async () => { + install(); + const platforms = storePlatforms(); + platforms.set([platform()]); + const newRom = scanningRom(3, true); + + fire("scan:scanning_rom", newRom); + await drainRomBatch(); + fire("scan:scanning_platform", { + id: 1, + name: "Nintendo 64", + display_name: "Nintendo 64", + slug: "n64", + fs_slug: "n64", + is_identified: true, + new_firmware_count: 0, + }); + fire("scan:scanning_rom", newRom); + await drainRomBatch(); + + expect(platforms.get(1)?.rom_count).toBe(3); + }); + + it("applies the last ROM batch before reconciling counts on scan:done", async () => { + vi.mocked(platformApi.getPlatforms).mockResolvedValueOnce({ + data: [platform({ rom_count: 3 })], + } as never); + install(); + const platforms = storePlatforms(); + platforms.set([platform()]); + + fire("scan:scanning_rom", scanningRom(3, true)); + fire("scan:done", makeStats()); + await drainRomBatch(); + + expect(platforms.get(1)?.rom_count).toBe(3); + }); + + it("counts a new ROM once when a scan fails with its next emit queued", async () => { + install(); + const platforms = storePlatforms(); + platforms.set([platform()]); + + fire("scan:scanning_rom", scanningRom(3, true)); + await drainRomBatch(); + fire("scan:scanning_rom", scanningRom(3, true)); + fire("scan:done_ko", "disk gone"); + await drainRomBatch(); + + expect(platforms.get(1)?.rom_count).toBe(3); + }); + + it("puts only the ROMs a scan added at the top of the recent list", async () => { + install(); + const roms = storeRoms(); + roms.setRecentRoms([simpleRom({ name: "Old" })]); + + fire("scan:scanning_rom", scanningRom(1, false, { name: "Rescanned" })); + fire("scan:scanning_rom", scanningRom(2, false)); + fire("scan:scanning_rom", scanningRom(3, true)); + await drainRomBatch(); + + expect(roms.recentRoms.map((r) => r.id)).toEqual([3, 1]); + expect(roms.recentRoms[1].name).toBe("Rescanned"); + }); + it("reconciles with a running scan job on install", async () => { getTaskStatus.mockResolvedValue({ data: [runningScanTask(makeStats({ scanned_roms: 40, total_roms: 100 }))], diff --git a/frontend/src/v2/composables/useScanLifecycle/index.ts b/frontend/src/v2/composables/useScanLifecycle/index.ts index d7beec9f8..c0c45e0f7 100644 --- a/frontend/src/v2/composables/useScanLifecycle/index.ts +++ b/frontend/src/v2/composables/useScanLifecycle/index.ts @@ -1,27 +1,27 @@ -// useScanLifecycle — global wire-up between the scan socket events and the +// useScanLifecycle: global wire-up between the scan socket events and the // `scanning` Pinia store. Mounted once at the top of the v2 tree (AppLayout) // so the navbar's ScanningIndicator, the dedicated /scan view, and anyone // else reading `scanning` / `scanningPlatforms` / `scanStats` always see -// the same truth — even when the user navigates between routes mid-scan. +// the same truth, even when the user navigates between routes mid-scan. // // Events handled: -// * `scan:scanning_platform` — backend announces the platform it's about +// * `scan:scanning_platform`: backend announces the platform it's about // to process; push it onto the live log so // the /scan view can render a panel for it. -// * `scan:scanning_rom` — per-ROM update during a scan. Batched on +// * `scan:scanning_rom`: per-ROM update during a scan. Batched on // a 100ms debounce window so a thousand // rapid-fire updates don't tank rendering. -// * `scan:update_stats` — periodic progress; keep `scanStats` fresh. -// * `scan:done` — scan finished; persist the final stats, +// * `scan:update_stats`: periodic progress; keep `scanStats` fresh. +// * `scan:done`: scan finished; persist the final stats, // flip `scanning` off so the indicator hides, // then refetch platforms to reconcile counts. -// * `scan:done_ko` — scan errored; flip `scanning` off. +// * `scan:done_ko`: scan errored; flip `scanning` off. // // Both reach every connected user, so only the tab that started the scan // toasts its end. Whoever else it concerns gets a notification. // // Events alone can't tell a tab that loads mid-scan what's going on, so -// install also reconciles against the running RQ job — see +// install also reconciles against the running RQ job; see // `reconcileWithRunningScan` below. // // `useSocketEvent` is the typed subscription wrapper that auto-cleans up @@ -42,6 +42,9 @@ import type { Events } from "@/types/emitter"; import { useSocketEvent } from "@/v2/composables/useSocketEvent"; import storeGalleryRoms from "@/v2/stores/galleryRoms"; +/** `scan:scanning_rom` payload; `is_new` marks a ROM this scan added. */ +type ScanningRom = SimpleRom & { is_new: boolean }; + export function installScanLifecycle() { const scanningStore = storeScanning(); const romsStore = storeRoms(); @@ -69,7 +72,7 @@ export function installScanLifecycle() { (p) => p.display_name !== display_name, ); // Prepend so the platform being scanned right now stays at the top of - // the live log — no scrolling to follow progress. + // the live log, where progress can be followed without scrolling. scanningStore.scanningPlatforms.unshift({ name, display_name, @@ -81,10 +84,8 @@ export function installScanLifecycle() { is_identified, }); - // Surface brand-new platforms in the canonical platforms store the - // moment the scan reaches them — previously they only appeared after - // a manual page refresh. The socket payload is a partial (8 fields), - // so fetch the full PlatformSchema before adding it to the store. + // Surface brand-new platforms the moment the scan reaches them. The + // socket payload is a partial, so fetch the full PlatformSchema. if (!platformsStore.has(id)) { platformApi .getPlatform(id) @@ -99,7 +100,10 @@ export function installScanLifecycle() { // Batch per-ROM updates so a fast scan doesn't trigger one render per // ROM. Queue drains every 100ms; matches the v1 behavior. Stored // outside the handler so multiple events share the same queue + flush. - const romUpdateQueue: SimpleRom[] = []; + const romUpdateQueue: ScanningRom[] = []; + // A new ROM emits several times, and a platform event can rebuild the live + // log between those emits, so each new ROM is counted once by id. + const countedNewRomIds = new Set(); const refreshGallery = debounce( () => { galleryRomsStore.invalidateWindows(); @@ -111,11 +115,24 @@ export function installScanLifecycle() { const processRomUpdates = debounce(() => { if (romUpdateQueue.length === 0) return; const updates = romUpdateQueue.splice(0, romUpdateQueue.length); - updates.forEach((rom) => { - // Keep the global "recent" list fresh so any view watching it - // (Home) reflects the new ROM at the top. - romsStore.removeFromRecent(rom); - romsStore.addToRecent(rom); + updates.forEach(({ is_new: isNew, ...rom }) => { + // Home's "recently added" row takes new ROMs at the top; a rescanned + // one is refreshed where it already sits. + if (isNew) { + romsStore.removeFromRecent(rom); + romsStore.addToRecent(rom); + if (!countedNewRomIds.has(rom.id)) { + countedNewRomIds.add(rom.id); + // The gallery/nav getters gate on `rom_count > 0`, so this is what + // makes a freshly-scanned platform render mid-scan. + const storePlatform = platformsStore.get(rom.platform_id); + if (storePlatform) storePlatform.rom_count += 1; + } + } else if (romsStore.recentRoms.some((r) => r.id === rom.id)) { + romsStore.recentRoms = romsStore.recentRoms.map((r) => + r.id === rom.id ? rom : r, + ); + } // If the user is currently looking at the gallery of the platform // being scanned, refresh from the server to preserve sorting/filtering. @@ -132,7 +149,7 @@ export function installScanLifecycle() { (p) => p.fs_slug === rom.platform_fs_slug, ); - // Socket may have dropped the `scan:scanning_platform` event — add + // The socket may have dropped the `scan:scanning_platform` event. Add // the platform synthetically so the user still sees something. if (!scannedPlatform) { scanningStore.scanningPlatforms.unshift({ @@ -156,18 +173,13 @@ export function installScanLifecycle() { r.id === rom.id ? rom : r, ); } else { - // Newest ROM first, same as platforms — most recent stays on top. + // Newest ROM first, same as platforms, so the most recent stays on top. scannedPlatform.roms.unshift(rom); - // Keep the canonical platforms store's count live for genuinely new - // ROMs: the gallery/nav getters gate on `rom_count > 0`, so this is - // what makes a freshly-scanned platform actually render mid-scan. - const storePlatform = platformsStore.get(rom.platform_id); - if (storePlatform) storePlatform.rom_count += 1; } }); }, 100); - useSocketEvent("scan:scanning_rom", (rom) => { + useSocketEvent("scan:scanning_rom", (rom) => { scanningStore.setScanning(true); romUpdateQueue.push(rom); processRomUpdates(); @@ -185,6 +197,10 @@ export function installScanLifecycle() { useSocketEvent("scan:done", (stats) => { markScanEnded(); + // Apply the last ROM batch now, or its count bumps would land on the + // platforms refetched below. + processRomUpdates.flush(); + countedNewRomIds.clear(); const startedHere = scanningStore.startedInThisTab; scanningStore.setScanStats(stats); scanningStore.setScanning(false); @@ -203,6 +219,9 @@ export function installScanLifecycle() { useSocketEvent("scan:done_ko", (msg) => { markScanEnded(); + // Apply the last ROM batch while its counted ids are still known. + processRomUpdates.flush(); + countedNewRomIds.clear(); const startedHere = scanningStore.startedInThisTab; scanningStore.setScanning(false); if (!startedHere) return;