PCIOS-868: iOS/Bluetooth: .routeConfigurationChange on an unchanged route tears down the player and never resumes

[pocketcasts]

Resolves https://linear.app/a8c/issue/PCIOS-868/iosbluetooth-routeconfigurationchange-on-an-unchanged-route-tears-down

Summary

When a Bluetooth head unit renegotiates its A2DP link, iOS can emit .routeConfigurationChange while the set of audio ports remains unchanged. The EffectsPlayer audio engine may stop during that reconfiguration and needs to be rebuilt.

Deep review also found a lifecycle race because the affected hardware emits reason-8 events in pairs. A duplicate event could arrive while PlaybackManager was asynchronously activating a replacement EffectsPlayer; the old route handler would then permanently pause and tear down that not-yet-started replacement.

Changes

• Handle .routeConfigurationChange directly from its API contract, without requiring previous-route metadata, and restart an actively intended EffectsPlayer without rebuilding Now Playing data.
• Serialize audio-route recovery onto the main thread before mutating playback state.
• Replace raw reason comparisons with a typed, testable route-change policy.
• Make restart-only route events a no-op while playback is paused, interrupted, or a replacement player has not started yet.
• Cancel queued EffectsPlayer setup if teardown wins the player lock, preventing an orphaned audio engine.
• Remove the non-atomic read/modify/write of shouldKeepPlaying.
• Log the rebuild only when EffectsPlayer actually performs it, and snapshot route outputs before the main-thread hop for accurate diagnostics.
• Add unit coverage for the route-policy mappings: pause, restart, configuration change, and ignored reasons.

Accepted trade-offs

• Recovery deliberately does not use engine.isRunning as a gate. Route and engine-configuration notifications have no documented ordering, so a stale true result could skip recovery and recreate the silent-playback failure.
• Route state changes remain serialized on the main thread. Audio-session activation is already dispatched to a background queue by default, while deactivation is delayed and cancelled by the immediate restart. This can be profiled separately if hardware testing shows a UI hitch.
• Cancelled engine setup does not invoke the play completion because that callback represents a successful start; firing it could run stale seek or pause work against a replacement player.

Verification

• make format: PASS
• make lint: PASS
• git diff --check: PASS
• Focused test/build execution is blocked locally before source compilation because the required watchOS 26.5 simulator runtime is not installed.

Manual verification

The original issue is hardware-specific. Playback should still be verified with the affected Bluetooth/A2DP head unit, including paired reason-8 notifications and active Trim Silence playback. The pass should also check for an unacceptable audio gap or UI hitch during route reconfiguration.

Confidence

MEDIUM-HIGH for the state-management, route-policy, and diagnostic changes. Hardware verification remains necessary to confirm audible recovery behavior.

[claude]

Claude finished @pocketcasts's task in 1m 20s —— View job

---

Review: PR #4834 — Bluetooth .routeConfigurationChange fix

• Read PR diff
• Analyze the change in context (handleRouteChanged)
• Check routeDidChange / EffectsPlayer restart logic
• Post review feedback

---

Overall this is a well-scoped, low-risk fix. It follows the existing pattern in handleRouteChanged and correctly narrows its action to the exact scenario in the bug (same device, route reconfigured). Nice touch adding a log line for future Bluetooth debugging. ✅

What's correct

• Narrow scope. Acting only when previousOutputUIDs == currentOutputUIDs means a real route change (different device) still falls through to existing behavior — good.
• Polymorphism is safe. player?.routeDidChange(shouldPause: false) is a no-op for DefaultPlayer (DefaultPlayer.swift:314-318 only acts when shouldPause == true), so this change only affects EffectsPlayer — which is exactly the player whose AVAudioEngine gets torn down. No collateral impact on the AVPlayer path.
• Restart mechanism is the right one. EffectsPlayer.routeDidChange(shouldPause:false) does pause() + play() guarded by shouldKeepPlaying and !interruptionInProgress() (EffectsPlayer.swift:358-363), which is the documented way to rebuild the dead engine.
• UID-based comparison is more robust than comparing portNames (which the existing logRouteChange uses for display only). 👍

Points worth considering (non-blocking)

1. Unconditional restart may add an audio hiccup in the common case — see the inline comment. .routeConfigurationChange also fires when the engine is not torn down; forcing pause/play there was previously a no-op and now introduces a brief gap. Consider gating on the engine actually being stopped inside EffectsPlayer.routeDidChange.
2. updateAllNowPlayingData() omitted vs. the sibling branch — reasonable since the device is unchanged, but a short intentional comment would help.
3. if let previousRoute failure path is silent. If the key is somehow absent, we do nothing (same as pre-fix behavior), so no regression — just be aware the fix is a strict superset only when the previous-route key is present.

Testing

The fix is inherently hardware-specific (A2DP renegotiation on older BT head units) and can't be reproduced in the simulator, as the PR notes. The MEDIUM confidence rating is appropriate — manual verification on the affected hardware is the real gate here. The pre-existing GenerateCredentials build failure is unrelated to this diff.

Verdict: Looks good to merge pending device testing. The considerations above are optional refinements, not required changes.

[claude]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[claude]

Claude finished @joashrajin's task in 2m 39s —— View job

---

Review: PR #4834 (updated) — Bluetooth .routeConfigurationChange fix

• Read the updated PR diff
• Analyze EffectsPlayer.routeDidChange changes in context
• Analyze PlaybackManager.handleRouteChanged + route-change policy
• Review new unit tests
• Post review feedback

This revision is a solid improvement over the first version I reviewed. The typed RouteChangeDecision policy, main-thread serialization, atomic shouldKeepPlaying handling, and unit coverage all address the earlier concerns well. I don't see any blocking issues. Notes below are for consideration.

What's correct

• Typed, testable policy. routeChangeDecision(for:) as a pure static function is a clean refactor of the previous raw rawValue comparisons, and the new PlaybackRouteChangePolicyTests cover pause / restart / config-change / ignored reasons. Good separation of decision from side-effects.
• Main-thread serialization (PlaybackManager.swift:2323-2328) is a real improvement — updateAllNowPlayingData() and player mutation now always run on main, and the recursive re-dispatch runs synchronously when already on main (no redundant hop). AVAudioSession posts these on a secondary thread, so this is the right fix.
• Atomic state. Replacing shouldKeepPlaying.value = shouldKeepPlaying.value && !shouldPause with the explicit if shouldPause { … } / guard branches (EffectsPlayer.swift:353-367) removes the non-atomic read-modify-write. 👍
• Guard against pausing an intended-but-not-started player. The guard shouldKeepPlaying.value early-return correctly makes a restart-only event a no-op while paused/interrupted, which is the documented fix for the paired reason-8 teardown race.
• DefaultPlayer unaffected. DefaultPlayer.routeDidChange (DefaultPlayer.swift:314-318) still only acts on shouldPause == true, so the AVPlayer path is unchanged — the restart only affects EffectsPlayer, which is exactly the engine that gets torn down.

Points worth considering (non-blocking)

1. Audio hiccup in the common case still stands. .routeConfigurationChange fires for reasons where the engine is not torn down (sample-rate/channel tweaks), and the unconditional pause() + play() will now introduce a brief gap where previously nothing happened. The inline comment at EffectsPlayer.swift:364-365 explicitly rejects gating on engine?.isRunning because it "can race the engine shutdown." That rationale is reasonable — a redundant restart is harmless, whereas trusting a stale isRunning == true would miss a dead engine — so the safe-but-slightly-noisier choice is defensible. Just calling out that the trade-off is intentional and audible recovery vs. gap should be part of the hardware verification.

2. Engine-setup runs off the main thread, so serialization is partial. play() sets shouldKeepPlaying.value = true synchronously, but the actual AVAudioEngine construction happens on DispatchQueue.global() (EffectsPlayer.swift:81). Main-thread serialization of handleRouteChanged therefore does not serialize against an in-flight engine build — a restart could still fire mid-setup. playerLock presumably guards the underlying state, so this is likely fine in practice, but it's the residual window that the main-thread fix does not close. Worth keeping in mind if the paired reason-8 issue resurfaces on device.

3. Minor: the FileLog line is only emitted for .routeConfigurationChange; the other restart reasons (newDeviceAvailable/override/categoryChange) get no equivalent trace. Since Bluetooth debugging is the motivation here, logging the reason in all restart cases would make future field diagnosis easier. Optional.

Testing

The policy function has good unit coverage. The behavioral parts (thread hop, EffectsPlayer restart, the paired reason-8 race) remain hardware-specific and can't be reproduced in the simulator, as the PR notes — so the MEDIUM-HIGH confidence rating is appropriate and manual verification on the affected A2DP head unit is the real gate.

Verdict: Looks good to merge pending device testing. The considerations above are optional refinements, not required changes.

[claude]

Claude finished @joashrajin's task in 2m 22s —— View job

---

Review: PR #4834 (synchronize) — Bluetooth .routeConfigurationChange fix

• Read updated diff vs origin/trunk
• Analyze new play() setup guard (da6276c)
• Re-check EffectsPlayer.routeDidChange + RouteChangeDecision policy
• Trace state safety (shouldKeepPlaying / aboutToPlay / playerLock)
• Post review

The new commit — "Address review: Cancel stale EffectsPlayer setup" — is a good, correct addition. It closes the exact race the PR body describes and I don't see any blocking issues. Details below.

What's correct in the new commit

• Stale-setup guard is sound. play() sets shouldKeepPlaying.value = true synchronously (EffectsPlayer.swift:79) before dispatching engine construction to DispatchQueue.global(). The new recheck under playerLock (EffectsPlayer.swift:85-90) means that if a pause()/teardown lands after the dispatch but before the closure acquires the lock, the setup bails instead of starting an orphaned engine. shouldKeepPlaying is an AtomicBool, so the cross-thread read is safe.
• No leaked aboutToPlay state. The early bail doesn't reset aboutToPlay, but that's fine: every path that flips shouldKeepPlaying to false — pause() (:211-212) and endPlayback() (:299-300) — also clears aboutToPlay before the guard would trip. So isPlaying() (:68) can't get stuck returning true.
• Consistent with existing bail paths. The pre-existing error returns in play() (:137, :172, :178, :190) also skip completion?(), so the new early return doesn't introduce a new "completion never fires" contract violation. In the restart flow play(userInitiated: false) is called with no completion anyway.
• Residual mid-setup window is bounded. If teardown arrives after the guard passes (engine mid-construction, lock held), endPlayback() (:296, defer unlock) blocks on playerLock until construction finishes, then tears the engine down — so a permanent cleanup still cleans up an engine that started inside the window.

Carry-over notes (unchanged, still non-blocking)

1. Audio hiccup in the common case. .routeConfigurationChange → .restart(updateNowPlaying: false) unconditionally does pause()+play(), so config changes that don't tear the engine down (sample-rate/channel tweaks) now get a brief gap where nothing happened before. The inline comment at EffectsPlayer.swift:370-371 explains why gating on engine?.isRunning was deliberately avoided (it can race the shutdown), so the safe-but-noisier choice is defensible — just something to confirm audibly during the hardware pass.
2. Off-main engine build. Main-thread serialization of handleRouteChanged doesn't serialize against the in-flight DispatchQueue.global() engine build; playerLock is what actually protects that state. The new guard narrows this window but a pause() (which doesn't take the lock) landing between the guard and engine.start() can still produce a short-lived engine — cleaned up by the lock-holding endPlayback() as noted above. Worth remembering if the paired reason-8 issue resurfaces on device.

Testing

routeChangeDecision(for:) has solid unit coverage (pause / restart / config-change / ignored reasons). The behavioral parts — thread hop, EffectsPlayer restart, the paired reason-8 race, and this new stale-setup guard — are inherently hardware-specific (A2DP renegotiation) and can't be reproduced in the simulator, so MEDIUM-HIGH confidence is right and manual verification on the affected head unit remains the real gate.

Verdict: The new commit is a clean, correct hardening of the race. Looks good to merge pending device testing; the two carry-over notes are optional refinements, not required changes.

[joashrajin]

Review feedback addressed

• In-flight engine construction: Added a playback-intent recheck after acquiring playerLock, so queued setup work cannot start an orphaned engine after teardown — done in da6276c.
• engine.isRunning gate: Not added. Route and engine-configuration notifications have no documented ordering, so a stale true result could miss the stopped engine and recreate the original silent-playback failure. The conservative rebuild remains intentional.
• Restart logging: No change needed. logRouteChange already logs every valid reason and both routes; the extra reason-8 message specifically records the new recovery action.
• Now Playing refresh: The omission is explicit in RouteChangeDecision as .restart(updateNowPlaying: false) and covered by PlaybackRouteChangePolicyTests.

Validation: make format, make lint, and git diff --check pass. Hardware verification remains the gate for the audible recovery-versus-gap trade-off.

[claude]

Claude finished @joashrajin's task in 2m 45s —— View job

---

Review: PR #4834 (synchronize) — Bluetooth .routeConfigurationChange fix

• Read updated diff (through ab19132) against the trunk baseline (ea74cb9)
• Analyze EffectsPlayer restart + stale-setup guard
• Analyze PlaybackManager.handleRouteChanged / processRouteChange + route-change policy
• Review unit tests
• Post review feedback

The two latest commits — "Cancel stale EffectsPlayer setup" (da6276c) and "Clarify route recovery behavior" (ab19132) — are clean, correct refinements. I verified the refactor is behavior-preserving against the trunk baseline and don't see any blocking issues.

Verified correct

• Refactor is faithful. I diffed routeChangeDecision(for:) against the old raw-rawValue chain on ea74cb9: .oldDeviceUnavailable → .pause, .newDeviceAvailable/.override/.categoryChange → .restart(updateNowPlaying: true) are identical to before, and .routeConfigurationChange → .restart(updateNowPlaying: false) is the only added case. No existing reason changed behavior, and the previous/current UID comparison was correctly dropped (reason 8 means the port set is unchanged by definition). default → nil preserves the old "do nothing" fallthrough.

• routeDidChange guard preserves the old condition. The old shouldKeepPlaying.value = shouldKeepPlaying.value && !shouldPause + if shouldKeepPlaying.value && !interruptionInProgress() collapses to the new guard shouldKeepPlaying.value, !PlaybackManager.shared.interruptionInProgress() else { return } for the shouldPause == false path — same effective gate, now without the non-atomic read-modify-write. The shouldPause == true branch still permanently pauses, matching .oldDeviceUnavailable. 👍

• Paired reason-8 race is closed at the design level. The shouldPause: false path always ends in pause() + play() — there is no code path where a restart-intended event resolves to a permanent pause. Combined with the guard shouldKeepPlaying.value early-return, a duplicate event landing on a not-yet-started replacement is now a no-op instead of a teardown. The final state is always "playing," which is exactly what the bug needed.

• Stale-setup guard is sound. play() sets shouldKeepPlaying.value = true synchronously (EffectsPlayer.swift:79), then the global-queue closure rechecks it under playerLock (:88). A pause()/endPlayback() that lands after the dispatch but before the closure acquires the lock now bails instead of starting an orphaned engine. shouldKeepPlaying is AtomicBool, so the cross-thread read is safe, and both flag-flipping paths (pause() :212-213, endPlayback() :300-301) also clear aboutToPlay, so playing() can't get stuck returning true after a bail.

• Thread + snapshot handling improved. handleRouteChanged now snapshots currentRoute.outputs on AVAudioSession's delivery thread before the main-thread hop (:2323), so rapid route churn can't swap the outputs recorded for this notification — a genuine improvement over reading currentRoute on main. The recursive processRouteChange hop runs synchronously when already on main (no redundant dispatch).

• DefaultPlayer unaffected. DefaultPlayer.routeDidChange still only acts on shouldPause == true, so the AVPlayer path is unchanged; the restart only touches EffectsPlayer, which is the engine that actually gets torn down.

• Tests match the policy. PlaybackRouteChangePolicyTests cover pause / restart+NowPlaying / config-change-without-NowPlaying / ignored reasons against the Equatable enum — good separation of decision from side-effects.

Standing trade-offs (non-blocking, hardware-gated)

1. Audio hiccup in the benign case. .routeConfigurationChange also fires for reconfigurations that don't tear the engine down (sample-rate/channel tweaks); those now get an unconditional pause()+play() where nothing happened before. The inline comment at EffectsPlayer.swift:371-372 explains why gating on engine?.isRunning was deliberately avoided (route and engine-configuration notifications have no documented ordering, so a stale true could miss a dead engine and re-create PCIOS-868). The conservative rebuild is the defensible choice — just confirm the recovery-vs-gap trade-off is acceptable audibly during the device pass.

2. Residual mid-setup window. Main-thread serialization of processRouteChange does not serialize against the in-flight DispatchQueue.global() engine build; playerLock is what protects that state. A pause() (which doesn't take the lock) landing after the new guard passes but before engine.start() can still briefly start an engine — but the lock-holding endPlayback() blocks until construction finishes and then tears it down, so it's short-lived, not leaked. Worth remembering if the paired reason-8 issue resurfaces on device.

Testing

The policy function has solid unit coverage. The behavioral parts — thread hop, EffectsPlayer restart, the paired reason-8 race, and the stale-setup guard — are inherently A2DP-renegotiation-specific and can't be reproduced in the simulator, so the MEDIUM-HIGH confidence rating is appropriate and manual verification on the affected head unit remains the real gate.

Verdict: The latest commits are a clean, correct hardening. Looks good to merge pending device testing; the two notes above are optional/awareness items, not required changes.

[joashrajin]

Additional review feedback addressed

• PR description: Removed the draft-history UID-comparison claim, narrowed the test-coverage wording, and documented accepted trade-offs.
• Rebuild logging: Moved the action log into EffectsPlayer, so DefaultPlayer and GoogleCastPlayer no longer appear to rebuild — done in ab19132.
• Cancelled setup completion: Kept intentionally silent and documented in code. The callback represents a successful start; invoking it after cancellation could seek or pause a replacement player.
• Main-thread recovery: Kept intentionally serialized. Activation is backgrounded by default, and delayed deactivation is cancelled by the immediate restart; moving state mutations off-main would reopen the race.
• Test diagnostics: Added the route raw value to looped assertion failures — done in ab19132.
• Route log timing: Snapshot current output names before the main-thread hop so rapid route churn cannot change the diagnostic record — done in ab19132.

Validation: make format, make lint, and git diff --check pass. The focused test remains blocked before compilation because the local Xcode installation lacks the watchOS 26.5 simulator runtime.