fix(mcp): never write discovery results to the row, heal already-stamped rows, and retry failed discovery with backoff - #34990
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Greptile SummaryThis PR removes database persistence of MCP OAuth discovery results and adds startup healing plus periodic, backed-off discovery retries.
Confidence Score: 3/5This PR is not yet safe to merge because the startup heal can overwrite a concurrent issuer update, while some previously stamped rows remain outside the heal. The backfill reads candidates and later clears each issuer using only server_id, so a newer administrator pin can be lost; additionally, the historical issue remains for stamped rows whose last-writer marker changed before upgrade. Files Needing Attention: litellm/proxy/_experimental/mcp_server/oauth_issuer_stamp_backfill.py
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/mcp_server_manager.py | Removes discovery write-back and adds flow-aware unresolved detection with per-server retry backoff. |
| litellm/proxy/_experimental/mcp_server/oauth_issuer_stamp_backfill.py | Adds the startup issuer heal, but its unguarded per-row update can overwrite a concurrent administrator change. |
| litellm/proxy/proxy_server.py | Adds the reload-only entry point and schedules it for deployments that previously loaded MCP servers only at startup. |
| litellm/proxy/management_endpoints/mcp_management_endpoints.py | Removes the obsolete discovery-persistence flag from temporary server construction. |
| ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/OAuthFormFields.tsx | Updates issuer guidance to reflect that the field now represents administrator intent only. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_oauth_issuer_stamp_backfill.py | Covers heal selection and idempotency but does not cover a concurrent administrator update between selection and clearing. |
Reviews (10): Last reviewed commit: "fix(mcp): never write discovery results ..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
70479f6 to
f77aa74
Compare
|
@greptileai rereview |
|
bugbot run |
|
Addressed in d351065. Both P1s and the Bugbot high share a root cause: the observation was treated as trusted by construction, so neither the pin it was validated against nor the corroboration rule that governs discovery applied to it. Legacy issuers remain incorrectly anchored. Real, and the reason the confidence score was right. A row stamped by an earlier version does still read as admin-pinned, and no signal on that row can separate it from a deliberate pin; Proved on a live proxy against one database: created the server on Stale witness defeats issuer anchoring. This one no longer applies to the code under review. It describes Observation fallback bypasses corroboration gate. Correct and mine. The fallback now runs through Each of the three is mutation-tested: forcing |
|
@greptileai rereview |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit d351065. Configure here.
|
Fixed in f5a617c, as a class fix rather than a guard on that one filter. The finding is right, and the truthy filter was a symptom of the observation being merged into the blob key by key with a change rule per key. That is the wrong shape for what the record holds, and it was wrong twice. An endpoint the upstream stops advertising is simply absent from the next document, which a truthy filter cannot tell apart from "not mentioned in this update", so the withdrawn value survived as last known good. That is the case you found. The second one follows from the same merge: because each key moved independently, the surviving keys could come from different documents, so an An observation is one snapshot of one document, so it is now replaced wholesale or left alone. One case the snapshot semantics opened up, closed in the same commit: nothing is recorded from a document the corroboration gate rejected. A rejected document means "this authorization server is not the one the pinned authorize endpoint vouches for", not "the endpoint is gone", so snapshotting it would let a compromised resource evict a server's last known good and take away the fallback it would otherwise keep. Both properties are mutation-tested. Restoring the truthy filter and removing the uncorroborated guard each kill their own tests and nothing else. |
|
@greptileai rereview |
|
bugbot run |
f5a617c to
e853d40
Compare
|
Both findings were symptoms of one design gap, addressed in e853d40 at the type level rather than at either call site The OBO writer compared against the build's resolved The discovery result now states which documents it actually contains: Each property is mutation-tested: ignoring either flag, or dropping OBO from the writer's gate, each kills its own regression test The branch is also squashed to a single commit; the two earlier P1s on |
|
@greptileai rereview |
|
bugbot run |
e853d40 to
ef59621
Compare
|
@greptileai rereview |
fe3d3dc to
6b00d6d
Compare
|
Both addressed in 6b00d6d The P1 is correct, and precise about the mode: with The M2M finding is right about the symptom and the fix deliberately differs from the suggestion. Inferring |
|
@greptileai rereview |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 6b00d6d. Configure here.
|
@greptile what is stopping you from giving 5/5 |
|
Both threads that were non-outdated in my last review have been addressed in the current code:
Looking at the current diff, I don't see any remaining issues. Both concerns are resolved, the persistence layer is gone (eliminating the root cause), the fast-path exemption for unresolved endpoints is in place, and the retry cadence exists in both |
6b00d6d to
b4d6d45
Compare
|
All four addressed in b4d6d45 Already-stamped rows are now healed by a one-time startup backfill. I checked for a sound signal first and there is none: The periodic job is narrowed. It now calls a reload-only entry point instead of the startup composite, so Backoff added: per-server, doubling from one reload cadence to a fifteen-minute cap, cleared on success. That bounds both the discovery-chain amplification and the warning volume with one mechanism On the completeness rule, Five mutations, one per property (drop the heal's endpoint narrowing, ignore the writer, remove backoff, drop each completeness arm), each fail exactly one test |
|
@greptileai rereview |
|
bugbot run |
b4d6d45 to
872d961
Compare
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 872d961. Configure here.
|
@greptileai is this a 5/5? |
|
CI note: the |
|
Yes, this is a 5/5. Here's the breakdown: Root cause fix — Startup heal — Reload retry — Tests — the test names map directly to the invariants: UI + warning — tooltip no longer promises auto-population; One very minor observation: Security tradeoff (loss of TOFU issuer anchoring for servers with neither a declared authorize endpoint nor a declared issuer) is clearly documented and the scope of exposure is accurately bounded. |
…se already stamped, and retry failed discovery with backoff An interactive oauth2 MCP server created with explicit endpoint URLs and no issuer served 400 "authorization url is not configured" from /authorize about a minute after creation, with the admin's endpoints intact in the row the whole time (#34985). Discovery wrote its trust-on-first-use issuer into the same column an admin writes, so the next registry build read the gateway's own output back as an admin pin, anchored the server to RFC 8414 section 3.3, and discarded the stored endpoint columns; one transient metadata fetch failure then had nothing to serve, and the reload fast path pinned the broken entry until an unrelated config write The core of the fix is a deletion. The gateway no longer writes discovery results anywhere: the OAuth columns and credentials.scopes carry admin intent alone, and everything discovery learns lives on the in-memory registry entry, as the existing carry-forward already assumes. With no gateway write there is no value whose provenance a later build can misread, so the accidental anchoring cannot be expressed Deleting the write cannot fix a row a released version already stamped, which still reads as pinned, so a one-time startup heal clears those stamps. The signal is necessarily a heuristic: updated_by records only the most recent writer and no audit trail says which field it touched. A row is therefore healed only on the full signature of the defect, which is discovery as the last writer plus an issuer plus at least one configured endpoint column that anchoring is actively discarding; rows with an issuer but no configured endpoints are left alone, since for them both paths resolve from the same upstream document. Every heal logs the cleared value so an admin who pinned deliberately can re-pin, and the heal records its own actor, which makes it idempotent The reload fast path exempts servers missing an endpoint their flow needs, so failed discovery retries on the normal reload cadence rather than waiting for a config write. Flow requirements are read through effective_oauth2_flow, the column-first shape-fallback judge every flow decision uses, so a legacy null-flow M2M row is classified exactly as the request path classifies it instead of re-discovering forever; a dcr_bridge server with no configured client needs its registration endpoint for the relay arm, and an entra_obo server needs a scope, both of which discovery can supply. Retries back off per server, doubling from one reload cadence to a fifteen-minute cap, so a permanently unresolvable server cannot re-run the RFC 9728 to 8414 chain and re-log its warning every cycle forever Deployments with store_model_in_db unset or false loaded MCP servers exactly once at startup, leaving that retry with no driver, so they now refresh the registry on the same reload interval. That job deliberately calls a reload-only entry point rather than the startup composite, keeping the one-time oauth2_flow backfill and issuer heal out of a recurring path Losing the persisted trust-on-first-use issuer also means the issuer column no longer changes underneath the OAuth token identity, so user tokens are purged only when an admin actually edits the server Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
872d961 to
7041f57
Compare
TLDR
Problem this solves:
issuercolumnHow it solves it:
Relevant issues
authorization_serversat an attacker (RFC 9700 mix-up). That protection goes away with the defect it caused. The exposure is narrow, since the corroboration gate already blocks the same swap for any server with a declaredauthorization_url(the reported configuration included), leaving only servers with neither a declared authorize endpoint nor a declared issuer, which return to the behavior that shipped before feat(mcp): issuer-anchored OAuth discovery (RFC 8414 §3.3) to close the authorization-server mix-up #33450 on 2026-07-16. Restoring it safely needs a distinct weaker trust mode (reject a changed issuer, but keep declared endpoints and do not fail closed on a fetch failure) rather than a stamp in a different column, since wiring trust-on-first-use into the existing anchoring is what produced this bug; that belongs in its own change. Typing the Issuer remains the supported way to get section 3.3 anchoringLinear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Live proxy on an isolated Postgres, upstream stub on
localhost:9985serving the real RFC 9728 -> RFC 8414 chain with a kill switch that makes metadata fetches 503The reporter's scenario
Create with explicit endpoint URLs and no issuer, drive
/authorizeso discovery runs, then read the row back:The issuer column stays NULL and
updated_byis still the creator: the gateway wrote nothing. Kill the upstream metadata, cold-restart the proxy, authorize again:On the shipped code this exact sequence stamps the issuer within a minute and the cold build serves
400 "MCP server authorization url is not configured"A row a shipped release already stamped, healed at boot
Forge the exact pre-fix state on a row with three configured endpoint URLs (
issuerstamped,updated_bythe discovery actor), take the upstream metadata endpoint down, and restart:A second restart heals 0 servers, since the row's
updated_byis no longer the discovery actorA row that stays pinned (deliberate, or outside the heal's signature)
Create a server with the issuer pinned (what an already stamped row looks like after upgrade) while the upstream metadata endpoint is dead:
with the new warning in the proxy log naming the row and the remedy:
Then revive the upstream and touch nothing:
On the shipped code that 400 persists until some unrelated config write
Type
🐛 Bug Fix
Changes
mcp_server_manager.pydeletes_persist_discovered_oauth_endpointsand_persist_discovered_obo_token_urlalong with their call sites and thepersist_discovered_endpointsflag; discovery results now exist only on the in-memory registry entry._flow_endpoints_missingis the completeness rule behind the new reload fast-path exemption: a registry entry missing an endpoint its flow needs (interactive needs authorize and token, client_credentials and OBO need token, and an OBO server with a configured exchange endpoint is never considered missing) is rebuilt on the next reload instead of being reused verbatim, which retries discovery on the normal cadence._endpoints_yield_to_issuerwarns when a pinned issuer is discarding stored endpoint columns, naming the server and the remedy_oauth_endpoints_unresolvedclassifies a row's flow througheffective_oauth2_flow, the column-first shape-fallback judge every flow decision uses, so a legacy null-flow M2M row is classified exactly as the request path classifies it rather than re-discovering on every reload. It also treats two flow-specific requirements as completeness: adcr_bridgeserver with no configured client needs its registration endpoint or it silently degrades to the short-circuit arm, and anentra_oboserver needs a scope or its token exchange fails closed; both are values discovery can supply. Scopes are otherwise not part of completeness, since they are a request hint the authorization server bounds at consent (RFC 6749 section 3.3). Retries back off per server, doubling from one reload cadence to a fifteen-minute cap and clearing on success, so a permanently unresolvable server cannot re-run the discovery chain and re-log its warning every cycleoauth_issuer_stamp_backfill.pyis the one-time startup heal for rows a released version stamped. The signal is necessarily a heuristic (updated_byrecords only the most recent writer, and no audit trail says which field it touched), so a row is healed only on the full signature of the defect: discovery as last writer, an issuer set, and at least one configured endpoint column that anchoring is discarding. Rows with an issuer but no configured endpoints are left alone, since for them the anchored and resource-rooted paths resolve from the same upstream document. The residual false positive is an admin who pinned an issuer, also filled endpoint columns the pin makes inert, and whose row was last touched by discovery backfilling scopes; every heal therefore logs the cleared value so that admin can re-pin, and the heal records its own actor, which makes it idempotentproxy_server.pyregisters the registry refresh on the reload interval whenstore_model_in_dbis not true (previously that mode loaded MCP servers exactly once at startup, leaving the retry with no driver), re-proved live in that mode with a ~30s self-heal and no admin action. That job calls a reload-only entry point rather than the startup composite, so the one-timeoauth2_flowbackfill and the issuer heal stay out of a recurring pathdb.py,types/mcp.py, andtypes/mcp_server/mcp_server_manager.pyare unchanged from the base. The Issuer field tooltip no longer promises auto-population, since the field is now only ever what an admin typedQA runbook
Final Attestation