feat: build a key's Limiter from its key, for datastore-backed limiters - #14
Merged
Merged
Conversation
A Limiter is bound to exactly one key -- Allow and Wait take no arguments, so
the instance IS the bucket. For an in-process limiter that is invisible: every
bucket is equivalent, so `newLimiter func() Limiter` suffices. For a limiter
whose counter lives somewhere else -- Redis, Valkey, any shared store -- it is
the whole problem, because such a limiter has to know WHICH remote key is its
own, and nothing in the API ever told it.
WithLimiterFactoryForKey(func(K) Limiter) closes that gap.
bl := ratelimiter.NewBucketLimiter(nil, time.Minute,
ratelimiter.NewInMemoryStorage[string, ratelimiter.Limiter](),
ratelimiter.WithLimiterFactoryForKey(func(key string) ratelimiter.Limiter {
if shared != nil {
return sharedLimiter{client: shared, key: "rl:" + key}
}
return ratelimiter.RateLimiter{Limiter: rate.NewLimiter(limit, burst)}
}),
)
That is the entire seam for "shared budget when the datastore is there,
in-process when it is not" -- and note the storage in both branches is the
ordinary in-memory one, because it only caches handles. The shared state lives
in the datastore, inside the Limiter.
WHY NOT A DISTRIBUTED Storage, which is what everyone tries first: GetOrAdd
hands the caller the Limiter and the caller calls Allow() on it. The manager
never writes back -- Store is not called by it at all. So a Storage that
serialised bucket state to Redis would deserialise a FULL bucket on every
request, let the caller spend a token from an object nobody persists, and drop
it. The limit would never be reached, and nothing would look wrong.
docs/CUSTOM_STORAGE.md already said so; this commit gives people the thing to
do instead of only the thing not to do.
The previous advice was to use Storage.LoadOrStore as a key->limiter resolver,
because it was the only place that saw both the key and a shared client. That
still works and is still documented, but it reinterprets an interface whose job
is to hold values and hides construction where nobody looks for it.
Option is deliberately NOT made generic. Changing it to Option[K] would force
every existing WithClock(now) call to be explicitly instantiated, breaking
source compatibility for every current user; the factory is stored as any and
recovered with a checked assertion at construction, so a key-type mismatch
panics there rather than surfacing as a nil limiter on the first request for a
new key. Supplying neither factory now panics at construction too, for the same
reason.
Tests, each verified to fail by mutating the code it guards -- and two of them
were rewritten because the first version could not fail:
- the key reaches the factory, and one key's spending does not affect another
- state survives eviction of the handle, which is the property that separates
a datastore-backed limiter from an in-process one
- the key-aware factory wins when both are supplied
- no factory at all panics at construction
- a mismatched key type panics, and the test asserts WHICH panic: checking
only that "a panic happened" passed with the type check removed, because
the both-factories-nil guard fired instead
- concurrent first use returns ONE instance, asserted by identity: the first
version counted tokens against shared state, which passes whether or not
creation is atomic, since every instance addresses the same budget
examples/keyfactory shows the difference in six lines of output: evict the
handle and a shared limiter stays spent while an in-process one comes back full.
docs/CUSTOM_STORAGE.md and README updated. The distributed section also stops
implying a Lua script is the only way to be atomic -- a token bucket is a
read-modify-write and needs EVAL or WATCH/MULTI/EXEC, but a sliding-window
counter is a single atomic INCR, and choosing the algorithm is a legitimate
alternative to choosing a scripting engine.
Coverage stays at 100% of statements.
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.
The gap
A
Limiteris bound to exactly one key —Allow()andWait()take noarguments, so the instance is the bucket.
For an in-process limiter that's invisible: every bucket is equivalent, so
newLimiter func() Limitersuffices. For a limiter whose counter livessomewhere else — Redis, Valkey, any shared store — it's the whole problem: such
a limiter must know which remote key is its own, and nothing in the API ever
told it.
The fix
That's the entire seam for "shared budget when the datastore is there,
in-process when it isn't". Note the storage in both branches: the ordinary
in-memory one. It only caches handles; the shared state lives in the datastore,
inside the
Limiter.Why not a distributed
Storage— the thing everyone tries firstGetOrAddhands the caller theLimiterand the caller callsAllow()on it.The manager never writes back —
Storeisn't called by it at all. So aStoragethat serialised bucket state to Redis would deserialise a fullbucket on every request, let the caller spend a token from an object nobody
persists, and drop it. The limit would never be reached, and nothing would look
wrong.
docs/CUSTOM_STORAGE.mdalready said so. This PR gives people the thing todo, instead of only the thing not to do.
The previous advice — use
Storage.LoadOrStoreas a key→limiter resolver — stillworks and is still documented, but it reinterprets an interface whose job is to
hold values, and hides construction where nobody looks for it.
Optionis deliberately not made genericChanging it to
Option[K]would force every existingWithClock(now)call to beexplicitly instantiated — breaking source compatibility for every current user.
The factory is stored as
anyand recovered with a checked assertion atconstruction, so a key-type mismatch panics there rather than surfacing as a
nil limiter on the first request for a new key. Supplying neither factory now
panics at construction too, for the same reason.
Fully backward compatible:
newLimiterkeeps working and is only ignored whenthe option is supplied.
Tests — each verified to fail by mutating what it guards
datastore-backed limiter from an in-process one ✅
Two of these were rewritten because the first version couldn't fail, which
is worth calling out:
removed it still passed, because the both-factories-nil guard fired instead.
It now asserts which panic.
or not creation is atomic, since every instance addresses the same budget. It
now asserts instance identity.
examples/keyfactoryShows the difference in six lines of output — evict the handle and a shared
limiter stays spent, an in-process one comes back full:
Docs
CUSTOM_STORAGE.mdand the README updated. The distributed section also stopsimplying a Lua script is the only way to be atomic: a token bucket is a
read-modify-write and needs
EVALorWATCH/MULTI/EXEC, but asliding-window counter is a single atomic
INCR. Choosing the algorithm is alegitimate alternative to choosing a scripting engine.
Gates
go test -race -coverprofile -covermode=atomic -tags=unit ./...— 100.0% ofstatements, unchanged ·
go vet·gofmtclean.🤖 Generated with Claude Code