[miniflare] Persist local rate limit counters so they survive Durable Object eviction - #14968
Conversation
🦋 Changeset detectedLatest commit: 6e7a029 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 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 |
|
✅ All changesets look good |
|
The implementation looks solid and closely mirrors established patterns. Let me examine one more potential concern: the Let me double-check the test logic for Looking at the period test:
These run 4 quick calls. Both must land in the same 60s and 10s window. After the first wait we have ≥10s in the 60s window. The second wait could sleep up to ~2s, still leaving ≥8s in the 60s window. Fine. The logic is sound. The implementation is correct, follows existing patterns (KV/R2/D1), the SQL usage is safe (parameterized statements), and concurrency within a single DO is serialized by workerd's input gates. I found no logic bugs, security issues, or backward-compatibility violations. LGTM |
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@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: |
…tion The emulated Ratelimit binding kept its bucket/epoch state on the JS heap of the internal `RateLimiterObject`. That namespace is evictable, so once workerd evicted the idle object (~10s) the counters were gone and the next `limit()` call started from zero, even though the window had not rolled over. `preventEviction` is not an option: `Server::deleteAllActors` skips non-evictable namespaces, so pinning the object would break the `reset()` provided by vitest-pool-workers. `inMemory` storage is no better -- workerd backs it with a per-actor `ActorCache` over empty read-only storage, so it is discarded on eviction too (and cannot support `state.storage.sql` at all). Move the counters into `state.storage`, backed by a disk directory service like the KV, R2 and D1 simulators. That survives eviction while remaining resettable, because `ActorNamespace::deleteAll()` calls `SqliteDatabase::reset()` before aborting the actor. This is a pure relocation of the existing state: counters stay keyed by key alone, so bindings sharing a `namespace_id` keep sharing a counter as documented, and windows still roll over together across the namespace. Closes #14962
3efd52c to
7366487
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
petebacondarwin
added
package:miniflare
Bindings that share a `namespace_id` but are configured with different `simple.period` values overwrote each other's counter on every call: each saw an epoch belonging to the other's window, reset the count to zero, and then deleted the other's row as expired. Neither binding ever limited anything. Production keys a counter by `(account_id, namespace, hash(key), bucket, bucket_start_ts)`, and both `bucket` and `bucket_start_ts` are derived from the period, so differing periods never share a counter there. Add `period` to the bucket table's primary key and scope the expiry sweep to it, which reproduces that partitioning. When periods match — the normal case, and the one the docs describe — the partition is unchanged, so bindings sharing a namespace still share a counter for a given key. Also point the two "keep in sync with" comments at the current location of the structs they track: they are `JSG_STRUCT`s in edgeworker's `internal-api/ratelimit.h`, not in the neighbouring capnp schema, and the repository has since moved from Bitbucket to GitLab.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ured
Wrangler passes `ratelimits: {}` rather than omitting the key when a Worker
declares no rate limit bindings, and `{}` is truthy, so the presence check
let every `wrangler dev` session fall through to `fs.mkdir()` and leave an
unused `ratelimit` directory in the user's persistence directory. Bail on
emptiness instead, which also stops an unreferenced Durable Object namespace
being registered.
Add a test for that, plus one covering the persistence the disk service buys
us: a counter exhausted by one Miniflare instance is still exhausted for the
next one sharing a `resourcePersistencePath`, as when restarting
`wrangler dev` mid-window.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
workers-devprod
left a comment
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
3 participants
Fixes #14962.
Miniflare's emulated Ratelimit binding kept its bucket/epoch state on the JS heap of the internal
RateLimiterObject. That namespace is evictable, so once workerd evicted the idle object (after ~10s) the counters were gone and the nextlimit()call started from zero, even though the rate limit window had not rolled over. In practice the emulation only held for tight bursts: inwrangler devyou could hit your Worker, pause for ~15 seconds to look at something, hit it again, and your limit had silently reset.Counters now live in the Durable Object's
state.storage, backed by a disk directory service like the KV, R2 and D1 simulators.The second commit fixes the related bug the issue also flagged — see "Mixed periods in one namespace" below. The third fixes a stray directory the first one would otherwise have created.
Why not
preventEviction?Server::deleteAllActorsguards withif (ns->isEvictable()), so pinning the namespace makes it immune todeleteAllDurableObjects()— which is exactly whatvitest-pool-workers'reset()calls. Confirmed empirically:preventEviction: truemakesfixtures/vitest-pool-workers-examples/reset/test/reset.test.tsfail on "sees reset ratelimit state after reset".Why not
inMemorystorage?The issue left this as an open question. It turns out
inMemorywould not have helped either, despite what thecompatibility-date.capnpdocs claim ("data will persist for the lifetime of the process... individual objects will still shut down when idle as normal"). When a namespace has noactorStorage,ActorContainer::start()buildskj::heap<ActorCache>(newEmptyReadOnlyActorStorage(), ...)per actor, owned by theWorker::Actorthat eviction destroys. It is exactly as lossy as the heap. Verified empirically: flipping this branch toinMemoryfails every rate limit test, because that mode isn't SQLite-backed sostate.storage.sqlisn't even available.Does
reset()still work?Yes, and no new mechanism was needed.
ActorNamespace::deleteAll()callsresetStorage()→SqliteDatabase::reset()before aborting the actor, which is precisely how the KV, R2 and D1 simulators are already reset. The existingresetfixture passes unchanged.Because the counters go through
getPersistPath(), they also now survive awrangler devrestart within the same window — consistent with every other stateful plugin, and arguably more faithful to production. Undervitest-pool-workers, which does not setresourcePersistencePath, they land in Miniflare's temp directory and are discarded on dispose as before.Mixed periods in one namespace
The issue also flagged that the epoch was namespace-wide while
periodarrives per request, so bindings sharing anamespace_idwith different periods clear each other's counters. This is fixed in the second commit, by addingperiodto the bucket table's primary key and scoping the expiry sweep to it.This went back and forth, so for the record — the deciding evidence is production's own counter identity. In
doppler-lib/src/counts.rs:bucketandbucket_start_tsare both derived from the period (get_buckets()indoppler-lib/src/memcache.rs), so production partitions counters by period implicitly. Two bindings on onenamespace_idwith different periods each keep a working counter there. Note also that the limit itself is not part of the key — it is a threshold applied to a shared count — which is why differingsimple.limiton a shared namespace remains coherent and is left alone here.Miniflare did the opposite: a single row per key meant each call saw an epoch belonging to the other's window, reset the count to zero, and then deleted the other's row as expired, so neither binding limited anything at all. That is a strictly worse divergence than the alternative, and it pre-exists this PR —
mainhas the same flaw via its single#epochscalar plus#buckets.clear().When periods match — the normal case, and the only one the public docs describe —
(key, period)andkeypartition identically, so the documented "bindings sharing anamespace_idshare counters for a given key" behaviour is unchanged.No stray directory when rate limiting isn't used
Wrangler passes
ratelimits: {}rather than omitting the key when a Worker declares no rate limit bindings (packages/wrangler/src/dev/miniflare/index.ts:1073), and{}is truthy. The plugin's!options.ratelimitsguard therefore let every dev session reach the newfs.mkdir()and leave an unusedratelimitdirectory behind. The third commit bails on emptiness instead, which also avoids registering an unreferenced Durable Object namespace.Known limitation (pre-existing, not introduced here)
ActorContainer::resetStorage()is guarded byKJ_IF_SOME(a, actor), so it only resets running actors. If a disk-backed simulator DO has already been evicted whenreset()runs, its.sqlitefile survives. This affects KV/R2/D1 identically today and only triggers if >10s elapse between touching a resource and callingreset(). Probably worth an upstream workerd issue.Also out of scope
Miniflare uses a fixed window; production uses a sliding window (
prev * (1 - elapsed/period) + cur) and skips the increment when already over the threshold. Worth a follow-up, but it is a behaviour change rather than a bug fix, so it is not in this PR.Four tests were added to
packages/miniflare/test/plugins/ratelimit/index.spec.ts:"ratelimit counters survive a workerd restart" — exhausts the limit, calls
setOptions()(which unconditionally kills and respawns workerd viaRuntime#updateConfig, tearing down every Durable Object), and asserts the next call is still rejected. This is a strictly stronger teardown than the ~10s idle eviction, and runs in milliseconds rather than needing a real sleep. Verified it fails against the old heap implementation and passes with the fix."ratelimit counters are scoped per period" — two bindings on one
namespace_id, one withperiod: 10and one withperiod: 60, bothlimit: 1. Verified it fails against the key-only schema, and with exactly the predicted symptom: the third call returns 200 instead of 429, i.e. no limiting whatsoever."ratelimit persists on file-system" — exhausts the limit, disposes, and builds a fresh
Miniflaresharing the sameresourcePersistencePath; the window hasn't rolled over, so the limit is still exhausted. Covers thewrangler devrestart claim above."ratelimit creates no storage directory when unconfigured" —
ratelimits: {}must not leave aratelimitdirectory in the persistence directory. Verified it fails against the presence-only guard.The issue's verbatim repro (one call, 20s sleep inside a single 60s window, second call) was also run as a throwaway test against a real eviction and passes with the fix. It is not included in the suite because it costs ~55s of wall clock for a property the restart test already covers.
waitForFreshRateLimitWindow()(added in #14961 to de-flake bucket boundaries) now takes a list of periods so the mixed-period test can require headroom in both windows at once. Its threshold for the existing 60s tests is unchanged, and because 10 divides 60 a single sleep realigns both, so the new test waits ~3s at worst.The existing "ratelimit counters are keyed by namespace_id" test and the
resetfixture's "shares ratelimit state across bindings with the same namespace_id" cover the documented shared-counter semantics, and both pass unchanged.Also verified: full miniflare suite (1006 passed), the
@scoped/resetvitest-pool-workers fixture (11 passed), andpnpm check(201/201).A picture of a cute animal (not mandatory, but encouraged)
Note
This is a contribution from an AI agent: opencode, claude-opus-5.