fix(ocr): round-5 review nits — empty-pages failure, comments, test cleanup
Round 5 review (PR #910), all nits, no blockers: - _result_from_success: a succeeded job with `pages=[]` (empty list, not just a missing key) is now a per-document failure ("no pages returned") instead of a silent 0-chunk success. Test added. - Comment the deadline-expiry path: the gateway-side job isn't cancelled (no cancel endpoint at this layer) — it's reaped by the gateway file purge; we just stop polling it. - Drop the vestigial status="pending" from the BatchOcrJob test fakes (the column was removed in round 4; BatchPollResult.status fakes are untouched). 1653 unit tests pass; ruff + ty green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
232684e881
commit
bb08245c91
@@ -479,6 +479,9 @@ class OcrProcessor(DocumentProcessor):
|
|||||||
await store.delete(
|
await store.delete(
|
||||||
user_id=user_id, doc_id=doc_id, doc_type=doc_type, etag=etag
|
user_id=user_id, doc_id=doc_id, doc_type=doc_type, etag=etag
|
||||||
)
|
)
|
||||||
|
# We don't cancel the gateway-side job (there's no cancel endpoint
|
||||||
|
# at this layer) — it keeps running and is reaped by the gateway's
|
||||||
|
# own file purge. Dropping the row just stops us polling it.
|
||||||
logger.warning(
|
logger.warning(
|
||||||
"batch OCR job %s exceeded max wait (%ss); marking failed",
|
"batch OCR job %s exceeded max wait (%ss); marking failed",
|
||||||
job.job_id,
|
job.job_id,
|
||||||
|
|||||||
@@ -184,8 +184,13 @@ def _result_from_success(body: dict[str, Any]) -> BatchPollResult:
|
|||||||
error="batch job succeeded but returned no results",
|
error="batch job succeeded but returned no results",
|
||||||
)
|
)
|
||||||
item = results[0]
|
item = results[0]
|
||||||
if item.get("error") is not None or item.get("pages") is None:
|
# ``not item.get("pages")`` catches both a missing key AND an empty list:
|
||||||
return BatchPollResult(status=_FAILED, pages=[], error=item.get("error"))
|
# a succeeded job that produced zero pages is a per-document failure (nothing
|
||||||
|
# to index), not a silent 0-chunk success.
|
||||||
|
if item.get("error") is not None or not item.get("pages"):
|
||||||
|
return BatchPollResult(
|
||||||
|
status=_FAILED, pages=[], error=item.get("error") or "no pages returned"
|
||||||
|
)
|
||||||
# Defensive on both fields (the page index falls back to position) so a
|
# Defensive on both fields (the page index falls back to position) so a
|
||||||
# malformed page object degrades rather than raising KeyError mid-parse.
|
# malformed page object degrades rather than raising KeyError mid-parse.
|
||||||
pages = [
|
pages = [
|
||||||
|
|||||||
@@ -140,6 +140,15 @@ async def test_poll_succeeded_with_per_document_error_is_failed(monkeypatch):
|
|||||||
assert result.is_failed and result.error == "bad page"
|
assert result.is_failed and result.error == "bad page"
|
||||||
|
|
||||||
|
|
||||||
|
async def test_poll_succeeded_empty_pages_is_failed(monkeypatch):
|
||||||
|
# A succeeded job that produced zero pages is a per-document failure, not a
|
||||||
|
# silent 0-chunk success.
|
||||||
|
body = {"status": "succeeded", "results": [{"custom_id": "d", "pages": []}]}
|
||||||
|
_patch_transport(monkeypatch, lambda r: httpx.Response(200, json=body))
|
||||||
|
result = await gbc.GatewayBatchOcrClient("https://gw", "m").poll("mistral/j")
|
||||||
|
assert result.is_failed and result.error == "no pages returned"
|
||||||
|
|
||||||
|
|
||||||
async def test_poll_succeeded_no_results_is_failed(monkeypatch):
|
async def test_poll_succeeded_no_results_is_failed(monkeypatch):
|
||||||
_patch_transport(
|
_patch_transport(
|
||||||
monkeypatch,
|
monkeypatch,
|
||||||
|
|||||||
@@ -299,7 +299,7 @@ class _FakeStore:
|
|||||||
self, *, user_id, doc_id, doc_type, etag, job_id, submitted_at=None
|
self, *, user_id, doc_id, doc_type, etag, job_id, submitted_at=None
|
||||||
):
|
):
|
||||||
self.rows[(user_id, doc_id, doc_type, etag)] = SimpleNamespace(
|
self.rows[(user_id, doc_id, doc_type, etag)] = SimpleNamespace(
|
||||||
job_id=job_id, status="pending", submitted_at=submitted_at or 1000
|
job_id=job_id, submitted_at=submitted_at or 1000
|
||||||
)
|
)
|
||||||
|
|
||||||
async def delete(self, *, user_id, doc_id, doc_type, etag):
|
async def delete(self, *, user_id, doc_id, doc_type, etag):
|
||||||
@@ -360,7 +360,7 @@ async def test_batch_first_run_submits_and_returns_pending_sentinel(monkeypatch)
|
|||||||
|
|
||||||
|
|
||||||
async def test_batch_existing_pending_polls_and_defers(monkeypatch):
|
async def test_batch_existing_pending_polls_and_defers(monkeypatch):
|
||||||
preset = SimpleNamespace(job_id="mistral/j", status="pending", submitted_at=1000)
|
preset = SimpleNamespace(job_id="mistral/j", submitted_at=1000)
|
||||||
client = _FakeBatchClient(poll=BatchPollResult(status="pending", pages=[]))
|
client = _FakeBatchClient(poll=BatchPollResult(status="pending", pages=[]))
|
||||||
store = _FakeStore(preset=preset)
|
store = _FakeStore(preset=preset)
|
||||||
# submitted just now -> deadline not reached
|
# submitted just now -> deadline not reached
|
||||||
@@ -377,7 +377,7 @@ async def test_batch_existing_pending_polls_and_defers(monkeypatch):
|
|||||||
|
|
||||||
|
|
||||||
async def test_batch_succeeded_returns_indexed_result(monkeypatch):
|
async def test_batch_succeeded_returns_indexed_result(monkeypatch):
|
||||||
preset = SimpleNamespace(job_id="mistral/j", status="pending", submitted_at=1000)
|
preset = SimpleNamespace(job_id="mistral/j", submitted_at=1000)
|
||||||
client = _FakeBatchClient(
|
client = _FakeBatchClient(
|
||||||
poll=BatchPollResult(status="succeeded", pages=[(0, "# One"), (1, "## Two")])
|
poll=BatchPollResult(status="succeeded", pages=[(0, "# One"), (1, "## Two")])
|
||||||
)
|
)
|
||||||
@@ -395,7 +395,7 @@ async def test_batch_succeeded_returns_indexed_result(monkeypatch):
|
|||||||
|
|
||||||
|
|
||||||
async def test_batch_failed_marks_parse_error(monkeypatch):
|
async def test_batch_failed_marks_parse_error(monkeypatch):
|
||||||
preset = SimpleNamespace(job_id="mistral/j", status="pending", submitted_at=1000)
|
preset = SimpleNamespace(job_id="mistral/j", submitted_at=1000)
|
||||||
client = _FakeBatchClient(
|
client = _FakeBatchClient(
|
||||||
poll=BatchPollResult(status="failed", pages=[], error="x")
|
poll=BatchPollResult(status="failed", pages=[], error="x")
|
||||||
)
|
)
|
||||||
@@ -414,7 +414,7 @@ async def test_batch_failed_marks_parse_error(monkeypatch):
|
|||||||
async def test_batch_unexpected_status_marks_failed_not_empty_success(monkeypatch):
|
async def test_batch_unexpected_status_marks_failed_not_empty_success(monkeypatch):
|
||||||
# A terminal status that isn't succeeded/failed (gateway skew) must NOT
|
# A terminal status that isn't succeeded/failed (gateway skew) must NOT
|
||||||
# produce a 0-chunk "success" that silently indexes empty text + loops.
|
# produce a 0-chunk "success" that silently indexes empty text + loops.
|
||||||
preset = SimpleNamespace(job_id="mistral/j", status="pending", submitted_at=1000)
|
preset = SimpleNamespace(job_id="mistral/j", submitted_at=1000)
|
||||||
client = _FakeBatchClient(poll=BatchPollResult(status="cancelled", pages=[]))
|
client = _FakeBatchClient(poll=BatchPollResult(status="cancelled", pages=[]))
|
||||||
store = _FakeStore(preset=preset)
|
store = _FakeStore(preset=preset)
|
||||||
_wire_batch(monkeypatch, client=client, store=store)
|
_wire_batch(monkeypatch, client=client, store=store)
|
||||||
@@ -430,7 +430,7 @@ async def test_batch_unexpected_status_marks_failed_not_empty_success(monkeypatc
|
|||||||
|
|
||||||
|
|
||||||
async def test_batch_deadline_exceeded_marks_timeout(monkeypatch):
|
async def test_batch_deadline_exceeded_marks_timeout(monkeypatch):
|
||||||
preset = SimpleNamespace(job_id="mistral/j", status="pending", submitted_at=1000)
|
preset = SimpleNamespace(job_id="mistral/j", submitted_at=1000)
|
||||||
client = _FakeBatchClient(poll=BatchPollResult(status="pending", pages=[]))
|
client = _FakeBatchClient(poll=BatchPollResult(status="pending", pages=[]))
|
||||||
store = _FakeStore(preset=preset)
|
store = _FakeStore(preset=preset)
|
||||||
# now far past submitted_at + max_wait (86400)
|
# now far past submitted_at + max_wait (86400)
|
||||||
|
|||||||
Reference in New Issue
Block a user