fix(github): route every engine GitHub call through the breaker
Stacks on preloopdev/preloop#685; retarget to main after it merges.
What & why
Thirteen engine GitHub call sites still sent straight through the shared HTTP
client, so a GitHub outage cost one round trip per caller for the whole
outage instead of one probe per credential. Every one of them now goes through
crates/preloop-runner-server/src/github_breaker.rs, on the credential bucket
the call actually spends.
The breaker applied to the webhook path only; the rest of the engine discovered
the same outage independently. This routes them all through one funnel:
send_github(state, credential, subsystem, request)— breaker + per-subsystem
accounting for a caller-built request (streaming relays, header-only reads,
per-request timeouts).send_github_json(state, credential, subsystem, method, url, body)— the
engine's standard GitHub JSON call, decoding with the failure text callers
already classify on.GithubCredential— token and bucket travel together, so a call cannot be
admitted or accounted under a credential it does not spend;authorize()
attaches the bearer and contributes nothing for an anonymous call.GithubApiError— the non-success status stays typed (dispatch auth branches
on it) whileDisplaykeepsstatus {code}for the push-back classifier.
Per-site routing
| # | Site | Call | Bucket | Subsystem |
|---|---|---|---|---|
| G1 | dispatch_auth.rs fetch_json (/installation, /installation/repositories, /user, /app) | installation-token round-trip, PAT and App-JWT actor lookups | InstallationUnknown for /installation, then Installation(id) for the listing; Pat; App(id) | dispatch |
| G2 | github_push.rs github_json + github_pr.rs | commit/tree verification, repo metadata, PR list/create | Installation(id) from the mint ledger, else InstallationUnknown; Pat | push-back |
| G3 | snapshots.rs forge_relay_forward (git + tarball + api/v3 relay, 3 callers) | anonymous forge relay | Anonymous | snapshots |
| G4 | dispatch.rs resolve_default_branch | GET /repos/{repo} | Installation(id) from the mint ledger, else InstallationUnknown; Pat (env PAT) | dispatch |
| G5 | runs.rs pat_oauth_scopes | GET {api}/ PAT scope probe | Pat | credentials |
| G6 | github.rs exchange_manifest_code | POST /app-manifests/{code}/conversions | Anonymous (the code is the credential) | app-auth |
| G7 | scheduler.rs scan_remote | repo metadata + commit lookup | Pat | workflows |
| G8 | snapshots.rs resolve_github_repo_meta | GET /repos/{slug} | Installation(id) / Pat / Anonymous, per the credential it already selected | snapshots |
Every call keeps its current bound and error mapping: dispatch auth still fails
closed (502) on a refusal while the breaker is open and still turns a 401/403
into a rejected credential; push-back answers 502 for it (the classifier now
recognises the breaker's own text as transient); the forge relay, the schedule
scan and the repository-metadata lookup still degrade quietly; the manifest
exchange still maps everything to 500. Per-call client timeouts are unchanged
(the 10 s snapshot metadata bound, the 300 s relay client).
Two new GithubCredentialBucket/GithubSubsystem shapes were needed: the
installation:unknown bucket (a third-party installation token whose id only
arrives in the response being made — inventing an id would charge another
installation, and the PAT's bucket belongs to the operator) and the dispatch,
push-back, credentials subsystems.
Deliberately outside the breaker
- G9 — presigned cross-host LFS blob downloads (
snapshots.rs) already
bypass it behind avia_githubcheck, since they are not GitHub traffic and
spend no GitHub API budget; the credential is stripped for them too. Unchanged. - G10 — orchestrator golden metadata lookups are not in this PR: the
orchestrator has no breaker/AppStatehandle (it would need plumbing through
RunnerPoolConfig). Reported to the parent workstream.
Protocol surface
- I confirm this change touches the runner protocol interface: NO
(no registration/broker/Twirp/check-run wire shapes; GitHub API calls only)
Verification performed
cargo fmt --all --check
cargo clippy --locked -p preloop-runner-server --all-targets -- -D warnings
cargo test --locked -p preloop-runner-server --lib -- github_breaker dispatch_tests snapshots
cargo test --locked -p preloop-runner-server --lib -- scheduler:: runs:: github_push:: github_pr:: dispatch github::
fmt --checkandclippy -D warningsare clean, re-run on the rebase onto
fix/bound-shared@ 793573744.- The required run (
github_breaker,dispatch_tests,snapshots) is green on
that head: 147 passed / 0 failed, and the five new tests pass individually. - Before the rebase, the adjacent modules were also green: 208 passed /
1 failed (the fixture fixed in the second commit) and 73 passed / 0 failed for
bootstrap/broker/distributed_task/github_app/webhook_watchdog/
webhook_health.
New tests (github_breaker.rs), all green:
an_open_breaker_refuses_a_json_call_without_touching_github— three 5xx from a
stub open the PAT bucket; the next call is refused with a typedBreakerOpen,
the stub's hit count stays at 3, andbreaker_blockedcounts the refusal.a_reachable_answer_closes_the_breaker_and_buckets_stay_independent— an answer
resets the failure count, a 2xx underAnonymousflows while the PAT bucket has
failures, and the anonymous request carries noAuthorizationheader.transport_failures_open_the_credential_breaker— connection-refused ×3 opens
the bucket it was called under.api_errors_carry_status_and_body— a non-2xx is typed and still reads as
status 404in the message.
New test (dispatch_tests.rs):
open_breaker_refuses_a_third_party_dispatch_without_touching_github— with the
installation:unknownbucket open, a third-party (online-validated) token
fails closed with 502 and the stub sees zero requests.
The one adapted test
github::tests::pat_throttle_on_actions_parks_check_run_sender stubbed a 429
for every path. The submit-time PAT scope probe now shares the PAT's breaker,
so the fixture's own submit parked the bucket before the test parked it, and
the test's first call failed with GitHub circuit breaker is open (retry after ~50s). The stub now throttles only the actions path; the probe's API root
answers 200 with no x-oauth-scopes header, which is the same Unverifiable
verdict the 429 produced. That is the only test whose fixture changed.
Checklist
- Tests added/updated for the observable contract (breaker routing, fail-fast,
bucket partitioning, credential never sent to an anonymous call) - Docs updated:
docs/control-plane/github/webhook-resilience.mdx(breaker
coverage now states the whole engine and the cross-host LFS exception; the
olddocs/webhook-resilience.mdwas retired by main's docs restructure) - Changelog entry under Unreleased → Fixed