Add builder for push queues #334
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Summary by CodeRabbit
WalkthroughIntroduce a prioritised push subsystem with high/low async queues, rate limiting and optional dead‑letter queue; add a builder-based construction API returning (PushQueues, PushHandle) with validation and error types; implement PushHandle push/try_push semantics and update tests/docs to use the builder. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test / Producer
participant Builder as PushQueuesBuilder::<F>
participant Creator as build_with_rate_dlq
participant PQ as PushQueues::<F>
participant PH as PushHandle::<F>
Test->>Builder: PushQueues::builder()
Builder-->>Test: PushQueuesBuilder
Test->>Builder: high_capacity(...), low_capacity(...), rate(...), dlq(...)
Test->>Builder: build()
Builder->>Creator: validate capacities & rate, create channels, limiter, DLQ
Creator-->>Builder: Result<(PushQueues, PushHandle), PushConfigError>
Builder-->>Test: Result<(PQ, PH), PushConfigError>
note right of Test: Tests unwrap via expect(...)
sequenceDiagram
autonumber
participant Producer as Producer
participant PH as PushHandle
participant RL as RateLimiter?
participant HQ as High-prio mpsc
participant LQ as Low-prio mpsc
participant PQ as PushQueues
participant Consumer as Connection/Consumer
Producer->>PH: try_push(frame, priority, policy)
alt rate limiter configured
PH->>RL: consume token (async)
end
alt priority == High
PH->>HQ: send(frame)
else priority == Low
PH->>LQ: send(frame)
end
par biased receive
PQ->>HQ: recv() (biased)
PQ->>LQ: recv()
end
PQ-->>Consumer: (priority, frame)
note over PH,PQ: On full: return `QueueFull` or drop to DLQ per `PushPolicy`.<br/>On closed: return `Closed`.
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Reviewer's GuideThis PR introduces a fluent builder pattern for configuring PushQueues, replaces legacy constructors with a shared build implementation, and updates all documentation and tests to use the new API. Class diagram for PushQueuesBuilder and PushQueues changesclassDiagram
class PushQueuesBuilder {
+high_capacity: usize
+low_capacity: usize
+rate: Option<usize>
+dlq: Option<mpsc::Sender<F>>
+new() PushQueuesBuilder
+high_capacity(capacity: usize) PushQueuesBuilder
+low_capacity(capacity: usize) PushQueuesBuilder
+rate(rate: Option<usize>) PushQueuesBuilder
+dlq(dlq: Option<mpsc::Sender<F>>) PushQueuesBuilder
+build() Result<(PushQueues<F>, PushHandle<F>), PushConfigError>
}
class PushQueues {
+builder() PushQueuesBuilder
+build_with_rate_dlq(...)
+bounded(...) [deprecated]
+bounded_no_rate_limit(...) [deprecated]
+bounded_with_rate(...) [deprecated]
+bounded_with_rate_dlq(...) [deprecated]
}
PushQueuesBuilder --> PushQueues : builds
Class diagram for deprecated PushQueues constructorsclassDiagram
class PushQueues {
+bounded(high_capacity: usize, low_capacity: usize) (deprecated)
+bounded_no_rate_limit(high_capacity: usize, low_capacity: usize) (deprecated)
+bounded_with_rate(high_capacity: usize, low_capacity: usize, rate: Option<usize>) (deprecated)
+bounded_with_rate_dlq(high_capacity: usize, low_capacity: usize, rate: Option<usize>, dlq: Option<mpsc::Sender<F>>) (deprecated)
}
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: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
tests/push.rs (3)
31-35: Correct variant name in test docs.Use PushError::QueueFull (not Full).
-/// return `PushError::Full` once the queue is at capacity. +/// return `PushError::QueueFull` once the queue is at capacity.
44-46: Assert the precise error variant.Tighten the assertion to validate policy behaviour.
- assert!(result.is_err()); + assert!(matches!(result, Err(PushError::QueueFull)));
79-113: Deduplicate queue construction with a rstest fixture.Reduce repetition and centralise capacities/rate/DLQ wiring.
Example fixture to add in this module:
#[fixture] fn queues() -> (PushQueues<u8>, wireframe::push::PushHandle<u8>) { PushQueues::builder() .high_capacity(2) .low_capacity(2) .rate(Some(1)) .build() .expect("queue creation failed") }Then replace repeated builders with
let (mut queues, handle) = queues();.Also applies to: 115-161, 181-210
tests/push_policies.rs (1)
15-18: Replace forbidden #[allow] with narrowly scoped #[expect].Fix compilation (reason is not valid in #[allow]) and follow lint policy.
-#[allow( - unused_braces, - reason = "rustc false positive for single line rstest fixtures" -)] +#[expect(unused_braces, reason = "rustc false positive for single line rstest fixtures")]src/push/queues.rs (2)
489-495: Bias towards high priority is explicit.Starvation is intentional; document this nuance in the enum or recv() docs.
/// Receive the next frame, preferring high priority frames when available. /// /// Returns `None` when both queues are closed and empty. /// /// # Examples +/// +/// Note: this method is biased towards high priority traffic and may starve +/// low priority frames if producers saturate the high queue.
26-34: Make the compile‑time guard explicit (optional).Assert the invariant directly for clarity.
-// Compile-time guard: DEFAULT_PUSH_RATE must not exceed MAX_PUSH_RATE. -const _: usize = MAX_PUSH_RATE - DEFAULT_PUSH_RATE; +// Compile-time guard: DEFAULT_PUSH_RATE must not exceed MAX_PUSH_RATE. +const _: () = assert!(DEFAULT_PUSH_RATE <= MAX_PUSH_RATE);Note: require a Rust toolchain where const panics are stable; otherwise keep existing guard.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
src/push/mod.rs(1 hunks)src/push/queues.rs(8 hunks)tests/advanced/concurrency_loom.rs(1 hunks)tests/advanced/interaction_fuzz.rs(1 hunks)tests/async_stream.rs(1 hunks)tests/connection_actor.rs(3 hunks)tests/correlation_id.rs(1 hunks)tests/push.rs(8 hunks)tests/push_policies.rs(3 hunks)tests/session_registry.rs(1 hunks)tests/stream_end.rs(2 hunks)tests/wireframe_protocol.rs(2 hunks)tests/world.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use precise names; boolean names should start with is/has/should
Use en-GB-oxendict spelling and grammar in comments
Function documentation must include clear examples; test documentation should omit redundant examples
Keep code files ≤ 400 lines; split long switch/dispatch logic by feature; move large test data to external files
Disallow Clippy warnings
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions for long functions; adhere to separation of concerns and CQRS
Group related parameters into meaningful structs when functions have many parameters
Consider using Arc for large error returns to reduce data size
Each Rust module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless 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
Do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect(..)] over #[allow(..)] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Files:
src/push/mod.rstests/stream_end.rstests/session_registry.rstests/wireframe_protocol.rstests/advanced/concurrency_loom.rstests/async_stream.rstests/connection_actor.rstests/world.rstests/push.rstests/correlation_id.rstests/push_policies.rssrc/push/queues.rstests/advanced/interaction_fuzz.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:
src/push/mod.rstests/stream_end.rstests/session_registry.rstests/wireframe_protocol.rstests/advanced/concurrency_loom.rstests/async_stream.rstests/connection_actor.rstests/world.rstests/push.rstests/correlation_id.rstests/push_policies.rssrc/push/queues.rstests/advanced/interaction_fuzz.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
src/push/mod.rstests/stream_end.rstests/session_registry.rstests/wireframe_protocol.rstests/advanced/concurrency_loom.rstests/async_stream.rstests/connection_actor.rstests/world.rstests/push.rstests/correlation_id.rstests/push_policies.rssrc/push/queues.rstests/advanced/interaction_fuzz.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/stream_end.rstests/session_registry.rstests/wireframe_protocol.rstests/advanced/concurrency_loom.rstests/async_stream.rstests/connection_actor.rstests/world.rstests/push.rstests/correlation_id.rstests/push_policies.rstests/advanced/interaction_fuzz.rs
🧬 Code graph analysis (12)
src/push/mod.rs (1)
tests/connection_actor.rs (1)
queues(25-31)
tests/stream_end.rs (2)
tests/connection_actor.rs (1)
queues(25-31)src/push/queues.rs (1)
builder(359-359)
tests/session_registry.rs (1)
src/push/queues.rs (1)
builder(359-359)
tests/wireframe_protocol.rs (2)
tests/connection_actor.rs (1)
queues(25-31)src/push/queues.rs (1)
builder(359-359)
tests/advanced/concurrency_loom.rs (2)
tests/connection_actor.rs (1)
queues(25-31)src/push/queues.rs (1)
builder(359-359)
tests/async_stream.rs (2)
tests/connection_actor.rs (1)
queues(25-31)src/push/queues.rs (1)
builder(359-359)
tests/connection_actor.rs (1)
src/push/queues.rs (1)
builder(359-359)
tests/world.rs (2)
tests/connection_actor.rs (1)
queues(25-31)src/push/queues.rs (1)
builder(359-359)
tests/push.rs (2)
tests/connection_actor.rs (1)
queues(25-31)src/push/queues.rs (1)
builder(359-359)
tests/correlation_id.rs (2)
tests/connection_actor.rs (1)
queues(25-31)src/push/queues.rs (1)
builder(359-359)
tests/push_policies.rs (2)
tests/connection_actor.rs (1)
queues(25-31)src/push/queues.rs (1)
builder(359-359)
tests/advanced/interaction_fuzz.rs (2)
tests/connection_actor.rs (1)
queues(25-31)src/push/queues.rs (1)
builder(359-359)
🔍 Remote MCP
Here are the key facts and concrete details to aid your review:
• A new fluent builder API was introduced in src/push/queues.rs:
– Added pub struct PushQueuesBuilder<F> with fields high_capacity, low_capacity, rate, and dlq.
– Chainable methods:
• high_capacity(usize) -> Self
• low_capacity(usize) -> Self
• rate(Option<usize>) -> Self
• dlq(Option<mpsc::Sender<F>>) -> Self
• build(self) -> Result<(PushQueues<F>, PushHandle<F>), PushConfigError> which delegates to the new private helper PushQueues::build_with_rate_dlq(...). (github.com)
• Exposed the builder via
impl<F: FrameLike> PushQueues<F> {
#[must_use]
pub fn builder() -> PushQueuesBuilder<F> { PushQueuesBuilder::new() }
// …
}and consolidated all queue‐construction logic—capacity setup, optional rate limiting (with 1s refill intervals), and optional dead‐letter queue—into build_with_rate_dlq. (github.com)
• The four legacy constructors on PushQueues were deprecated and repointed to the builder:
#[deprecated(since = "0.1.0", note = "Use `PushQueues::builder` instead")]
pub fn bounded(…) → (Self, PushHandle<F>) { … }
#[deprecated(…)] pub fn bounded_no_rate_limit(…) { … }
#[deprecated(…)] pub fn bounded_with_rate(…) { … }
#[deprecated(…)] pub fn bounded_with_rate_dlq(…) { … }All now simply call through to the builder API. (github.com)
• A new module file src/push/mod.rs (5 additions) was added to re-export the queues API:
//! Prioritised queues used for asynchronously pushing frames to a connection.
mod queues;
pub use queues::*;
``` ([github.com](https://github.com/leynos/wireframe/commit/35f4bf4))
• All impacted tests (13 files across `tests/`) were updated to replace direct `PushQueues::bounded*` calls with
```rust
let (queues, handle) = PushQueues::builder()
.high_capacity(…)
.low_capacity(…)
//.rate(Some(n)) // where applicable
//.dlq(Some(tx)) // where applicable
.build()
.unwrap(); // or `.expect("…")`preserving existing test logic but surfacing build‐step errors via Result. (github.com)
• PR metadata:
– Single commit 35f4bf4
– +293/−147 lines over 13 files
– Created Aug 27, 2025 by leynos to merge codex/introduce-pushqueuesbuilder-with-deprecation → main
– Label: codex
– CI status: 4 checks (build/test workflow, CodeQL, pull_request jobs, CodeScene delta analysis)
– Test steps: make fmt, make lint, make test (github.com)
• Linked task for context:
https://chatgpt.com/codex/tasks/task_e_68aeba8d630c8322bcab55de80b2ef06
🔇 Additional comments (6)
tests/push_policies.rs (1)
83-89: LGTM on builder usage and error handling.Builder configuration and
.expect("queue creation failed")reads well.Also applies to: 147-153
src/push/queues.rs (5)
1-8: Module docs read well and use en‑GB spelling.Intro and scope are clear and accurate.
293-305: Builder structure and defaults look sound.Defaults and fluent API are straightforward; docs explain behaviour clearly.
Also applies to: 307-354
397-466: Deprecations are correctly routed via the builder.Shims preserve behaviour while nudging callers to the new API.
212-281: DLQ routing and logging are robust.Error levels and payload visibility are appropriate; tests exercise both full and closed DLQ paths.
373-381: Confirm RateLimiter time source under tokio::time::pause()
Ensure tests that rely on virtual time remain deterministic.
- Verify the exact crate and version providing
RateLimiter- Inspect the crate’s docs or source to confirm
RateLimiter::builder()uses Tokio’s clock (notstd::thread::sleepor the system clock)- Ensure any tests that depend on refills explicitly call
tokio::time::advance()undertokio::time::pause()# Find which crate and version implements RateLimiter rg -n "RateLimiter" Cargo.lock # Locate the import in your code rg -n "use .*RateLimiter" -C2 src/push/queues.rs
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/push.rs (1)
176-179: Clarify the “capacity” in the burst test comment.The burst size equals the token‑bucket capacity (set to
rate), not the queue length.Apply this diff:
-/// A burst up to capacity succeeds and further pushes are blocked. -/// The maximum burst size equals the configured `capacity` parameter. +/// A burst up to the token‑bucket capacity succeeds and further pushes are blocked. +/// The maximum burst size equals the configured `rate` (tokens per second).
♻️ Duplicate comments (5)
src/push/queues.rs (3)
367-371: Simplify rate validation; avoid.filterfor control flow.Use a straight branch for readability and to ease future extensions.
Apply this diff:
- if let Some(r) = rate.filter(|r| *r == 0 || *r > MAX_PUSH_RATE) { - // Reject unsupported rates early to avoid building queues that cannot - // be used. The bounds prevent runaway resource consumption. - return Err(PushConfigError::InvalidRate(r)); - } + if let Some(r) = rate { + if r == 0 || r > MAX_PUSH_RATE { + // Reject unsupported rates early to avoid building unusable queues and runaway limits. + return Err(PushConfigError::InvalidRate(r)); + } + }
331-336: Model non‑zero rates at the type level viaOption<NonZeroUsize>.Add a primary
rate_nzsetter and keep the currentrateas a shim for compatibility.Apply this diff:
@@ - /// Set the global push rate limit in pushes per second. + /// Set the global push rate limit in pushes per second. #[must_use] pub fn rate(mut self, rate: Option<usize>) -> Self { - self.rate = rate; + self.rate = rate; self } + + /// Set the global push rate using a non‑zero type‑level guarantee. + #[must_use] + pub fn rate_nz(mut self, rate: Option<std::num::NonZeroUsize>) -> Self { + self.rate = rate.map(std::num::NonZeroUsize::get); + self + }Add the import outside this hunk:
use std::num::NonZeroUsize; // at the top with other std imports
147-153: Replaceunwrap()withexpect(..)in all doc examples.Align with repo guidelines and emit actionable failures in doctests.
Apply these diffs:
@@ - /// .build() - /// .unwrap(); + /// .build() + /// .expect("queue build failed"); - /// handle.push_high_priority(42u8).await.unwrap(); + /// handle.push_high_priority(42u8).await.expect("push high priority failed"); - /// let (priority, frame) = queues.recv().await.unwrap(); + /// let (priority, frame) = queues.recv().await.expect("recv failed");@@ - /// .build() - /// .unwrap(); + /// .build() + /// .expect("queue build failed"); - /// handle.push_low_priority(10u8).await.unwrap(); + /// handle.push_low_priority(10u8).await.expect("push low priority failed"); - /// let (priority, frame) = queues.recv().await.unwrap(); + /// let (priority, frame) = queues.recv().await.expect("recv failed");@@ - /// .build() - /// .unwrap(); + /// .build() + /// .expect("queue build failed"); @@ - /// .try_push(2u8, PushPriority::High, PushPolicy::DropIfFull) - /// .unwrap(); + /// .try_push(2u8, PushPriority::High, PushPolicy::DropIfFull) + /// .expect("try_push failed");@@ - /// .build() - /// .unwrap(); + /// .build() + /// .expect("queue build failed"); - /// handle.push_high_priority(2u8).await.unwrap(); + /// handle.push_high_priority(2u8).await.expect("push high priority failed"); - /// let (priority, frame) = queues.recv().await.unwrap(); + /// let (priority, frame) = queues.recv().await.expect("recv failed");@@ - /// let (mut queues, _handle) = PushQueues::<u8>::builder().build().unwrap(); + /// let (mut queues, _handle) = PushQueues::<u8>::builder().build().expect("queue build failed");Also applies to: 178-184, 231-237, 481-485, 507-510
tests/push_policies.rs (1)
42-47: Replace.unwrap()with.expect(..)onbuild().Emit clearer failures and align with test guidelines.
Apply this diff:
.low_capacity(1) - .build() - .unwrap(); + .build() + .expect("queue creation failed");tests/push.rs (1)
23-28: Replace.unwrap()with.expect(..)onbuild().Standardise failure messaging across tests.
Apply these diffs:
- .build() - .unwrap(); + .build() + .expect("queue creation failed");- .build() - .unwrap(); + .build() + .expect("queue creation failed");- .build() - .unwrap(); + .build() + .expect("queue creation failed");- .build() - .unwrap(); + .build() + .expect("queue creation failed");Also applies to: 47-52, 67-71, 161-167
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/push/queues.rs(8 hunks)tests/push.rs(8 hunks)tests/push_policies.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use precise names; boolean names should start with is/has/should
Use en-GB-oxendict spelling and grammar in comments
Function documentation must include clear examples; test documentation should omit redundant examples
Keep code files ≤ 400 lines; split long switch/dispatch logic by feature; move large test data to external files
Disallow Clippy warnings
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions for long functions; adhere to separation of concerns and CQRS
Group related parameters into meaningful structs when functions have many parameters
Consider using Arc for large error returns to reduce data size
Each Rust module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless 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
Do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect(..)] over #[allow(..)] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Files:
tests/push_policies.rstests/push.rssrc/push/queues.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:
tests/push_policies.rstests/push.rssrc/push/queues.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
tests/push_policies.rstests/push.rssrc/push/queues.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/push_policies.rstests/push.rs
🧬 Code graph analysis (2)
tests/push_policies.rs (3)
tests/push.rs (1)
queues(11-18)tests/connection_actor.rs (1)
queues(25-31)src/push/queues.rs (1)
builder(359-359)
tests/push.rs (2)
tests/connection_actor.rs (1)
queues(25-31)src/push/queues.rs (2)
builder(359-359)push_high_priority(159-161)
🔍 Remote MCP
Here are the key facts and concrete details to aid your review:
• A new fluent builder API was introduced in src/push/queues.rs:
– Added pub struct PushQueuesBuilder<F> with fields high_capacity, low_capacity, rate, and dlq.
– Chainable methods:
• high_capacity(usize) -> Self
• low_capacity(usize) -> Self
• rate(Option<usize>) -> Self
• dlq(Option<mpsc::Sender<F>>) -> Self
• build(self) -> Result<(PushQueues<F>, PushHandle<F>), PushConfigError> which delegates to the new private helper PushQueues::build_with_rate_dlq(...). (github.com)
• Exposed the builder via
impl<F: FrameLike> PushQueues<F> {
#[must_use]
pub fn builder() -> PushQueuesBuilder<F> { PushQueuesBuilder::new() }
// …
}and consolidated all queue‐construction logic—capacity setup, optional rate limiting (with 1s refill intervals), and optional dead‐letter queue—into build_with_rate_dlq. (github.com)
• The four legacy constructors on PushQueues were deprecated and repointed to the builder:
#[deprecated(since = "0.1.0", note = "Use `PushQueues::builder` instead")]
pub fn bounded(…) → (Self, PushHandle<F>) { … }
#[deprecated(…)] pub fn bounded_no_rate_limit(…) { … }
#[deprecated(…)] pub fn bounded_with_rate(…) { … }
#[deprecated(…)] pub fn bounded_with_rate_dlq(…) { … }All now simply call through to the builder API. (github.com)
• A new module file src/push/mod.rs (5 additions) was added to re-export the queues API:
//! Prioritised queues used for asynchronously pushing frames to a connection.
mod queues;
pub use queues::*;
``` ([github.com](https://github.com/leynos/wireframe/commit/35f4bf4))
• All impacted tests (13 files across `tests/`) were updated to replace direct `PushQueues::bounded*` calls with
```rust
let (queues, handle) = PushQueues::builder()
.high_capacity(…)
.low_capacity(…)
//.rate(Some(n)) // where applicable
//.dlq(Some(tx)) // where applicable
.build()
.unwrap(); // or `.expect("…")`preserving existing test logic but surfacing build‐step errors via Result. (github.com)
• PR metadata:
– Single commit 35f4bf4
– +293/−147 lines over 13 files
– Created Aug 27, 2025 by leynos to merge codex/introduce-pushqueuesbuilder-with-deprecation → main
– Label: codex
– CI status: 4 checks (build/test workflow, CodeQL, pull_request jobs, CodeScene delta analysis)
– Test steps: make fmt, make lint, make test (github.com)
• Linked task for context:
https://chatgpt.com/codex/tasks/task_e_68aeba8d630c8322bcab55de80b2ef06
🔇 Additional comments (3)
src/push/queues.rs (2)
1-7: Solid module docs and en‑GB spelling.Keep this level of clarity elsewhere in the module.
29-33: Keep the compile‑time guard; it prevents config drift.Expose
MAX_PUSH_RATEpublicly and assert the default stays within bounds. Good.tests/push.rs (1)
10-18: Good fixture: reuse queues with explicit rate for deterministic tests.Leave as is.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
tests/push.rs (4)
67-75: Destructurequeuesas mutable at sourceAvoid shadowing to make the binding mutable.
Apply:
- let (queues, handle) = PushQueues::builder() + let (mut queues, handle) = PushQueues::builder() @@ - let mut queues = queues; queues.close();
89-118: Prefer start_paused attribute over manual pauseUse
#[tokio::test(start_paused = true)]to remove explicittime::pause()and rely on deterministic auto‑advance in virtual time.Apply:
-#[tokio::test] +#[tokio::test(start_paused = true)] async fn rate_limiter_blocks_when_exceeded(#[case] priority: PushPriority) { - time::pause(); + // virtual time provided by start_pausedRepeat similarly in other virtual‑time tests in this file.
176-186: Fix misleading comment about burst “capacity”Refer to the configured rate/token‑bucket capacity rather than an undefined “capacity parameter”.
Apply:
-/// A burst up to capacity succeeds and further pushes are blocked. -/// The maximum burst size equals the configured `capacity` parameter. +/// A burst up to the token‑bucket capacity succeeds and further pushes are blocked. +/// The maximum burst size equals the configured `rate` (token‑bucket capacity = rate).
1-206: Verify migration completeness and improve coverage for DLQSearch for any remaining uses of deprecated constructors, stray
.unwrap()on builder results, and absence of DLQ tests. Add at least one test exercisingbuilder().dlq(Some(tx))to validate dead‑letter routing on overflow/closure.Run:
#!/usr/bin/env bash set -euo pipefail echo "Deprecated constructors still referenced?" rg -nP --type=rust '\bPushQueues::bounded(_no_rate_limit|_with_rate(_dlq)?|)\s*\(' || echo "OK: no deprecated constructors" echo "Builder results still unwrapped?" rg -nP --type=rust '\.build\(\)\s*\.unwrap\(\)' || echo "OK: no unwrap on builder results" echo "Old error variant lingering?" rg -nP --type=rust '\bPushError::Full\b' || echo "OK: only QueueFull used" echo "Any DLQ usage in tests?" rg -nP --type=rust 'builder\(\)[\s\S]{0,200}\.dlq\s*\(' tests || echo "No DLQ coverage in tests"tests/connection_actor.rs (1)
391-404: DRY the “small queues” setup behind a fixture.Create a
small_queues()rstest fixture to remove duplication and unify error messages.+#[fixture] +fn small_queues() -> (PushQueues<u8>, wireframe::push::PushHandle<u8>) { + PushQueues::<u8>::builder() + .high_capacity(1) + .low_capacity(1) + .build() + .expect("failed to build push queues") +} @@ -async fn push_queue_exhaustion_backpressure() { - let (mut queues, handle) = PushQueues::builder() - .high_capacity(1) - .low_capacity(1) - .build() - .expect("failed to build push queues"); +async fn push_queue_exhaustion_backpressure(mut queues: PushQueues<u8>, handle: wireframe::push::PushHandle<u8>) {Repeat the same substitution inside the for-loop body in
graceful_shutdown_waits_for_tasks.Also applies to: 481-488
src/push/queues.rs (1)
73-88: Prevent panics: validate capacities and return a typed error.
mpsc::channel(0)panics. Reject zero capacities and surface aPushConfigError::InvalidCapacity.@@ pub enum PushConfigError { /// The provided rate was zero or exceeded [`MAX_PUSH_RATE`]. InvalidRate(usize), + /// One or both queue capacities were zero, which would panic the runtime. + InvalidCapacity { high: usize, low: usize }, } @@ match self { Self::InvalidRate(r) => { write!(f, "invalid rate {r}; must be between 1 and {MAX_PUSH_RATE}") } + Self::InvalidCapacity { high, low } => { + write!(f, "invalid capacities (high={high}, low={low}); both must be ≥ 1") + } } @@ - /// Returns [`PushConfigError::InvalidRate`] if the rate is zero or above - /// [`MAX_PUSH_RATE`]. + /// Returns: + /// - [`PushConfigError::InvalidCapacity`] if a capacity is zero. + /// - [`PushConfigError::InvalidRate`] if the rate is zero or above [`MAX_PUSH_RATE`]. pub fn build(self) -> Result<(PushQueues<F>, PushHandle<F>), PushConfigError> { PushQueues::build_with_rate_dlq(self.high_capacity, self.low_capacity, self.rate, self.dlq) } @@ - ) -> Result<(Self, PushHandle<F>), PushConfigError> { - if let Some(r) = rate.filter(|r| *r == 0 || *r > MAX_PUSH_RATE) { + ) -> Result<(Self, PushHandle<F>), PushConfigError> { + if high_capacity == 0 || low_capacity == 0 { + return Err(PushConfigError::InvalidCapacity { + high: high_capacity, + low: low_capacity, + }); + } + if let Some(r) = rate.filter(|r| *r == 0 || *r > MAX_PUSH_RATE) { // Reject unsupported rates early to avoid building queues that cannot // be used. The bounds prevent runaway resource consumption. return Err(PushConfigError::InvalidRate(r)); }Also applies to: 345-353, 361-372
♻️ Duplicate comments (9)
tests/wireframe_protocol.rs (2)
59-63: Builder migration + expect usage — LGTMAdopt the builder and replace unwrap with expect; aligns with the guideline and prior review.
87-91: Builder migration + expect usage — LGTMUse of expect with a clear message is correct; keep this pattern repo-wide.
tests/advanced/concurrency_loom.rs (1)
24-28: Replace unwrap with expect — resolvedThe loom test now uses expect with a descriptive message; keep consistent across advanced tests.
tests/correlation_id.rs (1)
18-22: Builder migration + expect usage — LGTMAdopt the builder path and preserve clear failure context.
src/push/mod.rs (2)
1-17: Module docs with runnable example — LGTMProvide a concise no_run example using the builder and en‑GB spelling; meets docs guidance.
21-30: Explicit re-exports — LGTMAvoid glob exports; the curated surface matches queues.rs.
tests/push_policies.rs (1)
15-20: Document the lint suppression reason.Add the inline justification on the allow to match policy.
#[expect( unused_braces, reason = "rustc false positive for single line rstest fixtures" )] -#[allow(unfulfilled_lint_expectations)] +// allow(unfulfilled_lint_expectations): rustc occasionally fails to emit the expected lint for single-line rstest fixtures on stable. +#[allow(unfulfilled_lint_expectations)] #[fixture] fn rt() -> Runtime {src/push/queues.rs (2)
331-336: Expose a NonZero rate setter to model invariants at the type level.Add
rate_nz(Option<NonZeroUsize>)and keeprate(Option<usize>)as a thin shim.+use std::num::NonZeroUsize; @@ - pub fn rate(mut self, rate: Option<usize>) -> Self { - self.rate = rate; + pub fn rate(mut self, rate: Option<usize>) -> Self { + self.rate = rate; + self + } + + /// Set the global push rate using a non-zero type. + #[must_use] + pub fn rate_nz(mut self, rate: Option<NonZeroUsize>) -> Self { + self.rate = rate.map(NonZeroUsize::get); self }
361-371: Clarify invalid-rate branching.Expand the
filterinto straight-line logic for readability; behaviour unchanged.- if let Some(r) = rate.filter(|r| *r == 0 || *r > MAX_PUSH_RATE) { - // Reject unsupported rates early to avoid building queues that cannot - // be used. The bounds prevent runaway resource consumption. - return Err(PushConfigError::InvalidRate(r)); - } + if let Some(r) = rate { + if r == 0 || r > MAX_PUSH_RATE { + // Reject unsupported rates early to avoid unusable queues and runaway resource use. + return Err(PushConfigError::InvalidRate(r)); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
src/push/mod.rs(1 hunks)src/push/queues.rs(8 hunks)tests/advanced/concurrency_loom.rs(1 hunks)tests/advanced/interaction_fuzz.rs(1 hunks)tests/async_stream.rs(1 hunks)tests/connection_actor.rs(3 hunks)tests/correlation_id.rs(1 hunks)tests/push.rs(8 hunks)tests/push_policies.rs(4 hunks)tests/session_registry.rs(1 hunks)tests/stream_end.rs(2 hunks)tests/wireframe_protocol.rs(2 hunks)tests/world.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use precise names; boolean names should start with is/has/should
Use en-GB-oxendict spelling and grammar in comments
Function documentation must include clear examples; test documentation should omit redundant examples
Keep code files ≤ 400 lines; split long switch/dispatch logic by feature; move large test data to external files
Disallow Clippy warnings
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions for long functions; adhere to separation of concerns and CQRS
Group related parameters into meaningful structs when functions have many parameters
Consider using Arc for large error returns to reduce data size
Each Rust module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless 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
Do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect(..)] over #[allow(..)] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Files:
tests/async_stream.rstests/correlation_id.rstests/session_registry.rstests/wireframe_protocol.rstests/world.rstests/connection_actor.rstests/advanced/interaction_fuzz.rstests/push.rssrc/push/mod.rssrc/push/queues.rstests/stream_end.rstests/advanced/concurrency_loom.rstests/push_policies.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:
tests/async_stream.rstests/correlation_id.rstests/session_registry.rstests/wireframe_protocol.rstests/world.rstests/connection_actor.rstests/advanced/interaction_fuzz.rstests/push.rssrc/push/mod.rssrc/push/queues.rstests/stream_end.rstests/advanced/concurrency_loom.rstests/push_policies.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
tests/async_stream.rstests/correlation_id.rstests/session_registry.rstests/wireframe_protocol.rstests/world.rstests/connection_actor.rstests/advanced/interaction_fuzz.rstests/push.rssrc/push/mod.rssrc/push/queues.rstests/stream_end.rstests/advanced/concurrency_loom.rstests/push_policies.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/async_stream.rstests/correlation_id.rstests/session_registry.rstests/wireframe_protocol.rstests/world.rstests/connection_actor.rstests/advanced/interaction_fuzz.rstests/push.rstests/stream_end.rstests/advanced/concurrency_loom.rstests/push_policies.rs
🧬 Code graph analysis (12)
tests/async_stream.rs (3)
tests/connection_actor.rs (1)
queues(26-32)tests/push.rs (1)
queues(11-18)src/push/queues.rs (1)
builder(359-359)
tests/correlation_id.rs (3)
tests/connection_actor.rs (1)
queues(26-32)tests/push.rs (1)
queues(11-18)src/push/queues.rs (1)
builder(359-359)
tests/session_registry.rs (1)
src/push/queues.rs (1)
builder(359-359)
tests/wireframe_protocol.rs (3)
tests/connection_actor.rs (1)
queues(26-32)tests/push.rs (1)
queues(11-18)src/push/queues.rs (1)
builder(359-359)
tests/world.rs (3)
tests/connection_actor.rs (1)
queues(26-32)tests/push.rs (1)
queues(11-18)src/push/queues.rs (1)
builder(359-359)
tests/connection_actor.rs (3)
tests/push.rs (1)
queues(11-18)src/push/queues.rs (2)
builder(359-359)new(308-315)src/connection.rs (3)
new(30-34)new(138-151)new(517-526)
tests/advanced/interaction_fuzz.rs (3)
tests/connection_actor.rs (1)
queues(26-32)tests/push.rs (1)
queues(11-18)src/push/queues.rs (1)
builder(359-359)
tests/push.rs (2)
tests/connection_actor.rs (1)
queues(26-32)src/push/queues.rs (2)
builder(359-359)push_high_priority(159-161)
src/push/mod.rs (2)
tests/connection_actor.rs (1)
queues(26-32)tests/push.rs (1)
queues(11-18)
tests/stream_end.rs (3)
tests/connection_actor.rs (1)
queues(26-32)tests/push.rs (1)
queues(11-18)src/push/queues.rs (1)
builder(359-359)
tests/advanced/concurrency_loom.rs (3)
tests/connection_actor.rs (1)
queues(26-32)tests/push.rs (1)
queues(11-18)src/push/queues.rs (1)
builder(359-359)
tests/push_policies.rs (3)
tests/connection_actor.rs (1)
queues(26-32)tests/push.rs (1)
queues(11-18)src/push/queues.rs (1)
builder(359-359)
🔍 Remote MCP
Here are the key facts and concrete details to aid your review:
• A new fluent builder API was introduced in src/push/queues.rs:
– Added pub struct PushQueuesBuilder<F> with fields high_capacity, low_capacity, rate, and dlq.
– Chainable methods:
• high_capacity(usize) -> Self
• low_capacity(usize) -> Self
• rate(Option<usize>) -> Self
• dlq(Option<mpsc::Sender<F>>) -> Self
• build(self) -> Result<(PushQueues<F>, PushHandle<F>), PushConfigError> which delegates to the new private helper PushQueues::build_with_rate_dlq(...). (github.com)
• Exposed the builder via
impl<F: FrameLike> PushQueues<F> {
#[must_use]
pub fn builder() -> PushQueuesBuilder<F> { PushQueuesBuilder::new() }
// …
}and consolidated all queue‐construction logic—capacity setup, optional rate limiting (with 1s refill intervals), and optional dead‐letter queue—into build_with_rate_dlq. (github.com)
• The four legacy constructors on PushQueues were deprecated and repointed to the builder:
#[deprecated(since = "0.1.0", note = "Use `PushQueues::builder` instead")]
pub fn bounded(…) → (Self, PushHandle<F>) { … }
#[deprecated(…)] pub fn bounded_no_rate_limit(…) { … }
#[deprecated(…)] pub fn bounded_with_rate(…) { … }
#[deprecated(…)] pub fn bounded_with_rate_dlq(…) { … }All now simply call through to the builder API. (github.com)
• A new module file src/push/mod.rs (5 additions) was added to re-export the queues API:
//! Prioritised queues used for asynchronously pushing frames to a connection.
mod queues;
pub use queues::*;
``` ([github.com](https://github.com/leynos/wireframe/commit/35f4bf4))
• All impacted tests (13 files across `tests/`) were updated to replace direct `PushQueues::bounded*` calls with
```rust
let (queues, handle) = PushQueues::builder()
.high_capacity(…)
.low_capacity(…)
//.rate(Some(n)) // where applicable
//.dlq(Some(tx)) // where applicable
.build()
.unwrap(); // or `.expect("…")`preserving existing test logic but surfacing build‐step errors via Result. (github.com)
• PR metadata:
– Single commit 35f4bf4
– +293/−147 lines over 13 files
– Created Aug 27, 2025 by leynos to merge codex/introduce-pushqueuesbuilder-with-deprecation → main
– Label: codex
– CI status: 4 checks (build/test workflow, CodeQL, pull_request jobs, CodeScene delta analysis)
– Test steps: make fmt, make lint, make test (github.com)
• Linked task for context:
https://chatgpt.com/codex/tasks/task_e_68aeba8d630c8322bcab55de80b2ef06
⏰ 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 (18)
tests/session_registry.rs (2)
9-13: Use targeted #[expect] on fixture — LGTMScope and justification are clear for the rstest false positive.
23-28: Builder migration + expect usage — LGTMConstruct via builder and fail with context; this matches the new API.
tests/push.rs (4)
10-18: Good fixture extraction for common queue configKeep using this fixture to reduce duplication across rate‑limiter tests. The expect message is clear and aligns with guidelines.
23-28: Adopt builder API in routing testUse of
builder().high_capacity().low_capacity().build().expect(...)is correct and future‑proofs against deprecated constructors.
44-56: Align test with renamed error variantAssertion against
PushError::QueueFullis correct and the doc comment accurately describes behaviour.
161-166: Unlimited queues case is correct
rate(None)properly exercises the unlimited path; the timeout completes because the inner push is immediate under paused time.tests/stream_end.rs (2)
26-31: Adopt builder API with clear failure contextConstruction via builder with
.expect("failed to build push queues")is compliant and readable.
58-63: Repeat builder usage consistentlyMaintain this pattern across the file for consistency and clearer diagnostics.
tests/advanced/interaction_fuzz.rs (1)
26-31: Adopt builder API in fuzz harnessInitialisation via builder is correct; keep
.expect(...)for actionable failures in property tests.tests/world.rs (2)
145-150: Adopt builder API for correlation worldThe change is correct and the error message is informative.
187-192: Adopt builder API for stream‑end worldConsistent with the rest of the suite and aligned with deprecations.
tests/connection_actor.rs (3)
27-31: Adopt the builder + expect path.Good migration away from deprecated constructors and unwrap.
392-396: Adopt the builder + expect path.Good usage with minimal capacities for back-pressure tests.
483-487: Adopt the builder + expect path.Good usage in the loop; clear failure message.
tests/push_policies.rs (2)
42-47: Adopt the builder + expect path.Good migration; message is clear.
84-90: Adopt the builder + expect path.Good wiring of rate(None) and DLQ.
src/push/queues.rs (2)
1-7: Document the module.Good high-level overview and en‑GB spelling.
33-33: Guard defaults at compile time.Good invariant check.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/server.rs (1)
61-66: Eliminate fixed sleep to reduce flakiness.Probe readiness with a bounded retry loop rather than
sleep(100ms).Use this snippet to replace the fixed sleep:
// Poll for readiness with timeout to avoid flakes let deadline = tokio::time::Instant::now() + Duration::from_secs(3); loop { match TcpStream::connect(addr).await { Ok(stream) => { let _ = stream; break; } Err(e) if tokio::time::Instant::now() < deadline => tokio::time::sleep(Duration::from_millis(25)).await, Err(e) => panic!("connect failed after retries: {e}"), } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
src/push/queues/builder.rs(1 hunks)src/push/queues/errors.rs(1 hunks)src/push/queues/handle.rs(1 hunks)src/push/queues/mod.rs(1 hunks)tests/correlation_id.rs(1 hunks)tests/server.rs(1 hunks)tests/wireframe_protocol.rs(2 hunks)tests/world.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use precise names; boolean names should start with is/has/should
Use en-GB-oxendict spelling and grammar in comments
Function documentation must include clear examples; test documentation should omit redundant examples
Keep code files ≤ 400 lines; split long switch/dispatch logic by feature; move large test data to external files
Disallow Clippy warnings
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions for long functions; adhere to separation of concerns and CQRS
Group related parameters into meaningful structs when functions have many parameters
Consider using Arc for large error returns to reduce data size
Each Rust module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless 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
Do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect(..)] over #[allow(..)] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Files:
src/push/queues/builder.rstests/server.rstests/world.rssrc/push/queues/errors.rstests/correlation_id.rssrc/push/queues/mod.rstests/wireframe_protocol.rssrc/push/queues/handle.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:
src/push/queues/builder.rstests/server.rstests/world.rssrc/push/queues/errors.rstests/correlation_id.rssrc/push/queues/mod.rstests/wireframe_protocol.rssrc/push/queues/handle.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
src/push/queues/builder.rstests/server.rstests/world.rssrc/push/queues/errors.rstests/correlation_id.rssrc/push/queues/mod.rstests/wireframe_protocol.rssrc/push/queues/handle.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/server.rstests/world.rstests/correlation_id.rstests/wireframe_protocol.rs
🧬 Code graph analysis (6)
src/push/queues/builder.rs (1)
src/push/queues/mod.rs (1)
build_with_rate_dlq(69-103)
tests/world.rs (3)
tests/push.rs (1)
queues(11-18)tests/connection_actor.rs (1)
queues(26-32)src/push/queues/mod.rs (1)
builder(67-67)
tests/correlation_id.rs (3)
tests/push.rs (1)
queues(11-18)tests/connection_actor.rs (1)
queues(26-32)src/push/queues/mod.rs (1)
builder(67-67)
src/push/queues/mod.rs (2)
src/push/queues/builder.rs (5)
new(22-29)high_capacity(33-36)low_capacity(40-43)rate(47-50)dlq(54-57)src/push/queues/handle.rs (1)
from_arc(32-32)
tests/wireframe_protocol.rs (3)
tests/push.rs (1)
queues(11-18)tests/connection_actor.rs (1)
queues(26-32)src/push/queues/mod.rs (1)
builder(67-67)
src/push/queues/handle.rs (2)
src/push/queues/errors.rs (2)
fmt(15-20)fmt(33-43)src/push/queues/builder.rs (1)
dlq(54-57)
🔍 Remote MCP
Here are the key facts and concrete details to aid your review:
• A new fluent builder API was introduced in src/push/queues.rs:
– Added pub struct PushQueuesBuilder<F> with fields high_capacity, low_capacity, rate, and dlq.
– Chainable methods:
• high_capacity(usize) -> Self
• low_capacity(usize) -> Self
• rate(Option<usize>) -> Self
• dlq(Option<mpsc::Sender<F>>) -> Self
• build(self) -> Result<(PushQueues<F>, PushHandle<F>), PushConfigError> which delegates to the new private helper PushQueues::build_with_rate_dlq(...). (github.com)
• Exposed the builder via
impl<F: FrameLike> PushQueues<F> {
#[must_use]
pub fn builder() -> PushQueuesBuilder<F> { PushQueuesBuilder::new() }
// …
}and consolidated all queue‐construction logic—capacity setup, optional rate limiting (with 1s refill intervals), and optional dead‐letter queue—into build_with_rate_dlq. (github.com)
• The four legacy constructors on PushQueues were deprecated and repointed to the builder:
#[deprecated(since = "0.1.0", note = "Use `PushQueues::builder` instead")]
pub fn bounded(…) → (Self, PushHandle<F>) { … }
#[deprecated(…)] pub fn bounded_no_rate_limit(…) { … }
#[deprecated(…)] pub fn bounded_with_rate(…) { … }
#[deprecated(…)] pub fn bounded_with_rate_dlq(…) { … }All now simply call through to the builder API. (github.com)
• A new module file src/push/mod.rs (5 additions) was added to re-export the queues API:
//! Prioritised queues used for asynchronously pushing frames to a connection.
mod queues;
pub use queues::*;
``` ([github.com](https://github.com/leynos/wireframe/commit/35f4bf4))
• All impacted tests (13 files across `tests/`) were updated to replace direct `PushQueues::bounded*` calls with
```rust
let (queues, handle) = PushQueues::builder()
.high_capacity(…)
.low_capacity(…)
//.rate(Some(n)) // where applicable
//.dlq(Some(tx)) // where applicable
.build()
.unwrap(); // or `.expect("…")`preserving existing test logic but surfacing build‐step errors via Result. (github.com)
• PR metadata:
– Single commit 35f4bf4
– +293/−147 lines over 13 files
– Created Aug 27, 2025 by leynos to merge codex/introduce-pushqueuesbuilder-with-deprecation → main
– Label: codex
– CI status: 4 checks (build/test workflow, CodeQL, pull_request jobs, CodeScene delta analysis)
– Test steps: make fmt, make lint, make test (github.com)
• Linked task for context:
https://chatgpt.com/codex/tasks/task_e_68aeba8d630c8322bcab55de80b2ef06
⏰ 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 (7)
tests/server.rs (1)
46-46: Swap unwrap with expect.Good change. Preserve failure context per lint policy.
tests/correlation_id.rs (1)
18-22: Adopt builder API with explicit expect.Good migration to the builder and improved failure diagnostics.
tests/wireframe_protocol.rs (2)
59-63: Adopt builder API in protocol hooks test.Good update; complies with new construction path and lint policy.
87-91: Adopt builder API in connection actor test.Good update; message clarifies context on failure.
tests/world.rs (2)
145-149: Adopt builder with.expect(...)— compliant and clear. LGTM.The switch to the builder API with a descriptive
.expect(...)message matches the lint policy and keeps the test intent obvious.
187-191: Same here — correct builder usage with.expect(...).This aligns with the updated public API and avoids Clippy warnings from
.unwrap().src/push/queues/mod.rs (1)
37-39: Use a const block for the compile-time assertion and verify MSRV.Adopt the idiomatic const-assert form and ensure it compiles on the project’s MSRV.
Apply:
-// Compile-time guard: DEFAULT_PUSH_RATE must not exceed MAX_PUSH_RATE. -const _: () = assert!(DEFAULT_PUSH_RATE <= MAX_PUSH_RATE); +// Compile-time guard: DEFAULT_PUSH_RATE must not exceed MAX_PUSH_RATE. +const _: () = { assert!(DEFAULT_PUSH_RATE <= MAX_PUSH_RATE); };Run:
#!/bin/bash # Verify MSRV compatibility of const-assert and that no panics arise from capacities. rg -nP 'channel\(\s*0\s*\)' --type=rs -C2 cargo check -q
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
tests/session_registry.rs (3)
9-14: Attach a reason to the lint suppression or switch to #[expect].Align with the guideline “Prefer #[expect(..)] over #[allow(..)]” and require a reason. Keep the scope tight.
Apply:
-#[allow(unfulfilled_lint_expectations)] +#[allow(unfulfilled_lint_expectations, reason = "rstest fixtures use #[expect]; rustc may not emit the targeted lint on single-line fixtures")]
17-22: Repeat the lint-suppression rationale for consistency.Mirror the rationale on this fixture too.
Apply:
-#[allow(unfulfilled_lint_expectations)] +#[allow(unfulfilled_lint_expectations, reason = "rstest fixtures use #[expect]; rustc may not emit the targeted lint on single-line fixtures")]
23-28: Standardise the failure message casing in.expect(...).Use the type name “PushQueues” for consistency across tests and docs.
Apply:
- .expect("failed to build push queues") + .expect("failed to build PushQueues")src/push/queues/errors.rs (1)
7-16: Mark public error enums non-exhaustive to allow forward-compatible additions.Future-proof the API without semver breakage.
Apply:
-#[derive(Debug, Error)] +#[non_exhaustive] +#[derive(Debug, Error)] pub enum PushError {src/push/queues/builder.rs (2)
7-14: Add a Rustdoc usage example.Document an end-to-end builder flow to satisfy the public-API docs requirement.
Apply:
/// Builder for [`PushQueues`]. /// /// Allows configuration of queue capacities, rate limiting and an optional /// dead-letter queue before constructing [`PushQueues`] and its paired /// [`PushHandle`]. Defaults mirror the previous constructors: both queues have /// a capacity of one and pushes are limited to [`DEFAULT_PUSH_RATE`] per second /// unless overridden. +/// +/// # Examples +/// ```rust,no_run +/// use tokio::sync::mpsc; +/// use wireframe::push::PushQueues; +/// +/// # async fn demo() { +/// let (dlq_tx, _dlq_rx) = mpsc::channel(8); +/// let (_queues, _handle) = PushQueues::<u8>::builder() +/// .high_capacity(8) +/// .low_capacity(8) +/// .rate(Some(100)) +/// .dlq(Some(dlq_tx)) +/// .build() +/// .expect("failed to build PushQueues"); +/// # } +/// ```
45-50: Clarifyrate(None)semantics in docs.State explicitly that
Nonedisables rate limiting.Apply:
- /// Set the global push rate limit in pushes per second. + /// Set the global push rate limit in pushes per second. Pass `None` to disable rate limiting.src/push/queues/mod.rs (3)
166-173: Standardise hyphenation: “dead‑letter queue”.Keep terminology consistent across docs.
Apply:
- /// Create queues with a custom rate limit and optional dead letter queue. + /// Create queues with a custom rate limit and optional dead‑letter queue.
208-214: Fixrecv()early termination when the high-priority channel closes.The biased select returns
Noneas soon as one channel closes, dropping remaining frames in the other channel.Apply:
- pub async fn recv(&mut self) -> Option<(PushPriority, F)> { - tokio::select! { - biased; - res = self.high_priority_rx.recv() => res.map(|f| (PushPriority::High, f)), - res = self.low_priority_rx.recv() => res.map(|f| (PushPriority::Low, f)), - } - } + pub async fn recv(&mut self) -> Option<(PushPriority, F)> { + let mut high_closed = false; + let mut low_closed = false; + loop { + tokio::select! { + biased; + res = self.high_priority_rx.recv(), if !high_closed => match res { + Some(f) => return Some((PushPriority::High, f)), + None => high_closed = true, + }, + res = self.low_priority_rx.recv(), if !low_closed => match res { + Some(f) => return Some((PushPriority::Low, f)), + None => low_closed = true, + }, + else => return None, + } + } + }
31-39: ExposeDEFAULT_PUSH_RATEor avoid linking to a private item.Make the default discoverable and keep intra-doc links valid.
Apply:
-// Default maximum pushes per second when no custom rate is specified. -// This is an internal implementation detail and may change. -const DEFAULT_PUSH_RATE: usize = 100; +/// Default per‑second push rate used by [`PushQueuesBuilder`] when no custom rate is configured. +pub const DEFAULT_PUSH_RATE: usize = 100; // Highest supported rate for [`PushQueuesBuilder::rate`]. pub const MAX_PUSH_RATE: usize = 10_000; -// Compile-time guard: DEFAULT_PUSH_RATE must not exceed MAX_PUSH_RATE. -const _: () = assert!(DEFAULT_PUSH_RATE <= MAX_PUSH_RATE); +// Compile-time guard: DEFAULT_PUSH_RATE must not exceed MAX_PUSH_RATE. +const _: () = assert!(DEFAULT_PUSH_RATE <= MAX_PUSH_RATE);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/push/queues/builder.rs(1 hunks)src/push/queues/errors.rs(1 hunks)src/push/queues/mod.rs(1 hunks)tests/session_registry.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use precise names; boolean names should start with is/has/should
Use en-GB-oxendict spelling and grammar in comments
Function documentation must include clear examples; test documentation should omit redundant examples
Keep code files ≤ 400 lines; split long switch/dispatch logic by feature; move large test data to external files
Disallow Clippy warnings
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions for long functions; adhere to separation of concerns and CQRS
Group related parameters into meaningful structs when functions have many parameters
Consider using Arc for large error returns to reduce data size
Each Rust module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless 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
Do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect(..)] over #[allow(..)] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Files:
tests/session_registry.rssrc/push/queues/errors.rssrc/push/queues/builder.rssrc/push/queues/mod.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:
tests/session_registry.rssrc/push/queues/errors.rssrc/push/queues/builder.rssrc/push/queues/mod.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
tests/session_registry.rssrc/push/queues/errors.rssrc/push/queues/builder.rssrc/push/queues/mod.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/session_registry.rs
🧬 Code graph analysis (3)
tests/session_registry.rs (1)
src/push/queues/mod.rs (1)
builder(67-67)
src/push/queues/builder.rs (1)
src/push/queues/mod.rs (1)
build_with_rate_dlq(69-109)
src/push/queues/mod.rs (2)
src/push/queues/builder.rs (5)
new(22-29)high_capacity(33-36)low_capacity(40-43)rate(47-50)dlq(54-57)src/push/queues/handle.rs (1)
from_arc(32-32)
🔍 Remote MCP
Here are the key facts and concrete details to aid your review:
• A new fluent builder API was introduced in src/push/queues.rs:
– Added pub struct PushQueuesBuilder<F> with fields high_capacity, low_capacity, rate, and dlq.
– Chainable methods:
• high_capacity(usize) -> Self
• low_capacity(usize) -> Self
• rate(Option<usize>) -> Self
• dlq(Option<mpsc::Sender<F>>) -> Self
• build(self) -> Result<(PushQueues<F>, PushHandle<F>), PushConfigError> which delegates to the new private helper PushQueues::build_with_rate_dlq(...). (github.com)
• Exposed the builder via
impl<F: FrameLike> PushQueues<F> {
#[must_use]
pub fn builder() -> PushQueuesBuilder<F> { PushQueuesBuilder::new() }
// …
}and consolidated all queue‐construction logic—capacity setup, optional rate limiting (with 1s refill intervals), and optional dead‐letter queue—into build_with_rate_dlq. (github.com)
• The four legacy constructors on PushQueues were deprecated and repointed to the builder:
#[deprecated(since = "0.1.0", note = "Use `PushQueues::builder` instead")]
pub fn bounded(…) → (Self, PushHandle<F>) { … }
#[deprecated(…)] pub fn bounded_no_rate_limit(…) { … }
#[deprecated(…)] pub fn bounded_with_rate(…) { … }
#[deprecated(…)] pub fn bounded_with_rate_dlq(…) { … }All now simply call through to the builder API. (github.com)
• A new module file src/push/mod.rs (5 additions) was added to re-export the queues API:
//! Prioritised queues used for asynchronously pushing frames to a connection.
mod queues;
pub use queues::*;
``` ([github.com](https://github.com/leynos/wireframe/commit/35f4bf4))
• All impacted tests (13 files across `tests/`) were updated to replace direct `PushQueues::bounded*` calls with
```rust
let (queues, handle) = PushQueues::builder()
.high_capacity(…)
.low_capacity(…)
//.rate(Some(n)) // where applicable
//.dlq(Some(tx)) // where applicable
.build()
.unwrap(); // or `.expect("…")`preserving existing test logic but surfacing build‐step errors via Result. (github.com)
• PR metadata:
– Single commit 35f4bf4
– +293/−147 lines over 13 files
– Created Aug 27, 2025 by leynos to merge codex/introduce-pushqueuesbuilder-with-deprecation → main
– Label: codex
– CI status: 4 checks (build/test workflow, CodeQL, pull_request jobs, CodeScene delta analysis)
– Test steps: make fmt, make lint, make test (github.com)
• Linked task for context:
https://chatgpt.com/codex/tasks/task_e_68aeba8d630c8322bcab55de80b2ef06
🔇 Additional comments (1)
src/push/queues/mod.rs (1)
69-85: LGTM: validate capacities and rates before creating channels.Reject zero capacities and out-of-range rates to avoid panics and runaway resource use.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/session_registry.rs (2)
9-14: Add a reason to the lint suppression (repeat).Annotate
#[allow(unfulfilled_lint_expectations)]with an explicit reason per lint policy.-#[allow(unfulfilled_lint_expectations)] +#[allow(unfulfilled_lint_expectations, reason = "rstest fixtures use #[expect]; rustc may not emit the targeted lint on single-line fixtures")]
17-22: Add a reason to the second lint suppression (repeat).Apply the same reasoning annotation here.
-#[allow(unfulfilled_lint_expectations)] +#[allow(unfulfilled_lint_expectations, reason = "rstest fixtures use #[expect]; rustc may not emit the targeted lint on single-line fixtures")]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/session_registry.rs(1 hunks)tests/stream_end.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use precise names; boolean names should start with is/has/should
Use en-GB-oxendict spelling and grammar in comments
Function documentation must include clear examples; test documentation should omit redundant examples
Keep code files ≤ 400 lines; split long switch/dispatch logic by feature; move large test data to external files
Disallow Clippy warnings
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions for long functions; adhere to separation of concerns and CQRS
Group related parameters into meaningful structs when functions have many parameters
Consider using Arc for large error returns to reduce data size
Each Rust module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless 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
Do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect(..)] over #[allow(..)] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Files:
tests/stream_end.rstests/session_registry.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:
tests/stream_end.rstests/session_registry.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
tests/stream_end.rstests/session_registry.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/stream_end.rstests/session_registry.rs
🧬 Code graph analysis (2)
tests/stream_end.rs (3)
tests/push.rs (1)
queues(11-18)tests/connection_actor.rs (1)
queues(26-32)src/push/queues/mod.rs (1)
builder(67-67)
tests/session_registry.rs (1)
src/push/queues/mod.rs (1)
builder(67-67)
🔍 Remote MCP Ref
Summary of additional, review-relevant facts (sourced from the PR context and files in the change):
- New public builder API:
- PushQueues::builder() -> PushQueuesBuilder
- PushQueuesBuilder methods: high_capacity(usize), low_capacity(usize), rate(Option), dlq(Option<mpsc::Sender)), build(self) -> Result<(PushQueues, PushHandle), PushConfigError>
- Builder defaults: high_capacity = 1, low_capacity = 1, rate = Some(DEFAULT_PUSH_RATE), dlq = None
- Legacy constructors deprecated:
- bounded, bounded_no_rate_limit, bounded_with_rate, bounded_with_rate_dlq now deprecated and forward to the builder
- Configuration validation:
- build validates rate (must be between 1 and MAX_PUSH_RATE) and capacity (must be ≥ 1); invalid values return PushConfigError::{InvalidRate, InvalidCapacity}
- Error types and messages:
- PushError: QueueFull ("push queue full"), Closed ("push queue closed")
- PushConfigError: InvalidRate(usize) -> "invalid rate {0}; must be between 1 and {MAX_PUSH_RATE}", InvalidCapacity { high, low } -> "invalid capacities; high={high}, low={low}; each must be ≥ 1"
- Runtime behaviour preserved:
- PushQueues still uses two tokio::mpsc channels (high/low) with recv() biasing high priority and optional leaky_bucket::RateLimiter for per-second rate limiting
- PushHandle implements push_high_priority, push_low_priority, try_push, and downgrade; DLQ routing on drop-if-full behaviours present
- Test impact:
- ~13 test files updated to replace PushQueues::bounded* calls with the builder; build() now returns Result and tests use expect()/unwrap(), surfacing construction errors as panics
- New rstest fixture added in tests/push.rs to centralize builder usage
- API surface additions (public):
- PushQueuesBuilder, PushQueues::builder, PushPriority, PushPolicy, PushQueues, PushHandle, PushError, PushConfigError, MAX_PUSH_RATE, FrameLike trait (blanket impl for T: Send + 'static)
- Miscellaneous PR metadata:
Tool attempts for further external documentation (failed with HTTP 402):
- Attempted searches/reads via documentation tool(s) but requests returned 402 Payment Required; external doc fetches unavailable:,
Sources:
- PR contents and diffs provided in the review context (files under src/push/* and tests/*) — summarized above.
- Tool attempts noted:,
⏰ 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 (3)
tests/stream_end.rs (2)
26-30: Adopt builder API correctly — LGTM.Use of
PushQueues::<u8>::builder()with.expect(...)matches the new API and testing guideline.
58-62: Adopt builder API correctly — LGTM.Pattern mirrors the first test and is sound.
tests/session_registry.rs (1)
23-28: Builder-based setup — LGTM.Fixture returns
(PushQueues<u8>, PushHandle<u8>)via builder with.expect(...); this aligns with the new API and test style.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/hardening-wireframe-a-guide-to-production-resilience.md (1)
350-371: Use the new PushQueuesBuilder::dlq API in documentation.Replace references to a “WireframeApp builder” DLQ with the canonical
PushQueuesBuilder::dlq(...).-The WireframeApp builder will allow the user to provide a mpsc::Sender for a -DLQ. The PushHandle's internal logic for handling a full queue will be modified: +Configure a Dead Letter Queue via the per-connection `PushQueuesBuilder`: +call `.dlq(Some(mpsc::Sender<F>))` when building queues. The `PushHandle` +routes frames that cannot be enqueued (e.g., queue full) to the DLQ:tests/push_policies.rs (2)
59-62: Deflake the timeout.Bump the timeout to reduce CI flakiness under load.
- timeout(Duration::from_millis(20), queues.recv()) + timeout(Duration::from_millis(100), queues.recv())
79-81: Standardise hyphenation: “dead‑letter queue”.Align docs with preferred terminology.
-/// Dropped frames are forwarded to the dead letter queue. +/// Dropped frames are forwarded to the dead‑letter queue.
♻️ Duplicate comments (13)
tests/connection_actor.rs (3)
21-26: Document the lint suppression reason (repeat).Annotate the
allow(unfulfilled_lint_expectations)with an inline justification, per lint policy.#[expect( unused_braces, reason = "rustc false positive for single line rstest fixtures" )] -#[allow(unfulfilled_lint_expectations)] +// allow(unfulfilled_lint_expectations): rustc occasionally fails to emit the expected lint for single-line rstest fixtures on stable. +#[allow(unfulfilled_lint_expectations)] fn queues() -> (PushQueues<u8>, wireframe::push::PushHandle<u8>) {
35-40: Document the lint suppression reason (repeat).Add the inline justification for the
allow(...)to comply with project policy.#[expect( unused_braces, reason = "rustc false positive for single line rstest fixtures" )] -#[allow(unfulfilled_lint_expectations)] +// allow(unfulfilled_lint_expectations): rustc occasionally fails to emit the expected lint for single-line rstest fixtures on stable. +#[allow(unfulfilled_lint_expectations)] fn shutdown_token() -> CancellationToken { CancellationToken::new() }
43-48: Document the lint suppression reason (repeat).Justify the
allow(unfulfilled_lint_expectations)in-line.#[expect( unused_braces, reason = "rustc false positive for single line rstest fixtures" )] -#[allow(unfulfilled_lint_expectations)] +// allow(unfulfilled_lint_expectations): rustc occasionally fails to emit the expected lint for single-line rstest fixtures on stable. +#[allow(unfulfilled_lint_expectations)] fn empty_stream() -> Option<FrameStream<u8, ()>> { None }src/push/queues/builder.rs (2)
16-25: Expand the Rustdoc example to cover capacities, rate, and DLQ.Show end-to-end usage including
.dlq(...)and a non-default.rate(...).-/// # Examples -/// -/// ``` -/// use wireframe::push::PushQueues; -/// -/// let (_queues, _handle) = PushQueues::<u8>::builder() -/// .build() -/// .expect("failed to build push queues"); -/// # drop((_queues, _handle)); -/// ``` +/// # Examples +/// +/// ```rust,no_run +/// use tokio::sync::mpsc; +/// use wireframe::push::PushQueues; +/// +/// # async fn demo() { +/// let (dlq_tx, _dlq_rx) = mpsc::channel(8); +/// let (_queues, _handle) = PushQueues::<u8>::builder() +/// .high_capacity(8) +/// .low_capacity(8) +/// .rate(Some(100)) +/// .dlq(Some(dlq_tx)) +/// .build() +/// .expect("failed to build push queues"); +/// # } +/// ```
60-66: Clarifyrate(None)semantics.State explicitly that
Nonedisables rate limiting.- /// Set the global push rate limit in pushes per second. + /// Set the global push rate limit in pushes per second. Pass `None` to disable rate limiting. #[must_use] pub fn rate(mut self, rate: Option<usize>) -> Self {tests/push_policies.rs (2)
15-20: Scope and justify the lint suppression; keep #[allow] narrowly scoped.Add an inline justification for the scoped allow per our learnings (PR #153), and keep it attached to this fixture only.
#[expect( unused_braces, reason = "rustc false positive for single line rstest fixtures" )] -#[allow(unfulfilled_lint_expectations)] +// allow(unfulfilled_lint_expectations): work around rustc occasionally missing the expected lint in this rstest fixture (see PR #153 learning). +#[allow(unfulfilled_lint_expectations)] #[fixture] fn rt() -> Runtime {
90-91: Unify .expect(...) failure messages.Standardise on one message across this file (e.g., "failed to build push queues").
- .expect("queue creation failed"); + .expect("failed to build push queues"); @@ - .expect("queue creation failed"); + .expect("failed to build push queues");Also applies to: 154-155
src/push/queues/handle.rs (3)
7-7: Demote noisy success log to trace.Avoid debug‑level log spam on hot paths.
-use tracing::{debug, error, warn}; +use tracing::{error, warn, trace}; @@ - debug!(?priority, "frame pushed"); + trace!(?priority, "frame pushed");Also applies to: 47-47
39-41: Plumb cancellation into rate‑limit waits to avoid shutdown hangs.Do not await the limiter indefinitely; allow producers to abort on shutdown.
- if let Some(ref limiter) = self.0.limiter { - limiter.acquire(1).await; - } + if let Some(ref limiter) = self.0.limiter { + // TODO: thread a CancellationToken through public push_* APIs and select on it here. + // use tokio_util::sync::CancellationToken; + // tokio::select! { + // _ = limiter.acquire(1) => {}, + // _ = token.cancelled() => return Err(PushError::Closed), + // } + limiter.acquire(1).await; + }Check latest Tokio guidance on cancellation patterns for rate-limited producers using leaky_bucket::RateLimiter.
132-141: Document and decoupletry_pushfrom frame Debug; clarify rate‑limit bypass.State explicitly that
try_pushis non‑blocking and does not consume rate‑limiter tokens. Drop theF: Debugbound now that DLQ logging avoids payload dumps./// Attempt to push a frame with the given priority and policy. /// + /// This method is non‑blocking and does not consume rate‑limiter tokens. + /// It never waits on capacity or rate and may drop frames according to `policy`. /// # Errors @@ - ) -> Result<(), PushError> - where - F: std::fmt::Debug, - { + ) -> Result<(), PushError> {Also applies to: 168-176
src/push/queues/mod.rs (3)
31-39: Expose the default rate or document it for discoverability.Make the default public and documented, or document it on the builder. Prefer the constant for clarity.
-// Default maximum pushes per second when no custom rate is specified. -// This is an internal implementation detail and may change. -const DEFAULT_PUSH_RATE: usize = 100; +/// Default pushes per second when no custom rate is specified. +pub const DEFAULT_PUSH_RATE: usize = 100; @@ -// Compile-time guard: DEFAULT_PUSH_RATE must not exceed MAX_PUSH_RATE. +// Compile‑time guard: DEFAULT_PUSH_RATE must not exceed MAX_PUSH_RATE. const _: () = assert!(DEFAULT_PUSH_RATE <= MAX_PUSH_RATE);
166-174: Standardise hyphenation: “dead‑letter queue”.Align docs with consistent terminology.
- /// Create queues with a custom rate limit and optional dead letter queue. + /// Create queues with a custom rate limit and optional dead‑letter queue.
208-214: Fix recv() early termination when the high‑priority queue closes.Do not return
Nonewhile the low queue may still yield items.- pub async fn recv(&mut self) -> Option<(PushPriority, F)> { - tokio::select! { - biased; - res = self.high_priority_rx.recv() => res.map(|f| (PushPriority::High, f)), - res = self.low_priority_rx.recv() => res.map(|f| (PushPriority::Low, f)), - } - } + pub async fn recv(&mut self) -> Option<(PushPriority, F)> { + let mut high_closed = false; + let mut low_closed = false; + loop { + tokio::select! { + biased; + res = self.high_priority_rx.recv(), if !high_closed => match res { + Some(f) => return Some((PushPriority::High, f)), + None => high_closed = true, + }, + res = self.low_priority_rx.recv(), if !low_closed => match res { + Some(f) => return Some((PushPriority::Low, f)), + None => low_closed = true, + }, + else => return None, + } + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
docs/asynchronous-outbound-messaging-design.md(1 hunks)docs/efficiency-report.md(4 hunks)docs/hardening-wireframe-a-guide-to-production-resilience.md(1 hunks)src/push/mod.rs(1 hunks)src/push/queues/builder.rs(1 hunks)src/push/queues/handle.rs(1 hunks)src/push/queues/mod.rs(1 hunks)tests/advanced/concurrency_loom.rs(1 hunks)tests/advanced/interaction_fuzz.rs(1 hunks)tests/connection_actor.rs(3 hunks)tests/push.rs(8 hunks)tests/push_policies.rs(4 hunks)tests/wireframe_protocol.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use precise names; boolean names should start with is/has/should
Use en-GB-oxendict spelling and grammar in comments
Function documentation must include clear examples; test documentation should omit redundant examples
Keep code files ≤ 400 lines; split long switch/dispatch logic by feature; move large test data to external files
Disallow Clippy warnings
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions for long functions; adhere to separation of concerns and CQRS
Group related parameters into meaningful structs when functions have many parameters
Consider using Arc for large error returns to reduce data size
Each Rust module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless 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
Do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect(..)] over #[allow(..)] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Files:
src/push/queues/builder.rstests/wireframe_protocol.rstests/advanced/interaction_fuzz.rssrc/push/mod.rssrc/push/queues/handle.rstests/connection_actor.rstests/push_policies.rstests/advanced/concurrency_loom.rstests/push.rssrc/push/queues/mod.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:
src/push/queues/builder.rstests/wireframe_protocol.rstests/advanced/interaction_fuzz.rssrc/push/mod.rssrc/push/queues/handle.rstests/connection_actor.rstests/push_policies.rstests/advanced/concurrency_loom.rstests/push.rssrc/push/queues/mod.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
src/push/queues/builder.rstests/wireframe_protocol.rstests/advanced/interaction_fuzz.rssrc/push/mod.rssrc/push/queues/handle.rstests/connection_actor.rstests/push_policies.rstests/advanced/concurrency_loom.rstests/push.rssrc/push/queues/mod.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/wireframe_protocol.rstests/advanced/interaction_fuzz.rstests/connection_actor.rstests/push_policies.rstests/advanced/concurrency_loom.rstests/push.rs
docs/**/*.md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-oxendict) for documentation text.
The word "outwith" is acceptable in documentation.
Keep US spelling when used in an API, for examplecolor.
Use the Oxford comma in documentation text.
Treat company names as collective nouns in documentation (e.g., "Lille Industries are expanding").
Write headings in sentence case in documentation.
Use Markdown headings (#,##,###, etc.) in order without skipping levels.
Follow markdownlint recommendations for Markdown files.
Provide code blocks and lists using standard Markdown syntax.
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text.
Use-as the first level bullet and renumber lists when items change.
Prefer inline links using[text](url)or angle brackets around the URL; avoid reference-style links like[foo][bar].
Ensure blank lines before and after bulleted lists and fenced blocks in Markdown.
Ensure tables have a delimiter line below the header row in Markdown.
Expand any uncommon acronym on first use, for example, Continuous Integration (CI).
Wrap paragraphs at 80 columns in documentation.
Wrap code at 120 columns in documentation.
Do not wrap tables in documentation.
Use sequentially numbered footnotes referenced with[^1]and place definitions at the end of the file.
Where it adds clarity, include Mermaid diagrams in documentation.
When embedding figures, useand provide concise alt text describing the content.
Add a brief description before each Mermaid diagram in documentation for screen readers.Document examples showing how to deprecate old message versions gracefully
Write the official documentation for the new features. Create separate guides for "Duplex Messaging & Pushes", "Streaming Responses", and "Message Fragmentation". Each guide must include runnable examples and explain the relevant concepts and APIs.
docs/**/*.md: Use docs/ markdown ...
Files:
docs/efficiency-report.mddocs/asynchronous-outbound-messaging-design.mddocs/hardening-wireframe-a-guide-to-production-resilience.md
docs/**/*.{md,rs}
📄 CodeRabbit inference engine (docs/multi-packet-and-streaming-responses-design.md)
docs/**/*.{md,rs}: The official documentation and examples must exclusively use the declarativeResponsemodel for handler responses.
The async-stream pattern must be documented as the canonical approach for dynamic stream generation.
Files:
docs/efficiency-report.mddocs/asynchronous-outbound-messaging-design.mddocs/hardening-wireframe-a-guide-to-production-resilience.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns
Markdown code blocks must be wrapped at 120 columns
Do not wrap tables and headings in Markdown
Use dashes (-) for list bullets in Markdown
Use GitHub-flavoured Markdown footnotes ([^1])
Files:
docs/efficiency-report.mddocs/asynchronous-outbound-messaging-design.mddocs/hardening-wireframe-a-guide-to-production-resilience.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- 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/efficiency-report.mddocs/asynchronous-outbound-messaging-design.mddocs/hardening-wireframe-a-guide-to-production-resilience.md
🧠 Learnings (1)
📚 Learning: 2025-06-29T01:29:15.184Z
Learnt from: leynos
PR: leynos/wireframe#153
File: tests/session_registry.rs:9-9
Timestamp: 2025-06-29T01:29:15.184Z
Learning: In Rust test files using rstest fixtures, `#[allow(unused_braces)]` may be necessary due to a rustc bug that incorrectly flags braces as unnecessary in fixture functions. This is a legitimate use case for lint suppression to work around compiler issues, with active GitHub issues tracking the problem.
Applied to files:
tests/push_policies.rs
🧬 Code graph analysis (10)
src/push/queues/builder.rs (1)
src/push/queues/mod.rs (1)
build_with_rate_dlq(69-109)
tests/wireframe_protocol.rs (3)
tests/connection_actor.rs (1)
queues(26-32)tests/push.rs (1)
queues(18-25)src/push/queues/mod.rs (1)
builder(67-67)
tests/advanced/interaction_fuzz.rs (3)
tests/connection_actor.rs (1)
queues(26-32)tests/push.rs (1)
queues(18-25)src/push/queues/mod.rs (1)
builder(67-67)
src/push/mod.rs (2)
tests/connection_actor.rs (1)
queues(26-32)tests/push.rs (1)
queues(18-25)
src/push/queues/handle.rs (1)
src/push/queues/builder.rs (1)
dlq(69-72)
tests/connection_actor.rs (2)
tests/push.rs (1)
queues(18-25)src/push/queues/mod.rs (1)
builder(67-67)
tests/push_policies.rs (3)
tests/connection_actor.rs (1)
queues(26-32)tests/push.rs (1)
queues(18-25)src/push/queues/mod.rs (1)
builder(67-67)
tests/advanced/concurrency_loom.rs (3)
tests/connection_actor.rs (1)
queues(26-32)tests/push.rs (1)
queues(18-25)src/push/queues/mod.rs (1)
builder(67-67)
tests/push.rs (3)
tests/connection_actor.rs (1)
queues(26-32)src/push/queues/mod.rs (1)
builder(67-67)src/push/queues/handle.rs (1)
push_high_priority(79-81)
src/push/queues/mod.rs (2)
src/push/queues/builder.rs (5)
default(35-42)high_capacity(48-51)low_capacity(55-58)rate(62-65)dlq(69-72)src/push/queues/handle.rs (1)
from_arc(32-32)
🪛 LanguageTool
docs/efficiency-report.md
[typographical] ~29-~29: To join two clauses or introduce examples, consider using an em dash.
Context: ...onversion entirely. Status: ✅ FIXED - Optimized to use freeze().to_vec() whi...
(DASH_RULE)
[typographical] ~42-~42: To join two clauses or introduce examples, consider using an em dash.
Context: ...ent::Shutdown, ``` Impact: Moderate - these clones are necessary for the async...
(DASH_RULE)
[typographical] ~72-~72: To join two clauses or introduce examples, consider using an em dash.
Context: ...0.len()); ``` Impact: Low to medium - depends on registry size and pruning fre...
(DASH_RULE)
🔍 Remote MCP
Summary of additional, review-relevant facts (sourced from the PR context and files in the change):
- New public builder API:
- PushQueues::builder() -> PushQueuesBuilder
- PushQueuesBuilder methods: high_capacity(usize), low_capacity(usize), rate(Option), dlq(Option<mpsc::Sender)), build(self) -> Result<(PushQueues, PushHandle), PushConfigError>
- Builder defaults: high_capacity = 1, low_capacity = 1, rate = Some(DEFAULT_PUSH_RATE), dlq = None
- Legacy constructors deprecated:
- bounded, bounded_no_rate_limit, bounded_with_rate, bounded_with_rate_dlq now deprecated and forward to the builder
- Configuration validation:
- build validates rate (must be between 1 and MAX_PUSH_RATE) and capacity (must be ≥ 1); invalid values return PushConfigError::{InvalidRate, InvalidCapacity}
- Error types and messages:
- PushError: QueueFull ("push queue full"), Closed ("push queue closed")
- PushConfigError: InvalidRate(usize) -> "invalid rate {0}; must be between 1 and {MAX_PUSH_RATE}", InvalidCapacity { high, low } -> "invalid capacities; high={high}, low={low}; each must be ≥ 1"
- Runtime behaviour preserved:
- PushQueues still uses two tokio::mpsc channels (high/low) with recv() biasing high priority and optional leaky_bucket::RateLimiter for per-second rate limiting
- PushHandle implements push_high_priority, push_low_priority, try_push, and downgrade; DLQ routing on drop-if-full behaviours present
- Test impact:
- ~13 test files updated to replace PushQueues::bounded* calls with the builder; build() now returns Result and tests use expect()/unwrap(), surfacing construction errors as panics
- New rstest fixture added in tests/push.rs to centralize builder usage
- API surface additions (public):
- PushQueuesBuilder, PushQueues::builder, PushPriority, PushPolicy, PushQueues, PushHandle, PushError, PushConfigError, MAX_PUSH_RATE, FrameLike trait (blanket impl for T: Send + 'static)
- Miscellaneous PR metadata:
Tool attempts for further external documentation (failed with HTTP 402):
- Attempted searches/reads via documentation tool(s) but requests returned 402 Payment Required; external doc fetches unavailable:,
Sources:
- PR contents and diffs provided in the review context (files under src/push/* and tests/*) — summarized above.
- Tool attempts noted:,
🔇 Additional comments (13)
docs/efficiency-report.md (1)
26-31: One.to_vec()call remains at src/frame/processor.rs:75; decode usessplit_to(len).freeze().to_vec()and still returnsVec<u8>asFrame. No other.to_vec()sites found.tests/advanced/concurrency_loom.rs (1)
24-28: Disable rate limiting in loom to avoid time-coupled flakiness.The builder defaults a non‑zero rate. Under loom, time does not advance normally, which can stall pushes. Disable the limiter for determinism.
- let (queues, handle) = PushQueues::<u8>::builder() - .high_capacity(1) - .low_capacity(1) - .build() + let (queues, handle) = PushQueues::<u8>::builder() + .high_capacity(1) + .low_capacity(1) + .rate(None) + .build() .expect("failed to build push queues");Run:
tests/connection_actor.rs (3)
27-32: LGTM: builder-based fixture with expect()Builder usage and
.expect("failed to build push queues")align with guidelines.
392-396: LGTM: back-pressure test uses builder and expect()Good use of small capacities and a timeout to assert blocking.
483-488: LGTM: graceful shutdown test uses builder and expect()Pattern matches the shared approach across tests and adheres to lint policy.
tests/push.rs (6)
17-25: LGTM: shared queues fixture with expect()Fixture centralises a sane default (capacities + rate) and uses
.expect().
37-42: LGTM: routing test targets minimal capacitiesCorrectly verifies priority order using
recv()pairs.
61-66: LGTM:try_pushpolicy exercisedValidates
ReturnErrorIfFull→QueueFull.
81-86: LGTM: closed-queue error pathsCovers both high and low paths after
queues.close().
175-180: LGTM: unlimited rate path
.rate(None)test confirms absence of rate-induced blocking.
195-201: LGTM: burst semanticsConfirms limiter burst equals configured rate and blocks excess.
src/push/queues/builder.rs (1)
81-84: LGTM: build() validates and delegatesReturn type and error documentation match the constructor contract.
src/push/queues/mod.rs (1)
80-85: Good: reject zero capacities to avoid panic on mpsc::channel(0).This closes the hole flagged earlier and routes misconfiguration to a typed error.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/push.rs (1)
106-135: Deflake the real-time timeout.Increase the 10 ms real-time timeout to 50 ms to absorb scheduler jitter on CI while time is paused.
- time::timeout(Duration::from_millis(10), handle.push_high_priority(2u8)).await + time::timeout(Duration::from_millis(50), handle.push_high_priority(2u8)).await @@ - time::timeout(Duration::from_millis(10), handle.push_low_priority(2u8)).await + time::timeout(Duration::from_millis(50), handle.push_low_priority(2u8)).await
♻️ Duplicate comments (4)
src/push/queues/builder.rs (3)
16-36: Add a full end-to-end example (capacities, rate, DLQ).Provide a runnable docs example that demonstrates all knobs as required by the guidelines.
/// /// # Examples /// -/// ``` -/// use wireframe::push::PushQueues; -/// -/// let (_queues, _handle) = PushQueues::<u8>::builder() -/// .build() -/// .expect("failed to build push queues"); -/// # drop((_queues, _handle)); -/// ``` +/// ```rust,no_run +/// use tokio::sync::mpsc; +/// use wireframe::push::PushQueues; +/// +/// # async fn demo() { +/// let (dlq_tx, _dlq_rx) = mpsc::channel(8); +/// let (queues, handle) = PushQueues::<u8>::builder() +/// .high_capacity(8) +/// .low_capacity(8) +/// .rate(Some(100)) // set pushes-per-second; use `None` to disable +/// .dlq(Some(dlq_tx)) // send dropped frames to DLQ +/// .build() +/// .expect("failed to build push queues"); +/// # let _ = (queues, handle); +/// # } +/// ```
71-76: Clarifyrate(None)semantics.State explicitly that
Nonedisables rate limiting.- /// Set the global push rate limit in pushes per second. + /// Set the global push rate limit in pushes per second. Pass `None` to disable rate limiting.
78-83: Clarify DLQ behaviour when absent or full.Document drop behaviour when DLQ is
Noneor saturated.- /// Provide a dead-letter queue for discarded frames. + /// Provide a dead-letter queue for frames that cannot be enqueued (e.g., queue full). + /// If the DLQ is `None` or the DLQ is full, the frame is dropped.tests/push.rs (1)
28-35: Add capacity validation coverage.Test
InvalidCapacityfor zero high/low capacities to cover all builder validations.fn builder_rejects_invalid_rate(#[case] rate: usize) { let result = PushQueues::<u8>::builder().rate(Some(rate)).build(); assert!(matches!(result, Err(PushConfigError::InvalidRate(r)) if r == rate)); } + +/// Builder rejects zero capacities. +#[test] +fn builder_rejects_zero_capacity() { + let hi = PushQueues::<u8>::builder().high_capacity(0).build(); + assert!(matches!(hi, Err(PushConfigError::InvalidCapacity { high: 0, low: 1 }))); + let lo = PushQueues::<u8>::builder().low_capacity(0).build(); + assert!(matches!(lo, Err(PushConfigError::InvalidCapacity { high: 1, low: 0 }))); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/push/queues/builder.rs(1 hunks)tests/push.rs(8 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use precise names; boolean names should start with is/has/should
Use en-GB-oxendict spelling and grammar in comments
Function documentation must include clear examples; test documentation should omit redundant examples
Keep code files ≤ 400 lines; split long switch/dispatch logic by feature; move large test data to external files
Disallow Clippy warnings
Fix warnings emitted during tests in code rather than silencing them
Extract helper functions for long functions; adhere to separation of concerns and CQRS
Group related parameters into meaningful structs when functions have many parameters
Consider using Arc for large error returns to reduce data size
Each Rust module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless 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
Do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason
Prefer #[expect(..)] over #[allow(..)] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Files:
tests/push.rssrc/push/queues/builder.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:
tests/push.rssrc/push/queues/builder.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
tests/push.rssrc/push/queues/builder.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/push.rs
🧬 Code graph analysis (2)
tests/push.rs (4)
tests/connection_actor.rs (1)
queues(26-32)src/push/queues/mod.rs (1)
builder(67-67)src/push/queues/builder.rs (1)
rate(73-76)src/push/queues/handle.rs (1)
push_high_priority(79-81)
src/push/queues/builder.rs (1)
src/push/queues/mod.rs (1)
build_with_rate_dlq(69-109)
🔍 Remote MCP
Relevant facts to aid review (concise):
- New builder API:
- PushQueues::::builder() -> PushQueuesBuilder
- PushQueuesBuilder methods: high_capacity(usize), low_capacity(usize), rate(Option), dlq(Option<mpsc::Sender>), build() -> Result<(PushQueues, PushHandle), PushConfigError>
- Builder defaults: high=1, low=1, rate=Some(DEFAULT_PUSH_RATE), dlq=None
- Deprecated/removed direct constructors:
- bounded, bounded_no_rate_limit, bounded_with_rate, bounded_with_rate_dlq are deprecated and forward to the builder
- Validation and errors:
- build() validates capacities (≥1) and rate (1..=MAX_PUSH_RATE); errors return PushConfigError::InvalidCapacity or PushConfigError::InvalidRate(rate)
- Error types: PushError::{QueueFull, Closed} (with messages), PushConfigError::{InvalidRate(usize), InvalidCapacity{high,low}}
- Public constant MAX_PUSH_RATE = 10_000 (default internal rate = 100)
- Runtime behavior preserved:
- Two tokio::mpsc channels (high/low), recv() biases high priority; optional leaky_bucket::RateLimiter; optional DLQ with try_send on drop
- PushHandle provides push_high_priority, push_low_priority (async), try_push with PushPolicy handling, and downgrade() to Weak
- Test and docs impact:
- ~13 tests updated to use builder; build() now returns Result and tests call expect()/unwrap() (may panic if builder validation fails)
- New test added: builder_rejects_invalid_rate asserting InvalidRate for 0 and MAX_PUSH_RATE+1
- Docs updated to reflect builder usage and deprecations
- API surface changes to review:
- Added public: PushQueuesBuilder, PushConfigError, PushError, PushHandle (re-exported), PushPriority, PushPolicy, MAX_PUSH_RATE, FrameLike trait (blanket impl for T: Send + 'static)
- Ensure deprecated constructors are properly annotated and forwarded to builder, and public docs/examples updated accordingly
- Review focal points:
- Correctness of validation logic and error messages (boundary values for rate and capacity)
- Deprecation hygiene (attributes, forwarding implementations, docs)
- No behavioural regressions: DLQ routing, rate-limiting integration, recv() prioritisation, and PushHandle semantics
- Tests: ensure expect() usage is intentional (will panic on invalid config) and builder_rejects_invalid_rate covers edge cases
Sources: PR contents and diffs provided in the review context.
🔇 Additional comments (8)
tests/push.rs (8)
5-5: Imports and public API usage look good.Keep the imports explicit and local to the tests module.
Also applies to: 7-15
18-26: Good use of an rstest fixture.Centralise common configuration; default to a low rate to exercise the limiter.
40-56: Priority routing test reads well.Assert high-priority bias first; capacities of 1/1 keep the scenario tight.
64-79: Policy handling is correct.Verify QueueFull, drain, then accept; this exercises the fast path.
84-97: Closed-queue error paths are validated.Close, then assert Closed on both priorities; solid.
151-172: Shared limiter behaviour is asserted correctly.Block across priorities, advance, then succeed; ordering checks are apt.
176-191: Unlimited queues test is crisp.
rate(None)path is covered and non-blocking behaviour is asserted.
196-222: Burst and window semantics are well exercised.Push at rate capacity, assert block, advance, and drain in order.
|
@coderabbitai pause |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
✅ Actions performedReviews paused. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Gates Failed
(1 file with Code Duplication, Complex Conditional)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| mod.rs | 2 advisory rules | 10.00 → 9.10 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
|
Superseded by #338 |
1 participant
Summary
PushQueuesBuilderwith configuration settersbounded*constructors and exposePushQueues::builderTesting
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_68aeba8d630c8322bcab55de80b2ef06
Summary by Sourcery
Introduce a builder pattern for PushQueues to configure capacities, rate limiting, and an optional dead-letter queue; deprecate existing bounded constructors in favor of the builder and update tests accordingly.
New Features:
Enhancements:
Documentation:
Tests: