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, +);