fix(access groups): sync assigned_team_ids from the team write paths - #36825
Conversation
|
|
Greptile SummaryThe PR adds team-to-access-group reverse-membership reconciliation using transactional, set-based PostgreSQL updates and cache invalidation.
Confidence Score: 4/5The PR is not yet safe to merge because a synchronization failure during team creation can leave a committed, incorrectly mirrored team that the creation request cannot retry. Team creation persists before reconciliation, while duplicate-team validation prevents a retry with the same explicit id from reaching that reconciliation, leaving reverse membership and resulting resource authorization stale until a separate update occurs. Files Needing Attention: litellm/proxy/management_endpoints/team_endpoints.py
|
| Filename | Overview |
|---|---|
| litellm/proxy/management_helpers/access_group_team_sync.py | Adds atomic, idempotent reverse-membership reconciliation with same-team serialization and post-commit cache invalidation. |
| litellm/proxy/management_endpoints/team_endpoints.py | Wires reconciliation into all team write paths, but creation can commit before a synchronization failure and reject the corresponding retry. |
| litellm/proxy/management_endpoints/access_group_endpoints.py | Reuses the extracted access-group cache invalidation helper without changing endpoint behavior. |
| tests/proxy_admin_ui_tests/test_access_group_team_sync.py | Adds real-PostgreSQL coverage for SQL semantics, idempotency, deletion, null arrays, and same-team serialization. |
| tests/test_litellm/proxy/management_endpoints/test_team_endpoints.py | Adds wiring and authorization-consequence coverage while adapting existing mocks for the newly read field. |
Reviews (5): Last reviewed commit: "fix(access groups): sync assigned_team_i..." | Re-trigger Greptile
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 3 · PR risk: 0/10 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
a6d02b3 to
d442b5d
Compare
d442b5d to
d82571c
Compare
|
@greptileai please re-review at d82571c. The mirror is now two set-based statements in one transaction, replacing the read-modify-write loop both P1s flagged. |
d82571c to
2030899
Compare
|
@greptileai re-review at 2030899. Same-team race closed with a per-team advisory lock; the mirror now reads the committed team row. |
`assigned_team_ids` was only ever written from the access-group side, so a
team that dropped an access group on the Teams page kept showing up under the
group's Attached Teams forever.
That column is not display-only. `get_authorized_resources_from_key_access_groups`
reads it as an authorization input, so the stale entry also kept granting the
group's models, MCP servers and agents to keys on that team.
`/team/new`, `/team/update` (and `PATCH /team/{id}`, which delegates to it) and
team delete now mirror the change back onto every affected access group and
invalidate its cache entry.
2030899 to
9bb0ed2
Compare
|
@greptileai re-review at 9bb0ed2. Rebased onto current staging; same-team race closed with a per-team advisory lock over a committed-row read. |
|
Delete-then-mirror point is accurate but left as-is: both remedies are worse trades. Reasoning and the self-heal evidence are under Review notes. |
|
@greptileai re-review 9bb0ed2. The delete-path finding is addressed under Review notes with live evidence that the stale row self-heals on team id reuse. |
| include={"litellm_model_table": True}, | ||
| ) | ||
|
|
||
| await sync_team_access_group_membership(prisma_client=prisma_client, team_id=team_row.team_id) |
There was a problem hiding this comment.
Creation failure cannot reconcile
When access-group synchronization fails after new_team commits the team row, retrying the request with the same explicit team_id stops at duplicate-team validation before synchronization runs. The team remains created with stale reverse membership, causing its keys to miss the access-group resources until a separate team update reconciles it.
Knowledge Base Used:
A sync that failed after the insert committed left a team whose groups never learned about it, and the retry came back as a duplicate team id. Invalidate off the reconciled set so a retry after an unreachable cache still drops the stale grants. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…m_sync Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Staging moved team delete's cache eviction and reference sweep after the row delete, so the mirror runs last where it still sees the team gone. Immutable desired set and include mapping to stay inside the LIT002 ceiling. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
Verified all six legs through the Admin UI at 5399f0d, including the core clear-groups case. Merged current staging; both review findings addressed. |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…e xdist worker Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…s cannot break it Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…prisma_client isolation fix Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
6704a10
into
litellm_internal_staging
TLDR
Problem this solves:
How it solves it:
User Flow
Before: an admin who removes an access group from a team on the Teams page sees the team still attached to that group, and keys on the team keep the group's models
assigned_team_ids: ["<accounting id>"]while GET https://litellm-domain/team/info?team_id= returnsaccess_group_ids: []After: the removal takes effect on both pages, and the surviving grant goes with it
assigned_team_ids: []and GET https://litellm-domain/team/info?team_id= returnsaccess_group_ids: [], so the two agreeRelevant issues
Linear ticket
Resolves LIT-4916
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 port 4916 against a dedicated Postgres 16, same script every leg, master key
sk-1234. Both legs return 200 with real objects at every step, so the two runs differ only where the fix landsReproduce with, in order:
BEFORE, captured at 262ed53, current
litellm_internal_staging.grep -c sync_team_access_group_membership litellm/proxy/management_endpoints/team_endpoints.pyreturns 0 on that tree, so the leg really ran without the fixOn that same tree the concurrency script leaves the group with no attached teams at all, since nothing on the team side writes the mirror:
AFTER, captured at 9bb0ed2, this PR's head
CONCURRENCY, the review finding that changed this diff. Two teams attach the same access group at the same instant, so the group must end up listing both
At a6d02b3, this PR's first revision, which mirrored with a read-modify-write of the whole array. One team is lost on all three runs, while both teams' own rows say they are attached, so the group under-grants silently
At 9bb0ed2, this PR's head. Both teams survive on all three runs
CREATE PATH, re-run at 5399f0d after the create insert and its mirror moved into one transaction, and after merging current staging. A team created with an access group already set shows up under Attached Teams, clearing it on update removes it, and deleting the other team removes that one too
Type
🐛 Bug Fix
Caveats (if any)
assigned_key_idshas the same hole via/key/updateReview notes
Four things worth knowing before reading the diff.
First,
assigned_team_idsis not a display-only mirror.get_authorized_resources_from_key_access_groupsinlitellm/proxy/auth/auth_checks.pyreads it directly to decide whether an access group carried on a key is authorized for that key's team, and there is a test pinning that behavior with the team's ownaccess_group_idsleft empty on purpose. So the stale row was granting models, MCP servers and agents that the admin had already revoked, which makes this a correctness fix on an authorization input rather than a UI cleanup, and it is why the sync invalidates the group's cache entry instead of only writing the row.Second, the mirror takes no desired-state argument at all. It locks the team with
pg_advisory_xact_lockand reads the team row as committed, inside the same transaction as its writes. Reconciling against the committed row rather than a before/after delta is what lets a retry heal a half-applied sync, because the team row commits first and a retry's delta is empty. Holding the lock across the read is what makes two writers on the same team converge, rather than the slower one replaying its own stale snapshot over the newer one. Worth noting that both properties made the diff smaller. The signature lostprevious_access_group_idsand thenaccess_group_idstoo, so all three call sites now pass just a team id, and team deletion needs no special case: a team with no row reconciles to the empty set and detaches everywhere.On why an advisory lock rather than folding the team write into the mirror transaction or taking
SELECT ... FOR UPDATEon the team row, which is the more obvious reading of the review comment. The access-group endpoints already run a transaction that locks an access group and then a team. A mirror that locks the team and then access groups is the reverse order, and the two deadlock. I checked rather than reasoned about it, and Postgres aborts one side:The advisory lock is never taken by the access-group endpoints, so it cannot join their lock order to form a cycle, and the same probe reports no deadlock with it in place. It serializes per team, which is the property actually needed.
Third, the mirror is set-based statements inside one transaction, not a per-group read-modify-write loop. Greptile flagged the loop as a lost-update risk and it was right, so the concurrency leg above reproduces it three times out of three against the earlier revision and shows it fixed.
array_appendandarray_removemutate the array inside the statement, so a concurrent write for another team cannot clobber it, the pair commits together, and the statement count no longer scales with how many group ids a caller sends.COALESCEis load-bearing here because the column is nullable in Postgres, and without it both guards evaluate to NULL and the row is silently skipped.Fourth, the sync helper deliberately lives in
management_helpersrather than inaccess_group_endpoints. That module is a lazily registered feature router, and its routes reach the schema through the committed snapshot thatinject_lazy_stubsinjects for features it believes are unloaded. That check ismodule_path in sys.modules, so importing the module eagerly fromteam_endpointsmarks the feature loaded while its router was never included, and every one of its paths and schemas disappears. This is a live/openapi.jsondefect, not only a CI gate: a running proxy would serve a spec missing those routes until some request happened to warm the router, so Swagger and any generated client lose them. Countingaccess_grouppaths offapp.openapi()in one interpreter shows it directly, 10 with the helper extracted and 6 with the eager import restored. The first revision of this PR had the eager import, which is what turned the schema check red.On the one review point I did not act on. Team deletion commits before the mirror runs, and a retry of
/team/deleteis rejected at validation with a 404 before it reaches the mirror, so a mirror that failed after a successful delete leaves the team id behind on its groups. That is accurate, and I checked the 404 against a live proxy rather than reading for it.I left it alone because the remedies are worse and the residual is narrower than it looks. Detaching before the delete means a delete that fails partway leaves a live team stripped of its access groups, which is an outage for a team still in use, traded against a stale row for a team that no longer exists. Putting the delete in the mirror's transaction means replacing
delete_datawith raw SQL inside a function that also deletes that team's keys, models and memberships. Neither is a good trade for this.The residual also only bites if the team id is reused, since a stale id grants nothing while no team holds it, and team ids are caller-suppliable so reuse is possible. But reuse is exactly the moment
POST /team/newruns the reconcile, which clears it. Seeding a dangling id directly in the database and then recreating the team with that id:So the stale row is cleaned before anything can be issued against it. Worth saying plainly that this path is strictly better than before this PR either way: team deletion previously never cleaned the mirror at all, so what is left here is the old behavior surviving in a failure case rather than anything new.
The create path had a milder version of the same shape, and that one is now fixed rather than argued down.
POST /team/newused to commit the team row and then mirror it, so a mirror that failed left a team whose groups never learned about it while the retry was rejected as a duplicateteam_id. The insert and the reconcile now share one transaction, so the team row goes away with the failed mirror and the retry is a clean create.tests/proxy_admin_ui_tests/test_access_group_team_sync.pypins that against a real Postgres: raise inside the transaction after the reconcile and both the groups and the team table come back untouched.Cache invalidation is now driven by desired state rather than by which rows the current attempt changed. The mirror reads the groups the team should be on plus the groups that currently list it, before its writes, and the caller drops all of them. That is what makes a failed drop retryable: the retry's
UPDATEs match nothing because the database is already reconciled, and it still finds the same set to invalidate. Every drop is attempted even when one raises, so one unreachable cache cannot leave the rest of the groups serving a revoked grant, and the error is re-raised afterward so the caller sees the failure.On the merge with current staging, #36819 reworked team delete to evict the team cache and sweep dangling references after the row delete. The mirror now runs last on that path, after both, so it still reconciles against a team that is gone and nothing fallible sits between the delete and the eviction that comment asks to keep adjacent.
On completeness of the writer set,
team.access_group_idsis only ever written byPOST /team/new,POST /team/update, and team delete.PATCH /team/{team_id}is not a fourth writer: it builds anUpdateTeamRequestand callsupdate_team. Every one of those three now calls the same helper, so a reviewer can check completeness by grepping for writes to that column rather than trusting the list.Two notes on the tests. The three unrelated-looking edits in
test_team_endpoints.pyare a harness artifact: those tests build the team row asMagicMock(spec=LiteLLM_TeamTable), and pydantic v2 model fields are not class attributes, so a spec'd mock does not carry them and readingaccess_group_idsoff it raisesAttributeError. Each edit is a singleaccess_group_ids = Noneon the mock. Separately, the raw SQL needs a real database to mean anything, since a mocked prisma has to reimplement the array semantics in Python and then passes whatever the SQL says. I confirmed that by mutating the detach guard toAND TRUEand watching the unit test stay green.tests/proxy_admin_ui_tests/test_access_group_team_sync.pyruns the statements against a real Postgres and dropping eitherNOT (... = ANY(...))guard fails it 4/4.Be aware that this file is a local harness rather than a CI gate today.
tests/proxy_admin_ui_tests/is only referenced by.circleci/config.yml, and CircleCI is no longer connected to this repository, so no job on a pull request runs that directory. No PR workflow provides a Postgres service at all. Run it by hand with a database up:Giving that directory a home in GitHub Actions is worth doing, and it would light up five other files that have not run since the migration, but it is CI infrastructure work that does not belong in a bug fix.
The
proxy-endpointsred on this branch was a shared test-isolation bug, not this diff.tests/test_litellm/conftest.pyhas an autouse fixture that requestsmonkeypatch, so its undo runs after every other finalizer, and a test that monkeypatcheslitellm.proxy.proxy_server.prisma_clientwhile a module-levelpatch()of the same global is live gets that mock reinstalled permanently for the rest of the xdist worker, so later tests in the shard await a MagicMock. That fix has since landed on staging in #36872, so this branch carries staging's version of it and no isolation change of its ownFinal Attestation
Link to Devin session: https://app.devin.ai/sessions/2113173845894b95ab154df03e2d7637
Requested by: @yassin-berriai