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.