From 74d3af3ae1d2438af351a7dfa7b319e3207bbd49 Mon Sep 17 00:00:00 2001 From: edde746 <86283021+edde746@users.noreply.github.com> Date: Tue, 4 Aug 2026 04:14:33 +0200 Subject: [PATCH] perf(home): load the home screen once instead of twice per cold start The Discover tab fanned out its whole request set twice on every cold start and replayed slow rows on a shrinking timeout ladder, so a healthy remote server produced anywhere from 4s to 15s of loading. Measured against a remote Jellyfin server with four libraries, 24 interleaved cold-start samples per side: requests 19 -> 9 payload 219 KB -> 94 KB settled 5231ms -> 2502ms median, 13222ms -> 5927ms p95 Four independent causes: - Retry policy. `Client.send` resolves on response headers, so the connect budget covers the server's think time and a slow-but-alive query raises `connectionTimeout`. Replaying it made the server re-run the query with a shorter budget than the one it just missed; the `[10s, 8s, 5s]` ladder turned an 11s answer into an empty row after 23s. Hub surfaces now get one whole-request deadline, retry only immediate connection errors, and the deadline bounds the whole call including the request still in flight. - Request shape. `/Items/Latest` groups a TV library by series, so its rows are Series folder dtos and `RecursiveItemCount`/`ChildCount` cost a DB count each, per row. Hub rows now ask for `Overview` only; watch state survives because Jellyfin derives `UserData.Played` from `UnplayedItemCount` when the count fields are absent. `/Shows/NextUp` sends `NextUpDateCutoff` to bound the server's series-key scan, and `Thumb` leaves `EnableImageTypes` since nothing reads it. `UserData` and `PremiereDate` leave the browse set: neither is an `ItemFields` member, so the server dropped them anyway. - Fan-out. Per-library hubs ran in batches of three separated by a barrier, so one slow library stalled every library behind it. A sliding window keeps the same peak concurrency without head-of-line blocking. Concurrent `fetchLibraries` calls now share one `/Views` instead of racing two identical round trips, Plex's global and music hub legs start together, and Jellyfin gets Plex's pool tuning. - Duplicate pass. `DiscoverScreen.initState` starts a load and the online-entry hook asked for a full refresh on top of it, which `CoalescedLoadCoordinator` correctly queued as a trailing pass. The hook now calls `primeRefresh`, which rides along with a load already in flight; profile switches still go through `fullRefresh`. Refs #1784 --- lib/mixins/refreshable.dart | 10 ++ lib/providers/discover_provider.dart | 5 + lib/screens/discover_screen.dart | 11 ++ lib/screens/main_screen.dart | 21 ++- lib/services/data_aggregation_service.dart | 104 ++++++----- lib/services/jellyfin_client.dart | 5 + .../jellyfin_client/parts/browse.dart | 115 ++++++++---- lib/services/library_query_translator.dart | 7 +- lib/services/plex_client.dart | 12 +- lib/utils/coalesced_load_coordinator.dart | 5 + lib/utils/media_server_retry.dart | 93 ++++++---- lib/utils/media_server_timeouts.dart | 23 ++- test/screens/discover_screen_test.dart | 168 +++++++++++++++++ .../data_aggregation_bridge_test.dart | 102 ++++++++++- .../jellyfin_client_failures_test.dart | 30 +++- test/services/jellyfin_client_urls_test.dart | 86 ++++++--- .../library_query_translator_test.dart | 2 +- test/services/plex_home_retry_test.dart | 44 ++++- test/utils/media_server_retry_test.dart | 169 ++++++++++++++++-- 19 files changed, 831 insertions(+), 181 deletions(-) diff --git a/lib/mixins/refreshable.dart b/lib/mixins/refreshable.dart index 216494a3..6d22fb34 100644 --- a/lib/mixins/refreshable.dart +++ b/lib/mixins/refreshable.dart @@ -4,6 +4,16 @@ mixin Refreshable { mixin FullRefreshable { void fullRefresh(); + + /// Online-entry variant, used by `main_screen._primeOnlineServices` on cold + /// start and on reconnect-from-offline. Its job is to guarantee the tab + /// loads once servers are up — not to force a refetch. + /// + /// Screens that already kick off their own load in `initState` override this + /// to skip while that pass is still in flight; otherwise the prime queues an + /// identical trailing pass and the tab fetches everything twice (#1784). + /// Profile switches go through [fullRefresh] instead, which always refetches. + void primeRefresh() => fullRefresh(); } mixin FocusableTab { diff --git a/lib/providers/discover_provider.dart b/lib/providers/discover_provider.dart index c9123f66..12447144 100644 --- a/lib/providers/discover_provider.dart +++ b/lib/providers/discover_provider.dart @@ -167,6 +167,11 @@ class DiscoverProvider extends ChangeNotifier with DisposableChangeNotifierMixin return _loadCoordinator.requestFull(); } + /// Whether a [load] pass is already running. The startup online-entry hook + /// uses this to skip a prime that would only duplicate the load the screen + /// started in `initState`. + bool get isLoadInFlight => _loadCoordinator.isBusy; + Future _loadOnce() async { // Yield to the microtask queue before the first notify so a load() // kicked off during build (the screen's initState) doesn't mark diff --git a/lib/screens/discover_screen.dart b/lib/screens/discover_screen.dart index fb02bb7b..7de2d875 100644 --- a/lib/screens/discover_screen.dart +++ b/lib/screens/discover_screen.dart @@ -600,6 +600,17 @@ class _DiscoverScreenState extends State unawaited(_discover.load()); } + @override + void primeRefresh() { + // `initState` already fired `load()`. On cold start that pass is still + // running when the online-entry hook primes the tab, and asking again only + // queues an identical trailing pass — the whole home fan-out twice (#1784). + // When nothing is in flight (reconnect-from-offline, or a first pass that + // gave up because no server was online yet) a real refresh is still owed. + if (_discover.isLoadInFlight) return; + fullRefresh(); + } + /// Whether the loaded hubs span more than one connected server. bool _hubsSpanMultipleServers() { final serverIds = _hubs.where((hub) => hub.serverId != null).map((hub) => hub.serverId).toSet(); diff --git a/lib/screens/main_screen.dart b/lib/screens/main_screen.dart index 2ac1bdfe..d70cd54a 100644 --- a/lib/screens/main_screen.dart +++ b/lib/screens/main_screen.dart @@ -520,7 +520,7 @@ class _MainScreenState extends State } if (!mounted) return; - _fullRefreshContentTabs(); + _primeContentTabs(); } /// Single-shot "resume queued downloads once any client is online" rule, @@ -1634,15 +1634,26 @@ class _MainScreenState extends State if (_screenKeys[tab]?.currentState case final T state) fn(state); } - /// Full-refresh the primary content tabs. Shared by the online-entry hook - /// ([_primeOnlineServices]) and the profile-switch invalidation - /// ([_invalidateAllScreens]), which refresh the same set. + /// Full-refresh the primary content tabs. Used by the profile-switch + /// invalidation ([_invalidateAllScreens]), which must refetch everything for + /// the new identity. void _fullRefreshContentTabs() { - for (final tab in const [NavigationTabId.discover, NavigationTabId.libraries, NavigationTabId.search]) { + for (final tab in _contentTabs) { _onScreen(tab, (screen) => screen.fullRefresh()); } } + /// Online-entry variant used by [_primeOnlineServices] on cold start and on + /// reconnect-from-offline. Screens that already started their own load skip + /// it; see [FullRefreshable.primeRefresh]. + void _primeContentTabs() { + for (final tab in _contentTabs) { + _onScreen(tab, (screen) => screen.primeRefresh()); + } + } + + static const _contentTabs = [NavigationTabId.discover, NavigationTabId.libraries, NavigationTabId.search]; + Widget _buildBottomNavigationBar(BuildContext context, {required bool hideLabels}) { final tabs = _getBottomNavigationTabs(context); final selectedIndex = tabs.indexWhere((tab) => tab.id == _currentTab); diff --git a/lib/services/data_aggregation_service.dart b/lib/services/data_aggregation_service.dart index 5352f3f0..8f6847c3 100644 --- a/lib/services/data_aggregation_service.dart +++ b/lib/services/data_aggregation_service.dart @@ -453,29 +453,35 @@ class DataAggregationService { final serverLibraries = libraries?[serverId]; final shouldUseGlobalHubs = useGlobalHubs && client.capabilities.richHubs; final hubItemLimit = limit ?? defaultHubPreviewLimit; - final hubs = shouldUseGlobalHubs - ? [ - ...await client.fetchGlobalHubs(limit: hubItemLimit, includePlaybackHubs: includePlaybackHubs), - // Plex's promoted/global hub endpoint never includes music - // libraries — append their per-library hubs so music rows - // reach home. No-op (zero extra calls) without a visible - // music library. - ...await _fetchLibraryHubsForClient( - client, - limit: hubItemLimit, - hiddenLibraryKeys: hiddenLibraryKeys, - includePlaybackHubs: includePlaybackHubs, - libraries: serverLibraries ?? const [], - kinds: const {MediaKind.artist}, - ), - ] - : await _fetchLibraryHubsForClient( - client, - limit: hubItemLimit, - hiddenLibraryKeys: hiddenLibraryKeys, - includePlaybackHubs: includePlaybackHubs, - libraries: useGlobalHubs ? serverLibraries : null, - ); + List hubs; + if (shouldUseGlobalHubs) { + // Both legs are independent, so start them before awaiting either. + // Spreading `...await a, ...await b` into one list literal evaluates + // them in order, which serialised the music rows behind the global + // hub round trip. + final globalFuture = client.fetchGlobalHubs(limit: hubItemLimit, includePlaybackHubs: includePlaybackHubs); + // Plex's promoted/global hub endpoint never includes music + // libraries — append their per-library hubs so music rows + // reach home. No-op (zero extra calls) without a visible + // music library. + final musicFuture = _fetchLibraryHubsForClient( + client, + limit: hubItemLimit, + hiddenLibraryKeys: hiddenLibraryKeys, + includePlaybackHubs: includePlaybackHubs, + libraries: serverLibraries ?? const [], + kinds: const {MediaKind.artist}, + ); + hubs = [...await globalFuture, ...await musicFuture]; + } else { + hubs = await _fetchLibraryHubsForClient( + client, + limit: hubItemLimit, + hiddenLibraryKeys: hiddenLibraryKeys, + includePlaybackHubs: includePlaybackHubs, + libraries: useGlobalHubs ? serverLibraries : null, + ); + } return _postProcessHubs(hubs, serverId: ServerId(serverId), hiddenLibraryKeys: hiddenLibraryKeys); }, ); @@ -506,31 +512,39 @@ class DataAggregationService { return true; }).toList(); + // Sliding window rather than batches of three separated by a barrier: a + // batch waits for its slowest member before the next one starts, so wall + // time was the sum of per-batch maxima and one slow library stalled every + // library queued behind it (#1784). Starting the next request the moment + // any slot frees keeps the same peak concurrency with no head-of-line + // blocking. Results are written back by index so hub order stays the + // library order regardless of completion order. const concurrency = 3; - final all = []; - for (var start = 0; start < visible.length; start += concurrency) { - final batch = visible.skip(start).take(concurrency); - final results = await Future.wait( - batch.map((l) async { - try { - return await client.fetchLibraryHubs( - l.id, - libraryName: l.title, - limit: limit, - includePlaybackHubs: includePlaybackHubs, - libraryKind: l.kind, - ); - } catch (e, st) { - appLogger.e('Failed to fetch library hubs for ${l.globalKey}', error: e, stackTrace: st); - return []; - } - }), - ); - for (final list in results) { - all.addAll(list); + final results = List>.filled(visible.length, const []); + var next = 0; + + Future worker() async { + while (true) { + final index = next++; + if (index >= visible.length) return; + final library = visible[index]; + try { + results[index] = await client.fetchLibraryHubs( + library.id, + libraryName: library.title, + limit: limit, + includePlaybackHubs: includePlaybackHubs, + libraryKind: library.kind, + ); + } catch (e, st) { + appLogger.e('Failed to fetch library hubs for ${library.globalKey}', error: e, stackTrace: st); + } } } - return all; + + await Future.wait([for (var i = 0; i < concurrency && i < visible.length; i++) worker()]); + + return [for (final list in results) ...list]; } /// Filter hidden-library items and drop empty hubs. diff --git a/lib/services/jellyfin_client.dart b/lib/services/jellyfin_client.dart index 76e94740..e49b4c32 100644 --- a/lib/services/jellyfin_client.dart +++ b/lib/services/jellyfin_client.dart @@ -177,6 +177,11 @@ class JellyfinClient baseUrl: connection.baseUrl, defaultHeaders: headers, logLabel: 'Jellyfin', + // Same pool tuning Plex uses: the home fan-out issues several concurrent + // requests per pass, and the untuned dart:io default drops idle + // connections after 15s — a fresh TLS handshake per request on a + // high-RTT/CDN link. + usePlexApiClient: true, prioritizedEndpoints: connection.baseUrls, onEndpointSwitch: (newBaseUrl, {required persist}) => client._handleEndpointSwitch(newBaseUrl, persist: persist), onAllEndpointsExhausted: onAllEndpointsExhausted, diff --git a/lib/services/jellyfin_client/parts/browse.dart b/lib/services/jellyfin_client/parts/browse.dart index ebd11055..6302524f 100644 --- a/lib/services/jellyfin_client/parts/browse.dart +++ b/lib/services/jellyfin_client/parts/browse.dart @@ -2,23 +2,21 @@ part of '../../jellyfin_client.dart'; String _segment(String value) => Uri.encodeComponent(value); -/// Transport policy for a hub surface: bounded transient retries, no -/// endpoint failover. See `_getItemsResponse`. -typedef _HubRetryPolicy = ({String operation, List attemptTimeouts}); +/// Transport policy for a hub surface: one whole-request deadline, retries +/// only on immediate connection errors, no endpoint failover. See +/// `_getItemsResponse` and [retryTransientMediaServerCall]. +typedef _HubRetryPolicy = ({String operation, Duration deadline}); -const _HubRetryPolicy _homeHubRetry = ( - operation: 'Jellyfin home hubs', - attemptTimeouts: MediaServerTimeouts.homeHubAttemptTimeouts, -); +const _HubRetryPolicy _homeHubRetry = (operation: 'Jellyfin home hubs', deadline: MediaServerTimeouts.homeHubDeadline); const _HubRetryPolicy _libraryHubRetry = ( operation: 'Jellyfin library hubs', - attemptTimeouts: MediaServerTimeouts.libraryHubAttemptTimeouts, + deadline: MediaServerTimeouts.libraryHubDeadline, ); const _HubRetryPolicy _continueWatchingRetry = ( operation: 'Jellyfin continue watching', - attemptTimeouts: MediaServerTimeouts.homeHubAttemptTimeouts, + deadline: MediaServerTimeouts.homeHubDeadline, ); List> _itemsArray(Object? data) { @@ -58,15 +56,47 @@ LibraryPage _pagedItems( /// list calls; we ask for the minimum extras needed to drive the /// MediaItem mapper: /// - `RecursiveItemCount`/`ChildCount` for series leaf count -/// - `UserData` is included in defaults but pinned for safety -/// - `PremiereDate` for sort-by-release-date and episode metadata /// - `OriginalTitle`/`SortName` for sort + alphabetised display /// - `Overview` so list rows can show their description /// /// Heavier fields (`MediaSources`, `People`, `Genres`, `Tags`, `Studios`, /// `Taglines`, `ProviderIds`, `Chapters`) stay in [_detailFields] — together /// they added seconds to large-library pages on small home servers. -const _browseFields = 'RecursiveItemCount,ChildCount,UserData,PremiereDate,OriginalTitle,SortName,Overview'; +/// +/// `UserData` and `PremiereDate` are deliberately absent: neither is a member +/// of Jellyfin's `ItemFields` enum, so the server's +/// `CommaDelimitedCollectionModelBinder` drops them element-by-element and +/// they never did anything. `UserData` is governed by `EnableUserData` +/// (default true) and `dto.PremiereDate` is set unconditionally. +const _browseFields = 'RecursiveItemCount,ChildCount,OriginalTitle,SortName,Overview'; + +/// Field set for the home / per-library hub rows (Recently Added, Continue +/// Watching, Next Up). Poster cards render artwork, title, year and the +/// watch badge; only `Overview` needs asking for, to feed the mobile hero +/// (`discover_screen`) and the TV spotlight blurb. +/// +/// Crucially this drops `RecursiveItemCount`/`ChildCount`. `/Items/Latest` +/// groups a TV library by series, so those rows are Series FOLDER dtos and +/// each count field costs a per-row DB query server-side +/// (`Folder.GetRecursiveChildCount` / `Folder.GetChildCount`) — the same cost +/// already documented on [_folderBrowseFields] and [_musicAlbumRowFields], +/// which the home rows never got (#1784). +/// +/// Watch state survives intact: Jellyfin computes `UserData.Played` from +/// `UnplayedItemCount` when `RecursiveItemCount` is absent +/// (`Folder.FillUserDataDtoValues`), and [MediaItem.unwatchedCount] falls back +/// to `UserData.UnplayedItemCount`. Only the season progress bar needs real +/// leaf totals, and seasons never appear on a hub row. +const _hubRowFields = 'Overview'; + +/// How far back `/Shows/NextUp` looks for a series to resume, mirroring +/// Jellyfin web's `maxDaysForNextUp` default. Without it the server's +/// `GetNextUpSeriesKeys` scan is unbounded over every series the user has ever +/// played an episode of. +const _nextUpDateCutoffDays = 365; + +String _nextUpDateCutoff() => + DateTime.now().toUtc().subtract(const Duration(days: _nextUpDateCutoffDays)).toIso8601String(); /// Existing episode-row requests can show Plex-style quality labels when the /// response includes `MediaSources`. Keep this off broad library/search/latest @@ -213,11 +243,32 @@ mixin _JellyfinBrowseMethods on _JellyfinClientInternals { /// `_providerLibraries`. List? _loadedLibraryViews; + /// In-flight `/Views` request, shared by concurrent callers. + /// + /// At cold start `LibrariesProvider.loadLibraries()` and + /// `DataAggregationService.getHubsFromAllServers` both ask for libraries at + /// the same time, and the hub fan-out sits *serially* behind its copy. Two + /// identical uncached round trips is a full RTT of pure cold-start latency on + /// a remote server (#1784). Plex has never paid it — its library list comes + /// from the `/media/providers` response cached at client creation. + /// + /// Single-flight only: once the request settles the next caller re-fetches, + /// so a library added server-side still shows up on the next refresh. + Future>? _inFlightLibraries; + @override - Future> fetchLibraries() async { - final libraries = await _fetchLibraries(); - _loadedLibraryViews = libraries; - return libraries; + Future> fetchLibraries() { + final inFlight = _inFlightLibraries; + if (inFlight != null) return inFlight; + + final request = _fetchLibraries().then((libraries) { + _loadedLibraryViews = libraries; + return libraries; + }); + _inFlightLibraries = request; + return request.whenComplete(() { + if (identical(_inFlightLibraries, request)) _inFlightLibraries = null; + }); } /// [abort] tears the view fetch down with the pass that owns it — a @@ -1414,7 +1465,7 @@ mixin _JellyfinBrowseMethods on _JellyfinClientInternals { _fetchItemsArray('/UserItems/Resume', { 'userId': connection.userId, 'Limit': ?count?.toString(), - 'Fields': _browseFields, + 'Fields': _hubRowFields, 'MediaTypes': 'Video', 'Recursive': 'true', 'EnableTotalRecordCount': 'false', @@ -1423,8 +1474,9 @@ mixin _JellyfinBrowseMethods on _JellyfinClientInternals { _safeFetchItemsArray('/Shows/NextUp', { 'userId': connection.userId, 'Limit': ?count?.toString(), - 'Fields': _browseFields, + 'Fields': _hubRowFields, 'EnableResumable': 'false', + 'NextUpDateCutoff': _nextUpDateCutoff(), 'EnableTotalRecordCount': 'false', ...jellyfinImageQueryParameters, }, retry: _continueWatchingRetry), @@ -1519,7 +1571,7 @@ mixin _JellyfinBrowseMethods on _JellyfinClientInternals { final latestFuture = _safeFetchItemsArray('/Users/${_segment(connection.userId)}/Items/Latest', { 'Limit': limit.toString(), 'ParentId': ?parentId, - 'Fields': _browseFields, + 'Fields': _hubRowFields, 'IncludeItemTypes': ?latestItemTypes, ...jellyfinImageQueryParameters, }, retry: retry); @@ -1547,7 +1599,7 @@ mixin _JellyfinBrowseMethods on _JellyfinClientInternals { 'userId': connection.userId, 'ParentId': ?parentId, 'Limit': limit.toString(), - 'Fields': _browseFields, + 'Fields': _hubRowFields, 'MediaTypes': 'Video', 'Recursive': 'true', 'EnableTotalRecordCount': 'false', @@ -1558,8 +1610,9 @@ mixin _JellyfinBrowseMethods on _JellyfinClientInternals { 'userId': connection.userId, 'ParentId': ?parentId, 'Limit': limit.toString(), - 'Fields': _browseFields, + 'Fields': _hubRowFields, 'EnableResumable': 'false', + 'NextUpDateCutoff': _nextUpDateCutoff(), 'EnableTotalRecordCount': 'false', ...jellyfinImageQueryParameters, }, retry: retry) @@ -1701,7 +1754,7 @@ mixin _JellyfinBrowseMethods on _JellyfinClientInternals { 'IncludeItemTypes': 'Movie,Series,Episode,Video,MusicVideo,Photo', 'SortBy': 'DateCreated,SortName,ProductionYear', 'SortOrder': 'Descending,Descending,Descending', - 'Fields': _browseFields, + 'Fields': _hubRowFields, ...jellyfinImageQueryParameters, }, offset: offset, @@ -1732,7 +1785,7 @@ mixin _JellyfinBrowseMethods on _JellyfinClientInternals { 'userId': connection.userId, 'StartIndex': offset.toString(), 'Limit': effectiveLimit, - 'Fields': _browseFields, + 'Fields': _hubRowFields, 'Recursive': 'true', 'EnableTotalRecordCount': 'true', if (parentId != null) 'ParentId': parentId else 'MediaTypes': 'Video', @@ -1749,9 +1802,10 @@ mixin _JellyfinBrowseMethods on _JellyfinClientInternals { 'userId': connection.userId, 'StartIndex': offset.toString(), 'Limit': effectiveLimit, - 'Fields': _browseFields, + 'Fields': _hubRowFields, 'ParentId': ?parentId, 'EnableResumable': 'false', + 'NextUpDateCutoff': _nextUpDateCutoff(), 'EnableTotalRecordCount': 'true', ...jellyfinImageQueryParameters, }, @@ -2036,14 +2090,13 @@ mixin _JellyfinBrowseMethods on _JellyfinClientInternals { } /// GET [path], optionally under a hub-surface transport policy ([retry]): - /// bounded transient retries with per-attempt timeouts and **no endpoint - /// failover** — a slow hub row must not move the whole client off an - /// otherwise working endpoint (same policy as Plex's three hub fetches; - /// see [retryTransientMediaServerCall] / [FailoverHttpClient]). + /// one whole-request deadline, retries only on immediate connection errors, + /// and **no endpoint failover** — a slow hub row must not move the whole + /// client off an otherwise working endpoint (same policy as Plex's three hub + /// fetches; see [retryTransientMediaServerCall] / [FailoverHttpClient]). /// /// [timeout] and [allowEndpointFailover] configure the un-retried path only; a - /// [retry] policy carries its own per-attempt timeouts and always disables - /// failover. + /// [retry] policy carries its own deadline and always disables failover. Future _getItemsResponse( String path, Map queryParameters, @@ -2064,7 +2117,7 @@ mixin _JellyfinBrowseMethods on _JellyfinClientInternals { abort?.throwIfAborted(); return retryTransientMediaServerCall( operation: retry.operation, - attemptTimeouts: retry.attemptTimeouts, + deadline: retry.deadline, call: (timeout, attemptAbort) => _http.get( path, queryParameters: queryParameters, diff --git a/lib/services/library_query_translator.dart b/lib/services/library_query_translator.dart index b37a3615..942cec59 100644 --- a/lib/services/library_query_translator.dart +++ b/lib/services/library_query_translator.dart @@ -5,8 +5,13 @@ import 'plex_constants.dart'; /// Browse responses retain up to three backdrops so hero surfaces can rotate /// artwork without allowing image-tag payloads to grow without bound. const jellyfinBackdropImageLimit = 3; + +/// `Thumb` is deliberately absent: `JellyfinMappers` never reads +/// `ImageTags['Thumb']`, and `parentThumbPath`/`grandparentThumbPath` are built +/// from the season/series *Primary* tags. Asking for it added a dead image type +/// to ~40 requests and widened the server's inherited-image parent walk. const jellyfinImageQueryParameters = { - 'EnableImageTypes': 'Primary,Backdrop,Thumb,Logo', + 'EnableImageTypes': 'Primary,Backdrop,Logo', 'ImageTypeLimit': '$jellyfinBackdropImageLimit', }; diff --git a/lib/services/plex_client.dart b/lib/services/plex_client.dart index 76c88d48..c08cfdc9 100644 --- a/lib/services/plex_client.dart +++ b/lib/services/plex_client.dart @@ -1564,7 +1564,7 @@ class PlexClient final response = await retryTransientMediaServerCall( operation: 'Plex continue watching hubs', - attemptTimeouts: MediaServerTimeouts.homeHubAttemptTimeouts, + deadline: MediaServerTimeouts.homeHubDeadline, call: (timeout, abort) => _getWithFailover( continueWatchingHubKey ?? '/hubs', queryParameters: queryParameters, @@ -2244,7 +2244,7 @@ class PlexClient required String path, required Map queryParameters, required String operation, - required List attemptTimeouts, + required Duration deadline, required String failureLabel, int? librarySectionID, String? librarySectionTitle, @@ -2253,7 +2253,7 @@ class PlexClient try { final response = await retryTransientMediaServerCall( operation: operation, - attemptTimeouts: attemptTimeouts, + deadline: deadline, call: (timeout, abort) => _getWithFailover( path, queryParameters: queryParameters, @@ -2291,7 +2291,7 @@ class PlexClient path: '/hubs/sections/$sectionId', queryParameters: {'count': limit, 'includeGuids': 1}, operation: 'Plex library hubs', - attemptTimeouts: MediaServerTimeouts.libraryHubAttemptTimeouts, + deadline: MediaServerTimeouts.libraryHubDeadline, failureLabel: 'library hubs', librarySectionID: _librarySectionIdFromString(sectionId), librarySectionTitle: libraryName, @@ -2305,7 +2305,7 @@ class PlexClient path: _providerPromotedHubKey ?? _providerHomeHubKey ?? '/hubs', queryParameters: {'count': limit, 'includeGuids': 1}, operation: 'Plex global hubs', - attemptTimeouts: MediaServerTimeouts.homeHubAttemptTimeouts, + deadline: MediaServerTimeouts.homeHubDeadline, failureLabel: 'global hubs', ); @@ -2314,7 +2314,7 @@ class PlexClient path: '/hubs/metadata/$ratingKey/related', queryParameters: {'count': count}, operation: 'Plex related hubs', - attemptTimeouts: MediaServerTimeouts.libraryHubAttemptTimeouts, + deadline: MediaServerTimeouts.libraryHubDeadline, failureLabel: 'related hubs', filter: _videoOrCollectionHubItem, ); diff --git a/lib/utils/coalesced_load_coordinator.dart b/lib/utils/coalesced_load_coordinator.dart index 9d42952a..e260ff37 100644 --- a/lib/utils/coalesced_load_coordinator.dart +++ b/lib/utils/coalesced_load_coordinator.dart @@ -21,6 +21,11 @@ final class CoalescedLoadCoordinator { bool _pendingFull = false; bool _disposed = false; + /// Whether a pass is running (or queued behind the running one). Lets a + /// caller tell "this surface is already loading" from "nothing has started", + /// without changing the drain's trailing-replay contract. + bool get isBusy => _inFlight != null; + Future requestFull() { if (_disposed) return Future.value(); _pendingFull = true; diff --git a/lib/utils/media_server_retry.dart b/lib/utils/media_server_retry.dart index 51d230ea..4c412db2 100644 --- a/lib/utils/media_server_retry.dart +++ b/lib/utils/media_server_retry.dart @@ -4,50 +4,83 @@ import 'media_server_http_client.dart'; typedef MediaServerRetryCall = Future Function(Duration timeout, AbortController abort); -/// Retries media-server calls only when the failure is transient transport -/// noise. Callers pass per-attempt timeouts so cold-start surfaces can use a -/// bounded retry budget without changing global HTTP defaults. +/// Runs a media-server call under a single whole-request [deadline], retrying +/// only failures that cost nothing to retry. /// /// Retry vs failover (see `FailoverHttpClient` for the other half): retry is /// for a *slow-but-working* endpoint, failover is for a *dead* one. Surfaces /// wrapped in this helper should pass `allowEndpointFailover: false` on the /// inner GET so a slow row doesn't move the whole client off an otherwise /// working endpoint — every existing combined call site does. +/// +/// ## Why timeouts are not retried +/// +/// `http.Client.send` resolves when response *headers* arrive, so +/// [MediaServerHttpClient]'s connect budget covers DNS + TCP + TLS + request + +/// **the server's think time**. A Jellyfin `/Items/Latest` that needs 11s on a +/// large library therefore raises a `TimeoutException`, which +/// [MediaServerHttpException.from] types as +/// [MediaServerHttpErrorType.connectionTimeout] — indistinguishable from a +/// failed socket connect. +/// +/// Replaying it makes the server re-run the same expensive query from scratch, +/// with a *shorter* budget than the one it just missed. The old +/// `[10s, 8s, 5s]` ladder turned an 11s answer into an empty row after 23s +/// (#1784). A timeout means "this endpoint is slower than we can wait", and the +/// answer to that is to give up, not to ask three times. +/// +/// [MediaServerHttpErrorType.connectionError] is different: a refused +/// connection, DNS failure or reset socket fails immediately and costs the +/// server nothing, and is frequently a one-off on a mobile link. Those are +/// retried up to [maxConnectionRetries] times. +/// +/// [deadline] bounds the whole call — every attempt, plus whatever the last one +/// is still waiting on — and aborts the in-flight request when it expires. Each +/// attempt is *also* handed [deadline] as its own budget so the HTTP layer +/// fails on its own terms first; the outer bound is the backstop that makes the +/// guarantee unconditional. Future retryTransientMediaServerCall({ required String operation, - required List attemptTimeouts, + required Duration deadline, required MediaServerRetryCall call, -}) async { - if (attemptTimeouts.isEmpty) { - throw ArgumentError.value(attemptTimeouts, 'attemptTimeouts', 'must contain at least one timeout'); + int maxConnectionRetries = 2, +}) { + if (deadline <= Duration.zero) { + throw ArgumentError.value(deadline, 'deadline', 'must be positive'); } - for (var attempt = 0; attempt < attemptTimeouts.length; attempt++) { - final timeout = attemptTimeouts[attempt]; - final abort = AbortController(); - try { - return await call(timeout, abort); - } on MediaServerHttpException catch (e, st) { - abort.abort(); - final isLastAttempt = attempt == attemptTimeouts.length - 1; - if (!e.isTransient || isLastAttempt) { + AbortController? inFlight; + + Future attempts() async { + for (var attempt = 0; ; attempt++) { + final abort = AbortController(); + inFlight = abort; + try { + return await call(deadline, abort); + } on MediaServerHttpException catch (e, st) { + abort.abort(); + if (e.type != MediaServerHttpErrorType.connectionError || attempt >= maxConnectionRetries) { + Error.throwWithStackTrace(e, st); + } + appLogger.w( + 'Retrying $operation after a connection error', + error: {'attempt': attempt + 1, 'maxAttempts': maxConnectionRetries + 1, 'type': e.type.name}, + ); + } catch (e, st) { + abort.abort(); Error.throwWithStackTrace(e, st); } - - appLogger.w( - 'Retrying $operation after transient media-server failure', - error: { - 'attempt': attempt + 1, - 'maxAttempts': attemptTimeouts.length, - 'nextTimeoutMs': attemptTimeouts[attempt + 1].inMilliseconds, - 'type': e.type.name, - }, - ); - } catch (e, st) { - abort.abort(); - Error.throwWithStackTrace(e, st); } } - throw StateError('unreachable retry state'); + return attempts().timeout( + deadline, + onTimeout: () { + inFlight?.abort(); + throw MediaServerHttpException( + type: MediaServerHttpErrorType.connectionTimeout, + message: '$operation exceeded its ${deadline.inSeconds}s budget', + ); + }, + ); } diff --git a/lib/utils/media_server_timeouts.dart b/lib/utils/media_server_timeouts.dart index 2b3ba8f0..d8782647 100644 --- a/lib/utils/media_server_timeouts.dart +++ b/lib/utils/media_server_timeouts.dart @@ -7,13 +7,24 @@ class MediaServerTimeouts { static const receive = Duration(seconds: 120); - /// Retry budget for home `/hubs` startup calls. These endpoints can be slow - /// while Plex wakes idle disks, but should not block forever. - static const homeHubAttemptTimeouts = [Duration(seconds: 10), Duration(seconds: 5), Duration(milliseconds: 2500)]; + /// Whole-request deadline for home `/hubs` startup calls. These endpoints can + /// be slow while Plex wakes idle disks or a CDN-fronted Jellyfin runs a cold + /// query, but should not block forever. + /// + /// Deliberately a *single* budget rather than a retry ladder. `Client.send` + /// resolves when response headers arrive, so this budget covers the server's + /// think time, not just the socket connect — a slow-but-alive query trips it. + /// Replaying that request makes the server re-run the same expensive query + /// from scratch, so the old `[10s, 5s, 2.5s]` ladder turned an 11s answer + /// into a 17.5s empty row (#1784). See [retryTransientMediaServerCall]. + static const homeHubDeadline = Duration(seconds: 15); - /// Retry budget for per-library home hub rows (`/hubs/sections/{id}`). These - /// can be slower than the top-level home hub call on remote Plex servers. - static const libraryHubAttemptTimeouts = [Duration(seconds: 10), Duration(seconds: 8), Duration(seconds: 5)]; + /// Whole-request deadline for per-library home hub rows + /// (`/hubs/sections/{id}`, Jellyfin `/Items/Latest`). These can be slower + /// than the top-level home hub call on remote servers. Same single-budget + /// rationale as [homeHubDeadline] — it replaced `[10s, 8s, 5s]`, whose 23s + /// worst case was the dominant cold-start stall in #1784. + static const libraryHubDeadline = Duration(seconds: 20); /// Timeout for probing a cached/preferred endpoint (used in /// [PlexServer.findBestWorkingConnection]). diff --git a/test/screens/discover_screen_test.dart b/test/screens/discover_screen_test.dart index 510a05df..8f2d10af 100644 --- a/test/screens/discover_screen_test.dart +++ b/test/screens/discover_screen_test.dart @@ -1,3 +1,4 @@ +import 'dart:async'; import 'package:drift/native.dart'; import 'package:material_symbols_icons/symbols.dart'; import 'package:plezy/media/ids.dart'; @@ -14,6 +15,7 @@ import 'package:plezy/media/media_backend.dart'; import 'package:plezy/media/media_hub.dart'; import 'package:plezy/media/media_item.dart'; import 'package:plezy/media/media_kind.dart'; +import 'package:plezy/media/media_library.dart'; import 'package:plezy/media/media_server_client.dart'; import 'package:plezy/media/server_capabilities.dart'; import 'package:plezy/mixins/refreshable.dart'; @@ -240,6 +242,122 @@ void main() { expect(FocusManager.instance.primaryFocus?.debugLabel, 'tv_browse_rail'); }); + testWidgets('startup prime does not duplicate the load DiscoverScreen started in initState', (tester) async { + // DiscoverScreen.initState fires load(); main_screen._primeOnlineServices + // then primes the content tabs once libraries land. Asking for a full + // refresh there queued a trailing pass through CoalescedLoadCoordinator and + // ran the whole home fan-out twice on every cold start (#1784). + await SettingsService.getInstance(); + tester.view.devicePixelRatio = 1.0; + tester.view.physicalSize = const Size(1280, 720); + addTearDown(() { + tester.view.resetDevicePixelRatio(); + tester.view.resetPhysicalSize(); + }); + + final hub = MediaHub(id: 'hub_1', title: 'Recommended', type: 'movie', items: const [], size: 0); + final client = _GatedHubsFakeClient(hubs: [hub]); + // Disposes both the provider and the manager it wraps; MultiServerProvider + // does not own the manager, and manager.dispose() is what closes its + // status/progress controllers and the registered clients. + final multiServerProvider = testMultiServer(clients: [client]).provider; + final hiddenLibrariesProvider = HiddenLibrariesProvider(); + final librariesProvider = LibrariesProvider(); + final watchTogetherProvider = WatchTogetherProvider(); + final companionRemoteProvider = CompanionRemoteProvider(); + final db = AppDatabase.forTesting(NativeDatabase.memory()); + final connectionRegistry = _FakeConnectionRegistry(db); + final profileConnectionRegistry = _FakeProfileConnectionRegistry(db); + final storage = await StorageService.getInstance(); + final plexHome = PlexHomeService( + connections: connectionRegistry, + profileConnections: profileConnectionRegistry, + storage: storage, + plexHomeUserFetcher: (_) async => const [], + ); + final activeProfileProvider = ActiveProfileProvider( + registry: _FakeProfileRegistry(db), + plexHome: plexHome, + connections: connectionRegistry, + profileConnections: profileConnectionRegistry, + storage: storage, + ); + final discoverProvider = DiscoverProvider( + multiServerProvider, + hiddenLibrariesProvider, + librariesProvider, + profileId: null, + isProfileBinding: () => false, + ); + addTearDown(() async { + discoverProvider.dispose(); + activeProfileProvider.dispose(); + companionRemoteProvider.dispose(); + watchTogetherProvider.dispose(); + librariesProvider.dispose(); + hiddenLibrariesProvider.dispose(); + // multiServerProvider + its manager are torn down by testMultiServer. + await plexHome.dispose(); + await db.close(); + }); + + await tester.pumpWidget( + TranslationProvider( + child: MultiProvider( + providers: [ + ChangeNotifierProvider.value(value: multiServerProvider), + ChangeNotifierProvider.value(value: hiddenLibrariesProvider), + ChangeNotifierProvider.value(value: librariesProvider), + ChangeNotifierProvider.value(value: discoverProvider), + ChangeNotifierProvider.value(value: watchTogetherProvider), + ChangeNotifierProvider.value(value: companionRemoteProvider), + ChangeNotifierProvider.value(value: activeProfileProvider), + ], + child: MaterialApp( + theme: monoTheme(dark: true), + home: MainScreenFocusScope( + focusSidebar: () {}, + focusContent: () {}, + isSidebarFocused: false, + sideNavigationWidth: SideNavigationRailState.expandedWidth, + reservedSideNavigationWidth: SideNavigationRailState.tvCollapsedWidth, + foregroundLeft: 0, + foregroundWidth: 1280, + viewportWidth: 1280, + child: const SizedBox(width: 1280, height: 720, child: DiscoverScreen()), + ), + ), + ), + ), + ); + await tester.pump(); + + // The initState pass is in flight, waiting on the server. + expect(discoverProvider.isLoadInFlight, isTrue); + expect(client.hubCalls, 1); + + final screen = tester.state(find.byType(DiscoverScreen)) as FullRefreshable; + screen.primeRefresh(); + + client.release(); + // Bounded pumps, not pumpAndSettle: the hero carousel runs a repeating + // auto-scroll timer, so the frame loop never goes quiet. + await tester.pump(); + await tester.pump(); + await tester.pump(); + + expect(client.hubCalls, 1, reason: 'the prime rode along with the load already running'); + expect(discoverProvider.isLoadInFlight, isFalse); + + // A prime with nothing in flight (reconnect-from-offline) must still refetch. + screen.primeRefresh(); + await tester.pump(); + client.release(); + await tester.pump(); + await tester.pump(); + + expect(client.hubCalls, 2); + }); testWidgets('TV selects Continue Watching when it arrives after recommendation hubs', (tester) async { await SettingsService.getInstance(); @@ -540,6 +658,56 @@ class _FakeMediaServerClient implements MediaServerClient { dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); } +/// Hub client that holds each fetch open until [release], so a test can act +/// while a Discover load pass is genuinely in flight. +class _GatedHubsFakeClient implements MediaServerClient { + _GatedHubsFakeClient({required this.hubs}); + + final List hubs; + int hubCalls = 0; + final _gates = >>[]; + + void release() { + for (final gate in _gates.where((g) => !g.isCompleted)) { + gate.complete(hubs); + } + } + + @override + ServerId get serverId => ServerId('server_1'); + + @override + String? get serverName => 'Server'; + + @override + MediaBackend get backend => MediaBackend.plex; + + @override + ServerCapabilities get capabilities => ServerCapabilities.plex; + + @override + Future> fetchContinueWatching({int? count = 20}) async => const []; + + @override + Future> fetchLibraries() async => const []; + + @override + Future> fetchGlobalHubs({int limit = defaultHubPreviewLimit, bool includePlaybackHubs = true}) { + hubCalls++; + final gate = Completer>(); + _gates.add(gate); + return gate.future; + } + + /// Reached by `MultiServerManager.dispose()` via `_closeClientGracefully`. + /// Releases any gate still open so teardown cannot leave a fetch hanging. + @override + void close() => release(); + + @override + dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); +} + class _FakeProfileRegistry extends ProfileRegistry { _FakeProfileRegistry(super.db); diff --git a/test/services/data_aggregation_bridge_test.dart b/test/services/data_aggregation_bridge_test.dart index 7340eff1..8f7ef495 100644 --- a/test/services/data_aggregation_bridge_test.dart +++ b/test/services/data_aggregation_bridge_test.dart @@ -1,3 +1,4 @@ +import 'dart:async'; import 'dart:convert'; import 'package:plezy/media/ids.dart'; @@ -11,6 +12,8 @@ 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'; +import 'package:plezy/media/media_hub.dart'; +import 'package:plezy/media/server_capabilities.dart'; import 'package:plezy/media/media_server_client.dart'; import 'package:plezy/media/media_item.dart'; import 'package:plezy/models/plex/plex_config.dart'; @@ -82,6 +85,59 @@ class _LibrariesClient implements MediaServerClient { dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); } +/// Per-library hub client whose fetches are held open individually, so a test +/// can observe exactly when the fan-out starts each library. +class _GatedHubsClient implements MediaServerClient { + _GatedHubsClient(this.libraries); + + final List libraries; + final started = []; + final _gates = >>{}; + + void complete(String libraryId) { + _gates[libraryId]!.complete([ + MediaHub( + id: '$libraryId.recent', + title: libraryId, + type: 'mixed', + items: const [], + size: 0, + libraryId: libraryId, + ), + ]); + } + + @override + ServerId get serverId => ServerId('server-1'); + + @override + String get serverName => 'Server'; + + @override + ServerCapabilities get capabilities => ServerCapabilities.jellyfin; + + @override + Future> fetchLibraries() async => libraries; + + @override + Future> fetchLibraryHubs( + String libraryId, { + String? libraryName, + int limit = defaultHubPreviewLimit, + bool includePlaybackHubs = true, + MediaKind? libraryKind, + }) { + started.add(libraryId); + return (_gates[libraryId] = Completer>()).future; + } + + @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 @@ -112,6 +168,43 @@ void main() { expect(result.libraries, isEmpty); expect(result.succeededServerIds, isEmpty); }); + test('per-library hub fan-out refills a free slot instead of waiting for the batch', () async { + // Old shape: batches of three separated by `Future.wait`, so the 4th + // library could not start until ALL of the first three had answered and + // one slow row stalled every row queued behind it (#1784). A sliding + // window keeps the same peak concurrency with no head-of-line blocking. + final client = _GatedHubsClient([ + for (var i = 1; i <= 4; i++) + MediaLibrary( + id: 'lib-$i', + backend: MediaBackend.jellyfin, + title: 'Library $i', + kind: MediaKind.movie, + serverId: ServerId('server-1'), + ), + ]); + manager.debugRegisterClientForTesting(client); + + final pending = service.getHubsFromAllServers(useGlobalHubs: false, includePlaybackHubs: false); + await pumpEventQueue(); + + expect(client.started, ['lib-1', 'lib-2', 'lib-3'], reason: 'concurrency stays at 3'); + + // Free exactly one slot. The 4th library must take it immediately, while + // its two batch-mates are still in flight. + client.complete('lib-1'); + await pumpEventQueue(); + + expect(client.started, ['lib-1', 'lib-2', 'lib-3', 'lib-4']); + + // Finish out of order — hub order must still follow library order. + client.complete('lib-4'); + client.complete('lib-3'); + client.complete('lib-2'); + + final result = await pending; + expect(result.hubs.map((hub) => hub.libraryId), ['lib-1', 'lib-2', 'lib-3', 'lib-4']); + }); test('searchAcrossServers and getOnDeckFromAllServers return empty when no clients', () async { final search = await service.searchAcrossServers('hello'); @@ -944,17 +1037,20 @@ void main() { reason: 'the home screen excludes playback-derived music rows', ); // Music Latest returns album FOLDER dtos — count/user-data fields would - // each cost a recursive per-album COUNT query (#1552); video libraries - // keep the full browse fields (series leaf counts). + // each cost a recursive per-album COUNT query (#1552). final musicLatest = captured.singleWhere( (uri) => uri.path == '/Users/user-1/Items/Latest' && uri.queryParameters['ParentId'] == 'music', ); expect(musicLatest.queryParameters['Fields'], 'PremiereDate,OriginalTitle,SortName'); expect(musicLatest.queryParameters['EnableUserData'], 'false'); + // Video Latest rows carry the same risk: `/Items/Latest` groups a TV + // library by series, so the rows are Series FOLDER dtos and the count + // fields cost a per-row COUNT each (#1784). Watch state survives via + // UserData.UnplayedItemCount, so the hub row set asks for neither. final movieLatest = captured.singleWhere( (uri) => uri.path == '/Users/user-1/Items/Latest' && uri.queryParameters['ParentId'] == 'movies', ); - expect(movieLatest.queryParameters['Fields'], contains('RecursiveItemCount')); + expect(movieLatest.queryParameters['Fields'], 'Overview'); expect(movieLatest.queryParameters.containsKey('EnableUserData'), isFalse); }); diff --git a/test/services/jellyfin_client_failures_test.dart b/test/services/jellyfin_client_failures_test.dart index a8fac468..14ce9b87 100644 --- a/test/services/jellyfin_client_failures_test.dart +++ b/test/services/jellyfin_client_failures_test.dart @@ -311,7 +311,33 @@ void main() { expect(client.connection.baseUrl, 'https://primary.example.com'); }); - test('hub surfaces retry transient failures without hopping endpoints', () async { + test('hub surfaces do not replay a timeout, and do not hop endpoints', () async { + // `Client.send` resolves on response headers, so a hub row that times out + // is usually a server still working on an expensive query. Replaying it + // makes the server start over — the 23s cold-start stall in #1784. + final attemptsByPath = {}; + final client = JellyfinClient.forTesting( + connection: _conn( + baseUrl: 'https://primary.example.com', + baseUrls: const ['https://primary.example.com', 'https://fallback.example.com'], + ), + httpClient: MockClient((req) async { + expect(req.url.host, 'primary.example.com', reason: 'retry-wrapped hub fetches must not fail over'); + attemptsByPath.update(req.url.path, (n) => n + 1, ifAbsent: () => 1); + throw TimeoutException('slow row'); + }), + ); + addTearDown(client.close); + + await expectLater(client.fetchContinueWatching(), throwsA(isA())); + + expect(attemptsByPath.values, everyElement(1)); + expect(client.connection.baseUrl, 'https://primary.example.com'); + }); + + test('hub surfaces retry an immediate connection error without hopping endpoints', () async { + // A refused/reset socket costs the server nothing and is often a one-off, + // so unlike a timeout it is worth asking again on the same endpoint. final attemptsByPath = {}; final client = JellyfinClient.forTesting( connection: _conn( @@ -321,7 +347,7 @@ void main() { httpClient: MockClient((req) async { expect(req.url.host, 'primary.example.com', reason: 'retry-wrapped hub fetches must not fail over'); final attempt = attemptsByPath.update(req.url.path, (n) => n + 1, ifAbsent: () => 1); - if (attempt == 1) throw TimeoutException('slow row'); + if (attempt == 1) throw http.ClientException('connection reset', req.url); return http.Response(jsonEncode({'Items': []}), 200, headers: {'content-type': 'application/json'}); }), ); diff --git a/test/services/jellyfin_client_urls_test.dart b/test/services/jellyfin_client_urls_test.dart index 97b23b9c..3d9f71d1 100644 --- a/test/services/jellyfin_client_urls_test.dart +++ b/test/services/jellyfin_client_urls_test.dart @@ -102,6 +102,38 @@ void main() { tearDown(() { client.close(); }); + test('concurrent fetchLibraries calls share one /Views request', () async { + // At cold start LibrariesProvider.loadLibraries() and + // DataAggregationService.getHubsFromAllServers ask for libraries at the + // same time, and the hub fan-out waits serially behind its copy. Two + // identical round trips was a full RTT of dead cold-start latency (#1784). + var views = 0; + final scoped = JellyfinClient.forTesting( + connection: _conn(), + httpClient: MockClient((req) async { + if (req.url.path == '/Users/user-1/Views') views++; + return http.Response( + jsonEncode({ + 'Items': [ + {'Id': 'lib-1', 'Name': 'Movies', 'CollectionType': 'movies'}, + ], + }), + 200, + headers: {'content-type': 'application/json'}, + ); + }), + ); + addTearDown(scoped.close); + + final results = await Future.wait([scoped.fetchLibraries(), scoped.fetchLibraries()]); + + expect(views, 1); + expect(results.map((libs) => libs.single.id), ['lib-1', 'lib-1']); + + // Single-flight, not a cache: a later pass still sees server-side changes. + await scoped.fetchLibraries(); + expect(views, 2); + }); test('buildDirectStreamUrl includes static flag, api_key, and device id', () { final url = client.buildDirectStreamUrl('item-99'); @@ -264,7 +296,7 @@ void main() { '/Items/$encodedItemId/SpecialFeatures', }); expect(requests.every((uri) => uri.queryParameters['userId'] == 'user-1'), isTrue); - expect(requests.every((uri) => uri.queryParameters['EnableImageTypes'] == 'Primary,Backdrop,Thumb,Logo'), isTrue); + expect(requests.every((uri) => uri.queryParameters['EnableImageTypes'] == 'Primary,Backdrop,Logo'), isTrue); expect(requests.every((uri) => uri.queryParameters['ImageTypeLimit'] == '3'), isTrue); expect(extras.map((item) => item.id).toList(), ['trailer-1', 'featurette-1']); expect(extras.every((item) => item.kind.isVideo), isTrue); @@ -2291,7 +2323,7 @@ void main() { expect(captured!.queryParameters['EnableTotalRecordCount'], 'true'); expect(captured!.queryParameters['IncludeItemTypes'], 'Movie'); expect(captured!.queryParameters['Fields'], isNot(contains('MediaSources'))); - expect(captured!.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Thumb,Logo'); + expect(captured!.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Logo'); expect(captured!.queryParameters['ImageTypeLimit'], '3'); }); @@ -2617,7 +2649,7 @@ void main() { expect(captured!.queryParameters['SortBy'], 'PremiereDate,ProductionYear,SortName'); expect(captured!.queryParameters['SortOrder'], 'Descending,Descending,Ascending'); expect(captured!.queryParameters['CollapseBoxSetItems'], 'false'); - expect(captured!.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Thumb,Logo'); + expect(captured!.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Logo'); expect(captured!.queryParameters['ImageTypeLimit'], '3'); }); @@ -2643,7 +2675,7 @@ void main() { expect(capturedNextUp, isNotNull); expect(capturedNextUp!.queryParameters['seriesId'], 'show-1'); expect(capturedNextUp!.queryParameters['Limit'], '1'); - expect(capturedNextUp!.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Thumb,Logo'); + expect(capturedNextUp!.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Logo'); expect(capturedNextUp!.queryParameters['ImageTypeLimit'], '3'); expect(capturedNextUp!.queryParameters.containsKey('EnableResumable'), isFalse); expect(capturedNextUp!.queryParameters.containsKey('NextUpDateCutoff'), isFalse); @@ -2747,16 +2779,21 @@ void main() { expect(resume.queryParameters['MediaTypes'], 'Video'); expect(resume.queryParameters['Recursive'], 'true'); expect(resume.queryParameters['EnableTotalRecordCount'], 'false'); - expect(resume.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Thumb,Logo'); + expect(resume.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Logo'); expect(resume.queryParameters['ImageTypeLimit'], '3'); final nextUp = requests.singleWhere((uri) => uri.path == '/Shows/NextUp'); expect(nextUp.queryParameters['userId'], 'user-1'); expect(nextUp.queryParameters['Limit'], '3'); expect(nextUp.queryParameters['EnableResumable'], 'false'); expect(nextUp.queryParameters['EnableTotalRecordCount'], 'false'); - expect(nextUp.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Thumb,Logo'); + expect(nextUp.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Logo'); expect(nextUp.queryParameters['ImageTypeLimit'], '3'); - expect(nextUp.queryParameters.containsKey('NextUpDateCutoff'), isFalse); + // Bounds the server's unbounded GetNextUpSeriesKeys scan (#1784). + expect( + DateTime.parse(nextUp.queryParameters['NextUpDateCutoff']!), + isNot(null), + reason: 'must be a parseable ISO-8601 instant', + ); }); test('fetchContinueWatching orders a recently watched series Next Up above an older resume item', () async { @@ -3226,7 +3263,7 @@ void main() { expect(hubs.single.more, isTrue); }); - test('global Next Up excludes resumable episodes without date cutoff', () async { + test('global Next Up excludes resumable episodes and bounds the server scan with a date cutoff', () async { final client = buildClient(); addTearDown(client.close); @@ -3240,9 +3277,9 @@ void main() { expect(nextUp.queryParameters['Limit'], '12'); expect(nextUp.queryParameters['EnableResumable'], 'false'); expect(nextUp.queryParameters['EnableTotalRecordCount'], 'false'); - expect(nextUp.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Thumb,Logo'); + expect(nextUp.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Logo'); expect(nextUp.queryParameters['ImageTypeLimit'], '3'); - expect(nextUp.queryParameters.containsKey('NextUpDateCutoff'), isFalse); + expect(nextUp.queryParameters['NextUpDateCutoff'], isNotNull, reason: 'bounds the server-side scan (#1784)'); }); test('can skip global playback hubs', () async { @@ -3269,7 +3306,7 @@ void main() { return JellyfinClient.forTesting(connection: _conn(), httpClient: mock); } - test('show library Next Up excludes resumable episodes without date cutoff', () async { + test('show library Next Up excludes resumable episodes and bounds the server scan with a date cutoff', () async { final client = buildClient(); addTearDown(client.close); @@ -3281,9 +3318,9 @@ void main() { expect(nextUp.queryParameters['Limit'], '12'); expect(nextUp.queryParameters['EnableResumable'], 'false'); expect(nextUp.queryParameters['EnableTotalRecordCount'], 'false'); - expect(nextUp.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Thumb,Logo'); + expect(nextUp.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Logo'); expect(nextUp.queryParameters['ImageTypeLimit'], '3'); - expect(nextUp.queryParameters.containsKey('NextUpDateCutoff'), isFalse); + expect(nextUp.queryParameters['NextUpDateCutoff'], isNotNull, reason: 'bounds the server-side scan (#1784)'); }); test('movie library skips Next Up and disables resume total count', () async { @@ -3337,7 +3374,7 @@ void main() { expect(captured!.queryParameters['IncludeItemTypes'], 'Movie,Series,Episode,Video,MusicVideo,Photo'); expect(captured!.queryParameters['SortBy'], 'DateCreated,SortName,ProductionYear'); expect(captured!.queryParameters['SortOrder'], 'Descending,Descending,Descending'); - expect(captured!.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Thumb,Logo'); + expect(captured!.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Logo'); expect(captured!.queryParameters['ImageTypeLimit'], '3'); expect(captured!.queryParameters.containsKey('ParentId'), isFalse); client.close(); @@ -3355,7 +3392,7 @@ void main() { expect(captured!.queryParameters['MediaTypes'], 'Video'); expect(captured!.queryParameters['Recursive'], 'true'); expect(captured!.queryParameters['EnableTotalRecordCount'], 'true'); - expect(captured!.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Thumb,Logo'); + expect(captured!.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Logo'); expect(captured!.queryParameters['ImageTypeLimit'], '3'); expect(captured!.queryParameters.containsKey('ParentId'), isFalse); client.close(); @@ -3373,9 +3410,9 @@ void main() { expect(captured!.queryParameters.containsKey('ParentId'), isFalse); expect(captured!.queryParameters['EnableResumable'], 'false'); expect(captured!.queryParameters['EnableTotalRecordCount'], 'true'); - expect(captured!.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Thumb,Logo'); + expect(captured!.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Logo'); expect(captured!.queryParameters['ImageTypeLimit'], '3'); - expect(captured!.queryParameters.containsKey('NextUpDateCutoff'), isFalse); + expect(captured!.queryParameters['NextUpDateCutoff'], isNotNull, reason: 'bounds the server-side scan (#1784)'); client.close(); }); @@ -3392,7 +3429,7 @@ void main() { expect(captured!.queryParameters['Recursive'], 'true'); expect(captured!.queryParameters['EnableTotalRecordCount'], 'true'); expect(captured!.queryParameters['IncludeItemTypes'], 'Movie,Series,Episode,Video,MusicVideo,Photo'); - expect(captured!.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Thumb,Logo'); + expect(captured!.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Logo'); expect(captured!.queryParameters['ImageTypeLimit'], '3'); client.close(); }); @@ -3408,7 +3445,7 @@ void main() { expect(captured!.queryParameters['StartIndex'], '0'); expect(captured!.queryParameters['Recursive'], 'true'); expect(captured!.queryParameters['EnableTotalRecordCount'], 'true'); - expect(captured!.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Thumb,Logo'); + expect(captured!.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Logo'); expect(captured!.queryParameters['ImageTypeLimit'], '3'); client.close(); }); @@ -3424,9 +3461,9 @@ void main() { expect(captured!.queryParameters['StartIndex'], '0'); expect(captured!.queryParameters['EnableResumable'], 'false'); expect(captured!.queryParameters['EnableTotalRecordCount'], 'true'); - expect(captured!.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Thumb,Logo'); + expect(captured!.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Logo'); expect(captured!.queryParameters['ImageTypeLimit'], '3'); - expect(captured!.queryParameters.containsKey('NextUpDateCutoff'), isFalse); + expect(captured!.queryParameters['NextUpDateCutoff'], isNotNull, reason: 'bounds the server-side scan (#1784)'); client.close(); }); @@ -3602,12 +3639,9 @@ void main() { expect(itemsRequest.queryParameters['Limit'], '36'); expect(itemsRequest.queryParameters['SortBy'], 'SortName'); expect(itemsRequest.queryParameters['SortOrder'], 'Ascending'); - expect( - itemsRequest.queryParameters['Fields'], - 'RecursiveItemCount,ChildCount,UserData,PremiereDate,OriginalTitle,SortName,Overview', - ); + expect(itemsRequest.queryParameters['Fields'], 'RecursiveItemCount,ChildCount,OriginalTitle,SortName,Overview'); expect(itemsRequest.queryParameters.containsKey('EnableTotalRecordCount'), isFalse); - expect(itemsRequest.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Thumb,Logo'); + expect(itemsRequest.queryParameters['EnableImageTypes'], 'Primary,Backdrop,Logo'); expect(itemsRequest.queryParameters['ImageTypeLimit'], '3'); }); diff --git a/test/services/library_query_translator_test.dart b/test/services/library_query_translator_test.dart index d54bf0e6..e30840a9 100644 --- a/test/services/library_query_translator_test.dart +++ b/test/services/library_query_translator_test.dart @@ -89,7 +89,7 @@ void main() { expect(params['Fields'], 'UserData'); expect(params['IncludeItemTypes'], isNotEmpty); expect(params['EnableTotalRecordCount'], 'true'); - expect(params['EnableImageTypes'], 'Primary,Backdrop,Thumb,Logo'); + expect(params['EnableImageTypes'], 'Primary,Backdrop,Logo'); expect(params['ImageTypeLimit'], '3'); }); diff --git a/test/services/plex_home_retry_test.dart b/test/services/plex_home_retry_test.dart index aa1fdca5..b18468dd 100644 --- a/test/services/plex_home_retry_test.dart +++ b/test/services/plex_home_retry_test.dart @@ -62,13 +62,13 @@ void main() { }); group('PlexClient home hub retries', () { - test('fetchGlobalHubs retries a transient first failure', () async { + test('fetchGlobalHubs retries a first-attempt connection error', () async { final db = AppDatabase.forTesting(NativeDatabase.memory()); PlexApiCache.initialize(db); addTearDown(db.close); final httpClient = _SequenceClient([ - (_) async => throw TimeoutException('cold Plex start'), + (_) async => throw http.ClientException('connection reset on cold Plex start'), (_) async => _jsonResponse(_globalHubsPayload()), ]); final client = PlexClient.forTesting( @@ -96,6 +96,38 @@ void main() { expect(httpClient.requests.map((r) => r.url.queryParameters['count']), everyElement('12')); }); + test('fetchGlobalHubs does not replay a hub row that timed out', () async { + // `Client.send` resolves on response headers, so a hub timeout usually + // means the server is still working on the query. Replaying it made the + // server start over on a shorter budget — the #1784 cold-start stall. + final db = AppDatabase.forTesting(NativeDatabase.memory()); + PlexApiCache.initialize(db); + addTearDown(db.close); + + final httpClient = _SequenceClient([ + (_) async => throw TimeoutException('server still building the hub'), + (_) async => _jsonResponse(_globalHubsPayload()), + ]); + final client = PlexClient.forTesting( + config: PlexConfig( + baseUrl: 'http://server:32400', + token: 'token', + clientIdentifier: 'client-id', + product: 'Plezy', + version: 'test', + ), + serverId: ServerId('server-id'), + profileScopeId: buildPlexProfileScopeId(serverId: ServerId('server-id'), profileId: 'test-profile'), + serverName: 'Server', + httpClient: httpClient, + ); + addTearDown(client.close); + + // `_fetchHubs` degrades a failed row to empty rather than sinking home. + expect(await client.fetchGlobalHubs(limit: 12), isEmpty); + expect(httpClient.requests, hasLength(1)); + }); + test('fetchGlobalHubs sends configured Plex language headers', () async { final db = AppDatabase.forTesting(NativeDatabase.memory()); PlexApiCache.initialize(db); @@ -159,7 +191,7 @@ void main() { expect(httpClient.requests[1].headers['X-Plex-Language'], 'fr'); }); - test('fetchGlobalHubs retries transient failures without switching Plex endpoints', () async { + test('fetchGlobalHubs retries a connection error without switching Plex endpoints', () async { final db = AppDatabase.forTesting(NativeDatabase.memory()); PlexApiCache.initialize(db); addTearDown(db.close); @@ -167,7 +199,7 @@ void main() { const primary = 'http://primary:32400'; const fallback = 'http://fallback:32400'; final httpClient = _SequenceClient([ - (_) async => throw TimeoutException('queued behind cold handshakes'), + (_) async => throw http.ClientException('connection reset'), (_) async => _jsonResponse(_globalHubsPayload()), ]); final client = PlexClient.forTesting( @@ -327,7 +359,7 @@ void main() { expect(httpClient.requests.single.url.queryParameters['includeGuids'], '1'); }); - test('fetchLibraryHubs retries transient failures without switching Plex endpoints', () async { + test('fetchLibraryHubs retries a connection error without switching Plex endpoints', () async { final db = AppDatabase.forTesting(NativeDatabase.memory()); PlexApiCache.initialize(db); addTearDown(db.close); @@ -335,7 +367,7 @@ void main() { const primary = 'http://primary:32400'; const fallback = 'http://fallback:32400'; final httpClient = _SequenceClient([ - (_) async => throw TimeoutException('queued behind image downloads'), + (_) async => throw http.ClientException('connection reset'), (_) async => _jsonResponse(_globalHubsPayload()), ]); final client = PlexClient.forTesting( diff --git a/test/utils/media_server_retry_test.dart b/test/utils/media_server_retry_test.dart index ffdeb402..2b720e63 100644 --- a/test/utils/media_server_retry_test.dart +++ b/test/utils/media_server_retry_test.dart @@ -1,33 +1,164 @@ +import 'package:fake_async/fake_async.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:plezy/exceptions/media_server_exceptions.dart'; import 'package:plezy/utils/media_server_http_client.dart'; import 'package:plezy/utils/media_server_retry.dart'; +MediaServerHttpException _error(MediaServerHttpErrorType type) => + MediaServerHttpException(type: type, message: type.name); + void main() { group('retryTransientMediaServerCall', () { - test('retries transient failures in timeout order', () async { - const timeouts = [Duration(seconds: 10), Duration(seconds: 5), Duration(milliseconds: 2500)]; + test('runs the call once with the whole deadline as its budget', () async { final seenTimeouts = []; - final aborts = []; final result = await retryTransientMediaServerCall( operation: 'test operation', - attemptTimeouts: timeouts, - call: (timeout, abort) async { + deadline: const Duration(seconds: 20), + call: (timeout, _) async { seenTimeouts.add(timeout); - aborts.add(abort); - if (seenTimeouts.length < 3) { - throw MediaServerHttpException(type: MediaServerHttpErrorType.connectionTimeout, message: 'timed out'); - } return 'ok'; }, ); expect(result, 'ok'); - expect(seenTimeouts, timeouts); - expect(aborts[0].isAborted, isTrue); - expect(aborts[1].isAborted, isTrue); - expect(aborts[2].isAborted, isFalse); + expect(seenTimeouts, [const Duration(seconds: 20)]); + }); + + // The regression this whole policy exists for: `http.Client.send` resolves + // on response HEADERS, so a slow-but-alive server surfaces as + // connectionTimeout. Replaying it made the server re-run the same query and + // turned an 11s answer into an empty row after 23s (#1784). + test('does not replay a timeout — the server is working, just slow', () async { + var attempts = 0; + + await expectLater( + retryTransientMediaServerCall( + operation: 'test operation', + deadline: const Duration(seconds: 20), + call: (_, _) async { + attempts++; + throw _error(MediaServerHttpErrorType.connectionTimeout); + }, + ), + throwsA( + isA().having((e) => e.type, 'type', MediaServerHttpErrorType.connectionTimeout), + ), + ); + + expect(attempts, 1); + }); + + test('a slow response that lands inside the deadline resolves, and is requested once', () { + fakeAsync((async) { + var attempts = 0; + Object? result; + + // Answers at T+11s, well past the old 10s first-attempt budget. + retryTransientMediaServerCall( + operation: 'test operation', + deadline: const Duration(seconds: 20), + call: (_, _) async { + attempts++; + await Future.delayed(const Duration(seconds: 11)); + return 'late but fine'; + }, + ).then((value) => result = value); + + async.elapse(const Duration(seconds: 10, milliseconds: 900)); + expect(result, isNull, reason: 'still in flight'); + expect(attempts, 1); + + async.elapse(const Duration(milliseconds: 200)); + expect(result, 'late but fine'); + expect(attempts, 1, reason: 'never replayed'); + }); + }); + + test('retries an immediate connection error', () { + fakeAsync((async) { + final aborts = []; + Object? result; + + retryTransientMediaServerCall( + operation: 'test operation', + deadline: const Duration(seconds: 20), + call: (_, abort) async { + aborts.add(abort); + await Future.delayed(const Duration(seconds: 1)); + if (aborts.length < 3) throw _error(MediaServerHttpErrorType.connectionError); + return 'ok'; + }, + ).then((value) => result = value); + + async.elapse(const Duration(seconds: 5)); + + expect(result, 'ok'); + expect(aborts, hasLength(3)); + expect(aborts[0].isAborted, isTrue); + expect(aborts[1].isAborted, isTrue); + expect(aborts[2].isAborted, isFalse); + }); + }); + + test('gives up at the deadline and aborts the in-flight request', () { + fakeAsync((async) { + var attempts = 0; + AbortController? last; + Duration? settledAt; + Object? error; + + retryTransientMediaServerCall( + operation: 'test operation', + deadline: const Duration(seconds: 10), + call: (_, abort) async { + attempts++; + last = abort; + // Never answers: the request the deadline has to cut off. + await Future.delayed(const Duration(days: 1)); + }, + ).catchError((Object e) { + error = e; + settledAt = async.elapsed; + }); + + async.elapse(const Duration(seconds: 30)); + + expect(attempts, 1, reason: 'a timeout is never replayed'); + expect(settledAt, const Duration(seconds: 10)); + expect(last?.isAborted, isTrue, reason: 'the in-flight request is torn down'); + expect( + error, + isA().having((e) => e.type, 'type', MediaServerHttpErrorType.connectionTimeout), + ); + }); + }); + + test('bounds total wall time by the deadline, not by attempts × timeout', () { + fakeAsync((async) { + Duration? settledAt; + Object? error; + + retryTransientMediaServerCall( + operation: 'test operation', + deadline: const Duration(seconds: 15), + call: (_, _) async { + // Every attempt burns most of the budget before failing in a + // retryable way — the worst case for a retry loop. + await Future.delayed(const Duration(seconds: 6)); + throw _error(MediaServerHttpErrorType.connectionError); + }, + ).catchError((Object e) { + error = e; + settledAt = async.elapsed; + }); + + async.elapse(const Duration(seconds: 120)); + + expect(error, isA()); + expect(settledAt, isNotNull); + expect(settledAt!, lessThanOrEqualTo(const Duration(seconds: 15))); + }); }); test('does not retry non-transient failures', () async { @@ -36,7 +167,7 @@ void main() { await expectLater( retryTransientMediaServerCall( operation: 'test operation', - attemptTimeouts: const [Duration(seconds: 10), Duration(seconds: 5)], + deadline: const Duration(seconds: 20), call: (_, _) async { attempts++; throw MediaServerHttpException( @@ -52,22 +183,22 @@ void main() { expect(attempts, 1); }); - test('rethrows final transient failure after exhausting attempts', () async { + test('does not swallow a cancellation as a retryable failure', () async { var attempts = 0; await expectLater( retryTransientMediaServerCall( operation: 'test operation', - attemptTimeouts: const [Duration(seconds: 10), Duration(seconds: 5), Duration(milliseconds: 2500)], + deadline: const Duration(seconds: 20), call: (_, _) async { attempts++; - throw MediaServerHttpException(type: MediaServerHttpErrorType.receiveTimeout, message: 'receive timed out'); + throw _error(MediaServerHttpErrorType.cancelled); }, ), - throwsA(isA().having((e) => e.type, 'type', MediaServerHttpErrorType.receiveTimeout)), + throwsA(isA().having((e) => e.isCancellation, 'isCancellation', isTrue)), ); - expect(attempts, 3); + expect(attempts, 1); }); }); }