generated from john/python-template
380 lines
45 KiB
Markdown
380 lines
45 KiB
Markdown
# Architecture & Code Review Report
|
|
|
|
**Repository Target:** `C:\GitHub\transcription\`
|
|
**Target Stack:** Python 3.12+ | FastAPI | NiceGUI | SQLModel/SQLAlchemy | Pydantic V2 | asyncio | OpenRouter
|
|
**Review Date:** 2026-08-17
|
|
**Baseline verified:** `pytest` → 264 passed, 4 skipped. `ruff check` → 6 errors. `ty check` → 197 diagnostics.
|
|
|
|
---
|
|
|
|
## 1. Executive Summary
|
|
|
|
- **The codebase is disciplined and unusually well-structured for its size (~9k src LOC).** Error taxonomy (`errors.py`), evidence capture (`providers/evidence.py`), transaction-ownership helpers (`ServiceBase._finalize`), and CSS/asset discipline in the UI are genuinely strong and should be preserved.
|
|
- **Top risk is the job-claim path.** `JobService.read_next_queued_job` (`services/jobs.py:170-187`) selects *every* `QUEUED` job with three levels of eager loading, has no `LIMIT`, no `FOR UPDATE SKIP LOCKED`, and no compare-and-swap on the status transition. The code even carries a comment acknowledging the race (`services/workflows.py:193-194`) without fixing it.
|
|
- **Second risk is model-level eager loading.** Nearly every `Relationship` in `db/models.py` sets `lazy="selectin"` on *both* sides of bidirectional links (`Document.jobs` ↔ `Job.document`, `Job.job_sources` ↔ `JobSource.job`, `JobSource.source` ↔ `Source.job_sources`). Reading one `Job` cascades into loading effectively the whole related graph, and it makes every explicit `selectinload()` in the services redundant.
|
|
- **No index exists on `Job.status` or `Job.date_created`**, yet the worker polls `WHERE status='queued' ORDER BY date_created` once per second. Every poll is a full table scan.
|
|
- **`worker_provider_timeout_seconds` is hard-capped at `le=20.0`** (`config.py:110`). Vision transcription of a full document page routinely exceeds 20s; this cap makes systematic timeouts unconfigurable-away.
|
|
- **The provider HTTP client is destroyed and rebuilt for every single job** (`worker.py:157-174`), defeating connection pooling and TLS session reuse on the hottest path.
|
|
- **`app_state.py` is dead code containing a guaranteed `TypeError`** (verified at runtime): `resolve_session_factory` calls `get_session_factory()` with no arguments. `@functools.cache` erases the signature, so `ty` cannot see it.
|
|
- **`ty` is configured as a dev dependency but is not usable as a gate.** 197 diagnostics, ~160 of which are SQLModel relationship false positives already suppressed with `# pyright: ignore` comments that `ty` does not honor.
|
|
- **Schema evolution is hand-rolled** in `db/operations.py` with raw `ALTER TABLE`/`CREATE INDEX IF NOT EXISTS` and a SQLite-shaped `CHAR(32)` UUID column. There is no Alembic. Postgres portability is claimed but not actually exercised.
|
|
- **Meaningful duplication exists in the UI layer** (~500 lines): media-URL resolution, `_parse_uuid`, settings resolution, delete-confirmation scaffolds, and hand-rolled tables are each reimplemented 3-5 times.
|
|
|
|
---
|
|
|
|
## 2. Findings by Severity
|
|
|
|
### Critical Severity
|
|
|
|
#### [CRIT-01] Queued-job claim has no row lock, no CAS, and no LIMIT — duplicate processing and full-queue load
|
|
- **Location:** `src/transcription/services/jobs.py:170-187`; claim logic at `src/transcription/services/workflows.py:188-196`; divergent duplicate at `src/transcription/db/operations.py:143-151`
|
|
- **Problem & Consequence:** `read_next_queued_job` issues `SELECT ... WHERE status = 'queued' ORDER BY date_created, id` with `selectinload(Job.document)` and `selectinload(Job.job_sources).selectinload(JobSource.source)` — and **no `.limit(1)`**. It materializes the entire queue plus its document/job_source/source graph on every worker tick just to call `.first()`. With a backlog of N jobs this is O(N) rows and several extra SELECT round-trips per second.
|
|
|
|
Worse, the claim is a read-then-write with no atomicity: `process_queued_job` reads status `QUEUED`, then separately calls `mark_job_status(job.id, PROCESSING)`. Two workers (or an app replica plus the in-process worker) can both read the same row as `QUEUED` and both transcribe it — double provider spend and duplicate `ExecutionAttempt` evidence rows. The comment at `workflows.py:193-194` explicitly names this hazard ("otherwise other workers may see the job as still QUEUED") but the committed fix only narrows the window rather than closing it.
|
|
|
|
Note also that `db/operations.py:get_next_queued_job` is a second, *different* implementation of the same concept that *does* have `.limit(1)`. The worse implementation is the live one.
|
|
- **Recommendation:** Replace the read-then-write with a single atomic claim, and delete the duplicate.
|
|
```python
|
|
# Before (services/jobs.py) — no limit, no lock
|
|
query = select(Job).options(...).where(Job.status == JobStatus.QUEUED).order_by(Job.date_created, Job.id)
|
|
return (await _session.exec(query)).first()
|
|
|
|
# After — atomic claim, one row, dialect-aware
|
|
async def claim_next_queued_job(self, *, session=None) -> Job | None:
|
|
async with self._session_scope(session) as s:
|
|
stmt = (
|
|
select(Job)
|
|
.where(Job.status == JobStatus.QUEUED)
|
|
.order_by(Job.date_created, Job.id)
|
|
.limit(1)
|
|
)
|
|
if s.bind.dialect.name == "postgresql":
|
|
stmt = stmt.with_for_update(skip_locked=True)
|
|
job = (await s.exec(stmt)).first()
|
|
if job is None:
|
|
return None
|
|
job.status = JobStatus.PROCESSING
|
|
job.date_updated = datetime.now(UTC)
|
|
await self._finalize(session=s, caller_session=session, refresh=(job,))
|
|
return job
|
|
```
|
|
Load the eager relationships in a *second* query after the claim succeeds, so the hot poll stays a single narrow row. On SQLite, wrap the claim in `BEGIN IMMEDIATE` or accept single-worker-only and document it.
|
|
- **Effort:** M
|
|
|
|
#### [CRIT-02] Bidirectional `lazy="selectin"` on every relationship causes cascading read amplification
|
|
- **Location:** `src/transcription/db/models.py:71-73, 89-91, 108-115, 160-168, 211-212, 269-276, 332-333`
|
|
- **Problem & Consequence:** Every `Relationship` in the domain model sets `sa_relationship_kwargs={"lazy": "selectin"}`, including both sides of each pair. Fetching a single `Job` triggers: `Job` → `Job.document` → `Document.jobs` (all jobs for that document) → `Document.sources` → `Document.document_people` → `DocumentPerson.person` / `.role_ref` → each `Job.job_sources` → `JobSource.source` → `Source.job_sources` → … SQLAlchemy's identity map prevents infinite recursion but does **not** prevent the extra SELECT round trips per level.
|
|
|
|
Two concrete consequences: (a) the per-second worker poll is far more expensive than it appears from reading `jobs.py`; (b) the dozens of explicit `selectinload(...)` options in `documents.py`, `jobs.py`, `sources.py`, and `people.py` are dead weight — the relationship default already does it — and they are the source of ~160 of the 197 `ty` diagnostics.
|
|
- **Recommendation:** Flip the model default to `lazy="raise"` (or `"noload"`, as already correctly done for `Source.processing_artifacts` at `models.py:279` and `JobSource.execution_attempts` at `models.py:336`) and rely on the per-query `selectinload()` that services already declare. `lazy="raise"` converts silent N+1 into a loud test failure and would prove which eager loads are actually needed.
|
|
```python
|
|
# models.py
|
|
jobs: list["Job"] = Relationship(back_populates="document", sa_relationship_kwargs={"lazy": "raise"})
|
|
```
|
|
Roll out per-model with the existing test suite as the safety net; the suite already covers the read paths.
|
|
- **Effort:** M
|
|
|
|
### High Severity
|
|
|
|
#### [HIGH-01] `app_state.py` is unreferenced dead code containing a guaranteed `TypeError`
|
|
- **Location:** `src/transcription/app_state.py:29-34`; the called function at `src/transcription/db/session.py:20-26`
|
|
- **Problem & Consequence:** `resolve_session_factory` falls back to `get_session_factory()` with no arguments, but the signature is `get_session_factory(database_url: str)`. Verified at runtime:
|
|
```
|
|
TypeError: get_session_factory() missing 1 required positional argument: 'database_url'
|
|
```
|
|
`@functools.cache` wraps the function in a `_lru_cache_wrapper`, which erases the signature — so `ty check src\transcription\app_state.py` reports "All checks passed". The whole module has **zero importers** anywhere in `src`, `tests`, or `tools`, so the bug is currently latent; anyone wiring this helper up hits an immediate crash on the fallback path.
|
|
- **Recommendation:** Delete `app_state.py`. Its three live behaviors already exist elsewhere (`db/session.py:resolve_session_factory`, `db/runtime.py:get_database_runtime`, `worker.py:resolve_worker_notifier`). If retained instead, fix the fallback to `resolve_session_factory()` from `db.session`, and add a typed non-cached wrapper around cached functions so type checkers keep the signature.
|
|
- **Effort:** S
|
|
|
|
#### [HIGH-02] Provider HTTP client is rebuilt and torn down once per job
|
|
- **Location:** `src/transcription/worker.py:148-174` (`finally: await services.sources.aclose()`), driven by the tight inner loop at `src/transcription/worker.py:134-142`; client construction at `src/transcription/providers/openrouter.py:197-201`
|
|
- **Problem & Consequence:** `process_next_queued_job` constructs a fresh `ServiceBundle` per call and unconditionally closes the provider in `finally`. Since `workflows.py:243` accesses `services.sources.provider`, a new `httpx.AsyncClient` + `OpenRouter` SDK client is created and destroyed for **every job**. This throws away the connection pool and forces a full TLS handshake per job — added latency on the single most latency-sensitive path, plus churn of file descriptors during backlog drain.
|
|
- **Recommendation:** Hoist the `ServiceBundle` to worker-loop scope (or reuse `app.state.services`, which the lifespan already builds at `app.py:45-50`) and close the provider once at loop shutdown.
|
|
```python
|
|
# worker.py — before
|
|
async def process_next_queued_job(...):
|
|
services = ServiceBundle(...)
|
|
try: ...
|
|
finally: await services.sources.aclose()
|
|
|
|
# after: build once in run_worker_loop / lifespan, pass in, close in the lifespan finally
|
|
async def run_worker_loop(*, services: ServiceBundle, ...):
|
|
try:
|
|
while True: ... await process_next_queued_job(services=services, ...)
|
|
finally:
|
|
await services.sources.aclose()
|
|
```
|
|
- **Effort:** M
|
|
|
|
#### [HIGH-03] Provider timeout is capped at 20 seconds by configuration
|
|
- **Location:** `src/transcription/config.py:110`
|
|
- **Problem & Consequence:** `worker_provider_timeout_seconds: float = Field(default=20.0, gt=0.0, le=20.0)`. The `le=20.0` bound makes 20s both the default *and* the maximum. `workflows.py:238-248` wraps the provider call in `asyncio.wait_for(..., timeout=that_value)`. Multi-modal transcription of a full-page historical document commonly exceeds 20s; operators cannot raise the ceiling without editing source. Every such job fails with `failure_phase="local_timeout"`, and with `worker_max_retries` defaulting to `0` (`config.py:108`) it fails permanently on the first attempt.
|
|
|
|
Compounding this, `httpx.AsyncClient(follow_redirects=True)` at `openrouter.py:198` sets no explicit `timeout`, so it inherits httpx's 5-second default for connect/read/write/pool unless the OpenRouter SDK overrides it.
|
|
- **Recommendation:** Remove the `le=20.0` cap (keep `gt=0.0`), raise the default to something realistic (120s), and set an explicit `httpx.Timeout` derived from the same setting so the transport and the `wait_for` agree.
|
|
```python
|
|
worker_provider_timeout_seconds: float = Field(default=120.0, gt=0.0)
|
|
# openrouter.py
|
|
httpx.AsyncClient(follow_redirects=True, timeout=httpx.Timeout(settings.worker_provider_timeout_seconds))
|
|
```
|
|
- **Effort:** S
|
|
|
|
#### [HIGH-04] No index on the columns the worker polls every second
|
|
- **Location:** `src/transcription/db/models.py:171-212` (`Job.status`, `Job.date_created`, `Job.document_id` all lack `index=True`); also `Source.document_id:252`, `JobSource.job_id:313`, `JobSource.source_id:314`
|
|
- **Problem & Consequence:** The worker executes `WHERE status = 'queued' ORDER BY date_created` once per second (`worker.py:130`, `jobs.py:183-185`). Without a composite index this is a full scan plus sort on every tick, and it grows linearly with total job history — not with queue depth. The `JobSource` foreign keys are joined on every job read; PostgreSQL does not auto-index FKs.
|
|
- **Recommendation:** Add a composite index for the poll and plain indexes on the hot FKs.
|
|
```python
|
|
class Job(SQLModel, table=True):
|
|
__table_args__ = (Index("ix_job_status_date_created", "status", "date_created"),)
|
|
document_id: UUID = Field(foreign_key="document.id", index=True)
|
|
```
|
|
Note these must also be added to the hand-rolled upgrade path in `db/operations.py` (see [HIGH-05]).
|
|
- **Effort:** S
|
|
|
|
#### [HIGH-05] Hand-rolled schema migrations with SQLite-shaped DDL block the claimed Postgres support
|
|
- **Location:** `src/transcription/db/operations.py:25-109`
|
|
- **Problem & Consequence:** Schema evolution is a chain of `_upgrade_*` functions issuing raw `ALTER TABLE` / `CREATE INDEX IF NOT EXISTS` against whatever database is present, executed inside `create_all()`. Specific defects:
|
|
- `operations.py:77` adds `preferred_execution_attempt_id CHAR(32)` — but the model declares it a `UUID` FK to `execution_attempt.id` (`models.py:260-264`). On PostgreSQL this creates a `char(32)` column that will not compare or join against a native `uuid` column, and the declared foreign key is never created at all.
|
|
- Every upgrade is unversioned and re-inspected on each startup; there is no down path, no history table, and no way to tell whether a production database is current.
|
|
- `asyncpg` and `psycopg2-binary` are both dependencies (`pyproject.toml:17,21`) and `JSONBCompat` (`models.py:27-35`) carefully supports JSONB, so Postgres is clearly an intended target — but no test exercises it. All 264 tests run on SQLite.
|
|
- **Recommendation:** Adopt Alembic. Generate an initial revision from current metadata, convert the three `_upgrade_*` functions into explicit revisions, and keep `create_all()` for the test/dev bootstrap path only (`Settings.should_bootstrap_schema` already gates this correctly at `config.py:140-145`). At minimum, immediately fix the `CHAR(32)` type to match the model.
|
|
- **Effort:** L
|
|
|
|
#### [HIGH-06] `ty` is a configured dev tool but produces 197 diagnostics and cannot gate CI
|
|
- **Location:** `pyproject.toml:38`; suppression comments throughout, e.g. `src/transcription/services/jobs.py:67-68,103-104,123-124,156,180-181`
|
|
- **Problem & Consequence:** The project pins `ty` as its type checker, but the codebase suppresses SQLModel relationship typing with `# pyright: ignore[reportArgumentType]` — a *pyright* directive that `ty` does not honor. Result: `ty check` emits 197 diagnostics (160 `invalid-argument-type`, 18 `unresolved-attribute`, 13 `not-subscriptable`), so nobody can run it as a gate, and genuine errors hide in the noise. Two real bugs are buried in there:
|
|
- `tests/ui/test_sources_page.py:25` — `Source(...)` constructed without the required `document_id`.
|
|
- `tools/run_destructive_tests.py:76,80` — `fcntl` is imported and used, but `fcntl` does not exist on Windows, which is this project's development platform.
|
|
- **Recommendation:** Pick one checker and commit to it. If `ty`: replace `# pyright: ignore[...]` with `# ty: ignore[...]`, or better, eliminate the root cause by adopting [CRIT-02]'s `lazy="raise"` change plus typed column accessors, which removes most `selectinload` diagnostics outright. Then wire `ty check` into pre-commit (`pre-commit` is already a dev dependency at `pyproject.toml:35`).
|
|
- **Effort:** M
|
|
|
|
#### [HIGH-07] UI pages own persistence and ORM-loader concerns (violates `ui.instructions.md`)
|
|
- **Location:** `src/transcription/ui/pages/jobs_page.py:17,185-192`; `src/transcription/ui/pages/sources_page.py:13,439`; `src/transcription/ui/components/document_panzoom.py:12,61,65`
|
|
- **Problem & Consequence:** `ui.instructions.md` states pages must not import sessions or manage transactions, and components must not resolve app state. Three violations:
|
|
- `jobs_page.py` imports `transcription.db.session.session_scope` and manages the session lifecycle itself around `create_job_for_document`, while every sibling call site goes through a service.
|
|
- `sources_page.py:439` imports `sqlalchemy.inspect` and reads `inspect(attempt).unloaded` to decide rendering — the presentation layer is now coupled to the loader strategy, and will silently misbehave if a service changes its deferred columns.
|
|
- `document_panzoom.py` calls `get_settings()` inside a component and re-implements upload-path resolution.
|
|
- **Recommendation:** Add a `JobService`/workflow method that owns `session_scope` internally; have `SourceService` return a plain `transport_body_deferred: bool` flag on a read model; pass a ready media URL into `document_panzoom` (or delete it — it is exported from `components/__init__.py` but used by no page).
|
|
- **Effort:** M
|
|
|
|
### Medium Severity
|
|
|
|
#### [MED-01] Blocking filesystem and CPU work on the async event loop
|
|
- **Location:** `src/transcription/services/store.py:363`; `src/transcription/services/people.py:623`; `src/transcription/services/sources.py:908-923,941,1297,1354`; `src/transcription/services/normalization.py:52-101`; `src/transcription/services/prompts.py:138,162,173-176`; `src/transcription/ui/homepage_store.py:22,28,41`; `src/transcription/ui/pages/home_page.py:87,92`; `src/transcription/ui/pages/people_page.py:495`
|
|
- **Problem & Consequence:** All media persistence and artifact I/O is synchronous, called from `async def` paths. `_write_external_artifact` (`sources.py:908`) additionally calls `os.fsync()`, which can block for tens of milliseconds. `normalize_orientation` (`normalization.py:52`) runs full Pillow decode/transpose/re-encode at `quality=95, subsampling=0` inline — that is CPU-bound work measured in hundreds of milliseconds for a scanned page. Every one of these stalls the single event loop shared by the FastAPI API, all NiceGUI clients, and the worker.
|
|
- **Recommendation:** Route blocking work through `asyncio.to_thread` at the service boundary (one wrapper per operation, not per call site). For NiceGUI handlers, `nicegui.run.io_bound` / `run.cpu_bound` are the idiomatic equivalents. `normalize_orientation` is the highest-value single conversion.
|
|
- **Effort:** M
|
|
|
|
#### [MED-02] Dead configuration surface: three settings are defined and tested but never read
|
|
- **Location:** `src/transcription/config.py:99` (`sqlite_check_same_thread`), `:109` (`worker_retry_backoff_seconds`)
|
|
- **Problem & Consequence:** `sqlite_check_same_thread` is never read — `engine.py:43` hardcodes `{"check_same_thread": False}`. `worker_retry_backoff_seconds` is never read either; `tests/test_config.py:152` asserts its default, which gives false confidence that backoff exists. `services.instructions.md:59` mandates a retry path with backoff, and `workflows.py:159-169` implements the `FAILED → QUEUED` transition, but nothing ever sleeps between attempts. Additionally, `advance_job` is invoked exactly once per `process_next_queued_job` call, so a job that transitions `FAILED → QUEUED` is only retried on a later poll — a documented behavior that reads as accidental.
|
|
- **Recommendation:** Either wire `worker_retry_backoff_seconds` into the retry scheduler (a `next_attempt_at` column filtered in the claim query is the correct shape — sleeping in the worker loop would stall all other jobs) or delete both settings and their tests. Honor `sqlite_check_same_thread` in `engine.py:43` or remove it.
|
|
- **Effort:** S
|
|
|
|
#### [MED-03] Runtime `inspect.signature` and `getattr` duck-typing at the provider boundary
|
|
- **Location:** `src/transcription/services/sources.py:1237-1238,1242-1244`; `src/transcription/services/sources.py:152-154`; `src/transcription/services/workflows.py:297-302`
|
|
- **Problem & Consequence:** The `TranscriptionProvider` Protocol (`providers/base.py:102-117`) already declares `requested_model` as a parameter, yet `sources.py:1237` re-checks for it at runtime via `inspect.signature(adapter.transcribe).parameters` on **every transcription call**, then builds an untyped `dict` of kwargs. Similarly, `aclose` and `current_request_manifest` / `current_transport_evidence` are accessed via `getattr(..., None)` even though they are part of the de-facto contract. This defeats static checking on the most important interface in the system, adds per-call reflection overhead, and means a provider that silently drops `requested_model` fails only at runtime.
|
|
- **Recommendation:** Extend the Protocol to declare `aclose()`, `current_request_manifest`, and `current_transport_evidence`; then call `adapter.transcribe(...)` with real keyword arguments and drop the `inspect` import.
|
|
```python
|
|
class TranscriptionProvider(Protocol):
|
|
current_request_manifest: RequestManifest | None
|
|
current_transport_evidence: TransportEvidence | None
|
|
async def transcribe(self, *, prompt_text: str, ..., requested_model: str | None = None) -> TranscriptionResult: ...
|
|
async def aclose(self) -> None: ...
|
|
```
|
|
- **Effort:** S
|
|
|
|
#### [MED-04] `@cache` on `get_settings(**kwargs)` and on engine/session factories creates cross-test and cross-tenant coupling
|
|
- **Location:** `src/transcription/config.py:148-151`; `src/transcription/db/engine.py:39-55`; `src/transcription/db/session.py:20-26`
|
|
- **Problem & Consequence:** `get_settings(**kwargs: Any)` is `@cache`-decorated with arbitrary keyword arguments — any unhashable value raises `TypeError`, and the cache key is the kwargs tuple, so `get_settings()` and `get_settings(environment="test")` return different singletons. More seriously, `dispose_engine(database_url)` (`engine.py:50-55`) calls `get_engine.cache_clear()`, which evicts **all** cached engines, not just the one being disposed; a multi-database process would silently lose its other engines' pools. The same pattern applies to `dispose_session_factory` (`session.py:48-50`).
|
|
- **Recommendation:** Replace the caches with an explicit registry keyed by URL that supports targeted eviction. `db/runtime.py` already models lifespan-owned resources correctly — extend that pattern rather than layering `functools.cache` beneath it. Separately, drop `**kwargs` from `get_settings` and keep it a true zero-argument singleton.
|
|
- **Effort:** M
|
|
|
|
#### [MED-05] Dead compatibility aliases and a three-way import path for one function
|
|
- **Location:** `src/transcription/services/store.py:35,382-383`; `src/transcription/services/transcription.py:12,36`; imports at `store.py:26`, `workflows.py:42`, `sources.py:1263`
|
|
- **Problem & Consequence:** `build_prompt_execution` is defined in `sources.py:1263` and imported through three different paths: `store.py` uses `from .transcription import build_prompt_execution`, `workflows.py` uses `from .sources import ...`, and `tests/test_prompts.py:12` uses a third. `transcription.py` (41 lines) exists solely as a re-export shim. Alongside it, `UploadError = SourceStorageError` (`store.py:35`), `create_upload_job = create_document_job` (`store.py:382`), and `store_file = store_source_file` (`store.py:383`) are aliases with zero remaining callers.
|
|
- **Recommendation:** Delete the three aliases and the `transcription.py` shim; standardize all imports on `services.sources`.
|
|
- **Effort:** S
|
|
|
|
#### [MED-06] `ServiceBundle` default factories construct four services against global settings
|
|
- **Location:** `src/transcription/services/__init__.py:15-22`; consumed at `src/transcription/worker.py:157-158`
|
|
- **Problem & Consequence:** `ServiceBundle` declares `field(default_factory=DocumentService)` for all four services. Instantiating `ServiceBundle()` therefore calls `get_settings()` and `resolve_session_factory()` four times, binding to process-global state. `worker.py:157` takes exactly this path whenever `session_factory is None`. This is the "global singleton instead of injected dependency" pattern the FastAPI DI system exists to avoid, and it makes the worker's database target implicit.
|
|
- **Recommendation:** Remove the default factories and require explicit construction, plus a single `ServiceBundle.from_session_factory(factory, settings)` classmethod — which also removes the four-way duplication of the same construction block at `app.py:45-50` and `worker.py:160-165`.
|
|
- **Effort:** S
|
|
|
|
#### [MED-07] Unused `asyncio.Queue` allocated in every service instance
|
|
- **Location:** `src/transcription/services/base.py:20,26,30`
|
|
- **Problem & Consequence:** `ServiceBase.__init__` does `self.queue = queue or asyncio.Queue()`. No code anywhere reads `self.queue`. The annotation is the unparameterized `asyncio.Queue`. Constructing an `asyncio.Queue` also binds to the running event loop policy, so building a `ServiceBundle` outside a loop is a latent hazard, and per [MED-06] this happens four times per bundle.
|
|
- **Recommendation:** Delete the `queue` attribute and constructor parameter.
|
|
- **Effort:** S
|
|
|
|
#### [MED-08] Exception swallowed to `None` in an ORM model property
|
|
- **Location:** `src/transcription/db/models.py:220-233`
|
|
- **Problem & Consequence:** `Job.filename` reaches into `job_source.__dict__` to dodge lazy loading, then catches `DetachedInstanceError` *and* bare `Exception` (`models.py:227`), returning the string `"unknown"`. Any genuine error — a corrupted row, a mapper misconfiguration — is silently rendered as "unknown" in the UI with no log line. The workaround exists only because of the eager-loading design in [CRIT-02].
|
|
- **Recommendation:** Remove the property from the model and compute the display value in the feature table read model (`ui/components/table/jobs.py`), which is where `ui.instructions.md` says presentation formatting belongs. If it stays, drop the bare `except Exception` and log the `DetachedInstanceError` case.
|
|
- **Effort:** S
|
|
|
|
#### [MED-09] Large inline SVG asset embedded in a Python module
|
|
- **Location:** `src/transcription/ui/theme.py:36-40` (single 23,317-character line)
|
|
- **Problem & Consequence:** `VIBESCRIBE_LOGO_SVG` is a 23KB string literal inside a Python source file. It trips `ruff`'s `line-too-long`, makes the module unreadable and undiffable, and contradicts `ui.instructions.md`'s rule that static assets live under `ui/static/` and be read via `importlib.resources`. The project already has exactly the right helper for this — `ui/resources.py:10-19`'s cached `importlib.resources` reader.
|
|
- **Recommendation:** Move to `ui/static/vibescribe_logo.svg` and load it through a `read_svg` sibling of the existing `read_css`.
|
|
- **Effort:** S
|
|
|
|
### Low Severity
|
|
|
|
#### [LOW-01] `ruff check` fails on 6 issues, 5 auto-fixable
|
|
- **Location:** `src/transcription/ui/theme.py:38,40`; `tests/test_app.py:23`; `tests/ui/test_upload_page.py:44`; plus 2 others
|
|
- **Recommendation:** Run `ruff check --fix`; the only non-trivial one is the SVG line, addressed by [MED-09].
|
|
- **Effort:** S
|
|
|
|
#### [LOW-02] Stale path reference in project instructions
|
|
- **Location:** `.github/instructions/services.instructions.md:10`
|
|
- **Problem:** Points to `src/transcription/models.py`; the actual location is `src/transcription/db/models.py`.
|
|
- **Effort:** S
|
|
|
|
#### [LOW-03] `list_jobs` accepts and discards a parameter
|
|
- **Location:** `src/transcription/services/jobs.py:113-120` (`_ = load_docs`)
|
|
- **Problem:** A dead parameter kept alive only to satisfy `ARG` linting. Callers may believe it changes behavior.
|
|
- **Recommendation:** Remove the parameter and update callers.
|
|
- **Effort:** S
|
|
|
|
#### [LOW-04] `resolve_worker_notifier` returns unvalidated `getattr` results
|
|
- **Location:** `src/transcription/worker.py:54-61`
|
|
- **Problem:** Any non-`None` attribute is returned as a `WorkerNotifier` without checking it has `notify`. Compare `app_state.py:15-18`, which correctly uses `isinstance`.
|
|
- **Effort:** S
|
|
|
|
#### [LOW-05] Untyped handler parameters and loosely-typed dict returns in UI
|
|
- **Location:** `ui/pages/jobs_page.py:491`; `ui/pages/people_page.py:492`; `ui/pages/home_page.py:85`; `_render_document_form_fields` / `_render_person_form_fields` returning `dict[str, Any]`
|
|
- **Recommendation:** Annotate with `nicegui.events.UploadEventArguments`; replace the form-field dicts with frozen dataclasses.
|
|
- **Effort:** S
|
|
|
|
#### [LOW-06] Auto-refresh timer deactivated but never cancelled; magic interval
|
|
- **Location:** `ui/pages/jobs_page.py:245,251,253`
|
|
- **Problem:** `ui.timer(4.0, refresh_job)` is toggled via `.active = False` rather than `.cancel()`; `4.0` is an unnamed literal. Client-scoped, so impact is bounded.
|
|
- **Effort:** S
|
|
|
|
#### [LOW-07] `people_page.py:504` catches `Exception` and discards it entirely
|
|
- **Location:** `ui/pages/people_page.py:504`
|
|
- **Problem:** Unlike every sibling handler, this one shows a generic message without routing through `error_presenter.show_error`, so the user gets no `error_id` to report.
|
|
- **Effort:** S
|
|
|
|
---
|
|
|
|
## 3. Stack-Specific Analysis
|
|
|
|
### Python 3.12+ Best Practices
|
|
Modern syntax is used consistently and correctly: `type` statements (`db/session.py:17,45,73,102`), `X | None` unions, `StrEnum`, `match` statements (`db/engine.py:16-31`, `db/session.py:84-92`, `workflows.py:153-171`), frozen `dataclass(slots=True)`, and `pathlib` throughout — no `os.path` anywhere. Gaps: unparameterized `asyncio.Queue` ([MED-07]), untyped `prompt_execution` parameters (`store.py:192,247`), the untyped kwargs dict at `sources.py:1229-1238` ([MED-03]), and the swallowed exception at `models.py:227` ([MED-08]). Broad `except Exception` appears frequently but is almost always accompanied by `# noqa: BLE001` and immediate normalization through `classify_unexpected_error` — that is a defensible boundary pattern, not a defect.
|
|
|
|
### FastAPI
|
|
`create_app` (`app.py:87-111`) is a clean factory using the modern `lifespan` context manager, not the deprecated `@app.on_event`. Routers are domain-organized with prefixes and tags. `response_model` is declared on every route. Dependency injection is used correctly in `api/v4_documents.py:111-128`, with the useful touch that `get_document_service` prefers lifespan-owned state and falls back gracefully. Two gaps: service methods called from `async def` endpoints perform synchronous file I/O ([MED-01]), and `_recover_stale_processing_jobs` (`app.py:73-84`) constructs a throwaway `JobService` rather than using the bundle built five lines earlier.
|
|
|
|
### NiceGUI
|
|
The strongest layer of the codebase in terms of convention adherence. CSS discipline is exemplary: a single `ui.add_css(read_css("theme.css"), shared=True)` at the composition root (`ui/__init__.py:28`), read through `importlib.resources` with a `@cache`-backed loader and path validation (`ui/resources.py:10-19`), and zero inline `.style()` calls or `<style>` blocks in components. **No cross-client state leakage was found** — per-request state lives in page-function closures, and the only module-level globals are idempotent registration flags (`theme.py:15`, `_register_panzoom_assets`'s `lru_cache`). `error_presenter.show_error/summarize_error` is applied uniformly and preserves `AppError` id/category/suggestion. The table architecture (generic `build_table` + per-feature row read models) matches the documented split. Defects are the boundary violations in [HIGH-07], the blocking I/O in [MED-01], and the duplication catalogued in §4.
|
|
|
|
### SQLModel & SQLAlchemy
|
|
Session lifecycle is the clear high point: `ServiceBase._finalize` (`services/base.py:41-60`) implements a genuinely well-reasoned commit-vs-flush ownership protocol that lets orchestration functions commit exactly once at the workflow boundary, and `session_scope` / `transaction_scope` (`db/session.py:53-105`) express the two modes cleanly, with `transaction_scope` correctly rejecting a supplied session that has no active transaction. `JSONBCompat` (`models.py:27-35`) is the right cross-dialect abstraction, and `BigInteger` for `file_size_bytes` and `StaticPool` for in-memory SQLite show real attention to portability. Against that, the eager-loading defaults ([CRIT-02]), missing indexes ([HIGH-04]), unlocked job claim ([CRIT-01]), and hand-rolled DDL ([HIGH-05]) are the four issues that most need attention. Note also that `models.py` sets `updated_at` / `date_updated` via `default_factory` only — there is no `onupdate`, so these columns are stale unless a service sets them by hand (`jobs.py:166` does; most other update paths do not).
|
|
|
|
### Pydantic V2 & Settings
|
|
Fully V2-native. No `@validator`, no `class Config`, no `.dict()`, no `parse_obj` anywhere. `model_config = ConfigDict(...)` is used consistently, usually with `extra="forbid", frozen=True` — a good default that catches provider payload drift. `Settings` is a single `BaseSettings` source of truth with `env_nested_delimiter`, a discriminated `DatabaseSettings` union, `SecretStr` for credentials, and constrained `Annotated` types (`NonEmptyStr`, `Probability`, `Temperature`). There are **no scattered `os.getenv` calls** in `src`. The one wart is `object.__setattr__` in `normalize_provider_models` (`config.py:130,137`) to mutate a frozen model — functional but fragile; `model_copy(update=...)` or a computed property would express it more safely. Issues: [MED-02] dead settings, [HIGH-03] the timeout cap, [MED-04] the `@cache` signature.
|
|
|
|
### Asyncio Workers
|
|
`worker_consumer_lifespan` (`worker.py:64-94`) is well-built: it holds a strong reference to the task, sets the stop event, wakes the loop, waits with a bounded timeout, and escalates to `cancel()` + `suppress(CancelledError)` on timeout — a correct graceful-shutdown sequence. The `WorkerNotifier` Protocol with `Event`/`Noop` implementations is a clean seam. `_persist_page_outcome_durably` (`workflows.py:486-499`) uses `asyncio.shield` with correct cancellation re-raise so a shutdown mid-job cannot lose provider evidence — a genuinely subtle piece of code done right. Remaining concerns: the single-worker assumption is unenforced ([CRIT-01]), there is no backpressure or concurrency limit (jobs are processed strictly serially, so a large backlog drains slowly while the provider sits idle), and blocking I/O inside the loop ([MED-01]) stalls both the worker and all HTTP/UI clients on the same loop.
|
|
|
|
### OpenRouter / Adapter Boundary
|
|
Encapsulation is good — no OpenRouter-specific header, model name, or payload shape appears in `services`, `api`, or `ui`. `providers/__init__.py`'s factory keeps the concrete adapter behind `get_transcription_provider`. Responses are validated through real Pydantic schemas (`OpenRouterResponse`, `ResponseChoice`, `ResponseUsage`), and `_CapturingAsyncClient` / `_CapturingAsyncByteStream` (`openrouter.py:45-91`) is a thoughtful mechanism for retaining raw transport bytes for evidence without disturbing SDK parsing. The failures are lifecycle and typing: per-job client churn ([HIGH-02]), no explicit httpx timeout ([HIGH-03]), and runtime `inspect`/`getattr` duck-typing instead of an honest Protocol ([MED-03]).
|
|
|
|
### Testing & Quality Tooling
|
|
264 tests pass with 4 skipped; markers (`unit`/`integration`/`external`) are declared and `--strict-markers` is on; `filterwarnings` escalates never-awaited coroutines to errors — a good async-specific guard. Coverage is broad across services, providers, API, and UI pages. The material gaps: (a) `asyncio_mode = "strict"` is set but no `asyncio_default_fixture_loop_scope` is configured, which pytest-asyncio warns about and which will change behavior on upgrade; (b) **every test runs on SQLite**, so the Postgres support that `JSONBCompat`, `asyncpg`, and `psycopg2-binary` all exist to provide is entirely unverified — [HIGH-05]'s `CHAR(32)` bug is exactly the class of defect this would catch; (c) no test asserts concurrent job-claim safety, which is why [CRIT-01] survives; (d) `ty` cannot gate ([HIGH-06]); (e) `tools/run_destructive_tests.py:76,80` uses `fcntl`, unavailable on this project's Windows development platform.
|
|
|
|
---
|
|
|
|
## 4. Duplication & Consolidation Report
|
|
|
|
| Pattern / Duplication | Locations | Proposed Canonical Home | Est. Lines Removed |
|
|
| :--- | :--- | :--- | :--- |
|
|
| Delete-confirmation page scaffold (blocked-deps card + confirm/cancel row) | `ui/pages/documents_page.py:354-433`; `jobs_page.py:358-428`; `sources_page.py:204-277`; `people_page.py:~270-310` | `ui/components/confirm_delete.py` | ~120 |
|
|
| Upload/media URL resolution (`_resolve_*_src`, `_to_absolute_upload_url`) | `ui/pages/sources_page.py:711-770`; `people_page.py:525-579`; `ui/components/document_panzoom.py:59-75` | `ui/components/media_urls.py` (pure, takes `upload_dir` + `base_url`) | ~110 |
|
|
| Invalid-id / not-found guard (parse → red label → return) | `documents_page.py:172-184,219-231,275-287,359-371`; `jobs_page.py:212-221,310-319,363-372`; `sources_page.py:111-137,207-222` | `ui/components/guards.py:load_or_render_error(...)` | ~90 |
|
|
| Hand-rolled `ui.table` instead of `build_table` | `ui/pages/settings_page.py:65-113` + person-roles table; `ui/components/linked_people.py:66-75`; `print_preview_page.py:125-161` | `ui/components/table/common.py:build_table` (add selection / no-search options) | ~70 |
|
|
| File-picker upload wiring | `people_page.py:491-522`; `jobs_page.py:449-503`; `home_page.py:38-40,85-89` | `ui/components/upload_panel.py` | ~50 |
|
|
| `_parse_uuid` | `documents_page.py:591`; `jobs_page.py:558`; `sources_page.py:780`; `people_page.py:589`; `linked_people.py:175` | `ui/components/formatters.py` | ~35 |
|
|
| `_resolve_runtime_settings(request)` | `jobs_page.py:567`; `sources_page.py:773`; `people_page.py:582` | Shared page-helper module | ~18 |
|
|
| `_parse_iso_date` | `documents_page.py:600`; `people_page.py:598` | `ui/components/formatters.py` | ~14 |
|
|
| `ServiceBundle` construction block (4 identical service instantiations) | `app.py:45-50`; `worker.py:160-165`; `services/__init__.py:19-22` | `ServiceBundle.from_session_factory(...)` classmethod | ~20 |
|
|
| "Next queued job" query, two divergent implementations | `services/jobs.py:170-187` (no `LIMIT`); `db/operations.py:143-151` (has `LIMIT`) | `JobService.claim_next_queued_job` (per [CRIT-01]); delete the `operations.py` copy | ~12 |
|
|
| `build_prompt_execution` re-export shim + legacy aliases | `services/transcription.py` (whole module); `services/store.py:35,382,383` | `services/sources.py` (single import path) | ~45 |
|
|
| `store_source_file` / `store_person_portrait` / `store_homepage_image` — three near-identical validate-hash-write-bytes flows | `services/store.py:319-379`; `services/people.py:~610-630`; `ui/homepage_store.py:31-44` | `services/media_storage.py` (one async, `to_thread`-wrapped writer) | ~60 |
|
|
|
|
### Proposed Canonical Abstractions
|
|
|
|
```python
|
|
# src/transcription/services/media_storage.py
|
|
async def store_media(
|
|
*, filename: str, content: bytes, root: Path, relative_directory: Path | None = None,
|
|
filename_stem: str | None = None, validate: Callable[[str, bytes], None] | None = None,
|
|
) -> StoredMedia: ... # StoredMedia = frozen dataclass(path, sha256, byte_size, media_type)
|
|
# wraps the blocking write in asyncio.to_thread — resolves [MED-01]
|
|
|
|
# src/transcription/services/__init__.py
|
|
@classmethod
|
|
def from_session_factory(cls, factory: SessionFactory, settings: Settings | None = None) -> ServiceBundle: ...
|
|
|
|
# src/transcription/services/jobs.py
|
|
async def claim_next_queued_job(self, *, session: AsyncSession | None = None) -> Job | None: ...
|
|
# atomic QUEUED -> PROCESSING with LIMIT 1 + FOR UPDATE SKIP LOCKED
|
|
|
|
# src/transcription/ui/components/media_urls.py
|
|
def build_upload_url(*, file_path: Path, upload_dir: Path, base_url: str) -> str | None: ...
|
|
|
|
# src/transcription/ui/components/confirm_delete.py
|
|
def render_confirm_delete(
|
|
*, title: str, blockers: Sequence[str], on_confirm: Callable[[], Awaitable[None]],
|
|
on_cancel: Callable[[], None],
|
|
) -> None: ...
|
|
|
|
# src/transcription/ui/components/guards.py
|
|
def parse_uuid_or_render_error(raw: str, *, entity: str) -> UUID | None: ...
|
|
```
|
|
|
|
---
|
|
|
|
## 5. Prioritized Action Plan
|
|
|
|
### Phase 1: Quick Wins (PR 1-2)
|
|
1. Delete `src/transcription/app_state.py` — dead module with a live `TypeError` ([HIGH-01]).
|
|
2. Remove `le=20.0` from `worker_provider_timeout_seconds`, raise the default, and pass an explicit `httpx.Timeout` to the OpenRouter client ([HIGH-03]).
|
|
3. Add `Index("ix_job_status_date_created", "status", "date_created")` and `index=True` on the hot foreign keys ([HIGH-04]).
|
|
4. Add `.limit(1)` to `read_next_queued_job` — a one-line change that removes the full-queue load ahead of the full [CRIT-01] fix.
|
|
5. Delete `services/transcription.py`, the three `store.py` aliases, and `ServiceBase.queue`; standardize `build_prompt_execution` imports ([MED-05], [MED-07]).
|
|
6. Move the 23KB SVG to `ui/static/` and run `ruff check --fix` ([MED-09], [LOW-01]).
|
|
7. Resolve or delete `sqlite_check_same_thread` and `worker_retry_backoff_seconds` ([MED-02]).
|
|
|
|
### Phase 2: Reliability & Concurrency (PR 3-4)
|
|
8. Implement `claim_next_queued_job` with `LIMIT 1` + `FOR UPDATE SKIP LOCKED`, delete the `db/operations.py` duplicate, and add a concurrency test that runs two claimers against one queued job ([CRIT-01]).
|
|
9. Hoist `ServiceBundle` and the provider client to worker-loop scope so the HTTP connection pool survives across jobs ([HIGH-02], [MED-06]).
|
|
10. Wrap blocking media/artifact I/O and Pillow normalization in `asyncio.to_thread` behind a single `services/media_storage.py` ([MED-01]).
|
|
11. Adopt Alembic; first revision fixes `preferred_execution_attempt_id` from `CHAR(32)` to a real UUID FK. Add one Postgres-backed integration test job ([HIGH-05]).
|
|
12. Extend `TranscriptionProvider` Protocol to cover `aclose` and the evidence attributes; delete the `inspect.signature` reflection ([MED-03]).
|
|
|
|
### Phase 3: Consolidation & Refactoring (PR 5-6)
|
|
13. Flip relationship defaults to `lazy="raise"` model by model, letting the existing suite prove which explicit `selectinload()` calls are load-bearing ([CRIT-02]). This also removes most of the `# pyright: ignore` comments.
|
|
14. Standardize on `ty`, convert remaining suppressions to `# ty: ignore[...]`, and wire `ty check` into the existing pre-commit setup ([HIGH-06]).
|
|
15. Fix the three UI boundary violations: session ownership in `jobs_page`, `sqlalchemy.inspect` in `sources_page`, `get_settings()` in `document_panzoom` ([HIGH-07]).
|
|
16. Extract the UI duplication per §4, highest value first: `confirm_delete` → `media_urls` → `guards` → `formatters` (~500 lines removed).
|
|
17. Replace `functools.cache` on engine/session factories with an explicit URL-keyed registry supporting targeted eviction ([MED-04]).
|
|
|
|
---
|
|
|
|
## 6. Preserved Strengths
|
|
|
|
- **`ServiceBase._finalize` (`services/base.py:41-60`)** — the commit-vs-flush ownership protocol is the single best idea in the codebase. It lets orchestration functions compose multiple services into one atomic transaction without any service knowing about the others, and it is documented in `services.instructions.md`. Keep it and keep enforcing it.
|
|
- **Error taxonomy (`errors.py`)** — `AppError` carrying `category`, `suggestion`, `retriable`, and a short shareable `error_id`, with `classify_unexpected_error` normalizing at every boundary and `format_error_detail` producing a stable persisted string. It is applied consistently from services through API handlers to `ui/components/error_presenter.py`.
|
|
- **Evidence capture pipeline** — `_CapturingAsyncClient` / `_CapturingAsyncByteStream` (`openrouter.py:45-91`) plus `ExecutionAttempt` / `ProcessingArtifact` with content-addressed digests and integrity verification (`sources.py:925-959`) is a serious, well-executed provenance design that is rare to see done properly.
|
|
- **`asyncio.shield` around page-outcome persistence (`workflows.py:486-499`)** — correctly written, including the `await task` before re-raising `CancelledError`, so provider results survive shutdown mid-job.
|
|
- **Worker lifespan shutdown (`worker.py:64-94`)** — strong task reference, stop event, wake, bounded wait, then cancel-and-suppress. Textbook correct.
|
|
- **Pydantic V2 discipline** — zero V1 residue, `extra="forbid"` + `frozen=True` as the house default, constrained `Annotated` types, `SecretStr` for credentials, discriminated union for database config, and no `os.getenv` anywhere in `src`.
|
|
- **UI CSS and asset discipline** — one `add_css` at the composition root, `importlib.resources` with a `@cache`d reader and path validation, semantic `ui-*` classes, no inline styles. This is exactly what `ui.instructions.md` prescribes, followed without exception.
|
|
- **No cross-client state leakage in NiceGUI** — per-request state lives in page-function closures; the only module globals are idempotent registration flags. This is the most common NiceGUI defect and this codebase avoids it entirely.
|
|
- **Cross-dialect care** — `JSONBCompat`, `BigInteger` for byte sizes, `native_enum=False` with `values_callable` for stable enum storage, `StaticPool` for in-memory SQLite. The intent is right; it just needs Postgres CI to make it real.
|
|
- **The instruction files themselves** — `.github/instructions/services.instructions.md` and `ui.instructions.md` are specific, enforceable, and largely followed. Most findings in this report are deviations from rules the project already wrote down, which is a much healthier position than having no rules at all.
|