Add Windows daemon mode with named-pipe IPC#200
Add Windows daemon mode with named-pipe IPC#200Snowy7 wants to merge 6 commits intospacedriveapp:mainfrom
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:
WalkthroughAdds cross-platform daemon control (Unix domain sockets on Unix, named pipes on Windows), platform-specific filesystem usage reporting, expanded daemon lifecycle and IPC types/functions, and a CLI Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI / User
participant Daemon as Daemon Process
participant IPC as IPC Transport
participant OS as OS / Kernel
rect rgba(100,150,200,0.5)
Note over CLI,Daemon: Daemon startup and listener setup
CLI->>Daemon: start (may daemonize)
Daemon->>Daemon: write PID file
Daemon->>IPC: create listener (socket or pipe)
IPC->>OS: bind/register transport
end
rect rgba(150,200,100,0.5)
Note over CLI,Daemon: IPC command flow
CLI->>IPC: connect & send JSON IpcCommand
IPC->>Daemon: deliver command
Daemon->>Daemon: handle command (Status / Shutdown)
Daemon->>IPC: send JSON IpcResponse
IPC->>CLI: return response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| let mut first_server_options = ServerOptions::new(); | ||
| first_server_options.first_pipe_instance(true); | ||
| let mut server = first_server_options | ||
| .create(&pipe_name) |
There was a problem hiding this comment.
For Windows IPC: it might be worth explicitly restricting the named pipe to the current user (and/or rejecting remote clients, depending on what ServerOptions exposes). As written, any local process that can guess the pipe name can connect and send Shutdown.
There was a problem hiding this comment.
valid hardening concern.
I’ve kept the current PR scoped to cross-platform daemon functionality + reliability fixes, and I don’t want to ship a partial/incorrect Windows ACL implementation in this patch. Tokio’s named pipe defaults already reject remote clients, but local-user/process access control still needs a proper security descriptor/ACL setup.
There was a problem hiding this comment.
Pull request overview
This pull request adds Windows daemon mode support to Spacebot, enabling background operation on Windows systems. The implementation uses named pipes for IPC instead of Unix domain sockets, and implements process detachment via re-execution with platform-specific flags. The changes maintain the existing daemon API boundary while adding platform-specific implementations.
Changes:
- Added Windows-specific daemonization via detached process re-execution with
--daemon-childflag - Implemented named pipe IPC for Windows daemon control (start/stop/status)
- Added Windows filesystem capacity reporting using
GetDiskFreeSpaceExWAPI - Moved
daemonizecrate to Unix-only dependency and addedwindows-sysfor Windows
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.rs | Added daemon_child flag to prevent re-running onboarding and daemonization in child processes |
| src/daemon.rs | Added platform-specific implementations for daemonization, IPC server using named pipes (Windows) or Unix sockets (Unix), and process liveness checks |
| src/api/system.rs | Added Windows implementation of filesystem usage reporting using GetDiskFreeSpaceExW API |
| Cargo.toml | Made daemonize Unix-only dependency and added windows-sys for Windows with required features |
| Cargo.lock | Updated lockfile with windows-sys dependency |
| README.md | Documented that Windows uses named pipes for daemon control |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| Err(error) => { | ||
| return Err(error).with_context(|| { | ||
| format!("failed to connect to spacebot daemon pipe {pipe_name}") |
There was a problem hiding this comment.
For consistency with the Unix version, consider changing the error message to match: "failed to connect to spacebot daemon. is it running?" This provides a more user-friendly message that doesn't expose internal implementation details like the pipe name.
| format!("failed to connect to spacebot daemon pipe {pipe_name}") | |
| "failed to connect to spacebot daemon. is it running?" |
There was a problem hiding this comment.
including the pipe name helps diagnose local IPC issues during bring-up/debugging (especially on Windows where pipe naming is less obvious than Unix sockets).
| #[cfg(not(unix))] | ||
| { | ||
| #[cfg(windows)] | ||
| { | ||
| let mut available_bytes = 0u64; | ||
| let mut total_bytes = 0u64; | ||
| let mut free_bytes = 0u64; | ||
| let mut path_wide: Vec<u16> = path.as_os_str().encode_wide().collect(); | ||
| path_wide.push(0); | ||
|
|
||
| let result = unsafe { | ||
| GetDiskFreeSpaceExW( | ||
| path_wide.as_ptr(), | ||
| &mut available_bytes, | ||
| &mut total_bytes, | ||
| &mut free_bytes, | ||
| ) | ||
| }; | ||
| if result == 0 { | ||
| return Err(anyhow::anyhow!("GetDiskFreeSpaceExW call failed")); | ||
| } | ||
|
|
||
| let total_bytes = (block_size * total_blocks) as u64; | ||
| let used_bytes = directory_size_bytes(path)?; | ||
| let available_bytes = (block_size * avail_blocks) as u64; | ||
| let used_bytes = directory_size_bytes(path)?; | ||
| return Ok(StorageStatus { | ||
| used_bytes, | ||
| total_bytes, | ||
| available_bytes, | ||
| }); | ||
| } | ||
|
|
||
| Ok(StorageStatus { | ||
| used_bytes, | ||
| total_bytes, | ||
| available_bytes, | ||
| }) | ||
| #[cfg(not(windows))] | ||
| let used_bytes = directory_size_bytes(path)?; | ||
| #[cfg(not(windows))] | ||
| Ok(StorageStatus { | ||
| used_bytes, | ||
| total_bytes: 0, | ||
| available_bytes: 0, | ||
| }) | ||
| } |
There was a problem hiding this comment.
The nested conditional compilation structure (#[cfg(not(unix))] containing #[cfg(windows)] and #[cfg(not(windows))]) is complex and could be simplified. Consider using mutually exclusive top-level conditions like #[cfg(unix)], #[cfg(windows)], and #[cfg(not(any(unix, windows)))] for better readability and maintainability.
There was a problem hiding this comment.
Readability improvement only, behavior is correct and covered by cargo check --release after the platform changes.
Can be done in a follow up.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/daemon.rs (1)
97-134:⚠️ Potential issue | 🟡 MinorWindows
is_running()lacks pipe verification and can misidentify stale PID entries.The Windows implementation only checks PID liveness. A reused PID or IPC server startup failure would still return
Some(pid), blocking new daemon starts and making stop/status commands misleading. The Unix path verifies socket connectivity; Windows should similarly probe the named pipe. Consider adding the same pattern used insend_command(): attempt to open the pipe and treatERROR_PIPE_BUSYas running.🔧 Suggested adjustment (Windows pipe probe)
#[cfg(not(unix))] pub fn is_running(paths: &DaemonPaths) -> Option<u32> { let pid = read_pid_file(&paths.pid_file)?; if !is_process_alive(pid) { cleanup_stale_files(paths); return None; } + #[cfg(windows)] + { + let pipe_name = paths.pipe_name(); + match ClientOptions::new().open(&pipe_name) { + Ok(stream) => { + drop(stream); + return Some(pid); + } + Err(error) if error.raw_os_error() == Some(ERROR_PIPE_BUSY as i32) => { + return Some(pid); + } + Err(_) => { + cleanup_stale_files(paths); + return None; + } + } + } + Some(pid) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daemon.rs` around lines 97 - 134, The Windows branch of is_running currently only checks PID liveness and should also probe the daemon's named pipe similar to the Unix socket check; update the non-unix is_running implementation (which calls read_pid_file, is_process_alive, cleanup_stale_files) to attempt opening the IPC pipe (use the same logic/pattern as send_command()), treat a successful connect or ERROR_PIPE_BUSY as evidence the daemon is running (drop the handle and return Some(pid)), and on pipe-open failures treat the PID as stale by calling cleanup_stale_files(paths) and returning None.
🤖 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/daemon.rs`:
- Around line 97-134: The Windows branch of is_running currently only checks PID
liveness and should also probe the daemon's named pipe similar to the Unix
socket check; update the non-unix is_running implementation (which calls
read_pid_file, is_process_alive, cleanup_stale_files) to attempt opening the IPC
pipe (use the same logic/pattern as send_command()), treat a successful connect
or ERROR_PIPE_BUSY as evidence the daemon is running (drop the handle and return
Some(pid)), and on pipe-open failures treat the PID as stale by calling
cleanup_stale_files(paths) and returning None.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lock,!**/*.lockCargo.tomlis excluded by!**/*.toml
📒 Files selected for processing (4)
README.mdsrc/api/system.rssrc/daemon.rssrc/main.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/daemon.rs (1)
634-643: Non-(unix|windows) builds can still hit cfg-gated Context usage in write_pid_file.This remains compiled without cfg, but Context is only imported under cfg(any(unix, windows)). Consider gating write_pid_file or importing Context unconditionally.
#!/bin/bash # Inspect Context import gating and write_pid_file definition. rg -nC2 "anyhow::Context|write_pid_file"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daemon.rs` around lines 634 - 643, The write_pid_file function uses anyhow::Context methods but Context is only imported under cfg(any(unix, windows)), which breaks non-(unix|windows) builds; either gate the write_pid_file definition with the same cfg(any(unix, windows)) attribute (so it is only compiled when Context is available) or move the Context import out of its cfg so it is imported unconditionally; update the module-level import or add the same cfg attribute to the write_pid_file fn to ensure the symbol Context and the method with_context are available when the function is compiled.
🤖 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/daemon.rs`:
- Around line 679-684: The cleanup_stale_files function currently discards
remove_file Results with "let _ =", which violates the no-silent-errors rule;
update cleanup_stale_files to handle errors from std::fs::remove_file on
paths.pid_file (and on paths.socket inside the #[cfg(unix)] block) by either
returning a Result and propagating errors or by logging failures (e.g., using
the crate logger/error macro) with a clear message including the path and error;
replace the "let _ =" calls with proper match/if let Err(e) handlers that log or
propagate the error so filesystem delete failures are not silently ignored.
---
Duplicate comments:
In `@src/daemon.rs`:
- Around line 634-643: The write_pid_file function uses anyhow::Context methods
but Context is only imported under cfg(any(unix, windows)), which breaks
non-(unix|windows) builds; either gate the write_pid_file definition with the
same cfg(any(unix, windows)) attribute (so it is only compiled when Context is
available) or move the Context import out of its cfg so it is imported
unconditionally; update the module-level import or add the same cfg attribute to
the write_pid_file fn to ensure the symbol Context and the method with_context
are available when the function is compiled.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/daemon.rs (1)
468-471:⚠️ Potential issue | 🟡 Minor
let _ = std::fs::remove_file(...)silently discards errors.This violates the "no
let _ =on Results" guideline. Thecleanup_stale_filesfunction (line 704) already demonstrates the correct pattern — log on failure, ignoreNotFound.Suggested fix
tokio::spawn(async move { let _ = cleanup_rx.wait_for(|shutdown| *shutdown).await; - let _ = std::fs::remove_file(&cleanup_socket); + if let Err(error) = std::fs::remove_file(&cleanup_socket) + && error.kind() != std::io::ErrorKind::NotFound + { + tracing::warn!(%error, path = %cleanup_socket.display(), "failed to remove socket on shutdown"); + } });As per coding guidelines, "Don't silently discard errors; no let _ = on Results. Handle, log, or propagate errors."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daemon.rs` around lines 468 - 471, The spawned task currently drops the Result from std::fs::remove_file(&cleanup_socket) using "let _ =" which silently discards errors; change the tokio::spawn block that awaits cleanup_rx.wait_for(|shutdown| *shutdown) to handle the remove_file Result the same way cleanup_stale_files does: attempt to remove cleanup_socket, and on Err check for std::io::ErrorKind::NotFound (ignore) otherwise log the error via your logger (or eprintln!) with context (e.g., "failed to remove cleanup socket") so failures are visible instead of being discarded; reference the existing cleanup_stale_files error-handling pattern and functions (cleanup_rx.wait_for and cleanup_socket) when making the change.
🧹 Nitpick comments (4)
src/daemon.rs (4)
84-91: Moveuse sha2::Digest as _before the first usage for readability.The
useitem on line 86 is technically visible throughout the block (Rust items are order-independent), but placing it afterSha256::new()— which depends onDigest— is misleading to readers.Suggested reorder
#[cfg(windows)] fn pipe_name(&self) -> String { + use sha2::Digest as _; let mut hasher = sha2::Sha256::new(); - use sha2::Digest as _; hasher.update(self.pid_file.as_os_str().to_string_lossy().as_bytes());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daemon.rs` around lines 84 - 91, In the pipe_name function move the import "use sha2::Digest as _" to the top of the function before calling sha2::Sha256::new(); specifically, place the use statement before the hasher initialization so Digest is clearly in scope for functions like finalize() and encode_hex, keeping the rest of pipe_name (hasher.update, finalize, digest.encode_hex, format!) unchanged.
538-563: Connect phase is bounded, but the command/response exchange has no timeout.The
connect_deadlineloop (line 541-560) correctly caps the connect wait at 5 s. However,send_command_over_streamon line 562 can still block indefinitely if the daemon accepts the pipe but never sends a response.Consider wrapping the whole exchange in
tokio::time::timeout:Sketch
- send_command_over_stream(stream, command).await + tokio::time::timeout( + std::time::Duration::from_secs(10), + send_command_over_stream(stream, command), + ) + .await + .map_err(|_| anyhow!("timed out waiting for daemon IPC response"))?The same applies to the Unix
send_command(line 478-483).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daemon.rs` around lines 538 - 563, The connect loop in send_command (Windows: send_command(paths: &DaemonPaths, command: IpcCommand)) bounds only the connection phase but not the subsequent request/response, so wrap the entire exchange (the ClientOptions::open result followed by the call to send_command_over_stream(stream, command).await) in a tokio::time::timeout using a reasonable total timeout (e.g. 5s or configurable) and return a timed-out error if exceeded; apply the same change to the Unix variant of send_command so both connection+exchange are time-limited and the function returns an appropriate anyhow::Error on timeout.
195-222:std::process::exit(0)inside a function returningResultis surprising and skips all caller cleanup.On Unix,
daemonize()returnsOk(())in the child and the caller drives the rest of the lifecycle. On Windows, the parent hard-exits here, bypassing any Drop guards or cleanup in the call stack. This asymmetry makes the contract ofdaemonizeplatform-dependent and fragile.Consider returning
Ok(())and letting the caller exit, or changing the return type to-> !/-> anyhow::Result<!>to make the divergence explicit.One option: return a sentinel and let the caller exit
Return a boolean or enum indicating "I am the parent, you should exit now" versus "I am the child/daemon, continue":
- eprintln!("spacebot daemon started (pid {})", child.id()); - std::process::exit(0); + eprintln!("spacebot daemon started (pid {})", child.id()); + // Signal to the caller that the parent should exit cleanly. + Err(anyhow!("__parent_exit__"))Or better, introduce a
DaemonizeResultenum withContinueAsChild/ParentShouldExit { child_pid }variants.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daemon.rs` around lines 195 - 222, The Windows implementation of daemonize calls std::process::exit(0) which aborts caller cleanup; instead change daemonize's contract to return a sentinel so the caller decides whether to exit: define a DaemonizeResult enum (e.g., ContinueAsChild / ParentShouldExit { child_pid: u32 }), change fn daemonize(...)->anyhow::Result<DaemonizeResult>, and in the Windows branch return ParentShouldExit with child.id() rather than calling std::process::exit; leave the Unix path to return ContinueAsChild so the caller can perform cleanup/exit as appropriate (update callers of daemonize/DaemonStartOptions to handle the new enum).
37-37: Use theSYNCHRONIZEconstant fromwindows_sysinstead of a magic number.The
windows_sys::Win32::Storage::FileSystem::SYNCHRONIZEconstant (value0x0010_0000) already exists in the crate and should be imported instead of the hardcodedSYNCHRONIZE_ACCESS.Suggested fix
#[cfg(windows)] use windows_sys::Win32::Foundation::{ CloseHandle, ERROR_PIPE_BUSY, WAIT_FAILED, WAIT_OBJECT_0, WAIT_TIMEOUT, }; #[cfg(windows)] +use windows_sys::Win32::Storage::FileSystem::SYNCHRONIZE; +#[cfg(windows)] use windows_sys::Win32::System::Threading::{Then replace
SYNCHRONIZE_ACCESSwithSYNCHRONIZEat line 679 and remove the constant definition at line 37.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daemon.rs` at line 37, Replace the hardcoded SYNCHRONIZE_ACCESS const with the existing constant from windows_sys: remove the const declaration SYNCHRONIZE_ACCESS and import/use windows_sys::Win32::Storage::FileSystem::SYNCHRONIZE instead, then replace all occurrences of SYNCHRONIZE_ACCESS (e.g., the use in the Open* / access flags) with SYNCHRONIZE so the crate-provided constant is used.
🤖 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/daemon.rs`:
- Around line 593-596: The code uses BufReader::read_line without a cap which
allows a client to stream an unbounded line and cause OOM; fix by enforcing a
maximum line length (e.g. const MAX_LINE_BYTES) and use
reader.take(MAX_LINE_BYTES) when calling AsyncBufReadExt::read_line (e.g. create
a limited_reader via reader.take(MAX_LINE_BYTES) and call read_line on &mut
limited_reader), return an error if the read hits the limit or if the line is
empty/too large; apply the same change to the response path in
send_command_over_stream so both request and response reads are bounded.
- Around line 210-213: On Windows the child process in the `command` builder
uses Stdio::null() for stdout/stderr so early crashes before
`init_background_tracing` produce no diagnostics; open the same log files used
on Unix (the paths handled by the `_paths` parameter) and replace
`stdin/stdout/stderr(std::process::Stdio::null())` with `Stdio::from()` wrappers
around those opened file handles so the detached child writes to the logs, and
remove the unused `_` prefix on ` _paths` (or otherwise use the parameter) so
Windows follows the same logging behavior as the `daemonize` Unix branch.
- Around line 675-696: The Windows implementation of is_process_alive uses
handle.is_null() but in windows-sys 0.59 HANDLE is an integer type (isize-like),
so replace the null-pointer check with handle == 0; also check the BOOL return
from CloseHandle and log an error if it fails instead of discarding it. Locate
the is_process_alive function and change the OpenProcess null-check to use
handle == 0, call CloseHandle(handle) and inspect its return value (FALSE) to
log failures, keeping the existing WaitForSingleObject handling (WAIT_TIMEOUT =>
true, others => false).
---
Outside diff comments:
In `@src/daemon.rs`:
- Around line 468-471: The spawned task currently drops the Result from
std::fs::remove_file(&cleanup_socket) using "let _ =" which silently discards
errors; change the tokio::spawn block that awaits cleanup_rx.wait_for(|shutdown|
*shutdown) to handle the remove_file Result the same way cleanup_stale_files
does: attempt to remove cleanup_socket, and on Err check for
std::io::ErrorKind::NotFound (ignore) otherwise log the error via your logger
(or eprintln!) with context (e.g., "failed to remove cleanup socket") so
failures are visible instead of being discarded; reference the existing
cleanup_stale_files error-handling pattern and functions (cleanup_rx.wait_for
and cleanup_socket) when making the change.
---
Nitpick comments:
In `@src/daemon.rs`:
- Around line 84-91: In the pipe_name function move the import "use sha2::Digest
as _" to the top of the function before calling sha2::Sha256::new();
specifically, place the use statement before the hasher initialization so Digest
is clearly in scope for functions like finalize() and encode_hex, keeping the
rest of pipe_name (hasher.update, finalize, digest.encode_hex, format!)
unchanged.
- Around line 538-563: The connect loop in send_command (Windows:
send_command(paths: &DaemonPaths, command: IpcCommand)) bounds only the
connection phase but not the subsequent request/response, so wrap the entire
exchange (the ClientOptions::open result followed by the call to
send_command_over_stream(stream, command).await) in a tokio::time::timeout using
a reasonable total timeout (e.g. 5s or configurable) and return a timed-out
error if exceeded; apply the same change to the Unix variant of send_command so
both connection+exchange are time-limited and the function returns an
appropriate anyhow::Error on timeout.
- Around line 195-222: The Windows implementation of daemonize calls
std::process::exit(0) which aborts caller cleanup; instead change daemonize's
contract to return a sentinel so the caller decides whether to exit: define a
DaemonizeResult enum (e.g., ContinueAsChild / ParentShouldExit { child_pid: u32
}), change fn daemonize(...)->anyhow::Result<DaemonizeResult>, and in the
Windows branch return ParentShouldExit with child.id() rather than calling
std::process::exit; leave the Unix path to return ContinueAsChild so the caller
can perform cleanup/exit as appropriate (update callers of
daemonize/DaemonStartOptions to handle the new enum).
- Line 37: Replace the hardcoded SYNCHRONIZE_ACCESS const with the existing
constant from windows_sys: remove the const declaration SYNCHRONIZE_ACCESS and
import/use windows_sys::Win32::Storage::FileSystem::SYNCHRONIZE instead, then
replace all occurrences of SYNCHRONIZE_ACCESS (e.g., the use in the Open* /
access flags) with SYNCHRONIZE so the crate-provided constant is used.
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/daemon.rs (1)
442-503:⚠️ Potential issue | 🟡 MinorDon’t ignore wait_for errors in the cleanup task.
Line 490 drops the Result fromwait_for; log or handle it to avoid silent failures.
As per coding guidelines, “Don’t silently discard errors; no let _ = on Results. Handle, log, or propagate errors. Only exception is .ok() on channel sends where the receiver may be dropped”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daemon.rs` around lines 442 - 503, The cleanup task inside start_ipc_server currently discards the Result from cleanup_rx.wait_for(...) with let _ = ..., so change it to capture and handle/log the result (e.g., match cleanup_rx.wait_for(|shutdown| *shutdown).await { Ok(_) => {}, Err(err) => tracing::warn!(%err, "cleanup wait_for failed") }), then proceed to remove the socket using proper error handling: call std::fs::remove_file(&cleanup_socket) and if Err(e) and e.kind() != std::io::ErrorKind::NotFound log that error (do not combine remove_file and kind check with && as currently written); reference start_ipc_server, cleanup_rx.wait_for, and cleanup_socket to locate the code to update.
♻️ Duplicate comments (1)
src/daemon.rs (1)
516-567:⚠️ Potential issue | 🟡 MinorRestrict named-pipe access to the current user.
Default pipe security allows any local process that can guess the name to sendShutdown; consider tightening ACLs or security attributes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daemon.rs` around lines 516 - 567, The IPC named pipe is created with default security allowing any local process to connect; update start_ipc_server to set a restrictive security descriptor/ACL so only the current user can connect. Concretely, when building ServerOptions (the first_server_options and the subsequent ServerOptions::new() calls used before create(&pipe_name)), add the platform-specific security attributes (e.g., set a SECURITY_DESCRIPTOR/SDDL that grants access only to the current user or call the crate API that sets security attributes) so create(&pipe_name) produces a pipe limited to the current user; apply the same secure ServerOptions for both the initial first_server_options and the next_server creation, and test that handle_ipc_stream still receives connections from the current user only.
🤖 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/daemon.rs`:
- Around line 442-503: The cleanup task inside start_ipc_server currently
discards the Result from cleanup_rx.wait_for(...) with let _ = ..., so change it
to capture and handle/log the result (e.g., match cleanup_rx.wait_for(|shutdown|
*shutdown).await { Ok(_) => {}, Err(err) => tracing::warn!(%err, "cleanup
wait_for failed") }), then proceed to remove the socket using proper error
handling: call std::fs::remove_file(&cleanup_socket) and if Err(e) and e.kind()
!= std::io::ErrorKind::NotFound log that error (do not combine remove_file and
kind check with && as currently written); reference start_ipc_server,
cleanup_rx.wait_for, and cleanup_socket to locate the code to update.
---
Duplicate comments:
In `@src/daemon.rs`:
- Around line 516-567: The IPC named pipe is created with default security
allowing any local process to connect; update start_ipc_server to set a
restrictive security descriptor/ACL so only the current user can connect.
Concretely, when building ServerOptions (the first_server_options and the
subsequent ServerOptions::new() calls used before create(&pipe_name)), add the
platform-specific security attributes (e.g., set a SECURITY_DESCRIPTOR/SDDL that
grants access only to the current user or call the crate API that sets security
attributes) so create(&pipe_name) produces a pipe limited to the current user;
apply the same secure ServerOptions for both the initial first_server_options
and the next_server creation, and test that handle_ipc_stream still receives
connections from the current user only.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/daemon.rs`:
- Around line 134-157: The is_running function treats all errors from
ClientOptions::open as stale and calls cleanup_stale_files, which causes the PID
to be removed when the named pipe is simply not created yet; update is_running
(and its Windows ClientOptions::open handling) to treat ErrorKind::NotFound (or
equivalent "pipe does not exist") as a transient startup state: either skip the
pipe check and return Some(pid) (or retry the open briefly, e.g., a short loop
with a small deadline similar to send_command) instead of calling
cleanup_stale_files, while preserving the existing behavior for ERROR_PIPE_BUSY
and only cleaning up on genuine non-transient errors.
| #[cfg(not(unix))] | ||
| pub fn is_running(paths: &DaemonPaths) -> Option<u32> { | ||
| let pid = read_pid_file(&paths.pid_file)?; | ||
|
|
||
| if !is_process_alive(pid) { | ||
| cleanup_stale_files(paths); | ||
| return None; | ||
| } | ||
|
|
||
| #[cfg(windows)] | ||
| { | ||
| let pipe_name = paths.pipe_name(); | ||
| match ClientOptions::new().open(&pipe_name) { | ||
| Ok(stream) => { | ||
| drop(stream); | ||
| Some(pid) | ||
| } | ||
| Err(error) if error.raw_os_error() == Some(ERROR_PIPE_BUSY as i32) => Some(pid), | ||
| Err(_) => { | ||
| cleanup_stale_files(paths); | ||
| None | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Windows named pipe CreateFile ClientOptions::open error codes FILE_NOT_FOUND vs PIPE_BUSY when pipe does not exist Rust tokio_named_pipes
💡 Result:
On Windows, these two errors mean different things for CreateFile(r"\\.\pipe\NAME", ...):
-
ERROR_FILE_NOT_FOUND(2) /io::ErrorKind::NotFound: there is no named-pipe object with that name right now (i.e., the server hasn’t created any instance yet, or you’re looking in the wrong namespace/name). This is whattokio::net::windows::named_pipe::ClientOptions::open()reports asErrorKind::NotFound. [2] -
ERROR_PIPE_BUSY(231): the pipe does exist, but no instance is currently available (commonly: all instances are connected, or the server created instances but isn’t currently waiting/accepting a connection). Microsoft’s guidance is: ifCreateFilefails withERROR_PIPE_BUSY, callWaitNamedPipe(or sleep) and retry. [1] Tokios docs describe the same:ERROR_PIPE_BUSYmeans “server available, but busy”; retry later. [2]
So if the pipe truly “does not exist” you should get FILE_NOT_FOUND, not PIPE_BUSY. If you’re seeing PIPE_BUSY when you expect “doesn’t exist”, usually one of these is true:
- A server process did create that pipe name (maybe a stale/other instance), but it’s saturated/busy. [1]
- Your client/server names differ subtly (exact string matters:
\\.\pipe\...). [3] - Session/namespace mismatch (e.g., service vs user session), making you think “server isn’t there” when it is (or vice‑versa). [3]
Typical Tokio retry loop (mirrors the docs):
use tokio::net::windows::named_pipe::ClientOptions;
use tokio::time::{sleep, Duration};
use windows_sys::Win32::Foundation::ERROR_PIPE_BUSY;
let name = r"\\.\pipe\my-pipe";
let client = loop {
match ClientOptions::new().open(name) {
Ok(c) => break c,
Err(e) if e.raw_os_error() == Some(ERROR_PIPE_BUSY as i32) => {
sleep(Duration::from_millis(50)).await;
continue;
}
Err(e) => return Err(e), // includes NotFound (no server)
}
};Rule of thumb: treat NotFound as “server not up / wrong name”, and PIPE_BUSY as “server up, but no free instance yet—retry”. [1][2]
Sources: Microsoft “Named Pipe Client” (CreateFile / ERROR_PIPE_BUSY / WaitNamedPipe) [1], Tokio named-pipe client docs for ClientOptions::open error meanings [2], troubleshooting notes on name/timing/session causes [3].
🏁 Script executed:
# Search for daemon startup logic and pipe creation timing
rg "pipe_name|CreateNamedPipe|ClientOptions" src/daemon.rs -B 3 -A 3
# Check if there's any retry or startup sequencing logic
rg "retry|sleep|startup|initialize" src/daemon.rs -i
# Find where the daemon creates the named pipe
fd "*.rs" src | xargs rg "CreateNamedPipe\|NamedPipeServer" -lRepository: spacedriveapp/spacebot
Length of output: 2872
Treat FILE_NOT_FOUND from named pipe as transient startup state, not stale.
On Windows, ClientOptions::open() returns ErrorKind::NotFound when the pipe doesn't exist yet (the server hasn't created it), distinct from ERROR_PIPE_BUSY which means the pipe exists but is saturated. A race window exists between write_pid_file() and ServerOptions::create() in start_daemon(): if is_running() is called during that gap, the pipe check fails with NotFound, triggering cleanup and deleting the PID file. This allows a second daemon to start.
Currently, only ERROR_PIPE_BUSY is treated as "daemon alive"; all other errors (including NotFound) trigger cleanup_stale_files(), which is too aggressive. Consider either skipping the pipe check if NotFound occurs, or retrying briefly (similar to the 5-second deadline loop in send_command()).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/daemon.rs` around lines 134 - 157, The is_running function treats all
errors from ClientOptions::open as stale and calls cleanup_stale_files, which
causes the PID to be removed when the named pipe is simply not created yet;
update is_running (and its Windows ClientOptions::open handling) to treat
ErrorKind::NotFound (or equivalent "pipe does not exist") as a transient startup
state: either skip the pipe check and return Some(pid) (or retry the open
briefly, e.g., a short loop with a small deadline similar to send_command)
instead of calling cleanup_stale_files, while preserving the existing behavior
for ERROR_PIPE_BUSY and only cleaning up on genuine non-transient errors.
1 participant
Solving Issue #199
Summary
Adding Windows support for Spacebot daemon mode while preserving the existing daemon abstraction and CLI UX.
It includes two parts:
Why
cargo build --release on Windows was failing because the Unix-only daemonize crate (and related Unix APIs) were being compiled on Windows.
After fixing the build, Windows still lacked production-grade daemon behavior (background start, IPC control path, liveness checks). This PR fills that gap using Windows-native primitives while keeping the current architecture intact.
What Changed
1) Unix-only daemon dependency gating
2) src/daemon.rs kept as the cross-platform boundary
3) Windows background daemon startup (detached re-exec)
4) Windows IPC via named pipes (production path)
5) Windows process liveness checks
6) src/main.rs daemon child path
7) Windows filesystem storage stats parity
8) Docs
Architecture / Design Notes
Validation
Known Limitations / Follow-ups
on Windows in this session.