fix(budgets): fail an override write the proxy never received

[aivong-openhands]

HUMAN:

• A human has tested these changes.

AGENT:

---

Why

Setting or clearing a per-user budget override answered 200 even when the cap never reached LiteLLM. Both endpoints write the override row first and resync afterwards, and _sync_litellm_budgets records an 'error' row and returns rather than raising when the spend read or the cap write fails. The admin sees the new cap accepted while the proxy still enforces the old one, with nothing in the response to say so.

This checks the recorded sync status after the resync and answers 503 when it failed, naming the recorded error. The same check covers the delete path, where a failed resync otherwise leaves the proxy enforcing a cap the organization has just removed.

Summary

• Add _require_applied_sync and call it after the resync in both upsert_user_override and delete_user_override.
• Un-skip the reproduction test, which makes the LiteLLM read time out and asserts the write is refused.

Issue Number

N/A

How to Test

.venv/bin/python -m pytest -q \
  tests/unit/test_org_budget_service.py \
  tests/unit/server/routes/test_orgs.py

Expect 168 passed, 6 skipped. Reverting the check makes test_override_write_reports_failure_when_litellm_is_unreachable fail, because the call returns normally.

Video/Screenshots

N/A — no UI change.

Type

• Bug fix
• Feature
• Refactor
• Breaking change
• Docs / chore

Notes

Two things worth a reviewer's judgement. The override row is written before the resync, so a 503 leaves the request to be rolled back by the caller's transaction handling rather than by an explicit compensating write — worth confirming that is what happens on this path. And a proxy outage now makes override edits fail loudly instead of silently diverging; that is the point, but it is a availability-visible behaviour change for admins.

One of a set of draft PRs, each carrying a single defect the Quint model for org budgets surfaced, together with the reproduction test that was already committed but skipped.

🤖 Generated with Claude Code

---

Enterprise server image for this PR:

ghcr.io/openhands/enterprise-server:sha-608e295

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
server/services
org_budget_service.py					775, 795-799, 811-812
Project Total

This report was generated by python-coverage-comment-action

[aivong-openhands]

🔴 Taste Rating: Needs improvement

The problem statement is wrong, and the fix it motivates discards state the system deliberately keeps.

[CRITICAL ISSUES]

• [server/services/org_budget_service.py:774, :798] Breaking Change — this replaces a working, richer mechanism with a coarser one. The PR description says the endpoint "answers 200 even when the cap never reached LiteLLM." That is not what main does. Both routes already detect and report exactly this condition:

  • server/routes/orgs.py:1322-1323 — upsert_org_budget_override reads user_row['reconciliation_state'] and sets response.status_code = 503 when it is degraded or failed.
  • server/routes/orgs.py:1348-1350 — delete_org_budget_override calls get_reconciliation_state(org_id) and does the same.

  Both behaviours are pinned by existing tests in tests/unit/server/routes/test_org_budget_reconciliation_routes.py — test_member_budget_write_returns_503_for_unhealthy_reconciliation and test_member_budget_delete_returns_503_for_unhealthy_reconciliation, both parametrised over ['degraded', 'failed']. A failed sync sets litellm_last_sync_status='error', which drives reconciliation_state to degraded/failed in _budget_policy_comparison, which produces the 503. The admin already gets a 503 today.

  So this is not "200 becomes 503." It is "a 503 with a response body becomes a 503 with no response body." That body is the whole point of the existing design:

  before: 503 + OrgBudgetUserMutationResponse{effective_monthly_limit, reconciliation_state,
                                              reconciliation_error, applied_at, ...}
  after:  503 + {"detail": "The budget override was not applied to the LLM proxy: ..."}
  

  The current shape lets the UI say "your change is saved but not yet enforced by the proxy, here is what the proxy currently has" — degraded, honest, actionable. raise HTTPException throws that away. OrgBudgetUserMutationResponse exists specifically to carry reconciliation_state/reconciliation_error on writes; this change makes those fields dead on the failure path they were built for.

• [server/services/org_budget_service.py:774] The raise discards the committed override row. This is the concern the PR notes flag for a reviewer, and the answer is worse than "worth confirming." DbSessionInjector.inject (openhands/app_server/services/db_session_injector.py:326-333) commits on clean return and rolls back on any exception — HTTPException included, there is no special case. So raising from inside the service rolls back the override row that store.upsert_override already flushed.

  That converts a recoverable state into a lost write. Today: row saved, proxy stale, reconciliation_state='degraded', and the next run_budget_maintenance pass re-syncs the org and converges. After this change: the admin's edit is gone entirely, and there is nothing left for maintenance to converge to. A transient LiteLLM timeout now silently discards admin intent. Note also that _record_litellm_sync writes litellm_last_sync_status='error' to the settings row — that diagnostic write is rolled back too, so the failure leaves no trace in the database.

  The delete path is worse in the same way: rolling back means the override still exists, the admin believes they removed it, and the proxy keeps enforcing it. The PR description argues this path needs the check because "a failed resync otherwise leaves the proxy enforcing a cap the organization has just removed" — but rolling the delete back guarantees that outcome instead of fixing it.

• [tests/unit/test_org_budget_service.py:2123] The test asserts the wrong thing and cannot catch either problem above. It asserts only pytest.raises(HTTPException). It never checks the status code, never checks whether the override row survived, and — since it calls the service directly rather than through the route — never observes that main already returns 503 via response.status_code. A test at the route level (like the two already in test_org_budget_reconciliation_routes.py) would have shown there was no 200 to fix.

[IMPROVEMENT OPPORTUNITIES]

• [server/services/org_budget_service.py:813-819] Unnecessary comments: the docstring's body restates the PR description, including the premise I believe is incorrect ("without this an override write answers 200"). A comment asserting non-local behaviour of two route handlers, with no mechanism to stay true, is the exact category that drifts and misleads — and here it is already inaccurate against main.

• [server/services/org_budget_service.py:808, :822] get_reconciliation_state and _require_applied_sync now both test settings.litellm_last_sync_status == 'error' eight lines apart for different purposes. If a check is needed at all, it belongs next to the existing one rather than as a parallel predicate.

[TESTING GAPS]

If you keep any version of this, the tests that would actually prove it are: (1) a route-level test asserting the exact status code and body on sync failure, and (2) a test asserting whether the override row exists after the failure. (2) is the one that decides whether this change is safe, and neither this PR nor main has it.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🔴 HIGH

Changes the response contract of two admin endpoints from a structured error body to a bare detail, and — via the request-scoped session's rollback-on-exception — turns a recoverable "saved but not yet applied" state into a discarded write whenever the LLM proxy is briefly unreachable. Proxy timeouts are transient and routine; this converts each one into lost admin intent with no database record that anything happened. The frontend mutations in frontend/src/components/features/budgets/budgets.tsx have onSettled invalidation but no error handling, so the admin's most likely experience is the form reverting with no explanation.

The PR notes name the rollback question but do not resolve it, and no test covers it. That combination — a transaction-boundary behaviour change, on a shared session, unverified — is what pushes this to HIGH rather than MEDIUM.

Recommendation: Do not auto-merge. This needs a human reviewer who owns the budgets API contract to confirm the intended failure semantics: is a failed proxy sync meant to reject the admin's write, or accept it and report it as unapplied? main clearly implements the second. If the first is now wanted, it needs an explicit compensating write or an outer transaction boundary — not an exception thrown through a session that auto-rolls-back — plus a decision about the OrgBudgetUserMutationResponse contract.

VERDICT:
❌ Needs rework: The stated defect does not reproduce on main, and the fix regresses both the error contract and write durability.

KEY INSIGHT:
The system already distinguishes "saved but not yet enforced" from "rejected," and that distinction is the feature; collapsing it into an exception on an auto-rollback session deletes the admin's write to report a condition that was already being reported.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

---

This review was generated by an AI agent (OpenHands) on behalf of @aivong-openhands.