Skip to content

fix: release half-open trial slots after breaker transitions - #1396

Open
Bashman10 wants to merge 1 commit into
CalloraOrg:mainfrom
Bashman10:security/issue-1270-release-half-open-trial-slots-after-breaker
Open

Bashman10 wants to merge 1 commit into
CalloraOrg:mainfrom
Bashman10:security/issue-1270-release-half-open-trial-slots-after-breaker

Conversation

@Bashman10

Copy link
Copy Markdown

Overview

This PR fixes a state-leak bug in CircuitBreaker.execute where the activeTrials entry for a breaker was only removed in finally when metrics.state was still HALF_OPEN. Once a trial call closed or re-opened the breaker, the key was left behind, so the next HALF_OPEN window rejected every call with "Only one trial call allowed" — permanently wedging the breaker until restart.

The fix tracks slot acquisition in a local flag and always releases the slot in finally when this invocation actually acquired it. A regression test cycles the breaker through two full OPEN → HALF_OPEN → CLOSED recoveries under fake timers.

Related Issue

Changes

🔧 Circuit breaker fix

  • [MODIFY] src/lib/circuitBreaker.ts
    • In execute, introduce a local acquiredTrialSlot flag set only when this invocation successfully adds breakerKey to activeTrials in the HALF_OPEN branch.
    • In finally, delete breakerKey from activeTrials whenever acquiredTrialSlot is true, regardless of the resulting metrics.state (previously gated on state === HALF_OPEN).
    • Preserves the existing single-trial guard: concurrent callers that fail to acquire the slot still throw "Only one trial call allowed" and never touch activeTrials.

🧪 Regression tests

  • [MODIFY] src/lib/circuitBreaker.test.ts
    • Adds a fake-timer regression that drives the breaker through two consecutive recovery cycles (OPEN → HALF_OPEN → CLOSED → OPEN → HALF_OPEN → CLOSED) and asserts both cycles succeed.
    • Asserts activeTrials is empty after each execute call completes (success, failure, and rejection paths).
    • Asserts concurrent calls during HALF_OPEN still allow exactly one trial and reject the rest.

Verification Results

npm test -- src/lib/circuitBreaker.test.ts src/routes/gatewayRoutes.circuitBreaker.test.ts
✅ circuitBreaker.test.ts passed (including the new two-cycle regression)
✅ gatewayRoutes.circuitBreaker.test.ts passed
Acceptance Criteria Status
A breaker completes two full recovery cycles in a fake-timer test ✅ Regression test drives OPEN → HALF_OPEN → CLOSED twice under fake timers
activeTrials is empty after any execute call completes ✅ Asserted after success, failure, and rejected-trial paths
Concurrent calls in HALF_OPEN still allow only one trial ✅ Existing guard retained; concurrency assertion added
src/lib/circuitBreaker.test.ts contains the regression ✅ New test added in that file

Security and Failure-Mode Handling

  • No safeguard weakening: the HALF_OPEN single-trial admission check is unchanged; only the release path is corrected. Callers that do not acquire the slot still fail fast.
  • Failure-mode safety: releasing the slot in finally on the acquiring invocation guarantees the slot is freed whether the trial succeeds (→ CLOSED), fails (→ OPEN), or throws unexpectedly. This prevents a transient upstream outage from becoming permanent.
  • No new external surface: change is confined to internal bookkeeping in circuitBreaker.ts; no API, config, or dependency changes.

Compatibility

No public API or behavioral contract changes beyond the bug fix. Existing callers see identical semantics for the single-trial guard; the only observable difference is that breakers can now recover across multiple cycles instead of latching after the first.

Closes #1270

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.

Release half-open trial slots after breaker transitions

1 participant