fix(backend): return 404 instead of 500 for audio of failed generations#893
Open
flowbrix wants to merge 3 commits into
Open
fix(backend): return 404 instead of 500 for audio of failed generations#893flowbrix wants to merge 3 commits into
flowbrix wants to merge 3 commits into
Conversation
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 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughStorage path resolution now rejects empty values, audio endpoints require regular files, and generation audio returns status-specific 404 details. Regression tests cover failed, missing-file, directory, and unknown-generation cases. ChangesAudio path handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/routes/audio.py (1)
37-38: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd directory-path regression coverage for version and sample audio.
is_file()matters when the resolved path exists but is a directory. Please add or verify tests for version and sample records pointing to existing directories and assert 404, preventing a regression toexists()that could letFileResponsereturn a 500.Also applies to: 80-81
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/routes/audio.py` around lines 37 - 38, Extend the tests covering version and sample audio endpoints to use records whose resolved audio paths point to existing directories, and assert both return HTTP 404. Keep the implementation checks in the audio route’s audio_path handling and ensure coverage prevents replacing is_file() with exists().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/config.py`:
- Around line 77-84: Update resolve_storage_path to handle None separately and
return None when the supplied Path has no parts, preventing Path() from
resolving to the configured data directory. Preserve the existing empty-string
behavior, and add a regression test covering Path() alongside that case.
---
Nitpick comments:
In `@backend/routes/audio.py`:
- Around line 37-38: Extend the tests covering version and sample audio
endpoints to use records whose resolved audio paths point to existing
directories, and assert both return HTTP 404. Keep the implementation checks in
the audio route’s audio_path handling and ensure coverage prevents replacing
is_file() with exists().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d5dad44f-aaaa-4f93-a9b6-88eb0f8b1205
📒 Files selected for processing (3)
backend/config.pybackend/routes/audio.pybackend/tests/test_audio_failed_generation.py
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 jamiepine#893.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/tests/test_audio_failed_generation.py (1)
47-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
@pytest.fixturewithout parentheses.Ruff PT001 flags Line 47; change
@pytest.fixture()to@pytest.fixture.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_audio_failed_generation.py` at line 47, Replace the parenthesized decorator on the affected pytest fixture with the bare `@pytest.fixture` form, without changing the fixture’s implementation or behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/tests/test_audio_failed_generation.py`:
- Line 47: Replace the parenthesized decorator on the affected pytest fixture
with the bare `@pytest.fixture` form, without changing the fixture’s
implementation or behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82f39ef3-1fe7-46b2-a98a-f019261c6416
📒 Files selected for processing (2)
backend/config.pybackend/tests/test_audio_failed_generation.py
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
GET /audio/{generation_id}returned a 500 (RuntimeError: File at path .../data is not a file) for generations with statusfailed, because a failed generation stores an emptyaudio_path. It now returns a clean 404 with"Generation failed; no audio available".Why it happened
The route already had a 404 guard, but it was defeated upstream:
config.resolve_storage_path("")resolved the empty string to(_data_dir / "").resolve()— the data directory itself. A directory passes.exists(), so the guard let it through andFileResponseblew up on a non-file path.Changes
config.resolve_storage_pathnow returnsNonefor empty paths instead of resolving to the data dir (all ~25 call sites already handleNone; this also protects samples/avatars/captures endpoints from the same trap).is_file()instead ofexists(), so a directory can never reachFileResponse.GET /audio/{generation_id}distinguishes the failed-generation case in its 404 detail ("Generation failed; no audio available"vs"Audio file not found").Tests
Added
backend/tests/test_audio_failed_generation.pycovering: emptyaudio_path,NULLaudio_path, completed generation with a missing file, and unknown generation id — all return 404 with the expected detail, plus a unit test onresolve_storage_path(""). Verified the tests fail against the pre-fix code.Repro (before this fix): create a generation that fails (e.g. kill the engine mid-generation), then
GET /audio/{id}→ 500.🤖 Generated with Claude Code
Summary by CodeRabbit