Skip to content

fix(async/unstable): evaluate circuit breaker failure rate on success - #7308

Open
tomas-zijdemans wants to merge 2 commits into
denoland:mainfrom
tomas-zijdemans:fix-cb-evaluate-on-success
Open

tomas-zijdemans wants to merge 2 commits into
denoland:mainfrom
tomas-zijdemans:fix-cb-evaluate-on-success

Conversation

@tomas-zijdemans

@tomas-zijdemans tomas-zijdemans commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

A success in closed state returned before the failure rate was checked. When that success was the request that lifted the window to minimumThroughput, the circuit stayed closed even though the rate already qualified. With minimumThroughput: 3 and failureRateThreshold: 0.5, failure, failure, success left the circuit closed at a 67% failure rate. The docs say it opens.

Closed-state successes now run the same rate check as failures. Both handlers share #exceedsFailureRate() for the check and #open() for the transition, so they can't drift apart.

#handleFailure now fires onFailure before deciding whether to open, which keeps the callback order at onFailure, onStateChange, onOpen. Two visible effects:

  • On the failure that trips the circuit, breaker.state inside onFailure reads the state before the transition, not "open".
  • The from in onStateChange is the circuit's current state, not the state captured at request start. These only differ for stale requests. A half-open request that fails after the circuit closed now reports closed -> open.

Tests cover a success that trips the circuit, a success that keeps the rate below the threshold, and the callback order on a tripping failure.

I used Claude Code to help investigate and write this change.

@github-actions github-actions Bot added the async label Sep 6, 2026
@codecov

codecov Bot commented Sep 6, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.04%. Comparing base (2958335) to head (d7e7564).

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #7308    +/-   ##
========================================
  Coverage   95.04%   95.04%            
========================================
  Files         619      618     -1     
  Lines       52012    51760   -252     
  Branches     9450     9399    -51     
========================================
- Hits        49433    49195   -238     
+ Misses       2031     2022     -9     
+ Partials      548      543     -5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@bartlomieju

Copy link
Copy Markdown
Member

Thanks, the fix looks correct and matches Resilience4j's behaviour of checking thresholds on every recorded outcome.

This conflicts with #7306 (in #handleSuccess) and #7307 (in the tests), which will land first. Could you rebase once they're in? #handleSuccess will need the generation parameter and guard from #7306, and you should keep both groups of tests.

Optional nit: the closed → open block here repeats #handleFailure. A small #open() helper would keep the two in sync.

@tomas-zijdemans
tomas-zijdemans force-pushed the fix-cb-evaluate-on-success branch from 0b452d0 to 80072c4 Compare September 24, 2026 08:43
@tomas-zijdemans

Copy link
Copy Markdown
Contributor Author

Rebased onto main now that #7306 and #7307 are in. #handleSuccess takes generation and keeps the stale half-open guard, and both groups of tests are there.

I took the nit. #open() now does the state write plus onStateChange and onOpen, and both handlers call it. To keep the callbacks in the order they had (onFailure, then onStateChange, then onOpen), #handleFailure fires onFailure first and decides whether to open afterwards. That has two visible effects:

  • On the failure that trips the circuit, breaker.state read inside onFailure is now the state before the transition, not "open".
  • The from in onStateChange is the circuit's current state, not the state captured when the request started. These only differ for a stale request. A half-open request that fails after the circuit already closed now reports closed -> open instead of half_open -> open.

The second one corrects a mislabel. If you'd rather keep "open" visible inside onFailure, I can split the state write out of the helper. There's a new test that pins the callback order on a tripping failure.

I left forceOpen() alone because it also resets openedAt when the circuit is already open, which the helper doesn't do.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants