fix(discovery): never pin a failed upstream OpenAPI probe

internal/serviceoffercontroller/upstream_openapi.go

@@ -42,11 +42,19 @@ var upstreamOpenAPIClient = &http.Client{
 // directly.
 var tryUpstreamOpenAPI = fetchUpstreamOpenAPI
 
+// offerHasProbeableUpstream reports whether this offer could ever serve an
+// upstream OpenAPI document. Agent and inference offers describe their own
+// wire format, and an offer with no upstream Service has nothing to probe.
+// For those a nil fetch is TERMINAL — there is no point retrying — whereas
+// for every other offer a nil is a transient miss. upstreamOpenAPICache.refresh
+// relies on that distinction to decide what it may cache.
+func offerHasProbeableUpstream(offer *monetizeapi.ServiceOffer) bool {
+	return offer != nil && !offer.IsAgent() && !offer.IsInference() &&
+		strings.TrimSpace(offer.Spec.Upstream.Service) != ""
+}
+
 func fetchUpstreamOpenAPI(offer *monetizeapi.ServiceOffer) map[string]any {
-	if offer == nil || offer.IsAgent() || offer.IsInference() {
-		return nil
-	}
-	if strings.TrimSpace(offer.Spec.Upstream.Service) == "" {
+	if !offerHasProbeableUpstream(offer) {
 		return nil
 	}
 	base := upstreamOpenAPIBase(offer)
@@ -159,6 +167,10 @@ func (c *upstreamOpenAPICache) get(offer *monetizeapi.ServiceOffer) map[string]a
 // offer's generation has moved on from what's cached — a requeue with no
 // spec change (e.g. the 5s convergence retry, a tunnel URL change) reuses
 // the last-good result instead of hitting the upstream again.
+//
+// A failed probe on an offer that could serve a document is NOT cached; see
+// the comment at the nil check below. The cache holds last-GOOD results, so a
+// transient miss must not be allowed to pin the degraded fallback.
 func (c *upstreamOpenAPICache) refresh(offer *monetizeapi.ServiceOffer, fetch func(*monetizeapi.ServiceOffer) map[string]any) {
 	if offer == nil {
 		return
@@ -170,6 +182,20 @@ func (c *upstreamOpenAPICache) refresh(offer *monetizeapi.ServiceOffer, fetch fu
 		return
 	}
 	doc := fetch(offer)
+	if doc == nil && offerHasProbeableUpstream(offer) {
+		// A miss on an offer that COULD serve a document is transient — the
+		// upstream may still be rolling out, or the probe's short timeout may
+		// simply have been tight. Recording it would pin this generation to the
+		// route-table fallback until someone edits the CR, and because
+		// reconcileStaticSite rebuilds the shared bundle from this cache on
+		// every offer's reconcile, that one miss would also overwrite a good
+		// document for the whole stack. Leave the generation unrecorded so the
+		// next reconcile retries, and keep any last-good doc: stale beats
+		// silently collapsed. Offers that can never serve one (agent,
+		// inference, no upstream Service) still cache their nil below, so they
+		// are probed once per generation rather than on every reconcile.
+		return
+	}
 	c.mu.Lock()
 	if c.entries == nil {
 		c.entries = map[types.UID]upstreamOpenAPICacheEntry{}

internal/serviceoffercontroller/upstream_openapi_cache_test.go

@@ -0,0 +1,144 @@
+package serviceoffercontroller
+
+import (
+	"testing"
+
+	"github.com/ObolNetwork/obol-stack/internal/monetizeapi"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func probeableOffer(gen int64) *monetizeapi.ServiceOffer {
+	return &monetizeapi.ServiceOffer{
+		ObjectMeta: metav1.ObjectMeta{UID: "uid-probeable", Generation: gen},
+		Spec: monetizeapi.ServiceOfferSpec{
+			Type:     "http",
+			Hostname: "svc.example.org",
+			Upstream: monetizeapi.ServiceOfferUpstream{Service: "svc", Port: 8080},
+		},
+	}
+}
+
+func goodDoc() map[string]any {
+	return map[string]any{"paths": map[string]any{"/v1/thing": map[string]any{}}}
+}
+
+// TestUpstreamOpenAPICache_FailedProbeIsNotPinned is the regression test for
+// the sticky-nil cache.
+//
+// refresh keys on offer.Generation and short-circuits once an entry exists for
+// that generation. Caching a FAILED probe therefore pinned the offer to the
+// route-table fallback until someone edited the CR — and because
+// reconcileStaticSite rebuilds the shared bundle from this cache on every
+// offer's reconcile, one miss could overwrite a good document for the whole
+// stack. A miss must leave the generation unrecorded so the next reconcile
+// retries.
+func TestUpstreamOpenAPICache_FailedProbeIsNotPinned(t *testing.T) {
+	c := &upstreamOpenAPICache{}
+	offer := probeableOffer(1)
+
+	calls := 0
+	failing := func(*monetizeapi.ServiceOffer) map[string]any { calls++; return nil }
+
+	c.refresh(offer, failing)
+	if got := c.get(offer); got != nil {
+		t.Errorf("after a failed probe, get = %v, want nil", got)
+	}
+	if calls != 1 {
+		t.Fatalf("fetch called %d times, want 1", calls)
+	}
+
+	// Same generation, no spec change: the miss must NOT have been pinned, so
+	// this reconcile retries rather than short-circuiting.
+	c.refresh(offer, failing)
+	if calls != 2 {
+		t.Errorf("fetch called %d times after second refresh, want 2 — the failed probe was pinned", calls)
+	}
+
+	// Upstream comes up; the retry now succeeds and is cached.
+	c.refresh(offer, func(*monetizeapi.ServiceOffer) map[string]any { return goodDoc() })
+	if got := c.get(offer); got == nil {
+		t.Fatal("after a successful probe, get = nil, want the document")
+	}
+
+	// And a LATER failure must not evict the last-good document.
+	c.refresh(probeableOffer(2), failing)
+	if got := c.get(offer); got == nil {
+		t.Error("a later failed probe evicted the last-good document; stale beats collapsed")
+	}
+}
+
+// TestUpstreamOpenAPICache_SuccessIsCachedPerGeneration keeps the original
+// contract intact: a good result is fetched once per generation, not per
+// reconcile.
+func TestUpstreamOpenAPICache_SuccessIsCachedPerGeneration(t *testing.T) {
+	c := &upstreamOpenAPICache{}
+	offer := probeableOffer(1)
+
+	calls := 0
+	ok := func(*monetizeapi.ServiceOffer) map[string]any { calls++; return goodDoc() }
+
+	c.refresh(offer, ok)
+	c.refresh(offer, ok)
+	c.refresh(offer, ok)
+	if calls != 1 {
+		t.Errorf("fetch called %d times for one generation, want 1", calls)
+	}
+
+	c.refresh(probeableOffer(2), ok)
+	if calls != 2 {
+		t.Errorf("fetch called %d times after a generation bump, want 2", calls)
+	}
+}
+
+// TestUpstreamOpenAPICache_TerminalNilIsCached guards the other half: offers
+// that can NEVER serve an upstream document (agent, inference, or no upstream
+// Service) must still cache their nil, or they would be probed on every single
+// reconcile forever.
+func TestUpstreamOpenAPICache_TerminalNilIsCached(t *testing.T) {
+	for _, tc := range []struct {
+		name string
+		spec monetizeapi.ServiceOfferSpec
+	}{
+		{"agent", monetizeapi.ServiceOfferSpec{Type: "agent", Upstream: monetizeapi.ServiceOfferUpstream{Service: "svc"}}},
+		{"inference", monetizeapi.ServiceOfferSpec{Type: "inference", Upstream: monetizeapi.ServiceOfferUpstream{Service: "svc"}}},
+		{"no upstream service", monetizeapi.ServiceOfferSpec{Type: "http"}},
+	} {
+		c := &upstreamOpenAPICache{}
+		offer := &monetizeapi.ServiceOffer{
+			ObjectMeta: metav1.ObjectMeta{UID: "uid-terminal", Generation: 1},
+			Spec:       tc.spec,
+		}
+
+		calls := 0
+		fetch := func(o *monetizeapi.ServiceOffer) map[string]any { calls++; return fetchUpstreamOpenAPI(o) }
+
+		c.refresh(offer, fetch)
+		c.refresh(offer, fetch)
+		if calls != 1 {
+			t.Errorf("%s: fetch called %d times, want 1 — a terminal nil must be cached", tc.name, calls)
+		}
+	}
+}
+
+// TestOfferHasProbeableUpstream pins the terminal/transient split itself, since
+// both fetchUpstreamOpenAPI and refresh depend on it agreeing.
+func TestOfferHasProbeableUpstream(t *testing.T) {
+	for _, tc := range []struct {
+		name string
+		spec monetizeapi.ServiceOfferSpec
+		want bool
+	}{
+		{"http with upstream", monetizeapi.ServiceOfferSpec{Type: "http", Upstream: monetizeapi.ServiceOfferUpstream{Service: "svc"}}, true},
+		{"http without upstream", monetizeapi.ServiceOfferSpec{Type: "http"}, false},
+		{"agent", monetizeapi.ServiceOfferSpec{Type: "agent", Upstream: monetizeapi.ServiceOfferUpstream{Service: "svc"}}, false},
+		{"inference", monetizeapi.ServiceOfferSpec{Type: "inference", Upstream: monetizeapi.ServiceOfferUpstream{Service: "svc"}}, false},
+	} {
+		got := offerHasProbeableUpstream(&monetizeapi.ServiceOffer{Spec: tc.spec})
+		if got != tc.want {
+			t.Errorf("%s: offerHasProbeableUpstream = %v, want %v", tc.name, got, tc.want)
+		}
+	}
+	if offerHasProbeableUpstream(nil) {
+		t.Error("nil offer must not be probeable")
+	}
+}