generated from john/python-template
V4.6 Phase 3: worker and provider reliability
Claim jobs atomically [CRIT-01] - Replace JobService.read_next_queued_job with claim_next_queued_job, which selects and transitions QUEUED -> PROCESSING inside one transaction. The old read-then-write sequence left a window in which two workers could observe the same QUEUED row. - Add the missing .limit(1). The poll previously ordered the entire queued set and discarded all but the first row. - Drop the eager loads from the hot poll entirely. They were pure waste: process_queued_job immediately re-reads the job through read_job with the relationships it actually needs. - Guard the row with with_for_update(skip_locked=True) on PostgreSQL so the claim stays correct once more than one worker exists. On SQLite the claim is a bounded single-writer transaction. - Correct the comment at the remaining direct-call claim site, which described the hazard rather than the guarantee. Reuse the provider connection [HIGH-02] - Build the ServiceBundle once per worker loop instead of once per job, and close it at loop shutdown. Every job previously constructed a new SourceService, and with it a new provider adapter and a new httpx.AsyncClient, paying a full TLS handshake per page and discarding the connection pool. - process_next_queued_job now accepts an optional caller-owned bundle and only closes bundles it created itself. Uncap the provider timeout [HIGH-03] - Remove le=20.0 from worker_provider_timeout_seconds. The cap equalled the default, so the ceiling could never be raised, and dense-page vision transcription routinely needs longer. Default raised to 180s. - Pass an explicit httpx.Timeout to the OpenRouter AsyncClient. httpx defaults every phase to 5 seconds, so the real read budget was 5s regardless of the configured value; the outer asyncio.wait_for could never be the binding constraint. Connect stays at 10s. Tighten the provider boundary [MED-03] - Declare model, current_request_manifest, current_transport_evidence, and aclose on the TranscriptionProvider Protocol. - Delete the per-call inspect.signature(adapter.transcribe).parameters reflection and the untyped kwargs dict it fed. The Protocol had declared requested_model all along, so the reflection was dead defensive weight on the hot path. - Replace the three getattr probes for aclose and the evidence attributes with direct typed access. Deduplicate bundle construction [MED-06] - Add ServiceBundle.from_session_factory and ServiceBundle.aclose, replacing the duplicated four-service instantiation blocks in app.py and worker.py. - _recover_stale_processing_jobs now uses the bundle built moments earlier instead of constructing a second JobService. Tests - Claiming returns the oldest job, marks it PROCESSING, never hands the same job out twice, and emits exactly one unadorned SELECT carrying LIMIT and no JOIN. - The worker loop threads one bundle through consecutive jobs and closes it once at shutdown; a caller-owned bundle is left open. - Settings accepts a timeout above 20 seconds and still rejects zero. - The OpenRouter client's read, write, and pool timeouts track the configured budget rather than the httpx default. Note: .env in this checkout still pins WORKER_PROVIDER_TIMEOUT_SECONDS=20 and should be raised to pick up this fix. Verified: 268 passed, 4 skipped; ruff check clean. Co-authored-by: Copilot App <[email protected]>
This commit is contained in:
@@ -4,6 +4,7 @@ from datetime import timedelta
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from sqlalchemy import event
|
||||
|
||||
from transcription.db.models import Document
|
||||
from transcription.db.models import Job
|
||||
@@ -101,7 +102,7 @@ class TestJobService:
|
||||
assert result[0].id == job.id
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_read_next_queued_job_orders_by_created_date(
|
||||
async def test_claim_next_queued_job_claims_oldest_and_marks_processing(
|
||||
self,
|
||||
job_service: JobService,
|
||||
document_service: DocumentService,
|
||||
@@ -119,9 +120,42 @@ class TestJobService:
|
||||
await job_service.create_job(job=first)
|
||||
await job_service.create_job(job=second)
|
||||
|
||||
next_job = await job_service.read_next_queued_job()
|
||||
assert next_job is not None
|
||||
assert next_job.id == first.id
|
||||
claimed = await job_service.claim_next_queued_job()
|
||||
assert claimed is not None
|
||||
assert claimed.id == first.id
|
||||
assert claimed.status == JobStatus.PROCESSING
|
||||
|
||||
# The claim is exclusive: the same job is never handed out twice.
|
||||
next_claim = await job_service.claim_next_queued_job()
|
||||
assert next_claim is not None
|
||||
assert next_claim.id == second.id
|
||||
|
||||
assert await job_service.claim_next_queued_job() is None
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_claim_next_queued_job_emits_a_bounded_unadorned_query(
|
||||
self,
|
||||
job_service: JobService,
|
||||
):
|
||||
"""CRIT-01: the hot poll must not select a subgraph or scan the queue."""
|
||||
statements: list[str] = []
|
||||
|
||||
async with job_service._session_scope() as session:
|
||||
bind = session.get_bind()
|
||||
|
||||
def capture(_conn, _cursor, statement, *_rest):
|
||||
statements.append(statement)
|
||||
|
||||
event.listen(bind, "before_cursor_execute", capture)
|
||||
try:
|
||||
await job_service.claim_next_queued_job(session=session)
|
||||
finally:
|
||||
event.remove(bind, "before_cursor_execute", capture)
|
||||
|
||||
selects = [item for item in statements if item.lstrip().upper().startswith("SELECT")]
|
||||
assert len(selects) == 1, selects
|
||||
assert "LIMIT" in selects[0].upper()
|
||||
assert "JOIN" not in selects[0].upper()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_job_persists_provider_and_model(
|
||||
|
||||
@@ -149,3 +149,28 @@ class TestWorkerReliabilitySettings:
|
||||
"""worker retry settings default to no retries."""
|
||||
settings = _make_settings()
|
||||
assert settings.worker_max_retries == 0
|
||||
|
||||
|
||||
def test_provider_timeout_is_not_capped_at_twenty_seconds():
|
||||
"""HIGH-03: vision transcription regularly runs past the old le=20.0 ceiling."""
|
||||
settings = Settings(openrouter_api_key="test-key", worker_provider_timeout_seconds=300.0)
|
||||
assert settings.worker_provider_timeout_seconds == 300.0
|
||||
|
||||
|
||||
def test_provider_timeout_must_still_be_positive():
|
||||
with pytest.raises(ValidationError):
|
||||
Settings(openrouter_api_key="test-key", worker_provider_timeout_seconds=0.0)
|
||||
|
||||
|
||||
def test_openrouter_client_timeout_tracks_the_configured_budget():
|
||||
"""HIGH-03: httpx defaults every phase to 5s, silently capping the provider call."""
|
||||
from transcription.providers.openrouter import OpenRouterTranscriptionProvider
|
||||
|
||||
settings = Settings(openrouter_api_key="test-key", worker_provider_timeout_seconds=123.0)
|
||||
provider = OpenRouterTranscriptionProvider(settings=settings)
|
||||
timeout = provider._capturing_client._client.timeout
|
||||
|
||||
assert timeout.read == 123.0
|
||||
assert timeout.write == 123.0
|
||||
assert timeout.pool == 123.0
|
||||
assert timeout.connect == 10.0
|
||||
|
||||
+65
-5
@@ -3,6 +3,7 @@ import logging
|
||||
|
||||
import pytest
|
||||
|
||||
from transcription.services import ServiceBundle
|
||||
from transcription.worker import process_next_queued_job
|
||||
from transcription.worker import run_worker_loop
|
||||
|
||||
@@ -12,9 +13,9 @@ async def test_run_worker_loop_survives_process_next_exception(monkeypatch, capl
|
||||
calls = 0
|
||||
stop_event = asyncio.Event()
|
||||
|
||||
async def _fake_process_next_queued_job(*, session=None, session_factory=None):
|
||||
async def _fake_process_next_queued_job(*, session=None, session_factory=None, services=None):
|
||||
nonlocal calls
|
||||
_ = (session, session_factory)
|
||||
_ = (session, session_factory, services)
|
||||
calls += 1
|
||||
if calls == 1:
|
||||
raise RuntimeError("boom")
|
||||
@@ -31,7 +32,10 @@ async def test_run_worker_loop_survives_process_next_exception(monkeypatch, capl
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_process_next_closes_initialized_provider(monkeypatch):
|
||||
async def test_run_worker_loop_reuses_one_bundle_across_jobs(monkeypatch):
|
||||
"""HIGH-02: the provider client is built once per loop, not once per job."""
|
||||
stop_event = asyncio.Event()
|
||||
seen: list[object] = []
|
||||
closed = False
|
||||
|
||||
class _Sources:
|
||||
@@ -39,9 +43,43 @@ async def test_process_next_closes_initialized_provider(monkeypatch):
|
||||
nonlocal closed
|
||||
closed = True
|
||||
|
||||
services = type("_Services", (), {"sources": _Sources()})()
|
||||
bundle = ServiceBundle(sources=_Sources()) # type: ignore[arg-type]
|
||||
monkeypatch.setattr(
|
||||
"transcription.worker.ServiceBundle.from_session_factory",
|
||||
classmethod(lambda _cls, _factory=None, **_kwargs: bundle),
|
||||
)
|
||||
|
||||
monkeypatch.setattr("transcription.worker.ServiceBundle", lambda: services)
|
||||
async def _fake_process_next_queued_job(*, session=None, session_factory=None, services=None):
|
||||
_ = (session, session_factory)
|
||||
seen.append(services)
|
||||
if len(seen) >= 3:
|
||||
stop_event.set()
|
||||
return False
|
||||
return True
|
||||
|
||||
monkeypatch.setattr("transcription.worker.process_next_queued_job", _fake_process_next_queued_job)
|
||||
|
||||
await run_worker_loop(stop_event=stop_event, poll_interval_seconds=0)
|
||||
|
||||
assert len(seen) == 3
|
||||
assert all(item is bundle for item in seen)
|
||||
assert closed is True
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_process_next_closes_provider_for_the_bundle_it_owns(monkeypatch):
|
||||
closed = False
|
||||
|
||||
class _Sources:
|
||||
async def aclose(self):
|
||||
nonlocal closed
|
||||
closed = True
|
||||
|
||||
bundle = ServiceBundle(sources=_Sources()) # type: ignore[arg-type]
|
||||
monkeypatch.setattr(
|
||||
"transcription.worker.ServiceBundle.from_session_factory",
|
||||
classmethod(lambda _cls, _factory=None, **_kwargs: bundle),
|
||||
)
|
||||
|
||||
async def _no_job(*, services, session):
|
||||
_ = (services, session)
|
||||
@@ -51,3 +89,25 @@ async def test_process_next_closes_initialized_provider(monkeypatch):
|
||||
|
||||
assert await process_next_queued_job() is False
|
||||
assert closed is True
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_process_next_leaves_a_caller_owned_bundle_open(monkeypatch):
|
||||
"""A bundle passed in belongs to the caller and must outlive one job."""
|
||||
closed = False
|
||||
|
||||
class _Sources:
|
||||
async def aclose(self):
|
||||
nonlocal closed
|
||||
closed = True
|
||||
|
||||
bundle = ServiceBundle(sources=_Sources()) # type: ignore[arg-type]
|
||||
|
||||
async def _no_job(*, services, session):
|
||||
_ = (services, session)
|
||||
return False
|
||||
|
||||
monkeypatch.setattr("transcription.worker.process_next_queued_job_workflow", _no_job)
|
||||
|
||||
assert await process_next_queued_job(services=bundle) is False
|
||||
assert closed is False
|
||||
|
||||
Reference in New Issue
Block a user