Repository navigation
fix(router): buffer bodies when routing-key override is enabled #2355
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 |
|---|---|---|
|
|
@@ -1650,6 +1650,7 @@ impl Router { | |
| .worker_registry | ||
| .get_routing_pool(crate::worker::UNKNOWN_MODEL_ID, RoutingPool::HttpRegular); | ||
| decide_body_path(&BodyPathInputs { | ||
| routing_key_override: self.policy_registry.routing_key_override_enabled(), | ||
|
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. 🟡 Nit: All fn request_body_path(&self, headers: &HeaderMap, wasm_request_hooks: bool) -> BodyPath {
if self.policy_registry.routing_key_override_enabled() {
return BodyPath::Buffer(REASON_ROUTING_KEY_OVERRIDE);
}
...(with |
||
| policy_needs_text: self | ||
| .policy_registry | ||
| .any_policy_needs_request_text(Some(headers)), | ||
|
|
@@ -1688,8 +1689,8 @@ impl Router { | |
| let model_id = crate::worker::UNKNOWN_MODEL_ID; | ||
| // Buffered-path parity: a valid tokens hint is exactly what selection | ||
| // would have received there (text is never extracted alongside it). | ||
| // Streamed requests have no readable body, hence no rid key; the | ||
| // sticky override keys them by the header alone. | ||
| // Streamed requests have no readable body, hence no rid key; | ||
| // routing-key override is excluded by the body-path gate above. | ||
| let hinted_tokens = header_utils::parse_routing_tokens_hint(Some(req.headers())); | ||
| let Some(worker) = self.select_worker_for_model( | ||
| model_id, | ||
|
|
@@ -2167,7 +2168,7 @@ mod tests { | |
| routers::common::{ | ||
| body_policy::{ | ||
| REASON_NO_CONTENT_LENGTH, REASON_POLICY_NEEDS_TEXT, REASON_RETRYABLE, | ||
| REASON_RETRY_FORFEITED, REASON_WASM_REQUEST_HOOK, | ||
| REASON_RETRY_FORFEITED, REASON_ROUTING_KEY_OVERRIDE, REASON_WASM_REQUEST_HOOK, | ||
| }, | ||
| request_lease::test_probe::{spawn_release_gated_stub, DropProbeRequest}, | ||
| }, | ||
|
|
@@ -3038,61 +3039,35 @@ mod tests { | |
| } | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn key_hint_streams_under_cache_aware_only_with_override() { | ||
| // Without the sticky override nothing consumes the key content-blind. | ||
| let router = streaming_router( | ||
| #[test] | ||
| fn routing_key_override_always_decides_buffer() { | ||
| let cache_aware = streaming_router_with_key_override( | ||
| cache_aware_policy(), | ||
| 1024 * 1024, | ||
| vec![plain_worker("http://worker1:8080")], | ||
| ); | ||
| let req = with_header( | ||
| streamed_request(&[b"{\"text\":\"hello\"}"]), | ||
| "x-smg-routing-key", | ||
| "media-1", | ||
| ); | ||
| assert!(router | ||
| .route_streaming_request(req, "/generate", false) | ||
| .await | ||
| .is_err()); | ||
|
|
||
| let (url_a, _cap_a) = spawn_capture_stub("application/json", "{}").await; | ||
| let (url_b, _cap_b) = spawn_capture_stub("application/json", "{}").await; | ||
| let router = streaming_router_with_key_override( | ||
| cache_aware_policy(), | ||
| let no_hint = headers_with_content_length(Some("64")); | ||
| let mut key_hint = no_hint.clone(); | ||
| key_hint.insert("x-smg-routing-key", "request-unique-key".parse().unwrap()); | ||
| let mut tokens_hint = no_hint.clone(); | ||
| tokens_hint.insert("x-smg-routing-tokens", "1,2,3".parse().unwrap()); | ||
| for headers in [&no_hint, &key_hint, &tokens_hint] { | ||
| assert_eq!( | ||
| cache_aware.request_body_path(headers, false), | ||
| BodyPath::Buffer(REASON_ROUTING_KEY_OVERRIDE) | ||
| ); | ||
| } | ||
|
|
||
| let text_free = streaming_router_with_key_override( | ||
| least_load_policy(), | ||
| 1024 * 1024, | ||
| vec![plain_worker(&url_a), plain_worker(&url_b)], | ||
| vec![plain_worker("http://worker1:8080")], | ||
| ); | ||
| let first = routed_worker_id( | ||
| &router, | ||
| with_header( | ||
| streamed_request(&[b"{\"text\":\"hello\"}"]), | ||
| "x-smg-routing-key", | ||
| "media-1", | ||
| ), | ||
| ) | ||
| .await; | ||
| let second = routed_worker_id( | ||
| &router, | ||
| with_header( | ||
| streamed_request(&[b"{\"text\":\"other\"}"]), | ||
| "x-smg-routing-key", | ||
| "media-1", | ||
| ), | ||
| ) | ||
| .await; | ||
| assert_eq!(first, second, "keyed requests must stick to one worker"); | ||
|
|
||
| // Over-cap keys are ignored by the same extractor selection uses. | ||
| let req = with_header( | ||
| streamed_request(&[b"{\"text\":\"hello\"}"]), | ||
| "x-smg-routing-key", | ||
| &"k".repeat(129), | ||
| assert_eq!( | ||
| text_free.request_body_path(&no_hint, false), | ||
| BodyPath::Buffer(REASON_ROUTING_KEY_OVERRIDE) | ||
| ); | ||
| assert!(router | ||
| .route_streaming_request(req, "/generate", false) | ||
| .await | ||
| .is_err()); | ||
| } | ||
|
|
||
| /// With retries disabled the parsed request must be freed at dispatch: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -348,7 +348,10 @@ async fn build_test_app_context( | |
|
|
||
| // Initialize registries | ||
| let worker_registry = Arc::new(WorkerRegistry::new()); | ||
| let policy_registry = Arc::new(PolicyRegistry::new(config.policy.clone())); | ||
| let policy_registry = Arc::new(PolicyRegistry::with_override( | ||
| config.policy.clone(), | ||
| config.routing_key_override.clone(), | ||
| )); | ||
|
Comment on lines
+351
to
+354
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. 🟡 Nit: Good fix, but the same gap remains in the sibling harness: |
||
|
|
||
| // Initialize storage backends (Memory for tests). | ||
| let response_storage = Arc::new(MemoryResponseStorage::new()); | ||
|
|
||
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.
🟡 Nit: This accessor reports "the override may read body
rid" purely fromenabled, butselect_workeronly consults the sticky map whenrouting_key_override_applies(policy.name())is true — i.e. never formanual/consistent_hashing(line 346). With--routing-key-override+--policy manual, the override is a no-op for selection, yet every request now takes the buffered path for aridnothing will consume.any_policy_needs_request_textalready handles this asymmetry by foldingrouting_key_override_appliesinto its per-policy scan; mirroring it here (override enabled and at least one registered policy the override can apply to, since the model — hence the policy — is inside the unread body) would keep those deployments streamable.Related: because
decide_body_pathnow checksrouting_key_overridefirst, thekeyed_overridewaiver at line 735 can only be true in exactly the case where the result is discarded (the sole production caller isRouter::request_body_path). Its doc comment — "a valid routing-key header under the sticky override supersedes the policy before it reads text" — describes a streamed outcome that can no longer happen, so it's worth updating or dropping alongside the branch.