fix(proxy): invalidate token-exchange cache on 429

[andybons]

Summary

• preserve the existing token-exchange cache and TTL behavior
• evict an injected cached credential when the destination returns HTTP 429, in addition to the existing 401/403 invalidation
• do not retry the failed request; only the next request resolves again
• retain the existing 10-second per-key invalidation cooldown to bound repeated rate-limit responses

Problem

Some upstream APIs use HTTP 429 to report that a selected backing credential has exhausted its allowance while its token remains otherwise valid. For example, Codex maps usage_limit_reached from a 429 response in api_bridge.rs. Without invalidation, Gatekeeper continues serving the cached credential until expiry even when the broker would select a different backing credential on the next exchange.

Regression coverage

TestIntercept_TooManyRequestsInvalidatesCachedCredential proves that:

• the first request reaches the destination once and returns 429 (no automatic retry)
• the 429 evicts the cached credential
• the next request resolves again and sends the newly selected token
• existing 401/403 invalidation coverage remains unchanged

Before 429 was added to the invalidation statuses, this regression failed because the second request reused account A. With the fix, the second request resolves and sends account B.

Verification

• go test -race ./proxy -run 'TestIntercept_(TooManyRequestsInvalidatesCachedCredential|UpstreamAuthFailureInvalidatesCredential)' -count=1
• go test -race ./...
• go vet ./...
• go build ./...

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 1m 54s —— View job

---

Code review complete

• Gather context (diff, changed files, AGENTS.md)
• Understand the request (code review via /code-review skill)
• Review security/race/logic/error-handling concerns
• Post findings

Summary

This is a well-scoped fix. I traced the cache logic end-to-end (credentialsource/tokenexchange.go, gatekeeper_tokenexchange.go, config_credential.go):

• cache_ttl nil → defaults to maxTokenTTL (1m), preserving prior behavior exactly.
• cache_ttl: "0" → cacheTTL = 0, which forces ttl = 0 in Resolve (since any positive expires_in is > 0), and the ttl > 0 guard at the cache-write site correctly skips populating the map — so caching is disabled while the singleflight.Group still coalesces concurrent callers for the same (subject, actor) key. This is exactly what's needed to fix the account-switch bug described in the incident writeup.
• Negative cache_ttl is rejected at config-load time (gatekeeper_tokenexchange.go:206-208) and defensively clamped to 0 again in NewTokenExchangeSource if it somehow got through — reasonable belt-and-suspenders.
• time.ParseDuration("0") is valid Go (no unit required for zero), so cache_ttl: "0" in YAML parses fine.
• The regression test (TestResolveTokenExchange_CacheTTLZeroReExchangesAfterAccountSwitch) genuinely exercises the incident: same subject/actor, two STS responses, asserts 2 exchanges instead of 1 reused cache hit. Good encoding of the bug per AGENTS.md's testing guidance.
• Docs (docs/content/guides/06-token-exchange.md, 17-credential-lifecycle.md, reference/03-credential-sources.md) and CHANGELOG are consistent with the implementation.
• No new locking/race issues — cacheTTL is set once at construction and read without synchronization thereafter, which is safe since it's immutable after NewTokenExchangeSource.
• Didn't find any credential-leak, TOCTOU, or injection issues in the diff.

One nit (posted inline): the doc comment above TokenExchangeSource.Resolve in credentialsource/tokenexchange.go (lines 214-221) has a duplicated phrase — "forwarded to the STS as the" appears twice back-to-back — left over from the edit that introduced the cache_ttl wording. Cosmetic only, no behavior impact.

Note: I was unable to run go build ./..., go vet ./..., or go test -race ./... in this session — those commands required interactive approval that wasn't available. Everything above is from static reading of the diff; the PR description already reports these commands passing locally, and I have no reason to doubt that given the logic traced out fine by hand. If you'd like me to actually execute the test suite, --allowedTools will need to permit Bash(go build), Bash(go vet), and Bash(go test).

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 1m 29s —— View job

---

Code review: fix(credentialsource): allow disabling token exchange cache

Todo list

• Gather context (PR diff, changed files, CLAUDE.md/AGENTS.md instructions)
• Check whether the previously-flagged duplicated doc comment was fixed
• Review credentialsource/tokenexchange.go cache_ttl logic (races, TOCTOU, edge cases)
• Review config.go / config_credential.go wiring
• Review gatekeeper_tokenexchange.go and its test
• Post inline comments for any issues found
• Post final summary

Summary

The previously flagged duplicated doc-comment phrase ("forwarded to the STS as the ... forwarded to the STS as the") was already fixed in 893c2f2. Current text at credentialsource/tokenexchange.go:214-221 is clean.

No new correctness, security, or concurrency issues found. Specifics I checked:

• cacheTTL clamping (credentialsource/tokenexchange.go:88-96): negative values clamp to 0, values above maxTokenTTL clamp down — consistent with the doc comment on TokenExchangeConfig.CacheTTL.
• Resolve TTL-zero path (tokenexchange.go:255-276): when cacheTTL is 0, computed ttl is always 0 (ttl > s.cacheTTL is true for any positive expires_in, so ttl = s.cacheTTL = 0), and the new ttl > 0 guard at line 271 correctly skips the cache write while still returning the token to the caller.
• Singleflight coalescing is preserved with caching disabled: the sf.Do dedup is keyed on (subjectToken, actorToken) independent of cacheTTL, so concurrent callers for the same key still share one STS round trip even when nothing gets cached afterward — matches the doc comment and the regression test's exchanges == 2 assertion (one per distinct call, not per concurrent waiter).
• cacheGen invalidation race: unaffected by this change — the s.cacheGen == gen && ttl > 0 check at line 271 correctly combines both conditions, so an in-flight Invalidate still blocks a stale write regardless of TTL.
• Config validation (config_credential.go, gatekeeper_tokenexchange.go:196-210): cache_ttl was added to the extraneous-field rejection list for all seven non-token-exchange source types (verified each case in config_credential.go), and resolveTokenExchange rejects negative/unparseable durations at config-load time before NewTokenExchangeSource's runtime clamp would silently absorb them — good defense in depth, and the rejection is exercised by TestResolveTokenExchange_CacheTTLRejectsInvalidValues.
• Regression test (gatekeeper_tokenexchange_test.go:153-215) reproduces the incident precisely: same subject/actor across two resolves, STS returns a different token the second time, and the test asserts both the new token is returned and that two STS calls occurred (proving no accidental caching). This is a good test of the actual defect, not just the new field's plumbing.
• Docs (docs/content/guides/06-token-exchange.md, 17-credential-lifecycle.md, reference/03-credential-sources.md, CHANGELOG.md) are consistent with the code: default 1m, hard ceiling 1m, "0" disables caching, singleflight coalescing still applies.

One thing worth being aware of (not a bug, just calling it out): a non-positive expires_in from the STS still falls back to the configured cache_ttl as the TTL (ttl <= 0 || ttl > s.cacheTTL → ttl = s.cacheTTL) rather than treating it as "don't cache." This is pre-existing behavior (previously fell back to maxTokenTTL), unchanged in spirit by this PR, and is documented in both the reference doc and the guide.

Not run: go build ./..., go test -race ./..., and go vet ./... — the sandbox in this environment requires approval for Bash execution that wasn't available non-interactively. The PR description states these were run locally with a mutation check on the cache_ttl wiring; I did not re-verify by execution, only by static review of the diff.

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 1m 5s —— View job

---

Re-review complete (after latest push)

• Gather context (diff since last review, changed files, AGENTS.md)
• Understand the request (re-review via /code-review skill after new commits)
• Review security/race/logic/error-handling concerns in the new commits
• Post findings

Summary

Two commits landed since my last review:

• 893c2f2 — fixes the duplicated-phrase doc comment I flagged inline on credentialsource/tokenexchange.go. Confirmed: the "forwarded to the STS as the" repetition is gone and the paragraph reads correctly now.
• 9af069c — purely a test-comment/string generalization (gatekeeper_tokenexchange_test.go): renames the regression test's narrative from box-specific wording ("Boxes can change...", timestamped incident details, acct_old/acct_new_cv token values, same-box-subject/same-box-actor) to generic terms ("a credential broker can change...", account_a/account_b, same-subject/same-actor). No logic or assertion changes — exchanges.Load() != 2 check and control flow are untouched.

No new security, race, or logic issues in this diff. The cache_ttl implementation itself (subject of my original review) is unchanged and still correct.

One nit: the generalization in 9af069c didn't reach two other spots that use the same "long-running box" phrasing introduced earlier in this PR:

• CHANGELOG.md:11 — "...so a long-running box that switches its subscription account..."
• docs/content/guides/06-token-exchange.md:188 — "...such as a subscription account selected for a long-running box."

Since the point of the latest commit appears to be scrubbing that internal terminology from the regression test, these two remaining instances are likely worth the same treatment for consistency (e.g. "a long-running client"). Not a functional issue — just a documentation/wording consistency gap.

Fix this →

Note: go build, go vet, and go test -race were not available to run in this session (require interactive approval not present here). The PR description reports these passing locally; nothing in the diff suggests otherwise on static reading.

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 2m 2s —— View job

---

Reviewing PR #69

• Gather context (diff, changed files, AGENTS.md)
• Understand the request (re-review via /code-review skill after latest push)
• Review security/race/logic/error-handling concerns (in progress)
• Post findings

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 1m 51s —— View job

---

Re-review complete (after latest push)

• Gather context (diff since last review, changed files, AGENTS.md)
• Understand the request (re-review via /code-review skill after new commits)
• Review new changes in proxy/proxy.go, proxy/relay.go, proxy/intercept_test.go
• Post findings

Summary

The approach behind this PR changed completely since the last review. Commits ffb5648…e128702 revert the entire cache_ttl-based fix (the config knob, gatekeeper_tokenexchange.go/config_credential.go wiring, and the earlier regression test), and f3561c1 replaces it with a different fix for the same underlying incident: widening the existing 401/403 response-driven invalidation path to also cover 429 Too Many Requests.

Net effect vs. origin/main (matches the file list/line counts in the PR description, but the content is now different from what the description says):

• proxy/proxy.go — invalidateCredentialsOnAuthFailure renamed to invalidateCredentialsOnCredentialFailure, now also triggers on http.StatusTooManyRequests in addition to 401/403. All three call sites (proxy.go:2462, proxy.go:2925, relay.go:189) updated consistently — no stale references to the old name remain.
• credentialsource/tokenexchange.go — comment-only changes generalizing the rationale for defaultInvalidateCooldown/Invalidate to cover the 429 case; no logic changes (the existing per-key 10s cooldown and cache-eviction code already handle any caller of Invalidate, including this new trigger).
• proxy/intercept_test.go — new TestIntercept_TooManyRequestsInvalidatesCachedCredential, mirroring the existing TestIntercept_UpstreamAuthFailureInvalidatesCredential in style. It exercises the real incident: a resolver that returns different backing-account tokens on each call, a backend that 429s once then 200s, and asserts (a) the failed request is not retried, (b) the cache is evicted so the next request re-resolves and gets the new account's token, and (c) the wire-level Authorization headers reflect both tokens in order.
• Docs (CHANGELOG.md, the token-exchange guide, credential-lifecycle guide, credential-sources reference, token-exchange-endpoint.md) are all updated consistently to describe 401/403/429 instead of just 401/403, and the CHANGELOG entry accurately describes the new mechanism (not the reverted cache_ttl one).

I re-verified the reasoning by hand:

• invalidateCredentialsOnCredentialFailure nil-checks cred.Invalidate before calling it, so static/env/no-cache sources are unaffected — consistent with the docs table.
• Widening the trigger to 429 doesn't change the evict-only contract (failed request is never retried) or the per-key cooldown, so the existing safeguards against amplification under a client that loops on a persistently-failing request still apply.
• No other call site in the proxy package (e.g. mcp.go, postgres.go) needed updating — those don't go through this invalidation path today, and this PR doesn't change that scope.

No security, race, or logic issues found in the new diff.

Not run: go build ./..., go vet ./..., go test -race ./... — these require interactive Bash approval that isn't available in this session. I traced the rename/call-sites and struct field usage by hand (SetCredentialResolver, credentialHeader.Invalidate in proxy/proxy.go:549-561) and confirmed the new test compiles against the existing helpers, but did not execute the suite. The PR description's own "Tests" section is now stale — it describes the reverted cache_ttl approach's mutation check, not this one.

[andybons]

Thanks for the re-review. Confirmed the current PR title and description were updated after the pivot: they describe 429-triggered eviction, no retry, the existing 10-second cooldown, and the red/green account A → account B regression. There are no remaining cache_ttl claims in the current PR body; the stale description noted by the review was an earlier event snapshot. Full local checks also passed: go test -race ./..., go vet ./..., and go build ./....

[claude]

Claude finished @andybons's task in 3m 39s —— View job

---

Reviewing PR #69

• Gather context (diff, changed files, AGENTS.md)
• Kick off /code-review skill on the current diff
• Compile and verify findings from review agents
• Post findings