Skip to content

feat(node): manage user-level daemon service - #685

Open
rings-auto-reviewer[bot] wants to merge 2 commits into
masterfrom
codex/daemon-service
Open

feat(node): manage user-level daemon service#685
rings-auto-reviewer[bot] wants to merge 2 commits into
masterfrom
codex/daemon-service

Conversation

@rings-auto-reviewer

Copy link
Copy Markdown
Contributor

Summary

  • add rings daemon start, stop, status, and restart
  • install and manage Rings as a user-level launchd service on macOS
  • install and manage Rings as a systemd user service on Linux
  • preserve the selected config path, log level, and Tokio runtime in the service definition
  • document daemon behavior and keep the existing CLI command tree compatible

Behavior

  • start installs/enables the service and starts it
  • stop stops the current service without disabling login startup
  • status reports service-manager state, autostart state, and the installed definition path
  • restart reloads and restarts an installed service
  • macOS definitions live under ~/Library/LaunchAgents
  • Linux definitions live under XDG_CONFIG_HOME/systemd/user or ~/.config/systemd/user
  • service definitions are written atomically and command arguments are escaped for the target service manager

The command surface intentionally contains only the four requested daemon actions. The help entry is generated by Clap.

Validation

  • cargo +nightly fmt --all -- --check
  • cargo test -p rings-node --bin rings --no-default-features --features node
    • 9 passed, 0 failed
    • includes all four daemon actions and representative parsing coverage for every pre-existing command family
  • strict Clippy with warnings, unwrap, expect, panic, todo, unimplemented, and unchecked indexing denied
  • native macOS build and CLI help inspection
  • locked Linux cargo check in a read-only Rust container
  • git diff --check

Follow-up

OS-level transparent TCP routing over Rings onion circuits is tracked separately in #684.

@RyanKung

Copy link
Copy Markdown
Member

Review

Read the full diff (+1039 / -3, crates/node/bin/daemon.rs at 912 lines) and cross-checked it against the current tree. Findings below, ordered by severity.

Blocking

1. The cfg(not(any(macos, linux))) branch cannot compile — the "supported only on macOS and Linux" degradation is not real.

NativeServiceManager (crates/node/bin/daemon.rs:304) has both variants behind #[cfg(target_os = "macos")] / #[cfg(target_os = "linux")]. On any third platform it is a zero-variant enum, so all seven match self { ... } blocks in the impl (daemon.rs:317-380) become empty matches on &Self, which is E0004 — references are always considered inhabited. Reproduced:

error[E0004]: non-exhaustive patterns: type `&E` is non-empty
  = note: references are always considered inhabited

Consequently DaemonError::UnsupportedPlatform (daemon.rs:56) and the fallback current_service_manager (daemon.rs:392) are unreachable dead code, and that build would additionally trip dead_code on write_atomic, run_checked, and create_directory under -D warnings. CI only runs ubuntu + macos (.github/workflows/qaci.yml:132,270), so this path has never been exercised. Either emit a module-level compile_error!, or give the enum a real Unsupported(ServiceLayout) variant.

2. Incomplete model: the service definition carries only three settings, the rest are silently dropped.

ServiceSpec::arguments() (daemon.rs:290) emits exactly rings --log-level L --runtime R run --config C. But RunCommand (crates/node/bin/rings.rs:195-330) has ~25 flags, nearly all of them env-backed, and main calls dotenvy::dotenv() (rings.rs:810), which reads .env from the current working directory. Neither service definition sets WorkingDirectory, and neither sets Environment= / EnvironmentVariables:

  • launchd runs with cwd /; systemd --user runs with cwd $HOME — the two platforms already disagree with each other.
  • A user whose rings run works in a project directory (.env holding ECDSA_KEY, ICE_SERVERS, EXTERNAL_IP) gets a materially different node from rings daemon start, with no diagnostic.

Same command, two semantics. Either propagate the environment explicitly, or pin WorkingDirectory, or state prominently in the README that the daemon takes every runtime setting from the config file only.

3. On macOS, start SIGKILLs the process it just launched.

LaunchdManager::start (daemon.rs:499) does unload_if_loadedbootstrap (the plist sets RunAtLoad=true, so the node is already running at this point) → kickstart -k, and -k means SIGKILL-then-restart. The node bootstrap just started is killed milliseconds later. -k is only meaningful against an already-running job; after a fresh bootstrap it should be dropped.

The same function has the classic launchd race: bootout is asynchronous, so the immediately following bootstrap can fail with Bootstrap failed: 5: Input/output error. There is no wait and no retry.

Abstraction

4. Hand-written vtable. daemon.rs:304-380 is seven match self blocks that each forward to one of two structs — that is a trait, written by hand. A trait ServiceManager { fn start/stop/restart/status/name/definition_path } plus one cfg selecting the concrete type removes ~70 lines and makes finding 1 disappear on its own.

5. cfg noise distorts the names. #[cfg(any(target_os = "macos", test))] appears around twenty times, and it has forced two parameter names that state the wrong thing: _layout (daemon.rs:273) is used on macOS, and _xdg_config_home (daemon.rs:231) is used on Linux. The natural split is daemon/mod.rs + daemon/launchd.rs + daemon/systemd.rs, each platform file carrying #![cfg(...)] once, with each renderer's tests sitting next to it. That also brings the 912-line file back down.

6. format_command renders human-facing errors with systemd quoting. daemon.rs:813 reuses systemd_quote, so a failed /bin/launchctl invocation on macOS is echoed to the user with $ shown as $$ and % shown as %%. "Quote for a systemd ExecStart" and "render a command line for a human" are two different propositions sharing one function.

7. The exit status in the error type is stringly typed. DaemonError::CommandFailed.status: String (daemon.rs:114) stores ExitStatus.to_string(), but ExitStatus is Copy + Display — store the value. Likewise CommandFailureDetail(String) encodes "empty string means no detail" inside its Display impl; Option puts that proposition in the type.

8. create_directory does not create the directory it names, and duplicates an existing helper. daemon.rs:760 creates path.parent(), not path. And rings_node::util::ensure_parent_dir (crates/node/src/util.rs:96) already is exactly this function, available in the same crate.

9. launchd_domain() forks /usr/bin/id -u on every call. daemon.rs:746. wait_for_running (daemon.rs:440) polls up to 21 times, and each poll runs id plus launchctl print — up to 42 child processes for a single daemon start. The uid is invariant for the process; resolve it once when constructing LaunchdManager, which also moves InvalidUserId off the hot path.

10. Inconsistent executable resolution. macOS uses absolute /bin/launchctl and /usr/bin/id; Linux resolves systemctl through PATH (daemon.rs:569 and friends). A process that writes autostart definitions should use absolute paths uniformly.

11. The ValueEnum name table is now transcribed a third time. RuntimeFlavor::as_str (rings.rs:73) and log_level_name (rings.rs:940) duplicate the names generated by clap's ValueEnum derive; LogLevel already carries another copy in its FromStr (crates/node/src/logging.rs:47). Three sources of truth, and nothing witnesses that they agree. (The &'static str requirement makes to_possible_value() awkward — so pin it with a test instead, see 13.)

12. Blocking calls run inside the async context. daemon::execute uses thread::sleep and Command::output(), reached through run(cli) (rings.rs:835), which main enters via runtime.block_on(run(cli)). The daemon subcommand needs no tokio runtime at all — dispatching it in main before the runtime is built gives a much cleaner effect boundary.

Tests

13. The one law this feature depends on is not witnessed: the generated argv must parse back into the same CLI. One line covers it:

assert!(Cli::try_parse_from(service_spec().arguments()).is_ok());

plus a round-trip over every LogLevel / RuntimeFlavor variant (Law: forall v. parse(name(v)) == v). The current tests assert substrings of the rendered plist and unit, so they would stay green if the --log-level value name drifted.

14. cli_tests.rs only asserts is_ok(). It never asserts that rings daemon frobnicate is rejected, never parses daemon start -c <path>, and does not call Cli::command().debug_assert() — clap's own structural self-check, one line, and the one that most belongs here.

15. ServiceLayout::from_home is only tested with an absolute XDG_CONFIG_HOME. The is_absolute filter in discover (daemon.rs:223) is the interesting branch; neither the unset case nor the relative case is covered.

16. write_atomic leaks its .tmp file when rename fails (daemon.rs:772) — no cleanup, no test.

Documentation

17. Command::Run's clap about is still "Starts a long-running node daemon." (rings.rs:154), while the README rewritten in this PR says "run: runs the node in the foreground" and rings daemon now means something else entirely. The terminology should be swept in the same commit.

18. Neither README states where the daemon logs go (~/.rings/logs/daemon.log on macOS, the journal on Linux); that systemctl --user over SSH fails without a session bus and stops at logout without loginctl enable-linger; or that stop preserves the registration while no command removes it — the user has to delete the plist/unit by hand.

Noted as good

Deleting the stale "validate transactions / maintain the blockchain / earn rewards" copy from crates/node/README.md is a real improvement. The systemd_quote escaping ($$, %%, \\, \") checks out clause by clause. No unwrap / expect / unchecked indexing on production paths. The error type is a proper algebraic thiserror enum with #[source] throughout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant