fix(player): settle the watched patch the backend recorded itself

Watching an episode to the end left it stuck as watched for the rest of the
session. Unmarking it on another device and refreshing did nothing; only a
restart cleared it. Unlike #1829 this needs no second device to cause -- a
normal watch-through is enough, and the second device only makes it visible.

A threshold crossing writes an unacknowledged overlay patch, deliberately:
reporting success proves the backend received the report, not that it
classified the item as played, so the patch stays owed until something
settles it. _settleServerMark has three settled outcomes and only one of them
did. The explicit-mark branch promoted; the two branches that skip the mark
because the backend already recorded the watch itself -- Jellyfin from
/Sessions/Playing/Stopped, Plex from a timeline crossing past
LibraryVideoPlayedThreshold -- returned without promoting. Those are the
common paths, so nearly every completed playback stranded a patch that the
store then refused to suppress, because an unacknowledged entry is never
retired by an authoritative read.

Both now promote, through one idempotent helper that clears the id so the
delivery callback and the settle paths cannot promote twice.

Promotion has to follow delivery rather than the settle decision. A
marks-on-stop backend settles when the crossing latches, which happens before
the stop is sent, and until that stop lands the watch really is still owed --
promoting there would let a refresh retire a patch the server had never
heard about. MediaBrowser also drops a stop for a session it never opened, in
which case the watch it would have recorded never happens at all. So the stop
path promotes only once the report reached a session able to act on it, which
is the same condition that already governs whether the stop persists its
position; that condition is now named rather than recomputed, and reset with
its siblings when a session re-arms. The crossing branch needs no such gate:
it is assembled from two delivered reports, so delivery is already proven.

Verified against a live Jellyfin server driving the real client and tracker:
before, the server reported the item unwatched after a second device cleared
it while the overlay still rendered watched; after, the overlay follows the
server. The optimistic mark still appears immediately during playback -- it
now yields to a later authoritative read instead of outliving one.

The #1287 and #1740 contracts are unchanged: neither branch issues an
explicit mark, and the tests assert that alongside the promotion.
This commit is contained in:
edde746
2026-08-08 10:02:04 +02:00
parent 5f397a99d9
commit 636fd48f40
2 changed files with 212 additions and 5 deletions
+40 -5
View File
@@ -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.
///
@@ -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<WatchPatchId> listenForPromotions() {
final seen = <WatchPatchId>[];
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<WatchStateEvent> listenForWatchedEvents() {
final seen = <WatchStateEvent>[];
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<void>.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<void>.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<void>.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<void>.delayed(const Duration(milliseconds: 50));
player.position = const Duration(seconds: 95);
await tracker.sendProgress('stopped');
await Future<void>.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<void>.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<void>.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<void>.delayed(Duration.zero);
expect(promotions, hasLength(1));
// A new server-side session must earn its own delivered stop.
tracker.resumeAfterStoppedReport();
await Future<void>.delayed(Duration.zero);
expect(promotions, hasLength(1));
});
});
}
/// A more precise fake than [_FakePlexClient]: lets the test independently