diff --git a/internal/serviceoffercontroller/upstream_openapi.go b/internal/serviceoffercontroller/upstream_openapi.go index e6266432d..64d68b498 100644 --- a/internal/serviceoffercontroller/upstream_openapi.go +++ b/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{} diff --git a/internal/serviceoffercontroller/upstream_openapi_cache_test.go b/internal/serviceoffercontroller/upstream_openapi_cache_test.go new file mode 100644 index 000000000..e9ec7203a --- /dev/null +++ b/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") + } +}