Skip to content

Serve the cached CIMD client when a re-fetch fails transiently - #6857

Open
alex-feel wants to merge 3 commits into
stacklok:mainfrom
alex-feel:serve-stale-cimd-client-on-fetch-failure
Open

alex-feel wants to merge 3 commits into
stacklok:mainfrom
alex-feel:serve-stale-cimd-client-on-fetch-failure

Conversation

@alex-feel

@alex-feel alex-feel commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Summary

When a CIMD client's cache entry expires and the re-fetch of its metadata document fails for a transient reason (no answer within the 5-second timeout, a refused connection, a 200 whose body was cut short, a 5xx, a 408, or a 429), CIMDStorageDecorator returns fosite.ErrNotFound, and fosite renders that as 401 invalid_client at both /oauth/token and /oauth/authorize. invalid_client tells the client its credentials were rejected, so a public client discards its tokens and the user signs in again, although the server resolved and persisted that client minutes earlier, still holds the expired entry, and the same refresh succeeds as soon as the host answers. Since d90a504 (v0.56.0) the decorator serves such an entry when the fetch rate limiter refuses the re-fetch, for up to MaxStale past its expiry; a fetch that is attempted and fails transiently had no such fallback.

  • pkg/networking: FetchJSON reports a non-200 status before a body-read error, so a 4xx whose body was cut short or stalled is the host's 4xx and not a transport failure; the custom error handler receives the bytes that arrived. The CIMD fetcher is its only production caller.
  • pkg/oauthproto/cimd: FetchClientMetadataDocument wraps the failures in which the host delivered no usable response (a resolver, dial, TLS, or private-address-guard failure, a timeout, a 200 whose body was cut short or stalled, a 5xx, a 408, or a 429) in ErrDocumentUnavailable. A 3xx or any other 4xx, an unexpected content type, a malformed body, and every validation error stay definitive.
  • pkg/authserver/storage: on a transient failure the decorator answers from the expired entry within the same MaxStale window the limiter's refusal path uses, through one shared check (servableStale). The entry is not renewed and the persisted row is not rewritten, so every later lookup re-fetches again. Every failed re-fetch, fetch errors and policy rejections alike, now returns through fetchFailed, which evicts the entry whenever it does not answer from it: on a definitive failure, on a policy rejection, and on a transient failure with the entry already past the window. On main the expired entry survived a definitive failure and a later limiter refusal served it. Nothing is configurable: MaxStale is the configured access token lifespan, as for the limiter path.
  • docs/arch/11-auth-server-storage.md: the MaxStale paragraph covers the transient case, the eviction, and the persisted row; the revocation-lag text says these windows bound only how long old client metadata is accepted, not the lifetime of issued tokens and grants.

Closes #6856

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests (task test)
  • E2E tests (task test-e2e)
  • Linting (task lint-fix)
  • Manual testing (describe below)

Also run: task lint. No swagger, CRD, chart, or SDK source changes, so task docs is not needed.

New tests, each failing before the commit that adds it:

  • TestFetchJSON_StatusDecidesBeforeBodyRead: a 404 with a Content-Length longer than the bytes written gives IsHTTPError(err, 404); a 404 whose body stalls past the client timeout gives 404; a 503 cut short gives 503; a 200 cut short still gives io.ErrUnexpectedEOF; the error handler receives the bytes that arrived on a truncated 400.
  • TestFetchClientMetadataDocument_ErrDocumentUnavailable: which failures carry the sentinel (the private-address-guard refusal, a 200 cut short or stalled, a 408, and a 429 included) and which stay definitive (a 403, a 404 cut short, and a 410 that stalls among them).
  • TestCIMDStorageDecorator_TransientFetchFailure: served from the expired entry when the host disappears or answers 5xx, 408, or 429 within the window; fails closed with an empty cache past the window, with MaxStale at zero, on a 404, on a mismatched client_id, and on private_key_jwt. TestCIMDStorageDecorator_StaleServeNeitherRenewsEntryNorRepersists and TestCIMDStorageDecorator_FetchFailureWarnsOnceWithCause cover the entry, the row, and the log line; the latter's concurrent subtests show that callers sharing one singleflight call get one re-fetch, one line, and one outcome, served or failed closed with the entry evicted, and fail when fetchFailed runs outside the singleflight call.
  • TestCIMDStorageDecorator_DefinitiveFailureEndsStaleEligibility: resolve, then a 404, a 410, a 302, text/html, a malformed body, a mismatched client_id, private_key_jwt, a scope outside scopes_supported, or grant types without authorization_code; the cache is empty, a following 503, closed host, or limiter refusal fails closed, and a fresh resolve succeeds once the host answers. Main's subtest that pinned the old behavior is repurposed as "failed re-fetch evicts the entry so a later refused lookup is rate limited".

TestCIMDStorageDecorator_FetchOrCached_FetchFailureReturnsNotFound (a never-resolved client with the host answering 500) is unchanged and still passes. Every 408 and 429 row fails without the 408 and 429 classification.

Changes

File Change
pkg/networking/fetch.go FetchJSON reports a non-200 status before a body-read error; WithErrorHandler receives the bytes that arrived
pkg/oauthproto/cimd/fetch.go ErrDocumentUnavailable; classifyFetchError marks 5xx, 408, 429, transport errors, and a 200 body cut short or stalled as transient; 3xx and every other 4xx stay definitive whatever happened to the body
pkg/authserver/storage/cimd_decorator.go servableStale shared by the limiter refusal and the transient failure; fetchFailed decides, logs the fetch error with the outcome, and evicts on every failure it does not answer; fetch no longer logs a fetch failure itself
pkg/networking/fetch_test.go, pkg/oauthproto/cimd/fetch_test.go, pkg/authserver/storage/cimd_decorator_test.go the tests above; the stale fixture can switch the host's answer and close the host
docs/arch/11-auth-server-storage.md step 3 names the TTL and the persisted row; the MaxStale paragraph covers the transient case, the eviction, and the row; the shared-window and revocation-lag bullets; the known-limits bullet; the air-gapped note; the removed-client paragraph says a transient fetch failure ends in not-found only with no servable cache entry
docs/redis-storage.md the removed-client paragraph, the same correction

Does this introduce a user-facing change?

Yes, for deployments with cimd.enabled: true.

Before: a token refresh or an authorization request for a CIMD client whose cache entry (cache_fallback_ttl, default 5 minutes) had just expired was answered 401 invalid_client whenever the metadata host did not answer within 5 seconds, refused the connection, or returned 5xx, 408, or 429, unless the fetch rate limiter happened to refuse the re-fetch. Public clients treated the error as final and asked the user to sign in again. After a 404 or a rejected document the expired entry stayed in the cache, so a later limiter refusal served the withdrawn client until the cache TTL plus MaxStale had passed.

After: the same request is answered from the expired entry for up to MaxStale past its expiry, the window the limiter's refusal path already uses (the configured access token lifespan, 1 hour by default; nothing new is configurable), so the refresh succeeds. The client still waits for the 5-second fetch timeout before the answer. A 3xx or any other 4xx from the host, whatever happened to its body, an invalid or policy-rejected document, and a transient failure with the entry already past the window evict the entry, on the limiter path as well, so the next lookup needs a successful fetch, except that a lookup the limiter refuses still falls to the persisted row on /oauth/token, and in callbackClient (the callback, the consent step, and device verification) to finish an authorization already in progress (see the special notes). A client the server has never resolved, an entry evicted from the LRU, and CIMD disabled are answered invalid_client as before. The accepted trade-off is that metadata up to cache_fallback_ttl plus MaxStale old is honored while the host is unreachable or answers 5xx, 408, or 429; the same bound already applies while the limiter refuses.

Operators see the existing WARN CIMD document fetch failed once per failed fetch, keyed on client_id, now carrying the fetch error (for example HTTP 503 for URL ... or dial tcp ...) with fallback=expired_cache and age_past_expiry when the entry answered, or fallback=none. A policy-rejected document keeps its own CIMD client rejected warning and gets no second line. The limiter path's Debug lines are unchanged. No new log line is added and the rate is unchanged: a failed fetch already warned on main and already costs a limiter token.

Special notes for reviewers

  • Precedent: the transient case reuses the window the limiter's refusal path got in d90a504 (MaxStale, set to cfg.AccessTokenLifespan in pkg/authserver/server_impl.go) and the same check, so there is one bound for how long old metadata is accepted and one place that documents it. An earlier revision of this PR added its own cimd.stale_if_error knob with a CRD field; that is gone, and no config, CRD, swagger, or SDK file changes. The fallback is on by default with no off switch, as the limiter path's stale serving has been since d90a504; if a switch is wanted, it belongs on the shared window, so one setting governs both paths.
  • Delta versus main, with the gate: the stale entry answers only when errors.Is(err, cimd.ErrDocumentUnavailable) and servableStale holds; the entry exists only after a fetch that passed validation and every policy check, the fallback writes no row and renews nothing, and the window is anchored to the entry's original expiry. New: a transient failure is served. Tighter: a definitive failure evicts the entry on both paths (one cache.Remove in fetchFailed, inside the singleflight call), and a 4xx with a broken body is a 4xx. A host answering 403 or 404 to the fetch therefore evicts the entry, so until a re-fetch succeeds every attempted re-fetch fails closed and a limiter refusal on /oauth/authorize and /oauth/device_authorization is rate limited instead of served, which main answered from the stale entry; /oauth/token keeps its persisted-row fallback for limiter refusals, and callbackClient keeps its bounded one.
  • 5xx, 408, and 429 count as transient: RFC 5861 names 500, 502, 503, and 504 for stale-if-error, and a 501 or 505 equally says nothing about the document; 408 (RFC 9110 section 15.5.9) and 429 (RFC 6585 section 4) describe the request's timing or the requester's rate, not the document. readModernEnvelope in pkg/vmcp/client/modern.go already treats both as transient, and oautherr.IsTransientRetrieveError treats 429 that way. A 3xx or any other 4xx is the host's statement about the document or the requester's access to it and fails closed. Serving the entry at all is the availability-over-freshness trade-off that d90a504 introduced for the limiter path and docs/arch/11-auth-server-storage.md records (a departure from the CIMD draft's SHOULD abort on a failed fetch and from RFC 9111 for no-store documents); the transient path shares it.
  • The fallback also applies at /oauth/authorize and /oauth/device_authorization, because it is the same GetClient, exactly as the limiter's stale serving does. An unauthenticated caller gains nothing from it: the entry exists only for a client that passed every check on a successful fetch, the fallback creates no row and refreshes no TTL, a failed fetch still costs a limiter token, and the caller cannot make the host fail.
  • A refusal by the dial-time private-address guard (the host now resolving to a private address) is classified as unavailable, together with every other dial failure. A stale serve contacts nothing, so the guard's purpose is not touched; the window bounds how long the earlier document is honored, as for any other transient failure.
  • The persisted row is not a fallback source for a transient failure. On /oauth/token the row's age is bounded only by its 30-day TTL: storedClient (pkg/authserver/storage/redis.go) and the memory row carry no fetched-at time, so no tighter bound can be derived from them. callbackClient (the callback, the consent step, and device verification) reads the row within the pending-authorization or device-code lifetime plus the cache TTL plus MaxStale, as its comment states, but only on a limiter refusal; a transient failure with no servable entry is left out of it here (see below). The row fallbacks keep answering a limiter refusal only. A restarted process therefore starts with an empty cache and its first resolution of each client needs a successful fetch.
  • The persisted row is not removed on a definitive failure either: Storage has no client delete. On /oauth/token a later limiter refusal can still be answered from the row for up to 30 days after the last successful fetch; the other row fallbacks read it only to finish an authorization already in progress, within the bounds the docs give. The fetchFailed doc and the docs say so beside the eviction. Open question in the review thread: should a Storage delete for both backends go into this PR so /oauth/token stops answering from the row after a definitive failure, or should the row stay?
  • Why not a retryable error to the client: fosite wraps every GetClient error into ErrInvalidClient (fosite v0.49.0 client_authentication.go:200-202 for every token grant, authorize_request_handler.go:360-362), so the decorator cannot answer temporarily_unavailable itself; the handlers can, by errors.Is on the wrapped error, which is how a limiter refusal becomes a 429 today (handlers/token.go, handlers/authorize.go, handlers/device_authorization.go; temporarily_unavailable on /oauth/token and /oauth/device_authorization, plain text on /oauth/authorize). With a servable entry the lookup succeeds and no error code is involved, and the decorator is the only place that still holds the client. A transient failure with no servable entry (a cold cache, an entry past the window, an LRU eviction) still answers invalid_client as on main; mapping cimd.ErrDocumentUnavailable to the limiter's 429 there is a change of its own, left out below.
  • Logging: fetch no longer logs a fetch failure; fetchFailed logs it once after the stale decision, with the wrapped cause rather than fosite's bare not_found, so the line carries both the error and the outcome. Policy rejections keep their WARN in fetch.
  • Left out, not promised: stale-while-revalidate (answering from the entry at once and refreshing in the background); eviction on a limiter refusal past the window (main's behavior kept, since no document failure happened); the persisted row as a transient-failure source, which on /oauth/token needs a fetched-at stamp on the row and in callbackClient would be bounded as its limiter-refusal read is; a 429 for a transient failure with no servable entry, through the handler mapping the limiter refusal uses; reading Retry-After on a 429 (the fallback answers from the entry, and the fetch limiter paces re-fetches); widening the transient set to 403, 421, 423, or 425, which stay definitive.

@codecov

codecov Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.14%. Comparing base (1e617ad) to head (666612d).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6857   +/-   ##
=======================================
  Coverage   80.14%   80.14%           
=======================================
  Files         845      845           
  Lines       85670    85740   +70     
=======================================
+ Hits        68659    68717   +58     
- Misses      17006    17018   +12     
  Partials        5        5           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jhrozek jhrozek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three things from reading the fallback path. The first two are about definitive failures still being able to end up in a stale serve; the third is a docs wording point.

// invalid_client/invalid_request with the hint dropped by fosite's production
// error rendering, so without a server-side log line these failures are
// undiagnosable (see issue #6186).
func (d *CIMDStorageDecorator) fetchFailed(ctx context.Context, id string, err error) (fosite.Client, error) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expired entry stays in the cache after a definitive failure, so a later transient failure can bring a rejected client back. Sequence: resolve OK, TTL passes, the host answers 410 (or a document we reject, e.g. an unsupported token_endpoint_auth_method or scopes), then the host times out or returns 503 inside the window. staleEntry finds the old entry and serves it. Nothing in this file calls cache.Remove, and the window is anchored to the original expires, so it doesn't reset either.

Suggested change: when fetchFailed doesn't answer from the stale entry, call d.cache.Remove(id) before returning (an entry past the window is useless anyway). Do the same for the policy rejections in fetch; moving the post-fetch validation into a helper that removes on error keeps that to one place.

The test should run the whole sequence: resolve, switch the host to 404/410 (or a mismatched client_id, or private_key_jwt), wait out the TTL, expect the failure and an empty cache, then switch to 503 and expect ErrNotFound. The current table tests stop at the first rejection. The struct doc and the fetchFailed doc should also say a definitive failure ends stale eligibility.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in Evict CIMD cache entry on definitive fetch failure (3ea338d).

Slightly different shape from the suggestion: every failed re-fetch, fetch errors and policy rejections alike, now returns through fetchFailed, which evicts the entry whenever it does not answer from it. One d.cache.Remove(id) there covers the class without a separate validation helper, catches a rejection added later anywhere in fetch, and runs inside the singleflight call, so no concurrent fetch can add a fresh entry between the decision and the eviction. It evicts on a 3xx or 4xx (whatever happened to the body), an unexpected content type, a malformed body, a document that fails validation, a document this server rejects (token_endpoint_auth_method, grant types, response types, a scope outside scopes_supported), and a transient failure whose entry is already past MaxStale.

The branch now sits on the MaxStale path main gained in d90a504 (details in the summary), so the eviction applies to the limiter's stale serving too: main's subtest that pinned "a failed re-fetch leaves the entry for a later refused lookup" is repurposed as "failed re-fetch evicts the entry so a later refused lookup is rate limited".

Test: TestCIMDStorageDecorator_DefinitiveFailureEndsStaleEligibility runs the whole sequence: resolve, switch the host to each definitive answer (404, 410, a 302, text/html, a malformed body, a mismatched client_id, private_key_jwt, a scope outside scopes_supported, grant types without authorization_code), expect the failure and an empty cache, then switch to 503, a closed host, or a refusing limiter and expect ErrNotFound (wrapping ErrCIMDRateLimited only in the limiter case), then a fresh resolve once the host answers again. 27 subtests; the fail-closed rows of TestCIMDStorageDecorator_TransientFetchFailure assert the empty cache as well. The struct doc, the fetchFailed doc, and the MaxStale paragraph in docs/arch/11-auth-server-storage.md say a definitive failure ends stale eligibility.

Versus main: there the expired entry survived a 404 or a rejected document, and a later limiter refusal served it until the cache TTL plus MaxStale had passed; now the next lookup needs a successful fetch. One thing the eviction does not reach: the persisted row (Storage has no client delete), so on /oauth/token and /oauth/callback a later limiter refusal still answers from the row for up to 30 days after the last successful fetch. The code and the docs say so beside the eviction; whether to add a delete here is a question in the summary.

return fmt.Errorf("failed to fetch client metadata document: %w", err)
}
var netErr net.Error
if errors.As(err, &netErr) || errors.Is(err, io.ErrUnexpectedEOF) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

networking.FetchJSON reads the body before it checks the status (pkg/networking/fetch.go:147 vs :153). A 403/404/410 whose body is cut short returns io.ErrUnexpectedEOF, and a stalled body returns a timeout. Both land in this branch as transient, so a host that explicitly rejects the client can still get the stale entry served.

Smallest fix I can see: in FetchJSON, keep the read error, return the HTTPError for any non-200 status first, and only then return the body-read error. This is its only production caller, so nothing else changes. Test: a 404 that sets a Content-Length longer than the bytes it writes should give IsHTTPError(err, 404).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in Check HTTP status before body errors in FetchJSON (dc0c16f).

FetchJSON now keeps the read error, reports a non-200 status first (through the custom error handler or as an HTTPError), and returns the body-read error only for a 200. The handler receives whatever bytes arrived, a prefix of the body or nothing. pkg/oauthproto/cimd/fetch.go is the only production caller, and a response whose body arrives in full is reported exactly as before.

Tests: TestFetchJSON_StatusDecidesBeforeBodyRead has the case you named, a 404 with a Content-Length longer than the bytes written, asserting IsHTTPError(err, 404), plus a 404 whose body stalls past the client timeout (404), a 503 cut short (503), a 200 cut short (io.ErrUnexpectedEOF, the control), and the error handler receiving the bytes that arrived on a truncated 400. On the classifier side, TestFetchClientMetadataDocument_ErrDocumentUnavailable gained "404 whose body is cut short" and "410 whose body stalls past the fetch timeout" as definitive, beside "200 whose body is cut short" and "503 whose body is cut short" as unavailable.

Versus main: FetchJSON there reported a truncated or stalled 4xx as io.ErrUnexpectedEOF or a timeout; now it is the host's 4xx. The decorator fails closed on it as main did and evicts the entry; the previous head of this branch would have taken it for a transient failure and served the stale entry.

Comment thread docs/arch/11-auth-server-storage.md Outdated
- **Policy rejection** (a document that was fetched and validated but declares what this server does not accept): rejected with `invalid_client` and its own `CIMD client rejected` warning, as before. The window never applies here, because the fetch succeeded.
- **No entry to fall back to** (a client this server has never resolved, an entry evicted from the LRU, or one past the window): the lookup fails closed as before.

fosite renders every failed `GetClient` as `invalid_client`, which tells the client its credentials were rejected, so without the window a transient blip at the moment an entry expired made public clients discard valid tokens and sign in again. The trade-off is that metadata up to `cache_fallback_ttl` plus `stale_if_error` old is honored while the host is unreachable; a withdrawn redirect URI or scope takes effect no later than that, and operators who need a tighter bound shorten the window. Two cases are deliberately not covered: the client still waits for the 5-second fetch timeout before the stale answer (there is no background revalidation), and a restarted process starts with an empty cache, so its first resolution of each client needs a successful fetch (the persisted row carries no fetched-at time and is not used as a fallback).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Takes effect no later than" reads like a revocation guarantee. It only bounds how long old client metadata is accepted at later lookups; tokens and grants already issued follow their own expiry and revocation. A short clause saying so would keep operators from using this window as a revocation bound.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in Serve stale CIMD client on transient fetch failure (792a249).

The "takes effect no later than" sentence is gone with its section: the re-fetch behavior now lives in main's MaxStale paragraph of docs/arch/11-auth-server-storage.md, and the revocation-lag paragraph below it ends with: "These windows bound only how long old client metadata is accepted at later lookups; tokens and grants already issued follow their own expiry and revocation, so none of them is a revocation bound for issued credentials."

Docs only, no test. Versus main: that paragraph already said a stolen refresh token for a withdrawn client stays usable up to the lag and never beyond the refresh token's own lifetime; the clause makes explicit that none of these windows is a revocation bound.

@alex-feel
alex-feel force-pushed the serve-stale-cimd-client-on-fetch-failure branch from 899ee14 to 3ea338d Compare October 23, 2026 19:13
@alex-feel

Copy link
Copy Markdown
Contributor Author

All three are in, one commit each, in your order:

  1. Definitive failures leaving the stale entry behind: Evict CIMD cache entry on definitive fetch failure (3ea338d). Every failed re-fetch, fetch errors and policy rejections alike, returns through fetchFailed, which evicts the entry whenever it does not answer from it, so one cache.Remove covers the class instead of a separate validation helper. TestCIMDStorageDecorator_DefinitiveFailureEndsStaleEligibility runs the full sequence for nine definitive answers, each followed by a 503, a closed host, or a limiter refusal.
  2. FetchJSON reading the body before the status: Check HTTP status before body errors in FetchJSON (dc0c16f). A non-200 is reported first and the read error only for a 200; TestFetchJSON_StatusDecidesBeforeBodyRead has the truncated 404 giving IsHTTPError(err, 404).
  3. The docs wording: Serve stale CIMD client on transient fetch failure (792a249). The "takes effect no later than" sentence is gone, and the revocation-lag paragraph now says these windows bound only how long old client metadata is accepted; issued tokens and grants follow their own expiry and revocation.

The branch is rebuilt on main, which gained the fetch limiter and MaxStale in d90a504 (v0.56.0) while this was open. The cimd.stale_if_error knob is gone (RunConfig, Config, the CRD field, swagger, the Go SDK, the operator mapping): a transient re-fetch failure now serves the expired entry within the same MaxStale window the limiter's refusal path uses, through one shared check (servableStale), and nothing is configurable. The PR is down to 7 files (three source files, their tests, and the storage docs page), and the body is rewritten for this shape. The eviction from point 1 applies to both paths, which also closes the gap the docs stated for the limiter path (a failed re-fetch did not evict the entry, so a later refusal served the pre-failure client).

Two things for you to settle:

  • Shape: sharing MaxStale and servableStale with the refusal path, as here, or a separate configurable window for the transient case? I prefer the shared window: one bound for how long old metadata is accepted, documented once, and the access-token lifespan is already the smallest value that covers a refresh.
  • The persisted row: a definitive failure evicts the cache entry but not the row (Storage has no client delete), so on /oauth/token and /oauth/callback a later limiter refusal still answers from the row for up to 30 days after the last successful fetch. The docs and the fetchFailed doc say so. Should a Storage delete for both backends go into this PR, or should the row stay as it is? If you want it separate, I can open an issue for it.

Ready for another look when you have time.

@alex-feel
alex-feel requested a review from jhrozek October 23, 2026 19:56

@ChrisJBurns ChrisJBurns left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • high · intent — Described cimd.stale_if_error knob, CRD field, operator/SDK wiring are absent from the diff

    The diff reuses MaxStale, which is the access-token lifespan and not user-configurable, as the updated docs say. Operators told to set staleIfError: "0s" for the old behaviour have no such setting. config.go, runner, CRD and generated files are untouched.

  • medium · intent — Description says other failures fail closed as on main, but definitive failures now evict the entry

    The PR body and its test names (e.g. RefetchFailureAfterPriorResolution, NegativeStaleIfError) do not match the diff, which adds eviction on definitive failure and different tests. Reviewers judging from the description miss this.

  • medium · intent — Promised stale_if_error setting and opt-out are missing (docs/arch/11-auth-server-storage.md, pkg/authserver/storage/cimd_decorator.go)

    The implementation reuses MaxStale, tied to access_token_lifespan, and adds no configuration plumbing. Operators cannot select the promised independent window or disable transient fallback with "0s"; a 24-hour token lifespan permits 24 hours stale.

Comment thread pkg/oauthproto/cimd/fetch.go Outdated
func classifyFetchError(err error) error {
var httpErr *networking.HTTPError
if errors.As(err, &httpErr) {
if httpErr.StatusCode >= http.StatusInternalServerError {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium · correctness — HTTP 429/408 from the metadata host are definitive: entry evicted, client gets invalid_client

A shared metadata host that rate-limits (429) or times out a request (408) is answering transiently. Here that evicts a known client and signs users out, the outcome this PR aims to prevent. That is likely under load on a popular host.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in Serve stale CIMD client on transient fetch failure (a8ad4a6). A 408 or a 429 now carries ErrDocumentUnavailable like a 5xx: neither says anything about the document (RFC 9110 section 15.5.9, RFC 6585 section 4), and readModernEnvelope in pkg/vmcp/client/modern.go already treats both as transient, as oautherr.IsTransientRetrieveError does for 429. After a prior resolution, a 408 or 429 on the re-fetch is answered from the expired entry within MaxStale and the entry stays; past the window the lookup fails closed and evicts the entry, as for a 5xx. Every 3xx and every other 4xx stays definitive, 403 included: that is the host refusing the document, whatever produced it.

Tests: the "408 request timeout" and "429 too many requests" rows in TestFetchClientMetadataDocument_ErrDocumentUnavailable, and the "408 within window serves the expired entry" and "429 within window serves the expired entry" rows in TestCIMDStorageDecorator_TransientFetchFailure; all four fail without the change.

Versus main: main also answered invalid_client to a 408 or 429 on the re-fetch but kept the entry; the reviewed head (3ea338d) evicted it; now the entry answers.

FetchJSON read the response body before it checked the status, so a 4xx whose body was cut short or stalled was reported as io.ErrUnexpectedEOF or a timeout instead of an HTTPError.
A caller that treats a 3xx or 4xx as the host's definitive answer and a transport error as a transient failure would therefore take an explicit rejection for a blip.
A non-200 status is now reported first, through the custom error handler or as an HTTPError, and a body-read error only for a 200; the handler receives the bytes that arrived.
The only production caller is the CIMD document fetcher in pkg/oauthproto/cimd, and responses whose body arrives in full are reported as before.
The stalled-body row waits up to one second for the status so a loaded race run cannot time out before it arrives.

Signed-off-by: Aleksandr Filippov <71711753+alex-feel@users.noreply.github.com>
A CIMD client whose cache entry had expired was unresolvable while the re-fetch of its metadata document failed for a transient reason: no response from the host, a timeout, a 200 whose body was cut short, a 5xx, a 408, or a 429.
The decorator returned fosite.ErrNotFound, which fosite renders as 401 invalid_client at both the token and the authorization endpoint, so public clients discarded valid tokens and users signed in again although the server had resolved and persisted that client minutes earlier.
FetchClientMetadataDocument now wraps such failures in cimd.ErrDocumentUnavailable, while a 3xx or any other 4xx status, an unexpected content type, a malformed body, and every validation error stay definitive.
A 408 means the server did not receive a complete request in time (RFC 9110 section 15.5.9) and a 429 means the requester sent too many requests in a given time (RFC 6585 section 4); neither says anything about the document, and readModernEnvelope in pkg/vmcp/client/modern.go already classifies both as transient.
On a transient failure the decorator answers from the expired entry within the MaxStale window the fetch rate limiter's refusal path already uses, through the same check, without renewing the entry or rewriting the persisted row, so every later lookup re-fetches again.
MaxStale is the configured access token lifespan and nothing new is configurable.
A client the server never resolved, any other 4xx from the host, an invalid or policy-rejected document, an entry past the window or evicted from the LRU, and CIMD disabled keep failing closed as before.
Retry-After is not read: the fallback answers from the entry, and the fetch limiter paces re-fetches.
The warning CIMD document fetch failed is written once the stale decision is made, so it carries the fetch error and the outcome as fallback=expired_cache with age_past_expiry, or fallback=none.
The storage documentation describes the transient case beside the limiter case and says that these windows bound how long old client metadata is accepted, not the lifetime of tokens and grants already issued.

Signed-off-by: Aleksandr Filippov <71711753+alex-feel@users.noreply.github.com>
An expired cache entry survived a re-fetch that failed definitively, so after the host answered 404 or 410 or served a document this server rejects, a later limiter refusal or transient failure within MaxStale served the pre-failure client again.
fetchFailed now evicts the entry whenever it does not answer from it: on every definitive fetch failure, on a policy rejection, and on a transient failure with no servable entry, because an entry past the window can never answer again.
Every failed re-fetch passes through that one point, so the eviction closes the class at one site and also closes the gap the storage documentation stated for the limiter path.
The persisted row is not touched: on a later limiter refusal the token endpoint can still answer from it for up to 30 days after the last successful fetch, and callbackClient (the callback, the consent step, and device verification) reads it only to finish an authorization already in progress; the code and the storage documentation say so beside the eviction.
The subtest that pinned the old behavior is repurposed to assert the eviction, and a full-sequence test covers resolve, a definitive answer (a 4xx, a redirect, a bad body, or a document rejected for its auth method, grant types, or scopes), an empty cache, and a later 503, closed host, or limiter refusal that fails closed.
Concurrent callers sharing one singleflight call are shown to share one re-fetch, one warning, and one outcome, served or evicted.

Signed-off-by: Aleksandr Filippov <71711753+alex-feel@users.noreply.github.com>
@alex-feel

Copy link
Copy Markdown
Contributor Author

On 2026-10-23 the branch was rebuilt on main and the description rewritten: the cimd.stale_if_error setting is gone, the transient case shares main's MaxStale window with the limiter path, and a definitive failure evicts the cache entry. The first three points compare the diff with the 2026-10-19 revision of the description, which that rewrite replaced; the current description and the diff match each other. In your order:

  1. Setting, CRD field, wiring: the diff has no setting on purpose. Serve stale CIMD client on transient fetch failure (a8ad4a6) reuses MaxStale, the window the limiter's refusal path already has, through one shared check (servableStale); nothing is configurable, and the Summary and the first special note say so. CIMD client unresolved with 401 invalid_client when the metadata re-fetch fails transiently #6856 still described that revision's setting and test name; it now matches the PR.
  2. Description versus eviction, test names: the current description presents the eviction as a change from main in the Summary, the Before/After section, and the delta-versus-main note, and every test it lists exists at the head. The two names quoted are from the earlier revision: TestCIMDStorageDecorator_GetClient_RefetchFailureAfterPriorResolution became TestCIMDStorageDecorator_TransientFetchFailure, and TestNewCIMDStorageDecorator_NegativeStaleIfErrorReturnsError went with the setting it validated. TestCIMDStorageDecorator_DefinitiveFailureEndsStaleEligibility from Evict CIMD cache entry on definitive fetch failure (666612d) runs the whole sequence: resolve, a definitive answer, an empty cache, then a 503, a closed host, or a limiter refusal that fails closed. Two sentences in the description overstated fail-closed after an eviction; they now name the token endpoint's persisted-row fallback and callbackClient's bounded one.
  3. Window and opt-out: this is the window main already uses, documented with its bounds, not an independent one by design. The bound is the cache TTL plus MaxStale, the access token lifespan: 65 minutes on defaults and about 24 hours 5 minutes at the 24-hour maximum, the window the limiter path has had since d90a504, with no switch. It bounds only how long old client metadata resolves the client at a later lookup; issued tokens and grants keep their own expiry and revocation, and a definitive answer from the host ends it at once by evicting the entry. A switch for the transient case alone would leave the identical limiter window without one, so a setting, if wanted, belongs on the shared window (first question below).
  4. 408 and 429: fixed in Serve stale CIMD client on transient fetch failure (a8ad4a6); details in the thread. Each fix of this round is folded into the commit whose concern it completes, and the branch is rebased onto main, so the inline thread on fetch.go shows as outdated.

A correction to my previous summary and to my reply in the eviction thread: only /oauth/token can still answer from the un-evicted row, for up to 30 days after the last successful fetch. The other row readers (the callback, the consent step, and device verification) read it only to finish an authorization already in progress, within the bounds the storage docs give. The docs, the fetchFailed comment, and the description say so now, so the row question below concerns /oauth/token.

Two questions, @jhrozek and @ChrisJBurns:

  • Stale window: keep the shared MaxStale window with no switch, as now, or add one setting on the shared window in this PR (default the access token lifespan, 0s turning off stale serving from the cache on both paths), with its config, CRD, swagger, SDK, and operator plumbing? I prefer the window as it is: one bound for how long old metadata is accepted, on by default on the limiter path since d90a504, and the access token lifespan is the smallest value that covers a refresh. If it stays, I'd add a sentence to the access token lifespan field comments in the RunConfig and the CRD saying the value also bounds how long an expired CIMD entry is served.
  • Persisted row: add a Storage client delete for both backends in this PR, so /oauth/token stops answering from the row after a definitive failure, or leave the row as it is? I'd leave it out: the /oauth/token row fallback and its 30-day bound are the same as on main, and a client delete for both backends is a change of its own. If you want it, I can add it here or open an issue for it.

Ready for another look when you have time.

@alex-feel
alex-feel requested a review from ChrisJBurns October 27, 2026 20:26

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CIMD client unresolved with 401 invalid_client when the metadata re-fetch fails transiently

3 participants