From 409cd354ce80ee2302bb96aa198f100c6fa51672 Mon Sep 17 00:00:00 2001
From: Ravi Pina <ravi@cow.org>
Date: Tue, 16 Jun 2026 10:07:48 -0700
Subject: [PATCH] fix: clear stale refresh failure on next success

A successful refresh is always newer than any prior failure, so carrying
LastFailure forward made `show api status` report a stale failure after
recovery. Zero LastFailure/LastError on the success path instead.
---
 internal/vendors/cache_manager_refresh.go | 13 +++----
 internal/vendors/cache_types.go           |  8 ++---
 internal/vendors/refresh_failure_test.go  | 42 +++++++++++++++++++++++
 3 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/internal/vendors/cache_manager_refresh.go b/internal/vendors/cache_manager_refresh.go
index ed33f34..f8fd0a3 100644
--- a/internal/vendors/cache_manager_refresh.go
+++ b/internal/vendors/cache_manager_refresh.go
@@ -135,15 +135,12 @@ func (c *CacheManager) doRefreshAPI(ctx context.Context, apiLabel string, opts R
 	// Create new cache
 	cache := NewAPICache(apiLabel, config.Vendor, config.Credentials["org_id"])
 	cache.Meta.LastRefresh = startTime
-	// A fresh cache resets the failure fields; this is the success path, so leave
-	// LastError clear and carry the prior LastFailure forward as history (the
-	// prior cache, when not already loaded for device-config merge, is read once).
+	// This is the success path, and startTime is newer than any prior failure, so
+	// the failure is stale: clear both LastError and LastFailure. Leaving them
+	// (zero by construction in NewAPICache) means status reads as healthy with no
+	// lingering failure once a success supersedes it.
 	cache.Meta.LastError = ""
-	if existingCache != nil {
-		cache.Meta.LastFailure = existingCache.Meta.LastFailure
-	} else if prior, err := c.GetAPICache(apiLabel); err == nil {
-		cache.Meta.LastFailure = prior.Meta.LastFailure
-	}
+	cache.Meta.LastFailure = time.Time{}
 
 	// Fetch sites
 	fmt.Printf("    Fetching sites...")
diff --git a/internal/vendors/cache_types.go b/internal/vendors/cache_types.go
index 7b72e67..9394d01 100644
--- a/internal/vendors/cache_types.go
+++ b/internal/vendors/cache_types.go
@@ -11,10 +11,10 @@ type APICacheMeta struct {
 	LastRefresh       time.Time      `json:"last_refresh"` // last successful refresh
 	RefreshDurationMs int64          `json:"refresh_duration_ms"`
 	ItemCounts        map[string]int `json:"item_counts"`
-	// LastFailure/LastError record the most recent failed refresh. LastError is
-	// cleared on the next success so the current state reads cleanly, while
-	// LastFailure is retained as history. "Currently failing" is derived, not
-	// stored: LastFailure.After(LastRefresh).
+	// LastFailure/LastError record the most recent failed refresh. Both are
+	// cleared on the next success, since a refresh newer than the failure makes
+	// it stale. A non-zero LastFailure therefore always reflects the current
+	// state, never history.
 	LastFailure time.Time `json:"last_failure"`
 	LastError   string    `json:"last_error,omitempty"`
 }
diff --git a/internal/vendors/refresh_failure_test.go b/internal/vendors/refresh_failure_test.go
index a5aa866..83a7406 100644
--- a/internal/vendors/refresh_failure_test.go
+++ b/internal/vendors/refresh_failure_test.go
@@ -1,6 +1,7 @@
 package vendors
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"testing"
@@ -66,6 +67,47 @@ func TestRecordRefreshFailureLocked(t *testing.T) {
 	}
 }
 
+func TestRefreshAPIClearsPriorFailure(t *testing.T) {
+	registry := NewAPIClientRegistry()
+	registry.RegisterFactory("mock", func(config *APIConfig) (Client, error) {
+		return NewMockClientWithAllServices(config.Vendor, config.Credentials["org_id"]), nil
+	})
+	registry.InitializeClients(map[string]*APIConfig{
+		"test-api": {Label: "test-api", Vendor: "mock", Credentials: map[string]string{"org_id": "org-123"}},
+	})
+
+	cm := NewCacheManager(t.TempDir(), registry)
+	if err := cm.Initialize(); err != nil {
+		t.Fatalf("Initialize: %v", err)
+	}
+	ctx := context.Background()
+
+	// Seed a cache, then record a failure on top of it.
+	if err := cm.RefreshAPI(ctx, "test-api"); err != nil {
+		t.Fatalf("initial RefreshAPI: %v", err)
+	}
+	cm.recordRefreshFailureLocked("test-api", errors.New("POST /rest/login: context deadline exceeded"))
+
+	if got, _ := cm.GetAPICache("test-api"); got.Meta.LastFailure.IsZero() {
+		t.Fatal("precondition: LastFailure should be set after recorded failure")
+	}
+
+	// A subsequent successful refresh supersedes the failure, so it must clear.
+	if err := cm.RefreshAPI(ctx, "test-api"); err != nil {
+		t.Fatalf("second RefreshAPI: %v", err)
+	}
+	got, err := cm.GetAPICache("test-api")
+	if err != nil {
+		t.Fatalf("GetAPICache: %v", err)
+	}
+	if !got.Meta.LastFailure.IsZero() {
+		t.Errorf("LastFailure not cleared by success: got %v", got.Meta.LastFailure)
+	}
+	if got.Meta.LastError != "" {
+		t.Errorf("LastError not cleared by success: got %q", got.Meta.LastError)
+	}
+}
+
 func TestRecordRefreshFailureLocked_NoPriorCacheIsNoop(t *testing.T) {
 	cm := NewCacheManager(t.TempDir(), NewAPIClientRegistry())
 	if err := cm.Initialize(); err != nil {