From d6f0aa89360df4b24803aadcbc0ffd80ed01ddff Mon Sep 17 00:00:00 2001 From: edde746 <86283021+edde746@users.noreply.github.com> Date: Sun, 6 Sep 2026 00:53:55 +0200 Subject: [PATCH] fix(catalog): keep library copies held by a server that was never asked A title's copy disappeared from a catalog item's library matches when the server holding it went offline, and the screen then claimed the title was not in the library at all. The reverse-lookup fan-out only reaches online clients, so a registered server that is offline lands in neither the succeeded, failed nor cancelled set. The fold read that absence as "left the account" and dropped the server's verified copies. Worse, the surviving wave looked complete, so it was memoized for the rest of the profile session and never asked again. With no server online at all, every server's copies were erased and the detail screen asserted "Not in your library" over servers nobody had queried. LibraryLookupResult now names the registered servers a wave could not even reach, which needs a registered-server set on MultiServerManager because an auth-rejected Plex server holds no client at all. The fold's rule is stated positively -- only a server that answered may replace its own entry -- a wave that skipped a server is never memoized past the TTL, and the detail screen counts those servers as unchecked alongside the ones that failed. --- lib/screens/catalog_item_detail_screen.dart | 4 + .../catalog/catalog_library_matcher.dart | 50 ++++-- lib/services/data_aggregation_service.dart | 40 ++++- lib/services/multi_server_manager.dart | 8 + .../catalog_item_detail_screen_test.dart | 19 +++ .../catalog/catalog_library_matcher_test.dart | 156 ++++++++++++++++++ test/services/multi_server_manager_test.dart | 35 ++++ test/test_helpers/library_lookup.dart | 12 +- 8 files changed, 300 insertions(+), 24 deletions(-) diff --git a/lib/screens/catalog_item_detail_screen.dart b/lib/screens/catalog_item_detail_screen.dart index de3bd2f8c..0539991b4 100644 --- a/lib/screens/catalog_item_detail_screen.dart +++ b/lib/screens/catalog_item_detail_screen.dart @@ -189,9 +189,13 @@ class _CatalogItemDetailScreenState extends State { } if (!mounted) return; _checkedServerIds.addAll(result.succeededServerIds); + // A server that was never asked — offline, so absent from the wave — is + // as unchecked as one that failed; without it "not in your library" + // would be asserted over a server that never answered. _uncheckedServerIds ..addAll(result.failedServerIds) ..addAll(result.cancelledServerIds) + ..addAll(result.unqueriedServerIds) ..removeAll(_checkedServerIds); _mergeMatches(result.items); } diff --git a/lib/services/catalog/catalog_library_matcher.dart b/lib/services/catalog/catalog_library_matcher.dart index 619871e28..226c4994f 100644 --- a/lib/services/catalog/catalog_library_matcher.dart +++ b/lib/services/catalog/catalog_library_matcher.dart @@ -13,12 +13,14 @@ import '../data_aggregation_service.dart'; /// One reverse-lookup fan-out per tap (see /// `DataAggregationService.findByExternalIdsAcrossServers`), memoized for /// the session per server: a server's answer is replaced only by that -/// server. A complete positive wave is kept (library membership rarely -/// shrinks mid-session); negatives expire so newly-added media is picked up, -/// and so does any wave a server sat out — a slow or unreachable server is -/// not evidence of absence, and the next tap after [negativeTtl] asks it -/// again while its last-known copies stay on screen. Profile-scoped via the -/// provider subtree, so a profile switch drops the cache by construction. +/// server. A wave every registered server answered is kept when positive +/// (library membership rarely shrinks mid-session); negatives expire so +/// newly-added media is picked up, and so does any wave a server sat out — +/// a server that failed, was cancelled, or was never asked at all because +/// it is offline is not evidence of absence, and the next tap after +/// [negativeTtl] asks it again while its last-known copies stay on screen. +/// Profile-scoped via the provider subtree, so a profile switch drops the +/// cache by construction. class CatalogLibraryMatcher { static const Duration negativeTtl = Duration(minutes: 10); @@ -68,18 +70,29 @@ class CatalogLibraryMatcher { return entry.result; } - /// Fold a wave into the last-known per-server answers. Servers in the - /// failed or cancelled sets keep their previous copies — a timeout is not - /// evidence of removal (#2098) — and their sets pass through so the UI - /// still names the outage and the wave is retried after [negativeTtl]. - /// Every other server's entry is replaced by what it returned this time: - /// nothing for a server named in no set, which has left the account. + /// Fold a wave into the last-known per-server answers. + /// + /// Stated positively: only a server in the succeeded set may replace its + /// own entry, because it is the only one that just answered. Servers in + /// the failed, cancelled or unqueried sets keep their previous copies — a + /// timeout, a client abort, and a registered server that was offline and + /// so never asked at all are none of them evidence of removal (#2098) — + /// and their sets pass through so the UI still names them unchecked and + /// the wave is retried after [negativeTtl]. Everything else is dropped: + /// a server in none of the four sets is no longer on the account. _Entry _fold(Map>? previous, LibraryLookupResult result) { + bool sawNoAnswerFrom(String? serverId) => + result.failedServerIds.contains(serverId) || + result.cancelledServerIds.contains(serverId) || + result.unqueriedServerIds.contains(serverId); final byServer = >{ if (previous != null) for (final MapEntry(:key, :value) in previous.entries) - if (result.failedServerIds.contains(key) || result.cancelledServerIds.contains(key)) key: value, + if (sawNoAnswerFrom(key)) key: value, }; + // Carried copies are absent from `result.items`, so a wave that kept any + // has to be rebuilt around them; with nothing carried the wave already + // is the whole answer. final carried = byServer.isNotEmpty; for (final item in result.items) { (byServer[item.serverId] ??= []).add(item); @@ -93,6 +106,7 @@ class CatalogLibraryMatcher { succeededServerIds: result.succeededServerIds, cancelledServerIds: result.cancelledServerIds, failedServerIds: result.failedServerIds, + unqueriedServerIds: result.unqueriedServerIds, ) : result, ); @@ -114,14 +128,22 @@ class CatalogLibraryMatcher { /// to ask again. static List lookupTitles(CatalogItem item) => titleMatchCandidates([item.originalTitle, item.title]); + /// Whether [result] may be memoized for the rest of the session: a hit + /// every registered server took part in. One that sat out — failed, + /// cancelled or never asked — leaves the answer incomplete, so it expires + /// with [negativeTtl] instead of being cached until the profile switches. static bool _isAuthoritativeHit(LibraryLookupResult result) => - result.items.isNotEmpty && result.failedServerIds.isEmpty && result.cancelledServerIds.isEmpty; + result.items.isNotEmpty && + result.failedServerIds.isEmpty && + result.cancelledServerIds.isEmpty && + result.unqueriedServerIds.isEmpty; static const LibraryLookupResult _nothing = ( items: [], succeededServerIds: {}, cancelledServerIds: {}, failedServerIds: {}, + unqueriedServerIds: {}, ); /// The exact `plex://` guid for a Plex Discover item, which its own rating diff --git a/lib/services/data_aggregation_service.dart b/lib/services/data_aggregation_service.dart index eb74f4868..171fa6b7c 100644 --- a/lib/services/data_aggregation_service.dart +++ b/lib/services/data_aggregation_service.dart @@ -65,12 +65,16 @@ typedef SearchAggregationResult = ({ /// One reverse-lookup wave over every online server. `items` are the /// id-verified copies, deduped and ordered best-first; a server in /// `failedServerIds` or `cancelledServerIds` contributed nothing and says -/// nothing about what it holds. +/// nothing about what it holds. `unqueriedServerIds` names the registered +/// servers the wave could not even reach out to — offline, or auth-rejected +/// with no client at all — which say just as little: a caller must treat +/// them as unchecked, never as evidence of absence. typedef LibraryLookupResult = ({ List items, Set succeededServerIds, Set cancelledServerIds, Set failedServerIds, + Set unqueriedServerIds, }); typedef _FanOutResult = ({ List items, @@ -805,11 +809,14 @@ class DataAggregationService { /// (#1754). Results are deduped by global key and ordered best-first with /// [compareLibraryCopies] so the chooser is stable across repeated passes. /// - /// A server that could not be asked — slow past its lookup deadline, - /// unreachable, or aborted client-side — is reported in the failed or - /// cancelled set rather than folded into an empty answer (#2098): the - /// caller shows it as unchecked, and a caller holding earlier results must - /// merge rather than replace (see [mergeLibraryCopies]). + /// A server that could not be asked is reported rather than folded into an + /// empty answer (#2098): slow past its lookup deadline, unreachable, or + /// aborted client-side lands in the failed or cancelled set, and a + /// registered server the wave never even reached — offline, or auth- + /// rejected before a client existed — lands in `unqueriedServerIds`, since + /// [MultiServerManager.onlineClients] is the whole world a wave sees. The + /// caller shows all of them as unchecked, and a caller holding earlier + /// results must merge rather than replace (see [mergeLibraryCopies]). Future findByExternalIdsAcrossServers( ExternalIds ids, { required MediaKind kind, @@ -818,13 +825,29 @@ class DataAggregationService { String? plexGuid, ExternalSeasonRef? season, }) async { - final clients = _serverManager.onlineClients; - if ((!ids.hasAny && plexGuid == null) || clients.isEmpty) { + // Nothing to ask with: no server was skipped, the query itself is empty. + if (!ids.hasAny && plexGuid == null) { return ( items: const [], succeededServerIds: const {}, cancelledServerIds: const {}, failedServerIds: const {}, + unqueriedServerIds: const {}, + ); + } + + final clients = _serverManager.onlineClients; + final unqueriedServerIds = { + for (final serverId in _serverManager.registeredServerIds) + if (!clients.containsKey(serverId)) serverId, + }; + if (clients.isEmpty) { + return ( + items: const [], + succeededServerIds: const {}, + cancelledServerIds: const {}, + failedServerIds: const {}, + unqueriedServerIds: unqueriedServerIds, ); } @@ -839,6 +862,7 @@ class DataAggregationService { succeededServerIds: fetched.succeededServerIds, cancelledServerIds: fetched.cancelledServerIds, failedServerIds: fetched.failedServerIds, + unqueriedServerIds: unqueriedServerIds, ); } diff --git a/lib/services/multi_server_manager.dart b/lib/services/multi_server_manager.dart index f1bc56a32..cd3b386b8 100644 --- a/lib/services/multi_server_manager.dart +++ b/lib/services/multi_server_manager.dart @@ -186,6 +186,14 @@ class MultiServerManager { /// MediaBrowser-only profiles. List get serverIds => _clients.keys.toList(); + /// Every server this profile has registered, whether or not it currently + /// holds a client. A startup auth failure ([markPlexConnectionAuthError]) + /// and a token-refresh/connect failure register a Plex server with no + /// [_clients] entry at all, so [serverIds] cannot see it. Callers that must + /// distinguish "this server said nothing" from "this server was never + /// asked" — a fan-out only ever reaches [onlineClients] — need this set. + Set get registeredServerIds => {..._clients.keys, ..._plexServers.keys, ..._authErrorServers}; + List get onlineServerIds => _serverStatus.entries.where((e) => e.value).map((e) => e.key).toList(); List get offlineServerIds => _serverStatus.entries.where((e) => !e.value).map((e) => e.key).toList(); diff --git a/test/screens/catalog_item_detail_screen_test.dart b/test/screens/catalog_item_detail_screen_test.dart index 8da561db3..631cc967f 100644 --- a/test/screens/catalog_item_detail_screen_test.dart +++ b/test/screens/catalog_item_detail_screen_test.dart @@ -534,6 +534,25 @@ void main() { expect(find.text(t.explore.libraryCheckFailed(n: 1)), findsOneWidget); }); + testWidgets('a server that was never asked is reported instead of counted as a miss', (tester) async { + // An offline server is not in the fan-out at all, so it lands in no + // failed or cancelled set — but "Not in your library" is still a false + // claim about a server that never answered. + final source = _FakeCatalogSource(detail: const CatalogDetail(item: _enrichedRow)); + + await _pumpDetail( + tester, + source, + item: _bareRow, + matcherBuilder: (multiServer) => _ScriptedMatcher(multiServer, [ + () => libraryLookupResult(const [], unqueried: {'server-1'}), + ]), + ); + + expect(find.text(t.explore.notInLibrary), findsNothing); + expect(find.text(t.explore.libraryCheckFailed(n: 1)), findsOneWidget); + }); + testWidgets('an unchecked server is noted under the copies other servers found', (tester) async { final source = _FakeCatalogSource(detail: const CatalogDetail(item: _enrichedRow)); diff --git a/test/services/catalog/catalog_library_matcher_test.dart b/test/services/catalog/catalog_library_matcher_test.dart index 750a4ea7c..53ebee4e5 100644 --- a/test/services/catalog/catalog_library_matcher_test.dart +++ b/test/services/catalog/catalog_library_matcher_test.dart @@ -1,5 +1,8 @@ import 'package:flutter_test/flutter_test.dart'; +import 'package:plezy/media/ids.dart'; +import 'package:plezy/media/media_item.dart'; import 'package:plezy/media/media_kind.dart'; +import 'package:plezy/media/media_server_client.dart'; import 'package:plezy/models/catalog/catalog_item.dart'; import 'package:plezy/providers/multi_server_provider.dart'; import 'package:plezy/services/catalog/catalog_library_matcher.dart'; @@ -69,6 +72,61 @@ class _Harness { } } +/// A server that answers the reverse lookup with [copies], or throws [error] +/// to model one that could not be reached. +class _LookupClient implements MediaServerClient { + _LookupClient(String id, {this.copies = const [], this.error}) : serverId = ServerId(id); + + @override + final ServerId serverId; + List copies; + Object? error; + int calls = 0; + + @override + Future> findByExternalIds( + ExternalIds ids, { + required MediaKind kind, + List titles = const [], + int? year, + String? plexGuid, + ExternalSeasonRef? season, + }) async { + calls++; + final failure = error; + if (failure != null) throw failure; + return copies; + } + + @override + void close() {} + + @override + dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); +} + +/// A matcher over the real [DataAggregationService] and [MultiServerManager]. +/// Only they can tell a registered-but-offline server (never asked) from one +/// that left the account, so a canned-response harness cannot model it. +class _LiveHarness { + final MultiServerManager manager = MultiServerManager(); + late final MultiServerProvider multiServer; + late final CatalogLibraryMatcher matcher; + + _LiveHarness(DateTime Function() now) { + multiServer = MultiServerProvider(manager, DataAggregationService(manager)); + matcher = CatalogLibraryMatcher.withClock(multiServer, now); + } + + void register(_LookupClient client, {bool online = true}) => + manager.debugRegisterClientForTesting(client, online: online); + + void dispose() { + multiServer.dispose(); + manager.dispose(); + } +} + void main() { test('season entries sharing canonical ids keep independent cached matches', () async { final harness = _Harness(); @@ -228,6 +286,104 @@ void main() { expect(harness.aggregation.calls, hasLength(3)); }); + test('a registered server that is offline for the retry keeps its copies and stays retryable', () async { + // An offline server is not in the fan-out at all, so it answers in no + // set — indistinguishable, to the fold, from one that left the account. + // It never said the title was gone, and the surviving wave must not be + // memoized as a complete answer for the session either. + var now = DateTime.utc(2026, 7, 28, 12); + final harness = _LiveHarness(() => now); + addTearDown(harness.dispose); + final aCopy = testMediaItem(id: 'a-copy', serverId: 'a'); + final bCopy = testMediaItem(id: 'b-copy', serverId: 'b'); + final a = _LookupClient('a', copies: [aCopy]); + final b = _LookupClient('b', error: StateError('unreachable')); + harness.register(a); + harness.register(b); + const item = CatalogItem( + source: CatalogSourceId.trakt, + kind: MediaKind.movie, + title: 'Offline Server Movie', + ids: CatalogItemIds(trakt: 11, tmdb: 79), + ); + + expect((await harness.matcher.match(item)).items, [aCopy]); + + // A drops offline, B recovers and answers for itself. + harness.register(a, online: false); + b + ..error = null + ..copies = [bCopy]; + now = now.add(CatalogLibraryMatcher.negativeTtl); + + final retained = await harness.matcher.match(item); + expect(retained.items, [aCopy, bCopy], reason: 'A was never asked, so its verified copy stands'); + expect(a.calls, 1); + + // Only A's own answer may drop A's copy, so the wave stays retryable. + harness.register(a); + a.copies = const []; + now = now.add(CatalogLibraryMatcher.negativeTtl); + expect((await harness.matcher.match(item)).items, [bCopy]); + expect(a.calls, 2); + }); + + test('a wave with no online server keeps known copies and is not an authoritative absence', () async { + var now = DateTime.utc(2026, 7, 28, 12); + final harness = _LiveHarness(() => now); + addTearDown(harness.dispose); + final aCopy = testMediaItem(id: 'a-copy', serverId: 'a'); + final a = _LookupClient('a', copies: [aCopy]); + final b = _LookupClient('b', error: StateError('unreachable')); + harness.register(a); + harness.register(b); + const item = CatalogItem( + source: CatalogSourceId.trakt, + kind: MediaKind.movie, + title: 'All Servers Offline Movie', + ids: CatalogItemIds(trakt: 12, tmdb: 80), + ); + + expect((await harness.matcher.match(item)).items, [aCopy]); + + harness.register(a, online: false); + harness.register(b, online: false); + now = now.add(CatalogLibraryMatcher.negativeTtl); + + final blind = await harness.matcher.match(item); + expect(blind.items, [aCopy], reason: 'a wave nobody answered erases nothing'); + expect(blind.succeededServerIds, isEmpty); + + harness.register(a); + now = now.add(CatalogLibraryMatcher.negativeTtl); + expect((await harness.matcher.match(item)).items, [aCopy]); + expect(a.calls, 2, reason: 'the blind wave is no hit, so the next tap asks again'); + }); + + test('a server that left the account loses its copies', () async { + // The one wave shape that means removal: the server is in no set at all + // — it neither answered nor sat the wave out, so it is off the account. + var now = DateTime.utc(2026, 7, 28, 12); + final harness = _Harness(now: () => now); + addTearDown(harness.dispose); + final aCopy = testMediaItem(id: 'a-copy', serverId: 'a'); + final bCopy = testMediaItem(id: 'b-copy', serverId: 'b'); + harness.aggregation.responses.addAll([ + libraryLookupResult([aCopy], succeeded: {'a'}, failed: {'b'}), + libraryLookupResult([bCopy], succeeded: {'b'}), + ]); + const item = CatalogItem( + source: CatalogSourceId.trakt, + kind: MediaKind.movie, + title: 'Removed Server Movie', + ids: CatalogItemIds(trakt: 13, tmdb: 81), + ); + + expect((await harness.matcher.match(item)).items, [aCopy]); + now = now.add(CatalogLibraryMatcher.negativeTtl); + expect((await harness.matcher.match(item)).items, [bCopy]); + }); + test('forwards season-stripped title candidates and season reference', () async { final harness = _Harness(); addTearDown(harness.dispose); diff --git a/test/services/multi_server_manager_test.dart b/test/services/multi_server_manager_test.dart index 726a9075a..91cb9a2e4 100644 --- a/test/services/multi_server_manager_test.dart +++ b/test/services/multi_server_manager_test.dart @@ -127,6 +127,41 @@ void main() { }); }); + group('registeredServerIds', () { + test('covers an auth-rejected Plex server that never got a client', () { + // A startup auth failure registers the server before any client can + // exist, so `serverIds` — sourced from `_clients` — cannot see it. + // A fan-out that has to report the servers it never asked still must. + final m = MultiServerManager(); + addTearDown(m.dispose); + + m.markPlexConnectionAuthError( + _plexAccount('account-1', [ + PlexServer( + name: 'Plex', + clientIdentifier: 'server-1', + accessToken: 'rejected-token', + connections: const [], + owned: true, + ), + ]), + ); + + expect(m.serverIds, isEmpty); + expect(m.onlineClients, isEmpty); + expect(m.registeredServerIds, {'server-1'}); + }); + + test('covers a registered client that is offline', () { + final m = MultiServerManager(); + addTearDown(m.dispose); + m.debugRegisterClientForTesting(_jellyfinClient('user-a'), online: false); + + expect(m.onlineClients, isEmpty); + expect(m.registeredServerIds, {'jf-machine'}); + }); + }); + group('updateServerStatus + statusStream', () { test('emits a snapshot when status flips for a tracked server', () async { final m = MultiServerManager(); diff --git a/test/test_helpers/library_lookup.dart b/test/test_helpers/library_lookup.dart index ed911df50..f132a6313 100644 --- a/test/test_helpers/library_lookup.dart +++ b/test/test_helpers/library_lookup.dart @@ -2,10 +2,18 @@ import 'package:plezy/media/media_item.dart'; import 'package:plezy/services/data_aggregation_service.dart'; /// A reverse-lookup wave fixture. By default every listed server answered; -/// name [failed] / [cancelled] servers to model a wave a server sat out. +/// name [failed] / [cancelled] servers to model a wave a server sat out, and +/// [unqueried] ones to model a registered server the wave never asked. LibraryLookupResult libraryLookupResult( List items, { Set succeeded = const {}, Set failed = const {}, Set cancelled = const {}, -}) => (items: items, succeededServerIds: succeeded, cancelledServerIds: cancelled, failedServerIds: failed); + Set unqueried = const {}, +}) => ( + items: items, + succeededServerIds: succeeded, + cancelledServerIds: cancelled, + failedServerIds: failed, + unqueriedServerIds: unqueried, +);