From 2fdd3d7eb4257fa37b29b7351e5c51f88a7de150 Mon Sep 17 00:00:00 2001 From: Umang Date: Fri, 31 Jul 2026 08:09:32 -0500 Subject: [PATCH] =?UTF-8?q?Extension=20A=20completion=20=E2=80=94=20second?= =?UTF-8?q?ary-limit=20escalation=20and=20refresh-pool=20fairness?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- README.md | 25 ++-- app/services/github/budget_ledger.rb | 72 +++++++++- app/services/github/enrichment/fairness.rb | 38 +++-- app/services/github/rate_limit_policy.rb | 51 +++++-- app/services/github/rate_limit_snapshot.rb | 35 ++++- ...e_secondary_limits_to_github_api_budget.rb | 28 ++++ db/schema.rb | 4 +- ...nrichment-fairness-shares-and-borrowing.md | 21 +++ ...it-escalation-and-refresh-pool-fairness.md | 127 ++++++++++++++++ spec/config/github_initializer_spec.rb | 74 ++++++++++ spec/db/schema_spec.rb | 1 + spec/models/github_api_budget_spec.rb | 25 ++++ spec/services/github/budget_ledger_spec.rb | 95 +++++++++++- .../github/enrichment/fairness_spec.rb | 57 +++++++- .../services/github/rate_limit_policy_spec.rb | 136 +++++++++++++++++- .../github/rate_limit_snapshot_spec.rb | 31 ++++ 16 files changed, 776 insertions(+), 44 deletions(-) create mode 100644 db/migrate/20260731120000_add_consecutive_secondary_limits_to_github_api_budget.rb create mode 100644 docs/adr/0010-secondary-limit-escalation-and-refresh-pool-fairness.md create mode 100644 spec/config/github_initializer_spec.rb diff --git a/README.md b/README.md index 8cbc262..1b0d408 100644 --- a/README.md +++ b/README.md @@ -656,9 +656,14 @@ EnrichmentRunner ────────────────┘ │ `global_blocked_until`, which only ever moves later. - **`Github::RateLimitPolicy`** decides *which* response warrants a global block and until when — primary exhaustion to the reset GitHub named, a reserve breach to the same, a - secondary limit to `Retry-After` clamped between one minute and one hour. Class - exhaustion deliberately writes nothing there: it is derived from the counters, so - polling running out never stops enrichment and vice versa. + secondary limit to `Retry-After` (either RFC 9110 form) clamped between one minute and one + hour. When a secondary limit carries no usable `Retry-After`, the block backs off + exponentially with jitter across *consecutive* limits — 60s, 120s, 240s, capped at one + hour — counted in `github_api_budget.consecutive_secondary_limits`, which survives a + window rollover because secondary limits are IP-scoped rather than window-scoped. One live + request that completes without a secondary limit resets the run. Class exhaustion + deliberately writes nothing there: it is derived from the counters, so polling running out + never stops enrichment and vice versa. - **`Github::UrlPolicy`** is the SSRF boundary. It rebuilds every URL from validated components, and a URL that arrived inside a GitHub payload or a `Link` header always clears the full live policy first. @@ -685,9 +690,12 @@ EnrichmentRunner ────────────────┘ │ - **`Github::Enrichment::Fairness`** applies §10's ladder: a never-enriched candidate in a class still inside its guarantee, then the same borrowing when the other class has no *currently eligible* candidate, then a TTL-stale refresh only when no pending candidate - is eligible anywhere. It decides; the ledger enforces, so a wrong answer produces a - refused reservation rather than an overspend - ([ADR 0007](docs/adr/0007-enrichment-fairness-shares-and-borrowing.md)). + is eligible anywhere. The refresh pool allocates by the same two steps — prefer a class + with room, borrow only from a class with nothing to refresh — so neither pool can starve a + class. It decides; the ledger enforces, so a wrong answer produces a refused reservation + rather than an overspend + ([ADR 0007](docs/adr/0007-enrichment-fairness-shares-and-borrowing.md), + [ADR 0010](docs/adr/0010-secondary-limit-escalation-and-refresh-pool-fairness.md)). - **`Github::Enrichment::Claim`** prevents two workers enriching one entity by leasing the row — a conditional `UPDATE` that pushes `next_retry_at` forward. One column, one meaning: the same predicate excludes leases, backoffs and secondary-limit deferrals from @@ -708,8 +716,9 @@ Decisions behind this are recorded in [`docs/adr/`](docs/adr/): advisory locks and the gate (0002), the source and transport seams (0003), the class-aware ledger (0004), at-least-once processing with idempotent writes (0005), decomposed poll deferral state (0006), enrichment fairness shares -and borrowing (0007), post-commit enqueue with entity-scoped reconciliation (0008), and -runtime source allocation with shared-IP observability (0009). +and borrowing (0007), post-commit enqueue with entity-scoped reconciliation (0008), +runtime source allocation with shared-IP observability (0009), and secondary-limit +escalation with refresh-pool fairness (0010). ## Continuous ingestion diff --git a/app/services/github/budget_ledger.rb b/app/services/github/budget_ledger.rb index 410222d..0eb6619 100644 --- a/app/services/github/budget_ledger.rb +++ b/app/services/github/budget_ledger.rb @@ -102,6 +102,13 @@ class BudgetLedger # to the reset that has now passed, while preserving a secondary-limit block that # extends beyond it — the two conditions §10 distinguishes. # + # consecutive_secondary_limits is deliberately absent, and its absence is the + # behaviour rather than an omission. §10 calls secondary limits IP-scoped, which is a + # different scope from the primary window this statement resets: a streak that happens + # to span a window boundary is still a streak, and zeroing it here would hand a + # persistently throttled IP a fresh 60-second block every hour. Only a live request + # that completes without a secondary limit ends it — see #clear_secondary_limit_streak!. + # # The four counters are parameters rather than literal zeros so a rollover # discovered *during* reconciliation can carry the request that produced the # response into the window GitHub says it belongs to. See #reconcile!. @@ -152,6 +159,12 @@ class BudgetLedger # COALESCE on window_status keeps the label's fate in the policy's hands without a # boolean bind: nil leaves it alone. # + # The increment on consecutive_secondary_limits is a bind of 1 or 0 for the same + # reason — one statement whose behaviour the caller selects, rather than a second + # statement that could interleave with this one between the SELECT ... FOR UPDATE and + # the commit. §10's exponential backoff needs the count, and the count has to be taken + # under the same row lock that writes the block it feeds. + # # lock_version is bumped by hand for the reason DEBIT_SQL gives — this is not an # Active Record save, and a stale in-memory row must still raise StaleObjectError # rather than clobber the counters. @@ -159,10 +172,26 @@ class BudgetLedger UPDATE github_api_budget SET global_blocked_until = GREATEST(global_blocked_until, ?), window_status = COALESCE(?, window_status), + consecutive_secondary_limits = consecutive_secondary_limits + ?, lock_version = lock_version + 1, updated_at = ? WHERE id = ? SQL + # The other half of §10's exponential backoff: what ends a streak. + # + # Guarded in the WHERE rather than by a read-then-write, so the common case — every + # successful poll and every successful enrichment, on a service that has never seen a + # secondary limit — costs one statement that matches no row and takes no lock. Zero + # affected rows is the expected outcome here, not the LedgerInvariantViolation that + # #debit! raises on the same condition — a debit that matches no row means capacity was + # granted and then not taken, and this statement makes no such promise. + CLEAR_SECONDARY_STREAK_SQL = <<~SQL.squish + UPDATE github_api_budget + SET consecutive_secondary_limits = 0, + lock_version = lock_version + 1, updated_at = ? + WHERE id = ? AND consecutive_secondary_limits > 0 + SQL + def initialize(configuration: Github.configuration, allocation: SourceAllocation.new(configuration: configuration)) @configuration = configuration @allocation = allocation @@ -315,20 +344,61 @@ def block_globally!(until_at:, reason:, window_status: nil, now: Time.current) end end + # Ends the consecutive-secondary-limit streak that Github::RateLimitPolicy escalates + # from. Called for any live request that completed without a secondary limit — §10's + # backoff is exponential in *consecutive* limits, so one clean response is what breaks + # the run. + # + # A 304 counts as clean. It is a completed conditional request that GitHub answered + # and charged for, which is exactly the evidence that the IP is no longer being + # throttled. + # + # No row lock and no rollover check: this only ever writes a zero, so the worst a + # concurrent block can do is win and leave the streak at 1, which is the value that + # block just observed anyway. + # + # @return [Symbol] :cleared when a streak was ended, :unchanged when there was none + def clear_secondary_limit_streak!(now: Time.current) + assert_committable! + + updated = GithubApiBudget.connection.exec_update( + ActiveRecord::Base.sanitize_sql_array([ CLEAR_SECONDARY_STREAK_SQL, now, SINGLETON_ID ]), + "Github::BudgetLedger ClearSecondaryStreak" + ) + + return :unchanged if updated.zero? + + # §11 puts budget state transitions at INFO, and this one closes a story the + # budget.global_block_set lines opened: an operator reading a run of escalating + # blocks needs the line that says the escalation stopped. + Rails.logger.info(event: "budget.secondary_limit_streak_cleared") + :cleared + end + private def apply_block!(budget, until_at:, reason:, window_status:, now:) + # Only a secondary limit advances the streak. A primary exhaustion and a reserve + # breach are conditions of the budget window, not of GitHub throttling this IP, and + # counting them would escalate a secondary block on evidence that has nothing to do + # with secondary limits. + increment = reason == :secondary_rate_limit ? 1 : 0 + GithubApiBudget.connection.exec_update( - ActiveRecord::Base.sanitize_sql_array([ BLOCK_SQL, until_at, window_status, now, SINGLETON_ID ]), + ActiveRecord::Base.sanitize_sql_array([ BLOCK_SQL, until_at, window_status, increment, now, SINGLETON_ID ]), "Github::BudgetLedger BlockGlobally" ) # §11 lists "global_blocked_until set/cleared" among the budget state transitions # that belong at INFO. The instant is the field an operator greps for when nothing # is happening and nothing looks wrong. + # + # consecutive_secondary_limits is reported post-increment, because the number that + # explains the length of *this* block is the one that produced it. Rails.logger.info(event: "budget.global_block_set", reason: reason, blocked_until: until_at.utc.iso8601, previous_blocked_until: budget.global_blocked_until&.utc&.iso8601, + consecutive_secondary_limits: budget.consecutive_secondary_limits + increment, window_status: window_status || budget.window_status) :blocked end diff --git a/app/services/github/enrichment/fairness.rb b/app/services/github/enrichment/fairness.rb index a915e03..eb5d0bd 100644 --- a/app/services/github/enrichment/fairness.rb +++ b/app/services/github/enrichment/fairness.rb @@ -27,7 +27,7 @@ class Fairness # happened rather than only that nothing did. class Choice < Data.define(:entity_type, :pool, :borrow, :reason) REASONS = %w[ - pending borrowed_pending refresh + pending borrowed_pending refresh borrowed_refresh class_exhausted globally_blocked window_uninitialized no_candidate ].freeze @@ -116,17 +116,35 @@ def pending_choice(budget, requested, eligible) # eligible". Read over every class, not only the requested one, so --class actor # cannot promote an actor refresh above a repository still waiting to be enriched # for the first time. + # + # Then §10:898's second constraint on the same line — refreshes run "within each + # class's share" — which makes this the same two-step #pending_choice performs, over + # a different pool: prefer a class that still has room, and only borrow when the + # other class genuinely has nothing to do. Picking the first refreshable class + # outright instead would hand actor every borrowed request in the window while + # repository's untouched guarantee and eligible stale rows were never selected, which + # is the class starvation the split exists to prevent, reproduced one pool down. + # + # The eligibility map is rebuilt over the *refresh* pool rather than reusing + # `eligible`. CandidateSelector#pending_available? is deliberately pending-only — + # counting refreshes there would let a refresh outrank a never-enriched candidate and + # invert §10's ladder — but that hazard cannot arise here: this method is only + # reached when no class has a pending candidate at all, so there is nothing left for + # a refresh to outrank. def refresh_choice(budget, requested, eligible, now:) return nil if eligible.values.any? - type = requested.find { |candidate| selector.refresh_available?(candidate, now: now) } - return nil if type.nil? + refreshable = EntityType.all.index_with { |type| selector.refresh_available?(type, now: now) } + available = requested.select { |type| refreshable.fetch(type) } + return nil if available.empty? + + within = available.find { |type| room_within_guarantee?(budget, type) } + return Choice.new(entity_type: within, pool: :refresh, borrow: false, reason: "refresh") if within + + borrower = available.find { |type| other_classes_quiet?(type, refreshable) } + return nil if borrower.nil? - borrow = !room_within_guarantee?(budget, type) - # With no pending candidate anywhere, "the other class has no currently eligible - # candidate" is true by definition, so a refresh past the guarantee is a legal - # borrow rather than a starve. - Choice.new(entity_type: type, pool: :refresh, borrow: borrow, reason: "refresh") + Choice.new(entity_type: borrower, pool: :refresh, borrow: true, reason: "borrowed_refresh") end # Derived from the *stored* enrichment_allowance, exactly as @@ -142,6 +160,10 @@ def room_within_guarantee?(budget, entity_type) # §10's borrowing condition, verbatim: the other class has no CURRENTLY ELIGIBLE # candidate, not merely no rows. + # + # Generic over whichever eligibility map it is handed, because the condition is the + # same question asked of whichever pool is being allocated: #pending_choice passes + # the pending map, #refresh_choice the refresh one. def other_classes_quiet?(entity_type, eligible) eligible.except(entity_type).values.none? end diff --git a/app/services/github/rate_limit_policy.rb b/app/services/github/rate_limit_policy.rb index 3381837..9f86de5 100644 --- a/app/services/github/rate_limit_policy.rb +++ b/app/services/github/rate_limit_policy.rb @@ -66,7 +66,16 @@ def initialize(ledger: BudgetLedger.new, backoff: PollBackoff.new) # @return [Decision] def apply!(fetched, now: Time.current) decision = decide(fetched, now: now) - return decision unless decision.blocking? + + unless decision.blocking? + # A live request that completed without a secondary limit is the evidence that the + # IP is no longer being throttled, and §10's backoff is exponential in + # *consecutive* limits. Asked of successful? rather than of the decision alone: a + # 5xx or a timeout produced no verdict from GitHub about throttling, so it must + # neither escalate the streak nor end it. + @ledger.clear_secondary_limit_streak!(now: now) if fetched.successful? + return decision + end @ledger.block_globally!(until_at: decision.blocked_until, reason: decision.kind, window_status: decision.window_status, now: now) @@ -92,7 +101,11 @@ def decide(fetched, now:) # would spend its whole allowance re-asking a quota that is provably at zero. def primary_limit(fetched, now:) snapshot = fetched.rate_limit(observed_at: now) - blocked_until = snapshot&.reset_at || fallback_instant(snapshot, now: now) + # attempt: 1 — a primary exhaustion is not escalated on the secondary streak. The + # two conditions are unrelated: §10 attaches exponential backoff to secondary limits + # specifically, and a quota that is provably at zero is relieved by the window + # rolling, not by waiting longer each time. + blocked_until = snapshot&.reset_at || fallback_instant(snapshot, now: now, attempt: 1) Decision.new(kind: :primary_rate_limit, blocked_until: blocked_until, source_retry_at: nil, window_status: blocked_window_status) @@ -111,8 +124,15 @@ def primary_limit(fetched, now:) # exactly when the window rolls, and rollover is the transition that restores the # label; a secondary limit expires on an unrelated Retry-After and has no writer to # put the label back, so it would strand a value with no way out. + # The streak read here is stale by construction, and acceptably so. #apply! runs after + # the gate is released, so two responses can read the same count and compute the same + # block. The consequence is an under-escalated block and never an over-escalated one, + # and BLOCK_SQL's GREATEST means the loser's shorter instant cannot shorten the + # winner's. Computing it inside the ledger under the row lock would remove the staleness + # and cost the split this class exists for: it decides, the ledger records. def secondary_limit(fetched, now:) - blocked_until = fallback_instant(fetched.rate_limit(observed_at: now), now: now) + attempt = consecutive_secondary_limits + 1 + blocked_until = fallback_instant(fetched.rate_limit(observed_at: now), now: now, attempt: attempt) Decision.new(kind: :secondary_rate_limit, blocked_until: blocked_until, source_retry_at: blocked_until, window_status: nil) @@ -134,16 +154,27 @@ def reserve_breach(fetched, now:) source_retry_at: nil, window_status: blocked_window_status) end - # Retry-After may be absent, zero, negative, or an HTTP-date — RateLimitSnapshot - # parses only the delta-seconds form and deliberately leaves the rest nil. All four - # mean the same thing here: no usable instruction, so back off on our own terms. - # `now + nil` raises and `now + nil.to_i` is no block at all, which is the response - # most likely to escalate GitHub's throttling. - def fallback_instant(snapshot, now:) + # Retry-After may be absent, zero, negative, or unparseable. All of those mean the same + # thing here: no usable instruction, so back off on our own terms. `now + nil` raises + # and `now + nil.to_i` is no block at all, which is the response most likely to + # escalate GitHub's throttling. + # + # A server-supplied instruction is obeyed as given (within honoured's clamp), so + # `attempt` reaches PollBackoff only on the header-absent path — which is exactly where + # §10 puts the exponential: "from Retry-After (or >= 1 minute with exponential backoff + # when the header is absent)". + def fallback_instant(snapshot, now:, attempt:) seconds = snapshot&.retry_after_seconds return now + honoured(seconds) if seconds.is_a?(Integer) && seconds.positive? - @backoff.retry_at(1, now: now) + @backoff.retry_at(attempt, now: now) + end + + # Zero when no row exists yet: a secondary limit before the ledger is bootstrapped has + # no history to escalate from, and PollBackoff's floor still gives it the minute §10 + # requires. + def consecutive_secondary_limits + GithubApiBudget.find_by(id: GithubApiBudget::SINGLETON_ID)&.consecutive_secondary_limits.to_i end def honoured(seconds) diff --git a/app/services/github/rate_limit_snapshot.rb b/app/services/github/rate_limit_snapshot.rb index 2bae28d..437fff7 100644 --- a/app/services/github/rate_limit_snapshot.rb +++ b/app/services/github/rate_limit_snapshot.rb @@ -24,11 +24,13 @@ def from_headers(headers, observed_at:) used: integer(headers["x-ratelimit-used"]), reset_at: epoch(headers["x-ratelimit-reset"]), poll_interval_seconds: integer(headers["x-poll-interval"]), - # Seconds only. GitHub documents a delta, and the HTTP-date form is left - # unparsed on purpose: §10 already defines the fallback for a missing - # Retry-After (at least a minute, with exponential backoff), so nil is a - # handled outcome rather than a gap. - retry_after_seconds: integer(headers["retry-after"]), + # Both RFC 9110 forms, normalized to a delta. GitHub documents delta-seconds and + # is the only server this application talks to, but Retry-After is one of §10's + # "headers to process" and an unread HTTP-date silently collapses a + # server-supplied "wait 45 minutes" into the 60-second fallback — obeying an + # instruction far shorter than the one that was given is the response most likely + # to provoke further throttling. + retry_after_seconds: retry_after(headers["retry-after"], observed_at: observed_at), etag: presence(headers["etag"]), observed_at: observed_at ) @@ -50,6 +52,29 @@ def epoch(value) seconds = integer(value) seconds && Time.zone.at(seconds) end + + # Delta-seconds first, since that is what GitHub sends and what the majority of + # responses carry. The date form is resolved against observed_at rather than + # Time.current so the snapshot stays a pure function of its inputs — the same headers + # and the same observed_at always yield the same delta, which is what lets a spec + # assert an instant without freezing the clock. + # + # A date already in the past yields a non-positive delta. That is deliberately not + # normalized to nil here: this class reads and never decides, and + # Github::RateLimitPolicy#fallback_instant already treats non-positive exactly as it + # treats absent. + def retry_after(value, observed_at:) + integer(value) || http_date_delta(value, observed_at: observed_at) + end + + def http_date_delta(value, observed_at:) + text = presence(value) + return nil if text.nil? || observed_at.nil? + + (Time.httpdate(text) - observed_at).round + rescue ArgumentError + nil + end end # Whether there is enough here to reconcile the ledger's window at all. A 500 from diff --git a/db/migrate/20260731120000_add_consecutive_secondary_limits_to_github_api_budget.rb b/db/migrate/20260731120000_add_consecutive_secondary_limits_to_github_api_budget.rb new file mode 100644 index 0000000..dbf4609 --- /dev/null +++ b/db/migrate/20260731120000_add_consecutive_secondary_limits_to_github_api_budget.rb @@ -0,0 +1,28 @@ +# §10: "On any secondary-limit response: set global_blocked_until from Retry-After (or +# >= 1 minute with exponential backoff when the header is absent)." +# +# The exponential half needs a count of consecutive secondary limits, and that count has +# to outlive the process that observed them: the poller, the worker, and the one-shot all +# issue live requests against one IP-scoped limit, so a streak observed by the worker must +# escalate the block the poller then honours. It therefore lives on the same singleton row +# as global_blocked_until rather than in memory. +# +# Deliberately *not* reset by ROLL_WINDOW_SQL. A secondary limit is IP-scoped and has no +# relationship to the primary rate-limit window (§10), so a streak that spans a window +# boundary is still a streak. Github::RateLimitPolicy clears it on the first live request +# that completes without one. +class AddConsecutiveSecondaryLimitsToGithubApiBudget < ActiveRecord::Migration[8.1] + def change + add_column :github_api_budget, :consecutive_secondary_limits, :integer, + null: false, default: 0 + + # A separate named constraint rather than a rewrite of + # github_api_budget_counters_nonnegative. That one is a single expression covering the + # reservation counters; folding an unrelated counter into it would mean dropping and + # recreating a constraint every future column touches, and the failure message would + # then name seven columns that are fine. + add_check_constraint :github_api_budget, + "consecutive_secondary_limits >= 0", + name: "github_api_budget_secondary_limits_nonnegative" + end +end diff --git a/db/schema.rb b/db/schema.rb index 15037c8..d924955 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_07_31_100000) do +ActiveRecord::Schema[8.1].define(version: 2026_07_31_120000) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" @@ -63,6 +63,7 @@ create_table "github_api_budget", id: :integer, default: 1, force: :cascade do |t| t.integer "actor_share_used", default: 0, null: false + t.integer "consecutive_secondary_limits", default: 0, null: false t.datetime "created_at", null: false t.integer "enrichment_allowance", default: 0, null: false t.integer "enrichment_used", default: 0, null: false @@ -80,6 +81,7 @@ t.datetime "updated_at", null: false t.datetime "window_initialized_at" t.text "window_status", default: "uninitialized", null: false + t.check_constraint "consecutive_secondary_limits >= 0", name: "github_api_budget_secondary_limits_nonnegative" t.check_constraint "id = 1", name: "github_api_budget_singleton" t.check_constraint "poll_allowance >= 0 AND poll_used >= 0 AND enrichment_allowance >= 0 AND enrichment_used >= 0 AND actor_share_used >= 0 AND repository_share_used >= 0 AND reserve >= 0 AND (\"limit\" IS NULL OR \"limit\" >= 0) AND (remaining IS NULL OR remaining >= 0)", name: "github_api_budget_counters_nonnegative" t.check_constraint "window_status = ANY (ARRAY['uninitialized'::text, 'active'::text, 'globally_blocked'::text])", name: "github_api_budget_window_status_check" diff --git a/docs/adr/0007-enrichment-fairness-shares-and-borrowing.md b/docs/adr/0007-enrichment-fairness-shares-and-borrowing.md index 6a083b0..fd553b5 100644 --- a/docs/adr/0007-enrichment-fairness-shares-and-borrowing.md +++ b/docs/adr/0007-enrichment-fairness-shares-and-borrowing.md @@ -128,3 +128,24 @@ from inside the most contended row lock in the application would invert the lock type. The failure is precision rather than soundness — the sum invariant survives, because the repository guarantee is a subtraction — but it silently loses an attempt off the number on the page. + +## Amendment (2026-07-31): the refresh pool follows the same two steps + +This ADR decided how the pending pool allocates and left the refresh pool's *selection +order* unstated, and the shipped `#refresh_choice` did not follow it: it took the first +refreshable class in `EntityType.all` order — always actor — and set `borrow` from whether +that class happened to be past its guarantee. Its borrow test also read the pending +eligibility map, so the other class's stale rows were invisible to it. The result was the +starvation this ADR exists to prevent, reproduced one pool down: actor could spend the whole +enrichment allowance on refreshes while repository's untouched guarantee and eligible stale +rows were never selected. + +`#refresh_choice` now performs the same two steps as `#pending_choice` — prefer a class with +`room_within_guarantee?`, then borrow only from a class with nothing to refresh — over an +eligibility map built from the refresh pool. `CandidateSelector#pending_available?` is +unchanged and stays pending-only; decision 3's reasoning about the prioritization ladder is +about the *pending* borrow test and still holds. + +See [ADR 0010](0010-secondary-limit-escalation-and-refresh-pool-fairness.md) for the full +context, the rejected "refreshes never borrow" reading of §10:898, and the +`borrowed_refresh` choice reason. diff --git a/docs/adr/0010-secondary-limit-escalation-and-refresh-pool-fairness.md b/docs/adr/0010-secondary-limit-escalation-and-refresh-pool-fairness.md new file mode 100644 index 0000000..8c8a15a --- /dev/null +++ b/docs/adr/0010-secondary-limit-escalation-and-refresh-pool-fairness.md @@ -0,0 +1,127 @@ +# 10. Secondary-limit escalation and refresh-pool fairness + +Date: 2026-07-31 + +Status: Accepted + +## Context + +`IMPLEMENTATION_PLAN.md` §4's Extension A lists ten child capabilities. Nine shipped +across PRs 4, 6, 7 and 9. Reading the merged code against the plan's own wording, rather +than against the issue checklists, found two of them incomplete in ways their tests did not +catch — both because the test set up a single-class world in which the defect is invisible. + +**Item 8 — "Handle secondary rate limits globally (`Retry-After` → +`global_blocked_until`); add exponential backoff with jitter."** §10 words the fallback as +"set `global_blocked_until` from `Retry-After` (or ≥ 1 minute with exponential backoff when +the header is absent)". `Github::RateLimitPolicy#fallback_instant` called +`@backoff.retry_at(1, now:)` with a literal `1`. `Github::PollBackoff#delay_for` computes +`exponent = [attempt, 1].max - 1`, so attempt `1` fixes `exponent = 0` and `base = 60` on +every call. The ≥ 1 minute floor and the jitter were both real; the exponential was not. +Nothing else compensated — `Github::Ingestion::PollState` routes a `:secondary_limited` +outcome to `secondary_retry`, which deliberately does not increment `consecutive_failures`, +so the source-scoped ladder never advanced either. An IP GitHub was actively throttling was +re-probed every ~60–75 seconds for as long as the throttling lasted. + +**Item 9 — "Implement enrichment fairness shares (floor/remainder rounding) with +eligibility-aware borrowing."** §10:898 scopes refreshes "within each class's share". +`Github::Enrichment::Fairness#refresh_choice` selected +`requested.find { refresh_available? }` — first in `EntityType.all` order, always actor — +with no preference for a class still inside its guarantee, unlike `#pending_choice`, which +looks for `room_within_guarantee?` before it considers borrowing. Its borrow condition also +read the *pending* eligibility map, so the other class's stale refresh candidates were +invisible to it. 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 its way from 20 +to 40 while repository's untouched 20-request guarantee and eligible refresh work were never +selected. That is the class 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. + +## Decision + +### The secondary-limit streak is a counter on the budget singleton + +`github_api_budget.consecutive_secondary_limits`, `NOT NULL DEFAULT 0`, with its own named +check constraint. `Github::RateLimitPolicy#secondary_limit` reads it and passes +`streak + 1` as `PollBackoff`'s attempt, producing 60 → 120 → 240 → … capped at +`MAX_BLOCK_SECONDS` (3600) by the existing ceilings in both classes. Jitter is unchanged and +still additive-only, so the floor §10 states numerically is never undercut. + +Four boundaries make this the smallest correct change: + +- **It escalates only the header-absent path.** A server-supplied `Retry-After` is still + obeyed as given within `honoured`'s clamp, which is exactly how §10 words the alternative. +- **Only `:secondary_rate_limit` advances it.** A primary exhaustion and a reserve breach + are conditions of the budget window, not evidence that GitHub is throttling this IP. + `#primary_limit` passes a literal attempt of `1` for the same reason — a quota provably at + zero is relieved by the window rolling, not by waiting longer each time. +- **It survives window rollover.** §10 calls secondary limits IP-scoped, which is a + different scope from the primary window `ROLL_WINDOW_SQL` resets. Zeroing the streak at + the boundary would hand a persistently throttled IP a fresh 60-second block every hour. +- **One clean response ends it.** `#apply!` calls + `BudgetLedger#clear_secondary_limit_streak!` for any live request that completed — + including a `304`, which is a request GitHub answered and charged for and therefore the + same evidence a `200` carries. A timeout or a 5xx produced no verdict about throttling, so + it neither escalates nor clears. + +**The policy reads the count; the ledger writes it.** The increment is a bind on `BLOCK_SQL` +rather than a second statement, so it happens under the same `SELECT … FOR UPDATE` that +writes the block it feeds. The read in the policy is therefore stale by construction: +`#apply!` runs after the request gate is released, so two responses can read the same count. +That is accepted rather than removed. The consequence is an under-escalated block and never +an over-escalated one, `BLOCK_SQL`'s `GREATEST` means the loser's shorter instant cannot +shorten the winner's, and computing the instant inside the ledger would collapse the split +[ADR 0004](0004-class-aware-budget-ledger.md) draws between deciding and recording. + +`#clear_secondary_limit_streak!` guards in its `WHERE` (`AND consecutive_secondary_limits +> 0`) rather than reading first, so the overwhelmingly common case — every successful +request on a service that has never been throttled — is one statement that matches no row +and takes no lock. Zero affected rows is the expected outcome, so it deliberately does not +go through the `LedgerInvariantViolation` check `#debit!` applies to the same condition. + +### The refresh pool allocates the same way the pending pool does + +`#refresh_choice` now mirrors `#pending_choice`: build an eligibility map, prefer a class +with `room_within_guarantee?`, and only then borrow — and only from a class that has nothing +to do. A borrowed refresh reports `borrowed_refresh`, mirroring `borrowed_pending`. + +The eligibility map is rebuilt over the refresh pool rather than reusing the pending one. +`CandidateSelector#pending_available?` stays pending-only, and its documented reason stands: +counting refreshes in the *pending* borrow test would let a refresh outrank a never-enriched +candidate and invert §10's prioritization ladder. That hazard cannot arise inside +`#refresh_choice`, which is only reached when no class has a pending candidate at all — so +there is nothing left for a refresh to outrank. Keeping two maps preserves the invariant +verbatim instead of weakening it. + +## Consequences + +- A persistently throttled IP now backs off to an hour rather than re-probing every minute, + which is the behaviour §10 specified and the one least likely to escalate GitHub's + throttling further. +- One additional row read per secondary-limit response, and one guarded `UPDATE` per + successful live request that matches no row in the normal case. +- The refresh pool can no longer starve a class. Total enrichment spend is unchanged — the + ledger's cap was always the bound, and this is a fairness fix rather than an overspend fix. +- `Choice::REASONS` gains `borrowed_refresh`. The only consumer of `Choice#reason` is + `Github::EnrichmentRunner`'s deferral log, which reads it on the not-chosen path only. + +## Rejected alternatives + +**Escalate by multiplying an in-force block instead of counting.** No migration, but it can +only escalate while a block is still in the future — and the case §10 legislates for is the +repeat limit that arrives *after* the previous block expired, which is precisely when this +approach does nothing. + +**Read `Retry-After`'s HTTP-date form as unparseable, as before.** RFC 9110 permits both +forms and §10 lists the header among those to process without qualifying which. +`Github::RateLimitSnapshot` now normalizes both to a delta against its own `observed_at`; +leaving the date form unread silently collapsed a server-supplied "wait 45 minutes" into the +60-second fallback, and obeying an instruction far shorter than the one given is the +response most likely to provoke further throttling. A date already in the past yields a +non-positive delta, which `#fallback_instant` already treats exactly as an absent header. + +**Forbid borrowing in the refresh pool outright**, on the strictest reading of §10:898's +"within each class's share". Rejected: a class whose guarantee rounds to zero +(`ACTOR_ENRICHMENT_SHARE` at `0.0` or `1.0`) could then never refresh at all, and it would +leave capacity idle whenever the other class has no stale rows — while §10:812's borrowing +rule is stated generally rather than scoped to the pending pool. diff --git a/spec/config/github_initializer_spec.rb b/spec/config/github_initializer_spec.rb new file mode 100644 index 0000000..4a17af5 --- /dev/null +++ b/spec/config/github_initializer_spec.rb @@ -0,0 +1,74 @@ +require "rails_helper" + +# config/initializers/github.rb is the only thing that makes §10's "the allowance formula +# is computed at startup" true, and until this spec existed nothing referenced it: deleting +# the to_prepare registration left the whole suite green, because every example builds its +# own Github::Configuration and never depends on the one the boot resolved. +# +# The block is exercised through Rails.application.reloader.prepare!, which is what railties +# calls at boot (Finisher's :run_prepare_callbacks) and again on each development reload. +# Driving the real callback chain is the point — asserting against a proc pulled out of +# config.to_prepare_blocks would pass even if the registration were never installed. +RSpec.describe "the Github startup initializer" do + def boot = Rails.application.reloader.prepare! + + # §11 puts budget state transitions at INFO, and this line is the one an operator reads + # every other budget line against: it names the numbers the process is about to enforce + # before it issues a request. + it "logs the resolved allowances at boot" do + expect(Rails.logger).to receive(:info).with(hash_including( + event: "config.budget_resolved", mode: "live", + poll_allowance: 12, enrichment_allowance: 40, reserve: 8 + )) + + boot + end + + it "leaves Github.configuration validated and memoized for the process" do + boot + + expect(Github.configuration.allowances).to have_attributes(poll_allowance: 12, enrichment_allowance: 40) + end + + # The over-commitment the allowance formula cannot see: it counts one attempt per page, + # while §10 makes every retry and every redirect hop its own reservation. A warning and + # not a raise, because §10 requires runtime conditions to degrade rather than crash-loop. + describe "the amplification warning" do + # Built from an explicit hash rather than by mutating ENV, which would leak into + # whichever example ran next under config.order = :random. The initializer calls + # Github.reset! before Github.configuration, so stubbing the reader is what survives it. + def stub_configuration(**environment) + allow(Github).to receive(:configuration) + .and_return(Github::Configuration.new(environment.transform_keys(&:to_s))) + end + + it "warns when a single failing poll could out-spend the whole poll allowance" do + # ceil(3600/3600) * 1 * 1 = 1 poll attempt/hour against a worst case of 9. + stub_configuration("POLL_INTERVAL_SECONDS" => "3600") + + expect(Rails.logger).to receive(:warn).with(hash_including( + event: "config.amplification", poll_allowance: 1 + )) + + boot + end + + it "stays silent when the allowance covers the worst case" do + stub_configuration + + expect(Rails.logger).not_to receive(:warn).with(hash_including(event: "config.amplification")) + + boot + end + end + + # The rejection §10 states outright: "Startup validation rejects any configuration where + # poll_attempt_allowance + reserve >= effective_limit." Raising here stops the container + # rather than letting it poll into an over-commitment. + it "refuses to finish booting on a configuration that leaves no enrichment capacity" do + allow(Github).to receive(:configuration) + .and_return(Github::Configuration.new("POLL_INTERVAL_SECONDS" => "60", "MAX_PAGES_PER_POLL" => "2")) + + expect { boot }.to raise_error(Github::Errors::ConfigurationError) + end +end diff --git a/spec/db/schema_spec.rb b/spec/db/schema_spec.rb index 23f2bbe..57ec466 100644 --- a/spec/db/schema_spec.rb +++ b/spec/db/schema_spec.rb @@ -22,6 +22,7 @@ window_status window_initialized_at poll_allowance poll_used enrichment_allowance enrichment_used actor_share_used repository_share_used reserve + consecutive_secondary_limits observed_at lock_version created_at updated_at ]) end diff --git a/spec/models/github_api_budget_spec.rb b/spec/models/github_api_budget_spec.rb index 9df56be..f1b98ca 100644 --- a/spec/models/github_api_budget_spec.rb +++ b/spec/models/github_api_budget_spec.rb @@ -92,6 +92,31 @@ end end + # Not one of COUNTERS: nothing reserves against it and no allowance derives from it. It + # records how many secondary limits arrived in a row, which is what §10's exponential + # backoff escalates from. + describe "the consecutive-secondary-limit streak" do + it "starts at zero" do + expect(create_budget.consecutive_secondary_limits).to eq(0) + end + + it "rejects a negative count at the database level" do + budget = create_budget + + expect_violation(ActiveRecord::CheckViolation) do + described_class.where(id: budget.id).update_all(consecutive_secondary_limits: -1) + end + end + + # Its own named constraint rather than a clause folded into the reservation counters', + # so a violation names the column that is actually wrong. + it "carries its own named check constraint" do + names = ActiveRecord::Base.connection.check_constraints("github_api_budget").map(&:name) + + expect(names).to include("github_api_budget_secondary_limits_nonnegative") + end + end + describe "optimistic locking" do it "refuses a stale write so concurrent reservations cannot be lost" do create_budget diff --git a/spec/services/github/budget_ledger_spec.rb b/spec/services/github/budget_ledger_spec.rb index 956a139..c5f00a3 100644 --- a/spec/services/github/budget_ledger_spec.rb +++ b/spec/services/github/budget_ledger_spec.rb @@ -502,6 +502,93 @@ def superseding_snapshot end end + # The count §10's exponential backoff escalates from. It is written here, under the same + # row lock that writes the block it feeds, and read by Github::RateLimitPolicy. + describe "the consecutive-secondary-limit streak" do + def block(reason, until_at: frozen_time + 60) + ledger.block_globally!(until_at: until_at, reason: reason, now: frozen_time) + end + + it "counts a secondary limit" do + active_window + + 3.times { block(:secondary_rate_limit) } + + expect(current_budget.consecutive_secondary_limits).to eq(3) + end + + # A primary exhaustion and a reserve breach are conditions of the budget window, not + # evidence that GitHub is throttling this IP. Counting them would escalate a secondary + # block from a run that contained no secondary limits. + it "ignores the two block reasons that are not secondary limits" do + active_window + + block(:primary_rate_limit, until_at: frozen_time + 3600) + block(:reserve_reached, until_at: frozen_time + 3600) + + expect(current_budget.consecutive_secondary_limits).to eq(0) + end + + describe "#clear_secondary_limit_streak!" do + it "ends a run and reports that it did" do + active_window + 2.times { block(:secondary_rate_limit) } + + expect(ledger.clear_secondary_limit_streak!(now: frozen_time)).to eq(:cleared) + expect(current_budget.consecutive_secondary_limits).to eq(0) + end + + # The common case by an enormous margin: every successful request on a service that + # has never been throttled. It must cost one statement that matches nothing rather + # than a read, a lock, and a write. + it "is a silent no-op when there is no run" do + before_version = active_window.lock_version + + expect(ledger.clear_secondary_limit_streak!(now: frozen_time)).to eq(:unchanged) + expect(current_budget.lock_version).to eq(before_version) + end + + it "does nothing when no row exists" do + expect(ledger.clear_secondary_limit_streak!(now: frozen_time)).to eq(:unchanged) + expect(GithubApiBudget.count).to eq(0) + end + + it "refuses to run inside an application transaction" do + active_window + + expect do + ActiveRecord::Base.transaction(joinable: true) { ledger.clear_secondary_limit_streak!(now: frozen_time) } + end.to raise_error(Github::Errors::NestedTransaction) + end + end + + # §10 calls secondary limits IP-scoped, which is a different scope from the primary + # window ROLL_WINDOW_SQL resets. Zeroing the streak at the boundary would hand a + # persistently throttled IP a fresh 60-second block every hour. + it "survives a window rollover" do + active_window + 2.times { block(:secondary_rate_limit) } + + ledger.reserve!(:poll, now: frozen_time + 3601) + + expect(current_budget).to have_attributes(consecutive_secondary_limits: 2, + window_status: "uninitialized") + end + + # The counters the ledger reserves against are untouched by either write, which is why + # neither can return capacity a request already spent. + it "leaves the reservation counters alone" do + active_window + ledger.reserve!(:poll, now: frozen_time) + + block(:secondary_rate_limit) + ledger.clear_secondary_limit_streak!(now: frozen_time) + + expect(current_budget).to have_attributes(poll_used: 1, enrichment_used: 0, + actor_share_used: 0, repository_share_used: 0) + end + end + # Both guards used to key on the window_status string. That was safe while the column # held two values in practice; it stopped being safe once a global block could write a # third, so each now keys on the fact it actually means. @@ -531,9 +618,15 @@ def superseding_snapshot describe "failures stay spent (plan §7)" do # The guarantee is structural: there is no code path that gives a request back. + # + # #clear_secondary_limit_streak! writes one column, consecutive_secondary_limits, which + # no reservation reads and no counter derives from. It cannot return capacity for the + # same reason #block_globally! cannot: neither touches poll_used, enrichment_used, or + # either share counter. it "exposes no way to credit a reservation back" do expect(described_class.instance_methods(false)).to contain_exactly( - :reserve!, :reconcile!, :bootstrap!, :block_globally!, :configuration, :allocation + :reserve!, :reconcile!, :bootstrap!, :block_globally!, :clear_secondary_limit_streak!, + :configuration, :allocation ) end diff --git a/spec/services/github/enrichment/fairness_spec.rb b/spec/services/github/enrichment/fairness_spec.rb index eb22104..fa7896d 100644 --- a/spec/services/github/enrichment/fairness_spec.rb +++ b/spec/services/github/enrichment/fairness_spec.rb @@ -174,13 +174,64 @@ def stale_actor(github_id: 1) expect(choose(entity_class: :actor)).to have_attributes(chosen?: false, reason: "no_candidate") end - # With no pending candidate anywhere, "the other class has no currently eligible - # candidate" is true by definition, so a refresh past the guarantee is a legal borrow. + # The other class has no refresh of its own to do, so nothing is starved by lending + # its idle capacity — the same condition #pending_choice borrows under. it "borrows for a refresh once the class has spent its guarantee" do active_budget_window(now: now, actor_share_used: 20, enrichment_used: 20) stale_actor - expect(choose).to have_attributes(pool: :refresh, borrow: true) + expect(choose).to have_attributes(pool: :refresh, borrow: true, reason: "borrowed_refresh") + end + + def stale_repository(github_id: 2) + create_repository(github_id: github_id, enrichment_status: "complete", fetched_at: now - 90_000, + last_seen_at: now - 60) + end + + # §10:898 scopes refreshes "within each class's share", and §10's whole reason for + # having shares is that "a naive repo-first policy would starve actor enrichment to + # zero indefinitely". Selecting the first refreshable class outright reproduced exactly + # that inside the refresh pool, with the order reversed: actor spent its own twenty and + # then borrowed the remaining twenty, while repository's untouched guarantee and + # eligible stale rows were never chosen. + describe "when both classes have stale rows" do + before do + active_budget_window(now: now, actor_share_used: 20, enrichment_used: 20) + stale_actor + stale_repository + end + + it "chooses the class that still has room, not the first in class order" do + expect(choose).to have_attributes(entity_type: Github::Enrichment::EntityType.fetch(:repository), + pool: :refresh, borrow: false, reason: "refresh") + end + + # The borrow condition is about the class this cycle did not pick, so a class with + # refresh work of its own is not idle capacity to lend. + it "refuses to borrow while the other class has a refresh of its own" do + expect(choose(entity_class: :actor)).to have_attributes(chosen?: false, reason: "no_candidate") + end + + # Once the other class runs dry the capacity genuinely is idle, and §10's borrowing + # rule applies to the refresh pool exactly as it does to the pending one. + it "borrows again once the other class has nothing left to refresh" do + GithubRepository.update_all(fetched_at: now) + + expect(choose).to have_attributes(entity_type: Github::Enrichment::EntityType.fetch(:actor), + pool: :refresh, borrow: true, reason: "borrowed_refresh") + end + end + + # The pending pool's borrow test stays pending-only. CandidateSelector documents why: + # counting refreshes there would let a refresh outrank a never-enriched candidate and + # invert §10's ladder. Only the refresh pool's own test changed. + it "still borrows for a pending candidate while the other class has a stale refresh" do + active_budget_window(now: now, actor_share_used: 20, enrichment_used: 20) + pending_actor + stale_repository + + expect(choose).to have_attributes(entity_type: Github::Enrichment::EntityType.fetch(:actor), + pool: :pending, borrow: true, reason: "borrowed_pending") end end end diff --git a/spec/services/github/rate_limit_policy_spec.rb b/spec/services/github/rate_limit_policy_spec.rb index b56b217..e13f6ba 100644 --- a/spec/services/github/rate_limit_policy_spec.rb +++ b/spec/services/github/rate_limit_policy_spec.rb @@ -110,13 +110,15 @@ def rate_limited(remaining: "0", reset_at: now + 1800, **headers) expect(current_budget.global_blocked_until).to eq(now + described_class::MAX_BLOCK_SECONDS) end - # RateLimitSnapshot parses only the delta-seconds form and deliberately leaves the - # HTTP-date form nil. Absent, zero, negative and unparseable all mean the same thing: - # no usable instruction. `now + nil` raises and `now + nil.to_i` is no block at all, - # which is the response most likely to escalate GitHub's throttling. - it "backs off on its own terms when Retry-After is absent, zero, or an HTTP-date" do - [ nil, "0", "-5", "Wed, 21 Oct 2026 07:28:00 GMT" ].each do |value| - current_budget.update!(global_blocked_until: nil) + # Absent, zero, negative and unparseable all mean the same thing: no usable + # instruction. `now + nil` raises and `now + nil.to_i` is no block at all, which is the + # response most likely to escalate GitHub's throttling. + # + # The streak is reset between iterations so each case is measured against the same + # first-failure delay; the escalation across a genuine run is asserted separately below. + it "backs off on its own terms when Retry-After is absent, zero, negative, or unparseable" do + [ nil, "0", "-5", "immediately" ].each do |value| + current_budget.update!(global_blocked_until: nil, consecutive_secondary_limits: 0) headers = value.nil? ? {} : { "retry-after" => value } expect { policy.apply!(rate_limited(remaining: "42", **headers), now: now) }.not_to raise_error @@ -124,6 +126,24 @@ def rate_limited(remaining: "0", reset_at: now + 1800, **headers) end end + # RFC 9110 permits both Retry-After forms and §10 lists the header among the ones to + # process without qualifying which. Leaving the date form unread would collapse a + # server-supplied "wait 45 minutes" into the 60-second fallback — obeying an + # instruction far shorter than the one given. + it "honours an HTTP-date Retry-After, resolved against the observation instant" do + policy.apply!(rate_limited(remaining: "42", "retry-after" => (now + 900).httpdate), now: now) + + expect(current_budget.global_blocked_until).to eq(now + 900) + end + + # A date already in the past carries no usable instruction, so it falls through to the + # same backoff as an absent header rather than producing a block in the past. + it "falls back when the HTTP-date has already passed" do + policy.apply!(rate_limited(remaining: "42", "retry-after" => (now - 300).httpdate), now: now) + + expect(current_budget.global_blocked_until).to eq(now + described_class::MIN_BLOCK_SECONDS) + end + # A secondary limit expires on a Retry-After unrelated to the window boundary, and # nothing would put the label back — unlike the two reset-backed blocks, which # ROLL_WINDOW_SQL restores when the window rolls. @@ -132,6 +152,108 @@ def rate_limited(remaining: "0", reset_at: now + 1800, **headers) expect(current_budget).to be_active end + + # §10: "set global_blocked_until from Retry-After (or >= 1 minute with exponential + # backoff when the header is absent)". Without the streak the second half of that + # sentence does not exist: a hardcoded first attempt makes every block in a run the + # same ~60 seconds, so an IP GitHub is actively throttling is re-probed at a constant + # rate for as long as the throttling lasts. + describe "a run of secondary limits with no Retry-After" do + def block_delays(count) + (1..count).map do + current_budget.update!(global_blocked_until: nil) + policy.apply!(rate_limited(remaining: "42"), now: now) + (current_budget.global_blocked_until - now).to_i + end + end + + it "escalates exponentially from the minimum block" do + expect(block_delays(4)).to eq([ 60, 120, 240, 480 ]) + end + + it "counts each limit once, on the row every process reads" do + block_delays(3) + + expect(current_budget.consecutive_secondary_limits).to eq(3) + end + + # Waiting longer than a window buys nothing — the quota has refreshed by then — and + # an unbounded ladder would defer a recovered IP into next week. + it "caps at one rate-limit window" do + current_budget.update!(consecutive_secondary_limits: 20) + + expect(block_delays(1)).to eq([ described_class::MAX_BLOCK_SECONDS ]) + end + + # The escalation is in *consecutive* limits, so one live request that GitHub answers + # without throttling ends the run and the next block starts from the floor again. + it "starts over after a request completes without a secondary limit" do + block_delays(3) + + policy.apply!(fetched(status: 200, headers: { "x-ratelimit-resource" => "core" }), now: now) + + expect(current_budget.consecutive_secondary_limits).to eq(0) + expect(block_delays(1)).to eq([ 60 ]) + end + + # A 304 is a completed conditional request that GitHub answered and charged for, + # which is the same evidence a 200 carries: this IP is not being throttled. + it "treats a 304 as evidence the throttling ended" do + block_delays(2) + + policy.apply!(fetched(status: 304, headers: { "x-ratelimit-resource" => "core" }), now: now) + + expect(current_budget.consecutive_secondary_limits).to eq(0) + end + + # A timeout or a 5xx is GitHub failing to answer, not GitHub declining to throttle. + # Neither escalating nor clearing on it is what keeps the count a record of verdicts. + it "leaves the streak alone when the request never got a verdict" do + block_delays(2) + + policy.apply!(fetched(status: nil, error: Github::Errors::RequestTimeout.new("boom"), + classification: :timeout), now: now) + + expect(current_budget.consecutive_secondary_limits).to eq(2) + end + + # The two conditions are unrelated: a primary exhaustion is relieved by the window + # rolling, and deferring it on an unrelated secondary run would strand the poller + # past its own reset. + it "does not escalate a primary-limit fallback on the secondary streak" do + current_budget.update!(consecutive_secondary_limits: 5, global_blocked_until: nil) + + # remaining "0" with no parseable reset is the primary branch's fallback path. + policy.apply!(fetched(status: 403, headers: { + "x-ratelimit-remaining" => "0", "x-ratelimit-limit" => "60", + "x-ratelimit-resource" => "core" + }), now: now) + + expect(current_budget.global_blocked_until).to eq(now + described_class::MIN_BLOCK_SECONDS) + end + end + + # Every other example here injects rand: 0.0, which makes the assertions exact and + # would equally pass if jitter had been dropped from the production path. These two + # pin that a real Random reaches global_blocked_until, without making the suite + # depend on a draw. + describe "jitter on the fallback block" do + let(:band) { (now + described_class::MIN_BLOCK_SECONDS)..(now + (described_class::MIN_BLOCK_SECONDS * 1.25)) } + + it "reaches the persisted block through a seeded Random" do + seeded = described_class.new(backoff: Github::PollBackoff.new(random: Random.new(20_260_731))) + + seeded.apply!(rate_limited(remaining: "42"), now: now) + + expect(current_budget.global_blocked_until).to be_between(band.first, band.last).exclusive + end + + it "reaches it through the production default too" do + described_class.new.apply!(rate_limited(remaining: "42"), now: now) + + expect(current_budget.global_blocked_until).to be_between(band.first, band.last) + end + end end describe "a budget denial" do diff --git a/spec/services/github/rate_limit_snapshot_spec.rb b/spec/services/github/rate_limit_snapshot_spec.rb index 54d43ff..dbcf9ca 100644 --- a/spec/services/github/rate_limit_snapshot_spec.rb +++ b/spec/services/github/rate_limit_snapshot_spec.rb @@ -70,6 +70,37 @@ def snapshot(**overrides) end end + # RFC 9110 permits delta-seconds and an HTTP-date, and §10 lists Retry-After among the + # headers to process without qualifying which form. Both are normalized to a delta here + # so Github::RateLimitPolicy has one thing to reason about. + describe "Retry-After" do + def retry_after(value) = snapshot("retry-after" => value).retry_after_seconds + + it "reads the delta-seconds form GitHub actually sends" do + expect(retry_after("120")).to eq(120) + end + + # Resolved against the snapshot's own observed_at rather than the wall clock, so the + # same headers always produce the same delta. + it "converts the HTTP-date form to a delta against the observation instant" do + expect(retry_after((frozen_time + 2700).httpdate)).to eq(2700) + end + + # Not normalized to nil: this class reads and never decides, and the policy already + # treats a non-positive delta exactly as it treats an absent header. + it "reports a date that has already passed as a non-positive delta" do + expect(retry_after((frozen_time - 300).httpdate)).to eq(-300) + end + + it "reports a value that is neither form as absent" do + expect(retry_after("immediately")).to be_nil + end + + it "reports an absent header as absent" do + expect(retry_after(nil)).to be_nil + end + end + describe "#quantitative?" do # What the ledger needs before it can initialize or reconcile a window: without a # reset boundary there is no window to reconcile within.