Commit graph

2 commits

Author SHA1 Message Date
Deluan Quintão
3784fd0ea7
fix(artwork): honor a provider's explicit retry-later delay in the circuit breaker (#6056)
An explicit RetryLaterError now opens the agent's breaker immediately for the
provider's own delay, instead of counting it as one generic failure that needs
five to open and then always probes after a fixed minute.
2026-09-01 11:07:58 -04:00
Deluan Quintão
24311918c7
fix(artwork): ramp the external circuit breaker back up instead of closing on one answer (#5961)
* fix(artwork): ramp the external circuit breaker back up instead of closing on one answer

The breaker went straight from open to fully closed on a single non-transient response,
so recovery was a burst: the agent resumed at the limiter's full rate until five
consecutive failures reopened it. A not-found counted as that response, and a provider
that is blocking still answers the occasional request, so the cycle never settled.

Observed on a production library over 100 minutes with apple-music blocked. Of 232
responses, 228 were 403 and 4 were not-found, and those four closed the breaker four
times. Each close was followed by another open 1 to 3 seconds later, with about five
requests in between:

  00:59:05 closed -> 00:59:06 opened
  01:23:13 closed -> 01:23:16 opened
  01:41:21 closed -> 01:41:24 opened

Closing now needs breakerRecoveries consecutive answers, one per probe interval, and any
failure discards the count. A not-found still counts, because the provider did answer, but
it can no longer close the breaker by itself.

Unrelated to the plugin loading in the rest of this PR; it came out of investigating why
iTunes kept returning 403 while the breaker was open.

* fix(artwork): count only current-episode probes toward breaker recovery

The worker drains concurrently, so when the breaker opens there are already calls past
allow(), queued in the rate limiter or waiting on a response. Their answers arrive after
the open and reached the recovery counter, so breakerRecoveries of them closed the breaker
with no probe interval elapsed at all: the burst the ramp exists to prevent.

allow() now returns the open episode a call was admitted under, zero when the breaker was
closed, and only an answer whose generation matches the current episode counts. The
generation also invalidates a probe whose answer lands after the breaker closed and
reopened, which a plain probe flag would credit to the wrong episode.

The token never crosses the gateFunc seam: allow and record are both called inside
Worker.gate, so passthroughGate, tracingGate and offlineGate are untouched.

The regression test needs no fake clock. The race is an ordering, not a duration, so it is
reproduced by calling allow and record in the order concurrency produces, which is
deterministic where a goroutine-based test would pass on a lucky schedule.

Found by Codex.

* test(artwork): move the breaker ordering spec into the Ginkgo suite

The ordering regression does not need a fake clock, so it does not need the plain
testing.T runner either. That runner is only used here because testing/synctest requires
it; every other spec belongs in the Ginkgo suite.

The three specs left in worker_timing_test.go all drive the fake clock.
2026-08-15 10:36:14 -04:00