fix(audio): gate the post-device-error rebuild to a single owner#393
Conversation
Refs #365, #355. A DAC that resets its session on every exclusive grab sent the engine into a self-sustaining rebuild cascade. The recovery is deferred: the cpal error callback scheduled the rebuild 300 ms after DeviceNotAvailable. Because each error queued its own task and those tasks never overlap — one finishes before the next starts — the rebuild_in_progress debounce always found a free slot, so every error got its own rebuild. Each rebuild re-opened exclusive, which made this device reset again, which armed yet another pass. Eventually one opened exclusive while the previous stream was still settling and hit AUDCLNT_E_DEVICE_IN_USE (0x8889000A); its shared fallback then failed too, leaving the engine and the Settings toggle inconsistent. Note this corrects the analysis in #365: the two passes are not concurrent. .341 + 300 ms lands on the .653 pass, so the earlier one is the deferred rebuild of a *previous* error, not a racing trigger. Adds RebuildGate: arming now happens in the error callback, before the delay, and covers the whole arm -> delay -> run lifecycle plus a 2 s settle window afterwards. One burst of device errors therefore yields one rebuild. The settle window is the part that actually breaks the cascade, since our own re-open is what provokes the next error on this hardware. A genuine later failure still recovers once the window passes. Released through an RAII guard on every exit path of try_rebuild_after_device_error, including the error and early-return ones, so a failed rebuild cannot latch the gate shut. The S16_LE-instead-of-S24_4LE detail in #365 is a symptom of the collision, not a separate bug: the #174 negotiation ladder degrades because the device is still held. It should resolve with the cascade. Clock-injected and unit-tested like FlapWindow (7 cases, incl. a replay of the reporter's ~300 ms error cadence: 10 errors collapse to 2 rebuilds instead of 10). Green: cargo check --all-targets, cargo clippy --all-targets.
InstaZDLL
added
scope: backend
|
Warning Review limit reached
Next review available in: 57 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughLe chemin de reconstruction audio après ChangesCoordination des reconstructions audio
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant cpal
participant output_rs
participant AudioEngine
cpal->>output_rs: StreamError::DeviceNotAvailable
output_rs->>output_rs: attendre 300 ms
output_rs->>AudioEngine: try_arm_device_rebuild()
AudioEngine-->>output_rs: true ou false
output_rs->>AudioEngine: try_rebuild_after_device_error()
AudioEngine->>AudioEngine: GateGuard::finish()
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. 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 |
![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
🤖 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 `@src-tauri/crates/app/src/audio/output.rs`:
- Around line 517-548: Move recovery orchestration out of the audio error
callback: in src-tauri/crates/app/src/audio/output.rs lines 517-548, replace
try_state, tauri::async_runtime::spawn, delays, and tracing logs with a
lock-free error signal consumed by a non-real-time worker; in
src-tauri/crates/app/src/audio/engine.rs lines 617-638, update
try_arm_device_rebuild so the callback path does not call rebuild_gate.lock(),
using an atomic or equivalent non-blocking gate while preserving
single-rebuild-per-burst behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 23509f98-39b9-447f-bddd-8029b4a98b54
📒 Files selected for processing (2)
src-tauri/crates/app/src/audio/engine.rssrc-tauri/crates/app/src/audio/output.rs
…callback Review follow-up on #393. The previous revision looked the AudioEngine up in the cpal error callback to arm the gate before the 300 ms delay. That narrowed recovery: an error can fire while AudioEngine::new is still running, i.e. before the engine is registered in Tauri's managed state, and the eager lookup silently dropped the event for that window — a regression against the original code, which resolved the engine after the delay. Move the whole recovery orchestration into the deferred task: sleep, then resolve the engine, then arm, then rebuild. Burst collapsing is preserved because the gate still serialises — whichever task wakes first arms, and the others hit either `armed` or the settle window. Verified for both the ~300 ms cadence from the report and a tight burst waking simultaneously. As a side effect the error callback now only spawns the task; the engine lookup, the gate, the rebuild and its logs all run off it. Green: cargo check --all-targets, cargo clippy --all-targets.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Review follow-up on #393. try_rebuild_after_device_error is synchronous and can block for a long time: it joins the old output thread — which in WASAPI exclusive mode may sit in wait_for_event up to its 2 s timeout — and then opens the device inline (COM init plus the #174 format negotiation ladder). Calling it directly from the tokio task parked a runtime worker for that whole duration. Clone the Arc<AudioEngine> and run the rebuild through tokio::task::spawn_blocking instead. The #365 gate and its early return stay on the async side, unchanged, and the existing warning is preserved inside the blocking task. Green: cargo check --all-targets, cargo clippy --all-targets.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src-tauri/crates/app/src/audio/output.rs`:
- Around line 524-529: Update the spawned retry task around AudioEngine state
lookup so a missing engine after the initial 300 ms does not discard the pending
request. Keep retrying with bounded attempts or retain a wake-up signal until
try_state::<Arc<AudioEngine>>() succeeds, then add a test covering
initialization that exceeds the 300 ms delay.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 66c6aeb4-b575-4c79-8ee8-b26eebacc81f
📒 Files selected for processing (2)
src-tauri/crates/app/src/audio/engine.rssrc-tauri/crates/app/src/audio/output.rs
| tauri::async_runtime::spawn(async move { | ||
| tokio::time::sleep(std::time::Duration::from_millis(300)).await; | ||
| if let Some(engine) = app_clone.try_state::<std::sync::Arc<super::AudioEngine>>() { | ||
| if let Err(err) = engine.try_rebuild_after_device_error() { | ||
| tracing::warn!( | ||
| %err, | ||
| "auto-rebuild after DeviceNotAvailable failed" | ||
| ); | ||
| } | ||
| let Some(engine) = app_clone.try_state::<std::sync::Arc<super::AudioEngine>>() | ||
| else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Ne perdez pas l’erreur si l’engine reste non enregistré après 300 ms.
Le déplacement de try_state après le délai réduit la fenêtre de course, mais ne la ferme pas : si l’initialisation dure plus de 300 ms, cette tâche retourne et l’unique demande de reprise est définitivement perdue. Attendez l’enregistrement avec des tentatives bornées, ou conservez un signal de reprise jusqu’à ce que l’engine soit disponible, puis ajoutez un test couvrant ce délai.
🤖 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 `@src-tauri/crates/app/src/audio/output.rs` around lines 524 - 529, Update the
spawned retry task around AudioEngine state lookup so a missing engine after the
initial 300 ms does not discard the pending request. Keep retrying with bounded
attempts or retain a wake-up signal until try_state::<Arc<AudioEngine>>()
succeeds, then add a test covering initialization that exceeds the 300 ms delay.
1 participant
Refs #365, parent #355.
The mechanism (this corrects #365's own analysis)
#365 assumed the two rebuild passes were concurrent. The timestamps say otherwise: the error at
.341plus the 300 ms delay lands exactly on the.653pass — so the earlier pass at.377is the deferred rebuild of a previous error, not a racing trigger.What actually happens is a self-sustaining cascade:
DeviceNotAvailablefires → a rebuild is scheduled 300 ms later.DeviceNotAvailable.The
rebuild_in_progressdebounce could never catch this, because the recovery is deferred: each error queues its own task, and those tasks never overlap — one finishes before the next begins, so every single one finds a free debounce slot. Eventually a pass opens exclusive while the previous stream is still settling and collides withAUDCLNT_E_DEVICE_IN_USE (0x8889000A); its shared fallback then fails too (device no longer available), leaving the engine and the Settings toggle inconsistent — which is exactly the "only closing and reopening WaveFlow resets the button" symptom in #355.The
#322flap-storm suppression doesn't fire either, and correctly so: the reporter's events are hours apart, so 3 flaps never land inside the 12 s window. This isn't a storm, it's a one-off device reset that amplifies itself.The fix
A
RebuildGatethat owns the whole arm → delay → run lifecycle instead of just the in-flight call:try_rebuild_after_device_error— including the early returns and the error paths — so a failed rebuild can't latch the gate shut and block all future recovery.Clock-injected and unit-tested, same shape as the existing
FlapWindow.On the S16_LE / S24_4LE detail
#365 lists the format regression as point 3. It's a symptom, not a separate bug: the #174 negotiation ladder degrades precisely because the device is still held by the previous stream. It should resolve once the collision is gone — worth confirming in the reporter's next log rather than "fixing" separately.
Verification
cargo check -p waveflow --all-targets✅cargo clippy -p waveflow --all-targets✅ (no warnings)Summary by CodeRabbit