bug: Output token rate limiting can be bypassed and may need a reservation-based design

[nXtCyberNet]

The current output token rate limiting in pkg/kthena-router/filters/ratelimit/ratelimit.go relies on allowN, which does not permit the bucket state to become negative. As a result, output token consumption cannot be properly accounted for, allowing requests to effectively bypass the limiter and consume unlimited output tokens.

This impacts both token accounting accuracy and the rate limiter's primary purpose of protecting backend services from overload.

with the Proposal #1225 and PR #1265 we are already moving toward more accurate token accounting, while through PR #1014 i was trying to addresses preserving limiter state across reconciliation events. However, supporting accurate post-request reconciliation within the current implementation appears challenging, as it would likely require a token reservation/reconciliation mechanism rather than incremental fixes.

Would accurate enforcement of output-token limits be considered a requirement for the router? If so, a refactor of pkg/kthena-router/filters/ratelimit/ratelimit.go toward a reservation-based design may be cleaner than continuing to patch the existing implementation.

wants to know your thoughts on it.

/cc @FAUST-BENCHOU @hzxuzhonghu @LiZhenCheng9527

[shivansh-gohem]

Great catch, @nXtCyberNet! You're completely right about the behavior of AllowN—if the requested tokenCount during post-request reconciliation exceeds the currently available tokens, it returns false and consumes zero tokens. This essentially gives large requests a free pass and defeats the purpose of the output rate limiter.

Accurate enforcement of output tokens is definitely a core requirement for the router, especially for protecting backend capacity and ensuring fair multi-tenant usage.

I completely agree that patching the current AllowN-based logic is a dead end. Moving towards a reservation-based design is the cleanest and most robust path forward. Ideally, we would reserve an estimated amount upfront (e.g., based on the request's max_tokens or a configured default) and then reconcile/refund the difference once generation completes.

[nXtCyberNet]

Hi @hzxuzhonghu, I have one more question about the current rate limiter. What's the intended scope of the rate limiting?

Is it meant to control the overall throughput of a model (i.e., all requests to a model share the same quota), or is it intended to enforce per-user/per-client throughput limits?

I just want to know whats your future plan was

[hzxuzhonghu]

@nXtCyberNet Is this a umbrella issue, what's the relationship of those prs?

[nXtCyberNet]

@hzxuzhonghu Thanks. To clarify, PRs #1265 and #1014 were just my trial attempts around solving the token accuracy issue, not separate asks — both are smaller parts of the same accuracy effort.
#1265 is for the tokenizer accuracy
#1014 is reconciliation in rate limiter
And current issue - inaccuracy in output tokens

But I ran into a bigger question first: is the rate limiter meant to be model-based or user-based? Everything else — including how I approach the accuracy fix — depends on that answer.

[nXtCyberNet]

@hzxuzhonghu i will start working on this - based on the comment got to know about the future plans

Another requiremnent is do ratelimte based on user identity

Originally posted by @hzxuzhonghu in #283

[hzxuzhonghu]

For many commercial products, users can subscribe a plan, which provide a quota of tokens per month or other interval.

[hzxuzhonghu]

The current output token rate limiting in pkg/kthena-router/filters/ratelimit/ratelimit.go relies on allowN, which does not permit the bucket state to become negative. As a result, output token consumption cannot be properly accounted for, allowing requests to effectively bypass the limiter and consume unlimited output tokens.

I am not a fan of pusuing accuracy which introcuding unneccessary tricky. Cause tokens are relatively cheap right now, compared to the plan, vendors can stand the wide tolereance. And vendor donot have a 1 request concurrecncy per user, right? So it could not do strict rate limit by output tokens.

[nXtCyberNet]

Got it, thanks for the feedback. I understand that adding more complexity for accurate output token limiting isn’t worth it right now since tokens are cheap and some tolerance is fine.
Just one small question — you mentioned wanting rate limiting based on user identity in the future. Can I start working on that?
What exactly do you mean by it? Like quota-based (monthly tokens etc. for user plans), or similar to the current model-based rate limiting but per user for protecting the backend?