Add Windows support and CI #7
Conversation
- Replace Unix domain sockets with interprocess local sockets (Unix sockets on Unix, named pipes on Windows) - Add process.rs module for cross-platform process management - Move nix crate to Unix-only, add windows-sys for Windows - Update force_stop() to use platform-specific termination - Document Windows limitation: no graceful shutdown (immediate kill) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add 9 tests for ErrorContextBuffer (circular eviction, cloning, timestamps) - Add 4 process termination tests (SIGTERM→SIGKILL escalation, Windows) - Add 8 integration tests (panic recovery, large output, stale cleanup, stress) - Add 2 version tests (mismatch detection, protocol behavior) - Add Debug derive to TerminateResult for better test output Total test count increased from ~30 to 57 tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Runs on Linux, macOS, and Windows. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughAdds cross-platform IPC and process management, refactors client lifecycle to use them (startup, force-stop, restart), expands tests and examples, bumps package to 0.6.0 with new platform deps, adds CI + git hooks, and ignores Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Transport as SocketClient/SocketConnection
participant Daemon as SocketServer
participant Process as process.rs
Note over Client,Transport: Client issues connect or command
Client->>Transport: connect(daemon_name, root_path)
alt socket present & usable
Transport->>Daemon: accept connection
Client->>Daemon: VersionHandshake / Command
Daemon-->>Client: OutputChunk / CommandComplete
else socket missing or stale
Client->>Process: process_exists(pid)?
Process-->>Client: true / false
alt stale pid/socket detected
Client->>Process: terminate_process(pid, timeout_ms)
Process-->>Client: Terminated / AlreadyDead / PermissionDenied / Error
Client->>Transport: retry connect or spawn daemon
end
end
Note over Process,Daemon: kill_process used for force-stop or escalation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/client.rs (2)
499-500: Fix collapsible-if clippy warning.The nested
ifstatements can be collapsed into a single condition.Apply this diff:
- if let Err(ref error) = result { - if self.auto_restart_on_error && Self::is_fatal_connection_error(error) { + if let Err(ref error) = result + && self.auto_restart_on_error + && Self::is_fatal_connection_error(error) + {
544-548: Useinspect_errinstead ofmap_err.Since the closure returns the error unchanged,
inspect_erris more idiomatic.Apply this diff:
- .map_err(|e| { + .inspect_err(|_| { self.error_context.dump_to_stderr(); - e })?;
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
14-21: fmt job: Consider running on all platforms for consistency.While code formatting is platform-independent and running only on
ubuntu-latestis a reasonable optimization, some projects benefit from running fmt checks across all platforms to catch any platform-specific formatting inconsistencies or locale-related issues early. The current approach is acceptable; this is optional.tests/version_tests.rs (1)
347-397: Consider consolidating withtest_version_mismatch_detection.This test (
test_version_mismatch_triggers_client_action) is nearly identical totest_version_mismatch_detection(lines 75-125). The only differences are the daemon/client timestamps and slightly different assertion messages. Consider whether both tests provide unique value or if one could be removed to reduce redundancy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ci.yml(1 hunks).gitignore(1 hunks)Cargo.toml(1 hunks)src/client.rs(6 hunks)src/error_context.rs(1 hunks)src/lib.rs(1 hunks)src/process.rs(1 hunks)src/transport.rs(4 hunks)tests/integration_tests.rs(1 hunks)tests/version_tests.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/version_tests.rs (3)
src/server.rs (1)
new_with_name_and_timestamp(140-162)src/client.rs (1)
connect(59-71)src/transport.rs (1)
connect(149-166)
tests/integration_tests.rs (3)
src/lib.rs (1)
handle(272-278)src/client.rs (1)
connect_with_name_and_timestamp(81-192)src/transport.rs (3)
socket_path(43-47)socket_path(138-140)pid_path(50-54)
src/client.rs (2)
src/process.rs (6)
kill_process(41-43)kill_process(112-115)kill_process(176-182)process_exists(20-22)process_exists(52-59)process_exists(128-139)src/transport.rs (3)
daemon_socket_exists(76-87)socket_path(43-47)socket_path(138-140)
🪛 GitHub Actions: CI
tests/version_tests.rs
[error] 1-1: cargo fmt --check failed. Run 'cargo fmt' to format the code.
src/process.rs
[error] 1-1: cargo fmt --check failed. Run 'cargo fmt' to format the code.
tests/integration_tests.rs
[error] 1-1: cargo fmt --check failed. Run 'cargo fmt' to format the code.
src/client.rs
[error] 499-499: collapsible-if: this if statement can be collapsed. Clippy note: collapse nested if block. Command 'cargo clippy --all-targets --all-features -- -D warnings' failed with exit code 101.
[error] 545-545: manual-inspect: using map_err over inspect_err. Clippy suggest: use '.inspect_err' or adjust accordingly. Command 'cargo clippy --all-targets --all-features -- -D warnings' failed with exit code 101.
src/transport.rs
[error] 1-1: cargo fmt --check failed. Run 'cargo fmt' to format the code.
🔇 Additional comments (33)
.github/workflows/ci.yml (3)
1-47: Solid CI workflow structure for cross-platform Rust project.The workflow is well-organized with proper job dependency ordering, platform-specific matrix testing, and caching for efficiency. The three-job structure (fmt → clippy → test) enforces code quality gates before running tests, which is a best practice. Cross-platform testing on Linux, macOS, and Windows aligns well with the PR objective of adding Windows support.
9-11: RUSTFLAGS configuration treats warnings as errors—good practice.Using
-D warningsensures the codebase remains warning-free across all platforms. However, verify that this doesn't cause issues with generated code or platform-specific dependencies that might emit warnings outside your control.
33-33: Cache configuration: Verify cache key consistency across platforms.Both clippy and test jobs use
Swatinem/rust-cache@v2. Ensure that the cache keys are correctly differentiated by platform (os matrix) so that platform-specific artifacts don't interfere with one another. The action should handle this automatically, but it's worth confirming in your first run.Also applies to: 45-45
.gitignore (1)
1-1: LGTM!Adding
/.cacheto.gitignoreis appropriate for ignoring local development/runtime cache directories.src/error_context.rs (1)
159-278: Good test coverage forErrorContextBuffer.The tests comprehensively cover buffer initialization, entry management, circular eviction behavior, cloning semantics, and timestamp ordering. The test cases align well with the implementation details.
src/transport.rs (2)
68-87: Good cross-platform socket existence check.The platform-aware approach is well thought out—checking the socket file on Unix and using the PID file as a proxy on Windows (since named pipes aren't filesystem entities). This provides a consistent API while respecting platform differences.
148-166: Clean cross-platform client connection implementation.The platform-specific connection logic is well-structured using conditional compilation, keeping the public API consistent across platforms.
tests/integration_tests.rs (10)
776-798: LGTM!The
ImmediateOutputHandlercorrectly tests the race condition scenario where the handler completes before output is fully read. The chunked write pattern is appropriate for this test case.
831-857: LGTM!The
LargeOutputHandleris well-designed for testing large output streaming with realistic delays. The 8KB chunk size is reasonable for buffer testing.
889-905: LGTM!The
PanicHandlercorrectly tests panic handling in the daemon. The test verifies server resilience after a handler panic.
800-829: LGTM!This test correctly validates the critical fix for handler output/task completion race conditions. Using 64KB of output is appropriate for testing buffering behavior.
859-887: LGTM!Testing 1MB output streaming validates the daemon's ability to handle large data transfers reliably.
907-956: LGTM!Excellent test coverage for panic handling. The test correctly verifies both error reporting and server resilience after a handler panic.
958-999: LGTM!This test validates stale socket and PID file cleanup, which is essential for reliable daemon operation after crashes or unclean shutdowns.
1001-1071: LGTM!Good stress test for connection handling. The 80% success threshold (40/50) is reasonable to account for timing-related variations while still ensuring server stability.
1073-1169: LGTM!This test thoroughly validates the connection limit and immediate rejection behavior. The inline
SlowHandlerdefinition keeps the test self-contained and readable.
1175-1202: LGTM!Excellent security test verifying Unix socket permissions are set to 0600 (owner read/write only). This ensures proper access control for the daemon socket.
src/lib.rs (1)
108-108: LGTM!Adding the
processmodule integrates the platform-aware process management functionality needed for Windows support.src/client.rs (5)
2-4: LGTM!Good refactoring to use the new cross-platform abstractions from
processandtransportmodules.
105-109: LGTM!Using
daemon_socket_existsprovides proper cross-platform socket existence checking (file-based on Unix, PID file-based on Windows).
296-296: LGTM!Using
kill_processfrom the process module ensures cross-platform process termination.
304-332: LGTM!The documentation clearly explains the platform-specific behavior differences between Unix (graceful shutdown with SIGTERM/SIGKILL) and Windows (immediate termination).
353-395: LGTM!The force_stop implementation correctly uses the cross-platform
process_existsandterminate_processfunctions. TheTerminateResulthandling covers all variants with appropriate error messages.src/process.rs (10)
6-17: LGTM!The
TerminateResultenum comprehensively covers all process termination outcomes, enabling proper error handling in the client.
19-43: LGTM!Clean public API with clear documentation about platform-specific behavior. The async functions appropriately use async operations for waiting.
52-59: LGTM!The Unix
process_existsimplementation correctly useskill(pid, None)to check process existence. The EPERM handling (line 56) correctly returnstruesince the process exists even if we can't signal it.
61-110: LGTM!The Unix
terminate_processimplementation correctly implements the SIGTERM → wait → SIGKILL escalation pattern. The error handling covers all relevantErrnocases.
128-139: LGTM!The Windows
process_existscorrectly usesOpenProcesswithPROCESS_QUERY_LIMITED_INFORMATIONto check existence without requiring full process access.
141-174: LGTM!The Windows
terminate_processcorrectly handles the lack of graceful shutdown (no SIGTERM equivalent). The permission check fallback (lines 150-157) properly distinguishes between non-existent and permission-denied cases.
185-218: LGTM!Good basic unit tests for process existence and termination of non-existent processes. Using PID 4_000_000_000 safely tests non-existent process handling.
219-260: LGTM!Excellent Unix-specific test with proper documentation about zombie process behavior. The test correctly reaps the child process to verify cleanup.
262-308: LGTM!Good test for SIGTERM → SIGKILL escalation by using a process that traps SIGTERM. The zombie handling is properly documented.
310-345: LGTM!Appropriate Windows-specific test using the
timeoutcommand. The simpler assertion (onlyTerminated) reflects Windows' immediate termination behavior.
| #[cfg(unix)] | ||
| let listener = { | ||
| // Remove existing socket file if it exists | ||
| if sock_path.exists() { | ||
| fs::remove_file(&sock_path)?; | ||
| } | ||
| ListenerOptions::new() | ||
| .name(sock_path.clone().to_fs_name::<GenericFilePath>()?) | ||
| .create_tokio()? | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how the caller coordinates daemon cleanup before creating a new server
rg -nA10 'SocketServer::new' --type rustRepository: human-solutions/daemon-cli
Length of output: 796
🏁 Script executed:
#!/bin/bash
# Look at broader context before SocketServer::new call
rg -nB20 'SocketServer::new' --type rust src/server.rs | head -60
echo "---"
# Check if there's any daemon coordination before server creation
rg -n 'pid_path\|PID\|daemon.*check\|already.*running' --type rust src/server.rs | head -20Repository: human-solutions/daemon-cli
Length of output: 1217
🏁 Script executed:
#!/bin/bash
# Look at the socket cleanup code in transport.rs (lines 100-109)
sed -n '90,120p' src/transport.rs
echo "---"
# Look at the full SocketServer::new function
rg -n 'impl.*SocketServer\|fn new' --type rust src/transport.rs | head -10Repository: human-solutions/daemon-cli
Length of output: 1073
🏁 Script executed:
#!/bin/bash
# Look at the broader server.rs file to see if there's daemon coordination before run()
rg -n 'fn run\|async fn run\|daemon.*exist\|pid.*check\|already.*running' --type rust src/server.rs
echo "---"
# Check the entry point - how is the server started
rg -n 'main\|pub async fn\|impl.*Server' --type rust src/server.rs | head -20
echo "---"
# Look for any PID file checking logic
rg -n 'pid_path\|read.*pid\|PID' --type rust src/ --type rustRepository: human-solutions/daemon-cli
Length of output: 77
🏁 Script executed:
#!/bin/bash
# Find main.rs or lib.rs
find . -type f -name "main.rs" -o -name "lib.rs" | head -5
echo "---"
# Search for any daemon start/stop/status commands
rg -n 'start\|stop\|status' --type rust | grep -i daemon | head -20
echo "---"
# Look for the full context of how SocketServer::new is called
sed -n '150,210p' src/server.rsRepository: human-solutions/daemon-cli
Length of output: 2592
🏁 Script executed:
#!/bin/bash
# Look at lib.rs
cat -n src/lib.rs
echo "---"
# Check the transport module for pid_path function
rg -nA5 'fn pid_path' --type rust src/transport.rsRepository: human-solutions/daemon-cli
Length of output: 11329
🏁 Script executed:
#!/bin/bash
# Look at the client.rs to see how client connects
rg -n 'DaemonClient\|connect\|daemon.*exist\|check.*pid' --type rust src/client.rs | head -30
echo "---"
# Look for any version checking or daemon restart logic
rg -n 'mtime\|version\|restart\|existing' --type rust src/client.rs | head -20Repository: human-solutions/daemon-cli
Length of output: 73
🏁 Script executed:
#!/bin/bash
# Read client.rs file
wc -l src/client.rs
echo "---"
# Read the file
cat -n src/client.rsRepository: human-solutions/daemon-cli
Length of output: 26945
🏁 Script executed:
#!/bin/bash
# Check if there's any protection against concurrent daemon spawning
rg -n 'process_exists\|kill_process' --type rust src/process.rs | head -20
echo "---"
# Look for any locking mechanisms
rg -n 'Mutex\|Lock\|RwLock\|atomic' --type rust src/server.rs src/transport.rs | head -20
echo "---"
# Check if socket creation itself has any atomicity/locking
sed -n '95,130p' src/transport.rsRepository: human-solutions/daemon-cli
Length of output: 1269
🏁 Script executed:
#!/bin/bash
# Check if ListenerOptions::create_tokio has any built-in protection
rg -n 'create_tokio\|ListenerOptions' --type rust src/transport.rs
echo "---"
# Check if there's any documentation about daemon exclusivity
grep -r "single.*daemon\|one.*daemon\|exclusive\|concurrent.*daemon" --include="*.rs" --include="*.md" src/ . 2>/dev/null | head -10Repository: human-solutions/daemon-cli
Length of output: 890
Based on my comprehensive verification, I can now provide the rewritten review comment.
The socket cleanup race condition is confirmed—add daemon state validation before socket removal.
The code removes an existing socket file without checking whether a daemon holding it is actually running. If two daemon processes attempt to start concurrently (e.g., multiple clients simultaneously detect no running daemon), both will call SocketServer::new(), creating a TOCTOU (time-of-check-time-of-use) race:
- Process A checks socket exists, removes it
- Process B checks socket exists (may already be removed), removes it (no-op)
- Process A creates listener
- Process B creates listener (fails or causes socket state issues)
Recommended fix: Before removing the socket at line 104, verify that the daemon process recorded in the PID file is not actually running. The PID file is already being written after successful socket creation (lines 193-198 in src/server.rs), so check for and validate it before socket cleanup. Alternatively, handle socket creation failures gracefully or add a lock file to prevent concurrent daemon initialization.
🤖 Prompt for AI Agents
In src/transport.rs around lines 100 to 109, the code unconditionally removes an
existing socket file which creates a TOCTOU race when multiple daemons start;
before calling fs::remove_file(&sock_path) verify the daemon PID from the PID
file (as written in src/server.rs lines ~193-198) and only remove the socket if
that PID is absent or the process is not running; if the PID indicates a live
daemon, abort startup with a clear error; if the PID file is missing or stale,
remove the socket and proceed, but also catch and handle listener creation
failures (retry or surface a clear error) or implement a startup lock file to
prevent concurrent initializations.
- Fix collapsible if statement in client.rs - Use inspect_err instead of map_err in client.rs - Use assert! instead of assert_eq! with bool literal - Remove redundant trim() before split_whitespace() - Use .first() instead of .get(0) - Fix redundant guards in match statements - Apply cargo fmt fixes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/version_tests.rs (1)
467-472: Test is incomplete — second command is never sent.The test name
test_multiple_commands_same_connectionsuggests testing multiple commands, but only one command is sent. Either complete the test by sending a second command and verifying the expected behavior (connection closed after first command), or rename the test totest_single_command_after_handshake.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
examples/common/mod.rs(1 hunks)examples/concurrent.rs(1 hunks)src/client.rs(8 hunks)src/process.rs(1 hunks)src/tests.rs(2 hunks)src/transport.rs(5 hunks)tests/integration_tests.rs(3 hunks)tests/version_tests.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/tests.rs
🧰 Additional context used
🧬 Code graph analysis (3)
tests/version_tests.rs (3)
src/server.rs (1)
new_with_name_and_timestamp(140-162)src/client.rs (1)
connect(59-71)src/transport.rs (1)
connect(149-166)
src/client.rs (2)
src/process.rs (6)
process_exists(20-22)process_exists(52-59)process_exists(127-136)terminate_process(32-34)terminate_process(61-110)terminate_process(138-169)src/transport.rs (3)
daemon_socket_exists(76-87)socket_path(43-47)socket_path(138-140)
tests/integration_tests.rs (3)
src/lib.rs (1)
handle(272-278)src/client.rs (1)
connect_with_name_and_timestamp(81-192)src/transport.rs (3)
socket_path(43-47)socket_path(138-140)pid_path(50-54)
🪛 GitHub Actions: CI
src/process.rs
[error] 3-3: unused import: bail
[error] 4-4: unused imports: Duration and sleep
src/transport.rs
[error] 15-15: unused import: fs
🔇 Additional comments (19)
examples/common/mod.rs (1)
33-35: LGTM!The changes are idiomatic improvements:
split_whitespace()already skips leading/trailing whitespace, making.trim()redundant.parts.first()is more idiomatic thanparts.get(0).examples/concurrent.rs (1)
86-88: LGTM!Consistent with the refactoring in
examples/common/mod.rs— both improvements are idiomatic.src/process.rs (3)
45-116: Unix implementation looks correct.The SIGTERM → poll → SIGKILL escalation is well-structured. The handling of
EPERM(process exists but owned by another user) andESRCH(process doesn't exist) is correct.
118-178: Windows implementation is correct.Handle management is proper with
CloseHandlecalled in all code paths. The 5-second wait for process exit afterTerminateProcessis reasonable.
180-316: Tests are comprehensive with good platform coverage.The zombie process handling documentation (lines 233-247) is helpful for understanding expected behavior. The test assertions correctly account for the zombie edge case in Unix.
src/client.rs (4)
2-4: Imports are well-organized for cross-platform support.The new imports from
processandtransportmodules properly abstract platform-specific logic.
105-109: Good use of platform-aware socket existence check.Using
daemon_socket_existsinstead of direct file existence check properly handles the Windows case where named pipes can't be checked via filesystem.
333-395: Clean cross-platform implementation offorce_stop.The refactoring properly uses the platform-abstracted
process_existsandterminate_processAPIs. Error handling withTerminateResultis clear and provides informative error messages.
499-520: Let-if chain syntax is fully supported in this project.The project uses Rust 2024 edition (as confirmed in Cargo.toml), which natively supports let-if chains. The auto-restart logic on lines 499-520 is correctly structured and requires no changes. The condition check is clean and combines error detection, auto-restart enablement, and fatal error classification appropriately. The single retry after restart prevents infinite loops.
src/transport.rs (4)
68-87: Platform-aware socket existence checking is well-designed.The approach of using PID file as a proxy on Windows (since named pipes can't be checked via filesystem) is appropriate and documented.
96-131: Socket server creation with platform-specific listeners looks good.The socket cleanup on Unix (lines 103-105) is coordinated by the client's
cleanup_stale_processeswhich checks the PID file first, mitigating the TOCTOU concern raised in past reviews.
148-166: Platform-specific connection handling is correct.The client connection logic properly mirrors the server's platform-specific naming conventions.
216-229: LGTM!Formatting changes to enum variants are cosmetic and improve readability.
tests/version_tests.rs (1)
347-400: LGTM!This test properly validates that the server reports its own timestamp on version mismatch, allowing the client to detect and handle the mismatch.
tests/integration_tests.rs (5)
45-68: LGTM!The
start_test_daemon_with_limithelper properly exposes the connection limit parameter for testing connection limiting behavior.
774-905: Well-designed test handlers for edge cases.
ImmediateOutputHandlertargets the output buffering race conditionLargeOutputHandlervalidates streaming of significant data volumesPanicHandlerverifies server resilience after handler panics
958-999: Good coverage for stale file cleanup.The test properly simulates stale socket and PID files with a non-existent PID, then verifies the daemon cleans them up and starts successfully.
1001-1071: Good stress test for connection stability.The 40/50 success threshold (80%) is reasonable for stress testing, and the final connection verification ensures server stability after stress.
1170-1197: Good security test for Unix socket permissions.Verifying 0600 permissions ensures the socket is only accessible by the owner, which is important for daemon security.
Add #[cfg(unix)] to imports only used on Unix: - bail, Duration, sleep in process.rs - fs in transport.rs 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/transport.rs (1)
102-111: Note: Socket cleanup race condition previously flagged.The socket removal at lines 105-106 without PID validation has already been flagged in previous review comments. See the existing comment for detailed analysis and recommended fixes.
🧹 Nitpick comments (1)
src/process.rs (1)
164-164: Consider using graceful_timeout_ms for wait duration.The hardcoded 5000ms wait is inconsistent with the function's
graceful_timeout_msparameter. While Windows cannot perform graceful shutdown (as documented), the wait duration could still honor the caller's timeout preference for API consistency.-let wait = unsafe { WaitForSingleObject(handle, 5000) }; +let wait = unsafe { WaitForSingleObject(handle, graceful_timeout_ms as u32) };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/process.rs(1 hunks)src/transport.rs(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (macos-latest)
- GitHub Check: test (windows-latest)
- GitHub Check: test (ubuntu-latest)
🔇 Additional comments (10)
src/process.rs (2)
3-7: LGTM! Platform-specific imports correctly scoped.The unused import warnings flagged in previous reviews have been resolved by properly scoping
bail,Duration, andsleepwith#[cfg(unix)]attributes.
183-355: LGTM! Comprehensive test coverage with well-documented platform quirks.The tests appropriately cover:
- Existence checks for current and nonexistent processes
- Termination of nonexistent processes
- Actual process termination on both Unix and Windows
- SIGTERM→SIGKILL escalation on Unix
The zombie process behavior in Unix tests (lines 236-241, 295-296) is correctly identified as a testing artifact, not a production issue, since daemon processes are reaped by init/systemd.
src/transport.rs (8)
4-11: LGTM: Platform-specific imports are correctly structured.The conditional imports using
#[cfg(unix)]and#[cfg(windows)]properly separate platform-specific types from the interprocess crate.
13-14: LGTM: Thefsimport is now Unix-only.This change addresses the previous comment about unused imports on Windows. The
fsmodule is now only imported on Unix where it's actually used (lines 106, 126).
70-75: LGTM: Windows socket name helper is correctly structured.The
allow(dead_code)attribute on Unix is appropriate since this function is only needed for Windows named pipe naming.
122-127: LGTM: Unix socket permissions correctly restrict access.Setting permissions to 0600 ensures only the owner can read/write to the socket, which is appropriate for daemon IPC.
153-163: LGTM: Client connection logic is correctly implemented for both platforms.The platform-specific connection paths appropriately mirror the server-side listener creation, using filesystem paths on Unix and named pipe names on Windows.
189-193: LGTM: Generic Stream abstraction enables clean cross-platform support.Using the generic
Streamtype from the interprocess crate provides appropriate platform abstraction while maintaining the same framing semantics withLengthDelimitedCodec.
220-230: LGTM: Message enum formatting is consistent.The formatting changes improve readability of the struct variants without altering semantics.
77-89: PID file coordination is well-implemented; no issues found.The code shows proper sequencing:
SocketServer::new()is called first (line 191 of src/server.rs), then the PID file is written immediately after (line 196), with cleanup handled via ascopeguardthat removes the PID file on exit (lines 201-204). This minimizes the race window between socket creation and PID file write. The Windows approach using the PID file as a proxy is pragmatic and the synchronization is sound.
Use ceiling division so any positive timeout performs at least one iteration before escalating to SIGKILL. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
OpenProcess returns null on failure, not INVALID_HANDLE_VALUE. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Apply SDDL security descriptor D:P(A;;GA;;;OW) to restrict pipe access to the owner only, equivalent to Unix 0o600 socket permissions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Use GetExitCodeProcess to verify process is still running, not just that OpenProcess succeeds. Fixes test_terminate_spawned_process_windows which was failing because OpenProcess can succeed on terminated processes during kernel cleanup. Also fix clippy warning by using div_ceil(). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/transport.rs (2)
1-1: Pipeline still fails on cargo fmt—run locally to fix import formatting.Despite being marked as addressed in a previous commit, the CI pipeline continues to fail with
cargo fmt --check. The error indicates cfg(windows) imports need reordering. Runcargo fmtlocally and commit the formatted result.#!/bin/bash # Verify the exact formatting issue cargo fmt --check 2>&1 || echo "Formatting check failed - run 'cargo fmt' to fix"
110-112: TOCTOU race condition remains unresolved—validate daemon state before socket removal.The unconditional socket file removal still creates a race when multiple processes attempt to start the daemon simultaneously. This issue was flagged in a previous review but has not been addressed.
🧹 Nitpick comments (3)
src/process.rs (3)
9-20: Consider derivingPartialEqfor easier test assertions.The enum is well-designed. Deriving
PartialEqwould allow usingassert_eq!directly instead ofmatches!in tests, though this is optional.-#[derive(Debug)] +#[derive(Debug, PartialEq)] pub enum TerminateResult {
55-56: Potential overflow when castingpid: u32toi32.On Linux, PIDs are typically limited to values well under
i32::MAX(default max is ~32768, configurable up to ~4 million), so this cast is safe in practice. However, an explicit check ortry_into()with error handling would be more defensive.pub fn process_exists(pid: u32) -> Result<bool> { + let raw_pid: i32 = pid.try_into().map_err(|_| anyhow::anyhow!("PID {} exceeds i32::MAX", pid))?; - match kill(Pid::from_raw(pid as i32), None) { + match kill(Pid::from_raw(raw_pid), None) {
174-182: Consider includingGetLastErrordetails in error messages.When
WaitForSingleObjectfails or times out, the error message could be more informative by including the wait result or last error code.// Wait for process to exit (up to 5 seconds) let wait = unsafe { WaitForSingleObject(handle, 5000) }; unsafe { CloseHandle(handle) }; if wait == WAIT_OBJECT_0 { TerminateResult::Terminated } else { - TerminateResult::Error("Process did not terminate".to_string()) + TerminateResult::Error(format!("Process did not terminate (wait result: {})", wait)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Cargo.toml(1 hunks)src/process.rs(1 hunks)src/transport.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (1)
src/transport.rs (2)
src/server.rs (2)
new(115-119)e(435-435)src/client.rs (1)
connect(59-71)
🪛 GitHub Actions: CI
src/process.rs
[error] 147-147: mismatched types: expected u32, found i32 in expression Ok(exit_code == STILL_ACTIVE)
[error] 147-147: cannot compare u32 with i32; PartialEq<i32> is not implemented for u32
src/transport.rs
[error] 1-1: cargo fmt --check failed. Code is not properly formatted in transport.rs. Run 'cargo fmt' (or 'cargo fmt --check' after formatting) to fix formatting issues. The diff shows reordering of cfg(windows) imports.
🔇 Additional comments (11)
src/process.rs (7)
1-7: LGTM!The conditional compilation attributes correctly scope Unix-only imports (
bail,Duration,sleep) to avoid unused import warnings on Windows. This addresses the previous review feedback.
22-46: LGTM!The public API is clean with good documentation explaining platform-specific behavior differences. The delegation to platform modules keeps the abstraction clean.
84-94: Good fix for the edge case with small timeouts.Using
div_ceil(100)ensures at least 1 iteration for any positive timeout value, addressing the previous review feedback about timeouts below 100ms.
152-152: Unused parameter_graceful_timeout_msis appropriately documented.The documentation at lines 33-34 correctly explains that Windows has no SIGTERM equivalent, making the graceful timeout parameter unused. The underscore prefix correctly suppresses the unused warning.
194-227: LGTM!The basic tests cover key scenarios: current process existence, nonexistent process handling, and graceful degradation for kill operations. Using a very high PID (4 billion) is a reasonable approach for testing nonexistent process handling.
228-329: Well-documented zombie process handling in tests.The comments explaining why the test accepts
TerminateResult::Error(_)due to zombie processes are valuable. This correctly documents expected behavior when terminating child processes directly (vs. detached daemons reaped by init).
331-365: LGTM!The Windows test correctly expects
TerminateResult::Terminatedsince Windows doesn't have the zombie process issue that Unix has with direct child processes.src/transport.rs (4)
124-127: Excellent—Windows security descriptor properly restricts access to owner only.The implementation now creates a security descriptor using SDDL
"D:P(A;;GA;;;OW)"(DACL Protected, Allow Generic All to Owner), which correctly restricts the named pipe to the daemon owner, matching the Unix socket behavior (0o600). This addresses the critical security concern raised in the previous review.
82-94: Good approach using PID file as proxy for Windows named pipe existence.Since Windows named pipes cannot be checked via the filesystem like Unix sockets, using the PID file as a proxy for daemon existence is a reasonable cross-platform solution.
165-177: LGTM—platform-specific connection logic is clean and correct.The conditional compilation cleanly separates Unix domain socket connection from Windows named pipe connection, using the appropriate naming functions for each platform.
120-120: Widestring dependency is properly declared.The
widestringcrate is correctly specified in Cargo.toml (line 31:widestring = "1.0"), so the import ofu16cstron line 120 is valid and the security descriptor implementation is properly supported.
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Actually send a second command and verify the connection is closed, confirming the server's one-shot semantics. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- pre-commit: runs cargo fmt --check and clippy - pre-push: runs cargo nextest 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.cargo-husky/hooks/pre-push (1)
1-4: Verify cargo-nextest installation is documented for contributors.The pre-push hook assumes
cargo-nextestis installed. If a contributor doesn't have it, their push will fail with an unclear error. Consider adding a check or documenting the requirement in your contributing guidelines.Optionally, add user feedback to clarify what's running:
#!/bin/sh set -e +echo "Running tests with cargo nextest..." cargo nextest run.cargo-husky/hooks/pre-commit (1)
1-5: Strict pre-commit checks enforce code quality.The hook correctly runs formatting checks and linting before commits. The
--checkflag oncargo fmtwill fail commits with formatting issues (not auto-fix), and-D warningstreats all clippy warnings as errors, maintaining high code quality standards.Optionally, add user feedback to clarify what's running:
#!/bin/sh set -e +echo "Checking code formatting..." cargo fmt -- --check +echo "Running clippy..." cargo clippy -- -D warnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.cargo-husky/hooks/pre-commit(1 hunks).cargo-husky/hooks/pre-push(1 hunks)CHANGELOG.md(1 hunks)Cargo.toml(2 hunks)tests/version_tests.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/version_tests.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (windows-latest)
- GitHub Check: test (macos-latest)
- GitHub Check: test (ubuntu-latest)
🔇 Additional comments (5)
Cargo.toml (5)
3-3: Appropriate version bump for new Windows support.The minor version increment from 0.5.0 to 0.6.0 correctly reflects the addition of new platform support and features.
27-31: Windows dependencies updated as recommended.The
windows-sysversion has been updated to 0.61.2 as suggested in the previous review, addressing the outdated version concern. The platform-specific dependency scoping is correct.
24-24: The nix crate version 0.30.1 is valid and poses no known security concerns.Verification confirms that version 0.30.1 of the
nixcrate exists on crates.io (published May 4, 2025), is not yanked, and has no known security advisories. The version is the latest in the 0.30.x series, and the crate repository is actively maintained.
40-40: Verified: cargo-husky version 1.x is available with the "user-hooks" feature.The latest 1.x version is 1.5.0 (stable, not yanked), which explicitly supports the
user-hooksfeature. The configuration correctly disables default features to selectively enable onlyuser-hooks, making it appropriate for custom git hooks in a dev-dependency.
21-21: Dependency verified:interprocessv2.2 is secure and valid.The
interprocesscrate version 2.2 exists (resolves to 2.2.3, released March 2025) with no known security advisories in the RustSec database. Thetokiofeature is properly defined in the crate and compatible with the specified version. No issues identified.
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
Examples
✏️ Tip: You can customize this high-level summary in your review settings.