6.0 KiB
You are working in the transcription repository (Python 3.12+, FastAPI, NiceGUI, SQLModel/SQLAlchemy, Pydantic V2, asyncio). Follow .github/instructions/services.instructions.md and .github/instructions/error-handling.instructions.md for any code you touch, and keep docs/ver4/* as canonical authority for intended behavior. Do not modify unrelated code.
Goal
Implement Phase 1 (Blocking fixes) from docs/architecture-code-review-2026-08-20.md, addressing two findings:
1. [CRIT-01] SQLite queue claims are not atomic
Location: src/transcription/services/jobs.py:172-201 (claim_next_queued_job)
Problem: The claim does SELECT ... WHERE status == QUEUED ORDER BY date_created, id LIMIT 1, then sets job.status = PROCESSING and commits. with_for_update(skip_locked=True) is only applied on PostgreSQL (if _session.get_bind().dialect.name == "postgresql"). On SQLite — the project's default runtime per pyproject.toml/README — two concurrent workers/processes can both read the same QUEUED row before either commit becomes visible, causing the same job to be claimed twice: duplicate provider calls, duplicate ExecutionAttempt history, conflicting terminal writes.
Required fix:
- Replace the select-then-set pattern with a dialect-safe atomic claim for SQLite (and keep it correct for PostgreSQL). Prefer a single atomic
UPDATE ... WHERE id = (subquery selecting the oldest QUEUED row) AND status = 'queued'(optionally withRETURNINGwhere the dialect/driver supports it), so the claim and the status transition happen as one atomic write instead of read-then-write across two statements. IfRETURNINGisn't reliably usable through the SQLModel/SQLAlchemy async session in this codebase, do the atomicUPDATEfirst (checking rowcount == 1 to confirm the claim succeeded), then re-SELECTthe claimed row by id. - Preserve the existing method signature, docstring intent, ordering semantics (oldest
date_created, tie-broken byid), the "no eager loads on the hot poll" comment/behavior, and thesession-scoping pattern (self._session_scope,self._finalize) used elsewhere in this file. - Keep working correctly for both SQLite and PostgreSQL dialects — do not special-case away PostgreSQL's existing
SKIP LOCKEDcorrectness. - Add a concurrency regression test (in the appropriate existing test file for
services/jobs.py, e.g.tests/services/test_jobs.pyor similar — check what already exists) that races two concurrentclaim_next_queued_jobcalls against the same queued job on SQLite and asserts exactly one caller receives it and the other receivesNone(or the next distinct job, if a second job is queued). Useasyncio.gather/TaskGroupwith separate sessions to simulate concurrent claimers, matching existing async test patterns in the repo.
2. [HIGH-01] JobSource uniqueness on (job_id, source_id) is not enforced
Location: docs/ver4/requirements_v4.md:16-21, src/transcription/db/models.py:359-384, src/transcription/services/sources.py:339-361, src/transcription/services/sources.py:519-529
Problem: V4 requires exactly one JobSource row per (job, source) pair. Service code already assumes this (read_job_source_for_job docstring says "the unique JobSource association"; update_job_source_transcription uses .first()), but the JobSource model has no DB-level uniqueness constraint on (job_id, source_id). Duplicate rows can exist, causing writes to silently update the wrong row and making reads/deletes ambiguous.
Required fix:
- Add a
UniqueConstraint("job_id", "source_id")(via__table_args__on theJobSourceSQLModel, consistent with how other constraints/indexes are declared insrc/transcription/db/models.py) enforcing one row per(job_id, source_id)pair. - Add/update the corresponding Alembic migration (check
alembic/or the project's migration directory/tooling — follow whatever migration mechanism this repo already uses; look at recent migration files for the exact style) that creates the unique constraint/index, and includes a pre-migration cleanup step (or a documented manual step) to deduplicate any existing violating rows before the constraint is applied — do not let the migration fail on dirty data without a clear resolution path. Prefer keeping the most recently updated/created row per(job_id, source_id)pair and removing/merging older duplicates, but first inspect howsources.pypicks "the" row today (e.g..first()ordering) so the cleanup logic matches production behavior as closely as possible. - Update
docs/ver4/schema_v4.mdto document the now-enforced(job_id, source_id)uniqueness invariant onJobSource. - Convert any insert path that creates
JobSourcerows into deterministic conflict handling (e.g. catch the resulting integrity error and raise/return the appropriate domain-levelconflicterror per.github/instructions/error-handling.instructions.md, or use an upsert pattern) rather than letting a raw DB integrity error propagate. - Add or extend a test (near
tests/test_service_boundaries.py,tests/services/test_sources*.py, or whereverJobSourcebehavior is currently tested) asserting that attempting to create a secondJobSourcefor an existing(job_id, source_id)pair is rejected/handled deterministically rather than silently succeeding.
Validation
After implementing both fixes:
- Run
pytest(use the project's normal invocation frompyproject.toml, e.g. viauv run pytest) and ensure all tests pass, including the new regression tests. - Run
ruff checkand fix any new lint issues introduced by your changes (do not fix pre-existing unrelated ruff issues). - Run
ty check(or the project's type-checker command) and ensure no new type errors are introduced by your changes. - Do not touch Phase 2+ items (media URL resolver, error taxonomy, provenance env var, ruff/ty baseline cleanup) — those are out of scope for this task.
Report back with: files changed, a summary of the atomic-claim strategy chosen and why, the migration file added, and the final pytest/ruff/ty results.