diff --git a/lib/services/playback_progress_tracker.dart b/lib/services/playback_progress_tracker.dart index 8e353bd2..ba233c05 100644 --- a/lib/services/playback_progress_tracker.dart +++ b/lib/services/playback_progress_tracker.dart @@ -154,6 +154,11 @@ class PlaybackProgressTracker { /// Whether this backend reporting session has successfully opened. bool _hasDeliveredStart = false; + /// Whether a delivered stopped report reached a backend session able to act + /// on it. MediaBrowser drops a stop for a session it never opened, so both + /// the position it would persist and the watch it would record are lost. + bool _stoppedReportActedOn = false; + /// Whether the delivered stopped report persisted its position. bool _stoppedProgressServerAcknowledged = false; @@ -161,7 +166,8 @@ class PlaybackProgressTracker { /// upgrade local provenance even when the position delta is throttled. bool _lastProgressNotificationServerAcknowledged = false; - /// The exact report-derived watched patch that an explicit mark can settle. + /// The exact report-derived watched patch that a settled server-side watch + /// can promote. Cleared once promoted so promotion happens at most once. WatchPatchId? _watchedPatchId; /// Whether the final stopped progress event was already emitted locally. @@ -280,6 +286,7 @@ class PlaybackProgressTracker { _serverObservedCrossing = false; _sessionEnded = false; _hasDeliveredStart = false; + _stoppedReportActedOn = false; _stoppedProgressServerAcknowledged = false; } @@ -478,8 +485,15 @@ class PlaybackProgressTracker { final persistsPositionOnEveryReport = !metadata.backend.usesMediaBrowserApi; if (snapshot.isStopped) { // MediaBrowser needs a successfully opened session before Stopped can - // persist position; Plex timeline reports are independent. - _stoppedProgressServerAcknowledged = persistsPositionOnEveryReport || _hasDeliveredStart; + // persist position or record the watch; Plex timeline reports are + // independent. + _stoppedReportActedOn = persistsPositionOnEveryReport || _hasDeliveredStart; + _stoppedProgressServerAcknowledged = _stoppedReportActedOn; + // A backend that marks played from the stop has now done so, so a + // crossing latched earlier this session is no longer a write owed to + // it. Ordering runs both ways -- the crossing can latch before this + // stop or from it -- so _settleServerMark promotes on the other path. + if (_stoppedReportActedOn && (c?.marksWatchedOnPlaybackStopped ?? false)) _promoteWatchedPatch(); } else { final isStarted = !_hasDeliveredStart; _hasDeliveredStart = true; @@ -523,6 +537,10 @@ class PlaybackProgressTracker { // double-scrobble through the Jellyfin Trakt plugin (#1287). if (c.marksWatchedOnPlaybackStopped) { _serverMarkSettled = true; + // Only once the stop actually reached an open session: settling happens + // when the crossing latches, which can be before the stop is sent, and + // until it lands the watch is still owed to the server. + if (_stoppedReportActedOn) _promoteWatchedPatch(); await _runScrobbledHook(); return; } @@ -530,6 +548,9 @@ class PlaybackProgressTracker { // again records the same watch twice (#1740). if (_serverObservedCrossing) { _serverMarkSettled = true; + // Delivery is proven: the crossing was assembled from two delivered + // reports, so nothing is owed. + _promoteWatchedPatch(); await _runScrobbledHook(); return; } @@ -557,11 +578,25 @@ class PlaybackProgressTracker { _serverMarkSettled = false; // Retry on the next delivered report. return; } - final patchId = _watchedPatchId; - if (patchId != null) WatchPatchPromotionNotifier().promote(patchId); + _promoteWatchedPatch(); await _runScrobbledHook(); } + /// Settle the report-derived watched patch: the server has taken this watch, + /// so the overlay entry is no longer a write owed to it and a later + /// authoritative read may supersede it. + /// + /// Without this a crossing pins `watched` for the whole session — the read + /// that would clear it cannot, because an unacknowledged patch is never + /// suppressed. Idempotent: the id is cleared so repeated settle paths and + /// the delivery callback cannot promote twice. + void _promoteWatchedPatch() { + final patchId = _watchedPatchId; + if (patchId == null) return; + _watchedPatchId = null; + WatchPatchPromotionNotifier().promote(patchId); + } + /// Runs the post-watch hook once, after the item's own watched state is /// accounted for server-side. /// diff --git a/test/services/playback_progress_tracker_test.dart b/test/services/playback_progress_tracker_test.dart index 8947da5c..564e523d 100644 --- a/test/services/playback_progress_tracker_test.dart +++ b/test/services/playback_progress_tracker_test.dart @@ -1855,6 +1855,178 @@ void main() { expect(tracker.dispose, returnsNormally); }); }); + + // A report-derived crossing writes an unacknowledged overlay patch, which the + // store never suppresses. Unless the settled server-side watch promotes it, + // it pins `watched` for the rest of the session and no refresh can clear it. + group('watched-patch promotion', () { + List listenForPromotions() { + final seen = []; + final sub = WatchPatchPromotionNotifier().stream.listen((p) => seen.add(p.patchId)); + addTearDown(sub.cancel); + return seen; + } + + /// Watched events prove the crossing actually latched and created a patch, + /// so a "did not promote" assertion cannot pass vacuously. + List listenForWatchedEvents() { + final seen = []; + final sub = WatchStateNotifier().stream.listen((e) { + if (e.changeType == WatchStateChangeType.watched) seen.add(e); + }); + addTearDown(sub.cancel); + return seen; + } + + test('a delivered stop promotes the crossing on a backend that marks on stop', () async { + final promotions = listenForPromotions(); + final client = _StopMarksWatchedClient(); + final player = _FakePlayer(position: const Duration(seconds: 95), duration: const Duration(seconds: 100)); + final tracker = PlaybackProgressTracker(client: client, metadata: _meta(), player: player, isOffline: false); + addTearDown(tracker.dispose); + + await tracker.sendProgress('stopped'); + await Future.delayed(Duration.zero); + + expect(promotions, hasLength(1)); + // The #1287 contract is unchanged: still no explicit mark. + expect(client.markWatchedCalls, isEmpty); + }); + + test('a MediaBrowser stop for a session that never opened does not promote', () async { + // Jellyfin drops a stop for a session it never opened, so the watch it + // would have recorded never happened and the patch is still owed. + final promotions = listenForPromotions(); + final watched = listenForWatchedEvents(); + final client = _StopMarksWatchedClient(); + final player = _FakePlayer(position: const Duration(seconds: 95), duration: const Duration(seconds: 100)); + final tracker = PlaybackProgressTracker( + client: client, + metadata: testMediaItem( + id: '42', + backend: MediaBackend.jellyfin, + kind: MediaKind.movie, + serverId: ServerId('srv'), + ), + player: player, + isOffline: false, + ); + addTearDown(tracker.dispose); + + await tracker.sendProgress('stopped'); + await Future.delayed(Duration.zero); + + // The crossing latched locally, so there is a patch to leave owed. + expect(watched, hasLength(1)); + expect(promotions, isEmpty); + }); + + test('a MediaBrowser stop into an opened session promotes', () async { + final promotions = listenForPromotions(); + final client = _StopMarksWatchedClient(); + final player = _FakePlayer(position: const Duration(seconds: 10), duration: const Duration(seconds: 100)); + final tracker = PlaybackProgressTracker( + client: client, + metadata: testMediaItem( + id: '42', + backend: MediaBackend.jellyfin, + kind: MediaKind.movie, + serverId: ServerId('srv'), + ), + player: player, + isOffline: false, + ); + addTearDown(tracker.dispose); + + await tracker.sendProgress('playing'); + player.position = const Duration(seconds: 95); + await tracker.sendProgress('stopped'); + await Future.delayed(Duration.zero); + + expect(promotions, hasLength(1)); + }); + + test('a server-observed crossing promotes without an explicit mark (#1740)', () async { + final promotions = listenForPromotions(); + final client = _FakePlexClient(thresholdPercent: 90); + final player = _FakePlayer(position: const Duration(seconds: 10), duration: const Duration(seconds: 100)); + final tracker = PlaybackProgressTracker(client: client, metadata: _meta(), player: player, isOffline: false); + addTearDown(tracker.dispose); + + // Below then above, both delivered: the backend saw the crossing itself. + // The playing report is dispatched fire-and-forget, so let it land before + // the stop that completes the crossing. + await tracker.sendProgress('playing'); + await Future.delayed(const Duration(milliseconds: 50)); + player.position = const Duration(seconds: 95); + await tracker.sendProgress('stopped'); + await Future.delayed(Duration.zero); + + expect(promotions, hasLength(1)); + expect(client.markWatchedCalls, isEmpty); + }); + + test('the explicit-mark path promotes exactly once', () async { + final promotions = listenForPromotions(); + final client = _FakePlexClient(thresholdPercent: 90); + final player = _FakePlayer(position: const Duration(seconds: 95), duration: const Duration(seconds: 100)); + final tracker = PlaybackProgressTracker(client: client, metadata: _meta(), player: player, isOffline: false); + addTearDown(tracker.dispose); + + // No observable crossing, so this session takes the explicit mark. + await tracker.sendProgress('stopped'); + await Future.delayed(Duration.zero); + + expect(client.markWatchedCalls, ['42']); + expect(promotions, hasLength(1)); + }); + + test('a failed explicit mark leaves the patch owed', () async { + final promotions = listenForPromotions(); + final watched = listenForWatchedEvents(); + final client = _ScrobblePreciseClient(thresholdPercent: 90, failScrobbleFirstTime: true); + final player = _FakePlayer(position: const Duration(seconds: 95), duration: const Duration(seconds: 100)); + final tracker = PlaybackProgressTracker(client: client, metadata: _meta(), player: player, isOffline: false); + addTearDown(tracker.dispose); + + await tracker.sendProgress('stopped'); + await Future.delayed(Duration.zero); + + // The crossing latched locally, so there is a patch to leave owed. + expect(watched, hasLength(1)); + // The write never landed, so nothing may retire the local patch. + expect(promotions, isEmpty); + }); + + test('a re-armed session does not promote off the previous stop', () async { + final promotions = listenForPromotions(); + final client = _StopMarksWatchedClient(); + final player = _FakePlayer(position: const Duration(seconds: 10), duration: const Duration(seconds: 100)); + final tracker = PlaybackProgressTracker( + client: client, + metadata: testMediaItem( + id: '42', + backend: MediaBackend.jellyfin, + kind: MediaKind.movie, + serverId: ServerId('srv'), + ), + player: player, + isOffline: false, + ); + addTearDown(tracker.dispose); + + await tracker.sendProgress('playing'); + player.position = const Duration(seconds: 95); + await tracker.sendProgress('stopped'); + await Future.delayed(Duration.zero); + expect(promotions, hasLength(1)); + + // A new server-side session must earn its own delivered stop. + tracker.resumeAfterStoppedReport(); + await Future.delayed(Duration.zero); + expect(promotions, hasLength(1)); + }); + }); } /// A more precise fake than [_FakePlexClient]: lets the test independently