Repository navigation
🐛 fix(sink): keep one inventory's ownership conflicts out of the shared breaker - #532
Merged
Merged
Conversation
Terminal export errors trip the shared per-sink circuit breaker, so one misconfigured inventory opens it for every inventory on the sink. Spec set for excluding them (gobreaker IsExcluded), with test-first tasks.
Test isolation for the process-wide breaker registry, a constructor-only timeout seam, the before/at/after-breaker classification boundary, Forbidden and inferred-Terminal controls, half-open state assertions and a concurrent-trial case, and a corrected flake mechanism.
Spec-set review round 2 showed that Git auth and protected-branch rejections reach the breaker already Terminal and that Terminal exports repeat on every event, so excluding all Terminal results would drop load shedding for sink-wide failures. Re-decided: mark ownership rejections as inventory-scoped and exclude only those.
Verifies the spec set's premises on base: breaker settings, gobreaker v2.4.0 exclusion semantics, the three zones, Git sink-wide Terminal at the breaker, re-export on the next reconcile. Classifies every Terminal raised inside an export plan (25 invalidPrune call sites plus backend classifiers) and every ResetBreakersForTest caller, names the SCB-1e fixture, and measures the controller race flake on base (2 of 10 runs red).
An ownership conflict is about one inventory, yet it counted toward the per-sink circuit breaker, so five rejections of one inventory blocked every other inventory on the same sink for 30 s. Add an InventoryScoped marker in internal/errors (Terminal, text and chain unchanged), route the five inventory-scoped prune sites through ownershipConflict, and set IsExcluded on the breaker. Sink-wide failures (Git auth, shared prune metadata) still count. Extract newBreaker(key, timeout) so half-open tests can use a 1 s breaker. Tasks 1.1 + 1.2 of openspec change sink-breaker-terminal-isolation.
Controller tests shared the fixed sink name postgres-demo, so parallel siblings and -count reruns fed one process-wide circuit breaker key and could open it for a test that expects an export (3 of 10 race runs red on the previous commit). Each test now derives a per-invocation sink name from uniqueSinkName and builds its expected status name from it. After the change the race command shows no breaker failure in ten runs; one run failed on the unrelated debounce-metric delta test, so task 2.2 stays open on its ten-green clause pending a decision.
TestClusterInventory_DebouncedExportIncrementsMetric read the process-global kollect_export_debounced_total counter as a delta while running in parallel, so a concurrent debounced cluster export made the delta 2 (about 1 in 10 race runs). Run it non-parallel, as terminalCleanupIncrementsMetric already does. With the per-invocation sink names, ten race -count=3 runs of the controller package are green.
The troubleshooting row said the breaker opens on 5 transient failures and self-closes after the timeout. It counts every backend write failure except an inventory-scoped one (a Git ownership conflict), and after 30 s admits one trial that closes, re-opens or stays half-open. ADR-0602 gets a dated note; the decision is unchanged.
Branch review round 1 found that only one ownershipConflict raise site was pinned by a test. A table in internal/sink/git now drives every routed site through the function that raises it, plus the go-git engine through ClassifyExportError; each site reverted to invalidPrune fails its row. The InventoryScoped doc comment now says it always yields Terminal, pinned by a test, and T0.1's cleanup statement is narrowed: relational cleanup goes through RunExportEnvelope.
Round 2 approve/clean, per-commit gate green on all commits, and the troubleshooting wording for the breaker counting interval.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
konih
added a commit
that referenced
this pull request
Nov 5, 2026
Post-merge evidence for #532 and the living spec sink-circuit-breaker (SCB-1..SCB-3).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
shared per-sink circuit breaker. Every other export failure still counts, including sink-wide
Terminal failures such as a revoked Git token or a protected branch.
healthy inventory on the same sink for 30 s. The same shared breaker also made four reconciler
tests flaky under
-race -count=3.sheds load for every failure that is not marked, and a site nobody marked keeps today's
behaviour.
internal/sink/circuit_breaker.goand the marked sites ininternal/sink/git/prune_owned.goandprune_target.go.What changes
kollecterrors.InventoryScoped(err)makes an error Terminal, keeps its text andchain, and marks it with
ErrInventoryScoped.IsInventoryScopedtests for it.checks (7 sites) go through a new
ownershipConflicthelper. Rejections caused by shared state(prune metadata, repository contents) stay unmarked and keep counting.
IsExcluded: kollecterrors.IsInventoryScoped: a marked result counts neither as afailure nor as a success, does not reset the failure streak, and on a half-open trial frees the
slot without closing or re-opening the breaker. Settings, key, threshold and timeout are
unchanged;
newBreaker(key, timeout)is extracted for tests.real-Git-backend boundary test, a per-site table that fails if any marked site loses its
marker, a go-git engine case. Reconciler tests that shared the
postgres-demobreaker now usea per-invocation sink name; one parallel test that read a global metric delta is now serial.
Risk
shedding load for that fault. Each of the 7 sites was classified in the probe and checked by
two reviewers; ambiguous sites were left unmarked.
trial at a time). Each excluded trial frees the slot, so other inventories are not locked out.
Files to check first
internal/sink/circuit_breaker.gointernal/errors/errors.go(InventoryScoped)internal/sink/git/prune_owned.go,internal/sink/git/prune_target.gointernal/sink/circuit_breaker_test.go,internal/sink/git/prune_scoped_marker_test.goReview commit by commit; each commit builds and passes the packages it touches.
Testing
go test -race -count=3 ./internal/errors/ ./internal/sink/...andgo test -race -count=3 -skip TestControllers ./internal/controller/(ten consecutive green runsof the latter; 3 of 10 failed on the base before the isolation).
IsSuccessful, excluding every Terminal,excluding every error, and reverting each marked site one at a time are each caught by a named
test.
task verify,task lint,task spec:validate, scrub and gitleaks green on the head.Release note (fixes): a single inventory's Git ownership conflict no longer opens the sink circuit
breaker for the other inventories on that sink.
Optional: how this was decided
The first design excluded every Terminal error. The spec-set review showed that Git auth and
protected-branch rejections reach the breaker already Terminal, and that inventories re-export on
every resource event, so that design would have stopped the breaker shedding load for a revoked
token. It was re-decided to mark only inventory-scoped results. OpenSpec change
openspec/changes/sink-breaker-terminal-isolation/holds the spec, the per-task evidence and thereview records.