-
Notifications
You must be signed in to change notification settings - Fork 415
Add supervisor for local-stt servers #1655
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughThe changes introduce a supervisor-based architecture for managing STT server lifecycles using the ractor library. A new supervisor module orchestrates internal and external STT actors, replacing direct server lifecycle management. Shared state now holds a supervisor reference, error handling is updated with supervisor-specific variants, and server initialization flows through supervisor calls instead of direct actor invocations. Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin as Local STT Plugin
participant Supervisor as STT Supervisor
participant Internal as Internal STT Actor
participant External as External STT Actor
Plugin->>Supervisor: spawn_stt_supervisor()
Supervisor-->>Plugin: ActorRef<DynamicSupervisorMsg>
Plugin->>Supervisor: start_internal_stt(args)
Supervisor->>Internal: spawn child with InternalSTTArgs
Internal-->>Supervisor: spawned
Plugin->>Supervisor: start_external_stt(args)
Supervisor->>External: spawn child with ExternalSTTArgs
External-->>Supervisor: spawned
Note over Supervisor: Both actors under supervision<br/>with transient restart policy
Plugin->>Supervisor: stop_stt_server(server_type)
Supervisor->>Supervisor: terminate_child(actor_name)
Supervisor-->>Plugin: Result
Plugin->>Supervisor: stop_all_stt_servers()
Supervisor->>Internal: terminate
Supervisor->>External: terminate
Supervisor-->>Plugin: Result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/local-stt/src/server/internal.rs (1)
1-135: Fix compile-time error when propagating server failures
Stringdoes not implementstd::error::Error, soErr(e.into())fails to compile againstActorProcessingErr. We need to wrap the message in an actual error type before bubbling it up; otherwise the actor never builds.Apply this diff to store a real error and keep the actor stoppage semantics intact:
@@ -use std::{ - net::{Ipv4Addr, SocketAddr}, - path::PathBuf, -}; +use std::{ + io, + net::{Ipv4Addr, SocketAddr}, + path::PathBuf, +}; @@ -pub enum InternalSTTMessage { - GetHealth(RpcReplyPort<ServerInfo>), - ServerError(String), -} +pub enum InternalSTTMessage { + GetHealth(RpcReplyPort<ServerInfo>), + ServerError(ActorProcessingErr), +} @@ - let whisper_service = HandleError::new( + let whisper_service = HandleError::new( hypr_transcribe_whisper_local::TranscribeService::builder() .model_path(model_path) .build(), - move |err: String| async move { - let _ = myself.send_message(InternalSTTMessage::ServerError(err.clone())); + move |err: String| async move { + let actor_err: ActorProcessingErr = + io::Error::new(io::ErrorKind::Other, err.clone()).into(); + let _ = myself.send_message(InternalSTTMessage::ServerError(actor_err)); (StatusCode::INTERNAL_SERVER_ERROR, err) }, ); @@ - InternalSTTMessage::ServerError(e) => Err(e.into()), + InternalSTTMessage::ServerError(e) => Err(e),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
plugins/local-stt/Cargo.toml(1 hunks)plugins/local-stt/src/error.rs(1 hunks)plugins/local-stt/src/ext.rs(6 hunks)plugins/local-stt/src/lib.rs(3 hunks)plugins/local-stt/src/server/external.rs(5 hunks)plugins/local-stt/src/server/internal.rs(2 hunks)plugins/local-stt/src/server/mod.rs(1 hunks)plugins/local-stt/src/server/supervisor.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
plugins/local-stt/src/server/internal.rs (1)
plugins/local-stt/src/server/supervisor.rs (1)
e(101-101)
plugins/local-stt/src/lib.rs (2)
plugins/local-stt/src/ext.rs (5)
state(60-60)state(196-196)state(255-255)state(296-296)state(309-309)plugins/local-stt/src/server/supervisor.rs (2)
spawn_stt_supervisor(19-31)e(101-101)
plugins/local-stt/src/ext.rs (2)
plugins/local-stt/src/server/supervisor.rs (5)
stop_all_stt_servers(117-123)e(101-101)stop_stt_server(89-115)start_internal_stt(33-39)start_external_stt(41-47)plugins/local-stt/src/server/external.rs (2)
new(21-25)new(42-56)
plugins/local-stt/src/server/supervisor.rs (2)
plugins/local-stt/src/server/external.rs (1)
name(72-74)plugins/local-stt/src/server/internal.rs (1)
name(35-37)
plugins/local-stt/src/server/external.rs (3)
crates/am/src/client.rs (2)
new(11-16)new(99-107)crates/host/src/lib.rs (1)
kill_processes_by_matcher(25-47)plugins/local-stt/src/server/internal.rs (1)
handle(112-134)
⏰ 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). (1)
- GitHub Check: ci (macos, macos-14)
No description provided.