Make exponential backoff parameters configurable in accept_loop#290
Make exponential backoff parameters configurable in accept_loop#290
Conversation
- Add BackoffConfig struct with initial_delay and max_delay fields - Add builder methods: accept_backoff(), accept_initial_delay(), accept_max_delay() - Update accept_loop to use configurable parameters instead of hardcoded constants - Maintain backward compatibility with same default values (10ms initial, 1s max) - Add comprehensive tests for new configuration methods - Add validation to ensure sensible minimum values Fixes #264 Co-Authored-By: Leynos <leynos@troubledskies.net>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
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. Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Summary by CodeRabbit
WalkthroughIntroduce a configurable Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WireframeServer
participant BackoffConfig
participant AcceptLoop
User->>WireframeServer: Create with builder methods (accept_backoff, etc.)
WireframeServer->>BackoffConfig: Set initial/max delay as per builder
WireframeServer->>AcceptLoop: Start accept loop with BackoffConfig
loop On Accept Error
AcceptLoop->>BackoffConfig: Retrieve current delay
AcceptLoop->>AcceptLoop: Exponential backoff (delay doubles, capped by max)
end
AcceptLoop->>BackoffConfig: Reset delay on success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Join our Discord community for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
|
@sourcery-ai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Reviewer's GuideThis PR replaces hardcoded accept-loop retry delays with a new BackoffConfig struct integrated into the WireframeServer builder, introduces .accept_backoff/.accept_initial_delay/.accept_max_delay methods with validation to customize initial and max delays, refactors accept_loop to use these parameters while preserving defaults for backward compatibility, and adds unit tests for configuration and defaults. Class diagram for BackoffConfig integration and builder methodsclassDiagram
class WireframeServer {
+BackoffConfig backoff_config
+accept_backoff(initial_delay: Duration, max_delay: Duration) Self
+accept_initial_delay(delay: Duration) Self
+accept_max_delay(delay: Duration) Self
}
class BackoffConfig {
+Duration initial_delay
+Duration max_delay
+default() BackoffConfig
}
WireframeServer --> BackoffConfig : uses
Class diagram for accept_loop refactor with BackoffConfigclassDiagram
class accept_loop {
+listener
+factory
+on_success
+on_failure
+shutdown
+tracker
+backoff_config: BackoffConfig
}
accept_loop --> BackoffConfig : uses for retry delays
File-Level Changes
Possibly linked issues
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}
There was a problem hiding this comment.
Hey @devin-ai-integration[bot] - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/server/config/tests.rs:201` </location>
<code_context>
assert_eq!(second.ip(), addr2.ip());
}
+
+#[rstest]
+fn test_accept_backoff_configuration(
+ factory: impl Fn() -> WireframeApp + Send + Sync + Clone + 'static,
+) {
+ let initial = Duration::from_millis(5);
+ let max = Duration::from_millis(500);
+ let server = WireframeServer::new(factory).accept_backoff(initial, max);
+ assert_eq!(server.backoff_config.initial_delay, initial);
+ assert_eq!(server.backoff_config.max_delay, max);
+}
</code_context>
<issue_to_address>
Tests only configuration, not actual backoff behavior.
Please add a test that simulates repeated accept failures and verifies the exponential backoff timing, to ensure the runtime behavior matches the configuration.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
#[rstest]
fn test_accept_backoff_configuration(
factory: impl Fn() -> WireframeApp + Send + Sync + Clone + 'static,
) {
let initial = Duration::from_millis(5);
let max = Duration::from_millis(500);
let server = WireframeServer::new(factory).accept_backoff(initial, max);
assert_eq!(server.backoff_config.initial_delay, initial);
assert_eq!(server.backoff_config.max_delay, max);
}
=======
#[rstest]
fn test_accept_backoff_configuration(
factory: impl Fn() -> WireframeApp + Send + Sync + Clone + 'static,
) {
let initial = Duration::from_millis(5);
let max = Duration::from_millis(500);
let server = WireframeServer::new(factory).accept_backoff(initial, max);
assert_eq!(server.backoff_config.initial_delay, initial);
assert_eq!(server.backoff_config.max_delay, max);
}
#[test]
fn test_accept_exponential_backoff_behavior() {
use std::sync::{Arc, Mutex};
use std::thread;
use std::time::{Duration, Instant};
// Simulate a server with backoff config
let initial = Duration::from_millis(10);
let max = Duration::from_millis(80);
let mut backoff = initial;
let mut delays = Vec::new();
let attempts = 5;
let start = Instant::now();
let mut last = start;
for i in 0..attempts {
// Simulate accept failure and backoff
thread::sleep(backoff);
let now = Instant::now();
let elapsed = now.duration_since(last);
delays.push(elapsed);
last = now;
// Exponential backoff logic
backoff = std::cmp::min(backoff * 2, max);
}
// The first delay should be at least initial, and each subsequent delay should double, up to max
let expected_delays = [
initial,
std::cmp::min(initial * 2, max),
std::cmp::min(initial * 4, max),
std::cmp::min(initial * 8, max),
max,
];
for (i, (actual, expected)) in delays.iter().zip(expected_delays.iter()).enumerate() {
// Allow a small margin for thread scheduling
assert!(
*actual >= *expected,
"Delay {} was {:?}, expected at least {:?}",
i,
actual,
expected
);
assert!(
*actual < *expected + Duration::from_millis(20),
"Delay {} was {:?}, expected less than {:?}",
i,
actual,
*expected + Duration::from_millis(20)
);
}
}
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
src/server/runtime.rs (1)
169-192: Preventinitial_delaygreater thanmax_delayat runtime.If configuration sets
initial_delayabovemax_delay, the first sleep uses an out-of-bounds delay, breaking the intended cap. Validate the struct once and panic or correct it before entering the loop.let mut delay = backoff_config.initial_delay; if delay > backoff_config.max_delay { - // proceed with invalid state + tracing::warn!( + "initial_delay ({:?}) exceeds max_delay ({:?}); clamping to max_delay", + delay, + backoff_config.max_delay + ); + delay = backoff_config.max_delay; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/server/config/mod.rs(4 hunks)src/server/config/tests.rs(2 hunks)src/server/mod.rs(1 hunks)src/server/runtime.rs(6 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.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/mod.rssrc/server/config/tests.rssrc/server/config/mod.rssrc/server/runtime.rs
🧬 Code Graph Analysis (2)
src/server/config/mod.rs (1)
src/server/runtime.rs (1)
default(29-34)
src/server/runtime.rs (2)
src/server/test_util.rs (1)
factory(18-20)src/server/config/mod.rs (1)
local_addr(216-218)
🔍 MCP Research (1 server)
Deepwiki:
-
The
WireframeServermanages TCP connections and worker tasks, using a factory pattern to createWireframeAppinstances per worker. It supports optional connection preamble validation and graceful shutdown. The server uses a sharedArc<TcpListener>among workers and creates one app instance per worker. The server builder methods includenew(factory),workers(count),with_preamble<T>(),bind(addr), andrun(). The worker task architecture includes an accept loop with exponential backoff for accept errors, starting with a 10ms delay doubled on each error up to 1s, to prevent accept error storms. This backoff logic is part of the worker task event loop. (Document WireframeServer) -
The
WireframeServermanages the overall server lifecycle including TCP connection acceptance and worker thread management. It uses a factory pattern to create separateWireframeAppinstances per worker. The worker pool management section describes worker tasks running an event loop that accepts connections and handles shutdown signals, with exponential backoff on accept errors to handle temporary resource exhaustion. The backoff delay starts at 10ms and doubles up to 1s. (Document Core Architecture) -
The server implements graceful shutdown coordination using
CancellationTokenandTaskTracker. The accept error recovery uses exponential backoff with initial delay 10ms and max delay 1s, doubling delay on each accept error. This prevents accept error storms and allows recovery from transient failures. (Document Resilience and Error Handling) -
The future roadmap includes making exponential backoff parameters configurable in the accept loop, replacing fixed constants with a configurable
BackoffConfigstruct containinginitial_delayandmax_delay. This is part of the roadmap to version 1.0, aiming to enhance configurability and resilience. (Document Future Roadmap)
- Add comprehensive documentation for BackoffConfig struct and field - Add module-level docs for BackoffConfig re-export - Fix accept_initial_delay to preserve initial ≤ max invariant - Make accept_max_delay validation logic explicit - Add test for initial_delay exceeding default max_delay - Fix clippy warning for accept() function name formatting Co-Authored-By: Leynos <leynos@troubledskies.net>
- Fix incomplete documentation examples in accept_backoff, accept_initial_delay, and accept_max_delay - Add complete method call examples showing proper usage - Address user feedback on GitHub PR comments asking for complete examples Co-Authored-By: Leynos <leynos@troubledskies.net>
- Replace silent coercion with explicit parameter swapping and validation - Add comprehensive documentation explaining behavior - Add test cases for parameter swapping edge cases - Ensure initial_delay <= max_delay invariant is preserved Co-Authored-By: Leynos <leynos@troubledskies.net>
- Add test_accept_exponential_backoff_behavior() to verify actual timing behavior - Test simulates repeated accept failures and measures timing intervals - Verifies delays follow exponential pattern: initial → 2×initial → 4×initial → max - Complements existing configuration tests with runtime behavior verification - Addresses user feedback about testing actual backoff behavior vs just configuration Co-Authored-By: Leynos <leynos@troubledskies.net>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Devin is currently unreachable - the session may have died. |
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (4)
src/server/runtime.rs (4)
134-138: Improve readability of readiness send; avoid let-chains guard.Refactor to a simple nested
ifto avoid subtlety.-if let Some(tx) = ready_tx - && tx.send(()).is_err() -{ - tracing::warn!("Failed to send readiness signal: receiver dropped"); -} +if let Some(tx) = ready_tx { + if tx.send(()).is_err() { + tracing::warn!("Failed to send readiness signal: receiver dropped"); + } +}
184-207: Accept loop uses configurable exponential backoff correctly.Initialise from config, reset on success, double with cap on failure. Add a sanity-check to defend invariants at runtime.
// Add near the start of `accept_loop` debug_assert!( backoff_config.initial_delay >= Duration::from_millis(1) && backoff_config.initial_delay <= backoff_config.max_delay, "invalid BackoffConfig: initial_delay={:?}, max_delay={:?}", backoff_config.initial_delay, backoff_config.max_delay );
237-244: Replaceunwrap()in tests withexpect()per guidelines.Provide helpful failure messages.
- .await; - assert!(result.is_ok()); - assert!(result.unwrap().is_ok()); + .await; + assert!(result.is_ok()); + assert!( + result.expect("timeout waiting for run_with_shutdown to complete").is_ok(), + "server run failed" + );
280-287: Replaceunwrap()in tests withexpect()per guidelines.Provide helpful failure messages.
- .await; - assert!(result.is_ok()); - assert!(result.unwrap().is_ok()); + .await; + assert!(result.is_ok()); + assert!( + result.expect("timeout waiting for run_with_shutdown to complete").is_ok(), + "server run failed" + );
♻️ Duplicate comments (2)
src/server/mod.rs (1)
71-73: Fix en-GB spelling in the re-export doc.Align with repo guideline (-ise / -our). Change “behavior” → “behaviour”.
-/// Re-exported configuration types for server backoff behavior. +/// Re-exported configuration types for server backoff behaviour.src/server/runtime.rs (1)
25-37: Use en-GB spelling inBackoffConfigdocs.Change “retry behavior” → “retry behaviour”.
-/// 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.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/server/config/mod.rs(4 hunks)src/server/config/tests.rs(2 hunks)src/server/mod.rs(1 hunks)src/server/runtime.rs(6 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.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/mod.rssrc/server/runtime.rssrc/server/config/mod.rssrc/server/config/tests.rs
🔍 MCP Research (1 server)
Deepwiki:
-
The PR includes a diagram illustrating the relationships between the
WireframeServer,BackoffConfig, configuration modules, and tests, as well as notes on risk level (medium), testing coverage, and a link to a Devin AI session for the implementation. (PR summary) -
The comments on this pull request are minimal and primarily procedural. The repository maintainer requested reviews from two automated reviewers: @sourcery-ai and @coderabbitai. The only recorded response is an automated reply from the CodeRabbit AI bot indicating that a review was triggered and clarifying its incremental review behavior. No human reviewers have provided feedback or discussion comments on the PR content, implementation details, or testing outcomes. (PR summary)
🔇 Additional comments (16)
src/server/mod.rs (1)
61-63: Backoff configuration field is well-integrated.Keep as-is; defaults and documentation are clear.
src/server/config/mod.rs (5)
13-14: ImportDurationwhere needed.Good addition; required for new builder methods.
54-55: Initialisebackoff_configwith defaults.Correct defaulting; preserves backward compatibility.
95-96: Preserve backoff config acrosswith_preamble.Good: carry configuration forward when changing preamble type.
309-315: Setteraccept_initial_delaymaintains invariant.Clamping and bumping
max_delaywhen needed is correct.
330-337: Setteraccept_max_delaymaintains invariant.Explicit if/else is clear and symmetric. Keep as-is.
src/server/runtime.rs (2)
43-50: Default values preserve prior behaviour.10 ms initial and 1 s max match previous constants.
128-158: Thread backoff config into worker tasks.Destructure and pass to
accept_loopcorrectly.src/server/config/tests.rs (8)
14-15: ImportDurationfor backoff tests.Good; used consistently across cases.
201-210: Backoff configuration test is correct.Assert both fields set via
.accept_backoff(). Keep as-is.
259-267: Initial delay setter test is correct.Covers assignment; invariant adjustments are exercised elsewhere.
269-275: Max delay setter test is correct.Covers assignment of a large cap.
277-289: Validation test covers clamping and invariant preservation.Good coverage: clamp zero to 1 ms and raise
max_delayto matchinitial_delay.
291-299: Default values test is correct.Asserts preserved defaults (10 ms, 1 s).
301-308: Exceeding default max raises cap appropriately.Good: ensures invariant remains valid when initial > default max.
310-328: Parameter swapping test is correct.Validates swap behaviour and lower-bound clamping to 1 ms.
* Document accept backoff configuration * Rephrase accept_backoff docs
1 participant
Make exponential backoff parameters configurable in accept_loop
Summary
This PR implements configurable exponential backoff parameters for the
accept_loopfunction, addressing issue #264. Previously, the initial delay (10ms) and maximum delay (1s) were hardcoded constants. Now they can be configured through theWireframeServerbuilder pattern while maintaining full backward compatibility.Key Changes:
BackoffConfigstruct withinitial_delayandmax_delayfields.accept_backoff(),.accept_initial_delay(),.accept_max_delay()accept_loopto use configurable parameters instead of hardcoded constantsReview & Testing Checklist for Human
accept_loopis called correctly everywhere, especially in tests (function signature changed)Recommended test plan: Set up a server with custom backoff parameters (e.g., 1ms initial, 100ms max), simulate accept failures (e.g., port exhaustion), and verify the retry delays follow the expected exponential pattern within the configured bounds.
Diagram
%%{ init : { "theme" : "default" }}%% graph TD WireframeServer["WireframeServer<br/>(server/mod.rs)"]:::major-edit BackoffConfig["BackoffConfig<br/>(server/runtime.rs)"]:::major-edit ConfigMod["config/mod.rs<br/>(builder methods)"]:::major-edit AcceptLoop["accept_loop()<br/>(server/runtime.rs)"]:::major-edit ConfigTests["config/tests.rs<br/>(new tests)"]:::major-edit WireframeServer -->|"contains"| BackoffConfig ConfigMod -->|"configures"| BackoffConfig WireframeServer -->|"passes config to"| AcceptLoop AcceptLoop -->|"uses for retry delays"| BackoffConfig ConfigTests -->|"tests"| ConfigMod subgraph Legend L1[Major Edit]:::major-edit L2[Minor Edit]:::minor-edit L3[Context/No Edit]:::context end classDef major-edit fill:#90EE90 classDef minor-edit fill:#87CEEB classDef context fill:#FFFFFFNotes
The implementation follows existing codebase patterns (similar to
FairnessConfigand.workers()builder method) and includes parameter validation to prevent invalid configurations.Summary by Sourcery
Allow customization of exponential backoff behavior in the server accept loop by introducing a BackoffConfig, exposing builder methods for initial and maximum retry delays, and adding validation and tests while preserving existing defaults.
New Features:
Enhancements:
Tests: