Merge pull request #105 from tstromberg/main

auto-recover from auth failures

Author: tstromberg

cmd/review-goose/github.go

@@ -527,10 +527,7 @@ func (app *App) fetchTurnDataSync(ctx context.Context, issues []*github.Issue, u
 			continue
 		}
 
-		wg.Add(1)
-		go func(issue *github.Issue) {
-			defer wg.Done()
-
+		wg.Go(func() {
 			// Acquire semaphore
 			sem <- struct{}{}
 			defer func() { <-sem }()
@@ -548,7 +545,7 @@ func (app *App) fetchTurnDataSync(ctx context.Context, issues []*github.Issue, u
 				isOwner:      issue.GetUser().GetLogin() == user,
 				wasFromCache: wasFromCache,
 			}
-		}(issue)
+		})
 	}
 
 	// Close the results channel when all goroutines are done

cmd/review-goose/icons_badge.go

@@ -55,7 +55,7 @@ func getIcon(iconType IconType, counts PRCounts) []byte {
 	}
 
 	// Check cache
-	if cached, ok := cache.Get(incoming, outgoing); ok {
+	if cached, ok := cache.Lookup(incoming, outgoing); ok {
 		return cached
 	}
 

cmd/review-goose/main.go

@@ -70,6 +70,7 @@ const (
 	maxConcurrentTurnAPICalls = 20
 	defaultMaxBrowserOpensDay = 20
 	startupGracePeriod        = 1 * time.Minute // Don't play sounds or auto-open for first minute
+	authRetryInterval         = 2 * time.Minute // Retry authentication periodically when in error state
 )
 
 // PR represents a pull request with metadata.
@@ -420,6 +421,46 @@ func (app *App) handleReauthentication(ctx context.Context) {
 	}
 }
 
+// authRetryLoop periodically attempts to re-authenticate when in auth error state.
+// This ensures the app can recover from transient auth failures without user intervention.
+func (app *App) authRetryLoop(ctx context.Context) {
+	ticker := time.NewTicker(authRetryInterval)
+	defer ticker.Stop()
+
+	slog.Info("[AUTH] Starting auth retry loop", "interval", authRetryInterval)
+
+	for {
+		select {
+		case <-ctx.Done():
+			slog.Info("[AUTH] Auth retry loop stopped (context cancelled)")
+			return
+		case <-ticker.C:
+			app.mu.RLock()
+			hasAuthError := app.authError != ""
+			app.mu.RUnlock()
+
+			if !hasAuthError {
+				slog.Info("[AUTH] Auth error cleared, stopping retry loop")
+				return
+			}
+
+			slog.Info("[AUTH] Attempting automatic re-authentication")
+			app.handleReauthentication(ctx)
+
+			// Check if we recovered
+			app.mu.RLock()
+			stillHasError := app.authError != ""
+			app.mu.RUnlock()
+
+			if !stillHasError {
+				slog.Info("[AUTH] Automatic re-authentication successful, stopping retry loop")
+				return
+			}
+			slog.Info("[AUTH] Re-authentication failed, will retry", "nextRetry", authRetryInterval)
+		}
+	}
+}
+
 func (app *App) onReady(ctx context.Context) {
 	slog.Info("System tray ready")
 
@@ -501,6 +542,8 @@ func (app *App) onReady(ctx context.Context) {
 		app.rebuildMenu(ctx)
 		// Clean old cache on startup
 		app.cleanupOldCache()
+		// Start background auth retry loop
+		go app.authRetryLoop(ctx)
 		return
 	}
 
@@ -721,7 +764,7 @@ func (app *App) updatePRs(ctx context.Context) {
 
 	// Process notifications using the simplified state manager
 	slog.Debug("[DEBUG] Processing PR state updates and notifications")
-	app.updatePRStatesAndNotify(ctx)
+	app.processNotifications(ctx)
 	slog.Debug("[DEBUG] Completed PR state updates and notifications")
 }
 
@@ -884,7 +927,7 @@ func (app *App) updatePRsWithWait(ctx context.Context) {
 
 	// Process notifications using the simplified state manager
 	slog.Info("[FLOW] About to process PR state updates and notifications")
-	app.updatePRStatesAndNotify(ctx)
+	app.processNotifications(ctx)
 	slog.Info("[FLOW] Completed PR state updates and notifications")
 	// Mark initial load as complete after first successful update
 	if !app.initialLoadComplete {
@@ -955,9 +998,7 @@ func (app *App) tryAutoOpenPR(ctx context.Context, pr *PR, autoBrowserEnabled bo
 	}
 }
 
-// checkForNewlyBlockedPRs provides backward compatibility for tests
-// while using the new state manager internally.
+// checkForNewlyBlockedPRs provides backward compatibility for tests.
 func (app *App) checkForNewlyBlockedPRs(ctx context.Context) {
-	// Simply delegate to the new implementation
-	app.updatePRStatesAndNotify(ctx)
+	app.processNotifications(ctx)
 }

cmd/review-goose/main_test.go

@@ -880,3 +880,154 @@ func TestNewlyBlockedPRAfterGracePeriod(t *testing.T) {
 		t.Error("Expected FirstBlockedAt to be set for newly blocked PR in state manager")
 	}
 }
+
+// TestAuthRetryLoopStopsOnSuccess tests that the auth retry loop stops when auth succeeds.
+func TestAuthRetryLoopStopsOnSuccess(t *testing.T) {
+	ctx := t.Context()
+
+	app := &App{
+		mu:               sync.RWMutex{},
+		authError:        "initial auth error",
+		systrayInterface: &MockSystray{},
+	}
+
+	// Track how many times we check authError
+	checkCount := 0
+	done := make(chan struct{})
+
+	// Start a goroutine that simulates the auth retry loop behavior
+	go func() {
+		defer close(done)
+		for {
+			select {
+			case <-ctx.Done():
+				return
+			case <-time.After(10 * time.Millisecond): // Fast ticker for testing
+				app.mu.RLock()
+				hasError := app.authError != ""
+				app.mu.RUnlock()
+
+				checkCount++
+
+				if !hasError {
+					return // Loop should exit when auth succeeds
+				}
+
+				// Simulate clearing auth error on 3rd attempt
+				if checkCount >= 3 {
+					app.mu.Lock()
+					app.authError = ""
+					app.mu.Unlock()
+				}
+			}
+		}
+	}()
+
+	// Wait for the goroutine to finish
+	select {
+	case <-done:
+		// Success - loop exited
+	case <-time.After(1 * time.Second):
+		t.Fatal("Auth retry loop did not stop after auth succeeded")
+	}
+
+	// Verify auth error was cleared
+	app.mu.RLock()
+	finalError := app.authError
+	app.mu.RUnlock()
+
+	if finalError != "" {
+		t.Errorf("Expected auth error to be cleared, got: %s", finalError)
+	}
+
+	if checkCount < 3 {
+		t.Errorf("Expected at least 3 retry attempts, got: %d", checkCount)
+	}
+}
+
+// TestAuthRetryLoopStopsOnContextCancel tests that the auth retry loop stops on context cancellation.
+func TestAuthRetryLoopStopsOnContextCancel(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+
+	app := &App{
+		mu:               sync.RWMutex{},
+		authError:        "persistent auth error",
+		systrayInterface: &MockSystray{},
+	}
+
+	done := make(chan struct{})
+
+	// Start a goroutine that simulates the auth retry loop behavior
+	go func() {
+		defer close(done)
+		ticker := time.NewTicker(10 * time.Millisecond)
+		defer ticker.Stop()
+
+		for {
+			select {
+			case <-ctx.Done():
+				return
+			case <-ticker.C:
+				app.mu.RLock()
+				hasError := app.authError != ""
+				app.mu.RUnlock()
+
+				if !hasError {
+					return
+				}
+				// Auth error persists, loop continues
+			}
+		}
+	}()
+
+	// Cancel context after a short delay
+	time.Sleep(50 * time.Millisecond)
+	cancel()
+
+	// Wait for the goroutine to finish
+	select {
+	case <-done:
+		// Success - loop exited on context cancel
+	case <-time.After(1 * time.Second):
+		t.Fatal("Auth retry loop did not stop after context cancellation")
+	}
+}
+
+// TestAuthErrorStatePreservation tests that auth error state is correctly preserved and accessible.
+func TestAuthErrorStatePreservation(t *testing.T) {
+	app := &App{
+		mu:               sync.RWMutex{},
+		systrayInterface: &MockSystray{},
+	}
+
+	// Initially no auth error
+	app.mu.RLock()
+	if app.authError != "" {
+		t.Errorf("Expected no initial auth error, got: %s", app.authError)
+	}
+	app.mu.RUnlock()
+
+	// Set auth error
+	app.mu.Lock()
+	app.authError = "token expired"
+	app.mu.Unlock()
+
+	// Verify auth error is set
+	app.mu.RLock()
+	if app.authError != "token expired" {
+		t.Errorf("Expected auth error 'token expired', got: %s", app.authError)
+	}
+	app.mu.RUnlock()
+
+	// Clear auth error
+	app.mu.Lock()
+	app.authError = ""
+	app.mu.Unlock()
+
+	// Verify auth error is cleared
+	app.mu.RLock()
+	if app.authError != "" {
+		t.Errorf("Expected auth error to be cleared, got: %s", app.authError)
+	}
+	app.mu.RUnlock()
+}

cmd/review-goose/notifications.go

@@ -116,9 +116,3 @@ func (app *App) sendPRNotification(ctx context.Context, pr *PR, title string, so
 		*playedSound = true
 	}
 }
-
-// updatePRStatesAndNotify is the simplified replacement for checkForNewlyBlockedPRs.
-func (app *App) updatePRStatesAndNotify(ctx context.Context) {
-	// Simple and clear: just process notifications
-	app.processNotifications(ctx)
-}

cmd/review-goose/ratelimit.go

@@ -144,7 +144,7 @@ func (hm *healthMonitor) recordCacheAccess(hit bool) {
 	}
 }
 
-func (hm *healthMonitor) getMetrics() map[string]any {
+func (hm *healthMonitor) metrics() map[string]any {
 	hm.mu.RLock()
 	defer hm.mu.RUnlock()
 
@@ -172,7 +172,7 @@ func (hm *healthMonitor) getMetrics() map[string]any {
 }
 
 func (hm *healthMonitor) logMetrics() {
-	metrics := hm.getMetrics()
+	m := hm.metrics()
 
 	// Get sprinkler connection status
 	sprinklerConnected := false
@@ -186,11 +186,11 @@ func (hm *healthMonitor) logMetrics() {
 	}
 
 	slog.Info("[HEALTH] Application metrics",
-		"uptime", metrics["uptime"],
-		"api_calls", metrics["api_calls"],
-		"api_errors", metrics["api_errors"],
-		"error_rate_pct", fmt.Sprintf("%.1f", metrics["error_rate"]),
-		"cache_hit_rate_pct", fmt.Sprintf("%.1f", metrics["cache_hit_rate"]),
+		"uptime", m["uptime"],
+		"api_calls", m["api_calls"],
+		"api_errors", m["api_errors"],
+		"error_rate_pct", fmt.Sprintf("%.1f", m["error_rate"]),
+		"cache_hit_rate_pct", fmt.Sprintf("%.1f", m["cache_hit_rate"]),
 		"sprinkler_connected", sprinklerConnected,
 		"sprinkler_last_connected", sprinklerLastConnected)
 }