diff --git a/.trivyignore b/.trivyignore index 0bb6865..868a517 100644 --- a/.trivyignore +++ b/.trivyignore @@ -220,3 +220,91 @@ CVE-2026-9538 # Review Date: 2026-05-31 # Next Review: 2026-07-30 (60-day cap per CLAUDE.md unfixed-CVE policy) CVE-2026-48962 + +# ============================================================================ +# 2026-07-02 baseline refresh (Trivy scan on PR #75) +# All entries below: no fixed version in Debian 13 (trixie) as of 2026-07-02 +# (Trivy status "affected", Fixed Version empty for every finding). +# Documented in docs/known-vulnerabilities.md; Reassess-by: 2026-08-31 +# (60-day cap per CLAUDE.md unfixed-CVE policy). +# ============================================================================ + +# CVE-2026-58049 - ffmpeg family (ffmpeg, libavcodec-extra/61, libavdevice61, +# libavfilter10, libavformat61, libavutil59, libpostproc58, libswresample5, +# libswscale8) - memory corruption via crafted RASC video stream +# Severity: HIGH +# Risk Assessment: MEDIUM - unlike the rest of this baseline, ffmpeg IS in the +# application request path (user-uploaded media is passed to ffmpeg/ffprobe). +# Justification: +# - No fixed Debian package exists; nothing to upgrade to +# - RASC is an obscure screen-capture codec; mainstream audio/video uploads +# do not exercise the vulnerable demuxer path, but a crafted upload could +# - ffmpeg runs as a short-lived subprocess as non-root appuser inside the +# container; blast radius is the worker container, not the host +# - ADR-003 adds hard per-job subprocess timeouts, bounding hung exploits +# - Mitigation candidate at reassess time: restrict ffmpeg demuxers to the +# formats the API actually accepts (input format allowlist) +# Review Date: 2026-07-02 +# Next Review: 2026-08-31 (elevate priority; request-path exposure) +CVE-2026-58049 + +# CVE-2026-41992 - gzip global buffer overflow in LZH decompression +# Severity: HIGH +# Risk Assessment: LOW - the application never invokes gzip on untrusted +# input; the binary ships in the base image for packaging tooling only. +# Review Date: 2026-07-02 / Next Review: 2026-08-31 +CVE-2026-41992 + +# CVE-2026-54369 (libacl1), CVE-2026-54371 (libattr1) - symlink traversal +# local privilege escalation via libacl functions / getfattr +# Severity: HIGH +# Risk Assessment: LOW - local-privilege-escalation class; the container runs +# a single non-root user (appuser) with no untrusted local users and no +# setuid tooling in the request path. +# Review Date: 2026-07-02 / Next Review: 2026-08-31 +CVE-2026-54369 +CVE-2026-54371 + +# CVE-2026-56208, CVE-2026-56209, CVE-2026-56210, CVE-2026-56211 - libaom3 +# AV1 ENCODER bugs (first-pass stats buffer, SVC layer context, layer-id ctrl) +# Severity: HIGH +# Risk Assessment: LOW - all four are in AV1 encoding paths; this service +# converts uploads to WAV/PCM audio and never encodes AV1 video, so the +# vulnerable encoder code is not exercised. libaom is a transitive dep of +# libavcodec-extra. +# Review Date: 2026-07-02 / Next Review: 2026-08-31 +CVE-2026-56208 +CVE-2026-56209 +CVE-2026-56210 +CVE-2026-56211 + +# CVE-2026-58016 (CRITICAL), CVE-2026-58014, CVE-2026-58015 - libglib2.0-0t64 +# gdbus introspection integer underflow / gkeyfile off-by-one / gdbus auth +# path traversal +# Severity: CRITICAL / HIGH / HIGH +# Risk Assessment: NEGLIGIBLE - all three are in GLib's D-Bus and keyfile +# code; this headless container runs no D-Bus daemon and no code parses +# keyfiles or GDBus XML. GLib is present only as a transitive C dependency +# of the ffmpeg codec stack (same analysis as the existing CVE-2025-13601 +# glib entry above). +# Review Date: 2026-07-02 / Next Review: 2026-08-31 +CVE-2026-58016 +CVE-2026-58014 +CVE-2026-58015 + +# CVE-2026-58050 - libssh2-1t64 heap buffer overflow in publickey attribute +# allocation +# Severity: HIGH +# Risk Assessment: LOW - libssh2 is linked by curl; the application makes no +# SSH connections. curl is used only for the localhost healthcheck and +# image-build downloads (existing baseline rationale). +# Review Date: 2026-07-02 / Next Review: 2026-08-31 +CVE-2026-58050 + +# CVE-2026-12912 - libtiff6 heap overflow via crafted PixarLog TIFF +# Severity: HIGH +# Risk Assessment: LOW - the service processes audio/video media only; no +# code path decodes TIFF images. libtiff is a transitive dep of the ffmpeg +# codec extras. +# Review Date: 2026-07-02 / Next Review: 2026-08-31 +CVE-2026-12912 diff --git a/CHANGELOG.md b/CHANGELOG.md index 5681e7a..51e7b7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - feat(ci): add a qlty PR diff gate (`qlty-gate`, fail-level medium) as a required status check, plus a weekly informational full-codebase qlty health scan (`qlty-health`, Mondays 07:00 UTC); the workflow concurrency group now isolates runs by event type and head repository ### Fixed +- fix(security): upgrade vulnerable dependencies flagged by pip-audit/OSV: cryptography 48.0.1 (GHSA-537c-gmf6-5ccf), jupyter-server 2.20.0 (PYSEC-2026-366), jupyterlab 4.5.9 (GHSA-vmhf-c436-hxj4), msgpack 1.2.1 (GHSA-6v7p-g79w-8964), pydantic-settings 2.14.2 (GHSA-4xgf-cpjx-pc3j), starlette 1.3.1 (PYSEC-2026-248, PYSEC-2026-249), tornado 6.5.7 (GHSA-pw6j-qg29-8w7f); defer torch CVE-2025-3000 (no patched release, local-only, excluded from the production image) with documented suppressions and a 2026-08-31 reassess-by in `docs/known-vulnerabilities.md` - fix(jobs): correct field paths for `duration_ms` and `language` in `process_audio_job` result assembly; both now read from `TranscriptionResult.metadata` where they actually live, preventing `AttributeError` at runtime - fix(api): guard `content-length` header parsing against malformed values; `int()` conversion is now wrapped in a `ValueError` handler so a non-numeric header no longer raises an unhandled exception - fix(tests): restore `tmp_path` fixture in `test_custom_initialization` for `AudioConverter`, `AudioConditioner`, and `VADProcessor`; hardcoded `/custom/temp` caused `PermissionError` on systems without root access diff --git a/docs/known-vulnerabilities.md b/docs/known-vulnerabilities.md index af5ba4f..da8d146 100644 --- a/docs/known-vulnerabilities.md +++ b/docs/known-vulnerabilities.md @@ -17,12 +17,13 @@ tags: ## Status -One Python-dependency CVE (`py` 1.11.0) and a documented baseline of -base-image transitive C library CVEs (catalogued in the section -"Base-image transitive library CVEs" below and suppressed in -`.trivyignore`). +One Python-dependency CVE (`py` 1.11.0), two deferred torch CVEs with no +patched release (PYSEC-2026-139, CVE-2025-3000 — both excluded from the +production image), and a documented baseline of base-image transitive C +library CVEs (catalogued in the section "Base-image transitive library +CVEs" below and suppressed in `.trivyignore`). -Last reviewed: 2026-05-28 +Last reviewed: 2026-07-02 --- @@ -352,6 +353,75 @@ entry and its `osv-scanner.toml` suppression. --- +## CVE-2025-3000: torch 2.12.0 + +| Field | Value | +| --- | --- | +| **ID** | CVE-2025-3000 | +| **Package** | `torch` >= 2.9.0 (direct in `[ml]` extra; transitive via `silero-vad` in `[audio]` extra) | +| **CVE** | CVE-2025-3000 | +| **GHSA** | GHSA-rrmf-rvhw-rf47 | +| **Aliases** | CVE-2025-3000, GHSA-rrmf-rvhw-rf47, PYSEC-2025-194 (all three suppressed in `osv-scanner.toml`) | +| **Severity** | Low-Medium (local-only) | +| **CVSS Score** | CVSS:4.0 AV:L/AC:L/AT:N/PR:L/UI:N/VC:L/VI:L/VA:L (per OSV) | +| **Patched version** | None. OSV records `last_affected` only (no fixed release identified); published 2025-03-31. | +| **Status** | Deferred (compensating control in place) | +| **Discovered** | 2026-07-02 (pip-audit and OSV scanner failures on PR #75) | +| **Reassess-by** | 2026-08-31 (60-day cap) | +| **Suppressed in** | `osv-scanner.toml`, `[tool.pip-audit] ignore-vuln` in `pyproject.toml` | + +**Exploitation scenario**: The vulnerability requires a local attacker with a valid +user account (AV:L, PR:L) on the same host as the torch runtime, with low +confidentiality/integrity/availability impact. Remote exploitation is not possible. + +**Why deferred**: No patched release exists upstream. As with PYSEC-2026-139, `torch` +enters the dependency graph only via the `[ml]` extra (direct) and the `[audio]` extra +(transitive through `silero-vad`). The production Docker image runs +`uv sync --frozen --no-dev` with no `--extra` flags, so torch is not installed at +runtime. + +**Compensating control**: Identical to PYSEC-2026-139: (1) torch is absent from the +production container image; (2) the attack vector is local-only, and the service runs +in a containerised, non-root environment with no local user accounts accessible to +external parties. + +**Planned resolution**: Reassess 2026-08-31 (fold into the 2026-07-26 PYSEC-2026-139 +reassessment). Check the OSV advisory for a fixed release; if one ships, upgrade the +`torch>=` constraint, regenerate `uv.lock`, and remove this entry plus the +`osv-scanner.toml` and `pyproject.toml` suppressions. + +--- + +## Base-image baseline refresh — 2026-07-02 (Trivy scan on PR #75) + +Trivy reported 22 findings (21 HIGH, 1 CRITICAL) against the container image, +all with Debian status `affected` and **no fixed version available** — nothing +to upgrade to. Thirteen unique CVEs were added to `.trivyignore` with per-family +justifications inline in that file. Shared metadata: + +- **Discovered**: 2026-07-02 (Trivy scan on PR #75) +- **Reassess-by**: 2026-08-31 (60-day cap per CLAUDE.md unfixed-CVE policy) +- **Status**: Deferred (compensating controls documented per family) +- **Suppressed in**: `.trivyignore` + +Family summary: + +| Family | CVEs | Risk | Basis | +| --- | --- | --- | --- | +| ffmpeg stack | CVE-2026-58049 | **MEDIUM — request-path exposure** | ffmpeg processes user uploads; RASC is an obscure codec, subprocess is non-root/short-lived, ADR-003 adds hard timeouts. Consider a demuxer allowlist at reassess time. | +| glib | CVE-2026-58016 (CRITICAL), CVE-2026-58014, CVE-2026-58015 | Negligible | D-Bus/keyfile code paths; headless container runs no D-Bus, parses no keyfiles. | +| libaom (AV1) | CVE-2026-56208..56211 | Low | All encoder-side; service never encodes AV1. | +| acl/attr | CVE-2026-54369, CVE-2026-54371 | Low | Local priv-esc class; single non-root user, no untrusted local users. | +| gzip | CVE-2026-41992 | Low | LZH path never invoked on untrusted input. | +| libssh2 | CVE-2026-58050 | Low | Linked by curl; no SSH connections made. | +| libtiff | CVE-2026-12912 | Low | No TIFF decoding in any code path. | + +The ffmpeg entry is deliberately **not** grouped with the "not exercised by +the request path" baseline above: it is the application's primary attack +surface and carries an elevated-priority reassessment. + +--- + ## OSSF Scorecard Approved Deviations This section tracks Scorecard check results where an approved deviation is in diff --git a/docs/planning/adr/README.md b/docs/planning/adr/README.md index c16cfe4..e58d3b0 100644 --- a/docs/planning/adr/README.md +++ b/docs/planning/adr/README.md @@ -25,9 +25,12 @@ ADRs document significant architectural decisions along with their context and c ## ADR Index -| ADR | Title | Status | Date | -|----------------|-----------------------------------|--------|------| -| *No ADRs yet* | Generate with `/plan` command | - | - | +| ADR | Title | Status | Date | +|-----|-------|--------|------| +| [ADR-001](adr-001-initial-architecture.md) | Initial Architecture — Deepgram-Centric Audio Processing | Accepted | 2025-12-04 | +| [ADR-002](adr-002-audio-preprocessing-pipeline.md) | Audio Preprocessing Pipeline | Accepted | 2025-12-04 | +| [ADR-003](adr-003-async-execution-model.md) | Async Execution Model — Blocking Work, Timeouts, and Cancellation | Accepted | 2026-07-02 | +| [ADR-004](adr-004-job-lifecycle-state-machine.md) | Job Lifecycle State Machine — Transitions, Retries, Idempotency, and File Ownership | Accepted | 2026-07-02 | ## Creating ADRs diff --git a/docs/planning/adr/adr-003-async-execution-model.md b/docs/planning/adr/adr-003-async-execution-model.md new file mode 100644 index 0000000..0f909d2 --- /dev/null +++ b/docs/planning/adr/adr-003-async-execution-model.md @@ -0,0 +1,252 @@ +--- +title: "ADR-003: Async Execution Model — Blocking Work, Timeouts, and Cancellation" +schema_type: planning +status: published +owner: core-maintainer +purpose: "Define the binding contract for where blocking work runs, how timeouts are enforced, and what cancellation guarantees, so the event loop is never blocked in the API or the worker." +tags: + - adr + - architecture + - concurrency +component: Development-Tools +source: "Systems design review 2026-07-02" +--- + +> **Status**: Accepted +> **Date**: 2026-07-02 +> **Supersedes**: None +> **Relates to**: ADR-001 (Deepgram-centric pipeline), systems design review findings 3, 5, 6 + +## Context + +Both processes in this system run a single asyncio event loop, and both currently +execute long blocking calls directly on it: + +- The API route `process_audio` calls `AudioConverter.validate_file` inline, which + runs `subprocess.run(ffprobe, timeout=30)`. One slow upload stalls every + concurrent request, including `/health` — the container health check + (10 s timeout, 3 retries) can then kill a healthy container. +- The ARQ task `process_audio_job` runs ffmpeg (`subprocess.run`, up to 600 s), + librosa/numpy quality assessment, and the synchronous Deepgram HTTP call + directly in the async task. `max_jobs = 10` is therefore fiction: jobs + serialize behind each other, the worker's health-check heartbeat starves, and + ARQ's `job_timeout` — enforced by *cancelling the task* — cannot fire, because + a coroutine blocked inside a sync call never reaches an `await` where + `CancelledError` can be delivered. + +"Wrap it in a thread" alone is not a fix. `anyio.to_thread.run_sync` unblocks +the loop, but threads cannot be cancelled: with the default +`abandon_on_cancel=False`, ARQ's timeout cancellation *waits for the thread to +finish* (timeout still unenforced); with `abandon_on_cancel=True`, the +coroutine returns but the thread — and its ffmpeg child — keeps running +unbounded. Timeout enforcement must therefore live **inside** each blocking +primitive, with cancellation reduced to "stop waiting" plus a bounded +self-termination guarantee. + +Pinned versions this contract is written against: `anyio 4.13` +(`abandon_on_cancel` parameter), `arq 0.28` (timeout via task cancellation). + +## Decision + +Services stay synchronous (per `services/CLAUDE.md`). Async callers dispatch +them to threads. Every blocking primitive carries its own hard deadline, so an +abandoned stage always self-terminates. The orchestrator alone touches shared +state. + +### 1. Execution placement table (binding) + +| Call site | Work | Runs on | Timeout enforced by | Behavior when the awaiting task is cancelled | +|---|---|---|---|---| +| API `process_audio` | `validate_file` (ffprobe) | thread: `anyio.to_thread.run_sync(..., abandon_on_cancel=True, limiter=API_PROBE_LIMITER)` | existing 30 s `subprocess.run` timeout (kills the child) | thread abandoned; ffprobe self-terminates ≤ 30 s | +| API `process_audio` | `mkstemp`, upload streaming | thread / native async (already correct) | n/a | unchanged | +| Worker stage: convert/extract | ffmpeg via `AudioConverter` | thread: `anyio.to_thread.run_sync(..., abandon_on_cancel=True, limiter=WORKER_STAGE_LIMITER)` | `deadline.remaining()` passed as the `subprocess.run` timeout (`subprocess.run` kills the child on expiry) | abandoned; ffmpeg killed by its own timeout ≤ remaining budget | +| Worker stage: quality assessment | librosa/numpy over decoded audio | thread (same dispatch) | not interruptible — bounded only by input size; see §4 duration cap | abandoned; completes on local data, result discarded | +| Worker stage: transcription | Deepgram sync SDK | thread (same dispatch) | `min(settings.deepgram_timeout_seconds, deadline.remaining())` passed to the SDK's HTTP client per request (closes design-review Finding 5 — the setting is currently never applied) | abandoned; HTTP timeout bounds the thread | +| Worker stage: artifact generation | string building over words/utterances | thread (same dispatch) | bounded by input size (duration cap) | abandoned; result discarded | +| Worker orchestrator | job-store updates, unlink, progress | event loop (async), as today | n/a | cleanup runs in a shielded scope (§5) | + +**Loop-blocking rule (invariant I-1):** no synchronous section on the event +loop may exceed 100 ms. Concretely forbidden on the loop: any `subprocess.*` +call, any librosa/soundfile/numpy operation over audio arrays, any Deepgram SDK +call, any file read/write larger than 1 MiB. + +### 2. Deadline budget contract + +A single per-job wall-clock budget replaces the current pattern of reusing +`settings.job_timeout_seconds` as every individual subprocess timeout (which +lets one stage consume the entire job budget and the job still run to 3× it). + +```python +class StageDeadline: + """Monotonic per-job budget. Created once at job start.""" + def __init__(self, total_seconds: float) -> None: + self._deadline = time.monotonic() + total_seconds + + def remaining(self) -> float: + """Seconds left. Raises JobTimeoutError (AudioProcessorError subclass) + when the budget is exhausted, so a stage is never started with <= 0.""" +``` + +- Created in `process_audio_job` with `settings.job_timeout_seconds`. +- Every stage dispatch passes `deadline.remaining()` into the primitive that + enforces it (ffmpeg subprocess timeout; Deepgram HTTP timeout). +- `AudioConverter.extract_audio` / `convert_for_asr` gain a required + `timeout_seconds: float` parameter (replacing the internal read of + `settings.job_timeout_seconds`). `probe` keeps its fixed 30 s. +- `DeepgramTranscriptionClient.transcribe` gains `timeout_seconds: float`, + wired to the SDK's per-request HTTP timeout — not merely stored on `self`. +- ARQ `WorkerSettings.job_timeout` becomes + `settings.job_timeout_seconds + JOB_TIMEOUT_GRACE` with + `JOB_TIMEOUT_GRACE = 60`. It is a **backstop only** (catches a stage that + ignores its budget); the `StageDeadline` is the primary enforcement. A job + must terminate — success or `FAILED` — within budget + grace (invariant I-2). + +### 3. Concurrency bounds + +- `WORKER_STAGE_LIMITER = anyio.CapacityLimiter(settings.worker_max_jobs)` + (module-level in `jobs/audio_tasks.py`): at most one stage thread per + running job, so thread count can never exceed job concurrency. +- `API_PROBE_LIMITER = anyio.CapacityLimiter(8)` (module-level in + `api/routes.py`): bounds concurrent ffprobe processes regardless of request + volume; requests beyond it queue on the limiter, not on the loop. +- New setting `worker_max_jobs: int = 2` (ge=1, le=32); + `WorkerSettings.max_jobs` reads it instead of the hardcoded 10. + +### 4. Memory sizing (why `worker_max_jobs = 2` and the duration cap) + +Converted audio is 16 kHz mono s16le WAV: **32 000 bytes/s ≈ 115 MB per hour**. +With the current whole-file buffering, per-job peak memory is dominated by: + +- quality assessment: `soundfile.read` decodes to float64 → **4 × WAV bytes** +- Deepgram upload: `file.read()` into memory → 2 × WAV bytes + +Stages run sequentially within a job, so **peak/job ≈ 4 × WAV bytes**. The +binding constraint for the worker container: + +```text +worker_max_jobs × 4 × (115 MB × audio_max_duration_hours) + 300 MB baseline + ≤ container memory limit +``` + +The current defaults are infeasible: 4 h audio → 461 MB WAV → ~1.8 GB peak per +job, against a 512 MB limit in `docker-compose.prod.yml` — an OOM kill even at +concurrency 1. Interim posture (until streaming per design-review Finding 6 +removes the 4× and 2× factors): + +- `audio_max_duration_hours` default: **4.0 → 1.0** +- `worker_max_jobs`: **2** +- worker container memory limit: **2 GiB** (2 × 4 × 115 MB + 300 MB ≈ 1.2 GiB, + ~40 % headroom) + +Any change to duration limit, concurrency, or container memory must re-satisfy +the inequality above (invariant I-3). Both the quality-assessment decode and +the Deepgram read must move to streaming/chunked forms before the 4× factor may +be dropped from the formula. + +### 5. Cancellation and cleanup contract + +- **Orchestrator-only state (invariant I-4):** stage functions dispatched to + threads are pure with respect to shared state — they take paths/data in and + return values; they never write the job store. Only `process_audio_job` + (on the loop) updates the store. An abandoned thread therefore cannot race a + status write. +- **Shielded cleanup:** `process_audio_job` wraps the pipeline so that on any + exit — including `CancelledError` from ARQ timeout or worker shutdown — a + cleanup block runs inside `with anyio.CancelScope(shield=True):` and (a) + writes the terminal store status, (b) unlinks this job's temp files + (`os.unlink` on the loop is acceptable: < 100 ms). Cleanup must be + idempotent. *Which* files and statuses are written in each terminal state is + the job-lifecycle ADR's decision (design-review item 2); this ADR fixes the + mechanism. +- **Orphan bound:** an abandoned stage may still create its output temp file + after abandonment (ffmpeg finishing within its own timeout). Bound this with + a sweep in the worker `startup` hook: delete files in + `settings.audio_temp_dir` older than + `job_timeout_seconds + JOB_TIMEOUT_GRACE`. Orphan lifetime is therefore + ≤ one budget+grace window (invariant I-5). + +### 6. Caller-side rule (added to `services/CLAUDE.md`) + +Services stay sync; the missing rule is for their callers: + +> Any `async def` (route handler, ARQ task, lifespan hook) calling a service +> method that shells out, does CPU-bound audio work, or performs blocking I/O +> MUST dispatch it via +> `anyio.to_thread.run_sync(..., abandon_on_cancel=True, limiter=)` +> and MUST pass an explicit timeout that the service enforces internally. +> Calling these methods bare in async code is a review-blocking defect. + +## Acceptance criteria + +Implementation is complete only when all of these tests exist and pass: + +1. **API responsiveness (I-1):** with `AudioConverter.validate_file` + monkeypatched to block on a `threading.Event` the test releases afterwards + (not a fixed `time.sleep` — see `tests/CLAUDE.md`), a concurrent + `GET /health` completes in < 500 ms while an upload is in flight. +2. **Worker parallelism:** two jobs whose conversion stage is stubbed to block + 1 s in-thread finish in < 1.8 s combined (parallel), not ≥ 2 s (serial). +3. **Deadline propagation:** with total budget `T` and a first stage stubbed to + consume `t`, the mocked Deepgram transport receives a timeout ≤ `T − t`; + `subprocess.run` in the converter receives the computed remaining budget, + not `settings.job_timeout_seconds`. +4. **Timeout termination (I-2):** a job whose stage blocks past the budget ends + `FAILED` within budget + grace, the shielded cleanup ran (temp files gone, + terminal status written), and no child process survives. +5. **Thread bound (§3):** with `worker_max_jobs = N` and > N queued jobs, at + most N stage threads run concurrently (assert via the limiter's + `statistics()`). +6. **Loop watchdog:** the pipeline integration test runs with asyncio debug + mode (`loop.slow_callback_duration = 0.2`) and fails on any "Executing … + took" warning captured from the `asyncio` logger. +7. **Orphan sweep (I-5):** a pre-seeded stale file in the temp dir older than + budget + grace is removed by the worker `startup` hook; a fresh file is not. + +## Implementation tasks (ordered, mechanical) + +1. Add `JobTimeoutError` to `core/exceptions.py`; add `StageDeadline` (new + `jobs/deadline.py` or in `audio_tasks.py`). +2. Add `worker_max_jobs` setting; change `audio_max_duration_hours` default to + 1.0; set `WorkerSettings.max_jobs = settings.worker_max_jobs` and + `job_timeout = settings.job_timeout_seconds + 60`. +3. Add `timeout_seconds` parameters to `extract_audio` / `convert_for_asr` + (drop their internal `settings.job_timeout_seconds` read) and to + `DeepgramTranscriptionClient.transcribe`, wired to the SDK HTTP timeout. +4. In `api/routes.py`: add `API_PROBE_LIMITER`; dispatch `validate_file` via + `to_thread.run_sync(abandon_on_cancel=True, limiter=...)`. +5. In `jobs/audio_tasks.py`: add `WORKER_STAGE_LIMITER`; dispatch the four + stages per §1 with `deadline.remaining()`; restructure the task body around + the shielded cleanup block (§5). +6. Add the temp-dir sweep to the worker `startup` hook. +7. Add the caller-side rule to `services/CLAUDE.md` (§6). +8. Add acceptance tests 1–7; raise the worker memory limit to 2 GiB in + `docker-compose.prod.yml` per §4. + +## Alternatives considered + +- **Rewrite services as async (`asyncio.create_subprocess_exec`)**: cancellable + subprocesses without abandonment, but a much larger diff, breaks the + established sync-services convention, and offers no better wall-clock bound + than `subprocess.run`'s kill-on-timeout. Rejected for scope; the placement + table localizes a future migration to the dispatch sites. +- **`ProcessPoolExecutor` for CPU stages**: still not cancellable, adds + pickling constraints on models and duplicates audio buffers across + processes, worsening §4. Rejected. +- **Separate synchronous worker framework (Celery/RQ prefork)**: real + SIGKILL-based timeouts, but an infrastructure change that abandons ARQ and + the shared job-store design. Rejected at current scale; revisit if job + volume outgrows a single async worker with `worker_max_jobs` threads. + +## Consequences + +- The event loop in both processes stays responsive under any input; `/health` + and the ARQ heartbeat reflect real liveness. +- `job_timeout` becomes a real guarantee (budget + grace) instead of + aspirational config. +- Cancellation never corrupts state: threads are abandoned but side-effect-free + (I-4), self-terminate within their own deadlines, and orphaned files are + swept (I-5). +- Throughput is honestly bounded by `worker_max_jobs` and the memory formula — + raising it is now a sizing calculation, not a hope. +- Cost: abandoned stages can waste up to one stage-timeout of CPU after a + cancel; accepted as bounded and rare. diff --git a/docs/planning/adr/adr-004-job-lifecycle-state-machine.md b/docs/planning/adr/adr-004-job-lifecycle-state-machine.md new file mode 100644 index 0000000..03998e1 --- /dev/null +++ b/docs/planning/adr/adr-004-job-lifecycle-state-machine.md @@ -0,0 +1,295 @@ +--- +title: "ADR-004: Job Lifecycle State Machine — Transitions, Retries, Idempotency, and File Ownership" +schema_type: planning +status: published +owner: core-maintainer +purpose: "Define the canonical job state machine, who may write which transition, what actually retries and why, how redelivery stays idempotent, and which process owns each temp file at every point in a job's life." +tags: + - adr + - architecture + - reliability +component: Development-Tools +source: "Systems design review 2026-07-02" +--- + +> **Status**: Accepted +> **Date**: 2026-07-02 +> **Supersedes**: None +> **Relates to**: ADR-003 (async execution model — this ADR consumes its +> `StageDeadline`/`JobTimeoutError` and the deadline+grace window), systems +> design review findings 1, 2, 4, 8 + +## Context + +Job state currently lives in a shared store (`core/job_store.py`) written by +three uncoordinated writers — the API (`QUEUED`, enqueue-failure `FAILED`), the +worker orchestrator (`PREPROCESSING` → … → `COMPLETED`/`FAILED`), and Redis TTL +expiry (silent deletion). Nothing validates transitions: `store.update` merges +blindly, so a late writer can move a job *out of* a terminal state. The +lifecycle has real holes today: + +- **Stranded non-terminal records.** ARQ enforces `job_timeout` by cancelling + the task; `asyncio.CancelledError` is a `BaseException`, so the + `except Exception` handlers in `process_audio_job` never run on timeout and + no `FAILED` is written. With `retry_jobs = True`, ARQ re-runs the job up to + `max_tries`, then gives up **without executing the task again** — the record + is left in `PREPROCESSING`/`TRANSCRIBING` forever (until TTL deletion turns + it into a 404). Clients polling `/status` see a job that is "processing" + and never finishes. +- **Retry semantics are misdescribed.** `worker.py` promises "job retries with + exponential backoff" and `job_max_retries` is documented as "maximum retry + attempts for failed jobs". In arq 0.28, `retry_jobs=True` re-queues a job + only on **cancellation** (timeout, worker shutdown) or an explicit + `arq.worker.Retry` raise. Ordinary exceptions — every `ValidationError`, + `TranscriptionError`, unexpected error in the pipeline — fail permanently on + the first attempt. The configured retry budget currently applies *only* to + the timeout path, which is the one path where re-running is least useful. +- **No enqueue dedup, no addressability.** `enqueue_task` calls + `redis.enqueue_job(task_name, ...)` without `_job_id=`, so ARQ assigns its + own random id. ARQ's built-in same-id dedup is unused, and the application + `job_id` cannot be used to look up, abort, or reason about the ARQ job. +- **File ownership is asserted, not implemented.** `routes.py` deletes the + uploaded temp file only on failure, with a comment claiming "the worker + takes ownership of the temp file and deletes it when done" — but the worker + never deletes `file_path`. It unlinks only `converted_path`, and only on the + happy path. Every successful job leaks its input file; every failed or + timed-out job leaks both files. ADR-003's startup sweep bounds the damage + but is a backstop, not an ownership policy. +- **Redelivery is not idempotent.** A worker that dies after the Deepgram call + but before the `COMPLETED` write causes a full re-run on redelivery — + including a second billed Deepgram request — and a worker that dies *after* + the `COMPLETED` write causes a re-run that drags a finished job back to + `PREPROCESSING`. + +This ADR fixes the lifecycle at the contract level. ADR-003 governs *how* +stages execute; this ADR governs *what a job is allowed to be* at any moment, +observed from the store. + +## Decision + +One state machine, guarded at the store. Retries are reserved for redelivery +after worker death and explicitly-declared transient external failures — +never for validation or deadline failures. Terminal states are absorbing. +Every temp file has exactly one owner at any instant, and the owner that +writes a terminal state deletes the files. + +### 1. Canonical state machine (binding) + +States are the existing `JobStatus` values. `COMPLETED` and `FAILED` are +**terminal (absorbing)**: no writer may transition out of them, ever. + +| Transition | Writer | Trigger | +|---|---|---| +| ∅ → `QUEUED` | API | `store.create` on accepted upload | +| `QUEUED` → `PREPROCESSING` | worker | task entry, attempt 1 | +| any non-terminal → `PREPROCESSING` | worker | task entry, attempt > 1 (redelivery re-runs from the top) | +| `PREPROCESSING` → `TRANSCRIBING` | worker | conversion + quality stages done | +| `TRANSCRIBING` → `POSTPROCESSING` | worker | Deepgram call returned | +| `POSTPROCESSING` → `COMPLETED` | worker | result + artifacts persisted | +| any non-terminal → `FAILED` | worker | pipeline exception or `JobTimeoutError` | +| `QUEUED` → `FAILED` | API | enqueue failure (existing path) | +| any non-terminal → `FAILED` | API (lazy reaper, §4) | record stale beyond deadline + grace | +| any → *(deleted)* | Redis TTL | `job_result_ttl_seconds` since last write — the implicit `EXPIRED` state; observed as 404 | + +`CANCELLED` is a **reserved** state name for a future user-facing abort +endpoint (arq supports `Job.abort()` once `_job_id` is wired, §3). It is not +added now; nothing else may reuse the name. + +**Enforcement — guarded transitions (invariant L-1).** Status may only change +via a new store method: + +```python +async def transition( + self, job_id: str, *, from_statuses: frozenset[str], to_status: str, + **fields: object, +) -> bool: + """Atomically set status (and merge fields) iff the current status is in + from_statuses. Returns False — writing nothing — otherwise.""" +``` + +- `RedisJobStore.transition` MUST be atomic: a short Lua script (`EVAL`) that + `HGET`s `status`, checks membership, and applies `HSET` + `EXPIRE` in one + server-side step. A `WATCH`-based CAS is acceptable but the Lua form is + preferred (no retry loop). +- `InMemoryJobStore.transition` is a plain check-then-merge (single process, + single loop — already atomic between awaits). +- Terminal absorption follows for free: no transition lists `COMPLETED` or + `FAILED` in `from_statuses`. Progress-only updates (`percent_complete`, + stage messages, no status change) keep using `update`, but MUST NOT include + a `status` field; the orchestrator is the only status writer inside the + worker (ADR-003 invariant I-4 already requires this). +- A `transition(...) == False` at task entry means the record is already + terminal or missing: the task MUST return immediately without running any + stage (see §3). + +### 2. Retry contract (binding) + +What arq 0.28 actually does, and how each failure class maps onto it: + +| Failure class | Example | Mechanism | Outcome | +|---|---|---|---| +| Permanent input/domain error | `ValidationError`, unreadable file, corrupt media | ordinary raise (as today) | `FAILED` on attempt 1; no retry. Correct — re-running cannot fix the input. | +| Deadline exhausted | `JobTimeoutError` from ADR-003's `StageDeadline` | ordinary raise, caught, `FAILED` written | **No retry.** The budget is sized to the audio; a re-run repeats the same work against the same budget and re-bills Deepgram. This is why ADR-003 raises *inside* the job instead of letting ARQ's cancellation fire: it converts timeout into a handleable, terminal failure. | +| Transient external failure | Deepgram 429 / 5xx / connection reset | raise `arq.worker.Retry(defer=...)` from the orchestrator with exponential defer (`min(2 ** (attempt - 1) * 10, 300)` seconds — `attempt` from `job_try` is 1-based, so the first retry defers 10 s) | re-queued; `max_tries` caps total attempts. This is the **only** place `Retry` may be raised. | +| Worker death / shutdown | SIGTERM, OOM-kill, ARQ backstop cancellation (`job_timeout = budget + grace`, ADR-003 §2) | ARQ cancellation + `retry_jobs = True` | redelivered to a live worker; task entry re-runs from the top (§3). This — not "failed jobs" — is what `retry_jobs` is for. | + +Consequences to codify: + +- `retry_jobs = True` stays, **documented as worker-death redelivery**, not + error retry. Fix the lying docs: `worker.py` module docstring ("job retries + with exponential backoff") and the `job_max_retries` field description in + `config.py` ("maximum retry attempts for failed jobs" → "maximum delivery + attempts per job; consumed by worker-death redelivery and declared-transient + external failures, never by validation or deadline failures"). +- Only the orchestrator classifies errors. Services keep raising domain + exceptions (`services/CLAUDE.md`); `_transcribe`'s caller inspects + `ExternalServiceError` for the transient subset (HTTP status ≥ 500, 429, + connection errors) and converts *those alone* into `Retry`. Everything else + falls through to the existing `FAILED` handlers. +- **Attempt visibility (invariant L-2):** at task entry the worker stamps + `attempt = ctx["job_try"]` and `max_attempts = settings.job_max_retries` + onto the record. `/status` exposes them. A record on attempt 3 of 3 is + diagnosable; today attempts are invisible. +- When ARQ exhausts `max_tries` on the cancellation path it never runs the + task again, so the worker cannot write the final `FAILED` — the lazy reaper + (§4) is the designated writer for that case. + +### 3. Idempotency and dedup (binding) + +- **Tie the ARQ job id to the application job id (invariant L-3):** + `enqueue_task` MUST pass `_job_id=job_id`. Effects: a double `POST`-side + enqueue of the same job id is refused by ARQ (`enqueue_job` returns `None` — + the existing `RuntimeError` path already handles it); the ARQ job becomes + addressable (`arq.jobs.Job(job_id, redis)`) for the future abort endpoint; + operators can correlate ARQ internals with API job ids. `keep_result` makes + a completed id un-reenqueueable until its result TTL expires — harmless + here because job ids are fresh UUIDs per upload, and it is the correct + guard against accidental re-submission of a finished id. +- **Terminal check at task entry (invariant L-4):** the first act of + `process_audio_job` is + `transition(job_id, from_statuses=NON_TERMINAL, to_status=PREPROCESSING, attempt=..., ...)`. + If it returns `False`, log and return — the record is terminal (the + worst-case redelivery: prior attempt finished but the ack was lost) or gone + (TTL). This closes the "redelivery drags a `COMPLETED` job back to + `PREPROCESSING`" hole and skips a second Deepgram bill in the + finished-but-unacked case. +- **Stages must tolerate re-runs.** Conversion output paths become + deterministic per job (§5 naming), created with overwrite semantics, so a + re-run replaces a half-written file from a dead attempt instead of leaking a + new one. Quality assessment and artifact generation are pure. The Deepgram + call is the one non-idempotent, billed side effect; after L-4 the residual + double-billing window is [Deepgram returns → `COMPLETED` written], a few + store round-trips wide. **Accepted risk**, bounded by `max_tries`; not worth + a distributed transaction. `# #CRITICAL: Payment/Financial:` tag this at the + call site. + +### 4. Stranded-record reaping (binding) + +The reaper is **lazy, on read** — it needs no extra process and works even +when the worker fleet is gone (which is exactly when records strand): + +- `GET /status/{id}` (and `/results`), after fetching a record that is + **non-terminal**, computes staleness from `progress.updated_at` (fallback + `created_at`). If older than `job_timeout_seconds + JOB_TIMEOUT_GRACE` + (ADR-003's window — beyond it, ADR-003 invariant I-2 guarantees no live + attempt can still be running), the handler performs + `transition(from_statuses=NON_TERMINAL, to_status=FAILED, error="job lost: no worker progress within the deadline window")` + and serves the result of that transition. The guard makes concurrent + reads/worker writes race-safe: whoever wins, the record ends valid. +- `QUEUED` records are covered by the same rule. This also puts a floor under + the `enqueue_enabled=False` misconfiguration (review Finding 2): a job that + nothing will ever process stops lying to pollers after one deadline window. +- Records nobody polls die by TTL, as today. That is acceptable: reaping + exists for observers, and TTL already bounds storage. +- The worker startup sweep from ADR-003 §5 handles the **files**; the lazy + reaper handles the **records**. Neither depends on the other. + +### 5. File ownership contract (binding) + +Precondition (invariant L-5): the API and worker MUST resolve the same +`audio_temp_dir` — this is a real filesystem coupling and both compose files +MUST set `AUDIO_TEMP_DIR=/app/temp` explicitly (review Finding 2). The +`platformdirs` default is per-container and silently breaks handoff. + +Deterministic naming, keyed to the job: input `{job_id}.input{suffix}`, +converted `{job_id}.converted.wav`, both under `audio_temp_dir`. Retries +overwrite rather than accumulate, and any sweep can map file → job record. + +| File | Created by | Owner… | …until | Then deleted by | +|---|---|---|---|---| +| input temp file | API (`mkstemp` → renamed to `{job_id}.input{suffix}`) | API | successful return of `POST /process` (record stored, enqueue done or disabled) | **worker**, in a `finally` on every exit of `process_audio_job` — success, failure, `JobTimeoutError`, and the L-4 already-terminal early return | +| input temp file (failure before handoff) | API | API | any exception in the route | API `finally` (existing path, unchanged) | +| converted file | worker conversion stage | worker | task exit | same worker `finally` (today it is unlinked only on the happy path) | +| orphans (dead attempt, `abandon_on_cancel` stragglers) | either | nobody | file mtime older than deadline + grace | worker startup sweep (ADR-003 §5), which with deterministic names now also deletes files whose job record is terminal or absent | + +Rules: + +- The `finally` cleanup runs in the shielded scope required by ADR-003 §5, so + cancellation cannot skip it. +- The route's misleading comment is replaced by a pointer to this table. +- Ownership transfers exactly once, at successful route return. Between + enqueue and task start the worker owns a file it has not seen yet — that is + fine; the sweep covers the crash window. + +## Acceptance criteria + +1. **Terminal absorption**: seed a `COMPLETED` record; run `process_audio_job` + for that id; assert the task returns without invoking any stage (mock + converter/assessor assert-not-called) and status is still `COMPLETED`. +2. **Guarded transition atomicity**: two concurrent `transition` calls on the + same record (worker→`COMPLETED` vs reaper→`FAILED`) — exactly one wins; + the record never holds a mixed state. Redis-backed test via the Lua path. +3. **No stranded timeout**: force `JobTimeoutError` in a stage; assert the + record ends `FAILED` with the deadline error, and ARQ did **not** re-queue. +4. **Transient retry**: make the transcription stage raise a transient + `ExternalServiceError`; assert `Retry` is raised with the expected defer + and the record is non-terminal with `attempt` stamped. +5. **Lazy reaper**: seed a non-terminal record with `updated_at` older than + deadline + grace; `GET /status` returns `FAILED` with the "job lost" error + and the store reflects it; a second `GET` is a pure read. +6. **File lifecycle**: run a job to success and to failure; assert both the + input and converted files are gone in both cases; assert the API failure + path still cleans its own upload. +7. **Enqueue dedup**: enqueue the same job id twice; the second attempt + surfaces the existing `RuntimeError` path, and exactly one ARQ job exists. + +## Implementation tasks (ordered) + +1. Add `transition(...)` to `JobStore`/`InMemoryJobStore`/`RedisJobStore` + (Lua script for Redis), plus `NON_TERMINAL`/`TERMINAL` frozensets exported + next to `JobStatus`. +2. Wire `_job_id=job_id` through `enqueue_task` (worker.py). +3. Rework `process_audio_job`: L-4 terminal check + attempt stamping at entry; + all status writes via `transition`; transient-`ExternalServiceError` → + `Retry(defer=...)` classification; `finally` deleting input and converted + files (shielded per ADR-003). +4. Rename API temp files to the deterministic `{job_id}.input{suffix}` scheme; + replace the ownership comment with a reference to ADR-004 §5. +5. Add the lazy reaper to `/status` and `/results` handlers. +6. Extend the ADR-003 startup sweep to also delete files whose job record is + terminal or absent (deterministic names make the lookup trivial). +7. Fix the misdocumented retry semantics (`worker.py` docstring, `config.py` + `job_max_retries` description). +8. Set `AUDIO_TEMP_DIR=/app/temp` in both compose files (app and worker + services) — may land with the review Finding 2 fix if that ships first. +9. Tests for acceptance criteria 1–7. + +## Alternatives considered + +- **A dedicated `RETRYING` state.** Rejected: attempt metadata (`attempt`/ + `max_attempts` fields) carries the same information without doubling the + transition table; pollers care whether the job is done, not which delivery + is running. +- **Periodic reaper via ARQ `cron_jobs`.** Rejected as the primary mechanism: + it runs only while a worker is alive — precisely the condition under which + records strand least — and requires `SCAN`ning the keyspace. Lazy + reap-on-read covers every observed job with zero extra infrastructure; TTL + covers the unobserved. A cron sweep can be added later without contract + changes. +- **Exactly-once transcription via a Deepgram-call ledger.** Rejected: the + residual double-bill window after L-4 is a few milliseconds wide and only + reachable through worker death at that instant; a ledger adds a second + consistency domain to protect against a bounded, rare, non-corrupting cost. +- **Dropping `retry_jobs` entirely.** Rejected: it is the only redelivery + mechanism for worker death; without it an OOM-killed worker strands every + in-flight job with no recovery path but the reaper. diff --git a/docs/planning/test-plan-review-gaps.md b/docs/planning/test-plan-review-gaps.md new file mode 100644 index 0000000..de92a8d --- /dev/null +++ b/docs/planning/test-plan-review-gaps.md @@ -0,0 +1,247 @@ +--- +title: "Test Plan: Closing the Suite Gaps Behind the 2026-07-02 Review Findings" +schema_type: planning +status: published +owner: core-maintainer +purpose: "Diagnose why the existing test suite missed review findings 1-4, and specify the tiers, fixtures, and concrete tests that catch each failure class — not just each instance." +tags: + - testing + - reliability + - planning +component: Development-Tools +source: "Systems design review 2026-07-02" +--- + +> **Status**: Accepted +> **Date**: 2026-07-02 +> **Relates to**: ADR-003 (async execution model), ADR-004 (job lifecycle), +> systems design review findings 1–4 + +## Purpose + +The suite has ~20 unit test modules, an 80% coverage gate, and green CI — and +it shipped a system in which **every enqueue-enabled submission crashes** +(Finding 1) and **the shipped compose deployment cannot process a job +end-to-end** (Finding 2). Coverage measured the wrong thing: lines executed, +not behavior exercised. This plan names the four structural gaps that let +findings 1–4 through, then specifies the tests that close each *class*. The +per-feature acceptance tests in ADR-003 §Acceptance and ADR-004 §Acceptance +are incorporated by reference, not duplicated. + +## Part 1 — Why the suite missed the failures + +### G1. Self-masking mocks: patching the subject's own logger + +`tests/unit/test_worker.py:58,75,90` wrap every `enqueue_task` test in +`patch("audio_processor.jobs.worker.logger")`. The bug in Finding 1 **is a +logging call**: `worker.py` binds a stdlib logger (`logging.getLogger`) but +invokes it structlog-style (`logger.info("task_enqueued", task=..., job_id=...)`), +which raises `TypeError` on every real call — after the job is already +enqueued. The patch replaced the exact statement under test with a +`MagicMock` that accepts anything, so the test asserted the code works while +deleting the code. The `# type: ignore[call-arg]` on that line was the type +checker reporting the same bug; the suppression plus the mock closed both +detection channels. + +The irony: `tests/conftest.py` already ships an autouse `setup_logging` +fixture, so real logging *works* in every test. Simply not patching would +have surfaced the `TypeError` in three existing tests. + +### G2. Tautological configuration tests; the queue path never executes + +`test_worker.py:104-113` asserts `WorkerSettings.job_timeout == 600` — a +value copied from `settings.job_timeout_seconds`, compared against itself. +Every worker test calls hooks or `enqueue_task` with an `AsyncMock` Redis; no +test anywhere runs an ARQ worker, so nothing verifies that an enqueued job is +*picked up, executed, and lands in a terminal state*. That unexecuted seam is +where Finding 1 lived, and it is also why Finding 3's headline consequence — +`job_timeout` is configured but physically unenforceable while a coroutine is +blocked in sync code — was invisible: the timeout was asserted as a number, +never observed as a behavior. `tests/unit/_fake_redis.py` models only the +hash commands the job store uses; it cannot back ARQ (sorted sets, blocking +pops), so the queue path needs the integration tier (Part 2). + +### G3. Deployment descriptors are outside the test boundary + +`docker-compose.yml` and `docker-compose.prod.yml` are the *actual shipped +configuration* — and no test reads them. Finding 2 is three consistency +violations between those files and `config.py` defaults (enqueue disabled by +default, memory store with `replicas: 2`, unshared `audio_temp_dir`), all +statically checkable in milliseconds, none checked. `tests/integration/` +contains only `__init__.py`; the `integration` and `smoke` markers registered +in `conftest.py` have zero tests behind them. + +### G4. No invariant assertions: leaks and loop stalls are unobserved + +Tests assert return values, never *system invariants*. Nothing asserts "the +temp directory is empty after the job finishes" — so Finding 4 (input file +leaked on every successful job, converted file leaked on every failure) was +undetectable by construction. Nothing asserts "the event loop stayed +responsive while work was in flight" — so Finding 3's health-check starvation +was likewise unobservable. Invariants must be fixtures that *fail teardown*, +not assertions a test author has to remember. + +## Part 2 — Test architecture + +Three tiers, using the markers `conftest.py` already registers: + +| Tier | Marker | Infra | Runs | +|---|---|---|---| +| Unit | `unit` | none; external I/O mocked at the `services/` boundary (existing convention) | every push (existing CI job) | +| Integration | `integration` | real Redis (CI service container), real ARQ worker in burst mode, real ffmpeg on a checked-in <5 s fixture; Deepgram mocked at the client boundary | every push, separate CI job | +| Deployment smoke | `smoke` | `docker compose up` of the real compose files | on changes to `docker-compose*`, `Dockerfile*`, `config.py`; plus nightly | + +New shared fixtures (in `tests/conftest.py` / `tests/integration/conftest.py`): + +- **`temp_audio_dir` (autouse in integration tier)** — points + `settings.audio_temp_dir` at `tmp_path` and, on teardown, **fails the test + if any file remains** after the job under test reached a terminal state. + This is the G4 leak invariant as infrastructure: every integration test + checks it for free. +- **`arq_burst_worker`** — builds `arq.worker.Worker` from the real + `WorkerSettings.functions` against the test Redis and runs it with + `burst=True`, so "enqueue → execute → terminal state" is one awaited call. +- **`loop_watchdog`** — background task ticking every 25 ms and recording the + maximum inter-tick gap; teardown fails if the gap exceeded 100 ms + (ADR-003 invariant I-1 as a fixture). +- **`tiny_wav`** — 1-second silent WAV generated once per session with ffmpeg + into `tmp_path` (no binary blob in git; skips cleanly if ffmpeg is absent, + which only the integration tier requires). + +## Part 3 — Test specifications + +Each entry names the finding it retro-catches and why it fails against +today's code — the proof it would have caught the bug. + +**T-1 · Enqueue logs through real logging — F1 · unit.** +Call `enqueue_task` with a mocked Redis (that part is fine) and **no logger +patch**; assert the returned job id and, via `caplog`, that a `task_enqueued` +record was emitted. Fails today with `TypeError: Logger._log() got an +unexpected keyword argument 'task'`. Companion sweep: remove the logger +patches from the three existing enqueue tests; they become regression tests +for the same class. Broader guard: a unit test that imports every module +under `audio_processor` and asserts each module-level `logger` is a structlog +`BoundLogger` (i.e. produced by `utils.logging.get_logger`) — kills the +stdlib/structlog mixup everywhere at once. + +**T-2 · Queue path executes end-to-end — F1, F3 · integration.** +Store a `QUEUED` record via `RedisJobStore`, `enqueue_task(..., +"process_audio_job", job_id, record)`, run `arq_burst_worker`, then assert +the record is `COMPLETED` (Deepgram client mocked; ffmpeg real, `tiny_wav` +input). This single test executes the seam where Finding 1 lived: real ARQ +serialization, real task dispatch, real logging, real store writes. Fails +today on the F1 `TypeError` before the worker even starts. + +**T-3 · Compose files are consistent with the config model — F2 · unit.** +Parse both compose files with `yaml.safe_load` (dev + prod overlay merge) and, +for each service running app code, build `Settings(**environment)`; assert: +(a) if the service topology includes a worker, the API's settings have +`enqueue_enabled=True` and `job_store_backend="redis"` — else the worker is +decorative; (b) API and worker resolve the **same** `audio_temp_dir` and both +mount a shared volume at that path (ADR-004 invariant L-5); (c) any service +with `replicas > 1` has `job_store_backend="redis"` — process-local state +with two replicas is split-brain by construction; (d) the prod overlay sets +`AUTH_REQUIRED=true` (review Finding 8, same class: shipped-config drift). +All four assertions fail against today's compose files, which is Finding 2 +verbatim. This is milliseconds-cheap and belongs in the unit job: deployment +descriptors are test inputs from now on. + +**T-4 · Deployment smoke: one job through the shipped stack — F2 · smoke.** +`docker compose up` the real files (with a `.env.test` supplying dummy +secrets and a mocked-Deepgram flag), wait for `/health`, POST `tiny_wav` to +`/api/v1/process`, poll `/status/{id}` until `COMPLETED` (bounded), fetch +`/results/{id}`, assert `/health` latency stayed < 1 s throughout, and assert +the shared temp volume is empty afterward. This is the only test that +validates image + compose + env + volume wiring as a unit; it catches the +entire Finding-2 class including mistakes T-3's static model cannot see +(missing volume mounts, wrong service names, broken images). Today it cannot +pass — which is the review's point — so it lands together with the Finding-2 +compose fix and gates regressions thereafter. + +**T-5 · Loop responsiveness under load — F3 · integration.** +Two probes: (a) API — start a `process_audio` upload whose validation ffprobe +is slowed to ~2 s (stub executable on `PATH`), and concurrently assert +`/health` answers in < 100 ms (ADR-003 acceptance test 1); (b) worker — run +T-2's job with `loop_watchdog` active; the fixture fails on any >100 ms +stall. Both fail against today's inline `subprocess.run` calls and pass once +ADR-003 §1 lands. + +**T-6 · Timeout is enforced, not configured — F3 · integration.** +Set a small `job_timeout_seconds`, make the conversion stage block past it +(stub ffmpeg that sleeps), run the burst worker: assert the job reaches +`FAILED` with the deadline error within budget + grace, and was not +re-queued (ADR-003 acceptance test 4, ADR-004 acceptance test 3). Fails +today: the blocked coroutine never receives ARQ's cancellation, the "timed +out" job runs to whatever end ffmpeg finds, and the record strands +non-terminal. This test is the difference between asserting +`job_timeout == 600` (G2) and asserting a timeout *happens*. + +**T-7 · No file survives its job — F4 · integration.** +Explicit success-path and failure-path runs of T-2's pipeline asserting the +input file and the converted file are both gone at terminal state (ADR-004 +acceptance test 6) — plus the `temp_audio_dir` autouse fixture enforcing the +same invariant on every other integration test as a side effect. Fails today +on the success path: the input file is never deleted (the "worker takes +ownership" comment is aspirational), and on the failure path the converted +file survives. + +Coverage note: T-1/T-3 are cheap and run with the existing unit job; T-2, +T-5–T-7 share the Redis service container and the burst-worker fixture; T-4 +is the only docker-in-CI cost and is path-gated + nightly. + +## Part 4 — Binding suite rules (mirrored into `tests/CLAUDE.md`) + +1. **Never patch a logger belonging to the module under test.** Logging + calls are executable statements; a patched logger deletes them from the + test (Finding 1's mask). Assert on `caplog` / + `structlog.testing.capture_logs`. Patching a *collaborator's* logger to + silence noise is equally disallowed — use log levels. +2. **A configuration value may only be tested through the behavior it + controls.** `assert WorkerSettings.job_timeout == 600` is comparing a + variable to itself; the honest test is T-6. +3. **Deployment descriptors are test inputs.** Any file that configures a + shipped process (`docker-compose*.yml`, `Dockerfile*`, `.env.example`) + is covered by T-3-style consistency tests; editing one without a test + run is the Finding-2 failure mode. +4. **Resource invariants are fixtures, not assertions.** Leak checks + (`temp_audio_dir`) and loop-stall checks (`loop_watchdog`) fail in + teardown so no test author has to remember them. +5. **A `# type: ignore` / `# pyright: ignore` on a line that then gets + mocked in tests is a red flag, not a fix.** Finding 1 was visible to the + type checker and suppressed; the mock hid it at runtime. Suppressions on + call-argument errors require a comment proving the call is exercised + un-mocked somewhere. + +## Part 5 — CI wiring + +- **unit job (existing)**: add T-1, T-3; no new infra. +- **integration job (new)**: `services: redis:7-alpine`; installs ffmpeg; + runs `-m integration` (T-2, T-5–T-7). Target < 2 min. +- **smoke job (new)**: `docker compose -f docker-compose.yml -f + docker-compose.prod.yml up` with `.env.test`; runs `-m smoke` (T-4); + triggered by paths `docker-compose*`, `Dockerfile*`, + `src/audio_processor/core/config.py`, plus `schedule: nightly`. +- Coverage gate unchanged; integration tier reports under the existing + `codecov` flags. + +## Part 6 — Implementation tasks (ordered) + +1. Add rules 1–5 to `tests/CLAUDE.md` (done alongside this plan). +2. T-1 + logger-patch removal sweep + module-logger type test. *Lands with + the Finding-1 one-line fix (`get_logger(__name__)` in `worker.py`); the + test must be observed red first.* +3. T-3 compose-consistency tests. *Lands with the Finding-2 compose fix; + observed red first.* +4. Integration conftest: `temp_audio_dir`, `arq_burst_worker`, + `loop_watchdog`, `tiny_wav`; CI integration job with Redis service. +5. T-2 queue-path test (first consumer of the fixtures). +6. T-7 file-lifecycle tests — red until ADR-004 task 3 (worker `finally` + cleanup) lands; implement together. +7. T-5/T-6 — red until ADR-003 tasks land; implement together (they are + ADR-003 acceptance tests 1 and 4 made concrete). +8. T-4 smoke test + `.env.test` + smoke CI job — lands last, after the + Finding-2 fix makes the stack processable. + +The "observed red first" discipline is the point of the plan: each test is +committed failing against the pre-fix code (locally, or in the fix PR's first +commit), proving it detects the defect it is assigned to. diff --git a/osv-scanner.toml b/osv-scanner.toml index 4925558..40b64fa 100644 --- a/osv-scanner.toml +++ b/osv-scanner.toml @@ -89,3 +89,31 @@ DISPUTED vulnerability affecting SVN path parsing. interrogate (dev dependency) transitively requires py, but this project does not interact with Subversion repositories. """ + +# ---------------------------------------------------------------------------- +# torch - CVE-2025-3000 (no patched release; local-only) +# ---------------------------------------------------------------------------- +[[IgnoredVulns]] +id = "CVE-2025-3000" +reason = """ +Local-only vulnerability in torch (CVSS 4.0 AV:L/AC:L/PR:L, low C/I/A impact); +OSV lists no fixed release (last_affected only). torch enters via the [ml] extra +(direct) and the [audio] extra (transitive via silero-vad); the production +Dockerfile runs uv sync --frozen --no-dev with no --extra flags, so torch is not +installed at runtime. Same compensating controls as PYSEC-2026-139. Documented in +docs/known-vulnerabilities.md with reassess-by 2026-08-31. +""" + +[[IgnoredVulns]] +id = "GHSA-rrmf-rvhw-rf47" +reason = """ +GitHub Security Advisory alias of CVE-2025-3000 (torch, local-only, no patched +release). See the CVE-2025-3000 entry above and docs/known-vulnerabilities.md. +""" + +[[IgnoredVulns]] +id = "PYSEC-2025-194" +reason = """ +PyPI advisory alias of CVE-2025-3000 (torch, local-only, no patched release). +See the CVE-2025-3000 entry above and docs/known-vulnerabilities.md. +""" diff --git a/pyproject.toml b/pyproject.toml index 07a4a8f..7430f38 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -650,6 +650,12 @@ ignore-vuln = [ # (Dockerfile: uv sync --frozen --no-dev, no --extra flags). Documented in # docs/known-vulnerabilities.md with reassess-by 2026-07-26. "PYSEC-2026-139", + # CVE-2025-3000 (aliases GHSA-rrmf-rvhw-rf47, PYSEC-2025-194): local-only (CVSS 4.0 + # AV:L/PR:L, low impact) vulnerability in torch with no patched release. Same + # exposure analysis as PYSEC-2026-139: torch is only reachable via the [ml]/[audio] + # extras, which the production container never installs. Documented in + # docs/known-vulnerabilities.md with reassess-by 2026-08-31. + "CVE-2025-3000", ] # Bandit Security Configuration diff --git a/src/audio_processor/services/CLAUDE.md b/src/audio_processor/services/CLAUDE.md index d0d77a0..24de107 100644 --- a/src/audio_processor/services/CLAUDE.md +++ b/src/audio_processor/services/CLAUDE.md @@ -19,6 +19,16 @@ class MyService: - I/O-bound methods that call external APIs or read files: `def` wrapping a sync SDK (current pattern) or `async def` if the SDK supports it. - Raise `ExternalServiceError` (from `core.exceptions`) on network failures; never let SDK exceptions propagate raw to the caller. +## Caller-side rule for async contexts (binding — see ADR-003) + +Any `async def` (route handler, ARQ task, lifespan hook) calling a service method +that shells out, does CPU-bound audio work, or performs blocking I/O MUST dispatch +it via `anyio.to_thread.run_sync(..., abandon_on_cancel=True, limiter=)` +and MUST pass an explicit timeout that the service enforces internally. +Calling these methods bare in async code is a review-blocking defect. +Full contract (placement table, deadline budget, cancellation guarantees): +`docs/planning/adr/adr-003-async-execution-model.md`. + ## RAD tagging (mandatory for this layer) All methods that call external APIs or read files must carry `#CRITICAL: ExternalResources` tags. diff --git a/tests/CLAUDE.md b/tests/CLAUDE.md index 23e3009..b273e4a 100644 --- a/tests/CLAUDE.md +++ b/tests/CLAUDE.md @@ -38,3 +38,25 @@ Regenerating a golden file counts as a test change and requires reviewer sign-of - Run linters, type checkers, or formatters inside test functions; that belongs in pre-commit. - Import from `tests/` in production code. - Use `time.sleep` in tests; use `freezegun` or mock time instead. + +## Anti-masking rules (binding — see docs/planning/test-plan-review-gaps.md) + +Added after the 2026-07-02 systems review, whose Finding 1 shipped because +tests patched the exact statement under test: + +1. **Never patch a logger belonging to the module under test** (nor a + collaborator's, to silence noise — use log levels). Logging calls are + executable statements; assert on `caplog` or + `structlog.testing.capture_logs` instead. +2. **Test configuration values only through the behavior they control.** + Asserting a settings-derived attribute equals the setting it was copied + from is a tautology, not a test. +3. **Deployment descriptors are test inputs.** `docker-compose*.yml`, + `Dockerfile*`, and `.env.example` must stay consistent with `config.py` + via the compose-consistency tests; edit them and run those tests. +4. **Resource invariants are fixtures, not assertions**: temp-file leak + checks and event-loop stall checks live in autouse fixtures that fail on + teardown, so individual tests get them for free. +5. **A type-suppression comment on a line that is mocked in tests is a red + flag.** Suppressions on call-argument errors require the call to be + exercised un-mocked somewhere in the suite.