Prevent stale Cast resumes from breaking local playback

[joashrajin]

Description

A failed Cast SDK saved-session resume was routed through the same error path as a user-initiated Cast start. When the saved receiver was unavailable, the callback changed healthy local playback to an error state, removing the media notification and breaking lock-screen, headphone, and Android Auto controls.

This change preserves whether a Cast session failure came from an explicit start or an automatic resume. Resume failures are ignored only while local playback is active, while explicit Cast start failures and failures affecting an installed Cast player remain visible.

Fixes #5248
Fixes PCDROID-554

Testing Instructions

1. Cast an episode to a receiver.
2. Leave the Cast session without tapping Stop Casting, then make the receiver unavailable.
3. Start local playback and bring Pocket Casts to the foreground.
4. Wait for the saved Cast session resume attempt to time out.
5. Verify local playback continues without an error toast or banner.
6. Verify the media notification remains visible and lock-screen, headphone, and Android Auto controls still work.
7. Start a new Cast connection to an unavailable receiver and verify the explicit connection failure is still shown.

Automated validation:

JAVA_HOME='/Applications/Android Studio.app/Contents/jbr/Contents/Home' ./gradlew spotlessCheck :modules:services:repositories:testDebugUnitTest

Screenshots or Screencast

Not applicable — no UI changes.

Checklist

• If this is a user-facing change, I have added an entry in CHANGELOG.md
• Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
• I have 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 have updated (or requested that someone edit) the Event Horizon schema to reflect any new or changed analytics.

I have tested any UI changes...

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

[claude]

Claude finished @joashrajin's task in 1m 25s —— View job

---

Code Review

I reviewed the diff against origin/main. This is a clean, well-scoped fix. The core idea — threading a SessionFailureType through CastManager.SessionListener.sessionFailed and only tearing down local playback for failures that actually matter — directly addresses the root cause in #5248 without broadening behavior. The extraction of shouldSurfaceCastSessionFailure as an internal top-level function is a nice touch for testability.

✅ What's good

• Backward-compatible interface change. sessionFailed keeps a default empty body, and the only two call sites (onSessionStartFailed → START, onSessionResumeFailed → RESUME) and the sole implementer (PlaybackManager) are all updated. No other implementers exist (confirmed via search), so nothing silently breaks.
• Preserves prior behavior for the risky path. START failures always surface, so an explicit user-initiated Cast connection failure still shows the toast/error exactly as before. Only automatic cold resumes are suppressed, and only while a Cast player isn't active.
• Correct guard for null/local player. player?.isRemote == true means a RESUME failure with no player, or with a local player, is treated as "not a Cast player" and ignored — which is the desired outcome (no active remote playback to fail).
• Accurate log message. The "during local playback" branch is only reachable for RESUME + isCastPlayerActive == false, so the wording is truthful.

🔎 Observations / minor suggestions

1. Missing CHANGELOG entry. This is a user-facing bug fix (broken media notification / lock-screen / Android Auto controls), and the checklist item is unchecked. The 8.18 → Bug Fixes section already exists — worth adding an entry there. Fix this →

2. Thread-safety of player?.isRemote. The read happens synchronously inside the Cast callback (main executor), while the actual error handling is dispatched via launch(Dispatchers.Main). Reading player on the main executor is consistent with how player is accessed elsewhere in this class, so this is fine — just flagging that the decision is made on the callback thread rather than inside the Main coroutine, which is intentional and correct here.

3. Test coverage is limited to the pure helper. CastSessionFailureTest exercises the three branches of shouldSurfaceCastSessionFailure, which is the right unit to test given PlaybackManager's size. It does mean the integration (that the ignored branch actually skips the PlaybackState.ERROR transition and toast) isn't asserted. That's a reasonable trade-off; no action required, but worth noting the behavioral contract lives outside the test.

4. Edge case, informational only. The RESUME + isCastPlayerActive == true case (surface the error) appears rare in practice, since a resume that reaches onSessionResumeFailed typically hasn't switched player to the remote CastPlayer yet (castReconnected() only runs on onSessionResumed). The guard is still correct and safe as a defensive branch — just don't expect it to fire often.

Verdict

Looks good to merge. The only thing I'd genuinely recommend addressing is the CHANGELOG entry; the rest are observations.

[claude]

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

---

Code Review

• Read the full diff vs origin/main
• Review CastManager / CastManagerImpl changes
• Review PlaybackManager changes
• Review test coverage
• Post findings

I re-reviewed the current state of the branch against origin/main. This is a well-scoped, correct fix that addresses the root cause of #5248 without broadening behavior. The CHANGELOG entry flagged in the earlier review has now been added. 👍

✅ What's good

• Root cause addressed precisely. Threading SessionFailureType through sessionFailed and gating teardown on shouldSurfaceCastSessionFailure means only automatic cold resumes while local playback is active are suppressed. Explicit START failures still surface exactly as before, preserving the user-initiated connection error path.
• Interface change is safe. sessionFailed keeps a default empty body, both call sites in CastManagerImpl (onSessionStartFailed → START, onSessionResumeFailed → RESUME) are updated, and PlaybackManager is the only implementer (confirmed via search) — nothing silently breaks.
• Correct null/local guard. player?.isRemote == true treats a RESUME failure with no player or a local player as "not casting" → ignored, which is the desired outcome. On a cold-start resume the process has no active remote playback to tear down, so suppressing is right.
• Pure helper extracted for testability. shouldSurfaceCastSessionFailure as an internal top-level function is clean, and CastSessionFailureTest covers all three branches.
• Truthful log message. The "during local playback" wording is only reachable for RESUME + isCastPlayerActive == false, so it's accurate.

🔎 Minor observations (non-blocking)

1. Integration behavior isn't asserted. The tests exercise the pure helper, but not that the ignored branch actually skips the PlaybackState.ERROR transition + toast inside PlaybackManager.sessionFailed. Given PlaybackManager's size this is a reasonable trade-off, but the behavioral contract lives outside the test. No action required.
2. player?.isRemote read on the callback thread. The decision is made synchronously on the Cast main-executor callback, while the error handling itself dispatches via launch(Dispatchers.Main). This matches how player is accessed elsewhere in the class, so it's fine — just noting the read isn't inside the coroutine.
3. RESUME + isCastPlayerActive == true is a defensive branch. player only becomes remote on onSessionResumed (success), so a path reaching onSessionResumeFailed with an already-remote player is rare in practice. The branch is still correct and safe as a guard — just don't expect it to fire often.

Verdict

Looks good to merge — no blocking issues. The earlier CHANGELOG recommendation is resolved; the remaining points are observations only.

[wpmobilebot]

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