Run daemon at startup#1782
Merged
grahamc merged 1 commit intoMar 5, 2026
Merged
Conversation
cole-h
added
the
upload to s3
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughSet DeterminateNixDaemon plist Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/action/common/configure_init_service.rs (1)
460-484:⚠️ Potential issue | 🟠 MajorUnconditional
systemctl enableon service contradicts socket activation goal.Line 344 documents the intended behavior: "The goal state is the
socketenabled and active, the service not enabled and stopped (it activates via socket activation)". However, lines 482-484 unconditionally enable the service regardless ofstart_daemon, which directly contradicts this goal and systemd socket activation best practices.With socket activation, the socket alone manages when the service starts—the service should remain disabled. This is inconsistent with how socket units are handled (gated on
start_daemonat line 463) and other operations like daemon-reload and kickstart (both gated onstart_daemon).Gate this on
*start_daemon:Suggested fix
- enable(service_dest.display().to_string().as_ref(), true) - .await - .map_err(Self::error)?; + if *start_daemon { + enable(service_dest.display().to_string().as_ref(), true) + .await + .map_err(Self::error)?; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/action/common/configure_init_service.rs` around lines 460 - 484, The code unconditionally calls enable(service_dest..., true) which contradicts the socket-activation goal; change this to only enable the service when start_daemon is true. Locate the enable call for service_dest and wrap it in a conditional checking *start_daemon (or pass a boolean derived from *start_daemon) so the service is not enabled when socket activation is desired; keep the existing enable(...) call and error handling inside that conditional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/action/common/configure_determinate_nixd_init_service/mod.rs`:
- Around line 207-212: The plist currently hardcodes run_at_load=true in
generate_plist(), causing launchd to ignore the start_daemon flag; modify
generate_plist to accept a boolean parameter (e.g., start_daemon: bool) and set
DeterminateNixDaemonPlist.run_at_load based on that parameter (true when
start_daemon is true, false otherwise), then thread the caller-side start_daemon
value into generate_plist(start_daemon) so the RunAtLoad behaviour matches the
start_daemon setting.
---
Outside diff comments:
In `@src/action/common/configure_init_service.rs`:
- Around line 460-484: The code unconditionally calls enable(service_dest...,
true) which contradicts the socket-activation goal; change this to only enable
the service when start_daemon is true. Locate the enable call for service_dest
and wrap it in a conditional checking *start_daemon (or pass a boolean derived
from *start_daemon) so the service is not enabled when socket activation is
desired; keep the existing enable(...) call and error handling inside that
conditional.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/action/common/configure_determinate_nixd_init_service/mod.rssrc/action/common/configure_init_service.rs
cole-h
force-pushed
the
sep-cole/nix-284-automatically-start-determinate-nix-daemon-after
branch
from
fc6cc34 to
06bf5fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/action/common/configure_init_service.rs (1)
225-229:⚠️ Potential issue | 🟡 Minor
execute_descriptionnot updated to reflect the new unconditional service enableLines 225–229 document socket enables only when
start_daemonis true. The new unconditionalenable --nowon the service unit has no corresponding description entry. Users inspecting the planned actions will not see this step.📝 Proposed fix
if self.start_daemon { for SocketFile { name, .. } in self.socket_files.iter() { explanation.push(format!("Run `systemctl enable --now {}`", name)); } + explanation.push(format!( + "Run `systemctl enable --now {}`", + self.service_dest + .as_ref() + .expect("service_dest should be defined for systemd") + .display() + )); }Adjust the condition to match whatever guard is ultimately applied to the
enablecall.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/action/common/configure_init_service.rs` around lines 225 - 229, The description block in execute_description currently only documents socket enables under the conditional using self.start_daemon and iterating over self.socket_files (SocketFile { name, .. }) and pushing explanation entries; update it so the planned-action text matches the actual unconditional service "systemctl enable --now <service>" call: either remove the self.start_daemon guard around the explanation push or add an unconditional explanation.push for the service unit enable, and ensure the same service unit name used in the enable code is referenced in the explanation (use the same symbol/name as in the enable logic) so users see the service enable step in execute_description.
♻️ Duplicate comments (1)
src/action/common/configure_determinate_nixd_init_service/mod.rs (1)
207-209:⚠️ Potential issue | 🟠 Major
run_at_load: truestill ignoresstart_daemon— previously flagged
generate_plist()takes no parameters, soRunAtLoadis hardcoded totrue. Becauseretry_bootstrap(line 314) loads the plist with this flag, the daemon starts immediately upon installation even whenstart_daemon=false. The past review recommended threadingstart_daemonintogenerate_plist(run_at_load: bool)to align launchd behavior with the systemd path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/action/common/configure_determinate_nixd_init_service/mod.rs` around lines 207 - 209, generate_plist currently hardcodes run_at_load to true, ignoring the start_daemon flag; change the function signature generate_plist to accept a run_at_load: bool parameter and use that value for the DeterminateNixDaemonPlist.run_at_load field, then update all call sites (notably retry_bootstrap which loads the plist) to pass the start_daemon boolean so launchd behavior matches the systemd path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/action/common/configure_init_service.rs`:
- Around line 481-484: The enable call currently forces starting the unit by
passing true; change it to use the same guard as the socket enables (i.e.,
compute enable_now as *start_daemon || any_socket_was_active and pass that
instead of true), and ensure daemon-reload has run before calling enable so
systemd knows about the new unit (move or duplicate the daemon-reload invocation
outside the if *start_daemon guard or run it when enable_now is true). Also
avoid invoking enable --now when systemd is not running by ensuring the
plan/execute checks for /run/systemd/system are respected when enable_now is
true; use the existing start_daemon / any_socket_was_active logic rather than
unconditional true in the enable(service_dest.display().to_string().as_ref(),
...) call.
---
Outside diff comments:
In `@src/action/common/configure_init_service.rs`:
- Around line 225-229: The description block in execute_description currently
only documents socket enables under the conditional using self.start_daemon and
iterating over self.socket_files (SocketFile { name, .. }) and pushing
explanation entries; update it so the planned-action text matches the actual
unconditional service "systemctl enable --now <service>" call: either remove the
self.start_daemon guard around the explanation push or add an unconditional
explanation.push for the service unit enable, and ensure the same service unit
name used in the enable code is referenced in the explanation (use the same
symbol/name as in the enable logic) so users see the service enable step in
execute_description.
---
Duplicate comments:
In `@src/action/common/configure_determinate_nixd_init_service/mod.rs`:
- Around line 207-209: generate_plist currently hardcodes run_at_load to true,
ignoring the start_daemon flag; change the function signature generate_plist to
accept a run_at_load: bool parameter and use that value for the
DeterminateNixDaemonPlist.run_at_load field, then update all call sites (notably
retry_bootstrap which loads the plist) to pass the start_daemon boolean so
launchd behavior matches the systemd path.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/action/common/configure_determinate_nixd_init_service/mod.rssrc/action/common/configure_init_service.rs
cole-h
force-pushed
the
sep-cole/nix-284-automatically-start-determinate-nix-daemon-after
branch
from
06bf5fc to
3239b5e
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/action/common/configure_init_service.rs (1)
223-229:⚠️ Potential issue | 🟡 Minor
execute_descriptiondoes not mention the new service-enable stepThe service unit is now unconditionally enabled (with
--nowwhenstart_daemon=true, plain otherwise), butexecute_descriptionhas no corresponding entry for it. This leaves the human-readable plan description out of sync with whatexecuteactually does.📝 Proposed fix
if self.start_daemon { for SocketFile { name, .. } in self.socket_files.iter() { explanation.push(format!("Run `systemctl enable --now {}`", name)); } + explanation.push(format!( + "Run `systemctl enable --now {}`", + self.service_dest + .as_ref() + .expect("service_dest should be defined for systemd") + .display() + )); + } else { + explanation.push(format!( + "Run `systemctl enable {}`", + self.service_dest + .as_ref() + .expect("service_dest should be defined for systemd") + .display() + )); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/action/common/configure_init_service.rs` around lines 223 - 229, execute_description is out of sync with execute: the code now enables the service units unconditionally but the human-readable plan (explanation vector built in execute_description) lacks that step. Update execute_description so it iterates over self.socket_files (SocketFile { name, .. }) and pushes an appropriate enable message for each service—use "Run `systemctl enable --now {name}`" when self.start_daemon is true and "Run `systemctl enable {name}`" when false—so the explanation matches what execute performs.
♻️ Duplicate comments (1)
src/action/common/configure_init_service.rs (1)
344-344: Stale comment: "the service not enabled and stopped" no longer holdsThe comment still claims the goal state leaves the service not enabled, but the code now explicitly enables it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/action/common/configure_init_service.rs` at line 344, Update the stale inline comment that says "the service not enabled and stopped" to match the current behavior where the service is explicitly enabled; locate the comment near the configure_init_service logic (e.g., around the block that enables the service/socket in configure_init_service or the variables named `socket` and `service`) and change it to state the actual goal state (e.g., both socket and service are enabled and active, service may be socket-activated) or remove the inaccurate clause.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/action/common/configure_init_service.rs`:
- Around line 223-229: execute_description is out of sync with execute: the code
now enables the service units unconditionally but the human-readable plan
(explanation vector built in execute_description) lacks that step. Update
execute_description so it iterates over self.socket_files (SocketFile { name, ..
}) and pushes an appropriate enable message for each service—use "Run `systemctl
enable --now {name}`" when self.start_daemon is true and "Run `systemctl enable
{name}`" when false—so the explanation matches what execute performs.
---
Duplicate comments:
In `@src/action/common/configure_init_service.rs`:
- Line 344: Update the stale inline comment that says "the service not enabled
and stopped" to match the current behavior where the service is explicitly
enabled; locate the comment near the configure_init_service logic (e.g., around
the block that enables the service/socket in configure_init_service or the
variables named `socket` and `service`) and change it to state the actual goal
state (e.g., both socket and service are enabled and active, service may be
socket-activated) or remove the inaccurate clause.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/action/common/configure_determinate_nixd_init_service/mod.rssrc/action/common/configure_init_service.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/action/common/configure_determinate_nixd_init_service/mod.rs
cole-h
force-pushed
the
sep-cole/nix-284-automatically-start-determinate-nix-daemon-after
branch
from
3239b5e to
fa7231d
Compare
grahamc
approved these changes
Feb 26, 2026
grahamc
deleted the
sep-cole/nix-284-automatically-start-determinate-nix-daemon-after
branch
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
2 participants
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.
Description
Checklist
cargo fmtnix buildnix flake checkValidating with
install.determinate.systemsIf a maintainer has added the
upload to s3label to this PR, it will become available for installation viainstall.determinate.systems:Summary by CodeRabbit