Skip to content

fix(sdk): one-shot retryable resume; stop() clears interruption state#100

Merged
briankeane merged 2 commits into
developfrom
briankeane/resume-retry-fix
Jun 10, 2026
Merged

fix(sdk): one-shot retryable resume; stop() clears interruption state#100
briankeane merged 2 commits into
developfrom
briankeane/resume-retry-fix

Conversation

@briankeane

Copy link
Copy Markdown
Collaborator

Addresses the actionable findings from the 0.20.0 release PR (#99) review.

What this fixes

1. Resume retryability gap (Greptile P1 on #99). resumeAfterInterruption()'s "preserve token on failure" was defeated by play(), which resets isSuspended / wasPlayingBeforeInterruption / interruptedStationId at its very first lines — so a transient schedule-fetch failure during resume silently killed the documented retry path.

2. Re-arm races (Codex adversarial review). A re-arm-on-failure approach turned out to have races (reviving a station the host stop()'d mid-resume; same-station host restart). Rather than keep patching the re-arm, resumeAfterInterruption() is redesigned as a clean one-shot:

  • Consumes the armed interruption up-front; throws on failure (host knows — not silent); retry is via play(stationId:), documented.
  • A generation-supersession guard after restartEngine() abandons the resume if a host stop()/play() interleaved during it. 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").

3. API surface. handleAudioEngineConfigurationChange is now internal (was public) — it's an @objc notification selector target, not host API.

Not addressed (by design)

The cross-repo .paused compile break Greptile flags in playola-radio-ios is intentional lockstep coordination — the app adopts .paused in its Phase 1 alongside the pin bump to 0.20.0. The compile error is the coordination mechanism; it is not an SDK fix.

Review

  • Codex adversarial review: redesign is race-free across all orderings (restartEngine-window stop/play(other)/play(same); play-await-window supersession; restartEngine/play throws); generation-guard value confirmed correct; no inconsistent state.

Testing

  • swift test: 99 tests pass (local + CI=true, no skips/hangs). New: stop()-clears-armed-state and resume-after-stop-is-no-op (both CoreAudio-free).
  • swift build clean; SwiftLint strict + swift-format clean.
  • The armed-resume path itself calls restartEngine() (real CoreAudio / resolves .shared), so per the no-CoreAudio test rule it stays inspection-verified.

Merges to develop; release PR #99 (develop→main) picks it up automatically before the 0.20.0 tag.

🤖 Generated with Claude Code

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.
@greptile-apps

greptile-apps Bot commented Jun 10, 2026

Copy link
Copy Markdown

Greptile Summary

This PR addresses two post-0.20.0 race conditions in the audio interruption transport: resumeAfterInterruption() is redesigned as a true one-shot (consuming armed state up-front, using a generation guard to abandon if the host interleaves stop()/play() during restartEngine()), and stop() now fully clears all interruption fields so a later resume can never revive an explicitly-stopped station. handleAudioEngineConfigurationChange is demoted from public to internal, matching its role as a notification selector target only.

  • One-shot resume: resumeAfterInterruption() consumes isSuspended/interruptedStationId/wasPlayingBeforeInterruption before the first await, captures playGeneration as a supersession guard, publishes .error state on restartEngine() failure (mirroring play()'s own error path), and abandons cleanly if the generation changed mid-restart.
  • stop() forgets everything: Three new lines clear all interruption bookkeeping at the end of stop(), preventing the zombie-resume scenario where a host-stopped station could be silently re-armed by a pending resume retry.
  • Tests added: Two new @MainActor unit tests (stopClearsArmedInterruptionState, resumeAfterStopIsNoOp) cover both invariants without touching the shared CoreAudio graph.

Confidence Score: 5/5

Safe to merge — all three changed behaviours are logically consistent and correctly implemented.

Every scenario in the redesigned resume flow resolves to a coherent published state with no stale bookkeeping. The generation guard is captured at the right point, stop()'s clearing of interruption fields is ordered correctly, and cross-repo consumers reference neither handleAudioEngineConfigurationChange nor the interruption API directly.

No files require special attention.

Important Files Changed

Filename Overview
Sources/PlayolaPlayer/Player/PlayolaStationPlayer.swift Core player logic: adds three one-liner state clears to stop(), redesigns resumeAfterInterruption() as a one-shot with a generation guard and consistent error-state publication, and narrows handleAudioEngineConfigurationChange to internal. Logic is sound and race-analysis is correct.
Tests/PlayolaPlayerTests/InterruptionTransportTests.swift Two new tests cover the stop-clears-armed-state and resume-after-stop-is-no-op invariants; both are CoreAudio-free and follow existing test patterns cleanly.

Sequence Diagram

sequenceDiagram
    participant Host
    participant Player as PlayolaStationPlayer
    participant Mixer as PlayolaMainMixer

    Note over Host,Mixer: Happy path resume
    Host->>Player: pauseForInterruption()
    Note over Player: playGeneration++
    Host->>Player: resumeAfterInterruption()
    Note over Player: Consumes armed state, captures generation
    Player->>Mixer: await restartEngine()
    Mixer-->>Player: success
    Player->>Player: await play(stationId:)
    Player-->>Host: playing

    Note over Host,Mixer: stop() during restartEngine() window
    Host->>Player: pauseForInterruption()
    Host->>Player: resumeAfterInterruption()
    Note over Player: consumes state, generation = N
    Player->>Mixer: await restartEngine()
    Host->>Player: stop()
    Note over Player: playGeneration = N+1, state = .idle
    Mixer-->>Player: returns
    Note over Player: guard N==N+1 fails, abandon
    Player-->>Host: returns

    Note over Host,Mixer: restartEngine() throws
    Host->>Player: resumeAfterInterruption()
    Note over Player: consumes state, generation = M
    Player->>Mixer: await restartEngine()
    Mixer-->>Player: throws
    Note over Player: state = .error, rethrow
    Player-->>Host: throws
Loading

Reviews (2): Last reviewed commit: "fix(sdk): publish .error on resume engin..." | Re-trigger Greptile

Comment thread Sources/PlayolaPlayer/Player/PlayolaStationPlayer.swift
Comment thread Sources/PlayolaPlayer/Player/PlayolaStationPlayer.swift
…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.
@briankeane

Copy link
Copy Markdown
Collaborator Author

@greptile review this

@briankeane
briankeane merged commit dda547d into develop Jun 10, 2026
3 checks passed
@briankeane
briankeane deleted the briankeane/resume-retry-fix branch June 10, 2026 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant