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]>
zoltan57andCopilot App
John Lancaster