fix(discovery): never pin a failed upstream OpenAPI probe - #810
Merged
Conversation
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.
Merged
Contributor
Author
|
Superseded — the commit Verified in the release build on a live nine-offer stack. Note the scope, which is narrower than this PR's title suggests: it fixes the permanent form (a failed probe pinned for the whole generation, i.e. until the CR is edited). It does not remove the 30–90s window after a controller restart, when the cache starts empty and the shared bundle is rebuilt before every offer has reconciled. That is tracked separately. Closing as landed; rolled up to |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
upstreamOpenAPICache.refreshkeys onoffer.Generationand short-circuits once an entry exists for that generation:It records the result unconditionally, including
nil. So a single failed probe pins the offer to its route-table fallback until someone edits the CR.The blast radius is not one offer.
reconcileStaticSiterebuilds the shared bundle from this cache on every offer's reconcile, so one cachedniloverwrites a good document for the whole stack.A controller restart empties the cache and gives each offer exactly one attempt, which is where this bites hardest: any offer whose single post-restart probe misses — upstream still rolling out, cold start, transient 5xx, or simply the probe's short timeout — silently degrades from one resource per real paid route down to a single root entry in
/.well-known/x402andopenapi.json.The cache's own doc comment already describes it as holding "the last-good upstream fetch". Treating a
nilas good is what broke that contract.Observed live
After a controller image swap on a stack serving nine paid offers, three dropped from 12 / 12 / 16 advertised resources to 1 while two others were unaffected, then recovered as their reconciles landed. Same controller build throughout — no code change can explain it, which is what pointed at cache state.
This is pre-existing, not a regression:
git diffacross the rc2→rc3 range forupstream_openapi.gois empty.Fix
refreshnow 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 terminal/transient distinction matters, and is the reason this isn't a one-line change:
fetchUpstreamOpenAPIalso returnsnilby design for agent and inference offers, and for offers with no upstreamService. Those can never serve a document, so theirnilis terminal and must still be cached — otherwise they'd be probed on every single reconcile forever.offerHasProbeableUpstreamis extracted sorefreshandfetchUpstreamOpenAPIcannot disagree about which case they're in.Tests
upstream_openapi_cache_test.gocovers all four behaviours:nil(agent / inference / no upstream service) is cached, so those offers aren't probed foreverVerified the regression test genuinely catches the bug — with the guard removed it fails with
fetch called 1 times after second refresh, want 2 — the failed probe was pinned, and passes with it restored.go build ./...andgo test ./internal/serviceoffercontroller/...pass.Note on sequencing
v0.14.0-rc3is already published and does not contain this fix. Since the failure self-recovers as reconciles land, it wasn't judged a release blocker. This targets the integration branch for the next RC.