Fix external stt server startups #1402
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughRemoves the app-level AppExt::setup_local_ai API and its automatic startup; moves local LLM/STT startup into plugin RunEvent handlers triggered when the main window is focused; makes downloaded Pro STT models selectable; adds a local-llm dependency; updates macOS mic logging and external STT spawn timing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant OS as OS/Windowing
participant App as Tauri App
participant LLM as Local LLM Plugin
participant STT as Local STT Plugin
User->>OS: Focus main window
OS-->>App: RunEvent::WindowEvent(Focused=true, label="main")
App->>LLM: on_event(...)
Note right of LLM: parse label → if Main\nblock_in_place + start_server()
App->>STT: on_event(...)
Note right of STT: parse label → if Main\nblock_in_place + start_server(None)
sequenceDiagram
autonumber
participant AppBefore as Desktop App (Before)
participant Ext as AppExt
Note over AppBefore,Ext: Previous startup flow (removed)
AppBefore->>Ext: setup_local_ai()
Ext->>LLM: start LLM if available
Ext->>STT: start STT if available
Note over AppBefore: This startup path no longer exists
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ 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). (5)
✨ 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
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
plugins/local-stt/src/server/external.rs (1)
59-63: Replace fixed 500ms sleep with a short bounded probe to avoid flakinessThe fixed delay reduces flakes but can still race under load or slower teardown. Polling for port vacancy with a short backoff is more robust and avoids unnecessary waits when the port frees quickly.
Apply this diff locally to replace the fixed sleep with a bounded probe:
@@ let port = 50060; let _ = port_killer::kill(port); - tokio::time::sleep(std::time::Duration::from_millis(500)).await; + // Wait up to ~2s for the port to vacate; break early once no listener is present. + for _ in 0..20 { + if tokio::net::TcpStream::connect(("127.0.0.1", port)).await.is_err() { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + }This keeps behavior simple and avoids adding extra error-handling paths while improving reliability across platforms.
plugins/local-llm/Cargo.toml (1)
34-35: Gate tauri-plugin-windows to Windows targets to avoid cross-platform build surprisesIf
tauri-plugin-windowscontains any Windows-specific bindings, compiling on macOS/Linux could fail or slow CI. Consider moving it under a target-specific section.Proposed Cargo.toml snippet:
[target.'cfg(windows)'.dependencies] tauri-plugin-windows = { workspace = true }Please confirm non-Windows builds (Linux/macOS) remain green after this addition.
apps/desktop/src/components/settings/components/ai/stt-view-local.tsx (1)
251-255: Pro models re-enabled; update UX copy to avoid confusionSetting
disabled={false}makes Pro models clickable (as intended when downloaded). The section still says “Temporarily disabled…”, which now conflicts with behavior. Recommend updating the copy.Suggested edit (elsewhere in the same file, SectionHeader for Pro Models):
- description="Temporarily disabled to fix some issues. Sorry :(" + description="Advanced inference powered by AM models. Click a downloaded model to use it."Also verify that selecting a Pro model consistently restarts the server and applies the model (watch for rapid toggles causing overlapping restarts).
plugins/local-llm/src/events.rs (2)
21-25: Avoid blocking the event loop; use tauri::async_runtime::spawn instead of block_in_place/block_onCalling tokio::task::block_in_place + Handle::current().block_on from a Tauri event callback risks UI stalls and, depending on the thread/runtime context, can panic. Spawning the async task is safer and keeps the UI responsive while preserving idempotency if start_server() already guards concurrent starts.
Apply this diff:
- tokio::task::block_in_place(|| { - tokio::runtime::Handle::current().block_on(async { - let _ = app.start_server().await; - }); - }); + let app = app.clone(); + tauri::async_runtime::spawn(async move { + let _ = app.start_server().await; + });Please verify there are no regressions on macOS/Windows/Linux when focusing the main window repeatedly (no duplicate server processes and no missed startups).
7-13: Improve parse error diagnostics with structured loggingIncluding the label helps correlate which window label failed to parse.
- Err(e) => { - tracing::warn!("parse_error: {:?}", e); + Err(e) => { + tracing::warn!(%label, error = ?e, "parse_error"); return; }plugins/local-llm/src/lib.rs (1)
17-17: Prefer explicit import over wildcard for eventsImporting only on_event keeps the surface minimal and avoids future unused-import warnings if events gains more items.
-use events::*; +use crate::events::on_event;plugins/local-stt/src/events.rs (2)
21-25: Mirror non-blocking startup here as wellSame reasoning as the local-llm plugin: avoid block_in_place/block_on in event callbacks; spawn the async task instead.
- tokio::task::block_in_place(|| { - tokio::runtime::Handle::current().block_on(async { - let _ = app.start_server(None).await; - }); - }); + let app = app.clone(); + tauri::async_runtime::spawn(async move { + let _ = app.start_server(None).await; + });Validation suggestion:
- Focus/unfocus the main window rapidly; ensure only one external STT process ends up running.
- Confirm no UI hitch on focus.
6-13: Optional: enrich parse-error log for easier triageSame nit as in local-llm: include the offending label in the log.
- Err(e) => { - tracing::warn!("parse_error: {:?}", e); + Err(e) => { + tracing::warn!(%label, error = ?e, "parse_error"); return; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
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 ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
apps/desktop/src-tauri/src/ext.rs(0 hunks)apps/desktop/src-tauri/src/lib.rs(0 hunks)apps/desktop/src/components/settings/components/ai/stt-view-local.tsx(1 hunks)crates/detect/src/mic/macos.rs(3 hunks)plugins/local-llm/Cargo.toml(1 hunks)plugins/local-llm/src/events.rs(1 hunks)plugins/local-llm/src/lib.rs(2 hunks)plugins/local-stt/src/events.rs(1 hunks)plugins/local-stt/src/lib.rs(0 hunks)plugins/local-stt/src/server/external.rs(1 hunks)plugins/local-stt/src/server/internal.rs(1 hunks)
💤 Files with no reviewable changes (3)
- apps/desktop/src-tauri/src/lib.rs
- apps/desktop/src-tauri/src/ext.rs
- plugins/local-stt/src/lib.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
plugins/local-stt/src/server/external.rsplugins/local-stt/src/server/internal.rsplugins/local-llm/src/events.rscrates/detect/src/mic/macos.rsapps/desktop/src/components/settings/components/ai/stt-view-local.tsxplugins/local-stt/src/events.rsplugins/local-llm/src/lib.rs
🧬 Code graph analysis (3)
plugins/local-llm/src/events.rs (2)
plugins/windows/js/bindings.gen.ts (1)
HyprWindow(73-73)plugins/local-stt/src/events.rs (1)
on_event(4-32)
plugins/local-stt/src/events.rs (2)
plugins/local-llm/src/events.rs (1)
on_event(4-32)plugins/windows/src/events.rs (3)
app(33-33)window(12-12)window(35-35)
plugins/local-llm/src/lib.rs (1)
plugins/local-llm/src/events.rs (1)
on_event(4-32)
⏰ 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). (6)
- GitHub Check: build (macos, x86_64-apple-darwin, macos-latest)
- GitHub Check: build (windows, x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: build (macos, aarch64-apple-darwin, macos-latest)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci (windows, windows-latest)
🔇 Additional comments (6)
plugins/local-stt/src/server/internal.rs (1)
58-60: LGTM: stop log now includes base_url for better correlationIncluding the base_url in the shutdown log will help trace lifecycles when multiple servers are involved. Looks good.
crates/detect/src/mic/macos.rs (1)
211-214: LGTM: migrated println! to tracing with appropriate levelsThe switch to structured tracing (error/info/warn) improves observability and aligns with the rest of the codebase. Message keys are consistent (snake_case). Good change.
Also applies to: 232-233, 247-251
plugins/local-llm/src/events.rs (1)
15-17: Scope-correct filtering to Main window looks goodEarly-returning for non-Main windows avoids unnecessary work on background/floating windows.
plugins/local-llm/src/lib.rs (1)
88-88: Event hook registration is correctly placedWiring .on_event(on_event) on the plugin builder is correct and matches the new events module exposure.
plugins/local-stt/src/events.rs (2)
15-17: Main-window-only gating is correctKeeping startup limited to HyprWindow::Main aligns with the intent to avoid side-effects from auxiliary windows.
19-28: Confirm lifecycle expectations after removing close/destroy cleanupThe previous close/destroy handling (cleanup of internal/external servers and download tasks) was removed. If the app can close the main window while staying resident (tray/minimized), confirm we don’t leak an external STT process or leave ports occupied until a future run (you did add a port_killer delay elsewhere, but graceful shutdown may still be preferable).
Would you like me to propose a minimal Exit/ExitRequested hook to stop the external server only on app quit?
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
| tauri::WindowEvent::Focused(true) => { | ||
| tokio::task::block_in_place(|| { | ||
| tokio::runtime::Handle::current().block_on(async { | ||
| let _ = app.start_server().await; |
There was a problem hiding this comment.
Errors from start_server are ignored; log failures to aid diagnosis and reliability.
Prompt for AI agents
Address the following comment on plugins/local-llm/src/events.rs at line 23:
<comment>Errors from start_server are ignored; log failures to aid diagnosis and reliability.</comment>
<file context>
@@ -0,0 +1,32 @@
+use crate::LocalLlmPluginExt;
+use tauri_plugin_windows::HyprWindow;
+
+pub fn on_event<R: tauri::Runtime>(app: &tauri::AppHandle<R>, event: &tauri::RunEvent) {
+ match event {
+ tauri::RunEvent::WindowEvent { label, event, .. } => {
+ let hypr_window = match label.parse::<HyprWindow>() {
+ Ok(window) => window,
+ Err(e) => {
</file context>
|
|
||
| match event { | ||
| tauri::WindowEvent::Focused(true) => { | ||
| tokio::task::block_in_place(|| { |
There was a problem hiding this comment.
Blocking the event loop with block_in_place in a Tauri event handler can freeze or delay UI/event processing; spawn the async task instead of blocking.
Prompt for AI agents
Address the following comment on plugins/local-llm/src/events.rs at line 21:
<comment>Blocking the event loop with block_in_place in a Tauri event handler can freeze or delay UI/event processing; spawn the async task instead of blocking.</comment>
<file context>
@@ -0,0 +1,32 @@
+use crate::LocalLlmPluginExt;
+use tauri_plugin_windows::HyprWindow;
+
+pub fn on_event<R: tauri::Runtime>(app: &tauri::AppHandle<R>, event: &tauri::RunEvent) {
+ match event {
+ tauri::RunEvent::WindowEvent { label, event, .. } => {
+ let hypr_window = match label.parse::<HyprWindow>() {
+ Ok(window) => window,
+ Err(e) => {
</file context>
| <ModelEntry | ||
| key={model.key} | ||
| disabled={true} | ||
| disabled={false} |
There was a problem hiding this comment.
Pro models are enabled here while the section description says they are temporarily disabled; update the description or keep this disabled to avoid confusing UX.
Prompt for AI agents
Address the following comment on apps/desktop/src/components/settings/components/ai/stt-view-local.tsx at line 253:
<comment>Pro models are enabled here while the section description says they are temporarily disabled; update the description or keep this disabled to avoid confusing UX.</comment>
<file context>
@@ -250,7 +250,7 @@ function ProModelsSection({
{proModels.data?.map((model) => (
<ModelEntry
key={model.key}
- disabled={true}
+ disabled={false}
model={model}
selectedSTTModel={selectedSTTModel}
</file context>
| ) -> Result<ServerHandle, crate::Error> { | ||
| let port = 50060; | ||
| let _ = port_killer::kill(port); | ||
| tokio::time::sleep(std::time::Duration::from_millis(500)).await; |
There was a problem hiding this comment.
Arbitrary fixed 500ms sleep after killing the port can cause flaky startups and unnecessary delay; prefer awaiting port availability or process readiness with a bounded retry instead.
Prompt for AI agents
Address the following comment on plugins/local-stt/src/server/external.rs at line 61:
<comment>Arbitrary fixed 500ms sleep after killing the port can cause flaky startups and unnecessary delay; prefer awaiting port availability or process readiness with a bounded retry instead.</comment>
<file context>
@@ -58,6 +58,7 @@ pub async fn run_server(
) -> Result<ServerHandle, crate::Error> {
let port = 50060;
let _ = port_killer::kill(port);
+ tokio::time::sleep(std::time::Duration::from_millis(500)).await;
let (mut rx, child) = cmd.args(["--port", &port.to_string()]).spawn()?;
</file context>
|
|
||
| match event { | ||
| tauri::WindowEvent::Focused(true) => { | ||
| tokio::task::block_in_place(|| { |
There was a problem hiding this comment.
Blocking the runtime thread with block_in_place + block_on inside an event handler may stall the UI/event loop; prefer spawning the async task instead.
Prompt for AI agents
Address the following comment on plugins/local-stt/src/events.rs at line 21:
<comment>Blocking the runtime thread with block_in_place + block_on inside an event handler may stall the UI/event loop; prefer spawning the async task instead.</comment>
<file context>
@@ -1,54 +1,32 @@
-use tauri::Manager;
-
-use crate::{LocalSttPluginExt, SharedState};
+use crate::LocalSttPluginExt;
use tauri_plugin_windows::HyprWindow;
pub fn on_event<R: tauri::Runtime>(app: &tauri::AppHandle<R>, event: &tauri::RunEvent) {
- let state = app.state::<SharedState>();
-
</file context>
| tokio::task::block_in_place(|| { | ||
| tokio::runtime::Handle::current().block_on(async { | ||
| let _ = app.start_server(None).await; | ||
| }); |
There was a problem hiding this comment.
Result from start_server is ignored, hiding startup failures and making diagnostics harder; log or handle errors rather than discarding them.
Prompt for AI agents
Address the following comment on plugins/local-stt/src/events.rs at line 23:
<comment>Result from start_server is ignored, hiding startup failures and making diagnostics harder; log or handle errors rather than discarding them.</comment>
<file context>
@@ -1,54 +1,32 @@
-use tauri::Manager;
-
-use crate::{LocalSttPluginExt, SharedState};
+use crate::LocalSttPluginExt;
use tauri_plugin_windows::HyprWindow;
pub fn on_event<R: tauri::Runtime>(app: &tauri::AppHandle<R>, event: &tauri::RunEvent) {
- let state = app.state::<SharedState>();
-
</file context>
| let _ = app.start_server(None).await; | |
| if let Err(e) = app.start_server(None).await { tracing::warn!("start_server_error: {:?}", e); } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
plugins/local-stt/src/ext.rs (4)
258-262: Replace fixed sleeps with readiness polling and loginiterrors instead of ignoring themFixed waits are brittle and can still race. Also, discarding
initerrors hides real startup failures.Apply this diff to poll health with a short timeout and to log
initfailures:- let server = external::run_server(cmd, am_key).await?; - tokio::time::sleep(std::time::Duration::from_millis(250)).await; - let _ = server.init(am_model, data_dir).await; + let server = external::run_server(cmd, am_key).await?; + + use std::time::Duration; + use tokio::time::{sleep, Instant}; + let start = Instant::now(); + let timeout = Duration::from_secs(5); + // Wait until the server reports healthy or we time out. + while start.elapsed() < timeout { + if matches!(server.health().await, ServerHealth::Healthy) { + break; + } + sleep(Duration::from_millis(100)).await; + } + + if let Err(e) = server.init(am_model, data_dir).await { + tracing::warn!(error = ?e, "external_stt_init_failed"); + } let api_base = server.base_url.clone();
306-328: Avoid awaiting while holding theSharedStatemutex inget_servers
server.health().awaitis performed while the state mutex is held, which can unnecessarily serialize unrelated operations and risks deadlocks if future code also touches state during health checks.If
ServerHandleis cheap‑to‑clone (preferablyArc-backed), clone the handles and drop the lock before awaiting:- let state = self.state::<crate::SharedState>(); - let guard = state.lock().await; - - let internal_url = if let Some(server) = &guard.internal_server { - let status = server.health().await; - status - } else { - ServerHealth::Unreachable - }; - - let external_url = if let Some(server) = &guard.external_server { - server.health().await - } else { - ServerHealth::Unreachable - }; + let state = self.state::<crate::SharedState>(); + let (internal, external) = { + let guard = state.lock().await; + (guard.internal_server.clone(), guard.external_server.clone()) + }; + + let internal_url = if let Some(server) = internal { + server.health().await + } else { + ServerHealth::Unreachable + }; + + let external_url = if let Some(server) = external { + server.health().await + } else { + ServerHealth::Unreachable + };If
ServerHandleis not clonable, consider exposing a non‑async snapshot method or a smallhealth_url()accessor you can read under the lock and probe outside.
232-256: Debug path hardcodes macOS/aarch64 binaries; add target gating or fall back to sidecar in other debug environmentsToday this branch runs on all debug builds and returns
AmBinaryNotFoundon non‑mac debug setups. Gate it to macOS/aarch64 only (or fall back to the sidecar for other targets). Also, avoid unwrappinghome_dir().- let cmd: tauri_plugin_shell::process::Command = { - #[cfg(debug_assertions)] - { + let cmd: tauri_plugin_shell::process::Command = { + #[cfg(all(debug_assertions, target_os = "macos", target_arch = "aarch64"))] + { let passthrough_path = std::path::Path::new(env!("CARGO_MANIFEST_DIR")) .join("../../internal/passthrough-aarch64-apple-darwin"); let stt_path = std::path::Path::new(env!("CARGO_MANIFEST_DIR")) .join("../../internal/stt-aarch64-apple-darwin"); if !passthrough_path.exists() || !stt_path.exists() { return Err(crate::Error::AmBinaryNotFound); } self.shell() .command(passthrough_path) - .current_dir(dirs::home_dir().unwrap()) + .current_dir(dirs::home_dir().unwrap_or(std::env::current_dir().unwrap())) .arg(stt_path) .args(["serve", "-v", "-d"]) } - #[cfg(not(debug_assertions))] + #[cfg(not(all(debug_assertions, target_os = "macos", target_arch = "aarch64")))] self.shell() .sidecar("stt")? - .current_dir(dirs::home_dir().unwrap()) + .current_dir(dirs::home_dir().unwrap_or(std::env::current_dir().unwrap())) .args(["serve", "-v", "-d"]) };
428-434: Avoidunwrap()on checksum and file removal in the download pathA failed checksum calculation or remove can panic and crash the app during downloads. Prefer error logging and a negative progress signal instead of panicking.
- let checksum = hypr_file::calculate_file_checksum(&model_path).unwrap(); - - if checksum != m.checksum() { - tracing::error!("model_download_error: checksum mismatch"); - std::fs::remove_file(&model_path).unwrap(); - let _ = channel.send(-1); - } + match hypr_file::calculate_file_checksum(&model_path) { + Ok(checksum) if checksum == m.checksum() => {} + Ok(_) => { + tracing::error!("model_download_error: checksum mismatch"); + if let Err(e) = std::fs::remove_file(&model_path) { + tracing::warn!(error=?e, "failed_to_remove_corrupt_model_file"); + } + let _ = channel.send(-1); + } + Err(e) => { + tracing::error!(error=?e, "model_download_error: checksum_failed"); + let _ = channel.send(-1); + } + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
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)
plugins/local-stt/src/ext.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
plugins/local-stt/src/ext.rs
⏰ 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). (5)
- GitHub Check: build (macos, aarch64-apple-darwin, macos-latest)
- GitHub Check: build (windows, x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: build (macos, x86_64-apple-darwin, macos-latest)
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci (windows, windows-latest)
🔇 Additional comments (2)
plugins/local-stt/src/ext.rs (2)
275-302: Stopping servers only drops handles; ensure this actually terminates the processes (especially if-ddetaches)
stop_serverremoves the handles from state but doesn’t explicitly shut down or kill the processes. If-dcauses daemonization, dropping the handle may not stop the spawned server, leading to orphaned listeners and port conflicts.Would you confirm whether
internal::ServerHandle/external::ServerHandleimplement a terminatingDropor exposeshutdown()/kill()APIs? If available, prefer explicit shutdown:- Some(ServerType::External) => { - if let Some(_) = s.external_server.take() { + Some(ServerType::External) => { + if let Some(server) = s.external_server.take() { + if let Err(e) = server.shutdown().await { + tracing::warn!(error=?e, "external_stt_shutdown_failed"); + } stopped = true; } } - Some(ServerType::Internal) => { - if let Some(_) = s.internal_server.take() { + Some(ServerType::Internal) => { + if let Some(server) = s.internal_server.take() { + if let Err(e) = server.shutdown().await { + tracing::warn!(error=?e, "internal_stt_shutdown_failed"); + } stopped = true; } }If no explicit API exists, we should add one to the handle types to avoid relying on
Dropsemantics for lifecycle.
252-256: Manual Verification Required: Sidecar-dFlag SupportAfter searching the Rust sources, no definitions for the
-dflag (short or long form) were found in the sidecar CLI code. This suggests the flag handling may live outside this repo or in a vendored binary. Please perform the following checks on each supported platform:
- Run the sidecar’s help command (e.g.,
stt serve --help) and confirm:
- The
-dflag is recognized.- There is a corresponding long-form flag (e.g.,
--daemonor--detach), if applicable.- The semantics of
-dmatch whatexternal::run_serverexpects (i.e., it does not detach the process in a way that preventsstop_serverfrom terminating it).If you find that
-dcauses the server to daemonize, adjust the invocation (orstop_serverlogic) to ensure the child process can still be cleanly shut down.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
plugins/local-stt/src/server/external.rs (2)
59-62: Replace fixed 500 ms sleep with a bounded port-availability probe (avoid flakiness).Sleeping an arbitrary duration after killing the port is racy and can either be insufficient or waste time. Probe the port until it’s actually free (bounded by a short timeout) before spawning.
Apply this diff:
let port = 50060; let _ = port_killer::kill(port); - tokio::time::sleep(std::time::Duration::from_millis(500)).await; + if !wait_for_port_release(port, std::time::Duration::from_secs(3)).await { + tracing::warn!("port {} still busy after timeout; attempting spawn anyway", port); + }Add this helper above
run_server:async fn wait_for_port_release(port: u16, timeout: std::time::Duration) -> bool { let start = std::time::Instant::now(); loop { match std::net::TcpListener::bind(("127.0.0.1", port)) { Ok(listener) => { drop(listener); return true; } Err(e) if e.kind() == std::io::ErrorKind::AddrInUse => { // Still in use; backoff and retry } Err(_) => { // Transient bind error; retry } } if start.elapsed() >= timeout { return false; } tokio::time::sleep(std::time::Duration::from_millis(50)).await; } }
125-127: Don't sleep before returning; poll readiness with a short, bounded probe.A fixed 200 ms wait doesn’t guarantee the server is listening. Poll the status endpoint briefly and return as soon as it’s reachable.
Apply this diff:
- tokio::time::sleep(std::time::Duration::from_millis(200)).await; - tracing::info!("returning_handle"); + let start = std::time::Instant::now(); + let ready = loop { + match client.status().await { + Ok(_) => break true, // reachable is enough; model may still be loading + Err(_) => { + if start.elapsed() >= std::time::Duration::from_secs(5) { + break false; + } + tokio::time::sleep(std::time::Duration::from_millis(200)).await; + } + } + }; + tracing::info!("returning_handle ready={}", ready);plugins/local-stt/src/events.rs (1)
21-28: Don’t block the event loop; spawn the async start insteadCalling
tokio::task::block_in_place+Handle::current().block_oninside a TauriRunEventhandler can stall the UI/event loop and risks panicking if no Tokio runtime handle is present on the current thread. Prefer spawning on Tauri’s async runtime. This also resolves the earlier review note about blocking here.Apply this diff to replace the blocking section with a non-blocking spawn:
- tauri::WindowEvent::Focused(true) => { - tokio::task::block_in_place(|| { - tokio::runtime::Handle::current().block_on(async { - match app.start_server(None).await { - Ok(_) => tracing::info!("server_started"), - Err(e) => tracing::error!("server_start_failed: {:?}", e), - } - }); - }); - } + tauri::WindowEvent::Focused(true) => { + // why: avoid blocking the event loop on focus events + tauri::async_runtime::spawn({ + let app = app.clone(); + async move { + match app.start_server(None).await { + Ok(_) => tracing::info!("server_started"), + Err(e) => tracing::error!("server_start_failed: {:?}", e), + } + } + }); + }
🧹 Nitpick comments (3)
plugins/local-stt/src/server/external.rs (1)
10-14: Include base_url in shutdown log for better diagnostics (aligns with PR intent).This makes correlating shutdowns to specific server instances trivial.
Apply this diff:
impl Drop for ServerHandle { fn drop(&mut self) { - tracing::info!("stopping"); + tracing::info!(base_url = %self.base_url, "stopping"); let _ = self.shutdown.send(()); } }plugins/local-stt/src/events.rs (2)
23-26: Prefer structured fields in logs for easier correlationMinor: enrich logs with stable fields (component/window) to improve filtering without relying on message strings.
Example:
- Ok(_) => tracing::info!("server_started"), - Err(e) => tracing::error!("server_start_failed: {:?}", e), + Ok(_) => tracing::info!(component = "local-stt", window = "main", "server_started"), + Err(e) => tracing::error!(component = "local-stt", window = "main", error = ?e, "server_start_failed"),
19-31: start_server is already guarded—no duplicate spawns
Inspection ofstart_serverinplugins/local-stt/src/ext.rsshows it acquires the sharedStatemutex and returnsError::ServerAlreadyRunningif the server is already up, preventing concurrent starts. You don’t need an additional guard to prevent races or duplicate processes.Optional refinement:
- In
plugins/local-stt/src/events.rs, catch and ignore theServerAlreadyRunningerror so that repeatedFocused(true)events don’t log errors. For example:match app.start_server(None).await { Ok(_) => tracing::info!("server_started"),- Err(e) => tracing::error!("server_start_failed: {:?}", e),
- Err(crate::Error::ServerAlreadyRunning) => {
// server is already running – no action needed- }
- Err(e) => tracing::error!("server_start_failed: {:?}", e),
}</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Path: .coderabbit.yaml **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. <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 3237d6c880548dd97fda0ba45353a3b32e292a04 and 242e1a5c8a9bad11a978d6a398805aa1eeba924b. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `plugins/local-stt/src/events.rs` (1 hunks) * `plugins/local-stt/src/server/external.rs` (2 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (1)</summary> <details> <summary>**/*.{js,ts,tsx,rs}</summary> **⚙️ CodeRabbit configuration file** > `**/*.{js,ts,tsx,rs}`: 1. No error handling. > 2. No unused imports, variables, or functions. > 3. For comments, keep it minimal. It should be about "Why", not "What". > Files: - `plugins/local-stt/src/server/external.rs` - `plugins/local-stt/src/events.rs` </details> </details><details> <summary>🧬 Code graph analysis (1)</summary> <details> <summary>plugins/local-stt/src/events.rs (2)</summary><blockquote> <details> <summary>plugins/windows/js/bindings.gen.ts (1)</summary> * `HyprWindow` (73-73) </details> <details> <summary>plugins/local-llm/src/events.rs (1)</summary> * `on_event` (4-32) </details> </blockquote></details> </details> </details> <details> <summary>⏰ 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). (5)</summary> * GitHub Check: build (macos, aarch64-apple-darwin, macos-latest) * GitHub Check: build (macos, x86_64-apple-darwin, macos-latest) * GitHub Check: build (windows, x86_64-pc-windows-msvc, windows-latest) * GitHub Check: ci (macos, macos-latest) * GitHub Check: ci (windows, windows-latest) </details> <details> <summary>🔇 Additional comments (3)</summary><blockquote> <details> <summary>plugins/local-stt/src/events.rs (3)</summary><blockquote> `1-1`: **Import cleanup LGTM** Keeping only `LocalSttPluginExt` and dropping unused imports aligns with the "no unused imports" guideline. --- `7-13`: **Graceful window-label parse handling is fine** Early-return on parse failure keeps the handler cheap and avoids accidental startup for unknown windows. The concise warn log is adequate. --- `15-17`: **Scope startup to Main window only** Good guard. Prevents unexpected STT spins from auxiliary windows and reduces event noise. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| tracing::info!("spwaning_started"); | ||
| let (mut rx, child) = cmd.args(["--port", &port.to_string()]).spawn()?; | ||
| tracing::info!("spwaning_started"); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix typo and de-dup logs; add clearer pre/post-spawn messages.
Typo: “spwaning”. Also, the two identical logs don’t distinguish before/after spawn.
Apply this diff:
- tracing::info!("spwaning_started");
+ tracing::info!("spawning_external_stt port={}", port);
let (mut rx, child) = cmd.args(["--port", &port.to_string()]).spawn()?;
- tracing::info!("spwaning_started");
+ tracing::info!("spawned_external_stt port={}", port);🏁 Script executed:
#!/bin/bash
# Verify misspelling is gone and new messages exist
rg -n -C1 -F 'spwaning' --type=rust
rg -n -F 'spawning_external_stt' plugins/local-stt/src/server/external.rs
rg -n -F 'spawned_external_stt' plugins/local-stt/src/server/external.rsLength of output: 560
Fix typo and clarify pre/post-spawn log messages
The typo “spwaning” is still present and the two identical log calls at lines 63 and 65 don’t distinguish “before” vs “after” spawning. Please update plugins/local-stt/src/server/external.rs around lines 63–66 to:
- Correct the spelling from “spwaning” to “spawning”
- Remove the duplicate message
- Log both before‐spawn and after‐spawn events with the port number
Paths to update:
- plugins/local-stt/src/server/external.rs:63–66
Suggested diff:
- tracing::info!("spwaning_started");
+ tracing::info!("spawning_external_stt port={}", port);
let (mut rx, child) = cmd.args(["--port", &port.to_string()]).spawn()?;
- tracing::info!("spwaning_started");
+ tracing::info!("spawned_external_stt port={}", port);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tracing::info!("spwaning_started"); | |
| let (mut rx, child) = cmd.args(["--port", &port.to_string()]).spawn()?; | |
| tracing::info!("spwaning_started"); | |
| tracing::info!("spawning_external_stt port={}", port); | |
| let (mut rx, child) = cmd.args(["--port", &port.to_string()]).spawn()?; | |
| tracing::info!("spawned_external_stt port={}", port); |
🤖 Prompt for AI Agents
In plugins/local-stt/src/server/external.rs around lines 63 to 66, fix the typo
"spwaning" -> "spawning", remove the duplicate log call, and replace with two
distinct tracing::info! messages: one logged immediately before spawning that
includes the port number (e.g., "spawning child process on port {port}"), and
one logged after successful spawn that includes the port number (and optionally
the child PID) to indicate post-spawn success; keep existing spawn call and
error propagation unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
plugins/local-stt/src/server/external.rs (2)
63-65: Fix typo and improve spawn diagnostics (pre/post, port + pid)The “spwaning” typo persists and the messages don’t include useful context. Log before and after with the port and child PID.
- tracing::info!("spwaning_started"); + tracing::info!("spawning_external_stt port={}", port); let (mut rx, child) = cmd.args(["--port", &port.to_string()]).spawn()?; - tracing::info!("spwaning_ended"); + tracing::info!("spawned_external_stt port={} pid={:?}", port, child.pid());
61-61: Replace fixed 500 ms sleep after port kill with a bounded wait for port releaseThis exact concern was raised previously and still applies: a hard-coded delay is brittle and can be either too short (flaky) or unnecessarily long. Prefer polling the port until it’s not accepting connections, with an upper bound.
Apply within this hunk:
- tokio::time::sleep(std::time::Duration::from_millis(500)).await; + if !wait_port_closed(port, std::time::Duration::from_secs(1)).await { + tracing::warn!("port {} may still be in use; proceeding to spawn anyway", port); + }And add this helper in the same module (outside this hunk):
// Wait until nothing is listening on `port`, up to `timeout`. // Returns true if port is closed before timeout, false on timeout. async fn wait_port_closed(port: u16, timeout: std::time::Duration) -> bool { use tokio::net::TcpStream; use tokio::time::sleep; let start = std::time::Instant::now(); loop { match TcpStream::connect(("127.0.0.1", port)).await { Ok(_) => { if start.elapsed() >= timeout { return false; } sleep(std::time::Duration::from_millis(50)).await; } Err(_) => return true, // connect failing implies no listener } } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
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)
plugins/local-stt/src/server/external.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
plugins/local-stt/src/server/external.rs
⏰ 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). (5)
- GitHub Check: build (macos, aarch64-apple-darwin, macos-latest)
- GitHub Check: build (macos, x86_64-apple-darwin, macos-latest)
- GitHub Check: build (windows, x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci (windows, windows-latest)
| // impl Drop for ServerHandle { | ||
| // fn drop(&mut self) { | ||
| // tracing::info!("stopping"); | ||
| // let _ = self.shutdown.send(()); | ||
| // } | ||
| // } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Re-enable Drop (or add an explicit shutdown API) to avoid orphaned external STT processes
Commenting out Drop removes the only in-module path that can signal the background task to kill the child process. The shutdown sender is private (not pub) and there’s no public method that sends on it, so nothing external can currently trigger shutdown. This risks leaking the external STT child when the handle is dropped.
Recommend restoring Drop and including base_url in the log (per PR objectives). Minimal diff:
-// impl Drop for ServerHandle {
-// fn drop(&mut self) {
-// tracing::info!("stopping");
-// let _ = self.shutdown.send(());
-// }
-// }
+impl Drop for ServerHandle {
+ fn drop(&mut self) {
+ tracing::info!("stopping: {}", self.base_url);
+ if let Err(e) = self.shutdown.send(()) {
+ tracing::debug!("shutdown_channel_already_closed {:?}", e);
+ }
+ }
+}Optionally, instead of relying on Drop, expose a public fn shutdown(&self) -> bool that sends on the watch channel and returns whether a send occurred, and ensure all call sites invoke it on app lifecycle events.
Run this to confirm no alternative shutdown path exists right now:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# ServerHandle definitions and Drop impls
rg -nP '^\s*pub\s+struct\s+ServerHandle\b' -C3
rg -nP 'impl\s+Drop\s+for\s+ServerHandle\b' -C2
# Any explicit sends on the shutdown sender?
rg -n 'shutdown\.send\(' -C2Length of output: 3376
Re-enable Drop for External STT ServerHandle to Prevent Orphans
The plugins/local-stt/src/server/external.rs ServerHandle currently has no Drop implementation or public shutdown API, so its background task never receives the shutdown signal. This will leak the external STT child process whenever the handle is dropped.
Please restore the Drop impl (including base_url in the log) or expose a public shutdown method. For example:
--- a/plugins/local-stt/src/server/external.rs
+++ b/plugins/local-stt/src/server/external.rs
@@
// impl Drop for ServerHandle {
// fn drop(&mut self) {
// tracing::info!("stopping");
// let _ = self.shutdown.send(());
// }
// }
+
+impl Drop for ServerHandle {
+ fn drop(&mut self) {
+ tracing::info!("stopping external STT at {}", self.base_url);
+ if let Err(e) = self.shutdown.send(()) {
+ tracing::debug!("external STT shutdown channel already closed: {:?}", e);
+ }
+ }
+}Alternatively, mirror the LLM plugin’s pattern by adding:
impl ServerHandle {
/// Gracefully signal the external STT to stop; returns Err if already shut down.
pub fn shutdown(self) -> Result<(), tokio::sync::watch::error::SendError<()>> {
self.shutdown.send(())
}
}– plugins/local-stt/src/server/external.rs:3–15
🤖 Prompt for AI Agents
In plugins/local-stt/src/server/external.rs around lines 3 to 15, the
ServerHandle lacks a Drop implementation or public shutdown API so the
background task never receives the shutdown signal and the external STT child is
leaked; restore the Drop impl (or add a public shutdown method) so the handle
signals shutdown: reintroduce impl Drop for ServerHandle that logs including the
base_url (e.g., "stopping external stt at {base_url}") and then sends the
shutdown signal (ignoring the send error), or alternatively add a pub fn
shutdown(self) -> Result<(), tokio::sync::watch::error::SendError<()>> that
forwards to self.shutdown.send(()) and document it as a graceful stop; ensure
the shutdown sender is used exactly once and ownership/borrow rules compile
cleanly.
| tokio::time::sleep(std::time::Duration::from_millis(200)).await; | ||
| tracing::info!("returning_handle"); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace fixed 200 ms post-spawn delay with a readiness wait on the HTTP API
A static sleep is guesswork. Poll the external STT server until it responds on the base_url (or until timeout). This reduces flakiness and speeds up the common fast path.
- tokio::time::sleep(std::time::Duration::from_millis(200)).await;
- tracing::info!("returning_handle");
+ if !wait_server_ready(&client, std::time::Duration::from_secs(5)).await {
+ tracing::warn!("external_stt not ready within timeout: {}", base_url);
+ } else {
+ tracing::info!("external_stt_ready {}", base_url);
+ }Add this helper in the same module (outside this hunk):
// Returns true once the server responds to status() (Loaded or Loading), false on timeout.
async fn wait_server_ready(client: &hypr_am::Client, timeout: std::time::Duration) -> bool {
use tokio::time::{sleep, Instant};
let start = Instant::now();
loop {
match client.status().await {
Ok(res) => {
// Consider both Loading and Loaded as "responding"; callers can poll health() later.
if matches!(res.model_state, hypr_am::ModelState::Loaded | hypr_am::ModelState::Loading) {
return true;
}
}
Err(_) => {}
}
if start.elapsed() >= timeout {
return false;
}
sleep(std::time::Duration::from_millis(100)).await;
}
}🤖 Prompt for AI Agents
In plugins/local-stt/src/server/external.rs around lines 125 to 127, replace the
fixed tokio::time::sleep(Duration::from_millis(200)).await; and the subsequent
tracing::info!("returning_handle"); with a readiness wait: add the suggested
async helper wait_server_ready(client, timeout) in this module (outside the
current hunk), then call it after spawning the server and before returning the
handle; if wait_server_ready returns true proceed to return the handle and log
readiness, if false treat as timeout (log error and return Err or propagate
timeout) so callers don’t rely on a magic 200ms sleep.
Summary by cubic
Fixes unreliable external STT server startup by launching servers only when the main window is focused and waiting after freeing the port. Also adds the same focus-based start for local LLM, improves logging, and enables Pro STT model selection.
Bug Fixes
Refactors