feat: Backend — one limit shared across processes, any KV store - #15
Merged
Conversation
Storage holds limiters IN THIS PROCESS. There was no seam for holding the
COUNT somewhere else, so a limit shared across instances was out of reach
without abandoning this package.
Backend is that seam:
type Backend interface {
Take(ctx context.Context, key string, limit Limit, cost int) (Decision, error)
}
Implement it over Valkey, Redis, DynamoDB, Postgres — or use the bundled
MemoryBackend — and every process shares one budget.
WHY IT IS TAKE-SHAPED AND NOT GET/SET-SHAPED. The obvious design is a small
key-value interface — Get, Set, Incr — with the token arithmetic in this
package. That works in one process and is a race everywhere else: the update is
read (tokens, ts) -> refill by elapsed time -> compare -> write back
a read-modify-write. Two processes read the same state, both decide they may
proceed, both write, and the limit silently becomes 2x. Making it safe needs a
transaction with a retry loop on a key that is contended by definition, or a
server-side script; neither is expressible through Get/Set. So the decision has
to run where the state lives, and the interface has to be the DECISION.
That is also precisely why Storage cannot do this: Storage is a container of
Limiter values in this process, and its Load/Store shape IS the Get/Set shape.
WHAT BackendLimiter ADDS, and why it is here rather than copied into every
consumer:
- A LOCAL FALLBACK. Neither answer to "the datastore is down" is acceptable
alone: refusing turns a blip into a total outage, allowing deletes the
limiter exactly when it is needed. With a fallback there is nothing to
choose between -- an outage degrades to per-process limiting, which is what
you had before adding a backend at all.
- A CIRCUIT BREAKER. Falling back is only half a fallback. Without one, every
request during an outage pays a failed round trip before reaching the local
answer it was always going to get: the limiter keeps working and makes the
whole service slower by the timeout.
- A DEGRADED SIGNAL. A limiter silently enforcing N x the intended limit is
invisible from a request. Alert on the state, not the error rate.
MemoryBackend is deliberately a sliding-window counter rather than a token
bucket, even though this package has a better in-process limiter. It is the same
algorithm a datastore backend runs, so moving between them does not move the
behaviour underneath you. RateLimiter remains the right choice when you want the
best in-process limiter.
TWO BUGS THE TESTS FOUND, both in code I had already written and believed:
- The single-prober guard used a mutex with `defer Unlock()`, which releases
when shouldAsk RETURNS -- nanoseconds later, long before the backend
answers. Every caller acquired it in turn. Measured: 20 of 20 racers
reached the backend, which is the thundering herd the guard exists to
prevent. It is a CAS on an atomic held across the call now.
- A FAILED probe did not release the slot, so the breaker wedged shut for
ever: the backend was never re-tested and the limiter stayed degraded
permanently, including long after recovery. Worse than the outage, because
it does not end when the outage does.
Benchmarks drove one API addition. BackendLimiter.Allow measured 391 ns/op
against a backend that answers in 100 ns, all of it context.WithTimeout arming
a timer per request. WithoutBackendTimeout removes the deadline for in-process
backends: 110 ns/op. It is documented as never for a network backend, where the
deadline is what bounds a hung datastore.
Every test verified to fail by mutating the code it guards. Coverage 99.7%.
Docs: new docs/BACKENDS.md; doc.go rewritten (it claimed distributed limiting
was out of scope for the bundled types, which this makes false);
TOKEN_BUCKET.md's "deliberately out of scope" section replaced with the real
trade-off table; CUSTOM_STORAGE.md now routes readers to BACKENDS.md instead of
only explaining what not to do; MIGRATION.md notes this release is additive.
All four cross-link. Runnable example in examples/backend.
Purely additive: every existing call keeps working.
go.mod moves from 1.26.0 to 1.27.0. CI resolves its toolchain from `go-version-file: ./go.mod`, so this is what the build actually runs on, and it matches the toolchain the consumers of this library are already using. golang.org/x/time is already at v0.15.0, the latest; `go get -u ./...` and `go mod tidy` produce no change. The dependency list stays one entry, which is worth keeping that way. `go fix ./...` had one thing to say, in code added by the previous commit: a three-line clamp collapsed to `max(...)`. That branch was the last statement in the package no test reached, so coverage goes back to 100.0% -- the level this repo was at before the Backend work, and the reason to check rather than assume it was still true. NOTE FOR REVIEWERS: raising the `go` directive raises the minimum Go version for everyone who imports this library. That is a deliberate choice rather than a side effect of the tooling, and it is the one part of this change that is not purely additive.
Two changes, one of them undoing a mistake I made in the previous commit.
REVERT THE go DIRECTIVE to 1.26.0. Bumping it to 1.27.0 broke CodeQL:
go: go.mod requires go >= 1.27.0 (running go 1.26.6; GOTOOLCHAIN=local)
Extraction failed for all discovered Go projects.
The autobuilder runs whatever Go the runner image ships with GOTOOLCHAIN=local,
so it cannot fetch a newer toolchain and the analysis simply fails.
But the failure is the smaller half of the argument. The bump was justified as
"this is what CI builds with, and it matches the consumers" -- and CodeQL is a
counter-example to the first half, while the second is not a reason that binds
anybody else. Nothing in this package needs 1.27: `go vet` with the directive at
1.26 reports no use of a newer standard-library symbol, and the full suite
passes at 100% coverage. Raising the minimum Go version of a PUBLIC library
excludes every consumer still on the previous release, and Go supports the two
most recent majors. Doing that for alignment rather than for a feature is a cost
paid by other people for our convenience.
The `max(...)` rewrite `go fix` produced stays: it predates 1.27 and it is why
coverage reached 100%.
PIN GO IN THE CODEQL WORKFLOW anyway, from go.mod, before codeql-action/init.
The workflow is fragile independently of this revert: it analyses with whatever
Go the runner happens to have that week, so the day the directive legitimately
moves, this breaks again. And it breaks illegibly -- the reported error is "We
were unable to automatically build your code", which points nowhere near a
version mismatch. This is the same class of problem as the pr.yaml coverage step
noted in the PR: a workflow that depends on an unstated assumption about the
runner.
Verified at 1.26.0: go vet clean (including the stdversion check, which is what
proves no 1.27-only symbol is used), go fix -diff clean, race tests green,
coverage 100.0%.
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.
Builds on #14 (merged), which supplies the
WithLimiterFactoryForKeyseam.The gap
Storageholds limiters in this process. There was no seam for holding thecount somewhere else — so a limit shared across instances was out of reach
without abandoning this package.
Implement it over Valkey, Redis, DynamoDB, Postgres — or use the bundled
MemoryBackend— and every process shares one budget.Why
Take-shaped and notGet/Set-shapedThe obvious design is a small key-value interface with the token arithmetic in
this package. That works in one process and is a race everywhere else:
Two processes read the same state, both decide they may proceed, both write, and
the limit silently becomes 2×. Making it safe needs a transaction with a
retry loop on a key that is contended by definition, or a server-side script —
neither expressible through
Get/Set.So the decision has to run where the state lives, and the interface has to be
the decision. That is also precisely why
Storagecannot do this: it is acontainer of
Limitervalues in this process, and itsLoad/Storeshape isthe
Get/Setshape.StorageLimiterBackendWhat
BackendLimiteraddsalone: refusing turns a blip into a total outage; allowing deletes the limiter
exactly when it is needed. With a fallback there is nothing to choose between —
an outage degrades to per-process limiting, which is what you had before adding
a backend at all.
every request during an outage pays a failed round trip before reaching the
local answer it was always going to get.
invisible from a request. Alert on the state, not the error rate.
MemoryBackendis a window counter on purposeEven though this package has a better in-process limiter. It is the same
algorithm a datastore backend runs, so moving between them does not move the
behaviour underneath you.
RateLimiterstays the right choice when you want thebest in-process limiter — documented in
TOKEN_BUCKET.mdwith the trade-offtable.
Two bugs the tests found — in code I had already written and believed
defer Unlock(), which releases whenshouldAskreturns — nanosecondslater, long before the backend answers. Every caller acquired it in turn.
Measured: 20 of 20 racers reached the backend, the exact thundering herd
the guard exists to prevent. Now a CAS held across the call.
ever: the backend was never re-tested and the limiter stayed degraded
permanently, including long after recovery. Worse than the outage, because
it does not end when the outage does.
Both have tests now, both verified to fail by reverting the fix.
Benchmarks drove an API addition
BackendLimiter.Allowmeasured 391 ns/op against a backend answering in100 ns — all of it
context.WithTimeoutarming a timer per request.WithoutBackendTimeoutremoves the deadline for in-process backends, documentedas never for a network backend, where the deadline is what bounds a hung
datastore.
Tests
Every one verified to fail by mutating the code it guards — 7 mutations.
Coverage 100.0%, the level this repo was at before this work.
Docs — all of them
docs/BACKENDS.mddoc.godocs/TOKEN_BUCKET.mddocs/CUSTOM_STORAGE.mdBACKENDS.mdinstead of only explaining what not to dodocs/MIGRATION.mdBackendrelease is additiveREADME.mdAll four cross-link; link check clean. Runnable:
go run ./examples/backend.Dependencies and toolchain
golang.org/x/timeis already atv0.15.0, the latest —go get -u ./...andgo mod tidyproduce no change, and the dependency list stays one entry.The
godirective moves 1.26.0 → 1.27.0, which is what CI actually buildswith (
go-version-file: ./go.mod) and matches the consumers.go fixthen hadone thing to say — a three-line clamp collapsed to
max(...), which happened tobe the last statement no test reached, taking coverage back to 100%.
Unrelated finding
CI runs
go-test-coverage --config=./.testcoverage.ymland that file does notexist in the repo. It exits 0 because the step pipes through
teewithoutpipefail, so the coverage gate silently passes without ever running. Nottouched here — adding the config would start enforcing a threshold nobody has
chosen.
🤖 Generated with Claude Code