From c6176d6c85dedee871a5dfe582e6b77ba7e394c0 Mon Sep 17 00:00:00 2001 From: Quick <31828688+Quick104@users.noreply.github.com> Date: Wed, 27 May 2026 12:48:59 -0400 Subject: [PATCH] fix(catalog): address episode catalog review feedback --- .../2026-05-27-episode-catalog-performance.md | 3 + .../episode_catalog_entries_executor.go | 35 +++++---- .../episode_catalog_entries_executor_test.go | 76 +++++++++++++++++++ .../142_episode_catalog_entries.down.sql | 2 + migrations/142_episode_catalog_entries.up.sql | 45 ++++++++--- scripts/benchmark_episode_catalog.py | 35 ++++++++- web/src/pages/LibraryBrowse.test.tsx | 10 ++- web/src/pages/LibraryBrowse.tsx | 7 -- 8 files changed, 175 insertions(+), 38 deletions(-) diff --git a/docs/superpowers/plans/2026-05-27-episode-catalog-performance.md b/docs/superpowers/plans/2026-05-27-episode-catalog-performance.md index bdb0b7d2..1f62291f 100644 --- a/docs/superpowers/plans/2026-05-27-episode-catalog-performance.md +++ b/docs/superpowers/plans/2026-05-27-episode-catalog-performance.md @@ -512,6 +512,9 @@ For each case, capture: - API response changes around count modes may require Android and Apple follow-up. Keeping the existing `total_exact=false` behavior avoids most client churn. - Person filters may need fallback initially unless episode-level people data is available and indexed. - Array GIN filters should be checked with real plans. If separate GIN indexes do too much post-filtering by library, use `btree_gin` multi-column indexes or add selective partial indexes for common libraries. +- Index maintenance, disk, and write amplification should be tracked explicitly. The btree indexes such as `idx_episode_catalog_entries_title`, `idx_episode_catalog_entries_added`, and `idx_episode_catalog_entries_air_date`, plus GIN indexes such as `idx_episode_catalog_entries_genres_gin`, trade scanner write cost and disk growth for browse latency. Measure `episode_catalog_entries` table and index size before rollout, and watch trigger update latency during large scanner runs. +- Backfill safety needs a production runbook. Treat the `episode_catalog_entries` backfill as idempotent work that can be chunked or retried with low-locking settings, capture a production-sized duration estimate before enabling it broadly, and document rollback to dropping the read model plus its triggers. +- Read-model health should be observable. Alert when `episode_catalog_entries.updated_at` lags source table changes beyond a stale threshold or refresh errors appear, fall back to the generic executor when health checks fail, and provide a remediation command to rebuild stale entries. ## Recommended Decision diff --git a/internal/catalog/episode_catalog_entries_executor.go b/internal/catalog/episode_catalog_entries_executor.go index 54fe74be..930f2b56 100644 --- a/internal/catalog/episode_catalog_entries_executor.go +++ b/internal/catalog/episode_catalog_entries_executor.go @@ -29,6 +29,7 @@ type episodeCatalogUserStatePlan struct { sortOrder string sortOnly bool requiresFullPageGate bool + requireProgressRatio bool } func (e *QueryExecutor) tryEpisodeCatalogUserStatePreviewPage( @@ -74,7 +75,7 @@ func (e *QueryExecutor) tryEpisodeCatalogUserStatePreviewPage( offset = 0 } - ctes := []string{episodeCatalogUserStateCTE(plan.source)} + ctes := []string{episodeCatalogUserStateCTE(plan)} args := []any{access.UserID, access.ProfileID, libraryID} argIdx := 4 whereParts := []string{"ece.media_folder_id = $3"} @@ -375,6 +376,7 @@ func extractEpisodeCatalogUserStatePlan(def QueryDefinition) (episodeCatalogUser if plan.source == "" { return episodeCatalogUserStatePlan{}, false, nil } + plan.requireProgressRatio = plan.source == "progress" && plan.sortOnly return plan, true, nil } if def.Match != "all" { @@ -423,6 +425,7 @@ func extractEpisodeCatalogUserStatePlan(def QueryDefinition) (episodeCatalogUser return episodeCatalogUserStatePlan{}, false, nil } plan.entryDef.Groups = filteredGroups + plan.requireProgressRatio = plan.source == "progress" && plan.sortOnly return plan, true, nil } @@ -513,24 +516,28 @@ func rebindUserStateClauses(clauses []string, argIdx int) []string { return out } -func episodeCatalogUserStateCTE(source string) string { - switch source { +func episodeCatalogUserStateCTE(plan episodeCatalogUserStatePlan) string { + switch plan.source { case "progress": - return `user_progress AS ( - SELECT uwp.media_item_id AS episode_id, - uwp.position_seconds::double precision / NULLIF(uwp.duration_seconds, 0) AS progress_ratio - FROM user_watch_progress uwp + progressRatioGate := "" + if plan.requireProgressRatio { + progressRatioGate = ` + AND COALESCE(uwp.duration_seconds, 0) > 0` + } + return fmt.Sprintf(`user_progress AS ( + SELECT uwp.media_item_id AS episode_id, + uwp.position_seconds::double precision / NULLIF(uwp.duration_seconds, 0) AS progress_ratio + FROM user_watch_progress uwp LEFT JOIN user_history_hidden_items hhi ON hhi.user_id = $1 AND hhi.profile_id = $2 AND hhi.media_item_id = uwp.media_item_id WHERE uwp.user_id = $1 - AND uwp.profile_id = $2 - AND uwp.completed = FALSE - AND uwp.position_seconds > 0 - AND COALESCE(uwp.duration_seconds, 0) > 0 - AND (hhi.media_item_id IS NULL OR uwp.updated_at > hhi.hidden_before) - )` + AND uwp.profile_id = $2 + AND uwp.completed = FALSE + AND uwp.position_seconds > 0%s + AND (hhi.media_item_id IS NULL OR uwp.updated_at > hhi.hidden_before) + )`, progressRatioGate) default: return `user_viewed AS ( SELECT src.episode_id, @@ -810,7 +817,7 @@ func buildEpisodeCatalogEntryRuleWhere(rule QueryRule, argIdx int) (string, []an case "content_rating": return buildEpisodeCatalogEqualityClause("ece.content_rating", rule, argIdx) case "added_at": - return buildEpisodeCatalogComparisonClause("ece.added_at", rule, argIdx, "timestamptz") + return buildEpisodeCatalogComparisonClause("ece.episode_created_at", rule, argIdx, "timestamptz") case "release_date": return buildEpisodeCatalogComparisonClause("ece.episode_air_date", rule, argIdx, "date") case "status": diff --git a/internal/catalog/episode_catalog_entries_executor_test.go b/internal/catalog/episode_catalog_entries_executor_test.go index 964c7a25..f255f7ce 100644 --- a/internal/catalog/episode_catalog_entries_executor_test.go +++ b/internal/catalog/episode_catalog_entries_executor_test.go @@ -80,6 +80,32 @@ func TestBuildEpisodeCatalogEntryQueryWhereUsesReadModelFilters(t *testing.T) { } } +func TestEpisodeCatalogAddedAtFilterUsesEpisodeCreatedAt(t *testing.T) { + def := QueryDefinition{ + Match: "all", + Groups: []QueryGroup{{ + Match: "all", + Rules: []QueryRule{ + {Field: "added_at", Op: "in_last", Value: "30d"}, + }, + }}, + }.Normalize() + + where, _, _, ok, err := buildEpisodeCatalogEntryQueryWhere(def, 2) + if err != nil { + t.Fatalf("buildEpisodeCatalogEntryQueryWhere returned error: %v", err) + } + if !ok { + t.Fatal("buildEpisodeCatalogEntryQueryWhere unexpectedly fell back") + } + if !strings.Contains(where, "ece.episode_created_at >= NOW() - INTERVAL '30 days'") { + t.Fatalf("expected added_at filter to use episode_created_at, got %q", where) + } + if strings.Contains(where, "ece.added_at") { + t.Fatalf("added_at filter must not use library first_seen_at, got %q", where) + } +} + func TestEpisodeCatalogEntryQueryFallsBackForSameFileTechnicalAnd(t *testing.T) { group := QueryGroup{ Match: "all", @@ -98,6 +124,56 @@ func TestEpisodeCatalogEntryQueryFallsBackForSameFileTechnicalAnd(t *testing.T) } } +func TestEpisodeCatalogInProgressPlanKeepsUnknownDurationRows(t *testing.T) { + def := QueryDefinition{ + Match: "all", + Groups: []QueryGroup{{ + Match: "all", + Rules: []QueryRule{ + {Field: "in_progress", Op: "is", Value: true}, + }, + }}, + Sort: QuerySort{Field: "title", Order: "asc"}, + }.Normalize() + + plan, ok, err := extractEpisodeCatalogUserStatePlan(def) + if err != nil { + t.Fatalf("extractEpisodeCatalogUserStatePlan returned error: %v", err) + } + if !ok { + t.Fatal("expected in_progress filter to use a user-state plan") + } + sql := episodeCatalogUserStateCTE(plan) + if strings.Contains(sql, "COALESCE(uwp.duration_seconds, 0) > 0") { + t.Fatalf("in_progress filter must keep unknown-duration rows, got CTE:\n%s", sql) + } +} + +func TestEpisodeCatalogProgressSortRequiresProgressRatio(t *testing.T) { + def := QueryDefinition{ + Match: "all", + Groups: []QueryGroup{{ + Match: "all", + Rules: []QueryRule{ + {Field: "genre", Op: "is", Value: "Comedy"}, + }, + }}, + Sort: QuerySort{Field: "progress", Order: "desc"}, + }.Normalize() + + plan, ok, err := extractEpisodeCatalogUserStatePlan(def) + if err != nil { + t.Fatalf("extractEpisodeCatalogUserStatePlan returned error: %v", err) + } + if !ok { + t.Fatal("expected progress sort to use a user-state plan") + } + sql := episodeCatalogUserStateCTE(plan) + if !strings.Contains(sql, "COALESCE(uwp.duration_seconds, 0) > 0") { + t.Fatalf("progress sort must require a computable ratio before falling back, got CTE:\n%s", sql) + } +} + func TestExtractEpisodeCatalogUserStatePlanForLastWatched(t *testing.T) { def := QueryDefinition{ Match: "all", diff --git a/migrations/142_episode_catalog_entries.down.sql b/migrations/142_episode_catalog_entries.down.sql index 8165f97e..e93f6816 100644 --- a/migrations/142_episode_catalog_entries.down.sql +++ b/migrations/142_episode_catalog_entries.down.sql @@ -1,5 +1,7 @@ DROP TRIGGER IF EXISTS trg_episode_catalog_entries_series ON public.media_items; DROP TRIGGER IF EXISTS trg_episode_catalog_entries_episodes ON public.episodes; +DROP TRIGGER IF EXISTS trg_episode_catalog_entries_media_files_update ON public.media_files; +DROP TRIGGER IF EXISTS trg_episode_catalog_entries_media_files_insert_delete ON public.media_files; DROP TRIGGER IF EXISTS trg_episode_catalog_entries_media_files ON public.media_files; DROP TRIGGER IF EXISTS trg_episode_catalog_entries_episode_libraries ON public.episode_libraries; diff --git a/migrations/142_episode_catalog_entries.up.sql b/migrations/142_episode_catalog_entries.up.sql index e601a016..0020addb 100644 --- a/migrations/142_episode_catalog_entries.up.sql +++ b/migrations/142_episode_catalog_entries.up.sql @@ -183,8 +183,8 @@ BEGIN ) AS resolution_codes, MAX(mf.bitrate) FILTER (WHERE mf.bitrate IS NOT NULL AND mf.bitrate > 0) AS max_bitrate, MIN(mf.bitrate) FILTER (WHERE mf.bitrate IS NOT NULL AND mf.bitrate > 0) AS min_bitrate, - BOOL_OR(COALESCE(mf.hdr, false)) AS has_hdr, - BOOL_OR(NOT COALESCE(mf.hdr, false)) AS has_non_hdr, + BOOL_OR(mf.hdr IS TRUE) AS has_hdr, + BOOL_OR(mf.hdr IS FALSE) AS has_non_hdr, BOOL_OR(EXISTS ( SELECT 1 FROM jsonb_array_elements(COALESCE(mf.video_tracks, '[]'::jsonb)) AS vt @@ -376,7 +376,7 @@ WITH active_episode_files AS ( public.episode_catalog_normalized_resolution(mf.resolution) AS normalized_resolution, public.episode_catalog_resolution_rank(mf.resolution) AS resolution_rank, mf.bitrate, - COALESCE(mf.hdr, false) AS hdr, + mf.hdr, EXISTS ( SELECT 1 FROM jsonb_array_elements(COALESCE(mf.video_tracks, '[]'::jsonb)) AS vt @@ -398,8 +398,8 @@ file_stats AS ( FILTER (WHERE normalized_resolution IS NOT NULL) AS resolution_codes, MAX(bitrate) FILTER (WHERE bitrate IS NOT NULL AND bitrate > 0) AS max_bitrate, MIN(bitrate) FILTER (WHERE bitrate IS NOT NULL AND bitrate > 0) AS min_bitrate, - BOOL_OR(hdr) AS has_hdr, - BOOL_OR(NOT hdr) AS has_non_hdr, + BOOL_OR(hdr IS TRUE) AS has_hdr, + BOOL_OR(hdr IS FALSE) AS has_non_hdr, BOOL_OR(has_dolby_vision) AS has_dolby_vision, BOOL_OR(NOT has_dolby_vision) AS has_non_dolby_vision FROM active_episode_files @@ -595,14 +595,32 @@ AFTER INSERT OR UPDATE OR DELETE ON public.episode_libraries FOR EACH ROW EXECUTE FUNCTION public.episode_catalog_entries_episode_libraries_trigger(); DROP TRIGGER IF EXISTS trg_episode_catalog_entries_media_files ON public.media_files; -CREATE TRIGGER trg_episode_catalog_entries_media_files -AFTER INSERT OR UPDATE OF episode_id, media_folder_id, missing_since, resolution, bitrate, hdr, video_tracks, audio_tracks, subtitle_tracks, external_subtitles, created_at OR DELETE -ON public.media_files +DROP TRIGGER IF EXISTS trg_episode_catalog_entries_media_files_insert_delete ON public.media_files; +DROP TRIGGER IF EXISTS trg_episode_catalog_entries_media_files_update ON public.media_files; +CREATE TRIGGER trg_episode_catalog_entries_media_files_insert_delete +AFTER INSERT OR DELETE ON public.media_files FOR EACH ROW EXECUTE FUNCTION public.episode_catalog_entries_media_files_trigger(); +CREATE TRIGGER trg_episode_catalog_entries_media_files_update +AFTER UPDATE ON public.media_files +FOR EACH ROW +WHEN ( + OLD.episode_id IS DISTINCT FROM NEW.episode_id OR + OLD.media_folder_id IS DISTINCT FROM NEW.media_folder_id OR + OLD.missing_since IS DISTINCT FROM NEW.missing_since OR + OLD.resolution IS DISTINCT FROM NEW.resolution OR + OLD.bitrate IS DISTINCT FROM NEW.bitrate OR + OLD.hdr IS DISTINCT FROM NEW.hdr OR + OLD.video_tracks IS DISTINCT FROM NEW.video_tracks OR + OLD.external_subtitles IS DISTINCT FROM NEW.external_subtitles OR + OLD.audio_language_codes IS DISTINCT FROM NEW.audio_language_codes OR + OLD.subtitle_language_codes IS DISTINCT FROM NEW.subtitle_language_codes +) +EXECUTE FUNCTION public.episode_catalog_entries_media_files_trigger(); + DROP TRIGGER IF EXISTS trg_episode_catalog_entries_episodes ON public.episodes; CREATE TRIGGER trg_episode_catalog_entries_episodes -AFTER INSERT OR UPDATE OF content_id, series_id, title, air_date, runtime, rating_imdb, rating_tmdb, still_path, still_thumbhash, created_at OR DELETE +AFTER INSERT OR UPDATE OF content_id, series_id, title, episode_number, air_date, runtime, rating_imdb, rating_tmdb, still_path, still_thumbhash, created_at OR DELETE ON public.episodes FOR EACH ROW EXECUTE FUNCTION public.episode_catalog_entries_episodes_trigger(); @@ -639,6 +657,15 @@ changed_entries AS ( CROSS JOIN migration_clock mc WHERE si.updated_at >= mc.started_at AND si.type = 'series' + UNION + SELECT DISTINCT ece.episode_id, ece.media_folder_id + FROM public.episode_catalog_entries ece + WHERE NOT EXISTS ( + SELECT 1 + FROM public.episode_libraries el + WHERE el.episode_id = ece.episode_id + AND el.media_folder_id = ece.media_folder_id + ) ) SELECT COUNT(*) FROM ( diff --git a/scripts/benchmark_episode_catalog.py b/scripts/benchmark_episode_catalog.py index 0fca64dc..d6ff54d0 100644 --- a/scripts/benchmark_episode_catalog.py +++ b/scripts/benchmark_episode_catalog.py @@ -126,7 +126,14 @@ def getenv_required(name: str) -> str: return value +def validate_base_url(base_url: str) -> None: + parsed = urllib.parse.urlparse(base_url) + if parsed.scheme not in {"http", "https"} or not parsed.netloc: + raise ValueError("SILO_BASE_URL must be an http or https URL") + + def build_url(config: Config, params: dict[str, str], include_total: bool) -> str: + validate_base_url(config.base_url) query = { "source": "query", "type": "episode", @@ -137,7 +144,8 @@ def build_url(config: Config, params: dict[str, str], include_total: bool) -> st } if not include_total: query["include_total"] = "false" - return urllib.parse.urljoin(config.base_url.rstrip("/") + "/", "api/v1/catalog") + "?" + urllib.parse.urlencode(query) + api_url = urllib.parse.urljoin(config.base_url.rstrip("/") + "/", "api/v1/catalog") + return api_url + "?" + urllib.parse.urlencode(query) def fetch_case(config: Config, case: str, params: dict[str, str], include_total: bool) -> CaseResult: @@ -151,16 +159,31 @@ def fetch_case(config: Config, case: str, params: dict[str, str], include_total: }, ) started = time.perf_counter() + status = 0 try: with urllib.request.urlopen(request, timeout=config.timeout) as response: + status = response.status body = response.read() elapsed_ms = (time.perf_counter() - started) * 1000 - payload = json.loads(body) - items = payload.get("items") + try: + payload = json.loads(body) + items = payload.get("items") + except (json.JSONDecodeError, AttributeError, TypeError) as err: + return CaseResult( + case, + include_total, + status, + elapsed_ms, + None, + None, + None, + None, + str(err), + ) return CaseResult( case=case, include_total=include_total, - status=response.status, + status=status, elapsed_ms=elapsed_ms, total=payload.get("total"), total_exact=payload.get("total_exact"), @@ -225,6 +248,10 @@ def main() -> int: offset=args.offset, timeout=args.timeout, ) + try: + validate_base_url(config.base_url) + except ValueError as err: + raise SystemExit(str(err)) from err cases = DEFAULT_SORT_CASES + DEFAULT_FILTER_CASES include_total_values = [False, True] if args.include_exact else [False] diff --git a/web/src/pages/LibraryBrowse.test.tsx b/web/src/pages/LibraryBrowse.test.tsx index 8d2ecac0..c48a4d58 100644 --- a/web/src/pages/LibraryBrowse.test.tsx +++ b/web/src/pages/LibraryBrowse.test.tsx @@ -69,7 +69,7 @@ describe("LibraryBrowse", () => { expect(mocks.useCatalogWindow).toHaveBeenCalled(); }); - it("shows the filtered item count from the catalog result", () => { + it("does not show the estimated item count as an exact result count", () => { mocks.useCatalogWindow.mockReturnValue({ data: { totalItems: 1234, @@ -95,10 +95,11 @@ describe("LibraryBrowse", () => { />, ); - expect(markup).toContain("1,234 items"); + expect(markup).toContain("Filters"); + expect(markup).not.toContain("1,234 items"); }); - it("shows a loading state for the item count while catalog data loads", () => { + it("does not show an item count loading state when exact totals are skipped", () => { mocks.useCatalogWindow.mockReturnValue({ data: { totalItems: 0, @@ -124,7 +125,8 @@ describe("LibraryBrowse", () => { />, ); - expect(markup).toContain("Loading item count"); + expect(markup).toContain("Filters"); + expect(markup).not.toContain("Loading item count"); expect(markup).not.toContain("0 items"); }); diff --git a/web/src/pages/LibraryBrowse.tsx b/web/src/pages/LibraryBrowse.tsx index cabbdb58..c0cd4316 100644 --- a/web/src/pages/LibraryBrowse.tsx +++ b/web/src/pages/LibraryBrowse.tsx @@ -38,10 +38,6 @@ function getLibrarySortRelevanceScope( return "all"; } -function formatItemCount(count: number): string { - return `${count.toLocaleString()} item${count === 1 ? "" : "s"}`; -} - export default function LibraryBrowse({ libraryId, libraryType, @@ -112,7 +108,6 @@ export default function LibraryBrowse({ const totalItems = catalogQuery.data?.totalItems ?? 0; const pages = catalogQuery.data?.pages ?? new Map(); const isLoading = catalogQuery.isLoading; - const itemCountLabel = formatItemCount(totalItems); return (
@@ -146,8 +141,6 @@ export default function LibraryBrowse({ allowPersonalizedFilters allowPersonalizedSorts sortRelevanceScope={sortRelevanceScope} - resultCountLabel={itemCountLabel} - resultCountLoading={isLoading} />