Skip to content

Close attachment backing on copy/download error paths - #335

Open
Flerpharos wants to merge 1 commit into
mainfrom
fix/323-attachment-backing-leaks
Open

Close attachment backing on copy/download error paths#335
Flerpharos wants to merge 1 commit into
mainfrom
fix/323-attachment-backing-leaks

Conversation

@Flerpharos

Copy link
Copy Markdown
Collaborator

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.py Attachment.from_file: backing = SpooledTemporaryFile(...) is created before shutil.copyfileobj; the existing try/finally only restored the source cursor on error and never closed backing. Now wraps the copy in try/except Exception: backing.close(); raise, keeping the source-cursor restore in finally. backing is still left open (and handed off to Attachment) on success.
  • src/labapi/entry/entries/attachment.py AttachmentEntry._ensure_attachment: output = _make_backing_io(...) was never closed if the streaming write loop raised mid-download. Same try/except Exception: output.close(); raise pattern around the chunk loop; output is left open on success since it's handed to Attachment.

Added regression tests for both:

  • tests/entry/test_attachment.py::test_attachment_from_file_closes_backing_on_copy_error — a source whose read() 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 _filedata stays unset.

Test plan

  • uv run pytest --ignore=tests/test_integration.py -q — 432 passed, 1 skipped
  • uv run ruff check . — all checks passed
  • pre-commit hooks (pyright, ruff check, ruff format) passed on commit

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Attachment temp-file handles leak on download/copy error paths

1 participant