feat(router): coordinate session affinity across replicas [DYN-3249] - #11079
Conversation
Signed-off-by: PeaBrane <yanrpei@gmail.com>
Signed-off-by: PeaBrane <yanrpei@gmail.com>
This comment has been minimized.
This comment has been minimized.
WalkthroughThis PR replaces per-process in-memory session-affinity with a distributed claim layer. A new claim API ( ChangesDistributed Session Affinity
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/llm/src/session_affinity/push_router.rs (1)
146-158: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve explicit DP rank in direct no-session routing.
Both direct fallback branches extract an
AffinityTarget, but then only useworker_id. A request withprefill_dp_rank/dp_rankcan be dispatched without the resolved rank, unlike the affinity path that passes/setsrank.Suggested fix
let Some(session_id) = session_id else { let explicit = explicit_target(&request, RequestPhase::Prefill)?; - let Some(pinned_worker) = explicit else { + let Some(target) = explicit else { return Err(invalid_argument( "worker ID required for prefill request in Direct routing mode", )); }; + let rank = target.dp_rank; return self .inner .select_and_dispatch_exact( request, - Some(pinned_worker.worker_id), - move |request, worker_id| prepare(request, worker_id, None), + Some(target.worker_id), + move |request, worker_id| prepare(request, worker_id, rank), ) .await; };For the
generatebranch, similarly normalize the request routing rank beforeinner.direct(...)whentarget.dp_rankis set.Also applies to: 241-248
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/llm/src/session_affinity/push_router.rs` around lines 146 - 158, The direct no-session routing path is dropping the explicit affinity rank and only forwarding worker_id. Update the branches that use explicit_target in push_router’s no-session handling so the resolved dp_rank/prefill_dp_rank is preserved and applied before dispatch, matching the affinity-path behavior. In the prefill branch, pass the rank through the select_and_dispatch_exact/prepare flow, and in the generate branch normalize the request routing rank before calling inner.direct when target.dp_rank is present.
🧹 Nitpick comments (1)
lib/runtime/src/discovery/kv_store.rs (1)
43-44: 🩺 Stability & Availability | 🔵 TrivialTrack the hidden-watch-failure gap.
This TODO describes a stale-cache risk in the new claim invalidation path. Please link it to a GitHub issue or add a follow-up contract once
Bucket::watchcan surface backend errors.Do you want me to draft the follow-up issue text?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/runtime/src/discovery/kv_store.rs` around lines 43 - 44, The TODO in Bucket::watch should be tracked with a concrete follow-up, since it currently leaves hidden backend failures in the claim invalidation path unaddressed. Update the note near Bucket::watch in kv_store.rs to reference a GitHub issue or add a follow-up contract describing how etcd reconnect/compaction and FileStore overflow errors will be surfaced and converted into Reset once that support exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/llm/src/session_affinity/coordinator.rs`:
- Around line 720-760: `CloseAction::run` is closing the shared session claim
unconditionally after `finish()` drops only the current lease, which can evict a
binding that still has active requests. Update the close path in
`CloseAction`/`AffinityTrackedStream` to guard the eviction and `claims.close`
call behind a check that no other active leases remain for the same claim key,
using the existing `coordinator`/`claims` state and the `claim_key` to verify
exclusivity before terminal close.
In `@lib/llm/src/session_affinity/tests.rs`:
- Around line 435-443: The reacquire in session_affinity/tests.rs does not
verify that `ClaimEvent::Reset` rehydrates the authoritative claim from
`ClaimTestDiscovery::claims`; update the test around the
`coordinator.acquire(...).resolve(...)` flow to assert the reopened claim still
resolves to `target(8, Some(0))` and rejects the new `target(99, Some(0))`
proposal. Use the existing `coordinator`, `session_id`, and `ClaimTestDiscovery`
setup to prove the post-Reset claim comes from discovery rather than the local
cache.
In `@lib/runtime/src/storage/kv/file.rs`:
- Around line 643-648: The regression test
external_delete_is_observed_under_noncanonical_root currently uses t.path() for
both FileStore instances, so it never exercises a non-canonical root. Update the
test to create a symlinked root from the tempdir and construct one of the
FileStore values with that symlink path while keeping the other on the canonical
path, so the watcher/creator interaction in FileStore::new and the external
delete observation path are covered.
---
Outside diff comments:
In `@lib/llm/src/session_affinity/push_router.rs`:
- Around line 146-158: The direct no-session routing path is dropping the
explicit affinity rank and only forwarding worker_id. Update the branches that
use explicit_target in push_router’s no-session handling so the resolved
dp_rank/prefill_dp_rank is preserved and applied before dispatch, matching the
affinity-path behavior. In the prefill branch, pass the rank through the
select_and_dispatch_exact/prepare flow, and in the generate branch normalize the
request routing rank before calling inner.direct when target.dp_rank is present.
---
Nitpick comments:
In `@lib/runtime/src/discovery/kv_store.rs`:
- Around line 43-44: The TODO in Bucket::watch should be tracked with a concrete
follow-up, since it currently leaves hidden backend failures in the claim
invalidation path unaddressed. Update the note near Bucket::watch in kv_store.rs
to reference a GitHub issue or add a follow-up contract describing how etcd
reconnect/compaction and FileStore overflow errors will be surfaced and
converted into Reset once that support exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1211536f-d8df-45a5-a3c1-3987f8d67269
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locklib/bindings/python/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
components/src/dynamo/common/configuration/groups/router_args.pydocs/agents/session-ids.mddocs/components/frontend/nvext.mddocs/components/router/router-configuration.mdlib/llm/src/kv_router/push_router.rslib/llm/src/kv_router/push_router/request_guard.rslib/llm/src/kv_router/push_router/selection.rslib/llm/src/session_affinity/coordinator.rslib/llm/src/session_affinity/mod.rslib/llm/src/session_affinity/push_router.rslib/llm/src/session_affinity/tests.rslib/runtime/Cargo.tomllib/runtime/src/discovery/kube.rslib/runtime/src/discovery/kv_store.rslib/runtime/src/discovery/mod.rslib/runtime/src/pipeline/network/egress/push_router.rslib/runtime/src/storage/kv.rslib/runtime/src/storage/kv/file.rstests/router/common.pytests/router/router_process.pytests/router/test_router_e2e_with_mockers.py
Signed-off-by: PeaBrane <yanrpei@gmail.com>
Signed-off-by: PeaBrane <yanrpei@gmail.com>
Signed-off-by: PeaBrane <yanrpei@gmail.com>
Signed-off-by: PeaBrane <yanrpei@gmail.com>
Summary
Each frontend previously kept an isolated affinity cache, so a later turn handled by another replica could select a different worker and lose prefix-cache reuse.
Fixes #11035
Validation
cargo test -p dynamo-runtime --lib --no-default-features discovery::kv_storecargo test -p dynamo-runtime --lib --no-default-features pipeline::network::egress::push_routercargo test -p dynamo-llm --lib --no-default-features session_affinitycargo test -p dynamo-llm --lib --no-default-features kv_router::push_router.venv/bin/python -m pytest components/src/dynamo/common/tests/configuration/test_kv_router_args.py -q.venv/bin/python -m pytest tests/router/test_router_e2e_with_mockers.py::test_mocker_distributed_session_affinity -qcargo clippy --no-default-features -- -D warningsinlib/runtimeandlib/llmfern checkfern docs broken-linksgit diff --checkSummary by CodeRabbit
New Features
Documentation