Skip to content

fix(quota): correct breaker halfOpen comment and add concurrent test - #898

Open
vishalmore90 wants to merge 1 commit into
openkruise:masterfrom
vishalmore90:fix-issue-897
Open

fix(quota): correct breaker halfOpen comment and add concurrent test#898
vishalmore90 wants to merge 1 commit into
openkruise:masterfrom
vishalmore90:fix-issue-897

Conversation

@vishalmore90

Copy link
Copy Markdown
Contributor

Ⅰ. Describe what this PR does

This PR corrects a factually wrong comment in BreakerBackend.beforeCall() that misrepresents the state machine, and introduces a missing concurrent unit test to cover this specific race condition.

Context:
In pkg/sandbox-manager/quota/breaker.go line 118, the comment described the halfOpen == true state as: // 3. the probe acquire failed, breaker stays open.

This is incorrect. When beforeCall() executes the halfOpen == true branch, it means a probe acquire is currently in-flight in another goroutine, not that it has failed. The actual failure path is handled asynchronously inside afterCall(), which re-opens the breaker.

Changes:

  1. Comment Fix: Updated the comment in breaker.go to accurately describe the fail-fast thundering herd protection.
  2. Missing Test Added: Added a new test concurrent caller during in-flight probe gets ErrBackendUnavailable without touching inner to breaker_test.go. This test uses a new blockingBackend to intentionally pause the probe mid-flight and ensure that a concurrent caller correctly fails-fast with ErrBackendUnavailable without incrementing the inner backend call count.

Ⅱ. Does this pull request fix one issue?

fixes #897

Ⅲ. Describe how to verify it

  1. Review the logic trace in breaker.go to confirm the comment change aligns with the afterCall() state transitions.
  2. Run the newly added test suite to verify the race condition is properly protected against:
    go test -v -run TestBreaker ./pkg/sandbox-manager/quota

Signed-off-by: vishal <httpsvishal07@gmail.com>
@kruise-bot
kruise-bot requested review from AiRanthem and zmberg August 28, 2026 09:54
@kruise-bot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign airanthem for approval by writing /assign @airanthem in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@codecov

codecov Bot commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.40%. Comparing base (825de1d) to head (970839c).
⚠️ Report is 21 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #898      +/-   ##
==========================================
+ Coverage   82.31%   82.40%   +0.08%     
==========================================
  Files         244      254      +10     
  Lines       19782    21457    +1675     
==========================================
+ Hits        16284    17681    +1397     
- Misses       2888     3079     +191     
- Partials      610      697      +87     
Flag Coverage Δ
unittests 82.40% <ø> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

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.

[BUG] BreakerBackend.beforeCall() misidentifies the halfOpen state — comment says \"probe failed\" when probe is still in-flight

2 participants