Skip to content

fix: widen CSMA post-reset jitter to prevent thundering herd - #2304

Merged
ralphbean merged 1 commit into
mainfrom
fix/csma-jitter-window
Jun 16, 2026
Merged

fix: widen CSMA post-reset jitter to prevent thundering herd#2304
ralphbean merged 1 commit into
mainfrom
fix/csma-jitter-window

Conversation

@ralphbean

Copy link
Copy Markdown
Member

Summary

  • After a rate-limit sleep, all parallel runners wake at the same reset timestamp and collide within a 750ms jitter window
  • This caused unknown owner type errors from gh project view — its internal owner-resolution GraphQL call got trampled by sibling runners
  • Adds a post-reset spread of up to 60s (GITHUB_CSMA_SPREAD_MAX_SEC) so runners fan out over a wide window after waking

Context

Observed in https://github.com/fullsend-ai/.fullsend/actions/runs/27572357937/job/81511878257 — the prioritize agent succeeded but post-prioritize.sh failed because the CSMA sense function woke all runners simultaneously after a graphql rate limit reset.

Test plan

  • Existing post-prioritize-test.sh passes
  • Run a batch of 5+ prioritize jobs and observe post-scripts no longer collide after rate limit resets

When multiple runners exhaust the GraphQL rate limit simultaneously,
they all sleep until the same reset timestamp and wake up together.
The existing slot jitter (250-750ms) is too narrow to desynchronize
them, causing collisions that surface as "unknown owner type" errors
from gh project view.

Add a post-reset spread of up to 60s (configurable via
GITHUB_CSMA_SPREAD_MAX_SEC) so runners fan out over a wide window
after waking from a rate-limit sleep.

Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
@github-actions

Copy link
Copy Markdown

Site preview

Preview: https://44db34ab-site.fullsend-ai.workers.dev

Commit: 80a414d73e5833f3cde9bbe088cd3d6cb3c178f8

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 15, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:37 PM UTC · Completed 8:47 PM UTC
Commit: 80a414d · View workflow run →

@codecov

codecov Bot commented Jun 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@fullsend-ai-review

Copy link
Copy Markdown

Review

Findings

Medium

  • [incomplete fix / missing spread in retry path] internal/scaffold/fullsend-repo/scripts/lib/github-api-csma.sh:142_github_csma_sleep_after_rate_limit (lines 142-164) also sleeps until a rate-limit reset timestamp and is called on 429/secondary-rate-limit retries. This path has the same thundering-herd problem the PR aims to fix: multiple runners hitting a 429 will all sleep until the same reset timestamp and wake simultaneously. The new post-reset spread is only added to github_csma_sense, not to _github_csma_sleep_after_rate_limit. Note: the exponential backoff component (github_csma_backoff) introduces some randomness via RANDOM % (base + 1), which partially mitigates the herd but does not fully address it since runners on the same attempt get the same range.
    Remediation: Add the same post-reset spread logic after the sleep "${delay}" call in _github_csma_sleep_after_rate_limit, or extract the spread into a helper and call it from both locations.

Low

  • [off-by-one / weak randomness range] internal/scaffold/fullsend-repo/scripts/lib/github-api-csma.sh:99 — Bash $RANDOM produces values in [0, 32767]. With spread_max=60, the modulo bias is negligible. The PID+time seeding concern is theoretically valid but practically irrelevant for GitHub Actions runners which start as separate VMs/containers.

  • [variable-naming-consistency] internal/scaffold/fullsend-repo/scripts/lib/github-api-csma.sh:99 — The new code uses spread_secs=$(( RANDOM % spread_max )) yielding [0, spread_max-1], meaning a spread of 0 seconds is possible (no delay) and the documented maximum of 60 is never reached. The existing github_csma_backoff function uses RANDOM % (base + 1) to include the upper bound. Consider using spread_secs=$(( RANDOM % spread_max + 1 )) to ensure a minimum 1-second spread.

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Jun 15, 2026

@rh-hemartin rh-hemartin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The title of this PR sounds like magic 🤣

@ralphbean

Copy link
Copy Markdown
Member Author

Lol :D

@ralphbean
ralphbean added this pull request to the merge queue Jun 16, 2026
Merged via the queue into main with commit 3c9f0db Jun 16, 2026
14 checks passed
@ralphbean
ralphbean deleted the fix/csma-jitter-window branch June 16, 2026 16:51
@fullsend-ai-retro

fullsend-ai-retro Bot commented Jun 16, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 4:56 PM UTC · Completed 5:03 PM UTC
Commit: 80a414d · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #2304 — fix: widen CSMA post-reset jitter to prevent thundering herd

Timeline: Human-authored PR by ralphbean (with Claude assistance) adding a post-reset spread of up to 60s to github_csma_sense so runners fan out after rate-limit resets. The review agent completed in ~13 minutes and identified one medium finding: the fix was applied to github_csma_sense but not to _github_csma_sleep_after_rate_limit, which has the same thundering-herd problem on 429/retry paths. A human reviewer approved the PR without addressing this finding, and it was merged.

Review quality: Good. The review agent correctly identified a legitimate incomplete-fix gap. The two low findings (off-by-one range, naming consistency) were reasonable but not high-signal.

Unresolved finding merged: The medium finding about the missing spread in the retry path was not addressed before merge. This is a known pattern already tracked by multiple open issues (#1956, #870, #1941, #1201), so no new proposal is filed for that workflow gap.

One proposal filed: A concrete bug for the missing post-reset spread in _github_csma_sleep_after_rate_limit, which was correctly identified by the review agent but not acted on.

Proposals filed

ifireball pushed a commit to ifireball/fullsend that referenced this pull request Jun 16, 2026
…ter_rate_limit

PR fullsend-ai#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 fullsend-ai#2343
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

requires-manual-review Review requires human judgment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants