From 32c4810c21e5047038cc1bf046a76b0bcb56f41e Mon Sep 17 00:00:00 2001 From: edde746 <86283021+edde746@users.noreply.github.com> Date: Mon, 6 Jul 2026 15:30:18 +0200 Subject: [PATCH] fix(music): resync gapless state when queue edits race the track boundary A queue edit can un-arm the next entry in the same instant mpv rolls into it; the resulting transition was dropped as unexpected, leaving the UI and progress reporting on the finished track for the entire next file. Remember the cleared arm (generation-gated) so the transition is still adopted, and handle the armed track no longer being in the queue: advance to the queue's real next, or park when nothing follows. Also fixes the latent fallthrough that left the cursor on the finished track when the armed track vanished from the queue. Co-Authored-By: Claude Fable 5 --- .../music/music_playback_service_impl.dart | 64 ++++++++-- .../music/music_playback_service_test.dart | 119 +++++++++++++++++- 2 files changed, 171 insertions(+), 12 deletions(-) diff --git a/lib/services/music/music_playback_service_impl.dart b/lib/services/music/music_playback_service_impl.dart index 7d8dacc2..fb1573ce 100644 --- a/lib/services/music/music_playback_service_impl.dart +++ b/lib/services/music/music_playback_service_impl.dart @@ -109,6 +109,13 @@ class MusicPlaybackServiceImpl extends MusicPlaybackService with WidgetsBindingO MusicPlayContext? _playContext; PlaybackProgressTracker? _tracker; _ArmedTrack? _armed; + + /// The arm most recently cleared for a re-arm (generation-gated): a queue + /// edit can un-arm an entry in the same instant mpv rolls into it, so the + /// resulting transition must stay adoptable — dropping it leaves the + /// service tracking the finished track for the entire next file. + _ArmedTrack? _staleArm; + int _staleArmGeneration = -1; Timer? _completedConfirmTimer; /// Bumped on every open/advance/stop so stale async continuations @@ -255,6 +262,7 @@ class MusicPlaybackServiceImpl extends MusicPlaybackService with WidgetsBindingO _currentTrack = track; _currentSource = null; _armed = null; + _staleArm = null; _setStatus(MusicPlaybackStatus.loading, forceNotify: true); await _coordinator.claimMusic(); @@ -331,13 +339,13 @@ class MusicPlaybackServiceImpl extends MusicPlaybackService with WidgetsBindingO if (target == null) { if (_armed == null) return; appLogger.d('Music: clearing arm (queue end / end-of-track sleep)'); - _armed = null; + _rememberStaleArm(); await _trySetNext(player, null); return; } if (_armed?.track.globalKey == target.globalKey) return; - _armed = null; + _rememberStaleArm(); await _trySetNext(player, null); if (generation != _generation || _player != player) return; @@ -367,6 +375,16 @@ class MusicPlaybackServiceImpl extends MusicPlaybackService with WidgetsBindingO } } + /// Un-arm bookkeeping: [_armed] is cleared but remembered so + /// [_onTrackTransition] can adopt a transition that raced the clear. + void _rememberStaleArm() { + if (_armed != null) { + _staleArm = _armed; + _staleArmGeneration = _generation; + } + _armed = null; + } + Future _trySetNext(Player player, Media? media) async { try { await player.setNext(media); @@ -446,11 +464,22 @@ class MusicPlaybackServiceImpl extends MusicPlaybackService with WidgetsBindingO /// The backend auto-advanced into the pre-armed item: authoritative /// track change. void _onTrackTransition(String uri) { - final armed = _armed; + var armed = _armed; + if ((armed == null || armed.source.url != uri) && + _staleArm != null && + _staleArmGeneration == _generation && + _staleArm!.source.url == uri) { + // mpv rolled into the entry in the same instant a queue edit un-armed + // it — the transition is still authoritative for what is audibly + // playing. + armed = _staleArm; + } + _staleArm = null; if (armed == null || armed.source.url != uri) { appLogger.w('Unexpected track transition to $uri (armed: ${armed?.source.url})'); return; } + final adopted = armed; _armed = null; final generation = ++_generation; @@ -461,19 +490,33 @@ class MusicPlaybackServiceImpl extends MusicPlaybackService with WidgetsBindingO // Move the cursor to the armed entry: the expected natural-next when it // still matches, otherwise wherever the armed track now sits. final expected = _queue.nextIndex(); - if (expected != null && _queue.trackAt(expected)?.globalKey == armed.track.globalKey) { + if (expected != null && _queue.trackAt(expected)?.globalKey == adopted.track.globalKey) { _queue.jumpTo(expected); } else { - final index = _queue.queue.indexWhere((t) => t.globalKey == armed.track.globalKey); - if (index >= 0) _queue.jumpTo(index); + final index = _queue.queue.indexWhere((t) => t.globalKey == adopted.track.globalKey); + if (index < 0) { + // The track mpv advanced into was removed from the queue at the + // boundary: don't adopt it — play what the queue says comes next, + // or park when nothing does. + appLogger.d('Music: transition into removed track "${adopted.track.title}" — advancing past it'); + final nextCursor = _queue.nextIndex(); + if (nextCursor != null) { + unawaited(_advanceTo(nextCursor)); + } else { + unawaited(_player?.pause()); + _parkAtEnd(); + } + return; + } + _queue.jumpTo(index); } - _currentTrack = _queue.current ?? armed.track; - _currentSource = armed.source; + _currentTrack = _queue.current ?? adopted.track; + _currentSource = adopted.source; _consecutiveFailures = 0; - appLogger.d('Music: transition received "${armed.track.title}" → cursor ${_queue.cursor}'); + appLogger.d('Music: transition received "${adopted.track.title}" → cursor ${_queue.cursor}'); _setStatus(MusicPlaybackStatus.playing, forceNotify: true); - _bindTrackServices(_currentTrack!, armed.source); + _bindTrackServices(_currentTrack!, adopted.source); unawaited(_armNext(generation)); } @@ -938,6 +981,7 @@ class MusicPlaybackServiceImpl extends MusicPlaybackService with WidgetsBindingO _currentTrack = null; _currentSource = null; _armed = null; + _staleArm = null; _playContext = null; _resumeAfterInterruption = false; diff --git a/test/services/music/music_playback_service_test.dart b/test/services/music/music_playback_service_test.dart index 7b3250f7..0dae1ce0 100644 --- a/test/services/music/music_playback_service_test.dart +++ b/test/services/music/music_playback_service_test.dart @@ -87,6 +87,9 @@ class FakePlayer implements Player { final List openedUris = []; final List setNextCalls = []; final List seeks = []; + + /// Arming these URIs throws, simulating a native setNext failure. + final Set failingSetNextUris = {}; int playCalls = 0; int pauseCalls = 0; int stopCalls = 0; @@ -202,6 +205,9 @@ class FakePlayer implements Player { @override Future setNext(Media? media) async { setNextCalls.add(media); + if (media != null && failingSetNextUris.contains(media.uri)) { + throw StateError('setNext failed for ${media.uri}'); + } _armedMedia = media; } @@ -409,6 +415,9 @@ class FakeMusicSourceResolver implements MusicSourceResolver { final Set failingIds = {}; final Map resolveCounts = {}; + /// Per-track URL overrides (e.g. content:// shapes for offline tracks). + final Map urlOverrides = {}; + @override Future resolve(MediaItem track) async { resolveCounts[track.id] = (resolveCounts[track.id] ?? 0) + 1; @@ -416,7 +425,7 @@ class FakeMusicSourceResolver implements MusicSourceResolver { throw StateError('resolve failed for ${track.id}'); } return MusicSource( - url: _urlFor(track), + url: urlOverrides[track.id] ?? _urlFor(track), playSessionId: 'ps-${track.id}', playMethod: 'DirectPlay', reportingClient: client, @@ -480,6 +489,10 @@ class _Harness { final FakeMediaControlsManager controls; final List players; + /// Seeded into every created FakePlayer — lets a test configure arm + /// failures before the first player exists. + final Set failingSetNextUris = {}; + FakePlayer get player => players.last; factory _Harness.create() { @@ -487,11 +500,13 @@ class _Harness { final resolver = FakeMusicSourceResolver(client: client); final controls = FakeMediaControlsManager(); final players = []; + late final _Harness harness; final service = MusicPlaybackServiceImpl( serverManager: MultiServerManager(), resolver: resolver, audioPlayerFactory: () { final player = FakePlayer(); + player.failingSetNextUris.addAll(harness.failingSetNextUris); players.add(player); return player; }, @@ -500,7 +515,8 @@ class _Harness { // paths resolve within pumpEventQueue. completedConfirmDelay: Duration.zero, ); - return _Harness._(service, resolver, client, controls, players); + harness = _Harness._(service, resolver, client, controls, players); + return harness; } Future playTracks(List tracks, {MediaItem? startTrack, bool shuffle = false}) async { @@ -814,6 +830,105 @@ void main() { expect(h.client.reportsFor('stopped').map((r) => r.itemId), ['t1']); }); + group('gapless boundary races', () { + // The sync stream controllers make the race drivable deterministically: + // a transition emitted between the queue edit (which un-arms the entry + // synchronously) and the event pump lands exactly like mpv rolling into + // the armed entry as it is being cleared. + + test('transition raced by a reorder is adopted via the stale-arm memo', () async { + await h.playTracks([t1, t2, t3]); + expect(h.player.armed?.uri, _urlFor(t2)); + + h.service.reorder(1, 2); // queue [t1, t3, t2] — un-arms t2 + h.player.emitTransition(_urlFor(t2)); // ...but mpv already rolled into it + await pumpEventQueue(); + + expect(h.service.currentTrack?.id, 't2'); + expect(h.service.currentIndex, 2); + expect(h.service.status, MusicPlaybackStatus.playing); + expect(h.player.openedUris, [_urlFor(t1)], reason: 'adopted gaplessly, no re-open'); + final stopped = h.client.reportsFor('stopped').toList(); + expect(stopped.single.itemId, 't1'); + expect(stopped.single.position, _trackDuration); + expect(h.client.reportsFor('started').map((r) => r.itemId), ['t1', 't2']); + }); + + test('transition into a track removed at the boundary advances to the real next', () async { + final t4 = _track('t4'); + await h.playTracks([t1, t2, t3, t4]); + + h.service.removeAt(1); // queue [t1, t3, t4] — un-arms t2 + h.player.emitTransition(_urlFor(t2)); // mpv rolled into the removed track + await pumpEventQueue(); + + expect(h.service.currentTrack?.id, 't3'); + expect(h.player.openedUris, [_urlFor(t1), _urlFor(t3)]); + expect(h.player.armed?.uri, _urlFor(t4)); + expect(h.client.reportsFor('stopped').single.itemId, 't1'); + + // A stale boundary completed pulse must not double-advance past t3. + h.player.emitCompleted(); + await pumpEventQueue(); + expect(h.service.currentTrack?.id, 't3'); + expect(h.player.openedUris, [_urlFor(t1), _urlFor(t3)]); + }); + + test('removed-at-boundary with no next parks paused', () async { + await h.playTracks([t1, t2]); + + h.service.removeAt(1); // queue [t1] — un-arms t2 + h.player.emitTransition(_urlFor(t2)); + await pumpEventQueue(); + + expect(h.service.status, MusicPlaybackStatus.paused); + expect(h.service.currentTrack?.id, 't1'); + expect(h.player.pauseCalls, 1, reason: 'the removed track is audibly playing — silence it'); + expect(h.player.openedUris, [_urlFor(t1)]); + }); + + test('stale transition after a manual advance is dropped', () async { + await h.playTracks([t1, t2, t3]); + + h.service.removeAt(1); // memo t2 + await h.service.next(); // manual advance clears the memo + await pumpEventQueue(); + expect(h.service.currentTrack?.id, 't3'); + + h.player.emitTransition(_urlFor(t2)); + await pumpEventQueue(); + + expect(h.service.currentTrack?.id, 't3'); + expect(h.service.currentIndex, 1); + expect(h.player.openedUris, [_urlFor(t1), _urlFor(t3)]); + }); + + test('failed native arm falls back to an explicit open at completion', () async { + h.failingSetNextUris.add(_urlFor(t2)); + await h.playTracks([t1, t2]); + expect(h.player.armed, isNull); + + h.player.emitCompleted(); + await pumpEventQueue(); + + expect(h.player.openedUris, [_urlFor(t1), _urlFor(t2)]); + expect(h.service.currentTrack?.id, 't2'); + expect(h.service.status, MusicPlaybackStatus.playing); + }); + + test('transition matching is URL-shape agnostic (offline content://)', () async { + h.resolver.urlOverrides['t2'] = 'content://downloads/t2'; + await h.playTracks([t1, t2]); + expect(h.player.armed?.uri, 'content://downloads/t2'); + + h.player.emitTransition('content://downloads/t2'); + await pumpEventQueue(); + + expect(h.service.currentTrack?.id, 't2'); + expect(h.service.status, MusicPlaybackStatus.playing); + }); + }); + test('end-of-track sleep timer suppresses arming and pauses at completion', () async { await h.playTracks([t1, t2]); expect(h.player.armed?.uri, _urlFor(t2));