From 9c6549b5ec908a0880b611534b514460aa9d00fc Mon Sep 17 00:00:00 2001 From: Callan Barrett Date: Mon, 17 Aug 2026 04:01:44 +0800 Subject: [PATCH 1/4] fix(api): improve media response reliability --- docs/api/methods.md | 1 + pkg/api/methods/media.go | 10 +- pkg/api/methods/media_browse.go | 24 +++-- pkg/api/methods/media_browse_test.go | 13 +++ pkg/api/methods/media_history.go | 27 +++-- pkg/api/methods/media_history_test.go | 12 ++- pkg/api/methods/media_image.go | 22 +++- pkg/api/methods/media_meta.go | 20 +++- pkg/api/methods/media_search_test.go | 2 +- .../mediadb/mediadb_integration_test.go | 101 +++++++++++++++++- .../mediadb/scan_staging_repair_test.go | 4 +- pkg/database/mediadb/sql_browse.go | 63 +++++++++-- pkg/database/mediadb/sql_browse_cache.go | 55 +++++++++- 13 files changed, 312 insertions(+), 42 deletions(-) diff --git a/docs/api/methods.md b/docs/api/methods.md index 78fe36b30..2689048fb 100644 --- a/docs/api/methods.md +++ b/docs/api/methods.md @@ -756,6 +756,7 @@ All parameters are optional. When called with no parameters, returns root entrie | relativePath | string | No | Relative path from root directory. Present on `media` entries and logical single-game container `directory` entries on zip-as-directory platforms. | | tags | object[] | No | Tags attached to the media. Each object has `tag` (string) and `type` (string). Present on `media` entries and logical single-game container `directory` entries on zip-as-directory platforms. | | disambiguatingTags | object[] | No | Subset of `tags` whose values differ across same-named siblings of this title, ordered by display importance. Same object shape as `tags`. Omitted when the title has nothing to disambiguate. | +| hasCover | boolean | Yes | Whether media-level or title-level image properties are available. Meaningful for media-capable entries; clients can skip image requests when false. | ##### Browse pagination object diff --git a/pkg/api/methods/media.go b/pkg/api/methods/media.go index b60472cf7..d21f29359 100644 --- a/pkg/api/methods/media.go +++ b/pkg/api/methods/media.go @@ -1015,6 +1015,9 @@ func HandleMediaSearch(env requests.RequestEnv) (any, error) { //nolint:gocritic coverStarted := time.Now() coverStatuses := make(map[int64]bool) + // Unknown must remain true in the response so clients do not suppress a + // valid image request merely because optional enrichment timed out. + coverStatusesKnown := false if len(searchResults) > 0 { coverRefs := make([]database.MediaCoverRef, len(searchResults)) for i := range searchResults { @@ -1030,6 +1033,7 @@ func HandleMediaSearch(env requests.RequestEnv) (any, error) { //nolint:gocritic log.Debug().Err(coverErr).Msg("could not enrich media search cover status") } else { coverStatuses = resolvedCoverStatuses + coverStatusesKnown = true } } coverDuration := time.Since(coverStarted) @@ -1073,10 +1077,14 @@ func HandleMediaSearch(env requests.RequestEnv) (any, error) { //nolint:gocritic } relativePathDuration += time.Since(stageStarted) + hasCover := true + if coverStatusesKnown { + hasCover = coverStatuses[result.MediaID] + } results = append(results, models.SearchResultMedia{ MediaID: result.MediaID, RelPath: relPath, - HasCover: coverStatuses[result.MediaID], + HasCover: hasCover, System: resultSystem, Name: result.Name, Path: result.Path, diff --git a/pkg/api/methods/media_browse.go b/pkg/api/methods/media_browse.go index 42b49f6b4..2dd068aa7 100644 --- a/pkg/api/methods/media_browse.go +++ b/pkg/api/methods/media_browse.go @@ -395,15 +395,25 @@ func dedupeSystemRootEntries(entries []models.BrowseEntry) []models.BrowseEntry return entries } - filtered := make([]models.BrowseEntry, 0, len(entries)) - for i := range entries { - if systemRootEntryCoveredByDescendant(entries, i) { - continue + // Route candidates can contain several ancestor levels. One pass removes an + // intermediate route, but a grandparent evaluated against the original set + // double-counts both that intermediate subtree and its leaf routes. Repeat on + // the reduced set until stable so every covered ancestor is removed while + // preserving parents with genuinely unmatched direct media. + current := entries + for { + filtered := make([]models.BrowseEntry, 0, len(current)) + for i := range current { + if systemRootEntryCoveredByDescendant(current, i) { + continue + } + filtered = append(filtered, current[i]) } - filtered = append(filtered, entries[i]) + if len(filtered) == len(current) { + return filtered + } + current = filtered } - - return filtered } func systemRootEntryCoveredByDescendant(entries []models.BrowseEntry, parentIdx int) bool { diff --git a/pkg/api/methods/media_browse_test.go b/pkg/api/methods/media_browse_test.go index 95aafe554..308e2b343 100644 --- a/pkg/api/methods/media_browse_test.go +++ b/pkg/api/methods/media_browse_test.go @@ -1057,6 +1057,19 @@ func TestDedupeSystemRootEntries(t *testing.T) { }, want: []string{path("media", "fat", "games", "NES"), path("media", "fat", "games", "NES Hacks")}, }, + { + name: "grandparent absorbed by deeper descendants", + entries: []models.BrowseEntry{ + {Path: path("media", "fat", "games", "MegaDrive"), FileCount: count(3)}, + {Path: path("media", "fat", "games", "Genesis"), FileCount: count(8387)}, + {Path: path("media", "fat", "games"), FileCount: count(8390)}, + {Path: path("media", "fat"), FileCount: count(8390)}, + }, + want: []string{ + path("media", "fat", "games", "MegaDrive"), + path("media", "fat", "games", "Genesis"), + }, + }, { name: "parent retained when descendants do not cover count", entries: []models.BrowseEntry{ diff --git a/pkg/api/methods/media_history.go b/pkg/api/methods/media_history.go index dc6e6f60a..184e05ad1 100644 --- a/pkg/api/methods/media_history.go +++ b/pkg/api/methods/media_history.go @@ -106,6 +106,9 @@ func HandleMediaHistory(env requests.RequestEnv) (any, error) { //nolint:gocriti enrichStarted := time.Now() mediaIDs := make(map[mediaPathRef]int64) coverStatuses := make(map[int64]bool) + // Unknown must remain true in the response so clients do not suppress a + // valid image request merely because optional enrichment timed out. + coverStatusesKnown := false enrichCtx, cancelEnrichment := optionalDBEnrichmentContext(env.Context) defer cancelEnrichment() @@ -132,15 +135,17 @@ func HandleMediaHistory(env requests.RequestEnv) (any, error) { //nolint:gocriti }) } - resolvedCoverStatuses := make(map[int64]bool) - if len(coverRefs) > 0 { - resolvedCoverStatuses, enrichErr = env.Database.MediaDB.GetMediaCoverStatus(enrichCtx, coverRefs) - } - if enrichErr != nil { - log.Debug().Err(enrichErr).Msg("could not enrich media history cover status") + mediaIDs = resolvedMediaIDs + if len(coverRefs) == 0 { + coverStatusesKnown = true } else { - mediaIDs = resolvedMediaIDs - coverStatuses = resolvedCoverStatuses + resolvedCoverStatuses, coverErr := env.Database.MediaDB.GetMediaCoverStatus(enrichCtx, coverRefs) + if coverErr != nil { + log.Debug().Err(coverErr).Msg("could not enrich media history cover status") + } else { + coverStatuses = resolvedCoverStatuses + coverStatusesKnown = true + } } } enrichElapsed := time.Since(enrichStarted) @@ -158,11 +163,15 @@ func HandleMediaHistory(env requests.RequestEnv) (any, error) { //nolint:gocriti endedAt = &formatted } mediaID := mediaIDs[ref] + hasCover := true + if coverStatusesKnown { + hasCover = coverStatuses[mediaID] + } responseEntries = append(responseEntries, models.MediaHistoryResponseEntry{ MediaID: mediaID, RelPath: mediaResponseRelativePath(&env, entry.SystemID, entry.MediaPath), - HasCover: coverStatuses[mediaID], + HasCover: hasCover, SystemID: entry.SystemID, SystemName: entry.SystemName, MediaName: entry.MediaName, diff --git a/pkg/api/methods/media_history_test.go b/pkg/api/methods/media_history_test.go index 0c95dafae..d5906d47a 100644 --- a/pkg/api/methods/media_history_test.go +++ b/pkg/api/methods/media_history_test.go @@ -203,8 +203,9 @@ func TestHandleMediaHistory_EnrichmentFailuresAreNonFatal(t *testing.T) { } tests := []struct { - setup func(*testing.T, *helpers.MockMediaDBI) - name string + setup func(*testing.T, *helpers.MockMediaDBI) + name string + expectedMediaID int64 }{ { name: "media identity lookup", @@ -214,7 +215,8 @@ func TestHandleMediaHistory_EnrichmentFailuresAreNonFatal(t *testing.T) { }, }, { - name: "cover lookup", + name: "cover lookup", + expectedMediaID: 42, setup: func(t *testing.T, mockMediaDB *helpers.MockMediaDBI) { t.Helper() mockMediaDB.On("FindMediaIDsByPaths", mock.Anything, []string{mediaPath}). @@ -252,8 +254,8 @@ func TestHandleMediaHistory_EnrichmentFailuresAreNonFatal(t *testing.T) { response, ok := result.(models.MediaHistoryResponse) require.True(t, ok) require.Len(t, response.Entries, 1) - assert.Zero(t, response.Entries[0].MediaID) - assert.False(t, response.Entries[0].HasCover) + assert.Equal(t, tt.expectedMediaID, response.Entries[0].MediaID) + assert.True(t, response.Entries[0].HasCover) mockUserDB.AssertExpectations(t) mockMediaDB.AssertExpectations(t) }) diff --git a/pkg/api/methods/media_image.go b/pkg/api/methods/media_image.go index 4554070ee..4710b8023 100644 --- a/pkg/api/methods/media_image.go +++ b/pkg/api/methods/media_image.go @@ -38,6 +38,7 @@ import ( "sort" "strings" "sync/atomic" + "time" "github.com/KarpelesLab/gowebp" "github.com/ZaparooProject/zaparoo-core/v2/pkg/api/models" @@ -899,7 +900,23 @@ func validateMediaImageDelivery(delivery string, ref mediaRefParam) error { // HandleMediaImage returns a single best-match image inline or as a transient, // Core-owned cached thumbnail path when explicitly requested. -func HandleMediaImage(env requests.RequestEnv) (any, error) { //nolint:gocritic // single-use parameter in API handler +// +//nolint:gocritic // RequestEnv is copied once at the API handler boundary. +func HandleMediaImage(env requests.RequestEnv) (result any, resultErr error) { + started := time.Now() + deliveryForLog := "" + maxSizeForLog := 0 + hasMediaID := false + defer func() { + log.Debug(). + Dur("duration", time.Since(started)). + Str("delivery", deliveryForLog). + Int("maxSize", maxSizeForLog). + Bool("mediaId", hasMediaID). + Bool("ok", resultErr == nil). + Msg("media.image handler timing") + }() + ref, delivery, err := parseMediaImageRequest(env.Params) if err != nil { return nil, err @@ -907,6 +924,8 @@ func HandleMediaImage(env requests.RequestEnv) (any, error) { //nolint:gocritic if deliveryErr := validateMediaImageDelivery(delivery, ref); deliveryErr != nil { return nil, deliveryErr } + deliveryForLog = delivery + hasMediaID = ref.MediaID != nil localPath := delivery == mediaImageDeliveryPath // Snap the requested size onto a standard tier so every view shares one @@ -915,6 +934,7 @@ func HandleMediaImage(env requests.RequestEnv) (any, error) { //nolint:gocritic if ref.MaxSize != nil { snapped := snapThumbMaxSize(*ref.MaxSize) ref.MaxSize = &snapped + maxSizeForLog = int(snapped) } prefs := imagePrefs(nil, ref.ImageTypes) diff --git a/pkg/api/methods/media_meta.go b/pkg/api/methods/media_meta.go index 5a55bb44d..29accde34 100644 --- a/pkg/api/methods/media_meta.go +++ b/pkg/api/methods/media_meta.go @@ -22,21 +22,39 @@ package methods import ( "fmt" "sort" + "time" "github.com/ZaparooProject/zaparoo-core/v2/pkg/api/models" "github.com/ZaparooProject/zaparoo-core/v2/pkg/api/models/requests" "github.com/ZaparooProject/zaparoo-core/v2/pkg/database" + "github.com/rs/zerolog/log" ) // HandleMediaMeta returns the full metadata graph for a single Media record: // the Media itself, its parent MediaTitle, System, level-separated Tags, and // level-separated Properties. Binary payloads are not included; use media.image // to fetch image bytes. -func HandleMediaMeta(env requests.RequestEnv) (any, error) { //nolint:gocritic // single-use parameter in API handler +// +//nolint:gocritic // RequestEnv is copied once at the API handler boundary. +func HandleMediaMeta(env requests.RequestEnv) (result any, resultErr error) { + started := time.Now() + batch := false + itemCount := 0 + defer func() { + log.Debug(). + Dur("duration", time.Since(started)). + Bool("batch", batch). + Int("itemCount", itemCount). + Bool("ok", resultErr == nil). + Msg("media.meta handler timing") + }() + params, err := parseMediaRequest(env.Params, maxMediaMetaBatchItems) if err != nil { return nil, err } + batch = params.Batch + itemCount = len(params.Items) if !params.Batch && params.Items[0].MediaID == nil { return handleMediaMetaSinglePath(&env, params.Items[0]) } diff --git a/pkg/api/methods/media_search_test.go b/pkg/api/methods/media_search_test.go index 6d9120a79..92c44ac1d 100644 --- a/pkg/api/methods/media_search_test.go +++ b/pkg/api/methods/media_search_test.go @@ -331,7 +331,7 @@ func TestHandleMediaSearch_CoverFailureIsNonFatal(t *testing.T) { require.True(t, ok) require.Len(t, response.Results, 1) assert.Equal(t, int64(1), response.Results[0].MediaID) - assert.False(t, response.Results[0].HasCover) + assert.True(t, response.Results[0].HasCover) mockMediaDB.AssertExpectations(t) } diff --git a/pkg/database/mediadb/mediadb_integration_test.go b/pkg/database/mediadb/mediadb_integration_test.go index 7429f48f1..a06b99ea4 100644 --- a/pkg/database/mediadb/mediadb_integration_test.go +++ b/pkg/database/mediadb/mediadb_integration_test.go @@ -3210,7 +3210,8 @@ func TestMediaDB_SystemBrowseFallsBackWhenBrowseCacheNotReady_Integration(t *tes require.NoError(t, mediaDB.CommitTransaction()) require.NoError(t, sqlInvalidateBrowseCache(ctx, mediaDB.sql.Load())) - assert.Equal(t, browseCacheInvalidatedVersion, getDBConfigValue(t, mediaDB, DBConfigBrowseIndexVersion)) + assert.Equal(t, "0", getDBConfigValue(t, mediaDB, DBConfigBrowseIndexVersion), + "invalidation must not relabel an incompatible cache schema") _, err = mediaDB.sql.Load().ExecContext(ctx, "DELETE FROM BrowseDirs") require.NoError(t, err) @@ -3267,7 +3268,9 @@ func TestSqlPopulateBrowseCache_PopulatesSystemAndGlobalCounts_Integration(t *te assert.Equal(t, 2, countTableRows(t, mediaDB, "BrowseDirCounts", "ParentDirDBID = ? AND ChildDirDBID = ?", rootID, romsID)) assert.Equal(t, 1, countTableRows(t, mediaDB, "BrowseDirCounts", - "ChildDirDBID = ? AND SystemDBID = ?", steamID, snesSystem.DBID)) + "ParentDirDBID = ? AND ChildDirDBID = ? AND SystemDBID = ?", rootID, steamID, snesSystem.DBID)) + assert.Equal(t, 1, countTableRows(t, mediaDB, "BrowseDirCounts", + "ParentDirDBID = ? AND ChildDirDBID = ? AND SystemDBID = ?", steamID, steamID, snesSystem.DBID)) assert.Equal(t, 1, countTableRows(t, mediaDB, "BrowseDirCounts", "ChildDirDBID = ? AND SystemDBID = ?", romsID, nesSystem.DBID)) @@ -3283,6 +3286,27 @@ func TestSqlPopulateBrowseCache_PopulatesSystemAndGlobalCounts_Integration(t *te assert.Equal(t, 2, rootDirs[0].FileCount) rpgDir := filepath.ToSlash(filepath.Join(string(filepath.Separator), "roms", "snes", "RPG")) + "/" + snesDef, systemErr := systemdefs.GetSystem(snesSystem.SystemID) + require.NoError(t, systemErr) + directRPGFiles, err := mediaDB.BrowseFileCount(ctx, database.BrowseFileCountOptions{ + PathPrefix: rpgDir, + Systems: []systemdefs.System{*snesDef}, + }) + require.NoError(t, err) + assert.Equal(t, 1, directRPGFiles) + + romsDirectFiles, err := mediaDB.BrowseFileCount(ctx, database.BrowseFileCountOptions{ + PathPrefix: romsDir, + }) + require.NoError(t, err) + assert.Zero(t, romsDirectFiles, "subtree files must not count as direct children") + + virtualFiles, err := mediaDB.BrowseFileCount(ctx, database.BrowseFileCountOptions{ + PathPrefix: "steam://", + }) + require.NoError(t, err) + assert.Equal(t, 1, virtualFiles) + files, err := mediaDB.BrowseFiles(ctx, &database.BrowseFilesOptions{PathPrefix: rpgDir, Limit: 10}) require.NoError(t, err) require.Len(t, files, 1) @@ -3367,6 +3391,79 @@ func TestSqlInvalidateBrowseCache_MarksBrowseCacheStale_Integration(t *testing.T assert.Equal(t, browseCacheInvalidatedVersion, getDBConfigValue(t, mediaDB, DBConfigBrowseIndexVersion)) } +func TestSqlInvalidateBrowseCache_PreservesIncompatibleVersion_Integration(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test in short mode") + } + t.Parallel() + mediaDB, cleanup := setupTempMediaDB(t) + defer cleanup() + + ctx := context.Background() + mediaPath := filepath.ToSlash(filepath.Join( + string(filepath.Separator), "roms", "snes", "RPG", "super-rpg.sfc")) + insertSystemWithMedia(t, mediaDB, "SNES", "Super RPG", mediaPath) + require.NoError(t, mediaDB.PopulateBrowseCache(ctx)) + + _, err := mediaDB.sql.Load().ExecContext(ctx, + "UPDATE DBConfig SET Value = ? WHERE Name = ?", + "2", + DBConfigBrowseIndexVersion, + ) + require.NoError(t, err) + _, err = mediaDB.sql.Load().ExecContext(ctx, + "DELETE FROM BrowseDirCounts WHERE ParentDirDBID = ChildDirDBID AND ParentDirDBID != ?", + browseCacheDirID(t, mediaDB, "/"), + ) + require.NoError(t, err) + + require.NoError(t, sqlInvalidateBrowseCache(ctx, mediaDB.sql.Load())) + assert.Equal(t, "2", getDBConfigValue(t, mediaDB, DBConfigBrowseIndexVersion)) + + state, err := sqlBrowseCacheStatus(ctx, mediaDB.sql.Load()) + require.NoError(t, err) + assert.Equal(t, browseCacheAbsent, state) + + rpgDir := filepath.ToSlash(filepath.Join(string(filepath.Separator), "roms", "snes", "RPG")) + "/" + fileCount, err := mediaDB.BrowseFileCount(ctx, database.BrowseFileCountOptions{PathPrefix: rpgDir}) + require.NoError(t, err) + assert.Equal(t, 1, fileCount, "incompatible cache must fall back to Media rows") +} + +func TestPopulateBrowseCacheForSystems_ClearsIncompatibleCache_Integration(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test in short mode") + } + t.Parallel() + mediaDB, cleanup := setupTempMediaDB(t) + defer cleanup() + + ctx := context.Background() + snesPath := filepath.ToSlash(filepath.Join(string(filepath.Separator), "roms", "snes", "game.sfc")) + nesPath := filepath.ToSlash(filepath.Join(string(filepath.Separator), "roms", "nes", "game.nes")) + snesSystem := insertSystemWithMedia(t, mediaDB, "SNES", "SNES Game", snesPath) + nesSystem := insertSystemWithMedia(t, mediaDB, "NES", "NES Game", nesPath) + require.NoError(t, mediaDB.PopulateBrowseCache(ctx)) + + _, err := mediaDB.sql.Load().ExecContext(ctx, + "UPDATE DBConfig SET Value = ? WHERE Name = ?", + "2", + DBConfigBrowseIndexVersion, + ) + require.NoError(t, err) + + require.NoError(t, mediaDB.PopulateBrowseCacheForSystems(ctx, []string{"SNES"})) + assert.Equal(t, browseCacheInvalidatedVersion, getDBConfigValue(t, mediaDB, DBConfigBrowseIndexVersion)) + assert.Positive(t, countTableRows(t, mediaDB, "BrowseDirCounts", "SystemDBID = ?", snesSystem.DBID)) + assert.Zero(t, countTableRows(t, mediaDB, "BrowseDirCounts", "SystemDBID = ?", nesSystem.DBID), + "rows from an incompatible cache schema must not remain serveable") + + snesDir := filepath.ToSlash(filepath.Join(string(filepath.Separator), "roms", "snes")) + "/" + fileCount, err := mediaDB.BrowseFileCount(ctx, database.BrowseFileCountOptions{PathPrefix: snesDir}) + require.NoError(t, err) + assert.Equal(t, 1, fileCount) +} + func TestMediaDB_UnfilteredBrowseReadsFromMediaWhenBrowseCacheEmpty_Integration(t *testing.T) { if testing.Short() { t.Skip("skipping integration test in short mode") diff --git a/pkg/database/mediadb/scan_staging_repair_test.go b/pkg/database/mediadb/scan_staging_repair_test.go index 6f27a99c2..7082f49a7 100644 --- a/pkg/database/mediadb/scan_staging_repair_test.go +++ b/pkg/database/mediadb/scan_staging_repair_test.go @@ -132,12 +132,12 @@ func TestIndexedSystemsUsesBrowseCache(t *testing.T) { FileCount INTEGER NOT NULL, PRIMARY KEY (ParentDirDBID, ChildDirDBID, SystemDBID) ); - INSERT INTO DBConfig (Name, Value) VALUES ('BrowseIndexVersion', '2'); + INSERT INTO DBConfig (Name, Value) VALUES ('BrowseIndexVersion', ?); INSERT INTO Systems (DBID, SystemID) VALUES (1, 'SNES'), (2, 'NES'), (3, 'C64'); INSERT INTO BrowseDirs (DBID, Path) VALUES (1, '/'), (2, '/roms'), (3, '/more-roms'); INSERT INTO BrowseDirCounts (ParentDirDBID, ChildDirDBID, SystemDBID, FileCount) VALUES (1, 2, 2, 10), (1, 2, 1, 5), (1, 3, 2, 3); - `) + `, browseCacheSchemaVersion) require.NoError(t, err) systems, err := sqlIndexedSystems(ctx, sqlDB) diff --git a/pkg/database/mediadb/sql_browse.go b/pkg/database/mediadb/sql_browse.go index 26423682b..e4f14531f 100644 --- a/pkg/database/mediadb/sql_browse.go +++ b/pkg/database/mediadb/sql_browse.go @@ -1552,9 +1552,48 @@ func sqlBrowseFileCount( db sqlQueryable, opts database.BrowseFileCountOptions, ) (int, error) { + // Letter and tag scopes need row-level predicates absent from compact cache. + // Unfiltered directory totals can use v3's parent=self direct-file rows. + if opts.Letter == nil && len(opts.Tags) == 0 && browseRouteCacheKey(opts.PathPrefix) != "/" { + ready, err := sqlBrowseCacheReady(ctx, db) + if err != nil { + return 0, err + } + if ready { + count, parentFound, cacheErr := sqlBrowseDirectFileCountFromCache(ctx, db, opts) + if cacheErr != nil || parentFound { + return count, cacheErr + } + } + } return sqlBrowseFileCountFromMedia(ctx, db, opts) } +func sqlBrowseDirectFileCountFromCache( + ctx context.Context, + db sqlQueryable, + opts database.BrowseFileCountOptions, +) (count int, parentFound bool, err error) { + parentID, ok, err := sqlBrowseDirID(ctx, db, opts.PathPrefix) + if err != nil || !ok { + return 0, ok, err + } + + args := []any{parentID} + query := `SELECT COALESCE(SUM(c.FileCount), 0) + FROM BrowseDirCounts c + INNER JOIN Systems s ON c.SystemDBID = s.DBID + WHERE c.ParentDirDBID = ? AND c.ChildDirDBID = c.ParentDirDBID` + if systemClause, systemArgs := browseSystemFilterClause("s.SystemID", opts.Systems); systemClause != "" { + query += ` AND ` + systemClause + args = append(args, systemArgs...) + } + if scanErr := db.QueryRowContext(ctx, query, args...).Scan(&count); scanErr != nil { + return 0, true, fmt.Errorf("browse cache direct file count: %w", scanErr) + } + return count, true, nil +} + func sqlBrowseFileCountFromMedia( ctx context.Context, db sqlQueryable, @@ -1966,13 +2005,18 @@ func sqlBrowseRouteCountsFromCache( args := append([]any{dirID}, systemArgs...) var count int var systemIDs sql.NullString - err = db.QueryRowContext(ctx, - `SELECT COALESCE(SUM(c.FileCount), 0), GROUP_CONCAT(DISTINCT s.SystemID) + query := `SELECT COALESCE(SUM(c.FileCount), 0), GROUP_CONCAT(DISTINCT s.SystemID) FROM BrowseDirCounts c INNER JOIN Systems s ON c.SystemDBID = s.DBID - WHERE c.ChildDirDBID = ? AND `+systemClause, - args..., - ).Scan(&count, &systemIDs) + WHERE c.ChildDirDBID = ?` + // v3 self rows are direct-file counts and would double-count a route's + // parent→child subtree total. Root's historical self row is the global + // filesystem total and remains authoritative for the root route. + if browseRouteCacheKey(route) != "/" { + query += ` AND c.ParentDirDBID != c.ChildDirDBID` + } + query += ` AND ` + systemClause + err = db.QueryRowContext(ctx, query, args...).Scan(&count, &systemIDs) if err != nil { return nil, fmt.Errorf("browse cache route counts query: %w", err) } @@ -2318,10 +2362,11 @@ func sqlBrowseRootCounts(ctx context.Context, db sqlQueryable, rootDirs []string continue } var dbCount int - if scanErr := db.QueryRowContext(ctx, - `SELECT COALESCE(SUM(FileCount), 0) FROM BrowseDirCounts WHERE ChildDirDBID = ?`, - dirID, - ).Scan(&dbCount); scanErr != nil { + query := `SELECT COALESCE(SUM(FileCount), 0) FROM BrowseDirCounts WHERE ChildDirDBID = ?` + if browseRouteCacheKey(root) != "/" { + query += ` AND ParentDirDBID != ChildDirDBID` + } + if scanErr := db.QueryRowContext(ctx, query, dirID).Scan(&dbCount); scanErr != nil { return nil, fmt.Errorf("browse cache root counts query: %w", scanErr) } c := dbCount diff --git a/pkg/database/mediadb/sql_browse_cache.go b/pkg/database/mediadb/sql_browse_cache.go index f2a9149bc..375fad6ec 100644 --- a/pkg/database/mediadb/sql_browse_cache.go +++ b/pkg/database/mediadb/sql_browse_cache.go @@ -32,7 +32,7 @@ import ( "github.com/rs/zerolog/log" ) -const browseCacheSchemaVersion = "2" +const browseCacheSchemaVersion = "3" // browseCacheInvalidatedVersion is the sentinel written to // DBConfig.BrowseIndexVersion when the cache is marked stale (e.g. media changed @@ -244,7 +244,12 @@ type browseCacheCountPair struct { func (b *browseCacheBuilder) countPairsForPath(mediaPath string) []browseCacheCountPair { mediaPath = browseCacheNormalizePath(mediaPath) if idx := strings.Index(mediaPath, "://"); idx >= 0 { - return []browseCacheCountPair{{parent: b.ensureDir("/"), child: b.ensureDir(mediaPath[:idx+3])}} + root := b.ensureDir("/") + scheme := b.ensureDir(mediaPath[:idx+3]) + return []browseCacheCountPair{ + {parent: root, child: scheme}, + {parent: scheme, child: scheme}, + } } dirs := browseCacheAncestorDirs(mediaPath) @@ -257,6 +262,14 @@ func (b *browseCacheBuilder) countPairsForPath(mediaPath string) []browseCacheCo child: b.ensureDir(dirs[i+1]), }) } + // A self-pair on the media's immediate parent stores direct-child file + // count. Parent→child pairs intentionally remain subtree counts for route + // discovery; keeping both shapes lets media.browse answer totalFiles without + // scanning a large Media partition on every cold first page. + if len(dirs) > 1 { + leaf := b.ensureDir(dirs[len(dirs)-1]) + pairs = append(pairs, browseCacheCountPair{parent: leaf, child: leaf}) + } return pairs } @@ -449,6 +462,21 @@ func sqlPopulateBrowseCacheForSystems(ctx context.Context, db *sql.DB, systemDBI } defer func() { _ = tx.Rollback() }() + compatible, err := sqlBrowseCacheVersionCompatible(ctx, tx) + if err != nil { + return err + } + if !compatible { + for _, stmt := range []string{ + "DELETE FROM BrowseDirCounts", + "DELETE FROM BrowseDirs", + } { + if _, execErr := tx.ExecContext(ctx, stmt); execErr != nil { + return fmt.Errorf("browse cache: failed to clear incompatible cache: %w", execErr) + } + } + } + firstNewID, err := loadBrowseCacheDirs(ctx, tx, builder) if err != nil { return err @@ -508,11 +536,30 @@ func sqlPopulateBrowseCacheForSystems(ctx context.Context, db *sql.DB, systemDBI return nil } +func sqlBrowseCacheVersionCompatible(ctx context.Context, db sqlQueryable) (bool, error) { + var version string + err := db.QueryRowContext(ctx, + "SELECT Value FROM DBConfig WHERE Name = ?", + DBConfigBrowseIndexVersion, + ).Scan(&version) + if err == sql.ErrNoRows { + return false, nil + } + if err != nil { + return false, fmt.Errorf("browse cache version query: %w", err) + } + return version == browseCacheSchemaVersion || version == browseCacheInvalidatedVersion, nil +} + func sqlInvalidateBrowseCache(ctx context.Context, db sqlQueryable) error { - _, err := db.ExecContext(ctx, - "INSERT OR REPLACE INTO DBConfig (Name, Value) VALUES (?, ?)", + _, err := db.ExecContext(ctx, ` + INSERT INTO DBConfig (Name, Value) VALUES (?, ?) + ON CONFLICT(Name) DO UPDATE SET Value = excluded.Value + WHERE DBConfig.Value IN (?, ?)`, DBConfigBrowseIndexVersion, browseCacheInvalidatedVersion, + browseCacheSchemaVersion, + browseCacheInvalidatedVersion, ) if err != nil { return fmt.Errorf("failed to mark browse cache stale: %w", err) From 9aed7eac45889879b09fbf8b5aa2776308933448 Mon Sep 17 00:00:00 2001 From: Callan Barrett Date: Mon, 17 Aug 2026 04:22:26 +0800 Subject: [PATCH 2/4] fix(api): address media response review feedback --- docs/api/methods.md | 6 +- pkg/api/methods/media_image.go | 4 +- pkg/api/methods/media_image_test.go | 74 +++++++++++++++++++ pkg/api/methods/media_meta_test.go | 67 +++++++++++++++++ .../mediadb/mediadb_integration_test.go | 64 ++++++++++++++++ pkg/database/mediadb/optimization_test.go | 3 + pkg/database/mediadb/sql_browse.go | 50 ++++++++++++- pkg/database/mediadb/sql_browse_cache.go | 22 +++++- pkg/database/mediadb/sql_config.go | 1 + 9 files changed, 280 insertions(+), 11 deletions(-) diff --git a/docs/api/methods.md b/docs/api/methods.md index 2689048fb..e16334050 100644 --- a/docs/api/methods.md +++ b/docs/api/methods.md @@ -795,6 +795,7 @@ All parameters are optional. When called with no parameters, returns root entrie "path": "/roms/SNES", "type": "root", "fileCount": 150, + "hasCover": false, "systemId": "SNES", "systemIds": ["SNES"] } @@ -833,7 +834,8 @@ All parameters are optional. When called with no parameters, returns root entrie "name": "RPGs", "path": "/roms/SNES/RPGs", "type": "directory", - "fileCount": 42 + "fileCount": 42, + "hasCover": false }, { "mediaId": 42, @@ -841,6 +843,7 @@ All parameters are optional. When called with no parameters, returns root entrie "path": "/roms/SNES/Super Mario World.sfc", "type": "media", "systemId": "SNES", + "hasCover": true, "zapScript": "@SNES/Super Mario World", "relativePath": "Super Mario World.sfc", "tags": [ @@ -854,6 +857,7 @@ All parameters are optional. When called with no parameters, returns root entrie "path": "/roms/SNES/The Legend of Zelda - A Link to the Past.sfc", "type": "media", "systemId": "SNES", + "hasCover": false, "zapScript": "@SNES/The Legend of Zelda - A Link to the Past", "relativePath": "The Legend of Zelda - A Link to the Past.sfc", "tags": [ diff --git a/pkg/api/methods/media_image.go b/pkg/api/methods/media_image.go index 4710b8023..0093a03a6 100644 --- a/pkg/api/methods/media_image.go +++ b/pkg/api/methods/media_image.go @@ -921,11 +921,11 @@ func HandleMediaImage(env requests.RequestEnv) (result any, resultErr error) { if err != nil { return nil, err } + deliveryForLog = delivery + hasMediaID = ref.MediaID != nil if deliveryErr := validateMediaImageDelivery(delivery, ref); deliveryErr != nil { return nil, deliveryErr } - deliveryForLog = delivery - hasMediaID = ref.MediaID != nil localPath := delivery == mediaImageDeliveryPath // Snap the requested size onto a standard tier so every view shares one diff --git a/pkg/api/methods/media_image_test.go b/pkg/api/methods/media_image_test.go index 622bbb1cd..5384a7d2c 100644 --- a/pkg/api/methods/media_image_test.go +++ b/pkg/api/methods/media_image_test.go @@ -32,6 +32,7 @@ import ( "image/png" "os" "path/filepath" + "strings" "syscall" "testing" "time" @@ -44,6 +45,8 @@ import ( "github.com/ZaparooProject/zaparoo-core/v2/pkg/service/state" testhelpers "github.com/ZaparooProject/zaparoo-core/v2/pkg/testing/helpers" "github.com/ZaparooProject/zaparoo-core/v2/pkg/testing/mocks" + "github.com/rs/zerolog" + "github.com/rs/zerolog/log" "github.com/spf13/afero" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -73,6 +76,29 @@ func makeMediaImageEnv( } } +func captureHandlerLogEvent(t *testing.T, message string, run func()) map[string]any { + t.Helper() + var buf bytes.Buffer + originalLogger := log.Logger + log.Logger = zerolog.New(&buf).Level(zerolog.DebugLevel) + defer func() { log.Logger = originalLogger }() + + run() + for line := range strings.SplitSeq(strings.TrimSpace(buf.String()), "\n") { + var event map[string]any + decoder := json.NewDecoder(strings.NewReader(line)) + decoder.UseNumber() + if err := decoder.Decode(&event); err != nil { + continue + } + if event["message"] == message { + return event + } + } + t.Fatalf("log event %q not found in %q", message, buf.String()) + return nil +} + func makeMediaFullRow(mediaDBID, titleDBID int64) *database.MediaFullRow { return &database.MediaFullRow{ Media: database.Media{DBID: mediaDBID, Path: filepath.Join("games", fmt.Sprintf("test-%d.rom", mediaDBID))}, @@ -648,6 +674,54 @@ func TestImageHasTransparency(t *testing.T) { } } +func TestHandleMediaImage_TimingLog(t *testing.T) { + // Not parallel: swaps process-wide logger. + mediaImageNoImages.clear() + t.Cleanup(mediaImageNoImages.clear) + + t.Run("success", func(t *testing.T) { + var imageData bytes.Buffer + require.NoError(t, png.Encode(&imageData, image.NewRGBA(image.Rect(0, 0, 1, 1)))) + + mockDB := testhelpers.NewMockMediaDBI() + row := makeMediaFullRow(9200, 9210) + mockDB.On("GetMediaWithTitleAndSystemByIDs", mock.Anything, []int64{row.DBID}). + Return(map[int64]database.MediaFullRow{row.DBID: *row}, nil) + mockDB.On("GetMediaProperties", mock.Anything, row.DBID). + Return([]database.MediaProperty{}, nil) + mockDB.On("GetMediaTitleProperties", mock.Anything, row.Title.DBID). + Return([]database.MediaProperty{ + {TypeTag: "property:image-boxart", ContentType: "image/png", Binary: imageData.Bytes()}, + }, nil) + + env := makeMediaImageEnv(t, mockDB, json.RawMessage( + `{"mediaId":9200,"delivery":"inline","maxSize":300}`)) + event := captureHandlerLogEvent(t, "media.image handler timing", func() { + _, err := HandleMediaImage(env) + require.NoError(t, err) + }) + + assert.Equal(t, true, event["ok"]) + assert.Equal(t, mediaImageDeliveryInline, event["delivery"]) + assert.Equal(t, true, event["mediaId"]) + assert.Equal(t, json.Number("512"), event["maxSize"]) + mockDB.AssertExpectations(t) + }) + + t.Run("validation error", func(t *testing.T) { + env := makeMediaImageEnv(t, testhelpers.NewMockMediaDBI(), json.RawMessage( + `{"mediaId":9201,"delivery":"localPath"}`)) + event := captureHandlerLogEvent(t, "media.image handler timing", func() { + _, err := HandleMediaImage(env) + require.Error(t, err) + }) + + assert.Equal(t, false, event["ok"]) + assert.Equal(t, mediaImageDeliveryPath, event["delivery"]) + assert.Equal(t, true, event["mediaId"]) + }) +} + func TestHandleMediaImage_MaxSizeResizesAndCachesThumbnail(t *testing.T) { // Not parallel: installs the process-wide thumb cache pointer. fs := afero.NewMemMapFs() diff --git a/pkg/api/methods/media_meta_test.go b/pkg/api/methods/media_meta_test.go index 177d2f7ec..5cd353646 100644 --- a/pkg/api/methods/media_meta_test.go +++ b/pkg/api/methods/media_meta_test.go @@ -22,6 +22,7 @@ package methods import ( "context" "database/sql" + "encoding/json" "errors" "fmt" "path/filepath" @@ -58,6 +59,72 @@ func mediaMetaParams(row *database.MediaFullRow) string { return fmt.Sprintf(`{"system": %q, "path": %q}`, row.System.SystemID, row.Path) } +func TestHandleMediaMeta_TimingLog(t *testing.T) { + // Not parallel: captureHandlerLogEvent swaps process-wide logger. + t.Run("single", func(t *testing.T) { + mockDB := testhelpers.NewMockMediaDBI() + row := makeMediaFullRow(9300, 9310) + expectMediaMetaResolve(mockDB, row) + mockDB.On("GetMediaTagsByMediaDBID", mock.Anything, row.DBID).Return([]database.TagInfo{}, nil) + mockDB.On("GetMediaTitleTagsByMediaTitleDBID", mock.Anything, row.Title.DBID). + Return([]database.TagInfo{}, nil) + mockDB.On("GetMediaProperties", mock.Anything, row.DBID).Return([]database.MediaProperty{}, nil) + mockDB.On("GetMediaTitleProperties", mock.Anything, row.Title.DBID). + Return([]database.MediaProperty{}, nil) + + env := makeMediaMetaEnv(t, mockDB, mediaMetaParams(row)) + event := captureHandlerLogEvent(t, "media.meta handler timing", func() { + _, err := HandleMediaMeta(env) + require.NoError(t, err) + }) + assert.Equal(t, false, event["batch"]) + assert.Equal(t, json.Number("1"), event["itemCount"]) + assert.Equal(t, true, event["ok"]) + mockDB.AssertExpectations(t) + }) + + t.Run("batch", func(t *testing.T) { + mockDB := testhelpers.NewMockMediaDBI() + mockDB.On("GetMediaWithTitleAndSystemByIDs", mock.Anything, mock.Anything). + Return(map[int64]database.MediaFullRow{}, nil) + env := makeMediaMetaEnv(t, mockDB, `{"items":[{"mediaId":9301},{"mediaId":9302}]}`) + event := captureHandlerLogEvent(t, "media.meta handler timing", func() { + _, err := HandleMediaMeta(env) + require.NoError(t, err) + }) + assert.Equal(t, true, event["batch"]) + assert.Equal(t, json.Number("2"), event["itemCount"]) + assert.Equal(t, true, event["ok"]) + mockDB.AssertExpectations(t) + }) + + t.Run("parse error", func(t *testing.T) { + env := makeMediaMetaEnv(t, testhelpers.NewMockMediaDBI(), `{`) + event := captureHandlerLogEvent(t, "media.meta handler timing", func() { + _, err := HandleMediaMeta(env) + require.Error(t, err) + }) + assert.Equal(t, false, event["batch"]) + assert.Equal(t, json.Number("0"), event["itemCount"]) + assert.Equal(t, false, event["ok"]) + }) + + t.Run("downstream error", func(t *testing.T) { + mockDB := testhelpers.NewMockMediaDBI() + row := makeMediaFullRow(9303, 9330) + mockDB.On("FindSystemBySystemID", row.System.SystemID).Return(database.System{}, assert.AnError) + env := makeMediaMetaEnv(t, mockDB, mediaMetaParams(row)) + event := captureHandlerLogEvent(t, "media.meta handler timing", func() { + _, err := HandleMediaMeta(env) + require.Error(t, err) + }) + assert.Equal(t, false, event["batch"]) + assert.Equal(t, json.Number("1"), event["itemCount"]) + assert.Equal(t, false, event["ok"]) + mockDB.AssertExpectations(t) + }) +} + func TestHandleMediaMeta_FullResult(t *testing.T) { t.Parallel() diff --git a/pkg/database/mediadb/mediadb_integration_test.go b/pkg/database/mediadb/mediadb_integration_test.go index a06b99ea4..d5453c85c 100644 --- a/pkg/database/mediadb/mediadb_integration_test.go +++ b/pkg/database/mediadb/mediadb_integration_test.go @@ -3370,6 +3370,34 @@ func TestPopulateBrowseCacheForSystems_IncrementalRefresh_Integration(t *testing assert.Equal(t, 2, rootDirs[0].FileCount) } +func TestBrowseFileCount_PartialCacheFallsBackForIncompleteCoverage_Integration(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test in short mode") + } + t.Parallel() + mediaDB, cleanup := setupTempMediaDB(t) + defer cleanup() + + ctx := context.Background() + sharedDir := filepath.ToSlash(filepath.Join(string(filepath.Separator), "roms", "shared")) + "/" + insertSystemWithMedia(t, mediaDB, "SNES", "SNES Game", filepath.ToSlash(filepath.Join(sharedDir, "snes.sfc"))) + insertSystemWithMedia(t, mediaDB, "NES", "NES Game", filepath.ToSlash(filepath.Join(sharedDir, "nes.nes"))) + require.NoError(t, mediaDB.PopulateBrowseCacheForSystems(ctx, []string{"SNES"})) + + nes, err := systemdefs.GetSystem("NES") + require.NoError(t, err) + nesFiles, err := mediaDB.BrowseFileCount(ctx, database.BrowseFileCountOptions{ + PathPrefix: sharedDir, + Systems: []systemdefs.System{*nes}, + }) + require.NoError(t, err) + assert.Equal(t, 1, nesFiles, "unrefreshed requested system must use Media fallback") + + allFiles, err := mediaDB.BrowseFileCount(ctx, database.BrowseFileCountOptions{PathPrefix: sharedDir}) + require.NoError(t, err) + assert.Equal(t, 2, allFiles, "unfiltered partial cache must use Media fallback") +} + func TestSqlInvalidateBrowseCache_MarksBrowseCacheStale_Integration(t *testing.T) { if testing.Short() { t.Skip("skipping integration test in short mode") @@ -3464,6 +3492,42 @@ func TestPopulateBrowseCacheForSystems_ClearsIncompatibleCache_Integration(t *te assert.Equal(t, 1, fileCount) } +func TestSqlInvalidateBrowseCache_DoesNotCreateMissingVersion_Integration(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test in short mode") + } + t.Parallel() + mediaDB, cleanup := setupTempMediaDB(t) + defer cleanup() + + ctx := context.Background() + _, err := mediaDB.sql.Load().ExecContext(ctx, + "DELETE FROM DBConfig WHERE Name = ?", + DBConfigBrowseIndexVersion, + ) + require.NoError(t, err) + _, err = mediaDB.sql.Load().ExecContext(ctx, + "INSERT OR IGNORE INTO BrowseDirs (DBID, Path, Name, IsVirtual) VALUES (?, ?, ?, ?)", + 1, + "/", + "/", + false, + ) + require.NoError(t, err) + + require.NoError(t, sqlInvalidateBrowseCache(ctx, mediaDB.sql.Load())) + + var version string + err = mediaDB.sql.Load().QueryRowContext(ctx, + "SELECT Value FROM DBConfig WHERE Name = ?", + DBConfigBrowseIndexVersion, + ).Scan(&version) + require.ErrorIs(t, err, sql.ErrNoRows) + ready, err := sqlBrowseCacheReady(ctx, mediaDB.sql.Load()) + require.NoError(t, err) + assert.False(t, ready, "cache rows without a compatible version must not be served") +} + func TestMediaDB_UnfilteredBrowseReadsFromMediaWhenBrowseCacheEmpty_Integration(t *testing.T) { if testing.Short() { t.Skip("skipping integration test in short mode") diff --git a/pkg/database/mediadb/optimization_test.go b/pkg/database/mediadb/optimization_test.go index a1d412dd2..f09e5d5ed 100644 --- a/pkg/database/mediadb/optimization_test.go +++ b/pkg/database/mediadb/optimization_test.go @@ -82,6 +82,9 @@ func expectBrowseCacheStep(mock sqlmock.Sqlmock) { mock.ExpectExec("INSERT OR REPLACE INTO DBConfig"). WithArgs(DBConfigBrowseIndexVersion, browseCacheSchemaVersion). WillReturnResult(sqlmock.NewResult(1, 1)) + mock.ExpectExec("INSERT OR REPLACE INTO DBConfig"). + WithArgs(DBConfigBrowseIndexComplete, "1"). + WillReturnResult(sqlmock.NewResult(1, 1)) mock.ExpectCommit() } diff --git a/pkg/database/mediadb/sql_browse.go b/pkg/database/mediadb/sql_browse.go index e4f14531f..ff1e7c1ca 100644 --- a/pkg/database/mediadb/sql_browse.go +++ b/pkg/database/mediadb/sql_browse.go @@ -1560,8 +1560,8 @@ func sqlBrowseFileCount( return 0, err } if ready { - count, parentFound, cacheErr := sqlBrowseDirectFileCountFromCache(ctx, db, opts) - if cacheErr != nil || parentFound { + count, cacheUsable, cacheErr := sqlBrowseDirectFileCountFromCache(ctx, db, opts) + if cacheErr != nil || cacheUsable { return count, cacheErr } } @@ -1573,10 +1573,15 @@ func sqlBrowseDirectFileCountFromCache( ctx context.Context, db sqlQueryable, opts database.BrowseFileCountOptions, -) (count int, parentFound bool, err error) { +) (count int, cacheUsable bool, err error) { + covered, err := sqlBrowseCacheCoversSystems(ctx, db, opts.Systems) + if err != nil || !covered { + return 0, false, err + } + parentID, ok, err := sqlBrowseDirID(ctx, db, opts.PathPrefix) if err != nil || !ok { - return 0, ok, err + return 0, false, err } args := []any{parentID} @@ -1594,6 +1599,43 @@ func sqlBrowseDirectFileCountFromCache( return count, true, nil } +func sqlBrowseCacheCoversSystems( + ctx context.Context, + db sqlQueryable, + systems []systemdefs.System, +) (bool, error) { + var complete string + err := db.QueryRowContext(ctx, + "SELECT Value FROM DBConfig WHERE Name = ?", + DBConfigBrowseIndexComplete, + ).Scan(&complete) + if err != nil && err != sql.ErrNoRows { + return false, fmt.Errorf("browse cache coverage query: %w", err) + } + if complete == "1" { + return true, nil + } + if len(systems) == 0 { + return false, nil + } + + systemIDs := make([]string, len(systems)) + for i := range systems { + systemIDs[i] = systems[i].ID + } + expected := len(uniqueBrowseSystemIDs(systemIDs)) + systemClause, args := browseSystemFilterClause("s.SystemID", systems) + var covered int + query := `SELECT COUNT(DISTINCT s.SystemID) + FROM BrowseDirCounts c + INNER JOIN Systems s ON c.SystemDBID = s.DBID + WHERE ` + systemClause + if scanErr := db.QueryRowContext(ctx, query, args...).Scan(&covered); scanErr != nil { + return false, fmt.Errorf("browse cache system coverage query: %w", scanErr) + } + return covered == expected, nil +} + func sqlBrowseFileCountFromMedia( ctx context.Context, db sqlQueryable, diff --git a/pkg/database/mediadb/sql_browse_cache.go b/pkg/database/mediadb/sql_browse_cache.go index 375fad6ec..dd6d32c52 100644 --- a/pkg/database/mediadb/sql_browse_cache.go +++ b/pkg/database/mediadb/sql_browse_cache.go @@ -124,6 +124,13 @@ func sqlPopulateBrowseCache(ctx context.Context, db *sql.DB) error { ); cfgErr != nil { return fmt.Errorf("browse cache: failed to mark index ready: %w", cfgErr) } + if _, cfgErr := tx.ExecContext(ctx, + "INSERT OR REPLACE INTO DBConfig (Name, Value) VALUES (?, ?)", + DBConfigBrowseIndexComplete, + "1", + ); cfgErr != nil { + return fmt.Errorf("browse cache: failed to mark index complete: %w", cfgErr) + } commitStarted := time.Now() if err := tx.Commit(); err != nil { @@ -522,6 +529,14 @@ func sqlPopulateBrowseCacheForSystems(ctx context.Context, db *sql.DB, systemDBI ); cfgErr != nil { return fmt.Errorf("browse cache: failed to mark system refresh: %w", cfgErr) } + // Only a full rebuild proves unfiltered coverage across every system. + if _, cfgErr := tx.ExecContext(ctx, + "INSERT OR REPLACE INTO DBConfig (Name, Value) VALUES (?, ?)", + DBConfigBrowseIndexComplete, + "0", + ); cfgErr != nil { + return fmt.Errorf("browse cache: failed to mark partial index coverage: %w", cfgErr) + } if err := tx.Commit(); err != nil { return fmt.Errorf("browse cache: failed to commit system refresh: %w", err) @@ -553,11 +568,10 @@ func sqlBrowseCacheVersionCompatible(ctx context.Context, db sqlQueryable) (bool func sqlInvalidateBrowseCache(ctx context.Context, db sqlQueryable) error { _, err := db.ExecContext(ctx, ` - INSERT INTO DBConfig (Name, Value) VALUES (?, ?) - ON CONFLICT(Name) DO UPDATE SET Value = excluded.Value - WHERE DBConfig.Value IN (?, ?)`, - DBConfigBrowseIndexVersion, + UPDATE DBConfig SET Value = ? + WHERE Name = ? AND Value IN (?, ?)`, browseCacheInvalidatedVersion, + DBConfigBrowseIndexVersion, browseCacheSchemaVersion, browseCacheInvalidatedVersion, ) diff --git a/pkg/database/mediadb/sql_config.go b/pkg/database/mediadb/sql_config.go index 38b5f8674..dfabde27b 100644 --- a/pkg/database/mediadb/sql_config.go +++ b/pkg/database/mediadb/sql_config.go @@ -42,6 +42,7 @@ const ( DBConfigIndexingSystems = "IndexingSystems" DBConfigIndexingPlanSystems = "IndexingPlanSystems" DBConfigBrowseIndexVersion = "BrowseIndexVersion" + DBConfigBrowseIndexComplete = "BrowseIndexComplete" DBConfigMediaTotalCount = "MediaTotalCount" DBConfigMediaMissingCount = "MediaMissingCount" DBConfigTemporaryRepairParentDirVersion = "TemporaryRepairParentDirVersion" From 29ad995dc7fc1fe2014b6a245ffebc5d45dbf56e Mon Sep 17 00:00:00 2001 From: Callan Barrett Date: Mon, 17 Aug 2026 04:41:29 +0800 Subject: [PATCH 3/4] test(api): cover media timing and browse errors --- pkg/api/methods/media_image_test.go | 2 + pkg/api/methods/media_meta_test.go | 4 ++ pkg/database/mediadb/sql_browse_test.go | 56 +++++++++++++++++++++++++ 3 files changed, 62 insertions(+) diff --git a/pkg/api/methods/media_image_test.go b/pkg/api/methods/media_image_test.go index 5384a7d2c..eb3b87508 100644 --- a/pkg/api/methods/media_image_test.go +++ b/pkg/api/methods/media_image_test.go @@ -705,6 +705,7 @@ func TestHandleMediaImage_TimingLog(t *testing.T) { assert.Equal(t, mediaImageDeliveryInline, event["delivery"]) assert.Equal(t, true, event["mediaId"]) assert.Equal(t, json.Number("512"), event["maxSize"]) + assert.Contains(t, event, "duration") mockDB.AssertExpectations(t) }) @@ -719,6 +720,7 @@ func TestHandleMediaImage_TimingLog(t *testing.T) { assert.Equal(t, false, event["ok"]) assert.Equal(t, mediaImageDeliveryPath, event["delivery"]) assert.Equal(t, true, event["mediaId"]) + assert.Contains(t, event, "duration") }) } diff --git a/pkg/api/methods/media_meta_test.go b/pkg/api/methods/media_meta_test.go index 5cd353646..a414dccce 100644 --- a/pkg/api/methods/media_meta_test.go +++ b/pkg/api/methods/media_meta_test.go @@ -80,6 +80,7 @@ func TestHandleMediaMeta_TimingLog(t *testing.T) { assert.Equal(t, false, event["batch"]) assert.Equal(t, json.Number("1"), event["itemCount"]) assert.Equal(t, true, event["ok"]) + assert.Contains(t, event, "duration") mockDB.AssertExpectations(t) }) @@ -95,6 +96,7 @@ func TestHandleMediaMeta_TimingLog(t *testing.T) { assert.Equal(t, true, event["batch"]) assert.Equal(t, json.Number("2"), event["itemCount"]) assert.Equal(t, true, event["ok"]) + assert.Contains(t, event, "duration") mockDB.AssertExpectations(t) }) @@ -107,6 +109,7 @@ func TestHandleMediaMeta_TimingLog(t *testing.T) { assert.Equal(t, false, event["batch"]) assert.Equal(t, json.Number("0"), event["itemCount"]) assert.Equal(t, false, event["ok"]) + assert.Contains(t, event, "duration") }) t.Run("downstream error", func(t *testing.T) { @@ -121,6 +124,7 @@ func TestHandleMediaMeta_TimingLog(t *testing.T) { assert.Equal(t, false, event["batch"]) assert.Equal(t, json.Number("1"), event["itemCount"]) assert.Equal(t, false, event["ok"]) + assert.Contains(t, event, "duration") mockDB.AssertExpectations(t) }) } diff --git a/pkg/database/mediadb/sql_browse_test.go b/pkg/database/mediadb/sql_browse_test.go index f88a8912e..c3c211109 100644 --- a/pkg/database/mediadb/sql_browse_test.go +++ b/pkg/database/mediadb/sql_browse_test.go @@ -615,6 +615,62 @@ func TestSqlBrowseRouteCountsFromMedia_ReusesPresenceProbeAcrossTimeouts(t *test } } +func TestSqlBrowseRouteCountsFromMedia_PropagatesCountError(t *testing.T) { + t.Parallel() + db, mock, err := sqlmock.New() + require.NoError(t, err) + defer func() { _ = db.Close() }() + + route := browseTestPath("roms", "a") + countErr := errors.New("count failed") + mock.ExpectQuery("SELECT COUNT"). + WithArgs(browseRouteCacheKey(route), "SNES"). + WillReturnError(countErr) + + counts, err := sqlBrowseRouteCountsFromMedia(context.Background(), db, database.BrowseRouteCountsOptions{ + Routes: []string{route}, + Systems: []systemdefs.System{{ID: "SNES"}}, + }) + require.ErrorIs(t, err, countErr) + assert.Nil(t, counts) + require.NoError(t, mock.ExpectationsWereMet()) +} + +func TestSqlBrowseRouteCountsFromMedia_PropagatesCancellationDuringCount(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + probeQueries := 0 + matcher := sqlmock.QueryMatcherFunc(func(expectedSQL, actualSQL string) error { + if strings.Contains(actualSQL, "SELECT COUNT") { + cancel() + } + if strings.Contains(actualSQL, "SELECT 1") { + probeQueries++ + } + return sqlmock.QueryMatcherRegexp.Match(expectedSQL, actualSQL) + }) + db, mock, err := sqlmock.New(sqlmock.QueryMatcherOption(matcher)) + require.NoError(t, err) + defer func() { _ = db.Close() }() + + route := browseTestPath("roms", "a") + mock.ExpectQuery("SELECT COUNT"). + WithArgs(browseRouteCacheKey(route), "SNES"). + WillReturnError(context.Canceled) + + counts, err := sqlBrowseRouteCountsFromMedia(ctx, db, database.BrowseRouteCountsOptions{ + Routes: []string{route}, + Systems: []systemdefs.System{{ID: "SNES"}}, + }) + require.Error(t, err) + require.ErrorIs(t, ctx.Err(), context.Canceled) + assert.Nil(t, counts) + assert.Zero(t, probeQueries, "caller cancellation must not invoke timeout fallback") + require.NoError(t, mock.ExpectationsWereMet()) +} + func TestSqlBrowseRouteCountsFromMedia_PropagatesCancellationDuringProbe(t *testing.T) { t.Parallel() ctx, cancel := context.WithCancel(context.Background()) From e663f5ca19c9cea23dd3cf848598905775306fc4 Mon Sep 17 00:00:00 2001 From: Callan Barrett Date: Mon, 17 Aug 2026 04:51:16 +0800 Subject: [PATCH 4/4] fix(database): preserve browse cancellation errors --- pkg/database/mediadb/sql_browse.go | 4 ++-- pkg/database/mediadb/sql_browse_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/database/mediadb/sql_browse.go b/pkg/database/mediadb/sql_browse.go index ff1e7c1ca..ddcb00e7f 100644 --- a/pkg/database/mediadb/sql_browse.go +++ b/pkg/database/mediadb/sql_browse.go @@ -2124,8 +2124,8 @@ func sqlBrowseRouteCountsFromMedia( } // The caller's context (the whole request) is done: stop, don't degrade. - if ctx.Err() != nil { - return nil, fmt.Errorf("browse route counts media scan: %w", err) + if ctxErr := ctx.Err(); ctxErr != nil { + return nil, fmt.Errorf("browse route counts media scan: %w", ctxErr) } // A real error (not our sub-timeout) should surface. if !timedOut && !errors.Is(err, context.DeadlineExceeded) { diff --git a/pkg/database/mediadb/sql_browse_test.go b/pkg/database/mediadb/sql_browse_test.go index c3c211109..02b887e66 100644 --- a/pkg/database/mediadb/sql_browse_test.go +++ b/pkg/database/mediadb/sql_browse_test.go @@ -664,7 +664,7 @@ func TestSqlBrowseRouteCountsFromMedia_PropagatesCancellationDuringCount(t *test Routes: []string{route}, Systems: []systemdefs.System{{ID: "SNES"}}, }) - require.Error(t, err) + require.ErrorIs(t, err, context.Canceled) require.ErrorIs(t, ctx.Err(), context.Canceled) assert.Nil(t, counts) assert.Zero(t, probeQueries, "caller cancellation must not invoke timeout fallback")