fix(redis): enforce retry backoff via the retry queue - #390
Conversation
The sorted-set flow re-enqueued retries directly into the request queue with score now+backoff. The consumer pops with ZPopMin and no due-time check, so the computed exponential backoff was written and then ignored: against a persistently unavailable upstream a message retried at the poll cadence (observed 12 attempts per second), and because now+backoff sorts below any realistic deadline score, retries also preempted all fresh traffic, inverting earliest-deadline-first. Retries now park in the retry queue (retry_queue_name in the sorted set transport config, default retry-sortedset) scored by retry-due time, and a retry mover re-enters due messages into their origin queue with the original deadline as the score, enforcing the backoff and restoring EDF. Signed-off-by: Benjamin Braun <benjaminbraun@google.com>
Signed-off-by: Benjamin Braun <benjaminbraun@google.com>
There was a problem hiding this comment.
thanks, looks good!
aside: do you think each result queue should have its own corresponding retry queue, or would it be fine to have one shared across multiple sorted sets?
EDIT: we might want to add an e2e test, update the README and verify this new property gets through Helm config. Possibly related #295 #388 -- may be done in a follow-up if necessary
There was a problem hiding this comment.
Pull request overview
Routes Redis sorted-set retries through a dedicated retry queue so exponential backoff is actually enforced, then re-enters due retries into their origin request queue with the original deadline score to preserve EDF ordering.
Changes:
- Add
retry_queue_nameto the sorted-set transport config (defaultretry-sortedset) and plumb it intoRedisSortedSetFlow. - Update
flushRetryBatchto park retries in the retry queue scored by retry-due time, and introduce aretryMovergoroutine to move due retries back to request queues with deadline scores. - Extend unit tests to validate parking behavior and mover re-entry semantics (including origin-queue fallback).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| release-notes.d/unreleased/390.md | Documents the behavior change: retries now park until due instead of re-entering request traffic immediately. |
| pkg/redis/sortedset_impl.go | Implements retry-queue parking and adds a retry mover that re-inserts due retries into the correct request queue under the original deadline score. |
| pkg/redis/sortedset_impl_test.go | Adds/updates tests to assert retries park in the retry queue until due and are later moved back with deadline scoring. |
| pkg/redis/options.go | Adds retry_queue_name to sorted-set config and applies a default value. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Log retry queue read errors instead of silently stalling, drop the ineffective miniredis FastForward from the backoff test since the mover compares scores against wall clock time, and document retry_queue_name in the sorted set transport example. Signed-off-by: Benjamin Braun <benjaminbraun@google.com>
|
@evacchi I'll do the e2e test as a follow up since the chart still renders the deprecated --redis.ss.* flags, which have no way to set retry_queue_name, so right now an e2e would use the config path #388 is replacing. We have unit tests to cover the retry and re-entry behavior, so I would rather add the e2e together with the chart update so it tests --transport-config (As for separate retry queues, I don't think that's necessary. The retry queue only governs when an item becomes eligible again, not its dispatch order. Re-entry restores the request's original deadline in the origin queue, so tier priority is applied by the normal dispatch path when it's retried) |
What does this PR do?
Routes sorted-set retries through the retry queue instead of ZADDing them back into the request queue. Retries park in retry_queue_name (new sorted-set transport config field, default retry-sortedset) scored by retry-due time, and a retry mover re-enters due messages into their origin queue with the original deadline as the score.
Why
Currently,
flushRetryBatchwrites score now+backoff into the request queue, which changes where it sits in the queue but not when it becomes eligible, so instead of waiting out the backoff a retry is dispatched as soon as it reaches the head, about 12 times per second at a 100ms poll interval against a persistently unavailable upstream. This means a retry is scored now+backoff as if its deadline were at most 60 seconds away, so it will pop ahead of all messages with more time than that remaining (which under minute-scale deadlines would be nearly all of them).Fixes: #391
How was this tested?
Unit tests cover parking (due-time score in the retry queue, request queue untouched before the backoff elapses), the mover re-entering with the deadline score, and the first-queue fallback for retries without an origin queue. Verified E2E on a GKE cluster against an always-429 upstream: the same probe that previously showed 593 attempts in 50 seconds now follows the exponential curve (3 attempts by 13s, 5 by 70s) with the message parked in retry-sortedset between attempts.
Release note