fix(routing): scope context-overflow history by session and agent - #298
fix(routing): scope context-overflow history by session and agent#298elyasmnvidian wants to merge 1 commit into
Conversation
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/libsy/src/algorithms/fall_through.rs (1)
344-347: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlign tier resolution on the fallback path with
route.
routereads the tier from the deciding classifier only (deciding.and_then(|c| c.routing_tier(...))).fallback_decisionreads 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.tieris a metrics and routing-log label, so the same model then splits across two label values.Thread the deciding classifier into
call_llm_with_fallbackand 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
📒 Files selected for processing (11)
crates/libsy/src/algorithms/fall_through.rscrates/libsy/src/algorithms/util/affinity.rscrates/libsy/src/core/algorithm.rscrates/libsy/src/core/classifier.rscrates/protocol/src/client.rscrates/switchyard-server/src/lib.rscrates/switchyard-server/src/routing_log.rscrates/switchyard-server/src/stats/accumulator.rscrates/switchyard-server/src/usage_metrics.rscrates/switchyard-server/tests/server.rsdocs/internal/metrics_reference.md
WalkthroughFall-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. ChangesFallback routing
Estimated code review effort: 4 (Complex) | ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
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 winRemove the
.expect()on line 408.The coding guidelines forbid
.expect()in production Rust source. The invariant is correct today becausedecidingandmaybe_scoreare assigned in the same loop iteration, but the pairing is implicit. Carry the score and the deciding classifier in oneOptionso 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
📒 Files selected for processing (11)
crates/libsy/src/algorithms/fall_through.rscrates/libsy/src/algorithms/util/affinity.rscrates/libsy/src/core/algorithm.rscrates/libsy/src/core/classifier.rscrates/protocol/src/client.rscrates/switchyard-server/src/lib.rscrates/switchyard-server/src/routing_log.rscrates/switchyard-server/src/stats/accumulator.rscrates/switchyard-server/src/usage_metrics.rscrates/switchyard-server/tests/server.rsdocs/internal/metrics_reference.md
|
@coderabbitai review |
✅ Action performedReview finished.
|
aa055a5 to
64e3593
Compare
64e3593 to
8c30126
Compare
|
Core overflow history should stay in type OverflowIdentity = (String, Option<String>);The key semantics are:
I would not reuse I tested the tuple-alias version locally. The focused server regression, |
Signed-off-by: Elyas Mehtabuddin <emehtabuddin@nvidia.com>
8c30126 to
7dd20f8
Compare
|
@ayushag-nv Thanks. The current update keeps 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 This avoids two constructors drifting while preserving the same narrow behavior and ownership. The production regression fails on current |
With a local provider where
model/weakrejectsoverflowfor context length andmodel/strongsucceeds, send these requests under one session:The bug
One oversized child request marked
weakoverflowed for the whole session, so the parentand
child-bthen skippedweakeven though their shorter requests fit:The overflow history keyed every request by session ID alone. A child's overflow was
therefore recorded against the session and excluded
weakfor the parent and every sibling.The fix
SessionEvictionsnow uses the same crate-private routing identity asAffinityRouter:Both paths build that identity with
RoutingIdentity::from_request. A child missing either IDkeeps 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, andstage_routerroute their final model call throughFallThrough, so all three receive the fix.passthroughandnoopare unchanged.After
Proof
The same production-path regression test was applied to both revisions without copying the
production fix:
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
The full
cargo test --workspacecommand reaches the known local macOS PyO3 link failure forswitchyard-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.