feat: add daemon foundation#239
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Unix-socket JSON-protocol daemon and RunningDaemon runtime, CLI daemon subcommands (enable/disable/restart/run), state job-tracking APIs and socket helpers, workspace serde deps, and integration tests exercising protocol flows, version mismatch, malformed input, and recent_jobs behavior. ChangesDaemon Integration
Sequence DiagramsequenceDiagram
participant CLI as CLI
participant Daemon as RunningDaemon
participant Socket as UnixSocket
participant DB as Database
CLI->>Daemon: run_blocking(paths)
Daemon->>Daemon: start() (ensure fs layout, open DB, bind socket)
Daemon->>Socket: listen
rect rgba(200, 150, 200, 0.5)
Socket->>Daemon: accept connection
CLI->>Socket: send JSON request (protocol_version + command)
Socket->>Daemon: read request line
Daemon->>Daemon: validate protocol_version
alt health
Daemon->>Socket: write health Ok response
else run_job
Daemon->>DB: start_job(kind, scope)
DB-->>Daemon: JobRecord (job_id)
Daemon->>Socket: write Accepted response (job_id)
Daemon->>Socket: write JobStarted event
Daemon->>Socket: write Progress event
Daemon->>Socket: write JobCompleted event
Daemon->>DB: complete_job(job_id, summary)
end
end
CLI->>Daemon: Ctrl-C -> shutdown()
Daemon->>Daemon: send shutdown signal, await server task, remove socket
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/state/src/database.rs (1)
115-138: 💤 Low valueConsider adding a LIMIT to
recent_jobsfor future-proofing.The method is named
recent_jobsbut returns all jobs without a limit. As the jobs table grows over time, this could return unbounded results.For a foundation PR this is acceptable, but consider adding a
LIMITclause (e.g.,LIMIT 100) or renaming toall_jobsto clarify intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/state/src/database.rs` around lines 115 - 138, The recent_jobs method returns all rows and should be bounded; modify the SQL in recent_jobs to include a LIMIT (e.g., change the query to SELECT ... FROM jobs ORDER BY started_at, id LIMIT 100) so it returns a fixed number of recent JobRecord entries, or alternatively change recent_jobs signature to accept a configurable limit parameter and apply that in the prepared statement; update the SQL string and any call sites accordingly to use the new limit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cli/src/lib.rs`:
- Around line 70-85: finish_execution currently treats ExecuteError::Io as a
catch-all and forwards it into an anyhow error path instead of formatting it via
Output::error like ExecuteError::User; update finish_execution to add an
Err(ExecuteError::Io(err)) branch (similar to Err(ExecuteError::User(error)))
that creates Output::new(stderr, output_mode) and calls
output.error(&err.to_string())? and then returns Ok(ExitCode::FAILURE) so IO
errors are formatted via Output::error and exit code remains FAILURE; ensure you
reference ExecuteError::Io, finish_execution, and Output::error when locating
the change.
In `@crates/daemon/src/lib.rs`:
- Around line 129-143: In serve(), don't propagate connection errors from
handle_connection with the `?` — wrap the accept/handle path in a match or if
let and on Err(e) log the error (include context like which client or that
accept/handle failed) and continue the loop so the daemon keeps serving; keep
the shutdown branch unchanged and only return Ok(()) when shutdown fires.
Specifically update the block that calls listener.accept() and
handle_connection(paths.clone(), stream).await so errors are caught (e.g., match
accepted { Ok((stream, addr)) => if let Err(e) = handle_connection(...).await {
error!(%e, "connection handling failed for {addr}") }, Err(e) => error!(%e,
"accept failed") }) and then continue.
---
Nitpick comments:
In `@crates/state/src/database.rs`:
- Around line 115-138: The recent_jobs method returns all rows and should be
bounded; modify the SQL in recent_jobs to include a LIMIT (e.g., change the
query to SELECT ... FROM jobs ORDER BY started_at, id LIMIT 100) so it returns a
fixed number of recent JobRecord entries, or alternatively change recent_jobs
signature to accept a configurable limit parameter and apply that in the
prepared statement; update the SQL string and any call sites accordingly to use
the new limit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 212313a3-17ca-4c4c-9f55-64bbfb94ecd9
⛔ Files ignored due to path filters (8)
Cargo.lockis excluded by!**/*.lockcrates/daemon/tests/snapshots/daemon_foundation__protocol_mismatch_returns_restart_guidance_without_creating_a_job.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/daemon_foundation__socket_protocol_streams_job_progress_and_persists_final_status.snapis excluded by!**/*.snapit/snapshots/cli__completions_generate_bash_script.snapis excluded by!**/*.snapit/snapshots/cli__completions_generate_fish_script.snapis excluded by!**/*.snapit/snapshots/cli__completions_generate_zsh_script.snapis excluded by!**/*.snapit/snapshots/cli__daemon_lifecycle_commands_are_routed_as_stubs.snapis excluded by!**/*.snapit/snapshots/cli__daemon_run_is_hidden_from_top_level_help.snapis excluded by!**/*.snap
📒 Files selected for processing (16)
Cargo.tomlDESIGN.mdcrates/cli/Cargo.tomlcrates/cli/src/args.rscrates/cli/src/commands/daemon.rscrates/cli/src/commands/mod.rscrates/cli/src/error.rscrates/cli/src/lib.rscrates/daemon/Cargo.tomlcrates/daemon/src/lib.rscrates/daemon/tests/daemon_foundation.rscrates/state/src/database.rscrates/state/src/fs.rscrates/state/src/lib.rscrates/state/src/paths.rsit/cli.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/daemon/src/lib.rs`:
- Around line 137-142: The accept() result is being unwrapped with the ?
operator which will terminate the daemon on an accept() error; change the match
arm handling of the select branch so that the AcceptResult is matched and accept
errors are logged/ignored and the loop continues instead of returning—i.e.
replace the let (stream, _address) = accepted?; pattern in the listener.accept()
branch with a match (or if let) that handles Ok((stream, addr)) by calling
handle_connection(paths.clone(), stream).await (as currently done) and handles
Err(e) by logging or ignoring e and continuing the loop so the daemon stays
alive; reference the listener.accept() branch and the handle_connection(...)
call when making this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d94bc96-2c1a-4832-9e32-32239992ceeb
⛔ Files ignored due to path filters (2)
crates/daemon/tests/snapshots/daemon_foundation__malformed_request_does_not_stop_accepting_connections.snapis excluded by!**/*.snapcrates/state/tests/snapshots/state_foundation__recent_jobs_returns_the_latest_one_hundred_jobs.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
crates/cli/src/lib.rscrates/daemon/src/lib.rscrates/daemon/tests/daemon_foundation.rscrates/state/src/database.rscrates/state/tests/state_foundation.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/daemon/src/jobs.rs`:
- Around line 13-55: A started job may be left without a final persisted state
if any write_line call returns early; change the flow so Database::start_job
(and the created job.id) remains, attempt the write_line calls but ensure
database.complete_job(&job.id, summary) is always invoked before returning on
any write error; implement this by performing the write_line calls and capturing
their Result, and on Err call database.complete_job(&job.id, summary)
(propagating the original error afterward), or wrap the write sequence in a
scope that returns its Result but uses a match or finally-like cleanup to call
complete_job on Err, referencing Database::open, Database::start_job,
write_line, &job.id, database.complete_job, and the DaemonEvent writes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 945cbe38-2b77-4618-8c8e-63a7606618a1
📒 Files selected for processing (5)
crates/daemon/src/error.rscrates/daemon/src/jobs.rscrates/daemon/src/lib.rscrates/daemon/src/protocol.rscrates/daemon/src/server.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/daemon/tests/daemon_foundation.rs (1)
107-114: ⚡ Quick winAssert the health response payload instead of discarding it.
The follow-up health call is a good synchronization point, but ignoring its content makes this test less sensitive to protocol regressions.
Suggested patch
- let _health = request_lines( + let health_lines = request_lines( &paths, json!({ "protocol_version": daemon::PROTOCOL_VERSION, "command": "health", }), ) .await?; + assert_eq!(health_lines.len(), 1); + assert_eq!(health_lines[0]["type"], json!("response")); + assert_eq!(health_lines[0]["status"], json!("ok")); + assert_eq!(health_lines[0]["message"], json!("daemon healthy"));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/daemon/tests/daemon_foundation.rs` around lines 107 - 114, Replace the discarded `_health` result from the request_lines call with a real assertion: store the result (e.g., let health = request_lines(...).await?;), parse it as JSON (serde_json::Value) and assert the expected fields are present and correct — at minimum assert that health["protocol_version"] == daemon::PROTOCOL_VERSION and that a health/status field exists (for example health["status"] == "ok" if applicable); update the test around the request_lines call to fail if the health payload does not match these expectations so the test detects protocol regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/daemon/tests/daemon_foundation.rs`:
- Around line 107-114: Replace the discarded `_health` result from the
request_lines call with a real assertion: store the result (e.g., let health =
request_lines(...).await?;), parse it as JSON (serde_json::Value) and assert the
expected fields are present and correct — at minimum assert that
health["protocol_version"] == daemon::PROTOCOL_VERSION and that a health/status
field exists (for example health["status"] == "ok" if applicable); update the
test around the request_lines call to fail if the health payload does not match
these expectations so the test detects protocol regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f780682-4989-45f7-984c-10a87940f930
⛔ Files ignored due to path filters (1)
crates/daemon/tests/snapshots/daemon_foundation__disconnected_job_stream_still_persists_final_status.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
crates/daemon/src/jobs.rscrates/daemon/tests/daemon_foundation.rs
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/daemon/src/lib.rs (1)
43-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuarantee socket cleanup even when the server task exits with join errors.
On Line 43 and Line 97,
task.await?short-circuits before cleanup; on Line 107,task_result?can also return before applyingsocket_result. This can leave a stale daemon socket and break subsequent starts.Suggested patch
pub async fn shutdown(self) -> Result<(), DaemonError> { let _ = self.shutdown.send(()); - let task_result = self.task.await?; - let socket_result = state::fs::remove_daemon_socket(&self.paths); - - task_result?; - socket_result?; + let join_result = self.task.await; + let socket_result = state::fs::remove_daemon_socket(&self.paths); + socket_result?; + let task_result = join_result?; + task_result?; Ok(()) } @@ signal_result = &mut shutdown_signal => { signal_result?; let _ = shutdown.send(()); - let task_result = task.await?; let socket_result = state::fs::remove_daemon_socket(&paths); - - task_result?; socket_result?; + let task_result = task.await?; + task_result?; Ok(()) } task_result = &mut task => { let socket_result = state::fs::remove_daemon_socket(&paths); - let task_result = task_result?; - socket_result?; + let task_result = task_result?; task_result }Also applies to: 97-101, 106-110
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/daemon/src/lib.rs` around lines 43 - 48, The current code uses ? on self.task.await and on task_result which short-circuits and skips socket cleanup; change the flow in the functions using self.task.await and state::fs::remove_daemon_socket so you first capture the task join result without bubbling it (let task_result = self.task.await), then always call state::fs::remove_daemon_socket(&self.paths) to get socket_result, and only after the removal propagate errors (e.g. return task_result?; socket_result? or prefer a combined/error-prioritizing strategy); update the code paths that reference task_result and socket_result (the await and the remove_daemon_socket calls) so socket cleanup runs even when the task join returns Err.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/daemon/src/server.rs`:
- Around line 28-37: The match arm that currently does `Err(_error) => continue`
can busy-loop on persistent accept() failures; modify the error branch around
the `accepted` handling so it logs or captures the error and then awaits a small
backoff (e.g., tokio::time::sleep for a short Duration like 100–500ms) before
retrying instead of a bare continue. Locate the match in server.rs where
`accepted` is matched and adjust the `Err(...)` arm that is adjacent to
`connections.spawn(... handle_connection ...)` to perform the sleep/backoff (and
optionally a debug/error log) before looping.
- Around line 55-59: Add a read timeout around the initial read from the
transport (the call to transport.next().await that yields line) so a client
can't hold the connection open indefinitely; use tokio::time::timeout (or
futures::future::timeout) to await transport.next() with a configurable
duration, handle the timeout by returning an appropriate error or closing the
connection, and only proceed to serde_json::from_str::<DaemonRequest>(&line?)?
when the timeout succeeds; update any surrounding error handling to distinguish
a timed-out read from other I/O errors and document the timeout value where the
transport.next().await call is performed.
In `@crates/daemon/tests/daemon_foundation.rs`:
- Around line 128-130: The test uses a very tight timeout when awaiting
request_lines: change the Duration::from_millis(250) used in the timeout(...)
call (where `lines` is assigned) to a more permissive value (e.g.,
Duration::from_secs(2) or Duration::from_millis(1500)) to reduce CI flakiness;
update the literal in the timeout invocation that wraps the `request_lines(...)`
call so the test allows more time for async completion.
In `@crates/state/src/database.rs`:
- Around line 131-136: complete_job and fail_job treat UPDATE as successful even
if no row was updated; after calling transaction.execute(...) (which returns a
usize), capture the returned count (e.g., let updated =
transaction.execute(...)?;) and if updated == 0 return an appropriate error
(e.g., a NotFound/JobNotFound error from your crate) instead of proceeding to
prune_old_jobs or returning success. Apply this check in both complete_job (the
UPDATE that sets status = Succeeded) and fail_job (the UPDATE that sets status =
Failed) before calling prune_old_jobs(transaction) or committing.
---
Outside diff comments:
In `@crates/daemon/src/lib.rs`:
- Around line 43-48: The current code uses ? on self.task.await and on
task_result which short-circuits and skips socket cleanup; change the flow in
the functions using self.task.await and state::fs::remove_daemon_socket so you
first capture the task join result without bubbling it (let task_result =
self.task.await), then always call state::fs::remove_daemon_socket(&self.paths)
to get socket_result, and only after the removal propagate errors (e.g. return
task_result?; socket_result? or prefer a combined/error-prioritizing strategy);
update the code paths that reference task_result and socket_result (the await
and the remove_daemon_socket calls) so socket cleanup runs even when the task
join returns Err.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 972ab312-39ba-4468-b49e-10688d557406
⛔ Files ignored due to path filters (7)
Cargo.lockis excluded by!**/*.lockcrates/daemon/tests/snapshots/daemon_foundation__disconnected_job_stream_still_persists_final_status.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/daemon_foundation__idle_client_without_newline_does_not_block_health_requests.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/daemon_foundation__socket_protocol_streams_job_progress_and_persists_final_status.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/daemon_foundation__start_removes_stale_socket_before_binding.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/daemon_foundation__unsupported_job_streams_failure_event_and_persists_failed_status.snapis excluded by!**/*.snapcrates/state/tests/snapshots/state_foundation__recent_jobs_returns_the_latest_one_hundred_jobs.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
Cargo.tomlcrates/daemon/Cargo.tomlcrates/daemon/src/error.rscrates/daemon/src/jobs.rscrates/daemon/src/lib.rscrates/daemon/src/protocol.rscrates/daemon/src/server.rscrates/daemon/tests/daemon_foundation.rscrates/state/src/database.rscrates/state/src/error.rscrates/state/src/lib.rscrates/state/tests/state_foundation.rs
✅ Files skipped from review due to trivial changes (1)
- crates/daemon/Cargo.toml
This was referenced May 24, 2026
This was referenced Jun 3, 2026
This was referenced Jun 11, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Test Plan
Summary by CodeRabbit
New Features
Documentation
Tests