Improve reliability of real-time notifications

Author: Thomas Stromberg

cmd/goose/cache.go

@@ -25,7 +25,6 @@ type cacheEntry struct {
 
 // turnData fetches Turn API data with caching.
 func (app *App) turnData(ctx context.Context, url string, updatedAt time.Time) (*turn.CheckResponse, bool, error) {
-	prAge := time.Since(updatedAt)
 	hasRunningTests := false
 	// Validate URL before processing
 	if err := validateURL(url); err != nil {
@@ -57,13 +56,16 @@ func (app *App) turnData(ctx context.Context, url string, updatedAt time.Time) (
 				}
 			} else if time.Since(entry.CachedAt) < cacheTTL && entry.UpdatedAt.Equal(updatedAt) {
 				// Check if cache is still valid (10 day TTL, but PR UpdatedAt is primary check)
-				// But invalidate cache for PRs with running tests if they're fresh (< 90 minutes old)
-				if entry.Data != nil && entry.Data.PullRequest.TestState == "running" && prAge < runningTestsCacheBypass {
+				// But invalidate cache for PRs with incomplete tests if cache entry is fresh (< 90 minutes old)
+				cacheAge := time.Since(entry.CachedAt)
+				testState := entry.Data.PullRequest.TestState
+				isTestIncomplete := testState == "running" || testState == "queued" || testState == "pending"
+				if entry.Data != nil && isTestIncomplete && cacheAge < runningTestsCacheBypass {
 					hasRunningTests = true
-					slog.Debug("[CACHE] Cache invalidated - PR has running tests and is fresh",
+					slog.Debug("[CACHE] Cache invalidated - tests incomplete and cache entry is fresh",
 						"url", url,
-						"test_state", entry.Data.PullRequest.TestState,
-						"pr_age", prAge.Round(time.Minute),
+						"test_state", testState,
+						"cache_age", cacheAge.Round(time.Minute),
 						"cached_at", entry.CachedAt.Format(time.RFC3339))
 					// Don't return cached data - fall through to fetch fresh data with current time
 				} else {
@@ -160,18 +162,21 @@ func (app *App) turnData(ctx context.Context, url string, updatedAt time.Time) (
 	}
 
 	// Save to cache (don't fail if caching fails) - skip if --no-cache is set
-	// Also skip caching if tests are running and PR is fresh (updated in last 90 minutes)
+	// Don't cache when tests are incomplete - always re-poll to catch completion
 	if !app.noCache {
 		shouldCache := true
-		prAge := time.Since(updatedAt)
 
-		// Don't cache PRs with running tests unless they're older than 90 minutes
-		if data != nil && data.PullRequest.TestState == "running" && prAge < runningTestsCacheBypass {
+		// Never cache PRs with incomplete tests - we want fresh data on every poll
+		testState := ""
+		if data != nil {
+			testState = data.PullRequest.TestState
+		}
+		isTestIncomplete := testState == "running" || testState == "queued" || testState == "pending"
+		if data != nil && isTestIncomplete {
 			shouldCache = false
-			slog.Debug("[CACHE] Skipping cache for PR with running tests",
+			slog.Debug("[CACHE] Skipping cache for PR with incomplete tests",
 				"url", url,
-				"test_state", data.PullRequest.TestState,
-				"pr_age", prAge.Round(time.Minute),
+				"test_state", testState,
 				"pending_checks", len(data.PullRequest.CheckSummary.PendingStatuses))
 		}
 

cmd/goose/github.go

@@ -399,8 +399,10 @@ func (app *App) fetchPRsInternal(ctx context.Context) (incoming []PR, outgoing [
 		// Categorize as incoming or outgoing
 		// When viewing another user's PRs, we're looking at it from their perspective
 		if issue.GetUser().GetLogin() == user {
+			slog.Info("[GITHUB] Found outgoing PR", "repo", repo, "number", pr.Number, "author", pr.Author, "url", pr.URL)
 			outgoing = append(outgoing, pr)
 		} else {
+			slog.Info("[GITHUB] Found incoming PR", "repo", repo, "number", pr.Number, "author", pr.Author, "url", pr.URL)
 			incoming = append(incoming, pr)
 		}
 	}

cmd/goose/main.go

@@ -95,6 +95,7 @@ type App struct {
 	consecutiveFailures          int
 	mu                           sync.RWMutex
 	menuMutex                    sync.Mutex // Mutex to prevent concurrent menu rebuilds
+	updateMutex                  sync.Mutex // Mutex to prevent concurrent PR updates
 	enableAutoBrowser            bool
 	hideStaleIncoming            bool
 	hasPerformedInitialDiscovery bool // Track if we've done the first poll to distinguish from real state changes
@@ -508,6 +509,13 @@ func (app *App) updateLoop(ctx context.Context) {
 }
 
 func (app *App) updatePRs(ctx context.Context) {
+	// Prevent concurrent updates
+	if !app.updateMutex.TryLock() {
+		slog.Debug("[UPDATE] Update already in progress, skipping")
+		return
+	}
+	defer app.updateMutex.Unlock()
+
 	var incoming, outgoing []PR
 	err := safeExecute("fetchPRs", func() error {
 		var fetchErr error
@@ -687,6 +695,13 @@ func (app *App) updateMenu(ctx context.Context) {
 
 // updatePRsWithWait fetches PRs and waits for Turn data before building initial menu.
 func (app *App) updatePRsWithWait(ctx context.Context) {
+	// Prevent concurrent updates
+	if !app.updateMutex.TryLock() {
+		slog.Debug("[UPDATE] Update already in progress, skipping")
+		return
+	}
+	defer app.updateMutex.Unlock()
+
 	incoming, outgoing, err := app.fetchPRsInternal(ctx)
 	if err != nil {
 		slog.Error("Error fetching PRs", "error", err)

cmd/goose/sprinkler.go

@@ -339,6 +339,68 @@ func (sm *sprinklerMonitor) checkAndNotify(prURL string) {
 		return
 	}
 
+	// Log Turn API response details
+	prState := ""
+	prIsMerged := false
+	if turnData != nil {
+		prState = turnData.PullRequest.State
+		prIsMerged = turnData.PullRequest.Merged
+	}
+
+	slog.Info("[SPRINKLER] Turn API response",
+		"repo", repo,
+		"number", number,
+		"cached", wasFromCache,
+		"state", prState,
+		"merged", prIsMerged,
+		"has_data", turnData != nil,
+		"has_analysis", turnData != nil && turnData.Analysis.NextAction != nil)
+
+	// Skip closed/merged PRs and remove from lists immediately
+	if prState == "closed" || prIsMerged {
+		slog.Info("[SPRINKLER] PR closed/merged, removing from lists",
+			"repo", repo,
+			"number", number,
+			"state", prState,
+			"merged", prIsMerged,
+			"url", prURL)
+
+		// Remove from in-memory lists immediately
+		sm.app.mu.Lock()
+		originalIncoming := len(sm.app.incoming)
+		originalOutgoing := len(sm.app.outgoing)
+
+		// Filter out this PR from incoming
+		filteredIncoming := make([]PR, 0, len(sm.app.incoming))
+		for _, pr := range sm.app.incoming {
+			if pr.URL != prURL {
+				filteredIncoming = append(filteredIncoming, pr)
+			}
+		}
+		sm.app.incoming = filteredIncoming
+
+		// Filter out this PR from outgoing
+		filteredOutgoing := make([]PR, 0, len(sm.app.outgoing))
+		for _, pr := range sm.app.outgoing {
+			if pr.URL != prURL {
+				filteredOutgoing = append(filteredOutgoing, pr)
+			}
+		}
+		sm.app.outgoing = filteredOutgoing
+		sm.app.mu.Unlock()
+
+		slog.Info("[SPRINKLER] Removed PR from lists",
+			"url", prURL,
+			"incoming_before", originalIncoming,
+			"incoming_after", len(sm.app.incoming),
+			"outgoing_before", originalOutgoing,
+			"outgoing_after", len(sm.app.outgoing))
+
+		// Update UI to reflect removal
+		sm.app.updateMenu(sm.ctx)
+		return
+	}
+
 	if turnData == nil || turnData.Analysis.NextAction == nil {
 		slog.Debug("[SPRINKLER] No turn data available",
 			"repo", repo,
@@ -353,7 +415,8 @@ func (sm *sprinklerMonitor) checkAndNotify(prURL string) {
 		slog.Debug("[SPRINKLER] No action required for user",
 			"repo", repo,
 			"number", number,
-			"user", user)
+			"user", user,
+			"state", prState)
 		return
 	}
 
@@ -366,6 +429,36 @@ func (sm *sprinklerMonitor) checkAndNotify(prURL string) {
 		return
 	}
 
+	// Check if PR exists in our lists
+	sm.app.mu.RLock()
+	foundIncoming := false
+	foundOutgoing := false
+	for i := range sm.app.incoming {
+		if sm.app.incoming[i].URL == prURL {
+			foundIncoming = true
+			break
+		}
+	}
+	if !foundIncoming {
+		for i := range sm.app.outgoing {
+			if sm.app.outgoing[i].URL == prURL {
+				foundOutgoing = true
+				break
+			}
+		}
+	}
+	sm.app.mu.RUnlock()
+
+	// If PR not found in our lists, trigger a refresh to fetch it
+	if !foundIncoming && !foundOutgoing {
+		slog.Info("[SPRINKLER] New PR detected, triggering refresh",
+			"repo", repo,
+			"number", number,
+			"action", action.Kind)
+		go sm.app.updatePRs(sm.ctx)
+		return // Let the refresh handle everything
+	}
+
 	slog.Info("[SPRINKLER] Blocking PR detected via event",
 		"repo", repo,
 		"number", number,

cmd/goose/ui.go

@@ -501,6 +501,9 @@ func (app *App) generatePRSectionTitles(prs []PR, sectionTitle string, hiddenOrg
 		// Add action code if present
 		if sortedPRs[prIndex].ActionKind != "" {
 			title = fmt.Sprintf("%s — %s", title, sortedPRs[prIndex].ActionKind)
+		} else if sortedPRs[prIndex].TestState == "running" {
+			// Show "tests running" as a fallback when no specific action is available
+			title = fmt.Sprintf("%s — tests running...", title)
 		}
 
 		// Add bullet point or emoji for blocked PRs (same logic as in addPRSection)