From b20e6b663a8f2d7e84bc261f9aad0de213d94ad8 Mon Sep 17 00:00:00 2001 From: Edison Jwa Date: Thu, 11 Jun 2026 09:46:51 +0900 Subject: [PATCH] fix(audio): replace Mutex unwrap with poisoned-mutex recovery (TODO-006) Replace 42 .lock().unwrap() calls with .unwrap_or_else(|e| e.into_inner()) across 7 files. Poisoned mutex recovery prevents panics in realtime audio callbacks. Add SAFETY comment to WebRtcFallbackVad Send impl (TODO-007). --- .../chanora_audio/src/android_voice_unit.rs | 2 +- crates/chanora_audio/src/engine.rs | 62 +++++++++---------- crates/chanora_audio/src/ios_voice_unit.rs | 8 +-- .../chanora_audio/src/ptt_backends/macos.rs | 2 +- crates/chanora_audio/src/sdl_output.rs | 2 +- crates/chanora_audio/src/vad/mod.rs | 13 ++-- crates/chanora_audio/tests/ptt_privacy.rs | 4 +- 7 files changed, 48 insertions(+), 45 deletions(-) diff --git a/crates/chanora_audio/src/android_voice_unit.rs b/crates/chanora_audio/src/android_voice_unit.rs index 75ed66f..874a318 100644 --- a/crates/chanora_audio/src/android_voice_unit.rs +++ b/crates/chanora_audio/src/android_voice_unit.rs @@ -855,7 +855,7 @@ impl AndroidVoiceUnit { // by the capture callback's WebRtcApmProcessor. { use crate::audio_processing::EffectOwner; - let mut apm_cfg = apm_config_clone.lock().unwrap(); + let mut apm_cfg = apm_config_clone.lock().unwrap_or_else(|e| e.into_inner()); let hw_aec = hw_effects.aec.is_some(); let hw_ns = hw_effects.ns.is_some(); let hw_agc = hw_effects.agc.is_some(); diff --git a/crates/chanora_audio/src/engine.rs b/crates/chanora_audio/src/engine.rs index c87a3fe..b9d7479 100644 --- a/crates/chanora_audio/src/engine.rs +++ b/crates/chanora_audio/src/engine.rs @@ -481,7 +481,7 @@ impl AudioEngine { crate::android_voice_unit::chanora_android_abandon_audio_focus(); crate::android_voice_unit::clear_global_event_sender(); - if let Some(mut unit) = android_voice_unit.lock().unwrap().take() { + if let Some(mut unit) = android_voice_unit.lock().unwrap_or_else(|e| e.into_inner()).take() { use crate::mobile_voice_backend::MobileVoiceAudioBackend; if let Err(e) = unit.close() { warn!( @@ -524,7 +524,7 @@ impl AudioEngine { ), } - let cfg_av = android_voice_stream_config.lock().unwrap().clone(); + let cfg_av = android_voice_stream_config.lock().unwrap_or_else(|e| e.into_inner()).clone(); let params = crate::mobile_voice_backend::VoiceAudioParams { voice_out_tx: voice_out_tx.clone(), transmit_active: transmit_gate.flag_arc(), @@ -577,7 +577,7 @@ impl AudioEngine { "android: bluetooth route re-engaged after backend disconnect" ); } - *android_voice_unit.lock().unwrap() = Some(reopened); + *android_voice_unit.lock().unwrap_or_else(|e| e.into_inner()) = Some(reopened); } Err(e) => warn!( target: "chanora_audio", @@ -941,7 +941,7 @@ impl AudioEngine { match item { Some(v) => { let id = SessionAudioId(v.from_client); - let mut h = handler_for_task.lock().unwrap(); + let mut h = handler_for_task.lock().unwrap_or_else(|e| e.into_inner()); if let Err(e) = h.handle_packet(id, v.packet) { debug!(target: "chanora_audio", error = %e, "decode failed"); } else { @@ -1283,7 +1283,7 @@ impl AudioEngine { match item { Some(v) => { let id = SessionAudioId(v.from_client); - let mut h = handler_for_task.lock().unwrap(); + let mut h = handler_for_task.lock().unwrap_or_else(|e| e.into_inner()); let res = h.handle_packet(id, v.packet); drop(h); match res { @@ -1371,12 +1371,12 @@ impl AudioEngine { not(target_os = "android") ))] { - let _ = self._input_stream.lock().unwrap().take(); - let _ = self._output_stream.lock().unwrap().take(); + let _ = self._input_stream.lock().unwrap_or_else(|e| e.into_inner()).take(); + let _ = self._output_stream.lock().unwrap_or_else(|e| e.into_inner()).take(); } #[cfg(any(target_os = "ios", target_os = "macos"))] { - let _ = self._ios_voice_backend.lock().unwrap().take(); + let _ = self._ios_voice_backend.lock().unwrap_or_else(|e| e.into_inner()).take(); } // SDD-115 reverse-order teardown on Android: // 1) stop Bluetooth SCO + abandon audio focus; @@ -1390,7 +1390,7 @@ impl AudioEngine { crate::android_voice_unit::chanora_android_stop_bluetooth_sco(); crate::android_voice_unit::chanora_android_abandon_audio_focus(); crate::android_voice_unit::clear_global_event_sender(); - if let Some(mut unit) = self._android_voice_unit.lock().unwrap().take() { + if let Some(mut unit) = self._android_voice_unit.lock().unwrap_or_else(|e| e.into_inner()).take() { use crate::mobile_voice_backend::MobileVoiceAudioBackend; if let Err(e) = unit.close() { warn!( @@ -1456,7 +1456,7 @@ impl AudioEngine { pub fn ios_restart_voice_unit(&self) -> Result<(), AudioError> { #[cfg(any(target_os = "ios", target_os = "macos"))] { - let mut guard = self._ios_voice_backend.lock().unwrap(); + let mut guard = self._ios_voice_backend.lock().unwrap_or_else(|e| e.into_inner()); let backend = guard .as_mut() .ok_or_else(|| AudioError::Backend("ios voice backend not running".to_string()))?; @@ -1479,7 +1479,7 @@ impl AudioEngine { crate::android_voice_unit::chanora_android_abandon_audio_focus(); crate::android_voice_unit::clear_global_event_sender(); - if let Some(mut unit) = self._android_voice_unit.lock().unwrap().take() { + if let Some(mut unit) = self._android_voice_unit.lock().unwrap_or_else(|e| e.into_inner()).take() { unit.close().map_err(|e| { AudioError::Backend(format!( "android: failed to close Oboe voice unit during route restart: {e}" @@ -1501,7 +1501,7 @@ impl AudioEngine { ); } - let cfg_av = self.android_voice_stream_config.lock().unwrap().clone(); + let cfg_av = self.android_voice_stream_config.lock().unwrap_or_else(|e| e.into_inner()).clone(); let params = crate::mobile_voice_backend::VoiceAudioParams { voice_out_tx: self.voice_out_tx.clone(), transmit_active: self.transmit_gate.flag_arc(), @@ -1549,7 +1549,7 @@ impl AudioEngine { ); } - let mut guard = self._android_voice_unit.lock().unwrap(); + let mut guard = self._android_voice_unit.lock().unwrap_or_else(|e| e.into_inner()); *guard = Some(reopened); Ok(()) } @@ -1563,7 +1563,7 @@ impl AudioEngine { pub fn ios_pause_voice_unit(&self) -> Result<(), AudioError> { #[cfg(any(target_os = "ios", target_os = "macos"))] { - let mut guard = self._ios_voice_backend.lock().unwrap(); + let mut guard = self._ios_voice_backend.lock().unwrap_or_else(|e| e.into_inner()); let unit = guard .as_mut() .ok_or_else(|| AudioError::Backend("ios voice backend not running".to_string()))?; @@ -1579,7 +1579,7 @@ impl AudioEngine { pub fn ios_resume_voice_unit(&self) -> Result<(), AudioError> { #[cfg(any(target_os = "ios", target_os = "macos"))] { - let mut guard = self._ios_voice_backend.lock().unwrap(); + let mut guard = self._ios_voice_backend.lock().unwrap_or_else(|e| e.into_inner()); let unit = guard .as_mut() .ok_or_else(|| AudioError::Backend("ios voice backend not running".to_string()))?; @@ -1639,7 +1639,7 @@ impl AudioEngine { /// Current audio-processing config snapshot. pub fn audio_processing_config_snapshot(&self) -> crate::AudioProcessingConfig { - self.audio_processing_config.lock().unwrap().clone() + self.audio_processing_config.lock().unwrap_or_else(|e| e.into_inner()).clone() } /// Apply a voice-processing config after validating iOS invariants. @@ -1655,7 +1655,7 @@ impl AudioEngine { } #[cfg(not(any(target_os = "ios", target_os = "macos", target_os = "android")))] self.apply_desktop_vad_backend(&config); - let mut guard = self.audio_processing_config.lock().unwrap(); + let mut guard = self.audio_processing_config.lock().unwrap_or_else(|e| e.into_inner()); *guard = config; Ok(()) } @@ -1681,7 +1681,7 @@ impl AudioEngine { /// Current voice-processing stats snapshot. pub fn audio_processing_stats(&self) -> crate::AudioProcessingStats { - let config = self.audio_processing_config.lock().unwrap().clone(); + let config = self.audio_processing_config.lock().unwrap_or_else(|e| e.into_inner()).clone(); self.audio_processing_stats.snapshot(&config) } @@ -1816,7 +1816,7 @@ fn apply_desktop_vad_backend_to_worker( audio_processing_stats: &crate::SharedAudioProcessingStats, ) { if config.vad_backend != crate::VadBackend::SileroOnnx { - let mut worker_guard = silero_vad_worker.lock().unwrap(); + let mut worker_guard = silero_vad_worker.lock().unwrap_or_else(|e| e.into_inner()); if worker_guard.is_some() { info!( target: "chanora_audio", @@ -1841,12 +1841,12 @@ fn apply_desktop_vad_backend_to_worker( ); let new_worker = crate::vad::silero_onnx::SileroOnnxVadWorker::try_new(&model_path); { - let mut worker_guard = silero_vad_worker.lock().unwrap(); + let mut worker_guard = silero_vad_worker.lock().unwrap_or_else(|e| e.into_inner()); *worker_guard = new_worker; } // Check result after releasing the lock. Re-acquire is cheap and // ensures we log the correct state without holding the mutex. - let worker_installed = silero_vad_worker.lock().unwrap().is_some(); + let worker_installed = silero_vad_worker.lock().unwrap_or_else(|e| e.into_inner()).is_some(); if worker_installed { info!( target: "chanora_audio", @@ -1926,21 +1926,21 @@ mod tests { #[cfg(not(any(target_os = "ios", target_os = "macos", target_os = "android")))] #[test] fn desktop_startup_audio_processing_state_applies_default_silero_fallback() { - let _guard = crate::vad::SILERO_MODEL_PATH_TEST_LOCK.lock().unwrap(); + let _guard = crate::vad::SILERO_MODEL_PATH_TEST_LOCK.lock().unwrap_or_else(|e| e.into_inner()); crate::vad::clear_silero_model_path_for_test(); let (config, stats, worker) = new_desktop_audio_processing_state(); assert_eq!( - config.lock().unwrap().vad_backend, + config.lock().unwrap_or_else(|e| e.into_inner()).vad_backend, crate::VadBackend::SileroOnnx, "desktop startup config should keep the default Silero backend selected" ); assert!( - worker.lock().unwrap().is_none(), + worker.lock().unwrap_or_else(|e| e.into_inner()).is_none(), "missing startup model should not create an ONNX worker" ); - let snapshot = stats.snapshot(&config.lock().unwrap()); + let snapshot = stats.snapshot(&config.lock().unwrap_or_else(|e| e.into_inner())); assert!( snapshot.vad_fallback_active, "desktop startup should mark WebRTC fallback active when the default Silero worker cannot load" @@ -1983,7 +1983,7 @@ mod tests { #[cfg(not(any(target_os = "ios", target_os = "macos", target_os = "android")))] #[test] fn desktop_capture_silero_selection_reports_fallback_when_worker_unavailable() { - let _guard = crate::vad::SILERO_MODEL_PATH_TEST_LOCK.lock().unwrap(); + let _guard = crate::vad::SILERO_MODEL_PATH_TEST_LOCK.lock().unwrap_or_else(|e| e.into_inner()); crate::vad::clear_silero_model_path_for_test(); let gate = crate::ptt::AudioTransmitGate::new(false); @@ -2025,7 +2025,7 @@ mod tests { } capture.ingest(&voiced); - let snapshot = stats.snapshot(&config.lock().unwrap()); + let snapshot = stats.snapshot(&config.lock().unwrap_or_else(|e| e.into_inner())); assert_eq!(snapshot.vad_backend, crate::VadBackend::SileroOnnx); assert!( snapshot.vad_fallback_active, @@ -2132,7 +2132,7 @@ mod tests { capture.process_10ms_capture_frame(&voiced); - let snapshot = stats.snapshot(&config.lock().unwrap()); + let snapshot = stats.snapshot(&config.lock().unwrap_or_else(|e| e.into_inner())); assert!( snapshot.vad_fallback_active, "stale Silero worker output should report active WebRTC fallback" @@ -2725,7 +2725,7 @@ where .build_input_stream( *config, move |data: &[T], _: &cpal::InputCallbackInfo| { - let mut s = state.lock().unwrap(); + let mut s = state.lock().unwrap_or_else(|e| e.into_inner()); s.ingest(data); }, move |e| { @@ -2816,7 +2816,7 @@ where // only for the duration of fill_buffer; the inbound // forwarder uses handle_packet which is queue-fast. { - let mut h = handler.lock().unwrap(); + let mut h = handler.lock().unwrap_or_else(|e| e.into_inner()); h.fill_buffer(&mut scratch[..needed]); } @@ -2835,7 +2835,7 @@ where // Resample 48 kHz stereo → device-rate × // device-channels with continuity across // callback boundaries. - let mut state = resample_state.lock().unwrap(); + let mut state = resample_state.lock().unwrap_or_else(|e| e.into_inner()); let mut pos = state.pos; let mut last_l = state.last_l; let mut last_r = state.last_r; diff --git a/crates/chanora_audio/src/ios_voice_unit.rs b/crates/chanora_audio/src/ios_voice_unit.rs index b730325..7b70095 100644 --- a/crates/chanora_audio/src/ios_voice_unit.rs +++ b/crates/chanora_audio/src/ios_voice_unit.rs @@ -549,7 +549,7 @@ impl IosVoiceUnit { let unit_arc2 = Arc::clone(&unit_arc); dispatch2::DispatchQueue::main().exec_async(move || { - let mut guard = unit_arc2.lock().unwrap(); + let mut guard = unit_arc2.lock().unwrap_or_else(|e| e.into_inner()); let unit = guard.as_mut().unwrap(); let _ = tx.send(op(unit)); }); @@ -559,7 +559,7 @@ impl IosVoiceUnit { Err(_) => Err("vpio lifecycle: main thread channel closed unexpectedly".to_string()), }; - self.unit = unit_arc.lock().unwrap().take(); + self.unit = unit_arc.lock().unwrap_or_else(|e| e.into_inner()).take(); result.map_err(AudioError::Backend) } @@ -1072,7 +1072,7 @@ impl IosVoiceUnit { let unit_arc2 = unit_arc.clone(); dispatch2::DispatchQueue::main().exec_async(move || { - let mut guard = unit_arc2.lock().unwrap(); + let mut guard = unit_arc2.lock().unwrap_or_else(|e| e.into_inner()); let u = guard.as_mut().unwrap(); let result = u .initialize() @@ -1094,7 +1094,7 @@ impl IosVoiceUnit { } } - unit = unit_arc.lock().unwrap().take().unwrap(); + unit = unit_arc.lock().unwrap_or_else(|e| e.into_inner()).take().unwrap(); } info!( diff --git a/crates/chanora_audio/src/ptt_backends/macos.rs b/crates/chanora_audio/src/ptt_backends/macos.rs index 96b82d8..a4d14d8 100644 --- a/crates/chanora_audio/src/ptt_backends/macos.rs +++ b/crates/chanora_audio/src/ptt_backends/macos.rs @@ -543,7 +543,7 @@ impl DesktopPttBackend for MacOSEventTapBackend { } let runloop = unsafe { CFRunLoopGetCurrent() }; { - let mut g = worker_runloop.lock().unwrap(); + let mut g = worker_runloop.lock().unwrap_or_else(|e| e.into_inner()); *g = Some(RunLoopHandle(runloop)); } unsafe { diff --git a/crates/chanora_audio/src/sdl_output.rs b/crates/chanora_audio/src/sdl_output.rs index 13fa98e..de05e22 100644 --- a/crates/chanora_audio/src/sdl_output.rs +++ b/crates/chanora_audio/src/sdl_output.rs @@ -174,7 +174,7 @@ impl AudioCallback for TsPlaybackCallback { // in the cpal path, but the upstream design has shipped // this way for years. { - let mut data = self.handler.lock().unwrap(); + let mut data = self.handler.lock().unwrap_or_else(|e| e.into_inner()); let _removed_ids = data.fill_buffer(buffer); // `_removed_ids` is the list of clients whose stream the // handler just finished draining. We could publish that diff --git a/crates/chanora_audio/src/vad/mod.rs b/crates/chanora_audio/src/vad/mod.rs index fef5a7c..4d522e3 100644 --- a/crates/chanora_audio/src/vad/mod.rs +++ b/crates/chanora_audio/src/vad/mod.rs @@ -42,9 +42,12 @@ pub struct WebRtcFallbackVad { frame_i16: [i16; INPUT_FRAME_10MS], } -// `webrtc_vad::Vad` owns an FFI pointer and is only touched from the -// capture thread after construction. Moving the wrapper between threads is -// safe; sharing it concurrently is not required and not implemented. +// SAFETY: `webrtc_vad::Vad` wraps an opaque FFI pointer to the WebRTC C VAD +// state. The underlying C struct has no interior mutability that would cause +// data races when moved between threads — `WebRtcVad_Process()` reads/writes +// the struct exclusively through the passed pointer with no shared static state. +// This wrapper is only used from a single capture thread after construction; +// we never share `&WebRtcFallbackVad` across threads (no `Sync` impl). unsafe impl Send for WebRtcFallbackVad {} impl Default for WebRtcFallbackVad { @@ -250,7 +253,7 @@ mod tests { #[test] fn set_silero_model_path_rejects_missing_file() { - let _guard = SILERO_MODEL_PATH_TEST_LOCK.lock().unwrap(); + let _guard = SILERO_MODEL_PATH_TEST_LOCK.lock().unwrap_or_else(|e| e.into_inner()); clear_silero_model_path_for_test(); let result = set_silero_model_path("/definitely/not/a/silero_vad.onnx"); @@ -261,7 +264,7 @@ mod tests { #[test] fn set_silero_model_path_updates_override_and_epoch() { - let _guard = SILERO_MODEL_PATH_TEST_LOCK.lock().unwrap(); + let _guard = SILERO_MODEL_PATH_TEST_LOCK.lock().unwrap_or_else(|e| e.into_inner()); clear_silero_model_path_for_test(); let path = diff --git a/crates/chanora_audio/tests/ptt_privacy.rs b/crates/chanora_audio/tests/ptt_privacy.rs index bff49a1..7edae55 100644 --- a/crates/chanora_audio/tests/ptt_privacy.rs +++ b/crates/chanora_audio/tests/ptt_privacy.rs @@ -82,7 +82,7 @@ struct RecordingLayer { impl RecordingLayer { fn snapshot(&self) -> Vec { - self.records.lock().unwrap().clone() + self.records.lock().unwrap_or_else(|e| e.into_inner()).clone() } } @@ -118,7 +118,7 @@ where target: event.metadata().target().to_string(), field_names: names.0, }; - self.records.lock().unwrap().push(captured); + self.records.lock().unwrap_or_else(|e| e.into_inner()).push(captured); } }