From fb1e16d2ce81c01a845346c84c2c55b9555b432b Mon Sep 17 00:00:00 2001 From: Gregoire Moreels Date: Tue, 21 Jul 2026 07:35:04 +0200 Subject: [PATCH] fix(backend): return 404 instead of 500 for audio of failed generations (#893) * fix(backend): return 404 instead of 500 for audio of failed generations A failed generation stores an empty audio_path. resolve_storage_path("") resolved to the data directory itself, which exists, so the route's 404 guard passed and FileResponse raised RuntimeError ("File at path .../data is not a file"), surfacing as a 500. - resolve_storage_path now returns None for empty paths - audio routes check is_file() instead of exists() so directories never reach FileResponse - GET /audio/{generation_id} reports "Generation failed; no audio available" when the generation status is failed Co-Authored-By: Claude Fable 5 * fix(backend): reject empty Path objects in resolve_storage_path Path("") is truthy, so the previous `if not path` guard only caught None and empty strings. Callers such as database/migrations.py pass Path objects, so an empty Path could still resolve to the data dir. Check None separately and reject paths with no parts. Also add regression tests asserting the version and sample audio endpoints 404 when a stored path resolves to an existing directory (guards the is_file() checks against regressing to exists()). Addresses CodeRabbit review on PR #893. Co-Authored-By: Claude Fable 5 * style(tests): drop parentheses on pytest.fixture decorator (ruff PT001) Co-Authored-By: Claude Fable 5 * style(tests): satisfy Ruff naming rule --------- Co-authored-by: Claude Fable 5 Co-authored-by: Jamie Pine --- backend/config.py | 5 + backend/routes/audio.py | 13 +- backend/tests/test_audio_failed_generation.py | 164 ++++++++++++++++++ 3 files changed, 178 insertions(+), 4 deletions(-) create mode 100644 backend/tests/test_audio_failed_generation.py diff --git a/backend/config.py b/backend/config.py index 8d2df51a..cb6bc168 100644 --- a/backend/config.py +++ b/backend/config.py @@ -80,6 +80,11 @@ def resolve_storage_path(path: str | Path | None) -> Path | None: return None stored_path = Path(path) + # Empty paths (e.g. failed generations) must not resolve to the data + # dir itself, which exists and would defeat the callers' 404 guards. + # Path("") is truthy, so check parts rather than the raw value. + if not stored_path.parts: + return None if stored_path.is_absolute(): rebased_path = _path_relative_to_any_data_dir(stored_path) if rebased_path is not None: diff --git a/backend/routes/audio.py b/backend/routes/audio.py index 79175568..685136c7 100644 --- a/backend/routes/audio.py +++ b/backend/routes/audio.py @@ -34,7 +34,7 @@ async def get_version_audio(version_id: str, db: Session = Depends(get_db)): raise HTTPException(status_code=404, detail="Version not found") audio_path = config.resolve_storage_path(version.audio_path) - if audio_path is None or not audio_path.exists(): + if audio_path is None or not audio_path.is_file(): raise HTTPException(status_code=404, detail="Audio file not found") return FileResponse( @@ -52,8 +52,13 @@ async def get_audio(generation_id: str, db: Session = Depends(get_db)): raise HTTPException(status_code=404, detail="Generation not found") audio_path = config.resolve_storage_path(generation.audio_path) - if audio_path is None or not audio_path.exists(): - raise HTTPException(status_code=404, detail="Audio file not found") + if audio_path is None or not audio_path.is_file(): + detail = ( + "Generation failed; no audio available" + if generation.status == "failed" + else "Audio file not found" + ) + raise HTTPException(status_code=404, detail=detail) return FileResponse( audio_path, @@ -72,7 +77,7 @@ async def get_sample_audio(sample_id: str, db: Session = Depends(get_db)): raise HTTPException(status_code=404, detail="Sample not found") audio_path = config.resolve_storage_path(sample.audio_path) - if audio_path is None or not audio_path.exists(): + if audio_path is None or not audio_path.is_file(): raise HTTPException(status_code=404, detail="Audio file not found") return FileResponse( diff --git a/backend/tests/test_audio_failed_generation.py b/backend/tests/test_audio_failed_generation.py new file mode 100644 index 00000000..c85619cc --- /dev/null +++ b/backend/tests/test_audio_failed_generation.py @@ -0,0 +1,164 @@ +""" +Regression tests for GET /audio/{generation_id} on failed generations. + +A failed generation stores an empty ``audio_path``. Previously, +``config.resolve_storage_path("")`` resolved to the data directory itself, +which exists, so the route's 404 guard passed and ``FileResponse`` raised +``RuntimeError: File at path .../data is not a file`` — a 500 instead of +a clean 404. + +Usage: + python -m pytest backend/tests/test_audio_failed_generation.py -v +""" + +import sys +from pathlib import Path + +import pytest +from fastapi import FastAPI +from sqlalchemy import create_engine +from sqlalchemy.orm import sessionmaker +from starlette.testclient import TestClient + +# Repo root on sys.path so ``backend`` imports as a package (the audio +# routes use package-relative imports). +sys.path.insert(0, str(Path(__file__).parent.parent.parent)) + +from backend import config +from backend.database import ( + Base, + Generation, + GenerationVersion, + ProfileSample, + VoiceProfile, + get_db, +) +from backend.routes.audio import router as audio_router + + +def test_resolve_storage_path_empty_returns_none(): + """An empty stored path must not resolve to the data dir itself.""" + assert config.resolve_storage_path("") is None + assert config.resolve_storage_path(None) is None + # Path("") is truthy, so it must be rejected via its (empty) parts. + assert config.resolve_storage_path(Path("")) is None + + +@pytest.fixture +def client(tmp_path, monkeypatch): + """Minimal app with only the audio routes and a temp sqlite DB.""" + monkeypatch.setattr(config, "_data_dir", tmp_path) + # An existing directory that a stored audio_path may wrongly point to. + (tmp_path / "somedir").mkdir() + + engine = create_engine( + f"sqlite:///{tmp_path / 'test.db'}", + connect_args={"check_same_thread": False}, + ) + Base.metadata.create_all(bind=engine) + testing_session_local = sessionmaker(autocommit=False, autoflush=False, bind=engine) + + session = testing_session_local() + profile = VoiceProfile(id="profile-1", name="Test Profile") + session.add(profile) + + session.add_all( + [ + Generation( + id="gen-failed-empty", + profile_id="profile-1", + text="failed generation", + audio_path="", + status="failed", + error="engine exploded", + ), + Generation( + id="gen-failed-null", + profile_id="profile-1", + text="failed generation", + audio_path=None, + status="failed", + ), + Generation( + id="gen-missing-file", + profile_id="profile-1", + text="completed but file deleted", + audio_path="generations/does-not-exist.wav", + status="completed", + ), + Generation( + id="gen-with-version", + profile_id="profile-1", + text="generation with a broken version", + audio_path="somedir", + status="completed", + ), + GenerationVersion( + id="version-dir", + generation_id="gen-with-version", + label="original", + audio_path="somedir", + ), + ProfileSample( + id="sample-dir", + profile_id="profile-1", + audio_path="somedir", + reference_text="sample pointing at a directory", + ), + ] + ) + session.commit() + session.close() + + app = FastAPI() + app.include_router(audio_router) + + def override_get_db(): + db = testing_session_local() + try: + yield db + finally: + db.close() + + app.dependency_overrides[get_db] = override_get_db + return TestClient(app) + + +@pytest.mark.parametrize("generation_id", ["gen-failed-empty", "gen-failed-null"]) +def test_failed_generation_returns_404(client, generation_id): + """Failed generations (empty/null audio_path) get a clean 404, not a 500.""" + response = client.get(f"/audio/{generation_id}") + assert response.status_code == 404 + assert response.json()["detail"] == "Generation failed; no audio available" + + +def test_missing_audio_file_returns_404(client): + """A completed generation whose file vanished still 404s.""" + response = client.get("/audio/gen-missing-file") + assert response.status_code == 404 + assert response.json()["detail"] == "Audio file not found" + + +def test_unknown_generation_returns_404(client): + response = client.get("/audio/no-such-generation") + assert response.status_code == 404 + assert response.json()["detail"] == "Generation not found" + + +@pytest.mark.parametrize( + "url", + [ + "/audio/gen-with-version", + "/audio/version/version-dir", + "/samples/sample-dir", + ], +) +def test_audio_path_pointing_at_directory_returns_404(client, url): + """A stored path resolving to an existing directory must 404, not 500. + + Guards the is_file() checks: a directory passes exists() and would + crash FileResponse. + """ + response = client.get(url) + assert response.status_code == 404 + assert response.json()["detail"] == "Audio file not found"