Skip to content

fix(routing): scope context-overflow history by session and agent - #298

Open
elyasmnvidian wants to merge 1 commit into
mainfrom
emehtabuddin/p0-routing-resilience
Open

fix(routing): scope context-overflow history by session and agent#298
elyasmnvidian wants to merge 1 commit into
mainfrom
emehtabuddin/p0-routing-resilience

Conversation

@elyasmnvidian

@elyasmnvidian elyasmnvidian commented Aug 5, 2026

Copy link
Copy Markdown
Contributor
schema_version = 1

[llm_clients.mock]
format = "openai_chat"
base_url = "http://127.0.0.1:<port>/v1"
max_retries = 0

[targets.weak]
id = "model/weak"
llm_client = "mock"

[targets.strong]
id = "model/strong"
llm_client = "mock"

[routes.random]
id = "switchyard/random"
type = "random"
targets = ["weak", "strong"]
weights = [1, 0]

With a local provider where model/weak rejects overflow for context length and
model/strong succeeds, send these requests under one session:

child-a (agent=child-a): input=overflow
parent (root):           input=fits
child-b (agent=child-b): input=fits

The bug

One oversized child request marked weak overflowed for the whole session, so the parent
and child-b then skipped weak even though their shorter requests fit:

child-a calls: weak, strong; selected strong
parent calls:  strong        # wrong: weak was never tried
child-b calls: strong        # wrong: weak was never tried

The overflow history keyed every request by session ID alone. A child's overflow was
therefore recorded against the session and excluded weak for the parent and every sibling.

The fix

SessionEvictions now uses the same crate-private routing identity as AffinityRouter:

  • a root request uses its session ID;
  • a child request uses its session and agent IDs.

Both paths build that identity with RoutingIdentity::from_request. A child missing either ID
keeps no overflow history instead of sharing the parent's. When the host marks the session
final, Switchyard clears the root and every child's history.

random, llm_classifier, and stage_router route their final model call through
FallThrough, so all three receive the fix. passthrough and noop are unchanged.

After

child-a calls:     weak, strong; selected strong
child-a next turn: strong
parent calls:      weak; selected weak
child-b calls:     weak; selected weak

Proof

The same production-path regression test was applied to both revisions without copying the
production fix:

origin/main 31e0afb2: FAIL
left:  Some("model/strong")
right: Some("model/weak")

PR 7dd20f81: PASS

The test starts a local HTTP provider and drives the real header parser, route, client error
mapper, session-final cleanup, and missing-agent behavior. It does not call a live provider.

Checks

cargo fmt --all
cargo fmt --all --check
cargo clippy --workspace --all-targets -- -D warnings
cargo test -p switchyard-libsy
cargo test -p switchyard-server --test server overflow_history_is_scoped_to_agent_and_session_lifetime -- --exact
cargo test --workspace --exclude switchyard-py
uv run ruff check .
uv run mypy switchyard
uv run pytest tests/ -m "not integration"

The full cargo test --workspace command reaches the known local macOS PyO3 link failure for
switchyard-py; the workspace run excluding only that extension crate passes.

Scope

An earlier version of this PR also failed over to the next target on an unavailable target.
That is a separate change and will come in its own PR. This PR only fixes context-overflow
history isolation.

@elyasmnvidian
elyasmnvidian requested a review from a team as a code owner August 5, 2026 10:16
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1

🚀 View preview at
https://NVIDIA-NeMo.github.io/Switchyard/pr-preview/pr-298/

Built to branch gh-pages at 2026-08-06 09:56 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
crates/libsy/src/algorithms/fall_through.rs (1)

344-347: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Align tier resolution on the fallback path with route.

route reads the tier from the deciding classifier only (deciding.and_then(|c| c.routing_tier(...))). fallback_decision reads it from the first classifier in the cascade that returns a tier for the replacement model. In a cascade with more than one tier-defining classifier, the two paths can report different tiers for the same model. tier is a metrics and routing-log label, so the same model then splits across two label values.

Thread the deciding classifier into call_llm_with_fallback and reuse it here, so both decisions name the tier the same way.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/libsy/src/algorithms/fall_through.rs` around lines 344 - 347, Update
fallback tier resolution in call_llm_with_fallback to use the deciding
classifier passed through from the fallback decision, matching route’s
deciding.and_then(...). Thread that classifier into the fallback call and
replace the classifiers.iter().find_map lookup so both paths report the same
tier for a model.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@crates/libsy/src/algorithms/fall_through.rs`:
- Around line 344-347: Update fallback tier resolution in call_llm_with_fallback
to use the deciding classifier passed through from the fallback decision,
matching route’s deciding.and_then(...). Thread that classifier into the
fallback call and replace the classifiers.iter().find_map lookup so both paths
report the same tier for a model.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: a3a5f786-07cd-46ea-9a21-60190bf9ffe7

📥 Commits

Reviewing files that changed from the base of the PR and between c1c1b41 and 820ca94.

📒 Files selected for processing (11)
  • crates/libsy/src/algorithms/fall_through.rs
  • crates/libsy/src/algorithms/util/affinity.rs
  • crates/libsy/src/core/algorithm.rs
  • crates/libsy/src/core/classifier.rs
  • crates/protocol/src/client.rs
  • crates/switchyard-server/src/lib.rs
  • crates/switchyard-server/src/routing_log.rs
  • crates/switchyard-server/src/stats/accumulator.rs
  • crates/switchyard-server/src/usage_metrics.rs
  • crates/switchyard-server/tests/server.rs
  • docs/internal/metrics_reference.md

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

Fall-through routing now tracks overflow by routing identity, retries unavailable targets without rerouting, invalidates stale affinity assignments, and reports fallback reasons through decisions, logs, statistics, tests, and metrics documentation.

Changes

Fallback routing

Layer / File(s) Summary
Routing identity and fallback contracts
crates/libsy/src/core/algorithm.rs, crates/libsy/src/core/classifier.rs, crates/libsy/src/algorithms/util/affinity.rs, crates/protocol/src/client.rs
Requests now derive session or subagent routing identities. Classifiers can invalidate unavailable targets. Decisions expose context-window and unavailable fallback reasons.
Fall-through retry and overflow handling
crates/libsy/src/algorithms/fall_through.rs
Fall-through routing tracks bounded overflow history, excludes affected targets, classifies failures, retries alternate targets, and preserves the final error when no target remains.
Fallback logging and statistics
crates/switchyard-server/src/lib.rs, crates/switchyard-server/src/routing_log.rs, crates/switchyard-server/src/stats/accumulator.rs, crates/switchyard-server/src/usage_metrics.rs
Fallback reasons flow from routing decisions into usage records, routing logs, and serialized statistics.
End-to-end fallback validation
crates/switchyard-server/tests/server.rs, docs/internal/metrics_reference.md
Integration tests cover unavailable-target failover, request formats, statistics reset, and child-specific overflow history. Metrics documentation describes routing_fallbacks.

Estimated code review effort: 4 (Complex) | ~60 minutes

Poem

I’m a rabbit watching targets hop,
Overflow marks make weak paths stop.
When one goes down, the next runs through,
Logs count the reason, stats do too.
Identity keeps each burrow right—
Fallback routing works tonight!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 52.63% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the primary change: scoping context-overflow history by session and agent identity.

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/libsy/src/algorithms/fall_through.rs (1)

389-408: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Remove the .expect() on line 408.

The coding guidelines forbid .expect() in production Rust source. The invariant is correct today because deciding and maybe_score are assigned in the same loop iteration, but the pairing is implicit. Carry the score and the deciding classifier in one Option so the invariant is structural and no panic path remains.

♻️ Proposed refactor to pair the score with its classifier
-        let mut maybe_score: Option<Score> = None;
-        let mut deciding: Option<Arc<dyn Classifier<S>>> = None;
+        let mut decided: Option<(Score, Arc<dyn Classifier<S>>)> = None;
         let mut served: Option<Response> = None;
         for classifier in &self.classifiers {
             let (scores, response) = classifier.score(state, request, Some(driver)).await?;
-            maybe_score = scores.argmax(false)?;
-            if maybe_score.is_some() {
-                deciding = Some(Arc::clone(classifier));
+            if let Some(score) = scores.argmax(false)? {
+                decided = Some((score, Arc::clone(classifier)));
                 // Only the deciding classifier's response answers the turn; an abstaining
                 // classifier selected nothing for it to be the answer to.
                 served = response;
                 break;
             }
         }
-        let Some(score) = maybe_score else {
+        let Some((score, deciding)) = decided else {
             return Err(LibsyError::AlgorithmError {
                 message: "every classifier abstained".to_string(),
             });
         };
-        let deciding = deciding.expect("a score always has a deciding classifier");

As per coding guidelines: "In production Rust source, do not use panicking calls such as panic!(), unwrap(), or .expect(); propagate errors with ? or handle them explicitly."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/libsy/src/algorithms/fall_through.rs` around lines 389 - 408, Remove
the separate maybe_score and deciding options in the classifier loop and store
the selected Score together with its deciding Classifier in one Option,
preserving the existing abstention error when no classifier selects a score.
After the loop, destructure the paired value without calling expect, while
keeping served tied to the selected classifier response.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@crates/libsy/src/algorithms/fall_through.rs`:
- Around line 389-408: Remove the separate maybe_score and deciding options in
the classifier loop and store the selected Score together with its deciding
Classifier in one Option, preserving the existing abstention error when no
classifier selects a score. After the loop, destructure the paired value without
calling expect, while keeping served tied to the selected classifier response.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: b55e2359-875f-470f-9274-556d9451ef98

📥 Commits

Reviewing files that changed from the base of the PR and between c1c1b41 and 10ec05b.

📒 Files selected for processing (11)
  • crates/libsy/src/algorithms/fall_through.rs
  • crates/libsy/src/algorithms/util/affinity.rs
  • crates/libsy/src/core/algorithm.rs
  • crates/libsy/src/core/classifier.rs
  • crates/protocol/src/client.rs
  • crates/switchyard-server/src/lib.rs
  • crates/switchyard-server/src/routing_log.rs
  • crates/switchyard-server/src/stats/accumulator.rs
  • crates/switchyard-server/src/usage_metrics.rs
  • crates/switchyard-server/tests/server.rs
  • docs/internal/metrics_reference.md

@elyasmnvidian

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@elyasmnvidian
elyasmnvidian force-pushed the emehtabuddin/p0-routing-resilience branch 2 times, most recently from aa055a5 to 64e3593 Compare August 6, 2026 09:55
Comment thread crates/libsy/src/core/algorithm.rs
@elyasmnvidian elyasmnvidian changed the title fix(routing): isolate agent state and fail over unavailable targets fix(routing): separate context-overflow history by agent and fail over unavailable targets Aug 6, 2026
Comment thread crates/switchyard-server/tests/server.rs Outdated
Comment thread crates/libsy/src/algorithms/util/affinity.rs Outdated
@elyasmnvidian
elyasmnvidian force-pushed the emehtabuddin/p0-routing-resilience branch from 64e3593 to 8c30126 Compare August 6, 2026 18:08
@elyasmnvidian elyasmnvidian changed the title fix(routing): separate context-overflow history by agent and fail over unavailable targets fix(routing): scope context-overflow history by session and agent Aug 6, 2026
Comment thread crates/libsy/src/core/algorithm.rs
@ayushag-nv

Copy link
Copy Markdown
Contributor

Core overflow history should stay in SessionEvictions, but it does not need a second identity enum. The narrow design is a private tuple alias:

type OverflowIdentity = (String, Option<String>);

The key semantics are:

  • root request: (session_id, None)
  • identified child: (session_id, Some(agent_id))
  • child missing either ID: no key, so no overflow history is retained

FallThrough should compute this key once before routing and use the same value for both exclude_evicted and record. remove_session can remove every tuple whose session matches. This keeps the behavior in this PR: an overflow from one child does not exclude a target for the root or a sibling, while session_final still clears the whole session.

I would not reuse AffinityKey from core. It is private to the affinity component, and its construction includes affinity-only policy such as subagent-only mode and message-hash fallback. Making core overflow handling depend on it would couple two separate policies. If more components later need a canonical metadata identity, we can extract a neutral shared type in a separate change; this fix does not require one.

I tested the tuple-alias version locally. The focused server regression, cargo fmt --all --check, strict workspace Clippy, and cargo test --workspace all pass. The existing server regression remains sufficient.

Signed-off-by: Elyas Mehtabuddin <emehtabuddin@nvidia.com>
@elyasmnvidian
elyasmnvidian force-pushed the emehtabuddin/p0-routing-resilience branch from 8c30126 to 7dd20f8 Compare August 6, 2026 19:45
@elyasmnvidian

Copy link
Copy Markdown
Contributor Author

@ayushag-nv Thanks. The current update keeps SessionEvictions in core::algorithm and keeps affinity-only policy inside AffinityRouter, as you suggested.

Nachiket also flagged that the tuple/enum shape would duplicate the identity already used by affinity. I addressed both points with a neutral crate-private RoutingIdentity and RoutingIdentity::from_request in core::algorithm. That type only represents request metadata: root is session, identified child is session plus agent, and an incomplete child has no identity. subagents_only and message-hash fallback remain private to affinity.

This avoids two constructors drifting while preserving the same narrow behavior and ownership. The production regression fails on current main when the parent selects strong, passes on this head, and all local checks listed in the PR body pass.

Comment thread crates/libsy/src/algorithms/fall_through.rs
@ayushag-nv
ayushag-nv enabled auto-merge (squash) August 6, 2026 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants