Feat/redis read retry backoff - #161
Open
ruslanti wants to merge 5 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
It introduces breaking public API changes (traits) and has correctness/maintainability concerns around caller IP typing (IPv4-only) plus formatting drift that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves resiliency for Redis-backed key-value reads by adding a small, bounded retry loop with capped exponential backoff, and extends the stats plumbing to (a) optionally track cached KV reads and (b) pass the caller IP into stats row creation.
Changes:
- Add a capped exponential backoff retry wrapper for Redis read operations (
get,zrange_by_score,bf_exists) and retry-on-iterator-creation forscan/zscan. - Extend KV stats interfaces with a “served from in-process cache” counter and add a
Store::get_trackedoutcome type. - Thread a parsed
x-real-ipvalue intoContextT::new_stats_rowfrom the HTTP request handler.
File summaries
| File | Description |
|---|---|
| src/key_value.rs | Updates test stub ReadStats implementation for the new cached-read counter. |
| src/context.rs | Updates ContextT::new_stats_row impl signature and stats stub for new metrics method. |
| crates/runtime/src/util/stats.rs | Updates mock ReadStats in tests to include the new cached-read counter. |
| crates/runtime/src/store.rs | Updates test ReadStats stub to satisfy the expanded trait. |
| crates/runtime/src/lib.rs | Extends ContextT::new_stats_row API to accept caller_ip: Ipv4Addr. |
| crates/key-value-store/src/redis_impl.rs | Implements read retry/backoff helpers and applies them to Redis read commands. |
| crates/key-value-store/src/lib.rs | Introduces GetOutcome + get_tracked, and expands ReadStats with count_kv_read_cached. |
| crates/key-value-store/Cargo.toml | Adds tokio as an optional dependency for the redis feature (sleep/backoff). |
| crates/http-service/src/state.rs | Updates test ReadStats stub for the new cached-read counter. |
| crates/http-service/src/lib.rs | Parses x-real-ip into Ipv4Addr and passes it into new_stats_row. |
| crates/http-service/src/executor/http.rs | Updates test stubs for ContextT::new_stats_row and ReadStats. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+363
to
+367
| .headers() | ||
| .get(crate::executor::X_REAL_IP) | ||
| .and_then(|v| v.to_str().ok()) | ||
| .and_then(|v| v.parse::<std::net::Ipv4Addr>().ok()) | ||
| .unwrap_or(std::net::Ipv4Addr::UNSPECIFIED); |
| /// Increment the count of reads served from the in-process cache. Counted | ||
| /// in addition to [`ReadStats::count_kv_read`] / | ||
| /// [`ReadStats::count_kv_byod_read`], so it is a subset of the total reads. | ||
| fn count_kv_read_cached(&self); |
Comment on lines
433
to
+437
| fn new_stats_row( | ||
| &self, | ||
| request_id: &SmolStr, | ||
| app: &SmolStr, | ||
| caller_ip: Ipv4Addr, |
Comment on lines
+32
to
+36
| /// Retry a read command up to `REDIS_READ_RETRIES` times with capped | ||
| /// exponential backoff. `op` is re-invoked from scratch on each attempt so it | ||
| /// can pick a fresh pooled connection (used by `get`/`zrange_by_score`/ | ||
| /// `bf_exists`; `scan`/`zscan` retry the same connection since their iterator | ||
| /// borrows it across the loop). |
qrdl
approved these changes
Sep 2, 2026
Reads (get/exists/zrange_by_score/scan/zscan/bf_exists) in both the KV-store and cache-backend Redis clients now retry up to 2 times with capped exponential backoff (10ms/20ms, max 50ms) on top of the existing connection-manager reconnect. Kept deliberately small and bounded to avoid reintroducing the pile-up behavior behind the 2026-07-09 congestion-collapse incident. Mutating operations (incr/set/delete/ expire/clean_index) are left untouched since a lost-response retry could double-apply them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ruslanti
force-pushed
the
feat/redis-read-retry-backoff
branch
from
September 2, 2026 12:30
5d316b4 to
fa8455a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
pushing changes from fastedge repo subtree