Repository navigation
fix(proxy): stop fail-closed 503 for team users without a membership row - #43751
devin-ai-integration[bot] wants to merge 4 commits into
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…d under fail-closed budgets Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
|
|
| counter_key=f"spend:team_member:{valid_token.user_id}:{team_object.team_id}", | ||
| fallback_spend=team_member_spend, | ||
| max_budget=team_member_budget, | ||
| fallback_authoritative=loaded_membership is None, |
There was a problem hiding this comment.
Unverified spend bypasses member budget
With fail-closed enforcement enabled, a user without a membership row can accumulate spend in the Redis team-member counter. If Redis becomes unavailable, the database has no membership spend to recover, but this flag treats the zero fallback as verified. Requests can then continue past the member budget instead of receiving a 503. That violates the repository directive to prevent security incidents in the authentication layer.
How this was verified: The spend writer does not persist spend for a user absent from both the roster and membership table, while this flag suppresses the unverifiable-spend rejection.
Rule Used: What: Fail any PR which may contains a security incident on litellm's authentication layer Why: Do not cause security incidents Bad: ```python # Check cache first cache_key = ( f"oidc_userinfo_{token[:20]}" # Use fi... (source)
| paths: | ||
| - litellm/proxy/auth/auth_checks.py | ||
| - tests/e2e/conftest.py | ||
| - tests/e2e/pytest.ini | ||
| - tests/e2e/gateway/fail_closed_team_member_budget_ci_config.yml | ||
| - tests/e2e/quota_management/budgets/test_team_member_budget_e2e.py | ||
| - .github/workflows/test-e2e-fail-closed-team-member-budget.yml |
There was a problem hiding this comment.
Helper changes skip regression test
The new test uses budget_client.py to create a team with a member budget, but that file is missing from this workflow’s path filter. A PR changing only the helper will not run this regression test, so a broken team-creation request could go unnoticed by this lane.
| paths: | |
| - litellm/proxy/auth/auth_checks.py | |
| - tests/e2e/conftest.py | |
| - tests/e2e/pytest.ini | |
| - tests/e2e/gateway/fail_closed_team_member_budget_ci_config.yml | |
| - tests/e2e/quota_management/budgets/test_team_member_budget_e2e.py | |
| - .github/workflows/test-e2e-fail-closed-team-member-budget.yml | |
| paths: | |
| - litellm/proxy/auth/auth_checks.py | |
| - tests/e2e/conftest.py | |
| - tests/e2e/pytest.ini | |
| - tests/e2e/gateway/fail_closed_team_member_budget_ci_config.yml | |
| - tests/e2e/quota_management/budgets/budget_client.py | |
| - tests/e2e/quota_management/budgets/test_team_member_budget_e2e.py | |
| - .github/workflows/test-e2e-fail-closed-team-member-budget.yml |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Merging this PR will not alter performance
Comparing |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
TLDR
Problem this solves:
fail_closed_budget_enforcement: true, team users without a membership row get 503 foreverHow it solves it:
User Flow
Before: a user on a team with a default member budget, who was never added as a member, cannot make any call
fail_closed_budget_enforcement: trueand Redisteam_member_budgetvia POST http://localhost:4000/team/new and a key for the user on that teamAfter: the same user gets a normal completion
fail_closed_budget_enforcement: trueand Redisteam_member_budgetvia POST http://localhost:4000/team/new and a key for the user on that teambudget_exceededLinear ticket
Resolves LIT-8984
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/unit/<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)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 with real Postgres and Redis, config:
Setup for each run: POST /team/new with
team_member_budget: 100, POST /user/new withmax_budget: 100, POST /key/generate with thatteam_idanduser_id, no /team/member_add. A SQL check on the membership table returns no row for the pairRequest body:
{"model":"gpt-6.1-sol","messages":[{"role":"user","content":"Verify missing team membership budget behavior"}],"stream":false,"max_tokens":16,"cache":{"no-cache":true}}Before (0fe4028)
User without a membership row
curl -s -w '\nHTTP_STATUS=%{http_code}' http://127.0.0.1:4000/chat/completions -H "Authorization: Bearer $KEY" -H 'Content-Type: application/json' -d "$BODY"After (95585ba, the tip only adds test and CI files on top)
User without a membership row
Member with a row over budget is still blocked
Type
🐛 Bug Fix
✅ Test
Caveats (if any)
Medium
E2E_FAIL_CLOSED_BUDGET_STACKgates it, and the shared e2e gateway stays fail-openQA runbook
team_member_budget: 100, POST /user/new, POST /key/generate with thatteam_idanduser_id, and skip /team/member_addFinal Attestation
Link to Devin session: https://app.devin.ai/sessions/06700bf6371c45d68fe0dba6c6d20084
Open in Devin Desktop: https://app.devin.ai/desktop/session/06700bf6371c45d68fe0dba6c6d20084?variant=devin