Skip to content

fix(sdk): generation-gate + .error state in engine-config recovery#101

Merged
briankeane merged 2 commits into
developfrom
briankeane/engine-recovery-generation-gate
Jun 10, 2026
Merged

fix(sdk): generation-gate + .error state in engine-config recovery#101
briankeane merged 2 commits into
developfrom
briankeane/engine-recovery-generation-gate

Conversation

@briankeane

Copy link
Copy Markdown
Collaborator

Addresses the 3 P1 threads from the 0.20.0 release PR (#99) re-review — all on handleAudioEngineConfigurationChange. They're the same supersession + state-consistency patterns already applied to resumeAfterInterruption(); this carries them into the engine-recovery handler.

Fixes

  1. Generation gate (threads on lines 963/964). The recovery Task called play(stationId: stationToRecover) after restartEngine() with no supersession check. A host stop()/play(other) during the ~100–200ms restart would be clobbered: the recovery would revive a stopped station or override the host's explicit station switch. Now snapshots playGeneration with stationToRecover and guard generation == playGeneration before play() — identical to resumeAfterInterruption().
  2. .error on restart failure (line 963 thread). A restartEngine() throw previously only went to Sentry; state stayed .playing with dead audio. Now publishes state = .error(...) (gated on generation) so a host driving UI from $state sees the failure — mirrors resumeAfterInterruption().

Testing

  • swift test: 99 tests pass (local + CI=true). Build / SwiftLint strict / swift-format clean.
  • The recovery path calls restartEngine()/play() (real CoreAudio / resolves .shared), so per the no-CoreAudio test rule it's inspection-verified; the synchronous guard is covered by the existing engine-config tests.

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

🤖 Generated with Claude Code

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

greptile-apps Bot commented Jun 10, 2026

Copy link
Copy Markdown

Greptile Summary

Ports the supersession-gate and .error state-publishing patterns (already in resumeAfterInterruption()) into handleAudioEngineConfigurationChange. The engine-config observer is also re-registered block-based with queue: .main + MainActor.assumeIsolated, eliminating the synchronous race on @MainActor state that was flagged in the prior review.

  • Generation gate on recovery Task: snapshots playGeneration before the Task, guards restartEngine() and play() calls behind generation == playGeneration, so a host stop()/play() during the ~100–200ms restart no longer clobbers the host's choice.
  • .error on restart failure: restartEngine() throws now publish state = .error(...) (gated on generation) and report to Sentry, mirroring resumeAfterInterruption() instead of leaving the host on a stale .playing with dead audio.
  • Observer threading fix: block-based addObserver(forName:object:queue:using:) with queue: .main replaces the @objc/#selector form so all synchronous handler reads run on the main actor.

Confidence Score: 4/5

Safe to merge after fixing the missing removeObserver call in deinit — the core generation-gate and .error state logic is correct and well-structured.

The block-based observer token is stored in engineConfigObserver but deinit never calls NotificationCenter.default.removeObserver(engineConfigObserver). The block-based API requires explicit cleanup — unlike the prior selector form which auto-removed on iOS 9+ when the observer was deallocated. The example app already applies the correct cleanup pattern for its own block-based observer, so the fix is one line.

Sources/PlayolaPlayer/Player/PlayolaStationPlayer.swift — specifically the deinit block, which needs a NotificationCenter.default.removeObserver(engineConfigObserver) call to match the block-based observer API contract.

Important Files Changed

Filename Overview
Sources/PlayolaPlayer/Player/PlayolaStationPlayer.swift Adds generation-gate + .error state to the engine-config recovery Task, and switches the AVAudioEngineConfigurationChange observer to a block-based queue: .main pattern for correct thread safety. The block-based token is stored in engineConfigObserver but never removed in deinit, leaking the observer registration.

Sequence Diagram

sequenceDiagram
    participant NC as NotificationCenter
    participant H as handleAudioEngineConfigurationChange
    participant T as Task @MainActor
    participant M as mainMixer
    participant P as play()
    participant S as state / errorReporter

    NC->>H: AVAudioEngineConfigurationChange (on main queue)
    H->>H: guard !isSuspended, isPlaying, stationId
    H->>H: "let generation = playGeneration"
    H->>T: "spawn Task @MainActor"

    T->>M: restartEngine()
    alt restartEngine throws
        M-->>T: throw error
        T->>T: "if generation == playGeneration"
        T->>S: "state = .error(...)"
        T->>S: reportError(...)
        T->>T: return
    else restartEngine succeeds
        M-->>T: ok
        T->>T: "guard generation == playGeneration"
        alt generation stale
            T->>T: return (silent abandon)
        else generation current
            T->>P: play(stationId: stationToRecover)
            alt play throws
                P-->>T: throw error
                T->>S: reportError(...) no generation guard
            else play succeeds
                P-->>T: ok
            end
        end
    end
Loading

Reviews (2): Last reviewed commit: "fix(sdk): deliver engine-config recovery..." | Re-trigger Greptile

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

Copy link
Copy Markdown
Collaborator Author

Re: the outside-diff note about a PlayolaMainMixing protocol to make the recovery Task's .error/generation-stale branches unit-testable — deferring as a follow-up, not blocking this fix. Introducing the protocol would also have to thread injection through SpinPlayer (which hard-codes PlayolaMainMixer.shared for its engine), so it's a broader change than this PR. It's the same .shared-coupling follow-up already noted in the code. The new branches are inspection-verified (and Codex-reviewed race-free), consistent with how every restartEngine()/play() CoreAudio path in this file is covered.

@briankeane

Copy link
Copy Markdown
Collaborator Author

@greptile review this

@briankeane
briankeane merged commit fd00e3b into develop Jun 10, 2026
3 checks passed
@briankeane
briankeane deleted the briankeane/engine-recovery-generation-gate branch June 10, 2026 23:21
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