Refactorings / cleanups in the daemon #462
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Split message handling and physical transport over ZMQ/bincode * Makes it more easily mockable/testable
* Split workers stuff up by function, just like RPC * Move both into their own directory for sanitation
rdaum
force-pushed
the
rd/rpc-server-again
branch
from
bbc0903 to
e14e79a
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the daemon by modularizing worker and RPC logic, introducing a task monitor and a new system control handle, and cleaning up legacy code and comments.
- Added structured
workersandrpcmodules, separating transport, server, and session concerns - Introduced
TaskMonitorfor task lifecycle management and replaced the oldsys_ctrlwithsystem_control.rs - Updated
main.rsto wire up new interfaces and removed outdated comments and legacy files
Reviewed Changes
Copilot reviewed 14 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/daemon/src/workers/transport.rs | New ZMQ transport layer for worker communication |
| crates/daemon/src/workers/server.rs | Worker server coordinator and threading setup |
| crates/daemon/src/workers/mod.rs | Module declarations for workers |
| crates/daemon/src/task_monitor.rs | TaskMonitor implementation for task lifecycle |
| crates/daemon/src/system_control.rs | New SystemControlHandle implementation |
| crates/daemon/src/sys_ctrl.rs | Removed legacy sys_ctrl implementation |
| crates/daemon/src/rpc/transport.rs | New ZMQ transport for RPC with proxying |
| crates/daemon/src/rpc/session.rs | Updated RpcSession and SessionActions variants |
| crates/daemon/src/rpc/server.rs | New RPC server coordinator and threading logic |
| crates/daemon/src/rpc/mod.rs | Module declarations for RPC |
| crates/daemon/src/main.rs | Updated main wiring to use new modules |
| crates/daemon/src/lib.rs | Adjusted crate attributes (recursion limit) |
| crates/daemon/src/connections/in_memory.rs | Removed outdated compatibility comments |
| .licensure.yml | Added cowbell to licensing exclusions |
| public_key, | ||
| private_key, | ||
| zmq_ctx.clone(), | ||
| &args.workers_request_listen, |
There was a problem hiding this comment.
Passing workers_request_listen to WorkersServer::new for the workers_broadcast parameter seems incorrect; consider using the broadcast endpoint (e.g., args.workers_broadcast) instead.
Suggested change
| &args.workers_request_listen, | |
| &args.workers_broadcast, |
| message_handler: Arc<H>, | ||
| ) -> eyre::Result<()> { | ||
| let num_io_threads = self.zmq_context.get_io_threads()?; | ||
| info!("0mq server listening on {rpc_endpoint} with {num_io_threads} IO threads"); |
There was a problem hiding this comment.
Log message uses "0mq" (zero) instead of "ZMQ"; consider updating to "ZMQ" for clarity.
Suggested change
| info!("0mq server listening on {rpc_endpoint} with {num_io_threads} IO threads"); | |
| info!("ZMQ server listening on {rpc_endpoint} with {num_io_threads} IO threads"); |
| match selector.wait_timeout(timeout) { | ||
| Ok((index, result)) => { | ||
| let client_id = task_client_ids[index].1; | ||
| let task_id = task_client_ids[index].0; |
There was a problem hiding this comment.
[nitpick] This task_id shadows a later variable of the same name inside the nested match; consider renaming one (e.g., indexed_task_id) to avoid confusion.
Suggested change
| let task_id = task_client_ids[index].0; | |
| let indexed_task_id = task_client_ids[index].0; |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.