Let a tripped circuit breaker keep retrying instead of latching open

[wickedOne]

fix for the problem mentioned in the Notes section of git-pkgs/proxy#275

Problem

CircuitBreakerFetcher.getBreaker builds its own backoff.ExponentialBackOff
and sets InitialInterval, MaxInterval and Multiplier, but leaves
MaxElapsedTime at cenk/backoff's DefaultMaxElapsedTime of 15 minutes.

Once the elapsed time passes that, NextBackOff() returns backoff.Stop, and
rubyist/circuitbreaker's state() only half-opens if cb.nextBackOff != backoff.Stop && since > cb.nextBackOff. The breaker therefore stops admitting
probes entirely. The one thing that would clear it is Success(), the sole
caller of BackOff.Reset() — and a success is unreachable while no call gets
through. So any outage lasting longer than 15 minutes leaves that host's breaker
open for the life of the process, long after the registry has recovered. Only a
restart clears it.

The breaker library itself does not have this problem: NewBreakerWithOptions
sets MaxElapsedTime to defaultBackoffMaxElapsedTime, which is 0, on the
backoff it constructs when no BackOff option is given. Supplying a custom
backoff is what silently opts into the 15 minute cut-off.

This was found in production. A proxy built on this package served npm metadata
normally while every uncached tarball returned 502 in about 0.2s with
circuit breaker open for registry [registry.npmjs.org](http://registry.npmjs.org/), for hours after
registry.npmjs.org was healthy again. Metadata does not go through the fetcher,
so only artifact downloads for that one host were affected, which made it look
like an npm-specific outage rather than latched local state.

A second defect made recovery slower and noisier than intended. Each fetch method
called breaker.Ready() as a pre-check and then breaker.Call(), which checks
the breaker again. A Ready() that observes half-open advances the backoff and
clears the half-open flag, so the check inside Call() re-tested against the
already-advanced interval and usually lost — spending the probe the call was
about to make. Measured against a dead upstream over 20 one-minute steps, only
3 requests actually reached it. Those refusals also returned the library's bare
circuit.ErrBreakerOpen, which does not wrap ErrUpstreamDown, so callers
branching on errors.Is(err, ErrUpstreamDown) — including
fetch/fetcher.go — did not recognise them.

Change

• expBackoff.MaxElapsedTime = 0 in getBreaker, matching the library's own
  default, so a tripped breaker keeps admitting one probe per backoff interval
  for as long as the registry stays down and closes as soon as one succeeds.
  Backoff growth is unchanged: 30s initial, doubling, capped at 5 minutes.
• Removed the redundant Ready() pre-check from FetchWithHeaders,
  FetchObservedWithHeaders and Head. The same measurement now shows 6 probes
  over the same 20 steps, at the intervals the backoff actually specifies.
• Added breakerError, which maps circuit.ErrBreakerOpen onto the wrapped
  ErrUpstreamDown and passes fetch errors through untouched, so every refusal
  to contact a registry reports the same way to callers.
• expBackoff.Clock is now set to the same clock the breaker uses. Previously
  the breaker read circuit.Options.Clock while its backoff read
  backoff.SystemClock, so the two measured time from independent sources.
• A new unexported clock field on CircuitBreakerFetcher supplies that clock,
  nil meaning clock.New(). It exists so the regression above can be tested:
  reproducing it requires pushing the backoff's elapsed time past 15 minutes,
  which is not something a test should wait for, and both clocks have to advance
  together for the reproduction to be faithful. It does not reach the breaker's
  failure-count window, which the library keeps on the system clock. No public
  API changes.
• [github.com/facebookgo/clock](http://github.com/facebookgo/clock%60) moves from indirect to direct in go.mod. It
  was already in the module graph as a dependency of rubyist/circuitbreaker;
  go.sum is unchanged.

Testing

TestCircuitBreakerRecoversAfterProlongedOutage trips a breaker against a
server returning 503, then advances a mock clock through an hour of failing
probes in 10 minute steps — each step longer than the 5 minute maximum interval,
so every step admits exactly one probe and the sequence is deterministic. The
server then recovers and the test asserts the next fetch succeeds and
GetBreakerState reports closed.

Without the one-line backoff fix it fails with the production symptom:

fetch after the registry recovered: circuit breaker open for registry
127.0.0.1:50460: upstream registry unavailable

TestCircuitBreakerProbesOncePerBackoffInterval covers the second defect: after
each interval elapses, exactly one request reaches the registry, a second call
in the same interval reaches it zero times, and both errors wrap
ErrUpstreamDown.

go build ./..., go vet ./... and go test ./... pass; the breaker tests also
pass under -race; golangci-lint run ./fetch/... reports 0 issues.

Docs

The README circuit breaker section now describes the trip condition accurately —
5 failures inside the breaker's rolling 10 second failure window, which is what
ThresholdTripFunc measures, rather than 5 consecutive failures — and states
that one request per backoff interval is let through as a probe while the rest
fail with ErrUpstreamDown without contacting the registry, and that retries
never give up, so a breaker recovers however long the registry was down. The
matching comment in getBreaker is corrected the same way.

Downstream

Consumers pick this up on their next dependency bump. Until then, a latched
breaker still needs a process restart to clear.

[Copilot]

Pull request overview

This PR updates the per-registry circuit breaker behavior in fetch so that a breaker does not permanently “latch open” after long outages, and so “breaker open” errors are consistently reported to callers.

Changes:

• Configure the custom exponential backoff used by CircuitBreakerFetcher to retry indefinitely (MaxElapsedTime = 0) and to share the same clock as the breaker.
• Remove redundant Ready() pre-checks so Call() alone governs probe admission, avoiding accidental probe “spending”.
• Normalize open-breaker errors via breakerError() so callers consistently see ErrUpstreamDown.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
README.md	Updates circuit breaker documentation to reflect rolling-window trip behavior and indefinite probing/retry semantics.
go.mod	Promotes github.com/facebookgo/clock to a direct dependency (used for breaker/backoff time source).
fetch/circuit_breaker.go	Implements indefinite backoff, unified clock usage, removes redundant Ready() checks, and maps breaker-open errors to ErrUpstreamDown.

---

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