Phase 5: contain worker faults instead of discarding the classification

Review log [8]. classify_unexpected_error already returned retriable=False and
the verdict was logged and then thrown away. Measured across src/: retriable
was assigned in 9 places and read in none.

The plan asks for a test that a programming error "does not silently retry".
Probing with an injected AttributeError showed that is not what happens, and
the two real failure modes need different fixes.

Mode A, raised after the claim commits (inside advance_job): raised exactly
once, job left at PROCESSING, retry_count 0, never re-claimed, because
claim_next_queued_job filters status == QUEUED. A permanently stranded job
with one swallowed log line, not a retry. advance_job's PROCESSING branch,
commented "Recover mid-flight jobs", is unreachable from the worker for the
same reason.

Mode B, raised before or during the claim: 20 raises in 1.2s, an unbounded hot
spin at the poll interval. It never reaches the per-job retry machinery, so
WORKER_MAX_RETRIES does not cap it and the plan's 60s worst case understates
this path.

services/workflows.py
  _advance_job_with_containment wraps advance_job. Any escaping exception is
  classified and the job driven to terminal FAILED, which is visible in the UI
  and resubmittable. The caller session is rolled back first and the terminal
  write runs in its own transaction, so it stays atomic even when the failure
  left that session dirty (plan task 3). The loop continues, so one poison job
  cannot halt transcription for every other job.

worker.py
  handle_worker_exceptions re-raises non-retriable faults rather than
  suppressing them; retriable ones are still suppressed so transient
  conditions do not stop work. run_worker_loop catches that, logs CRITICAL and
  returns cleanly. Returning rather than propagating matters: the exception
  would otherwise surface only at app shutdown, through the wait_for in
  worker_consumer_lifespan.

tests
  test_run_worker_loop_survives_process_next_exception asserted the loop
  SURVIVES a RuntimeError and continues, which is the Mode B defect written
  down as an expectation. Replaced by
  test_run_worker_loop_stops_on_non_retriable_exception, with a new
  test_run_worker_loop_survives_retriable_exception so suppression of genuinely
  transient faults stays covered, and
  test_error_after_claim_fails_the_job_instead_of_stranding_it for Mode A.

  All three were verified to fail on pre-fix code. The Mode B guard fails by
  timing out, which is the infinite spin made visible.

Verified: 295 passed, 4 skipped, 0 ruff, 0 ty.

Co-authored-by: Copilot App <[email protected]>
This commit is contained in:
zoltan57
2026-08-18 16:12:52 -05:00
co-authored by Copilot App
parent 110f40a28b
commit fca959fa5d
4 changed files with 163 additions and 5 deletions
+37 -1
View File
@@ -416,10 +416,46 @@ async def process_next_queued_job(
if session is not None:
await session.commit()
await advance_job(job=job, services=services, settings=settings, session=session)
await _advance_job_with_containment(job=job, services=services, settings=settings, session=session)
return True
async def _advance_job_with_containment(
*,
job: Job,
services: ServiceBundle,
settings: Settings | None,
session: AsyncSession | None,
) -> None:
"""Advance a claimed job, guaranteeing it never stays stuck in PROCESSING.
The job has already been committed as PROCESSING at this point, and
``claim_next_queued_job`` only ever selects QUEUED rows. So an exception
escaping ``advance_job`` used to strand the job in PROCESSING permanently,
with a single swallowed log line and no recovery path (review log [8]).
Any escaping exception is therefore classified and the job driven to the
terminal FAILED state, which is visible in the UI and resubmittable. The
terminal write runs in its own transaction so it is atomic even when the
caller's session was left dirty by the failure.
"""
try:
await advance_job(job=job, services=services, settings=settings, session=session)
except Exception as exc:
error = exc if isinstance(exc, AppError) else classify_unexpected_error(exc, operation="worker.advance_job")
logger.exception(
"Job processing failed outside page handling operation=worker.advance_job "
"job_id=%s error_id=%s category=%s retriable=%s",
job.id,
error.error_id,
error.category.value,
error.retriable,
)
if session is not None:
await session.rollback()
await services.jobs.update_job_state(job_id=job.id, status=JobStatus.FAILED)
def _resolve_job_sources(job: Job) -> list[Source]:
"""Resolve pending linked sources for a job in deterministic page order.
+29 -2
View File
@@ -94,16 +94,30 @@ async def worker_consumer_lifespan(
@contextmanager
def handle_worker_exceptions(operation: str = "worker.loop"):
"""Context manager to log and suppress exceptions in the worker loop."""
"""Log worker-loop exceptions, suppressing only the retriable ones.
``classify_unexpected_error`` marks unknown exceptions non-retriable, but that
verdict used to be logged and then discarded, so a programming error raised
before a job could be claimed spun the loop at the poll interval indefinitely.
Nothing capped it, because it never reached the per-job retry machinery
(review log [8]).
A non-retriable fault is a defect rather than a transient condition, so it is
re-raised for the caller to stop on. Faults raised after a job is claimed are
contained at the job level instead, and never reach here.
"""
try:
yield
except Exception as exc:
error = exc if isinstance(exc, AppError) else classify_unexpected_error(exc, operation=operation)
logger.exception(
"Worker loop exception error_id=%s category=%s",
"Worker loop exception error_id=%s category=%s retriable=%s",
error.error_id,
error.category.value,
error.retriable,
)
if not error.retriable:
raise error from exc
async def run_worker_loop(
@@ -121,6 +135,10 @@ async def run_worker_loop(
The service bundle — and with it the provider's pooled HTTP client — is built
once for the lifetime of the loop, so consecutive jobs reuse one connection
instead of paying a fresh TLS handshake each time.
The loop returns early on a non-retriable error raised before a job could be
claimed. Faults raised after a claim are contained by marking that job FAILED,
so a single poison job cannot stop transcription for every other job.
"""
services = ServiceBundle.from_session_factory(session_factory)
try:
@@ -150,6 +168,15 @@ async def run_worker_loop(
if wake_event is None and not processed_any:
await asyncio.sleep(poll_interval_seconds)
except AppError as error:
# Stop rather than spin. A non-retriable fault here means no job could be
# claimed, so there is no row to mark FAILED and no reason to expect the
# next poll to behave differently.
logger.critical(
"Worker loop stopped after a non-retriable error error_id=%s category=%s",
error.error_id,
error.category.value,
)
finally:
await services.aclose()