From b8be27f0c98164b7373628c9bf2b328a1e6fa081 Mon Sep 17 00:00:00 2001 From: Jim Lancaster <40281233+zoltan57@users.noreply.github.com> Date: Tue, 11 Aug 2026 16:42:09 -0500 Subject: [PATCH] Continue GC code review and cleanup --- README.md | 17 +- src/transcription/app.py | 17 +- src/transcription/db/runtime.py | 18 +- src/transcription/db/session.py | 6 + src/transcription/providers/base.py | 2 +- src/transcription/providers/openrouter.py | 21 ++- src/transcription/services/base.py | 5 +- src/transcription/services/jobs.py | 26 ++- src/transcription/services/sources.py | 8 +- src/transcription/worker.py | 22 --- tests/providers/test_openrouter.py | 22 +++ tests/services/test_job_service.py | 15 +- tests/test_app.py | 39 +++-- tests/tools/test_run_destructive_tests.py | 50 ++++++ tests/ui/conftest.py | 60 ++++--- tools/run_destructive_tests.py | 196 +++++++++++++++------- 16 files changed, 363 insertions(+), 161 deletions(-) create mode 100644 tests/tools/test_run_destructive_tests.py diff --git a/README.md b/README.md index 4fa1528..0b1e826 100644 --- a/README.md +++ b/README.md @@ -145,14 +145,15 @@ The canonical MVP prompt is: ## Destructive test procedure (with data backup) -AI execution policy: before running any unit tests, create a backup of `./data` first. After tests succeed, always pause and ask whether to restore now. +AI execution policy: before the first unit-test run in a test/fix cycle, create one backup of `./data`. Reuse that same backup for every subsequent test run in the cycle. After tests succeed, always pause and ask whether to restore now. Use the cross-platform Python wrapper below whenever an AI agent runs tests against this repository. -1. Create backup of `./data`. +1. Create one backup of `./data` and mark it as the active test-cycle backup. 2. Run your test command. -3. On success, always prompt whether to restore now (do not auto-restore unless explicitly approved). -4. On failure, keep backup and current state for inspection. +3. On failure, fix the errors and run the wrapper again; it reuses the active backup and never backs up post-test data. +4. On success, always prompt whether to restore now (do not auto-restore unless explicitly approved). +5. Close the cycle only by restoring the active backup or explicitly accepting the current data. Preflight behavior: @@ -183,10 +184,18 @@ uv run python tools/run_destructive_tests.py --skip-restore-prompt -- pytest This keeps both the current post-test state and the backup, so restore can be decided explicitly later. +Repeated wrapper invocations reuse the backup recorded in `.test-backups/.active-backup`. If that backup is missing, the wrapper stops rather than creating a replacement from potentially destructive post-test data. + ### Restore later from a saved backup ```bash uv run python tools/run_destructive_tests.py --restore-from data-backup-YYYYMMDD-HHMMSS ``` +To keep the current data and close the active cycle without restoring: + +```bash +uv run python tools/run_destructive_tests.py --accept-current-data +``` + Backups are stored in `.test-backups/` and ignored by git. diff --git a/src/transcription/app.py b/src/transcription/app.py index b09f0e1..32af016 100644 --- a/src/transcription/app.py +++ b/src/transcription/app.py @@ -24,7 +24,10 @@ from .db import create_all from .db import dispose_database_runtime from .db import initialize_database_runtime from .services import ServiceBundle +from .services.documents import DocumentService from .services.jobs import JobService +from .services.people import PeopleService +from .services.sources import SourceService from .ui import register_pages from .worker import worker_consumer_lifespan @@ -36,8 +39,14 @@ async def _lifespan(app: FastAPI): settings = getattr(app.state, "settings", None) or get_settings() configure_logging(settings) app.state.settings = settings - app.state.services = ServiceBundle() app.state.runtime = initialize_database_runtime(settings=settings) + session_factory = app.state.runtime.session_factory + app.state.services = ServiceBundle( + documents=DocumentService(session_factory=session_factory, settings=settings), + sources=SourceService(session_factory=session_factory, settings=settings), + jobs=JobService(session_factory=session_factory, settings=settings), + people=PeopleService(session_factory=session_factory, settings=settings), + ) if settings.should_bootstrap_schema: await create_all(engine=app.state.runtime.engine) @@ -93,12 +102,8 @@ def create_app(settings: Settings | None = None) -> FastAPI: async def ui_redirect() -> RedirectResponse: return RedirectResponse(url="/ui/homepage", status_code=status.HTTP_307_TEMPORARY_REDIRECT) - @app.get("/healthz") - def health() -> dict[str, str]: - return {"status": "ok"} - register_error_handlers(app) - register_pages(app) app.include_router(health_router) app.include_router(v4_documents_router) + register_pages(app) return app diff --git a/src/transcription/db/runtime.py b/src/transcription/db/runtime.py index 7bcc10f..7b1356c 100644 --- a/src/transcription/db/runtime.py +++ b/src/transcription/db/runtime.py @@ -1,5 +1,4 @@ import logging -from contextvars import ContextVar from dataclasses import dataclass from sqlalchemy.ext.asyncio import AsyncEngine @@ -23,21 +22,28 @@ class DatabaseRuntime: session_factory: async_sessionmaker[AsyncSession] -_runtime: ContextVar[DatabaseRuntime | None] = ContextVar("database_runtime", default=None) +_runtime: DatabaseRuntime | None = None + + +def get_database_runtime() -> DatabaseRuntime | None: + """Return the process-owned database runtime.""" + return _runtime async def dispose_database_runtime() -> None: """Dispose lifespan-owned async database resources.""" - runtime = _runtime.get() + global _runtime + runtime = _runtime if runtime is None: return await runtime.engine.dispose() - _runtime.set(None) + _runtime = None def initialize_database_runtime(*, settings: Settings | None = None) -> DatabaseRuntime: """Initialize lifespan-owned async DB resources once per process.""" - runtime = _runtime.get() + global _runtime + runtime = _runtime if runtime is not None: return runtime @@ -46,6 +52,6 @@ def initialize_database_runtime(*, settings: Settings | None = None) -> Database engine = get_engine(database_url) session_factory = get_session_factory(database_url) runtime = DatabaseRuntime(engine=engine, session_factory=session_factory) - _runtime.set(runtime) + _runtime = runtime logger.debug("Initialized async database runtime for database_url=%s", engine.url) return runtime diff --git a/src/transcription/db/session.py b/src/transcription/db/session.py index 366ddd2..4007759 100644 --- a/src/transcription/db/session.py +++ b/src/transcription/db/session.py @@ -33,6 +33,12 @@ def resolve_session_factory( ) -> SessionFactory: if database_url is not None: return get_session_factory(database_url) + if settings is None: + from .runtime import get_database_runtime + + runtime = get_database_runtime() + if runtime is not None: + return runtime.session_factory return get_session_factory(get_database_url(settings or get_settings())) diff --git a/src/transcription/providers/base.py b/src/transcription/providers/base.py index 10d023a..d86095f 100644 --- a/src/transcription/providers/base.py +++ b/src/transcription/providers/base.py @@ -23,8 +23,8 @@ class TranscriptionResult: text: str provider: str - prompt_name: str model: str + prompt_name: str | None = None prompt_hash: str | None = None system_prompt: str | None = None user_prompt: str | None = None diff --git a/src/transcription/providers/openrouter.py b/src/transcription/providers/openrouter.py index f4cd1db..fedc292 100644 --- a/src/transcription/providers/openrouter.py +++ b/src/transcription/providers/openrouter.py @@ -95,7 +95,7 @@ class OpenRouterTranscriptionProvider: return TranscriptionResult( text=text, provider="openrouter", - prompt_name="", + prompt_name=None, prompt_hash=None, system_prompt=None, user_prompt=prompt_text, @@ -158,7 +158,8 @@ class OpenRouterTranscriptionProvider: if callable(serializer): try: return self._to_json_compatible(serializer()) - except Exception: # noqa: BLE001 + except Exception as exc: # noqa: BLE001 + logger.debug("OpenRouter response serializer %s failed: %s", method_name, exc) continue object_dict = getattr(value, "__dict__", None) @@ -182,13 +183,27 @@ class OpenRouterTranscriptionProvider: ) -> OpenRouterRequest: image_b64 = base64.b64encode(image_bytes).decode("ascii") data_url = f"data:{mime_type};base64,{image_b64}" + media_content: dict[str, Any] + if mime_type == "application/pdf": + media_content = { + "type": "file", + "file": { + "filename": "source.pdf", + "file_data": data_url, + }, + } + else: + media_content = { + "type": "image_url", + "image_url": {"url": data_url}, + } messages: list[dict[str, Any]] = [ { "role": "user", "content": [ {"type": "text", "text": prompt_text}, - {"type": "image_url", "image_url": {"url": data_url}}, + media_content, ], } ] diff --git a/src/transcription/services/base.py b/src/transcription/services/base.py index 693c062..afcb941 100644 --- a/src/transcription/services/base.py +++ b/src/transcription/services/base.py @@ -23,9 +23,10 @@ class ServiceBase(ABC): self, session_factory: async_sessionmaker[AsyncSession] | None = None, queue: asyncio.Queue | None = None, + settings: Settings | None = None, ): - self.settings = get_settings() - self.session_factory = session_factory or resolve_session_factory() + self.settings = settings or get_settings() + self.session_factory = session_factory or resolve_session_factory(settings=self.settings) self.queue = queue or asyncio.Queue() @asynccontextmanager diff --git a/src/transcription/services/jobs.py b/src/transcription/services/jobs.py index 5a3b207..5a81d21 100644 --- a/src/transcription/services/jobs.py +++ b/src/transcription/services/jobs.py @@ -7,13 +7,13 @@ from sqlalchemy.orm import selectinload from sqlmodel import select from sqlmodel.ext.asyncio.session import AsyncSession -from ..errors import AppError -from ..errors import ErrorCategory from ..db.models import Job from ..db.models import JobSource from ..db.models import JobSourceStatus from ..db.models import JobStatus from ..db.models import Source +from ..errors import AppError +from ..errors import ErrorCategory from .base import ServiceBase @@ -29,6 +29,10 @@ class JobResubmitBlockedError(AppError): """Raised when a job resubmit operation is blocked by lifecycle policy.""" +class JobNotFoundError(AppError): + """Raised when a requested Job does not exist.""" + + class JobService(ServiceBase): """Thin service class for managing jobs in the database.""" @@ -61,7 +65,7 @@ class JobService(ServiceBase): ) job = (await _session.exec(query)).first() if job is None: - raise ValueError(f"Job with id {job_id} not found") + raise self._not_found(job_id) return job async def update_job(self, job: Job, session: AsyncSession | None = None) -> Job: @@ -148,7 +152,7 @@ class JobService(ServiceBase): ) job = (await _session.exec(query)).first() if job is None: - raise ValueError(f"Job with id {job_id} not found") + raise self._not_found(job_id) job.status = status if retry_count_increment: job.retry_count += retry_count_increment @@ -216,7 +220,7 @@ class JobService(ServiceBase): ) job = (await _session.exec(query)).first() if job is None: - raise ValueError(f"Job with id {job_id} not found") + raise self._not_found(job_id) if job.status == JobStatus.PROCESSING: raise JobDeleteBlockedError( @@ -244,7 +248,7 @@ class JobService(ServiceBase): ) job = (await _session.exec(query)).first() if job is None: - raise ValueError(f"Job with id {job_id} not found") + raise self._not_found(job_id) if job.status in {JobStatus.TRANSCRIBED, JobStatus.COMPLETED}: raise JobCancelBlockedError( @@ -283,7 +287,7 @@ class JobService(ServiceBase): ) job = (await _session.exec(query)).first() if job is None: - raise ValueError(f"Job with id {job_id} not found") + raise self._not_found(job_id) if job.status == JobStatus.PROCESSING: raise JobResubmitBlockedError( @@ -314,3 +318,11 @@ class JobService(ServiceBase): await self._finalize(session=_session, caller_session=session, refresh=(job,)) return len(candidates) + + @staticmethod + def _not_found(job_id: UUID) -> JobNotFoundError: + return JobNotFoundError( + f"Job with id {job_id} not found", + category=ErrorCategory.NOT_FOUND, + suggestion="Verify the Job id and retry.", + ) diff --git a/src/transcription/services/sources.py b/src/transcription/services/sources.py index 503fadc..2a87c5e 100644 --- a/src/transcription/services/sources.py +++ b/src/transcription/services/sources.py @@ -81,8 +81,12 @@ class SourceService(ServiceBase): provider: TranscriptionProvider - def __init__(self, session_factory: async_sessionmaker[AsyncSession] | None = None): - super().__init__(session_factory=session_factory) + def __init__( + self, + session_factory: async_sessionmaker[AsyncSession] | None = None, + settings: Settings | None = None, + ): + super().__init__(session_factory=session_factory, settings=settings) self.provider = get_transcription_provider(settings=self.settings) async def create_source(self, source: Source, *, session: AsyncSession | None = None) -> Source: diff --git a/src/transcription/worker.py b/src/transcription/worker.py index c6f8586..f6e67f5 100644 --- a/src/transcription/worker.py +++ b/src/transcription/worker.py @@ -9,7 +9,6 @@ from contextlib import asynccontextmanager from contextlib import contextmanager from contextlib import suppress from typing import Protocol -from uuid import UUID from sqlalchemy.ext.asyncio import async_sessionmaker from sqlmodel.ext.asyncio.session import AsyncSession @@ -23,7 +22,6 @@ from .services.documents import DocumentService from .services.jobs import JobService from .services.people import PeopleService from .services.sources import SourceService -from .services.workflows import advance_job from .services.workflows import process_next_queued_job as process_next_queued_job_workflow logger = logging.getLogger(__name__) @@ -96,19 +94,6 @@ async def worker_consumer_lifespan( await worker_task -async def queue_consumer_loop(queue: asyncio.Queue[UUID], stop_event: asyncio.Event): - """Main worker loop that consumes jobs from the queue and processes them. - - The queue contains Job UUIDs whose Document and Source records already exist. - """ - service = JobService() - while not stop_event.is_set(): - with handle_worker_exceptions(): - async with _get_queue_item(queue) as job_id: - job = await service.read_job(job_id) - asyncio.create_task(advance_job(job=job, services=ServiceBundle())) - - @contextmanager def handle_worker_exceptions(operation: str = "worker.loop"): """Context manager to log and suppress exceptions in the worker loop.""" @@ -123,13 +108,6 @@ def handle_worker_exceptions(operation: str = "worker.loop"): ) -@asynccontextmanager -async def _get_queue_item(queue: asyncio.Queue[UUID]) -> AsyncGenerator[UUID]: - """Context manager to enqueue a job and ensure it is marked done.""" - yield await queue.get() - queue.task_done() - - async def run_worker_loop( *, session_factory: async_sessionmaker[AsyncSession] | None = None, diff --git a/tests/providers/test_openrouter.py b/tests/providers/test_openrouter.py index f3067f9..882d382 100644 --- a/tests/providers/test_openrouter.py +++ b/tests/providers/test_openrouter.py @@ -120,6 +120,7 @@ class TestOpenRouterProviderTranscribe: assert result.text == "Line 1\nLine 2" assert result.provider == "openrouter" + assert result.prompt_name is None assert result.model == "vendor/model-b" assert result.ai_metadata == { "finish_reason": "stop", @@ -142,6 +143,27 @@ class TestOpenRouterProviderTranscribe: mime_type="image/png", ) + @pytest.mark.asyncio + async def test_sends_pdf_as_file_content(self): + """PDF payloads use OpenRouter's file content contract.""" + response = {"model": "vendor/model-a", "choices": [{"message": {"content": "Transcript text"}}]} + client = _FakeClient(response=response) + provider = OpenRouterTranscriptionProvider( + settings=Settings(openrouter_api_key="test-key"), + client=client, + ) + + await provider.transcribe( + prompt_text="Prompt body", + image_bytes=b"pdf-bytes", + mime_type="application/pdf", + ) + + content = client.chat.calls[0]["messages"][0]["content"] + assert content[1]["type"] == "file" + assert content[1]["file"]["filename"] == "source.pdf" + assert content[1]["file"]["file_data"].startswith("data:application/pdf;base64,") + @pytest.mark.asyncio async def test_raises_on_empty_or_invalid_response(self): """Transcribe raises ProviderResponseError for missing completion text.""" diff --git a/tests/services/test_job_service.py b/tests/services/test_job_service.py index 8996263..6bac264 100644 --- a/tests/services/test_job_service.py +++ b/tests/services/test_job_service.py @@ -1,7 +1,7 @@ -from uuid import uuid4 from datetime import UTC from datetime import datetime from datetime import timedelta +from uuid import uuid4 import pytest @@ -12,8 +12,9 @@ from transcription.db.models import JobSourceStatus from transcription.db.models import JobStatus from transcription.db.models import Source from transcription.services.documents import DocumentService -from transcription.services.jobs import JobDeleteBlockedError from transcription.services.jobs import JobCancelBlockedError +from transcription.services.jobs import JobDeleteBlockedError +from transcription.services.jobs import JobNotFoundError from transcription.services.jobs import JobResubmitBlockedError from transcription.services.jobs import JobService @@ -228,7 +229,7 @@ class TestJobService: await job_service.delete_job_with_guardrails(job_id=job.id) - with pytest.raises(ValueError): + with pytest.raises(JobNotFoundError): await job_service.read_job(job_id=job.id) @pytest.mark.asyncio @@ -355,8 +356,12 @@ class TestJobService: refreshed = await job_service.read_job(job_id=job.id) assert refreshed.status == JobStatus.QUEUED - failed_entry = next(item for item in refreshed.job_sources if item.source is not None and item.source.page_number == 1) - transcribed_entry = next(item for item in refreshed.job_sources if item.source is not None and item.source.page_number == 2) + failed_entry = next( + item for item in refreshed.job_sources if item.source is not None and item.source.page_number == 1 + ) + transcribed_entry = next( + item for item in refreshed.job_sources if item.source is not None and item.source.page_number == 2 + ) assert failed_entry.status == JobSourceStatus.PENDING assert failed_entry.error_detail is None assert failed_entry.source is not None diff --git a/tests/test_app.py b/tests/test_app.py index f5f3da3..c84a43b 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -7,18 +7,19 @@ from fastapi import FastAPI from fastapi.testclient import TestClient from transcription.app import create_app +from transcription.config import Settings @pytest.mark.unit class TestAppFactory: """Verify FastAPI app factory wiring.""" - def test_create_app_returns_fastapi_instance(self): + def test_create_app_returns_fastapi_instance(self, monkeypatch): """create_app returns a FastAPI application instance.""" + monkeypatch.setattr("transcription.app.register_pages", lambda _app: None) app = create_app() assert isinstance(app, FastAPI) - @pytest.mark.integration class TestAppLifespan: """Verify startup and shutdown lifecycle behavior.""" @@ -28,6 +29,7 @@ class TestAppLifespan: calls = [] monkeypatch.setattr("transcription.app.configure_logging", lambda _settings: calls.append("logging")) + monkeypatch.setattr("transcription.app.register_pages", lambda _app: None) async def _create_all(**_kwargs): calls.append("schema") @@ -56,12 +58,14 @@ class TestAppLifespan: monkeypatch.setattr("transcription.app.worker_consumer_lifespan", _worker_lifespan) - class _Settings: - should_bootstrap_schema = True - upload_dir = tmp_path / "uploads" - prompt_dir = tmp_path / "prompts" - - monkeypatch.setattr("transcription.app.get_settings", lambda: _Settings()) + settings = Settings( + openrouter_api_key="test-key", + environment="test", + bootstrap_schema_on_startup=True, + upload_dir=tmp_path / "uploads", + prompt_dir=tmp_path / "prompts", + ) + monkeypatch.setattr("transcription.app.get_settings", lambda: settings) app = create_app() with TestClient(app): @@ -73,14 +77,15 @@ class TestAppLifespan: assert "worker_start" in calls assert "worker_stop" in calls assert "dispose_db" in calls - assert _Settings.upload_dir.exists() - assert _Settings.prompt_dir.exists() + assert settings.upload_dir.exists() + assert settings.prompt_dir.exists() def test_shutdown_stops_worker_resources(self, monkeypatch, tmp_path): """Shutdown signals and stops worker resources cleanly.""" calls = [] monkeypatch.setattr("transcription.app.configure_logging", lambda _settings: calls.append("logging")) + monkeypatch.setattr("transcription.app.register_pages", lambda _app: None) async def _create_all(**_kwargs): calls.append("schema") @@ -109,12 +114,14 @@ class TestAppLifespan: monkeypatch.setattr("transcription.app.worker_consumer_lifespan", _worker_lifespan) - class _Settings: - should_bootstrap_schema = True - upload_dir = tmp_path / "uploads" - prompt_dir = tmp_path / "prompts" - - monkeypatch.setattr("transcription.app.get_settings", lambda: _Settings()) + settings = Settings( + openrouter_api_key="test-key", + environment="test", + bootstrap_schema_on_startup=True, + upload_dir=tmp_path / "uploads", + prompt_dir=tmp_path / "prompts", + ) + monkeypatch.setattr("transcription.app.get_settings", lambda: settings) app = create_app() with TestClient(app): diff --git a/tests/tools/test_run_destructive_tests.py b/tests/tools/test_run_destructive_tests.py new file mode 100644 index 0000000..48ae7f5 --- /dev/null +++ b/tests/tools/test_run_destructive_tests.py @@ -0,0 +1,50 @@ +from pathlib import Path + +import pytest + +from tools import run_destructive_tests + + +def test_reuses_initial_backup_across_test_attempts(tmp_path: Path) -> None: + data_path = tmp_path / "data" + backup_root = tmp_path / ".test-backups" + data_path.mkdir() + backup_root.mkdir() + (data_path / "transcription.db").write_text("original", encoding="utf-8") + + initial_backup = run_destructive_tests.create_or_reuse_backup(data_path, backup_root) + (data_path / "transcription.db").write_text("overwritten", encoding="utf-8") + reused_backup = run_destructive_tests.create_or_reuse_backup(data_path, backup_root) + + assert reused_backup == initial_backup + assert (reused_backup / "transcription.db").read_text(encoding="utf-8") == "original" + assert [path for path in backup_root.iterdir() if path.is_dir()] == [initial_backup] + + +def test_missing_active_backup_stops_instead_of_replacing_it(tmp_path: Path) -> None: + data_path = tmp_path / "data" + backup_root = tmp_path / ".test-backups" + data_path.mkdir() + backup_root.mkdir() + (data_path / "transcription.db").write_text("post-test", encoding="utf-8") + (backup_root / run_destructive_tests.ACTIVE_BACKUP_FILENAME).write_text("data-backup-missing", encoding="utf-8") + + with pytest.raises(FileNotFoundError, match="Active backup is missing"): + run_destructive_tests.create_or_reuse_backup(data_path, backup_root) + + assert not any(path.is_dir() for path in backup_root.iterdir()) + + +def test_closing_cycle_preserves_backup(tmp_path: Path) -> None: + data_path = tmp_path / "data" + backup_root = tmp_path / ".test-backups" + data_path.mkdir() + backup_root.mkdir() + (data_path / "transcription.db").write_text("original", encoding="utf-8") + backup_path = run_destructive_tests.create_or_reuse_backup(data_path, backup_root) + + closed_backup = run_destructive_tests.close_active_backup_cycle(backup_root) + + assert closed_backup == backup_path + assert backup_path.is_dir() + assert not (backup_root / run_destructive_tests.ACTIVE_BACKUP_FILENAME).exists() diff --git a/tests/ui/conftest.py b/tests/ui/conftest.py index 586b0ec..eb7c3fc 100644 --- a/tests/ui/conftest.py +++ b/tests/ui/conftest.py @@ -2,11 +2,12 @@ from __future__ import annotations -import asyncio -from collections.abc import AsyncGenerator, Callable -from datetime import UTC, datetime +from collections.abc import AsyncGenerator +from collections.abc import Awaitable +from collections.abc import Callable +from datetime import UTC +from datetime import datetime from pathlib import Path -from typing import Awaitable from uuid import UUID import pytest @@ -16,45 +17,52 @@ from fastapi.testclient import TestClient from sqlmodel import delete from transcription.app import create_app -from transcription.config import Settings, SqliteSettings -from transcription.db import create_all, initialize_database_runtime, session_scope -from transcription.db.models import ( - Document, - DocumentPerson, - Job, - JobSource, - JobSourceStatus, - JobStatus, - Person, - Source, -) +from transcription.config import Settings +from transcription.config import SqliteSettings +from transcription.db import session as db_session_module +from transcription.db import session_scope +from transcription.db.models import Document +from transcription.db.models import DocumentPerson +from transcription.db.models import Job +from transcription.db.models import JobSource +from transcription.db.models import JobSourceStatus +from transcription.db.models import JobStatus +from transcription.db.models import Person +from transcription.db.models import Source @pytest.fixture(scope="session") -def app_client(tmp_path_factory: pytest.TempPathFactory) -> AsyncGenerator[tuple[FastAPI, TestClient], None]: +def app_client(tmp_path_factory: pytest.TempPathFactory) -> AsyncGenerator[tuple[FastAPI, TestClient]]: """Provide a real application and test client backed by in-memory SQLite.""" tmp_path = tmp_path_factory.mktemp("ui") settings = Settings( openrouter_api_key="test-key", - database=SqliteSettings(path=":memory:"), + database=SqliteSettings(path=str(tmp_path / "ui-tests.db")), environment="test", bootstrap_schema_on_startup=True, upload_dir=tmp_path / "uploads", prompt_dir=tmp_path / "prompts", ) - app = create_app() - app.state.runtime = initialize_database_runtime(settings=settings) - asyncio.run(create_all(engine=app.state.runtime.engine)) + app = create_app(settings=settings) with TestClient(app) as client: yield app, client @pytest_asyncio.fixture(autouse=True) -async def clear_ui_database(app_client: tuple[FastAPI, TestClient]) -> None: +async def clear_ui_database( + app_client: tuple[FastAPI, TestClient], + monkeypatch: pytest.MonkeyPatch, +) -> None: """Reset UI-facing tables asynchronously before each test for isolation.""" - async with session_scope() as session: + app, _ = app_client + monkeypatch.setattr( + db_session_module, + "resolve_session_factory", + lambda *_args, **_kwargs: app.state.runtime.session_factory, + ) + async with session_scope(session_factory=app.state.runtime.session_factory) as session: await session.exec(delete(JobSource)) await session.exec(delete(DocumentPerson)) await session.exec(delete(Source)) @@ -118,9 +126,7 @@ async def seed_job(app_client: tuple[FastAPI, TestClient]) -> Callable[..., Awai job_id=job.id, source_id=source.id, status=( - JobSourceStatus.TRANSCRIBED - if transcription_text is not None - else JobSourceStatus.FAILED + JobSourceStatus.TRANSCRIBED if transcription_text is not None else JobSourceStatus.FAILED ), raw_transcription=transcription_text, error_detail=error_detail, @@ -135,4 +141,4 @@ async def seed_job(app_client: tuple[FastAPI, TestClient]) -> Callable[..., Awai await session.commit() return job.id - return _seed \ No newline at end of file + return _seed diff --git a/tools/run_destructive_tests.py b/tools/run_destructive_tests.py index 4de9355..8dbc4de 100644 --- a/tools/run_destructive_tests.py +++ b/tools/run_destructive_tests.py @@ -3,16 +3,16 @@ from __future__ import annotations import argparse import ctypes import os -from ctypes import wintypes -from datetime import datetime -from pathlib import Path import shlex import shutil import subprocess import sys - +from ctypes import wintypes +from datetime import datetime +from pathlib import Path RETRY_CANCEL_CHOICES = {"n", "no", "c", "cancel", "a", "abort", "q", "quit"} +ACTIVE_BACKUP_FILENAME = ".active-backup" def show_phase(title: str) -> None: @@ -88,11 +88,11 @@ def wait_for_restore_preflight(db_file_path: Path) -> bool: while not test_file_unlocked(db_file_path): show_phase("Restore Preflight") print(f"WARNING: Restore preflight blocked: database appears to be in use: {db_file_path}") - print("Close conflicting applications (for example DB Browser for SQLite, uvicorn, or any process using this DB).") + print( + "Close conflicting applications (for example DB Browser for SQLite, uvicorn, or any process using this DB)." + ) print("Restore is paused and waiting for your input.") - answer = input( - f"Attempt {attempt}: type 'retry' to check again, or 'cancel' to skip restore: " - ).strip() + answer = input(f"Attempt {attempt}: type 'retry' to check again, or 'cancel' to skip restore: ").strip() print(f"Input received: '{answer}'") if answer.lower() in RETRY_CANCEL_CHOICES: return False @@ -115,10 +115,61 @@ def restore_backup(backup_path: Path, data_path: Path) -> None: print(f"Restored data from backup: {backup_path}") +def active_backup_path(backup_root: Path) -> Path | None: + marker_path = backup_root / ACTIVE_BACKUP_FILENAME + if not marker_path.exists(): + return None + + backup_name = marker_path.read_text(encoding="utf-8").strip() + if not backup_name or Path(backup_name).name != backup_name: + raise RuntimeError(f"Invalid active backup marker: {marker_path}") + + backup_path = backup_root / backup_name + if not backup_path.is_dir(): + raise FileNotFoundError( + f"Active backup is missing: {backup_path}. " + f"Restore it or remove {marker_path} only after confirming the original data is safe." + ) + return backup_path + + +def create_or_reuse_backup(data_path: Path, backup_root: Path) -> Path: + current_backup = active_backup_path(backup_root) + if current_backup is not None: + print(f"Reusing active test-cycle backup: {current_backup}") + return current_backup + + db_path = data_path / "transcription.db" + if not test_file_unlocked(db_path): + print(f"WARNING: Backup preflight warning: database appears to be in use: {db_path}") + print("WARNING: Proceeding with backup, but hot backups can capture an in-flight state.") + + timestamp = datetime.now().astimezone().strftime("%Y%m%d-%H%M%S") + backup_path = backup_root / f"data-backup-{timestamp}" + shutil.copytree(data_path, backup_path) + + marker_path = backup_root / ACTIVE_BACKUP_FILENAME + temporary_marker_path = marker_path.with_suffix(".tmp") + temporary_marker_path.write_text(backup_path.name, encoding="utf-8") + temporary_marker_path.replace(marker_path) + print(f"Created test-cycle backup: {backup_path}") + return backup_path + + +def close_active_backup_cycle(backup_root: Path, backup_path: Path | None = None) -> Path: + current_backup = active_backup_path(backup_root) + if current_backup is None: + raise RuntimeError("No active test backup cycle exists.") + if backup_path is not None and current_backup.resolve() != backup_path.resolve(): + return current_backup + + (backup_root / ACTIVE_BACKUP_FILENAME).unlink() + print(f"Closed test backup cycle: {current_backup}") + return current_backup + + def parse_args(argv: list[str]) -> argparse.Namespace: - parser = argparse.ArgumentParser( - description="Run potentially destructive tests with backup/restore protection." - ) + parser = argparse.ArgumentParser(description="Run potentially destructive tests with backup/restore protection.") parser.add_argument("--auto-restore", action="store_true", help="Restore immediately after successful tests.") parser.add_argument("--keep-backup", action="store_true", help="Keep the backup even after a successful restore.") parser.add_argument( @@ -130,14 +181,22 @@ def parse_args(argv: list[str]) -> argparse.Namespace: "--restore-from", help="Restore from an existing backup name or absolute backup path instead of running tests.", ) + parser.add_argument( + "--accept-current-data", + action="store_true", + help="Close the active test cycle without restoring; preserve its backup.", + ) parser.add_argument("command", nargs=argparse.REMAINDER, help="Command to run after '--'.") args = parser.parse_args(argv) - if args.restore_from and args.command: - parser.error("--restore-from cannot be combined with a test command.") + if args.restore_from and args.accept_current_data: + parser.error("--restore-from cannot be combined with --accept-current-data.") - if not args.restore_from and not args.command: - parser.error("A test command is required unless --restore-from is provided.") + if (args.restore_from or args.accept_current_data) and args.command: + parser.error("Restore/accept modes cannot be combined with a test command.") + + if not args.restore_from and not args.accept_current_data and not args.command: + parser.error("A test command is required unless --restore-from or --accept-current-data is provided.") return args @@ -159,45 +218,50 @@ def run_command(command: list[str]) -> int: return completed.returncode -def main(argv: list[str] | None = None) -> int: - args = parse_args(argv or sys.argv[1:]) - repo_root = Path(__file__).resolve().parent.parent - data_dir = repo_root / "data" - backup_root = repo_root / ".test-backups" - db_path = data_dir / "transcription.db" +def accept_current_data(backup_root: Path) -> int: + show_phase("Complete Test Cycle") + backup_path = close_active_backup_cycle(backup_root) + print(f"Current data accepted. Backup preserved at: {backup_path}") + return 0 - if not data_dir.exists(): - raise FileNotFoundError(f"Data directory not found: {data_dir}") - backup_root.mkdir(parents=True, exist_ok=True) +def restore_saved_backup(restore_from: str, data_path: Path, backup_root: Path) -> int: + restore_path = Path(restore_from) + if not restore_path.is_absolute(): + restore_path = backup_root / restore_path - if args.restore_from: - restore_path = Path(args.restore_from) - if not restore_path.is_absolute(): - restore_path = backup_root / restore_path + show_phase("Restore Phase") + if not wait_for_restore_preflight(data_path / "transcription.db"): + print(f"Restore cancelled. Backup preserved at: {restore_path}") + return 1 - show_phase("Restore Phase") - if not wait_for_restore_preflight(db_path): - print(f"Restore cancelled. Backup preserved at: {restore_path}") - return 1 + restore_backup(restore_path, data_path) + current_backup = active_backup_path(backup_root) + if current_backup is not None: + if current_backup.resolve() == restore_path.resolve(): + close_active_backup_cycle(backup_root, restore_path) + else: + print(f"Active test cycle remains unchanged: {current_backup}") + return 0 - restore_backup(restore_path, data_dir) - return 0 +def restore_requested(args: argparse.Namespace) -> bool: + if args.auto_restore: + return True + if args.skip_restore_prompt: + return False + answer = input("Tests passed. Restore data backup now? [y/N] ").strip().lower() + return answer in {"y", "yes"} + + +def run_protected_tests(args: argparse.Namespace, data_path: Path, backup_root: Path) -> int: command = normalize_command(list(args.command)) if not command: raise ValueError("No test command provided.") show_phase("Backup Phase") - if not test_file_unlocked(db_path): - print(f"WARNING: Backup preflight warning: database appears to be in use: {db_path}") - print("WARNING: Proceeding with backup, but hot backups can capture an in-flight state.") - - timestamp = datetime.now().strftime("%Y%m%d-%H%M%S") - backup_name = f"data-backup-{timestamp}" - backup_path = backup_root / backup_name - shutil.copytree(data_dir, backup_path) - print(f"Created backup: {backup_path}") + backup_path = create_or_reuse_backup(data_path, backup_root) + backup_name = backup_path.name test_exit_code = run_command(command) if test_exit_code != 0: @@ -208,36 +272,30 @@ def main(argv: list[str] | None = None) -> int: print(f" uv run python tools/run_destructive_tests.py --restore-from {backup_name}") return test_exit_code - should_restore = False - if args.auto_restore: - should_restore = True - elif args.skip_restore_prompt: - should_restore = False - else: - answer = input("Tests passed. Restore data backup now? [y/N] ").strip().lower() - if answer in {"y", "yes"}: - should_restore = True - - if not should_restore: - print(f"Restore skipped by user. Backup kept at: {backup_path}") + if not restore_requested(args): + print(f"Restore skipped by user. Test cycle remains active with backup: {backup_path}") + print("Further test runs will reuse this backup instead of creating another.") print("Restore later with:") print(f" uv run python tools/run_destructive_tests.py --restore-from {backup_name}") + print("Or accept the current data and close the cycle with:") + print(" uv run python tools/run_destructive_tests.py --accept-current-data") return 0 show_phase("Restore Phase") - if not wait_for_restore_preflight(db_path): + if not wait_for_restore_preflight(data_path / "transcription.db"): print(f"Restore cancelled. Backup preserved at: {backup_path}") return 1 try: - restore_backup(backup_path, data_dir) + restore_backup(backup_path, data_path) + close_active_backup_cycle(backup_root, backup_path) if args.keep_backup: print(f"Kept backup: {backup_path}") else: shutil.rmtree(backup_path) print(f"Deleted backup: {backup_path}") - except Exception as exc: - print("WARNING: Restore failed. Your current data remains unchanged.") + except (OSError, RuntimeError) as exc: + print("WARNING: Restore failed. The backup remains available.") print(f"WARNING: {exc}") print("Likely cause: another process has data/transcription.db open.") print("Stop the process and retry restore with:") @@ -247,5 +305,23 @@ def main(argv: list[str] | None = None) -> int: return 0 +def main(argv: list[str] | None = None) -> int: + args = parse_args(argv or sys.argv[1:]) + repo_root = Path(__file__).resolve().parent.parent + data_path = repo_root / "data" + backup_root = repo_root / ".test-backups" + + if not data_path.exists(): + raise FileNotFoundError(f"Data directory not found: {data_path}") + + backup_root.mkdir(parents=True, exist_ok=True) + + if args.accept_current_data: + return accept_current_data(backup_root) + if args.restore_from: + return restore_saved_backup(args.restore_from, data_path, backup_root) + return run_protected_tests(args, data_path, backup_root) + + if __name__ == "__main__": - raise SystemExit(main()) \ No newline at end of file + raise SystemExit(main())