Conversation
|
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. Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
Summary by CodeRabbit
WalkthroughIntroduce an AcceptListener trait and BackoffConfig; implement AcceptListener for TcpListener; make accept_loop generic over AcceptListener and accept shutdown/tracker/backoff parameters; add async tests with a MockAcceptListener to validate exponential backoff; rename a docs example trait to AcceptListener. Changes
Sequence Diagram(s)sequenceDiagram
participant AcceptLoop
participant AcceptListener
participant Backoff
participant Worker
loop accept cycle
AcceptLoop->>AcceptListener: accept()
alt success
AcceptListener-->>AcceptLoop: (TcpStream, SocketAddr)
AcceptLoop->>Worker: spawn connection task
Backoff-->>AcceptLoop: reset backoff
else failure
AcceptListener-->>AcceptLoop: io::Error
AcceptLoop->>Backoff: compute next delay
Backoff-->>AcceptLoop: delay (exponential)
AcceptLoop->>AcceptLoop: await delay or observe shutdown
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Assessment against linked issues
Possibly related PRs
Suggested reviewers
✨ 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 (
|
Reviewer's GuideThe PR refactors the accept loop to abstract over a generic Listener trait and adds an async test verifying exponential backoff behavior using a mock listener that always errors. Sequence diagram for accept_loop with MockListener and exponential backoffsequenceDiagram
participant accept_loop
participant MockListener
participant CancellationToken
loop until cancelled
accept_loop->>MockListener: accept()
MockListener-->>accept_loop: Err(io::Error)
accept_loop->>accept_loop: wait (exponential backoff)
end
accept_loop->>CancellationToken: check cancel
accept_loop-->>accept_loop: exit loop
Class diagram for Listener trait and implementationsclassDiagram
class Listener {
<<trait>>
+accept() io::Result<(TcpStream, SocketAddr)> [async]
+local_addr() io::Result<SocketAddr>
}
class TcpListener {
+accept() io::Result<(TcpStream, SocketAddr)> [async]
+local_addr() io::Result<SocketAddr>
}
class MockListener {
+calls: Arc<Mutex<Vec<Instant>>>
+accept() io::Result<(TcpStream, SocketAddr)> [async]
+local_addr() io::Result<SocketAddr>
}
Listener <|.. TcpListener
Listener <|.. MockListener
Class diagram for updated accept_loop functionclassDiagram
class accept_loop {
+listener: Arc<L>
+factory: F
+on_success: Option<PreambleCallback<T>>
+on_failure: Option<PreambleErrorCallback>
+token: CancellationToken
+tracker: TaskTracker
+backoff_config: BackoffConfig
+async fn accept_loop()
}
class Listener {
<<trait>>
}
accept_loop --> Listener: uses
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![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 Have the following now been resolved?
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/server/runtime.rs (1)
188-199: Add module-level documentation for the public function.Add documentation explaining the purpose, parameters, and behaviour of this newly exposed
pub(super)function.+/// Accepts incoming connections and spawns tasks to handle them. +/// +/// This function runs an accept loop that: +/// - Accepts connections from the provided listener +/// - Spawns connection handler tasks using the provided factory +/// - Applies exponential backoff on accept errors +/// - Respects the shutdown signal +/// +/// # Type Parameters +/// +/// * `F` - Factory function that creates `WireframeApp` instances +/// * `T` - Preamble type for connection handshaking +/// * `L` - Listener type implementing `AcceptListener` pub(super) async fn accept_loop<F, T, L>(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/mocking-network-outages-in-rust.md(1 hunks)src/server/runtime.rs(7 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
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/mocking-network-outages-in-rust.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/mocking-network-outages-in-rust.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/mocking-network-outages-in-rust.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/mocking-network-outages-in-rust.md
**/*.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/server/runtime.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.
Files:
src/server/runtime.rs
{src,tests}/**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
src/server/runtime.rs
🧬 Code Graph Analysis (1)
src/server/runtime.rs (3)
src/server/config/mod.rs (2)
local_addr(216-218)new(45-57)tests/preamble.rs (1)
oneshot(71-71)src/server/test_util.rs (1)
factory(18-20)
🔍 MCP Research (2 servers)
Deepwiki:
- src/server.rs:392-429: The accept loop in worker_task implements exponential backoff on accept errors — starts with a 10ms delay, doubles the delay after each error up to a 1s cap, and resets the delay to 10ms after a successful accept. (DeepWiki search result)
- docs/mocking-network-outages-in-rust.md:427-435: Shows an example using tokio-test::io::Builder to inject io::Error into streams, which can be adapted to simulate accept() errors for testing backoff behavior. (DeepWiki search result)
Context7:
- Defining Asynchronous Tests with pytest-asyncio: async tests use the @pytest.mark.asyncio decorator allowing await inside test bodies (source: https://github.com/pytest-dev/pytest-asyncio/blob/main/README.rst#_snippet_0)
- Applying Class-Scoped Event Loop to Async Tests: pytest-asyncio supports loop_scope via @pytest.mark.asyncio(loop_scope="class") to run class tests in the same event loop (source: https://github.com/pytest-dev/pytest-asyncio/blob/main/docs/how-to-guides/run_class_tests_in_same_loop.rst#_snippet_0)
- Migrating Pytest Asyncio Marker Scope: the scope argument is deprecated; use loop_scope instead (source: https://github.com/pytest-dev/pytest-asyncio/blob/main/docs/how-to-guides/migrate_from_0_23.rst#_snippet_2)
- Configuring asyncio_mode via pytest.ini/pyproject.toml: pytest-asyncio can be configured with asyncio_mode = auto (or strict) in pytest config, affecting discovery/handling of async tests (source: https://github.com/pytest-dev/pytest-asyncio/blob/main/docs/reference/configuration.rst#_snippet_0)
🔇 Additional comments (5)
src/server/runtime.rs (4)
24-27: LGTM! Well-designed trait abstraction.The
AcceptListenertrait provides a clean abstraction for the accept loop, enabling effective testing whilst maintaining production behaviour. The trait methods align perfectly with the requirements for mocking network behaviour.
30-36: LGTM! Clean trait implementation.The implementation correctly delegates to the underlying
TcpListenermethods. The explicit method paths (TcpListener::accept,TcpListener::local_addr) avoid ambiguity.
339-357: LGTM! Well-structured mock implementation.The
MockListenerprovides excellent test infrastructure. Recording call times withInstant::now()enables precise backoff timing verification. The implementation correctly returns errors to trigger the backoff logic.
360-407: Excellent async test implementation!This test comprehensively verifies the exponential backoff behaviour:
- Uses
start_paused = truefor deterministic time control- Correctly advances time to trigger backoff delays (5ms → 10ms → 20ms)
- Verifies actual intervals between accept calls
- Properly cleans up with cancellation and tracker closure
The test aligns perfectly with the PR objectives and the documented backoff behaviour from the external context.
docs/mocking-network-outages-in-rust.md (1)
582-584: LGTM! Documentation correctly updated.The trait name change from
ListenertoAcceptListenerin the documentation correctly reflects the implementation insrc/server/runtime.rs.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1774 🏁 Script executed: Length of output: 1982 🏁 Script executed: Length of output: 884 @leynos Yes, both issues have been resolved: ✅ Trait renaming: The trait has been renamed from ✅ Deterministic timing: The test now uses The implementation uses tokio's built-in time control features (which is actually preferable to tokio-test) by:
This ensures the backoff timing behavior (5ms → 10ms → 20ms) is tested deterministically without any race conditions or timing dependencies. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (4)
src/server/runtime.rs (4)
38-53: Fix en-GB spelling in docs (“behaviour” not “behavior”).Align with the project’s en-GB spelling guideline and keep terminology consistent in this module’s docs.
Apply this diff:
-/// Controls retry behavior when `accept()` calls fail on the server's TCP listener. +/// Controls retry behaviour when `accept()` calls fail on the server's TCP listener.
239-251: Parameter list is getting long; group related inputs.Reduce the cognitive load by grouping accept-loop inputs into a context struct (e.g., AcceptLoopArgs) to avoid a 7-parameter signature and ease future extension.
Example shape outside this range:
pub(super) struct AcceptLoopArgs<F, T, L> { pub listener: Arc<L>, pub factory: F, pub on_success: Option<PreambleCallback<T>>, pub on_failure: Option<PreambleErrorCallback>, pub shutdown: CancellationToken, pub tracker: TaskTracker, pub backoff: BackoffConfig, }Then accept
AcceptLoopArgsby value inaccept_loop.
271-275: Augment error log with back-off metadata.Emit the next retry delay to aid triage when accept storms happen.
Apply this diff:
- let local_addr = listener.local_addr().ok(); - tracing::warn!(error = ?e, ?local_addr, "accept error"); - sleep(delay).await; - delay = (delay * 2).min(backoff_config.max_delay); + let local_addr = listener.local_addr().ok(); + tracing::warn!(error = ?e, ?local_addr, retry_in = ?delay, "accept error; backing off"); + sleep(delay).await; + delay = (delay * 2).min(backoff_config.max_delay);
281-466: Reduce file size by moving tests to a sibling tests module.The file is 467 lines, exceeding the 400-line limit. Extract the #[cfg(test)] module into a dedicated
src/server/runtime/tests.rs(ortests/integration tests if suitable) andmod tests;gate it to bring this file back under the limit.
♻️ Duplicate comments (1)
src/server/runtime.rs (1)
410-465: Async back-off test is deterministic and thorough.Start the clock paused, advance in discrete steps with
yield_now()between, and assert the 5→10→20ms sequence. The immediate first-call assertion is present and correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/server/runtime.rs(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/server/runtime.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.
Files:
src/server/runtime.rs
{src,tests}/**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
src/server/runtime.rs
🧬 Code Graph Analysis (1)
src/server/runtime.rs (2)
src/server/config/mod.rs (2)
local_addr(216-218)new(45-57)src/server/test_util.rs (1)
factory(18-20)
🔍 MCP Research (1 server)
Deepwiki:
- Document WireframeServer: src/server.rs contains an "Accept Error Recovery" snippet that initializes backoff with Duration::from_millis(10), doubles the delay on each accept error, and caps it at Duration::from_secs(1) while logging the accept error (see WireframeServer: src/server.rs:392 and src/server.rs:425-429)
- Document WireframeServer: several places describe the worker/task architecture using a shared Arc among worker tasks (e.g., WireframeServer worker pool and diagrams referencing Arc) — code/docs still reference Arc as the shared listener (see WireframeServer: src/server.rs:49-75)
🔇 Additional comments (5)
src/server/runtime.rs (5)
23-27: Generalise the accept loop behind AcceptListener (good abstraction).Define a small, focused trait for acceptance and local_addr and keep it
pub(super). This cleanly decouples the accept loop from TcpListener and enables deterministic testing. BoundSend + Syncat call sites rather than the trait, which is the right choice.
29-36: TcpListener implementation is correct and minimal.Delegate to the inherent methods directly. Keep the one-liner for local_addr as-is; it’s concise and idiomatic.
59-66: Backoff defaults align with documented standard.Initial delay of 10ms and cap of 1s match the “Accept Error Recovery” defaults in WireframeServer docs.
373-387: Shut-down path test is solid.Spawn the loop, cancel, close the tracker, and assert timely completion under a timeout. This gives good confidence in the cancellation precedence with
select! { biased; }.
390-409: MockListener is minimal and effective.Record timestamps on each accept attempt and return errors. Keep it simple and thread-safe with Mutex<...>.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (6)
src/server/runtime.rs (6)
236-284: Make back-off sleep cancellable to avoid delayed shutdown.Cancellation during back-off currently waits until
sleep(delay)completes before exiting, delaying shutdown. Gate the sleep withselect!so the loop exits immediately onshutdown.Apply this diff:
Err(e) => { let local_addr = listener.local_addr().ok(); tracing::warn!(error = ?e, ?local_addr, "accept error"); - sleep(delay).await; - delay = (delay * 2).min(backoff_config.max_delay); + // Wait for either shutdown or the back-off delay before retrying. + select! { + () = shutdown.cancelled() => break, + _ = sleep(delay) => {} + } + delay = (delay * 2).min(backoff_config.max_delay); }
88-96: Correct error semantics inrundocumentation.
runnever propagates transientacceptfailures; it logs and continues with back-off. It returns an error only for pre-run issues (e.g., missing listener). Align the docs with behaviour.Apply this diff:
- /// # Errors - /// - /// Returns an [`io::Error`] if accepting a connection fails. + /// # Errors + /// + /// Returns an [`io::Error`] if the server was not properly initialised (e.g., no listener + /// was bound) before running. Transient `accept` failures are logged and retried with back-off.
127-131: Correct error semantics inrun_with_shutdowndocumentation.This method does not return errors for runtime
acceptfailures; it logs and retries. It returns an error only if the server was not bound.Apply this diff:
- /// # Errors - /// - /// Returns an [`io::Error`] if accepting a connection fails during runtime. + /// # Errors + /// + /// Returns an [`io::Error`] only if the server was not bound before starting. Runtime `accept` + /// failures are logged and retried with exponential back-off.
236-256: Enforce all documentedBackoffConfiginvariants.Add a debug assertion ensuring
max_delayis non-zero andinitial_delayis non-zero (already covered), and document thatmax_delaymust be ≥ 1ms explicitly if that’s a requirement.Apply this diff:
debug_assert!( backoff_config.initial_delay >= Duration::from_millis(1), "initial_delay must be at least 1ms", ); debug_assert!( backoff_config.initial_delay <= backoff_config.max_delay, "initial_delay must not exceed max_delay", ); + debug_assert!( + backoff_config.max_delay >= Duration::from_millis(1), + "max_delay must be at least 1ms" + );
1-472: Split this module to keep files ≤ 400 lines.This file is 472 lines; the guideline caps files at 400. Move tests into
tests/server/runtime.rs(integration) or a siblingsrc/server/runtime/tests.rs(with#[cfg(test)]) to reduce size. Keep the runtime code focused.I can generate the new test module layout if you want.
275-280: Augment logs with next retry delay.Emit the current
delayin the warning to aid diagnostics when accept flaps.Apply this diff:
- tracing::warn!(error = ?e, ?local_addr, "accept error"); + tracing::warn!(error = ?e, ?local_addr, ?delay, "accept error; retrying after back-off");
♻️ Duplicate comments (1)
src/server/runtime.rs (1)
38-50: Remove empty doc lines to satisfy style checks.Tidy doc comments by removing standalone
///lines.Apply this diff:
-/// -/// Controls retry behaviour when `accept()` calls fail on the server's TCP listener. +/// Controls retry behaviour when `accept()` calls fail on the server's TCP listener. /// The back-off starts at `initial_delay` and doubles on each failure, capped at `max_delay`. -/// -/// +/// /// # Default Values -/// /// - `initial_delay`: 10 milliseconds -/// /// - `max_delay`: 1 second -/// /// /// # Invariants /// /// - `initial_delay` must not exceed `max_delay` /// - `initial_delay` must be at least 1 millisecond
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/server/runtime.rs(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/server/runtime.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.
Files:
src/server/runtime.rs
{src,tests}/**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
Write unit and behavioural tests for new functionality
Files:
src/server/runtime.rs
🧬 Code Graph Analysis (1)
src/server/runtime.rs (3)
src/server/config/mod.rs (2)
local_addr(216-218)new(45-57)tests/preamble.rs (1)
oneshot(71-71)src/server/test_util.rs (1)
factory(18-20)
🔍 MCP Research (1 server)
Deepwiki:
- Document docs/mocking-network-outages-in-rust.md: recommends using tokio-test::io::Builder to create custom I/O streams for simulating network errors in tests (https://deepwiki.com/search/add-async-backoff-test-for-acc_aec236f7-23a9-4596-9b5f-fe5135a19dbf)
🔇 Additional comments (2)
src/server/runtime.rs (2)
439-446: LGTM: Assert the initial accept happens immediately.The additional assertion for the first immediate
acceptcall resolves the earlier feedback and strengthens test determinism.
447-470: LGTM: Deterministic back-off test with paused time.The test cleanly asserts the 5ms → 10ms → 20ms sequence using
start_paused,advance, andyield_now(), and cancels/awaits cleanly.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (5)
src/server/runtime.rs (5)
320-320: Replaceunwrap()with.expect(...)to satisfy project guidelines.Avoid
unwrap()in tests per the coding guidelines.Apply this diff:
- assert!(result.unwrap().is_ok()); + assert!(result.expect("server did not finish in time").is_ok());
363-363: Replaceunwrap()with.expect(...)to satisfy project guidelines.Avoid
unwrap()in tests per the coding guidelines.Apply this diff:
- assert!(result.unwrap().is_ok()); + assert!(result.expect("server did not finish in time").is_ok());
95-101: Correct the “Errors” section forrun: accept failures are retried, not returned.The accept loop handles errors with exponential back-off;
runonly errors when there is no bound listener.Apply this diff:
- /// # Errors - /// - /// Returns an [`io::Error`] if accepting a connection fails. + /// # Errors + /// + /// Returns an [`io::Error`] if the server was not bound to a listener. + /// Accept failures are retried with exponential back-off and do not surface as errors.
132-135: Alignrun_with_shutdownerror docs with actual behaviour.This function does not bubble up accept errors; it only errors if no listener was bound.
Apply this diff:
- /// # Errors - /// - /// Returns an [`io::Error`] if accepting a connection fails during runtime. + /// # Errors + /// + /// Returns an [`io::Error`] if the server was not bound to a listener. + /// Accept failures are retried with exponential back-off and do not surface as errors.
288-465: Reduce module size below 400 lines by extracting tests.The file is 466 lines. Adhere to the 400-line limit. Move the
#[cfg(test)] mod testsblock into a sibling test module or integration tests to keep the runtime module focused and maintainable.Follow-up outline:
- Create
src/server/runtime/tests.rs(ortests/accept_backoff.rs) and move the test module there.- Keep
MockAcceptListenergeneration in the same module as the trait (you already gate it with#[cfg_attr(test, automock)]), so tests can importsuper::MockAcceptListener.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.toml(1 hunks)src/server/runtime.rs(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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.
Files:
src/server/runtime.rs
🧬 Code Graph Analysis (1)
src/server/runtime.rs (2)
src/server/config/mod.rs (2)
local_addr(216-218)new(45-57)src/server/test_util.rs (1)
factory(18-20)
🔇 Additional comments (3)
Cargo.toml (1)
43-43: Addmockallfor test-only mocking — LGTM.Keep mocking confined to tests as you’ve done. This aligns with the test guidelines.
src/server/runtime.rs (2)
251-258: Validate back-off invariants — LGTM.The debug assertions correctly enforce documented invariants and fail fast in debug builds.
441-444: Deterministic timing approach — LGTM.Use of
#[tokio::test(start_paused = true)],advance, andyield_nowdeterministically validates the 5ms → 10ms → 20ms back-off sequence. The initial immediate accept assertion is correctly enforced.Also applies to: 452-463
Exercise accept_loop with a mock listener that always errors to observe backoff doubling and capping in an async context.
leynos
force-pushed
the
codex/add-async-test-for-accept_loop-backoff
branch
from
1662717 to
2ad2597
Compare
|
@coderabbitai new GitHub issue, please: 288-465: Reduce module size below 400 lines by extracting tests. The file is 466 lines. Adhere to the 400-line limit. Move the #[cfg(test)] mod tests block into a sibling test module or integration tests to keep the runtime module focused and maintainable. Follow-up outline: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll create a GitHub issue for reducing the module size by extracting tests. @leynos I've created a GitHub issue to track the module size reduction task. The issue includes your specific outline for extracting the tests while keeping the 🏁 Script executed: Length of output: 996 @leynos Successfully created GitHub issue #307: Reduce module size below 400 lines by extracting tests The issue includes your detailed follow-up outline and has been assigned to you. |
1 participant
Summary
Testing
make fmtmake lintmake testcloses #294
https://chatgpt.com/codex/tasks/task_e_6896851cd32083229ab75609438062c4
Summary by Sourcery
Abstract the accept loop over a new async Listener trait, adapt the existing TcpListener to this interface, and add a test to ensure the loop applies exponential backoff delays correctly when accept errors occur
New Features:
Enhancements:
Tests: