Report upstream circuit breaker state in /health and /metrics

[wickedOne]

Problem

The artifact fetcher wraps every upstream in a per-host circuit breaker, but its state was invisible from outside the process. metrics.UpdateCircuitBreakerState and metrics.RecordCircuitBreakerTrip existed and were never called outside their own unit test, and CircuitBreakerFetcher.GetBreakerState() was never consulted.

That made an open breaker hard to diagnose, because everything around it still looks healthy:

• package metadata for the same ecosystem keeps serving (metadata goes through Proxy.HTTPClient, not the fetcher, so no breaker applies),
• already-cached artifacts keep serving (the cache is checked before the fetcher),
• other ecosystems keep serving (breakers are per host),
• /health reports database and storage as ok.

Only uncached artifact downloads for that one host fail, with HTTP 502 and no request reaching the upstream. We hit this on an npm-only failure: every *.tgz returned 502 {"error":"failed to fetch package"} in ~0.2s while packuments returned 200 and cargo and gem downloads were unaffected. Identifying the breaker as the cause meant reading pod logs for circuit breaker open for registry [registry.npmjs.org](http://registry.npmjs.org/).

Change

Adds a breakerMonitor that reads GetBreakerState() and publishes it to three places:

• /health: a new circuit_breakers object mapping upstream host to "open" / "closed", omitted while no breaker exists.
• /metrics: populates proxy_circuit_breaker_state (0 closed, 2 open) and proxy_circuit_breaker_trips_total, refreshed on each scrape. Trips are counted on observed closed→open transitions, since the fetcher exposes current
  state rather than trip events.
• Logs: one ERROR when a breaker opens and one INFO when it closes, instead of a per-request error that gives no indication the breaker is the cause.

Two deliberate choices:

An open breaker leaves status as "ok" and the HTTP status code unchanged. It describes an upstream refusing to serve, not this proxy being unfit for traffic, and /health is documented as a readiness probe: failing it over one bad upstream would pull the pod from rotation for every other ecosystem. Alerting belongs on the gauge.

Metric series are published only for registries that have tripped at least once. A breaker is created per host the proxy fetches artifacts from, and for some ecosystems that host comes from upstream metadata rather than configuration (composer takes it from a package's dist.url, helm from the chart URLs in index.yaml), so publishing every host would let upstream content grow the series count for the life of the process. A host that has never tripped carries no information a series could convey; once it trips it keeps reporting, so recovery is still visible as a transition to 0 rather than as a series that
vanishes. /health is a per-request response rather than a persistent series and lists every breaker either way.

The monitor takes a one-method breakerStateSource interface rather than *fetch.CircuitBreakerFetcher directly, so trip and recovery transitions can be tested without waiting out a 30-second backoff.

Notes

This is observability only. It does not change breaker behaviour, and in particular does not fix the upstream cause of a breaker that never recovers: git-pkgs/registries' CircuitBreakerFetcher.getBreaker supplies a custom
backoff.ExponentialBackOff without setting MaxElapsedTime, so it keeps the cenk/backoff default of 15 minutes. Once NextBackOff() returns backoff.Stop, rubyist/circuitbreaker's state() never returns half-open again and the breaker stays open for the life of the process even after the upstream recovers. That needs a one-line fix in registries (expBackoff.MaxElapsedTime = 0, matching the library's own default); until then a restart is the only way to clear a latched breaker, which this MR documents.

Worth a second opinion: /health is unauthenticated on the same port package clients use, and now returns upstream hostnames. For deployments pointing upstream.* at internal mirrors, those hostnames become readable by anything that can reach the proxy. Exposing the host is the point of the field, so the alternative is restricting /health and /metrics at the ingress.

Testing

• New unit tests: open breaker reported and counted once (not once per scrape); healthy registry reported to /health but publishing no series; recovery after a trip reported as 0 with the series retained; a re-trip counted separately; nil-safe monitor; /health includes an open breaker while staying 200; and circuit_breakers omitted when no breaker exists. Metric assertions collect from the registry rather than calling WithLabelValues, which would create the series under test.
• Manual verification against a local instance with upstream.npm pointed at a dead port plus one successful crates.io download: /health reported {"127.0.0.1:19999":"open","[static.crates.io](http://static.crates.io/)":"closed"} while /metrics carried series for the tripped host only, with state 2 and one trip, and exactly one transition log line.
• go test ./... and golangci-lint run ./internal/server/... clean; go test -race -count=2 ./internal/server -run 'Breaker|Health' clean.
• Swagger regenerated via go generate ./internal/server.

Docs

• README.md: both metrics added to the metrics table; refresh, trip-counting and series-publishing semantics; alerting guidance including what does and does not keep working while a breaker is open; circuit_breakers in the /health sample and prose; and why an open breaker does not fail the probe.
• docs/architecture.md: the /health bullet covers the new field.

[Copilot]

Pull request overview

This PR adds observability for per-upstream artifact-fetch circuit breakers by surfacing breaker state via /health, exporting Prometheus metrics on /metrics, and emitting transition logs when breakers open/close.

Changes:

• Introduces a breakerMonitor that snapshots CircuitBreakerFetcher breaker state, updates Prometheus metrics, and logs open/close transitions.
• Extends /health (HealthResponse) to optionally include a circuit_breakers map (host → "open"/"closed"), without affecting readiness status.
• Updates documentation and Swagger outputs to describe the new /health field and the two new metrics.

Reviewed changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
README.md	Documents breaker state in /health and the new Prometheus metrics + semantics.
internal/server/server.go	Wires breakerMonitor into server startup, /metrics scrape path, and /health response.
internal/server/server_test.go	Exposes the constructed *Server from the test harness to allow injecting a breaker monitor.
internal/server/health.go	Adds CircuitBreakers field to HealthResponse with omitempty JSON tag and explanatory comment.
internal/server/breakers.go	New monitor implementation: snapshots breaker state, updates metrics, and logs transitions.
internal/server/breakers_test.go	New unit tests covering state reporting, metric series behavior, transitions, and /health output.
docs/swagger/swagger.json	Regenerates Swagger to include circuit_breakers in the health schema.
docs/swagger/docs.go	Regenerates embedded Swagger template to include circuit_breakers.
docs/architecture.md	Updates architecture docs to mention breaker state in /health and metrics.

Files not reviewed (1)

• docs/swagger/docs.go: Generated file

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[andrew]

Thanks, this fills a real gap and the design calls (open breaker keeps /health 200, series only for tripped hosts) both look right to me.

One change:

internal/server/breakers.go — m.source.GetBreakerState() is read before m.mu.Lock(). Two concurrent snapshot() calls (/health and a /metrics scrape landing during a transition) can interleave so an older closed read is applied to seen after a newer open one: one real trip becomes two in the counter, plus a spurious "circuit breaker closed" log and the gauge briefly at 0 while the breaker is actually open. GetBreakerState() is just a map copy, so moving it under the lock removes the ordering hazard at no cost. -race won't flag this since it's a logical ordering issue rather than a data race.

On the two open points:

• /health exposing upstream hostnames: /metrics on the same unauthenticated port already carries the same hostnames in the registry label as of this change, so /health adds nothing new. Restricting both at the ingress is the answer for deployments that care; no change needed here.
• The MaxElapsedTime latching in git-pkgs/registries is real and there's no issue for it yet. I'll open one there; nothing to do in this PR.

[andrew]

Correction on the last point: there is already a fix open at git-pkgs/registries#70, so no separate issue needed.