JITSU-182: config-keeper circuit breaker — hold last-known-good on mass option changes - #1476
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.
…ept endpoint The maps are now guarded by a RWMutex (the lazy path was already mutating repositories from a handler); concurrent duplicate initialization closes the loser and reuses the canonical instance. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g through after one tick Pre-existing: the single select meant a repo not Loaded() within the first 1s tick was served unloaded (nil data) without registration. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…order (#1480) Follow-up to the config-keeper circuit breaker (#1476), from today's prod field experience. Linear: `JITSU-182`. ## Why Within hours of the breaker going live, #1478's deploy reordered JSON keys in every export row (zod `parse` emits schema keys first). Content was byte-for-byte identical after key normalization — but the breaker hashes raw row bytes, so it read 100% mass change and tripped fleet-wide on both replicas (correctly per its design, but the "serializer changes are rare" trade-off documented in the code did not survive day one). The breaker is currently disabled in prod pending this fix — all repos must survive field reorders. ## What - `hashRows` now hashes each row **canonically**: object keys sorted, ',' separator + length-prefixed keys (uniquely decodable — an encoding-collision construction from review is closed), type-tagged values, numbers by literal via `json.Number` (no float64 precision loss), decoder-normalized string escapes. - Key-order changes at any nesting depth are invisible to the breaker; real value changes still trip. Number-literal formatting changes (`1` vs `1.0`) still trip by design — single stable producer, and that failure mode is a confirmation, not a miss. - **No baseline migration needed**: seeding re-hashes the cached payload with the new function, so deploying this cannot itself trip — even onto a payload that byte-changed since the cache was written. - The whole-payload `lastRejected` short-circuit deliberately stays byte-based (it only suppresses re-parsing of a byte-identical rejected generation). Tests: reorder-immunity end-to-end (incl. clearing a held state with a reordered good payload), collision-resistance of the encoding (string/number/boundary/precision cases), escape normalization, literal sensitivity. 25 config-keeper tests green. After deploy, re-enable with `CFGKPR_BREAKER_ENABLED=true` in jitsu-cloud-infra. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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