refactor: address rust-code-reviewer findings (pass 2) (#20)

* refactor: address rust-code-reviewer pass-2 findings

- wisp-audiokit: swap std::sync::mpsc for crossbeam-channel so the
  Sender shared via raw user_data is Sync. The Swift side may invoke
  on_result / on_log thunks from different threads concurrently; with
  std mpsc that formed overlapping &Sender references (UB). Also expose
  Session::recv_timeout.
- wisp-storage migrations: use Connection::unchecked_transaction so a
  panic mid-step doesn't leak an open transaction, and turn the u32
  overflow case into a statically-asserted unreachable so we can't
  silently skip a migration step.
- wisp-audiokit-sys: drop the non-macOS stub module entirely; the
  wrapper crate already cfg-gates every call site to macOS, so the
  stubs were dead code that forced callers to write unnecessary
  unsafe {}.
- session_runner: replace try_recv + sleep(20ms) with
  Session::recv_timeout(20ms) so events are forwarded immediately
  instead of waiting up to a tick.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs: correct Session SAFETY comment after crossbeam swap

`crossbeam_channel::Receiver<T>` is `Sync` (unlike `mpsc::Receiver`),
so the old "Receiver is `Send` but `!Sync`" justification no longer
matched the truth — `Session` is `!Sync` because of its `NonNull<…>`
field, not its receiver. Rewrite the SAFETY comment accordingly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* style: cargo fmt

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: pranc1ngpegasus

Cargo.lock

@@ -10,6 +10,7 @@ resolver = "3"
 
 [workspace.dependencies]
 chrono = { version = "0.4.44", default-features = false, features = ["clock", "std"] }
+crossbeam-channel = "0.5.15"
 gpui = "0.2.2"
 rusqlite = { version = "0.34.0", features = ["bundled", "chrono"] }
 serde = { version = "1.0.228", features = ["derive"] }

Cargo.toml

@@ -12,6 +12,12 @@ use std::time::Duration;
 
 use wisp_audiokit::{Event, Session, SessionError};
 
+/// How often the running session checks for UI commands (Stop / Shutdown)
+/// while waiting for the next audio event. Sets the worst-case latency for
+/// a Stop press to be honoured. Events themselves are delivered
+/// immediately — this only bounds the *idle* wake-up cadence.
+const CMD_POLL_INTERVAL: Duration = Duration::from_millis(20);
+
 /// Commands the UI sends to the worker.
 pub enum Command {
     Start { output_dir: PathBuf, locale: String },
@@ -118,9 +124,10 @@ fn run_session(
     }
     let _ = update_tx.send(Update::Started);
 
-    // Pump events until the UI asks to stop. We poll the cmd channel
-    // between event reads so a Stop request doesn't have to wait for the
-    // next audio event.
+    // Pump events until the UI asks to stop. Between events we wake at
+    // most every `CMD_POLL_INTERVAL` to check the command channel so a
+    // Stop request doesn't have to wait for the next audio event; when
+    // events are arriving we forward them immediately without polling.
     loop {
         match cmd_rx.try_recv() {
             Ok(Command::Stop) => break,
@@ -131,10 +138,8 @@ fn run_session(
             },
             Ok(Command::Start { .. }) | Err(TryRecvError::Empty) => {},
         }
-        if let Some(event) = session.try_recv() {
+        if let Some(event) = session.recv_timeout(CMD_POLL_INTERVAL) {
             let _ = update_tx.send(Update::Event(event));
-        } else {
-            std::thread::sleep(Duration::from_millis(20));
         }
     }
 

apps/wisp-desktop/src/session_runner.rs

@@ -4,12 +4,16 @@
 //! crate's `build.rs` into a static library (`libWispAudioKit.a`). The C ABI
 //! surface is hand-mirrored from `native/WispAudioKit/include/wisp_audiokit.h`.
 //!
-//! On non-macOS targets every binding is a stub returning a null pointer or
-//! a non-zero error code so the workspace stays buildable.
+//! `WispAudioKit` is macOS-only, so the `extern "C"` block below is gated to
+//! `target_os = "macos"`. On other targets the crate exposes only the type
+//! aliases and constants — there are no function symbols to link against,
+//! and consumers must (and do) cfg-gate any code that touches them.
 
 #![allow(unsafe_code, non_camel_case_types)]
 
-use std::os::raw::{c_char, c_int, c_void};
+#[cfg(target_os = "macos")]
+use std::os::raw::c_int;
+use std::os::raw::{c_char, c_void};
 
 /// Opaque handle for a `WispSession`. Construct via [`wisp_session_new`].
 #[repr(C)]
@@ -71,74 +75,3 @@ unsafe extern "C" {
     /// null. Invalidated by the next mutating call.
     pub fn wisp_session_last_error_message(session: *mut WispSession) -> *const c_char;
 }
-
-// ---- Non-macOS stubs ----------------------------------------------------
-
-#[cfg(not(target_os = "macos"))]
-mod stubs {
-    //! Non-macOS stubs. `WispAudioKit` is macOS-only, so on Linux / Windows
-    //! every entry point is a no-op returning null / `-1`. The functions are
-    //! marked `unsafe` only to keep their signatures interchangeable with
-    //! the real `extern "C"` declarations on macOS — they are trivially
-    //! safe to call.
-
-    use super::{WispLogCallback, WispResultCallback, WispSession, c_char, c_int, c_void};
-
-    /// Stub: always returns null on non-macOS.
-    ///
-    /// # Safety
-    /// Trivially safe; no pointers are dereferenced.
-    #[must_use]
-    pub unsafe fn wisp_audiokit_version() -> *const c_char {
-        core::ptr::null()
-    }
-
-    /// Stub: always returns null on non-macOS.
-    ///
-    /// # Safety
-    /// Trivially safe; no pointers are dereferenced.
-    pub unsafe fn wisp_session_new(
-        _output_dir: *const c_char,
-        _locale: *const c_char,
-        _on_result: Option<WispResultCallback>,
-        _on_log: Option<WispLogCallback>,
-        _user_data: *mut c_void,
-    ) -> *mut WispSession {
-        core::ptr::null_mut()
-    }
-
-    /// Stub: always returns `-1` on non-macOS.
-    ///
-    /// # Safety
-    /// Trivially safe; no pointers are dereferenced.
-    pub unsafe fn wisp_session_start(_session: *mut WispSession) -> c_int {
-        -1
-    }
-
-    /// Stub: no-op on non-macOS.
-    ///
-    /// # Safety
-    /// Trivially safe; no pointers are dereferenced.
-    pub unsafe fn wisp_session_stop(_session: *mut WispSession) {}
-
-    /// Stub: no-op on non-macOS.
-    ///
-    /// # Safety
-    /// Trivially safe; no pointers are dereferenced.
-    pub unsafe fn wisp_session_free(_session: *mut WispSession) {}
-
-    /// Stub: always returns null on non-macOS.
-    ///
-    /// # Safety
-    /// Trivially safe; no pointers are dereferenced.
-    #[must_use]
-    pub unsafe fn wisp_session_last_error_message(_session: *mut WispSession) -> *const c_char {
-        core::ptr::null()
-    }
-}
-
-#[cfg(not(target_os = "macos"))]
-pub use stubs::{
-    wisp_audiokit_version, wisp_session_free, wisp_session_last_error_message, wisp_session_new,
-    wisp_session_start, wisp_session_stop,
-};

crates/wisp-audiokit-sys/src/lib.rs

@@ -21,4 +21,5 @@ unsafe_code = "allow"
 [dependencies]
 wisp-audiokit-sys = { path = "../wisp-audiokit-sys" }
 wisp-core = { path = "../wisp-core" }
+crossbeam-channel = { workspace = true }
 thiserror = { workspace = true }

crates/wisp-audiokit/Cargo.toml

@@ -12,8 +12,9 @@ mod imp {
     use std::ffi::{CStr, CString};
     use std::path::Path;
     use std::ptr::NonNull;
-    use std::sync::mpsc;
+    use std::time::Duration;
 
+    use crossbeam_channel as channel;
     use wisp_audiokit_sys as sys;
     use wisp_core::SourceLabel;
 
@@ -66,20 +67,26 @@ mod imp {
     /// running session is always cleaned up.
     pub struct Session {
         handle: NonNull<sys::WispSession>,
-        receiver: mpsc::Receiver<Event>,
+        receiver: channel::Receiver<Event>,
         // Kept alive so the callbacks' user_data pointer stays valid for
         // as long as the Swift side might call them.
         _ctx: Box<CallbackContext>,
     }
 
     // SAFETY: Session owns the C handle and the receiver. The handle is
     // an opaque pointer we never deref ourselves; the C side serializes
-    // access internally. Receiver is `Send` but `!Sync`, which matches the
-    // semantics we expose.
+    // access internally, so it is sound to move the handle across threads.
+    // (`Session` stays `!Sync` overall because the `NonNull` field is
+    // `!Sync` — only `Send` needs the manual impl.)
     unsafe impl Send for Session {}
 
+    // Swift may invoke `on_result_thunk` / `on_log_thunk` from different
+    // threads. The thunks form `&CallbackContext` from a raw `user_data`
+    // pointer, so `CallbackContext` must be `Sync`. `crossbeam_channel`'s
+    // `Sender` is `Sync` (unlike `std::sync::mpsc::Sender`), which lets
+    // those callbacks fire concurrently without UB.
     struct CallbackContext {
-        sender: mpsc::Sender<Event>,
+        sender: channel::Sender<Event>,
     }
 
     impl Session {
@@ -108,7 +115,7 @@ mod imp {
             let locale_c =
                 CString::new(locale).map_err(|_| SessionError::InvalidLocale(locale.to_owned()))?;
 
-            let (sender, receiver) = mpsc::channel();
+            let (sender, receiver) = channel::unbounded();
             let ctx = Box::new(CallbackContext { sender });
             let user_data = std::ptr::from_ref::<CallbackContext>(ctx.as_ref()) as *mut _;
 
@@ -176,6 +183,17 @@ mod imp {
         pub fn recv(&self) -> Option<Event> {
             self.receiver.recv().ok()
         }
+
+        /// Block until the next event arrives or `timeout` elapses.
+        /// Returns `None` on timeout or when the session has been
+        /// dropped / closed.
+        #[must_use]
+        pub fn recv_timeout(
+            &self,
+            timeout: Duration,
+        ) -> Option<Event> {
+            self.receiver.recv_timeout(timeout).ok()
+        }
     }
 
     impl Drop for Session {
@@ -251,6 +269,7 @@ mod imp {
 #[cfg(not(target_os = "macos"))]
 mod imp {
     use std::path::Path;
+    use std::time::Duration;
 
     use wisp_core::SourceLabel;
 
@@ -312,6 +331,15 @@ mod imp {
         pub fn recv(&self) -> Option<Event> {
             None
         }
+
+        /// Always returns `None`.
+        #[must_use]
+        pub fn recv_timeout(
+            &self,
+            _timeout: Duration,
+        ) -> Option<Event> {
+            None
+        }
     }
 }
 

crates/wisp-audiokit/src/lib.rs

@@ -53,6 +53,14 @@ const MIGRATIONS: &[&str] = &[
     ",
 ];
 
+// Statically guard the `usize -> u32` conversion in `run` below: the
+// migration index is bounded by `MIGRATIONS.len()`, so as long as that
+// fits in a u32 the cast is infallible.
+const _: () = assert!(
+    MIGRATIONS.len() <= u32::MAX as usize,
+    "MIGRATIONS must fit in a u32 (PRAGMA user_version is u32)",
+);
+
 /// Apply any pending migrations, advancing `PRAGMA user_version` as each
 /// step succeeds. Idempotent: running on an already-current database is a
 /// no-op.
@@ -64,20 +72,16 @@ pub(crate) fn run(conn: &Connection) -> Result<()> {
     let current: u32 = conn.query_row("PRAGMA user_version", [], |row| row.get(0))?;
 
     for (i, sql) in MIGRATIONS.iter().enumerate().skip(current as usize) {
-        // i+1 <= MIGRATIONS.len(), a compile-time constant of small size,
-        // so this conversion can't overflow u32.
         let Ok(target) = u32::try_from(i + 1) else {
-            continue;
+            unreachable!("MIGRATIONS.len() statically asserted to fit in u32");
         };
-        let tx_result = (|| -> std::result::Result<(), rusqlite::Error> {
-            conn.execute_batch("BEGIN;")?;
-            conn.execute_batch(sql)?;
-            conn.pragma_update(None, "user_version", target)?;
-            conn.execute_batch("COMMIT;")?;
-            Ok(())
+        let result = (|| -> std::result::Result<(), rusqlite::Error> {
+            let tx = conn.unchecked_transaction()?;
+            tx.execute_batch(sql)?;
+            tx.pragma_update(None, "user_version", target)?;
+            tx.commit()
         })();
-        if let Err(source) = tx_result {
-            let _ = conn.execute_batch("ROLLBACK;");
+        if let Err(source) = result {
             return Err(StorageError::Migration { target, source });
         }
     }