0.20.0 - Host-only audio-session ownership#99
Merged
Merged
Conversation
…aused handling (review)
…e-download audio guard (final review)
- configure() no longer resolves the shared mixer on the default path (.sdkOwned, no injection) — develop's configure never touched the mixer, and resolving .shared builds the CoreAudio graph, which hangs headless CircleCI runs that merely construct + configure a player - gate AudioSessionOwnershipTests behind CI env check: every test in the suite constructs PlayolaMainMixer (same failure class as e8604a5) - guard handleAudioEngineConfigurationChange on handlesSessionEventsInternally: auto-resume is interruption policy and must not race a host-driven resume (greptile outside-diff #1) - pauseForInterruption cancels playTask for symmetry with stop() (greptile outside-diff #2) - seam test: graceful skip when sources aren't present at runtime (compile-time #filePath; greptile outside-diff #4) - lint: file_length disable pair on PlayolaStationPlayer.swift (matches SpinPlayer precedent), for-where + line length in seam test
The SDK no longer manages the AVAudioSession at all. Removes the .sdkOwned/.hostOwned dual mode and all of its transitional machinery, which was the source of repeated review churn (split-brain release fallback, late-application ordering, double-configure divergence) and had no value given a single consumer that migrates in lockstep. Deleted: PlayolaAudioSessionOwnership, AudioSessionManager + NoOpAudioSessionManager + AudioSessionManaging (whole file), applyOwnership/appliedOwnership/sessionTouched/hasAppliedOwnershipToMixer, handlesSessionEventsInternally, observer gating, and the legacy interruption/route/engine-config handlers (removing the two known pre-existing legacy bugs with them). configure() loses its audioSessionOwnership parameter. The host now owns the AVAudioSession: it configures/activates it and drives pauseForInterruption()/resumeAfterInterruption() from its own interruption handlers. SpinPlayer/PlayolaMainMixer no longer touch the session; engine.start() throwing on an inactive session surfaces through the normal error path. Unchanged: pauseForInterruption/resumeAfterInterruption/.paused, lazy mixer resolution, stale-download guard. The seam invariant test now asserts the SDK references AVAudioSession nowhere in Sources/. Example app gains its own session setup. README documents the host contract and a 0.19->0.20 migration guide. BREAKING CHANGE: host apps must own the AVAudioSession (see README migration guide). Ships as 0.20.0.
…ure (review) Adversarial review (Codex challenge) findings on the host-only rework: - [P1] resumeAfterInterruption() cleared the armed resume token in a defer even when restartEngine()/play() threw, permanently disarming retry. Now disarms only after a successful resume; a failed resume (e.g. host hasn't reactivated the session yet) stays armed for the next attempt. (This was unsafe to do under the old dual-mode config-change observer; that observer is gone, so preserving the token can no longer cause a double-resume.) - [P1] removing the engine-config observer left no recovery when AVAudioEngine stops itself on a hardware/format/route reconfiguration — silent dead audio while state stayed .playing. Re-added a PURE engine-recovery observer (AVAudioEngineConfigurationChange, not an AVAudioSession API — seam intact): restarts the engine and re-syncs to wall clock, guarded to skip while host-paused or not playing. object: nil avoids resolving the shared mixer at construction (no CI hang). This is engine ownership, not session ownership. - README: corrected the stale 'session management' architecture bullet. - Example app: demonstrates the full host contract (interruption observer driving pause/resume), not just launch-time activation. Accepted as follow-ups (noted on PR): cold-start engine-start failure is retried like a transient download failure and surfaces as .scheduleError (only reachable when the host violates the activate-before-play contract, caught at dev time); the seam test is a substring tripwire, not a complete invariant (documented in-test).
…o own engine - Add CHANGELOG 0.20.0 entry (pre-1.0 breaking = minor bump) documenting the host-owns-AVAudioSession change + migration steps, in the project's style. - Scope the AVAudioEngineConfigurationChange observer to the SDK's OWN engine (registered lazily on first playback) instead of object: nil. Fixes the Greptile 4/5 concern: a host running its own AVAudioEngine no longer triggers spurious SDK restartEngine()+play() on unrelated config changes. Lazy registration keeps construction from building the CoreAudio graph (no CI hang). - README migration guide: drop the phantom 'audioSessionOwnership parameter' step (that param never shipped in a release) — configure() is otherwise unchanged.
…level-audio refactor!: host-only audio-session ownership + host-driven pause/resume
Greptile SummaryThis release (0.20.0) removes all
Confidence Score: 5/5Safe to merge. All three defects raised in prior review rounds are correctly addressed: the generation gate, The core logic — generation-gated recovery, one-shot interruption consumption, stop() clearing armed state, stale-download guard in SpinPlayer — is sound and covered by the new InterruptionTransportTests. The SessionSeamInvariantTests enforce the host-session-ownership contract structurally. The only new findings are a missing removeObserver in deinit (harmless for the singleton, only observable via non-production test patterns) and double Sentry reporting when play() fails inside the config-change recovery Task. The deinit in PlayolaStationPlayer.swift should add removeObserver(engineConfigObserver) to keep the class safe for non-singleton use. The playola-radio-ios companion repo requires .paused arms in both its StationPlayer.processPlayolaStationPlayerState and NowPlayingUpdater.processPlayolaStationPlayerState switches before this SDK version is consumed there. Important Files Changed
Sequence DiagramsequenceDiagram
participant Host as Host App
participant SDK as PlayolaStationPlayer
participant Mixer as PlayolaMainMixer
participant NC as NotificationCenter
Host->>SDK: configure(authProvider:)
Host->>Host: AVAudioSession.setCategory(.playback)
Host->>Host: AVAudioSession.setActive(true)
Host->>SDK: play(stationId:)
SDK->>Mixer: start() [lazy, first play]
NC-->>SDK: AVAudioEngineConfigurationChange
SDK->>Mixer: restartEngine() [generation-gated]
SDK->>SDK: play(stationId:) [if still current gen]
Note over Host,NC: Interruption flow
NC-->>Host: interruptionNotification(.began)
Host->>SDK: pauseForInterruption()
SDK->>SDK: "state = .paused(spin)"
SDK->>SDK: "playGeneration += 1"
NC-->>Host: interruptionNotification(.ended, .shouldResume)
Host->>Host: AVAudioSession.setActive(true)
Host->>SDK: resumeAfterInterruption()
SDK->>Mixer: restartEngine()
SDK->>SDK: "guard generation == playGeneration"
SDK->>SDK: play(stationId: interruptedStationId)
Reviews (4): Last reviewed commit: "Merge pull request #101 from playola-rad..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile (PR #99) caught that the 'preserve token on failure' behavior was defeated by play(), which resets the armed interruption fields at its very first lines — a transient schedule-fetch failure during resume silently killed the host's retry path. Codex adversarial review then showed a re-arm-on-failure approach has races (reviving a station the host stop()'d mid-resume; same-station host restart). Rather than keep patching the re-arm, the resume is redesigned as a clean one-shot: - resumeAfterInterruption() consumes the armed interruption up-front and is a single attempt. It throws on failure (host knows — not silent); retry is via play(stationId:), documented. No re-arm, so no re-arm races. - A generation-supersession guard after restartEngine() abandons the resume if a host stop()/play() interleaved during it (so it can't revive a stopped station or override a freshly-started one). Supersession during play()'s own awaits is handled by play()'s existing generation gate. - stop() now clears isSuspended / wasPlayingBeforeInterruption / interruptedStationId ('stop means forget everything, including any armed interruption'). Also made handleAudioEngineConfigurationChange internal (was public) — @objc selector target, not host API. Tests: stop()-clears-armed-state and resume-after-stop-is-no-op (CoreAudio-free). The armed-resume path itself calls restartEngine() (real CoreAudio) so stays inspection-verified per the no-CoreAudio test rule. The cross-repo .paused compile break Greptile flagged is intentional lockstep coordination (app adopts .paused in Phase 1), not an SDK change.
Collaborator
Author
|
Re: the Greptile review here —
Hold the |
…try doc (review) PR #100 review (Greptile P2s): - On restartEngine() failure during resume, publish state = .error(...) (guarded by the supersession check) so a host driving UI from $state isn't left on a stale .paused with no working resume — mirrors play()'s own error path. - Doc: clarify that the station for a retry play() is available as the player's stationId (still set after a failed resume; only stop() clears it), rather than implying the host must cache it externally.
fix(sdk): one-shot retryable resume; stop() clears interruption state
Collaborator
Author
|
@greptile review this |
Collaborator
Author
PR #99 re-review (Greptile, 3 P1s): handleAudioEngineConfigurationChange's recovery Task was missing the supersession discipline that resumeAfterInterruption() already uses. Two issues, same fixes as resume: - Generation gate: snapshot playGeneration (with stationToRecover) before restartEngine(); if a host stop()/play() bumps it during the restart, abandon the recovery instead of calling play(stationToRecover) — which would override the host's explicit station switch / revive a stopped station. - State on failure: if restartEngine() throws, publish state = .error(...) (gated on generation) so a host driving UI from $state isn't left on a stale .playing with dead audio. Previously the failure only went to Sentry. The recovery path itself calls restartEngine()/play() (real CoreAudio / resolves .shared), so per the no-CoreAudio test rule it stays inspection-verified; the synchronous guard (!isSuspended/isPlaying/stationId) is covered by the existing engine-config tests.
… on generation PR #101 review (Greptile P2s): - Thread-safety: AVAudioEngineConfigurationChange fires on an unspecified thread and @objc dispatch bypasses @mainactor, so the handler's synchronous @mainactor reads (isSuspended/isPlaying/stationId/playGeneration) could race. Register the observer block-based with queue: .main and assumeIsolated, so the handler runs on the main actor; drop @objc (no longer a #selector target). - Sentry noise: the restart-failure path now reports only when still current (generation == playGeneration) — a superseded recovery is moot, matching the state gate. Not addressed (tracked follow-up): a PlayolaMainMixing protocol to inject a mock mixer would unblock unit-testing the recovery Task's .error / generation-stale branches (currently inspection-verified, like all restartEngine()/play() paths). That's the same .shared-coupling follow-up already noted for SpinPlayer; it touches SpinPlayer too and is out of scope for this fix.
…-generation-gate fix(sdk): generation-gate + .error state in engine-config recovery
Collaborator
Author
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.
Release 0.20.0 —
develop→main.Pre-1.0 minor bump (SemVer: breaking changes bump the minor version below 1.0).
Theme
Host-only audio-session ownership. The SDK no longer manages the
AVAudioSession; the host app owns it and drives interruption transport. See the0.20.0CHANGELOG entry for the full migration guide.PRs included
Highlights
AVAudioSessionmanagement (AudioSessionManagergone;configureAudioSession/ensureAudioSessionConfigured/deactivateAudioSessionremoved; no interruption/route observers). Host must configure + activate the session and drive interruptions.pauseForInterruption()/resumeAfterInterruption() async throws, newState.paused(Spin), and engine self-recovery scoped to the SDK's own engine.Version Bump
0.19.00.20.0After merge
Tag the merge commit on
mainas0.20.0and push the tag — that's what SPM consumers pin to. (Consistent with how0.19.0was cut.)