Fix crossfade actor isolation crash - #83
Conversation
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCrossfade ramp timers now run on the main queue, directly validate active generation and crossfade state, and finalize without an additional actor handoff. A regression test verifies main-actor ramp completion and cleanup. ChangesCrossfade actor isolation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideThis pull request fixes a Swift 6 main-actor isolation crash in the crossfade path by ensuring the crossfade ramp timer and completion handling both execute on the main actor, and adds a regression test to verify the behavior using generated audio files. Sequence diagram for crossfade ramp and completion on main actorsequenceDiagram
actor Test
participant AVEnginePlayback
participant DispatchSourceTimer
participant MainActor
Test->>AVEnginePlayback: play(url:onReady:)
Test->>AVEnginePlayback: beginCrossfade(url:duration:ramp:)
AVEnginePlayback->>DispatchSourceTimer: DispatchSource.makeTimerSource(queue:.main)
AVEnginePlayback->>DispatchSourceTimer: timer.schedule(deadline:repeating:leeway:)
AVEnginePlayback->>DispatchSourceTimer: timer.setEventHandler(handler:)
AVEnginePlayback->>DispatchSourceTimer: timer.resume()
loop [timer event handler]
MainActor->>AVEnginePlayback: finalizeCrossfade()
AVEnginePlayback-->>Test: onCrossfadeCompleted
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new timer now relies on
DispatchQueue.mainto align with the main-actor-isolated engine; consider explicitly annotatingAVEnginePlayback(or the relevant APIs) with@MainActorto make the executor guarantees clear and avoid future coupling to the dispatch queue choice. - The regression test
crossfadeRampUsesMainActorcurrently only verifies crossfade completion and state, not that the ramp actually executes on the audio engine actor; consider either tightening the assertions to check executor behavior or renaming the test to more accurately reflect what it validates.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new timer now relies on `DispatchQueue.main` to align with the main-actor-isolated engine; consider explicitly annotating `AVEnginePlayback` (or the relevant APIs) with `@MainActor` to make the executor guarantees clear and avoid future coupling to the dispatch queue choice.
- The regression test `crossfadeRampUsesMainActor` currently only verifies crossfade completion and state, not that the ramp actually executes on the audio engine actor; consider either tightening the assertions to check executor behavior or renaming the test to more accurately reflect what it validates.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
TwinskaraokeTests/AVEnginePlaybackRegressionTests.swift (1)
94-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the crossfade completion assertions.
The test confirms the callback fired and
isCrossfadingbecame false, but it does not verify that the incoming URL became current or that the pending marker was cleared. Add assertions forengine.currentURL == incomingURLand!engine.isCrossfadePending; expose a test-only volume snapshot if ramp endpoints must also be verified.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TwinskaraokeTests/AVEnginePlaybackRegressionTests.swift` around lines 94 - 120, Strengthen crossfadeRampUsesMainActor by asserting that engine.currentURL equals incomingURL and engine.isCrossfadePending is false after completion, alongside the existing completion and isCrossfading checks. Only add a test-only volume snapshot if needed to validate ramp endpoints.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Twinskaraoke/Services/Audio/AVEnginePlayback.swift`:
- Around line 1038-1042: Move the self, crossfadeRampGeneration, and
isCrossfading guard to the beginning of the timer handler, before any volume
writes or player-node mutations; keep finalizeCrossfade() conditional on that
same validated state.
---
Nitpick comments:
In `@TwinskaraokeTests/AVEnginePlaybackRegressionTests.swift`:
- Around line 94-120: Strengthen crossfadeRampUsesMainActor by asserting that
engine.currentURL equals incomingURL and engine.isCrossfadePending is false
after completion, alongside the existing completion and isCrossfading checks.
Only add a test-only volume snapshot if needed to validate ramp endpoints.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2dad630a-1883-432f-86d0-16bb48965b7d
📒 Files selected for processing (2)
Twinskaraoke/Services/Audio/AVEnginePlayback.swiftTwinskaraokeTests/AVEnginePlaybackRegressionTests.swift
1 participant
Summary
Root cause
Swift 6 detected the main-actor-isolated crossfade timer closure running on the custom
crossfade-rampdispatch queue and stopped in_dispatch_assert_queue_failas a song reached the transition point.Impact
Songs can now reach the end and crossfade into the next queued song without triggering the Swift executor assertion.
Validation
Summary by Sourcery
Ensure crossfade volume ramp execution stays on the main actor to prevent isolation-related crashes when transitioning between songs.
Bug Fixes:
Tests:
Summary by CodeRabbit
Bug Fixes
Tests