fix(scim): propagate team roster write failures on group and user writes - #37700
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 SummaryThe PR replaces fail-fast SCIM roster reconciliation with collect-then-report behavior, allowing every requested membership write to run before an aggregate error is returned.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported fail-fast reconciliation path now attempts every membership write and reports all failures while preventing inconsistent user-team data from being saved.
|
| Filename | Overview |
|---|---|
| litellm/proxy/management_endpoints/scim/scim_v2.py | The previous fail-fast issue is resolved: all roster writes are attempted, nested failures are preserved, and aggregate errors propagate through the SCIM endpoints before inconsistent user-team data is saved. |
| tests/test_litellm/proxy/management_endpoints/scim/test_scim_v2_endpoints.py | Tests exercise every failure position, aggregate status handling, endpoint propagation, continued reconciliation after failures, and suppression of inconsistent user-row updates. |
Reviews (2): Last reviewed commit: "fix(scim): propagate team roster write f..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
SCIM roster writes were swallowed, so a group or user push returned 200 while the team roster never received the membership. Surfacing the failure fixes that, but aborting on the first failed write leaves the rest of the batch unattempted on top of unrolled-back, which is worse than what it replaces. Every roster write in a reconciliation is now attempted, and the ones that did not land are reported together, naming each failed add and remove. Rollback would be the other option and it is not safe here: the compensating write can fail too, and it can strip a membership that pre-dated the push. SCIM reconciliation is idempotent, so a named partial failure is what the IdP's next push needs to close the gap. The reported status still follows the failures, so a unanimous 404 stays a 404 and only a batch whose failures disagree falls back to 500.
cd670d5 to
c3daf63
Compare
|
@greptileai re-review c3daf63 please. The P1 fail-fast reconciliation is replaced by collect-then-report, so every roster write runs and failures are named |
tin-berri
left a comment
There was a problem hiding this comment.
Auth/provisioning-critical (SCIM roster sync) so reviewed the diff directly. This is a genuine improvement over the abort-on-first-failure approach: every roster write in a push is now attempted before any failures are reported, so one bad write can't strand later good writes unattempted (the exact gap the previous fix left — tested directly via test_patch_team_membership_attempts_every_team_before_reporting and test_handle_group_membership_changes_attempts_every_member_and_names_failures, both confirming every write is called even after an earlier one fails). Status-code aggregation is correct and tested: a unanimous failure status is preserved (so a 404 an IdP shouldn't retry stays 404), while a mixed batch correctly falls back to 500. Idempotent no-ops (already-in-team, already-absent) are still correctly swallowed in _add_user_to_team/_remove_user_from_team rather than counted as failures — same logic, just relocated. Live proxy proof with content-addressed before/after file hashes for attribution. Small structural concern to flag, not a blocker: this touches the exact same functions (_handle_group_membership_changes, patch_team_membership, _handle_team_membership_changes) that #37688 (also open, not yet merged) modified with a different — now-superseded — abort-on-first-failure strategy; whichever of the two merges second will need a rebase to reconcile, worth a heads up to the author. CI green.
|
@tin-berri thanks for the direct review. Correcting my first take on the overlap you flagged: #37688 is a different ticket (LIT-5105, group members silently dropped in larger groups, plus member-id resolution and dedupe), not a competing fix for LIT-5381, so the two are complementary in intent but do collide on Re-verified locally on the current head: |
…p-and-user-writes-report-success-to-the-idp-when Staging's strict path aborted roster reconciliation on the first failed write, so both conflicted hunks resolve to this branch's attempt-everything-then-report version, keeping staging's stable-user_id upsert and member creation guard. Dropped staging's test_handle_group_membership_changes_propagates_add_failure, which pinned the first-failure abort and is subsumed by the parametrized attempts-every-member test. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
TLDR
Problem this solves:
How it solves it:
User Flow
Before: an admin pushing an Okta user whose group list still carries two stale groups gets a green push, and the user ends up claiming teams nobody put them in
"groups"namingmissing-team-a,engineering, andmissing-team-b, where the first and last no longer exist200with a SCIM user body, so the IdP records the push as successful and never retries it["missing-team-a", "engineering", "missing-team-b"]After: the same push fails, names both stale groups, and still lands the group that was good
404whose message names both writes that failed,missing-team-aandmissing-team-b, so the IdP records the push as failed and retries it["engineering"]only, with no membership that no roster backsRelevant issues
Linear ticket
Resolves LIT-5381
Pre-Submission checklist
uv run pytest tests/test_litellm/<your_test_file>.py -vScreenshots / Proof of Fix
Both legs are one uninterrupted run against a live proxy on a pinned port 4700, backed by its own Postgres, swapping only
scim_v2.pybetween them. Each leg prints the blob hash of the file it is serving next to the hash git holds for that commit, so the two halves are attributable:The push under test puts a group that exists between two that do not, so a roster write can only fail after another one has already landed. That is what separates "the whole push was rejected" from "the push kept going and told you exactly which writes it lost".
Before (cde1344, merge base)
groupslists only the one team the roster actually receivedso two of the three memberships exist only on the user record, and the IdP was told the whole push succeeded
After (c3daf63, PR tip)
so the good write still landed after an earlier one failed, and the two groups the error named are all the retry has to fix
Type
🐛 Bug Fix
Caveats
Review notes
Greptile's P1 was that failing part-way through a reconciliation leaves the roster half written. That is real, and rollback is the wrong answer to it. The compensating delete can fail on its own, and it can strip a membership that pre-dated the push, so a tidy-looking undo can destroy state the IdP never asked to change
The fix is to stop aborting instead. Every add and remove in a push is attempted, and the ones that failed are reported together by name, which is what makes the leftover partial state safe to leave. SCIM reconciliation is idempotent by design: the IdP re-pushes the whole desired member set, so the writes that already landed replay as no-ops and only the named failures have to succeed
Set against what this replaces, a silently lost write is unrecoverable because nobody knows it happened, while a named partial failure converges on the next push. The live run above shows both halves of that: the response names
missing-team-aandmissing-team-b, andscim-v3-head-teamstill received its member even though a write ahead of it failedFinal Attestation
Link to Devin session: https://app.devin.ai/sessions/043f92f2e3a041eeb796f63ad87c68c9
Requested by: @yassin-berriai