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]>
Review log [55]. Three historical local_timeout rows recorded 0.4-2.0s more
than the configured budget because the measurement window opened before the
provider call.
The plan named two causes, and both were already gone. Diffed against
f86c0ff~1: at V4.6 the window held resolve_provider_input (async;
normalization + artifact write + DB work) and a session.commit(). Phase 1
deleted both. What remains between the clock and the wait_for is
build_provider_input, now pure field copying because normalization moved to
ingest and file_hash is already stored: 6.2 us per call, zero awaits, so it
cannot yield to the event loop.
A third cause was still there and is not in the plan. The regression test
below measured 890ms where ~200ms was expected. services.sources.provider is
a lazy property that appears as an argument expression to _call_transcriber,
so it is evaluated after the clock starts but before wait_for begins timing.
Constructing OpenRouterTranscriptionProvider costs 475ms on first access and
0.001ms after, so the first attempt of every worker process booked half a
second of HTTP client construction as provider latency. That plausibly
accounts for the low end of the historical overshoot.
workflows.py
- Re-capture monotonic_started_at immediately before the wait_for, reusing
the same variable. The pre-loop assignment stays as the fallback: binding
a new name inside the try would leave the general-exception handler
referencing an unbound variable when build_provider_input raises. All
three duration write sites (success, TimeoutError, general failure) then
measure the correct window with no further change.
- Hoist the provider property above the per-source loop. It is
loop-invariant, so this also removes the repeated lookup from the two
evidence-capture sites.
tests/services/test_workflows_reliability.py
test_timeout_duration_excludes_pre_call_setup simulates 400ms of blocking
setup against a 200ms budget and asserts the recorded duration sits near
the budget and well clear of budget+setup. Confirmed to fail on the pre-fix
code (assert 625 < 540) and pass after, so it guards behaviour rather than
restating it. This is the plan's verification criterion as a test.
ui/pages/sources_page.py
_format_duration renders >=1s as "27.6 s" and below that as "612 ms",
replacing the raw "27612 ms". No test asserted the old format.
Plan task 3 (record preprocessing as its own value) declined and logged as a
deviation: after Phase 1 there is no preprocessing left to record, and a
preprocessing_ms column to measure 6 us of attribute copying is complexity
without a reader.
Verified: 293 passed, 4 skipped, 0 ruff, 0 ty.
Co-authored-by: Copilot App <[email protected]>
Decompose SourceService along the aggregate boundary and then correct the
instruction file that caused it to grow, in that order. The refactor is the
empirical test of the rule.
services/evidence.py (new)
EvidenceService owns ExecutionAttempt: read_latest_execution_attempt,
list_execution_attempts, promote_machine_attempt, build_evidence_export,
plus the LatestExecutionAttempt projection. Moved verbatim from sources.py.
services/errors.py (new)
The five-class error hierarchy (PromptLoadError, TranscriptionError,
TranscriptionNotFoundError, SourceDeleteBlockedError,
CandidatePromotionError) moved out of sources.py. evidence.py needs
TranscriptionNotFoundError, and test_service_boundaries.py correctly
rejected the sibling import. errors.py defines no *Service class, so it is
a legal shared home. This was the boundary test doing its job, not an
obstacle to route around.
sources.py 1,389 -> 885 lines (1,063 after Phase 2).
services/__init__.py
ServiceBundle and from_session_factory register evidence. Note that
field-by-field ServiceBundle construction silently binds services to the
process-global session factory via default_factory; from_session_factory is
the only safe constructor. Two test bundles were fixed for this.
.github/instructions/services.instructions.md
Rewritten to describe the boundaries the decomposition actually produced,
per plan Phase 3 task 7 and review log [59].
- "1 service class per data model" -> one service class per aggregate.
The table-shaped rule is the measured cause of sources.py reaching
1,389 lines; DocumentType has no lifecycle without Document.
- New Model Ownership section. Junctions are owned by their lifecycle
owner, the service that creates and deletes the rows: document_person
to PeopleService (sole writer, measured), job_source to SourceService.
Two carve-outs are stated rather than left as silent violations:
cascade deletion when a service deletes its own aggregate root, and
status transitions that create and delete nothing (cancel_job,
resubmit_failed_sources), which are Job lifecycle events on the work
queue. EvidenceService.promote_machine_attempt's two-field write to
Source is named and scoped.
- Mandatory CRUD softened to intent. It was already false: five modules
define no service class, EvidenceService has no create/delete because
ExecutionAttempt is append-only, RegistryService uses <op>_entry.
- Separated reading across models via eager loads from the owning root,
which is allowed, from importing another service, which is not. The old
line 13 and lines 75-77 read as contradictory.
- Typo: picutre.
No code was moved to satisfy the rule.
tests/test_service_boundaries.py
Docstring no longer cites the instruction file by line number; that anchor
would desynchronise silently. errors.py added to the neutral-module list.
Verified: 292 passed, 4 skipped, 0 ruff, 0 ty. All 25 /ui/* routes walked
against the live app; 24x 200. /ui/documents/{id}/sources 404s via a 307 that
drops the /ui prefix, confirmed pre-existing (last touched in 6a3ee26) and
left alone as out of scope.
Co-authored-by: Copilot App <[email protected]>
Jim Lancaster
John Lancaster