Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 91 additions & 0 deletions sdk/cliproxy/auth/conductor.go
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,81 @@ func (m *Manager) RefreshSchedulerEntry(authID string) {
m.scheduler.upsertAuth(snapshot)
}

// ReconcileRegistryModelStates clears stale per-model runtime failures for
// models that are currently registered for the auth in the global model registry.
//
// This keeps the scheduler and the global registry aligned after model
// re-registration. Without this reconciliation, a model can reappear in
// /v1/models after registry refresh while the scheduler still blocks it because
// auth.ModelStates retained an older failure such as not_found or quota.
func (m *Manager) ReconcileRegistryModelStates(ctx context.Context, authID string) {
if m == nil || authID == "" {
return
}

supportedModels := registry.GetGlobalRegistry().GetModelsForClient(authID)
if len(supportedModels) == 0 {
return
}

supported := make(map[string]struct{}, len(supportedModels))
for _, model := range supportedModels {
if model == nil {
continue
}
modelKey := canonicalModelKey(model.ID)
if modelKey == "" {
continue
}
supported[modelKey] = struct{}{}
}
if len(supported) == 0 {
return
}

var snapshot *Auth
now := time.Now()

m.mu.Lock()
auth, ok := m.auths[authID]
if ok && auth != nil && len(auth.ModelStates) > 0 {
changed := false
for modelKey, state := range auth.ModelStates {
if state == nil {
continue
}
baseModel := canonicalModelKey(modelKey)
if baseModel == "" {
baseModel = strings.TrimSpace(modelKey)
}
Comment on lines +269 to +272
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The logic to determine baseModel has a potentially confusing fallback. If canonicalModelKey(modelKey) returns an empty string, it falls back to strings.TrimSpace(modelKey). The supported map is keyed by canonical model keys. If canonicalModelKey fails (returns empty), it's likely that the model key is invalid or not in a form that can be canonicalized. Falling back to the raw (but trimmed) modelKey will likely result in a lookup miss in the supported map anyway if the key contains suffixes or other non-canonical parts. This makes the code harder to reason about. It would be clearer to skip reconciliation for model keys that cannot be canonicalized.

Suggested change
baseModel := canonicalModelKey(modelKey)
if baseModel == "" {
baseModel = strings.TrimSpace(modelKey)
}
baseModel := canonicalModelKey(modelKey)
if baseModel == "" {
continue
}

if _, supportedModel := supported[baseModel]; !supportedModel {
continue
}
if modelStateIsClean(state) {
continue
}
resetModelState(state, now)
changed = true
}
if changed {
updateAggregatedAvailability(auth, now)
if !hasModelError(auth, now) {
auth.LastError = nil
auth.StatusMessage = ""
auth.Status = StatusActive
}
Comment on lines +295 to +299

Choose a reason for hiding this comment

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

P2 Badge Keep disabled auths from being marked active

When ReconcileRegistryModelStates removes or resets stale ModelStates, it unconditionally sets auth.Status = StatusActive if no model errors remain. This path is now invoked from applyCoreAuthAddOrUpdate/refreshModelRegistrationForAuth after model registration, including cases where an auth is intentionally disabled (auth.Disabled == true), so a disabled auth can end up persisted/reporting as active despite still being disabled. That status regression is user-visible in management output and can break workflows that rely on status semantics; this reset should be skipped when the auth is disabled (or already StatusDisabled).

Useful? React with 👍 / 👎.

auth.UpdatedAt = now
_ = m.persist(ctx, auth)
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The error returned by m.persist(ctx, auth) is being ignored. If persistence fails, the in-memory state of the auth manager will be out of sync with the backing store. This could lead to inconsistent behavior or loss of state changes after a service restart. The error should be handled, for example by logging it.

            if err := m.persist(ctx, auth); err != nil {
                logEntryWithRequestID(ctx).WithField("auth_id", auth.ID).Warnf("failed to persist auth changes during model state reconciliation: %v", err)
            }

snapshot = auth.Clone()
}
}
m.mu.Unlock()

if m.scheduler != nil && snapshot != nil {
m.scheduler.upsertAuth(snapshot)
}
}

func (m *Manager) SetSelector(selector Selector) {
if m == nil {
return
Expand Down Expand Up @@ -1735,6 +1810,22 @@ func resetModelState(state *ModelState, now time.Time) {
state.UpdatedAt = now
}

func modelStateIsClean(state *ModelState) bool {
if state == nil {
return true
}
if state.Status != StatusActive {
return false
}
if state.Unavailable || state.StatusMessage != "" || !state.NextRetryAfter.IsZero() || state.LastError != nil {
return false
}
if state.Quota.Exceeded || state.Quota.Reason != "" || !state.Quota.NextRecoverAt.IsZero() || state.Quota.BackoffLevel != 0 {
return false
}
return true
}

func updateAggregatedAvailability(auth *Auth, now time.Time) {
if auth == nil || len(auth.ModelStates) == 0 {
return
Expand Down
3 changes: 3 additions & 0 deletions sdk/cliproxy/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,7 @@ func (s *Service) applyCoreAuthAddOrUpdate(ctx context.Context, auth *coreauth.A
// This operation may block on network calls, but the auth configuration
// is already effective at this point.
s.registerModelsForAuth(auth)
s.coreManager.ReconcileRegistryModelStates(ctx, auth.ID)

// Refresh the scheduler entry so that the auth's supportedModelSet is rebuilt
// from the now-populated global model registry. Without this, newly added auths
Expand Down Expand Up @@ -1019,6 +1020,7 @@ func (s *Service) refreshModelRegistrationForAuth(current *coreauth.Auth) bool {
s.ensureExecutorsForAuth(current)
}
s.registerModelsForAuth(current)
s.coreManager.ReconcileRegistryModelStates(context.Background(), current.ID)

latest, ok := s.latestAuthForModelRegistration(current.ID)
if !ok || latest.Disabled {
Expand All @@ -1032,6 +1034,7 @@ func (s *Service) refreshModelRegistrationForAuth(current *coreauth.Auth) bool {
// no auth fields changed, but keeps the refresh path simple and correct.
s.ensureExecutorsForAuth(latest)
s.registerModelsForAuth(latest)
s.coreManager.ReconcileRegistryModelStates(context.Background(), latest.ID)
s.coreManager.RefreshSchedulerEntry(current.ID)
return true
}
Expand Down
Loading