Fix Semaphore.withPermits leaking permits when interrupted - #6910
Merged
tim-smart merged 2 commits intoAug 3, 2026
Merged
Conversation
🦋 Changeset detectedLatest commit: b47fd15 The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Semaphore.withPermitsfix: Moved the permit-increment and finalizer-installation inside theuninterruptibleMaskcallback, making the acquire-to-release span atomic with respect to interruption. Only the waiter queue stays interruptible viarestore(wait).- Regression tests: Two sweep tests that inject
interruptUnsafeat every runtime operation of both an uncontended and a contended (queued) acquisition, then assert no permits are leaked and the guarded effect runs.
DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) ｜ 𝕏
Contributor
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Extracted
waitForPermitshelper: The waiter callback/observer pattern was duplicated betweentakeandwithPermits. Extracting it into a standalonewaitForPermits(self, n, effect)function deduplicates the logic and avoids per-call-site closure overhead. - Renamed
updateTakenUnsafe→releaseUnsafe: The method now takes a directncount instead of an update function, matching its sole usage pattern (this.taken -= n). Removed the deadupdateTakenEffect wrapper sincereleaseandreleaseAllnow callreleaseUnsafedirectly viacore.withFiber. - Replaced manual iterator with
for...ofinreleaseUnsafe's waiter notification loop. Thebreakonthis.free <= 0preserves the early-exit behavior. Set iteration handles deletion of the current element correctly per the spec.
DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) ｜ 𝕏
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
2 participants
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.
Fix Semaphore.withPermits leaking permits when interrupted during acquisition
Type
Description
Semaphore.withPermits(n)could permanently lose its permits when the acquiring fiber was interrupted between taking them and installing their release. The permits were then held by nobody and freed by nothing, so every later acquirer of them blocked forever, with no failure and no log.Summary
withPermitswithout having them handed over, so a resumed waiter takes them itself on the retryeffectRoot cause
Semaphore.withPermitswrapped its acquisition inrestore:this.taken += ntherefore ran inside an interruptible region, and theonExitPrimitivethat installs the release was pushed by theflatMapcontinuation one fiber operation later. Between those two operations the count had been raised and no finalizer existed. A fiber interrupted there exited with the permits held by nobody, and nothing lowers the count afterwards:updateTakenUnsafeis only reached from a finalizer that was installed, andtakenis a plain number with no ownership record, so a permit with no holder is indistinguishable from one in use.The window is reached two ways.
interruptUnsafesets_deferredInterruptwhen the fiber is running, and the run loop converts it to afailCauseat the top of the next iteration. Alternatively the scheduler parks the fiber at that operation on itsMaxOpsBeforeYieldbudget and an ordinaryFiber.interruptarrives while it is parked.The contended path reached the same window: a queued waiter was resumed with
resume(take)and re-rantake, performing the same increment in the same restored region before the finalizer existed. #6081 moved the increment out of the observer and into that retry, which fixed a different leak but left this one, since the retry still commits insiderestore.acquireUseReleaseandacquireReleasedo not have this problem because they keep acquire uninterruptible and restore only theuse, making the acquire-to-install span atomic with respect to interruption.withPermitscould not copy that directly, because a fiber queued for permits has to stay interruptible.withPermitsnow owns its acquisition instead of delegating totake. It waits on the samewaitersqueue, but its wait resumes withvoidrather than with a take, so it hands nothing over and only signals that the caller should re-check. Only that wait is restored; the increment and theonExitPrimitiveboth run withinterruptible === false, whereinterruptUnsafecan only record the cause. A fiber parked at that boundary resumes, installs the finalizer, and receives the recorded cause atrestore(self), so the guarded effect never runs and the permits are released. On the contended path the resumed waiter re-enters that same uninterruptible commit, so the handover window stops existing rather than being guarded.This duplicates the parking block between
takeandwithPermits. The two are no longer the same operation —takehands the retry over on resume andwithPermitsdeliberately hands nothing over — so sharing them would mean parameterising the difference that is the fix.Semaphore.takeis therefore untouched.Impact
No API, type, or semantic changes, and the diff is confined to
SemaphoreImpl.withPermits. Waiting for permits stays interruptible, so queues of waiters remain killable. Spurious wakeups still re-park, exactly as before, since a resumed waiter re-checksfree.Semaphore.take,takeIfAvailable,release, andresizeare unchanged.withPermitsIfAvailablewas already correct — it commits inside the mask callback — and is unchanged.Semaphore.takeused on its own still has the window it always had: after the count is raised the fiber is interruptible again before the caller can install any release. That is inherent to a baretakeand is not addressed here;withPermitsis the bracketed API.Reproduction
The leak was reproduced deterministically against
mainatb75884413by sweeping the interrupt across every operation of the acquiring fiber.Uncontended, using the stock
MixedSchedulerwith onlyMaxOpsBeforeYieldvaried, so the fiber parks on its ordinary cooperative yield and is then interrupted by a plainFiber.interruptfrom another fiber:Contended, with the waiter queued and handed the permit by a release before being interrupted:
taken=1with the guarded effect never having run and the acquiring fiber already dead is the leak. Neither reproduction needs a scheduler that interrupts re-entrantly; both use an ordinary yield followed by an ordinary interrupt.Validation
pnpm test --run packages/effect/test/Semaphore.test.ts(20 passed)pnpm test --run --project effect(244 files, 7650 passed, 3 skipped)pnpm checkpnpm lintpnpm exec changeset status --since origin/mainpackages/effect/src/Semaphore.ts, and pass with it restoredThe regression tests derive their sweep bound from the operation count of an uninterrupted acquisition rather than hard-coding it, and assert that the sweep reached the operations that hold permits, so added runtime operations widen the sweep instead of escaping it.
Related