Test invalid builder rates #338
Conversation
Reviewer's GuideThis PR replaces the old PushQueues constructors with a fluent PushQueuesBuilder API, adds strict validation and error handling for rate and capacity settings, updates documentation and examples accordingly, and refactors all tests to exercise the new builder interface while covering invalid configuration cases. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Note Reviews pausedUse the following commands to manage reviews:
Summary by CodeRabbit
WalkthroughIntroduce a builder-based API for prioritised push queues, replacing direct constructors and restructuring the push subsystem into modular components. Add error types, handle implementation, and queue orchestration with optional rate limiting and DLQ. Update documentation and tests to use the builder and adjust minor test linting and messages. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant PQ as PushQueues::<F>
participant B as PushQueuesBuilder::<F>
participant H as PushHandle::<F>
rect rgba(220,235,255,0.4)
note over Caller,PQ: Build time
Caller->>PQ: builder()
PQ-->>Caller: PushQueuesBuilder
Caller->>B: .high_capacity(h).low_capacity(l).rate(opt).dlq(opt).build()
B->>PQ: build_with_rate_dlq(h, l, rate, dlq)
PQ-->>Caller: (PushQueues, PushHandle) or PushConfigError
end
sequenceDiagram
autonumber
actor Producer
participant H as PushHandle
participant Lim as RateLimiter
participant HQ as high_prio_tx
participant LQ as low_prio_tx
participant DLQ as dlq_tx
participant Q as PushQueues.recv()
rect rgba(235,255,235,0.5)
note over Producer,Q: Push flow
alt async push
Producer->>H: push_high_priority(frame)
opt rate limiting
H->>Lim: acquire(1)
Lim-->>H: permit
end
H->>HQ: send(frame)
HQ-->>H: Ok / Closed
H-->>Producer: Ok or PushError::Closed
else try_push (non-blocking)
Producer->>H: try_push(frame, priority, policy)
H->>HQ: try_send(...) or LQ: try_send(...)
alt Full
opt policy routes to DLQ
H->>DLQ: try_send(frame)
DLQ-->>H: Ok / Full / Closed
end
H-->>Producer: Ok or PushError::QueueFull
else Closed
H-->>Producer: PushError::Closed
else Sent
H-->>Producer: Ok
end
end
end
rect rgba(255,245,220,0.5)
note over Q: Receive flow (priority-biased)
loop until both queues closed and empty
Q->>HQ: try_recv()
alt got High
HQ-->>Q: (High, frame)
else none
Q->>LQ: recv()
LQ-->>Q: (Low, frame) or None
end
end
end
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.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
src/push/queues/mod.rs
Outdated
| if let Some(r) = rate | ||
| && (r == 0 || r > MAX_PUSH_RATE) |
There was a problem hiding this comment.
🧩 Analysis chain
@leynos I'll analyze the complex conditional and provide a fix with an AI agent prompt.
Based on the complex conditional flagged by CodeScene, here's my suggested fix:
Fix for Complex Conditional
Extract the rate validation logic into a predicate function to reduce complexity and improve readability:
/// Validates if the provided rate is invalid (None, 0, or exceeds MAX_PUSH_RATE).
fn is_invalid_rate(rate: Option<usize>) -> bool {
match rate {
Some(r) => r == 0 || r > MAX_PUSH_RATE,
None => false,
}
}
pub(super) fn build_with_rate_dlq(
high_capacity: usize,
low_capacity: usize,
rate: Option<usize>,
dlq: Option<mpsc::Sender<F>>,
) -> Result<(Self, PushHandle<F>), PushConfigError> {
if is_invalid_rate(rate) {
// ... rest of the validation logic
}
// ... rest of the function
}AI Agent Prompt
Refactor the complex conditional in the `build_with_rate_dlq` function in `src/push/queues/mod.rs`:
1. Create a new private function `is_invalid_rate(rate: Option<usize>) -> bool` that:
- Returns `false` if rate is `None`
- Returns `true` if rate is `Some(r)` where `r == 0 || r > MAX_PUSH_RATE`
- Add a doc comment explaining it validates if the rate is invalid
2. Replace the existing complex conditional `if let Some(r) = rate && (r == 0 || r > MAX_PUSH_RATE)` with `if is_invalid_rate(rate)`
3. Place the new function before the `build_with_rate_dlq` function in the same impl block
4. Ensure the function follows the coding guidelines:
- Use en-GB spelling in documentation
- Place any attributes after doc comments
- Keep the function concise and focused on single responsibility
The goal is to reduce the cyclomatic complexity by extracting the validation logic into a dedicated predicate function.
This refactoring separates the validation concern into its own function, making the main logic clearer and reducing the complexity of the conditional statement.
🏁 Script executed:
#!/bin/bash
# Get more context around the complex conditional to understand the full function
rg -n -A 10 -B 5 "build_with_rate_dlq" src/push/queues/mod.rs
Length of output: 1416
Perfect! Now I can see the complete context. Here's my refined fix and AI agent prompt:
Fix for Complex Conditional
Extract the rate validation logic into a predicate function within the same impl block:
impl<F: FrameLike> PushQueues<F> {
/// Start building a new set of push queues.
#[must_use]
pub fn builder() -> PushQueuesBuilder<F> { PushQueuesBuilder::default() }
/// Validates if the provided rate is invalid (zero or exceeds maximum).
fn is_invalid_rate(rate: Option<usize>) -> bool {
matches!(rate, Some(r) if r == 0 || r > MAX_PUSH_RATE)
}
pub(super) fn build_with_rate_dlq(
high_capacity: usize,
low_capacity: usize,
rate: Option<usize>,
dlq: Option<mpsc::Sender<F>>,
) -> Result<(Self, PushHandle<F>), PushConfigError> {
if Self::is_invalid_rate(rate) {
// Reject unsupported rates early to avoid building queues that cannot
// be used. The bounds prevent runaway resource consumption.
// ... rest of validation logic
}
// ... rest of function
}
}AI Agent Prompt
Refactor the complex conditional in the `build_with_rate_dlq` function in `src/push/queues/mod.rs` at line 75-77:
1. Add a new private helper function within the `impl<F: FrameLike> PushQueues<F>` block, positioned after the `builder()` function and before `build_with_rate_dlq()`:
```rust
/// Validates if the provided rate is invalid (zero or exceeds maximum).
fn is_invalid_rate(rate: Option<usize>) -> bool {
matches!(rate, Some(r) if r == 0 || r > MAX_PUSH_RATE)
}
```
2. Replace the existing complex conditional on lines 75-77:
- FROM: `if let Some(r) = rate && (r == 0 || r > MAX_PUSH_RATE)`
- TO: `if Self::is_invalid_rate(rate)`
3. Keep the existing comment block that follows the conditional unchanged, as it explains the validation purpose.
4. Ensure the new function follows Rust coding guidelines:
- Use `matches!` macro for concise pattern matching
- Include a clear doc comment with en-GB spelling
- Make it a private function (no `pub` keyword)
This refactoring reduces the cyclomatic complexity by extracting the validation logic into a dedicated predicate function, making the main conditional simpler and more readable.
This solution uses the matches! macro for cleaner pattern matching and keeps the validation logic encapsulated while simplifying the main conditional structure.
There was a problem hiding this comment.
Actionable comments posted: 30
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/push.rs (3)
137-165: Fix: timeout under paused time never fires (test will hang)With time::pause(), timeout(...) won’t elapse unless you advance time. Build the timeout future first, advance virtual time, then await it.
Apply:
- let attempt = match priority { - PushPriority::High => { - time::timeout(Duration::from_millis(10), handle.push_high_priority(2u8)).await - } - PushPriority::Low => { - time::timeout(Duration::from_millis(10), handle.push_low_priority(2u8)).await - } - }; - - assert!(attempt.is_err(), "second push should block"); + let attempt = match priority { + PushPriority::High => time::timeout(Duration::from_millis(10), handle.push_high_priority(2u8)), + PushPriority::Low => time::timeout(Duration::from_millis(10), handle.push_low_priority(2u8)), + }; + time::advance(Duration::from_millis(10)).await; + assert!(attempt.await.is_err(), "second push should block");
186-202: Fix: same paused-time timeout issueAdvance time before awaiting the timeout.
Apply:
- let attempt = time::timeout(Duration::from_millis(10), handle.push_low_priority(2u8)).await; - assert!(attempt.is_err(), "second push should block across queues"); + let attempt = time::timeout(Duration::from_millis(10), handle.push_low_priority(2u8)); + time::advance(Duration::from_millis(10)).await; + assert!(attempt.await.is_err(), "second push should block across queues");
239-244: Fix: paused-time timeout againAdvance virtual time before awaiting the timeout.
Apply:
- let res = time::timeout(Duration::from_millis(10), handle.push_high_priority(99)).await; - assert!( - res.is_err(), + let res = time::timeout(Duration::from_millis(10), handle.push_high_priority(99)); + time::advance(Duration::from_millis(10)).await; + assert!( + res.await.is_err(), "Push exceeding burst capacity should be rate limited" );
📜 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 (23)
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/connection.rs(2 hunks)src/push.rs(0 hunks)src/push/mod.rs(1 hunks)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)src/session.rs(6 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/server.rs(1 hunks)tests/session_registry.rs(1 hunks)tests/stream_end.rs(3 hunks)tests/wireframe_protocol.rs(2 hunks)tests/world.rs(2 hunks)
💤 Files with no reviewable changes (1)
- src/push.rs
🧰 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:
tests/server.rssrc/push/mod.rssrc/push/queues/errors.rssrc/connection.rssrc/session.rstests/advanced/concurrency_loom.rstests/connection_actor.rstests/stream_end.rstests/session_registry.rssrc/push/queues/handle.rstests/world.rssrc/push/queues/builder.rstests/push.rstests/correlation_id.rstests/push_policies.rstests/advanced/interaction_fuzz.rstests/wireframe_protocol.rstests/async_stream.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/server.rssrc/push/mod.rssrc/push/queues/errors.rssrc/connection.rssrc/session.rstests/advanced/concurrency_loom.rstests/connection_actor.rstests/stream_end.rstests/session_registry.rssrc/push/queues/handle.rstests/world.rssrc/push/queues/builder.rstests/push.rstests/correlation_id.rstests/push_policies.rstests/advanced/interaction_fuzz.rstests/wireframe_protocol.rstests/async_stream.rssrc/push/queues/mod.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
tests/server.rssrc/push/mod.rssrc/push/queues/errors.rssrc/connection.rssrc/session.rstests/advanced/concurrency_loom.rstests/connection_actor.rstests/stream_end.rstests/session_registry.rssrc/push/queues/handle.rstests/world.rssrc/push/queues/builder.rstests/push.rstests/correlation_id.rstests/push_policies.rstests/advanced/interaction_fuzz.rstests/wireframe_protocol.rstests/async_stream.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/server.rstests/advanced/concurrency_loom.rstests/connection_actor.rstests/stream_end.rstests/session_registry.rstests/world.rstests/push.rstests/correlation_id.rstests/push_policies.rstests/advanced/interaction_fuzz.rstests/wireframe_protocol.rstests/async_stream.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/asynchronous-outbound-messaging-design.mddocs/efficiency-report.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/asynchronous-outbound-messaging-design.mddocs/efficiency-report.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/asynchronous-outbound-messaging-design.mddocs/efficiency-report.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/asynchronous-outbound-messaging-design.mddocs/efficiency-report.mddocs/hardening-wireframe-a-guide-to-production-resilience.md
🧬 Code graph analysis (15)
src/push/mod.rs (3)
tests/connection_actor.rs (1)
queues(28-34)tests/push.rs (1)
queues(19-26)tests/stream_end.rs (1)
queues(19-25)
tests/advanced/concurrency_loom.rs (4)
tests/connection_actor.rs (1)
queues(28-34)tests/push.rs (1)
queues(19-26)tests/stream_end.rs (1)
queues(19-25)src/push/queues/mod.rs (1)
builder(67-67)
tests/connection_actor.rs (3)
tests/push.rs (1)
queues(19-26)tests/stream_end.rs (1)
queues(19-25)src/push/queues/mod.rs (1)
builder(67-67)
tests/stream_end.rs (2)
tests/connection_actor.rs (1)
queues(28-34)tests/push.rs (1)
queues(19-26)
tests/session_registry.rs (2)
src/push/queues/builder.rs (1)
default(55-62)src/push/queues/mod.rs (1)
builder(67-67)
src/push/queues/handle.rs (1)
src/push/queues/builder.rs (1)
dlq(95-98)
tests/world.rs (4)
tests/connection_actor.rs (1)
queues(28-34)tests/push.rs (1)
queues(19-26)tests/stream_end.rs (1)
queues(19-25)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-111)
tests/push.rs (5)
tests/connection_actor.rs (1)
queues(28-34)tests/stream_end.rs (1)
queues(19-25)src/push/queues/mod.rs (1)
builder(67-67)src/push/queues/builder.rs (4)
rate(86-89)high_capacity(68-72)build(107-109)low_capacity(76-80)src/push/queues/handle.rs (1)
push_high_priority(79-81)
tests/correlation_id.rs (4)
tests/connection_actor.rs (1)
queues(28-34)tests/push.rs (1)
queues(19-26)tests/stream_end.rs (1)
queues(19-25)src/push/queues/mod.rs (1)
builder(67-67)
tests/push_policies.rs (4)
tests/connection_actor.rs (1)
queues(28-34)tests/push.rs (1)
queues(19-26)tests/stream_end.rs (1)
queues(19-25)src/push/queues/mod.rs (1)
builder(67-67)
tests/advanced/interaction_fuzz.rs (4)
tests/connection_actor.rs (1)
queues(28-34)tests/push.rs (1)
queues(19-26)tests/stream_end.rs (1)
queues(19-25)src/push/queues/mod.rs (1)
builder(67-67)
tests/wireframe_protocol.rs (4)
tests/connection_actor.rs (1)
queues(28-34)tests/push.rs (1)
queues(19-26)tests/stream_end.rs (1)
queues(19-25)src/push/queues/mod.rs (1)
builder(67-67)
tests/async_stream.rs (4)
tests/connection_actor.rs (1)
queues(28-34)tests/push.rs (1)
queues(19-26)tests/stream_end.rs (1)
queues(19-25)src/push/queues/mod.rs (1)
builder(67-67)
src/push/queues/mod.rs (2)
src/push/queues/builder.rs (5)
default(55-62)high_capacity(68-72)low_capacity(76-80)rate(86-89)dlq(95-98)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
Relevant metadata from Pull Request #338 in leynos/wireframe:
-
Title & branches: “Test invalid builder rates” (#338) by leynos, merging 1 commit from branch codex/introduce-pushqueuesbuilder-with-deprecation-bmrpqd into main.
-
Scope of changes: 1 commit with +944 additions, −562 deletions across 23 files; the PR currently has 12 conversation comments and 4 checks.
-
Commit details: single commit SHA 74c4d75 titled “Refactor deprecated queue constructors,” authored by leynos on Aug 30, 2025.
-
External task link: includes reference to 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 (23)
docs/hardening-wireframe-a-guide-to-production-resilience.md (1)
256-270: LGTM: builder example accurately shows rate(None) disabling throttling.The example uses the builder API correctly, returns a Result, and explains the unlimited path. No action required.
src/session.rs (1)
86-89: Remove unnecessary capacity specification note: builder defaults to high_capacity = 1 and low_capacity = 1, soPushQueues::<u8>::builder().build()compiles as-is.src/push/queues/errors.rs (1)
1-30: Error types look solid and aligned with the builder API.
#[non_exhaustive]usage, messages, and separation between push-time and config-time errors are appropriate.tests/connection_actor.rs (2)
398-410: Backpressure check reads well; ensure deterministic timing on slow CIThe 50ms timeout is small; bump to 100–200ms to reduce flakes on slow runners.
Do you want a commit to raise the timeout and rerun CI?
489-494: LGTM: Builder usage in loopThe switch to the builder is correct and keeps each actor isolated.
src/push/mod.rs (1)
21-31: LGTM: Re-exports and crate-only internalsPublic surface and crate-visible internals are appropriately split.
tests/push.rs (12)
5-15: LGTM: Imports reflect the new public APINew names and constants are correctly referenced.
18-26: LGTM: Builder-based fixturesFixtures align with the builder API and explicit generics.
Also applies to: 28-35
37-44: LGTM: Invalid rate coverageThe test correctly exercises 0 and MAX+1.
46-79: LGTM: Zero-capacity paths (debug/release)Assertions match debug_assert behaviour and release-time validation.
81-97: LGTM: Priority routing sanity-checkAssertions match expected ordering.
98-116: LGTM: Policy returns QueueFullVariant and behaviour match the new API.
120-127: LGTM: Closed-state errorsBoth priorities surface Closed as expected.
171-179: LGTM: Rate reset after windowVirtual time usage is correct.
208-214: LGTM: Unlimited rate pathrate(None) behaves as expected.
228-234: LGTM: Burst config (cap=4, rate=3)Config matches the test’s intent.
248-252: LGTM: Dequeue order after refillExpectations align with rate-limited burst semantics.
137-165: The script will list anytime::pause()andtime::timeoutusages intests/push.rsso you can confirm that every timeout under a paused clock is intended and paired with a subsequenttime::advance. After seeing the output, verify there are no unintentional blocking tests.tests/push_policies.rs (1)
42-47: LGTM: Builder replaces deprecated constructors (with DLQ/rate where needed)Conversions look correct and explicit generics aid clarity.
Also applies to: 85-91, 149-155
src/push/queues/builder.rs (2)
1-46: LGTM: Clear module docs and examplesDocs are thorough and use rust,no_run appropriately.
54-63: LGTM: Defaults, validation, and Result-returning build
- Defaults mirror prior constructors.
- Debug assertions complement release-time validation in build().
- API shape matches tests and exports.
Also applies to: 65-81, 82-99, 100-109
src/push/queues/handle.rs (1)
34-49: Core push path looks correct and cancellation-safeAwait the limiter, pick the correct channel, map send failures to PushError::Closed, and emit debug logs. No correctness issues here.
src/push/queues/mod.rs (1)
217-234: LGTM on biased recv loopBias towards high priority is explicit and documented; closed-channel handling is correct.
| +builder(): PushQueuesBuilder~F~ | ||
| +recv(): Option<(PushPriority, F)> | ||
| } | ||
| class PushQueuesBuilder~F~ { | ||
| +high_capacity(cap: usize): PushQueuesBuilder~F~ | ||
| +low_capacity(cap: usize): PushQueuesBuilder~F~ | ||
| +rate(rate: Option<usize>): PushQueuesBuilder~F~ | ||
| +dlq(sender: Option<mpsc::Sender<F>>): PushQueuesBuilder~F~ | ||
| +build(): (PushQueues~F~, PushHandle~F~) | ||
| } |
There was a problem hiding this comment.
Fix builder signature to return Result in public API diagram.
Docs elsewhere and code use build() -> Result<(PushQueues, PushHandle), PushConfigError>. Keep the surface consistent here.
- class PushQueuesBuilder~F~ {
- +high_capacity(cap: usize): PushQueuesBuilder~F~
- +low_capacity(cap: usize): PushQueuesBuilder~F~
- +rate(rate: Option<usize>): PushQueuesBuilder~F~
- +dlq(sender: Option<mpsc::Sender<F>>): PushQueuesBuilder~F~
- +build(): (PushQueues~F~, PushHandle~F~)
- }
+ class PushQueuesBuilder~F~ {
+ +high_capacity(cap: usize): PushQueuesBuilder~F~
+ +low_capacity(cap: usize): PushQueuesBuilder~F~
+ +rate(rate: Option<usize>): PushQueuesBuilder~F~
+ +dlq(sender: Option<mpsc::Sender<F>>): PushQueuesBuilder~F~
+ +build(): Result<(PushQueues~F~, PushHandle~F~), PushConfigError>
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| +builder(): PushQueuesBuilder~F~ | |
| +recv(): Option<(PushPriority, F)> | |
| } | |
| class PushQueuesBuilder~F~ { | |
| +high_capacity(cap: usize): PushQueuesBuilder~F~ | |
| +low_capacity(cap: usize): PushQueuesBuilder~F~ | |
| +rate(rate: Option<usize>): PushQueuesBuilder~F~ | |
| +dlq(sender: Option<mpsc::Sender<F>>): PushQueuesBuilder~F~ | |
| +build(): (PushQueues~F~, PushHandle~F~) | |
| } | |
| builder(): PushQueuesBuilder~F~ | |
| recv(): Option<(PushPriority, F)> | |
| } | |
| class PushQueuesBuilder~F~ { | |
| high_capacity(cap: usize): PushQueuesBuilder~F~ | |
| low_capacity(cap: usize): PushQueuesBuilder~F~ | |
| rate(rate: Option<usize>): PushQueuesBuilder~F~ | |
| dlq(sender: Option<mpsc::Sender<F>>): PushQueuesBuilder~F~ | |
| build(): Result<(PushQueues~F~, PushHandle~F~), PushConfigError> | |
| } |
🤖 Prompt for AI Agents
In docs/asynchronous-outbound-messaging-design.md around lines 356 to 365 the
PushQueuesBuilder build() signature in the public API diagram incorrectly
returns (PushQueues~F~, PushHandle~F~); update it to return
Result<(PushQueues~F~, PushHandle~F~), PushConfigError> so the diagram matches
the rest of the docs and code; keep the generic and error type exactly as used
elsewhere (PushConfigError) and update any adjacent text if it repeats the old
signature.
docs/efficiency-report.md
Outdated
| **Impact**: Moderate - these clones are necessary for the async select pattern | ||
| but could be optimized in some cases. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Replace spaced hyphens with em dashes.
Improve readability and follow typographic best practice flagged by tooling.
-**Impact**: Moderate - these clones are necessary for the async select pattern
+**Impact**: Moderate—these clones are necessary for the async select pattern-**Impact**: Low to medium - depends on registry size and pruning frequency.
+**Impact**: Low to medium—depends on registry size and pruning frequency.Also applies to: 72-73
🧰 Tools
🪛 LanguageTool
[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)
🤖 Prompt for AI Agents
In docs/efficiency-report.md around lines 42-43 (and also apply same change at
72-73), replace the spaced hyphens used as separators (" - ") with proper em
dashes (—) without surrounding spaces so phrases like "Impact: Moderate - these
clones..." become "Impact: Moderate — these clones..."; update each occurrence
accordingly to use typographically correct em dashes and remove the extra spaces
around them.
| /// let (queues, handle) = PushQueues::<u8>::builder() | ||
| /// .high_capacity(4) | ||
| /// .low_capacity(4) | ||
| /// .build() | ||
| /// .expect("failed to build PushQueues"); | ||
| /// let token = CancellationToken::new(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mark the doctest as no_run for consistency and faster docs.
Prevent unintended execution in docs and match the prior example style.
- /// ```
+ /// ```no_run
/// use tokio_util::sync::CancellationToken;
/// use wireframe::{connection::ConnectionActor, push::PushQueues};
///
/// let (queues, handle) = PushQueues::<u8>::builder()🤖 Prompt for AI Agents
In src/connection.rs around lines 136 to 141, the documentation example should
be marked as a non-executing doctest for consistency and faster docs; update the
code block to use the no_run doctest tag (i.e., add the no_run fence/attribute
to the opening triple-backtick of the example) so the example is not executed by
rustdoc and matches the prior example style.
| //! use wireframe::push::PushQueues; | ||
| //! | ||
| //! let (queues, handle) = PushQueues::<u8>::builder() | ||
| //! .high_capacity(8) | ||
| //! .low_capacity(8) | ||
| //! .build() | ||
| //! .expect("failed to build PushQueues"); | ||
| //! # drop((queues, handle)); | ||
| //! ``` |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use rustdoc code-fence language for the example
Annotate the fence as rust,no_run so it’s tested/highlighted by rustdoc.
Apply:
-//! ```no_run
+//! ```rust,no_run
//! use wireframe::push::PushQueues;
@@
-//! ```
+//! ```🤖 Prompt for AI Agents
In src/push/mod.rs around lines 9 to 17, the rustdoc example fence is not
annotated; update the opening code fence comment from "//! ```" to "//!
```rust,no_run" so the block is tested and syntax-highlighted by rustdoc,
leaving the closing "//! ```" unchanged and ensure spacing/punctuation matches
existing comment style.
| }); | ||
|
|
||
| let (queues, handle) = PushQueues::bounded(1, 1); | ||
| let (queues, handle) = queues(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid name shadowing for readability.
Apply the same rename here.
- let (queues, handle) = queues();
+ let (push_queues, handle) = queues();
+ let queues = push_queues;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let (queues, handle) = queues(); | |
| let (push_queues, handle) = queues(); | |
| let queues = push_queues; |
🤖 Prompt for AI Agents
In tests/stream_end.rs around line 63, the local binding let (queues, handle) =
queues(); shadows an existing name; rename the local variable to the same
non-shadowing identifier used in the earlier refactor (i.e., replace the first
'queues' with the new name used elsewhere in the codebase) so it no longer
shadows the outer definition and update any subsequent usages in this test
accordingly.
tests/wireframe_protocol.rs
Outdated
| let (queues, handle) = PushQueues::<Vec<u8>>::builder() | ||
| .high_capacity(1) | ||
| .low_capacity(1) | ||
| .build() | ||
| .expect("failed to build PushQueues"); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Disable throttling in tests.
Turn off rate limiting to remove time-coupling and match the docs note that None disables throttling.
let (queues, handle) = PushQueues::<Vec<u8>>::builder()
.high_capacity(1)
.low_capacity(1)
+ .rate(None)
.build()
.expect("failed to build PushQueues");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let (queues, handle) = PushQueues::<Vec<u8>>::builder() | |
| .high_capacity(1) | |
| .low_capacity(1) | |
| .build() | |
| .expect("failed to build PushQueues"); | |
| let (queues, handle) = PushQueues::<Vec<u8>>::builder() | |
| .high_capacity(1) | |
| .low_capacity(1) | |
| .rate(None) | |
| .build() | |
| .expect("failed to build PushQueues"); |
🤖 Prompt for AI Agents
In tests/wireframe_protocol.rs around lines 59 to 63, the PushQueues builder
currently sets capacities but leaves rate limiting enabled; update the builder
invocation to disable throttling per the docs by setting the throttle to None
(or calling the builder method that disables throttling), so tests are not
time-coupled; ensure the builder chain includes the throttle-disabling call
before build() and keep the expect() error handling as-is.
tests/wireframe_protocol.rs
Outdated
| let (queues, handle) = PushQueues::<Vec<u8>>::builder() | ||
| .high_capacity(8) | ||
| .low_capacity(8) | ||
| .build() | ||
| .expect("failed to build PushQueues"); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Disable throttling in tests.
Apply the same change here.
let (queues, handle) = PushQueues::<Vec<u8>>::builder()
.high_capacity(8)
.low_capacity(8)
+ .rate(None)
.build()
.expect("failed to build PushQueues");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let (queues, handle) = PushQueues::<Vec<u8>>::builder() | |
| .high_capacity(8) | |
| .low_capacity(8) | |
| .build() | |
| .expect("failed to build PushQueues"); | |
| let (queues, handle) = PushQueues::<Vec<u8>>::builder() | |
| .high_capacity(8) | |
| .low_capacity(8) | |
| .rate(None) | |
| .build() | |
| .expect("failed to build PushQueues"); |
🤖 Prompt for AI Agents
In tests/wireframe_protocol.rs around lines 87 to 91, the PushQueues builder
needs throttling disabled for tests; update the builder chain to disable
throttling by calling the builder's disable_throttling() method (i.e., add
.disable_throttling() into the chain before .build()) so the test uses
unthrottled queues.
tests/world.rs
Outdated
| let (queues, handle) = PushQueues::<Envelope>::builder() | ||
| .high_capacity(1) | ||
| .low_capacity(1) | ||
| .build() | ||
| .expect("failed to build PushQueues"); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Be explicit about unlimited rate to prevent future flakiness.
Set rate(None) to lock in unthrottled behaviour for this world step.
let (queues, handle) = PushQueues::<Envelope>::builder()
.high_capacity(1)
.low_capacity(1)
+ .rate(None)
.build()
.expect("failed to build PushQueues");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let (queues, handle) = PushQueues::<Envelope>::builder() | |
| .high_capacity(1) | |
| .low_capacity(1) | |
| .build() | |
| .expect("failed to build PushQueues"); | |
| let (queues, handle) = PushQueues::<Envelope>::builder() | |
| .high_capacity(1) | |
| .low_capacity(1) | |
| .rate(None) | |
| .build() | |
| .expect("failed to build PushQueues"); |
🤖 Prompt for AI Agents
In tests/world.rs around lines 145 to 149, the PushQueues builder does not
explicitly set rate, risking future flakiness; call .rate(None) on the builder
chain before .build() to lock in unthrottled (unlimited) behavior, then keep the
existing .expect(...) handling.
tests/world.rs
Outdated
| let (queues, handle) = PushQueues::<u8>::builder() | ||
| .high_capacity(1) | ||
| .low_capacity(1) | ||
| .build() | ||
| .expect("failed to build PushQueues"); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Repeat: disable throttling explicitly here as well.
Mirror the above change for consistency.
let (queues, handle) = PushQueues::<u8>::builder()
.high_capacity(1)
.low_capacity(1)
+ .rate(None)
.build()
.expect("failed to build PushQueues");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let (queues, handle) = PushQueues::<u8>::builder() | |
| .high_capacity(1) | |
| .low_capacity(1) | |
| .build() | |
| .expect("failed to build PushQueues"); | |
| let (queues, handle) = PushQueues::<u8>::builder() | |
| .high_capacity(1) | |
| .low_capacity(1) | |
| .rate(None) | |
| .build() | |
| .expect("failed to build PushQueues"); |
🤖 Prompt for AI Agents
In tests/world.rs around lines 187–191, the PushQueues builder call needs to
explicitly disable throttling to match the earlier change; update the builder
chain to call the disable_throttling() method (before build()) on
PushQueues::<u8>::builder() so throttling is turned off here as well, and add
any necessary use/import if disable_throttling is not in scope.
|
@coderabbitai pause |
✅ 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.
|
@coderabbitai Have the following now been resolved?
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Has the following now been resolved?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos Looking at the current state of the test duplication issue: Test Duplication Status: The analysis shows:
Areas still needing consolidation:
The issue remains partially resolved - while some progress has been made with fixtures, significant duplication still exists that could be further consolidated through additional shared test utilities or more consistent use of existing fixtures. 🏁 Script executed: Length of output: 9983 @leynos Based on the updated analysis, the test duplication status has significantly improved: Test Duplication Status: ✅ LARGELY RESOLVED The current state shows much better consolidation than previously assessed:
Remaining duplication analysis:
Conclusion: The test duplication issue is now largely resolved. The remaining 19 direct builder calls are mostly justified by different test requirements or are already consolidated into fixtures. This represents a significant improvement from the initial state, and the remaining duplication is minimal and largely appropriate for the specific testing needs. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Superseded by #339 |
1 participant
Summary
Nonedisables throttling and frames are dropped when the DLQ is absent or fullPushErroras non-exhaustivebuild_via_builderhelperTesting
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_68aeba8d630c8322bcab55de80b2ef06
Summary by Sourcery
Introduce a fluent builder API for PushQueues with configurable capacities, rate limits, and dead-letter queue support, replacing the deprecated bounded_* constructors.
New Features:
Enhancements:
Documentation:
Tests:
Chores: