fix(kv-router): scope KvRouter cancellation to a child token - #11455
Conversation
Signed-off-by: davilu-nvidia <134493379+davilu-nvidia@users.noreply.github.com> Signed-off-by: PeaBrane <yanrpei@gmail.com> Co-authored-by: PeaBrane <yanrpei@gmail.com> Signed-off-by: Dan Gil <dagil@nvidia.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b6b04abbd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let component = endpoint.component(); | ||
| let cancellation_token = component.drt().primary_token(); | ||
| // Router-owned tasks derive from this token so a rebuild cannot cancel the runtime. | ||
| let cancellation_token = component.drt().child_token(); |
There was a problem hiding this comment.
Keep router tasks alive through graceful shutdown
When Runtime::shutdown() is called while a KV-routed endpoint still has in-flight or queued requests, this token is cancelled in shutdown Phase 1 because Runtime::child_token() is a child of endpoint_shutdown_token, and Phase 2 only waits for graceful endpoints afterward (lib/runtime/src/runtime.rs). That tears down the scheduler/indexer/sequence tasks before the active requests finish, regressing graceful shutdown behavior; the mocker explicitly avoids this by deriving from primary_token() so it only stops in Phase 3. Use an independently cancellable child of the primary token (or equivalent) so Drop can stop this router without cancelling the runtime, while runtime shutdown keeps router support alive until Phase 3.
Useful? React with 👍 / 👎.
| kv_router_config: &KvRouterConfig, | ||
| block_size: u32, | ||
| model_name: Option<&str>, | ||
| cancellation_token: CancellationToken, |
There was a problem hiding this comment.
Adding cancellation_token to public Indexer::new breaks downstream dynamo-llm callers. Fix: keep the old public constructor and move the token-taking implementation behind a new crate-private constructor used by KvRouter.
🤖 AI Fix
In lib/llm/src/kv_router/indexer/mod.rs, restore pub async fn Indexer::new(component, kv_router_config, block_size, model_name) with the old signature and make it call a new pub(crate) async fn new_with_cancellation_token(..., cancellation_token: CancellationToken) containing the current implementation; update lib/llm/src/kv_router.rs KvRouter::new to call Indexer::new_with_cancellation_token.
| overloaded_worker_provider: Option<OverloadedWorkerProvider>, | ||
| model_name: Option<&str>, | ||
| worker_type: &'static str, | ||
| cancellation_token: CancellationToken, |
There was a problem hiding this comment.
Adding cancellation_token to public KvScheduler::start breaks downstream Rust callers. Fix: preserve the old public start signature and add a token-aware internal start helper for router-owned scoped cancellation.
🤖 AI Fix
In lib/llm/src/kv_router/scheduler.rs, restore pub async fn KvScheduler::start(...) without the cancellation_token parameter, have it call a new pub(crate) async fn start_with_cancellation_token(..., cancellation_token: CancellationToken) containing the current implementation, and update lib/llm/src/kv_router.rs KvRouter::new to call KvScheduler::start_with_cancellation_token.
| replica_sync: bool, | ||
| router_id: u64, | ||
| worker_type: &'static str, | ||
| cancellation_token: CancellationToken, |
There was a problem hiding this comment.
Adding cancellation_token to public create_multi_worker_sequences breaks downstream dynamo-llm callers. Fix: keep the old public function and put the new cancellation-token plumbing in a crate-private variant.
🤖 AI Fix
In lib/llm/src/kv_router/sequence.rs, restore pub async fn create_multi_worker_sequences(...) without the cancellation_token parameter, have it call a new pub(crate) async fn create_multi_worker_sequences_with_cancellation_token(..., cancellation_token: CancellationToken) containing the current implementation, and update lib/llm/src/kv_router/scheduler.rs to call the token-aware variant.
Signed-off-by: Dan Gil <dagil@nvidia.com>
Summary
Original PR
Conflict Resolution Notes
subscriber.rs/worker_query.rs: kept release/1.3.0 signatures; added scopedCancellationTokenparams instead of main-onlyworkers_with_configs/ health-monitor args.e2e_harness.py: omittedrun_cache_salt_isolation_test(not present on release/1.3.0).Test plan