Close attachment backing on copy/download error paths - #335
Open
Flerpharos wants to merge 1 commit into
Open
Conversation
Attachment.from_file created its SpooledTemporaryFile backing before copying into it, but only restored the source cursor on error and never closed the backing itself, leaking a handle (and an on-disk temp file past the 4 MB rollover) if the copy failed. AttachmentEntry._ensure_attachment had the same issue: the backing buffer created for a download was never closed if the streaming write loop raised mid-transfer. Both now close the backing in an except/raise around the copy, while leaving it open (and handed off to Attachment) on success. Fixes #323
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.
Fixes #323
Two resource leaks where a backing file/handle was created before a copy/download and never closed on the error path:
src/labapi/entry/attachment.pyAttachment.from_file:backing = SpooledTemporaryFile(...)is created beforeshutil.copyfileobj; the existingtry/finallyonly restored the source cursor on error and never closedbacking. Now wraps the copy intry/except Exception: backing.close(); raise, keeping the source-cursor restore infinally.backingis still left open (and handed off toAttachment) on success.src/labapi/entry/entries/attachment.pyAttachmentEntry._ensure_attachment:output = _make_backing_io(...)was never closed if the streaming write loop raised mid-download. Sametry/except Exception: output.close(); raisepattern around the chunk loop;outputis left open on success since it's handed toAttachment.Added regression tests for both:
tests/entry/test_attachment.py::test_attachment_from_file_closes_backing_on_copy_error— a source whoseread()raises mid-copy; asserts the spooled backing is closed and the source cursor is still restored.tests/entry/entries/test_attachment.py::TestAttachmentEntryIntegration::test_ensure_attachment_closes_backing_on_download_error— a download whose chunk iterator raises mid-stream; asserts the backing buffer is closed and_filedatastays unset.Test plan
uv run pytest --ignore=tests/test_integration.py -q— 432 passed, 1 skippeduv run ruff check .— all checks passed