Fix double unlock of EffectsPlayer's playerLock on play() failure

[kean]

Fixes a double unlock of EffectsPlayer.playerLock. When player.play() threw, the lock was released in the catch and then again by play() — and unlocking an NSLock you don't hold is undefined behavior. The helper is now inlined so the failure path unlocks exactly once and returns, matching the other failure paths.

Only triggers when AVAudioPlayerNode.play() throws (rare, e.g. corrupt audio). When it did, the second unlock could release the lock while another thread held it (the same lock guards endPlayback teardown) → intermittent crash or half-torn-down engine. Happy-path playback is unaffected.

To test

1. Play episodes normally — playback, scrubbing, and effects work as before.
2. If possible, play a corrupt/truncated file and confirm it pauses cleanly without crashing.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[SergioEstevao]

@copilot please review this

[SergioEstevao]

Should completion be called inside the catch now?

[SergioEstevao]

Or alternative and looking at other early returns above when exceptions happens call:

PlaybackManager.shared.playbackDidFail(error: .fileCorrupted(logMessage: "AVAudioEngine reported not running"))

[Copilot]

Yes, good catch! The original code inadvertently called completion on the failure path because the catch block had no return, so execution fell through to the completion?() call below. The fix added return but missed preserving that behavior — completion?() is now called in the catch block before returning.

[kean]

It is called for backward-compatibility. Here's a quick repot on how it's used:

PlaybackProtocol.play(completion:) — What the completion is for and how it's used

Summary

func play(completion: (() -> Void)?) (podcasts/PlaybackProtocol.swift:9) takes an optional
"playback has started" callback. Starting playback isn't always synchronous — depending on which
concrete player backs the PlaybackProtocol, actually getting audio flowing can involve async work
(activating the audio session, spinning up an AVAudioEngine on a background queue). The completion
lets the caller run code after the play command has been carried through, rather than assuming it
finished the moment play() returns.

How it flows

PlaybackManager.play(...) (podcasts/PlaybackManager.swift:225) is the public entry point.
It doesn't fire the completion itself — it activates the audio session and then forwards it down to
the concrete player (PlaybackProtocol):

self.player?.play {
    completion?()
}

The three concrete players each honor it differently:

• DefaultPlayer (podcasts/DefaultPlayer.swift:140, AVPlayer-based) — starts playback
  synchronously and calls completion?() at the very end (line 150).
• GoogleCastPlayer (podcasts/Playback/Players/Google Cast/GoogleCastPlayer.swift:41) — tells
  the cast manager to play, then calls completion?() synchronously (line 44).
• EffectsPlayer (podcasts/EffectsPlayer.swift:77, the custom AVAudioEngine player) — this
  is the one where the async nature matters. All the engine setup runs on
  DispatchQueue.global().async, and completion?() fires only once the engine is built and started
  (line 182). Here the completion is genuinely deferred off the calling thread.

How callers use it

Most internal callers pass nil — they only care about starting playback, not being notified:

• DefaultPlayer re-calls self.play(completion: nil) after a seek finishes
  (podcasts/DefaultPlayer.swift:183, :796, :870).
• PlaybackManager.playPause() → play() uses the default nil.

The one meaningful non-nil use is tvOS Pocket Casts TV App/UI/Player/NowPlayingTab.swift:23: on
appear, if nothing is playing, it calls play and in the completion immediately pauses again
(userInitiated: false). That's a deliberate trick — play-then-pause forces the player to load and
prepare the current episode so the Now Playing UI is populated, without actually leaving audio
running.

One caveat worth knowing

The completion is not guaranteed to fire on failure paths. In EffectsPlayer, the early-return
error branches (playbackDidFail, e.g. lines 132–133, 167–168, 174–175) return without calling
completion?(). Likewise PlaybackManager.play returns early without invoking it if the audio
session fails to activate (:245-247). So it's best treated as a "started successfully" signal, not
a "play attempt finished" signal — don't rely on it always running.

[kean]

I'm not yet sure if we should keep or remove it. It should probably, to the very least, be throwable.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[CLAassistant]

All committers have signed the CLA.

[SergioEstevao]

It's working correctly but left two comments for your consideration.