Skip to content

peer: hard-bound the breaker map under failed-address churn (fixes #54) - #58

Merged
Coldwings merged 1 commit into
mainfrom
fix/issue-54-breaker-hard-cap
Aug 26, 2026
Merged

peer: hard-bound the breaker map under failed-address churn (fixes #54)#58
Coldwings merged 1 commit into
mainfrom
fix/issue-54-breaker-hard-cap

Conversation

@Coldwings

Copy link
Copy Markdown
Collaborator

What

The breaker peer-map cap (maxTrackedPeers = 4096, added for #9) was not a hard bound: the sweep only removes information-free entries (closed, zero failures, no probes in flight), so a stream of distinct addresses that each carry failure state grew peers past 4096 without bound, contradicting the documented "peer map is bounded" contract. This PR makes the cap a true hard bound via least-recently-active eviction when the sweep frees nothing.

Triage evidence (issue verified authentic)

Verified against the exact commit the issue names (84aac4e, current main at review time):

  • git show 84aac4e:internal/peer/breaker.goentryLocked invokes sweepLocked and then unconditionally inserts; sweepLocked deletes an entry only when state == BreakerClosed && failures == 0 && inFlight == 0. A churned address with any failure/open/half-open state is never evicted. Exactly as the issue describes.
  • The existing regression TestBreakerSweepsCleanEntries churns only clean addresses, so it verified the information-free sweep, not the bound under failed-address churn — also as the issue states.
  • docs/peer.md claimed "The peer map is bounded" — false under failed-address churn. Doc/code contract mismatch, not a deliberate minimal-design stance (nothing in the trust model requires unbounded retention of departed-address health state).

Classification: implementation (residual design gap after #9; doc/code contract mismatch fixed on the code side).

Fixes

  • breakerEntry gains a lastActive stamp, written only by request paths (Allow — including rejected attempts — and the three Record* reports). Read-only observers (State/Healthy/OpenCount) never stamp, so a metrics scrape cannot launder every entry into "recently used" (this matters: OpenCount iterates the whole map per scrape).
  • entryLocked: at the cap, sweep first; if the sweep freed nothing, evictStalestLocked drops the entry with the oldest lastActive — by construction the address no request has touched for the longest time, i.e. almost always a departed node. If it is genuinely sick and still in rotation, the next request re-creates the entry and re-accumulates the failures, so breaker behavior for active peers is unchanged.
  • Eviction is O(n) but runs only once per insertion at a full map of dirty entries — never on the hot path.
  • No exported API changes; all call sites (Client, engine wiring, metrics) unaffected.
  • docs/peer.md §3.5 rewritten to document the hard bound, the stamping rule, and why evicting a dirty entry is safe; test-inventory table updated.

Regression tests

Both fail on the pre-fix code (verified by restoring origin/main's breaker.go under the new tests: map reached 4196+ entries, FAIL in all four subtests):

  • TestBreakerCapHardUnderFailedChurn (table-driven, three flavors of dirty entries: below-threshold failures / open circuits / half-open probes in flight): churns maxTrackedPeers + 100 distinct dirty addresses, asserts len(peers) <= maxTrackedPeers, and asserts the most-recently-touched address keeps correct breaker behavior (still open / still half-open / still closed-below-threshold).
  • TestBreakerEvictionDropsStalest: fills the map to the cap with dirty entries on an injected clock, keeps one sick peer actively routed-to (freshest stamp), churns 100 more dirty addresses, then asserts: bound holds, the stalest entry was the first victim, and the active peer's open circuit survived.

Verification (actual commands + results, Go 1.22.12, -count=1)

gofmt -l internal/peer/        → clean
go vet ./...                   → clean
go test ./... -race -count=1   → ok (all 13 packages, incl. internal/peer 2.247s)
go test ./... -cover -count=1  → ok all; internal/peer 87.7% coverage

Pre-fix failure proof (old breaker.go + new tests):

--- FAIL: TestBreakerCapHardUnderFailedChurn/failures_below_threshold
    peers map = 4196 entries after churning 4196 dirty addresses, want <= 4096
--- FAIL: .../open_circuits, .../half-open_probes_in_flight  (same)
--- FAIL: TestBreakerEvictionDropsStalest
    peers map = 4197 entries, want <= 4096

Fixes #54

The #9 sweep only removes information-free entries, so a stream of
distinct addresses that each carry failure state (below-threshold
failures, open circuits, half-open probes in flight) grew the peer map
past maxTrackedPeers without bound, contradicting the documented
"bounded" contract.

Make the cap a hard bound: when the sweep frees nothing, evict the
least-recently-active entry. Activity is stamped only by request paths
(Allow and the Record* reports) so read-only observers (State/Healthy/
OpenCount) cannot launder stale entries into recent use. The victim is
by construction the address no request has touched for the longest time
— almost always a departed node; if it is genuinely sick and still in
rotation, the next request re-creates the entry and re-accumulates the
failures, so breaker behavior for active peers is unchanged.

Regression tests churn cap+100 dirty addresses in three flavors and pin
the bound plus victim selection (stalest first, active peer preserved);
both fail on the pre-fix code (map reaches 4196+ entries).

Fixes #54
@Coldwings

Copy link
Copy Markdown
Collaborator Author

@copilot please review this PR. Focus points:

  1. The LRU eviction in evictStalestLocked / entryLocked — is the victim selection correct under all breaker states (closed-with-failures, open, half-open with probes in flight)?
  2. The lastActive stamping rule: stamped in Allow (even on rejection) and the three Record* methods, deliberately NOT in State/Healthy/OpenCount — could any caller pattern make an actively-used entry look stale?
  3. Concurrency: all mutations remain under b.mu; any missed path that creates entries without going through the cap check?
  4. Do the new tests genuinely pin the hard bound (they fail on pre-fix code — verified)?

Please do NOT push commits to this branch; leave review comments only.

Copilot AI commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

@copilot please review this PR. Focus points:

  1. The LRU eviction in evictStalestLocked / entryLocked — is the victim selection correct under all breaker states (closed-with-failures, open, half...

Reviewed at 936470c.

  1. Victim selection looks correct across states. evictStalestLocked chooses strictly by oldest lastActive after sweepLocked fails to free anything, so closed-with-failures, open, and half-open entries are all eligible, and the least recently request-active one is evicted.

  2. The stamping rule is consistent with intent. In this repo’s request path, active peers always go through Allow + Record* (Client.Get/Stream), so truly active entries are refreshed. A peer observed only via State/Healthy can look stale, but that is deliberate observer behavior and prevents metrics/read-only probes from laundering recency.

  3. Concurrency looks sound: all mutations are under b.mu, and all entry creation goes through entryLocked (which enforces the cap path). I don’t see a bypass path that inserts into b.peers without the cap check.

  4. The new tests do pin the hard bound behavior. They cover dirty churn for below-threshold failures, open circuits, and half-open/in-flight probes, plus explicit stalest-victim eviction. I re-ran the two new regressions and they pass on current code.

@Coldwings
Coldwings merged commit e8873b0 into main Aug 26, 2026
8 checks passed
@Coldwings
Coldwings deleted the fix/issue-54-breaker-hard-cap branch August 26, 2026 04:16
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.

[Design Gap] peer: breaker map cap is bypassed by failed-address churn

2 participants