From 20d3c85cf945439bcf9889d5a56facf9ce1c02f6 Mon Sep 17 00:00:00 2001 From: bussyjd Date: Fri, 7 Aug 2026 18:35:34 +0400 Subject: [PATCH] fix(discovery): never pin a failed upstream OpenAPI probe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit upstreamOpenAPICache.refresh keys on offer.Generation and short-circuits once an entry exists for that generation. It recorded the fetch result unconditionally — including nil — so a single failed probe pinned the offer to its route-table fallback until someone edited the CR. The blast radius is not one offer. reconcileStaticSite rebuilds the SHARED bundle from this cache on every offer's reconcile, so one cached nil overwrites a good document for the whole stack. A controller restart empties the cache and gives each offer exactly one attempt, which is where this is most likely to bite: any offer whose single post-restart probe misses — upstream still rolling, cold start, transient 5xx, or the probe's short timeout — silently degrades from one resource per real paid route to a single root entry. The cache doc comment already described it as holding "the last-good upstream fetch"; treating a nil as good is what broke that contract. refresh now leaves the generation unrecorded when a probe misses on an offer that COULD serve a document, so the next reconcile retries and any last-good doc survives: stale beats silently collapsed. The distinction matters, because fetchUpstreamOpenAPI also returns nil by design for agent and inference offers and for offers with no upstream Service. Those can never serve a document, so their nil is terminal and must still be cached — otherwise they would be probed on every single reconcile forever. offerHasProbeableUpstream is extracted so refresh and fetchUpstreamOpenAPI cannot disagree about which case they are in. Observed on a live stack: after a controller image swap, three offers dropped from 12/12/16 advertised resources to 1 while two others were unaffected, then recovered as their reconciles landed. Verified the new test fails without the guard (fetch called once instead of twice, and the later success never cached) and passes with it. --- .../upstream_openapi.go | 34 ++++- .../upstream_openapi_cache_test.go | 144 ++++++++++++++++++ 2 files changed, 174 insertions(+), 4 deletions(-) create mode 100644 internal/serviceoffercontroller/upstream_openapi_cache_test.go 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") + } +}