Phase 1 of docs/reviews/2026-08-23-code-review.md. HIGH-01: process_queued_job committed page evidence and the terminal job status in separate transactions, so a crash between them left a transcript persisted against a job stuck in PROCESSING that the worker never reclaims. The final page's write is now deferred into _finalize_batch_outcome so it shares the terminal transaction. Intermediate pages remain individually durable, and the terminal commit is shielded against cancellation the same way per-page writes already were. HIGH-04: added tests/integration/test_pipeline_atomicity.py covering both Transaction B and Transaction C. Confirmed failing against the previous implementation before the fix. HIGH-03: classify_unexpected_error interpolated the raw exception into AppError.message, which the UI renders and the API serializes, leaking the database path from OperationalError. message is now generic. Because message also feeds format_error_detail, which writes evidence records, the root cause is preserved on a new internal-only AppError.detail field rather than discarded. HIGH-02: replaced 8 hand-rolled ui.notify error calls in home_page and people_page with error_presenter.show_error, restoring the correlation error_id, canonical category, and suggestion. Added an AST guard to test_ui_boundaries.py so pages cannot hand-roll error notifications again. Docs updated per documentation-sync: the message/detail split in docs/error_handling.md and the multi-page atomicity rule in services.instructions.md. Verification: ruff clean, 381 tests passing, ty unchanged at 10 known SQLAlchemy descriptor false positives. Co-authored-by: Copilot App <[email protected]>
9.4 KiB
description, applyTo
| description | applyTo |
|---|---|
| Follow these guidelines when editing the services | src/transcription/services/*.py |
Services
Structure
- Project core data models are defined in models
- One service class per aggregate, not per table. An aggregate is a root model plus
the models that have no independent lifecycle of their own.
DocumentTypehas no meaning withoutDocument, so it belongs toDocumentService; it does not get its own service. Splitting per table produces services that must reach across each other for every real operation, which is what line 13 forbids. - Only services interact with the database, and only through async methods.
- A service module must not import another service module. This is enforced by test_service_boundaries. Shared types go in a neutral module that defines no service class (see errors).
- Not every module in this package is a service. Helper modules that define no
*Serviceclass (base,errors,normalization,prompts,quality,media_storage,source_media) are free-function modules and are exempt from the service rules below. - Cross-cutting error behavior must follow error-handling instructions.
Model Ownership
Every model has exactly one owning service. The owner defines that model's invariants and is the only service that may create or delete its rows.
| Model | Owner |
|---|---|
Document, DocumentType |
DocumentService |
Source, JobSource |
SourceService |
Job |
JobService |
Person, PersonRole, DocumentPerson |
PeopleService |
ExecutionAttempt |
SourceService |
Junction tables
A junction table is owned by the service that creates and deletes its rows — its
lifecycle owner. The service on the other side may read through the junction (via
selectinload) but must not create rows in it.
document_person->PeopleService. Every write is there;DocumentServiceonly eager-loads through it.job_source->SourceService, which creates the row, records each page's outcome, and deletes it.
Two consequences follow, and both are deliberate:
- Cascade deletion is not a violation. A service deleting the aggregate root it owns
may delete junction rows referencing that root, because they cannot outlive it
(
JobService.delete_job_with_guardrails). - Ownership governs creation and deletion, not every state transition.
job_sourceis both a link and the transcription work queue.JobService.cancel_jobandresubmit_failed_sourcestransitionjob_source.statusacross a whole job, because that transition is a Job lifecycle event, not a per-page outcome. They create and delete nothing.
EvidenceService is read-focused and projection-focused. It may coordinate selection
flows, but append-only attempt creation remains in SourceService write paths.
If a new operation cannot be expressed within one owner, it belongs in an orchestration module, not in a cross-service import.
Error Handling
- Errors used by a single service are defined at the top of that module and inherit from
AppError. - Errors shared by more than one service go in errors, which defines no service class and is therefore importable by any of them.
- Use a context manager for large
try/exceptblocks, likehandle_transcription_errorsin sources. - Category mapping, retry behavior, and translation boundaries are defined in error-handling instructions.
- Service-edge exception translation must be deterministic: map to canonical categories and preserve clear provider->service->API/UI boundaries.
Checklist
- Uses
ServiceBasefor common logic - Session kwarg for
AsyncSessionto pass a session object into each method - Services use
self._session_scopein their methods to pass the session through- Multiple operations on the same object(s) require sharing a session between all the methods used
- Every model the module touches is either owned by it or reached read-only
- Evidence writes preserve append-only semantics
CRUD Methods
- Name format
<operation>_<model>, for examplecreate_documentorupdate_job. - Where a service exposes create/read/update/delete for its root model, define them at the top of the class in that order, before derived reads and workflow helpers.
- Not every aggregate needs all four.
ExecutionAttemptis append-only evidence written bySourceServiceworkflow-facing methods, soEvidenceServicedeliberately exposes reads and no create or delete. Do not add unused CRUD methods to satisfy symmetry. RegistryServiceis generic across small lookup models and uses<operation>_entrynaming instead.
Transaction Finalization
When a service method accepts an optional session kwarg, write methods must use self._finalize to finalize the transaction properly according to whether or not they are sharing a session.
- If
sessionisNone: the method owns the transaction and shouldcommit(). - If
sessionis provided: the method must not commit; it shouldflush()so IDs and FK values are available to the caller's transaction. - Use
refresh()on returned ORM objects when the caller needs DB-populated values (defaults, triggers, merged state).
Workflow Transaction Boundaries
For multi-step job lifecycles, orchestration functions must use explicit transaction phases.
- Transaction A (claim): transition
JobStatus.QUEUED -> JobStatus.PROCESSINGand commit immediately. - Perform provider/network work outside database transactions.
- Transaction B (terminal success): write transcript content and set
JobStatus.TRANSCRIBEDin the same shared-session commit. - Transaction B (terminal failure): write transcript error detail and set
JobStatus.FAILEDin the same shared-session commit. - Transaction C (retry path): write transcript error detail, increment retry count, and set
JobStatus.QUEUEDin one shared-session commit.
Atomicity rules:
- Never commit transcript updates separately from the paired terminal/retry job status change.
- Terminal state (
TRANSCRIBEDorFAILED) and transcript row changes must succeed or roll back together. - Retry persistence (
QUEUED+ retry increment + error detail) must succeed or roll back together.
Multi-page batches
These two requirements are in tension for multi-page jobs: each page should be durable as
soon as its provider call returns, but the last page must commit together with the terminal
status. process_queued_job resolves it by committing every page except the last one
individually, then deferring the final page's write into _finalize_batch_outcome so it
shares the terminal transaction.
Both paths are shielded against cancellation, so the final page is no less durable than the
pages before it. Enforced by tests/integration/test_pipeline_atomicity.py; per-page
durability is separately enforced by
tests/services/test_workflows_reliability.py::TestWorkflowReliability::test_transcribed_page_is_committed_before_next_provider_call_finishes.
Contract Alignment
- Treat
docs/as the active architecture and requirements baseline. - Legacy revision trees are out of scope for active implementation decisions and must not be referenced as authoritative service guidance.
- Treat
src/transcription/db/models.pyas runtime schema ground truth anddocs/schema.mdas the field-accurate contract mirror. Job.statussuccess path isTRANSCRIBED.JobSource.statusis queue/projection state only (PENDING,TRANSCRIBED,FAILED,CANCELLED).- Source ingest may normalize media before persistence; persisted bytes/hash are canonical for processing and provenance.
ExecutionAttemptis append-only evidence history; do not mutate historical attempt rows in runtime code.Source.raw_transcriptionis a projection, not authoritative history.- Service/UI read paths that touch relationships must be eager-loaded for
lazy="raise"compatibility. - If model fields, enums, constraints, indexes, or relationship-loading semantics change, update
docs/schema.mdin the same change. - If
Settingsfields or defaults change insrc/transcription/config.py, update.env.examplein the same change so keys/defaults remain synchronized and no stale settings remain documented.
Schema Drift and Legacy Compatibility Policy
- Prefer schema migration over startup reconciliation or runtime compatibility paths in service writes.
- Do not add legacy read/write compatibility code in service workflows by default.
- If drift is discovered and a migration decision is ambiguous (for example, one-way destructive DDL, uncertain data retention impact, or unknown deployment sequence), pause and ask the user to choose migration vs compatibility before coding.
- If a temporary compatibility path is explicitly approved, document an expiration/removal plan in the same change.
Service Composition
A service method may read across models it does not own, using eager loads from its own aggregate root. What it may not do is import another service.
Operations that must write models owned by more than one service are composed in an orchestration module (store, workflows).