Repository navigation
feat(router): unify frontend and standalone selection core - #14570
ishandhanani merged 111 commits into
Conversation
…h the frontend scheduler First half of bringing the standalone selection core to input parity with the frontend KvRouter so one engine can serve both. - SelectionHostHooks on SelectionServiceBuilder: prefill-load estimator, overloaded-worker exclusion set, hard-availability inclusion set, shared KV cache, and LoRA worker filter, applied to every partition scheduler the core creates. The standalone service leaves them unset, so its behavior is unchanged; an embedding host feeds the same state it passes to KvScheduler. - Shared-cache hits are queried concurrently with the indexer and reach the scorer and custom policies; select requests accept the frontend's structured session_context (parent_session_id, session_final, kv_hints, input_trigger), with the flat session_id kept as the legacy form. - LoRA-aware candidate narrowing as a runtime-free LoraWorkerFilter trait with the frontend's exact semantics: never widens the allow-set, keeps a pinned worker inside it, falls back to the caller's set when narrowing empties it. - selection_id -> partition index so prefill_complete, free_reservation, and add_output_block stop scanning every partition; a lazy sweep drops ids whose booking was released out of band. - Advisory select through select_without_admission (no queue, no booking) with the chosen worker's projected load and prefill_busy / decode_busy against the conditional-disagg thresholds; admitted responses gain the same two fields. Tests: providers steer selection through the core; shared-cache hits and every session field reach the picker; LoRA helper matrix and injected-filter core test; reservation index population, indexed-first ordering, removal, and sweep; advisory and admitted responses with and without thresholds. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
…ctionService Second half of input parity: the standalone core indexes the way the frontend KvRouter does. - IndexerPolicy::from_router_config resolves the frontend's shape rules: event-driven primary, event-driven plus a short-TTL predict-on-route side indexer when router_predicted_ttl_secs is set, approximate TTL primary when use_kv_events=false. Bookings (select_and_reserve, cached replay, explicit reservations with tokens or hashes) are recorded into the approximate indexer; query-only selects record nothing. Previously a service with use_kv_events=false had an empty index and no cache awareness. - Indexer::Remote serves the primary index from a standalone indexer over HTTP (POST /query_tiered_by_hash returns lossless TieredMatchDetails). With a remote primary the core requires no kv_events endpoints and registers no ZMQ listeners. Exposed as SelectionServiceBuilder::remote_indexer, --remote-indexer-url, and remote_indexer_url= on the Python constructor. - Router hints: catalog records carry router_hint_worker_type and per-rank source control endpoints; a booking retains the matched chain when the partition has a hint-capable worker and SelectResponse::router_hint names a same-role source that beats the target's own cached prefix. Tests: backend policy matrix; approximate primary and side-indexer merge for 1 and 2 threads; bookings credited through all three paths; /query_tiered_by_hash round trip; remote indexer against a live standalone indexer including offline mapping; hint produced for a cold-worker booking and absent for warm-worker and query-only selections. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
…rtition Phase 1a of the runtime-optional frontend roadmap. Behind KvRouterConfig::router_embedded_selection (--router-embedded-selection / DYN_ROUTER_EMBEDDED_SELECTION, default off), the frontend KvRouter runs its scheduling and active-sequence accounting on one partition of an in-process SelectionService instead of the runtime-bound KvScheduler: - RouterScheduler enum in KvRouter dispatches statically to either backend; every router method (admitted and advisory scheduling, bookings, lifecycle, potential loads) goes through it. Request leases fence with the partition scheduler's booking_cleanup exactly as with KvScheduler. - kv_router/embedded.rs builds the service with the registry-resolved policy (or Dynamo's default scorer/picker under the router's metric label), the router's overload/availability providers and prefill estimator as host hooks, and the router's own indexer as the dequeue-time overlap refresh source. A catalog feeder mirrors the runtime-config watch into the partition's worker catalog (dp layout, capacities, taints, topology, KV-transfer, router-hint metadata). - The router keeps its indexer and request transport: it computes overlap against its indexer and hands the partition a fully formed ScheduleRequest. Selection-service additions used by the host: SelectionPartition handle, ensure_partition (create the scheduler before workers register), external_kv_events (host feeds KV state; no ZMQ listeners), a policy-factory override on the builder, and OverlapRefreshSource (partition indexer, external, or disabled). Not carried over in embedded mode: NATS active-sequence replica sync (router_replica_sync warns), per-class queue metrics, and scheduler load publication to the routing-load monitor. Tests: routing_host::embedded_selection_books_and_releases_like_the_runtime_scheduler mirrors the runtime preview/plan/abort/track/free flow on the embedded path; embedded::worker_request_mirrors_runtime_config covers the catalog mapping. All existing kv_router tests pass on the runtime path. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
KvWorkerMonitor's prefill-token overload detection reads the scheduler's own load snapshots through SchedulerLoadSender. The embedded selection partition never published them, so --active-prefill-tokens-threshold could not trigger 529 under --router-embedded-selection (tests/router test_mocker_kv_router_overload_529[prefill-tokens] failed only on that path; decode-blocks worked because it uses worker-published metrics). Add SelectionHostHooks.scheduler_load_sink (a SchedulerLoadSink) that the partition's ScopedSequencePublisher forwards publish_scheduler_load(_batch) into; the embedded host bridges it to the router's SchedulerLoadSender. tests/router overload_529 (agg + disagg, prefill-tokens + decode-blocks) now pass on the embedded path. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com> (cherry picked from commit beb6ddc)
Phase 1 exit gates now pass: the frontend A/B benchmark (+6% req/s, lower p50/p99), offline KV replay byte-parity against main on four rows, and tests/router e2e green on both scheduler paths. Flip router_embedded_selection to default true (serde default/skip updated, PyO3 constructor, .pyi, --router-embedded-selection default, docs). The runtime-bound KvScheduler path stays reachable through --no-router-embedded-selection for one release as a rollback and logs a deprecation warning at startup; deleting it is the follow-up. A caller-injected custom WorkerSelector type (not DefaultWorkerSelector) cannot run on the partition, so KvRouter::new keeps the runtime scheduler for it with a warning instead of silently changing selection semantics. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com> (cherry picked from commit 568d06c)
--router-replica-sync previously logged that it was not carried over to the embedded partition. Add SelectionHostHooks.replica_sync, a per-partition factory returning HostReplicaChannels (outbound events, inbound peer events, this replica's id); when the service runs no ZMQ peer mesh the partition's publisher forwards to the host's outbound channel and its replica consumer reads the host's inbound channel. The embedded host builds those channels over the runtime event plane (ACTIVE_SEQUENCES_SUBJECT, same wire formats and subscriber as the runtime scheduler used), so multi-replica frontends keep active-sequence sync on the default scheduler path. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com> (cherry picked from commit d586893)
…d partition Install the process-wide WorkerSelectionPolicyRegistry (the linked custom policy catalog the Python bindings register under --features custom-policy) through dynamo_llm::kv_router::install_worker_selection_policy_registry; embedded partitions and the static frontend resolve KvRouterConfig policy instances against it instead of the built-in default registry. A KvRouter built with Sel = WorkerSelectionPolicy therefore runs on the embedded partition too; only an arbitrary caller-injected WorkerSelector type (unit test fakes) still selects the runtime scheduler. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com> (cherry picked from commit fc22bac)
…tion Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
The frontend router now always schedules on an embedded selection-service partition. Remove `KvScheduler` (kv_router/scheduler.rs keeps only its runtime-free re-exports), the runtime sequence publisher and its constructors, the direct-ZMQ active-sequence ingress, the two-variant `RouterScheduler` dispatch, and the LoRA load estimator's uncalled scheduler polling. `KvRouter<Sel>` keeps its type parameter for API compatibility (`PhantomData<fn() -> Sel>`). A caller-injected `WorkerSelector<ModelRuntimeConfig>` that is neither the default selector nor a registry `WorkerSelectionPolicy` now runs inside the partition: `WorkerSelectionPolicy::delegated` (new `DelegatedWorkerSelector` trait in dynamo-kv-router) hands the selector the scheduling request and a config-erased view of the eligible workers (`ErasedWorkerConfig`), which the embedded host projects back onto `ModelRuntimeConfig`. The unit tests that inject fake selectors run unchanged on that path. The per-worker `router_worker_registered` gauge is now maintained by the embedded catalog feeder. `router_embedded_selection` is deprecated and ignored; `false` logs a warning. Four sequence-level tests that built the runtime sequences directly are removed; their behavior is covered by the replica-lifecycle and session-affinity e2e tests on the embedded path. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com> (cherry picked from commit 71cea67)
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
The embedded selection partition is the only scheduler, so the flag has been a no-op since the runtime scheduler was deleted. It never shipped in a release, so no wire compatibility shim is needed: drop it from KvRouterConfig (Rust and serde), the DYN_ROUTER_EMBEDDED_SELECTION environment override, the Python argument group, the bindings signature, and the frontend configuration reference. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
…router KvRouter, RoutingHost, RoutePlan, RequestGuard, PrefillRouter, ManagedKvRouter, ModelWatcher, and PreprocessedRouting were generic over a caller-injected WorkerSelector type that only ever named the policy the embedded partition would run. Replace the parameter with SelectionPolicySource: Registry (the linked policy the router config names, else Dynamo's default) or Factory (an explicit WorkerSelectionPolicyFactory). The router resolves it once at construction and instantiates one policy to learn the worker inputs it needs, so shared-cache and indexer gating are unchanged. ModelManager constructors take the policy source in place of a selector (kv_chooser_for_with_policy[_and_client], managed_kv_router_for_with_policy); HttpFrontend, ModelWatcher, and PrefillRouter carry it instead of a per-model selector factory. The bindings' custom-policy feature no longer needs a separate router type. The delegating-selector adapter in the embedded backend is removed with its only caller. Unit tests that injected fake selectors now inject picker or filter policies through the public worker-selection API; the overload test becomes a filtered-workers test because a policy cannot report overload directly. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
DelegatedWorkerSelector, ErasedWorkerConfig, and the Delegated policy state existed only so the frontend could run an arbitrary caller-injected WorkerSelector type on a partition. That path is gone with the selector type parameter; custom policies use WorkerSelectionPolicy's filter, scorer, and picker API like every other host. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
…ionHost
SelectionHostHooks was a flat bag of eight optional fields. Regroup it by
purpose as SelectionHost { load, cache, eligibility, telemetry, replication },
each group defaulting to the standalone service's behavior so a host sets only
the groups it owns. The builder method becomes SelectionServiceBuilder::host.
No behavior change; the frontend's embedded partition sets the same values.
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Phase 4 of the runtime-optional frontend roadmap: the EPP pod reflector is the Kubernetes catalog feeder, and it registered every pod as a single data-parallel rank (the hardcoded data_parallel_size: 1 in selector.rs). - EppStandaloneConfig gains DYN_EPP_DATA_PARALLEL_SIZE (default 1) and DYN_EPP_KV_EVENT_PORT_STRIDE (default 1). Rank r publishes KV events on kv_event_port + r * stride; the reflector emits one KV-event endpoint per rank (RawWorker.kv_events_endpoints) and the registration carries data_parallel_size, which the selector passes through with data_parallel_start_rank 0. Tests: strided per-rank endpoints for a 3-rank pod; existing reflector, adapter, and selector tests updated to the per-rank shape (144 pass). Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
… membership A host owns discovery and exposes it as a WorkerCatalogSource: a stream of complete desired membership snapshots. CatalogReconciler applies each snapshot to a SelectionCore: it upserts new or changed records, retries records the core reported not schedulable, deletes workers that left (including ones that never converged), rejects duplicate ids before mutating anything, and clears the catalog when the source closes so selection fails closed. A CatalogObserver lets a host keep per-worker metrics in step. This is the reconcile loop the EPP standalone selector and the frontend's embedded partition each carried; both switch to it in the next commits. SelectionService exposes its core for hosts that drive the catalog directly. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
…ogReconciler The runtime-config watch becomes a WorkerCatalogSource (RuntimeDiscoverySource) and the partition's catalog is driven by the shared CatalogReconciler instead of the router-local CatalogFeeder. The per-worker router_worker_registered gauge is kept through a CatalogObserver. Behavior is unchanged except that a worker the core reports not schedulable is now retried on every membership snapshot rather than only when its runtime config changes. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
The standalone EPP's topology adapter yields the reflector's Ready pods as WorkerRequest snapshots (PodReflectorSource) and lets CatalogReconciler own the desired-versus-actual diff. Selector::reconcile, WorkerRegistration, and the EPP-local reconcile state are removed; their tests moved to dynamo-kv-router with the reconciler. A closed reflector channel still yields one empty snapshot so selection fails closed, as before. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
…ex comes from
Replace three knobs that together described the same thing (the builder's
external_kv_events() and remote_indexer(), and SelectionHost's
OverlapRefreshSource) with one KvIndexSource on HostCache:
- Owned: the core owns the index and subscribes to worker KV events over ZMQ
(the standalone default; workers need KV-event endpoints).
- External { refresh }: the host owns and feeds the index; workers need no
KV-event metadata; refresh optionally lets the core re-score queued
requests against the host's index (the frontend's embedded partition).
- Remote(url): a standalone indexer serves the primary index.
The listener decision, schedulability metadata, and dequeue-time refresh all
derive from this one value. SelectionServiceBuilder::kv_index sets it;
SelectionServiceConfig and the Python bindings map remote_indexer_url onto
Remote. No behavior change.
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
…ion service lib/llm and lib/kv-router each carried an Indexer enum with the same Single/Concurrent/Remote shape (primary, lower tiers, predict-on-route side overlay, primary_records_routing_decisions) and diverging method sets. Fold the frontend's richer implementation into dynamo_kv_router::services::indexer: - lookup.rs: the borrowed/owned HashInput lookup pipeline (primary, lower tiers seeded from confirmed primary matches, side overlay merged by per-worker max), find_matches_by_tier[_ref][_with_options], find_primary_*. - recording.rs: routing-decision recording targets (side overlay, approximate primary, remote primary) and the per-request ApproximateRequestLease. - backend.rs: the merged enum gains None, residency projection setters, try_apply_event (the backpressure-aware admit with a completion barrier on Cleared that recovery relies on), reset_worker_dp_rank_and_wait, and the approximate-LRU capacity and stats API. apply_event_routed and remove_worker[_dp_rank] stay as the standalone listener's path. Remote primaries go behind RemotePrimary so the HTTP standalone client here and the request-plane RemoteIndexer in lib/llm plug into the same enum. dump_events returns KvRouterError like the rest of the surface. - The frontend keeps only construction (indexer::build from KvRouterConfig with component metrics and the request-plane remote), the served-indexer service, and recovery. Its 16 indexer tests move with the code. No behavior change: both hosts run the exact code they ran before for their own paths; TieredMatchProvider keeps the capability-gated variant. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
KvIndexSource::External { refresh } gave the core only a refresh handle onto
an index the host kept elsewhere. Replace it with Provided(Arc<Indexer>): the
host builds and feeds the index, and the core holds it as every partition's
index, reading it for selection and dequeue-time refresh (when the index
supports refresh) and never writing to it. The frontend's embedded partition
now shares the KvRouter's Indexer object instead of a TieredMatchProvider
view of it. RefreshProvider loses its External variant.
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
KvIndexSource::Owned carries the ingress that feeds the core-built index. The trait decides what metadata a worker needs to be schedulable, attaches its ranks when the worker becomes schedulable, and detaches them when it leaves. ZmqDirectIngress is the existing standalone behavior moved out of SelectionCore: endpoint validation, one ZMQ listener per rank on the registry, replay endpoint, deregistration on delete. The core keeps the lifecycle decisions (when to attach, how to mark Incomplete) and no longer knows about ZMQ endpoints. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
…tend KvEventIngress now builds the partition's index (open) as well as feeding it, so KvIndexSource::Owned covers both hosts and Provided goes away: - ZmqDirectIngress opens the registry's index and, on detach, deregisters the worker's listeners or, when there are none (events off or a remote primary), drops the worker from the index: the two behaviors SelectionCore used to branch on itself. - RuntimeIngress (lib/llm/src/kv_router/indexer/ingress.rs) builds the frontend's index from the router config, starts the event-plane subscription with per-rank recovery and the served-indexer endpoint, and hands the index to the partition. Its per-worker hooks are no-ops because membership comes from the KV-source watch. KvRouter::new shrinks to constructing it; task guard and shutdown go through it. The core no longer special-cases a host-fed index anywhere: every Owned index is opened by and detached through its ingress. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
… host Move the session table out of lib/llm/src/session_affinity into dynamo_kv_router::services::selection::affinity: SessionAffinity (session id -> bound worker/rank, versioned, TTL reaper, entry and id limits), the AffinityInitialization / AffinityLease lifecycle (commit, rebind, invalidate, release on drop), hard and soft validation, and replica apply with the same outcome table. Replication becomes an AffinityReplicaSink the host installs; the table stays transport-free. The frontend AffinityCoordinator is now a thin wrapper: it converts the pipeline RouteTarget at the boundary, abandons an initialization wait when the request context stops, holds a lease for the life of a response stream, and keeps its runtime event-plane replica sync (same subject and wire format) behind the sink. SessionAffinityMode moves with the table; lib/llm re-exports it. All 37 session-affinity tests and the routing-host affinity tests run unchanged against the wrapper. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
SelectionServiceBuilder::session_affinity(ttl) gives the core its own SessionAffinity table. A booked selection whose request carries a session id (session_id or session_context) and no explicit pin or affinity target acquires the session first: a bound session steers selection to its worker (exclusive for the default selector) and a new one is bound to the worker the scheduler chose. The lease is held with the booking and released by free_reservation; a bound worker that was not selected (it left) is invalidated and the session rebound. Advisory selects read the binding. Bindings replicate over the existing ZMQ peer mesh on their own topic (dynamo.session-affinity.v1) so peers that do not subscribe never see them; the active-sequence topic and wire format are untouched. The publisher multiplexes both channels onto one socket; PeerManager subscribes to the second topic only when a handler is given. SelectionServiceConfig carries the TTL for the standalone binary. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Preserve main's explicit-worker validation and isolated request counters alongside the shared selection core and existing affinity ownership fixes. Adapt incoming routing tests to the current host and policy APIs. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
|
/ok to test e1878f6 |
There was a problem hiding this comment.
Reviewed as a unification. Two P3 findings, nothing blocking from me. Both base-vs-head differentials came back byte-identical.
Bar applied, and the base commit it was measured against
Reviewed as a unification: the bar I applied is that the frontend path and the standalone/core path must produce the same worker for the same request, before and after. I built two independent base-vs-head differentials and both came back byte-identical. Two P3s below; nothing blocking from me.
Base used throughout: c76d3208f3 (compare/main...e1878f68 — merge_base_commit.sha is null on the API, and the branch is 0 commits behind main; the diffstat reproduces exactly, 104 files / +15752 / −9531).
1. Frontend selection differential, measured with a passing control
An integration test driving KvRouter::find_best_match_details_with_policy_class (identical signature on both trees) over a seeded 400-request stream against 4 workers, 6 planted prefix families. The only difference between the two harness files is the policy argument — DefaultWorkerSelector::new(...) on base vs SelectionPolicySource::Registry on head — which is exactly what production passes on each tree (model_manager.rs:2027 on base; model_manager.rs:2030 → prepare() → Prepared on head).
Two sub-traces, 801 lines total, recording chosen worker, dp_rank, overlap_blocks, effective_overlap_blocks, cached_tokens, potential_decode_blocks, whether hashes/hint were attached, and the full sorted per-worker projected-load table before every decision:
- A — advisory, no bookings, with an asymmetric pre-booked backlog per worker so every pick is a real overlap-vs-load tradeoff (picks spread 252/103/33/12 across the four workers).
- B — tracked bookings, worker pinned round-robin, 400 accumulating bookings, settling on the scheduler's own load table between steps.
head: DETERMINISTIC across 3 runs (126174 bytes / 801 lines)
base: DETERMINISTIC across 3 runs (126174 bytes / 801 lines)
base vs head: BYTE-IDENTICAL
The known tie-break nondeterminism is real and I had to defeat it: at router_temperature == 0 an exact cost tie is resolved by process-global fastrand over HashMap candidate order (scheduling/selector/default.rs:497). With integer costs I measured 49/400 picks flipping between two runs of the same binary. Setting overlap_score_credit to a non-round 0.9137 makes every cost distinct and the tie-break never fires — that is what makes the result above meaningful rather than lucky.
Control: perturbing the term the decode branch actually evaluates ((decode_cost_blocks - 1.07 * overlap_credit_blocks)) changes 6/400 picks and the harness catches it. My first two control attempts came back blind — one mutated a line after an early return, the other was a no-op because prefill_cost_blocks is 0 under router_track_prefill_tokens: false. Worth stating plainly, since a differential without a passing control proves nothing.
2. Offline replay parity, measured with a passing control
The PR notes replay parity was not rerun because replay does not pass through SelectionCore. It does, however, pass through LocalScheduler and ActiveSequencesMultiWorker, both of which this PR changes (scheduling/local.rs +97/−16, sequences/multi_worker.rs +69/−64), so I ran it:
offline_replay_bench testdata/mooncake_trace_1000.jsonl --num-workers 8 --canonical-reports-jsonl
base.jsonl == head.jsonl BYTE-IDENTICAL, 1394604 bytes
1000 per-request records, decode_worker_idx spread 179/93/104/85/123/148/130/138
Control: a 7% perturbation of the scorer changes 857/1000 decisions and the comparison catches it. lib/mocker and lib/bench are byte-identical to main, so the harness itself is not a variable.
3. What the deletions cost, swept over all 9,531 deleted lines
I swept all 9,531 deleted lines for guards, error variants, defaults and constants. No numeric constant, TTL, threshold or size limit changed value — MAX_SESSION_AFFINITY_TTL_SECS = 31_536_000, MAX_SESSION_AFFINITY_ENTRIES = 65_536, MAX_SESSION_AFFINITY_ID_BYTES = 256, REPLICA_EVENT_CHANNEL_CAPACITY = 100_000, DEFAULT_MAX_BATCHED_TOKENS = 10_000_000, DIRECT_ZMQ_RCVHWM = 1_024 all survive byte-for-byte. Of 47 functions deleted from core/mod.rs, 43 exist by name in the new modules; of the remaining four, three are inlined with their behavior intact (I checked mark_incomplete_after_reconcile_error line by line — same "reconciliation failed: {error}" reason, now with an added cleanup_indexer_registration) and one is a test whose successor is in core/tests.rs:1969-1988. map_scheduler_error is unchanged. One real regression found, filed below.
4. Fan-out, by compilation
All clean on 1.96.1 with --cfg tokio_unstable: dynamo-kv-router default --all-targets; standalone-selection,standalone-slot-tracker,testing,bench,metrics,runtime-protocols --all-targets; isolated --no-default-features --features standalone-slot-tracker; dynamo-ext-proc --all-targets; dynamo-llm --no-default-features test targets.
Noted, not filed
lib/llmhad exactly one#[cfg(feature = "bench")]on base (the per-requesthash_us/find_matches_us/schedule_usbreakdown atkv_router.rs:1769) and has none on head. The equivalent measurements survive asLookupTimings→RoutingOverheadMetrics::observe, so this is a lost debug log rather than lost signal — flagging it only because the frontend-benchmark workflow reads it.dynamo-kv-routeranddynamo-llmare published on crates.io (dynamo-kv-routermax1.4.2). This PR removes ungated public API from both —LocalScheduler::supports_overlap_refresh,SchedulerQueue::supports_overlap_refresh,LocalScheduler::add_request_if_registered, theKvRouter<Sel>generic parameter,KvRouter::{shutdown, find_best_match, find_best_match_details, register_workers, free_if_worker, add_output_block},pub mod scheduler, and several re-exports. The workspace version is release-managed and this PR does not touch it, so this is not a finding against the PR; it is a note for whoever cuts1.5.0, since Cargo treats1.4 → 1.5as compatible and a downstream on"1.4"would break at compile time.
Boundaries, and what I did not run
- macOS ARM64 only;
--no-default-featuresfordynamo-llm(block-managerneedsO_DIRECT/fallocate). No GPU, no multi-node, no NIXL path exercised by me — I am relying on the PR's own B200 runs for those. - Trace B pins the worker, so it proves booking/accounting/overlap-reporting parity but not picker parity under load; picker parity is what trace A covers, and its control is the 6/400 above.
- I did not re-litigate the deferred affinity-policy items (
Held(Bound{other})classification, join-through-initializer-cancellation, LoRA-excluded bound worker). Those are existing threads with the author's documented deferrals to the DEP follow-up, and I found no new evidence to reopen them.jthomson04's CHANGES_REQUESTED is still the review decision of record; nothing here clears it. - Read line-by-line:
kv_router.rs,embedded.rs,core/run.rsselection path,scheduling/{queue,local,overlap,mod}.rs,selector/default.rs,core/workers.rsupsert. Read structurally:affinity.rs,membership.rs,reservations.rs,services/indexer/*. Scanned only:core/tests.rs(3,141 lines), the ext-proc adapters, and the docs.
There was a problem hiding this comment.
Approving on my own review. Zero P0, zero P1, two non-blocking P3 items outstanding. I authored no commits on this PR.
The bar, stated
Approving on my own review. I authored no commits on this PR.
Bar: zero P0, zero P1, two P3 outstanding (metric label continuity; the isolated standalone-selection CI check). Both are non-blocking and neither touches routing behavior.
What I verified by execution, base `c76d3208f3` vs head `e1878f68`
- Frontend selection is unchanged. 801 decisions over a seeded stream with planted prefixes and an asymmetric pre-booked backlog, recording the chosen worker,
dp_rank, overlap, effective overlap, cached tokens, projected decode blocks and the full per-worker load table per decision — byte-identical, 126,174 bytes, deterministic across 3 runs per tree, with a passing control (a 7% scorer perturbation flips 6/400 picks and is caught). - Offline replay is unchanged.
mooncake_trace_1000.jsonl --num-workers 8canonical reports — byte-identical, 1,394,604 bytes, 1000 per-request records, control catches a 7% perturbation at 857/1000. - The scoring core itself (
scheduling/selector/,policy.rs,config.rs,overlap_refresh.rs,filter.rs,queue_admission/,prefill_load.rs) plus all oflib/mockerandlib/benchare byte-identical tomain, so the unification did not touch the scorer — it re-plumbed its inputs, and the two differentials say the inputs arrive the same. - No constant, TTL, threshold or size limit changed value across the 9,531 deleted lines; 43 of 47 functions deleted from
core/mod.rssurvive by name, three are inlined intact, one is a test with a successor. - Fan-out by compilation is clean across
dynamo-kv-router(default, full-feature, and isolated),dynamo-ext-proc, anddynamo-llm.
Deliberately deferred, so it is a decision and not an oversight
- I did not re-open the affinity-policy items already on this PR (
Held(Bound{other})error classification, binding ownership through initializer cancellation, a bound worker excluded by the LoRA filter). They have existing threads with documented deferrals to the DEP follow-up, and I found no new evidence to justify reopening them. If those deferrals are wrong, that call belongs to the reviewers who raised them. jthomson04's CHANGES_REQUESTED is still the review decision of record. My approval does not clear it.- macOS ARM64 only,
dynamo-llm --no-default-features. No GPU, multi-node or NIXL path was exercised by me; I am relying on the PR's own B200 runs for those.
|
/ok to test 0d80f5c |
There was a problem hiding this comment.
Re-approving at 0d80f5ca4. The merge carried no author content change. Zero P0, zero P1, two P3 outstanding, and no routing decision moved on any measurement I ran.
Round 2 scope, and why I re-read the tree
Round 2 at 0d80f5ca4
The merge carried no author content change. It only absorbed main. I re-read the tree my earlier approval is now attached to, and I re-measured both open P3 findings at the new pair. One of them grew a second symptom, which I added to its thread.
What moved, measured two ways
The head is 14 commits ahead of e1878f684c. Thirteen of those are main commits that arrived through one merge. The only commit the author wrote is the merge itself.
A plain e1878f684c...0d80f5ca4 compare shows 76 files and is misleading. I built the two pull-request-scoped diffs instead and compared them:
git diff c76d3208f3...e1878f684c -> 104 files, +15752, -9531
git diff 40a853b578...0d80f5ca40 -> 104 files, +15752, -9531
diff of the two diffs: one line
< index c2b9fcef48..94dff9c050 100644 (Cargo.lock)
> index d1c6a3dd6b..5abb2e966f 100644
The single difference is the Cargo.lock blob identity line. The file set is the same 104 paths, with no additions and no removals.
Per-file blob comparison over the same 104 paths, e1878f684c against 0d80f5ca40:
identical_blobs=103 changed_blobs=1
CHANGED: Cargo.lock 94dff9c050... -> 5abb2e966f...
The Cargo.lock change is exactly the delta main applied. Stripping the index lines, git diff c76d3208f3 40a853b578 -- Cargo.lock and git diff e1878f684c 0d80f5ca40 -- Cargo.lock differ only in one hunk header offset, because this pull request adds one line earlier in the same block.
Base movement, one answer each for the four commits
Four commits entered the base. I checked each against this pull request's code paths.
8f9510112 (#14878, release unowned radix branches after eviction). Reaches these code paths, and I resolved it by execution. Both trees use one radix tree, lib/kv-router/src/indexer/radix_tree.rs. This pull request does not touch that file, and the commit's new helper prune_unreachable is private, so no caller signature moved. I re-ran the canonical offline replay on the new pair, which drives that tree through 1000 requests and 50,104 KV ingest batches:
offline_replay_bench testdata/mooncake_trace_1000.jsonl --num-workers 8 --canonical-reports-jsonl
base 40a853b578 vs head 0d80f5ca40: BYTE-IDENTICAL, 1394604 bytes
1000 per-request records, decode_worker_idx spread 179/93/104/85/123/148/130/138
Control on the same harness: changing let logit = prefill_cost_blocks + decode_cost_blocks + ... to + 1.07 * decode_cost_blocks moves 992 of 1000 records, so the comparison is not blind. My first control attempt was blind and I discarded it. It perturbed (decode_cost_blocks - overlap_credit_blocks) at scheduling/selector/default.rs:325, which sits in a decode branch this configuration never takes. The full dynamo-kv-router library suite also passes at the merged head, 1196 passed and 2 ignored, including the two regression tests #14878 added:
test indexer::radix_tree::tests::cache_churn_releases_evicted_branches ... ok
test indexer::radix_tree::tests::removing_detached_node_preserves_replacement ... ok
e292c76b2 (#14737, publish native KV events from the vLLM gRPC server). Ruled out. This pull request's core does consume KV events, so the question was real. The commit changes only the producer in lib/mocker. It changes socket binding, port discovery and a new publishing call site. No serialized type, field, tag or topic changed: the diff of lib/mocker/src/services/zmq_events.rs between the old and the new base adds only #[test] and #[tokio::test] attributes on the type lines, and lib/kv-router/src/protocols.rs did not change on main at all. lib/mocker does not reference services::common, so it does not share the ZMQ module this pull request edits.
fb7da85d0 (#14721, normalize equivalent worker hash inputs in the operator). Ruled out, and I checked it rather than waving it through. The change is Go, inside deploy/operator/internal/dynamo/hash.go, and it normalizes Experimental.Grove, Experimental.Checkpoint and CompilationCache.MountPath on a DynamoComponentDeploymentSpec. Its only callers are dgd_grove_workload_renderer.go and dgd_worker_rollout_reconciler.go, so it feeds Kubernetes rollout and Grove workload rendering. Router worker identity is WorkerWithDpRank in lib/kv-router/src/protocols.rs, built from runtime instance ids. The two never meet, and this pull request changes no Go.
cfada2fd9 (#14543, prepare the vLLM v0.29.0 bump). Ruled out. It touches Python under components/src/dynamo/vllm, container templates, and docs. No file it changes is in this pull request's 104-file set.
Both P3 findings re-measured, not carried forward
The metric label finding still reproduces. I measured the emitted series on both trees rather than reading the code, because the finding is about an observable series name:
DTKPROBE BASE pending_requests model=unknown,policy_class=default,worker_type=decode
DTKPROBE BASE pending_isl_tokens model=unknown,policy_class=default,worker_type=decode
DTKPROBE BASE pending_cached_tokens model=unknown,policy_class=default,worker_type=decode
DTKPROBE BASE backpressure_total model=unknown,...,reason=request_limit,...
DTKPROBE HEAD pending_requests model=default,policy_class=default,worker_type=decode
DTKPROBE HEAD pending_isl_tokens model=default,policy_class=default,worker_type=decode
DTKPROBE HEAD pending_cached_tokens model=default,policy_class=default,worker_type=decode
DTKPROBE HEAD backpressure_total model=default,...,reason=request_limit,...
All six series still move. The five files that decide this are byte-identical to the trees I ran the full-router probe on in round 1, on both sides, so the round 1 measurement transfers exactly.
The isolated-feature CI finding also still reproduces. .github/workflows/pre-merge.yml still checks standalone-slot-tracker and standalone-indexer and not standalone-selection, and lib/llm/Cargo.toml:75 still enables standalone-selection for every workspace build. The feature still builds on its own:
cargo check --locked -p dynamo-kv-router --no-default-features --features standalone-selection
Finished `dev` profile [unoptimized + debuginfo] target(s) in 36.00s
The per-variant override question, both overrides traced
I asked once whether any configuration this pull request unifies still has a per-variant override the unification does not cover. Two overrides exist, and I traced both.
The per-worker-role policy override is covered. resolve_for_worker_type in services/selection/policy_registry.rs picks a different policy per WorkerType. That file is not in this diff. On the base, only the standalone service called it. On the head, the frontend calls it too, through SelectionPolicySource::resolve at lib/llm/src/kv_router.rs:116, and the standalone service and EPP still call it at services/selection/service.rs:148. The unification widens coverage here rather than losing it, and it is inert unless a custom policy is registered.
The per-model policy override is the one that is not fully covered, and it is the same root cause as the metric label finding. I put the measurement on that thread.
Earlier findings, and their thread state
I opened no threads before round 1, and round 1 opened exactly the two P3 threads above. Both were resolved without a reply and without a change, and both still reproduce. I reopened the metric label thread, because it now carries evidence that reaches scheduling configuration and not only telemetry. I left the CI coverage thread resolved and replied there with the re-measurement. That one is a pure coverage nit with no behavior behind it, so I treat the author's resolution as a decision.
Fan-out at the merged head
All clean on macOS ARM64:
cargo check --locked -p dynamo-llm --no-default-features --all-targets Finished in 2m 00s
cargo check --locked -p dynamo-ext-proc --all-targets Finished in 59.93s
cargo check --locked -p dynamo-kv-router --all-targets Finished in 10.39s
cargo test --locked -p dynamo-kv-router --features standalone-selection,standalone-slot-tracker,testing,metrics,runtime-protocols --lib
test result: ok. 1196 passed; 0 failed; 2 ignored
Boundaries, and what I did not run
macOS ARM64 only, and dynamo-llm with --no-default-features. I exercised no GPU, no multi-node and no NIXL path.
I did not rebuild the round 1 frontend selection trace at the new pair. The 103 identical blobs and the byte-identical replay result cover the same ground, and the frontend files that trace exercised did not move.
I did not reopen the deferred affinity policy items. They have existing threads with the author's documented deferrals, and I found no new evidence.
jthomson04's CHANGES_REQUESTED from 2026-09-11 is still the review decision of record. Nothing here clears it.
Decision, and why the approval is dated
Re-approving at 0d80f5ca4. I authored no commits on this pull request.
Bar: zero P0, zero P1, and two P3 outstanding. The two P3 items are the metric label and profile key fallback, which is now reopened, and the isolated standalone-selection CI check, which I left resolved. Neither changes a routing decision on any measurement I ran.
My earlier approval was recorded against e1878f684c and GitHub then re-pointed it at the merge commit, so it stood on a tree nobody had read. This round read that tree. The re-approval replaces an implicit carry-over with a dated one.
atchernych
left a comment
There was a problem hiding this comment.
Two more findings from a pass over the changed files that did not already have review threads.
-
core/reservations.rs—ReservationIndexObserver::admitteddrops a displaced reservation's affinity lease while the reservation-index write guard is held. This is the pattern already fixed inforget_reservation_ifandsweep_reservation_indexin the same file;admittedlooks like the site the fix was not applied to. -
lib/llm/src/kv_router.rs— the end-to-end overload test is removed and replaced only by a direct unit test ofmap_scheduler_error, so nothing covers the re-hosted selection path preservingSelectionError::Scheduler(AllEligibleWorkersOverloaded)— the input to the HTTP 529 the DEP lists as unchanged.
Details inline.
Preserve newer cached selections when an older replay fails. Release matching reservation ownership before cancellable queue progress, and drop displaced affinity leases outside the reservation-index lock. Record explicit multimodal-only reservations, remove unused worker registration helpers, and restore the embedded-router overload regression. Add deterministic coverage for replay replacement, cancelled frees, and lock-held lease cleanup. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
|
/ok to test 8827c61 |
There was a problem hiding this comment.
Round 3. Re-approved at 8827c6132. I found no defect introduced by the one new commit. One P3 open, one P3 closed as a nit, no P0 and no P1.
What changed since the last approval, by blob comparison
The previous approval sat at 0d80f5ca4. The real delta since then is one commit, 8827c6132, fix(router): address selection core review feedback. A per-file blob comparison over the 104 PR-scoped paths confirms it: exactly 6 blobs moved, and they are the 6 files in the commit. The merge base is unchanged at 40a853b.
The fix commit, mutation-tested change by change
The fix commit reviewed as its own change
Four behavior changes, one dead-code removal, one visibility change. I mutation-tested every new test. Each one fails when I revert the behavior it guards, and passes when I restore it.
| Change | Test | Mutation applied | Result |
|---|---|---|---|
remove(key, id, generation) in place of discard on the BadRequest replay path (core/reservations.rs:203) |
failed_replay_preserves_a_newer_cached_selection |
restore discard |
FAILED |
free_if_booking_with_cleanup runs owner cleanup before the cancellable queue wait (scheduling/local.rs:616) |
cancelled_free_releases_reservation_and_affinity |
restore the post-await forget_reservation_if |
FAILED |
displaced Reservation dropped outside the reservation-index write lock (core/reservations.rs:108) |
index_observer_releases_displaced_affinity_after_unlock |
drop it inside the lock | FAILED |
can_record reads routing_tokens_and_mm_infos() (core/reservations.rs:322) |
bookings_populate_the_approximate_primary_without_kv_events::case_2_multimodal_only |
restore req.prompt.token_ids.is_some() |
FAILED, and case_1_text still passed |
AllEligibleWorkersOverloaded maps to ResourceExhausted (kv_router.rs:525) |
test_find_best_match_maps_overload_to_resource_exhausted |
map to Unavailable |
FAILED |
Two notes on the can_record change, because widening a guard can open a new error path. routing_tokens_and_mm_infos() returns Some for every input where token_ids.is_some() held, so it is a strict widening and never narrows. And the new condition is now exactly the success precondition of block_hashes_for_indexer (input.rs:147 and :160), so the widened guard cannot reach the BadRequest arm that the ? would propagate.
register_workers is gone from both LocalScheduler and SchedulerQueue. No reference to it remains anywhere in the repository.
SchedulerQueue no longer holds slots or workers_with_configs. The spawned actor now holds the only copies. That is safe, because the actor stops only when admission_rx closes, and admission_rx closes only when the queue drops its admission_tx. So the actor never outlives the queue, and no live sender can see a closed watch channel. The PhantomData<fn() -> (P, C, Sel, RF)> comment is accurate. fn() -> T is covariant and adds no auto-trait bound.
I found no defect introduced by this commit.
Compile fan-out
cargo check --locked -p dynamo-kv-router --no-default-features --all-targets Finished
cargo check --locked -p dynamo-kv-router --no-default-features --features standalone-selection --all-targets Finished
cargo check --locked -p dynamo-kv-router --no-default-features --features standalone-indexer --all-targets Finished
cargo check --locked -p dynamo-kv-router --no-default-features --features standalone-slot-tracker --all-targets Finished
cargo check --locked -p dynamo-llm --no-default-features --all-targets Finished
cargo check --locked -p dynamo-ext-proc --all-targets Finished
Regression check against the approved tree, with the pre-existing failures named
cargo test -p dynamo-kv-router --no-default-features --features standalone-selection --lib: 1162 passed, 6 failed at 8827c6132. The identical 6 tests fail at 0d80f5ca4 on this machine. They are replica-sync and indexer-listener tests that need sockets this host does not give them. Pre-existing, not from this commit.
cargo test -p dynamo-llm --no-default-features --lib: 137 failed at head and 137 at 0d80f5ca4, and the failing sets differ by one name, disaggregated_pair_sharing_an_alias_reports_one_claim_per_role. That test passes 3 out of 3 in isolation on both trees, and its file is not in the commit. It is load-related flake, not a regression.
I did not re-run the offline replay byte-compare this round, and here is why rather than a claim I did not test. Every behavior change in this commit sits behind create_reservation, free_reservation, or the peer-admitted observer. A grep over lib/llm/src and lib/mocker/src finds no call to create_reservation or free_reservation at all. The frontend and the offline replay reach the core through select_and_reserve (kv_router/routing_host/occupancy.rs:32). So the replay path cannot observe these changes. The one change that the embedded partition can reach, the displaced-lease drop, moves lock scope only and leaves the routing outcome alone.
Findings, and the one I chose not to file
One P3 open, one P3 closed as a nit. No P0 and no P1.
- P3 open,
lib/llm/src/kv_router/embedded.rs. The"default"fallback string. Re-measured at this head and unchanged. Therouter_queue_*modellabel moves fromunknowntodefault, and an operator whoserouter_policy_configkeys a modeldefaultgets a different queue policy profile on the stock path. Full measurement and its control are on the thread. - P3 closed as a nit,
.github/workflows/pre-merge.yml. The isolated-feature guard still checks onlystandalone-slot-trackerandstandalone-indexer.standalone-selectiondoes build on its own today, and I re-ran that check above.
One thing I chose not to file. lib/kv-router/src/scheduling/queue.rs:3310 still says "Lazily register two workers in the slot tracker (EPP supplies pod list)" inside a test the commit renamed away from that concept. It is a stale comment and nothing more.
Approving. jthomson04 has changes requested on this pull request, and that remains the decision of record.
atchernych
left a comment
There was a problem hiding this comment.
Non-blocking cleanup notes on the indexer lookup API — two default-argument wrappers lost their last production callers in this refactor. Not asking for changes, just flagging before they calcify.
Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Integrate main through 4df3f3a while retaining the shared selection core. Preserve prefill completion observation, independent cancellation, and video contract discovery; adapt the new prefill regression to the non-generic router. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
|
/ok to test 3165ff2 |
Integrate main through adfe2aa, preserving session-prefix tracking through the shared indexer and selection core. Move the session update lane and recovery tests to the shared crate and cover frontend session tracking for both local indexer implementations. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Include the vLLM kvwarm fix from 3dd603b unchanged. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
|
/ok to test c66159b |
Avoid cloning the complete worker snapshot on every reconciliation pass. Borrow worker requests while computing changes and retain the owned snapshot for retries, cloning only records that need an upsert. Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
|
/ok to test 1b82fe2 |
Summary
Unifies frontend, standalone SelectionService and EPP selection around one
SelectionCorewith shared scheduling, worker membership, KV indexing and the affinity table implementation. This removes the parallel runtime scheduler and frontend selection pipeline. Host affinity policy around dispatch, retries, table exhaustion and prefill coordinator ownership still differs and is tracked as follow-up work.This draft brings the complete aggregate-serving stack from fork PRs #45–#53 to
ai-dynamo/dynamo:main, implementing DEP #14310. It uses the exact head of combined fork PR #55, retains the existing stack for review history, and excludes the separate DisaggCoordinator PR #40.Current scope after review
c71f3d3d11; it is tracked in DEP #14310. Existing standalone indexer HTTP APIs and Dynamo-native remote indexing remain. Historical validation below describes the feature set at each named head.fd09b4bf70to preserve non-KV affinity and existing Python kwargs. Broader affinity-policy consolidation is a separate follow-up.Implementation
SelectionCore::run_selection; the frontend's own copy of the pipeline (index and shared-cache lookup, LoRA narrowing, transfer hints, booking) and the oldKvSchedulerare deleted, and selector generics and temporary adapters are replaced withSelectionPolicySourceandSelectionHost.WorkerCatalogSource/CatalogReconciler, including EPP data-parallel ranks; metadata updates preserve surviving bookings and unchanged listeners.IndexerandKvIndexSourcecontract for event-driven, approximate and remote indexing, withRuntimeIngressandZmqDirectIngressproviding host-specific event transport.Walkthrough
Selection and state ownership
SelectionPolicySourceresolves the configured registry policy or an explicit factory when the partition is built.SelectionHostsupplies the runtime-specific load, eligibility, shared-cache, telemetry and replication inputs while the core owns scheduling and bookings. Indexed overlap and optional shared-cache hits feed selection alongside caller constraints. LoRA narrowing preserves the caller's allow-set and allowed pin; structured session metadata takes precedence over the legacy flat session ID.The same partition index serves initial selection, dequeue-time overlap refresh and routing-decision recording. Event transport remains behind the ingress interface: runtime recovery stays in the frontend ingress, and standalone ingress owns direct ZMQ worker subscriptions. Remote indexing reads the primary without opening local worker subscriptions.
Membership and request lifecycle
CatalogReconcilerapplies desired worker snapshots, retries incomplete registration and removes withdrawn workers. Upserts prepare records before taking the mutation lock, so independent policy construction can proceed concurrently; committed updates reconcile only removed or changed ranks and endpoints. An empty partition keeps its index so a rejoined worker feeds the index the scheduler still holds.A booked request with a session ID and no explicit pin or affinity target acquires the partition's binding before selection. The selected worker commits a new binding; a stale binding can be invalidated and rebound after worker withdrawal. Waiting for another initializer honors cancellation. Advisory selection reads existing bindings and projected load without queue admission, booking or affinity initialization.
The selection id is claimed in the reservation index before booking and the reservation is installed after the affinity commit, so a rejected commit leaves no booking behind. Explicit free and stale-record cleanup drop the reservation's affinity lease through the same ownership path. Standalone bookings retain periodic expiry; frontend bookings retain
RequestLeaseManagerexpiry ownership, and inbound replica events start only once that manager exists. Enabled affinity is partition-scoped, preventing equal session IDs in different models or routing groups from colliding; frontend wrappers borrow that table and share replica initialization and coordinator lifetime.DEP feedback and remaining design differences
Tom's affinity ownership proposal is implemented at the partition boundary, but tables still initialize lazily when enabled rather than existing unconditionally. Builtin routing modes without a selection partition retain their host-owned table. EPP uses hard affinity because it cannot observe the actual dispatch outcome.
The other proposed host-boundary removals are not implemented in this draft. Availability and overload callbacks retain transport inhibition, immediate overload feedback, backend KV occupancy and the two-signal latch, which catalog and slot state alone do not contain. The prefill estimator still resides in
HostLoad; moving it to partition construction remains outstanding.Controller-aware LoRA filtering still carries
replica_setandis_activesemantics beyond loaded-adapter metadata. ModelManager still owns shared-cache subscriptions to preserve configuration gates, overrides and reconnect behavior; the proposed config-source/core-owned-client split remains outstanding. The LoRA scorer/plugin redesign was explicitly a follow-up. These are reviewed design differences, not resolved DEP comment threads.The reservation lease leak, cross-partition affinity collision, sole-worker rejoin failure, metadata-update state loss and Python TTL panic were reproduced before their fixes.
Validation
Final review cleanup (
6bef1c458d)3b40f4df3epreserves main's frontend early-peer accounting while leaving standalone policy unchanged.9d1859a2cffences claim cleanup/installation and retains booking rollback on rejected installation.6bef1c458dyields after bounded repeated affinity invalidations while retaining Hard binding checks.Historical validation
Validated source head:
b91015f183bc159e1fbf6968f2cc967cbbc0b23f. The benchmark baseline and stack merge base areab58558c2dff28f4ff1ea4e219030eac5da3f48f. Opening this upstream PR changes neither source nor tested tree.At opening, upstream
mainwas7650b765fda7e411650393d3265d0a6ce0fb0b33, 12 commits beyond the tested baseline, with one import conflict inlib/llm/src/entrypoint/input/http.rs. The branch has since mergedorigin/mainat464ea4f82a0c(commit8071b3e1c6), resolving that conflict, and the validation below applies to the reviewed headb91015f183plus the review follow-up commits listed next.Review follow-up (after
b91015f183)Fixes for findings from the inline review, each as its own commit:
efb4c7e7bd).router_replica_sync, and the direct-ZMQ active-sequence ingress (DYN_ROUTER_ACTIVE_SEQUENCE_DIRECT_ZMQ,DYN_ROUTER_ACTIVE_SEQUENCE_ZMQ_RCVHWM, its metrics) is restored rather than removed (533ff450aa).standalone-slot-trackerfeature builds in isolation, with a pre-merge CI step for the isolated feature builds (ab4c959628).max_num_batched_tokensdefault to the scheduler's fallback capacity instead of becoming unschedulable (0c0096d780).76c4374177,e5fe3f00a1).960500e469,3003cb8ae0); KV listeners lost to a cancelled worker update are re-registered and their stale index blocks purged (92ea625801,b4a81a5d40).testingfeature; dead re-exports andLoadEstimatorConfig::poll_intervalremoved (20c307dd86).Behavior changes relative to the reviewed head, all in the standalone service and EPP unless noted:
ca0a0c6a1d).x-dynamo-session-idvalues return 400 from the EPP instead of 503 (17c73cc2b3).not_readyinstead of 500, with a 2 s connect timeout (9493e06e2b).DYN_EPP_DATA_PARALLEL_SIZEmust be 1, and a multi-rank standalone worker that setsreplay_endpointstaysincomplete(774115241b)./query_tiered_by_hashrejectscache_saltwith 400 like/query_by_hash(f81f949832).960500e469).Docs corrected for the
kv_hintenvelope, builder method names, lru fallback,advisorybeing ignored byselect_and_reserve, replication requiring a replica mesh, TTL bounds andhost_pinnedcasing.Gate at head
b4a81a5d40(merged withorigin/main464ea4f82a0c)cargo test --locked -p dynamo-kv-routerwithstandalone-selectionand withstandalone-indexer; isolatedcargo check --no-default-featuresforstandalone-slot-trackerandstandalone-indexer;cargo test --locked -p dynamo-ext-proc: all pass.cargo test --locked --workspace --all-targets: 51 test binaries pass;dynamo-llm --test preprocessorfails only under parallel test threads because two non-#[serial]embedding tests race the#[serial]DYN_EMBEDDING_TOKENIZATION_ADD_SPECIAL_TOKENStest (the files are identical toorigin/main; the binary passes alone, 40/40).cargo clippy --locked --workspace --all-targets -- -D warnings: pass.lib/bindings/python:cargo test --locked --all-targetspasses; with--features select-servicethe lib test binary passes (58 tests) once libpython is linked explicitly, as in the original validation.pytest tests/router/test_router_e2e_with_mockers.py(serial, bindings built withkv-indexer,slot-tracker,select-service,aic-forward-pass, pinned etcd v3.5.33 and nats-server v2.12.14): 50 passed, 6 skipped (pre-existingFlaky on CIskip on the TCP round-robin path). Covers routing decisions, overload 529, session affinity, two-router indexer sync, disaggregated startup and routing, AIC, busy threshold, EPP direct-mode headers, per-worker config and taints.Offline replay parity (
.agents/skills/dynamo-kv-replay-parity) was not rerun:lib/kv-router/src/scheduling,sequences,lib/mockerandlib/benchare byte-identical to the reviewed headb91015f183, and offline replay does not pass throughSelectionCore, so the campaign would compare identical code.All-target check passed:
cargo check -p dynamo-kv-router -p dynamo-llm -p dynamo-ext-proc --no-default-features --features dynamo-kv-router/standalone-selection,dynamo-kv-router/standalone-slot-tracker --all-targets.Python bindings check passed:
cargo check --manifest-path lib/bindings/python/Cargo.toml --features select-service --all-targets.Rust suites: 1,115 KV-router tests, 2,420 LLM tests with no default features, and 154 EPP tests passed (3,689 total; three ignored). Strict core Clippy passed.
Python: 23 targeted routing, affinity, custom-policy and stream-error checks passed. All 16 focused SelectionService bindings tests passed, including 13 TTL cases; the isolated Rust shutdown test passed with explicit libpython linking.
Regular/sidecar and custom-policy binding checks passed. Bindings all-target Clippy passed with
kv-indexer,slot-tracker,select-service. All nine fork stack boundaries passed the Rust compile check above; bindings withselect-servicealso passed at fork PRs chore: remove ucx-py from requirements and fix UCX env variable #46–chore: address comments for #35 #53.Frontend cutover to the shared core (after
b4a81a5d40)The frontend now runs every selection through
SelectionCore::run_selectioninstead of its own copy of the pipeline; commits in order:e2e4e618d8(lifecycle-lease guard and claimed selection ids),fe816fc6a2(shared affinity commit and mode),9740a38ec1(one core operation),d6b1c8f6cc(hint and readiness hot paths,kv_event_source_mode),6616406aae(frontend wired without cutover, golden-trace differential tests),c6702146d4(cutover, frontend pipeline deleted),1172f06f30(affinity coordinator commits through the table),fabac4b322and9c4b3347f7(core split into one module per concern, dead paths deleted),89785a3e7b(replica ingress starts after the lease manager is installed),25aea16655(selection future held once on the cleanup-policy path). Each step had a Graham-style review and an independent adversarial review before commit; findings were applied before pushing.Behavior changes from these commits, frontend unless noted:
select_and_reserveand two-phase reservations.find_best_match_details_without_admissionis removed from the crate API.KvPushRouter::newon a partition another host configured Soft fails at construction.LookupTimings.lookups).SelectionServiceBuilder::session_affinity_mode,LocalScheduler::supports_overlap_refresh,SessionAffinity::new_with_limitsand thefree_if_workerwrappers are removed (no callers).Production-only line delta for these commits (non-test
.rs,#[cfg(test)]modules stripped): +2,843 / −2,653, net +190; the frontendkv_router.rsalone lost 1,196 lines androute_lookup.rsis deleted, while the core gained the lease guard, claims, affinity commit and operation types.Gate at head
25aea16655(the stack fix25aea16655included):cargo test -p dynamo-kv-router --features standalone-selection(1,127 tests),cargo test -p dynamo-llm --lib --tests(2,672 lib tests plus integration binaries),cargo test -p dynamo-ext-proc(156 tests): all pass; isolatedcargo check --no-default-features --features standalone-slot-trackerpasses.cargo clippy --workspace --all-targets -- -D warnings: pass.cargo fmt --check: pass.lib/bindings/pythonwith--features select-service: select-service tests pass with explicit libpython linking.dynamo-mocker,dynamo-bench,dynamo-backend-common,kvbm-consolidator, the custom-policy examples and catalog): 468 tests across 34 binaries pass.pytest tests/router/test_router_e2e_with_mockers.py(bindings built withkv-indexer,slot-tracker,select-service,aic-forward-pass, etcd 3.5.21, nats-server 2.10.28): 50 passed, 6 skipped (the pre-existing TCP round-robin skip). Two overload cases with a 45 s budget timed out once while a bindings build shared the machine and pass alone. The first run of this suite at89785a3e7bfailed every disaggregated case: the frontend segfaulted with a stack overflow inawait_with_cleanup_policy, whose state held the now-larger selection future three times under the prefill router's nesting.25aea16655pins the operation once in that wrapper and boxes the core selection future in the embedded host; a unit test bounds the selection future's size.Review round, structure cleanup and merge of main (after
25aea16655)e22fab5ee7: the eleven review items on the previous head (DashMap guard before an await, directTieredQueryResponseserialization, 8 MiB body limit on the hash routes, reservation drops outside the index lock,Book-only hash copy, LoRA filter intersected with its candidate set, replica writer id stored after the sink installs, pytest timeout marker, the no-opregister_workerschain deleted, two-phase doc corrected).317eb30e86: forty-six small self-review findings (imports, duplicated constants, dead getters, cfg-gated test helpers moved, log fields and casing, borrowed hash lookups, a mutex no longer held across an indexer build).c95307e99egates the LoRA helper so the default-feature build has no dead code.ea037f3b85: the five booking lifecycle invariants are written at the top ofcore/run.rsand the missing tests for them are added: aLeaseadmission installs no index row and records nothing; dropping aBookrun during its routing record frees the booking and the id claim; a public enrollment cancelled during its routing update frees the booking. Each test fails when its guard is removed.1f91e531b0:BookingHandleis the one type a host receives for a booking (descriptor,commit,release,Drop), replacing the boxed lifecycle lease and its fallible commit inSelected, the frontend's selection types,KvRequestCleanup, the public enrollment path andPrefillReservation. The lease is now crate-private and covers only the queued window. A unit test pins the handle's armed contract.bbd8412a91: the frontend's request lease manager is built before the scheduler and passed directly as the partition observer; the late-bound observer shim is gone.45383b4c9f: merge ofmainat0027a8ef0b(75 commits). The indexer registry takes main's cleanup lifecycle mutex andwith_retained_indexers; main's first-wins admission (WorkerSetTarget) is carried onto the non-generic prefill router; main's two new selection-core tests are ported intocore/tests.rs. One documented behavior differs from main and is now described in the standalone-selection and standalone-indexer docs: re-registering a worker whose event endpoints are unchanged keeps its listeners and cache view, so a publisher restart at the same address needsDELETE /workers/{id}before the newPOST /workers. Every test added by the twelve main commits that touch the router, discovery, EPP and recovery paths is present and passes.Gate at
45383b4c9f: fmt, clippy-D warningson kv-router (default andstandalone-selectionfeatures), llm, ext-proc, bindingscargo check --features select-service; kv-router 1140, llm 2743, ext-proc 158, runtime routing policy 9; mocker e2e 50 passed / 6 skipped. GPU functional probes at this head (sa-b200, SGLang 0.5.19, Qwen3-0.6B; aggregated job 14430, disaggregated job 14401): all pass. Aggregated: health, prefix reuse routes 4/4 to the cached worker, distinct prompts spread over both workers, hard session sticky across unary and stream requests, explicit pin to the other worker rejected, cancelled stream then same-session completion, migration after SIGKILL of one worker with 8/8 in-flight streams completed and the survivor taking the next requests; plus the 49-completion smoke and the 34-completion standalone selector probe (catalog failover, scale-to-zero and rejoin). Disaggregated (1P+1D over NIXL): 12/12 requests carry a prefill id from the prefill pool and a decode id from the decode pool; prefix reuse, stickiness and cancellation hold.AgentX agentic replay, 4 replicas (after
27c03ee2a4)DeepSeek-R1-0528 FP8, four TP8/EP8 aggregated replicas on four B200 nodes per arm, KV router with KV events, aiperf
inferencex-agentx-mvpover the SemiAnalysis cc-traces corpus (with sub-agents, traces above 160k context dropped), first-turn-prefix cache bust, 20 minutes per concurrency, both arms run in parallel on separate node sets. Mean prompt 68k tokens, max 160k.Router KV hit rate 0.620 vs 0.615; frontend CPU 4.8% vs 5.1% of one core over the job; event-loop delay mean 1.1 ms on both. Branch ingress funnel: 1.1 events per drain batch, queue depth 0 on every sample, no drops.
Structural review items (after the cleanup rounds)
The six structural threads from the review are addressed in this PR rather than deferred:
9e1e92a87b,d4be24436d: a session commit that finds another request initializing the session joins that initialization and holds a lease on the new binding, instead of proceeding unpinned; a joiner that finishes first is uncounted again. The commit still never waits.29b2b9c632,2dfc529eb2: the worker-selection policy is constructed once per partition (was three times on the model-manager path); the inspected instance is the one that serves.7b2f6c3faf,3cecd2af8d: bookings read hint capability from the published partition snapshot instead of scanning the catalog, and deleted workers leave the catalog (they no longer appear inGET /workersas unschedulable).9a18ffb109,4cc6f49acf: a catalog snapshot reconciles as one batch with one publish per partition (a completed partition move still publishes immediately), continues past a failed record, and a failed snapshot is retried after five seconds unless a newer one arrives.02165d72b1,27c03ee2a4: the active-sequence ingress funnel is instrumented (queue depth, events applied, drain batches, per partition and worker type). At C1024 on 8 workers it drains 4 events per batch with depth 0 on every sample and no drops, so the single apply task is kept; the thread carries the numbers and the decision rule.7b2f6c3faf: per-rank worker gauges clear ranks that leave a worker's data-parallel range;29a05b4833: the remaining mechanical cleanups (shared test fixtures, thevalidate_policy_worker_rolerename,Resultalias).Each change has a test that fails without it; each commit was reviewed before landing.
Cleanup rounds (after
45383b4c9f)Three targeted rounds to shrink the diff without losing a check. Every deleted test names the surviving test in its commit message, and each round was reviewed for lost assertions before landing.
5c2eb04504: test-only shims (find_best_match,find_best_match_details,query_prefill_worker), dead consts and re-exports, thekv_router/scheduler.rsshim; six duplicate tests; 28 repeated core-test constructor literals folded into helpers.63f7ac4998: one selection result shape in the frontend instead of three; ten forwarding methods on the embedded host replaced by an accessor; wrappers and forwarders in the core removed.99f8574f50: moved code restored to main's exact text where the branch had renamed or reworded it; unusedDebugimpls, accessors and a third copy of the TTL range check removed; repeated test literals replaced by helpers; doc comments that restated code removed, three that stated a rule restored.37cfba9b5a: two tests that existed on main and had no successor after the cutover are back (skip_initial_worker_wait_still_monitors_worker_config_updates,active_sequence_replica_sync_isolated_by_endpoint).Against the merged
maincommit0027a8ef0b: before the rounds +14,965 / −8,755 (net +6,210); after +14,399 / −9,461 (net +4,938). Of what remains, about 5,800 added lines are tests, benches and test data; the production growth is the frontend host over the shared core, the catalog and event-ingress reconcile that lets the frontend drive it, the reservation claim transaction, and the session-affinity replication.GPU functional validation at
25aea16655(sa-b200, two B200 GPUs, SGLang 0.5.19, Qwen3-0.6B)Release bindings built with
select-service; frontend--router-mode kv --kv-cache-block-size 16 --router-session-affinity-ttl-secs 60 --router-session-affinity-mode hard --migration-limit 3; worker KV events on. Every probe attributes responses bynvext.worker_id.SelectionServicecompletions with pinned forwarding, catalog failover and scale-to-zero/rejoin, zero bookings left. Functional probes: the same 700-word prefix with four different tails routed all four requests to the worker holding the cache; 16 distinct prompts used both workers; a hard session stuck across three requests and an explicitbackend_instance_idpin to the other worker returned 400; a stream cancelled mid-way left the session usable. Migration: with 8 streams in flight, one worker was SIGKILLed; all 8 completed with no error, the frontend observed the departure in 11 s, and the next 6 requests all landed on the survivor.Paired performance at
25aea16655(same two B200 GPUs, alternating fresh deployments)Three main/branch pairs (jobs 13776–13781), each a fresh two-TP1-worker deployment, AIPerf v0.12.0 chat streaming, C16, 512 measured requests plus 32 warmups, ISL 512, OSL 128 forced, seed 42. All six runs completed 512/512 with zero errors.
Means over the three pairs: request throughput main 58.16 vs branch 56.27 req/s (−3.3%); TTFT p50 unchanged (43.0 vs 43.0 ms); TTFT p95 lower on the branch (69.6 vs 58.3 ms); ITL p50 higher on the branch (1.76 vs 1.86 ms, +5.9%). Run-to-run spread within an arm is of the same order as the gap (main 56.2–60.3 req/s), so this is a small, consistently-signed difference rather than a measured regression of known size. This round's
mainarm was the earlier baselineab58558c2d; the branch's actual merge base is464ea4f82a, so the baseline was rebuilt there and the three pairs rerun:464ea4f82a464ea4f82a464ea4f82aAgainst the real merge base (jobs 13803–13808, same protocol, all 512/512 with zero errors): request throughput main 56.42 vs branch 55.30 req/s (−2.0%), TTFT p50 unchanged (43.0 ms both), ITL p50 1.81 vs 1.88 ms (+3.5%). Across both rounds the branch trails main in five of six pairs and ties in one; the sign is consistent, the size is within the arms' own run-to-run spread, and the difference sits in the per-token path rather than in selection (TTFT is flat). A per-token profile of the frontend stream path is the follow-up; these runs do not establish zero overhead, statistical equivalence or production-scale capacity.
Extended performance at
25aea16655(sa-b200, one B200 node per run)High concurrency, 8 x TP1 Qwen3-0.6B, KV routing (jobs 13894/13895, 13917/13918, 13919/13920; three alternating main/branch pairs; AIPerf chat streaming, ISL 512 / OSL 128 forced, 2048/3072/12288 requests at C64/C256/C1024; all runs zero errors). Means over the three pairs:
Throughput and ITL are at parity at every concurrency. TTFT p50 at C1024 is higher on the branch in all three pairs (158/133/160 ms main vs 302/177/169 ms branch; +18% with the first pair's outlier excluded), and the frontend's own router TTFT histogram agrees (mean 175-202 ms main vs 201-270 ms branch over 18,080 requests per run). Event-loop delay (1.09 ms mean both arms), queued requests (0) and KV hit rate (0.11 both) are unchanged, and the scheduler request the core sends differs from the pre-cutover request only in always carrying
block_hashes. This is the one open performance item; at C64 and C256 TTFT is equal.InferenceMAX-style 8k/1k, DeepSeek-R1-0528 FP8, TP8/EP8 aggregated (jobs 13896 main vs 13897 branch; sa-bench ISL 8192 / OSL 1024, 3 x concurrency requests at C32/64/128/256; frontend, worker, DCGM and node telemetry scraped at 1 s):
Throughput is identical to three digits at every point and TPOT is within 2%; TTFT p99 is within 1% at all four points. Over the whole run the frontend's router TTFT mean (12.26 s vs 12.25 s over 2409 requests), frontend ITL mean (57.4 vs 57.3 ms over 2.2 M tokens) and event-loop delay (1.09 vs 1.10 ms) match. At this model size the frontend is not on the critical path.
Agent-style trace replay, 8 x TP1 Qwen3-0.6B, KV routing (jobs 13947 main vs 13948 branch; AIPerf
mooncake_tracereplay of the first 3,000 requests of the Mooncake tool-agent trace, mean ISL 7,089 / mean OSL 171 tokens with shared prefixes, replayed at the trace's own arrival rate at C32/128/512, so throughput is fixed at 5.49 req/s and latency is the comparison; all 18,000 requests completed with zero errors):The frontend's router KV hit rate is 0.62 on both arms over the 9,197 routed requests per run, so the cutover reproduces main's routing decisions on a prefix-heavy workload; router TTFT mean 65.6 vs 64.2 ms, frontend ITL mean 1.66 ms both, event-loop delay 1.06 vs 1.07 ms.
Frontend CPU profile (jobs 13944/13946 at 40 stops and 13949 at 120 stops per arm; 8 x TP1 Qwen3-0.6B started from one shell on one node, AIPerf C256, ISL 512 / OSL 128;
perfis unavailable on the nodes, so the frontend runs under gdb from launch and a driver stops it at fixed intervals during steady load to capture every thread's stack; an unprofiled control run at the same load measures frontend CPU from/proc):464ea4f82a25aea16655464ea4f82a25aea16655Frontend CPU per request is the same in both rounds (6.52 vs 6.52 ms, then 6.84 vs 6.79 ms over 38,400 requests each); throughput, TTFT and ITL are within run noise and the branch uses 80-110 MB less resident memory. In the 120-stop on-CPU stacks (256 main / 204 branch busy thread samples) the routing path (
kv_router+routing_host, including selection and booking) is 12% of on-CPU samples on main and 9% on the branch; HTTP/SSE serialization, the response stream, preprocessing and postprocessing account for the rest in both arms. The 40-stop branch run showed two stops with 12-22 threads queued on the glibc malloc arena lock; the 120-stop run has 4 (main) vs 2 (branch) such thread samples, so that was a one-off burst rather than a property of the branch. Nothing in the profile attributes cost to the new selection core, and the C1024 TTFT p50 gap above remains unexplained by frontend CPU; it is the one open item.Aggregated end-to-end validation
Two B200 GPUs ran two TP1 aggregated SGLang 0.5.19 workers serving Qwen3-0.6B, with a KV-aware Dynamo frontend, hard session affinity and worker ZMQ KV events enabled. Baseline was
ab58558c2dff28f4ff1ea4e219030eac5da3f48f. The GPU candidate was built at63e998744f53194c7a1a6a99adbc6e5c5c6bb317, whose Git tree exactly equals this published headb91015f183bc159e1fbf6968f2cc967cbbc0b23f; the later change only moved a bindings compatibility fix to its earliest stack boundary. Both arms used identical installed package versions, model revision and deployment settings.SelectionServicebooked workers, requests were forwarded through the frontend with explicit worker pins, and every response's actual worker matched the selection. The probe exercised repeated sessions, metadata refresh, catalog withdrawal/failover, catalog scale-to-zero/rejoin and reservation release, finishing with zero active bookings.The standalone probe used approximate indexing and pinned frontend forwarding. EPP was covered by its Rust suite, not a live Envoy/Kubernetes deployment. This validation was aggregated only; no disaggregated serving test or separate DisaggCoordinator work is claimed. All benchmark jobs completed and compute was released.
Prompt artifacts were byte-identical between matched arms. Short-run prompts were 512 client tokens / 520 server tokens after chat framing; two prompts per longer arm retokenized to 513/521, with matching histograms on both arms. Client output retokenization varies slightly around the verified 128 server tokens, so request throughput is the primary comparison.
The small measured throughput reduction consistently favors main. These runs do not establish zero overhead, statistical equivalence or production-scale capacity.
The fork staging series had CI failures from the missing fork Fastchecker runner group, helm-tester image and approval-bot token, plus published-docs composition and an unchanged external documentation link. Those are historical fork results; upstream CI has not yet been evaluated. The local compilation and final-stack validation above are separate from GitHub CI.
Summary by CodeRabbit