fix(#2343): add post-reset spread to _github_csma_sleep_after_rate_limit - #2344
Conversation
PR #2304 added post-reset spread to github_csma_sense to prevent thundering herd when runners wake after a rate-limit reset. The structurally parallel _github_csma_sleep_after_rate_limit function was missing the same treatment — multiple runners hitting a 429 would all wake at the same reset timestamp and fire simultaneously. Extract the spread logic into a shared _github_csma_post_reset_spread helper and call it from both github_csma_sense (replacing the inline code) and _github_csma_sleep_after_rate_limit (added after the backoff sleep). Both paths now use GITHUB_CSMA_SPREAD_MAX_SEC to stagger runner wake times. Note: pre-commit and make lint could not run due to shellcheck-py network restriction in sandbox. Scaffold Go tests pass. Closes #2343
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
Site previewPreview: https://37674be6-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 5:29 PM UTC · Completed 5:39 PM UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ReviewFindingsLow
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| @@ -176,6 +182,9 @@ _github_csma_sleep_after_rate_limit() { | |||
| fi | |||
There was a problem hiding this comment.
[low] edge-case
After this PR, a retry iteration can accumulate two spread delays: one from _github_csma_post_reset_spread at the end of _github_csma_sleep_after_rate_limit, and a second from _github_csma_post_reset_spread inside github_csma_sense at the top of the next loop iteration. Worst case ~2x GITHUB_CSMA_SPREAD_MAX_SEC (default 120s). Unlikely in practice because the backoff already tries to sleep until the reset timestamp.
| if (( spread_max > 0 )); then | ||
| local spread_secs=$(( RANDOM % spread_max )) | ||
| echo "Rate limit reset — spreading ${spread_secs}s to desync from other runners..." >&2 | ||
| sleep "${spread_secs}" |
There was a problem hiding this comment.
[low] code-comment-accuracy
The echo message Rate limit reset inside _github_csma_post_reset_spread is specific to the github_csma_sense context. When called from _github_csma_sleep_after_rate_limit, the sleep may be exponential backoff rather than a reset-timestamp wait, though the function preferentially uses the reset timestamp so the message is accurate for the common path.
Suggested fix: Optionally use a more generic message, e.g.: Post-sleep spread — adding ${spread_secs}s delay to desync from other runners...
fullsend-ai-review
Bot
added
the
ready-for-merge
|
🤖 Finished Retro · ✅ Success · Started 5:48 PM UTC · Completed 5:56 PM UTC |
Retro: PR #2344 — Add post-reset spread to CSMA rate-limit sleepVerdict: Clean workflow, no new proposals. This PR demonstrates the full retro-to-merge pipeline working well. The retro agent on PR #2304 identified a missed thundering-herd fix in Timeline
What went well
Minor gaps (all covered by existing issues)
No new proposals filed — existing open issues already cover the improvement opportunities identified. |
1 participant
PR #2304 added post-reset spread to github_csma_sense to prevent thundering herd when runners wake after a rate-limit reset. The structurally parallel _github_csma_sleep_after_rate_limit function was missing the same treatment — multiple runners hitting a 429 would all wake at the same reset timestamp and fire simultaneously.
Extract the spread logic into a shared _github_csma_post_reset_spread helper and call it from both github_csma_sense (replacing the inline code) and _github_csma_sleep_after_rate_limit (added after the backoff sleep). Both paths now use GITHUB_CSMA_SPREAD_MAX_SEC to stagger runner wake times.
Note: pre-commit and make lint could not run due to shellcheck-py network restriction in sandbox. Scaffold Go tests pass.
Closes #2343
Post-script verification
agent/2343-csma-spread-rate-limit)df020f5fe71b7a899cd6cb391cf6e701e124548d..HEAD)