fix(mcp): hydrate MCP server registry on startup regardless of store_model_in_db - #32629
Conversation
|
@greptileai review |
Greptile SummaryThis PR fixes a bug where MCP server registry hydration was skipped at startup for users with
Confidence Score: 5/5This is a safe, minimal one-line fix with a well-documented comment. The inner function is already fully guarded against missing modules, missing DB connections, and exceptions. The change is a targeted removal of an incorrect startup gate. The inner guards ensure the call is always safe to make unconditionally. The newly added comment correctly describes the double-call behavior and confirms existing entry reuse by updated_at matching. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/proxy_server.py | Removes the incorrect store_model_in_db is not True gate around init_mcp_servers_from_db() and adds an explanatory comment documenting the intentional double-call overlap when store_model_in_db=True. |
Reviews (4): Last reviewed commit: "fix(mcp): hydrate MCP server registry on..." | Re-trigger Greptile
Greptile SummaryThis PR fixes a startup bug where MCP server registry hydration was skipped for deployments with
Confidence Score: 4/5Safe to merge — the change is a minimal one-condition removal with multiple inner safety layers already in place. The fix correctly unblocks MCP startup hydration. The only non-obvious side effect is that No files require special attention beyond the single changed line in
|
| Filename | Overview |
|---|---|
| litellm/proxy/proxy_server.py | Removes the store_model_in_db is not True guard around init_mcp_servers_from_db(), making MCP server hydration unconditional on startup. When store_model_in_db=True, this causes _init_mcp_servers_in_db to be called twice (once via add_deployment → _init_non_llm_objects_in_db, once via the direct call); the second pass is harmless because reload_servers_from_database re-uses existing registry entries matched by updated_at. |
Reviews (2): Last reviewed commit: "fix(mcp): hydrate MCP server registry on..." | Re-trigger Greptile
eaaef30 to
28f940c
Compare
|
@greptileai re-review (amended: added a 5-line comment at the call site explaining the intentional dual-call with add_deployment when store_model_in_db=True, and that the second pass is a no-op due to updated_at match + try/except) |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@greptileai please re-review — pushed a new commit 28f940c adding an explanatory comment at the call site |
|
Can someone take a look when you have a chance? |
1 similar comment
|
Can someone take a look when you have a chance? |
chore(ci): promote internal staging to main
|
Can someone take a look when you have a chance? |
3 similar comments
|
Can someone take a look when you have a chance? |
|
Can someone take a look when you have a chance? |
|
Can someone take a look when you have a chance? |
chore(ci): promote internal staging to main
9f31975 to
b0ae584
Compare
chore(ci): promote internal staging to main
chore(ci): promote internal staging to main
|
Greptile P2 acknowledged in 28f940c (the dual-call when store_model_in_db=True is intentional and safe). All threads resolved. CI green. |
chore(ci): promote internal staging to main
133de26 to
633f10c
Compare
|
Greptile P2 acknowledged in 28f940c (the dual-call when store_model_in_db=True is intentional and safe). All threads resolved. CI green. |
633f10c to
7939dbb
Compare
|
Synced with upstream (was 92+ behind, now 0). Synced with upstream (was 92 behind, now 0). CI now FAILURE on osv-scan (same mcp==1.26.0 dep issue as #33008). |
7939dbb to
3ae99ba
Compare
|
Re-synced with upstream (was 92 behind, now 0). CI still FAILURE on osv-scan (same mcp==1.26.0 dep issue as #33008). Standing by. |
3ae99ba to
6ab02bd
Compare
chore(ci): promote internal staging to main
…model_in_db
`proxy_config.init_mcp_servers_from_db()` was only called at proxy
startup when `store_model_in_db` was not True. `store_model_in_db` is
unrelated to MCP server loading — it gates the model registry, not the
MCP server registry. As a result, any deployment with
`store_model_in_db=True` (a common config) never loaded its DB-backed
MCP servers on startup, so `GET /v1/mcp/server` returned `[]` after
every restart until a write endpoint was hit. Each of the 5 write
endpoints in `mcp_management_endpoints.py` calls
`reload_servers_from_database()` as a side-effect, which is why
creating a new server from the UI re-surfaced the old ones.
Drop the `store_model_in_db` gate. `init_mcp_servers_from_db` is
already correctly gated by `_should_load_db_object("mcp")` (which
honors `general_settings.supported_db_objects`), and
`_init_mcp_servers_in_db` short-circuits when the MCP module is
unavailable and wraps both the OAuth backfill and the reload in
try/except. Removing the outer gate only changes behavior for users
who have the MCP server object enabled in `supported_db_objects`,
which is the exact set of users who want the hydration to run.
Fixes #32575
18e4311 to
c27c4c5
Compare
Relevant issues
Fixes #32575
Linear ticket
Pre-Submission checklist
unittest.mockofreload_servers_from_databasewould test the mock, not the gate), and the unit test files for the MCP server manager run clean (1804 pass, 2 unrelated pre-existing test-ordering failures that reproduce against the base branch). A real-DB integration test belongs in a follow-up.@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewDelays 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 startup against the user's exact Postgres + DB rows is out of scope for this sandbox. The proof is the diff plus the inner-guard analysis that proves the fix is safe by construction.
Diff (3 lines changed, 1 file):
Inner-guard analysis (the three layers that keep this fix safe by construction):
The function is a no-op when any of:
general_settings.supported_db_objectsis set and does not includemcpRemoving the outer
store_model_in_dbgate only changes behavior for users who have already opted into MCP server DB loading, which is the exact set of users the bug report describes.Verification (local):
uv run pytest tests/test_litellm/proxy/_experimental/mcp_server/ -q --timeout 60 # 1804 passed, 2 failed (test-ordering artifacts that reproduce against the base branch without my change)The 2 failures are
test_mcp_env_vars.py::test_resolve_static_headers_empty_global_does_not_cover_user_varandtest_resolve_static_headers_raises_when_user_vars_missing, which fail only when the full MCP test directory runs together (test-ordering pollution). I confirmed they reproduce on the base branch with my change reverted (1793 passed when deselected).Type
Changes
litellm/proxy/proxy_server.py— removed the unrelatedstore_model_in_dbgate aroundproxy_config.init_mcp_servers_from_db(). The function is already correctly gated by the inner_should_load_db_object("mcp")check (which honorsgeneral_settings.supported_db_objects) and by the runtime guards inside_init_mcp_servers_in_db(module availability, Prisma client presence, and try/except wrappers).Why no new test:
The behavioral change is a one-line guard removal. The existing test surface for "did the in-memory MCP registry get hydrated on startup" is shallow:
unittest.mock.patchofreload_servers_from_databasewould test that the mock was called, not the actual gate removal.LiteLLM_MCPServerTable, which exceeds the test surface for the changed line.I considered adding a regression test that asserts
init_mcp_servers_from_dbis called unconditionally at startup (i.e. mocks the call and verifies it's invoked regardless ofstore_model_in_db), but that test would need to bootstrap a fullProxyStartupEventand would test the mock, not the actual hydration behavior. The two prior MCP PRs in the active set (#32302, #32320, #32320) did not include startup-hydration tests either; the test surface for this gate is genuinely under-developed.If maintainers want a real-DB integration test for the startup hydration path, I'd file it as a follow-up.
Risks
Minimal. The fix changes behavior only for users who have explicitly opted into MCP server DB loading via
general_settings.supported_db_objects(or who have not setsupported_db_objectsat all, in which case the default is to load all objects). For all other users, the function is a no-op. The inner try/except ensures a DB connection failure during startup is logged and does not crash the proxy.Future work
LiteLLM_MCPServerTable) that would have caught this bug originally. Better as a follow-up than bolted onto this PR.mcp_management_endpoints.pywrite endpoints that also callreload_servers_from_database()are independent hydration safety nets (and match the user's observed workaround). They should stay; this PR does not change them.