feat(token_rate_limit): sliding-window/token-bucket rate limiting (M1/M2/M6)

[jordigilh]

Summary

Implements the uncontested MVP core of the token rate limiting proposal (00121_token-rate-limiting.md in praxis-proxy/enhancements, epic ai#121):

• M1 — ordered rules:, each an optional static header-value match condition bound to its own budget (catch-all default rule when no match: is given)
• M2 — reservation-based admission: reserve an estimated token cost at request time, reconcile against actual provider-reported usage (token_count's token.total) once the response completes
• M6 — 429 responses with X-RateLimit-*-Tokens headers on hard deny

Two admission algorithms, chosen per rule via algorithm: (sliding_window | token_bucket, per the maintainer framing on ai#789/praxis#551 that this is "a per-rule choice, similar to shadow/enforcement-action knobs elsewhere"), both behind a pluggable backend: in-process by default, or a shared Valkey backend (backend: {kind: valkey}) for state shared across gateway instances/replicas.

Descoped from this PR: M5 (per-header/per-app bucket keys, ai#129) has been pulled out. It conflicted with the quota-key design ai#790 already established for the Valkey-backed shared quota work (namespace + rule + principal + canonical model, not an arbitrary header value), and ai#790 itself isn't approved yet. Every rule now resolves to a single shared budget regardless of request headers; ai#129 tracks per-key budgets separately pending the quota-key question being resolved with ai#790's stakeholders.

Also deliberately deferred (see filters/src/token_rate_limit/mod.rs module doc for the full list and rationale): configurable estimation (M3), token-type-aware weighting (M4), multiple budgets/soft-limit tiers per rule, observability, and metering.

Additional fixes included

• Valkey backend now reuses a cached multiplexed connection instead of opening a fresh one per reserve/reconcile call.
• Fixed a doc-generator bug where token_rate_limit's AlgorithmConfig enum collided with an unrelated struct of the same name in a sibling filter, producing phantom fields in generated docs; renamed to RuleAlgorithm. Also fixed the generator leaking Rust's r# raw-identifier prefix into the match field name.

Testing

• 113 unit tests in filters/src/token_rate_limit/, covering both algorithms, both backends, config validation, and reservation/reconciliation edge cases (expired-reservation reaping, lost-request handling, capacity denials on all three configured bounds).
• Integration test exercising the full HTTP round trip against both example configs (examples/configs/token-rate-limit.yaml, examples/configs/token-rate-limit-mixed-algorithms.yaml).
• make lint (clippy, fmt, cargo doc -D warnings, separator/filter-doc/example-test xtask lints) passes clean.

Also fixed: every Valkey-backed test is gated on TOKEN_RATE_LIMIT_VALKEY_URL, but CI never set it, so they silently no-opped instead of running. Added .github/workflows/valkey.yaml (same shape as postgres.yaml) to run them for real, which surfaced a few more gaps now closed too: ValkeyEval's invalidate-on-failure path, the in-process backends' unused reconcile/enqueue_reconcile trait methods, and two no-op paths in mod.rs.

Supersedes the closed draft #773, which called out the sliding-window-vs-token-bucket question as unresolved; this PR resolves it by supporting both, per rule.

[praxis-bot-app]

Missing Signed-off-by: 47f6876, 82eab8e. All commits require sign-off (via git commit --signoff).

[praxis-bot-app]

Unsigned commits: e156a1a, 47f6876, 82eab8e. Please sign your commits.

[praxis-bot]

Review: feat(token_rate_limit): per-header rate-limit bucket keys (ai#129)

Well-structured PR with thorough test coverage (112 unit tests, integration tests for all three example configs, both algorithms, both backends). The reserve/reconcile design, fail-closed Valkey behavior, and per-key budget isolation are all solid. Two issues worth addressing before merge.

Severity	Count
Critical	0
Large	2
Medium	1

All comments are inline.

[nerdalert]

Thanks for putting this together. The reservation/reconciliation split and shared backend direction are solid. I found four areas where tightening the trust boundary and hot-path behavior would make the implementation safer to operate.

[asaadbalum]

Reviewed the full diff, correct proposal alignment (M1/M2/M6), and both algorithms working with pluggable backends.

On Brent's review comments:
Trust boundary and probe traffic: agree these are real concerns. I'd suggest tracking them as follow-up issues rather than blocking this PR since the proposal's Non-Goals already state "assumes identity resolved by upstream," so this is a known boundary.

Approving, let's get this in.

[jordigilh]

Two commits landed after your approval, flagging since the diff changed:

991724a fixes Brent's Lua-safe-integer-bound comment (mod.rs:305/311) -- sliding_window was missing the same 2^53 capacity check token_bucket already had, a real gap once Valkey's Lua scripts convert it with tonumber. Added regression coverage for both algorithms.

d45e1e9: a coverage audit found every Valkey-backed test (cross-instance state, background-worker reconciliation) silently no-ops in CI -- TOKEN_RATE_LIMIT_VALKEY_URL is required but was never set anywhere, so they'd been passing by skipping, not running. Added .github/workflows/valkey.yaml (same shape as postgres.yaml) to run them for real, plus tests for the gaps that surfaced once they did.

Trust-boundary and probe-traffic are unchanged, still open per your note above and grid#101.

Let me know if you want another look given the new commits.

[nerdalert]

Reviewed the full diff, correct proposal alignment (M1/M2/M6), and both algorithms working with pluggable backends.

On Brent's review comments: Trust boundary and probe traffic: agree these are real concerns. I'd suggest tracking them as follow-up issues rather than blocking this PR since the proposal's Non-Goals already state "assumes identity resolved by upstream," so this is a known boundary.

Approving, let's get this in.

@asaadbalum wfm! Added them to the post merge tracking issue. praxis-proxy/grid#101

@jordigilh the additions look good. Lets get CI passing and the review issues resolved and once @jland-redhat gives a 👍 we will get it queued for merge, ty!

[asaadbalum]

LGTM

[alexsnaps]

So, just for my understanding here, is this meant to become the TRLPolicy enforcing mechanism at some point?
It is a conscious decision that break backwards compatibility here? If I read this right probably on semantic, but certainly on "counters"? Finally, this is a 3.6 grid feature, yes? No model serving? Should we as such move this behind a feature flag? Experimental even?

[nerdalert]

So, just for my understanding here, is this meant to become the TRLPolicy enforcing mechanism at some point? It is a conscious decision that break backwards compatibility here? If I read this right probably on semantic, but certainly on "counters"? Finally, this is a 3.6 grid feature, yes? No model serving? Should we as such move this behind a feature flag? Experimental even?

Thanks for looking @alexsnaps! I think experimental makes a lot of sense! It's currently opt-in but making it explicitly experimental wfm since there is a lot of follow up work. WDYT @jordigilh? We should track something for how it ties back to TRLPolicy explicitly if there isn't already an issue.

Tagging in cc/ @jland-redhat @asaadbalum ty! Jamie already flagged the TRLPolicy CR somewhere. Ty!

[alexsnaps]

Moving this forward in experimental is perfectly fine with me, to be clear. But the concerns wrt backwards compatibility are a big one to me, let's make sure we're (BU?) are all aligned on this.

If we then move forward with this implementation, we need to create follow up work on a few things (don't want to slow a PoC down tho, hence the experimental proposal, but HA, clustered Redis/sharded redis-alikes, and failure scenarios need to be addressed and probably consolidated - this currently suffer from a few pathological "edge cases").

[jland-redhat]

Moving this forward in experimental is perfectly fine with me, to be clear. But the concerns wrt backwards compatibility are a big one to me, let's make sure we're (BU?) are all aligned on this.

If we then move forward with this implementation, we need to create follow up work on a few things (don't want to slow a PoC down tho, hence the experimental proposal, but HA, clustered Redis/sharded redis-alikes, and failure scenarios need to be addressed and probably consolidated - this currently suffer from a few pathological "edge cases").

And just as a note from the MaaS and MaaSSubscription perspective at least. My hope is that we can make keep backwards with existing subscriptions that we can just translate differently on the backend. While also exposing this new functionality.

Basically an existing subscription should just be a Sliding Window config with 0 reserved tokens

[jordigilh]

Feature gate's in: token-rate-limit-filter Cargo feature, off by default. The PR itself is also labeled experimental, for reviewer visibility.

TRLPolicy relationship: Will open a spike shortly and link it here once filed.

HA/clustered Valkey: Will open a spike to enumerate the failure modes before committing to a design. Will link here too.

[praxis-bot]

PR Re-Review

Summary: Previous Large findings (TCP connection churn, phantom doc fields) have been partially addressed. The Valkey connection is now cached correctly. However, the documentation still has a critical gap: the algorithm field (required discriminator for sliding_window vs token_bucket) is completely absent from the configuration reference table.

Severity	Count
Critical	0
Large	1
Medium	0

[jordigilh]

Force pushed after rebasing and removed all commits that were not part of this PR. All comments were already addressed prior to the force push, but feel free to re-review again at your discretion.