fix(kv-router): stamp request identity on decode-affinity scoring row - #12955
Conversation
The decode-affinity branch in `DefaultWorkerScorer::worker_logit` emits its own candidate scoring row and returns early, so it never reaches the two rows that carry `request_id` and `worker_type`. ai-dynamo#12370 stamped those two rows; ai-dynamo#11720 added this third row in parallel and merged without the fields, so any decision taking this branch emits candidates in the pre-ai-dynamo#12370 shape. The predicate keys off the configured `overlap_score_credit` (default 1.0) rather than any measured overlap, so on an affected deployment every decision takes the branch and every candidate row loses its identity. Grouping the candidates of one routing decision then falls back to line adjacency, which is unreliable when decisions interleave. Add the two fields so all three candidate rows share one shape. Both values are already in scope in this branch and are evaluated inside the macro, so they cost nothing when DEBUG is disabled. Observability only: routing behaviour, metrics and API surfaces are unchanged, and these rows are DEBUG-level. Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com>
|
👋 Hi nv-yna! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughThe decode-worker scoring debug log now includes ChangesDecode scoring logging
Estimated code review effort: 1 (Trivial) | ~2 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
|
🎯 Code Coverage (details) 🔗 Commit SHA: e6c76ef | Docs | Datadog PR Page | Give us feedback! |
Summary
DefaultWorkerScorer::worker_logithas threetracing::debug!sites that emit one candidate scoring row each. Two of them carryrequest_idandworker_typeso the candidates of a single routing decision can be grouped by id in one hop. The third — the decode-affinity branch — carries neither, and itreturns before reaching the other two.The result is that any decision taking the decode-affinity branch emits candidate rows without request identity, so grouping them back into a decision requires the line-adjacency heuristic that stamping was added to replace.
This is not occasional. The branch predicate reads the configured
overlap_score_credit(default1.0), not any measured overlap:so on an affected deployment every decision takes the branch and 100% of candidate rows lose their identity fields — even when the measured overlap is zero.
Affected configurations
worker_typetrack_prefill_tokensoverlap_score_credit--no-router-track-prefill-tokensdecodefalse(flag)1.0(default)decodetrue1.0decodefalse(forced)0.0(forced)prefilltrue1.0Aggregated deployments route through
worker_type = "decode"unconditionally, so the aggregated case only needs the user-facing--no-router-track-prefill-tokensflag to be affected. The disaggregated decode pool is spared becausebuild_decode_router_overrideforcesoverlap_score_credit: Some(0.0), making the third condition false.Origin
Two PRs landed the same day touching this function. #12370 (15:24 PT) added the identity fields to the two rows that existed then. #11720 (19:39 PT) added this third row from a branch that predated the stamping.
git merge-base --is-ancestorconfirms #12370 was already in #11720's ancestry, so this is a parallel-development merge miss rather than an intentional change — the two PRs touched disjoint lines, so git saw no conflict.Fix
Add the two fields so all three candidate rows share one shape.
context.request_idandself.worker_typeare already in scope in this branch (it already readsself.worker_typeandcontext.track_prefill_tokensin its own predicate), both are borrowed&strwith no allocation, and both are evaluated inside the macro so they cost nothing when DEBUG is disabled.Severity is observability only. Routing behaviour, metrics and API surfaces are unchanged, and these rows are DEBUG-level, so a default INFO deployment sees no change.
Follow-ups not in this PR
tests/utils/router_logs.pyalready parsesrequest_idout of router logs, so the assertion has a natural home.Validation
cargo check -p dynamo-kv-router— cleancargo clippy -p dynamo-kv-router— no new warningscargo fmt --all -- --check— cleanBehavioural verification was done on the
1.4.0line, where the same defect is present, by running an aggregated deployment (dynamo.frontend --router-mode kv+dynamo.mocker --num-workers 3, Qwen3-0.6B) twice with only--no-router-track-prefill-tokensdiffering:overlap_credit_blocks: 0.000confirmed the branch fired with no measured overlapThe control arm establishes that the log directives, deployment and field parsers all work, so the subject result is attributable to the branch and not to test setup.
An internal Linear ticket also tracks this (DYN-3804).
Summary by CodeRabbit