Skip to content

[vitest-pool-workers] fix: reset() now clears ratelimit binding state between tests#14409

Merged
petebacondarwin merged 22 commits into
cloudflare:mainfrom
matingathani:fix/vitest-pool-workers-reset-ratelimit
Jul 6, 2026
Merged

[vitest-pool-workers] fix: reset() now clears ratelimit binding state between tests#14409
petebacondarwin merged 22 commits into
cloudflare:mainfrom
matingathani:fix/vitest-pool-workers-reset-ratelimit

Conversation

@matingathani

@matingathani matingathani commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Fixes #14392.

Problem

reset() from cloudflare:test only called workerdUnsafe.deleteAllDurableObjects(). Ratelimit bindings (RATE_LIMITERS) store their bucket counts in a Map on an in-memory Ratelimit instance. 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:

  1. Adds a reset() method to the Ratelimit class that clears buckets and resets epoch.
  2. Maintains a module-level Set<Ratelimit> and writes it to globalThis.__cfRatelimitInstances__ so the reset helper can reach it without importing an internal module directly.

reset.ts in vitest-pool-workers now iterates that set (if present) and calls reset() on each instance after deleteAllDurableObjects(). 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 reset fixture (fixtures/vitest-pool-workers-examples/reset) is extended with a RATE_LIMITER binding 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 again

  • Tests
    • Tests included/updated
  • Public documentation
    • Documentation not necessary because: internal behaviour fix for existing reset() API, no new user-facing API surface

Open in Devin Review

… 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.
Copilot AI review requested due to automatic review settings June 24, 2026 10:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@changeset-bot

changeset-bot Bot commented Jun 24, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: c78da86

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
miniflare Patch
@cloudflare/vitest-pool-workers Patch
@cloudflare/deploy-helpers Patch
@cloudflare/pages-shared Patch
@cloudflare/vite-plugin Patch
wrangler Patch

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

@github-project-automation github-project-automation Bot moved this to Untriaged in workers-sdk Jun 24, 2026
@workers-devprod workers-devprod requested review from a team and ascorbic and removed request for a team June 24, 2026 10:01
@workers-devprod

Copy link
Copy Markdown
Contributor

Codeowners approval required for this PR:

  • @cloudflare/wrangler
Show detailed file reviewers
  • .changeset/vitest-pool-workers-reset-ratelimit.md: [@cloudflare/wrangler]
  • fixtures/vitest-pool-workers-examples/reset/src/env.d.ts: [@cloudflare/wrangler]
  • fixtures/vitest-pool-workers-examples/reset/test/reset.test.ts: [@cloudflare/wrangler]
  • fixtures/vitest-pool-workers-examples/reset/wrangler.jsonc: [@cloudflare/wrangler]
  • packages/miniflare/src/workers/ratelimit/ratelimit.worker.ts: [@cloudflare/wrangler]
  • packages/vitest-pool-workers/src/worker/reset.ts: [@cloudflare/wrangler]

@pkg-pr-new

pkg-pr-new Bot commented Jun 24, 2026

Copy link
Copy Markdown
@cloudflare/autoconfig

npm i https://pkg.pr.new/@cloudflare/autoconfig@14409

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@14409

@cloudflare/deploy-helpers

npm i https://pkg.pr.new/@cloudflare/deploy-helpers@14409

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@14409

miniflare

npm i https://pkg.pr.new/miniflare@14409

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@14409

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@14409

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@14409

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@14409

@cloudflare/workers-auth

npm i https://pkg.pr.new/@cloudflare/workers-auth@14409

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@14409

@cloudflare/workers-utils

npm i https://pkg.pr.new/@cloudflare/workers-utils@14409

wrangler

npm i https://pkg.pr.new/wrangler@14409

commit: c78da86

devin-ai-integration[bot]

This comment was marked as resolved.

matingathani and others added 2 commits June 25, 2026 11:19
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
devin-ai-integration[bot]

This comment was marked as resolved.

@petebacondarwin petebacondarwin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a global to track instances feels very hacky.
Can we try to find a more elegant solution?

Comment thread fixtures/vitest-pool-workers-examples/reset/src/env.d.ts Outdated
@github-project-automation github-project-automation Bot moved this from Untriaged to In Review in workers-sdk Jun 29, 2026
@workers-devprod

workers-devprod commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Codeowners approval required for this PR:

  • @cloudflare/wrangler
Show detailed file reviewers
  • .changeset/vitest-pool-workers-reset-ratelimit.md: [@cloudflare/wrangler]
  • fixtures/vitest-pool-workers-examples/reset/src/env.d.ts: [@cloudflare/wrangler]
  • fixtures/vitest-pool-workers-examples/reset/test/reset.test.ts: [@cloudflare/wrangler]
  • fixtures/vitest-pool-workers-examples/reset/wrangler.jsonc: [@cloudflare/wrangler]
  • packages/miniflare/src/plugins/ratelimit/index.ts: [@cloudflare/wrangler]
  • packages/miniflare/src/workers/ratelimit/ratelimit-object.worker.ts: [@cloudflare/wrangler]
  • packages/miniflare/src/workers/ratelimit/ratelimit.worker.ts: [@cloudflare/wrangler]
  • packages/miniflare/test/plugins/local-explorer/r2.spec.ts: [@cloudflare/wrangler]
  • packages/miniflare/test/test-shared/miniflare.ts: [@cloudflare/wrangler]
  • packages/wrangler/e2e/dev.test.ts: [@cloudflare/wrangler]

@petebacondarwin petebacondarwin marked this pull request as draft June 29, 2026 11:51
@petebacondarwin

Copy link
Copy Markdown
Contributor

@petebacondarwin The globalThis bridge is needed because the ratelimit extension module is marked internal: true in workerd — it can't be imported from reset.ts. The cleanest alternative I can think of is adding a resetRatelimitBindings() to workerd:unsafe (similar to deleteAllDurableObjects()), but that requires a workerd change. Do you have a specific pattern in mind?

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.

matingathani and others added 4 commits June 30, 2026 16:22
…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).
@matingathani matingathani force-pushed the fix/vitest-pool-workers-reset-ratelimit branch from c4ca21e to e2f2246 Compare July 1, 2026 05:53
@matingathani

Copy link
Copy Markdown
Contributor Author

Pushed an update that removes the global entirely.

What changed

ratelimit.worker.ts no longer touches globalThis at all — the Ratelimit class just keeps its plain reset() method, nothing else. The miniflare ratelimit plugin is untouched.

The actual bridge moved into vitest-pool-workers, which already has everything it needs without reaching into Miniflare internals:

  • pool/index.ts reads the ratelimit binding names directly from the parsed wrangler config (same place RATE_LIMITER: RateLimit itself comes from) and injects them into the worker as a small module, __VITEST_POOL_WORKERS_RATELIMIT_BINDING_NAMES.
  • reset.ts imports that list and calls .reset() directly on each named binding via env:
    import { env } from "cloudflare:workers";
    import ratelimitBindingNames from "__VITEST_POOL_WORKERS_RATELIMIT_BINDING_NAMES";
    
    for (const name of ratelimitBindingNames) {
      (env[name] as { reset?: () => void }).reset?.();
    }

Since env.RATE_LIMITER already is the Ratelimit instance (the binding is just that object, wrapped), .reset() is a normal method call on a binding we already know by name — no globalThis, no shared internal module, no new binding, no touching Miniflare's extension/wrapped-binding machinery at all.

Why not the other things I tried

  • A dedicated "control" binding sharing the ratelimit extension's moduleName (my previous push) — workerd rejects two wrapped bindings pointing at the same internal module (wrapped binding module can't be resolved (internal modules only)), and it broke dev-server startup broadly, not just ratelimit-using workers. Reverted.
  • Walking Object.values(env) and duck-typing for .reset() — touches every binding including SELF/entrypoint-style ones, and property access on those triggers a real RPC handshake, throwing Expected default export ... to be a subclass of WorkerEntrypoint for RPC. The binding-name approach above avoids this by only ever touching bindings we already know are ratelimits.

Verification

Ran the full vitest-pool-workers-examples suite locally (56 test files / 160 tests, including reset, rpc, workflows, assets) — all green, no unhandled errors. pnpm check:type clean for both miniflare and vitest-pool-workers. CI is running now.

__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.
@matingathani matingathani marked this pull request as ready for review July 3, 2026 10:00

@petebacondarwin petebacondarwin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_LOOPBACKSERVICE_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 in reset.test.ts, so I'm fine with it — just noting it.
  • The DO now takes limit/period per request, so the instance is config-agnostic. That's fine while it's keyed per-binding, but it interacts with the namespace_id change 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.)

@petebacondarwin

Copy link
Copy Markdown
Contributor

I just looked into workerd and it seems that preventEviction will indeed mean that deleteAllDurableObjects() will skip that DO. So this is probably not going to work...

@matingathani

Copy link
Copy Markdown
Contributor Author

So should i go with 1,2,4 and 5 or wait for it ?

@petebacondarwin

Copy link
Copy Markdown
Contributor

If the epoch is short enough, then perhaps the DO eviction period will be longer and we don't need preventEviction...

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 this.ctx.storage.deleteAll() internally.

@petebacondarwin

Copy link
Copy Markdown
Contributor

So should i go with 1,2,4 and 5 or wait for it ?

Let's do 2 & 5 now.
I am on the fence whether you need to pull the fixes (4) out into a separate PR. Your call.

@matingathani

Copy link
Copy Markdown
Contributor Author

Ok , let me have a look at it and i will get back with a fix

…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.
@matingathani

Copy link
Copy Markdown
Contributor Author

Addressed 2, 4, 5 — leaving 1 (preventEviction/deleteAllDurableObjects interaction) and 3 (namespace_id keying, #14537) with you as discussed.

  • 2: RATELIMIT_ENTRY_SERVICE_PREFIX's worker definition now binds SharedBindings.MAYBE_SERVICE_LOOPBACKSERVICE_LOOPBACK and spreads getMiniflareObjectBindings(unsafeStickyBlobs), matching Queues/KV.
  • 4: split the Windows ECONNRESET retry (dispatchFetchWithRetry) and hyperdrive e2e EPIPE-ignore fixes into their own PR — [miniflare][wrangler] Fix Windows CI flakes: local-explorer ECONNRESET retry, hyperdrive e2e EPIPE #14568. Reverted both commits from this branch.
  • 5: .changeset/vitest-pool-workers-reset-ratelimit.md now also bumps @cloudflare/vitest-pool-workers.

Verified locally: packages/miniflare/test/plugins/ratelimit/index.spec.ts (2/2), the reset vitest-pool-workers-examples fixture (10/10, including the ratelimit reset case), and pnpm --filter miniflare check:type clean.

@workers-devprod

workers-devprod commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Codeowners approval required for this PR:

  • ✅ @cloudflare/wrangler
Show detailed file reviewers

devin-ai-integration[bot]

This comment was marked as resolved.

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.
@petebacondarwin petebacondarwin removed the request for review from ascorbic July 6, 2026 12:16
@petebacondarwin

Copy link
Copy Markdown
Contributor

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 preventEviction: true.

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 workers-devprod left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codeowners reviews satisfied

@github-project-automation github-project-automation Bot moved this from In Review to Approved in workers-sdk Jul 6, 2026
@petebacondarwin

Copy link
Copy Markdown
Contributor

I'll follow up with my PRs

@petebacondarwin petebacondarwin merged commit 16fbf81 into cloudflare:main Jul 6, 2026
62 checks passed
@github-project-automation github-project-automation Bot moved this from Approved to Done in workers-sdk Jul 6, 2026
@matingathani

Copy link
Copy Markdown
Contributor Author

Sounds good , let me know if you any other issue persist , i will open up new pr to solve it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[vitest-pool-workers] reset helper does not affect ratelimit bindings

4 participants