make audio capture send #958
Conversation
WalkthroughAdds an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
crates/recording/src/sources/screen_capture/windows.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
| unsafe impl Send for WindowsAudioCapture {} | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Unsafe Send for WindowsAudioCapture is likely unsound without proof; kameo/tokio may move the actor across threads
The actor holding WindowsAudioCapture can be polled on different Tokio worker threads (multi-thread runtime), which requires the actor state to be Send. By adding unsafe impl Send, we assert that moving WindowsAudioCapture (and thus its scap_cpal::Capturer) between threads is sound. That’s a high bar: CPAL on Windows (WASAPI/COM) often involves thread-affine resources; if scap_cpal::Capturer or anything it retains is !Send, this unsafely sidesteps the compiler and can lead to UB/data races.
Action items:
- Either justify this
unsafe implwith concrete invariants (and references to upstream trait impls) or avoid needing it.
Safer alternatives:
- Prefer running this actor on a single-threaded executor or a LocalSet so it doesn’t need
Send(e.g., a “local actor” variant if kameo supports it). - Offload the
scap_cpal::Capturerto a dedicated OS thread and communicate with it via channels; keep the actor itselfSendby only storing channel ends in the actor. - If it’s actually safe: document the invariants and add compile-time checks (see below). Even better, make
scap_cpal::CaptureritselfSendin its crate with a justified unsafe impl rather than layering the promise here.
Minimal immediate improvement (if you keep this):
- Add a rigorous SAFETY comment that enumerates why moving across threads is sound, and what invariants must be upheld.
Proposed inline change (documentation only):
- unsafe impl Send for WindowsAudioCapture {}
+ // SAFETY:
+ // - Explain why moving WindowsAudioCapture across threads is sound.
+ // - Demonstrate that all fields are Send or otherwise thread-agnostic.
+ // - State that methods are only called from one thread at a time (actor's mailbox),
+ // and that underlying CPAL/WASAPI handles tolerate cross-thread moves.
+ unsafe impl Send for WindowsAudioCapture {}If kameo supports a non-Send actor path, consider removing the impl instead:
- unsafe impl Send for WindowsAudioCapture {}
+ // NOTE: Keep WindowsAudioCapture !Send and run this actor on a LocalSet/single-thread runtime.
+ // See follow-up in PR discussion for wiring.Verification steps I recommend running in the repo to reduce guesswork:
And confirm upstream CPAL’s guarantees for the exact version in Cargo.lock:
If CPAL Stream is not Send for the version in use, this unsafe impl is a correctness bug and should be reverted in favor of a local actor/thread confinement approach.
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "=== Locate scap_cpal::Capturer definition ==="
rg -nP --type=rust -C3 '^\s*(pub\s+)?struct\s+Capturer\b' || true
echo
echo "=== Check for existing Send/Sync impls on Capturer ==="
rg -nP --type=rust -C2 'impl\s+(unsafe\s+)?(Send|Sync)\s+for\s+Capturer' || true
echo
echo "=== What CPAL version are we on? ==="
rg -n 'name = "cpal"' Cargo.lock -n -C1 || true
echo
echo "=== Usage: where is WindowsAudioCapture spawned? (confirm multithread runtime) ==="
rg -nP --type=rust -C3 'WindowsAudioCapture::spawn\(|spawn\(\s*WindowsAudioCapture' || trueLength of output: 3397
🌐 Web query:
Does cpal::<version from Cargo.lock>::Stream implement Send on Windows (WASAPI)? Cite source lines from the CPAL docs or code.
Error during web search
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "=== Locate WindowsAudioCapture struct ==="
rg -nP --type=rust 'pub struct WindowsAudioCapture' -C5 crates/recording/src/sources/screen_capture/windows.rs || true
rg -nP --type=rust 'struct WindowsAudioCapture' -C5 crates/recording/src/sources/screen_capture/windows.rs || true
echo
echo "=== Inspect WindowsAudioCapture fields for Capturer usage ==="
rg -nP --type=rust 'WindowsAudioCapture\s*{?[^;]*' -C5 crates/recording/src/sources/screen_capture/windows.rs || trueLength of output: 2895
🌐 Web query:
Does CPAL 0.15.3's Stream implement Send on Windows (WASAPI)? Please cite source code or docs.
Error during web search
Unsound unsafe impl Send for WindowsAudioCapture – Action Required
The unsafe impl Send for WindowsAudioCapture {} on lines 470–471 asserts that its sole field, scap_cpal::Capturer, is safe to move across threads. However:
- The
Capturerstruct incrates/scap-cpal/src/lib.rsis defined as:and there are nopub struct Capturer { stream: Stream, config: StreamConfig, _output_device: cpal::Device, }
Sendimplementations for it or its innercpal::Streamon Windows. By default, CPAL’s WASAPI stream is notSenddue to COM objects being thread-affine and requiring strict thread ownership. - Overriding this with an
unsafe implwithout a documented safety contract opens the door to UB and data races when Tokio’s multi-thread scheduler moves the actor between worker threads.
Next steps – choose one:
• Remove the unsafe impl Send and confine the actor to a single-threaded context:
- unsafe impl Send for WindowsAudioCapture {}
+ // NOTE: WindowsAudioCapture is !Send. Run this actor on a Tokio LocalSet
+ // or single-thread runtime to maintain COM thread-affinity.This ensures correct WASAPI usage without unsound thread-crossing.
• Justify and document the safety if truly safe to send:
- unsafe impl Send for WindowsAudioCapture {}
+ // SAFETY: Moving WindowsAudioCapture across threads is sound because:
+ // - `scap_cpal::Capturer` only holds thread-agnostic handles
+ // (cpal::Device and StreamConfig implement Send/Sync, and
+ // the underlying WASAPI COM objects are marshaled if moved).
+ // - All public methods synchronize access via the actor mailbox,
+ // guaranteeing no concurrent calls.
+ unsafe impl Send for WindowsAudioCapture {}And consider moving the Send impl upstream into the scap-cpal crate where the invariants can be centrally audited.
Without one of these fixes, the code unsafely sidesteps Rust’s guarantees. Please address as a critical fix.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/recording/src/sources/screen_capture/windows.rs (1)
472-473: Unsafe Send for WindowsAudioCapture is likely unsound; avoid asserting cross-thread safety without proof
WindowsAudioCaptureholdsscap_cpal::Capturer. For WASAPI/CPAL-backed streams, thread affinity is common; forcingSendcan cause UB/races if the actor is moved across threads. Unless you can prove every field isSendand cross-thread moves are safe, thisunsafe implmust be removed or the actor confined to a single thread.Immediate options:
- Remove the impl and run the actor on a single-threaded executor/LocalSet or dedicated OS thread.
- If you must keep it, document invariants with a rigorous SAFETY comment and add compile-time assertions for the fields.
Apply this diff to revert and add guidance:
- unsafe impl Send for WindowsAudioCapture {} + // NOTE: WindowsAudioCapture is intentionally !Send. CPAL/WASAPI resources can be + // thread-affine. Run this actor on a LocalSet or a dedicated thread to preserve + // affinity and avoid UB from cross-thread moves. + // If you later decide to make this Send, add a rigorous SAFETY section explaining + // why all fields are Send and why moving across threads is sound.Optionally, if you can prove safety, at minimum annotate with invariants:
- unsafe impl Send for WindowsAudioCapture {} + // SAFETY: + // - Explain why scap_cpal::Capturer (and its internals) are safe to move across threads. + // - State that the actor model ensures no concurrent method calls. + // - Provide links/versions of CPAL guaranteeing Send on Windows. + unsafe impl Send for WindowsAudioCapture {}#!/usr/bin/env bash set -euo pipefail echo "=== Locate scap_cpal::Capturer definition and its Send/Sync impls ===" rg -nP --type=rust -C3 '^\s*(pub\s+)?struct\s+Capturer\b' || true rg -nP --type=rust -C2 'impl\s+(unsafe\s+)?(Send|Sync)\s+for\s+Capturer\b' || true echo echo "=== Check CPAL types used by Capturer for Send/Sync ===" rg -nP --type=rust -C3 '\bcpal::(Stream|Device|StreamConfig)\b' || true rg -n 'name = "cpal"' Cargo.lock -n -C1 || true echo echo "=== Where is WindowsAudioCapture spawned/run (confirm threading context) ===" rg -nP --type=rust -C3 'WindowsAudioCapture::spawn\(|spawn\(\s*WindowsAudioCapture' || true
🧹 Nitpick comments (2)
crates/recording/src/sources/screen_capture/windows.rs (2)
200-202: Downgrading StartCapturingAudio to String loses error structure and consistency with StartCapturingVideoSwitching from a typed error to
Stringmakes downstream handling and diagnostics worse and is inconsistent withStartCapturingVideo, which remains strongly typed. Prefer preserving the error type or at least keep the error chain.Two viable approaches:
- Keep the typed send error (consistent with video):
- #[error("StartCapturingAudio/{0}")] - StartCapturingAudio( - String, /* SendError<audio::StartCapturing, cpal::PlayStreamError> */ - ), + #[error("StartCapturingAudio/{0}")] + StartCapturingAudio( + kameo::error::SendError<audio::StartCapturing, cpal::PlayStreamError> + ),
- If trait bounds force erasure, preserve the error chain:
- #[error("StartCapturingAudio/{0}")] - StartCapturingAudio(String), + #[error("StartCapturingAudio")] + StartCapturingAudio { + #[source] + cause: Box<dyn std::error::Error + Send + Sync>, + },This keeps
std::error::Error::source()intact for better logs/telemetry.
294-294: Preserve error context when mapping the ask() failureConverting to plain string drops variant info (e.g., mailbox closed vs. actor gone). Map to a structured error (or keep typed).
If adopting approach (1) above:
- .map_err(|v| SourceError::StartCapturingAudio(v.to_string()))?; + .map_err(SourceError::StartCapturingAudio)?;If adopting approach (2) above:
- .map_err(|v| SourceError::StartCapturingAudio(v.to_string()))?; + .map_err(|e| SourceError::StartCapturingAudio { cause: Box::new(e) })?;At minimum, enrich the string to aid debugging:
- .map_err(|v| SourceError::StartCapturingAudio(v.to_string()))?; + .map_err(|e| SourceError::StartCapturingAudio(format!("ask(StartCapturing) send error: {e}")))?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
crates/recording/src/sources/screen_capture/windows.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
Summary by CodeRabbit