Extension A completion — secondary-limit escalation and refresh-pool fairness - #33
Merged
Merged
Conversation
…fairness Closes #9 Extension A's ten child items were delivered across PRs 4, 6, 7 and 9. Auditing the merged code against the plan's own wording found two of them incomplete. Item 8 — RateLimitPolicy#fallback_instant passed a literal 1 as PollBackoff's attempt, fixing exponent = 0 and base = 60 on every call. §10's "≥ 1 minute" held; its "with exponential backoff" did not. A new github_api_budget.consecutive_secondary_limits counter, incremented under the same row lock that writes the block, gives 60/120/240/… capped at one hour. It survives window rollover because secondary limits are IP-scoped, and one live request that completes without a secondary limit clears it. Item 9 — Enrichment::Fairness#refresh_choice took the first refreshable class in EntityType order and read the pending eligibility map for its borrow test, so actor could spend the whole allowance on refreshes while repository's untouched guarantee and eligible stale rows were never selected. It now mirrors #pending_choice: prefer a class with room, borrow only from a class with nothing to refresh, over a refresh-scoped eligibility map. CandidateSelector#pending_available? is unchanged. Also: RateLimitSnapshot parses both RFC 9110 Retry-After forms; specs pin that jitter reaches global_blocked_until through the production wiring; and spec/config/github_initializer_spec.rb covers the to_prepare registration that nothing referenced before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Closes #9
Why this PR exists
Issue #9 tracks Extension A — Rate Limiting and Fan-Out Control (
IMPLEMENTATION_PLAN.md§4). Its three sub-issues — #14 (PR 4), #16 (PR 6), #19 (PR 9) — are all merged, and #17 (PR 7) delivered the fairness andeffective_enrichment_timehalves. Auditing all ten child items against the merged code, rather than against the issue checklists, found eight complete and two incomplete. Both were invisible to their existing tests because each test set up a single-class world.Defect 1 — item 8's exponential backoff did not escalate
§10: "set
global_blocked_untilfromRetry-After(or ≥ 1 minute with exponential backoff when the header is absent)".RateLimitPolicy#fallback_instantcalled@backoff.retry_at(1, now:)with a literal1.PollBackoff#delay_forcomputesexponent = [attempt, 1].max - 1, so attempt1fixedexponent = 0andbase = 60on every call. The ≥ 1 minute floor and the jitter were both real; the exponential was not. Nothing compensated —Ingestion::PollStateroutes a:secondary_limitedoutcome tosecondary_retry, which deliberately does not incrementconsecutive_failures, so the source-scoped ladder never advanced either. An IP GitHub was actively throttling was re-probed every ~60–75s for as long as the throttling lasted.Fix. A
consecutive_secondary_limitscounter on the budget singleton, incremented as a bind onBLOCK_SQL— under the sameSELECT … FOR UPDATEthat writes the block it feeds.#secondary_limitreads it and passesstreak + 1, producing 60 → 120 → 240 → … capped at one hour by the existing ceilings.Four boundaries keep it minimal:
Retry-Afteris still obeyed withinhonoured's clamp, which is how §10 words the alternative.:secondary_rate_limitadvances it. A primary exhaustion and a reserve breach are conditions of the budget window, not evidence this IP is being throttled —#primary_limitpasses a literal1for the same reason.ROLL_WINDOW_SQLresets. Zeroing it at the boundary would hand a persistently throttled IP a fresh 60s block every hour.#apply!callsclear_secondary_limit_streak!for any request that completed, including a304— a request GitHub answered and charged for is the same evidence a200carries. A timeout or 5xx produced no verdict, so it neither escalates nor clears.Defect 2 — item 9's refresh pool starved a class
§10:898 scopes refreshes "within each class's share".
Enrichment::Fairness#refresh_choiceselectedrequested.find { refresh_available? }— first inEntityType.allorder, always actor — with no preference for a class still inside its guarantee, unlike#pending_choice. Its borrow test also read the pending eligibility map, so the other class's stale rows were invisible.Concretely, with guarantees at 20/20,
actor_share_used: 20,repository_share_used: 0, no pending work anywhere and stale rows in both classes: actor borrowed from 20 to 40 while repository's untouched 20-request guarantee and eligible refresh work were never selected. That is the starvation the split exists to prevent (§10: "a naive repo-first policy would starve actor enrichment to zero indefinitely"), reproduced one pool down with the order reversed.Fix.
#refresh_choicenow performs#pending_choice's two steps over a refresh-scoped eligibility map: prefer a class withroom_within_guarantee?, then borrow only from a class with nothing to refresh. A borrowed refresh reportsborrowed_refresh, mirroringborrowed_pending.CandidateSelector#pending_available?is unchanged and stays pending-only. Its documented reason still holds — counting refreshes in the pending borrow test would let a refresh outrank a never-enriched candidate and invert §10's ladder — and that hazard cannot arise inside#refresh_choice, which is only reached when no class has a pending candidate at all.Also included
Retry-AfterHTTP-date form. RFC 9110 permits both forms and §10 lists the header among those to process.RateLimitSnapshotnow normalizes both to a delta against its ownobserved_at; leaving the date form unread collapsed a server-supplied "wait 45 minutes" into the 60s fallback. A past date yields a non-positive delta, which#fallback_instantalready treats as absent.global_blocked_until. Every existing example injectedRandomwithrand: 0.0, so the assertions would have passed with jitter removed from the production path.spec/config/github_initializer_spec.rb. Nothing referencedconfig/initializers/github.rb— deleting itsto_prepareregistration left the whole suite green, so item 2's "computed at startup" had no test behind it.Rejected alternatives
ACTOR_ENRICHMENT_SHAREat0.0/1.0) could then never refresh, and capacity would sit idle whenever the other class has no stale rows — while §10:812's borrowing rule is stated generally rather than scoped to one pool.BLOCK_SQL'sGREATESTmeans a shorter instant can never shorten a longer one.Verification
Both fixes are regression-proven: reverting
fairness.rbfails 4 of the new examples (including "chooses the class that still has room, not the first in class order"), and revertingrate_limit_policy.rbfails 4 more (including "escalates exponentially from the minimum block").1720 examples, 0 failures10 examples, 0 failuresbin/ingest,bin/enrich --limit 6,solid_queue:check— exit 0Deferred
PR 12 scope (#22); the
RateLimitPolicy/BudgetLedgerdisagreement over a non-corex-ratelimit-resourceon a 403/429 (unreachable today — every URL this app issues is acoreendpoint); and the stale-validator case where a page-one200carries noETagheader (no budget or correctness impact).🤖 Generated with Claude Code