Conversation
Reviewer's GuideThis PR implements a UDS-based daemon listener by wrapping the queue sender in a Tokio Mutex, spawning a dedicated task per client connection, refactoring the listener and client handlers accordingly, and bolstering coverage with both unit and end-to-end tests as well as updated docs and dependencies. Sequence diagram for UDS listener handling multiple clientssequenceDiagram
participant Client1
participant Client2
participant Listener
participant TokioTask1 as ClientTask1
participant TokioTask2 as ClientTask2
participant MutexSender
Client1->>Listener: Connect via UDS
Listener->>TokioTask1: Spawn task for Client1
Client2->>Listener: Connect via UDS
Listener->>TokioTask2: Spawn task for Client2
TokioTask1->>MutexSender: lock().send(request1)
TokioTask2->>MutexSender: lock().send(request2)
MutexSender-->>TokioTask1: Ack
MutexSender-->>TokioTask2: Ack
TokioTask1-->>Listener: Task completes
TokioTask2-->>Listener: Task completes
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughUpdate the daemon's listener implementation to wrap the queue sender in a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UnixListener
participant ListenerTask
participant MutexSender
participant Queue
Client->>UnixListener: Connects via Unix socket
UnixListener->>ListenerTask: Spawns new handler task
ListenerTask->>MutexSender: Acquires lock, sends request
MutexSender->>Queue: Enqueue request
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- Instead of wrapping the sender in a Mutex for single-writer semantics, consider using a multi‐producer channel or other concurrency primitive that avoids locking on every enqueue.
- Add a graceful shutdown signal to run_listener so you can cleanly terminate the accept loop instead of relying on task::abort.
- Introduce a small backoff or delay after an accept error to prevent tight looping if listener.accept() continuously fails.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of wrapping the sender in a Mutex for single-writer semantics, consider using a multi‐producer channel or other concurrency primitive that avoids locking on every enqueue.
- Add a graceful shutdown signal to run_listener so you can cleanly terminate the accept loop instead of relying on task::abort.
- Introduce a small backoff or delay after an accept error to prevent tight looping if listener.accept() continuously fails.
## Individual Comments
### Comment 1
<location> `crates/comenqd/src/daemon.rs:119` </location>
<code_context>
mod tests {
use super::*;
use tempfile::tempdir;
</code_context>
<issue_to_address>
The module does not begin with a `//!` comment as required by the review instructions.
Please add a `//!` module-level doc comment at the top of this module to describe its purpose, as required by the review instructions.
</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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/comenqd/src/daemon.rs (1)
150-150: Past review comment addressed.The module-level doc comment has been added as requested in the previous review.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
crates/comenqd/src/daemon.rs(5 hunks)docs/comenq-design.md(1 hunks)tests/steps/listener_steps.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/comenq-design.md
**/*.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.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.
Files:
tests/steps/listener_steps.rscrates/comenqd/src/daemon.rs
🧬 Code Graph Analysis (1)
tests/steps/listener_steps.rs (2)
crates/comenqd/src/daemon.rs (2)
queue_writer(41-51)run_listener(84-113)crates/comenq/src/client.rs (1)
serde_json(109-109)
🔇 Additional comments (13)
docs/comenq-design.md (1)
1019-1022: Documentation accurately reflects the implementation changes.The updated description of the queue writer task and MPSC channel forwarding mechanism is technically accurate and well-explained. The en-GB spelling is correct throughout.
crates/comenqd/src/daemon.rs (7)
41-51: Well-designed queue writer implementation.The queue writer function properly serialises access to the yaque sender whilst maintaining error resilience by logging failures and continuing to process subsequent messages.
54-82: Concurrent architecture properly implemented.The refactored run function correctly establishes the MPSC channel communication pattern and ensures proper task lifecycle management. The shutdown mechanism ensures graceful cleanup of the queue writer task.
84-113: Excellent concurrent listener implementation.The listener properly handles concurrent client connections whilst remaining responsive to shutdown signals. The retry delay on accept failures prevents resource exhaustion during error conditions.
115-123: Client handling with proper validation.The deserialisation and re-serialisation of CommentRequest provides early validation of client input before enqueueing. The error handling for dropped queue writer is appropriate.
195-206: Comprehensive test for socket preparation.The test properly verifies both stale file removal and permission setting, covering critical security-related functionality.
208-234: Excellent integration test for client enqueueing.The test comprehensively verifies the complete flow from client handling through queue writer to storage. The use of UnixStream::pair() effectively tests the handling logic without filesystem dependencies.
236-281: Comprehensive end-to-end listener test.The test thoroughly validates the complete listener implementation including socket creation, client handling, and message enqueueing with proper cleanup and shutdown.
tests/steps/listener_steps.rs (5)
39-73: Comprehensive test setup implementation.The function properly initialises all required infrastructure components and includes socket readiness polling to ensure reliable test execution.
75-90: Valid client simulation properly implemented.The step correctly creates and sends a valid JSON request over the Unix socket.
92-100: Appropriate negative test case.The step properly tests invalid JSON handling by sending malformed data to the listener.
102-108: Proper verification of request enqueueing.The step correctly verifies that valid requests are enqueued and maintains data integrity through the process.
110-129: Excellent negative verification and cleanup implementation.The timeout-based verification cleverly proves that invalid requests are rejected. The Drop implementation ensures proper cleanup of all spawned tasks and prevents resource leaks.
Replace allow with expect attributes and remove unused field.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/steps/listener_steps.rs(1 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 usingArcto 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 unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not usereturnin 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.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums. Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary. Useeyre::Reportfor 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 toeyreonly in the mainmain()entrypoint or top-level async task.
Files:
tests/steps/listener_steps.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.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.
Files:
tests/steps/listener_steps.rs
🔇 Additional comments (7)
tests/steps/listener_steps.rs (7)
1-4: Well-structured module documentation and lint configuration.The module documentation clearly explains the purpose, and the lint expectations are properly configured with appropriate reasons for test code.
5-19: Clean and well-organised imports.The imports are logically grouped and all appear necessary for the test implementation.
20-28: Well-designed test world struct.The ListenerWorld struct properly manages test state with appropriate type safety and resource handles for comprehensive test lifecycle management.
30-34: Appropriate Debug implementation for test world.The minimal Debug implementation avoids exposing potentially sensitive test state while maintaining debuggability.
72-87: Correct client simulation for valid request scenario.The function properly creates and sends a valid JSON request over the Unix socket with appropriate async I/O handling.
89-112: Correct test verification logic for both valid and invalid scenarios.The functions properly test both positive and negative cases, using appropriate timeout patterns to verify that invalid requests are rejected.
114-126: Proper resource cleanup in Drop implementation.The Drop implementation correctly handles cleanup of all spawned tasks and communication channels, preventing resource leaks in tests.
1 participant
Summary
run_listenerwith per-client tasks and queue enqueuetokio::sync::MutexTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_688621ceff28832290bbe00dcf3de623
Summary by Sourcery
Implement a Unix domain socket listener for the daemon that spawns concurrent handlers to enqueue JSON requests into the queue, add necessary synchronization, and cover the new functionality with unit and behavioural tests while updating documentation and dependencies.
New Features:
Enhancements:
Build:
Documentation:
Tests: