PR 4 — GitHub request infrastructure - #25
Merged
Merged
Conversation
PR 4 opens the first socket in this project, so the dependency and the guarantee that no spec ever uses it land together. Faraday is the HTTP client IMPLEMENTATION_PLAN.md §2A pins. It is added to the application group because Github::Transports::Faraday is production code; its Gemfile comment records the decision that the connection carries the adapter and nothing else, since retries and redirects belong to the request executor where each attempt re-reserves budget and each redirect target is re-validated (§10). WebMock is the §2A test pin. spec/support/webmock.rb is auto-required by the existing spec/support/**/*.rb glob and closes localhost as well as the wider network: request specs reach the app through Rack, so there is no local HTTP dependency to exempt. webmock/rspec itself only enables WebMock and resets stubs between examples — it sets no connection policy, so the policy is stated here. spec/network_boundary_spec.rb asserts the boundary instead of trusting it, so loosening the support file fails the suite rather than quietly spending real rate-limit quota from a CI runner. It also asserts PostgreSQL still answers, because libpq's socket is not one WebMock hooks. Gemfile.lock was regenerated inside ruby:3.4.10-slim with bundler 2.6.9 to match the pinned toolchain: 22 added lines, PLATFORMS and BUNDLED WITH untouched. addressable, crack, hashdiff, public_suffix, net-http and rexml enter the lock as WebMock's transitive dependencies; none is used by application code. docker compose run --rm test — 133 examples, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every knob PR 4 introduces, the plan's one authoritative allowance formula, and the
startup validation that refuses an over-committed budget before the process can
poll into it.
Github::Configuration is constructed from an explicit environment hash rather than
reading ENV internally, so a spec can describe an invalid configuration without
mutating the global environment — which under `config.order = :random` would leak
into whichever example ran next. It parses with Integer() rather than #to_i, since
#to_i turns "abc" into 0 and a zero interval or page count is the most permissive
misconfiguration available. A blank value is treated as unset, because Compose
passes an empty string for an unset ${VAR:-} interpolation.
Two values are deliberately not environment variables. The allowed API host stays
in Github::UrlPolicy: an env var there would make the SSRF boundary a deployment
setting. The REST API version stays pinned to 2022-11-28, the version this
project's live-probe evidence was gathered under. GitHub's documented
unauthenticated limit of 60 is a constant for the same reason — it is an external
fact, it is only the starting point for the first window, and the observed
x-ratelimit-limit supersedes it. The rejection path is exercised by lowering
POLL_INTERVAL_SECONDS, not by moving that number.
Github::Allowances derives both allowances rather than storing them, because
keeping 12 and 40 alongside the inputs that produce them guarantees they disagree
eventually. #feasible? states plan §10's rule as "at least one enrichment
attempt", which is the same predicate as poll_allowance + reserve >= limit and
says why it exists. #clamped handles the other direction: an observed limit lower
than the configured default is GitHub's business, not an operator error, so
runtime degrades with polling winning the clamp instead of crash-looping the
worker. Only boot raises.
Validation runs in a to_prepare block, not at initializer scope, because
Github::Configuration is autoloaded and a bare top-level reference would pin a
stale class object across development reloads. It touches no database, network, or
schema, so `bin/rails db:prepare`, `rails runner`, CI's schema load, and a
container starting before `setup` completes are all unaffected; the default set is
feasible (12 + 8 = 20 < 60), so a clean checkout never raises.
Github::Errors is one file under one namespace because Zeitwerk requires it, and
it draws the line the transports depend on: an HTTP status — any status, including
304, 403, and 500 — is a response, never an exception.
docker compose run --rm test — 165 examples, 0 failures. rubocop clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The immutable objects the chain passes around, and the two pure policies that decide what a response means and whether it is worth another request attempt. Github::Request carries the request class as a required, validated constructor argument. That field is what the ledger debits (§7) and what PR 5 and PR 7 use to tell a source failure from an entity failure (§10), so nothing is allowed to infer it. Protocol headers merge last in #headers, which makes them un-overridable by a caller passing its own User-Agent — the live transport applies them again as connection defaults, so even a hand-built request carries them. #redirected_to keeps the class, because a redirect hop is a real outbound request that debits the same counter, and drops the ETag, which was scoped to the URL that redirected. Github::RateLimitSnapshot parses all eight headers §10 processes and decides nothing. It is tolerant on purpose: an unreadable rate-limit header must never be the reason a successful response is discarded, so every unparseable value becomes nil and reconciliation simply has nothing to apply. Retry-After is read as seconds only; §10 already defines the fallback for a missing value, so the HTTP-date form is a handled outcome rather than a gap. Github::ResponseClassifier names what happened and never acts on it. The line it does not cross is PR 6's: setting global_blocked_until, deriving class blocking, computing effective_poll_time, and persisting ETags all branch on this vocabulary without being implemented here. It separates primary exhaustion from a secondary limit on §10's own discriminator — the remaining count, not the status — because the two have different remedies and a secondary limit is IP-scoped, so deferring only the source would leave enrichment hammering the same address. 404 and 410 get their own classification so that one deleted repository can never disable /events. Github::RetryPolicy makes :server_error and a network-level failure the only retryable outcomes, exactly as §10 lists them. A TLS failure is not retried: it will not clear inside two attempts, it may be an interception attempt, and every retry re-reserves one of sixty requests an hour. Its Random is injected so the backoff schedule is asserted without a sleep anywhere in the suite. DISPOSITIONS is the seam PR 7 consumes, and a spec iterates every constant under Github::Errors so a future error class cannot fall through to an undefined entity outcome. Github::FetchResult is the single return type. Budget denial, URL rejection, gate deferral, exhausted retries, and transport failure are all ordinary outcomes of talking to a rate-limited third party, and a caller forced to rescue four exception classes to poll a page will eventually miss one. All four value objects subclass Data.define rather than taking its block: a constant assigned inside that block is scoped to the enclosing module, so CLASSES would silently have become Github::CLASSES. Caught by the specs before the commit. docker compose run --rm test — 234 examples, 0 failures. rubocop clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both PostgreSQL session advisory locks, namespaced (SOURCE_LOCK, event_source_id) and (REQUEST_GATE, 1) per plan §2A, plus the first runtime enforcement of the lock-order invariant. Session-level rather than pg_advisory_xact_lock, because a transaction-scoped lock ends at transaction end and cannot own an HTTP operation, and no transaction may be held open across network I/O. The trade is that nothing releases a session lock implicitly, so every acquisition is a lexical block with an ensure *inside* the with_connection block, and hard process death releases it by closing the session. with_connection, never checkout/checkin. If the execution context already holds a lease — always true inside a job, a controller action, or an RSpec example — it yields that same connection, so the lock statements and every query inside the block share one session and pool use stays at one connection per context. A bare checkout returns a connection outside the lease cache, which would hold the lock on a session doing none of the work. The two acquisition strategies are deliberately different. SourceLock uses pg_try_advisory_lock: once for the poller (§2A), in a bounded retry loop for the one-shot (§9 pins the mechanism), with CLOCK_MONOTONIC so an NTP step cannot move the deadline and with the clock and sleeper injected so the 30-second contract is asserted in zero wall-clock time. RequestGate blocks under SET LOCAL lock_timeout instead: `try` does not queue, so under real contention it degrades into a lottery that can starve a waiter, while blocking gets PostgreSQL's own FIFO queue in one round trip. An unbounded blocking acquire was rejected for a different reason — it has no failing outcome to assert, so a broken gate spec would hang CI rather than fail it. SET LOCAL rather than SET means the GUC reverts on commit and can never strand a lock_timeout on a pooled connection; the session lock deliberately outlives that commit, which is the whole reason it is a session lock. Release verifies the session. pg_advisory_unlock releases in whatever session runs it, and Active Record can silently reconnect a stale connection — the lock is held outside a transaction, so it considers the state restorable, nothing raises, and the lock evaporates. The backend PID is captured at acquisition and the unlock is conditional on it inside the statement, so a session that does not own the lock never issues one. The lock statements use exec_query, not select_value. The query cache wraps select_all, and a probe inside the container confirmed identical select_all and select_value calls are answered from cache while exec_query is not. Either cached answer is wrong here: a cached "held by someone else" locks this process out of a lock that is now free, and a cached "acquired" hands two holders the same gate. The spec that covers this was rewritten after the first version failed to detect the defect — its two attempts now have genuinely different correct answers with nothing between them that could clear the cache. Verified by switching try_lock to select_all and watching it fail, then restoring it and watching it pass. Github::LockOrder tracks held keys per execution context in ActiveSupport::IsolatedExecutionState. Lock ordering is an intra-thread property by definition, and this is exact because the design gives each context exactly one connection. It gives two guarantees: taking a source lock while the gate is held raises rather than deadlocking two containers with nothing in the logs, and a nested acquisition raises rather than relying on PostgreSQL's re-entrancy, which would silently permit two in-flight requests from one context. spec/support/advisory_lock_helpers.rb opens an out-of-pool PG::Connection rather than a thread. While use_transactional_fixtures has the pool pinned, ConnectionPool#checkout returns the pinned connection to every thread, so a second thread shares the first thread's session and session advisory locks are re-entrant within a session — a thread-based contention example would pass even if RequestGate.hold were an empty method. Its after hook fails any example that leaks a lock, because checkin expires the lease without resetting the connection and a leaked lock would ride into the next example and silently invert a contention assertion. docker compose run --rm test — 271 examples, 0 failures. rubocop clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The transactional reservation, the per-window bootstrap, and monotonic header reconciliation — the accounting every later PR spends against. The division of labour with PR 6 is: the ledger enforces, the policy decides. reserve! refuses a reservation there is no capacity for, and it reads global_blocked_until — but it never sets that column, never derives poll_class_blocked_until for scheduling, and never handles a secondary limit. Reading a column this PR does not write costs one clause and means PR 6 never has to reopen the ledger's critical section. Enforcing at all is not scope creep in the other direction either: §9's pagination stop conditions already name "the budget ledger denies the next reservation", and a reservation that cannot refuse is not a reservation. Fairness between actors and repositories stays PR 7's; PR 4 records actor_share_used and repository_share_used accurately so PR 7 adds predicates over data that is already correct. Pessimistic SELECT ... FOR UPDATE rather than an optimistic lock_version retry. Contention on this one row is the design — every outbound request in the system passes through it — so optimistic locking would degrade into a retry storm. The critical section is also not a compare-and-swap: it rolls the window, re-derives three allowances, evaluates four ordered denial conditions, and only then debits, and a zero-row conditional UPDATE could not report which condition refused it. The cost is nil because the reservation runs inside the request gate. lock_version is still bumped by hand in the raw SQL, so a stale in-memory record cannot clobber the counters through an ordinary save — PR 3's optimistic-locking spec now means something, and a new spec covers the raw-debit path. Two PostgreSQL NULL traps are defeated explicitly, and both mutations were run to prove the specs catch them. GREATEST ignores NULL, so GREATEST(NULL - 1, 0) evaluates to 0. Decrementing an un-bootstrapped remaining that way would create a permanent reserve breach denying every request — including the bootstrap poll that is the only thing able to refresh it. Replacing the CASE guard with GREATEST fails 5 examples. Rollover nulls remaining rather than carrying it. A dead window's remaining of 3 against a reserve of 8 has exactly the same deadlock. Dropping `remaining = NULL` from the rollover fails 2 examples. "limit" is deliberately retained as the last authoritative observation, and the CASE on global_blocked_until clears a primary-exhaustion block set to a reset that has now passed while preserving a secondary-limit block that outlives it. The window bootstrap is the first real poll, not an extra request. An uninitialized window grants a poll and counts it; enrichment is refused until a response has initialized the window from authoritative headers, because a co-tenant behind the same IP may have spent the budget the moment it reset and 60 remaining is never assumed. Initialization deliberately does not touch the counters, which is what keeps that first poll spent. A denial raises after the transaction commits, not inside it, so a window reset that genuinely happened is not undone by the reservation that discovered it. Reconciliation verifies x-ratelimit-resource against the stored resource before any arithmetic. A mismatch logs, changes nothing, and returns — folding a search bucket's numbers in through LEAST would clamp us to its remaining and import its 60-second reset as our window boundary, producing denials indistinguishable from real exhaustion, while raising would crash-loop the poller against §10. Failures stay spent structurally: there is no credit!, refund!, or release!, and a spec asserts the public surface stays that small. The debit's durability is guarded by testing current_transaction.joinable? rather than open? — RSpec opens its fixture transaction with joinable: false, so the guard fires on genuine application transactions in every environment with no Rails.env check. Since nothing commits under transactional fixtures, budget_ledger_durability_spec.rb turns them off to observe the committed debit and the FOR UPDATE serialisation from a second session, cleaning up in an around/ensure because a leftover row 1 would break every other spec under random ordering. The singleton row is created by reserve! with ON CONFLICT DO NOTHING, in its own statement before the reservation transaction opens — a RecordNotUnique inside would poison it. Seeds and migrations were both rejected: db:test:prepare never seeds and schema.rb carries no rows, so either would leave the row present in development and missing in test. docker compose run --rm test — 319 examples, 0 failures. rubocop and brakeman clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Enrichment follows URLs that arrive inside event payloads and pagination follows URLs from Link headers, so plan §10 draws a strict trust boundary. Every rule it lists is implemented and covered: HTTPS only, host exactly api.github.com case-insensitively, no userinfo, no non-default port, no IP-literal host. Every edge case in the spec was verified against Ruby's RFC 3986 parser inside the image before being asserted, rather than assumed. That probe changed two things. First, https://api.github.com@evil.test/ is already refused by the *host* rule, not the userinfo rule: RFC 3986 puts everything before the @ in userinfo, so the host really is evil.test. The userinfo rule exists for user:pw@api.github.com. Second, IPAddr recognises neither 2130706433 nor 0x7f000001, so those are refused as disallowed hosts — it is the exact-host allowlist, not the IP detector, that makes them safe. Both are stated in the comments and asserted in examples, because implying the IP check is load-bearing would be a false claim about the boundary. The parser is pinned to URI::RFC3986_PARSER rather than URI.parse, which consults URI::DEFAULT_PARSER and can be reassigned by any gem in the bundle through URI.parser= — the boundary must not be movable from outside this file. Addressable was rejected for a second reason beyond not being in §2A's stack: it normalises IDN and percent-encoding, which is exactly the helpful transformation you do not want at a trust boundary, because the validator and the HTTP client then disagree about what the string meant. Nothing the caller supplied is forwarded. validate! returns a frozen ValidatedUrl rebuilt from the components that passed — userinfo gone, explicit default port gone, fragment gone, host case-folded — so one canonical request form exists regardless of how a payload spelled it, and the target cannot be repointed between the check and the request. Its constructor is private to the policy, so a transport cannot be handed a hand-rolled one. validate_payload_url! implements the ratified fixture decision. A URL from a payload or a Link header is always validated against the *full live* policy first, whatever the mode, and only then projected onto the fixture scheme. So payload bodies in the corpus stay realistic https GitHub JSON, a fixture-mode database is indistinguishable from a live one, and fixture mode is never a weaker boundary — a response body cannot forge a corpus address, and a hostile https URL cannot become a fixture URL. Violations accumulate rather than stopping at the first, so a log names the whole reason; only a parse failure short-circuits, because nothing else about the URL is knowable. The vocabulary is closed and asserted, since PR 7 maps these reasons onto permanent_failure and a silently added one would fall through its branch. docker compose run --rm test — 360 examples, 0 failures. rubocop clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The static JSON corpus plan §12 pins, plus the loader that turns a request into a scripted response. VCR is deliberately not used: scripted conditional responses, changing rate-limit headers, and failure sequences have to be authored, not recorded. The corpus lives at fixtures/github/ rather than spec/fixtures/. Under GITHUB_MODE=fixture this is runtime data read by the web and worker containers, so production code would otherwise be reading from a test directory — and excluding spec/ from the image is a normal future hardening step that must not be able to break the reviewer's demo. spec/fixtures also already means Active Record YAML fixtures in a suite where use_transactional_fixtures is live. Entries are keyed by a canonical request key — path, then query parameters sorted by name — with scheme and host omitted because UrlPolicy has already proved them. One entry therefore answers a request from either transport, and a Link header that spells its parameters in a different order cannot miss an entry the corpus defines. Body paths are authored in the manifest and never derived from a URL. That is a security property, not a convenience: fixture://api.github.com/../../etc/passwd parses with its path preserved, so a corpus that derived filenames from URLs would read it. Here it is simply a miss, and the loader additionally proves every resolved path sits inside bodies/ — covered by a spec that builds a hostile manifest in a tmpdir. The manifest is JSON rather than YAML so a corpus file cannot instantiate Ruby objects. A key's value is an ordered list whose last entry repeats forever, so a long-running fixture container stays defined instead of erroring after N polls, and this maps exactly onto WebMock's multi-value to_return — which is what lets one corpus drive both the live transport under WebMock and the offline transport. Conditional If-None-Match matching was rejected: positional scripting is fully deterministic, and "the request carried the stored ETag" is better asserted against the fixture transport's recorded requests. Every URL inside a body and every Link target is a real https://api.github.com URL. The fixture scheme is produced only by the offline event source and by UrlPolicy.validate_payload_url! after the full live policy has passed, so a database written in fixture mode is indistinguishable from one written live and a response body cannot forge a corpus address. Eight scenarios ship because §12 names them as corpus contents: default (200 then 304 forever), paginated, rate_limited, secondary_rate_limited, transient_failure (500, 500, 200 — two retries, each its own reservation), transient_failure_exhausted, redirecting_repository, and hostile_redirect. PR 6 and PR 11 consume most of them. A scenario lists only its overrides and inherits the rest one level deep, so its behaviour stays readable in one place. The integrity spec is what keeps the corpus honest over time: every actor URL, repository URL, and Link target in every page must resolve within the corpus, every body must parse, no body file may be orphaned, only api.github.com URLs may appear in bodies, and every PushEvent payload must carry exactly the five post-2025-10-07 fields with no commits array. Without those, a fixture-mode run would 404 silently and look like an enrichment bug rather than a corpus gap. fixtures/github/README.md describes the corpus contents factually and stops there — what each malformed entry becomes once processed is PR 5's to define and assert. docker compose run --rm test — 381 examples, 0 failures. rubocop clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two implementations of one seam (plan §6), so the contract is exercised rather than speculative. A shared example group asks both the same questions, which is what stops them drifting. The Faraday connection carries the adapter and nothing else, and a spec pins connection.builder.handlers empty. That is the highest-value regression test in this PR: faraday-retry would retry beneath the request gate and beneath the ledger, breaking both §10's "each attempt is a reservation" and §2A's one-request-per-hold rule while sleeping with a session advisory lock held; follow_redirects would bypass the SSRF boundary; RaiseError would turn 304, 403 and 404 into exceptions this application classifies; a JSON parser would discard the raw body §7 retains. Adding `response :raise_error` alone fails 3 examples — verified, then reverted. No base url: is set either, so there is no relative-resolution path away from the allowed host. Both transports accept only a Github::UrlPolicy::ValidatedUrl, and only one built for their own mode. Since the sole way to obtain one is through the policy, the SSRF boundary is unbypassable rather than merely conventional, and a corpus response can never be served as live or the reverse. Every reference to the gem inside Github::Transports::Faraday carries a leading ::, because Ruby walks Module.nesting before Object and the bare constant resolves to this class. Plan §5 pins the class name, so the guard is the leading :: plus a spec asserting the connection really is a ::Faraday::Connection. The fixture transport keeps its cursor on the instance, never at class level, so one spec cannot advance another's script under random ordering. It records every request with the headers actually sent — which is how the conditional-request path is asserted without the corpus having to match on If-None-Match — and it applies the same pinned protocol headers as the live transport, so a header assertion cannot pass offline and fail against the real API. Fixture mode still takes the gate, still reserves budget, still passes the URL policy, and still reconciles the corpus's rate-limit headers. §12's "zero live quota consumed" means zero GitHub quota, not zero accounting — the corpus's x-ratelimit-* headers are exactly what make the per-window bootstrap runnable offline. That is the difference between a fixture mode that proves the architecture and one that bypasses it. One spec was corrected to match observed behaviour rather than the assumption behind it: WebMock's to_timeout surfaces as Net::OpenTimeout, which Faraday's net_http adapter reports as a connection failure rather than a read timeout. Both are §10's "network timeout" and both are retryable, so the suite now asserts each mapping separately plus the fact that both dispositions are :retry. docker compose run --rm test — 424 examples, 0 failures. rubocop clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The chain CLAUDE.md and plan §5 mandate — request gate, budget ledger reservation, URL policy, transport — assembled so nothing outside it can reach GitHub. One gate hold is one HTTP attempt is one reservation. §2A puts "exactly one GitHub request" inside the gate and §10 requires every retry to be its own reservation "through the same gate and ledger" — the same mechanism, not the same hold. Holding a global serial advisory lock across a backoff sleep would stall every other process in the application for the duration of that sleep, so retries re-acquire and the sleep happens with nothing held. Specs assert both directly: three attempts observe poll_used 1, 2, 3, and the injected sleeper records the gate as unheld each time. Redirects are hops through the same chain rather than middleware. Each one re-acquires the gate, reserves again against the originating request's class — §7: every actual outbound attempt debits its class counter, and a redirect hop is a real outbound request — and is re-validated by UrlPolicy before it is followed. A 301 off api.github.com is refused with the transport called exactly once. Retries sit outside redirects, so MAX_HTTP_RETRIES keeps its plain meaning. The URL is validated twice, deliberately. The in-chain call is the authoritative one and its return value is what the transport receives, so an unvalidated URL cannot physically reach a socket. The pre-gate call exists so a refused URL debits nothing: §7's "failures stay spent" is about outbound attempts, and a URL the policy rejects is never sent. Removing that line fails the example asserting a hostile enrichment URL costs no budget — verified, then restored. The executor is never handed an event_source_id, which is the structural half of §5's source-lock separation; a spec closes the loop by attempting a source lock from inside the transport and asserting LockOrder refuses it. #call returns a FetchResult for every runtime outcome — budget denial, busy gate, URL rejection, exhausted retries, transport failure — because a caller forced to rescue four exception classes to poll a page will eventually miss one. The single exception is a fixture corpus gap, which §6 requires to be raised: an authoring bug is not a runtime outcome, and laundering it into a failed fetch would let a fixture-mode demo report a plausible-looking failure instead of naming the missing entry. Github.transport and Github.executor are memoised per process, because the fixture transport's scripted sequences advance across requests and a fresh instance per call would restart every script. spec/support/budget_helpers.rb now states what an active ledger window looks like once, shared by the ledger's specs and the executor's. docker compose run --rm test — 447 examples, 0 failures. rubocop clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The seam plan §6 defines, with the two implementations that make it exercised rather than speculative — the condition Appendix A item 7 attached when it upheld this design against a speculation critique. The contract deliberately knows nothing about PushEvent filtering or the processor registry (PR 5), and nothing about Link-header pagination (PR 6). Pagination is a *parameter* rather than a method: #request_for builds a request for any URL, so PR 6 calls it with a Link target and PR 4 calls it only via #first_page_request. That way PR 6 needs no contract change and PR 4 ships no abstract method without a caller. #first_page_request accepts an etag: because the corpus carries a scripted 304 scenario that cannot be authored without conditional-header construction, but nothing here reads or writes event_sources.etag. per_page=100 belongs to the canonical page-one URL rather than to a caller, because §9 scopes the persisted ETag to exactly that URL with its stable query parameters — a URL that varied between polls could never match. Github::Request gained an origin, which is the change that makes the offline source work without weakening the boundary. A location this application constructed — an event source's own endpoint — is validated against the current mode directly, which is what lets FixtureEvents address the corpus with a fixture:// URL. A location GitHub supplied — a payload URL, a Link target, a Location header — always clears the full live policy first and is only then projected onto the fixture scheme. A redirect hop becomes payload-origin however the original request was built, because a Location header is server-supplied. So a response body still cannot forge a corpus address, and fixture mode is still never a weaker boundary than live. Both sources produce the same canonical corpus key, since the key omits scheme and host precisely because UrlPolicy has already proved them. A spec asserts the two keys are equal and that the corpus answers it — that equality is what "one corpus serves both transports" actually means. #events returns raw envelopes untouched, including nulls, which a valid JSON array can legitimately contain and which PR 5 quarantines. A body that is not a JSON array raises MalformedResponse, so PR 5's taxonomy can tell a malformed response from a malformed event. Base.for validates a row's source_type against the implemented adapters and fails fast naming the ones that exist (§6), and a spec pins the registry to exactly two entries — RepositoryEvents stays the documented, unbuilt seam §5 describes. app/services/github.rb completes the composition root: configuration, transport, and executor memoised per process, all dropped by reset! on reload. docker compose run --rm test — 488 examples, 0 failures. rubocop clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
.env.example and the README environment table now carry the eleven variables this PR introduces, replacing the "arrive with PRs 4-7" placeholder. Both state what is deliberately absent and why: there is no GITHUB_API_BASE_URL, because an environment variable for the allowed host would make the SSRF boundary a deployment setting, and no GITHUB_API_VERSION, because §2A pins 2022-11-28 to the version every live probe behind this plan was run under. GITHUB_MODE goes in the compose x-app-env anchor so the future worker and ingest services inherit it — the fixture decision has to be process-wide, since a poller and an enrichment worker disagreeing about the mode would be worse than either setting. It defaults to live, which §12 states is the default runtime behavior. The test service pins it explicitly rather than inheriting, so `GITHUB_MODE=fixture docker compose run --rm test` cannot quietly run a different suite than CI. The README Status section says plainly that nothing polls GitHub yet: the infrastructure exists and is tested, but no event source is provisioned and no code path fetches a page. A new "The request path" section lands the request half of the Architecture summary row, and the remaining row narrows to ingestion and enrichment in PRs 5 and 7. The fixture-verification row is left at PR 11 — the corpus exists, but nothing consumes it end to end yet, and §16 forbids overstating. Three ADRs, all topics plan §14 already lists, following ADR 0001's shape of stating what a decision buys and what it costs: 0002 covers the session advisory locks. Why not FOR UPDATE (a row lock ends at transaction end and cannot own an HTTP operation), why literal namespaces rather than a hashed seed, why the source lock and the gate use different acquisition strategies, and why an unbounded blocking acquire was rejected — it has no failing outcome to assert, so a broken gate spec would hang CI rather than fail it. 0003 covers the two seams. Why the Faraday connection carries no middleware at all, why redirects and retries are executor iterations, and why payload URLs stay real https GitHub URLs while the fixture scheme is produced only after the full live policy has passed. 0004 covers the ledger. The one derived formula, the enforces-versus-decides split with PR 6, and the two PostgreSQL NULL behaviours that would otherwise deadlock the ledger permanently. docker compose run --rm test — 488 examples, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two defects found by running the chain end to end in the compose stack rather than only in the suite. The corpus pinned x-ratelimit-reset to a fixed epoch, which was already in the past. The ledger then did exactly the right thing and rolled the window on every single poll, so counters never accumulated: two polls in a row both reported poll_used=1. Fixture mode was silently failing to demonstrate the accounting it exists to demonstrate. A header value of "+N" now resolves to N seconds from now, applied only to that header; bodies stay byte-static and the transport's clock is injectable, so specs stay deterministic. Two polls now report poll_used=2 and remaining 59 then 58 — the 304 spending quota, per §10. pg_advisory_lock returns void, and Active Record printed "unknown OID 2278: failed to recognize type of 'pg_advisory_lock'" to stderr on every gate acquisition. That is noise outside the single JSON stream §11 specifies, so the lock now selects the backend PID from a subquery and the void column never reaches the adapter. Verified in the compose stack: the fixture chain returns 200 with 8 events decoded and bootstraps the window to active with the formula's 12/40/8 allowances; a corpus gap raises FixtureMiss rather than reaching the network; POLL_INTERVAL_SECONDS=60 refuses to boot, naming the offending numbers; and pg_locks shows both lock families under their own greppable classids (0x47504901 for the source lock, 0x47504902 for the gate). docker compose run --rm test — 491 examples, 0 failures. rubocop, brakeman, and bundler-audit clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
batbrainy
commented
Jul 30, 2026
| next :no_ledger if budget.nil? | ||
| next :resource_mismatch if resource_mismatch?(budget, snapshot) | ||
|
|
||
| budget = roll_window!(budget, now: now) if window_elapsed?(budget, now) |
Owner
Author
There was a problem hiding this comment.
When a request is reserved just before the stored reset boundary and reconciled after GitHub reports the next reset window, this path can re-read stale budget state after roll_window! under the Rails query cache and raise Github::Errors::LedgerInvariantViolation. I reproduced it in the Docker runner by reserving at 12:59:59 and reconciling at 13:00:01 with x-ratelimit-reset=14:00; it logs budget.window_rolled twice before raising. With the cache disabled, the raise disappears, but the initialized window has poll_used: 0 even though remaining: 59 shows the just-completed request consumed capacity in that new window. Can we make rollover return a fresh row from the DB and preserve the in-flight reservation when a response initializes a superseding window?
batbrainy
commented
Jul 30, 2026
| end | ||
|
|
||
| # @return [Github::Request] | ||
| def request_for(url, etag: nil) |
Owner
Author
There was a problem hiding this comment.
This helper is documented as the PR 6 pagination primitive for URLs extracted from Link headers, but it always builds an application-origin request. The fixture corpus publishes real https://api.github.com/... Link targets; in fixture mode source.request_for(link_url) is rejected with scheme_not_allowed instead of being projected through the payload URL path. That means offline Link-driven pagination will fail or have to bypass this adapter contract. Consider accepting an origin: argument, defaulting first-page calls to :application, and having Link callers pass :payload, with a spec that uses the actual paginated corpus Link header.
Three defects, both reviewer findings reproduced in the Docker runner before being touched. The Active Record query cache wraps select_all, so GithubApiBudget.find is cacheable — while the raw exec_update statements the ledger writes with do not invalidate it. A reservation's post-debit read populated the cache, and the rollover that followed was handed the pre-rollover row: reconcile! rolled the window twice, then fell through apply_observation's branches and raised LedgerInvariantViolation. Reserving at 12:59:59 and reconciling at 13:00:01 with x-ratelimit-reset=14:00 reproduced it exactly as reported. Both ledger transactions now run inside GithubApiBudget.uncached, so no ledger read can be served from a cache the ledger's own writes do not clear. The two rollover predicates were evaluated in sequence, which is what made a second rollover reachable at all. They describe one event — the window moved on — so they are now one decision, and double-rolling is structurally impossible rather than merely unreachable. The third defect is the one the reviewer found underneath the raise. A request reserved just before a reset boundary is counted by GitHub in the *new* window, which is precisely what a superseding reset_at means. Zeroing the counters outright discarded that debit, so the new window would have issued one more request than GitHub honours — visible as poll_used: 0 alongside remaining: 59. The rollover statement now takes its post-reset counters as parameters, and reconcile! carries the in-flight request's class forward. The clock-driven rollover inside reserve! has no in-flight request and still starts clean. reconcile! takes request_class: for this, and the executor passes it. EventSources::Base#request_for always built application-origin requests, so a Link target — which the corpus publishes as a real https://api.github.com URL, because GitHub does — was refused as scheme_not_allowed in fixture mode. Offline Link-driven pagination could not have worked through the adapter contract at all. request_for now takes an origin:, and #linked_page_request names the Link case so the origin is not something a caller has to remember. The spec drives it from the actual header the paginated corpus scenario serves rather than a hand-written URL. Every fix was reverted individually to confirm its spec fails without it: the uncached read (1 failure), the carry-forward (2), and the Link origin (2). Verified in the compose stack. The reported sequence now logs one window_rolled carrying "poll", then window_initialized, returns :initialized, and leaves poll_used=1 remaining=59 in the 14:00 window. Offline pagination walks page 1 (8 events) to the published Link target to page 2 (3 events) with poll_used=2. docker compose run --rm test — 500 examples, 0 failures. rubocop, brakeman, and bundler-audit clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Linked issue
Closes #14 (PR 4 — GitHub request infrastructure;
IMPLEMENTATION_PLAN.md§13 "PR 4"). Parent: #9 — Extension A (Rate Limiting and Fan-Out Control). Depends on: #13. Related tracks: #5 (Story 1 — Ingest GitHub Push Events), #10 (Extension B — Idempotency and Restart Safety), #11 (Extension D — Testing Strategy).Problem
Every PR after this one spends GitHub request budget. §13 is dependency-ordered precisely so the request gate and the budget ledger land before anything that can spend: PR 5 ingests, PR 6 paginates and schedules, PR 7 enriches. None of them can be written correctly until the chain they all flow through exists.
Before this PR none of it existed. No
Github::namespace, nofaraday, nowebmock, no fixture corpus.GithubApiBudgetwas a schema surface whose own header comment said the reservation logic "isGithub::BudgetLedgerin PR 4", and no process seeded its row.Scope
Included:
Github::SourceLockandGithub::RequestGate— namespaced session advisory locks, with the lock-order invariant enforced at runtimeGithub::BudgetLedger— class-aware transactional reservation, the allowance formula, startup validation, per-window bootstrap, monotonic header reconciliationGithub::UrlPolicy— the §10 SSRF boundaryGithub::RequestExecutor— the chain, with executor-owned retries and redirectsGithub::Transports::{Faraday,Fixture}andGithub::EventSources::{Base,PublicEvents,FixtureEvents}Request(protocol headers, request class, origin),RateLimitSnapshot,ResponseClassifier,FetchResult,RetryPolicyGithub::Configuration+Github::Allowances+ a boot-time initializer; eleven new environment variablesfixtures/github/faradayandwebmock, and a suite-wide network boundaryDeliberately not included (later PRs per §13):
Github::Client— no caller exists untilIngestionRunner; §16 forbids speculative infrastructurePushEventnormalization, quarantine, the one-shot command,event_sourcesrow provisioningGithub::RateLimitPolicy; settingglobal_blocked_until;Linkpagination; ETag persistence and304scheduling;Retry-Afterhandling;effective_poll_time; secondary-limit backoff; the dated live-probe transcriptskipped_budget, entitypermanent_failuremarking,effective_enrichment_time/statusledger presenter and coverage formulasGithub::EventSources::RepositoryEventsspec/db/schema_spec.rbis untouched: itsmatch_arrayalready names exactly the columns the ledger reserves against, and §10's class blocking is derived rather than stored.event_sourcescolumns are untouched too — PR 4 readssource_typeonly.Technical decisions
The chain is a loop the executor drives, not a middleware stack. The Faraday connection carries the adapter and nothing else, and a spec pins
connection.builder.handlersempty.faraday-retryretries inside one connection call — beneath the gate and beneath the ledger — so §10's "each attempt is a reservation" would silently become one debit per logical fetch, and the middleware would sleep its backoff while holding a session advisory lock.follow_redirectswould bypass the SSRF boundary.RaiseErrorwould turn 304, 403 and 404 into exceptions this application classifies. A JSON parser would discard the raw body §7 retains. Addingresponse :raise_erroralone fails 3 examples.url:on the connection (there is none, so no relative-resolution path can escape the allowed host).One gate hold = one HTTP attempt = one reservation. §2A puts "exactly one GitHub request" inside the gate; §10 requires retries "through the same gate and ledger" — the same mechanism, not the same hold. Retries and redirect hops each re-acquire and re-reserve, and backoff sleeps happen with nothing held.
PR 4 enforces; PR 6 decides.
reserve!refuses on a global block, an uninitialized window for an enrichment class,remaining <= reserve, and class-allowance exhaustion. It readsglobal_blocked_untilbut never writes it. §13 lists "poll-attempt allowance enforcement" under PR 6, so this is called out explicitly rather than left for a reviewer to notice: §9's pagination stop conditions already name "the budget ledger denies the next reservation", and a reservation that cannot refuse is not a reservation. Reading a column this PR never writes costs one clause and means PR 6 never reopens the ledger's critical section.Pessimistic
SELECT … FOR UPDATE, not optimistic retry. Contention on this one row is the design — every outbound request passes through it — so optimistic locking would degrade into a retry storm. The critical section rolls the window, re-derives three allowances, evaluates four ordered denial conditions, then debits; a zero-row conditionalUPDATEcould not report which condition refused.lock_versionis still bumped by hand, so a stale in-memory record still raisesStaleObjectError.lock_versionretry loops around a budget debit — a retry placed one line too high double-debits.Two PostgreSQL NULL behaviours are load-bearing.
GREATESTignores NULL, soGREATEST(NULL - 1, 0)is 0 — decrementing an un-bootstrappedremainingthat way creates a permanent reserve breach that denies every request, including the bootstrap poll that could have refreshed it. Rollover carrying a staleremainingforward has the same deadlock. Both mutations were run: the first fails 5 examples, the second fails 2.Payload URLs stay real; the fixture scheme is a projection.
Github::Requestcarries an origin. An application-constructed URL is validated against the current mode directly, which letsFixtureEventsaddress the corpus. A GitHub-supplied URL — payload,Linktarget,Locationheader — always clears the full live policy first and is only then projected ontofixture://. So a fixture-mode database is indistinguishable from a live one, a response body cannot forge a corpus address, and fixture mode is never a weaker boundary than live.Every SSRF edge case was verified against Ruby's RFC 3986 parser before being asserted. That changed two claims.
https://api.github.com@evil.test/is refused by the host rule, not the userinfo rule — RFC 3986 puts the pre-@part in userinfo, so the host really isevil.test. And IPAddr recognises neither2130706433nor0x7f000001, so it is the exact-host allowlist, not the IP detector, that makes them safe. Both are stated in the code and asserted, because implying the IP check is load-bearing would be a false claim about the boundary.URI.parseis also rejected: it consultsURI::DEFAULT_PARSER, which any gem can reassign.Advisory locks are session-level, on a leased connection, released in an
ensureinside the lease. AFOR UPDATErow claim ends at transaction end and cannot own an HTTP operation. The gate blocks underSET LOCAL lock_timeoutrather than pollingpg_try_advisory_lock:trydoes not queue, so under real contention it becomes a lottery. An unbounded blocking acquire was rejected because it has no failing outcome to assert — a broken gate spec would hang CI rather than fail it. Lock statements useexec_query, notselect_value: a probe inside the image confirmed the query cache answers identicalselect_all/select_valuecalls from cache whileexec_queryis not cached, and either cached answer breaks the gate silently.Three additions §5 does not name, each called out here:
Github::AdvisoryLock(the shared mechanism plus the registry that makes the lock-order invariant checkable),Github::LockOrder, andGithub::Configuration/Allowances(§10 says the formula is "computed at startup" and §7 says allowances are "derived at startup"; there was no home for either).No
GITHUB_API_BASE_URLand noGITHUB_API_VERSION. An environment variable for the allowed host would make the SSRF boundary a deployment setting; the API version is pinned to2022-11-28, the version every live probe behind this plan was run under.60is likewise a constant, not a knob — it is an external GitHub fact and only the starting point for the very first window.Testing performed
docker compose run --rm test— 500 examples, 0 failures (was 130 onmain).Mapped to §12: ledger accounting (class reservation, allowance formula and startup validation,
304debits, failure-stays-spent, monotonic reconciliation, per-window bootstrap and counter reset) · transient retries (D8) · enrichment URL policy, ~35 examples covering non-HTTPS, wrong host, userinfo, ports, IP literals, homographs, and traversal · event-source request construction and protocol headers · retry and error classification · the deterministic fixture corpus (D1).Guards proven non-vacuous by removing them and watching the suite fail, then restoring:
exec_queryoverselect_valueselect_allCASE WHEN remaining IS NULLGREATESTremainingresponse :raise_errorOne spec's query-cache coverage was rewritten after the first version failed to detect its own mutation — the intervening
release!cleared the cache. The replacement uses two attempts with genuinely different correct answers and nothing between them.Advisory-lock contention is tested against a real out-of-pool PostgreSQL session, never a thread: while transactional fixtures have the pool pinned,
ConnectionPool#checkoutreturns the pinned connection to every thread, so a thread-based contention example would pass even ifRequestGate.holdwere an empty method. Anafterhook fails any example that leaks a lock.bundle exec rubocop(103 files, no offenses),bin/brakeman(0 warnings, 0 errors),bundler-audit(no vulnerabilities).Docker verification
docker compose up --buildstill boots;/health/liveand/health/readyboth return{"status":"ok"}.The chain end to end, offline:
Fail-closed:
raised Github::Errors::FixtureMiss: the corpus defines no response for "/users/nobody"— no network attempt.Startup validation:
POLL_INTERVAL_SECONDS=60refuses to boot withpoll_allowance (60) + RATE_LIMIT_RESERVE (8) reaches the 60/hour limit, leaving -8 enrichment attempts.Locks in
pg_locks, greppable by namespace:Two defects were found by this verification, not by the suite, and are fixed in
7326954. The corpus pinnedx-ratelimit-resetto a fixed epoch already in the past, so the ledger correctly rolled the window on every poll and counters never accumulated — two polls both reportedpoll_used=1. Fixture mode was silently failing to demonstrate the accounting it exists to demonstrate; a+Nrelative value now resolves against an injectable clock, applied to that header alone. Separately,pg_advisory_lockreturnsvoidand Active Record printedunknown OID 2278to stderr on every gate acquisition — noise outside the single JSON stream §11 specifies — so the lock now selects the backend PID from a subquery.Documentation updates
.env.exampleand the README environment table carry all eleven new variables plus the formula and what is deliberately absent. The README Status section says plainly that nothing polls GitHub yet, and a new "The request path" section lands the request half of §14's Architecture summary.fixtures/github/README.mddocuments the corpus contract. Three ADRs, all topics §14 already lists.docker-compose.ymlgainsGITHUB_MODEin the sharedx-app-envanchor — the choice has to be process-wide — defaulting toliveper §12, and pinned toliveon thetestservice soGITHUB_MODE=fixture docker compose run --rm testcannot quietly run a different suite than CI.Known limitations
event_sourcesrow is provisioned and no code path fetches a page. Ingestion is PR 5, scheduled polling PR 6.fixtures/github/README.mddescribes contents rather than outcomes.addressableappears inGemfile.lockas a WebMock test-only transitive dependency; no application code uses it.Review round 1
Two findings, both reproduced in the Docker runner before anything was touched. Three defects fixed in
75c36db.1. Stale ledger reads under the query cache, and a lost reservation (
app/services/github/budget_ledger.rb)Reproduced exactly as described — reserve at
12:59:59, reconcile at13:00:01withx-ratelimit-reset=14:00, twobudget.window_rolledlines, thenLedgerInvariantViolation.Root cause: the Active Record query cache wraps
select_all, soGithubApiBudget.findis cacheable, while the rawexec_updatestatements the ledger writes with do not invalidate it. The reservation's post-debitfindpopulated the cache, androll_window!'sfindwas handed the pre-rollover row — sowindow_superseded?still saw the oldreset_at, rolled a second time, and then fell throughapply_observation's branches to theelse. Both ledger transactions now run insideGithubApiBudget.uncached.The two rollover predicates were also evaluated in sequence, which is what made a second rollover reachable at all. They describe one event, so they are now one decision — double-rolling is structurally impossible rather than merely unreachable.
The third defect is the one underneath the raise, and it was the more important of the two: a request reserved just before a reset boundary is counted by GitHub in the new window, which is what a superseding
reset_atmeans. Zeroing the counters discarded that debit, so the window would have issued one more request than GitHub honours —poll_used: 0besideremaining: 59.ROLL_WINDOW_SQLnow takes its post-reset counters as parameters, andreconcile!carries the in-flight request's class forward.reconcile!gainedrequest_class:for this and the executor passes it. The clock-driven rollover insidereserve!has no in-flight request and still starts clean.After the fix, the same sequence logs one
window_rolledcarrying"poll", thenwindow_initialized, returns:initialized, and leavespoll_used=1 remaining=59in the 14:00 window.2.
request_foralways built application-origin requests (app/services/github/event_sources/base.rb)Correct, and it would have made offline Link-driven pagination impossible through the adapter contract — the corpus publishes real
https://api.github.comLink targets because GitHub does, so an application-origin request built from one is refused asscheme_not_allowedin fixture mode.request_fornow takes anorigin:, defaulting to:application, and#linked_page_requestnames the Link case so the origin is not something a caller has to remember. The spec drives it from the actual header thepaginatedcorpus scenario serves rather than a hand-written URL, and the shared contract group now asserts a Link target survives the URL policy in either mode.Verified offline end to end: page 1 (8 events) → the published Link target → page 2 (3 events),
poll_used=2.Each fix was reverted individually to confirm its spec fails without it: uncached reads (1 failure), carry-forward (2), Link origin (2).
docker compose run --rm test— 500 examples, 0 failures. rubocop, brakeman, and bundler-audit clean.