fix(proxy): make /team/member_delete's four cleanups atomic - #37959
Conversation
The team roster update, the user.teams update, the team membership delete, and the team-scoped verification token delete ran as four sequential writes with no transaction around them, so a failure between any two left the removal half applied. Thread a single prisma transaction through all four writes, following the same tx.<table> pattern /team/member_add and /team/member_update already use, so either all four land or none do.
|
|
|
@greptileai please review commit 9c5f874 |
Greptile SummaryThe PR makes
Confidence Score: 5/5The PR appears safe to merge because the affected cleanup writes and deleted-token audit insertion consistently use the same Prisma transaction. The transaction interface matches established management-endpoint patterns, failures roll back the grouped database mutations, and no new reachable correctness or security failure remains.
|
| Filename | Overview |
|---|---|
| litellm/proxy/management_endpoints/team_endpoints.py | Moves the member-removal writes into one transaction and emits the membership metric only after a successful commit; no changed-code defect was established. |
| litellm/proxy/management_endpoints/key_management_endpoints.py | Adds an optional transaction client for deleted verification-token audit inserts while preserving existing non-transactional callers. |
| tests/test_litellm/proxy/management_endpoints/test_team_endpoints.py | Wires transaction table mocks into existing deletion tests and adds coverage for failure between transactional writes. |
Reviews (1): Last reviewed commit: "fix(proxy): make /team/member_delete's f..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
TLDR
Problem this solves:
/team/member_deletewrites the team roster, the user's teams list, the team membership row, and the verification token as four separate statementsHow it solves it:
tx.<table>pattern/team/member_addand/team/member_updatealready useUser Flow
Before: an admin removing a member from a team can leave that member with full API access if any of the four database writes
/team/member_deleteperforms fails partway through{"team_id": "team-1", "user_id": "user-1"}After: the same transient failure now leaves the member fully in place instead of half removed
Relevant issues
Linear ticket
Resolves LIT-5541
Pre-Submission checklist
uv run pytest tests/test_litellm/<your_test_file>.py -v. Leave the suites (make test-unit-*,make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
Setup shared by both runs: a fresh Postgres 16 container, schema pushed with
prisma db push, proxy started withDISABLE_SCHEMA_UPDATE=trueand no other flags, master keysk-lit5541-test. A team is seeded with one extra member who has a per-member budget (so a membership row exists) and an active key:A failure between writes is forced with a Postgres trigger that raises on the third write:
Before (490c9f9)
{"error":{"message":"Internal server error","type":"internal_server_error"}}, HTTP 500lit5541-user-aandLiteLLM_UserTable.teamsis now{}, both committed, while the membership row and the verification token are both still present, since the delete on them is what the trigger blocked. The member reads as removed while their key and membership row still workAfter (9c5f874)
lit5541-team-after/lit5541-user-b:{"error":{"message":"Internal server error","type":"internal_server_error"}}, HTTP 500lit5541-team-after/lit5541-user-b: the roster still listslit5541-user-b,LiteLLM_UserTable.teamsstill contains the team, the membership row still exists, and the verification token still exists. Nothing committedmembers_with_rolesdown to just the team adminlit5541-user-b,LiteLLM_UserTable.teamsis{}, the membership row is gone, the verification token is gone, andLiteLLM_DeletedVerificationTokennow carries its audit record. All four writes landed togetherType
🐛 Bug Fix
Caveats (if any)
/team/member_deletecalls against each other is a separate change, tracked in LIT-5544Final Attestation