Skip to content

feat: per-upstream circuit breaker to stop saturated-upstream pile-ups - #266

Merged
ety001 merged 4 commits into
nextfrom
feat/upstream-circuit-breaker
Sep 19, 2026
Merged

ety001 merged 4 commits into
nextfrom
feat/upstream-circuit-breaker

Conversation

@ety001

@ety001 ety001 commented Sep 18, 2026

Copy link
Copy Markdown
Member

Background

During the 2026-09-18 production incident, hivemind's DB connection pool saturated and every request jussi forwarded waited out its 3s deadline, got cancelled, and was immediately replaced by the next arrival. Jussi logged ~1.3M context deadline exceeded errors in 2 hours; the hivemind internal ALB recorded those cancelled requests as ~1.3M ELB-side 400s (they never reached a target); wallet SSR requests behind it hit 60s nginx timeouts and users saw steemitwallet.com 504s. Recovery only happened after the environment was scaled out — i.e. when the pool got enough slack for in-flight SQL to finish faster than new work arrived.

That is the gap this PR closes: when an upstream saturates, jussi has no mechanism to stop feeding it. A circuit breaker converts saturation into fast failures, which stops the arrival burst, lets the in-flight upstream work drain, and lets the backend self-heal — the same mechanism that scaling provided, without needing a human at 7am.

What this adds

A per-upstream-host circuit breaker (internal/upstream/breaker.go), wired into RequestProcessor.callHTTPUpstream:

  • Closed — requests flow normally. Failures feed a sliding window: upstream timeouts (including context deadlines jussi set itself), transport errors, and 5xx responses.
  • Open — requests fail fast without dialing the upstream, so the arrival burst stops and in-flight upstream work can drain.
  • Half-open — after the open period (plus jitter), exactly one probe request is admitted. Success closes the breaker (and clears the window, so the old failures cannot instantly re-trip it); failure reopens for another cycle.

Default tuning: 30s sliding window, 50% failure rate with a 20-sample minimum (so a handful of timeouts never trips it), 10s open period with 25% jitter (so multiple jussi instances don't probe a recovering backend in lockstep). Values chosen against the incident's traffic profile; all configurable via BreakerConfig.

Design notes:

  • One breaker per upstream host (scheme://host), so pooled URL variants share state.
  • Broadcast transactions keep their no-retry, fail-fast path — the breaker wraps both paths but does not change retry semantics. Breaker rejections are not RetriableUpstreamError, so the existing bounded retry cannot re-enter an open breaker.
  • The window clears on close and slides with time, so a recovered upstream is not haunted by its own history.

Observability

  • jussi_upstream_circuit_state gauge per upstream host (0=closed, 1=open, 2=half-open)
  • jussi_upstream_circuit_rejects_total counter per upstream host
  • Rejected requests return a structured JSON-RPC error (CodeUpstreamResponseErr, message "Upstream temporarily unavailable"), keeping them in the same client-visible error class as other upstream failures

Testing

  • 8 new unit tests in internal/upstream/breaker_test.go covering: stays-closed under low failure rate, trips at threshold, min-samples guard, rejects-while-open, single-probe half-open, close-on-successful-probe, reopen-on-failed-probe, window sliding, and registry keying. All pass.
  • go build ./..., go vet, and the full go test ./... suite pass.

Operational follow-ups (not in this PR)

  • The per-URN timeout config (configfiles.json / 08-upstream.config) can still be tuned independently — e.g. raising get_state specifically — and composes with the breaker: the breaker handles saturation, the timeout handles per-request budgets.
  • The 2→4 hivemind scale-out done during the incident can be revisited once this breaker is deployed, since saturation will now fail fast instead of piling up.

When an upstream saturates, jussi currently keeps forwarding every
request: each one waits out its timeout, gets cancelled, and the SQL the
upstream already started keeps running and holding its connection. New
requests keep arriving, so a slow backend never drains. This is the
amplification loop behind the 2026-09-18 hivemind storm (jussi logged
~1.3M deadline cancellations in 2 hours; the hivemind ALB recorded them
as ELB-side 400s while wallet SSR requests hit 504).

Add a per-upstream-host circuit breaker (sliding 30s window, 50% failure
rate with a 20-sample minimum, 10s open period with 25% jitter, and a
single-probe half-open state):

- closed: requests flow; failures (timeouts, transport errors, 5xx) feed
  the window
- open: requests fail fast without dialing the upstream, so arrivals
  stop and in-flight upstream work can drain
- half-open: exactly one probe request is admitted; success closes the
  breaker and clears the window, failure reopens for another cycle

Broadcast transactions keep their no-retry path; the breaker wraps both
the fail-fast and retry paths in callHTTPUpstream.

Observability: jussi_upstream_circuit_state gauge per upstream host and
jussi_upstream_circuit_rejects_total counter; rejections return a
structured JSON-RPC error (CodeUpstreamResponseErr, 'Upstream
temporarily unavailable') so clients see a normal upstream error class.

The breaker is deliberately not retried through (rejections are not
RetriableUpstreamError), so the existing bounded retry logic cannot
re-enter an open breaker.
…ns, env config

Review follow-ups for the circuit breaker PR:

1. get_state workaround bypassed the breaker (github review n.1).
   callSteemd dialed the upstream directly — beta-hivemind in
   production, i.e. the exact backend this breaker protects — with a
   15s sub-request timeout, invisible to the breaker. Route it through
   Allow()/Record() like every other upstream call; the emulated paths
   are idempotent reads, so failing fast while open is acceptable.

2. Structured JSON-RPC errors were flattened before reaching clients
   (review n.2). HandleError used a type switch, so any *JSONRPCError
   wrapped by fmt.Errorf fell through to the generic Internal error:
   code 1100 and message lost, plus one ERROR log line per rejection
   (~hundreds/sec while open). Switch to errors.As so wrapped typed
   errors survive; the generic path for plain errors is unchanged
   (regression-tested).

3. Half-open probe could be answered by a stale request (review n.3).
   Allow() now returns an opaque ProbeToken and Record() requires the
   matching token to decide the half-open outcome. Tokens are
   generation-numbered so a token from a previous cycle is rejected;
   a liveness guard re-admits a probe if the previous one never
   records, so half-open cannot deadlock.

4. Breaker parameters are now configurable via environment variables
   (review n.4): JUSSI_UPSTREAM_CIRCUIT_ENABLED / _WINDOW_SECONDS /
   _FAILURE_RATE / _MIN_SAMPLES / _OPEN_DURATION_SECONDS /
   _JITTER_FRACTION, wired through the standard viper binding table.
   Defaults match the previous hardcoded tuning; disabling the breaker
   (ENABLED=false) makes it a pass-through.

Also fix malformed object-form ttls/timeouts entries in
TEST_UPSTREAM_CONFIG.json so it parses under loadUpstreamConfig (found
while testing the env override path).
@ety001

ety001 commented Sep 19, 2026

Copy link
Copy Markdown
Member Author

Review follow-ups pushed (commit e78fe40)

All four review points are addressed in the second commit on steemit/jussi#266. go build, go vet, and the full test suite pass.

1. callSteemd bypass — fixed in this PR

callSteemd now goes through Allow() on entry and Record() on exit, exactly like callHTTPUpstream. Rejections return the same structured "Upstream temporarily unavailable" error (the emulated paths are idempotent reads, so failing fast is fine as you noted).

2. HandleError — switched to errors.As

HandleError now unwraps with errors.As, so every *JSONRPCError wrapped by fmt.Errorf("%w") reaches the client with its real code and message. This fixes the circuit-rejection path and all other wrapped typed errors (e.g. the upstream call failed: %w wrapper in ProcessSingleRequest). The default branch for plain errors is unchanged — generic Internal error, no text leakage — and I added regression tests for exactly that: wrapped typed error survives, double-wrapped survives, plain error stays generic without leaking internal details. Also kills the per-rejection ERROR log storm you flagged.

3. Probe attribution — Allow() returns a token

Allow() now returns (bool, ProbeToken). Only the half-open probe holder gets a non-nil token, and Record() requires the matching token to decide the half-open outcome:

  • tokens carry a generation number, so a token from a previous half-open cycle cannot answer the current probe;
  • a nil token (stale request admitted before the trip) can never decide the probe;
  • a liveness guard re-admits a fresh probe if the current one never records (lost response / panic), so half-open cannot deadlock.

New tests: stale-request-cannot-answer-probe, token-is-single-cycle, liveness-guard.

4. Env-var configuration — implemented (was already on my list, now done)

The breaker is wired into the viper config tree as upstream.circuit.* and bound to env vars through the same mechanism every other jussi knob uses:

JUSSI_UPSTREAM_CIRCUIT_ENABLED=true            # false = pass-through kill switch
JUSSI_UPSTREAM_CIRCUIT_WINDOW_SECONDS=30       # sliding window
JUSSI_UPSTREAM_CIRCUIT_FAILURE_RATE=0.5        # trip threshold
JUSSI_UPSTREAM_CIRCUIT_MIN_SAMPLES=20          # min samples before evaluating
JUSSI_UPSTREAM_CIRCUIT_OPEN_DURATION_SECONDS=10
JUSSI_UPSTREAM_CIRCUIT_JITTER_FRACTION=0.25

Defaults equal the hardcoded tuning; unset values fall back to defaults. Two tests cover the defaults and a full env-override pass through the real LoadConfig(). (Inherited constraint, same as all jussi config: picked up at instance start, so a change rolls out with a deploy rather than mid-flight.)

One extra fix that fell out of testing the env path: tests/data/configs/TEST_UPSTREAM_CONFIG.json had malformed object-form ttls/timeouts entries that didn't parse under loadUpstreamConfig at all — normalized them to array form.

Answers you asked for as proposals (no code yet)

Third item (integration test proving callHTTPUpstream actually passes the breaker): proposal — a handler-level test with an httptest.Server as the upstream: (a) server returns 500s → assert requests stop hitting the server after the window trips (counter on the server handler); (b) server recovers → assert the probe gets through and traffic resumes; (c) breaker open → assert the client-visible error is the structured circuit-open JSON-RPC error, not Internal error. I can add this to this PR or a follow-up — say which.

Fourth item (metric labels use full URL while the registry aggregates per host): proposal — label by scheme://host (the registry key) instead of the raw URL. Concretely: thread the registry key through For() (have it return breaker + key), use the key for jussi_upstream_circuit_state / _rejects_total, keeping one series per host so Grafana dashboards don't show duplicate states per URL variant. Small diff, but it changes metric cardinality/labels, so I'd rather do it as a separate commit after you confirm the label shape (existing dashboards would need a note since the label value changes from full URL to host).

Also noting: Registry.Snapshot() (the other dead-code remark) is now actually useful for the health endpoint once we pick the label shape — I'd wire Snapshot() into /health in the same follow-up.

…ker state in /health

Closes out the remaining review items:

Integration tests (review item 3): a new handler-level suite exercises
the breaker against a real httptest upstream —

- failure window trips the breaker and requests stop reaching the
  upstream while it is open (hit counter proves zero leakage)
- after the upstream heals and the open period lapses, a single probe
  gets through, succeeds, and closes the breaker so traffic resumes
- rejections surface as the structured 'Upstream temporarily
  unavailable' JSON-RPC error (code 1100) with breaker context in
  data.details, not the generic Internal error
- upstream.circuit.enabled=false is a true pass-through

Per-host metric labels (review item 4): Registry.For now returns the
breaker's registry key (scheme://host) alongside the breaker, and the
jussi_upstream_circuit_state / jussi_upstream_circuit_rejects_total
series use that key instead of the full URL, so all URL variants of one
upstream produce exactly one series.

/health observability: the health payload now includes
circuit_states (per-upstream breaker state), plus circuit_degraded and
circuit_worst_state flags when any breaker is open or half-open. This
gives the ELB health path an explicit, scrape-independent view of
breaker state alongside the Prometheus gauge.
Dashboard panels, Prometheus alert rules, and the cross-service view
for the circuit breaker, tuned against the 2026-09-18 incident profile.
Written for the watchtower stack (Prometheus + Grafana + OpenObserve);
no new jussi-side collection is required.
@ety001
ety001 merged commit dc1983d into next Sep 19, 2026
2 checks passed
@ety001
ety001 deleted the feat/upstream-circuit-breaker branch September 19, 2026 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant