Repository navigation
fix(policies): honor the body rid over the header under manual and consistent_hashing - #2517
Conversation
…nsistent_hashing With --routing-key-override, the rid-derived session key is documented to outrank the routing-key header on every policy. PolicyRegistry::select_worker skips the sticky override for manual and consistent_hashing because they key on the header themselves, but neither consulted info.rid_key, so under those policies a request with header key_a and body rid key_b landed on key_a's worker. Both policies now read info.rid_key before the header. The registry populates rid_key only when the override is enabled (already capped and lineage stripped), so behavior without the flag is unchanged. PD legs namespace the rid key exactly like header keys. Closes smg-project#2480 Signed-off-by: ishan <ishanvgf@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughManual and consistent hashing policies now prefer ChangesRID-aware policy routing
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to RID-aware routing now consistently pins requests using the body-derived key when enabled, while retaining the documented fallback behavior. No current merge-blocking risk was identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Description
Problem
With
--routing-key-override, a request whose body carries aridis documented to pin by the rid-derived key, with the routing-key header as the fallback, on any policy. Undermanualandconsistent_hashingthe bodyridwas ignored and the header alone decided the worker.PolicyRegistry::select_workerdeliberately skips the sticky override for those two policies (routing_key_override_applies) because they key on the header themselves, but neitherManualPolicy::select_worker_implnorConsistentHashingPolicy::select_worker_implconsultedinfo.rid_key. The gRPC and HTTP pipelines already deriverid_keyfor every request under the override, so the key was available and simply not read.Solution
Both policies now resolve their key as
info.rid_keyfirst, then the header (consistent_hashingkeepsinfo.routing_keyahead of the raw header as before).PolicyRegistry::derive_rid_keyreturnsNoneunless the override is enabled, and the key it returns is already capped and lineage-stripped, so:--routing-key-overridenothing changes;manual/consistent_hashingpin on the same effective key the sticky override uses for every other policy, and the same key the routers use for keyed-load accounting (WorkerLoadGuard::with_key).PD legs namespace the rid key exactly like header keys, so prefill and decode still stick independently.
Changes
model_gateway/src/policies/manual.rs: consultinfo.rid_keybeforeX-SMG-Routing-Key; module doc lists both key sources.model_gateway/src/policies/consistent_hashing.rs:rid_key→routing_keyhint → raw header.model_gateway/src/policies/registry.rs: doc comments onselect_worker/routing_key_override_appliesdescribe the rid-first contract the key-native policies now uphold.manual::tests::test_manual_rid_key_outranks_header_key— rotating header keys with one rid land on one worker; the pin is stored under the rid key.manual::tests::test_manual_rid_key_namespaces_per_leg— same rid under prefill/decode gets independent entries.consistent_hashing::tests::test_rid_key_outranks_routing_key_hint— rid wins over a validated header hint that hashes to a different worker.registry::tests::rid_key_outranks_header_under_key_native_policies— end-to-end throughPolicyRegistry::with_overridefor bothPolicyConfig::ManualandPolicyConfig::ConsistentHashing, usingderive_rid_key("conv_tN").Test Plan
All four new tests were written first and failed against
mainfor the expected reason (rid ignored, header decided), e.g.:After the change:
Manual repro from the issue (two workers,
smg --policy manual --routing-key-override): pinkey_aandkey_bto different workers viax-smg-routing-key, then send-H 'x-smg-routing-key: key_a'with body"rid": "key_b"— now lands onkey_b's worker.Once #2462 lands,
TestRoutingKeyPinning::test_body_rid_outranks_header_keycan run undermanualagain.Closes #2480
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspasses