[vitest-pool-workers] fix: reset() now clears ratelimit binding state between tests#14409
[vitest-pool-workers] fix: reset() now clears ratelimit binding state between tests#14409matingathani wants to merge 3 commits into
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: 681bd41 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 |
workers-devprod
requested review from
a team and
ascorbic
and removed request for
a team
|
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: |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const __ratelimitInstances__ = new Set<Ratelimit>(); | ||
| ( | ||
| globalThis as { __cfRatelimitInstances__?: Set<Ratelimit> } | ||
| ).__cfRatelimitInstances__ = __ratelimitInstances__; |
There was a problem hiding this comment.
🟡 Reset hook uses a global variable, violating repository coding rules
A function is attached to globalThis unconditionally (__cfRatelimitReset__ at packages/miniflare/src/workers/ratelimit/ratelimit.worker.ts:101) whenever any rate-limit binding loads, so every worker with a rate-limit binding has its global scope polluted — even outside test runs.
Impact: Violates the REVIEW.md rule "Avoid using global variables. Repository maintainers will reject code that introduces global variables."
Mechanism and context
The ratelimit extension module sets globalThis.__cfRatelimitReset__ at module load time (ratelimit.worker.ts:100-106). The consumer in packages/vitest-pool-workers/src/worker/reset.ts:10-12 reads it back from globalThis. This is the only way to communicate across the internal: true module boundary, but the REVIEW.md rule is unconditional: "Avoid using global variables. Repository maintainers will reject code that introduces global variables."
Alternative approaches might include having the vitest pool plugin inject the ratelimit reset capability through workerd bindings or configuration rather than a runtime global.
Was this helpful? React with 👍 or 👎 to provide feedback.
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
| R2_BUCKET: R2Bucket; | ||
| DATABASE: D1Database; | ||
| COUNTER: DurableObjectNamespace<import("./index").Counter>; | ||
| RATE_LIMITER: RateLimit; |
There was a problem hiding this comment.
🚩 Generated env.d.ts appears manually edited rather than regenerated
The env.d.ts file at fixtures/vitest-pool-workers-examples/reset/src/env.d.ts has a generated-file header with hash 524c60174bc1732153e84c1b8699023e (line 2), but the new RATE_LIMITER: RateLimit binding on line 8 was added without updating the hash. Since wrangler types supports ratelimit bindings (see packages/wrangler/src/type-generation/index.ts:2329), running it after updating wrangler.jsonc would regenerate this file with a correct hash. The stale hash suggests manual editing. AGENTS.md says 'Never modify generated files directly — modify the generator or config, then regenerate.' This is in a test fixture so the practical impact is minimal, but it could cause confusion if the file is later regenerated and produces unexpected diffs.
(Refers to lines 2-8)
Was this helpful? React with 👍 or 👎 to provide feedback.
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