test(router): isolate request lifecycle counters

[jthomson04]

Summary

Fix the shared-counter race in router_request_counters_follow_admission_and_completion_lifecycle and four related tests. The reported CI failure read 40 started requests instead of 39 after a query-only request. Unmarked tests can update the same process-global counters while a serial test runs.

• Store the started counter beside the completion counter and use direct field access for both.
• Give the shared router fixture fresh, unregistered lifecycle counters through a test-only helper. Retain the existing handles for all other metrics, including their labels and initialized series.
• Keep the five original serial markers, use explicit expected counts, and check that a second router cannot change either lifecycle counter. Each fixture runtime has its own in-memory discovery store.

Production metric registration, exported names, and global ownership remain unchanged. Component-level production ownership is deferred to a separate change covering registration, recording paths, and exports.

Validation

All builds and tests ran locally outside the sandbox with dynamo-llm --no-default-features and default test concurrency.

• Routing-host group: 70 passed, then all 50 repeat runs passed (3,500 test executions).
• Full library suite: 2,590 passed, 3 ignored.
• Checked the regression with each counter shared separately: sharing the started counter fails with 5 instead of 4; sharing the completion counter fails with 2 instead of 1. Restored the fixed source after both checks.
• cargo fmt --all -- --check and git diff --check passed.

Related Issues

• Confirmed — no related issue.
• Failure observed in PR fix(router): reject unknown explicit worker targets #14858; the shared-counter code is also on main.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 3bc56f42-a976-4f07-ba0d-5189ef49ba72

📥 Commits

Reviewing files that changed from the base of the PR and between 95cadce and ab2494b.

📒 Files selected for processing (3)

• lib/llm/src/kv_router/metrics.rs
• lib/llm/src/kv_router/routing_host/request_guard.rs
• lib/llm/src/kv_router/routing_host/tests.rs

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.

---

Walkthrough

Router request metrics now store counters per instance. Test helpers and KV routers use standalone metrics. Several tests run without serial execution, and the lifecycle test verifies that activity on one router does not change another router’s counters.

Changes

Router metrics isolation

Layer / File(s)	Summary
Metrics instance ownership and test factory lib/llm/src/kv_router/metrics.rs	RouterRequestMetrics now stores requests_started_total. Registration retains the counter on the instance, and for_test() creates unregistered standalone metrics.
Test metrics wiring lib/llm/src/kv_router/routing_host/request_guard.rs, lib/llm/src/kv_router/routing_host/tests.rs	Request guard tests and KV test routers use isolated metrics from RouterRequestMetrics::for_test().
Parallel test validation lib/llm/src/kv_router/routing_host/tests.rs	Five KV routing tests no longer use serial execution. The lifecycle test verifies counter isolation between two routers.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to ab249

The counter ownership change is isolated per metrics instance, with test wiring updated accordingly. No merge-blocking issue remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 63.16% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the main change: isolating request lifecycle counters in router tests.
Description check	✅ Passed	The description explains the problem, implementation, validation, and issue status. It does not use the template's exact Overview, Details, or reviewer-start headings, but it provides the correspondin…

• Fix all pre-merge checks with AI

---

_{Comment @coderabbitai help to get the list of available commands.}

[jthomson04]

Code review

Nine comments below, most severe first.

The headline: moving requests_started_total into RouterRequestMetrics does not make it per-router, because from_component still memoizes the whole struct in a process-global OnceLock. Two RoutingHosts in one process (decode + prefill pool in a disaggregated frontend) still share one counter set, so the test-side field override is working around a real production bug rather than fixing it.

The rest split into test-isolation gaps — most test routers still carry the global metrics, and the five dropped #[serial] markers rest on an unverified premise — and cleanups in the new test and helper code.

_{Review generated with Claude Code. Line references are against ab2494b.}

[dynamo-review-agent]

Previously reported defects still present:

• Original discussion: The previously reported component-ownership defect remains: RouterRequestMetrics::from_component still memoizes the entire metric set in one process-global OnceLock, so later routing hosts reuse counters registered to the first host's component and router labels. In a disaggregated frontend, this continues to merge/mislabel pool metrics.
• Original discussion: The previously reported component-scoping defect is still present: RouterRequestMetrics::from_component still memoizes the entire metrics struct in the process-global ROUTER_REQUEST_METRICS, so later RoutingHost instances reuse the first component's registered counter handles rather than getting independently labeled request metrics.
• Original discussion: Verified: RouterRequestMetrics::from_component still memoizes the entire metric set in the process-global ROUTER_REQUEST_METRICS. Multiple RoutingHosts in one process therefore share the first host's component-labeled counters, so requests from another pool remain attributed to that first component. The author explicitly deferred this production metrics-ownership defect.
• Original discussion: RouterRequestMetrics::from_component still memoizes the entire metrics set in the process-global ROUTER_REQUEST_METRICS OnceLock, so multiple routing hosts in one process share the first component's counters and labels.