fix(check_batch_cost): retire permanently-unroutable and not-found batches - #36656
anneheartrecord wants to merge 1 commit into
Conversation
Greptile SummaryThis PR retires permanently unroutable and missing-provider batch reconciliation rows so they no longer starve later work.
Confidence Score: 4/5The recoverable deployment-not-found path must be distinguished from a genuinely absent provider batch before this PR is safe to merge. The new broad NotFoundError handler can permanently remove a batch from reconciliation when Azure reports a missing deployment, preventing spend recovery after configuration is restored. Files Needing Attention: enterprise/litellm_enterprise/proxy/common_utils/check_batch_cost.py
|
| Filename | Overview |
|---|---|
| enterprise/litellm_enterprise/proxy/common_utils/check_batch_cost.py | Adds permanent retirement paths, but the NotFoundError catch also retires recoverable Azure deployment-not-found failures and the new helper conflicts with repository Python conventions. |
| tests/proxy_unit_tests/test_check_batch_cost.py | Adds focused regression coverage, though its generic NotFoundError fixture does not distinguish a missing batch from other 404 mappings. |
Reviews (1): Last reviewed commit: "fix(check_batch_cost): retire permanentl..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "batch_ignore_default_logging": True, | ||
| }, | ||
| ) | ||
| except LiteLLMNotFoundError as e: | ||
| verbose_proxy_logger.info( | ||
| f"Retiring job {job.unified_object_id}: provider reports batch " | ||
| f"{batch_id} not found — this is permanent, no retry will recover it: {e}" | ||
| ) | ||
| self._record_error(prom_logger, "provider_batch_not_found") |
There was a problem hiding this comment.
When Azure batch retrieval reports DeploymentNotFound, LiteLLM maps it to LiteLLMNotFoundError, and this catch permanently retires the batch as not_found, causing its cost never to reconcile after the deployment configuration is restored.
| provably permanent (see PERMANENTLY_UNPROCESSABLE_STATUSES). Leaving | ||
| batch_processed=False here would let the row re-occupy the fixed-size, | ||
| oldest-first poll window on every cycle and starve every batch behind it. |
There was a problem hiding this comment.
The new helper constructs update_data as Dict[str, Any] and then mutates it conditionally, discarding useful static guarantees about the database payload and conflicting with the repository's immutable, fully typed Python conventions.
Context Used: CLAUDE.md (source)
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!
| # Statuses a row is written back with when a failure is provably permanent — no retry | ||
| # or config change can ever resolve it — so it must stop occupying the fixed-size, | ||
| # oldest-first poll window instead of being retried forever. Both are excluded from | ||
| # every find_many query the same way the provider-native terminal statuses are. | ||
| PERMANENTLY_UNPROCESSABLE_STATUSES = ("unroutable", "not_found") | ||
|
|
||
|
|
||
| class _UnroutableBatchError(Exception): | ||
| """Raised by _resolve_job_routing for the one routing failure that is permanent: | ||
| a unified id that decodes successfully but embeds no model id. Every other | ||
| routing failure is config-dependent (enabling unmanaged tracking, restoring a | ||
| deployment) and must keep being retried, so it returns None instead of raising. | ||
| """ |
There was a problem hiding this comment.
The new constant and exception add lengthy comments and docstrings that repeat the nearby control flow rather than limiting commentary to essential complex business logic, increasing maintenance cost when the retirement behavior changes.
Context Used: CLAUDE.md (source)
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!
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5113f4237
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "batch_ignore_default_logging": True, | ||
| }, | ||
| ) | ||
| except LiteLLMNotFoundError as e: |
There was a problem hiding this comment.
When Azure returns DeploymentNotFound or another configuration-related 404, this catch permanently retires the batch, so restoring the deployment can never reconcile its cost
Useful? React with 👍 / 👎.
| PERMANENTLY_UNPROCESSABLE_STATUSES = ("unroutable", "not_found") | ||
|
|
||
|
|
||
| class _UnroutableBatchError(Exception): |
There was a problem hiding this comment.
Return a tagged routing outcome instead of introducing an exception solely for internal control flow, as the repository explicitly requires CLAUDE.mdL80-L89
Useful? React with 👍 / 👎.
| # Statuses a row is written back with when a failure is provably permanent — no retry | ||
| # or config change can ever resolve it — so it must stop occupying the fixed-size, | ||
| # oldest-first poll window instead of being retried forever. Both are excluded from | ||
| # every find_many query the same way the provider-native terminal statuses are. |
There was a problem hiding this comment.
Remove this narration because the constant is self-explanatory; repository guidance permits comments only when complex business logic genuinely requires them CLAUDE.mdL1-L9
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…tches _resolve_job_routing() returned None for every routing failure and the retrieval except-block continued on every provider error, so a row that can never be costed (a unified id with no model_id, or a batch the provider reports 404 for) stayed batch_processed=False forever. Since the poll query orders oldest-first with a fixed page size, one such row permanently occupies a slot and starves every batch behind it. Only these two failures are provably permanent; every other routing/ retrieval failure is config-dependent (flag off, deployment missing, transient error) and must keep retrying. Retire just the permanent ones by marking batch_processed=True with a synthetic terminal status (unroutable/not_found), excluded from both the primary and fallback poll queries the same way the provider-native terminal statuses are.
f5113f4 to
1a5b941
Compare
|
Closing as superseded by #36714, which merged with the same page-slot retirement plus a staleness sweep for terminal rows. Thanks for the PR! |
2 participants
TLDR
Problem this solves:
How it solves it:
User Flow
Before: a team lead who runs nightly batch jobs sees $0.00 spend for every batch, forever, because one old batch sits at the head of the reconciliation queue and never clears
GET /v1/files/{output_file_id}/content— the work was really done/ui/?page=logsfiltered to their key — the batch is$0.00GET /key/infoshows unchanged spend$0.00, indefinitely — the first un-costable or provider-expired batch blocks all of themAfter: the un-costable/expired batch is retired once and stops blocking newer batches
/ui/?page=logsand/key/infoshow real spend for those batchesRelevant issues
Fixes #36640
Linear ticket
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
This is a background poller bug (fixed-page-size, oldest-first DB poll), so proof is the unit-test regression suite rather than a live proxy curl — same as the sibling fixes to this poller (#35360, #34785).
Before the fix (source at parent commit
0e9cd9893e, new tests present):After the fix (this branch,
f5113f4237):The new tests assert:
status=unroutable,batch_processed=True) instead of retried foreveraretrieve_batchis retired (status=not_found,batch_processed=True)batch_processedcolumnType
🐛 Bug Fix
Caveats (if any)
unroutable/not_foundstatuses are new values for theLiteLLM_ManagedObjectTable.statuscolumn (a free-formString?, no enum), following the existingstale_expiredprecedent for rows the poller gives up on.Final Attestation