[sztomek]

Description

Android 17 blocks background audio unless a foreground service is already running. We did it the other way round: we asked for audio focus and started the player first, and the service only went foreground after that. So background plays got their focus request denied, and we stopped playback.

This PR starts the foreground service before we ask for focus. It sits behind the FOREGROUND_BEFORE_PLAYBACK flag.

What's in here:

• A gate at the top of PlaybackManager.play(). It starts the service, waits for it to reach foreground, and only then do we ask for focus.
• Both services (Media3 and legacy) report their foreground state through a shared isServiceForeground signal.
• On Android 17, if the system refuses the service start, we give up and never ask for focus. Asking would fail anyway, kill playback, and log a hardening violation against us. Below Android 17 there's no suppression, so we fall through and play like before.
• Playback state goes back to paused when that happens, so the UI doesn't sit there showing "playing" with no sound.
• We no longer ask for local audio focus when casting. Before, a denied request killed Chromecast playback, even though no local audio is involved.
• The two confirmation tones now set audio attributes. They had none at all.

Testing Instructions

Tested on a Pixel 9 Pro on Android 17 (API 37). Hardening is already on by default there, so there's no setup.

One gotcha: adb shell cmd audio set-enable-hardening does nothing on a production build. It exits 0 but never changes anything. To test the stricter targetSdk 37 rules, build with targetSdkVersion = 37 instead.

1. Install, play an episode, then put the app in the background.
2. Pause with a media key, then play again. Audio should come back.
3. In logcat, the foreground service start should come before "Trying to gain audio focus".
4. Turn MEDIA3_SESSION off and repeat. This path matters more, because it drops the foreground service on pause.
5. Force-stop the app, then fire a background play broadcast. Playback should not start. Logcat should say "Foreground service start refused, abandoning playback", with no focus request and no "blocked by hardening".
6. Run adb shell dumpsys audio and look for AudioHardening lines with our package. There should be none.

What I checked:

• Media3 path and legacy path, both fine.
• targetSdk 36 and targetSdk 37, both fine.
• Before the fix, step 5 logged "Audio focus request blocked by hardening" and tore playback down. After, nothing.

Still not covered: Automotive, Wear, TV, and Chromecast on a real device.

One thing this can't fix: if the app process is cached, the system refuses the foreground service outright. We can't start one. We now fail cleanly instead of killing playback, but the audio still won't play.

Screenshots or Screencast

n/a, no UI changes.

Checklist

• If this is a user-facing change, I added an entry in CHANGELOG.md
• Ensure linter passes (./gradlew spotlessApply automatically applies formatting/linting)
• I considered whether it makes sense to add tests for my changes
• All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
• Any jetpack compose components I added or changed are covered by compose previews
• I updated (or requested someone edit) the Event Horizon schema to reflect any new or changed analytics.

I tested any UI changes...

• on different themes
• on landscape orientation
• on a device set to a large display font size
• for accessibility with TalkBack

[claude]

Claude finished @sztomek's task in 4m 25s —— View job

---

PR Review: Enforce using foreground service during playback

Solid, well-scoped change — the suspend gate + shared isServiceForeground signal is a clean way to guarantee FGS-before-focus across both the Media3 and legacy paths, and gating everything behind FOREGROUND_BEFORE_PLAYBACK with a fail-open fallback is the right call. Nice touch keeping the service foreground across auto-play switches via isSwitchingPlayer. A few things worth addressing before merge (details left as inline comments):

🔴 Correctness

1. Early promotion + isSwitchingPlayer not rolled back when play() returns early (PlaybackManager.kt:2261-2268). Once the gate promotes the service to the foreground (with the "Loading" notification) and resetPlayer() has set isSwitchingPlayer = true, a subsequent tryToGetAudioFocus() denial makes play() return without undoing either. Result: a stale "Loading" FGS notification lingers with no audio, and because both new notification-removal branches gate on !isSwitchingPlayer, the notification can never be removed on pause afterwards → leaked foreground service. Focus can still be denied after going foreground (another app refusing to yield), so this is reachable. This is my main concern. Fix this →

🟡 Worth verifying / polish

2. Manual startForeground() inside the Media3 MediaLibraryService (PlaybackService.kt:160-186). Media3 manages the service's foreground state itself; promoting out-of-band can desync its internal bookkeeping (service not demoted at end-of-playback, or the plain notification not being cleanly replaced by DefaultMediaNotificationProvider). Understandable to satisfy the startForegroundService() 5s contract — please confirm on-device the notification hand-off and end-of-playback stop still behave.

3. Inconsistent failure handling between the two services (LegacyPlaybackService.kt:139). Legacy calls stopSelf() unconditionally on early-start failure; the Media3 path only stops when there are no connected controllers. Align them so a failed early-start doesn't tear down a session that already has an active controller.

4. Duplicated logic. startForegroundEarly() in both services (notification build + ForegroundServiceStartNotAllowedException → setTimesToShowBatteryWarning(2 + …) + PlaybackForegroundServiceErrorEvent) is near-identical. Extracting the shared catch/battery-warning helper would prevent drift.

🟢 Minor notes

• ensureForegroundServiceStarted() can suspend up to timeoutMs (2s) before playback proceeds in the failure case. It's a suspend (non-blocking) wait so no ANR risk, but worth being aware of the added latency on the unhappy path.
• startServiceForeground() returns a Boolean that startServiceIfNeeded() ignores (fire-and-forget) — fine, just flagging the asymmetry with ensureForegroundServiceStarted() which does check it.
• Tests: good coverage of the gate's three states. Consider adding a case for the roll-back behavior in point 1 (focus denied → foreground demoted, isSwitchingPlayer cleared) once fixed, and something exercising the isSwitchingPlayer-across-switch path.

✅ Looks good

• Feature-flag definition, Free tier + Firebase remote flag + dev toggle all consistent with existing entries.
• Notification channel (NOTIFICATION_CHANNEL_ID_PLAYBACK) is created in PocketCastsApplication, so the cold-start startForegroundEarly() notification won't hit a missing-channel failure.
• Automotive and remote (Cast) players correctly excluded from the gate.

---

• Read changed files and understand the approach
• Review MediaSessionManager & PlaybackManager gating logic
• Review both service implementations (Media3 + legacy)
• Review NotificationDrawer changes
• Review feature flag & tests
• Post review feedback

[wpmobilebot]

Version 8.17 has now entered code-freeze, so the milestone of this PR has been updated to 8.18.

[wpmobilebot]

Version 8.18 has now entered code-freeze, so the milestone of this PR has been updated to 8.19.

[wpmobilebot]

Version 8.19 has now entered code-freeze, so the milestone of this PR has been updated to 8.20.

[wpmobilebot]

Version 8.20 has now entered code-freeze, so the milestone of this PR has been updated to 8.21.

[wpmobilebot]

Version 8.21 has now entered code-freeze, so the milestone of this PR has been updated to 8.22.

[claude]

Claude finished @sztomek's task in 1m 0s —— View job

---

Re-review (rebase push)

This push is a rebase onto current main — the five commits have new committer dates but identical content (diff is still +285/-7 and the blob SHAs match what I reviewed on 13 Jul). The only main commit touching these files since then is e2671e8 (episode details RxJava→coroutines), which doesn't interact with this change. Nothing new to review; all three findings from the previous pass are still open.

Blocking

• modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/PlaybackManager.kt:2480 — if tryToGetAudioFocus() is denied after the gate promoted the service, a stale "Loading" notification is left behind and isSwitchingPlayer stays true so the FGS can never be demoted; reset both on the focus-denied return path. (existing thread on this line)

Non-blocking

• modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/LegacyPlaybackService.kt:139 — unconditional stopSelf() on early-start failure can tear down a session with an active Bluetooth/Auto controller; guard it the way the Media3 path does. (existing thread)

• modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/PlaybackService.kt:186 — manual startForeground() inside a Media3 MediaLibraryService can desync Media3's own foreground bookkeeping; still needs on-device confirmation of the notification hand-off and end-of-playback stop. (existing thread)

• Diff PR against origin/main and compare with previously reviewed state

• Check whether earlier findings are still open

• Post summary
  • feat/android-17-audio-hardening

[claude]

Claude finished @sztomek's task in 1m 24s —— View job

---

Re-review (new commit since 21 Sep)

One new commit since my last pass: 01827d5, which sets AudioAttributes (USAGE_MEDIA + CONTENT_TYPE_SONIFICATION) on the bookmark and sleep-timer MediaPlayers. The call ordering is correct (set before setDataSource/prepare, AudioAttributes is immutable so sharing one instance across both players is fine) and the usage choice keeps the tones on STREAM_MUSIC, so no volume regression. Nothing blocking in the new commit; the three findings from the earlier passes are all still open in the current tree.

Blocking

• modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/PlaybackManager.kt:2492 — still open: if tryToGetAudioFocus() is denied after the gate promoted the service, the stale "Loading" notification is left behind and isSwitchingPlayer stays true so the FGS can never be demoted; reset both on the focus-denied return path. (existing thread)

Non-blocking

• modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/PlaybackManager.kt:206 — contentType alone likely doesn't exempt the tones from the Android 17 background-audio restrictions (those key off usage); confirm on-device or note the USAGE_MEDIA volume trade-off in the comment. (thread)
• modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/LegacyPlaybackService.kt:139 — still open: unconditional stopSelf() on early-start failure can tear down a session with an active Bluetooth/Auto controller; guard it the way the Media3 path does. (existing thread)
• modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/PlaybackService.kt:174 — still open: manual startForeground() inside a Media3 MediaLibraryService can desync Media3's own foreground bookkeeping; needs on-device confirmation of the notification hand-off and end-of-playback stop. (existing thread)

--- • feat/android-17-audio-hardening

[claude]

Claude finished @sztomek's task in 2m 40s —— View job

---

Re-review (rebase onto current main)

This push is a rebase — the five newer commits keep their content (187d8b3 is byte-identical to the previously reviewed 2e738c9), and the only real change is the conflict resolution in the two service commits: the PR now reports through main's PlaybackServiceErrorReporter (trackServiceStartFailed / trackForegroundStartFailed + resetFailureCount()) instead of the old direct eventHorizon.track(...) and setTimesToShowBatteryWarning(2 + …) bump. That resolution is correct: it matches the pattern in LegacyPlaybackService.handlePlaybackState and PlaybackService.onUpdateNotification, and dropping the battery-warning increment is right since BatteryWarningPolicy now owns that counter and decrements it. Nothing new blocking; only main commit touching these paths since the last pass is 05d0422 (bookmark swipe), which doesn't interact.

Non-blocking

• modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/LegacyPlaybackService.kt:140 — still open: unconditional stopSelf() on early-start failure can tear down a session with an active Bluetooth/Auto controller; guard it the way the Media3 path does. (thread)
• The in-service refusal still isn't plumbed into the gate (startForeground() throwing inside either service leaves the wait to time out to Unconfirmed) — noted as a deliberate follow-up in that thread, not re-raised.

Nits

• modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/MediaSessionManager.kt:616 — still open: the flag branch in startServiceIfNeeded is unreachable because its only caller (PlaybackManager.resetPlayer(), line 2641) already guards on the same flag; drop one of the two. (thread)
  • feat/android-17-audio-hardening

[geekygecko]

Could this notification be built in NotificationDrawer instead of inline? This PR already adds NotificationDrawer.buildPreparingNotification() for LegacyPlaybackService, so the two services currently build their preparing notification in different ways. An overload that takes the Media3 MediaSession and a title would keep them together, and it could use notificationHelper.playbackChannelBuilder() rather than hard-coding the channel ID.

[sztomek]

valid, addressed it!

[wpmobilebot]

Version 8.22 has now entered code-freeze, so the milestone of this PR has been updated to 8.23.

[claude]

Claude finished @sztomek's task in 1m 4s —— View job

---

Re-review (1 new commit)

The one new commit, 2b4e2c1, fixes both findings still open from the last pass, and it adds nothing blocking.

• Dead flag branch removed. The unreachable flag branch in MediaSessionManager.startServiceIfNeeded is gone. startServiceForeground is now only called from the ensureForegroundServiceStarted check in play(), so this resolves r4060976733.
• stopSelf() now guarded. In LegacyPlaybackService.startForegroundEarly(), a failed start only calls stopSelf() when nothing is playing. This is enough for r2205851994:
  • A bound Bluetooth/Auto controller keeps the service alive after stopSelf() anyway.
  • A new-episode play has already pushed PLAYING from loadCurrentEpisode() before play() waits for the foreground service, so the service is no longer killed underneath a playback that is about to go ahead.

Still not plumbed: a refusal thrown inside the service doesn't reach the check in play(). You already marked that as a follow-up, so I'm not raising it again.
• feat/android-17-audio-hardening

[wpmobilebot]

Version 8.23 has now entered code-freeze, so the milestone of this PR has been updated to 8.24.

[claude]

Claude finished @sztomek's task in 29s —— View job

---

Re-review (rebase + 1 new commit)

I found nothing new to flag, and none of the earlier findings are still open. This push is a rebase onto current main plus one new commit, 622b242. That commit fixes a gap I missed in earlier passes: on Automotive with the flag on, play() skips the foreground check and resetPlayer() skipped startServiceIfNeeded, so the playback service was never started. resetPlayer() now still calls startServiceIfNeeded on Automotive, which lines up with the !Util.isAutomotive(application) exclusion on the check in play() (PlaybackManager.kt:2539 / :2657). Cast playback isn't affected, because both changes sit in the SimplePlayer branch. A refusal thrown inside the service still doesn't reach the check in play(). That's the follow-up you already agreed to, so I'm not raising it again.
• feat/android-17-audio-hardening

[claude]

Claude finished @sztomek's task in 28s —— View job

---

Re-review (1 new commit)

The one new commit since my last pass, 82f2339, adds nothing to flag and leaves no earlier findings open. It moves the Media3 preparing notification into NotificationDrawer.buildPreparingNotification(session, title), as requested in geekygecko's comment, and builds it with notificationHelper.playbackChannelBuilder(). Those are the same channel, title fallback, icon and style as before, so it doesn't change behaviour. NotificationDrawerImpl is the only implementation and it's bound in RepositoryModule, so the new injection in PlaybackService also resolves for AutoPlaybackService.
• feat/android-17-audio-hardening