fix(lifecycle): reduce GitHub API rate limiting from batch enrichment bypass#906
Open
harsh-batheja wants to merge 8 commits intomainfrom
Open
fix(lifecycle): reduce GitHub API rate limiting from batch enrichment bypass#906harsh-batheja wants to merge 8 commits intomainfrom
harsh-batheja wants to merge 8 commits intomainfrom
Conversation
… bypass Three optimizations to prevent API storms in the lifecycle manager poll cycle: 1. **CRITICAL - maybeDispatchMergeConflicts**: Gate the getMergeability() fallback to only run when batch enrichment didn't run at all. Previously it called getMergeability() (3 REST calls) whenever hasConflicts was undefined, even when the batch had already fetched PR data. Now uses cachedData.hasConflicts ?? false when the batch ran. 2. **HIGH - maybeDispatchCIFailureDetails**: Use batch enrichment ciChecks when available instead of calling getCIChecks() (separate REST call) on every poll. The GraphQL batch query now fetches statusCheckRollup contexts (individual check names, statuses, URLs) alongside the rollup state. Falls back to getCIChecks() only when batch didn't run. 3. **MEDIUM - maybeDispatchReviewBacklog**: Throttle getPendingComments + getAutomatedComments API calls to at most once per 2 minutes per session. These were called every 30s even when nothing had changed. Impact: ~8-10 API calls/PR/poll reduced to ~2-4, enabling 3-4x more concurrent sessions before hitting GitHub's 5,000/hr REST limit. Also extends PREnrichmentData with ciChecks?: CICheck[] and adds parseCheckContexts() helper to graphql-batch.ts for parsing CheckRun and StatusContext nodes from the GraphQL statusCheckRollup.contexts field.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…ncated
When a PR has >20 CI checks, contexts(first: 20) silently truncates the
list. Setting ciChecks to undefined when pageInfo.hasNextPage is true
ensures maybeDispatchCIFailureDetails falls back to the getCIChecks()
REST call, which returns all checks without truncation.
Also adds pageInfo { hasNextPage } to the contexts GraphQL query so
truncation can be detected.
Add the new throttle map to the existing pruning loop that removes stale entries for sessions no longer in the session list. Previously the map was only cleared on terminal status transitions, leaving orphaned entries for sessions removed externally (killed + cleaned up without transition).
…xt conclusion Two fixes for automated review findings: 1. Bypass review backlog throttle when a transition reaction just fired for humanReactionKey or automatedReactionKey. The transitionReaction branch needs to read the current fingerprint via the API to record lastPendingReviewDispatchHash. Without bypassing, the throttle prevents this write and the next unthrottled poll sees a stale (empty) hash, clears the reaction tracker, and fires a duplicate dispatch. 2. Set conclusion on StatusContext nodes in parseCheckContexts() to match the REST getCIChecksFromStatusRollup() format (rawState.toUpperCase()). The CI failure fingerprint includes c.conclusion ?? '', so inconsistent conclusion values between GraphQL and REST paths caused phantom fingerprint changes when switching sources, triggering duplicate dispatches.
…pped Two consistency fixes in parseCheckContexts() vs the REST path: 1. NEUTRAL conclusion: was mapped to 'passed' (with SUCCESS), but mapRawCheckStateToStatus() in the REST path maps NEUTRAL to 'skipped'. Changed to treat NEUTRAL the same as SKIPPED. 2. CheckRun conclusion: was stored as the raw GraphQL string (may be lowercase). REST getCIChecks/getCIChecksFromStatusRollup always store conclusion as rawState.toUpperCase(). Now stores rawConclusion which is already uppercased during the status branching logic. Both fixes prevent phantom fingerprint changes when maybeDispatchCIFailureDetails switches between GraphQL batch and REST fallback across poll cycles.
parseCheckContexts() was mapping these conclusions to 'failed' via the else fallback, while mapRawCheckStateToStatus() in the REST path explicitly maps all of them to 'skipped'. Added them to the skipped branch alongside SKIPPED and NEUTRAL to fully mirror the REST mapping.
parseCheckContexts() mapped QUEUED and WAITING CheckRun statuses to 'running', but mapRawCheckStateToStatus() in the REST path maps both to 'pending'. Only IN_PROGRESS maps to 'running' in the REST path. Fixes fingerprint inconsistency when switching between GraphQL batch and REST fallback across poll cycles.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b6813b5. Configure here.
- STARTUP_FAILURE conclusion now falls through to the "skipped" branch (matching mapRawCheckStateToStatus() REST default) instead of the explicit failure enumeration catch-all - Null pageInfo guard prevents TypeError from typeof null === "object" JavaScript quirk when accessing hasNextPage on a null pageInfo field - Tests added for both cases
7 tasks
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
1 participant
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.
Summary
Fixes GitHub API rate limiting caused by commit 53ef778 (Apr 2) which added new poll-cycle handlers that created API call storms.
Before: ~8-10 API calls/PR/30s poll → 112 calls/min with 7 sessions → hits 5,000/hr limit in ~45min
After: ~2-4 API calls/PR/poll → 3-4x reduction, supporting 20-30+ sessions
Changes
1. CRITICAL —
maybeDispatchMergeConflicts: Kill getMergeability() fallbackgetMergeability()(3 REST calls) wheneverhasConflictswasundefinedin cached data — even when the batch had successfully fetched PR datacachedDatabeing absent (batch didn't run), not onhasConflictsbeing undefinedcachedData.hasConflicts ?? falsewhen batch ran2. HIGH —
maybeDispatchCIFailureDetails: Use batch ciChecksPR_FIELDSto includestatusCheckRollup.contexts.nodes(individual CI check names, statuses, URLs)parseCheckContexts()to parseCheckRunandStatusContextnodes intoCICheck[]ciChecks?: CICheck[]toPREnrichmentDatainterfacemaybeDispatchCIFailureDetailsnow usescachedEnrichment.ciCheckswhen available, falls back togetCIChecks()REST call only when batch didn't run3. MEDIUM —
maybeDispatchReviewBacklog: 2-minute throttlelastReviewBacklogCheckAt: Map<SessionId, number>per-session timestampgetPendingComments+getAutomatedCommentsAPI calls if checked within last 2 minutesTest plan
pnpm typecheckpasses for core and scm-github🤖 Generated with Claude Code