Persist refused refresh-token suppression

internal/auth/auth.go

@@ -48,9 +48,10 @@ type Manager struct {
 	// delete, so this process remembers not to send it again. Guarded by mu.
 	refusedRefreshToken string
 
-	// credentialCleared runs after the manager has deleted a credential on its own
-	// verdict, so the owner of the response cache can drop what that credential
-	// fetched. Logout is not that: its callers already clear the cache themselves.
+	// credentialCleared runs after the manager has deleted a credential, or
+	// replaced it with a signed-out record, on its own verdict. The owner of the
+	// response cache can then drop what that credential fetched. Logout is not
+	// that: its callers already clear the cache themselves.
 	credentialCleared func()
 }
 
@@ -312,9 +313,9 @@ func (m *Manager) LoginWithCookie(cookie string) error {
 }
 
 // OnCredentialCleared registers what to run when the manager clears a credential
-// on its own — today, when the server has refused the refresh token. It does not
-// run for Logout, whose callers clear the cache themselves, and not when the store
-// refused the deletion, because the credential is then still there to be used.
+// on its own — today, when the server has refused the refresh token. Replacing a
+// credential with a signed-out record after deletion fails counts as clearing it.
+// Logout does not: its callers clear the cache themselves.
 func (m *Manager) OnCredentialCleared(fn func()) {
 	m.mu.Lock()
 	defer m.mu.Unlock()
@@ -401,6 +402,9 @@ func (m *Manager) refreshLocked(ctx context.Context, creds *Credentials) error {
 		return nil
 	}
 	creds = cloneCredentials(stored)
+	if creds.AccessToken == "" && creds.RefreshToken == "" && creds.SessionCookie == "" {
+		return errNoCredential("stored credentials are signed out", nil)
+	}
 
 	// Cookie-based auth has nothing to refresh. The fresh read above still lets
 	// a 401 adopt a cookie another process stored before the SDK retries.
@@ -486,10 +490,22 @@ func (m *Manager) accountForRefreshFailure(err error, sentRefreshToken string) e
 	// Forget it under the lock that already spans this load-refresh-save, so the
 	// next command asks for a login instead of re-sending it.
 	if delErr := m.store.delete(m.baseURL); delErr != nil {
-		// The credential is still on disk for the next command to load, so the
-		// refusal is remembered here instead.
+		// Keep the process-local guard even if the durable fallback also fails.
 		m.refusedRefreshToken = sentRefreshToken
-		return errRefusedGrant(delErr)
+
+		// The keyring can occasionally refuse a delete while still allowing a
+		// write. Replace the dead grant with a signed-out record so another
+		// process cannot load and submit it again. A later login overwrites this
+		// through the ordinary save path.
+		tombstone := &Credentials{}
+		if saveErr := m.store.save(m.baseURL, tombstone); saveErr != nil {
+			return errRefusedGrant(errors.Join(delErr, fmt.Errorf("replace refused credential: %w", saveErr)))
+		}
+		m.cachedCredentials = cloneCredentials(tombstone)
+		if m.credentialCleared != nil {
+			m.credentialCleared()
+		}
+		return errRefusedGrant(nil)
 	}
 	m.cachedCredentials = nil
 	// Cached mail must not outlive the credential that fetched it, here as much

internal/auth/auth_test.go

@@ -982,51 +982,69 @@ func TestRefreshForgetsAGrantTheServerRefused(t *testing.T) {
 }
 
 // Deleting the credential is what normally stops a refused grant being sent again.
-// When the store will not let go of it, the refusal has to be remembered instead, or
-// the next command loads the same dead token and spends another attempt on it.
-func TestARefusedGrantIsNotResentWhenItCannotBeDeleted(t *testing.T) {
+// When the store will not let go of it, an empty credential takes its place so a
+// later process cannot load and resend the same dead token.
+func TestARefusedGrantIsNotResentByANewManagerWhenItCannotBeDeleted(t *testing.T) {
 	calls := 0
 	server := httptest.NewServer(invalidGrantHandler(&calls))
 	defer server.Close()
 
 	t.Setenv("HEY_TOKEN", "")
 	t.Setenv("HEY_NO_KEYRING", "")
-	mgr := NewManager(server.URL, server.Client(), t.TempDir())
-
-	// A keyring that stores and reads but refuses to delete.
+	configDir := t.TempDir()
 	stored := ""
-	mgr.GetStore().useKeyring = true
-	mgr.GetStore().initOnce.Do(func() {})
-	mgr.GetStore().keyring = credentialKeyring{
-		set:    func(_, _, password string) error { stored = password; return nil },
-		get:    func(_, _ string) (string, error) { return stored, nil },
-		delete: func(_, _ string) error { return errors.New("keyring is locked") },
+	newManager := func() *Manager {
+		mgr := NewManager(server.URL, server.Client(), configDir)
+		mgr.GetStore().useKeyring = true
+		mgr.GetStore().initOnce.Do(func() {})
+		mgr.GetStore().keyring = credentialKeyring{
+			set:    func(_, _, password string) error { stored = password; return nil },
+			get:    func(_, _ string) (string, error) { return stored, nil },
+			delete: func(_, _ string) error { return errors.New("keyring is locked") },
+		}
+		return mgr
 	}
 
-	saveExpiredCredential(t, mgr)
-
-	for range 4 {
-		if _, err := mgr.AccessToken(t.Context()); err == nil {
-			t.Fatal("AccessToken succeeded with a refused grant")
-		}
+	first := newManager()
+	saveExpiredCredential(t, first)
+	second := newManager()
+	if authenticated, err := second.AuthenticationStatus(); err != nil || !authenticated {
+		t.Fatalf("second manager did not cache the original credential: authenticated = %t, error = %v", authenticated, err)
 	}
 
-	if calls != 1 {
-		t.Errorf("refresh requests = %d, want 1 — the refusal has to outlive a delete that failed", calls)
+	if _, err := first.AccessToken(t.Context()); err == nil {
+		t.Fatal("first manager accepted a refused grant")
 	}
 
-	_, err := mgr.AccessToken(t.Context())
+	_, err := second.AccessToken(t.Context())
 	var authErr *apierr.Error
 	if !errors.As(err, &authErr) || authErr.Code != apierr.CodeAuth {
-		t.Errorf("error = %v, want one coded %q", err, apierr.CodeAuth)
+		t.Fatalf("second manager error = %v, want one coded %q", err, apierr.CodeAuth)
+	}
+	if calls != 1 {
+		t.Errorf("refresh requests = %d, want one across both managers", calls)
+	}
+	authenticated, statusErr := second.AuthenticationStatus()
+	if statusErr != nil {
+		t.Fatalf("AuthenticationStatus: %v", statusErr)
+	}
+	if authenticated {
+		t.Error("the refused credential still reports authenticated")
+	}
+
+	if err := second.LoginWithToken("fresh-access"); err != nil {
+		t.Fatalf("LoginWithToken: %v", err)
+	}
+	if token, err := second.AccessToken(t.Context()); err != nil || token != "fresh-access" {
+		t.Fatalf("AccessToken after login = %q, %v", token, err)
 	}
 }
 
 // Forgetting the credential is the manager's call, so whoever owns the response
 // cache has to hear about it from here: cached mail must not outlive the credential
-// that fetched it. The hook runs only when the credential actually went — a refusal
-// the store would not delete, or a failure that is no verdict on the grant, keeps
-// the credential and so keeps the cache.
+// that fetched it. The hook runs when the credential is deleted or replaced by a
+// signed-out record. A failure that is no verdict on the grant keeps the credential
+// and so keeps the cache.
 func TestTheClearedCredentialHookRunsOnlyWhenTheCredentialWent(t *testing.T) {
 	tests := []struct {
 		name         string
@@ -1036,7 +1054,7 @@ func TestTheClearedCredentialHookRunsOnlyWhenTheCredentialWent(t *testing.T) {
 		wantRuns     int
 	}{
 		{name: "refused grant", status: http.StatusBadRequest, body: `{"error":"invalid_grant"}`, wantRuns: 1},
-		{name: "refused grant the store keeps", status: http.StatusBadRequest, body: `{"error":"invalid_grant"}`, refuseDelete: true, wantRuns: 0},
+		{name: "refused grant replaced after delete fails", status: http.StatusBadRequest, body: `{"error":"invalid_grant"}`, refuseDelete: true, wantRuns: 1},
 		{name: "rate limited", status: http.StatusTooManyRequests, body: `{"error":"rate_limit_exceeded"}`, wantRuns: 0},
 		{name: "origin failure", status: http.StatusBadGateway, body: "upstream unavailable", wantRuns: 0},
 	}
@@ -1083,8 +1101,15 @@ func TestTheClearedCredentialHookRunsOnlyWhenTheCredentialWent(t *testing.T) {
 			if runs != tt.wantRuns {
 				t.Errorf("hook ran %d times, want %d", runs, tt.wantRuns)
 			}
-			if kept := stored != ""; kept != (tt.wantRuns == 0) {
-				t.Errorf("credential kept = %v; the hook has to run exactly when it is gone", kept)
+			var remaining Credentials
+			if stored != "" {
+				if err := json.Unmarshal([]byte(stored), &remaining); err != nil {
+					t.Fatalf("stored credential: %v", err)
+				}
+			}
+			kept := remaining.AccessToken != "" || remaining.SessionCookie != ""
+			if kept != (tt.wantRuns == 0) {
+				t.Errorf("usable credential kept = %v; the hook has to run exactly when it is gone", kept)
 			}
 		})
 	}