✨ Add auto-resume, stuck detection, and error recovery to native player#23
✨ Add auto-resume, stuck detection, and error recovery to native player#23williamchong merged 3 commits intolikecoin:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds resilience features to the native (expo-audio) bridge to better recover from interruptions and playback stalls, aligning native behavior with the web player’s robustness goals.
Changes:
- Introduces auto-resume logic for unexpected pauses with bounded retries.
- Adds “stuck” detection via a 5s timeout with one retry, then emits an error.
- Resets/cleans up new timers and state flags across load/resume/stop/teardown paths.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
services/audio-bridge.native.ts
Outdated
| const track = queue[currentIndex]; | ||
| if (!track) return; | ||
| p.replace({ uri: track.uri, headers: track.headers }); | ||
| activatePlayer(p, track); | ||
| stuckTimer = setTimeout(() => { |
There was a problem hiding this comment.
In the stuck-retry path you call replace() and then activatePlayer() without pausing first. Earlier in playTrack() there’s an explicit comment that pausing before replace() is required to avoid expo-audio’s internal auto-resume race and incorrect end-notification registration. The stuck-retry should follow the same pause→replace→play sequence (or reuse playTrack()/a shared helper) to avoid reintroducing that failure mode.
| // The `audible` guard ensures this only fires on the playing→paused transition, | ||
| // not on every subsequent paused status tick. | ||
| if (state === 'paused' && audible && !userPaused && active && !errored | ||
| && !autoResumeTimer && !status.didJustFinish) { |
There was a problem hiding this comment.
armStuckTimer() remains armed across an OS interruption where playback transitions to paused unexpectedly. Because audible is set to false on the paused transition, the stuck timer can fire ~5s later and treat the interruption as “stuck”, triggering a full replace() retry while the auto-resume logic is also running. Consider clearing the stuck timer when you detect the unexpected pause, and only re-arming it when you explicitly call play() (auto-resume/manual resume), or gate the stuck timer on buffering/“attempting to play” state rather than just !audible.
| && !autoResumeTimer && !status.didJustFinish) { | |
| && !autoResumeTimer && !status.didJustFinish) { | |
| // An OS interruption has paused playback while a stuck timer may still be armed. | |
| // Clear the stuck timer here so it doesn't fire and conflict with auto-resume. | |
| clearStuckTimer(); |
Port three resilience features from the web audio player: - Auto-resume after OS interruption (phone call, Siri) with 3 retries - Stuck detection with 5s timeout, retry once then error - Errored guard to prevent auto-resume/stuck retry on failed players Also fixes: handleSkipTo sets active/userPaused flags, handleResume clears stale timers and resets retry budgets, teardown cleans up all timer and flag state.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
services/audio-bridge.native.ts
Outdated
| if (audible || !active || stuckRetried || errored) return; | ||
| console.warn('Audio stuck — retrying playback'); | ||
| stuckRetried = true; | ||
| const p = getActivePlayer(); | ||
| if (!p) return; | ||
| const track = queue[currentIndex]; | ||
| if (!track) return; |
There was a problem hiding this comment.
armStuckTimer() logs "Audio stuck — retrying playback" and flips stuckRetried = true before confirming there is an active track to retry (queue[currentIndex]). If handleResume() is called with no loaded queue/track (e.g., after handleStop()), this will emit a misleading warning and permanently consume the one retry. Consider checking currentIndex/track (and p) first, and only then logging and setting stuckRetried.
| if (audible || !active || stuckRetried || errored) return; | |
| console.warn('Audio stuck — retrying playback'); | |
| stuckRetried = true; | |
| const p = getActivePlayer(); | |
| if (!p) return; | |
| const track = queue[currentIndex]; | |
| if (!track) return; | |
| const p = getActivePlayer(); | |
| const track = queue[currentIndex]; | |
| if ( | |
| audible || | |
| !active || | |
| stuckRetried || | |
| errored || | |
| !p || | |
| !track | |
| ) { | |
| return; | |
| } | |
| console.warn('Audio stuck — retrying playback'); | |
| stuckRetried = true; |
services/audio-bridge.native.ts
Outdated
| activatePlayer(p, track); | ||
| stuckTimer = setTimeout(() => { | ||
| stuckTimer = null; | ||
| if (audible || !active || errored) return; | ||
| console.warn('Audio stuck — retry failed'); | ||
| errored = true; | ||
| notifyWebView?.({ type: 'error', message: 'Playback stuck' }); | ||
| }, STUCK_TIMEOUT_MS); |
There was a problem hiding this comment.
armStuckTimer() schedules a second timeout for the "retry failed" path without any generation/guarding against concurrent handlePause()/handleStop() calls. If the first timeout callback is already executing when a pause/stop clears the timer, this code can still schedule a new stuckTimer that won't be cleared until it fires. Consider adding an attempt/generation id (similar to preload.resetCount), or re-checking active/errored right before scheduling the inner timeout and bailing without scheduling when playback is no longer active.
services/audio-bridge.native.ts
Outdated
| errored = false; | ||
| stuckRetried = false; | ||
| getActivePlayer()?.play(); | ||
| armStuckTimer(); |
There was a problem hiding this comment.
handleResume() unconditionally calls armStuckTimer(), even when no track is loaded (currentIndex === -1 / queue empty). That can trigger the stuck handler and warnings despite nothing being playable. Consider only arming the stuck timer when there is a valid current track (and ideally when the player has a source loaded).
| armStuckTimer(); | |
| if (currentIndex >= 0 && currentIndex < queue.length) { | |
| armStuckTimer(); | |
| } |
| } | ||
| const headers = cookieHeader ? { Cookie: cookieHeader } : undefined; | ||
|
|
||
| if (!msg.tracks.length || msg.startIndex < 0 || msg.startIndex >= msg.tracks.length) return; |
There was a problem hiding this comment.
doLoad() now returns early when tracks is empty or startIndex is out of range, but it leaves any previous queue/currentIndex and active playback state intact. If the web layer ever sends a malformed load message, native can keep playing stale content while the web UI thinks a new load occurred. Consider either notifying the web view with an error or explicitly calling handleStop()/resetting state before returning.
| if (!msg.tracks.length || msg.startIndex < 0 || msg.startIndex >= msg.tracks.length) return; | |
| if (!msg.tracks.length || msg.startIndex < 0 || msg.startIndex >= msg.tracks.length) { | |
| notifyWebView?.({ | |
| type: 'loadError', | |
| reason: 'InvalidLoadMessage', | |
| tracksLength: msg.tracks.length, | |
| startIndex: msg.startIndex, | |
| }); | |
| return; | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| autoResumeRetries += 1; | ||
| autoResumeTimer = setTimeout(() => { | ||
| autoResumeTimer = null; | ||
| if (active && !audible) { | ||
| getActivePlayer()?.play(); | ||
| } | ||
| }, 1000); |
There was a problem hiding this comment.
Auto-resume is intended to retry up to MAX_AUTO_RESUME_RETRIES, but this logic only schedules a single retry per playing→paused transition. After the first unexpected pause, audible is set to false and subsequent paused status updates won't trigger additional retries, so retries >1 will never happen if the interruption persists longer than the first 1s delay. Consider implementing a retry loop that re-schedules another attempt while still paused (until playing is observed or the retry budget is exhausted).
| autoResumeRetries += 1; | |
| autoResumeTimer = setTimeout(() => { | |
| autoResumeTimer = null; | |
| if (active && !audible) { | |
| getActivePlayer()?.play(); | |
| } | |
| }, 1000); | |
| // Schedule a retry loop that will keep attempting auto-resume | |
| // while the interruption persists and the retry budget is not exhausted. | |
| autoResumeRetries += 1; | |
| const scheduleAutoResumeAttempt = () => { | |
| autoResumeTimer = setTimeout(function autoResumeAttempt() { | |
| autoResumeTimer = null; | |
| // Only attempt auto-resume if playback is still interrupted by the OS | |
| // (not user-paused, not errored, still active, and not yet audible). | |
| if (!active || audible || userPaused || errored) { | |
| return; | |
| } | |
| const player = getActivePlayer(); | |
| if (player) { | |
| player.play(); | |
| } | |
| // If we still have retries left and the interruption persists, | |
| // schedule another attempt. | |
| if (autoResumeRetries < MAX_AUTO_RESUME_RETRIES) { | |
| autoResumeRetries += 1; | |
| scheduleAutoResumeAttempt(); | |
| } | |
| }, 1000); | |
| }; | |
| scheduleAutoResumeAttempt(); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| autoResumeTimer = setTimeout(() => { | ||
| autoResumeTimer = null; | ||
| if (active && !audible && !userPaused && !errored) { | ||
| audible = true; // re-arm so next paused status can trigger another retry |
There was a problem hiding this comment.
audible is used as a proxy for “actually playing” (it gates stuck detection and unexpected-pause detection), but here it is set to true inside the auto-resume timer before playback is confirmed. If play() is a no-op/fails to transition to playing, this can suppress stuck detection and can also make later paused ticks look like a fresh playing→paused transition. Consider keeping audible strictly derived from status (state === 'playing') and using a separate flag/previous-state tracker for the transition/retry bookkeeping.
| audible = true; // re-arm so next paused status can trigger another retry | |
| // Attempt to resume playback; `audible` will be updated on the next | |
| // status callback if playback actually transitions to `playing`. |
| // Detect unexpected pause (OS interruption: phone call, Siri, other app). | ||
| // The `audible` guard ensures this only fires on the playing→paused transition, | ||
| // not on every subsequent paused status tick. | ||
| if (state === 'paused' && audible && !userPaused && active && !errored | ||
| && !autoResumeTimer && !status.didJustFinish) { | ||
| audible = false; |
There was a problem hiding this comment.
The “audible guard” is only cleared in the unexpected-pause branch. If playback reaches paused for reasons that bypass this branch (e.g., didJustFinish is true, or userPaused/active gates the branch), audible can remain true while paused, and subsequent paused status updates (often with didJustFinish no longer true) may incorrectly trigger the auto-resume loop and restart a finished track. Consider resetting audible whenever state !== 'playing' (or tracking previous state explicitly) so the retry logic only fires on a real playing→paused transition.
- Increase stuck timeout from 5s to 15s for iOS blocking=1 URLs where the server generates full TTS audio before responding - Use play() instead of replace() for stuck retry so we don't nuke a nearly-ready buffer on slow connections - Don't arm stuck timer on handleResume (source already loaded) - Reset lastFinishTime on skipTo to prevent dropping ended events for back-to-back short tracks within the 500ms dedup window
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
services/audio-bridge.native.ts
Outdated
| resetIdle(); | ||
| active = false; | ||
| userPaused = false; | ||
| audible = false; |
There was a problem hiding this comment.
The unregister/teardown function resets active/userPaused/audible but leaves other added state (autoResumeRetries, stuckRetried, errored) unchanged. If listeners are re-registered in the same JS runtime (e.g. WebView reload), stale state can affect auto-resume/stuck logic. Reset those variables here as well to fully clean up state.
| audible = false; | |
| audible = false; | |
| autoResumeRetries = 0; | |
| stuckRetried = false; | |
| errored = false; |
| const MAX_AUTO_RESUME_RETRIES = 3; | ||
| // Longer than web player's 5s because iOS uses blocking=1 URLs where the | ||
| // server generates the full TTS audio before responding. | ||
| const STUCK_TIMEOUT_MS = 15000; | ||
| let active = false; |
There was a problem hiding this comment.
PR description mentions a 5s stuck timeout, but STUCK_TIMEOUT_MS is set to 15000ms (and the code waits up to 2× this before emitting "Playback stuck"). Either update the PR description (and any downstream assumptions), or make the timeout configurable / platform-specific so the behavior matches the stated 5s target where appropriate.
services/audio-bridge.native.ts
Outdated
| export function handleStop(): void { | ||
| active = false; | ||
| userPaused = false; | ||
| audible = false; |
There was a problem hiding this comment.
handleStop() clears timers but doesn’t reset the new resilience state (autoResumeRetries, stuckRetried, errored). If the WebView issues a resume/play without a full reload, these latched values can incorrectly suppress retries or stuck detection. Consider resetting those counters/flags here alongside active/userPaused/audible.
| audible = false; | |
| audible = false; | |
| autoResumeRetries = 0; | |
| errored = false; | |
| stuckRetried = false; |
Unify the repeated 5-line recovery reset pattern (audible, errored, stuckRetried, autoResumeRetries, clearAutoResumeTimer) into a single helper. Removes redundant resets in doLoad that playTrack immediately redoes, and ensures handleStop/teardown fully reset resilience state.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Port three resilience features from the web audio player:
Also fixes: handleSkipTo sets active/userPaused flags, handleResume clears stale timers and resets retry budgets, teardown cleans up all timer and flag state.