0.19.0#97
Merged
Merged
Conversation
Two production-incident hardening changes to PlayolaStationPlayer, both exercising the schedule fetch that wedged the app during a backend 5xx outage. Issue 1 — initial play() had no retry/backoff. play(stationId:) fetched the schedule once and threw immediately on a 500, while the rest of the player (spin loading, schedule polling) already recovered. getUpdatedScheduleWithRetry now retries transient failures (5xx, connectivity URLErrors) up to 3 times with exponential backoff (0.5/1/2s); 404/decode/empty-schedule still fail fast. Issue 2 — play() threw on schedule failure without emitting any state, leaving $state subscribers stuck (the permanent loading spinner). State gains a terminal .error(StationPlayerError) case; play() now emits .loading(0) at the start and .error(_) on terminal failure (cancellation excepted) while still throwing. Adds a URLSessionProtocol injection seam to PlayolaStationPlayer (mirroring ListeningSessionReporter) so the schedule fetch is testable. Updates the example app to handle the new .error state (reference UX: show the message, tap to retry). Source-breaking for consumers switching exhaustively over State.
…method - fetchScheduleOnce no longer re-reports errors that inner steps (reportHTTPError / validateHTTPResponse / decodeSchedule) already report, so a failed fetch produces one error report per attempt instead of two. Raw transport errors (URLError) are still reported once. - Document getUpdatedScheduleWithRetry as intentionally internal for tests. Note: the Greptile "Comments Outside Diff" item (playola-radio-ios switch statements needing a .error arm) is a downstream consumer change handled in the app repo at upgrade time, not in this package PR; documented in CHANGELOG.
…eview P1) schedulingTask?.cancel() previously ran only in play()'s success path, so a poll loop started by a prior successful session kept running after a failed play(). Because SpinPlayerDelegate.player(_:startedPlaying:) sets state to .playing, a spin completing after the failure would override the new .error state, leaving the player incoherent. The catch block now cancels the scheduling loop before emitting .error. schedulingTask is now internal (was private) so tests can observe its lifecycle.
isCancellation(_:) did not recognise FileDownloadError.downloadCancelled, which scheduleSpin throws when stop() cancels an in-flight download (e.g. a station switch). play()'s catch then wrote .error over the new play()'s .loading state, flashing an error instead of a loading spinner. isCancellation is now consistent with shouldSkipRetry. Made internal for testability.
…view) Addresses three review findings about the new .error state interacting with concurrency: - #1 (P2) cooperative-cancellation race: a prior session's scheduling loop or a late spin callback could publish .playing over a newer .error/.idle, because Task.cancel() is cooperative. Introduce a monotonic play generation bumped on every play()/stop(); the scheduling loop self-exits when superseded, and SpinPlayerDelegate.startedPlaying / progress / .playing writes are gated on the generation that scheduled them. Robust regardless of cancellation timing. - #3 (P2) over-broad teardown: the failed-play catch no longer force-cancels a prior session's schedulingTask. It only publishes .error when still the current generation; superseded loops self-exit via the token instead. Behavior is now "last play() wins" — a failed switch lands on .error. - #2 (P3) retry breadth: classifyScheduleFetchError now retries only an explicit allow-list of connectivity URLError codes; non-connectivity errors (.badURL, .unsupportedURL, ...) fail fast instead of burning the 3.5s backoff. playGeneration / SpinPlayer.playGeneration are internal for tests. Adds tests for stale-spin suppression, no-force-cancel, and connectivity-only retry.
…CI hang)
The stale-generation test built a real SpinPlayer, which lazily instantiates
PlayolaMainMixer.shared and wires AVAudioEngine nodes to the output mixer. On a
headless CI machine with no audio output device that blocks, so `swift test`
built and linked but the runner hung before any test output ("tests never
started"). It passed locally only because the dev machine has an audio device.
Nothing else instantiates the mixer on macOS (the observer that references it is
iOS-only), so this test was the sole trigger. Extract the generation check into
`isCurrentGeneration(_:)` and assert the gate directly — no CoreAudio in unit
tests. Same coverage of the Marge #1 gating, CI-safe.
…PR review) Marge P2: the download-progress and optimistic-.playing writes read the generation back off the reusable SpinPlayer (spinPlayer.playGeneration), which a recycled player could have re-tagged. Capture the generation as an immutable local in scheduleSpin and gate those callbacks on it via isCurrentGeneration(). The player tag remains only for the startedPlaying delegate path, where the player is never .available (reusable) while a start callback is pending.
Independent review (Codex) confirmed two of Marge's findings were reachable P1s: the generation was threaded inconsistently — scheduleSpin and the poll loop re-read the global playGeneration instead of inheriting the caller's captured value, so a superseded session could tag a SpinPlayer as current and defeat the startedPlaying guard, or run one stale schedule update after supersession. Thread one immutable generation through the whole chain: - scheduleSpin(generation:) uses the parameter (never the global) to tag the player; threaded through handleLoadResult/handleLoadFailure/handleSchedulingError -> retryIfPossible, which re-checks isCurrentGeneration after its backoff sleep. - performScheduleUpdate(stationId:generation:) re-checks after the fetch await and before each per-spin scheduleSpin. - play() assigns currentSchedule only after the post-fetch generation guard, so a superseded attempt no longer writes the shared field. Codex verified all 5 points of its "done" checklist are met (no stale task self-certifies via the global; post-await checks before every state/schedule commit; SpinPlayer gets the caller's captured generation; retry paths preserve it; delegate callbacks check generation). 88 tests pass.
…-retry-state fix: backoff + terminal error state for initial schedule fetch
SpinPlayer is @mainactor, so its play paths called synchronous AVAudioEngine / AVAudioPlayerNode operations (engine.start, playerNode.play / play(at:), lastRenderTime) on the main thread, blocking on the audio IO cycle and producing multi-second App Hangs in production (Sentry APPLE-IOS-Y, APPLE-IOS-12, APPLE-IOS-10) — the same root-cause family as the merged NowPlayingUpdater fix (APPLE-IOS-1C). Move the blocking node/engine control calls onto a single serial audio-control queue and make playNow/schedulePlay/scheduleAndPlaySegment async. A playbackEpoch token (with a thread-safe OSAllocatedUnfairLock mirror) is captured before each await and re-checked after — and inside each queue block before starting the node — so a stop()/clear() or newer play during the off-main window can't write stale state, fire delegate callbacks, or leave orphaned audio. handleSuccessfulDownload re-checks spin ownership after its awaits. hardResetTrackMixer graph surgery intentionally stays on the main actor (documented residual).
`import os` already provides OSLog/os_log and OSAllocatedUnfairLock, so the separate `import os.log` tripped swiftlint's duplicate_imports rule.
…Future) Greptile review: - Move the residual main-thread engine.start() in the fade path off-main: ensureEngineRunning/installStartTapIfNeeded/scheduleFades are now async and start the engine via playolaMainMixer.start(). In the normal path the engine is already running so this short-circuits; the off-main start only matters on interruption recovery, where it previously could block the main thread on AUIOClient_StartIO — the same hang this PR fixes elsewhere. - playFuture now returns a FuturePlayOutcome enum (.skipped / .played) instead of a Bool that conflated "superseded" with "played", so a future refactor can't silently drop a missingRenderTime report.
Round-1's async fade path (scheduleFades/installStartTapIfNeeded/ ensureEngineRunning) introduced two reentrancy bugs flagged in re-review: - the new await in scheduleFades let a clear() interleave before state=.loaded; - setting startTapInstalled=true before the await orphaned the tap if a clear() landed during suspension. Revert those three back to synchronous. ensureEngineRunning still moves the engine start off the main thread, but fire-and-forget via a @mainactor Task awaiting playolaMainMixer.start() — installing the tap doesn't require the engine to already be running, and keeping the method synchronous removes the suspension point between startTapInstalled=true and installTap. The existing spin-ownership guard before scheduleFades is now sufficient.
…ssues-7533514388 fix: run SpinPlayer audio control off the main thread to fix App Hangs
Greptile SummaryThis release PR (0.19.0) merges two feature branches into
Confidence Score: 4/5The SDK changes are solid, but the iOS consumer needs a Both Both exhaustive switches in the iOS consumer need attention: Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant PSP as PlayolaStationPlayer
participant Retry as RetryLoop
participant Net as URLSession
participant SP as SpinPlayer
participant AQ as audioControlQueue
Caller->>PSP: play(stationId)
PSP->>PSP: "playGeneration += 1"
PSP-->>Caller: "state = .loading(0)"
PSP->>Retry: getUpdatedScheduleWithRetry
loop Up to 3 retries on transient errors
Retry->>Net: fetchScheduleOnce
alt 5xx or connectivity URLError
Net-->>Retry: "ClassifiedScheduleError isTransient=true"
Retry->>Retry: Task.sleep backoff
else 404 or decode error
Net-->>Retry: "ClassifiedScheduleError isTransient=false"
Retry-->>PSP: throw publicError
else Success
Net-->>Retry: Schedule
Retry-->>PSP: Schedule
end
end
alt Schedule fetch succeeded
PSP->>SP: "scheduleSpin generation=N"
SP->>SP: "beginPlayback epoch=E"
SP->>Net: download audio
SP->>AQ: "runAudioControl epoch=E"
Note over AQ: skip if epoch superseded
AQ-->>SP: done
SP-->>PSP: "delegate startedPlaying gated gen==N"
PSP-->>Caller: "state = .playing(spin)"
else All retries exhausted
PSP-->>Caller: "state = .error(StationPlayerError)"
PSP-->>Caller: throws StationPlayerError
end
Reviews (3): Last reviewed commit: "fix: don't report schedule-fetch cancell..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…breaking in 0.19.0 changelog
Collaborator
Author
|
@greptile review this |
…layTask Address Greptile re-review (4/5): - fetchScheduleOnce's catch-all fired reportScheduleFetchError at .error for cancellations (URLError(.cancelled)/CancellationError), so a routine stop() during the in-flight initial fetch polluted monitoring with a spurious production error. Guard the report with the existing isCancellation() helper; the error still propagates so callers treat it as a cancellation. - Remove the vestigial playTask property: it was declared, cancelled, and niled but never assigned a Task, so every use was a no-op. - Add a deterministic test that a cancelled URLError fails fast without retrying.
Collaborator
Author
|
@greptile review this |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
This release ships the
0.19.0CHANGELOG section. SeeCHANGELOG.mdfor fullnotes, including the source-breaking changes (
State.errorcase;playNow/schedulePlaynowasync).Version Bump
After Merge
Tag main with
0.19.0:```bash
git checkout main && git pull
git tag 0.19.0
git push origin 0.19.0
```