fix(orchestrator): route the golden-release GitHub calls through the engine's circuit breaker

Merged
BN
5d ago
+1292-142

Stacks on #693 (fix/bound-orchestrator) → #685 (fix/bound-shared); retarget to main after they merge.

Closes G10, the last row of the "no GitHub call may bypass the breaker" audit: the orchestrator's golden-release calls. #693 bounded their deadlines (30 s metadata, 1 h downloads) but left them outside the breaker — the pool had no handle on AppState's registry, so a GitHub outage discovered by the webhook path was rediscovered by the golden lookup, at the cost of its own 30-second wait.

What changed

One funnel, carried to both sides. New GithubFunnel in github_breaker.rs bundles the breaker registry and the per-subsystem consumption recorder — the pair AppState already held separately — so it can be cloned into a component that is not the server:

  • ServerConfig::github: Option<GithubFunnel> is adopted by AppState::new_with_store before the App registry attaches its accounting, so app-auth traffic and the pool share one registry rather than two.
  • RunnerPoolConfig::github: Option<GithubFunnel> carries the same value (cloned) into the pool. preloop serve creates it once and passes it to both sides.
  • None (a one-shot preloop golden build, tests, the standalone preloop-server binary) means "no engine to share with": the pool keeps a private funnel rather than letting a GitHub call go unobserved. The funnel is per pool, not a process-wide static, so parallel tests cannot trip each other's breakers.

Every golden call now goes through send_observed_labeled on the anonymous bucket (all of them are unauthenticated) under a new golden subsystem label, via one golden_send helper in the orchestrator so the funnel sees the whole dependency:

SiteCallBucketSubsystemDeadline
resolve_latest_golden_tagGET api.github.com/repos/preloopdev/preloop/releases (the tag lookup)Anonymousgolden30 s metadata client
oci::get_manifestOCI manifest, index follow-upAnonymousgolden30 s metadata client
oci::request_bearer_tokenghcr.io/token bearer handshakeAnonymousgolden30 s metadata client
download_release_asset<asset>.sha256 sidecarAnonymousgolden30 s client + 15 s request timeout (unchanged)
download_release_assetrelease-asset probe and every resume attempt (Range)Anonymousgolden1 h download client (unchanged)
download_oci_goldenblob probe and every resume attempt (registry_get)Anonymousgolden1 h download client (unchanged)

oci's public API gained an Option<&GithubFunnel> gate (registry_get, request_bearer_token, get_manifest, fetch_json); preloop init's one-shot manifest probe passes None and sends plainly.

Behaviour

  • Breaker open ⇒ the metadata lookup fails fast with a clear error. The refusal comes back as BreakerOpen ("GitHub circuit breaker is open (retry after 30s)"), nothing reaches the network, and none of the 30 s metadata deadline is spent. resolve_latest_golden_tag logs it at warn and — deliberately — does not write its tag cache: a cached negative answer would outlive the breaker's own window (5 min TTL against a 30 s base open) and park the lookup after recovery. The caller still falls through to the latest redirect, as it always did for a failed lookup.
  • Downloads change only in their admission decision. A refused transfer attempt fails like any other failed attempt (logged, next candidate, then local build); an admitted one keeps its 1 h budget, its resume offsets and its retry loop.
  • Refusals are accounted as breaker_blocked on the golden row of the consumption snapshot (and successful exchanges as requests), so the ops snapshot and the webhooks health endpoint show the pool's GitHub spend next to every other subsystem.

Deviations, called out

  • Based on #693, not #685. #693 restructures exactly these functions (the golden_http_client split, GOLDEN_METADATA_TIMEOUT, the .sha256/manifest clients) and the 30 s metadata deadline this PR must keep exists only there. Stacking one level up keeps the diff minimal and avoids re-implementing #693's bound.
  • send_observed_labeled, not send_observed. The golden calls run where an engine funnel exists, and the audit asks for a subsystem label; the labeled funnel is what makes the label real (and the refusal visible in the snapshot). The neutral send_observed is still what the pre-AppState bootstrap paths use.
  • The plumbing is Option<GithubFunnel> rather than Option<Arc<GithubBreakerRegistry>>: send_observed_labeled needs the recorder too, and creating the pair in the CLI is what lets AppState and the pool share one registry (the pool config is built before serve constructs the state).

Tests

  • open_breaker_refuses_the_golden_release_lookup_before_it_spends_the_deadline (new): trips the anonymous bucket, then asserts the lookup returns None in well under GOLDEN_METADATA_TIMEOUT, that a counting stub server received zero connections, that the refusal is reported as breaker_blocked under golden, and that after the open window lapses the same lookup reaches the stub, answers v9.9.9, and closes the breaker.
  • subsystem_labels_are_a_small_closed_set extended with golden.
  • Every existing golden-download/orchestrator test passes the funnel explicitly (test_github()), so the download path is exercised with a closed breaker exactly as before.

Checklist

  • Tests added/updated for the observable contract (fail-fast with the breaker open, no request sent, breaker closes on a reachable answer, golden accounting)
  • Docs updated: docs/webhook-resilience.md (breaker coverage now names the golden path)
  • Changelog entry under Unreleased → Fixed
  • Gates: cargo fmt --all --check, cargo clippy --locked -p preloop-orchestrator -p preloop-runner-server -p preloop-cli --all-targets -- -D warnings, cargo test --locked -p preloop-orchestrator, cargo test --locked -p preloop-runner-server github_breaker, cargo test --locked -p preloop-runner-server bootstrap

Devin Review


Summary by cubic

Routes the runner pool's golden-release GitHub calls through the engine's circuit breaker — the last GitHub traffic that bypassed it — and fixes a spurious 408 that dropped the upload-complete event behind queued progress frames.

  • A new GithubFunnel bundles the breaker registry and per-subsystem accounting so AppState and the runner pool share one registry. All golden calls — release listing, OCI manifest and token lookups, .sha256 sidecars, and transfers — go through the anonymous bucket under a new golden subsystem label; one-shot CLI paths (golden build, preloop init probes, the standalone server) keep a private funnel rather than leaving a call unobserved.
  • Breaker state is keyed by (service origin, credential), so a mirror, non-GHCR registry, token realm, or GHES instance keeps its own availability and rate-limit state; golden_send consults the target origin's cross-bucket state first, so an outage the webhook path already parked refuses the lookup before a connection opens. The anonymous Bearer 401 handshake no longer clears the failure streak; it is accounted as an expected protocol step and the retried token-bearing request keeps its old semantics.
  • The golden row reports bytes moved: streamed chunks are charged as they arrive and metadata bodies by advertised length.
  • With the breaker open, the metadata lookup fails fast: no connection is opened, none of the 30 s deadline is spent, and the refusal is deliberately not cached as a negative tag, whose 5-minute TTL would outlive the breaker's own window. Downloads keep their 1-hour budget, resume offsets, and retry loop — only their admission decision changes. Refusals are accounted as breaker_blocked on the golden row, so ops and health snapshots show the pool's GitHub spend.

Bug Fixes

  • The upload-complete event is no longer dropped behind progress, and progress no longer queues memory on the unauthenticated ingress: the gate reports the upload's phase over a watch channel, so a one-byte-frame burst replaces one phase instead of enqueueing ~33.5M events and Complete can never sit behind a Progress. A small successful upload cannot turn into a spurious 408 under load.

Tests trip the anonymous bucket and assert the lookup returns None well under the deadline, a counting stub server sees zero connections, the refusal is accounted, and the same lookup answers and closes the breaker once the window lapses. The fail-fast test builds its client before tripping the breaker so the OS certificate-store load (up to five seconds on the process's first client) cannot race the open window and admit the request the test needs refused. Additional tests cover origin partitioning, foreign-host naming, the expected-challenge streak, a parked App bucket refusing across buckets, and transfer byte accounting.