Skip to content

fix(auth): preserve refreshed quota observations #5824

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
camy-x wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

fix(auth): preserve refreshed quota observations #5824

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 1 addition & 3 deletions internal/runtime/executor/devin_executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,9 +200,7 @@ func (e *DevinExecutor) Refresh(ctx context.Context, auth *cliproxyauth.Auth) (*
}

// Quota observation signals for management UI and conductor
if updated.Quota.Signals == nil {
updated.Quota.Signals = make(map[string]string)
}
updated.Quota.Signals = make(map[string]string)
if status.Plan != "" {
updated.Quota.Signals["plan"] = status.Plan
}
Expand Down
14 changes: 14 additions & 0 deletions internal/runtime/executor/devin_executor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,6 +638,15 @@ func TestDevinExecutor_Refresh(t *testing.T) {
"api_key": "devin-session-token$test",
"base_url": server.URL,
},
Quota: cliproxyauth.QuotaState{
ObservedAt: time.Unix(10, 0),
Signals: map[string]string{
"plan": "Free",
"plan_start": "2025-01-01T00:00:00Z",
"plan_end": "2025-02-01T00:00:00Z",
"obsolete_signal": "stale",
},
},
}

updated, err := exec.Refresh(context.Background(), auth)
Expand Down Expand Up @@ -666,6 +675,11 @@ func TestDevinExecutor_Refresh(t *testing.T) {
if updated.Quota.Signals["weekly_quota_remaining_percent"] != "45%" {
t.Errorf("expected quota signal 45%%, got %q", updated.Quota.Signals["weekly_quota_remaining_percent"])
}
for _, staleKey := range []string{"plan_start", "plan_end", "obsolete_signal"} {
if _, exists := updated.Quota.Signals[staleKey]; exists {
t.Errorf("expected stale quota signal %q to be removed, got %#v", staleKey, updated.Quota.Signals)
}
}
if updated.Quota.ObservedAt.IsZero() {
t.Error("expected non-zero Quota.ObservedAt")
}
Expand Down
6 changes: 5 additions & 1 deletion sdk/cliproxy/auth/conductor_lifecycle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,11 @@ func (m *Manager) updateInternal(ctx context.Context, base, auth *Auth, mode upd
if existing.Quota.Exceeded && existing.Quota.Reason == "credential_quota" && existing.Quota.NextRecoverAt.After(time.Now()) {
auth.Unavailable = existing.Unavailable
auth.NextRetryAfter = existing.NextRetryAfter
auth.Quota = existing.Quota
if mode == updateModeRefresh {
applyCooldownFields(&auth.Quota, existing.Quota)
} else {
auth.Quota = existing.Quota
}
if auth.Status == StatusActive {
auth.Status = existing.Status
}
Expand Down
49 changes: 49 additions & 0 deletions sdk/cliproxy/auth/conductor_update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,3 +251,52 @@ func TestManager_Update_ActiveInheritsModelStates(t *testing.T) {
t.Fatalf("expected BackoffLevel to be %d, got %d", backoffLevel, state.Quota.BackoffLevel)
}
}

func TestManager_UpdateRefreshedAuthPreservesQuotaObservationDuringCredentialCooldown(t *testing.T) {
manager := NewManager(nil, nil, nil)
ctx := context.Background()
baseObservedAt := time.Unix(10, 0)
refreshedObservedAt := time.Unix(20, 0)
recoverAt := time.Now().Add(time.Hour)

base, errRegister := manager.Register(ctx, &Auth{
ID: "auth-devin-quota-refresh",
Provider: "devin",
Status: StatusActive,
Quota: QuotaState{
ObservedAt: baseObservedAt,
Signals: map[string]string{"plan": "free"},
},
})
if errRegister != nil {
t.Fatalf("Register() error = %v", errRegister)
}

concurrent := base.Clone()
concurrent.Status = StatusError
concurrent.Unavailable = true
concurrent.Quota.Exceeded = true
concurrent.Quota.Reason = "credential_quota"
concurrent.Quota.NextRecoverAt = recoverAt
concurrent.Quota.BackoffLevel = 2
if _, errUpdate := manager.Update(ctx, concurrent); errUpdate != nil {
t.Fatalf("Update() concurrent cooldown error = %v", errUpdate)
}

refreshed := base.Clone()
refreshed.Quota.ObservedAt = refreshedObservedAt
refreshed.Quota.Signals = map[string]string{
"plan": "pro",
"daily_quota_remaining_percent": "75%",
}
merged, errRefresh := manager.UpdateRefreshedAuth(ctx, base, refreshed)
if errRefresh != nil {
t.Fatalf("UpdateRefreshedAuth() error = %v", errRefresh)
}
if !merged.Quota.ObservedAt.Equal(refreshedObservedAt) || merged.Quota.Signals["plan"] != "pro" || merged.Quota.Signals["daily_quota_remaining_percent"] != "75%" {
t.Fatalf("refreshed quota observation was not preserved: %#v", merged.Quota)
}
if !merged.Quota.Exceeded || merged.Quota.Reason != "credential_quota" || !merged.Quota.NextRecoverAt.Equal(recoverAt) || merged.Quota.BackoffLevel != 2 {
t.Fatalf("concurrent cooldown was not preserved: %#v", merged.Quota)
}
}
1 change: 1 addition & 0 deletions sdk/cliproxy/auth/metadata_merge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ func MergeRefreshedAuth(base, current, updated *Auth) *Auth {
if base != nil && current.RegistrationEpoch != base.RegistrationEpoch {
return merged
}
merged.Quota = mergeQuotaObservation(current.Quota, updated.Quota)

@chatgpt-codex-connector chatgpt-codex-connector Bot Sep 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve observations through the manager cooldown merge

When the current credential has an active credential_quota cooldown, the normal UpdateRefreshedAuth path subsequently executes auth.Quota = existing.Quota in Manager.updateInternal, overwriting the observation merged here. Consequently, a successful Devin refresh during that cooldown still discards its new plan and remaining-quota signals before persistence or the management UI can see them; the added test only calls this helper directly and misses the overwrite. Preserve only the existing cooldown fields at that later merge, or reapply the refreshed observation afterward.

Useful? React with 👍 / 👎.

@camy-x camy-x Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Addressed in 5ae3e63f. The regression now exercises Register → a concurrent credential-quota cooldown → UpdateRefreshedAuth, so it covers the later Manager.updateInternal merge. Refresh mode now applies only the existing cooldown fields to the newly merged quota observation; replace and prepare behavior remains unchanged. The affected package tests, go vet, and server build pass.

@chatgpt-codex-connector chatgpt-codex-connector Bot Sep 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Rebuild the refreshed quota snapshot before merging

When a subsequent Devin status response omits an optional value such as plan, a reset timestamp, or a plan date, DevinExecutor.Refresh clones the base quota and conditionally overwrites only values present in the response (internal/runtime/executor/devin_executor.go:203-223). This merge now copies that inherited map after its ObservedAt has been advanced, so omitted values from an older response are incorrectly presented to the management UI as part of the latest snapshot. Clear/rebuild updated.Quota.Signals from the status response before merging it.

Useful? React with 👍 / 👎.

@camy-x camy-x Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Addressed in 6cd99935. DevinExecutor.Refresh now rebuilds Quota.Signals from an empty map for every successful status response, so optional values omitted by the latest response cannot survive from the cloned base. The existing Refresh test now seeds stale plan-period and unknown signals and verifies that they are removed, while the manager-level test still verifies that active cooldown fields survive persistence.


// 1. Refresh Lifecycle Timestamps
if !updated.LastRefreshedAt.IsZero() {
Expand Down
Loading