Repository navigation
fix(connector): clamp exponential backoff shift to avoid uint32 wraparound - #109
Conversation
…round calculateIncrementalDelay does uint32(1 << redeliveryCount) for exponential backoff. Once redeliveryCount hits 32 that shift wraps a uint32 to 0, so a message that's been redelivered that many times gets an instant redelivery instead of the configured max delay - worse than no backoff at all. Clamp the shift amount before applying it. Signed-off-by: Akanksha Trehun <akankshatrehun@gmail.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Verified the fix: Go's shift operator on uint32 is defined for any shift count (unlike C), so 1 << 32 legitimately wraps to 0 — the bug as described is real and the fix (clamping the shift to 31 once redeliveryCount >= 32) is correct.
Checked the boundary cases:
redeliveryCount <= 31:shiftis unchanged, so behavior is identical to before (no regression).redeliveryCount >= 32:shiftclamps to 31, giving2^31(fits safely inuint32, no overflow), which then gets capped bymaxRedeliveryDelayMultiplieras before instead of wrapping to 0.
The added test (redeliveryCount: 32) exercises the public Next() path end-to-end and matches this. No blockers — LGTM.
2 participants
Noticed this while reading through the DLQ backoff policy -
calculateIncrementalDelay's exponential path doesuint32(1 << redeliveryCount). OnceredeliveryCountreaches 32, that shift wraps a uint32 around to 0, so a message that's been redelivered that many times gets an instant redelivery instead of the configured max delay. That's the opposite of what backoff is for - a message stuck failing for a while ends up getting hammered instead of throttled.Clamped the shift so it stops growing at the point where it would overflow, same behavior as before for every redelivery count anyone's actually likely to hit, correct behavior past it.
Added a test at redeliveryCount 32 with a max configured - confirmed it returns 0s on the old code and the capped delay now.