Conversation
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughAdd a new public async helper to WorkerHooks: wait_or_shutdown(secs: u64, shutdown: &mut watch::Receiver<()>), which awaits either a timeout via Tokio sleep or a shutdown signal via shutdown.changed(), returning when either completes. No existing code paths are altered. Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant WH as WorkerHooks::wait_or_shutdown
participant T as tokio::time::sleep
participant S as shutdown watch::Receiver
Caller->>WH: wait_or_shutdown(secs, &mut shutdown)
activate WH
WH->>T: sleep(secs)
WH->>S: changed()
alt timeout
T-->>WH: sleep completes
WH-->>Caller: return (timeout elapsed)
else shutdown
S-->>WH: change observed
WH-->>Caller: return (shutdown signalled)
end
deactivate WH
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
⚙️ CodeRabbit configuration file
Files:
🔍 Remote MCP(no further information available through external documentation sources). Context Summary for wait_or_shutdown PR ReviewBased on the provided pull request context, I can identify several key areas of improvement and concerns that would be valuable for reviewing this PR effectively: Function Implementation Analysis: pub async fn wait_or_shutdown(secs: u64, shutdown: &mut watch::Receiver<()>)This function uses Key Reviewer Concerns Identified: 1. Select Handling Priority Issue
2. API Design Improvement
3. Testing Gap
4. CI/Test Failure
Bot Integration: Recommendation Priority:
🔇 Additional comments (1)
✨ Finishing touches
🧪 Generate unit tests
Comment |
Reviewer's GuideAdded comprehensive Rust documentation to the Class diagram for WorkerHooks with documented wait_or_shutdownclassDiagram
class WorkerHooks {
+wait_or_shutdown(secs: u64, shutdown: &mut watch::Receiver<()>)
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/comenqd/src/worker.rs (3)
122-127: Prioritise shutdown when both events are ready; bias the select and reorder arms.Ensure immediate responsiveness to shutdown by preferring the shutdown branch when both futures are ready in the same tick.
Apply:
- tokio::select! { - _ = tokio::time::sleep(Duration::from_secs(secs)) => {}, - _ = shutdown.changed() => {}, - } + tokio::select! { + biased; + _ = shutdown.changed() => {}, + _ = tokio::time::sleep(Duration::from_secs(secs)) => {}, + }
122-127: Take Duration instead of seconds to improve call-site clarity and prevent unit mistakes.Accept
Durationdirectly and remove implicit unit conversion. Update call sites accordingly.Apply in this block:
- pub async fn wait_or_shutdown(secs: u64, shutdown: &mut watch::Receiver<()>) { + pub async fn wait_or_shutdown(duration: Duration, shutdown: &mut watch::Receiver<()>) { tokio::select! { - biased; - _ = shutdown.changed() => {}, - _ = tokio::time::sleep(Duration::from_secs(secs)) => {}, + biased; + _ = shutdown.changed() => {}, + _ = tokio::time::sleep(duration) => {}, } }Also update call sites (outside this hunk):
-WorkerHooks::wait_or_shutdown(config.cooldown_period_seconds, shutdown).await; +WorkerHooks::wait_or_shutdown(Duration::from_secs(config.cooldown_period_seconds), shutdown).await;
122-127: Add focused tests for timeout and shutdown paths using rstest.Exercise both branches and the zero-duration edge to meet the testing guideline.
Add a new async test module:
#[cfg(test)] mod tests { use super::*; use rstest::rstest; use tokio::sync::watch; #[tokio::test] async fn returns_immediately_on_zero_secs() { let (_tx, mut rx) = watch::channel(()); WorkerHooks::wait_or_shutdown(Duration::from_secs(0), &mut rx).await; } #[tokio::test] async fn returns_on_shutdown_before_timeout() { let (tx, mut rx) = watch::channel(()); let mut rx2 = tx.subscribe(); let fut = WorkerHooks::wait_or_shutdown(Duration::from_secs(60), &mut rx2); tx.send(()).expect("notify"); fut.await; } }Do you want me to open a follow-up PR with these tests?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/comenqd/src/worker.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 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/worker.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/worker.rs
⏰ 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: build-test
🔇 Additional comments (1)
crates/comenqd/src/worker.rs (1)
122-122: Make wait_or_shutdown crate-visible if it is not intended for external callers.File: crates/comenqd/src/worker.rs
Run to verify external usages:
# From repository root (tracked files) git grep -n --no-color 'wait_or_shutdown' | rg -v '^crates/comenqd/' || true # Fallback (search all files then exclude the crate) rg -n --hidden --no-ignore -S '\bwait_or_shutdown\b' -C2 | rg -v '^crates/comenqd/' || trueApply:
- pub async fn wait_or_shutdown(secs: u64, shutdown: &mut watch::Receiver<()>) { + pub(crate) async fn wait_or_shutdown(secs: u64, shutdown: &mut watch::Receiver<()>) {
|
@coderabbitai please create new GitHub issues for the following: crates/comenqd/src/worker.rs (3) |
|
@coderabbitai please create a new GitHub issue for the following: Compiling cucumber v0.20.2 error: could not compile |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll create three GitHub issues for the improvement suggestions you've outlined. Let me create them now: @leynos I've created three separate GitHub issues for your improvement suggestions:
Each issue includes detailed descriptions, the proposed solutions with code examples, file locations, and references back to this PR and comment. All issues have been assigned to you for future implementation. 🏁 Script executed: Length of output: 177 @leynos Perfect! I've successfully created three GitHub issues for your improvement suggestions:
Each issue contains detailed descriptions, the specific problems they address, proposed solutions with code examples, file locations, and references back to this PR. All issues have been assigned to you for future implementation. |
|
Tip For best results, initiate chat on the files or code changes. crates/comenqd/tests/util.rs:16:14 ImpactThis is causing compilation failures in the test suite and preventing CI from completing successfully. Suggested SolutionEither:
Context
|
|
@coderabbitai please create a new GitHub issue for the following compilation error: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos I've created a GitHub issue to track the compilation error that's blocking your CI pipeline. The issue documents the unfulfilled lint expectation in This is a critical blocker that needs to be resolved before the PR can be merged successfully. 🏁 Script executed: Length of output: 42 @leynos Perfect! I've successfully created Issue #91: Fix unfulfilled lint expectation in crates/comenqd/tests/util.rs The issue documents the CI compilation failure and includes:
This should help track and resolve the critical blocker preventing your PR from passing CI tests. |
1 participant
Summary
wait_or_shutdownwith parameters and a simple exampleTesting
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_68c1e4cd07788322bb69b6895799390d
Summary by Sourcery
Documentation: