fix(storage): close the upload file handle when the request fails

[tushardev-365]

What

When a caller passes a path instead of an already-open file, storage3 opens the handle itself:

files = {"file": (filename, open(file, "rb"), content_type)}

_request then closed it after the except HTTPStatusError block, so the close only ran when the request succeeded. Any non-2xx raised StorageApiError straight past it and the handle stayed open — and since it only ever existed inside _request's local files dict, the caller has no way to close it either.

It leaks whenever the exception is retained: logging with exc_info, collecting failures in a batch-upload loop, raise ... from, an error reporter. The traceback keeps the frame holding the reader alive. Python also emits ResourceWarning: unclosed file on that path.

Counting open fds against a mock transport returning 409 (the most common upload failure), with the error retained each time:

baseline: 0
after failed upload #1: 1
after failed upload #2: 2
...
after failed upload #5: 5
after successful upload: 0     <- success path was always fine

Both upload() / update() (via _upload_or_update) and upload_to_signed_url() are affected.

Fix

Move the close into a finally so it runs on both paths. The condition is unchanged — still only BufferedReader, i.e. only handles storage3 opened itself, so a caller-supplied stream is still left alone to close as they see fit.

Tests

test_upload_closes_file_handle_on_error (async + sync): mock the transport into an error, then assert the handle storage3 opened is closed. Reverting only the source change fails it on .closed is False.

Note

The diff on file_api.py looks larger than it is — wrapping the body in try reindents it. The only behavioural change is where the close happens.

_sync is generated, so the change was made in _async and regenerated with make build-sync; unrelated regenerated files are excluded.

This does not touch the resp.text line a few lines above (#1563), which has PRs open already — different defect, different code path.

[grdsdev]

Closing this in favour of #1586, which carries the same try/finally fix but wider tests — it also covers update() and keeps a success-path regression test, where this one covers upload() on error only.

Two things this PR did better, and I've asked over there to fold them in:

• Your test asserts on the real handle (captured["files"]["file"][1].closed) rather than patching builtins.open process-wide and asserting on a mock's close() call. That's the stronger check.
• Your error body is well-formed, so the test exercises only the leak. fix(storage): close upload file handle when the request fails #1586 invents a _MalformedErrorBody fixture to work around the unrelated resp.text defect (storage3: AttributeError masks the real error when a storage error body lacks message/error/statusCode #1563), which conflates two bugs in one test.

For the record: the analysis in this PR — the fd-count table, the note that the traceback keeps the frame holding the reader alive, and spotting that upload_to_signed_url() is affected too — is what identified the bug properly, and #1586 reuses much of that write-up. Thanks for it, and for the read on #1574 which was also correct.

One heads-up for future PRs: ruff check src/storage fails on this branch with 3 errors. Adding the Generator import to the _async test made run-unasync.py emit from collections.abc import Generator, Generator in the generated _sync copy.