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 <noreply@anthropic.com>
2026-07-06 15:30:18 +02:00
parent 28bc4a5df8
commit 32c4810c21
2 changed files with 171 additions and 12 deletions
@@ -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<bool> _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;

@@ -87,6 +87,9 @@ class FakePlayer implements Player {
  final List<String> openedUris = [];
  final List<Media?> setNextCalls = [];
  final List<Duration> seeks = [];
+
+  /// Arming these URIs throws, simulating a native setNext failure.
+  final Set<String> failingSetNextUris = {};
  int playCalls = 0;
  int pauseCalls = 0;
  int stopCalls = 0;
@@ -202,6 +205,9 @@ class FakePlayer implements Player {
  @override
  Future<void> 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<String> failingIds = {};
  final Map<String, int> resolveCounts = {};

+  /// Per-track URL overrides (e.g. content:// shapes for offline tracks).
+  final Map<String, String> urlOverrides = {};
+
  @override
  Future<MusicSource> 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<FakePlayer> players;

+  /// Seeded into every created FakePlayer — lets a test configure arm
+  /// failures before the first player exists.
+  final Set<String> 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 = <FakePlayer>[];
+    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<void> playTracks(List<MediaItem> 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));