Repository navigation
fix(rate-limit): never start the work of a caller aborted before dispatch - #16566
Merged
diegosouzapw merged 1 commit intoOct 16, 2026
Conversation
…atch withRateLimit() checks signal.aborted once on entry and then awaits awaitProviderDefaultSlot(). An abort that lands during that await is handled by onAbort(), which calls abandonQueuedJob() and rejects the caller with AbortError. With limiter capacity free, though, Bottleneck dispatches the job inside its submit lock, so the job has already left QUEUED when cancelQueuedJob() gets that lock and the removal is a no-op. wrappedFn then called fn with the already-aborted signal. Work that honors an already-aborted signal ends at once. Work that only listens for a later "abort" event keeps the limiter slot, and Bottleneck's ref'd expiration timer, until executionMaxWaitMs (10 min by default) expires the job. The last test in tests/unit/rate-limit-manager.test.ts does exactly that, so without --test-force-exit the file passed in about 30 s and then kept the process alive for about 10 more minutes. wrappedFn now rejects with the abort reason before calling fn, the same way it already handled a queue-wait timeout. The caller never sees that rejection: the race has already settled with the AbortError. Tests: a new case aborts while withRateLimit awaits its provider slot and asserts that fn never runs and that the limiter drains. A drained limiter holds no executing job, so no expiration timer is left to keep the process alive.
diegosouzapw
merged commit Oct 16, 2026
b3807a0
into
cheaper-inference:release/v3.8.52
30 of 51 checks passed
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.
Summary
withRateLimit()could still run the scheduled work of a caller it had already rejected withAbortError. The abort lands whilewithRateLimitawaitsawaitProviderDefaultSlot().onAbort()then tries to pull the job from the queue, but with limiter capacity free Bottleneckhas already dispatched it, so
fnruns with an already-aborted signal. This PR adds the missingcheck to
wrappedFn, next to the existing queue-timeout check.The visible symptom is in the test suite.
tests/unit/rate-limit-manager.test.tspasses in about30 s and then keeps the Node process alive for about 10 more minutes. That happens when the file is
run the way AGENTS.md documents a focused run (
node --import tsx/esm --test <file>, without--test-force-exit).Symptom
Base
9ecddd15a1, file run on its own with--test-concurrency=1:--test-force-exittimeoutkilled it at 195 s--test-force-exitAn
async_hookspreload dumped the live, ref'd resources right after the rootafter()hook. Itfound one
Timeoutof 600000 ms, created in Bottleneck's_run()(throughpatchedRuninopen-sse/services/bottleneckPatch.ts). That is theexpirationtimer,executionMaxWaitMs(10 min by default), of a job that never finished.
Root cause
withRateLimit()checkssignal.abortedonce on entry, then awaitsawaitProviderDefaultSlot().DOMException reason", calls
controller.abort()during that await.limiter.schedule()runs.onAbort()then runs at once, because the signalis already aborted: it calls
abandonQueuedJob()and rejects the caller.cancelQueuedJob()(open-sse/services/rateLimitManager/queuedJobCancel.ts) waits forBottleneck's
_submitLockand removes the job only while it is stillQUEUED. But_addToQueue()runs_drainAll()inside that same lock. With capacity free, the job hasalready been dispatched when the cancel gets the lock, so the cancel does nothing.
wrappedFnonly checkedqueueTimedOut, so it calledfnwith the aborted signal. The test'sfnregisters anabortlistener on a signal that has already fired, so it never settles. Thejob stays
EXECUTING, and Bottleneck's ref'd expiration timer keeps the process alive until thetimer fires 10 minutes later.
In production the same path starts work for a caller that has already given up:
executeProviderRequest.tsmarks the requestrate_limit_slot_acquiredand enters the executor;modelTestRunner.tsenters the internal chat or responses pipeline.Both get an aborted signal. Work that honors an already-aborted signal ends at once. Work that only
waits for a future
abortevent holds the limiter slot untilexecutionMaxWaitMsexpires the job.Fix
One line in
wrappedFn:It mirrors the existing
queueTimedOutcheck. The rejection never reaches the caller:scheduled.catch(() => {})swallows it;onAbort()rejectsabortPromiseas soon as thesignal fires, before Bottleneck can run
wrappedFn.The comment above the race now explains why a job can already be past
QUEUEDat that point.Tests
aborts while
withRateLimitawaits its provider slot, then asserts that:AbortError;RECEIVED + QUEUED + RUNNING + EXECUTING === 0);fnwas never called.The drain assertion is the symptom check, scoped to the limiter: a drained limiter holds no
executing job, so no expiration timer is left to keep the process alive. There is no process-wide
check such as
process.getActiveResourcesInfo()in the rootafter()hook. Undertest:unit:fast(--test-isolation=none) all files share one process, so that check would alsosee the timers of every other file.
Validation
Run on GitHub Actions (ubuntu-24.04, Node 24.21.0 from
.nvmrc) as a focused run,node --import tsx/esm --test tests/unit/rate-limit-manager.test.ts, without--test-force-exit. It ran twice: once with the branch, and once withopen-sse/services/rateLimitManager.tsrestored to9ecddd15a1while the test file stays.rateLimitManager.tsfrom9ecddd15a1fnran once (1 !== 0)On the base, the new test fails about 5 s into the run. The remaining 600 s is the symptom: the
process waits for the 600000 ms expiration timer of the last test's job.
Earlier local runs used the same source change and an earlier form of the new test:
23 s.
The machine was under heavy load, and the timing-sensitive watchdog test "preserves a real
Bottleneck minTime wait" failed. It also fails on the base under the same load, and it does not
involve an aborted signal.
Static checks:
tscon the test file: only the 4 existing TS2503 errors (Cannot find namespace 'Bottleneck'),none from this change.
npm run typecheck:coredoes not include these two files.check:changelog-integrityagainstrelease/v3.8.52: OK.check:file-size:rateLimitManager.tsis 1313 lines (frozen at 1329);(
open-sse/services/combo/executeTargetAttempt.ts: 1288 > 1284).Notes
(
test:unit,test:unit:serial,test:coverage:runner) already passes--test-force-exit,which ends this file's process as soon as its tests are done. The hang only shows up in a focused
run without that flag.
clean tree (checked against its head
8931826c82), withrateLimitManager.tsat 1320 lines.