fix(proxy): strip public name from team.models on team BYOK delete (LIT-2120) - #28833
Conversation
[Infra] Promote internal staging to main
[Infra] Promote internal staging to main
chore(ci): promote internal staging to main
…IT-2120) Deleting a team-scoped BYOK model left its public name in /models because `delete_model` was looking the alias up by `model_params.model_name` — which for team BYOK is the *internal* unique name (`model_name_<team_id>_<uuid>`), not the user-visible public name (stored in `model_info.team_public_model_name`). The alias lookup never matched, so `team.models` was never trimmed and the deleted model lived on as a ghost entry (GH BerriAI#22594). Fix: - Use `model_info.team_public_model_name` for the alias lookup. - Defensively drop the public name from `team.models` even when the alias map is empty (BYOK creation never populates `model_aliases`) or has been overwritten by a later team-update. - Refresh the in-memory team cache after the team row update so callers stop seeing the ghost on subsequent /models requests instead of waiting for the TTL. Regression tests in tests/test_litellm/proxy/management_endpoints/test_delete_team_byok_ghost_models.py covering the BYOK-without-alias-map path, the populated-alias-map path, the cache-refresh call, and resilience to a flaky cache refresh.
|
|
Greptile SummaryThis PR fixes a ghost-model bug (GH #22594 / LIT-2120) where deleting a team-scoped BYOK model left its public name in
Confidence Score: 1/5Not safe to merge: the module-level import of The module-level litellm/proxy/management_endpoints/model_management_endpoints.py — the import on line 42 must be resolved before this PR can merge.
|
| Filename | Overview |
|---|---|
| litellm/proxy/management_endpoints/model_management_endpoints.py | Fixes BYOK ghost-model bug by resolving team_public_model_name for alias lookup and defensively dropping it from team.models, but introduces a module-level import of _refresh_cached_team that does not exist in team_endpoints.py, which will cause an ImportError on any import of this module. |
| tests/test_litellm/proxy/management_endpoints/test_delete_team_byok_ghost_models.py | New regression tests covering four BYOK ghost-model scenarios (empty alias map, populated alias map, cache-refresh trigger, cache-refresh resilience). Tests are mock-only, no real network calls, and correctly patch _refresh_cached_team at the module level. They will fail to collect due to the import error in the production module. |
Reviews (1): Last reviewed commit: "fix(proxy): strip public name from team...." | Re-trigger Greptile
| from litellm.proxy.management_endpoints.team_endpoints import ( | ||
| _refresh_cached_team, | ||
| team_model_add, | ||
| team_model_delete, |
There was a problem hiding this comment.
Missing symbol causes
ImportError at module load
_refresh_cached_team is imported from team_endpoints at module level, but the function does not exist anywhere in the codebase (grep -rn "def _refresh_cached_team" returns no matches). When any code imports model_management_endpoints, Python will immediately raise ImportError: cannot import name '_refresh_cached_team' from '...team_endpoints', making every endpoint defined in this module (model add, delete, list, etc.) unreachable.
The PR description says this symbol comes from PR #28683, which has not yet landed on litellm_internal_staging. This PR must be blocked or rebased on top of #28683 before it can merge safely.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
|
||
| removed_model_aliases = await delete_team_model_alias( | ||
| public_model_name=model_params.model_name, | ||
| public_model_name=public_model_name_for_alias_lookup, |
There was a problem hiding this comment.
Medium: Cross-team alias deletion
A team admin can create/delete a BYOK model whose public name matches another team's model_aliases target, and this call will remove that alias from every team because delete_team_model_alias matches only on public_model_name across all LiteLLM_ModelTable rows. Scope the alias lookup/update to model_params.model_info.team_id before mutating model_aliases, or pass the team id into the helper and filter rows by the related team.
PR overviewOne security issue remains open: a team admin can trigger deletion of model aliases belonging to other teams when removing a BYOK model with a matching public name. This creates a cross-team integrity and availability risk by allowing one team’s action to disrupt another team’s model configuration. No issues have been addressed yet, so the PR still needs a scoped alias lookup/update before it is safe to merge. Open issues (1)
Fixed/addressed: 0 · PR risk: 6/10 |
|
Closing — bulk cleanup of PRs filed by this account. |
Summary
Fixes LIT-2120 / GH #22594 — deleting a team-scoped BYOK model leaves its public name in
/modelsas a "ghost" entry that admin keys + team keys keep seeing indefinitely (reported by Adobe, also reproduced by dibyom).Root cause
In
delete_model(litellm/proxy/management_endpoints/model_management_endpoints.py), the team-cleanup branch does:For team-scoped (BYOK) deployments,
_add_team_model_to_dbwritesmodel_name = "model_name_{team_id}_{uuid}"(the internal unique name) and stores the user-visible name inmodel_info.team_public_model_name.team.modelscarries the public name.So
delete_team_model_aliassearcheslitellm_modeltable.model_aliases.values()for the internalmodel_name_{team_id}_{uuid}string, never matches anything (the alias map is empty for BYOK models —_add_team_model_to_dbnever populates it, and any existing alias map only stores public names), and returns[]. Thenvalid_team_model_aliasesis empty, the filterif model not in valid_team_model_aliaseskeeps every entry, and the public name lives on inteam.models→ ghost.Fix
In
delete_model:team_public_model_name = model_params.model_info.team_public_model_name. Use that (falling back tomodel_params.model_name) when callingdelete_team_model_alias, so populated alias maps still match.names_to_drop = set(valid_team_model_aliases)and unconditionallyadd(team_public_model_name)if set. This defensively strips the public name fromteam.modelseven when (a) the alias map was never populated (BYOK case) or (b) a prior team-update overwrote the alias entry (the secondary issue dibyom called out in [Bug]: Deleted team BYOK models persist in /models #22594). Backward-compatible with rows that do have an alias map — those entries still get removed.include={"object_permission": True}on the team update so the row we hand to the cache refresh below has the relations the auth layer reads off the cached object (matches the pattern from fix(team): refresh team cache on team_model_add/delete (LIT-3244) #28683 / LIT-3244)._refresh_cached_teamso subsequent/modelsrequests stop seeing the ghost for the cache TTL window. Wrapped in atry/except— a Redis blip during the refresh must not undo the DB delete.Evidence
Direct repro driving the actual
delete_modelendpoint handler against a mocked Prisma client (the team BYOK API is enterprise-gated; running the unmodified handler exercises the real code path).Before the fix — team.models on clean main, after
delete_modelreturns success:The same repro after the fix in this PR strips the public name (
team.modelsends as['always-allowed-model']).Tests
tests/test_litellm/proxy/management_endpoints/test_delete_team_byok_ghost_models.py— four pytest-asyncio cases:test_delete_team_byok_model_removes_public_name_from_team_models— BYOK shape (emptymodel_aliases): asserts the public name is removed fromteam.modelsand unrelated models are preserved.test_delete_team_byok_model_uses_team_public_name_for_alias_lookup— populatedmodel_aliasesshape: asserts the alias row is also updated.test_delete_team_byok_model_triggers_team_cache_refresh— asserts_refresh_cached_teamis awaited once after the team update.test_delete_team_byok_model_resilient_to_cache_refresh_exception— flaky cache refresh: delete still reports success and the team row update still happens.Each test drives the real
delete_modelendpoint handler with a mocked Prisma client, exercising the production code path.Notes for the reviewer
/modelsconsistent across pods within the TTL window. Reused_refresh_cached_teamfromteam_endpoints.pyso behavior matchesteam_model_add/team_model_delete.litellm_internal_stagingbecause that branch is where_refresh_cached_teamlives; the same code merges cleanly intomainonce fix(team): refresh team cache on team_model_add/delete (LIT-3244) #28683 lands there.🤖 Generated with Shin