Update audio.py#45
Conversation
WalkthroughThe audio endpoint implementation was modified to wrap the audio loading operation in a finally block that ensures the temporary uploaded file is deleted regardless of whether the operation succeeds or raises an exception. The transcription logic and response schema remain unchanged, with the change focused purely on resource cleanup. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/endpoints/audio.py (1)
78-85:⚠️ Potential issue | 🔴 CriticalCritical: indentation breaks the module —
try/finally(and the lines below) are at column 0, outside theasync def.The new
try:/finally:block, as well asresult = transcribe(...)andreturn AudioTranscription(**result), are written at the top level (column 0) instead of being indented insideaudio_transcriptions. Python will raise anIndentationErrorat import time, so the endpoint — and likely the whole app — will fail to start. This needs to be fixed before merging.🛠️ Proposed fix
logger.info("Loading audio file to whisper…") -try: - audio = whisperx.load_audio(temp_file_path) -finally: - os.remove(temp_file_path) - - result = transcribe(audio, settings, language) - - return AudioTranscription(**result) + try: + audio = whisperx.load_audio(temp_file_path) + finally: + os.remove(temp_file_path) + + result = transcribe(audio, settings, language) + + return AudioTranscription(**result)Note that
result = transcribe(...)andreturn AudioTranscription(...)should remain outside thefinallyblock (so cleanup runs before transcription, but transcription still happens on the loadedaudiovalue).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/endpoints/audio.py` around lines 78 - 85, The try/finally and subsequent lines were dedented to column 0 and must be moved inside the async endpoint function audio_transcriptions; indent the try: block (calling whisperx.load_audio(temp_file_path)) and the finally: block (calling os.remove(temp_file_path)) so they execute within audio_transcriptions, ensure the variable audio is defined in the function scope before the finally, and keep the lines result = transcribe(audio, settings, language) and return AudioTranscription(**result) outside the finally block (so cleanup runs first but transcription and return still use the loaded audio).
🧹 Nitpick comments (1)
app/endpoints/audio.py (1)
80-81: Minor: guardos.removeagainst a missing file and log cleanup failures.If
whisperx.load_audiosomehow deletes/moves the temp file, or the file was never fully created,os.remove(temp_file_path)insidefinallywill raise and mask the original exception from thetryblock. Consideros.path.existscheck or swallowing/logging the cleanup error so the real error surfaces to the caller.♻️ Suggested hardening
try: audio = whisperx.load_audio(temp_file_path) finally: - os.remove(temp_file_path) + try: + os.remove(temp_file_path) + except OSError as cleanup_err: + logger.warning("Failed to remove temp file %s: %s", temp_file_path, cleanup_err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/endpoints/audio.py` around lines 80 - 81, The finally block currently unconditionally calls os.remove(temp_file_path) which can raise and mask earlier exceptions (e.g., after whisperx.load_audio moved/deleted the file); update the cleanup to guard removal by checking os.path.exists(temp_file_path) or wrapping os.remove(...) in its own try/except that logs cleanup failures (use the module logger or processLogger) and suppresses the removal error so the original exception from the try block still surfaces; reference the temp_file_path variable and the finally block around whisperx.load_audio to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/endpoints/audio.py`:
- Around line 78-85: The try/finally and subsequent lines were dedented to
column 0 and must be moved inside the async endpoint function
audio_transcriptions; indent the try: block (calling
whisperx.load_audio(temp_file_path)) and the finally: block (calling
os.remove(temp_file_path)) so they execute within audio_transcriptions, ensure
the variable audio is defined in the function scope before the finally, and keep
the lines result = transcribe(audio, settings, language) and return
AudioTranscription(**result) outside the finally block (so cleanup runs first
but transcription and return still use the loaded audio).
---
Nitpick comments:
In `@app/endpoints/audio.py`:
- Around line 80-81: The finally block currently unconditionally calls
os.remove(temp_file_path) which can raise and mask earlier exceptions (e.g.,
after whisperx.load_audio moved/deleted the file); update the cleanup to guard
removal by checking os.path.exists(temp_file_path) or wrapping os.remove(...) in
its own try/except that logs cleanup failures (use the module logger or
processLogger) and suppresses the removal error so the original exception from
the try block still surfaces; reference the temp_file_path variable and the
finally block around whisperx.load_audio to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3a8fa206-1f1b-4ad6-bc27-82aa11abf2a1
📒 Files selected for processing (1)
app/endpoints/audio.py
1 participant
The temp file is written, then whisperx.load_audio() is called, then os.remove() is called. If load_audio raises an exception the remove call is never reached, leaking the file on disk. Any crash, unsupported format, or corrupt audio leaves a dangling temp file.
In audio.py, the endpoint writes a temp file, then calls whisperx.load_audio(), then deletes the file. If load_audio throws — due to a corrupt upload, unsupported codec, or an OOM error — the os.remove() call is never reached and the file sits on disk permanently. The fix is a try/finally block so deletion is guaranteed regardless of what happens