Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
115 changes: 114 additions & 1 deletion sdk/cliproxy/auth/conductor.go
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,84 @@ func (m *Manager) RefreshSchedulerEntry(authID string) {
m.scheduler.upsertAuth(snapshot)
}

// ReconcileRegistryModelStates aligns per-model runtime state with the current
// registry snapshot for one auth.
//
// Supported models are reset to a clean state because re-registration already
// cleared the registry-side cooldown/suspension snapshot. ModelStates for
// models that are no longer present in the registry are pruned entirely so
// renamed/removed models cannot keep auth-level status stale.
func (m *Manager) ReconcileRegistryModelStates(ctx context.Context, authID string) {
if m == nil || authID == "" {
return
}

supportedModels := registry.GetGlobalRegistry().GetModelsForClient(authID)
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{}{}
}

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 {
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 {
// Drop state for models that disappeared from the current registry
// snapshot. Keeping them around leaks stale errors into auth-level
// status, management output, and websocket fallback checks.
delete(auth.ModelStates, modelKey)
changed = true
continue
}
if state == nil {
continue
}
if modelStateIsClean(state) {
continue
}
resetModelState(state, now)
changed = true
}
if len(auth.ModelStates) == 0 {
auth.ModelStates = nil
}
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
if errPersist := m.persist(ctx, auth); errPersist != nil {
logEntryWithRequestID(ctx).WithField("auth_id", auth.ID).Warnf("failed to persist auth changes during model state reconciliation: %v", errPersist)
}
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,19 +1813,41 @@ 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 {
if auth == nil {
return
}
if len(auth.ModelStates) == 0 {
clearAggregatedAvailability(auth)
return
}
allUnavailable := true
earliestRetry := time.Time{}
quotaExceeded := false
quotaRecover := time.Time{}
maxBackoffLevel := 0
hasState := false
for _, state := range auth.ModelStates {
if state == nil {
continue
}
hasState = true
stateUnavailable := false
if state.Status == StatusDisabled {
stateUnavailable = true
Expand Down Expand Up @@ -1777,6 +1877,10 @@ func updateAggregatedAvailability(auth *Auth, now time.Time) {
}
}
}
if !hasState {
clearAggregatedAvailability(auth)
return
}
auth.Unavailable = allUnavailable
if allUnavailable {
auth.NextRetryAfter = earliestRetry
Expand All @@ -1796,6 +1900,15 @@ func updateAggregatedAvailability(auth *Auth, now time.Time) {
}
}

func clearAggregatedAvailability(auth *Auth) {
if auth == nil {
return
}
auth.Unavailable = false
auth.NextRetryAfter = time.Time{}
auth.Quota = QuotaState{}
}

func hasModelError(auth *Auth, now time.Time) bool {
if auth == nil || len(auth.ModelStates) == 0 {
return false
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