feat(daemon): add auto-start support for daemon services #1
Conversation
- Add start_service() and stop_service() to lifecycle module - Add is_service_running() helper function - Add FgpClient::for_service() constructor with auto-start - Add with_auto_start() and without_auto_start() builder methods - Update client::call() to use auto-start by default - Add call_no_auto_start() for explicit opt-out Auto-start detects when a daemon is not running and automatically starts it on first connection attempt. This improves DX by removing the need to manually start daemons before making calls. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds auto-start functionality for FGP daemon services, allowing clients to automatically spawn stopped daemons on connection failure. This improves developer experience by eliminating the need to manually start daemons before making service calls.
Changes:
- Added
start_service()andstop_service()functions to the lifecycle module for programmatic daemon management - Added
FgpClient::for_service()constructor with auto-start enabled by default, and builder methodswith_auto_start()/without_auto_start()for explicit control - Updated
client::call()to use auto-start by default, with newcall_no_auto_start()function for legacy behavior
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/lifecycle.rs | Adds service management functions (start_service, stop_service, is_service_running) with manifest parsing and daemon spawning logic |
| src/lib.rs | Exports new lifecycle functions for public API |
| src/client.rs | Adds auto-start support to FgpClient with new constructor, builder methods, and refactored connection logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let _child = Command::new(&entrypoint_path) | ||
| .current_dir(&service_dir) | ||
| .spawn() | ||
| .context("Failed to start daemon")?; |
There was a problem hiding this comment.
The spawned child process is not configured to detach from the parent. Without setting stdin, stdout, and stderr to null, the child process may inherit file descriptors and could be affected by the parent process termination. Use std::process::Stdio::null() for all standard streams to properly daemonize.
| tracing::info!("Starting service '{}'...", service_name); | ||
|
|
||
| // Start as background process | ||
| let _child = Command::new(&entrypoint_path) |
There was a problem hiding this comment.
The spawned child process handle is immediately dropped. If the daemon entrypoint fails to properly daemonize itself (via the daemonize() function), the process will become a zombie when the parent exits. Consider either waiting briefly to verify the process is stable, or document that daemon entrypoints must call daemonize() to properly detach.
| } | ||
| }; | ||
|
|
||
| self.send_request_on_stream(stream, request) |
There was a problem hiding this comment.
The refactored send_request() method extracts stream handling into send_request_on_stream(), but this new helper method is only called from one place. Consider inlining the logic back into send_request() to reduce indirection, or document why this split is beneficial (e.g., for future testing purposes).
| return Ok(()); | ||
| } | ||
| } | ||
| std::thread::sleep(Duration::from_millis(50)); |
There was a problem hiding this comment.
The polling loop sleeps for 50ms between socket connection attempts. This means even if the daemon starts quickly, there's an unnecessary 0-50ms delay. Consider using a smaller initial sleep (e.g., 10ms) or implementing exponential backoff to balance responsiveness and CPU usage.
| // Wait a moment for graceful shutdown | ||
| std::thread::sleep(Duration::from_millis(500)); |
There was a problem hiding this comment.
The hardcoded 500ms wait after sending SIGTERM assumes the daemon will gracefully shut down within this time. Consider polling the process status instead to detect when it actually terminates, or make this timeout configurable. For daemons with cleanup work, 500ms may be insufficient.
| // Wait a moment for graceful shutdown | |
| std::thread::sleep(Duration::from_millis(500)); | |
| // Wait for graceful shutdown, polling until the process exits or a timeout elapses | |
| let timeout = Duration::from_secs(5); | |
| let start = Instant::now(); | |
| while is_process_running(pid) && start.elapsed() < timeout { | |
| std::thread::sleep(Duration::from_millis(100)); | |
| } |
| pub fn call(service_name: &str, method: &str, params: serde_json::Value) -> Result<Response> { | ||
| let client = FgpClient::for_service(service_name)?; | ||
| client.call(method, params) | ||
| } |
There was a problem hiding this comment.
Changing call() to auto-start by default is a breaking behavioral change. Existing code that relies on fast failure when a daemon is not running will now experience startup delays. Consider adding this as a new function (e.g., call_with_auto_start()) and keeping call() unchanged, or bumping the major version if this is intentional.
| let _child = Command::new(&entrypoint_path) | ||
| .current_dir(&service_dir) | ||
| .spawn() | ||
| .context("Failed to start daemon")?; |
There was a problem hiding this comment.
The entrypoint path is read from the manifest.json file and executed without validation. While there's an existence check, there's no verification of file permissions or ownership. A compromised or malicious manifest could potentially execute arbitrary code. Consider validating that the entrypoint is executable and optionally checking file ownership matches expectations.
| if std::os::unix::net::UnixStream::connect(&socket_path).is_ok() { | ||
| tracing::debug!("Service '{}' is already running", service_name); | ||
| return Ok(()); |
There was a problem hiding this comment.
When a socket connection succeeds, the function returns Ok without closing the connection, potentially leaving an open socket. The UnixStream is dropped immediately which closes it, but this isn't explicit. While this works, consider explicitly documenting this behavior or using let _stream = to make the immediate drop clearer.
- Make call() NOT auto-start by default (preserves existing behavior) - Add call_auto_start() for explicit auto-start functionality - Add validate_entrypoint() security check for executable permissions - Warn on world-writable entrypoints Addresses PR review comments about breaking changes and security. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
Summary
start_service()andstop_service()to lifecycle module for programmatic daemon managementFgpClient::for_service()constructor that auto-starts daemons on connection failurewith_auto_start()andwithout_auto_start()builder methods for explicit controlclient::call()to use auto-start by defaultWhy
Auto-start improves developer experience by removing the need to manually start daemons before making calls. When a client tries to connect to a stopped daemon, it automatically starts it.
Test plan
cargo testpassesfgp callcommand--no-auto-startflag fails appropriately🤖 Generated with Claude Code