Skip to content

fix(runtime): make push router direct dispatch exact - #10429

Merged
kthui merged 3 commits into
mainfrom
jacky-ft-migrate-worker-removed
Jul 30, 2026
Merged

fix(runtime): make push router direct dispatch exact#10429
kthui merged 3 commits into
mainfrom
jacky-ft-migrate-worker-removed

Conversation

@kthui

@kthui kthui commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Make PushRouter::direct() an exact-dispatch API. Once a caller supplies an instance_id, dispatch targets that worker or returns typed CannotConnect; it no longer silently reselects another worker if the selected instance disappears before transport resolution.

Transport resolution now also precedes overload checking, so stale overload metadata cannot mask a missing exact target and availability is checked against the final worker selected by transport resolution.

This keeps routing and migration responsibilities separate: routing selects a worker, direct() honors that selection, and migration may retry after a typed worker-specific failure.

Details

  • Change direct() to dispatch with TransportFallback::Deny.
  • Return DynamoError(ErrorType::CannotConnect) when an exact target is no longer discoverable, preserving migration error classification.
  • Keep fallback behavior available through the explicitly fallback-enabled APIs:
    • direct_within(..., None) permits ordinary fallback.
    • direct_within(..., Some(ids)) constrains fallback to the supplied worker set.
  • Preserve CannotConnect when a fallback worker was selected but disappears before its transport lookup completes.
  • Leave an ambiguous no-fallback-candidate result untyped. Because free_ids() can be empty due to no discoverable eligible worker, policy exclusion, or pool-wide overload, that branch retains its existing generic error and carries a TODO for fix(migration): distinguish worker-local overload from pool-wide exhaustion #12383.
  • Resolve transport before calling check_workers_available() in both unary/prepared and bidirectional dispatch. The overload check therefore applies to the final resolved worker and returns ResourceExhausted only after a worker has been pinned.
  • Clarify the exact versus fallback-enabled API contracts in the Rust documentation.
  • Extend regression coverage for fallback policy, error classification, and resolve-before-overload ordering.
  • Leave the KV-router call site unchanged; it already calls direct(), so the exact-dispatch contract is enforced in the shared runtime API.

This complements #11993: that change allows an already-selected, locally inhibited worker to remain directly dispatchable while it is still discoverable; this change handles the later race where the selected worker disappears before transport resolution.

The broader distinction between worker-local overload and pool-wide capacity exhaustion is intentionally deferred to #12383.

Validation

  • cargo fmt --manifest-path /workspace/Cargo.toml --all -- --check
  • cargo test -p dynamo-runtime transport_resolution -- --nocapture (4 passed)
  • cargo test -p dynamo-runtime exact_dispatch -- --nocapture (1 passed)
  • cargo test -p dynamo-runtime --lib (497 passed, 2 ignored)
  • cargo test -p dynamo-runtime direct_dispatch_ignores_local_inhibition -- --nocapture
  • cargo test -p dynamo-llm router_request_counters_follow_admission_and_completion_lifecycle --lib -- --nocapture
  • cargo clippy -p dynamo-runtime --no-deps --all-targets -- -D warnings
  • cargo clippy -p dynamo-llm --no-deps --lib -- -D warnings
  • pre-commit run --files lib/runtime/src/pipeline/network/egress/push_router.rs

Reviewer guide

Start with lib/runtime/src/pipeline/network/egress/push_router.rs, especially:

  • direct() and its TransportFallback::Deny contract;
  • resolve-before-overload ordering in generate_with_fault_detection_prepared() and bidirectional_dispatch();
  • the Deny, selected-fallback lookup failure, and ambiguous no-candidate branches in resolve_transport();
  • transport_resolution_precedes_stale_overload_check and transport_resolution_honors_fallback_policy.

Related issues

@github-actions github-actions Bot added the fix label Jun 8, 2026
@kthui kthui changed the title fix: fix: Silent fallback to another worker and make request to removed-worker migratable Jun 8, 2026
@kthui
kthui marked this pull request as ready for review June 8, 2026 19:56
@kthui
kthui requested a review from a team June 8, 2026 19:56
@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

PushRouter routing behavior is refactored to rely on typed failures instead of pre-validation and silent fallback. direct() removes eligibility checks, resolve_transport() errors on missing instances, and a new report_instance_down_if_inhibited() helper centralizes fault reporting. Tests verify typed CannotConnect failures for removed instances and confirm no fallback behavior.

Changes

PushRouter typed-failure routing

Layer / File(s) Summary
Direct routing and transport resolution refactoring
lib/runtime/src/pipeline/network/egress/push_router.rs
PushRouter::direct() removes pre-dispatch eligibility checks and delegates correctness to typed failures. resolve_transport() removes fallback-to-another-free-instance behavior and returns explicit CannotConnect error when the selected instance_id cannot be resolved.
Instance-down reporting helper and integration
lib/runtime/src/pipeline/network/egress/push_router.rs
New report_instance_down_if_inhibited() helper centralizes the logic for reporting inhibited worker errors as instance-down. The helper is integrated into wrap_with_fault_detection() for both immediate dispatch failures and stream item error paths, replacing prior inline is_inhibited checks.
Test updates for typed-failure semantics
lib/runtime/src/pipeline/network/egress/push_router.rs
Tests verify that direct calls targeting removed instances return typed DynamoError with ErrorType::CannotConnect and inhibited classification. Transport resolution tests confirm typed failures, suppression/quarantine behavior, and that no silent fallback occurs when the selected instance disappears between selection and dispatch.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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 is concise and accurately captures the main change: exact direct dispatch without silent fallback.
Description check ✅ Passed The description covers overview, details, reviewer guidance, validation, and issue links, though the heading names differ from the template.

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

Comment thread lib/runtime/src/pipeline/network/egress/push_router.rs Outdated
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions Bot added the Stale label Jul 9, 2026
@github-actions github-actions Bot removed the Stale label Jul 20, 2026
Signed-off-by: Jacky <18255193+kthui@users.noreply.github.com>
@kthui
kthui force-pushed the jacky-ft-migrate-worker-removed branch from b3d2825 to 43ec8ae Compare July 28, 2026 21:59
@kthui
kthui requested a review from a team as a code owner July 28, 2026 21:59
@pull-request-size pull-request-size Bot added size/M and removed size/L labels Jul 28, 2026
@datadog-official

datadog-official Bot commented Jul 28, 2026

Copy link
Copy Markdown

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 39.62% (-3.33%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 1c20260 | Docs | Datadog PR Page | Give us feedback!

@kthui kthui changed the title fix: Silent fallback to another worker and make request to removed-worker migratable fix(runtime): make direct dispatch exact Jul 28, 2026
@kthui kthui changed the title fix(runtime): make direct dispatch exact fix(runtime): make push router direct dispatch exact Jul 28, 2026
@kthui
kthui requested review from jh-nv and nnshah1 July 28, 2026 22:24
@kthui kthui self-assigned this Jul 28, 2026
@kthui
kthui requested a review from PeaBrane July 28, 2026 22:28

@PeaBrane PeaBrane left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One non-blocking edge case below; fine to defer it as a follow-up TODO.

Comment thread lib/runtime/src/pipeline/network/egress/push_router.rs
Signed-off-by: Jacky <18255193+kthui@users.noreply.github.com>
@pull-request-size pull-request-size Bot added size/L and removed size/M labels Jul 29, 2026

@PeaBrane PeaBrane left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kthui I traced this through the runtime error producers and the KV migration/re-dispatch path. My earlier ResourceExhausted comment was broader than the stale-overload race fixed in the latest commit.

I think the original exact-dispatch changes in 43ec8ae are semantically sound and should stay:

  • direct(instance_id) must honor the upstream-selected worker; transport fallback must not silently change KV-router selection, booking, or DP-rank state.
  • If that exact target disappears before transport resolution, typed CannotConnect is correct and migratable.
  • Resolving transport before checking overload is also correct: stale overload metadata should not mask a genuinely missing target, and any overload check must apply to the final resolved worker.

The part I think should be rolled back or narrowed is the blanket conversion of every unresolved Allow/Within fallback to CannotConnect. free_ids() can be empty because otherwise eligible, live workers are overloaded. That is a capacity failure, not a discovery/transport failure; classifying it as CannotConnect makes a pool-wide exhaustion migratable and can change its HTTP/error semantics. An actual failed lookup—including Deny for a missing exact target or a selected fallback that also disappeared—can remain CannotConnect.

The broader issue is that ResourceExhausted currently represents both a single selected worker being busy and the entire eligible pool being exhausted. The former should permit reselection when policy allows; the latter should remain non-migratable. That needs its own worker-scoped overload classification and should build on #11647's worker-targeted booking/admission cleanup, rather than relabeling overload as CannotConnect.

I opened #12383 to track that separate follow-up. So for this PR: keep exact dispatch, typed missing-target failure, and resolve-before-overload ordering; defer the worker-local versus pool-wide overload split, and avoid typing ambiguous no-fallback capacity cases as CannotConnect.

@kthui
kthui disabled auto-merge July 29, 2026 21:24
Signed-off-by: Jacky <18255193+kthui@users.noreply.github.com>

kthui commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @PeaBrane — confirmed. Current HEAD (1c202605) now matches this scope:

  • Exact dispatch stays exact: direct() and dispatch_exact() use TransportFallback::Deny, so KV-router’s selected worker is not silently replaced.
  • Actual missing-target failures stay typed: a missing exact target, or a fallback selected from free_ids() that disappears before lookup completes, returns CannotConnect.
  • Transport resolution precedes overload checking: both unary/prepared and bidirectional paths resolve the final worker first, then check that worker’s availability.
  • Ambiguous fallback exhaustion remains untyped: when free_ids() yields no fallback candidate, we do not classify it as CannotConnect, because that can also represent pool-wide overload or policy exclusion. The branch has a TODO linking fix(migration): distinguish worker-local overload from pool-wide exhaustion #12383.

This keeps the PR focused on exact dispatch and correct ordering while deferring worker-local versus pool-wide overload classification to #12383. The targeted transport_resolution tests (4) and exact_dispatch test pass.

@kthui
kthui enabled auto-merge (squash) July 29, 2026 21:38
@kthui
kthui merged commit ddf2b8e into main Jul 30, 2026
302 of 315 checks passed
@kthui
kthui deleted the jacky-ft-migrate-worker-removed branch July 30, 2026 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants