Repository navigation
fix(proxy): let a ModelPool fail over when one member's model is missing - #1274
Conversation
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
|
Warning Review limit reached
Next review available in: 25 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change adds pool-specific provider error classification. Model-not-found failures now allow ModelPool failover, while general provider fallback and authentication handling retain their existing behavior. New tests cover classification, failover, and test-suite integration. ChangesModel-not-found pool failover
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant NeuroLink as NeuroLink.generate()
participant ModelPool
participant ProviderMember as Provider member
NeuroLink->>ModelPool: Submit generation request
ModelPool->>ProviderMember: Invoke current member
ProviderMember-->>ModelPool: Return model-not-found error
ModelPool->>ProviderMember: Invoke next member
ProviderMember-->>NeuroLink: Return successful response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
There was a problem hiding this comment.
Pull request overview
This PR adjusts ModelPool failover behavior so that “model not found” errors are treated as retryable within the pool (allowing the next pool member to be attempted), while preserving the existing “non-retryable” contract for provider fallback outside ModelPool.
Changes:
- Added
looksLikeModelNotFound()+isNonRetryableForPool()to distinguish pool-scoped retryability from the general provider-fallback contract. - Updated the two ModelPool error catch sites in
src/lib/neurolink.tsto useisNonRetryableForPool()instead of the generalisNonRetryableProviderError(). - Added a dedicated continuous test suite covering classification edge-cases and end-to-end
NeuroLink.generate()ModelPool failover, and wired it intopackage.json.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/continuous-test-suite-model-not-found-retryable.ts | New regression + classification suite asserting that model-not-found fails over in ModelPool while auth/access failures still short-circuit. |
| src/lib/utils/providerErrorClassification.ts | Adds pool-specific retryability classifier and model-not-found detection helper. |
| src/lib/neurolink.ts | Switches ModelPool short-circuit logic (generate + stream) to use the pool-scoped classifier. |
| package.json | Adds test:model-not-found-retryable script and includes it in test:unit. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const msg = error.message; | ||
| if ( | ||
| msg.includes("PERMISSION_DENIED") || | ||
| msg.includes("UNAUTHENTICATED") || | ||
| looksLikeModelAccessDenied(error) | ||
| ) { | ||
| return false; | ||
| } | ||
| const lower = msg.toLowerCase(); | ||
| const namesAModel = lower.includes("model"); | ||
| return ( |
| * Detects "the provider does not have this model": HTTP 404 model lookups, | ||
| * Google's NOT_FOUND, Anthropic's `not_found_error` naming the model, and the | ||
| * typed InvalidModelError providers raise for an unrecognised model id. | ||
| * | ||
| * Deliberately narrow. A 404 alone is not enough — only a 404 that names a | ||
| * model qualifies, so an unrelated missing endpoint stays non-retryable. | ||
| * Auth-flavoured messages are excluded outright: PERMISSION_DENIED means the | ||
| * key is not entitled to the model, which every member sharing that key would | ||
| * hit identically. |
Review Summary ✅ APPROVEDThis PR fixes a critical bug where ModelPool incorrectly treated "model not found" errors as non-retryable, preventing failover to alternative pool members that might have different models configured. Changes Reviewed1.
|
3c71ef0 to
a212133
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Summary
This PR adds a ModelPool-specific retryability rule that allows the pool to fail over when one member's model is missing, while preserving the general contract where provider fallback typically carries the same model ID across providers.
Key Changes:
- Added
looksLikeModelNotFound()function to detect model-not-found errors (Anthropic 404, Google NOT_FOUND, InvalidModelError) - Added
isNonRetryableForPool()which treats model-not-found as retryable (pool members differ by design) - Changed two ModelPool error handlers from
isNonRetryableProviderErrortoisNonRetryableForPool - Added comprehensive test suite (21 tests) covering detection logic and end-to-end failover
Impact: Fixes regression where a pool with one dead member would stop entirely instead of failing over to healthy members. The observed case was an Anthropic member naming a retired model (404), where the pool should have tried the OpenAI member.
Verification: Test suite test:model-not-found-retryable.ts passes 21/21, including end-to-end failover scenarios. Existing test:model-pool continues to pass. No breaking changes to provider fallback behavior.
Decision: APPROVED
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Summary
This PR adds a ModelPool-specific retryability rule that allows the pool to fail over when one member's model is missing, while preserving the general contract where provider fallback typically carries the same model ID across providers.
Key Changes:
- Added
looksLikeModelNotFound()function to detect model-not-found errors (Anthropic 404, Google NOT_FOUND, InvalidModelError) - Added
isNonRetryableForPool()which treats model-not-found as retryable (pool members differ by design) - Changed two ModelPool error handlers from
isNonRetryableProviderErrortoisNonRetryableForPool - Added comprehensive test suite (21 tests) covering detection logic and end-to-end failover
Impact: Fixes regression where a pool with one dead member would stop entirely instead of failing over to healthy members. The observed case was an Anthropic member naming a retired model (404), where the pool should have tried the OpenAI member.
Verification: Test suite test:model-not-found-retryable.ts passes 21/21, including end-to-end failover scenarios. Existing test:model-pool continues to pass. No breaking changes to provider fallback behavior.
Decision: APPROVED
A pool member is an explicit {provider, model} pair, and members exist
precisely because they differ. "This provider has no such model" is a
fact about ONE member, not about the request — member#2 runs a different
model and may well serve it. The pool nonetheless treated it as
non-retryable and stopped dead, which defeats the point of configuring a
pool: a member naming a retired Anthropic model made the API answer 404
not_found_error, and the pool gave up instead of failing over to the
member that was perfectly healthy.
Adds isNonRetryableForPool, used at the two ModelPool catch sites in
neurolink.ts. It differs from isNonRetryableProviderError in exactly one
respect: model-not-found does not stop the pool.
The general contract is deliberately left alone. Provider fallback
usually carries the same model id from one provider to the next, so
retrying a typo across every provider would turn one crisp error into a
slow fan-out of identical ones. Only the pool, where members are distinct
by construction, gets the looser rule.
looksLikeModelNotFound is deliberately narrow. A 404 alone does not
qualify — only one that names a model — so an unrelated missing endpoint
still stops the pool. Auth-flavoured messages are excluded outright:
PERMISSION_DENIED means the key is not entitled to the model, which every
member sharing that key would hit identically.
Verified end to end: the [LIVE] pool-fallback test in
continuous-test-suite-model-pool.ts has been failing against real
Anthropic credentials for exactly this reason (404 classified
non-retryable, so no fallback). With this change that suite is 56/56.
a212133 to
abc1896
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
|
Review summary for PR #1274: fix(proxy): let a ModelPool fail over when one member's model is missing Decision: APPROVED ✅ Summary:
Files Reviewed:
Key Improvements:
Impact Analysis:
Test Results:
Recommendation: |
|
Yama Review - APPROVED ✅ No issues found. This is a clean, well-tested fix for model-not-found retryability in ModelPool. SummaryThe PR correctly implements the intended behavior: when a specific Key Changes Verified:
Test Coverage:
Impact:
Recommendation:Safe to merge. Improves production reliability in multi-provider deployments without introducing risks. |
|
🔍 VERIFYING: Reviewing PR #1274 - Model-Not-Found Retryability Fix SummaryThis PR fixes a regression where ModelPool would stop on encountering a "model not found" error from one member, instead of failing over to another member with a different model. The change makes model-not-found errors retryable within pools while keeping them non-retryable for general provider fallback (since the same typo would fail across all providers). Impact Analysis
The change touches core error handling in NeuroLink's model pool logic. Let me review file-by-file starting with the utility functions. |
|
🎉 This PR is included in version 10.8.18 🎉 The release is available on: Your semantic-release bot 📦🚀 |
YAMA Review Summary for PR #1274Decision: CHANGES_REQUESTEDSummaryThis PR fixes a regression where ModelPool would stop on encountering a "model not found" error from one member, instead of failing over to another member with a different model. The change makes model-not-found errors retryable within pools while keeping them non-retryable for general provider fallback. Findings🔒 CRITICAL (1)File:
💡 MINOR (1)File:
Impact on Existing Code
Review ScopeReviewed all 4 changed files:
ConclusionThe core fix is sound and well-tested, but there's a CRITICAL security issue with case-sensitive auth error detection that must be fixed before merging. Once resolved, this would be APPROVED. |
|
🔒 CRITICAL: Case-sensitive auth error detection creates security vulnerability Location: Issue: The function uses case-sensitive string matching: msg.includes("PERMISSION_DENIED") || msg.includes("UNAUTHENTICATED")Impact: If an auth error arrives with lowercase text like "permission denied" or without a numeric status code, it bypasses the exclusion check. This means:
Evidence: Test at Fix: Use the already-computed if (lower.includes('permission denied') || lower.includes('unauthenticated')) {
return false;
} |
|
💡 MINOR: JSDoc documentation doesn't match implementation Location: Issue: The JSDoc says: But the implementation (lines 65-90) NEVER checks status codes - it only looks at:
Impact: Developers reading the code will assume HTTP status validation occurs when it doesn't. This creates confusion, especially since Fix: Rewrite JSDoc to accurately describe the actual detection logic: /**
* Detects errors indicating "the provider does not have this model": typed
* InvalidModelError instances, and Error messages containing model-related
* "not found" phrases (e.g., "model not found", "unknown model").
*
* Deliberately narrow: excludes bare 404s that don't mention models, and
* excludes auth-flavoured messages (PERMISSION_DENIED, UNAUTHENTICATED).
* Does not inspect HTTP status codes - relies on message content and error type.
*/ |
🛡️ Yama Review Verdict: BLOCKEDSeverity counts — 🔒 CRITICAL: 2 · This PR correctly implements model-not-found retryability within ModelPool to allow failover when a provider member lacks the requested model. However, there is a CRITICAL security issue: auth error detection uses case-sensitive string matching which allows lowercase auth errors to bypass the exclusion check. This could cause ModelPool to fail over to other members despite invalid credentials, confusing users who would get errors after trying multiple providers. The fix requires changing line 82-83 to use the already-computed Findings behind this verdict
|
A pool member is an explicit
{provider, model}pair, and members exist precisely because they differ. "This provider has no such model" is a fact about one member, not about the request — member#2 runs a different model and may well serve it.The pool nonetheless treated it as non-retryable and stopped dead, which defeats the point of configuring a pool. Observed case: a member naming a retired Anthropic model, so the API answered
404 not_found_error, and the pool gave up instead of failing over to the member that was perfectly healthy.The change
Adds
isNonRetryableForPool, used at the two ModelPool catch sites inneurolink.ts. It differs fromisNonRetryableProviderErrorin exactly one respect: model-not-found does not stop the pool.The general contract is deliberately left alone. Provider fallback usually carries the same model id from one provider to the next, so retrying a typo across every provider would turn one crisp error into a slow fan-out of identical ones. Only the pool — where members are distinct by construction — gets the looser rule.
looksLikeModelNotFoundis deliberately narrow:PERMISSION_DENIEDmeans the key is not entitled to the model, which every member sharing that key would hit identically.NON_RETRYABLE_HTTP_STATUS_CODESis untouched, so generic HTTP retry behaviour is unchanged.Verification
End to end against real credentials. The
[LIVE] pool falls back from bogus model to real providertest incontinuous-test-suite-model-pool.tshas been failing onreleasefor exactly this reason. With this change:New suite
test:model-not-found-retryable— 21/21, covering:NOT_FOUND,unknown model, typedInvalidModelError) and what does not (a bare 404 naming no model,PERMISSION_DENIED,UNAUTHENTICATED, LiteLLM team access-denied, rate limits, non-Errorvalues);ModelAccessDeniedError/PERMISSION_DENIEDstill stop the pool;NeuroLink.generate(): member#1 404s and member#2 serves the request, plus the converse — an auth failure still short-circuits and member#2 is never attempted.Negative control: reverting
isNonRetryableForPoolto the general contract fails exactly 2 of the 21 — the two that assert the new behaviour — and nothing else.One note on the fixtures: the real SDK error carries
status: 404, and it is the numeric status the classifier keys on, not the message. My first draft used a bareErrorwith only the text, which is already retryable today — so it would have passed with or without the fix. The fixture now carries the status, which is what makes the control meaningful.Summary by CodeRabbit
Bug Fixes
Tests