Pace bulk refresh against the GitHub API quota#473
Conversation
The refresh-* backfills and the startup open-pulls refresh share one GitHub token — and one hourly quota — with the live webhook/socket refreshes that keep Pulldasher current. A bulk run is a tight loop, so it drains the quota to zero and starves normal operation, taking Pulldasher down. The throttle plugin only reacts once the quota is already exhausted, which is too late to protect live traffic. Add a proactive pacer (lib/pacer.js). It observes x-ratelimit-* on every response (live calls included, so its view stays fresh) and gates only the bulk path: requests are spread across the rate-limit reset window, and bulk pauses entirely below a configurable reserve floor (config.github.bulkReserve, default 1000). The token-bucket cursor advances by the x-ratelimit-used delta between gates, so a pull's multi-call fan-out — and any live-traffic spike — pushes bulk's next slot out; bulk yields automatically on both signals. Gate at enqueue (pushAllOnQueue) and between fetch pages (pacedPaginate), never inside the shared queue consumer. The wait happens off the queue, so a paced or floor-paused backfill can't park the single consumer that webhook refreshes also depend on — live traffic keeps draining throughout. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Picks up master (merged into the base branch) under the stacked pacing work. Clean auto-merge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sctice-ifixit
dismissed stale reviews from ianrohde and djmetzle
The base branch was changed.
|
QA 🌷 per the description. |
| * | ||
| * This is the bulk path, so it paces enqueueing against the GitHub quota: | ||
| * `pacer.gate()` waits for each item's slot *before* pushing it, keeping the | ||
| * shared queue shallow. The wait happens here, off the queue — so a paced or | ||
| * floor-paused backfill never blocks the single consumer, and live | ||
| * webhook/socket refreshes keep draining promptly. |
There was a problem hiding this comment.
I don't understand..
A backfill is going to happen from the CLI, not the running app. i.e. they are in separate processes. They share the API key of course, but not the queue.
There was a problem hiding this comment.
Is the idea that pushOnQueue() is given higher priority than pushAllOnQueue()?
There was a problem hiding this comment.
A backfill is going to happen from the CLI, not the running app. i.e. they are in separate processes.
True except for refresh.openPulls(), which gets called by the running app on startup. It pushes to the same queue as web hooks and uses pushAllOnQueue() to do its work.
Is the idea that
pushOnQueue()is given higher priority thanpushAllOnQueue()?
Via a crude mechanism that sits in front of adding to the queue. pushAllOnQueue() waits for the rate limit before adding to the queue, so its waiting happens async independent of the queue processing, which itself happens async of the queue pushes. While it's waiting, it isn't adding things to the queue while web hooks are free to add to the queue during that time. If the bulk producer coroutine isn't rate-limited at all, it will add many items to the queue, the processing of which will be in direct competition with the web hook items.
|
I had a feeling that I remember doing this before.. or something very similar (with a quota buffer). Hmm... Looks like we deleted it when we upgraded the octokit api: #211 https://github.com/iFixit/pulldasher/blob/811a7a222b38c43e2a85009f4c282141aba4713e/lib/rate-limit.js Anyway, the octokit plugin supports throttling across instances but it requires a common storage medium (redis or similar) to coordinate. |
I believe this pull's approach should work across instances using the headers we get back from GH as the coordination mechanism. |
| return async function (githubResponses) { | ||
| for (const githubResponse of githubResponses) { | ||
| await pacer.gate(); | ||
| queue.push({ response: githubResponse, onFailure: onFailure }); |
There was a problem hiding this comment.
It still feels quite weird to be rate-limiting pushing things onto the queue.
i.e. pulling from the queue and executing the api call is what updates the knowledge of the world (the headers) and pushing to the queue is about future commitment.
I haven't hard time imagining if it work work out to have a queue that you're throttling additions to, but draining the queue is actually what gives you the feedback of allowable rates.
Maybe instead we throttle the actual thing that needs throttling to, but have different bulkReserve depending on the source of the action (smaller reserve for things triggered by the web-app and big reserve for cli-triggered actions). The refreshOpenPulls on load doesn't need special treatment as it lasts less than a minute and only happens on startup.
5 participants
Connects #467
Why
Stacked on #472 (review/merge that first). The bulk refreshes (
bin/refresh-*and the startup open-pulls refresh) share one GitHub token, and one hourly quota, with the live webhook/socket refreshes that keep Pulldasher current. A backfill is a tight loop, so it drains the quota to zero and starves normal operation, taking Pulldasher down. Pulldasher already had@octokit/plugin-throttling, and #472 added@octokit/plugin-retry, but both only react at exhaustion (429/5xx), which is too late. This adds a proactive pacer that reserves quota headroom for live traffic and spreads bulk work across the rate-limit reset window.What changed
lib/pacer.js, a process-wide pacer.observe(headers)recordsx-ratelimit-remaining/-reset/-usedfrom every response (live calls included, so its view stays current), fed by theafter/errorhooks ingit-manager.js.gate()waits before a bulk request. The purecomputePaceeven-spreads over the reset window (interval =(reset - now) / (remaining - reserve)) and hard-pauses below a configurable reserve floor (config.github.bulkReserve, default1000, 20% of the 5000/hr token). The token-bucket cursor advances by thex-ratelimit-useddelta between gates, so a pull's multi-call fan-out (and any live spike) pushes bulk's next slot out. Pacing tracks real consumption, and bulk yields automatically.pushAllOnQueue) and between fetch pages (pacedPaginateingit-manager.js), never in theNotifyQueueconsumer. A paged or floor-paused backfill can't block the single consumer that webhook refreshes also use, so live traffic keeps draining throughout. The single-item path (pushOnQueue, used by webhook/socket refreshes) is unchanged and never paces.config.example.jsdocumentsbulkReserve.I gated at enqueue rather than in the queue's pop handler on purpose. A
gate()wait in the pop handler blocks the single sharedNotifyQueueconsumer, so every webhook item queued behind a paused bulk item stalls too (up to the roughly 1h floor-pause). The one queue exists (commit041e000, 2016) to serialize refreshes and to never refresh the same issue twice concurrently (racing DB writes). A separate bulk queue would reintroduce that hazard, so enqueue-side gating keeps both guarantees and keeps the wait off the queue.Tests
5 unit tests for the pure
computePace(node --test): unknown quota allows, a stale reset window allows, at/below the reserve pauses to reset, the first gate records the usage baseline without delaying, and the cursor advances by the requests actually spent since the last gate (a 1-call gate vs an 8-call gate).Validation (QA)
Deployed to
pulldasher-dev. The startup open-pulls refresh (456 pulls) paced and spread across the reset window (pulldasher:pacerlogs), peaking around a 22s inter-slot delay during the initial enqueue burst and then settling.remainingheld between 4913 and 4930 throughout, never near the 1000 reserve, with 0 floor pauses. Live traffic kept flowing alongside the bulk work (581getPull/webhook events in a 2-minute window), 0 errors,restarts=0. The prod-style token reads a 5000/hr ceiling.