From 2cb117dac82119eab49defa7bf521b37fa0cc30f Mon Sep 17 00:00:00 2001 From: 9tong Date: Sat, 8 Aug 2026 08:10:07 +0800 Subject: [PATCH 1/5] fix(player): recover media keys after an audio route loss MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Media keys stopped controlling playback after Bluetooth headphones disconnected and reconnected, and only clicking Play in the window restored them. A timestamped trace showed three defects stacking. WebKit registers its own Now Playing client, separate from the app's MPNowPlayingInfoCenter. Losing the audio route leaves that registration behind: its Control Center card still captures the media keys but its transport actions do nothing. The registration cannot be withdrawn from the app side — clearing navigator.mediaSession only blanks the card — so the only thing that rebinds it is playback running again, which is exactly what the manual workaround did. macOS stops playback on a vanished route by sending an ordinary pause remote command, indistinguishable from the user pressing Pause. Kaset recorded it as a deliberate pause, and isExplicitPauseIntentActive then made the observer re-pause the page the instant anything resumed it, so even a media key reaching a healthy session started playback and had it killed a moment later. The native Now Playing claim's hands-off branch waited for WebKit to replace the app-wide metadata before standing down. WebKit never does that, so the claim was never released and Kaset kept a second, stale Control Center entry once it had paused even once. Resume playback when the route returns, scoped to playback the route loss itself stopped. MusicPauseOrigin separates a system pause from a user one; only a user pause records a standing intent to stay paused. DefaultOutputDeviceMonitor identifies a route loss by the previous default device becoming unusable — checking DeviceIsAlive as well as list membership, since Core Audio can mark a device dead before dropping its ID — so a manual output switch never arms recovery. Remote commands drain onto the MainActor asynchronously, so both the classification window and the recovery marker anchor on the ingress admission instant, and the route timeline is reconstructed as of that instant rather than as of handling time. Recovery is bounded by intent rather than a timer: the marker is retired by a user pause or by playback actually starting, and survives issuing a resume so the retry can act while the route is still settling. The YouTube video source keeps the same gap and is documented as out of scope in ADR-0033; it needs its own recovery path and could not be verified against this reproduction. Co-Authored-By: Claude Opus 5 --- .../Audio/DefaultOutputDeviceMonitor.swift | 319 ++++++++++++++++++ .../Services/Player/MusicPlaybackIntent.swift | 18 + .../Services/Player/NowPlayingManager.swift | 152 +++++++-- .../PlayerService+PlaybackControls.swift | 53 ++- .../Kaset/Services/Player/PlayerService.swift | 14 + Tests/KasetTests/NowPlayingClaimTests.swift | 103 +++++- .../PlayerServiceRouteChangePauseTests.swift | 140 ++++++++ ...outeDisappearanceClassificationTests.swift | 143 ++++++++ docs/adr/0033-audio-route-loss-recovery.md | 113 +++++++ docs/adr/README.md | 1 + 10 files changed, 1030 insertions(+), 26 deletions(-) create mode 100644 Sources/Kaset/Services/Audio/DefaultOutputDeviceMonitor.swift create mode 100644 Tests/KasetTests/PlayerServiceRouteChangePauseTests.swift create mode 100644 Tests/KasetTests/RouteDisappearanceClassificationTests.swift create mode 100644 docs/adr/0033-audio-route-loss-recovery.md diff --git a/Sources/Kaset/Services/Audio/DefaultOutputDeviceMonitor.swift b/Sources/Kaset/Services/Audio/DefaultOutputDeviceMonitor.swift new file mode 100644 index 000000000..be68716e5 --- /dev/null +++ b/Sources/Kaset/Services/Audio/DefaultOutputDeviceMonitor.swift @@ -0,0 +1,319 @@ +import CoreAudio +import Foundation + +// MARK: - RouteChangeEvent + +/// One observed change of the system output device. +struct RouteChangeEvent: Equatable { + let at: ContinuousClock.Instant + /// True when the device that had been the default went away, as opposed to the user + /// switching outputs or a new device arriving. + let isDisappearance: Bool + /// Set once this disappearance has explained a pause, so it cannot explain another. + var isConsumed = false +} + +// MARK: - RouteChangeRecord + +/// Thread-safe log of recent system output-device changes. +/// +/// Core Audio delivers notifications on its own thread while these are read on the MainActor, +/// and consumers classify events by how closely they followed a route change — tens of +/// milliseconds. Storing this in actor-isolated state would date a change to whenever a hop +/// happened to be scheduled instead of to when the route actually changed. +/// +/// A log rather than a latest-pair: classification runs after an unbounded hop, so further +/// route events can land before a queued command is judged, and keeping only the newest pair +/// would discard the very event that explains it. +private final class RouteChangeRecord: @unchecked Sendable { + private let lock = NSLock() + private var events: [RouteChangeEvent] = [] + private var defaultDeviceID: AudioDeviceID? + /// Far longer than any classification window, and still a hard bound on growth. + private static let retention = Duration.seconds(30) + private static let capacity = 32 + + func prime(defaultDeviceID: AudioDeviceID?) { + self.lock.withLock { self.defaultDeviceID = defaultDeviceID } + } + + func currentDefaultDeviceID() -> AudioDeviceID? { + self.lock.withLock { self.defaultDeviceID } + } + + /// Appends a change, marking it a disappearance when the device that had been the default is + /// gone — the difference between headphones being unplugged and the user picking a different + /// output. `previousDeviceIsUsable` is evaluated by the caller against the device it is + /// replacing, since only the caller can reach Core Audio. + func record( + at instant: ContinuousClock.Instant, + defaultDeviceID: AudioDeviceID?, + previousDeviceIsUsable: Bool + ) { + self.lock.withLock { + // A `nil` dispatch queue means Core Audio invokes the listener directly on the + // notifying thread, so callbacks are not serialized: each timestamps itself and then + // queries Core Audio outside this lock, and a slow one can arrive after a newer one. + // Drop anything already superseded rather than letting a stale reading overwrite + // fresh state or leave the log out of order. + if let newest = self.events.last?.at, instant <= newest { + return + } + + let isDisappearance = self.defaultDeviceID != nil && !previousDeviceIsUsable + self.events.append(RouteChangeEvent(at: instant, isDisappearance: isDisappearance)) + self.events.removeAll { instant - $0.at > Self.retention } + if self.events.count > Self.capacity { + self.events.removeFirst(self.events.count - Self.capacity) + } + self.defaultDeviceID = defaultDeviceID + } + } + + func routeRestored(since instant: ContinuousClock.Instant) -> Bool { + self.lock.withLock { + DefaultOutputDeviceMonitor.routeRestored(in: self.events, since: instant) + } + } + + /// Claims the disappearance that explains a pause admitted at `instant`, if there is one. + /// + /// Claiming is what keeps a single disconnect from excusing every pause that follows it: + /// a user pressing Pause moments after the system already did must keep its own meaning. + func claimRouteLoss(admittedAt instant: ContinuousClock.Instant, within window: Duration) -> Bool { + self.lock.withLock { + guard let index = DefaultOutputDeviceMonitor.routeLossIndex( + in: self.events, + admittedAt: instant, + within: window + ) else { return false } + self.events[index].isConsumed = true + return true + } + } +} + +// MARK: - DefaultOutputDeviceMonitor + +/// Reports system default-output-device changes: Bluetooth connect/disconnect, +/// headphone plug/unplug, and manual output switches. +/// +/// `EqualizerService` installs its own listener, but that one is gated behind the +/// equalizer being enabled. Playback-state correctness must not depend on a +/// user-facing audio feature being switched on, so this monitor stays independent. +/// +/// Known limitation: this watches only which device is the default output. A route change +/// *within* one device — some Macs expose speakers and the headphone jack as data sources on +/// the same built-in device — leaves that selection untouched and goes unseen. Covering it +/// means also listening on the current device's output data source and retargeting those +/// listeners on every default-device change. Bluetooth, the case this was built for, always +/// swaps the device itself; consumers degrade to their pre-existing behavior when a change is +/// missed rather than misbehaving. +@MainActor +final class DefaultOutputDeviceMonitor { + static let shared = DefaultOutputDeviceMonitor() + + private var handler: (() -> Void)? + private var isListening = false + private let logger = DiagnosticsLogger.player + // swiftformat:disable:next modifierOrder + nonisolated private static let record = RouteChangeRecord() + + private init() {} + + /// Claims the disappearance explaining a pause admitted at `instant`, if there is one. + /// + /// macOS stops playback when a route vanishes by sending the app a `pause` remote command, + /// which is indistinguishable from the user pressing Pause. Pairing it with a device that + /// just went away is what tells the two apart — and why a device merely being added, or the + /// user switching outputs by hand, deliberately does not qualify. + /// + /// Each disappearance can explain at most one pause. Otherwise a user pressing Pause shortly + /// after the system already paused for the same disconnect would also read as system-driven, + /// losing their explicit intent and letting a later reconnect resume against it. + /// + /// `instant` must be when the command was admitted, not when it is being handled: commands + /// are drained onto the MainActor asynchronously, so measuring against the handling time + /// would let a delayed main actor age a genuine route pause out of the window, or backdate + /// a user pause into one. + nonisolated func claimRouteLossPause(admittedAt instant: ContinuousClock.Instant, within window: Duration) -> Bool { + Self.record.claimRouteLoss(admittedAt: instant, within: window) + } + + /// Pure decision behind ``claimRouteLossPause(admittedAt:within:)``: which logged + /// disappearance, if any, explains a pause admitted at `instant`. + /// + /// Reconstructs the route timeline *as of* `instant` rather than as of now. Classification + /// runs after an unbounded hop, so route events can land in between; judging by the latest + /// state would let a reconnect arriving after the pause retroactively turn a genuine + /// route-loss pause into a user pause and disable recovery. + nonisolated static func routeLossIndex( + in events: [RouteChangeEvent], + admittedAt instant: ContinuousClock.Instant, + within window: Duration + ) -> Int? { + guard let index = events.lastIndex(where: { event in + event.isDisappearance + && !event.isConsumed + && event.at <= instant + && instant - event.at <= window + }) else { return nil } + + // Output coming back between that disappearance and the command means there was a route + // to play to when the command was admitted, so the pause is the user's. A restoration + // after the command cannot explain a pause that had already been admitted. + let disappearance = events[index] + let restoredBeforeCommand = events.contains { event in + !event.isDisappearance && event.at > disappearance.at && event.at <= instant + } + return restoredBeforeCommand ? nil : index + } + + /// Whether output came back after `instant` and is still available. + /// + /// Not merely "something changed": a disconnect fires this monitor too, and under flapping + /// the newest event after a route-loss pause can be another disappearance. Resuming then + /// would start playback with no usable output — and because playback is already paused, that + /// second loss may produce no pause command to re-arm the marker. + nonisolated func routeRestored(since instant: ContinuousClock.Instant) -> Bool { + Self.record.routeRestored(since: instant) + } + + /// Pure decision behind ``routeRestored(since:)``. + nonisolated static func routeRestored( + in events: [RouteChangeEvent], + since instant: ContinuousClock.Instant + ) -> Bool { + guard let latest = events.last(where: { $0.at > instant }) else { return false } + return !latest.isDisappearance + } + + /// Installs `handler` and starts listening on first use. Later calls replace the handler. + func start(onChange handler: @escaping () -> Void) { + self.handler = handler + guard !self.isListening else { return } + + Self.record.prime(defaultDeviceID: Self.currentDefaultOutputDeviceID()) + + var address = Self.defaultOutputDeviceAddress + let status = AudioObjectAddPropertyListenerBlock( + AudioObjectID(kAudioObjectSystemObject), + &address, + nil, + Self.listener + ) + guard status == noErr else { + self.logger.warning("failed to listen for default-output changes: \(status)") + return + } + self.isListening = true + } + + private func notifyChange() { + self.handler?() + } + + // Core Audio invokes this on its own callback queue. It is a `nonisolated static` + // constant so it does not inherit MainActor isolation from the enclosing class — + // otherwise Swift 6's runtime isolation check trips with `dispatch_assert_queue_fail` + // the first time the block fires off-main. The hop to MainActor happens in the Task. + // swiftformat:disable:next modifierOrder + nonisolated private static let listener: + @Sendable (UInt32, UnsafePointer) -> Void = { _, _ in + // Timestamp on entry, before the Core Audio queries below and before the hop. Those + // queries take real time, and the `pause` this change provokes is admitted on + // another callback — a timestamp taken afterwards can sort after that command and + // read as though the route were still fine when it arrived. + let changedAt = ContinuousClock.now + let previousDeviceIsUsable = DefaultOutputDeviceMonitor.record.currentDefaultDeviceID() + .map(DefaultOutputDeviceMonitor.isDeviceUsable) ?? true + DefaultOutputDeviceMonitor.record.record( + at: changedAt, + defaultDeviceID: DefaultOutputDeviceMonitor.currentDefaultOutputDeviceID(), + previousDeviceIsUsable: previousDeviceIsUsable + ) + Task { @MainActor in + DefaultOutputDeviceMonitor.shared.notifyChange() + } + } + + // swiftformat:disable:next modifierOrder + nonisolated private static var defaultOutputDeviceAddress: AudioObjectPropertyAddress { + AudioObjectPropertyAddress( + mSelector: kAudioHardwarePropertyDefaultOutputDevice, + mScope: kAudioObjectPropertyScopeGlobal, + mElement: kAudioObjectPropertyElementMain + ) + } + + // swiftformat:disable:next modifierOrder + nonisolated private static func currentDefaultOutputDeviceID() -> AudioDeviceID? { + var address = Self.defaultOutputDeviceAddress + var deviceID = AudioDeviceID(0) + var size = UInt32(MemoryLayout.size) + let status = AudioObjectGetPropertyData( + AudioObjectID(kAudioObjectSystemObject), + &address, + 0, + nil, + &size, + &deviceID + ) + guard status == noErr, deviceID != AudioDeviceID(kAudioObjectUnknown) else { return nil } + return deviceID + } + + // Whether `deviceID` is still a device the system could play through. + // + // Core Audio can mark a device dead before dropping its ID from the device list, so being + // listed is not enough: an unplugged Bluetooth output often lingers as an ID whose + // `DeviceIsAlive` has already gone to zero, and treating that as present would classify + // the pause it triggers as the user's. + // swiftformat:disable:next modifierOrder + nonisolated private static func isDeviceUsable(_ deviceID: AudioDeviceID) -> Bool { + guard self.availableDeviceIDs().contains(deviceID) else { return false } + + var address = AudioObjectPropertyAddress( + mSelector: kAudioDevicePropertyDeviceIsAlive, + mScope: kAudioObjectPropertyScopeGlobal, + mElement: kAudioObjectPropertyElementMain + ) + var isAlive = UInt32(0) + var size = UInt32(MemoryLayout.size) + let status = AudioObjectGetPropertyData(deviceID, &address, 0, nil, &size, &isAlive) + // A device that cannot even answer the query is not one we can play through. + guard status == noErr else { return false } + return isAlive != 0 + } + + // swiftformat:disable:next modifierOrder + nonisolated private static func availableDeviceIDs() -> Set { + var address = AudioObjectPropertyAddress( + mSelector: kAudioHardwarePropertyDevices, + mScope: kAudioObjectPropertyScopeGlobal, + mElement: kAudioObjectPropertyElementMain + ) + var size = UInt32(0) + guard AudioObjectGetPropertyDataSize( + AudioObjectID(kAudioObjectSystemObject), + &address, + 0, + nil, + &size + ) == noErr, size > 0 else { return [] } + + var deviceIDs = [AudioDeviceID]( + repeating: 0, + count: Int(size) / MemoryLayout.size + ) + guard AudioObjectGetPropertyData( + AudioObjectID(kAudioObjectSystemObject), + &address, + 0, + nil, + &size, + &deviceIDs + ) == noErr else { return [] } + return Set(deviceIDs) + } +} diff --git a/Sources/Kaset/Services/Player/MusicPlaybackIntent.swift b/Sources/Kaset/Services/Player/MusicPlaybackIntent.swift index aced272c4..fe73250a5 100644 --- a/Sources/Kaset/Services/Player/MusicPlaybackIntent.swift +++ b/Sources/Kaset/Services/Player/MusicPlaybackIntent.swift @@ -11,11 +11,26 @@ struct MusicPlaybackIntent: Equatable { let generation: UInt64 } +// MARK: - MusicPauseOrigin + +/// Why playback is being paused. The two are indistinguishable at the remote-command layer — +/// macOS asks an app to pause a vanished route exactly as it asks for a user's Pause — so the +/// distinction has to be carried explicitly. +enum MusicPauseOrigin: Equatable { + case user + /// The system took the audio route away, as of when the command was admitted. + case routeLoss(at: ContinuousClock.Instant) +} + // MARK: - MusicRemoteTransportCommand enum MusicRemoteTransportCommand: Equatable { case play case pause + /// A `pause` the system imposed by taking the audio route away, not a user request. + /// Carries the ingress admission instant so the recovery marker is dated to when the + /// command arrived rather than to whenever the MainActor got around to draining it. + case pauseForRouteChange(admittedAt: ContinuousClock.Instant) case togglePlayPause case next case previous @@ -219,6 +234,9 @@ extension PlayerService { case .pause: self.clearRemoteMusicSkipCoalescingTarget() await self.pause(intent: intent) + case let .pauseForRouteChange(admittedAt): + self.clearRemoteMusicSkipCoalescingTarget() + await self.pause(intent: intent, origin: .routeLoss(at: admittedAt)) case .togglePlayPause: self.clearRemoteMusicSkipCoalescingTarget() await self.playPause(intent: intent) diff --git a/Sources/Kaset/Services/Player/NowPlayingManager.swift b/Sources/Kaset/Services/Player/NowPlayingManager.swift index 540ea312a..d60a29136 100644 --- a/Sources/Kaset/Services/Player/NowPlayingManager.swift +++ b/Sources/Kaset/Services/Player/NowPlayingManager.swift @@ -111,6 +111,20 @@ final class RemoteMusicCommandIngress: @unchecked Sendable { } } +// MARK: - NowPlayingInfoCenter + +/// The slice of `MPNowPlayingInfoCenter` the claim logic writes, so withdrawal can be tested +/// without a live system center. +@MainActor +protocol NowPlayingInfoCenter: AnyObject { + var nowPlayingInfo: [String: Any]? { get set } + var playbackState: MPNowPlayingPlaybackState { get set } +} + +// MARK: - MPNowPlayingInfoCenter + NowPlayingInfoCenter + +extension MPNowPlayingInfoCenter: NowPlayingInfoCenter {} + // MARK: - NowPlayingManager /// Manages remote-command routing and the app's Now Playing ownership. @@ -141,6 +155,13 @@ final class NowPlayingManager { private static let defaultSkipInterval: TimeInterval = 15 nonisolated static let nativeClaimServiceIdentifier = "com.sertacozercan.Kaset.native-now-playing-claim" + @ObservationIgnored private var routeRestoreTask: Task? + @ObservationIgnored private var routeRestoreGeneration: UInt64 = 0 + private static let routeRestoreDelays: [Duration] = [ + .milliseconds(500), + .milliseconds(2000), + ] + private init() {} // MARK: - Now Playing Claim @@ -151,8 +172,9 @@ final class NowPlayingManager { } /// What Kaset should tell the system Now Playing center for the current player state. - /// `handsOff` lets WebKit replace an existing fallback during active playback, while - /// `release` clears a native claim only when no resumable media remains. + /// Both non-claim cases withdraw Kaset's tagged entry — they differ only in why: + /// `handsOff` because WebKit's own Now Playing client owns the card during playback, + /// `release` because no resumable media remains. enum NowPlayingClaim: Equatable { case handsOff case release @@ -187,8 +209,19 @@ final class NowPlayingManager { } switch state { - case .playing, .buffering, .loading: + case .playing, .buffering: + // `buffering` is a stall inside playback that already started, so WebKit owns + // its card by now. Claiming here would recreate the competing second entry. + // Nothing in the music player assigns `buffering` today; if that changes, decide + // this case by whether playback was ever confirmed rather than by the state name, + // the way `activeVideo` already does with `isPlaybackConfirmed`. return .handsOff + case .loading: + // Playback is starting and WebKit has not published its card yet. Withdrawing + // here would leave the app with no Now Playing entry — and no media keys — for + // the whole load, so hold a playing-state claim until `.playing` hands over. + guard let track else { return .release } + return .claim(title: track.title, artist: track.artist, playbackState: .playing) case .idle, .paused, .ended, .error: guard let track else { return .release } return .claim(title: track.title, artist: track.artist, playbackState: .paused) @@ -209,25 +242,39 @@ final class NowPlayingManager { self.applyNowPlayingClaim(claim) } - /// Maps a claim onto `MPNowPlayingInfoCenter`. Hands-off only clears info we still own. + /// Maps a claim onto the Now Playing center, withdrawing our entry unless we want one. private func applyNowPlayingClaim(_ claim: NowPlayingClaim) { - let center = MPNowPlayingInfoCenter.default() + self.isAssertingNativeClaim = Self.applyClaim( + claim, + isAssertingNativeClaim: self.isAssertingNativeClaim, + to: MPNowPlayingInfoCenter.default() + ) + } + + /// Applies `claim` to `center`, returning whether Kaset asserts a native claim afterwards. + /// + /// Withdrawing is the interesting half. WebKit registers its own Now Playing client with the + /// system rather than writing `MPNowPlayingInfoCenter`, so a Kaset claim left in place during + /// playback does not get replaced — it survives as a second, competing entry for the same + /// app. Whichever entry was updated most recently owns the media keys, so a stale claim can + /// silently capture them and answer Play/Pause from a playback state that no longer matches + /// the page. Clearing here cannot disturb WebKit's card; they are separate clients. + /// + /// Only metadata carrying our tag is ever cleared, so a card published by anything else is + /// left alone. + @discardableResult + static func applyClaim( + _ claim: NowPlayingClaim, + isAssertingNativeClaim: Bool, + to center: any NowPlayingInfoCenter + ) -> Bool { switch claim { - case .handsOff: - guard self.isAssertingNativeClaim else { return } - guard Self.isNativeClaim(center.nowPlayingInfo) else { - self.isAssertingNativeClaim = false - return - } - // Preserve the fallback until WebKit atomically replaces the app-wide metadata. - // A non-destructive state update cannot clear a concurrently published WebKit card. - center.playbackState = .playing - case .release: - guard self.isAssertingNativeClaim else { return } - self.isAssertingNativeClaim = false - guard Self.isNativeClaim(center.nowPlayingInfo) else { return } + case .handsOff, .release: + guard isAssertingNativeClaim else { return false } + guard self.isNativeClaim(center.nowPlayingInfo) else { return false } center.playbackState = .stopped center.nowPlayingInfo = nil + return false case let .claim(title, artist, playbackState): var info: [String: Any] = [ MPMediaItemPropertyTitle: title, @@ -241,7 +288,7 @@ final class NowPlayingManager { case .playing: .playing case .paused: .paused } - self.isAssertingNativeClaim = true + return true } } @@ -303,11 +350,49 @@ final class NowPlayingManager { self.logger.info("NowPlayingManager configured") self.observeSettingsChanges() + self.observeOutputDeviceChanges() self.updateNowPlayingClaim() self.restartNowPlayingObservation() } + // MARK: - Transport Reconciliation + + private func observeOutputDeviceChanges() { + DefaultOutputDeviceMonitor.shared.start { [weak self] in + self?.resumeAfterRouteRestoredSoon() + } + } + + /// Resumes playback that a vanished audio route had stopped, once a route is back. + /// + /// The device is not immediately usable when the notification lands, and WebKit settles a + /// route change in stages, so try twice rather than racing it once. Bursts of notifications + /// collapse into the latest request. `resumeAfterRouteRestored` itself decides whether a + /// resume is warranted, so both a disconnect and a reconnect can drive this safely. + private func resumeAfterRouteRestoredSoon() { + self.routeRestoreGeneration &+= 1 + let generation = self.routeRestoreGeneration + self.routeRestoreTask?.cancel() + self.routeRestoreTask = Task { @MainActor [weak self] in + defer { + if self?.routeRestoreGeneration == generation { + self?.routeRestoreTask = nil + } + } + var elapsed = Duration.zero + for delay in Self.routeRestoreDelays { + try? await Task.sleep(for: delay - elapsed) + elapsed = delay + guard !Task.isCancelled, + let self, + self.routeRestoreGeneration == generation + else { return } + await self.playerService?.resumeAfterRouteRestored() + } + } + } + /// Registers the YouTube video player for media-key routing. /// Additive: without this call (or when video is inactive), all commands /// route to the music player exactly as before. @@ -486,7 +571,14 @@ final class NowPlayingManager { issuedAtMilliseconds: capturedCommand.issuedAtMilliseconds ) } else { - self.enqueueMusicRemoteCommand(.pause, capturedCommand: capturedCommand, player: player) + let command: MusicRemoteTransportCommand = Self.isRouteChangePause(capturedCommand) + ? .pauseForRouteChange(admittedAt: capturedCommand.admittedAt) + : .pause + self.enqueueMusicRemoteCommand( + command, + capturedCommand: capturedCommand, + player: player + ) } case .togglePlayPause: if self.routesToYouTubeVideo, let youtube = self.youtubePlayerService { @@ -538,6 +630,26 @@ final class NowPlayingManager { } } + /// Whether the `pause` we just received is the system reacting to a vanished audio route + /// (unplugged headphones, a Bluetooth device disconnecting) rather than a user request. + /// + /// macOS delivers both as the same remote command, so the only thing separating them is + /// that a route-driven one lands immediately after the output device it was playing to + /// disappears — measured at 40–60ms. The window absorbs scheduling jitter while staying far + /// below the time it takes a person to reach for the key, and requiring a *disappearance* + /// rather than any device change keeps a deliberate pause next to a manual output switch + /// from being mistaken for one. + /// + /// The comparison anchors on when the ingress admitted the command, not on when this drain + /// runs: the hop to the MainActor is unbounded, and dating the decision to handling time + /// would let a busy main actor push a real route pause out of the window. + private static func isRouteChangePause(_ capturedCommand: CapturedRemoteMusicCommand) -> Bool { + DefaultOutputDeviceMonitor.shared.claimRouteLossPause( + admittedAt: capturedCommand.admittedAt, + within: .milliseconds(1500) + ) + } + private func handleNextPreviousMediaKey( direction: RemoteMusicCommandDirection, capturedCommand: CapturedRemoteMusicCommand, diff --git a/Sources/Kaset/Services/Player/PlayerService+PlaybackControls.swift b/Sources/Kaset/Services/Player/PlayerService+PlaybackControls.swift index 0798e853e..028b15cea 100644 --- a/Sources/Kaset/Services/Player/PlayerService+PlaybackControls.swift +++ b/Sources/Kaset/Services/Player/PlayerService+PlaybackControls.swift @@ -368,6 +368,7 @@ extension PlayerService { guard shouldHideMiniPlayer || didStartPlayback || shouldRecordInteraction else { return } self.showMiniPlayer = false + self.routeLossPauseAt = nil self.state = .playing if shouldRecordInteraction { @@ -483,12 +484,29 @@ extension PlayerService { await self.pause(intent: intent) } - func pause(intent: MusicPlaybackIntent) async { + /// Pauses playback. + /// + /// `origin` separates a pause the system imposed from one the user asked for — losing an + /// audio route arrives as an ordinary `pause` remote command. A system pause must not record + /// a standing intent to stay paused: `isExplicitPauseIntentActive` makes the observer + /// re-pause the page the instant anything resumes it, so a media key handled by WebKit would + /// start playback and be killed a moment later, and clearing `shouldResumeAfterInterruption` + /// additionally blocks transport recovery from resuming. + func pause(intent: MusicPlaybackIntent, origin: MusicPauseOrigin = .user) async { guard self.acceptsMusicPlaybackIntent(intent) else { return } - self.logger.debug("Pausing playback") - self.shouldResumeAfterInterruption = false + self.logger.debug("Pausing playback (origin: \(String(describing: origin)))") self.isAwaitingPlaybackConfirmation = false - self.isExplicitPauseIntentActive = true + switch origin { + case .user: + self.shouldResumeAfterInterruption = false + self.isExplicitPauseIntentActive = true + self.routeLossPauseAt = nil + case let .routeLoss(at): + // Dated to when the command was admitted, not to now: this runs after an unbounded + // hop to the MainActor, and a marker stamped late would sort *after* a reconnect + // that already happened, leaving `hasAudioRouteChanged` permanently false. + self.routeLossPauseAt = at + } if self.isPendingRestoredLoadDeferred { self.state = .paused @@ -525,6 +543,33 @@ extension PlayerService { await self.resume(intent: intent) } + /// Resumes playback that the system stopped when its audio route disappeared, once a route + /// is available again. + /// + /// The offer stands for as long as the pause does — there is no expiry. Intent, not elapsed + /// time, is what retires it: a user pause or playback actually starting clears + /// `routeLossPauseAt`, so a resume can only ever continue exactly what the route loss + /// interrupted. Reconnecting headphones an hour later is still the same interrupted song. + /// + /// The marker deliberately survives issuing the resume; only confirmed playback clears it. + /// A route needs a moment to become usable, so the first attempt can be rejected while it + /// settles, and retiring the marker here would leave nothing for the retry to act on. + /// + /// Beyond restoring what the user was listening to, this is what rebinds WebKit's media + /// session: while it stays stale its Now Playing entry keeps swallowing media keys without + /// acting on them, which is why F8 does nothing until playback runs once. + func resumeAfterRouteRestored() async { + guard self.routeLossPauseAt.map({ self.hasAudioRouteReturned($0) }) == true, + // The disconnect drives this same path, and its route change predates the pause, + // so only output that came back afterwards — and is still there — qualifies. + !self.isPlaying, + !self.isExplicitPauseIntentActive, + self.currentTrack != nil || self.pendingPlayVideoId != nil + else { return } + self.logger.info("Audio route restored — resuming playback paused by the route loss") + await self.resume() + } + func resume(intent: MusicPlaybackIntent) async { guard self.acceptsMusicPlaybackIntent(intent) else { return } self.logger.debug("Resuming playback") diff --git a/Sources/Kaset/Services/Player/PlayerService.swift b/Sources/Kaset/Services/Player/PlayerService.swift index 8a0a499bc..88c7da376 100644 --- a/Sources/Kaset/Services/Player/PlayerService.swift +++ b/Sources/Kaset/Services/Player/PlayerService.swift @@ -116,6 +116,20 @@ final class PlayerService: NSObject, PlayerServiceProtocol { var isAwaitingPlaybackConfirmation = false var isExplicitPauseIntentActive = false + /// When the system paused playback because its audio route disappeared, if that is why + /// playback is currently stopped. + /// + /// The route coming back is the cue to resume. That matters beyond convenience: WebKit's + /// Now Playing registration survives the route loss as an entry that still captures the + /// media keys but no longer acts on them, and playing again is the only thing that rebinds + /// it. Until then F8 reaches a session that does nothing. + @ObservationIgnored var routeLossPauseAt: ContinuousClock.Instant? + + /// Whether output came back after the given instant and is still available. Injectable for tests. + @ObservationIgnored var hasAudioRouteReturned: @MainActor (ContinuousClock.Instant) -> Bool = { + DefaultOutputDeviceMonitor.shared.routeRestored(since: $0) + } + /// Currently playing track. var currentTrack: Song? { didSet { diff --git a/Tests/KasetTests/NowPlayingClaimTests.swift b/Tests/KasetTests/NowPlayingClaimTests.swift index 1876183ee..5760bb95a 100644 --- a/Tests/KasetTests/NowPlayingClaimTests.swift +++ b/Tests/KasetTests/NowPlayingClaimTests.swift @@ -3,14 +3,32 @@ import MediaPlayer import Testing @testable import Kaset +// MARK: - NowPlayingClaimTests + @Suite("Now playing claim", .serialized, .tags(.service)) +@MainActor struct NowPlayingClaimTests { - @Test("Actively playing or starting yields hands-off (WebKit owns the card)") + @Test("Started playback yields hands-off (WebKit owns the card)") func activePlaybackIsHandsOff() { + // `buffering` is a stall inside playback that already started, so it belongs with + // `playing`: re-claiming mid-track would recreate the competing second entry. let track = (title: "Song", artist: "Artist") #expect(NowPlayingManager.desiredClaim(state: .playing, track: track, activeVideo: nil) == .handsOff) #expect(NowPlayingManager.desiredClaim(state: .buffering, track: track, activeVideo: nil) == .handsOff) - #expect(NowPlayingManager.desiredClaim(state: .loading, track: track, activeVideo: nil) == .handsOff) + } + + @Test("A starting track keeps a playing claim until WebKit publishes its card") + func startingPlaybackKeepsClaimUntilHandover() { + // Withdrawing during the load would leave the app with no Now Playing entry, and + // therefore no media keys, for as long as the track takes to start. + let track = (title: "Song", artist: "Artist") + let expected = NowPlayingManager.NowPlayingClaim.claim( + title: "Song", + artist: "Artist", + playbackState: .playing + ) + #expect(NowPlayingManager.desiredClaim(state: .loading, track: track, activeVideo: nil) == expected) + #expect(NowPlayingManager.desiredClaim(state: .loading, track: nil, activeVideo: nil) == .release) } @Test("Not playing with a track yields a minimal claim") @@ -93,6 +111,79 @@ struct NowPlayingClaimTests { ) == expected) } + @Test("Hands-off and release both withdraw Kaset's entry so only one card can exist") + func nonClaimDecisionsWithdrawTheEntry() { + // WebKit registers a separate Now Playing client rather than overwriting + // `MPNowPlayingInfoCenter.default()`, so a claim kept during playback survives as a + // second entry for the same app and can capture the media keys with a stale state. + // Both non-claim decisions must therefore map onto the same withdrawal. + let track = (title: "Song", artist: "Artist") + let playingClaim = NowPlayingManager.desiredClaim(state: .playing, track: track, activeVideo: nil) + let emptyClaim = NowPlayingManager.desiredClaim(state: .idle, track: nil, activeVideo: nil) + + #expect(playingClaim == .handsOff) + #expect(emptyClaim == .release) + for claim in [playingClaim, emptyClaim] { + switch claim { + case .handsOff, .release: + break + case .claim: + Issue.record("Playback and empty states must not publish a native claim") + } + } + } + + @Test("Handing the card to WebKit withdraws our entry") + func handsOffClearsOwnedMetadata() { + // The central fix: the hands-off branch used to wait for WebKit to replace the app-wide + // metadata, which never happens, leaving a second permanently stale Control Center entry. + let center = MockNowPlayingInfoCenter() + NowPlayingManager.applyClaim( + .claim(title: "Song", artist: "Artist", playbackState: .paused), + isAssertingNativeClaim: false, + to: center + ) + #expect(NowPlayingManager.isNativeClaim(center.nowPlayingInfo)) + + let stillAsserting = NowPlayingManager.applyClaim( + .handsOff, + isAssertingNativeClaim: true, + to: center + ) + + #expect(stillAsserting == false) + #expect(center.nowPlayingInfo == nil) + #expect(center.playbackState == .stopped) + } + + @Test("Releasing with nothing to play withdraws our entry too") + func releaseClearsOwnedMetadata() { + let center = MockNowPlayingInfoCenter() + NowPlayingManager.applyClaim( + .claim(title: "Song", artist: "Artist", playbackState: .paused), + isAssertingNativeClaim: false, + to: center + ) + + NowPlayingManager.applyClaim(.release, isAssertingNativeClaim: true, to: center) + + #expect(center.nowPlayingInfo == nil) + } + + @Test("A card we do not own is never cleared") + func withdrawalLeavesForeignMetadataAlone() { + // WebKit publishes through its own client, but anything landing in the shared center + // that lacks our tag must survive a withdrawal untouched. + let center = MockNowPlayingInfoCenter() + center.nowPlayingInfo = [MPMediaItemPropertyTitle: "Someone else's card"] + center.playbackState = .playing + + NowPlayingManager.applyClaim(.handsOff, isAssertingNativeClaim: true, to: center) + + #expect(center.nowPlayingInfo?.isEmpty == false) + #expect(center.playbackState == .playing) + } + @Test("Only tagged native metadata is treated as Kaset's claim") func nativeClaimOwnershipTag() { let nativeInfo: [String: Any] = [ @@ -105,3 +196,11 @@ struct NowPlayingClaimTests { #expect(NowPlayingManager.isNativeClaim(nil) == false) } } + +// MARK: - MockNowPlayingInfoCenter + +@MainActor +final class MockNowPlayingInfoCenter: NowPlayingInfoCenter { + var nowPlayingInfo: [String: Any]? + var playbackState: MPNowPlayingPlaybackState = .unknown +} diff --git a/Tests/KasetTests/PlayerServiceRouteChangePauseTests.swift b/Tests/KasetTests/PlayerServiceRouteChangePauseTests.swift new file mode 100644 index 000000000..b8ec18de5 --- /dev/null +++ b/Tests/KasetTests/PlayerServiceRouteChangePauseTests.swift @@ -0,0 +1,140 @@ +import Foundation +import Testing +@testable import Kaset + +/// Losing an audio route (Bluetooth disconnecting, headphones unplugged) reaches the app as an +/// ordinary `pause` remote command. Recording it as a deliberate user pause is what broke media +/// keys after reconnecting: `isExplicitPauseIntentActive` makes the observer re-pause the page +/// the moment a WebKit-handled media key resumes it, so playback starts and dies immediately. +@Suite("Player service route-change pause", .serialized, .tags(.service)) +@MainActor +struct PlayerServiceRouteChangePauseTests { + @Test("A user pause records a standing intent to stay paused") + func userPauseHoldsPauseIntent() async { + let playerService = self.makePlayingService() + + await playerService.pause(intent: playerService.currentMusicPlaybackIntent) + + #expect(playerService.state == .paused) + #expect(playerService.isExplicitPauseIntentActive) + #expect(playerService.shouldResumeAfterInterruption == false) + } + + @Test("A route-change pause stops playback without claiming the user wants it paused") + func routeChangePauseKeepsResumeIntent() async { + let playerService = self.makePlayingService() + + await playerService.pause( + intent: playerService.currentMusicPlaybackIntent, + origin: .routeLoss(at: .now) + ) + + #expect(playerService.state == .paused) + // The two flags that would otherwise fight the next resume. + #expect(playerService.isExplicitPauseIntentActive == false) + #expect(playerService.shouldResumeAfterInterruption) + } + + @Test("After a route-change pause the page resuming is adopted, not undone") + func resumeAfterRouteChangePauseSurvives() async { + let playerService = self.makePlayingService() + await playerService.pause( + intent: playerService.currentMusicPlaybackIntent, + origin: .routeLoss(at: .now) + ) + + // A media key handled by WebKit resumes the page; the observer reports it. + playerService.updatePlaybackState( + isPlaying: true, + progress: 137, + duration: 300, + observedVideoId: "route-change" + ) + + #expect(playerService.state == .playing) + } + + @Test("After a user pause the page resuming is still refused") + func resumeAfterUserPauseIsStillRefused() async { + let playerService = self.makePlayingService() + await playerService.pause(intent: playerService.currentMusicPlaybackIntent) + + playerService.updatePlaybackState( + isPlaying: true, + progress: 137, + duration: 300, + observedVideoId: "route-change" + ) + + #expect(playerService.state == .paused) + } + + @Test("A returning route resumes what the route loss stopped") + func routeRestoredResumes() async { + let playerService = self.makeRouteLossPausedService(routeReturned: true) + + await playerService.resumeAfterRouteRestored() + + #expect(playerService.isAwaitingPlaybackConfirmation) + } + + @Test("The recovery marker survives an unconfirmed resume so the retry can act") + func markerSurvivesUntilPlaybackConfirms() async { + // A route needs a moment to become usable, so the first attempt can be rejected while + // it settles. Retiring the marker on issue would leave the retry nothing to act on. + let playerService = self.makeRouteLossPausedService(routeReturned: true) + + await playerService.resumeAfterRouteRestored() + #expect(playerService.routeLossPauseAt != nil) + + playerService.confirmPlaybackStarted() + #expect(playerService.routeLossPauseAt == nil) + } + + @Test("The disconnect's own reconciliation does not resume") + func disconnectDoesNotResumeItself() async { + // Losing the route schedules the same reconciliation the reconnect uses, and its route + // change predates the pause. Mistaking it for a reconnect would restart the music the + // system just stopped. + let playerService = self.makeRouteLossPausedService(routeReturned: false) + + await playerService.resumeAfterRouteRestored() + + #expect(playerService.isAwaitingPlaybackConfirmation == false) + #expect(playerService.state == .paused) + } + + @Test("A user pause is never resumed by a returning route") + func userPauseIsNotResumed() async { + let playerService = self.makeRouteLossPausedService(routeReturned: true) + await playerService.pause(intent: playerService.currentMusicPlaybackIntent) + + await playerService.resumeAfterRouteRestored() + + #expect(playerService.isAwaitingPlaybackConfirmation == false) + #expect(playerService.state == .paused) + } + + private func makeRouteLossPausedService(routeReturned: Bool) -> PlayerService { + let playerService = self.makePlayingService() + playerService.hasAudioRouteReturned = { _ in routeReturned } + playerService.state = .paused + playerService.routeLossPauseAt = .now + return playerService + } + + private func makePlayingService() -> PlayerService { + let playerService = PlayerService() + playerService.currentTrack = Song( + id: "route-change", + title: "route-change", + artists: [], + duration: 300, + videoId: "route-change" + ) + playerService.state = .playing + playerService.shouldResumeAfterInterruption = true + playerService.isExplicitPauseIntentActive = false + return playerService + } +} diff --git a/Tests/KasetTests/RouteDisappearanceClassificationTests.swift b/Tests/KasetTests/RouteDisappearanceClassificationTests.swift new file mode 100644 index 000000000..1cdd5f171 --- /dev/null +++ b/Tests/KasetTests/RouteDisappearanceClassificationTests.swift @@ -0,0 +1,143 @@ +import Foundation +import Testing +@testable import Kaset + +/// A vanished audio route and a user pressing Pause arrive as the same remote command, so the +/// only thing separating them is a device disappearing just before the command was admitted. +/// +/// Classification runs after an unbounded hop to the MainActor, so every case here is expressed +/// in terms of the instant the command was *admitted* — never the instant it is judged, and +/// never assuming the route stood still in between. +@Suite("Route disappearance classification", .tags(.service)) +struct RouteDisappearanceClassificationTests { + private let window = Duration.milliseconds(1500) + private let base = ContinuousClock.now + + @Test("A pause admitted just after the output device vanished is route-driven") + func pauseFollowingDisappearanceQualifies() { + let events = [self.disappearance(at: 0)] + + #expect(self.routeLossIndex(in: events, admittedAt: 60) == 0) + } + + @Test("A pause admitted before the device vanished is the user's") + func pausePrecedingDisappearanceDoesNotQualify() { + // The command was already in flight when the route dropped, so the route cannot be + // what prompted it. + let events = [self.disappearance(at: 100)] + + #expect(self.routeLossIndex(in: events, admittedAt: 90) == nil) + } + + @Test("A pause admitted long after the device vanished is the user's") + func pauseOutsideWindowDoesNotQualify() { + let events = [self.disappearance(at: 0)] + + #expect(self.routeLossIndex(in: events, admittedAt: 1600) == nil) + } + + @Test("Output restored before the pause makes it the user's") + func restorationBeforeTheCommandDoesNotQualify() { + // Disconnect then reconnect quickly: there was somewhere to play by the time the + // command arrived, so pausing then was deliberate. + let events = [self.disappearance(at: 0), self.restoration(at: 200)] + + #expect(self.routeLossIndex(in: events, admittedAt: 250) == nil) + } + + @Test("Output restored after the pause still leaves it route-driven") + func restorationAfterTheCommandStillQualifies() { + // The reconnect landed while the command was still queued for the MainActor. Judging by + // the latest route state would misread this as a user pause and disable recovery. + let events = [self.disappearance(at: 0), self.restoration(at: 160)] + + #expect(self.routeLossIndex(in: events, admittedAt: 60) == 0) + } + + @Test("A later disconnect does not hide the one that explains a queued pause") + func newerDisappearanceDoesNotShadowTheRelevantOne() { + // Flapping Bluetooth can complete a whole disconnect/reconnect/disconnect cycle before + // the first pause is drained. Keeping only the newest events would leave nothing that + // predates the command, and a genuine route pause would read as the user's. + let events = [ + self.disappearance(at: 0), + self.restoration(at: 400), + self.disappearance(at: 800), + ] + + #expect(self.routeLossIndex(in: events, admittedAt: 60) == 0) + } + + @Test("A disappearance already claimed cannot explain a second pause") + func consumedDisappearanceDoesNotQualify() { + // The system's own route-loss pause claims the disconnect. A user pause moments later + // must keep its own meaning, or a future reconnect would resume against it. + var events = [self.disappearance(at: 0)] + events[0].isConsumed = true + + #expect(self.routeLossIndex(in: events, admittedAt: 60) == nil) + } + + @Test("Route changes that never removed a device do not qualify") + func changeWithoutDisappearanceDoesNotQualify() { + // Plugging in a new output or switching by hand changes the default device without + // taking one away. + let events = [self.restoration(at: 0)] + + #expect(self.routeLossIndex(in: events, admittedAt: 60) == nil) + } + + // MARK: - Route restoration + + @Test("Output coming back after the pause counts as restored") + func restorationAfterMarkerQualifies() { + let events = [self.disappearance(at: 0), self.restoration(at: 400)] + + #expect(DefaultOutputDeviceMonitor.routeRestored(in: events, since: self.base + .milliseconds(60))) + } + + @Test("Nothing after the pause is not a restoration") + func noEventAfterMarkerDoesNotQualify() { + let events = [self.disappearance(at: 0)] + + #expect(DefaultOutputDeviceMonitor.routeRestored( + in: events, + since: self.base + .milliseconds(60) + ) == false) + } + + @Test("A route lost again after coming back is not restored") + func laterDisappearanceRevokesRestoration() { + // Flapping Bluetooth: reconnect then drop again before the delayed retry runs. Treating + // any later event as a restoration would resume onto an output that is gone, and with + // playback already paused there may be no second pause command to re-arm recovery. + let events = [ + self.disappearance(at: 0), + self.restoration(at: 400), + self.disappearance(at: 800), + ] + + #expect(DefaultOutputDeviceMonitor.routeRestored( + in: events, + since: self.base + .milliseconds(60) + ) == false) + } + + // MARK: - Helpers + + private func disappearance(at milliseconds: Int) -> RouteChangeEvent { + RouteChangeEvent(at: self.base + .milliseconds(milliseconds), isDisappearance: true) + } + + private func restoration(at milliseconds: Int) -> RouteChangeEvent { + RouteChangeEvent(at: self.base + .milliseconds(milliseconds), isDisappearance: false) + } + + private func routeLossIndex(in events: [RouteChangeEvent], admittedAt milliseconds: Int) -> Int? { + DefaultOutputDeviceMonitor.routeLossIndex( + in: events, + admittedAt: self.base + .milliseconds(milliseconds), + within: self.window + ) + } +} diff --git a/docs/adr/0033-audio-route-loss-recovery.md b/docs/adr/0033-audio-route-loss-recovery.md new file mode 100644 index 000000000..e1eee759b --- /dev/null +++ b/docs/adr/0033-audio-route-loss-recovery.md @@ -0,0 +1,113 @@ +# ADR-0033: Audio Route Loss Recovery and Now Playing Ownership + +## Status + +Implemented + +## Context + +Media keys (F8) stopped controlling playback after Bluetooth headphones disconnected and +reconnected. Clicking Play in Kaset's window restored them. The behavior was reproduced with a +timestamped file trace (see `docs/common-bug-patterns.md`) rather than inferred; three separate +defects turned out to stack. + +### 1. WebKit's Now Playing session survives a route loss as a zombie + +WebKit registers its own Now Playing client with the system, independent of the app's +`MPNowPlayingInfoCenter`. When the audio route disappears, that registration outlives the media +session it described: Control Center still shows its card, and the card still captures the media +keys — but its transport actions do nothing. Pressing the card's own play button is a no-op, +while Kaset's card next to it works. + +The registration cannot be withdrawn from the app side. Clearing `navigator.mediaSession` +metadata and `playbackState` was tried and only blanks the card's contents; WebKit keeps the +registration as long as a media element that has played still exists. The only thing that +rebinds the session to a live route is playback actually running again — which is exactly what +the manual "click Play" workaround did. + +### 2. A route-loss pause was recorded as a deliberate user pause + +macOS stops playback on a vanished route by sending the app an ordinary `pause` remote command, +indistinguishable from the user pressing Pause. Kaset's handler set +`isExplicitPauseIntentActive`, whose purpose is to stop YouTube's autoplay from overriding a +user's pause. With it set, `applyObservedPlaybackState` re-pauses the page the instant anything +resumes it — so even a media key that did reach a healthy session started playback and had it +killed a moment later. + +### 3. The native Now Playing claim could never be released + +`NowPlayingManager` published a tagged minimal claim while paused so media keys still reached +Kaset (ADR-less, from #387). Its hands-off branch waited for "WebKit to atomically replace the +app-wide metadata" before standing down. WebKit never does that — it does not write +`MPNowPlayingInfoCenter` at all — so the claim was never withdrawn and Kaset kept a second, +permanently stale Control Center entry once it had paused even once. + +## Decision + +**Resume playback when the audio route returns**, scoped precisely to playback the route loss +itself stopped. This restores what the user was listening to and, as a direct consequence, +rebinds WebKit's media session so the media keys work again for everything afterward. + +Supporting decisions: + +- **Classify the pause at its source.** `MusicPauseOrigin` distinguishes `.user` from + `.routeLoss(at:)`. Only a user pause records the standing intent to stay paused. +- **Identify a route loss by a device disappearing**, not by any output change. A manual output + switch or a device being added must not arm recovery. `DefaultOutputDeviceMonitor` remembers + the previous default device and checks both device-list membership and + `kAudioDevicePropertyDeviceIsAlive` — Core Audio can mark a device dead before dropping its + ID. +- **Anchor timing to command admission.** Remote commands drain onto the MainActor + asynchronously, so both the classification window and the recovery marker use the ingress + admission instant. Dating either to handling time lets a busy main actor misclassify a real + route pause, or sort the marker after a reconnect that already happened. +- **Bound recovery by intent, not by a timer.** The marker is retired by a user pause or by + playback actually starting — never by elapsed time. A resume can therefore only ever continue + exactly what the route loss interrupted, so reconnecting an hour later is still the same + interrupted song. It deliberately survives *issuing* a resume so the second attempt can retry + while the route is still settling. +- **Release the native claim whenever WebKit owns the card**, since the event it was waiting for + does not exist. + +## Consequences + +### Positive + +- Media keys work after a Bluetooth disconnect/reconnect without touching the app. +- Reconnecting headphones continues the interrupted track, matching AirPods behavior elsewhere. +- Exactly one Control Center entry during playback instead of a stale duplicate. + +### Negative + +- Playback restarts on its own when a route returns. This is a deliberate behavior change; it is + narrowly scoped to a pause the system imposed, and any user transport action retires it. +- Classification rests on a 1.5s correlation window between a device disappearing and the `pause` + command (measured at 40–60ms). It is a heuristic, not a signal the OS provides. + +### Out of scope: the YouTube video source + +Only the music player recovers. When `PlaybackArbiter` routes media keys to +`YouTubePlayerService`, the same system pause still reaches `handleRemotePause` unclassified, no +recovery marker is recorded, and reconnecting cannot rebind that video's WebKit media session — +so the media-key failure described above remains for video playback. + +This is a gap, not a regression: nothing about the video path changed. It is left out because it +needs its own recovery marker and resume path in a separate service, and none of it could be +verified against the reproduction that drove this work, which was music-only. Shipping an +unverified parallel implementation alongside a verified one was judged worse than recording the +gap. It deserves its own change, reproduced and traced the same way. + +### Known limitation + +`DefaultOutputDeviceMonitor` watches only which device is the *default output*. A route change +within one device — some Macs expose speakers and the headphone jack as data sources on the same +built-in device — leaves that selection untouched and goes unseen, so recovery does not arm. +Covering it requires listening on the current device's output data source and retargeting those +listeners on every default-device change. Bluetooth always swaps the device itself. A missed +change degrades to the previous behavior rather than misbehaving. + +## References + +- ADR-0001: WebView-Based Playback +- ADR-0026: Generation-Scoped Web Playback Bridge +- `docs/common-bug-patterns.md` — the trace workflow that localized this diff --git a/docs/adr/README.md b/docs/adr/README.md index 4fc7ee573..2ab160c32 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -67,3 +67,4 @@ What becomes easier or more difficult because of this change? | [0030](0030-account-scoped-favorites.md) | Account-Scoped Favorites Persistence | Accepted | | [0031](0031-saved-album-library-reconciliation.md) | Saved-Album Library Identity and Reconciliation | Accepted | | [0032](0032-youtube-ask-gemini.md) | Watch-Scoped YouTube Ask Gemini | Accepted; fixed WEB profile enabled in production | +| [0033](0033-audio-route-loss-recovery.md) | Audio Route Loss Recovery and Now Playing Ownership | Accepted | From f3eaa9e59a51f9c50527cdc063e77da2a2898cb0 Mon Sep 17 00:00:00 2001 From: 9tong Date: Sat, 8 Aug 2026 08:51:22 +0800 Subject: [PATCH 2/5] fix(player): re-attribute a pause that outran its route event MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Core Audio timestamps a route change immediately but publishes it only after several synchronous device queries, while the pause command it provokes drains onto the MainActor independently. When the command wins that race it is judged against a route log that does not yet contain the disappearance, so a genuine route-loss pause records explicit pause intent — and nothing later can undo it, leaving exactly the media-key failure this change exists to repair. Classify from both sides instead. A remote pause no route loss explains still behaves as the user's, but keeps its admission instant; publishing the route event retries the attribution and flips the pause when the disappearance accounts for it. The claim is the same one-shot used at admission, so a disconnect still explains at most one pause. Co-Authored-By: Claude Opus 5 --- .../Services/Player/MusicPlaybackIntent.swift | 11 ++++-- .../Services/Player/NowPlayingManager.swift | 22 ++++++++++-- .../PlayerService+PlaybackControls.swift | 31 ++++++++++++++++ .../Kaset/Services/Player/PlayerService.swift | 8 +++++ .../PlayerServiceRouteChangePauseTests.swift | 35 +++++++++++++++++++ docs/adr/0033-audio-route-loss-recovery.md | 6 ++++ 6 files changed, 107 insertions(+), 6 deletions(-) diff --git a/Sources/Kaset/Services/Player/MusicPlaybackIntent.swift b/Sources/Kaset/Services/Player/MusicPlaybackIntent.swift index fe73250a5..97ef303f0 100644 --- a/Sources/Kaset/Services/Player/MusicPlaybackIntent.swift +++ b/Sources/Kaset/Services/Player/MusicPlaybackIntent.swift @@ -18,6 +18,9 @@ struct MusicPlaybackIntent: Equatable { /// distinction has to be carried explicitly. enum MusicPauseOrigin: Equatable { case user + /// A remote pause that no route loss explained when it arrived. Treated as the user's, but + /// its admission instant is kept so a route event published afterwards can still claim it. + case unattributedRemote(admittedAt: ContinuousClock.Instant) /// The system took the audio route away, as of when the command was admitted. case routeLoss(at: ContinuousClock.Instant) } @@ -26,7 +29,9 @@ enum MusicPauseOrigin: Equatable { enum MusicRemoteTransportCommand: Equatable { case play - case pause + /// A remote `pause` no route loss explained at admission. Carries the admission instant so + /// a route event that lands afterwards can still re-attribute it. + case pause(admittedAt: ContinuousClock.Instant) /// A `pause` the system imposed by taking the audio route away, not a user request. /// Carries the ingress admission instant so the recovery marker is dated to when the /// command arrived rather than to whenever the MainActor got around to draining it. @@ -231,9 +236,9 @@ extension PlayerService { case .play: self.clearRemoteMusicSkipCoalescingTarget() await self.resume(intent: intent) - case .pause: + case let .pause(admittedAt): self.clearRemoteMusicSkipCoalescingTarget() - await self.pause(intent: intent) + await self.pause(intent: intent, origin: .unattributedRemote(admittedAt: admittedAt)) case let .pauseForRouteChange(admittedAt): self.clearRemoteMusicSkipCoalescingTarget() await self.pause(intent: intent, origin: .routeLoss(at: admittedAt)) diff --git a/Sources/Kaset/Services/Player/NowPlayingManager.swift b/Sources/Kaset/Services/Player/NowPlayingManager.swift index d60a29136..49f6366ea 100644 --- a/Sources/Kaset/Services/Player/NowPlayingManager.swift +++ b/Sources/Kaset/Services/Player/NowPlayingManager.swift @@ -360,10 +360,22 @@ final class NowPlayingManager { private func observeOutputDeviceChanges() { DefaultOutputDeviceMonitor.shared.start { [weak self] in - self?.resumeAfterRouteRestoredSoon() + self?.handleOutputDeviceChange() } } + /// Runs once the route event has been published, which is the earliest moment a pause that + /// beat it can be attributed correctly. + private func handleOutputDeviceChange() { + self.playerService?.reattributeRemotePauseToRouteLoss { admittedAt in + DefaultOutputDeviceMonitor.shared.claimRouteLossPause( + admittedAt: admittedAt, + within: Self.routeLossPauseWindow + ) + } + self.resumeAfterRouteRestoredSoon() + } + /// Resumes playback that a vanished audio route had stopped, once a route is back. /// /// The device is not immediately usable when the notification lands, and WebKit settles a @@ -573,7 +585,7 @@ final class NowPlayingManager { } else { let command: MusicRemoteTransportCommand = Self.isRouteChangePause(capturedCommand) ? .pauseForRouteChange(admittedAt: capturedCommand.admittedAt) - : .pause + : .pause(admittedAt: capturedCommand.admittedAt) self.enqueueMusicRemoteCommand( command, capturedCommand: capturedCommand, @@ -646,10 +658,14 @@ final class NowPlayingManager { private static func isRouteChangePause(_ capturedCommand: CapturedRemoteMusicCommand) -> Bool { DefaultOutputDeviceMonitor.shared.claimRouteLossPause( admittedAt: capturedCommand.admittedAt, - within: .milliseconds(1500) + within: self.routeLossPauseWindow ) } + /// How closely a `pause` must follow a device disappearing to be attributed to it. Measured + /// at 40-60ms; generous enough for scheduling jitter, far below human reaction time. + private static let routeLossPauseWindow = Duration.milliseconds(1500) + private func handleNextPreviousMediaKey( direction: RemoteMusicCommandDirection, capturedCommand: CapturedRemoteMusicCommand, diff --git a/Sources/Kaset/Services/Player/PlayerService+PlaybackControls.swift b/Sources/Kaset/Services/Player/PlayerService+PlaybackControls.swift index 028b15cea..cc4abb251 100644 --- a/Sources/Kaset/Services/Player/PlayerService+PlaybackControls.swift +++ b/Sources/Kaset/Services/Player/PlayerService+PlaybackControls.swift @@ -369,6 +369,7 @@ extension PlayerService { self.showMiniPlayer = false self.routeLossPauseAt = nil + self.unattributedRemotePauseAt = nil self.state = .playing if shouldRecordInteraction { @@ -501,7 +502,17 @@ extension PlayerService { self.shouldResumeAfterInterruption = false self.isExplicitPauseIntentActive = true self.routeLossPauseAt = nil + self.unattributedRemotePauseAt = nil + case let .unattributedRemote(admittedAt): + // Behaves as the user's until proven otherwise. Core Audio publishes a route change + // only after several synchronous device queries while this command drains + // independently, so a genuine route-loss pause can arrive first and look deliberate. + self.shouldResumeAfterInterruption = false + self.isExplicitPauseIntentActive = true + self.routeLossPauseAt = nil + self.unattributedRemotePauseAt = admittedAt case let .routeLoss(at): + self.unattributedRemotePauseAt = nil // Dated to when the command was admitted, not to now: this runs after an unbounded // hop to the MainActor, and a marker stamped late would sort *after* a reconnect // that already happened, leaving `hasAudioRouteChanged` permanently false. @@ -543,6 +554,25 @@ extension PlayerService { await self.resume(intent: intent) } + /// Re-attributes a remote pause to the route loss that caused it, once the route event is + /// published and `claim` confirms it explains that pause. + /// + /// Classification at admission is a race the pause can win: the Core Audio listener runs + /// several synchronous device queries before publishing, so the command may be judged + /// against a route log that does not yet contain the disappearance. Retrying from the other + /// side closes it — the route event is the thing that arrives late, not the pause. + func reattributeRemotePauseToRouteLoss(claim: (ContinuousClock.Instant) -> Bool) { + guard let admittedAt = self.unattributedRemotePauseAt, + !self.isPlaying, + claim(admittedAt) + else { return } + self.unattributedRemotePauseAt = nil + self.shouldResumeAfterInterruption = true + self.isExplicitPauseIntentActive = false + self.routeLossPauseAt = admittedAt + self.logger.info("Re-attributed a remote pause to the audio route loss that caused it") + } + /// Resumes playback that the system stopped when its audio route disappeared, once a route /// is available again. /// @@ -573,6 +603,7 @@ extension PlayerService { func resume(intent: MusicPlaybackIntent) async { guard self.acceptsMusicPlaybackIntent(intent) else { return } self.logger.debug("Resuming playback") + self.unattributedRemotePauseAt = nil self.isStoppingPlayback = false self.shouldResumeAfterInterruption = true self.isAwaitingPlaybackConfirmation = true diff --git a/Sources/Kaset/Services/Player/PlayerService.swift b/Sources/Kaset/Services/Player/PlayerService.swift index 88c7da376..6a1bbc7ab 100644 --- a/Sources/Kaset/Services/Player/PlayerService.swift +++ b/Sources/Kaset/Services/Player/PlayerService.swift @@ -125,6 +125,14 @@ final class PlayerService: NSObject, PlayerServiceProtocol { /// it. Until then F8 reaches a session that does nothing. @ObservationIgnored var routeLossPauseAt: ContinuousClock.Instant? + /// A remote pause that no route loss explained when it arrived. + /// + /// The Core Audio listener timestamps a route change immediately but publishes it only after + /// several synchronous device queries, while the `pause` command it provokes drains + /// independently. When the command wins that race the pause looks deliberate, so its instant + /// is kept and the attribution retried once the route event lands. + @ObservationIgnored var unattributedRemotePauseAt: ContinuousClock.Instant? + /// Whether output came back after the given instant and is still available. Injectable for tests. @ObservationIgnored var hasAudioRouteReturned: @MainActor (ContinuousClock.Instant) -> Bool = { DefaultOutputDeviceMonitor.shared.routeRestored(since: $0) diff --git a/Tests/KasetTests/PlayerServiceRouteChangePauseTests.swift b/Tests/KasetTests/PlayerServiceRouteChangePauseTests.swift index b8ec18de5..2dc2b9170 100644 --- a/Tests/KasetTests/PlayerServiceRouteChangePauseTests.swift +++ b/Tests/KasetTests/PlayerServiceRouteChangePauseTests.swift @@ -115,6 +115,41 @@ struct PlayerServiceRouteChangePauseTests { #expect(playerService.state == .paused) } + @Test("A pause that beat the route event is re-attributed once it lands") + func lateRouteEventReattributesThePause() async { + // The Core Audio listener publishes only after several synchronous device queries, so a + // genuine route-loss pause can drain first and look deliberate. Classifying at admission + // alone would leave the explicit pause intent set and recovery permanently disabled. + let playerService = self.makePlayingService() + let admittedAt = ContinuousClock.now + await playerService.pause( + intent: playerService.currentMusicPlaybackIntent, + origin: .unattributedRemote(admittedAt: admittedAt) + ) + #expect(playerService.isExplicitPauseIntentActive) + + playerService.reattributeRemotePauseToRouteLoss { $0 == admittedAt } + + #expect(playerService.isExplicitPauseIntentActive == false) + #expect(playerService.shouldResumeAfterInterruption) + #expect(playerService.routeLossPauseAt == admittedAt) + } + + @Test("A pause no route loss explains keeps its user semantics") + func unclaimedPauseStaysDeliberate() async { + let playerService = self.makePlayingService() + await playerService.pause( + intent: playerService.currentMusicPlaybackIntent, + origin: .unattributedRemote(admittedAt: .now) + ) + + // No disappearance can account for it, so the claim is refused. + playerService.reattributeRemotePauseToRouteLoss { _ in false } + + #expect(playerService.isExplicitPauseIntentActive) + #expect(playerService.routeLossPauseAt == nil) + } + private func makeRouteLossPausedService(routeReturned: Bool) -> PlayerService { let playerService = self.makePlayingService() playerService.hasAudioRouteReturned = { _ in routeReturned } diff --git a/docs/adr/0033-audio-route-loss-recovery.md b/docs/adr/0033-audio-route-loss-recovery.md index e1eee759b..eba64c8e0 100644 --- a/docs/adr/0033-audio-route-loss-recovery.md +++ b/docs/adr/0033-audio-route-loss-recovery.md @@ -57,6 +57,12 @@ Supporting decisions: the previous default device and checks both device-list membership and `kAudioDevicePropertyDeviceIsAlive` — Core Audio can mark a device dead before dropping its ID. +- **Attribute from both sides.** Core Audio's listener timestamps a change immediately but only + publishes it after several synchronous device queries, while the `pause` it provokes drains + independently. Either can win. A pause that arrives first is treated as the user's but keeps + its admission instant, and the route event re-attributes it when it lands — the route event is + the thing that arrives late, not the pause. Classifying only at admission would leave the + explicit-pause intent set with no way to undo it, which is precisely the original failure. - **Anchor timing to command admission.** Remote commands drain onto the MainActor asynchronously, so both the classification window and the recovery marker use the ingress admission instant. Dating either to handling time lets a busy main actor misclassify a real From 756dce659e950170ec62b31249cbcf1e306ff34b Mon Sep 17 00:00:00 2001 From: 9tong Date: Sat, 8 Aug 2026 09:02:30 +0800 Subject: [PATCH 3/5] fix(player): serialize route-change reads against concurrent callbacks Core Audio invokes the listener directly on the notifying thread, so overlapping default-output notifications ran their device queries concurrently and committed independently. Two callbacks could interleave badly: one pairing its captured device ID with another's usability result, or a slower earlier callback being dropped by the monotonic guard after a later one appended. Dropping it discards a disappearance that a pause admitted between the two timestamps needs, so the pause keeps user semantics and recovery never arms. Take the timestamp and run both queries under the record's lock. The sequence is then atomic per callback, timestamps are monotonic by construction rather than by a guard that discards, and no commit can mix state read by a different one. Co-Authored-By: Claude Opus 5 --- .../Audio/DefaultOutputDeviceMonitor.swift | 49 +++++++------------ .../PlayerService+PlaybackControls.swift | 3 ++ 2 files changed, 20 insertions(+), 32 deletions(-) diff --git a/Sources/Kaset/Services/Audio/DefaultOutputDeviceMonitor.swift b/Sources/Kaset/Services/Audio/DefaultOutputDeviceMonitor.swift index be68716e5..da0805c1e 100644 --- a/Sources/Kaset/Services/Audio/DefaultOutputDeviceMonitor.swift +++ b/Sources/Kaset/Services/Audio/DefaultOutputDeviceMonitor.swift @@ -37,36 +37,29 @@ private final class RouteChangeRecord: @unchecked Sendable { self.lock.withLock { self.defaultDeviceID = defaultDeviceID } } - func currentDefaultDeviceID() -> AudioDeviceID? { - self.lock.withLock { self.defaultDeviceID } - } - /// Appends a change, marking it a disappearance when the device that had been the default is /// gone — the difference between headphones being unplugged and the user picking a different - /// output. `previousDeviceIsUsable` is evaluated by the caller against the device it is - /// replacing, since only the caller can reach Core Audio. - func record( - at instant: ContinuousClock.Instant, - defaultDeviceID: AudioDeviceID?, - previousDeviceIsUsable: Bool + /// output. + /// + /// Timestamping and the Core Audio queries all happen under this lock. A `nil` dispatch queue + /// means the listener runs directly on the notifying thread with no serialization, so + /// overlapping callbacks would otherwise interleave: one could pair its device ID with + /// another's usability result, or commit out of order and drop the very disappearance a + /// queued pause needs to be attributed. Serializing the whole sequence also makes the + /// timestamps monotonic by construction. + func recordChange( + currentDefaultDeviceID: () -> AudioDeviceID?, + isDeviceUsable: (AudioDeviceID) -> Bool ) { self.lock.withLock { - // A `nil` dispatch queue means Core Audio invokes the listener directly on the - // notifying thread, so callbacks are not serialized: each timestamps itself and then - // queries Core Audio outside this lock, and a slow one can arrive after a newer one. - // Drop anything already superseded rather than letting a stale reading overwrite - // fresh state or leave the log out of order. - if let newest = self.events.last?.at, instant <= newest { - return - } - - let isDisappearance = self.defaultDeviceID != nil && !previousDeviceIsUsable + let instant = ContinuousClock.now + let isDisappearance = self.defaultDeviceID.map { !isDeviceUsable($0) } ?? false self.events.append(RouteChangeEvent(at: instant, isDisappearance: isDisappearance)) self.events.removeAll { instant - $0.at > Self.retention } if self.events.count > Self.capacity { self.events.removeFirst(self.events.count - Self.capacity) } - self.defaultDeviceID = defaultDeviceID + self.defaultDeviceID = currentDefaultDeviceID() } } @@ -220,17 +213,9 @@ final class DefaultOutputDeviceMonitor { // swiftformat:disable:next modifierOrder nonisolated private static let listener: @Sendable (UInt32, UnsafePointer) -> Void = { _, _ in - // Timestamp on entry, before the Core Audio queries below and before the hop. Those - // queries take real time, and the `pause` this change provokes is admitted on - // another callback — a timestamp taken afterwards can sort after that command and - // read as though the route were still fine when it arrived. - let changedAt = ContinuousClock.now - let previousDeviceIsUsable = DefaultOutputDeviceMonitor.record.currentDefaultDeviceID() - .map(DefaultOutputDeviceMonitor.isDeviceUsable) ?? true - DefaultOutputDeviceMonitor.record.record( - at: changedAt, - defaultDeviceID: DefaultOutputDeviceMonitor.currentDefaultOutputDeviceID(), - previousDeviceIsUsable: previousDeviceIsUsable + DefaultOutputDeviceMonitor.record.recordChange( + currentDefaultDeviceID: DefaultOutputDeviceMonitor.currentDefaultOutputDeviceID, + isDeviceUsable: DefaultOutputDeviceMonitor.isDeviceUsable ) Task { @MainActor in DefaultOutputDeviceMonitor.shared.notifyChange() diff --git a/Sources/Kaset/Services/Player/PlayerService+PlaybackControls.swift b/Sources/Kaset/Services/Player/PlayerService+PlaybackControls.swift index cc4abb251..e5d273922 100644 --- a/Sources/Kaset/Services/Player/PlayerService+PlaybackControls.swift +++ b/Sources/Kaset/Services/Player/PlayerService+PlaybackControls.swift @@ -562,6 +562,9 @@ extension PlayerService { /// against a route log that does not yet contain the disappearance. Retrying from the other /// side closes it — the route event is the thing that arrives late, not the pause. func reattributeRemotePauseToRouteLoss(claim: (ContinuousClock.Instant) -> Bool) { + // The marker is checked before `claim` on purpose: this can run before the pause has + // drained, and consuming the disappearance here would leave nothing for the admission + // classification to find. Whichever of the two arrives second does the attribution. guard let admittedAt = self.unattributedRemotePauseAt, !self.isPlaying, claim(admittedAt) From 4a41e939c3bce13397710dc75feb48b0b4e9c4c4 Mon Sep 17 00:00:00 2001 From: 9tong Date: Sat, 8 Aug 2026 09:11:11 +0800 Subject: [PATCH 4/5] fix(player): keep late route events and pauses able to find each other MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two orderings the design claims to support were still broken. Moving the route timestamp inside the record lock defeated the late re-attribution added a commit earlier: that lock is also held by the pause claim, so when the pause got there first the disappearance was stamped after `admittedAt` and `routeLossIndex` rejected it. Capture the instant at listener entry again, before any contention, and keep the Core Audio queries under the lock. Arrival and commit order can then differ, so events are inserted in timestamp order rather than appended, and never dropped — a discarded disappearance is one a queued pause may still need. Recovery attempts were also scheduled only by the output-device callback. A reconnect can be recorded and handled before its earlier pause drains, which the classification deliberately allows, spending both attempts while no marker existed yet. Installing the route-loss marker now schedules them too; it is a no-op when no route has returned, since the resume re-checks that itself. Co-Authored-By: Claude Opus 5 --- .../Audio/DefaultOutputDeviceMonitor.swift | 30 +++++++++++++------ .../Services/Player/MusicPlaybackIntent.swift | 5 ++++ .../Services/Player/NowPlayingManager.swift | 2 +- 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/Sources/Kaset/Services/Audio/DefaultOutputDeviceMonitor.swift b/Sources/Kaset/Services/Audio/DefaultOutputDeviceMonitor.swift index da0805c1e..cdb3e8ec3 100644 --- a/Sources/Kaset/Services/Audio/DefaultOutputDeviceMonitor.swift +++ b/Sources/Kaset/Services/Audio/DefaultOutputDeviceMonitor.swift @@ -41,21 +41,30 @@ private final class RouteChangeRecord: @unchecked Sendable { /// gone — the difference between headphones being unplugged and the user picking a different /// output. /// - /// Timestamping and the Core Audio queries all happen under this lock. A `nil` dispatch queue - /// means the listener runs directly on the notifying thread with no serialization, so - /// overlapping callbacks would otherwise interleave: one could pair its device ID with - /// another's usability result, or commit out of order and drop the very disappearance a - /// queued pause needs to be attributed. Serializing the whole sequence also makes the - /// timestamps monotonic by construction. + /// `instant` must be captured at listener entry, before any contention. This lock is also + /// held by `claimRouteLoss`, so stamping the event after acquiring it would date a + /// disappearance *after* a pause that got there first — exactly the ordering the late + /// re-attribution exists to serve, silently defeated. + /// + /// The Core Audio queries still run under the lock. A `nil` dispatch queue means the + /// listener runs directly on the notifying thread with no serialization, so overlapping + /// callbacks would otherwise interleave their reads and pair one callback's device ID with + /// another's usability result. Arrival order and commit order can then differ, so events are + /// inserted in timestamp order rather than appended — and never dropped, since a discarded + /// disappearance is one a queued pause may still need. func recordChange( + at instant: ContinuousClock.Instant, currentDefaultDeviceID: () -> AudioDeviceID?, isDeviceUsable: (AudioDeviceID) -> Bool ) { self.lock.withLock { - let instant = ContinuousClock.now let isDisappearance = self.defaultDeviceID.map { !isDeviceUsable($0) } ?? false - self.events.append(RouteChangeEvent(at: instant, isDisappearance: isDisappearance)) - self.events.removeAll { instant - $0.at > Self.retention } + let event = RouteChangeEvent(at: instant, isDisappearance: isDisappearance) + let index = self.events.firstIndex { $0.at > instant } ?? self.events.count + self.events.insert(event, at: index) + + let newest = self.events.last?.at ?? instant + self.events.removeAll { newest - $0.at > Self.retention } if self.events.count > Self.capacity { self.events.removeFirst(self.events.count - Self.capacity) } @@ -213,7 +222,10 @@ final class DefaultOutputDeviceMonitor { // swiftformat:disable:next modifierOrder nonisolated private static let listener: @Sendable (UInt32, UnsafePointer) -> Void = { _, _ in + // Before any lock contention, so the stamp is arrival time rather than acquisition. + let changedAt = ContinuousClock.now DefaultOutputDeviceMonitor.record.recordChange( + at: changedAt, currentDefaultDeviceID: DefaultOutputDeviceMonitor.currentDefaultOutputDeviceID, isDeviceUsable: DefaultOutputDeviceMonitor.isDeviceUsable ) diff --git a/Sources/Kaset/Services/Player/MusicPlaybackIntent.swift b/Sources/Kaset/Services/Player/MusicPlaybackIntent.swift index 97ef303f0..f9310b2ee 100644 --- a/Sources/Kaset/Services/Player/MusicPlaybackIntent.swift +++ b/Sources/Kaset/Services/Player/MusicPlaybackIntent.swift @@ -242,6 +242,11 @@ extension PlayerService { case let .pauseForRouteChange(admittedAt): self.clearRemoteMusicSkipCoalescingTarget() await self.pause(intent: intent, origin: .routeLoss(at: admittedAt)) + // A reconnect can be recorded and handled before this pause drains, spending + // both recovery attempts while there was still no marker to act on. Schedule + // from here too, so installing the marker is itself a trigger. Harmless when + // no route has returned: the resume re-checks that for itself. + NowPlayingManager.shared.resumeAfterRouteRestoredSoon() case .togglePlayPause: self.clearRemoteMusicSkipCoalescingTarget() await self.playPause(intent: intent) diff --git a/Sources/Kaset/Services/Player/NowPlayingManager.swift b/Sources/Kaset/Services/Player/NowPlayingManager.swift index 49f6366ea..4175926fe 100644 --- a/Sources/Kaset/Services/Player/NowPlayingManager.swift +++ b/Sources/Kaset/Services/Player/NowPlayingManager.swift @@ -382,7 +382,7 @@ final class NowPlayingManager { /// route change in stages, so try twice rather than racing it once. Bursts of notifications /// collapse into the latest request. `resumeAfterRouteRestored` itself decides whether a /// resume is warranted, so both a disconnect and a reconnect can drive this safely. - private func resumeAfterRouteRestoredSoon() { + func resumeAfterRouteRestoredSoon() { self.routeRestoreGeneration &+= 1 let generation = self.routeRestoreGeneration self.routeRestoreTask?.cancel() From 0143bcd44fa09df4265f8eaffeaf4743818bb006 Mon Sep 17 00:00:00 2001 From: 9tong Date: Sat, 8 Aug 2026 09:21:41 +0800 Subject: [PATCH 5/5] fix(player): close the last two route-attribution ordering gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Classification runs in NowPlayingManager before the command is enqueued, while the player's transport queue drains later. A disappearance recorded in that window has its callback find no marker to attribute, and the drain then installed the marker without rechecking — leaving the event unclaimed with nothing guaranteed to come back for it. The drain now retries the claim after installing the marker, so the stated invariant actually holds: whichever of the route event and the pause lands second performs the attribution. The route log also sorted events by arrival stamp while `defaultDeviceID` advanced in lock-acquisition order, so a callback that arrived earlier but committed later compared against a future baseline and could log a restoration where there was a disappearance. Transitions are consistent with the baseline they were compared against, so the log now orders the same way: an arrival stamp older than the previous commit is clamped forward rather than sorted behind it. Uncontended, which is the ordinary case, the stamp is still exactly arrival time. Co-Authored-By: Claude Opus 5 --- .../Audio/DefaultOutputDeviceMonitor.swift | 23 +++++++++---------- .../Services/Player/MusicPlaybackIntent.swift | 6 +++++ .../Services/Player/NowPlayingManager.swift | 13 ++++++++++- 3 files changed, 29 insertions(+), 13 deletions(-) diff --git a/Sources/Kaset/Services/Audio/DefaultOutputDeviceMonitor.swift b/Sources/Kaset/Services/Audio/DefaultOutputDeviceMonitor.swift index cdb3e8ec3..a4b3f8f48 100644 --- a/Sources/Kaset/Services/Audio/DefaultOutputDeviceMonitor.swift +++ b/Sources/Kaset/Services/Audio/DefaultOutputDeviceMonitor.swift @@ -46,12 +46,14 @@ private final class RouteChangeRecord: @unchecked Sendable { /// disappearance *after* a pause that got there first — exactly the ordering the late /// re-attribution exists to serve, silently defeated. /// - /// The Core Audio queries still run under the lock. A `nil` dispatch queue means the - /// listener runs directly on the notifying thread with no serialization, so overlapping - /// callbacks would otherwise interleave their reads and pair one callback's device ID with - /// another's usability result. Arrival order and commit order can then differ, so events are - /// inserted in timestamp order rather than appended — and never dropped, since a discarded - /// disappearance is one a queued pause may still need. + /// The Core Audio queries run under the lock. A `nil` dispatch queue means the listener runs + /// directly on the notifying thread with no serialization, so overlapping callbacks would + /// otherwise interleave their reads and pair one callback's device ID with another's + /// usability result. Each transition is therefore consistent with the baseline it was + /// compared against, and the log is ordered the same way that baseline advanced: an arrival + /// stamp that predates the previous commit is clamped forward rather than sorted behind it, + /// which would describe a transition against a baseline that no longer applied. + /// Uncontended — the ordinary case — the stamp is exactly arrival time. func recordChange( at instant: ContinuousClock.Instant, currentDefaultDeviceID: () -> AudioDeviceID?, @@ -59,12 +61,9 @@ private final class RouteChangeRecord: @unchecked Sendable { ) { self.lock.withLock { let isDisappearance = self.defaultDeviceID.map { !isDeviceUsable($0) } ?? false - let event = RouteChangeEvent(at: instant, isDisappearance: isDisappearance) - let index = self.events.firstIndex { $0.at > instant } ?? self.events.count - self.events.insert(event, at: index) - - let newest = self.events.last?.at ?? instant - self.events.removeAll { newest - $0.at > Self.retention } + let at = self.events.last.map { max(instant, $0.at) } ?? instant + self.events.append(RouteChangeEvent(at: at, isDisappearance: isDisappearance)) + self.events.removeAll { at - $0.at > Self.retention } if self.events.count > Self.capacity { self.events.removeFirst(self.events.count - Self.capacity) } diff --git a/Sources/Kaset/Services/Player/MusicPlaybackIntent.swift b/Sources/Kaset/Services/Player/MusicPlaybackIntent.swift index f9310b2ee..1bfcd6b3d 100644 --- a/Sources/Kaset/Services/Player/MusicPlaybackIntent.swift +++ b/Sources/Kaset/Services/Player/MusicPlaybackIntent.swift @@ -239,6 +239,12 @@ extension PlayerService { case let .pause(admittedAt): self.clearRemoteMusicSkipCoalescingTarget() await self.pause(intent: intent, origin: .unattributedRemote(admittedAt: admittedAt)) + // Classification ran before this command was enqueued, and this queue drains + // later. A disappearance recorded in between — whose callback found no marker to + // attribute — is still unclaimed, and nothing else is guaranteed to come back for + // it. Retry now that the marker exists, completing the symmetry: whichever of the + // route event and the pause lands second performs the attribution. + NowPlayingManager.shared.attributeRouteLossPauseIfPending() case let .pauseForRouteChange(admittedAt): self.clearRemoteMusicSkipCoalescingTarget() await self.pause(intent: intent, origin: .routeLoss(at: admittedAt)) diff --git a/Sources/Kaset/Services/Player/NowPlayingManager.swift b/Sources/Kaset/Services/Player/NowPlayingManager.swift index 4175926fe..904396a03 100644 --- a/Sources/Kaset/Services/Player/NowPlayingManager.swift +++ b/Sources/Kaset/Services/Player/NowPlayingManager.swift @@ -367,12 +367,23 @@ final class NowPlayingManager { /// Runs once the route event has been published, which is the earliest moment a pause that /// beat it can be attributed correctly. private func handleOutputDeviceChange() { - self.playerService?.reattributeRemotePauseToRouteLoss { admittedAt in + self.attributeRouteLossPauseIfPending() + self.resumeAfterRouteRestoredSoon() + } + + /// Attributes a pending unattributed pause to a route loss, if one now explains it. + /// + /// Safe to call from either side of the race and at any time: it is a no-op without a pending + /// pause, and the claim is one-shot, so a disconnect still explains at most one pause. + func attributeRouteLossPauseIfPending() { + guard let playerService = self.playerService else { return } + playerService.reattributeRemotePauseToRouteLoss { admittedAt in DefaultOutputDeviceMonitor.shared.claimRouteLossPause( admittedAt: admittedAt, within: Self.routeLossPauseWindow ) } + guard playerService.routeLossPauseAt != nil else { return } self.resumeAfterRouteRestoredSoon() }