fix: make global model listing best effort - #4165
Open
lugnicca wants to merge 3 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughRefactors ListAllModels to use per-provider timeout contexts and watchdog timers, preserves provider KeyStatuses, adds OpenAI configured-model fallback on list-models failures, and updates backfill gating for empty restricted allowlists. Tests exercise timeout and fallback scenarios. ChangesTimeout Resilience and Provider Fallback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@core/bifrost.go`:
- Around line 636-643: The current branch treats a provider that returns an
empty models slice and no keyStatuses/error as a synthetic failure by setting
result.err = listModelsNilResponseError, which incorrectly marks successful
empty inventories as errors; remove or change this logic so that when
result.models is empty and result.keyStatuses is empty but result.err == nil you
do not set result.err nor populate keyStatuses from listModelsNilResponseError
(so functions like ListAllModels won't treat empty-but-successful responses as
failures), i.e. only set listModelsNilResponseError when there is a real failure
reported by the provider or an explicit nil-response condition proven to be an
error. Ensure references to result.models, result.keyStatuses and
listModelsNilResponseError are adjusted accordingly.
- Around line 581-593: The timeout branch can race with the worker finishing and
enqueue a synthetic timeout that overwrites real provider results; inside the
timer.C case (where listModelsProviderTimeoutError, providerResult, providerKey
and results are used) guard the timeout send by doing a non-blocking select on
done first (if <-done then skip sending the timeout), and ensure you Stop and
drain the timer to avoid races; this preserves real worker-produced
providerResult when done and only sends the synthetic timeout if the worker
truly hasn't finished.
In `@core/providers/openai/list_models_fallback_test.go`:
- Around line 14-117: Add a new unit test that simulates the upstream returning
503 and verifies that an empty but restricted allowlist falls through to
alias-based backfilling: create a httptest server that returns
ServiceUnavailable, call ListModelsByKey with Models set to an empty
schemas.WhiteList{} (not wildcard), include a Key.Aliases map (e.g.
"desktop-picker-name":"actual-local-model"), use filtered=false and a local
provider (e.g. schemas.ModelProvider("local-openai")), and assert the response
contains one model with ID "local-openai/desktop-picker-name" and Alias
"actual-local-model"; name it e.g.
TestListModelsByKey_EmptyRestrictedAllowlistFallsBackToAliases and model it
after the existing fallback tests using ListModelsByKey to locate behavior in
models.go.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed9ca7d3-f741-4ba0-ac7e-4329aebbe751
📒 Files selected for processing (5)
core/bifrost.gocore/bifrost_test.gocore/providers/openai/list_models_fallback_test.gocore/providers/openai/openai.gocore/providers/utils/models.go
Comment on lines
+581
to
+593
| select { | ||
| case <-timer.C: | ||
| timeoutErr := listModelsProviderTimeoutError(providerKey, timeout) | ||
| results <- providerResult{ | ||
| provider: providerKey, | ||
| keyStatuses: []schemas.KeyStatus{{ | ||
| Provider: providerKey, | ||
| Status: schemas.KeyStatusListModelsFailed, | ||
| Error: timeoutErr, | ||
| }}, | ||
| err: timeoutErr, | ||
| } | ||
| case <-done: |
Contributor
There was a problem hiding this comment.
Timer race can overwrite concrete provider failure metadata at timeout boundary.
At Line 581, if timer.C and done become ready together, the timer branch can win and enqueue a synthetic timeout even though the worker finished. Because results are deduped by first-seen provider, concrete key/provider failure details can be dropped nondeterministically.
Suggested fix
select {
case <-timer.C:
+ select {
+ case <-done:
+ return
+ default:
+ }
timeoutErr := listModelsProviderTimeoutError(providerKey, timeout)
results <- providerResult{
provider: providerKey,🤖 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 `@core/bifrost.go` around lines 581 - 593, The timeout branch can race with the
worker finishing and enqueue a synthetic timeout that overwrites real provider
results; inside the timer.C case (where listModelsProviderTimeoutError,
providerResult, providerKey and results are used) guard the timeout send by
doing a non-blocking select on done first (if <-done then skip sending the
timeout), and ensure you Stop and drain the timer to avoid races; this preserves
real worker-produced providerResult when done and only sends the synthetic
timeout if the worker truly hasn't finished.
Comment on lines
+636
to
+643
| if len(result.models) == 0 && len(result.keyStatuses) == 0 && result.err == nil { | ||
| result.err = listModelsNilResponseError(result.provider) | ||
| result.keyStatuses = []schemas.KeyStatus{{ | ||
| Provider: result.provider, | ||
| Status: schemas.KeyStatusListModelsFailed, | ||
| Error: result.err, | ||
| }} | ||
| } |
Contributor
There was a problem hiding this comment.
Do not convert valid empty provider inventories into synthetic failures.
At Line 636, this branch turns (models == 0 && keyStatuses == 0 && err == nil) into listModelsNilResponseError. That path is reachable when a provider successfully returns an empty model list, which then incorrectly sets firstError and can make ListAllModels fail despite no real provider error.
Suggested fix
- if len(result.models) == 0 && len(result.keyStatuses) == 0 && result.err == nil {
- result.err = listModelsNilResponseError(result.provider)
- result.keyStatuses = []schemas.KeyStatus{{
- Provider: result.provider,
- Status: schemas.KeyStatusListModelsFailed,
- Error: result.err,
- }}
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if len(result.models) == 0 && len(result.keyStatuses) == 0 && result.err == nil { | |
| result.err = listModelsNilResponseError(result.provider) | |
| result.keyStatuses = []schemas.KeyStatus{{ | |
| Provider: result.provider, | |
| Status: schemas.KeyStatusListModelsFailed, | |
| Error: result.err, | |
| }} | |
| } |
🤖 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 `@core/bifrost.go` around lines 636 - 643, The current branch treats a provider
that returns an empty models slice and no keyStatuses/error as a synthetic
failure by setting result.err = listModelsNilResponseError, which incorrectly
marks successful empty inventories as errors; remove or change this logic so
that when result.models is empty and result.keyStatuses is empty but result.err
== nil you do not set result.err nor populate keyStatuses from
listModelsNilResponseError (so functions like ListAllModels won't treat
empty-but-successful responses as failures), i.e. only set
listModelsNilResponseError when there is a real failure reported by the provider
or an explicit nil-response condition proven to be an error. Ensure references
to result.models, result.keyStatuses and listModelsNilResponseError are adjusted
accordingly.
Comment on lines
+14
to
+117
| func TestListModelsByKey_FallsBackToConfiguredModelsOnUpstreamError(t *testing.T) { | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| http.Error(w, "upstream unavailable", http.StatusServiceUnavailable) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| ctx := schemas.NewBifrostContext(context.Background(), schemas.NoDeadline) | ||
| resp, err := ListModelsByKey( | ||
| ctx, | ||
| &fasthttp.Client{ReadTimeout: time.Second, WriteTimeout: time.Second}, | ||
| server.URL, | ||
| schemas.Key{ | ||
| ID: "local-key", | ||
| Models: schemas.WhiteList{"hermes-qwen"}, | ||
| Aliases: schemas.KeyAliases{ | ||
| "hermes-qwen": "Qwen/Qwen3-32B", | ||
| }, | ||
| }, | ||
| false, | ||
| nil, | ||
| schemas.ModelProvider("bao-qwen"), | ||
| false, | ||
| false, | ||
| ) | ||
|
|
||
| if err != nil { | ||
| t.Fatalf("ListModelsByKey returned error: %v", err) | ||
| } | ||
| if resp == nil || len(resp.Data) != 1 { | ||
| t.Fatalf("expected one fallback model, got %#v", resp) | ||
| } | ||
| if resp.Data[0].ID != "bao-qwen/hermes-qwen" { | ||
| t.Fatalf("expected configured model id, got %q", resp.Data[0].ID) | ||
| } | ||
| if resp.Data[0].Alias == nil || *resp.Data[0].Alias != "Qwen/Qwen3-32B" { | ||
| t.Fatalf("expected alias to preserve upstream model id, got %#v", resp.Data[0].Alias) | ||
| } | ||
| } | ||
|
|
||
| func TestListModelsByKey_FallsBackToConfiguredAliasesWithWildcardModels(t *testing.T) { | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| http.Error(w, "upstream unavailable", http.StatusServiceUnavailable) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| ctx := schemas.NewBifrostContext(context.Background(), schemas.NoDeadline) | ||
| resp, err := ListModelsByKey( | ||
| ctx, | ||
| &fasthttp.Client{ReadTimeout: time.Second, WriteTimeout: time.Second}, | ||
| server.URL, | ||
| schemas.Key{ | ||
| ID: "local-key", | ||
| Models: schemas.WhiteList{"*"}, | ||
| Aliases: schemas.KeyAliases{ | ||
| "desktop-picker-name": "actual-local-model", | ||
| }, | ||
| }, | ||
| false, | ||
| nil, | ||
| schemas.ModelProvider("local-openai"), | ||
| false, | ||
| false, | ||
| ) | ||
|
|
||
| if err != nil { | ||
| t.Fatalf("ListModelsByKey returned error: %v", err) | ||
| } | ||
| if resp == nil || len(resp.Data) != 1 { | ||
| t.Fatalf("expected one alias fallback model, got %#v", resp) | ||
| } | ||
| if resp.Data[0].ID != "local-openai/desktop-picker-name" { | ||
| t.Fatalf("expected alias model id, got %q", resp.Data[0].ID) | ||
| } | ||
| if resp.Data[0].Alias == nil || *resp.Data[0].Alias != "actual-local-model" { | ||
| t.Fatalf("expected alias target, got %#v", resp.Data[0].Alias) | ||
| } | ||
| } | ||
|
|
||
| func TestListModelsByKey_UnfilteredDoesNotUseConfiguredFallback(t *testing.T) { | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| http.Error(w, "upstream unavailable", http.StatusServiceUnavailable) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| ctx := schemas.NewBifrostContext(context.Background(), schemas.NoDeadline) | ||
| resp, err := ListModelsByKey( | ||
| ctx, | ||
| &fasthttp.Client{ReadTimeout: time.Second, WriteTimeout: time.Second}, | ||
| server.URL, | ||
| schemas.Key{ | ||
| ID: "local-key", | ||
| Models: schemas.WhiteList{"hermes-qwen"}, | ||
| }, | ||
| true, | ||
| nil, | ||
| schemas.ModelProvider("bao-qwen"), | ||
| false, | ||
| false, | ||
| ) | ||
|
|
||
| if err == nil { | ||
| t.Fatalf("expected upstream error for unfiltered request, got response %#v", resp) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider adding test coverage for empty restricted allowlist fallback.
The backfill logic change in models.go (line 350) specifically enables alias-based fallback when AllowedModels is restricted but empty by skipping Case A and falling through to Case B. However, none of the three tests exercises this scenario:
- Test 1 uses
Models: ["hermes-qwen"](non-empty) - Test 2 uses
Models: ["*"](wildcard) - Test 3 uses
Models: ["hermes-qwen"](non-empty)
The PR stack description states: "Empty restricted allowlists now fall through to 'Case B' alias-based backfilling, enabling fallback via configured aliases when the allowlist is restrictive but empty."
Suggested test case
+func TestListModelsByKey_EmptyAllowlistFallsBackToAliases(t *testing.T) {
+ server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ http.Error(w, "upstream unavailable", http.StatusServiceUnavailable)
+ }))
+ defer server.Close()
+
+ ctx := schemas.NewBifrostContext(context.Background(), schemas.NoDeadline)
+ resp, err := ListModelsByKey(
+ ctx,
+ &fasthttp.Client{ReadTimeout: time.Second, WriteTimeout: time.Second},
+ server.URL,
+ schemas.Key{
+ ID: "local-key",
+ Models: schemas.WhiteList{}, // empty restricted allowlist
+ Aliases: schemas.KeyAliases{
+ "my-alias": "actual-model",
+ },
+ },
+ false,
+ nil,
+ schemas.ModelProvider("test-provider"),
+ false,
+ false,
+ )
+
+ if err != nil {
+ t.Fatalf("ListModelsByKey returned error: %v", err)
+ }
+ if resp == nil || len(resp.Data) != 1 {
+ t.Fatalf("expected one alias fallback model, got %#v", resp)
+ }
+ if resp.Data[0].ID != "test-provider/my-alias" {
+ t.Fatalf("expected alias model id, got %q", resp.Data[0].ID)
+ }
+ if resp.Data[0].Alias == nil || *resp.Data[0].Alias != "actual-model" {
+ t.Fatalf("expected alias target, got %#v", resp.Data[0].Alias)
+ }
+}🤖 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 `@core/providers/openai/list_models_fallback_test.go` around lines 14 - 117,
Add a new unit test that simulates the upstream returning 503 and verifies that
an empty but restricted allowlist falls through to alias-based backfilling:
create a httptest server that returns ServiceUnavailable, call ListModelsByKey
with Models set to an empty schemas.WhiteList{} (not wildcard), include a
Key.Aliases map (e.g. "desktop-picker-name":"actual-local-model"), use
filtered=false and a local provider (e.g.
schemas.ModelProvider("local-openai")), and assert the response contains one
model with ID "local-openai/desktop-picker-name" and Alias "actual-local-model";
name it e.g. TestListModelsByKey_EmptyRestrictedAllowlistFallsBackToAliases and
model it after the existing fallback tests using ListModelsByKey to locate
behavior in models.go.
Contributor
Confidence Score: 5/5Safe to merge; the best-effort timeout mechanism is correctly bounded per provider, goroutines cannot block indefinitely, and the channel buffer is sized to absorb both worker and watchdog sends per provider. No new blocking defects were found beyond the two items already flagged in prior review threads. The BackfillModels condition change for empty allowlists is a deliberate fix that aligns implementation with the pre-existing ShouldEarlyExit comment. Integration tests with real httptest servers cover the core timing and fallback scenarios. core/bifrost.go — the watchdog grace-period and post-cancellation error-synthesis concerns flagged in previous review threads are the only open items worth a follow-up read. Important Files Changed
Reviews (3): Last reviewed commit: "fix: surface list models fallback status" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Make global
ListAllModels()best-effort and bounded by each provider's existingnetwork_config.default_request_timeout_in_secondsinstead of waiting for every provider goroutine to finish before returning any models.This prevents one slow or broken OpenAI-compatible custom provider from making
GET /v1/modelsunusable for clients that rely on the global model list, such as OpenAI-compatible model pickers.What changed
/v1/modelsfails but the key has explicit configuredmodels[]/ aliases, return those configured models for filtered requests.Unfilteredsemantics strict: unfiltered requests still require upstream inventory and do not synthesize configured allowlist fallback.Reproduction
Before this change,
ListAllModels()launched provider list calls concurrently but then waited onwg.Wait(). A slow/down custom OpenAI-compatible provider could block the entire global/v1/modelsresponse, even when other providers had already returned models.The new regression test configures two custom OpenAI-compatible providers backed by local test servers: one responds immediately, while the other blocks beyond its configured one-second timeout.
ListAllModels()now returns the fast provider model without waiting for the slow provider to complete.Tests
All commands pass locally.
Notes
No production config or secrets were used. The live-style validation is covered with local
httptestcustom OpenAI-compatible providers, including a simulated slow/v1/modelsendpoint.Summary by CodeRabbit