JITSU-182: config-keeper circuit breaker — hold last-known-good on mass option changes - #1476
JITSU-182: config-keeper circuit breaker — hold last-known-good on mass option changes#1476absorbb wants to merge 7 commits into
Conversation
…od on mass option changes (JITSU-182)
Rejects a repository refresh when >CFGKPR_BREAKER_MAX_CHANGE_PERCENT (50%) of
rows present in both generations changed at once (floor: 20 rows) — the
2026-07-30 blank-options incident shape. appbase.AbstractRepository keeps
last-known-good on Init error, so every config-keeper consumer is protected
with zero framework changes. Baseline seeds from the on-disk cache (restarts
cannot bypass); loadCached replays keep the held state; zero-diff network
payloads recover it. Operator override: POST /breaker/{repo}/accept (per
replica). Held repos stay healthy in /health with a breaker status field.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![jitsu-code-review[bot]](https://avatars.githubusercontent.com/in/3391383?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the circuit-breaker integration in config-keeper (startup wiring, health behavior, operator accept endpoint, and breaker lifecycle tests). I found one correctness regression in /health around repository name parsing and left an inline comment with details.
…rough config-keeper Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Reviewed the config-keeper circuit-breaker implementation (app.go, breaker.go, router.go, config wiring) and ran go test ./... in bulker/config-keeper (passes). I left one inline comment for a potential fail-open path where malformed rows without id can bypass trip detection.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Reviewed the changes in , focusing on breaker behavior, repository wiring, health semantics, and the new operator accept endpoint.
I didn’t find additional actionable issues beyond the existing open discussion threads; overall the breaker implementation and regression tests look coherent for the intended failure modes.
There was a problem hiding this comment.
Reviewed the 793d02a..6e98392 changes in config-keeper, focusing on breaker behavior, repository wiring, health semantics, and the new operator accept endpoint.
I didn’t find additional actionable issues beyond the existing open discussion threads; overall the breaker implementation and regression tests look coherent for the intended failure modes.
…rage collapse Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Reviewed the config-keeper changes in 793d02a..6e98392, including the new repository circuit-breaker flow, syncs repository wiring, health-path behavior, and breaker endpoint handling.\n\nI also ran the config-keeper test suite (FAIL ./... [setup failed]
FAIL in ) and checked existing review threads to avoid duplicating resolved items.\n\nNo additional bugs, security issues, or correctness regressions stood out in this range.
There was a problem hiding this comment.
Reviewed the config-keeper changes in 793d02a..6e98392, including the new repository circuit-breaker flow, syncs repository wiring, health behavior, and breaker endpoint handling.\n\nI also ran the config-keeper test suite (go test ./... in bulker/config-keeper) and checked existing review threads to avoid duplicating resolved items.\n\nNo additional bugs, security issues, or correctness regressions stood out in this range.
There was a problem hiding this comment.
Reviewed the breaker integration across app.go, router.go, breaker.go, and tests. The overall design and coverage look solid (including baseline seeding, id-coverage validation, and held-state handling). I left one inline question about a potential privilege/safety regression around POST /breaker/:repository/accept.
…-arm cannot bypass a future incident Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Reviewed the new config-keeper circuit-breaker flow across app.go, router.go, and breaker.go, including the new tests.
I found one edge-case bug candidate worth addressing: the id-coverage guard appears to still be gated by breaker thresholds/floor, which can allow small payloads that lose all id fields to be accepted.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Reviewed the config-keeper circuit-breaker change set end-to-end (wiring in app/config/router plus breaker logic and tests), and cross-checked previously resolved review threads to avoid re-raising handled items.
I also ran go test ./... in bulker/config-keeper successfully.
No new correctness, security, or user-visible regression issues stood out in the current diff.
The validity rejections (unparseable payload, id-coverage collapse) returned plain errors, so a console shipping id-less rows — the JITSU-158 shape — surfaced only as a generic stale-repository health failure, not as a breaker alert. All rejection paths now go through one throttled alert helper built on the existing logging.SystemErrorf instead of a hand-rolled prefix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Reviewed the config-keeper breaker changes across app wiring, breaker behavior, router endpoints, and tests.
I also checked existing review threads and avoided re-raising resolved points.
No new actionable bugs, security issues, or correctness regressions stood out in this range.
| repository = appbase.NewHTTPRepository[[]byte](repName, r.appContext.config.RepositoryBaseURL+"/"+repName, r.appContext.config.RepositoryAuthToken, appbase.HTTPTagLastModified, &RawRepositoryData{validateJSON: true}, 2, r.appContext.config.RepositoryRefreshPeriodSec, r.appContext.config.CacheDir) | ||
| // lazily created repositories get breaker protection too when their | ||
| // name is in BREAKER_REPOSITORIES; they are not registered in | ||
| // appContext.breakers (populated at startup only — mutating it here |
There was a problem hiding this comment.
Potential recoverability gap: this path can create a breaker for lazily initialized repos, but because it is not added to appContext.breakers, POST /breaker/{repository}/accept will return 404 and /health won’t expose held status for that repo. Is that trade-off intentional for guarded repos outside REPOSITORIES, or should we register these breakers in a concurrency-safe structure so operators can still recover a held replica?
1 participant
Containment control from the 2026-07-30 postmortem (
JITSU-158item 6, tracked asJITSU-182): config-keeper now rejects a repository refresh that materially changes too many rows at once, holding the last-known-good payload for every consumer — the control that would have reduced the blank-options incident to zero damage regardless of the console bug.How it works
BreakerRepositoryDatawraps the payload cache for guarded repos (bulker-connections,rotor-connections,streams-with-destinations,workspaces-with-profiles). Each accepted generation's rows (keyed byid, fnv64a over raw bytes) become the baseline; a refresh where >50% of common rows changed AND ≥20 rows trips the breaker.Initreturns an error →appbase.AbstractRepositorykeeps last-known-good and re-checks every poll. Zero framework changes; self-heals when the console serves a sane payload again.CFGKPR_BREAKER_MAX_REMOVE_PERCENT(50%) of the baseline vanishing at once (>=20-row floor) is rejected — a console bug filtering rows out is as destructive as blanking them. Moderate cleanups pass.loadCachedbootstrap replay keeps the held state (distinguished from a genuine zero-diff network recovery via the nil tag — otherwise the stored Last-Modified would 304-wedge held forever).System error:marker on trip transition, then ≤1/min (JITSU-183wires the alert)./healthshowsbreaker: {held, heldSince, reason}but held repos stay healthy — failing liveness would restart pods into the same held state.POST /breaker/{repository}/accept(bearer-auth, one-shot, per replica — response includes the pod hostname). Global switches:CFGKPR_BREAKER_ENABLED=false, thresholds viaCFGKPR_BREAKER_MAX_CHANGE_PERCENT/CFGKPR_BREAKER_MIN_CHANGED_ROWS/CFGKPR_BREAKER_REPOSITORIES.Known limitations (documented in code + ticket)
14 unit tests cover trip shape, floor, adds/removals, one-shot accept, invalid JSON, cache seeding, bootstrap replay vs network recovery, and rejection short-circuiting.
Design note: see the plan-of-record comment on
JITSU-182.🤖 Generated with Claude Code