Fix temp file leaks and correct return type annotations#185
Merged
Conversation
…annotations - auto_sync() now cleans up the .ass and .srt temp files (previously only the extensionless NamedTemporaryFile bases were deleted) - WhisperAPIModel.transcribe() now removes the converted audio file in a finally block so it doesn't leak on success or failure - Changed transcribe() return type from -> str to -> SSAFile in all 6 model files to match the actual return value and the abstract base class - Replaced type(model) == str with isinstance(model, str) in 3 places Co-Authored-By: Claude Opus 4.6 (1M context) <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
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.
Summary
Found a few bugs while reading through the codebase -- two temp file leaks, incorrect return type annotations, and some
type() == strchecks that should beisinstance().Bug 1: Temp file leak in
auto_sync()auto_sync()creates temp files and appends.ass/.srtextensions to them (e.g.srtin = srtin_file.name + '.ass'). Thefinallyblock was deleting the extensionless base files (srtin_file.name,srtout_file.name) but never the actual.assand.srtfiles that ffsubsync writes to. Every call toauto_sync()leaked two files in the system temp directory.Fix: Added cleanup for
srtinandsrtout(the paths with extensions) in thefinallyblock, withos.path.exists()guards since they may not have been created if an error happened early.Bug 2: Temp file leak in
WhisperAPIModel.transcribe()convert_video_to_audio_ffmpeg()creates a temp audio file (e.g./tmp/filename.mp3) but after transcription finishes, that file was never cleaned up. Repeated transcriptions would accumulate audio files in the temp directory.Fix: Wrapped the transcription body in a
try/finallythat removes the converted audio file after use.Bug 3: Wrong
-> strreturn type ontranscribe()in 6 modelsThe abstract base class correctly declares
transcribe() -> SSAFile, and all implementations actually returnSSAFileobjects. But 6 concrete model files had-> stras the return type annotation:whisper_model.pywhisper_api_model.pywhisper_timestamped_model.pywhisperX_model.pyfaster_whisper_model.pywhispercpp_model.pyFix: Changed all to
-> SSAFile. Added the missingfrom pysubs2 import SSAFiletowhisper_model.py(the others already had it).Bug 4:
type(model) == strinstead ofisinstance()Three places in
main.pyusedtype(model) == strwhich won't match subclasses ofstrand is generally considered non-Pythonic.Fix: Changed all three to
isinstance(model, str).Test plan
auto_sync()no longer leaves.ass/.srtfiles in temp directory after useWhisperAPIModel.transcribe()cleans up the converted audio filetranscribe()python -m pytest tests/)🤖 Generated with Claude Code