Repository navigation
fix(router): hash routing text when a request carries no tokens #2148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,8 +6,9 @@ | |
| //! | ||
| //! ## Algorithm | ||
| //! | ||
| //! 1. Extract first N tokens from the request (configurable prefix length) | ||
| //! 2. Hash the token sequence using xxhash for fast, stable hashing | ||
| //! 1. Extract first N tokens from the request (configurable prefix length), | ||
| //! or the equivalent span of routing text when the request is untokenized | ||
| //! 2. Hash the prefix using xxhash for fast, stable hashing | ||
| //! 3. Use consistent hash ring to find the target worker | ||
| //! 4. If worker is overloaded (load > avg * load_factor), find least loaded | ||
| //! 5. Return least loaded worker that passes load check, or initial if all overloaded | ||
|
|
@@ -37,7 +38,8 @@ use crate::{observability::metrics::Metrics, worker::Worker}; | |
| /// Configuration for the PrefixHash load balancing policy | ||
| #[derive(Debug, Clone)] | ||
| pub struct PrefixHashConfig { | ||
| /// Number of prefix tokens to use for hashing. | ||
| /// Number of prefix tokens to use for hashing, or `CHARS_PER_TOKEN` times | ||
| /// as many characters when the request carries no tokens. | ||
| /// Longer prefixes = more precise routing but less grouping. | ||
| /// Shorter prefixes = more requests grouped together. | ||
| /// Default: 256 tokens (~1 paragraph of text) | ||
|
|
@@ -63,7 +65,7 @@ impl Default for PrefixHashConfig { | |
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| enum Branch { | ||
| NoHealthyWorkers, | ||
| NoTokens, | ||
| NoRoutingKey, | ||
| RingHit, | ||
| LoadBalanceWalk, | ||
| FallbackLeastLoad, | ||
|
|
@@ -74,14 +76,19 @@ impl Branch { | |
| const fn as_str(self) -> &'static str { | ||
| match self { | ||
| Self::NoHealthyWorkers => "no_healthy_workers", | ||
| Self::NoTokens => "no_tokens", | ||
| Self::NoRoutingKey => "no_routing_key", | ||
| Self::RingHit => "ring_hit", | ||
| Self::LoadBalanceWalk => "load_balance_walk", | ||
| Self::FallbackLeastLoad => "fallback_least_load", | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Characters of routing text treated as one token's worth of prefix. Four is | ||
| /// the usual rule of thumb for English text, so an untokenized request hashes | ||
| /// roughly the same span of the prompt as `prefix_token_count` would cover. | ||
| const CHARS_PER_TOKEN: usize = 4; | ||
|
|
||
| /// Prefix Hash load balancing policy | ||
| /// | ||
| /// Routes requests based on prefix token hash for KV cache locality. | ||
|
|
@@ -112,6 +119,36 @@ impl PrefixHashPolicy { | |
| xxhash_rust::xxh3::xxh3_64(bytes) | ||
| } | ||
|
|
||
| /// Compute hash of the leading text of an untokenized request | ||
| /// | ||
| /// Only pre-tokenized requests carry token IDs; chat, completions and | ||
| /// text-form generate requests reach the policy with routing text alone, | ||
| /// and hashing it keeps them on a stable worker instead of leaving them | ||
| /// unroutable. | ||
| #[inline] | ||
| fn compute_text_prefix_hash(&self, text: &str) -> u64 { | ||
| let budget = self | ||
| .config | ||
| .prefix_token_count | ||
| .saturating_mul(CHARS_PER_TOKEN); | ||
| let end = text | ||
| .char_indices() | ||
| .nth(budget) | ||
| .map_or(text.len(), |(offset, _)| offset); | ||
|
|
||
| xxhash_rust::xxh3::xxh3_64(&text.as_bytes()[..end]) | ||
| } | ||
|
|
||
| /// Index of the least loaded healthy worker | ||
| fn least_loaded_healthy(workers: &[Arc<dyn Worker>]) -> Option<usize> { | ||
| workers | ||
| .iter() | ||
| .enumerate() | ||
| .filter(|(_, w)| w.is_healthy()) | ||
| .min_by_key(|(_, w)| w.load()) | ||
| .map(|(idx, _)| idx) | ||
| } | ||
|
|
||
| /// Check if a worker's load is acceptable | ||
| #[inline] | ||
| fn load_ok(&self, worker_load: usize, total_load: usize, num_workers: usize) -> bool { | ||
|
|
@@ -190,12 +227,10 @@ impl PrefixHashPolicy { | |
| } | ||
|
|
||
| // Fallback: no ring or ring lookup failed, use least loaded worker | ||
| let least_loaded = healthy_workers | ||
| .iter() | ||
| .min_by_key(|(_, w)| w.load()) | ||
| .map(|(idx, _)| *idx); | ||
|
|
||
| (least_loaded, Branch::FallbackLeastLoad) | ||
| ( | ||
| Self::least_loaded_healthy(workers), | ||
| Branch::FallbackLeastLoad, | ||
| ) | ||
| } | ||
|
|
||
| fn select_worker_impl( | ||
|
|
@@ -207,15 +242,15 @@ impl PrefixHashPolicy { | |
| return (None, Branch::NoHealthyWorkers); | ||
| } | ||
|
|
||
| // Get tokens from SelectWorkerInfo | ||
| let tokens = match info.tokens { | ||
| Some(t) if !t.is_empty() => t, | ||
| _ => return (None, Branch::NoTokens), | ||
| // Pre-tokenized requests hash their token prefix, the rest hash the | ||
| // equivalent span of routing text. | ||
| let prefix_hash = match (info.tokens, info.request_text) { | ||
| (Some(tokens), _) if !tokens.is_empty() => self.compute_prefix_hash(tokens), | ||
| (_, Some(text)) if !text.is_empty() => self.compute_text_prefix_hash(text), | ||
| // Nothing to hash: stay serviceable by falling back to load. | ||
| _ => return (Self::least_loaded_healthy(workers), Branch::NoRoutingKey), | ||
| }; | ||
|
|
||
| // Compute prefix hash | ||
| let prefix_hash = self.compute_prefix_hash(tokens); | ||
|
|
||
| // Find worker using ring with load balancing | ||
| self.find_worker_with_load_balance(workers, info, prefix_hash) | ||
| } | ||
|
|
@@ -242,7 +277,7 @@ mod tests { | |
| use openai_protocol::worker::{HealthCheckConfig, WorkerStatus}; | ||
|
|
||
| use super::*; | ||
| use crate::worker::{BasicWorkerBuilder, HashRing, WorkerType}; | ||
| use crate::worker::{BasicWorkerBuilder, HashRing, WorkerLoadGuard, WorkerType}; | ||
|
|
||
| fn create_workers(urls: &[&str]) -> Vec<Arc<dyn Worker>> { | ||
| urls.iter() | ||
|
|
@@ -342,33 +377,133 @@ mod tests { | |
| } | ||
|
|
||
| #[test] | ||
| fn test_no_tokens_returns_none() { | ||
| fn test_untokenized_request_routes_consistently() { | ||
| let policy = PrefixHashPolicy::with_defaults(); | ||
| let workers = create_workers(&["http://w1:8000"]); | ||
| let workers = create_workers(&["http://w1:8000", "http://w2:8000", "http://w3:8000"]); | ||
| let ring = Arc::new(HashRing::new(workers.iter().map(|w| w.url()))); | ||
|
|
||
| // Chat and completions requests reach the policy with text only. | ||
| let info = SelectWorkerInfo { | ||
| tokens: None, | ||
| request_text: Some("summarize the quarterly earnings report"), | ||
| hash_ring: Some(ring), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let (first_result, branch) = policy.select_worker_impl(&workers, &info); | ||
| assert!(first_result.is_some(), "text alone must be routable"); | ||
| assert_eq!(branch, Branch::RingHit); | ||
|
|
||
| for _ in 0..10 { | ||
| let (result, _) = policy.select_worker_impl(&workers, &info); | ||
| assert_eq!(result, first_result); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_shared_text_prefix_routes_same() { | ||
| let policy = PrefixHashPolicy::new(PrefixHashConfig { | ||
| prefix_token_count: 2, // 8 characters of text | ||
| ..Default::default() | ||
| }); | ||
| let workers = create_workers(&["http://w1:8000", "http://w2:8000", "http://w3:8000"]); | ||
| let ring = Arc::new(HashRing::new(workers.iter().map(|w| w.url()))); | ||
|
|
||
| let info1 = SelectWorkerInfo { | ||
| request_text: Some("system: you are a helpful assistant"), | ||
| hash_ring: Some(ring.clone()), | ||
| ..Default::default() | ||
| }; | ||
| let info2 = SelectWorkerInfo { | ||
| request_text: Some("system: you are a terse assistant"), | ||
| hash_ring: Some(ring), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let (result1, _) = policy.select_worker_impl(&workers, &info1); | ||
| let (result2, _) = policy.select_worker_impl(&workers, &info2); | ||
|
|
||
| assert_eq!( | ||
| result1, result2, | ||
| "text past the prefix budget must not change the worker" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_text_prefix_budget_respects_char_boundaries() { | ||
| let policy = PrefixHashPolicy::new(PrefixHashConfig { | ||
| prefix_token_count: 1, // 4 characters, mid-way through the emoji run | ||
| ..Default::default() | ||
| }); | ||
| let workers = create_workers(&["http://w1:8000", "http://w2:8000"]); | ||
| let ring = Arc::new(HashRing::new(workers.iter().map(|w| w.url()))); | ||
|
|
||
| // Empty tokens | ||
| let info = SelectWorkerInfo { | ||
| request_text: Some("🌊🌊🌊🌊🌊🌊 tide report"), | ||
| hash_ring: Some(ring), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let (result, branch) = policy.select_worker_impl(&workers, &info); | ||
| assert!(result.is_some()); | ||
| assert_eq!(branch, Branch::RingHit); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_tokens_take_priority_over_text() { | ||
| let policy = PrefixHashPolicy::with_defaults(); | ||
| let workers = create_workers(&["http://w1:8000", "http://w2:8000", "http://w3:8000"]); | ||
| let ring = Arc::new(HashRing::new(workers.iter().map(|w| w.url()))); | ||
|
|
||
| let tokens: Vec<u32> = vec![7, 8, 9]; | ||
| let tokens_only = SelectWorkerInfo { | ||
| tokens: Some(&tokens), | ||
| hash_ring: Some(ring.clone()), | ||
| ..Default::default() | ||
| }; | ||
| let tokens_and_text = SelectWorkerInfo { | ||
| tokens: Some(&tokens), | ||
| request_text: Some("text that must not influence the ring key"), | ||
| hash_ring: Some(ring), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let (result1, _) = policy.select_worker_impl(&workers, &tokens_only); | ||
| let (result2, _) = policy.select_worker_impl(&workers, &tokens_and_text); | ||
|
|
||
| assert_eq!(result1, result2); | ||
|
Comment on lines
+471
to
+474
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win 🟡 Nit: Make the token-precedence test distinguish the two routing keys. A text-first implementation can pass this assertion when the token-only and text-only hashes select the same worker. First assert that the selected token fixture and a text-only fixture route to different workers. Then assert that 🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| #[test] | ||
| fn test_no_routing_key_falls_back_to_load() { | ||
| let policy = PrefixHashPolicy::with_defaults(); | ||
| let workers = create_workers(&["http://w1:8000", "http://w2:8000"]); | ||
| let ring = Arc::new(HashRing::new(workers.iter().map(|w| w.url()))); | ||
| let _busy = WorkerLoadGuard::new(workers[0].clone(), None); | ||
|
|
||
| // Empty tokens, empty text | ||
| let tokens: Vec<u32> = vec![]; | ||
| let info = SelectWorkerInfo { | ||
| tokens: Some(&tokens), | ||
| request_text: Some(""), | ||
| hash_ring: Some(ring.clone()), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let (result, branch) = policy.select_worker_impl(&workers, &info); | ||
| assert_eq!(result, None); | ||
| assert_eq!(branch, Branch::NoTokens); | ||
| assert_eq!(result, Some(1), "a keyless request still has to be served"); | ||
| assert_eq!(branch, Branch::NoRoutingKey); | ||
|
|
||
| // No tokens field | ||
| let info_no_tokens = SelectWorkerInfo { | ||
| // Neither field set | ||
| let info_empty = SelectWorkerInfo { | ||
| tokens: None, | ||
| hash_ring: Some(ring), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let (result2, branch2) = policy.select_worker_impl(&workers, &info_no_tokens); | ||
| assert_eq!(result2, None); | ||
| assert_eq!(branch2, Branch::NoTokens); | ||
| let (result2, branch2) = policy.select_worker_impl(&workers, &info_empty); | ||
| assert_eq!(result2, Some(1)); | ||
| assert_eq!(branch2, Branch::NoRoutingKey); | ||
| } | ||
|
|
||
| #[test] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Important:
PrefixHashPolicynow routes oninfo.request_text, but theLoadBalancingPolicyimpl (line 259) doesn't overrideneeds_request_text()— it inherits the defaultfalse.The regular HTTP router and gRPC router happen to populate
request_textunconditionally (when tokens are absent), so this works there. But the PD router (pd_router.rs:804-807) gates text extraction onpolicies_need_request_text(), which checks the trait method. Withprefix_hashas a PD policy,request_textwill always beNoneand every untokenized request will land in theNoRoutingKeyfallback instead of hashing onto the ring.Fix: add
needs_request_textto theimpl LoadBalancingPolicyblock:(The actual fix belongs in the
impl LoadBalancingPolicyblock around line 266 — addfn needs_request_text(&self) -> bool { true }next toname().)