[vitest-pool-workers] fix: reset() now clears ratelimit binding state between tests#14409
Conversation
… between tests Fixes cloudflare#14392. The `reset()` helper from `cloudflare:test` only called `deleteAllDurableObjects()`, leaving ratelimit bucket counts intact across test boundaries. Subsequent tests in the same file could see stale exhaustion state from an earlier test. The fix uses a module-level registry in the miniflare ratelimit extension: each `Ratelimit` instance registers itself on `globalThis.__cfRatelimitInstances__` when created. `reset()` iterates that set and calls the new `Ratelimit.reset()` method, which clears `buckets` and resets `epoch`. The global is only populated when ratelimit bindings are actually configured, so the check is a no-op for workers without ratelimits. A test fixture (`fixtures/vitest-pool-workers-examples/reset`) is updated to verify that exhausting a ratelimit in one test does not bleed into the next.
🦋 Changeset detectedLatest commit: c78da86 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
@cloudflare/autoconfig
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…approach - Add miniflare: patch to changeset — without it the feature silently doesn't work because vitest-pool-workers pins to a miniflare version that lacks the instance registry and reset() method - Refactor the globalThis key from an instances Set to a single reset function reference (__cfRatelimitReset__) to minimise what's exposed on globalThis - The ratelimit extension module is marked `internal: true` in workerd, which prevents dynamic import from reset.ts; globalThis is the only cross-module communication path available within the same Worker isolate
petebacondarwin
left a comment
There was a problem hiding this comment.
Using a global to track instances feels very hacky.
Can we try to find a more elegant solution?
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
If fixing this inside workerd is the best solution then we should do that rather than working around it - we do effectively own both parts of the system. I did briefly look for the rate limiting code in workerd but wasn't sure where it was found. |
…bol.for for ratelimit reset
Symbol.for("cloudflare:miniflare:ratelimit:reset") is collision-safe by
construction and documents the same-isolate semantics: internal extension
modules run inside the calling worker's V8 isolate, so globalThis is
legitimately shared between ratelimit.worker.ts and reset.ts.
Claude-Session: https://claude.ai/code/session_011QMWU4d8zZN3ERqCHrJz5i
…ing instead of globalThis Removes the globalThis[Symbol.for(...)] side-channel between the ratelimit extension module and reset(). Instead, the ratelimit plugin registers a reserved control binding (__MINIFLARE_RATELIMIT_CONTROL__) using the same wrapped-binding mechanism RATE_LIMITER itself already uses, and reset() calls resetAll() on it directly through env.
…ad of globalThis Removes the globalThis[Symbol.for(...)] bridge between the ratelimit extension module and reset(). The Ratelimit class already exposes a public reset() method; reset() now calls it directly on each configured ratelimit binding, using binding names read from the parsed wrangler config (pool/index.ts) via a new __VITEST_POOL_WORKERS_RATELIMIT_BINDING_NAMES module. This avoids walking `env` (which would touch RPC/entrypoint bindings like SELF) and avoids the miniflare wrapped-binding module entirely, so it needs no changes to the ratelimit plugin. Also fixes formatting on the existing changeset file (oxfmt).
c4ca21e to
e2f2246
Compare
|
Pushed an update that removes the global entirely. What changed
The actual bridge moved into
Since Why not the other things I tried
Verification Ran the full |
__VITEST_POOL_WORKERS_RATELIMIT_BINDING_NAMES was a virtual module from the original approach (vitest-pool-workers tracking ratelimit binding names for reset()). The DO-backed redesign removed that mechanism entirely; this tsdown external entry was the one leftover reference.
petebacondarwin
left a comment
There was a problem hiding this comment.
Thanks for this — the Durable-Object-backed redesign is the right call and it's cleanly done. Moving the bucket/epoch state into a MiniflareDurableObject so that deleteAllDurableObjects() (and therefore reset()) tears it down for free is exactly the pattern the other stateful plugins use, and the structure is near-identical to D1/KV (wrapped client + fetcher service inner-binding → per-binding object-entry worker → one DO-hosting service). Keeping option validation in the client isolate so errors still surface synchronously to the caller is a nice touch.
A few things I'd like to see addressed before merge:
1. In-memory DO is missing preventEviction: true
packages/miniflare/src/plugins/ratelimit/index.ts — the closest precedent is the Queues broker, which is also durableObjectStorage: { inMemory: kVoid } and sets preventEviction: true on the namespace (see packages/miniflare/src/plugins/queues/index.ts:97-105). Without it, the rate-limiter DO can be evicted while idle, which silently clears #buckets/#epoch mid-period. Please add preventEviction: true, and confirm it doesn't interfere with deleteAllDurableObjects() clearing state — it shouldn't, since that's a forced disposal, but it's worth verifying given that clearing is the whole point of this PR.
2. DO-hosting service is missing the standard object bindings
Both Queues and KV bind SharedBindings.MAYBE_SERVICE_LOOPBACK → SERVICE_LOOPBACK and spread getMiniflareObjectBindings(unsafeStickyBlobs) into the DO-hosting worker (queues/index.ts:106-111, kv/index.ts:167-177). The new RATELIMIT_ENTRY_SERVICE_PREFIX service has no bindings at all. RateLimiterObject only touches in-memory Maps today so it may work as-is, but the MiniflareDurableObject base machinery can reach for the loopback/blob services on some paths — please confirm they're genuinely unused here, or align with the Queues pattern to be safe.
3. Rate-limiter identity is the binding name, not namespace_id (I'll handle this in a follow-up)
getBindings/getServices key the object-entry service and the DO instance by the binding name. In production — and in the KV/D1 plugins, which key by id — a limiter is identified by namespace_id, so two bindings that share a namespace_id should share state, and two Workers that happen to use the same binding name should not collide. This is a pre-existing divergence that your redesign now makes straightforward to fix. I'm going to take this in a follow-up PR (key by namespace_id, and re-enable the multiworker ratelimits that are currently deleted in MultiworkerRuntimeController), so it does not block this PR — just flagging it here for context.
4. Unrelated changes bundled in
The Windows ECONNRESET retry helper (dispatchFetchWithRetry + the r2.spec.ts switch) and the EPIPE addition to ignoreEconnreset in e2e/dev.test.ts look like separate CI-flake fixes, unrelated to reset()/ratelimit. Could you split them into their own PR (or at least call them out in the description)? Keeps this one focused and easy to revert.
5. Changeset only bumps miniflare
reset() is the user-facing surface and it's re-exported from @cloudflare/vitest-pool-workers, but only miniflare gets a changeset entry. Consider adding a @cloudflare/vitest-pool-workers changeset too, so the fix shows up in the changelog where users of cloudflare:test will actually look.
Minor
- The two new fixture tests are order-dependent (the second assumes the first exhausted the limit and that
reset()ran in between). That's consistent with the existing Counter/KV/R2 cases inreset.test.ts, so I'm fine with it — just noting it. - The DO now takes
limit/periodper request, so the instance is config-agnostic. That's fine while it's keyed per-binding, but it interacts with thenamespace_idchange in (3); I'll account for it in the follow-up.
Overall this is close — happy to approve once 1, 2, 4, and 5 are sorted. (3 is on me.)
|
I just looked into workerd and it seems that |
|
So should i go with 1,2,4 and 5 or wait for it ? |
|
If the epoch is short enough, then perhaps the DO eviction period will be longer and we don't need If that doesn't work out, we will still need to turn on eviction prevention, but then provide a direct API to that Rate Limiter DO that does |
Let's do 2 & 5 now. |
|
Ok , let me have a look at it and i will get back with a fix |
…rdown handler" This reverts commit 220a54d.
… ECONNRESET" This reverts commit 322c480.
…hosting service Every other DO-hosting plugin (queues, kv, d1, r2) binds SharedBindings.MAYBE_SERVICE_LOOPBACK and spreads getMiniflareObjectBindings(unsafeStickyBlobs) into its object-entry service; the ratelimit plugin's RATELIMIT_ENTRY_SERVICE_PREFIX service had no bindings at all. Align it with the established pattern in case MiniflareDurableObject's base machinery reaches for the loopback/blob services on some path. Also add a changeset entry for @cloudflare/vitest-pool-workers, since reset() is the user-facing surface re-exported from that package. Addresses review items 2 and 5 from petebacondarwin on cloudflare#14409.
|
Addressed 2, 4, 5 — leaving 1 (preventEviction/deleteAllDurableObjects interaction) and 3 (namespace_id keying, #14537) with you as discussed.
Verified locally: |
|
Codeowners approval required for this PR:
Show detailed file reviewers |
RatelimitConfigSchema's period was optional with no default, so an omitted period reached buildJsonBindings() as undefined, producing an invalid `json: undefined` Worker_Binding instead of a valid JSON string — the client's RatelimitConfig.period is typed as a required number. Wrangler's own config validation already requires period for real wrangler.jsonc configs, but callers using the Miniflare API directly could hit this. Default it in the schema instead. Devin review finding on cloudflare#14409.
|
Eviction of DOs is done after 10 seconds of inactivity. But I don't think that any rate limiter would be expected to rate limit (in local dev) if there are fewer than one request every 10 secs... So I think this is a safe implementation without adding If we get reports of this causing a problem we can also persist the DO state so that if the DO gets evicted it can reload it again afterwards. |
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
|
I'll follow up with my PRs |
|
Sounds good , let me know if you any other issue persist , i will open up new pr to solve it |
Fixes #14392.
Problem
reset()fromcloudflare:testonly calledworkerdUnsafe.deleteAllDurableObjects(). Ratelimit bindings (RATE_LIMITERS) store their bucket counts in aMapon an in-memoryRatelimitinstance. That instance is never restarted between tests, so bucket counts bleed across test boundaries — a test that exhausts a ratelimit causes subsequent tests in the same file to see it as already exhausted.Solution
The miniflare ratelimit extension (
ratelimit.worker.ts) now:reset()method to theRatelimitclass that clearsbucketsand resetsepoch.Set<Ratelimit>and writes it toglobalThis.__cfRatelimitInstances__so the reset helper can reach it without importing an internal module directly.reset.tsin vitest-pool-workers now iterates that set (if present) and callsreset()on each instance afterdeleteAllDurableObjects(). The global is only populated when the ratelimit extension is loaded, so the check is a no-op for workers with no ratelimit bindings.Test
The existing
resetfixture (fixtures/vitest-pool-workers-examples/reset) is extended with aRATE_LIMITERbinding and two new tests:"exhausts ratelimit then resets between tests"— exhausts the 100-request limit in one test"sees reset ratelimit state after reset"— verifies the first call in the next test succeeds againreset()API, no new user-facing API surface