Cut GitHub REST quota burn: ETag conditional GETs + throttled waiter pumps#18
Conversation
…pumps With ~7 concurrent 'crq loop' waiters plus the autoreview daemon, steady-state polling burned ~7300 REST requests/hour — 1.45x GitHub's 5000/hr account limit — so the account spent part of every window rate-limited. Two fixes: - The GitHub client now caches every 200 GET response under its ETag and sends If-None-Match on repeat requests. GitHub serves 304 Not Modified for free (no quota charge), so polling unchanged PRs, comments, reviews, and the crq-state ref/blobs costs nothing between actual changes. The cache is per-process, bounded (1024 entries, 1 MB/body), and 304 replays preserve headers so Link-based pagination stitches pages exactly as before. - Feedback waiters no longer Pump() on every poll tick. Each waiter now pumps at most once a minute (or once per poll when polling slower than that); the autoreview daemon already pumps every pass, so per-tick pumping from N concurrent waiters was pure redundancy. Claude-Session: https://claude.ai/code/session_015TSx8Erp1Q2yB6snkYPbAh
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds throttled queue pumping in ChangesPump Throttling
ETag Caching for Conditional GETs
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant GitHub.send
participant etags map
participant GitHub API
Caller->>GitHub.send: GET request
GitHub.send->>etags map: lookup cached entry
GitHub.send->>GitHub API: request with If-None-Match
GitHub API-->>GitHub.send: 304 Not Modified or 200 OK
GitHub.send->>etags map: replay cached body or store new response
GitHub.send-->>Caller: response
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab2a967719
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/crq/github.go`:
- Around line 556-564: The cache/replay path in `github.go` is discarding return
values from `io.Copy(io.Discard, resp.Body)` and the `resp.Body.Close()` call in
`cacheGET`, which triggers `errcheck`. Update both spots to explicitly ignore
the errors by assigning the results to `_` so the intent is clear, and keep the
behavior in the conditional response handling and `cacheGET` unchanged.
- Around line 108-118: The cacheGET helper in GitHub is truncating oversized
responses by reading only maxETagBody+1 bytes and then replacing resp.Body with
that partial buffer, so large CRQ-state blobs get corrupted instead of just
bypassing the cache. Update cacheGET to fully consume the response body, then
decide whether to store it in etagStore based on the full length and ETag; if it
exceeds maxETagBody, return the original full body to the caller without
caching. Keep the fix localized to cacheGET and its ETag/body handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 24e7ba48-4745-414e-8008-794a8a012cc4
📒 Files selected for processing (4)
internal/crq/feedback.gointernal/crq/feedback_test.gointernal/crq/github.gointernal/crq/github_test.go
📜 Review details
🧰 Additional context used
🪛 golangci-lint (2.12.2)
internal/crq/github.go
[error] 110-110: Error return value of resp.Body.Close is not checked
(errcheck)
[error] 557-557: Error return value of io.Copy is not checked
(errcheck)
🔇 Additional comments (3)
internal/crq/feedback.go (1)
264-267: LGTM!Also applies to: 301-309, 510-519
internal/crq/feedback_test.go (1)
665-672: LGTM!internal/crq/github_test.go (1)
344-472: LGTM!
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f9b68106c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Problem
With ~7 concurrent
crq loopwaiters plus the autoreview daemon running, steady-state polling burned a measured ~7,300 REST requests/hour — 1.45× GitHub's 5,000/hr per-account limit — so the account spent part of every hour rate-limited, starving every other tool on the token (gh CLI included). Each 15s wait tick cost ~4 uncached calls:Feedback()fetches, a fullPump(), and astore.Load()(GET /git/ref+GET /git/blobs).Fixes
ETag conditional requests (
github.go): every 200 GET response is cached under its ETag (per-process, bounded at 1024 entries / 1 MB per body) and repeat requests sendIf-None-Match. GitHub serves304 Not Modifiedwithout charging quota, so polling unchanged PRs, comments, reviews, and the crq-state ref/blobs is free between actual changes. 304 replays preserve headers, soLink-based pagination stitches pages exactly as before — covered by a dedicatedrequestPagedtest. Blob GETs are content-addressed, so those revalidations hit every time.Throttled waiter pumps (
feedback.go): feedback waiters no longerPump()on every poll tick — each waiter pumps at most once a minute (or once per poll when polling slower than that). The autoreview daemon already pumps every pass, so per-tick pumping from N concurrent waiters was pure redundancy.Impact
Steady-state quota cost of a waiting loop drops to near zero (revalidations are free; only actual changes pay). Even before considering ETags, pump throttling alone removes the largest per-tick write-path cost. The 7-loop scenario that burned 146% of quota now fits comfortably with room for interactive gh use.
Testing
go vet ./...and fullgo test ./...passhttps://claude.ai/code/session_015TSx8Erp1Q2yB6snkYPbAh
Summary by CodeRabbit