feat(grpc): add exponential backoff for reconnection attempts#789
feat(grpc): add exponential backoff for reconnection attempts#789Molter73 wants to merge 8 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCentralized duration parsing added; BackoffConfig introduced and wired into GrpcConfig and CLI; scan_interval parsing moved to Duration; gRPC reconnect loop uses exponential backoff with optional jitter; tests, changelog, and rand dependency updated. ChangesgRPC Exponential Backoff Configuration and Duration Parsing
Sequence Diagram(s)sequenceDiagram
participant GrpcRunLoop as gRPC Run Loop
participant CreateChannel as create_channel()
participant Backoff
GrpcRunLoop->>Backoff: Initialize from BackoffConfig
loop Reconnect attempts
GrpcRunLoop->>CreateChannel: create_channel()
alt Channel creation succeeds
CreateChannel-->>GrpcRunLoop: Ok(Channel)
GrpcRunLoop->>Backoff: reset()
else Channel creation fails
CreateChannel-->>GrpcRunLoop: Err
GrpcRunLoop->>Backoff: next()
Backoff-->>GrpcRunLoop: next_delay (Duration)
GrpcRunLoop->>GrpcRunLoop: sleep(next_delay)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Assisted-by: claude-opus-4-6@default <noreply@opencode.ai>
…d env vars Add BackoffConfig to GrpcConfig with YAML (grpc.backoff.initial, grpc.backoff.max), CLI (--backoff-initial, --backoff-max), and env var (FACT_GRPC_BACKOFF_INITIAL, FACT_GRPC_BACKOFF_MAX) support. Extract yaml_to_duration_secs and parse_duration_secs helpers, reusing them for scan_interval parsing as well. Assisted-by: claude-opus-4-6@default <noreply@opencode.ai>
Molter73
force-pushed
the
mauro/feat/reconnect-backoff
branch
from
ddb3e74 to
fc69ffe
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #789 +/- ##
==========================================
+ Coverage 27.96% 31.95% +3.99%
==========================================
Files 21 21
Lines 2596 2766 +170
Branches 2596 2766 +170
==========================================
+ Hits 726 884 +158
- Misses 1867 1879 +12
Partials 3 3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
vladbologa
left a comment
vladbologa
left a comment
There was a problem hiding this comment.
Just some high level feedback, as I don't think I should be reviewing rust code. 😅
Why not use this existing crate?
And also, I checked the stackrox repo and there we use 10s initial, 5 minute max, and a 1.5 multiplier (and also jitter). So you could use these settings for consistency.
You are not the only one in the team thinking this, but I need to start getting people into it because I can't be the only one 🤣
With all the supply chain attacks going on these days I've become quite paranoid and would prefer we keep dependencies to a minimum. I've never heard of that crate, it looks interesting, but for a straightforward backoff implementation that is only used once (which is what I was going for), I didn't even consider looking for another dependency.
Sounds good, I'll modify the defaults and try to add jitter as well (probably make the multiplier and jitter configurable as well). |
Actually, as I went to change the deafults I realized for a component that does runtime security, we probably want to keep the reconnection as fast as possible, otherwise we will start to drop events. I will add jitter and change the multiplier though. |
Add configurable jitter to the exponential backoff using rand crate for uniform distribution over [0, delay]. Jitter is enabled by default and can be toggled via YAML (grpc.backoff.jitter), CLI (--backoff-jitter / --no-backoff-jitter), or env var (FACT_GRPC_NO_BACKOFF_JITTER). Assisted-by: claude-opus-4-6@default <noreply@opencode.ai>
Add multiplier field to BackoffConfig (default 1.5x). Configurable via YAML (grpc.backoff.multiplier), CLI (--backoff-multiplier), and env var (FACT_GRPC_BACKOFF_MULTIPLIER). Must be > 1.0. Drops Eq derive from config types in favor of PartialEq to support storing multiplier as f64 directly. Assisted-by: claude-opus-4-6@default <noreply@opencode.ai>
![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
🧹 Nitpick comments (1)
fact/src/config/mod.rs (1)
425-433: 💤 Low valueConsider extracting multiplier validation into a shared helper.
The multiplier validation logic (finite, > 1.0) is duplicated between
parse_multiplier(line 42) and this YAML parsing. While functionally correct, extracting a shared validation function would reduce duplication and keep the validation rules in one place.♻️ Optional refactor to reduce duplication
+fn validate_multiplier(mult: f64) -> anyhow::Result<f64> { + if !mult.is_finite() || mult <= 1.0 { + bail!("multiplier must be > 1.0, got {mult}"); + } + Ok(mult) +} + fn parse_multiplier(s: &str) -> anyhow::Result<f64> { let mult = s.parse::<f64>()?; - if !mult.is_finite() || mult <= 1.0 { - bail!("multiplier must be > 1.0, got {mult}"); - } - Ok(mult) + validate_multiplier(mult) }Then simplify the YAML parsing:
"multiplier" => { - let multiplier = match v.as_f64().or_else(|| v.as_i64().map(|v| v as f64)) { - Some(m) if !m.is_finite() || m <= 1.0 => { - bail!("invalid grpc.backoff.multiplier: {v:?}") - } - None => { - bail!("invalid grpc.backoff.multiplier: {v:?}") - } - Some(m) => m, - }; + let Some(m) = v.as_f64().or_else(|| v.as_i64().map(|v| v as f64)) else { + bail!("invalid grpc.backoff.multiplier: {v:?}") + }; + let multiplier = validate_multiplier(m) + .with_context(|| format!("invalid grpc.backoff.multiplier: {v:?}"))?; backoff.multiplier = Some(multiplier); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fact/src/config/mod.rs` around lines 425 - 433, The YAML parsing block duplicates multiplier validation logic; extract the shared validation into a helper (e.g., parse_multiplier or validate_multiplier) and call it from both places (the existing parse_multiplier caller and the YAML parsing code that currently binds `multiplier`). Implement a function that accepts a serde_json/serde_yaml Value or f64 and checks is_finite() and > 1.0, returns Result<f64, Error>, then replace the match in the YAML branch with a call to that helper to eliminate duplication and keep rules centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fact/src/config/mod.rs`:
- Around line 607-617: The env var and visibility are inverted for the backoff
jitter flags: move the env attribute from the negative flag to the positive flag
and swap visibility so the positive flag is the public env-controlled option.
Concretely, on the backoff_jitter field (symbol backoff_jitter) add env =
"FACT_GRPC_BACKOFF_JITTER" and remove hide = true (make it exposed), and on
no_backoff_jitter remove the env attribute and add hide = true (hide the
negative flag); keep the overrides_with relationships (backoff_jitter
overrides_with = "no_backoff_jitter" and vice versa) so behavior stays
consistent but follows the established positive-flag env pattern.
---
Nitpick comments:
In `@fact/src/config/mod.rs`:
- Around line 425-433: The YAML parsing block duplicates multiplier validation
logic; extract the shared validation into a helper (e.g., parse_multiplier or
validate_multiplier) and call it from both places (the existing parse_multiplier
caller and the YAML parsing code that currently binds `multiplier`). Implement a
function that accepts a serde_json/serde_yaml Value or f64 and checks
is_finite() and > 1.0, returns Result<f64, Error>, then replace the match in the
YAML branch with a call to that helper to eliminate duplication and keep rules
centralized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 5206ea5e-dd20-4dac-8c98-2e989a4b7c5b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.tomlfact/Cargo.tomlfact/src/config/mod.rsfact/src/config/tests.rsfact/src/output/grpc.rs
✅ Files skipped from review due to trivial changes (1)
- fact/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- fact/src/output/grpc.rs
- fact/src/config/tests.rs
vladbologa
left a comment
vladbologa
left a comment
There was a problem hiding this comment.
LGTM! You could make the PR a bit shorter by not exposing the jitter and multiplier configuration (e.g. stackrox doesn't expose them).
Fair, but I like having as much knobs as I can in case something goes wrong. |
3 participants
Description
If connection to a gRPC server fails, fact was into a loop trying to reconnect every second which is pretty noisy and not really needed. With this change we add an exponential backoff algorithm that will try to reconnect with some leeway.
In the interest of making it obvious
factis failing to connect on k8s, we probably want to crash the application if we reach the maximum backoff, but there is currently no easy way to do that, so I'll leave it to a follow up.Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Without this change, reconnection attempts every second
With this change, exponential attempts
Summary by CodeRabbit
New Features
Improvements
scan_intervaland backoff durations now parse directly intoDurationwith stricter validation and clearer error handling.Tests
scan_intervaland gRPC backoff.Documentation