fix(limiter): release permit when inner factory rejects the call - #35
Merged
Conversation
Both limiter variants acquire a permit before delegating inward, so an error from the inner factory has to release it again: no Observer is returned in that case, so nothing else ever would. Leaking here is terminal. The limiter sits outside the circuit's state check, so every call dropped while the circuit is open permanently shrinks the effective limit, until all calls are rejected with ErrConcurrencyLimitReached and none can reach the circuit to ever close it again. Document the obligation on the ObserverFactory contract, since it applies to any middleware that claims resources before delegating. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
costela
requested review from
totallyunknown and
yvz5
and
a lite review from Copilot
August 4, 2026 08:16
costela
marked this pull request as ready for review
August 4, 2026 08:17
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a permit leak in the ConcurrencyLimiter middleware when the wrapped ObserverFactory rejects a call (returns an error). Without releasing the already-acquired permit on that error path, the limiter can permanently drain its capacity—especially when placed outside a circuit breaker’s state check—eventually preventing any calls from reaching the circuit so it can recover.
Changes:
- Release the semaphore permit when the inner
ObserverFactoryreturns an error after the limiter has acquired a permit. - Add a regression test covering both blocking and non-blocking limiter variants for the inner-error path.
- Document the
ObserverFactorycontract obligation to clean up any claimed resources before returning an error.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| limiter.go | Releases acquired permits when the inner factory errors, preventing terminal capacity leaks. |
| limiter_test.go | Adds a regression test ensuring permits are released when the inner factory rejects calls. |
| hoglet.go | Documents the ObserverFactory error-path responsibility to release claimed resources. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
totallyunknown
approved these changes
Aug 4, 2026
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.
Both
ConcurrencyLimitervariants acquire a permit before delegating inward, so an error from the inner factory (e.g.ErrCircuitOpen) has to release it again — noObserveris returned in that case, so nothing else ever would.Leaking there is terminal: the limiter sits outside the circuit's state check, so every call dropped while the circuit is open permanently shrinks the effective limit, until all calls are rejected with
ErrConcurrencyLimitReachedand none can reach the circuit to ever close it again.Also documents the obligation on the
ObserverFactorycontract, since it applies to any middleware claiming resources before delegating.🤖 Generated with Claude Code