Skip to content

Improve playback error handling and retry logic#4053

Draft
SergioEstevao wants to merge 6 commits intotrunkfrom
sergio/improve_error_handling
Draft

Improve playback error handling and retry logic#4053
SergioEstevao wants to merge 6 commits intotrunkfrom
sergio/improve_error_handling

Conversation

@SergioEstevao
Copy link
Contributor

@SergioEstevao SergioEstevao commented Mar 6, 2026

📘 Part of: # |
:---:

Fixes PCIOS-556

This PR improves error handling during playback in two ways:

  • Fix retry playback loop: Reset haveCalledPlayerLoad to false before retrying playback when a URL fails to load, so the player correctly reloads instead of skipping the load step.
  • Error detection on full screen player: Subscribe to playbackFailed notifications in NowPlayingPlayerItemViewController and add an updateError() method that reads and logs playback/download error details from the current episode.

To test

  1. Open an episode that has a known bad/expired stream URL.
  2. Observe that the player attempts to reload/retry instead of silently failing.
  3. Open the full screen player during a playback error and verify the error details are logged.

Checklist

  • I have considered if this change warrants user-facing release notes and have added them to CHANGELOG.md if necessary.
  • I have considered adding unit tests for my changes.
  • I have updated (or requested that someone edit) the spreadsheet to reflect any new or changed analytics.

This is need to ensure that streaming playback can start again from zero.
@SergioEstevao SergioEstevao requested a review from a team as a code owner March 6, 2026 16:11
@SergioEstevao SergioEstevao requested review from bjtitus and Copilot and removed request for a team March 6, 2026 16:11
@SergioEstevao SergioEstevao added this to the 8.8 milestone Mar 6, 2026
@SergioEstevao SergioEstevao marked this pull request as draft March 6, 2026 16:13
@dangermattic
Copy link
Collaborator

dangermattic commented Mar 6, 2026

1 Warning
⚠️ View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.
1 Message
📖 This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves playback failure recovery and surfaces playback/download error details in the full-screen Now Playing UI to aid debugging.

Changes:

  • Reset haveCalledPlayerLoad before attempting a playback retry after a URL-load failure.
  • Subscribe the full-screen player to playbackFailed notifications.
  • Add updateError() to read and log playback/download error details from the current episode.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
podcasts/PlaybackManager.swift Adjusts retry behavior after stream URL load failure by resetting internal “player loaded” state.
podcasts/NowPlayingPlayerItemViewController+Update.swift Hooks full-screen Now Playing updates into playback failure events and logs stored error details.
Comments suppressed due to low confidence (1)

podcasts/PlaybackManager.swift:2278

  • urlFailedToLoad runs inside an unstructured Task, and ServerPodcastManager.updatePodcastIfRequired invokes its completion on subscribeQueue (background). This means haveCalledPlayerLoad and load(episode:autoPlay:...) are being touched off the main thread, which can race with playback state and UI-driven calls into PlaybackManager. Consider hopping to MainActor (or a dedicated serial queue) before mutating PlaybackManager state and before calling load.
            haveCalledPlayerLoad = false
            FileLog.shared.addMessage("PlaybackManager: URL failed to load, trying to update episode and playing again")
            lastRetryEpisodeUuid = episodeUuid

            ServerPodcastManager.shared.updatePodcastIfRequired(podcast: podcast) { [weak self] wasUpdated in

updateChapterInfo()
updateChapterProgress()
updateColors()
updateError()
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constants.Notifications.playbackFailed is wired to update(), and update() now calls updateError(). This can cause the same error details to be logged repeatedly on unrelated update triggers (play/pause, chapter updates, foreground, etc.) and does a full UI refresh on playback-failed events. Consider observing playbackFailed with a dedicated handler that only invokes updateError() (and/or remove updateError() from the general update() path).

Suggested change
updateError()

Copilot uses AI. Check for mistakes.
Comment on lines +177 to +186
var errorMessage = ""
if let playbackError = playingEpisode.playbackErrorDetails {
errorMessage = playbackError
}
if !errorMessage.isEmpty {
print("Error: \(errorMessage)")
}

if let downloadError = playingEpisode.downloadErrorDetails {
print("Error: \(downloadError)")
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updateError() uses print(...) for logging. In this codebase, playback-related logging appears to go through FileLog.shared.addMessage, and print can be noisy in production and hard to collect from real devices. Consider switching to the existing logging facility and include context (episode UUID/title, whether it’s playback vs download) to make the log actionable.

Suggested change
var errorMessage = ""
if let playbackError = playingEpisode.playbackErrorDetails {
errorMessage = playbackError
}
if !errorMessage.isEmpty {
print("Error: \(errorMessage)")
}
if let downloadError = playingEpisode.downloadErrorDetails {
print("Error: \(downloadError)")
let episodeTitle = playingEpisode.displayableTitle()
if let playbackError = playingEpisode.playbackErrorDetails, !playbackError.isEmpty {
FileLog.shared.addMessage("Playback error for episode '\(episodeTitle)': \(playbackError)")
}
if let downloadError = playingEpisode.downloadErrorDetails, !downloadError.isEmpty {
FileLog.shared.addMessage("Download error for episode '\(episodeTitle)': \(downloadError)")

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings March 13, 2026 17:22
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comment on lines +175 to +187
func updateError() {
guard let playingEpisode = PlaybackManager.shared.currentEpisode() else { return }
var errorMessage = ""
if let playbackError = playingEpisode.playbackErrorDetails {
errorMessage = playbackError
}
if !errorMessage.isEmpty {
print("Error: \(errorMessage)")
}

if let downloadError = playingEpisode.downloadErrorDetails {
print("Error: \(downloadError)")
}
Comment on lines +381 to +387
<view contentMode="scaleToFill" translatesAutoresizingMaskIntoConstraints="NO" id="Owh-jf-YTg" userLabel="Error View">
<rect key="frame" x="0.0" y="796" width="414" height="100"/>
<subviews>
<label opaque="NO" userInteractionEnabled="NO" contentMode="left" horizontalHuggingPriority="251" verticalHuggingPriority="251" text="Error Message" textAlignment="center" lineBreakMode="tailTruncation" baselineAdjustment="alignBaselines" adjustsFontForContentSizeCategory="YES" adjustsFontSizeToFit="NO" translatesAutoresizingMaskIntoConstraints="NO" id="96b-yr-mZE">
<rect key="frame" x="8" y="8" width="398" height="16"/>
<fontDescription key="fontDescription" style="UICTFontTextStyleBody"/>
<nil key="textColor"/>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants