fix(batch): raise on download failure instead of a silent empty result - #957
Conversation
PgLlmBatchBackend.retrieve() and PgLlmBatchEmbeddingBackend.retrieve() used
to convert an explicit backend download failure ({"success": false, ...})
into a bare `return []`, indistinguishable from a batch that legitimately
completed with zero items. For embeddings this was severe:
embeddings_batch_document() cached the resulting fabricated zero-vector
result under status="completed", permanently poisoning that batch_id since
the cache short-circuits all future poll/retrieve calls.
Add BatchDownloadError (job id + backend-reported reason), raised instead of
the silent []. retrieve_batch() lets it propagate (server.py maps it to 502
batch_download_failed); embeddings_batch_document() catches it, returns
status="failed" with no caching, so a failed retrieval stays retryable.
Also: LocalBatchBackend (the default backend for standalone deployments)
was discarding real per-request usage from orchestrator.complete()'s nested
result["trace"][i]["usage"], leaving BatchResultItem's token counts at their
0 default. It now aggregates the real counts, and BatchResultItem carries
the original request messages through so retrieve_batch()'s heuristic
estimate fallback estimates from the real prompt instead of a hardcoded
blank "" placeholder.
|
Warning Review limit reachedNext included review available in 50 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (12)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Generated by Claude Code |
|
@opencode-agent please review this draft PR. Generated by Claude Code |
test_batch_routing_jobs_endpoint_submits_multiple_requests hardcoded "one ledger row per batch item," which the per-trace-step usage attribution contract already documented in CostRoutingCoordinator.complete() contradicts: each conduct-mode trace step is a separate billable provider call. Submitting 2 requests through /api/v1/batch_routing_jobs with the default single-agent mock pool triages to the conduct path (4 steps each), producing 8 ledger rows, not 2 -- this was breaking CI on the latest commit. The assertion now derives its expectation from the retrieved results' own usage_record_ids, matching the fix already applied to the sibling assertions in tests/test_cost_router.py.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Two independent PRs hitting the identical failure signature within ~15 minutes points to live Re-ran the failed job once ( Generated by Claude Code |
|
That HTTP 404 observation is now fixed in #979 (draft, fully validated): a hand-maintained denylist excludes the two known-retired NVIDIA NIM ids ( Generated by Claude Code |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Cross-PR integration contract: routing identity is provider-neutral |
|
Acknowledged. This PR's diff ( Generated by Claude Code |
|
Contextual-Orchestrator와 관계한 것들을 같이 손보든 어쩌든 해결하세요. Bypass merge 필요하면 가능 (chicken and eggs 상황이라면) + NVIDIA NIM 만 쓰는 건 허용하지 않아요. Contextual-Orchestrator를 쓰세요. Timeout은 적어도 3시간으로 잡으세요. 120초 같은 건 당황스럽군요. Opencode와 Noema 는 Coderabbitai 및 Devin 수준으로 실제로 리뷰를 하게 하시오. Strix도 보안 리뷰를 꼼꼼하게 하도록 하시오. 특히 보안 리뷰는 전체 코드로 수행하는 것입니다. Contextual-Orchestrator는 실시간으로 빠르면서 능력이 좋은 모델에 요청을 보내어 시간을 당기시오. |
|
Update on head Correcting my own earlier comment: I previously pointed at No code change needed in this PR. Already spent this failure's one re-run earlier ( Generated by Claude Code |
|
Genuine state change since my last comment: confirmed Generated by Claude Code |
|
흠.. 아직 Strix가 해결이 안 됐네요. 여긴. |
|
"Strix가 아직 해결이 안 됐다"는 말씀 관련 현황입니다.
이 PR 자체는 이미 Generated by Claude Code |
|
Generated by Claude Code |
|
Correction to my earlier comment: the re-run did not pick up the fix. The underlying fix is real and already on Generated by Claude Code |
…y and reconcile with PR 956
| persistence_settled = self.ledger.wait_for_usage_record_ids( | ||
| list(record_ids), | ||
| timeout=_BATCH_LEDGER_SETTLEMENT_TIMEOUT_SECONDS, | ||
| ) |
There was a problem hiding this comment.
🔴 Ledger read failures abort batch results
When wait_for_usage_record_ids encounters a ledger query error, it propagates after successful result download. Batch callers receive a server error instead of completed results.
Prompt for agents
Make batch ledger settlement tolerate lookup failures in CostRoutingCoordinator.retrieve_batch and CostLedger.wait_for_usage_record_ids. A successful backend result must still be returned with usage_persistence_status set to pending when the ledger lookup raises, matching the existing best-effort behavior for append failures. Preserve deterministic usage IDs and avoid hiding the persistence failure from telemetry.
Was this helpful? React with 👍 or 👎 to provide feedback.
| counts = self._provider_usage(usage) | ||
| if counts is not None: | ||
| race_usage.append({ | ||
| "agent_id": endpoint_id, | ||
| "usage": {"prompt_tokens": counts[0], "completion_tokens": counts[1]}, | ||
| }) |
There was a problem hiding this comment.
🟡 Unmetered race losers disappear from billing
When a local batch race loser reports missing or malformed usage, _run_local_batch drops it. Cost reports omit a completed billable provider call.
Prompt for agents
Preserve every completed local-batch race loser in _run_local_batch, including entries whose usage cannot be parsed. Carry explicit unavailable-measurement evidence into BatchResultItem.race_usage, then have retrieve_batch create a deterministic measurement_status="unavailable" ledger row with zero token placeholders. Aggregate batch cost status with unavailable taking precedence, as the synchronous race path already does.
Was this helpful? React with 👍 or 👎 to provide feedback.
| # The original request messages, carried through so a caller whose | ||
| # backend reports no usage (e.g. LocalBatchBackend against a mock/local | ||
| # runner) can estimate from the real prompt instead of a blank placeholder. | ||
| messages: List[Dict[str, str]] = field(default_factory=list) | ||
| # Local runs keep their per-call identities and usage intact so the cost | ||
| # router can meter heterogeneous conduct workflows one provider at a time. | ||
| trace: List[Dict[str, Any]] = field(default_factory=list) | ||
| cache_status: Optional[str] = None | ||
| race_usage: List[Dict[str, Any]] = field(default_factory=list) |
There was a problem hiding this comment.
🟡 Existing batch result constructors misbind fields
Positional callers that supplied usage_valid now bind that value to messages. Retrieval can crash token estimation or misclassify provider usage.
Prompt for agents
Restore BatchResultItem positional compatibility by keeping usage_valid in its previous field position and appending the new messages, trace, cache_status, and race_usage fields after it. Update internal constructors to use keywords where practical and add a regression test that constructs the public dataclass with the former positional signature.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _validated_download_responses( | ||
| payload: Dict[str, Any], | ||
| *, | ||
| expected_custom_ids: set[str], | ||
| request_count: int, | ||
| job_id: str, | ||
| ) -> List[Dict[str, Any]]: | ||
| """Return a complete one-to-one set of successful downloaded rows.""" | ||
| responses = payload.get("responses") | ||
| if not isinstance(responses, list): | ||
| raise BatchDownloadError(job_id, "malformed response list") | ||
| if request_count and not expected_custom_ids: | ||
| raise BatchDownloadError(job_id, "submitted request metadata is unavailable") | ||
|
|
||
| seen: set[str] = set() | ||
| for entry in responses: | ||
| if not isinstance(entry, dict): | ||
| raise BatchDownloadError(job_id, "malformed response row") | ||
| custom_id = entry.get("custom_id") | ||
| if ( | ||
| not isinstance(custom_id, str) | ||
| or custom_id not in expected_custom_ids | ||
| or custom_id in seen | ||
| ): | ||
| raise BatchDownloadError(job_id, "response identifiers do not match the submission") | ||
| seen.add(custom_id) | ||
| if entry.get("error") is not None: | ||
| raise BatchDownloadError(job_id, "one or more batch items failed") | ||
| response = entry.get("response") | ||
| if not isinstance(response, dict): | ||
| raise BatchDownloadError(job_id, "malformed response row") | ||
| status_code = response.get("status_code") | ||
| if status_code is not None and ( | ||
| type(status_code) is not int or not 200 <= status_code < 300 | ||
| ): | ||
| raise BatchDownloadError(job_id, "one or more batch items failed") | ||
|
|
||
| if seen != expected_custom_ids or len(responses) != request_count: | ||
| raise BatchDownloadError(job_id, "download returned an incomplete result set") | ||
| return responses |
There was a problem hiding this comment.
📝 Info: Body validation remains downstream
_validated_download_responses validates row identity and transport success only. Empty answers or vectors retain the existing endpoint-specific handling.
Was this helpful? React with 👍 or 👎 to provide feedback.
| self._embedding_part_counts = registry.mapping("embedding_part_counts") | ||
| self._embedding_part_limits = registry.mapping("embedding_part_limits") | ||
| self._embedding_documents = registry.mapping("embedding_documents") | ||
| self._batch_documents = registry.mapping("batch_documents") |
There was a problem hiding this comment.
🔍 Batch document registry is unused
_batch_documents is never read or written. Deterministic ledger IDs provide idempotency, so this mapping misleadingly suggests response caching exists.
Was this helpful? React with 👍 or 👎 to provide feedback.
3 participants
Summary
Two related "hollow path" bugs found while auditing the batch/cost-review hub for cases where a request path reports success or a plausible-looking cost without the underlying evidence actually existing (per this repo's own Honest metrics convention).
Bug 1 — a batch download failure was indistinguishable from a legitimately empty result.
PgLlmBatchBackend.retrieve()andPgLlmBatchEmbeddingBackend.retrieve()returned a bare[]whenever the injected client reported{"success": false, ...}— the same shape returned by a batch that genuinely completed with zero items. For embeddings this was severe:CostRoutingCoordinator.embeddings_batch_document()cached that fabricated zero-vector result understatus: "completed", and the cache short-circuits every laterpoll/retrievecall before ever touching the backend again — so one transient download failure permanently poisoned thatbatch_idwith no way to recover except an out-of-band cache clear.New
BatchDownloadError(job_id, reason), raised instead of[].retrieve_batch()lets it propagate (mirroring how an unknown/unowned job id'sKeyErroralready propagates);server.pymaps it to502 batch_download_failed.embeddings_batch_document()catches it, returnsstatus: "failed"with anerrorfield, and deliberately does not cache the result — a failed retrieval stays retryable.Bug 2 —
LocalBatchBackend(the default backend for every standalone/self-hosted deployment withoutpg-llm-batch) discarded real usage.orchestrator.complete()'s result has no top-levelusagekey — real provider usage is nested per workflow step inresult["trace"][i]["usage"].LocalBatchBackend.submit()never read it, so every local-batch result carried the dataclass's0/0token defaults.BatchResultItemnow aggregates realprompt_tokens/completion_tokensfromtrace. Separately,retrieve_batch()'s heuristic-estimate fallback (triggered whenever a batch item reports no usage) always estimated from a hardcoded blank""prompt regardless of what was actually asked — every such row was labeledmeasurement_status="estimated"while being a constant ~3-token estimate, misrepresenting what "estimated" means to a buyer reading the ledger.BatchResultItemnow carries the original requestmessagesthrough so the fallback estimates from the real prompt.Developer experience
BatchDownloadErrorexported from the package (contextual_orchestrator/__init__.py).tests/test_batch_routing.py,tests/test_batch_routing_boundaries.py,tests/test_cost_router.py,tests/test_cost_router_boundaries.py.python -m pytest tests -q: 2833 passed, 1 skipped, 1 pre-existing unrelated failure (test_psychometric_routing.py::test_fast_mlsirm_fit_uses_judge_acceptance_item_for_context_score—ModuleNotFoundError: No module named 'fast_mlsirm', the same pre-existing environment gap already confirmed unrelated in#955and#956's own validation).origin/main(c6c3a0c9), no conflicts.batch_routing.py::retrieve/cost_router.py::retrieve_batch/embeddings_batch_documentbefore this one (checked before opening).User experience
502 batch_download_failed(with the job id and reason) on a real download failure instead of a misleading200with an empty/zero-item result, and can retry — previously an embeddings batch could get stuck permanently reporting zero-vector "completed" results.Test plan
python -m pytest tests -q(2833 passed / 1 skipped / 1 pre-existing unrelated failure)interrogate— repo docstring-coverage gate unaffected by this change's scopeGenerated by Claude Code