From 87aea49ab6edbc8b13f23824bd855e712eb0f504 Mon Sep 17 00:00:00 2001 From: edde746 <86283021+edde746@users.noreply.github.com> Date: Sat, 4 Jul 2026 23:44:38 +0200 Subject: [PATCH] fix(servers): stop rebinds from flashing an empty home screen after sign-in Signing in triggered two back-to-back profile rebinds; the second re-added the same Jellyfin connection, which tore down the live client and aborted the home screen's in-flight fetches. The aborted pass was committed as loaded-empty, flashing 'no content available' until the follow-up load landed. Fix at the root instead of patching the sign-in window: - addJellyfinConnection now reuses the live client when the connection is unchanged (token, deviceId, URL set), matching the existing Plex refreshTokensForProfile behavior; material changes still recreate it. - Cancelled requests are classified end-to-end: the client's treat-as-empty helpers rethrow cancellations, and the aggregation fan-outs report cancelledServerIds alongside succeededServerIds. - A fetch pass in which zero servers succeeded is never authoritative: it keeps existing content instead of wiping it (also fixes the pre-existing blanking of home/sidebar on a totally failed refresh), stays in loading while disrupted (cancellation or binding in flight), and only commits loaded-empty on a settled failure. --- lib/exceptions/media_server_exceptions.dart | 6 + lib/navigation/profile_session_screen.dart | 12 +- lib/providers/discover_provider.dart | 52 ++++++-- lib/providers/libraries_provider.dart | 41 ++++++- lib/services/data_aggregation_service.dart | 42 +++++-- .../jellyfin_client/parts/browse.dart | 7 ++ lib/services/multi_server_manager.dart | 81 +++++++++++- test/providers/discover_provider_test.dart | 115 +++++++++++++++++- test/providers/libraries_provider_test.dart | 98 +++++++++++++++ .../data_aggregation_bridge_test.dart | 62 ++++++++++ .../jellyfin_client_failures_test.dart | 61 ++++++++++ test/services/multi_server_manager_test.dart | 101 +++++++++++++++ 12 files changed, 648 insertions(+), 30 deletions(-) diff --git a/lib/exceptions/media_server_exceptions.dart b/lib/exceptions/media_server_exceptions.dart index f28404cf..2316211b 100644 --- a/lib/exceptions/media_server_exceptions.dart +++ b/lib/exceptions/media_server_exceptions.dart @@ -92,6 +92,12 @@ class MediaServerHttpException extends MediaServerException { type == MediaServerHttpErrorType.connectionError || type == MediaServerHttpErrorType.receiveTimeout; + /// Whether the request was aborted client-side (client teardown or an + /// explicit abort), as opposed to failing against the server. A cancelled + /// fetch says nothing about the server's actual content — callers must not + /// treat it as an empty result. + bool get isCancellation => type == MediaServerHttpErrorType.cancelled; + @override String toString() { final parts = [type.name]; diff --git a/lib/navigation/profile_session_screen.dart b/lib/navigation/profile_session_screen.dart index 74d20c5e..52df4710 100644 --- a/lib/navigation/profile_session_screen.dart +++ b/lib/navigation/profile_session_screen.dart @@ -144,10 +144,14 @@ class _ProfileSessionScreenState extends State { lazy: true, ), ChangeNotifierProvider( - create: (context) => LibrariesProvider( - storageService: context.read(), - multiServer: context.read(), - ), + create: (context) { + final activeProfile = context.read(); + return LibrariesProvider( + storageService: context.read(), + multiServer: context.read(), + isProfileBinding: () => activeProfile.isBinding, + ); + }, ), ChangeNotifierProvider( create: (context) { diff --git a/lib/providers/discover_provider.dart b/lib/providers/discover_provider.dart index 3b808cad..69905633 100644 --- a/lib/providers/discover_provider.dart +++ b/lib/providers/discover_provider.dart @@ -61,8 +61,10 @@ class DiscoverProvider extends ChangeNotifier with DisposableChangeNotifierMixin final LibrariesProvider _libraries; /// Whether the profile binder is still wiring servers — a no-servers load - /// during binding stays in the loading state instead of flashing an error - /// (main_screen primes another load once binding settles). + /// during binding stays in the loading state instead of flashing an error, + /// and a zero-success pass during binding stays in the loading state + /// instead of flashing the empty placeholder (main_screen primes another + /// load once binding settles). final bool Function() isProfileBinding; StreamSubscription? _watchStateSubscription; @@ -198,18 +200,52 @@ class DiscoverProvider extends ChangeNotifier with DisposableChangeNotifierMixin includePlaybackHubs: false, ); + // A pass in which zero servers succeeded is never authoritative: it + // must not wipe existing content, and it may only commit "loaded, + // empty" when the failure is settled — not a client-side abort + // (teardown mid-fetch) and not mid-binding. In both of those cases a + // follow-up load is guaranteed (binding-settle prime, or + // syncToOnlineServers falling through to load() while not loaded). final fetchedOnDeck = await onDeckFuture; if (isDisposed) return; - _applyOnDeck(fetchedOnDeck.items); - _onDeckState = DiscoverLoadState.loaded; - _loadedOnDeckServerIds = fetchedOnDeck.succeededServerIds; - _loadGeneration++; - safeNotifyListeners(); - unawaited(_syncSystemShelf(_onDeck)); + if (fetchedOnDeck.succeededServerIds.isEmpty && _onDeck.isNotEmpty) { + // Keep the stale rows; the empty succeeded set makes the next status + // emission refetch every server. + appLogger.w('DiscoverProvider: on-deck pass failed on all servers; keeping previous items'); + _onDeckState = DiscoverLoadState.loaded; + _loadedOnDeckServerIds = fetchedOnDeck.succeededServerIds; + safeNotifyListeners(); + } else if (fetchedOnDeck.succeededServerIds.isEmpty && + (fetchedOnDeck.cancelledServerIds.isNotEmpty || isProfileBinding())) { + // Disrupted with nothing to show yet: stay in loading so the screen + // keeps its skeleton instead of flashing the empty placeholder. + // Don't return — the hubs fetch is still in flight below. + appLogger.d('DiscoverProvider: on-deck pass disrupted with no prior content; keeping loading state'); + } else { + _applyOnDeck(fetchedOnDeck.items); + _onDeckState = DiscoverLoadState.loaded; + _loadedOnDeckServerIds = fetchedOnDeck.succeededServerIds; + _loadGeneration++; + safeNotifyListeners(); + unawaited(_syncSystemShelf(_onDeck)); + } final fetchedHubs = await hubsFuture; if (isDisposed) return; + if (fetchedHubs.succeededServerIds.isEmpty && _hubs.isNotEmpty) { + appLogger.w('DiscoverProvider: hub pass failed on all servers; keeping previous hubs'); + _hubsState = DiscoverLoadState.loaded; + _loadedHubServerIds = fetchedHubs.succeededServerIds; + safeNotifyListeners(); + return; + } + if (fetchedHubs.succeededServerIds.isEmpty && + (fetchedHubs.cancelledServerIds.isNotEmpty || isProfileBinding())) { + appLogger.d('DiscoverProvider: hub pass disrupted with no prior content; keeping loading state'); + return; + } + final filteredHubs = _filterDiscoverHubs(fetchedHubs.hubs); sortMediaHubsByLibraryOrder(filteredHubs, _libraries.libraries); diff --git a/lib/providers/libraries_provider.dart b/lib/providers/libraries_provider.dart index ba1d38ed..65707041 100644 --- a/lib/providers/libraries_provider.dart +++ b/lib/providers/libraries_provider.dart @@ -15,9 +15,13 @@ enum LibrariesLoadState { initial, loading, loaded, error } /// Both SideNavigationRail and LibrariesScreen consume this provider /// instead of independently fetching library data. class LibrariesProvider extends ChangeNotifier with DisposableChangeNotifierMixin { - LibrariesProvider({StorageService? storageService, MultiServerProvider? multiServer}) - : _storageService = storageService, - _multiServer = multiServer { + LibrariesProvider({ + StorageService? storageService, + MultiServerProvider? multiServer, + bool Function()? isProfileBinding, + }) : _storageService = storageService, + _multiServer = multiServer, + _isProfileBinding = isProfileBinding ?? _neverBinding { // Reload libraries when a new server comes online. Servers bind in waves // on sign-in / profile switch and slow ones reconnect after the initial // load; without this they stay missing from the sidebar until a re-switch @@ -26,7 +30,16 @@ class LibrariesProvider extends ChangeNotifier with DisposableChangeNotifierMixi _multiServer?.addOnlineServersListener(syncToOnlineServers); } + static bool _neverBinding() => false; + final MultiServerProvider? _multiServer; + + /// Whether the profile binder is still wiring servers — a zero-success + /// first load during binding stays in the loading state instead of + /// flashing "no libraries" (main_screen primes another load once binding + /// settles). + final bool Function() _isProfileBinding; + StorageService? _storageService; DataAggregationService? _aggregationService; List _libraries = []; @@ -194,6 +207,28 @@ class LibrariesProvider extends ChangeNotifier with DisposableChangeNotifierMixi // internally; Jellyfin clients return MediaLibrary natively. final result = await _aggregationService!.getMediaLibrariesFromAllServers(); + // A pass in which zero servers succeeded is never authoritative — it + // must not replace existing data, and it may only commit "loaded, + // empty" when the failure is settled (not a client-side abort, not + // mid-binding). Recovery is guaranteed: the binding-settle prime and + // the next status emission both re-drive a load while state isn't a + // fully-covered `loaded`. + if (result.succeededServerIds.isEmpty) { + if (reloadInPlace) { + // A totally-failed silent refresh keeps the last good list instead + // of wiping the sidebar. Clear the succeeded set so the next + // status emission refetches rather than treating the stale list as + // covering those servers. + appLogger.w('LibrariesProvider: refresh failed on all servers; keeping previous libraries'); + _loadedServerIds = result.succeededServerIds; + return false; + } + if (result.cancelledServerIds.isNotEmpty || _isProfileBinding()) { + appLogger.d('LibrariesProvider: first load disrupted (zero successful servers); staying in loading state'); + return false; + } + } + // Filter out music libraries (not supported) final filteredLibraries = result.libraries.where((lib) => !ContentTypeHelper.isMusicLibrary(lib)).toList(); diff --git a/lib/services/data_aggregation_service.dart b/lib/services/data_aggregation_service.dart index 74adc1e8..b8d145e3 100644 --- a/lib/services/data_aggregation_service.dart +++ b/lib/services/data_aggregation_service.dart @@ -6,14 +6,22 @@ import '../media/media_item.dart'; import '../media/media_kind.dart'; import '../media/media_library.dart'; import '../media/media_server_client.dart'; +import '../exceptions/media_server_exceptions.dart'; import '../utils/app_logger.dart'; import '../utils/external_ids.dart'; import '../utils/global_key_utils.dart'; import '../utils/search_relevance.dart'; import 'multi_server_manager.dart'; -typedef OnDeckAggregationResult = ({List items, Set succeededServerIds}); -typedef HubAggregationResult = ({List hubs, Set succeededServerIds}); +typedef OnDeckAggregationResult = ({List items, Set succeededServerIds, Set cancelledServerIds}); +typedef HubAggregationResult = ({List hubs, Set succeededServerIds, Set cancelledServerIds}); +typedef LibraryAggregationResult = ({List libraries, Set succeededServerIds, Set cancelledServerIds}); + +/// Whether [error] is a client-side abort (client teardown mid-request) +/// rather than a genuine server failure. Aggregation reports these servers +/// in `cancelledServerIds` so callers can tell a *disrupted* pass — whose +/// results say nothing about actual content — from a settled failure. +bool _isCancellation(Object error) => error is MediaServerHttpException && error.isCancellation; /// Cross-server aggregation: fans calls out to every online client and /// merges the results. Single-server operations now go through the @@ -47,28 +55,34 @@ class DataAggregationService { /// list. [succeededServerIds] lets callers tell a *failed* fetch apart from a /// server that genuinely has no libraries — both contribute nothing, so /// conflating them would let a transient failure be cached as "loaded" and - /// never retried. - Future<({List libraries, Set succeededServerIds})> getMediaLibrariesFromAllServers({ - Set? serverIds, - }) async { + /// never retried. Servers whose fetch was aborted client-side land in + /// `cancelledServerIds` — a disrupted pass, unlike a settled failure, must + /// never be committed as authoritative. + Future getMediaLibrariesFromAllServers({Set? serverIds}) async { final clients = _clientsFor(serverIds); if (clients.isEmpty) { appLogger.w('No online servers available for fetching libraries (neutral)'); - return (libraries: const [], succeededServerIds: const {}); + return (libraries: const [], succeededServerIds: const {}, cancelledServerIds: const {}); } final succeededServerIds = {}; + final cancelledServerIds = {}; final futures = clients.entries.map((entry) async { try { final libraries = await entry.value.fetchLibraries(); succeededServerIds.add(entry.key); return libraries; } catch (e, stackTrace) { + if (_isCancellation(e)) cancelledServerIds.add(entry.key); appLogger.e('Failed neutral library fetch from ${entry.key}', error: e, stackTrace: stackTrace); return []; } }); final results = await Future.wait(futures); - return (libraries: [for (final list in results) ...list], succeededServerIds: succeededServerIds); + return ( + libraries: [for (final list in results) ...list], + succeededServerIds: succeededServerIds, + cancelledServerIds: cancelledServerIds, + ); } /// Fetch "On Deck" (Continue Watching) from all servers and merge by recency. @@ -83,15 +97,17 @@ class DataAggregationService { final clients = _clientsFor(serverIds); if (clients.isEmpty) { appLogger.w('No online servers available for fetching on deck'); - return (items: const [], succeededServerIds: const {}); + return (items: const [], succeededServerIds: const {}, cancelledServerIds: const {}); } + final cancelledServerIds = {}; final futures = clients.entries.map((entry) async { final client = entry.value; try { final items = await client.fetchContinueWatching(count: limit); return (serverId: entry.key, items: items); } catch (e, st) { + if (_isCancellation(e)) cancelledServerIds.add(entry.key); appLogger.e('Failed on-deck fetch from ${entry.key}', error: e, stackTrace: st); return (serverId: null, items: []); } @@ -125,7 +141,7 @@ class DataAggregationService { appLogger.i('Fetched ${items.length} on deck items from all servers'); - return (items: items, succeededServerIds: succeededServerIds); + return (items: items, succeededServerIds: succeededServerIds, cancelledServerIds: cancelledServerIds); } /// Merge an [existing] Continue Watching list with [fresh] rows from @@ -289,7 +305,7 @@ class DataAggregationService { final clients = _clientsFor(serverIds); if (clients.isEmpty) { appLogger.w('No online servers available for fetching hubs'); - return (hubs: const [], succeededServerIds: const {}); + return (hubs: const [], succeededServerIds: const {}, cancelledServerIds: const {}); } // Only fallback clients need a library prefetch when home layout is on; @@ -299,6 +315,7 @@ class DataAggregationService { ? _groupLibrariesByServer((await getMediaLibrariesFromAllServers(serverIds: serverIds)).libraries) : null; + final cancelledServerIds = {}; final futures = clients.entries.map((entry) async { final serverId = entry.key; final client = entry.value; @@ -320,6 +337,7 @@ class DataAggregationService { hubs: _postProcessHubs(hubs, serverId: ServerId(serverId), hiddenLibraryKeys: hiddenLibraryKeys), ); } catch (e, stackTrace) { + if (_isCancellation(e)) cancelledServerIds.add(serverId); appLogger.e('Failed to fetch hubs from server $serverId', error: e, stackTrace: stackTrace); return (serverId: null, hubs: []); } @@ -335,7 +353,7 @@ class DataAggregationService { all.addAll(result.hubs); } final hubs = limit != null && limit < all.length ? all.sublist(0, limit) : all; - return (hubs: hubs, succeededServerIds: succeededServerIds); + return (hubs: hubs, succeededServerIds: succeededServerIds, cancelledServerIds: cancelledServerIds); } /// Per-library hub fetch for a single client. Filters to visible diff --git a/lib/services/jellyfin_client/parts/browse.dart b/lib/services/jellyfin_client/parts/browse.dart index 20fcd07a..0b531c7a 100644 --- a/lib/services/jellyfin_client/parts/browse.dart +++ b/lib/services/jellyfin_client/parts/browse.dart @@ -1279,6 +1279,9 @@ mixin _JellyfinBrowseMethods on MediaServerCacheMixin { final page = await fetchMoreHubItemsPage(hubId, start: 0, size: limit ?? 50); return page.items; } catch (e, st) { + // A cancelled request says nothing about the hub's contents — let it + // propagate so the caller classifies the fetch as disrupted, not empty. + if (e is MediaServerHttpException && e.isCancellation) rethrow; appLogger.w('JellyfinClient: failed to fetch hub items for $hubId (treating as empty)', error: e, stackTrace: st); return const []; } @@ -1598,6 +1601,10 @@ mixin _JellyfinBrowseMethods on MediaServerCacheMixin { } return _itemsArray(data); } catch (e, st) { + // A cancelled request says nothing about the endpoint's contents — let + // it propagate so the caller classifies the fetch as disrupted, not + // empty. + if (e is MediaServerHttpException && e.isCancellation) rethrow; appLogger.w('JellyfinClient: $path failed (treating as empty)', error: e, stackTrace: st); return const []; } diff --git a/lib/services/multi_server_manager.dart b/lib/services/multi_server_manager.dart index ccae3227..b5fa62fd 100644 --- a/lib/services/multi_server_manager.dart +++ b/lib/services/multi_server_manager.dart @@ -547,6 +547,15 @@ class MultiServerManager { /// Add a Jellyfin server backed by an authenticated [JellyfinConnection]. /// Returns true on success. /// + /// When a live client already exists for the same compound id and the + /// connection is equivalent (see [canReuseJellyfinClient]), that client is + /// reused instead of recreated — profile rebinds re-add unchanged + /// connections routinely, and tearing the client down would abort its + /// in-flight requests. A material change (token, deviceId, URL set) still + /// replaces the client. This mirrors the Plex rebind path, where + /// [refreshTokensForProfile] reuses the online client via an in-place + /// token update. + /// /// Jellyfin clients use the shared endpoint-racing flow when multiple URLs /// are configured, then instantiate the client against the lowest-latency /// reachable URL. @@ -557,6 +566,13 @@ class MultiServerManager { /// client (preserves any in-flight operations on the prior profile). Future addJellyfinConnection(JellyfinConnection connection) async { try { + // Every close path detaches the client from [_jellyfinByCompoundId] + // before closing it, so a client found here is never mid-close. + final existing = _jellyfinByCompoundId[connection.id]; + if (existing != null && canReuseJellyfinClient(live: existing.connection, incoming: connection)) { + return _reuseJellyfinClient(existing); + } + var resolvedConnection = connection; if (connection.baseUrls.length > 1) { try { @@ -591,8 +607,9 @@ class MultiServerManager { final compoundId = resolvedConnection.id; final machineId = resolvedConnection.serverMachineId; - // Replace any prior client for this exact compound id (re-add of the - // same user — e.g., token refresh or settings re-add). + // Replace the prior client for this compound id — reaching here means + // the connection materially changed (token refresh, URL-set edit); an + // unchanged re-add was already handled by the reuse branch above. final oldClient = _jellyfinByCompoundId[compoundId]; if (oldClient != null) _closeClient(oldClient); _jellyfinByCompoundId[compoundId] = client; @@ -619,6 +636,66 @@ class MultiServerManager { } } + /// Whether the live client bound to [live] can serve [incoming] without + /// being recreated. Recreation is required when a field baked into the + /// client at construction time changes: + /// - `accessToken` / `deviceId` are embedded in the auth headers when the + /// HTTP client is built; + /// - `baseUrls` fixes the failover candidate set. Compared as a set: both + /// the client and the add-path endpoint race reorder the list as + /// endpoints are promoted, so ordering drifts on an unchanged server. + /// + /// Everything else is deliberately ignored: the active `baseUrl` drifts as + /// the client rotates endpoints, `isAdministrator` self-refreshes on health + /// checks, and the remaining fields are display metadata. `userId` and + /// `serverMachineId` equality is implied by the compound-id lookup that + /// precedes this check. + @visibleForTesting + static bool canReuseJellyfinClient({required JellyfinConnection live, required JellyfinConnection incoming}) { + return live.accessToken == incoming.accessToken && + live.deviceId == incoming.deviceId && + setEquals(live.baseUrls.toSet(), incoming.baseUrls.toSet()); + } + + /// Re-add of an unchanged connection: keep the live client (preserving its + /// in-flight requests and settled endpoint choice), re-bind it as the + /// machine's active user, and run a fresh health probe so callers still + /// get a current result. Skips the endpoint race ([JellyfinClient] has + /// per-request failover plus exhaustion-triggered reconnect), the + /// connection-update wiring (already attached when the client was first + /// added), and the connection persist (the client persists its own + /// endpoint rotations). + Future _reuseJellyfinClient(JellyfinClient client) async { + final compoundId = client.connection.id; + final machineId = client.connection.serverMachineId; + final rebound = _activeJellyfinMachine[machineId] != compoundId; + _clients[machineId] = client; + _activeJellyfinMachine[machineId] = compoundId; + + final health = await client.checkHealth(); + _jellyfinHealthByCompoundId[compoundId] = health; + if (_activeJellyfinMachine[machineId] != compoundId) { + // A concurrent remove/re-add won while the probe was in flight. + appLogger.d('Ignoring stale Jellyfin reuse result for ${client.connection.serverName}'); + return health == HealthStatus.online; + } + _applyHealth(ServerId(machineId), health); + if (rebound) { + // The machine's active user changed even if its online status didn't; + // client-map consumers need to observe the swap. + _statusController.add(Map.from(_serverStatus)); + } + final healthy = health == HealthStatus.online; + appLogger.i( + 'Reusing existing Jellyfin client for ${client.connection.serverName}' + '${healthy ? '' : ' (unhealthy)'} (connection unchanged)', + ); + if (_connectivitySubscription == null && healthy) { + _startNetworkMonitoring(); + } + return healthy; + } + void _wireJellyfinConnectionUpdates(JellyfinClient client) { client.onConnectionUpdated = (updated) async { if (_jellyfinByCompoundId[updated.id] != client) { diff --git a/test/providers/discover_provider_test.dart b/test/providers/discover_provider_test.dart index a414e4b4..c9fe29fc 100644 --- a/test/providers/discover_provider_test.dart +++ b/test/providers/discover_provider_test.dart @@ -57,6 +57,8 @@ class _FakeAggregationService extends DataAggregationService { Set? lastHubsServerIds; Set? onDeckSucceededServerIds; Set? hubSucceededServerIds; + Set onDeckCancelledServerIds = const {}; + Set hubCancelledServerIds = const {}; List Function() onDeckResult = () => const []; List Function() hubsResult = () => const []; @@ -72,6 +74,7 @@ class _FakeAggregationService extends DataAggregationService { return ( items: limit != null && items.length > limit ? items.sublist(0, limit) : items, succeededServerIds: onDeckSucceededServerIds ?? serverIds ?? const {'server_1'}, + cancelledServerIds: onDeckCancelledServerIds, ); } @@ -85,7 +88,11 @@ class _FakeAggregationService extends DataAggregationService { }) async { hubCalls++; lastHubsServerIds = serverIds; - return (hubs: hubsResult(), succeededServerIds: hubSucceededServerIds ?? serverIds ?? const {'server_1'}); + return ( + hubs: hubsResult(), + succeededServerIds: hubSucceededServerIds ?? serverIds ?? const {'server_1'}, + cancelledServerIds: hubCancelledServerIds, + ); } } @@ -288,6 +295,112 @@ void main() { expect(binderProvider.errorMessage, isNotNull); }); + // A pass in which zero servers succeeded is never authoritative: it must + // not wipe existing content, and it may only commit "loaded, empty" when + // the failure is settled (no cancellations, binder not running). The + // sign-in empty-flash regression: a rebind tore down the client mid-fetch, + // the aborted pass committed loaded-empty, and the screen flashed + // "no content available" until the follow-up load landed. + + test('zero-success pass with cancellations stays loading instead of committing empty', () async { + aggregation.onDeckSucceededServerIds = const {}; + aggregation.hubSucceededServerIds = const {}; + aggregation.onDeckCancelledServerIds = const {'server_1'}; + aggregation.hubCancelledServerIds = const {'server_1'}; + + await provider.load(); + + expect(provider.isLoading, isTrue); + expect(provider.areHubsLoading, isTrue); + expect(provider.errorMessage, isNull); + expect(provider.loadGeneration, 0); + + // The guaranteed follow-up load lands the real content. + aggregation.onDeckSucceededServerIds = null; + aggregation.hubSucceededServerIds = null; + aggregation.onDeckCancelledServerIds = const {}; + aggregation.hubCancelledServerIds = const {}; + aggregation.onDeckResult = () => [_item('a')]; + aggregation.hubsResult = () => [_hub('hub-1')]; + await provider.load(); + + expect(provider.onDeck.map((i) => i.id), ['a']); + expect(provider.hubs.map((h) => h.id), ['hub-1']); + expect(provider.isLoading, isFalse); + expect(provider.areHubsLoading, isFalse); + }); + + test('zero-success pass during profile binding stays loading (no cancellations)', () async { + // Covers the timeout-during-bind window: every fetch failed while the + // binder was still wiring servers, with no cancellation marker. + isBinding = true; + aggregation.onDeckSucceededServerIds = const {}; + aggregation.hubSucceededServerIds = const {}; + + await provider.load(); + + expect(provider.isLoading, isTrue); + expect(provider.areHubsLoading, isTrue); + expect(provider.errorMessage, isNull); + }); + + test('settled zero-success pass with no prior content commits loaded-empty', () async { + // Locks the no-eternal-spinner constraint: a genuinely dead server + // outside any disruption window keeps today's empty state. + aggregation.onDeckSucceededServerIds = const {}; + aggregation.hubSucceededServerIds = const {}; + + await provider.load(); + + expect(provider.isLoading, isFalse); + expect(provider.areHubsLoading, isFalse); + expect(provider.onDeck, isEmpty); + expect(provider.hubs, isEmpty); + expect(provider.errorMessage, isNull); + }); + + test('totally failed refresh keeps previous content instead of wiping it', () async { + aggregation.onDeckResult = () => [_item('a')]; + aggregation.hubsResult = () => [_hub('hub-1')]; + await provider.load(); + final generationBefore = provider.loadGeneration; + + aggregation.onDeckSucceededServerIds = const {}; + aggregation.hubSucceededServerIds = const {}; + aggregation.onDeckResult = () => const []; + aggregation.hubsResult = () => const []; + await provider.load(); + + expect(provider.onDeck.map((i) => i.id), ['a']); + expect(provider.hubs.map((h) => h.id), ['hub-1']); + expect(provider.isLoading, isFalse); + expect(provider.areHubsLoading, isFalse); + // No new data: a failed pass must not reset the hero carousel. + expect(provider.loadGeneration, generationBefore); + + // The kept content does not count as covering the failed servers — the + // next status emission refetches them. + aggregation.onDeckSucceededServerIds = null; + aggregation.hubSucceededServerIds = null; + aggregation.onDeckResult = () => [_item('b')]; + final callsBefore = aggregation.onDeckCalls; + await provider.syncToOnlineServers({'server_1'}); + expect(aggregation.onDeckCalls, greaterThan(callsBefore)); + }); + + test('a disrupted half is independent: on-deck commits while hubs stay loading', () async { + aggregation.onDeckResult = () => [_item('a')]; + aggregation.hubSucceededServerIds = const {}; + aggregation.hubCancelledServerIds = const {'server_1'}; + + await provider.load(); + + expect(provider.isLoading, isFalse); + expect(provider.onDeck.map((i) => i.id), ['a']); + expect(provider.areHubsLoading, isTrue); + expect(provider.hubs, isEmpty); + }); + test('updateItem refetches one item and swaps it in place', () async { aggregation.onDeckResult = () => [_item('ep-1')]; aggregation.hubsResult = () => [ diff --git a/test/providers/libraries_provider_test.dart b/test/providers/libraries_provider_test.dart index e4ebc9ae..e3753907 100644 --- a/test/providers/libraries_provider_test.dart +++ b/test/providers/libraries_provider_test.dart @@ -2,6 +2,7 @@ import 'dart:async'; import 'package:plezy/media/ids.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:plezy/exceptions/media_server_exceptions.dart'; import 'package:plezy/media/media_backend.dart'; import 'package:plezy/media/media_kind.dart'; import 'package:plezy/media/media_library.dart'; @@ -364,6 +365,103 @@ void main() { manager.dispose(); }); + test('a first load disrupted by cancellations stays loading instead of flashing empty', () async { + // The sign-in empty-flash regression: a rebind tore the client down + // mid-fetch, the aborted pass used to commit loaded-empty, and the + // sidebar flashed "no libraries found" until the follow-up load landed. + final manager = MultiServerManager(); + final clientA = _FakeClient(serverId: ServerId('A'), libraries: [_serverLib(ServerId('A'), '1', 'Movies A')]) + ..error = MediaServerHttpException(type: MediaServerHttpErrorType.cancelled, message: 'HTTP client is closing'); + manager.debugRegisterClientForTesting(clientA); + final p = LibrariesProvider()..initialize(DataAggregationService(manager)); + + await p.loadLibraries(); + + expect(p.isLoading, isTrue); + expect(p.hasLoaded, isFalse); + expect(p.errorMessage, isNull); + + // The guaranteed follow-up load (binding-settle prime / next status + // emission) lands the real list. + clientA.error = null; + await p.loadLibraries(); + expect(p.hasLoaded, isTrue); + expect(p.libraries.map((l) => l.title), ['Movies A']); + + p.dispose(); + manager.dispose(); + }); + + test('a zero-success first load during profile binding stays loading', () async { + // The timeout-during-bind window: every fetch failed while the binder + // was still wiring servers, with no cancellation marker. + final manager = MultiServerManager(); + final clientA = _FakeClient(serverId: ServerId('A'), libraries: [_serverLib(ServerId('A'), '1', 'Movies A')]) + ..error = Exception('probe timed out'); + manager.debugRegisterClientForTesting(clientA); + var binding = true; + final p = LibrariesProvider(isProfileBinding: () => binding)..initialize(DataAggregationService(manager)); + + await p.loadLibraries(); + expect(p.isLoading, isTrue); + expect(p.hasLoaded, isFalse); + + binding = false; + clientA.error = null; + await p.loadLibraries(); + expect(p.hasLoaded, isTrue); + expect(p.libraries.map((l) => l.title), ['Movies A']); + + p.dispose(); + manager.dispose(); + }); + + test('a settled zero-success first load still commits loaded-empty', () async { + // Locks the no-eternal-spinner constraint: a genuinely dead server + // outside any disruption window keeps today's empty state. + final manager = MultiServerManager(); + final clientA = _FakeClient(serverId: ServerId('A'))..error = Exception('connection refused'); + manager.debugRegisterClientForTesting(clientA); + final p = LibrariesProvider()..initialize(DataAggregationService(manager)); + + await p.loadLibraries(); + + expect(p.hasLoaded, isTrue); + expect(p.libraries, isEmpty); + expect(p.errorMessage, isNull); + + p.dispose(); + manager.dispose(); + }); + + test('a totally failed silent refresh keeps the last good list', () async { + // Regression (pre-existing wipe bug): a reload-in-place where every + // server fails without throwing used to replace the list with [] — + // blanking the sidebar on a transient outage. + final manager = MultiServerManager(); + final clientA = _FakeClient(serverId: ServerId('A'), libraries: [_serverLib(ServerId('A'), '1', 'Movies A')]); + manager.debugRegisterClientForTesting(clientA); + final p = LibrariesProvider()..initialize(DataAggregationService(manager)); + + await p.loadLibraries(); + expect(p.libraries.map((l) => l.title), ['Movies A']); + + clientA.error = Exception('offline'); + await p.loadLibraries(); + expect(p.hasLoaded, isTrue); + expect(p.libraries.map((l) => l.title), ['Movies A']); + + // The kept list does not count as covering the failed server — the + // next sync refetches it. + clientA.error = null; + final callsBefore = clientA.fetchLibrariesCalls; + await p.syncToOnlineServers({'A'}); + expect(clientA.fetchLibrariesCalls, callsBefore + 1); + + p.dispose(); + manager.dispose(); + }); + test('online-servers listener is removed on dispose', () { final manager = MultiServerManager(); final multiServer = MultiServerProvider(manager, DataAggregationService(manager)); diff --git a/test/services/data_aggregation_bridge_test.dart b/test/services/data_aggregation_bridge_test.dart index be825799..fbf9fb3d 100644 --- a/test/services/data_aggregation_bridge_test.dart +++ b/test/services/data_aggregation_bridge_test.dart @@ -7,6 +7,9 @@ import 'package:http/http.dart' as http; import 'package:http/testing.dart'; import 'package:plezy/connection/connection.dart'; import 'package:plezy/database/app_database.dart'; +import 'package:plezy/exceptions/media_server_exceptions.dart'; +import 'package:plezy/media/media_backend.dart'; +import 'package:plezy/media/media_library.dart'; import 'package:plezy/media/media_server_client.dart'; import 'package:plezy/models/plex/plex_config.dart'; import 'package:plezy/services/data_aggregation_service.dart'; @@ -29,6 +32,34 @@ JellyfinConnection _conn() => JellyfinConnection( http.Response _json(Object body) => http.Response(jsonEncode(body), 200, headers: {'content-type': 'application/json'}); +/// Minimal client whose `fetchLibraries` either returns canned libraries or +/// throws [error] — enough surface to exercise the fan-out's per-server +/// failure classification without a real backend. +class _LibrariesClient implements MediaServerClient { + _LibrariesClient(this.serverId, {this.error, this.libraries = const []}); + + @override + final ServerId serverId; + + @override + final String serverName = 'Server'; + + final Object? error; + final List libraries; + + @override + Future> fetchLibraries() async { + if (error != null) throw error!; + return libraries; + } + + @override + void close() {} + + @override + dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); +} + /// Smoke tests for the surviving cross-server aggregation surface on /// [DataAggregationService]. Single-server passthroughs were removed in /// favour of `context.tryGetMediaClientForServer(...).()`; what's @@ -65,6 +96,37 @@ void main() { expect(onDeck.succeededServerIds, isEmpty); }); + test('classifies cancelled per-server failures apart from settled ones', () async { + // A cancelled fetch (our own client torn down mid-request) says nothing + // about the server's content; consumers use cancelledServerIds to keep + // a disrupted pass from being committed as authoritative. A settled + // failure (server down) lands in neither set. + manager.debugRegisterClientForTesting( + _LibrariesClient( + ServerId('ok'), + libraries: [MediaLibrary(id: '1', backend: MediaBackend.plex, title: 'Movies', serverId: ServerId('ok'))], + ), + ); + manager.debugRegisterClientForTesting( + _LibrariesClient( + ServerId('torn-down'), + error: MediaServerHttpException(type: MediaServerHttpErrorType.cancelled, message: 'HTTP client is closing'), + ), + ); + manager.debugRegisterClientForTesting( + _LibrariesClient( + ServerId('down'), + error: MediaServerHttpException(type: MediaServerHttpErrorType.connectionError, message: 'refused'), + ), + ); + + final result = await service.getMediaLibrariesFromAllServers(); + + expect(result.libraries.map((l) => l.title), ['Movies']); + expect(result.succeededServerIds, {'ok'}); + expect(result.cancelledServerIds, {'torn-down'}); + }); + test('searchAcrossServers overfetches and ranks before trimming across backends', () async { final plexRequests = []; final jellyfinRequests = []; diff --git a/test/services/jellyfin_client_failures_test.dart b/test/services/jellyfin_client_failures_test.dart index e7bf3532..5720861d 100644 --- a/test/services/jellyfin_client_failures_test.dart +++ b/test/services/jellyfin_client_failures_test.dart @@ -231,4 +231,65 @@ void main() { expect(requests.map((uri) => uri.host), ['primary.example.com', 'fallback.example.com', 'primary.example.com']); }); }); + + group('cancellation vs treat-as-empty', () { + // The hub/next-up fetch helpers swallow per-endpoint failures into empty + // lists so one broken endpoint doesn't sink a whole row. A *cancelled* + // request is different: it means our own client was torn down mid-fetch + // and says nothing about the server's content, so it must propagate — + // otherwise a disrupted server counts as "succeeded with partial data" + // and aborted sign-in fetches flash an empty home screen. + MediaServerHttpException cancelled() => + MediaServerHttpException(type: MediaServerHttpErrorType.cancelled, message: 'HTTP client is closing'); + + test('fetchContinueWatching propagates a cancelled NextUp sub-fetch', () async { + final client = _withMock( + MockClient((req) async { + if (req.url.path == '/Shows/NextUp') throw cancelled(); + return http.Response(jsonEncode({'Items': []}), 200, headers: {'content-type': 'application/json'}); + }), + ); + addTearDown(client.close); + + await expectLater( + client.fetchContinueWatching(), + throwsA(isA().having((e) => e.isCancellation, 'isCancellation', isTrue)), + ); + }); + + test('fetchContinueWatching still treats a NextUp server error as empty', () async { + final client = _withMock( + MockClient((req) async { + if (req.url.path == '/Shows/NextUp') return http.Response('Internal error', 500); + return http.Response( + jsonEncode({ + 'Items': [ + {'Id': 'ep-1', 'Type': 'Episode', 'Name': 'Resume Me'}, + ], + }), + 200, + headers: {'content-type': 'application/json'}, + ); + }), + ); + addTearDown(client.close); + + final items = await client.fetchContinueWatching(); + + expect(items.map((i) => i.id), ['ep-1']); + }); + + test('fetchMoreHubItems propagates a cancellation and swallows server errors', () async { + final cancelledClient = _withMock(MockClient((_) async => throw cancelled())); + addTearDown(cancelledClient.close); + await expectLater( + cancelledClient.fetchMoreHubItems('home.nextup'), + throwsA(isA().having((e) => e.isCancellation, 'isCancellation', isTrue)), + ); + + final failingClient = _withMock(MockClient((_) async => http.Response('Internal error', 500))); + addTearDown(failingClient.close); + expect(await failingClient.fetchMoreHubItems('home.nextup'), isEmpty); + }); + }); } diff --git a/test/services/multi_server_manager_test.dart b/test/services/multi_server_manager_test.dart index 8e08f53d..64071e07 100644 --- a/test/services/multi_server_manager_test.dart +++ b/test/services/multi_server_manager_test.dart @@ -322,6 +322,107 @@ void main() { }); }); + // ============================================================ + // addJellyfinConnection reuse + // ============================================================ + + group('addJellyfinConnection reuse', () { + // The reuse branch is what keeps a passive rebind (re-adding the same + // persisted connection) from tearing down a live client and aborting its + // in-flight requests. The identity assertions are load-bearing: any + // recreation implies the prior client was closed. + test('re-adding an identical connection reuses the live client', () async { + var probes = 0; + final client = JellyfinClient.forTesting( + connection: _jellyfinConnection('user-a'), + httpClient: MockClient((request) async { + probes++; + expect(request.url.path, '/Users/Me'); + return http.Response('{}', 200, headers: {'content-type': 'application/json'}); + }), + ); + addTearDown(client.close); + final m = MultiServerManager(); + addTearDown(m.dispose); + m.debugRegisterJellyfinClientForTesting(client); + + final healthy = await m.addJellyfinConnection(_jellyfinConnection('user-a')); + + expect(healthy, isTrue); + expect(m.getJellyfinClientByCompoundId('jf-machine/user-a'), same(client)); + expect(m.getClient(ServerId('jf-machine')), same(client)); + // One fresh health probe on the existing client; no recreation. + expect(probes, 1); + }); + + test('reuse rebinds an inactive compound client without closing the active one', () async { + JellyfinClient clientFor(String userId) => JellyfinClient.forTesting( + connection: _jellyfinConnection(userId), + httpClient: MockClient((_) async => http.Response('{}', 200, headers: {'content-type': 'application/json'})), + ); + final userA = clientFor('user-a'); + final userB = clientFor('user-b'); + addTearDown(userA.close); + addTearDown(userB.close); + final m = MultiServerManager(); + addTearDown(m.dispose); + m.debugRegisterJellyfinClientForTesting(userA); + m.debugRegisterJellyfinClientForTesting(userB); // takes the machine slot + + await m.addJellyfinConnection(_jellyfinConnection('user-a')); + + expect(m.getClient(ServerId('jf-machine')), same(userA)); + expect(m.getJellyfinClientByCompoundId('jf-machine/user-a'), same(userA)); + // The other user's client stays registered for a future switch back. + expect(m.getJellyfinClientByCompoundId('jf-machine/user-b'), same(userB)); + }); + }); + + group('canReuseJellyfinClient', () { + // A false verdict routes addJellyfinConnection to the pre-existing + // replace path (covered by 'ignores stale admin-status persistence from + // a replaced Jellyfin client' above). + final base = _jellyfinConnection('user-a'); + + test('identical connection is reusable', () { + expect(MultiServerManager.canReuseJellyfinClient(live: base, incoming: _jellyfinConnection('user-a')), isTrue); + }); + + test('changed access token requires recreation', () { + expect( + MultiServerManager.canReuseJellyfinClient(live: base, incoming: base.copyWith(accessToken: 'rotated')), + isFalse, + ); + }); + + test('changed device id requires recreation', () { + expect( + MultiServerManager.canReuseJellyfinClient(live: base, incoming: base.copyWith(deviceId: 'other-device')), + isFalse, + ); + }); + + test('same URL set with a different active URL is reusable', () { + // The live client rotates its active endpoint on its own; the add-path + // race reorders the candidate list. Neither warrants a teardown. + final live = base.copyWith( + baseUrl: 'https://a.example.com', + baseUrls: ['https://a.example.com', 'https://b.example.com'], + ); + final incoming = base.copyWith( + baseUrl: 'https://b.example.com', + baseUrls: ['https://b.example.com', 'https://a.example.com'], + ); + expect(MultiServerManager.canReuseJellyfinClient(live: live, incoming: incoming), isTrue); + }); + + test('an added or removed URL requires recreation', () { + final twoUrls = base.copyWith(baseUrls: ['https://jf.example.com', 'https://alt.example.com']); + expect(MultiServerManager.canReuseJellyfinClient(live: twoUrls, incoming: base), isFalse); + expect(MultiServerManager.canReuseJellyfinClient(live: base, incoming: twoUrls), isFalse); + }); + }); + // ============================================================ // removeServer // ============================================================