feat(grpc): wire tenant token rate limiting into gRPC router - #2016
Open
XinyueZhang369 wants to merge 6 commits into
Open
feat(grpc): wire tenant token rate limiting into gRPC router#2016XinyueZhang369 wants to merge 6 commits into
XinyueZhang369 wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Thorough review of the tenant rate limiting wiring into the gRPC router. The "guarded reserve-once" mechanism via RateLimitCell / RateLimitReserveStage is clean: reserves exactly once per logical request across retry attempts, settles with real backend-reported usage (not just the reserve estimate), and has proper RAII safety nets for every lifecycle edge (preemption/Drop, streaming handoff, non-success close, disconnect via ReservationAttachment).
Key correctness points verified:
n>1handling:max()(not sum) for shared prompt/cached tokens,sum()for per-choice completion tokens, and expected-choices guard before settling streaming usage- Harmony PD mode tracks
decode_completed_indicesseparately from prefill-populatedprompt_tokensto avoid masking a decode phase that never finished - CAS-guarded resolution on
SharedReservationHandleensures first-resolver-wins across all paths (inline settle, streaming settle,ReservationAttachmentabandon, cell Drop) - Retry loop correctly short-circuits on
Denied— never re-reserves or retries a rate-limit denial - Integration tests (real mock gRPC backend) prove settle_success runs with real usage by choosing budget numbers that distinguish reserve-only from reserve+settle
0 issues found.
XinyueZhang369
force-pushed
the
xz/grpc-rate-limit-reserve-guard
branch
from
August 1, 2026 01:43
f02da9a to
791d787
Compare
…essages Wires RateLimitManager::reserve/settle_success/close_reserved_only (Phase 1, merged via #1958/#1966/#1969 -- nothing called it yet) into the gRPC router, via a guarded reserve-once mechanism rather than hoisting preparation out of the retry loop. A prior attempt at this (splitting prep from dispatch into a PreparedRequest replayed across retry attempts) was implemented, reviewed, and reverted -- see branch xz/grpc-pipeline-prepare-execute-split-archived. It introduced real bugs (alias-handover mismatch, a multimodal double-clone) because prep and dispatch became independently-constructed states that had to be kept in sync by hand. This approach avoids that entirely: every execute_* method stays exactly as it is today (full prep + dispatch, fused, rerun independently per retry attempt), and a small RateLimitCell shared across attempts of one logical request tracks whether reserve() has already run. - New RateLimitReserveStage, inserted after preparation (before worker selection) in the chat/messages/completion/harmony stage lists. Checks the cell: already Admitted -> skip; already Denied -> reject (defensive, should_retry stops the loop first); otherwise reserve() using the exact token count preparation just produced. - router.rs's should_retry now also checks the cell for a cached Denied outcome and stops retrying immediately -- a rate-limit denial is not a transient worker failure, and retrying it defeats retry_after_secs. - Non-streaming success settles inline using the response's real usage. Streaming settles with real accumulated totals at each existing mark_completed() site in streaming.rs and harmony/streaming.rs (~10 sites across chat/generate/messages/completion, both regular and prefill-decode); ReservationAttachment is layered onto the response body's AttachedBody as a Drop-based safety net for early disconnect/error before that point is reached. - router.rs closes any still-open reservation after the retry loop for any non-2xx final response (repeated prep failure, exhausted retries) -- safe to call unconditionally since settle/close/abandon are all CAS-guarded to only the first resolution. Responses endpoint, embeddings, and classify are left unwired (no cell passed) for now, matching the original Phase 2 scope cut -- none of them use RetryExecutor today. Signed-off-by: XinyueZhang369 <zoeyzhang369@gmail.com>
Tier 2 of the tenant rate limiting test plan: verify the gRPC router's reserve/settle wiring against a real (mock) gRPC backend, rather than only at the RateLimitReserveStage unit level. - crates/mock_worker: add a [lib] target so its HTTP/gRPC simulators (previously private modules of the standalone binary) are importable as a dev-dependency. main.rs now depends on the lib instead of declaring its own modules; behavior unchanged. - model_gateway/tests/common/mod.rs: add create_test_context_with_tokenizer_registry, a variant of create_test_context that takes a caller-supplied tokenizer registry (needed since mock-worker's gRPC service deliberately has no tokenizer artifacts to autoload) and real parser factories (GrpcRouter::new requires both Some, unlike the HTTP-only default). The original create_test_context is now a thin wrapper, unchanged for existing callers. - app_context.rs: add AppContextBuilder::rate_limit_manager, a direct setter mirroring the existing rate_limiter() one -- the config-driven maybe_rate_limit_manager is private to this module, unreachable for test harnesses that build AppContext piecemeal instead of through from_config. - New test file spawns mock_worker::grpc::serve in-process against a picked port, registers it as a real gRPC worker, and drives RouterTrait::route_generate directly (mirroring routers/grpc/router.rs's own pd_tests module) rather than through the lightweight HTTP test app, which doesn't wire tenant-resolution middleware. Three tests: - denial_returns_429_with_retry_after: a budget too small for one reservation denies immediately, before ever reaching the worker. - non_streaming_settle_uses_real_usage_not_just_the_estimate: proves settle_success runs with the backend's real reported usage rather than just the reserve-time estimate. mock-worker's canned mode always reports prompt_tokens=1 regardless of real input length while completion_tokens is exactly the configured output_tokens; a budget sized between the reserve estimate and the true settled cost makes a second identical request's admit/deny outcome a direct, deterministic proof that settle used the backend's real numbers. - feature_disabled_is_a_no_op: baseline, no interference when tenant rate limiting isn't configured. Full verification gate green: cargo test -p smg --lib (1339 passed) + the new integration test (3 passed, stable across repeated runs), cargo +nightly fmt --all, cargo clippy -p smg --all-targets -D warnings, cargo clippy -p mock-worker --all-targets -D warnings, and cargo clippy --workspace --all-targets -D warnings all clean. Signed-off-by: XinyueZhang369 <zoeyzhang369@gmail.com>
…ias handover, and impossible-request Retry-After Four review findings on the tenant rate-limit reserve-guard wiring, all verified against current code before fixing: - [P1] Batched completions reserved only the first prompt's tokens. PreparationOutput::token_ids() is deliberately a single-item routing proxy (unchanged); RateLimitReserveStage now uses a new total_input_token_count(), which sums every item in a batched Completion request instead of delegating to the routing proxy. A batch with a small first prompt and large later prompts was passing admission without reserving their real input cost. - [P1] Preemption/cancellation leaked active reservations. The route future can be dropped (priority-scheduler preemption, or any other cancellation) before any response -- streaming or not -- is ever produced, at which point neither the inline non-streaming settle nor the streaming ReservationAttachment ever gets the chance to resolve the handle, and SharedReservationHandle has no cleanup-on-drop of its own. RateLimitCell now does: a Drop impl abandons any still-open reservation it holds, guarded by a new handed_off flag set only when a streaming response takes over via the new take_for_streaming_handoff (replaces the old peek-then-match in all 5 response_processing.rs call sites) -- without that flag, Drop would race and always win against the streaming path's later, correctly-timed real-usage settle, since route_*_impl returns the initial SSE response long before the stream itself finishes. - [P2] Retries could dispatch a different canonical model than the one reserved. Each attempt re-resolved the raw alias fresh (via RequestContext::new), while the cached reservation stayed bound to whichever model the first attempt resolved to. An alias repointed mid-retry could dispatch to model B while the reservation -- and its eventual settle -- stayed against model A's budget, bypassing model B's own policy entirely. Fixed by resolving the canonical model once, before the retry loop, in all four route_*_impl functions (new resolve_canonical_model_id), reusing the same value for both the reservation and every dispatch attempt. - [P2] An impossible request (estimated cost exceeding the tenant's total capacity) exposed the backend's u64::MAX "can never fit, no matter how long you wait" sentinel as a literal Retry-After header. rejection_response now omits the header for that sentinel too, matching the existing retry_after_secs > 0 omission. New tests: total_input_token_count_sums_every_batched_completion_item (context.rs); three RateLimitCell::drop tests using an in-crate counting fake backend (drop-without-handoff abandons, drop-after-handoff does not, drop-after-denial does not); rejection_response's u64::MAX case; two new end-to-end integration tests (impossible_request_denies_without_retry_after, and denial_returns_429_with_retry_after rewritten to a genuine finite-wait denial rather than an impossible one, which the Retry-After fix now correctly treats differently). Full verification gate green: cargo test -p smg --lib (1344 passed), the gRPC integration test (13 passed, stable across repeated runs), cargo +nightly fmt --all, cargo clippy -p smg --all-targets -D warnings, cargo clippy --workspace --all-targets -D warnings all clean. Signed-off-by: XinyueZhang369 <zoeyzhang369@gmail.com>
…and stop double-charging n>1 prompts - router.rs: rewrite the cloned request body's model field to the resolved canonical model_id, not just model_id_cloned itself -- RequestContext::new's own alias resolve no-ops on an already-canonical id, so without this the body (and therefore response metadata and parser selection) kept reporting the client's alias. - regular/streaming.rs, harmony/streaming.rs: a clean stream EOF that never produced an authoritative Complete message was settling with actual_input_tokens=0, incorrectly refunding the reservation's estimate. Track whether Complete was ever seen and close_reserved_only instead when it wasn't, across chat/generate/messages/completion (PD variants delegate to the same functions) and Harmony. - response_formatting.rs: build_usage summed prompt_tokens across every n>1 choice instead of taking the max, double- (or n-times-) charging the one prompt those choices share. Completion's own usage computation already does this correctly; mirrored here for Chat/Harmony. Signed-off-by: XinyueZhang369 <zoeyzhang369@gmail.com>
…ing usage - regular/streaming.rs, harmony/streaming.rs: a clean EOF partway through an n>1 stream (some choices finished, others didn't) was treated as fully authoritative usage as long as at least one Complete had arrived, understating actual_input_tokens/completion_tokens instead of closing as no-usage. Track completed choices explicitly (a HashSet of indices, or an existing per-index map's length where that map is only ever populated from Complete) and require it to cover every expected choice before settling; close_reserved_only otherwise. - harmony/streaming.rs: in PD mode the prefill phase pre-populates the same prompt_tokens map the decode phase later reads, so checking that map alone could treat a prefill-only Complete as proof decode finished when it never did. Track decode's own Complete messages separately. - regular/streaming.rs, harmony/streaming.rs: streaming settle (and, for chat, the client-visible usage SSE chunk) still summed prompt_tokens across n>1 choices instead of taking the max, double-charging the one prompt they share -- same bug already fixed for the non-streaming path in the previous commit, now applied to streaming too. Signed-off-by: XinyueZhang369 <zoeyzhang369@gmail.com>
cached_tokens is a property of the shared prompt (how much of it hit the KV cache), same as prompt_tokens, but was still summed across every n>1 choice instead of taking the max -- same bug as prompt_tokens, just missed in the earlier fix. Fixed in build_usage (non-streaming Chat/Harmony), regular chat streaming's usage chunk, and Harmony's decode settle. Completion was already correct (its own per-prompt max already covered cached_tokens). Signed-off-by: XinyueZhang369 <zoeyzhang369@gmail.com>
XinyueZhang369
force-pushed
the
xz/grpc-rate-limit-reserve-guard
branch
from
August 1, 2026 02:25
791d787 to
bfe9a8a
Compare
XinyueZhang369
marked this pull request as ready for review
August 1, 2026 03:01
XinyueZhang369
requested review from
CatherineSue,
key4ng and
slin1237
as code owners
August 1, 2026 03:01
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.
Description
Problem
RateLimitManager::reserve()/settle_success()/close_reserved_only()(Phase 1, already merged onmain) aren't called anywhere yet — nothing enforces tenant token-per-minute limits on the gRPC router's chat/generate/completion/messages endpoints.Solution
Wire tenant rate limiting into the gRPC pipeline with a "guarded reserve-once" mechanism: a small
RateLimitCellshared across all retry attempts of one logical request, checked by a newRateLimitReserveStageinserted right after preparation. The first attempt to reach the stage reserves (or is denied) and caches the outcome; every later retry attempt sees the cached outcome and skips straight through.RetryExecutorreruns the full pipeline per attempt unchanged — no prepare/dispatch split, no independently-constructed state that has to be kept in sync by hand.Changes
RateLimitCell/RateLimitOutcome/RateLimitReserveStage(new,routers/grpc/common/stages/rate_limit.rs), inserted into the Chat/Messages/Completion/Harmony pipelines right after preparation.router.rs: eachroute_*_implconstructs the cell, canonicalizes the model once before the retry loop (reused for both the reservation and every dispatch attempt, including the request body's ownmodelfield), stops retrying on a cached denial, and closes any reservation a non-2xx final response never got to settle.reservation: Option<Arc<SharedReservationHandle>>parameter threaded throughregular/streaming.rsandharmony/streaming.rs, with aReservationAttachmentlayered ontoAttachedBodyas the Drop-based safety net for disconnect/preemption before that point.crates/mock_workergained a[lib]target so its real (mock) gRPC server is spawnable in-process frommodel_gateway/tests/, enabling real end-to-end integration coverage (tests/tenant_rate_limiting_grpc_test.rs) rather than only unit tests against the pipeline stage in isolation.Retry-After: 18446744073709551615for impossible requests.Completeframe was settling with 0 input tokens instead of keeping the reservation;n>1requests were charging the shared prompt once per choice instead of once per request.n>1stream (some choices finished, others didn't) was accepted as fully authoritative instead of requiring every expected choice'sComplete; Harmony PD mode's prefill-populated map could mask a decode phase that never actually finished.cached_tokenshad the same per-choice multiplication bug asprompt_tokens.RetryExecutortoday); OpenAI/Anthropic/Gemini routers (Phase 3).Test Plan
model_gateway/tests/tenant_rate_limiting_grpc_test.rs), driven against a real (mock) gRPC backend: a finite-wait 429 withRetry-After, an impossible-request 429 withoutRetry-After, non-streaming settle using the backend's real reported usage (not just the reserve-time estimate), and the feature-disabled no-op path.RateLimitCell::dropsafety-net behavior (abandons on preemption, doesn't double-resolve after streaming handoff or denial),total_input_token_countbatching, theu64::MAXRetry-Aftersentinel, andbuild_usage's per-choiceprompt_tokens/cached_tokensmax-not-sum behavior.Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspasses