Don't upload a partial file when the writer fails

[i18n-tribe]

The problem

S3File buffers writes into a tempfile.TemporaryFile() and uploads the whole thing in one upload_fileobj call when the file is closed. __exit__ closes unconditionally:

def __exit__(self, exc_type, exc_value, traceback):
    self.close()

So an exception raised part way through writing still uploads whatever happened to be buffered at that point.

with s3fs.openbin("report.json.gz", "wb") as f:
    for chunk in stream:      # connection drops half way through
        f.write(chunk)        # <- raises
# a truncated report.json.gz is now in the bucket

Why this is worse than it looks

The upload itself succeeds, so S3 stores a perfectly valid object that simply happens to be short. There is no failed multipart to abort and no marker of any kind — nothing downstream can distinguish it from a file that was written in full.

That matters because the usual protections don't apply. An interrupted PUT creates no object, and an abandoned multipart upload leaves no visible object either. Here the failure happens while writing to the local buffer, long before S3 is involved, and then the library goes on to publish it.

Depending on the format, a consumer either reads silently incomplete data, or — for anything compressed or otherwise checksummed — fails on every single read of an object that will never become valid, because the write that would have completed it is long gone.

The zero-byte case is the same bug at its limit: a writer that raises before writing anything leaves an empty object where callers expect either a whole file or nothing at all. That one is particularly easy to miss, because a pipeline that decides what still needs fetching by listing keys will treat the empty object as "already done" and never retry it.

The change

__exit__ now discards the buffer when it is unwinding an exception, and uploads only when the block completed:

def __exit__(self, exc_type, exc_value, traceback):
    if exc_type is None:
        self.close()
    else:
        self.discard()

This makes the context manager all-or-nothing, which is the guarantee the buffer-then-upload design already offers everywhere else — the object appears in one step, at close, or not at all.

discard() also clears _on_close, because io.IOBase.__del__ calls close(). Without that, the abandoned buffer would be uploaded anyway as soon as the object was garbage collected, which is a nastier version of the original bug since it happens at an arbitrary later moment.

Compatibility

This is a behaviour change, so worth being explicit about who it affects.

Anyone relying on a partial upload surviving a failed write will no longer get one. I'd argue that is the point rather than a regression, but the capability is still there for anyone who wants it — calling close() explicitly, rather than relying on the context manager, uploads exactly as before.

Local file semantics are the obvious counter-argument: with open(path, "wb") does leave a partial file behind. The difference is that a local partial file is written incrementally and is inspectable at a path the caller already knows, whereas here the library buffers the entire write and then makes a deliberate choice to publish it to a remote store, typically into a pipeline that will consume it as complete.

Tests

Three unit tests in TestS3FileClose, using a stub on_close so they need no bucket or network:

• a block that completes still uploads
• a block that raises does not
• close() after discard() is a no-op, covering the __del__ path above