fix(proxy): streaming chat commits actual tokens at end-of-stream - #124
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughFixed streaming chat completions to correctly enforce token-based rate limits by adding a post-stream callback that reports observed token usage to the rate limiter. The ChangesStreaming Token Accounting for Rate Limiting (Issue
🎯 3 (Moderate) | ⏱️ ~20 minutes Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes a rate-limiting bug in the streaming chat path where TPM/TPD were never charged with actual token usage, allowing streaming traffic to bypass token caps (issue #108).
Changes:
- Added
Limiter::add_tokens_post_stream()to retroactively account for TPM/TPD once a streaming response completes. - Updated streaming chat dispatch to drop the
Reservationand instead post-commit token usage via an end-of-stream callback. - Added unit + end-to-end regression tests to ensure streaming token usage is accounted and TPM caps affect subsequent requests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/aisix-ratelimit/src/limiter.rs | Adds post-stream token accounting API and limiter-level regression tests for streaming TPM enforcement. |
| crates/aisix-proxy/src/chat.rs | Wires streaming SSE completion to limiter post-accounting via build_sse_stream(..., on_complete). |
| crates/aisix-proxy/src/lib.rs | Adds end-to-end regression test ensuring streaming usage affects next request’s TPM enforcement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Stream done — account for the upstream's reported tokens. | ||
| // For providers that don't emit usage in the stream this stays | ||
| // 0; on_complete is responsible for treating 0 as "no signal". | ||
| on_complete(total_tokens); | ||
| yield Ok::<_, Infallible>(Event::default().data("[DONE]")); |
| // Drop the reservation now: concurrency releases (the SSE | ||
| // stream that follows is driven by the client, not by the | ||
| // proxy holding open an upstream-bound future), and RPM was | ||
| // already counted by pre_commit. TPM is updated retroactively | ||
| // on stream-end by `add_tokens_post_stream` — see issue #108. | ||
| // Pre-fix this path called commit_tokens(0) and never came | ||
| // back, leaving TPM caps blind for all streaming traffic. | ||
| drop(reservation); |
Pre-fix the streaming chat path called reservation.commit_tokens(0)
right after kicking off the upstream stream and never came back to
account for what actually flowed through. TPM caps were silently
bypassed for all streaming traffic; cost telemetry reported $0 for
the majority of LLM traffic in production. The non-streaming path
was correct (commit_tokens(total)); the cache-hit path was correct
(0 by design); only the streaming success path was broken.
Fix:
* aisix-ratelimit: new Limiter::add_tokens_post_stream(key, n).
Bumps TPM/TPD without going through a Reservation, no-op on
n==0 so handlers don't lazily-create empty per-key state for
keys that streamed and got nothing back.
* aisix-proxy/src/chat.rs:
- build_sse_stream now takes an `on_complete: FnOnce(u64)`
callback. The stream tracks the largest `total_tokens` seen
on any chunk's `usage` block (providers typically populate
this on the terminal chunk only — OpenAI when
`stream_options.include_usage=true`, Anthropic on the
message_delta carrying output_tokens). At end-of-stream the
callback fires with the accumulated total.
- dispatch's streaming branch now drops the reservation up
front (concurrency releases, RPM was already counted by
pre_commit) and hands the stream a closure that calls
state.limiter.add_tokens_post_stream on completion.
Tests:
* aisix-ratelimit:
- add_tokens_post_stream_increments_tpm — pre_commit + drop
leaves TPM at 0; the post-stream call brings it up.
- add_tokens_post_stream_zero_is_a_noop — confirms unknown
keys don't get an empty state on a 0-token stream.
- streaming_path_tpm_cap_blocks_next_request_after_post_stream_commit
— drives the exploit shape at the limiter level.
* aisix-proxy:
- streaming_chat_tpm_cap_enforced_after_post_stream_commit_issue_108
— wiremock SSE upstream emits a terminal chunk with
`usage.total_tokens=1500`, api_key has tpm=1000. First
streaming request is 200; second 429 (TPM over-shot from
the first). Pre-fix this would have been 200/200 — the
cap was bypassed.
Closes #108
f71e966 to
1a84850
Compare
Summary
The streaming chat path called
reservation.commit_tokens(0)rightafter kicking off the upstream stream and never came back to
account for what actually flowed through. Net effect:
\$0for the majority of LLM trafficin production.
The non-streaming path was correct (
commit_tokens(total)); thecache-hit path was correct (
0by design — no upstream tokensconsumed); only the streaming success path was broken. Two of three
paths in the same function got it right.
Fix
aisix-ratelimit— newLimiter::add_tokens_post_stream(key, n).Bumps TPM/TPD without going through a
Reservation. No-op onn == 0so handlers don't lazily-create empty per-key state forkeys that streamed and got nothing back.
aisix-proxy/src/chat.rs—build_sse_streamnow takes anon_complete: FnOnce(u64)callback. The stream tracks the largest
total_tokensseen onany chunk's
usageblock (providers typically populate this onthe terminal chunk only — OpenAI when
stream_options.include_usage=true, Anthropic on themessage_deltacarryingoutput_tokens). At end-of-stream thecallback fires with the accumulated total.
dispatch's streaming branch drops the reservation up front(concurrency releases; RPM was already counted by
pre_commit)and hands the stream a closure that calls
state.limiter.add_tokens_post_streamon completion.Test plan
Limiter unit tests
add_tokens_post_stream_increments_tpm—pre_commit+ dropleaves TPM at 0; the post-stream call brings it up.
add_tokens_post_stream_zero_is_a_noop— unknown keys don'tget empty state on a 0-token stream.
streaming_path_tpm_cap_blocks_next_request_after_post_stream_commit— drives the exploit shape at the limiter level.
End-to-end proxy regression
streaming_chat_tpm_cap_enforced_after_post_stream_commit_issue_108— wiremock SSE upstream emits a terminal chunk with
usage.total_tokens = 1500. API keytpm = 1000. Firststreaming request → 200. Second → 429 (TPM over-shot from
the first). Pre-fix this was 200/200 and the cap was bypassed.
Tooling
cargo build --workspaceclean.cargo test --workspaceall green (no regressions; 129 inproxy, 24 in ratelimit).
cargo clippy -p aisix-proxy -p aisix-ratelimit -- -D warningsclean.
cargo fmt --all -- --checkclean.Notes
For providers that don't emit
usagein the stream,total_tokensstays 0 and
add_tokens_post_streamis a no-op (no information lost,no over-counting). Wiring the proxy to set
stream_options.include_usage=trueon outbound OpenAI streams is asmall follow-up; today the bridge already parses
usageoff anychunk that carries it, which is enough to validate the fix path
end-to-end.
Closes #108
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests