Atomically bind Unix socket to prevent race#102
Conversation
Reviewer's Guideprepare_listener now atomically binds the Unix socket by creating a uniquely named temporary socket file, setting its permissions, and renaming it into place, and introduces a concurrent-race test to ensure no attacker file creation can hijack the socket path. Sequence diagram for atomic Unix socket binding in prepare_listenersequenceDiagram
participant Caller
participant "prepare_listener()"
participant "stdfs"
participant "UnixListener"
Caller->>"prepare_listener()": call with socket path
"prepare_listener()"->>"stdfs": get parent directory
"prepare_listener()"->>"prepare_listener()": generate unique temp socket path
"prepare_listener()"->>"UnixListener": bind to temp socket
"prepare_listener()"->>"stdfs": set permissions on temp socket
"prepare_listener()"->>"stdfs": rename temp socket to target path
"prepare_listener()"->>Caller: return listener
Class diagram for updated prepare_listener and test moduleclassDiagram
class prepare_listener {
+prepare_listener(path: &Path) Result<UnixListener>
-atomic temp-path creation
-bind to temp socket
-set permissions
-rename into place
}
class tests {
+prepare_listener_prevents_pre_bind_race()
-spawns attacker thread
-calls prepare_listener
-verifies socket file type
}
prepare_listener <.. tests: uses
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughRewrite prepare_listener to bind to a UUID-named temporary Unix socket in the target directory, set permissions to 0o660, atomically rename the temp socket into place, remove pre-bind unlink logic, add contextual error handling and a unit test that races destination creation and verifies the final path is a socket. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Proc as Process
participant OS as OS/Unix
participant FS as Filesystem
Note over Proc,OS: prepare_listener(path)
Proc->>Proc: derive parent dir and temp name (orig + UUID)
Proc->>OS: bind UnixListener at temp path
OS-->>Proc: listener(temp)
Proc->>OS: chmod(temp, 0o660)
OS-->>Proc: ok
Proc->>FS: rename(temp -> final) [atomic]
alt rename ok
FS-->>Proc: ok
Note right of Proc: listener reachable at final path
Proc-->>Proc: return listener
else rename fails
FS-->>Proc: error
Proc->>FS: unlink temp (best-effort)
Proc-->>Proc: propagate error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `crates/comenqd/src/listener.rs:49` </location>
<code_context>
+ let tmp = parent.join(unique);
+ let listener = UnixListener::bind(&tmp)?;
+ stdfs::set_permissions(&tmp, stdfs::Permissions::from_mode(0o660))?;
+ stdfs::rename(&tmp, path).inspect_err(|_| {
+ let _ = stdfs::remove_file(&tmp);
+ })?;
Ok(listener)
</code_context>
<issue_to_address>
Error handling during rename could be more robust.
Consider handling or logging errors from remove_file to avoid leaving orphaned socket files if rename fails.
Suggested implementation:
```rust
stdfs::rename(&tmp, path).inspect_err(|_| {
if let Err(e) = stdfs::remove_file(&tmp) {
log::error!("Failed to remove orphaned socket file {}: {}", tmp.display(), e);
}
})?;
```
If the `log` crate is not already included in your dependencies, add it to your `Cargo.toml`:
```
log = "0.4"
```
Also, ensure that logging is initialized in your application (e.g., with `env_logger` or similar) so that error messages are actually output.
</issue_to_address>
### Comment 2
<location> `crates/comenqd/src/listener.rs:48` </location>
<code_context>
+ );
+ let tmp = parent.join(unique);
+ let listener = UnixListener::bind(&tmp)?;
+ stdfs::set_permissions(&tmp, stdfs::Permissions::from_mode(0o660))?;
+ stdfs::rename(&tmp, path).inspect_err(|_| {
+ let _ = stdfs::remove_file(&tmp);
</code_context>
<issue_to_address>
Setting permissions before renaming may not guarantee final socket permissions.
Set permissions on the final socket path after renaming to ensure the correct mode is applied.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
let listener = UnixListener::bind(&tmp)?;
stdfs::set_permissions(&tmp, stdfs::Permissions::from_mode(0o660))?;
stdfs::rename(&tmp, path).inspect_err(|_| {
let _ = stdfs::remove_file(&tmp);
})?;
Ok(listener)
=======
let listener = UnixListener::bind(&tmp)?;
stdfs::rename(&tmp, path).inspect_err(|_| {
let _ = stdfs::remove_file(&tmp);
})?;
stdfs::set_permissions(&path, stdfs::Permissions::from_mode(0o660))?;
Ok(listener)
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `crates/comenqd/src/listener.rs:38` </location>
<code_context>
+ let parent = path
+ .parent()
+ .ok_or_else(|| anyhow::anyhow!("socket path missing parent"))?;
+ let unique = format!(
+ ".{}.{}.{}",
+ path.file_name().unwrap().to_string_lossy(),
</code_context>
<issue_to_address>
Consider using a UUID for the temporary socket name and simplifying error handling to reduce boilerplate and improve clarity.
Here’s a much shorter version that keeps atomic replace, drops all of the PID/timestamp boilerplate, and still cleans up on error. It uses `uuid` for a randomized suffix instead of rolling your own:
```rust
use uuid::Uuid;
use anyhow::Context;
use std::{fs, os::unix::fs::PermissionsExt};
pub fn prepare_listener(path: &Path) -> Result<UnixListener> {
let parent = path
.parent()
.with_context(|| "socket path missing parent")?;
// generate a random “.filename.UUID” temp name
let tmp = parent.join(format!(
".{}.{}",
path.file_name().unwrap().to_string_lossy(),
Uuid::new_v4()
));
// bind, set perms, then rename into place
let listener = UnixListener::bind(&tmp)
.with_context(|| format!("binding to temp socket {:?}", tmp))?;
fs::set_permissions(&tmp, fs::Permissions::from_mode(0o660))
.with_context(|| format!("setting perms on {:?}", tmp))?;
if let Err(e) = fs::rename(&tmp, path) {
let _ = fs::remove_file(&tmp);
return Err(e).context("renaming socket into place");
}
Ok(listener)
}
```
What this does:
1. Drops manual PID/timestamp formatting—UUIDs give you unique names with zero effort.
2. Collapses the `inspect_err` into a simple `if let Err(...)` so it’s obvious we clean up on failure.
3. Adds only a few lines of error‐context to keep debugging just as straightforward.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/comenqd/src/listener.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider using Arc to reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not use return in single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Use rstest fixtures for shared setup.
Replace duplicated tests with #[rstest(...)] parameterised cases.
Prefer mockall for mocks/stubs.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary. Use eyre::Report for human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library. Convert to domain enums at API boundaries, and to eyre only in the main main() entrypoint or top-level async task.
Files:
crates/comenqd/src/listener.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
crates/comenqd/src/listener.rs
🪛 GitHub Check: build-test
crates/comenqd/src/listener.rs
[failure] 175-175:
no method named is_socket found for struct FileType in the current scope
🪛 GitHub Actions: CI
crates/comenqd/src/listener.rs
[error] 175-175: Rust compile error (E0599): no method named is_socket found for struct FileType in the current scope. Location crates/comenqd/src/listener.rs:175. Command: cargo llvm-cov --workspace --summary-only --lcov --output-path lcov.info
🔍 Remote MCP Ref
Summary — additional repo/PR facts relevant for review
-
Change summary (implementation): prepare_listener now binds a UnixListener to a uniquely-named temporary socket in the same parent directory, sets permissions to 0o660, then atomically renames the temporary socket into the final path; on rename failure the temporary socket is removed and the error is returned. Signature unchanged.
-
Rationale: this replaces the prior remove-then-bind sequence to eliminate the window where another process could create or hijack the final socket path.
-
Tests added: a unit test spawns concurrent attempts to create the destination while prepare_listener runs and asserts the final path is a socket (intended to verify the absence of hijack). Review the test for determinism/OS-specific assumptions.
-
Important reviewer notes:
- Temp file is created inside the socket's parent directory (avoids cross-filesystem rename issues in normal cases). Rename semantics will atomically replace an existing destination; permission or other rename errors are cleaned up and propagated by the function.
- Consider verifying behavior on target platforms/CI (permissions, selinux/AppArmor, or other filesystem edge-cases) and whether tests run reliably under heavy concurrency.
Sources consulted
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (2)
crates/comenqd/src/listener.rs (2)
22-23: Docs: Correctly state the atomic rename approach.Retain this; it matches the implementation and closes the remove→bind race.
47-52: Atomic rename + cleanup path looks good.Binding at a tmp path, fixing permissions, then atomically renaming into place is the right shape. Cleanup on rename error is adequate; RAII will close the unlinked socket on early return.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/comenqd/src/listener.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider using Arc to reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not use return in single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Use rstest fixtures for shared setup.
Replace duplicated tests with #[rstest(...)] parameterised cases.
Prefer mockall for mocks/stubs.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary. Use eyre::Report for human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library. Convert to domain enums at API boundaries, and to eyre only in the main main() entrypoint or top-level async task.
Files:
crates/comenqd/src/listener.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
crates/comenqd/src/listener.rs
🔍 Remote MCP Ref
Summary of additional review-relevant facts (concise)
-
Change is focused on atomic replacement when creating the Unix domain socket: create & bind to a temporary socket file in the destination directory (using a UUID-based name), chmod it to 0o660, then atomically rename it into place; old remove-then-bind flow was removed. This eliminates the window where an attacker/process could create a file at the socket path.
-
A multithreaded test was added that repeatedly races concurrent creation at the destination path against prepare_listener and asserts the final path is a socket. Check test robustness for CI flakiness (timeouts, retries, platform differences).
-
Dependency changes:
- Workspace Cargo.toml: adds uuid = { version = "1", features = ["v4"] }.
- crates/comenqd/Cargo.toml: adds uuid = { workspace = true, features = ["v4"] }.
Confirm workspace policy and lockfile effects; ensure CI updates lockfile and that adding uuid is acceptable.
-
Review risk/verification checklist (concrete items to verify in review):
- Temp file placement: ensure temp socket is created in the same parent directory as the final path (so rename is atomic on the same filesystem). Verify corner cases (path is root, relative paths, symlinked parent directories).
- Error/cleanup paths: verify all failure branches attempt cleanup of the temp socket and that concurrent unlink/rename races don't leave stale files. Confirm failures surface helpful context (with_context used).
- Permissions & security: confirm chmod to 0o660 is applied after rename and that this behavior is acceptable under target OS policies (SELinux/AppArmor, ACLs). Consider whether umask or directory permissions could affect final socket permissions.
- UUID uniqueness and predictability: using uuid v4 is sufficient to avoid predictable collisions; confirm no information leak from temp name in logs.
- Tests: run on CI OS matrix (Linux variants, macOS if supported) to ensure rename semantics and socket behavior are consistent. Consider making the test less timing-sensitive if flakes are observed.
Citations of sources/tools used
- PR page: #102
- Search for related code/PR context (attempted)
🔇 Additional comments (4)
crates/comenqd/src/listener.rs (4)
7-7: Imports: good additionsImport
ContextandUuidto improve error messages and generate unique temp names. LGTM.Also applies to: 17-17
36-44: Avoid panics on path parts: LGTMGuard
parent()andfile_name()withoutunwrap(). This matches the repo’s error‑handling guidance.
153-187: Race test: sound and targetedImporting
FileTypeExtand usingsymlink_metadataavoids false positives; yielding in the attacker loop reduces CPU burn. LGTM.
45-47: Call out gotcha: local_addr() may report the temp pathAfter the rename, getsockname/local_addr() can still return the original temp pathname on some Unix platforms; verify no code depends on local_addr().as_pathname() equalling config.socket_path.
Run this locally to search for risky usages:
rg -n -C2 --hidden -g '!.git' -g '**/*.rs' -e 'local_addr\s*\(' -e '\.as_pathname\b' || git grep -n -E -e 'local_addr\s*\(' -e '\.as_pathname\b' -- '*.rs' || trueIf matches exist, update comparisons to use canonicalised paths (std::fs::canonicalize), compare inode/dev, or validate against the final socket path after rename.
|
@coderabbitai Please suggest a fix for the issue causing the following lint violation and supply a prompt for an AI coding agent to enable it to apply the fix: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI need to examine the current code structure to understand what constants are available and fix the import issue. @leynos The compilation error occurs because the test is trying to import constants Looking at the code, these constants are defined as private ( FixExport these constants by making them public in // Change from:
const CLIENT_READ_TIMEOUT_SECS: u64 = 5;
const MAX_REQUEST_BYTES: usize = 1024;
// To:
pub const CLIENT_READ_TIMEOUT_SECS: u64 = 5;
pub const MAX_REQUEST_BYTES: usize = 1024;AI Agent Prompt🏁 Script executed: Length of output: 7566 🏁 Script executed: Length of output: 2821 🏁 Script executed: Length of output: 26 🏁 Script executed: Length of output: 299 🏁 Script executed: Length of output: 73 🏁 Script executed: Length of output: 69 Now I understand the issue. The constants and function exist, but the import path in the test is incorrect. @leynos The compilation error is due to an incorrect import path in the test file. The listener functionality was moved during the refactoring, and the test is trying to import from the old path. AnalysisLooking at the code structure:
FixChange the import statement in use comenqd::daemon::listener::{CLIENT_READ_TIMEOUT_SECS, MAX_REQUEST_BYTES, handle_client};To: use comenqd::listener::{CLIENT_READ_TIMEOUT_SECS, MAX_REQUEST_BYTES, handle_client};AI Agent Prompt🏁 Script executed: Length of output: 1221 🏁 Script executed: Length of output: 620 Perfect! Now I can see the issue clearly. The @leynos The compilation error occurs because the AnalysisLooking at
But the constants FixAdd the missing constants to the re-exports in pub mod listener {
pub use crate::listener::{handle_client, prepare_listener, run_listener};
}To: pub mod listener {
pub use crate::listener::{
handle_client, prepare_listener, run_listener,
CLIENT_READ_TIMEOUT_SECS, MAX_REQUEST_BYTES
};
}AI Agent Prompt |
Prevent a brief window of overly permissive mode by setting the temporary socket's permissions before renaming into place. Adds error context for renaming and permission adjustments and reinforces final mode after the rename.
leynos
force-pushed
the
codex/fix-race-condition-in-unix-socket-handling
branch
from
cbb330b to
d6ef3ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.toml(1 hunks)crates/comenqd/Cargo.toml(1 hunks)crates/comenqd/src/daemon.rs(1 hunks)crates/comenqd/src/listener.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: Use explicit version ranges in Cargo.toml and keep dependencies up-to-date.
Mandate caret requirements for all dependencies. All crate versions specified in Cargo.toml must use SemVer-compatible caret requirements (e.g., some-crate = "1.2.3").
Prohibit unstable version specifiers. The use of wildcard (*) or open-ended inequality (>=) version requirements are strictly forbidden, as are tilde requirements (~) except for documented reasons.
Files:
Cargo.toml
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider using Arc to reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not use return in single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Use rstest fixtures for shared setup.
Replace duplicated tests with #[rstest(...)] parameterised cases.
Prefer mockall for mocks/stubs.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary. Use eyre::Report for human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library. Convert to domain enums at API boundaries, and to eyre only in the main main() entrypoint or top-level async task.
Files:
crates/comenqd/src/listener.rscrates/comenqd/src/daemon.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
crates/comenqd/src/listener.rscrates/comenqd/src/daemon.rs
🧬 Code graph analysis (1)
crates/comenqd/src/daemon.rs (1)
crates/comenqd/src/listener.rs (3)
handle_client(141-160)prepare_listener(37-68)run_listener(81-127)
🔍 Remote MCP Ref
Summary of additional relevant facts for reviewing PR #102 (atomic Unix-socket bind)
- Implementation summary (from PR context):
- prepare_listener now creates a temp socket in the same parent directory (temp name uses UUID v4), binds to it, sets temp perms to 0o660, atomically renames the temp into the final path, re-applies 0o660 on the final path, and returns the listener. It removes the temp file on rename failure. Tests added that race concurrent creators against prepare_listener. (source: PR context)
- Tests & CI:
- New multithreaded test may be timing-sensitive; verify it runs reliably on CI matrix (Linux/macOS) and isn’t flaky. Ensure Cargo.lock/workspace changes (uuid v1 with v4 feature) are included so CI builds succeed. (source: PR context)
- Export/import CI break:
- Tests failed because daemon::listener currently did not re-export CLIENT_READ_TIMEOUT_SECS and MAX_REQUEST_BYTES. Recommended fix: add those constants to the pub use list in crates/comenqd/src/daemon.rs so tests importing comenqd::daemon::listener::{...} compile. (source: PR context)
- Security/behavioral checks to perform in review:
- Confirm temp socket is created in the target parent directory and that parent resolution handles symlinks, relative paths, and permission errors robustly.
- Confirm cleanup paths always attempt to unlink the temp socket on failures (including rename and chmod failures).
- Validate that setting 0o660 after rename is appropriate across intended deployment environments (consider umask/ACLs).
- Confirm UUID dependency is acceptable for workspace and that Cargo.toml/Cargo.lock updates are included in PR.
- Ensure error contexts (with_context) provide actionable messages for bind/rename/chmod failures.
- Actionable edits to unblock CI:
- Add missing re-exports in crates/comenqd/src/daemon.rs:
pub use crate::listener::{ handle_client, prepare_listener, run_listener, CLIENT_READ_TIMEOUT_SECS, MAX_REQUEST_BYTES }; - Or update tests to import constants directly from comenqd::listener instead of comenqd::daemon::listener.
- Add missing re-exports in crates/comenqd/src/daemon.rs:
Tools queried (search attempts encountered errors; no external docs retrieved):
- Ref.ref_search_documentation — search attempts returned errors (402).
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (5)
Cargo.toml (1)
61-61: Add workspaceuuid— OKAdopt the workspace‑scoped
uuidwith v4; version constraint complies with caret semantics. No further changes needed here.crates/comenqd/src/listener.rs (3)
23-26: Document atomicity and permission timing — OKThe doc now correctly states same‑dir rename atomicity and the pre/post‑rename permission steps. Ship it.
38-67: Close the unlink‑bind race with atomic rename — LGTM
- Bind to a temp socket under the same parent.
- Set
0o660before visibility; enforce again after rename.- Clean up the orphan on rename failure with contextual errors.
This removes the hijack window without introducing new hazards.
162-196: Stabilise the race test — cannot verify in sandbox; run the check locallySandbox lacks /proc (error: "no /proc/self/exe available"); run the loop below locally or on CI to confirm the test is stable and report failures.
#!/bin/bash set -e for i in $(seq 1 50); do echo "Run $i" cargo test -p comenqd prepare_listener_prevents_pre_bind_race -- --quiet \ || { echo "Test failed on run $i"; exit 1; } done echo "All good"Report failures.
crates/comenqd/src/daemon.rs (1)
31-36: Re‑export constants to fix test imports — OKKeep the re-exports of CLIENT_READ_TIMEOUT_SECS and MAX_REQUEST_BYTES under daemon::listener — tests import them at crates/comenqd/tests/listener.rs:12. Cargo check failed in the sandbox (no /proc); run the commands below locally to confirm the build:
rg -nP --type=rust -C2 '\buse\s+comenqd::daemon::listener::\{[^}]*CLIENT_READ_TIMEOUT_SECS[^}]*MAX_REQUEST_BYTES' crates/comenqd/tests || true cargo check -p comenqdReport build failures.
| figment = { version = "0.10", default-features = false, features = ["env", "toml"] } | ||
| test-support = { workspace = true, optional = true } | ||
| backon = { workspace = true } | ||
| uuid = { workspace = true, features = ["v4"] } |
There was a problem hiding this comment.
🧹 Nitpick
Avoid per‑crate feature duplication; rely on workspace features
Let the workspace drive features to prevent drift. Drop the local features = ["v4"] here.
Apply this diff:
-uuid = { workspace = true, features = ["v4"] }
+uuid = { workspace = true }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uuid = { workspace = true, features = ["v4"] } | |
| uuid = { workspace = true } |
🤖 Prompt for AI Agents
In crates/comenqd/Cargo.toml around line 24, the crate is overriding
workspace-managed features by specifying `features = ["v4"]` for the uuid
dependency; remove the `features = ["v4"]` fragment so the dependency is
declared as just `uuid = { workspace = true }` and let the workspace Cargo.toml
control which uuid features are enabled.
| let parent = path.parent().context("socket path missing parent")?; | ||
| let file_name = path | ||
| .file_name() | ||
| .ok_or_else(|| anyhow::anyhow!("socket path missing file name"))?; | ||
| let tmp = parent.join(format!( | ||
| ".{}.{}", | ||
| file_name.to_string_lossy(), | ||
| Uuid::new_v4() | ||
| )); | ||
| let listener = UnixListener::bind(&tmp) | ||
| .with_context(|| format!("binding to temp socket {}", tmp.display()))?; | ||
| // Ensure correct permissions before the temp socket becomes visible at the final path. | ||
| stdfs::set_permissions(&tmp, stdfs::Permissions::from_mode(0o660)) | ||
| .with_context(|| format!("setting permissions on {}", tmp.display()))?; | ||
|
|
||
| stdfs::rename(&tmp, path) | ||
| .inspect_err(|_| { | ||
| if let Err(e) = stdfs::remove_file(&tmp) { | ||
| tracing::error!( | ||
| "failed to remove orphaned socket file {}: {}", | ||
| tmp.display(), | ||
| e | ||
| ); | ||
| } | ||
| }) | ||
| .with_context(|| format!("renaming socket {} -> {}", tmp.display(), path.display()))?; | ||
| // Belt-and-braces: enforce final permissions in case of ACL quirkiness. | ||
| stdfs::set_permissions(path, stdfs::Permissions::from_mode(0o660)) | ||
| .with_context(|| format!("setting permissions on {}", path.display()))?; | ||
| Ok(listener) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unexport opaque errors from a library API
Stop exposing anyhow::Result from public functions in a library. Define a typed error (derive thiserror::Error) and return Result<_, ListenerError>. Map IO/context errors accordingly. Keep eyre/opaque errors at the app boundary only.
Example sketch (apply outside this hunk):
#[derive(thiserror::Error, Debug)]
pub enum ListenerError {
#[error("binding temp socket {0}: {1}")]
Bind(std::path::PathBuf, #[source] std::io::Error),
#[error("setting permissions on {0}: {1}")]
Chmod(std::path::PathBuf, #[source] std::io::Error),
#[error("renaming socket {0} -> {1}: {2}")]
Rename(std::path::PathBuf, std::path::PathBuf, #[source] std::io::Error),
#[error("missing parent for socket path")]
MissingParent,
#[error("socket path missing file name")]
MissingFileName,
}Then change signatures to return Result<UnixListener, ListenerError> and replace with_context/bail/anyhow! with typed variants.
🤖 Prompt for AI Agents
In crates/comenqd/src/listener.rs around lines 38 to 67, the function currently
returns anyhow::Result and uses anyhow/with_context which exposes opaque errors;
replace this with a typed ListenerError (derive thiserror::Error) and change the
function signature to Result<UnixListener, ListenerError>. Add variants for
MissingParent and MissingFileName (no source) and Bind/Chmod/Rename carrying
PathBuf(s) and #[source] std::io::Error. Replace context/bail/anyhow! calls by
mapping errors into those variants (e.g.
parent().ok_or(ListenerError::MissingParent), file_name ok_or
ListenerError::MissingFileName, and map Errs from bind/set_permissions/rename
into Bind/Chmod/Rename using map_err or .map_err(|e|
ListenerError::Bind(tmp.clone(), e))). Ensure all path values are cloned/moved
as needed so the error variants hold the relevant PathBufs.
🧹 Nitpick
Remove stale socket on shutdown to reduce operational noise
Unlink the final socket path during orderly shutdown after dropping the listener. This avoids littering /run//tmp with stale entries and keeps service restarts clean.
Add at the end of run_listener (outside this hunk):
// after breaking the accept loop
drop(listener);
let _ = std::fs::remove_file(&config.socket_path);🤖 Prompt for AI Agents
In crates/comenqd/src/listener.rs around lines 38 to 67, the temp socket is
created and renamed but the final socket file is not unlinked on orderly
shutdown; modify run_listener (outside this hunk, after breaking the accept
loop) to explicitly drop the UnixListener and then attempt to remove the final
socket path (use std::fs::remove_file(&config.socket_path)), ignoring or logging
any error; ensure the drop(listener) happens before remove_file so the socket is
closed, and tolerate failures from remove_file (e.g. with let _ =
std::fs::remove_file(...)) so shutdown is best-effort.
1 participant
Summary
Testing
make fmtmake lintmake testcloses #6
https://chatgpt.com/codex/tasks/task_e_68c6b288590c8322903d7378ac95afcf
Summary by Sourcery
Use an atomic temporary socket rename when binding a Unix domain socket to eliminate race conditions and add a test that simulates concurrent file creation to verify the fix.
Bug Fixes:
Enhancements:
Documentation:
Tests: