Leave the existing object unchanged when put_file() fails

[laughingman7743]

WHAT

S3FileSystem.put_file() and AioS3FileSystem._put_file_in_transaction() no longer publish a partial or empty object when the upload fails.

• The local file is opened before the remote file, so a local file that cannot be read (e.g. PermissionError) fails before anything is opened on S3.
• The blocks are written through a new S3FileSystem._write_file_and_close() helper instead of fsspec's with block. On any BaseException from the read, the write, or the progress callback, it closes the remote file with S3File._close_without_commit(), which drops the buffer and aborts the multipart upload, if any, and re-raises. The file is closed (committed) only after the loop completes. This is the same approach pipe_file() takes since Write pipe_file() data without committing a failed write #1003.
• _finish_multipart_upload() aborts the multipart upload on BaseException instead of Exception, so a KeyboardInterrupt while waiting for the parts or the completion no longer leaves the upload behind. S3File.commit() still clears its multipart state only on Exception: an interrupt may stop the helper's cleanup before the abort, so the upload is kept for a later discard(), which fsspec's transaction calls after a failed commit().

Behavior change (release note): a failed put_file() used to replace the existing object with the data written so far (a prefix of the local file when the failure followed an uploaded multipart part), or with an empty object when nothing was written (an unreadable local file, or a failure on the first read or write); it now leaves the object unchanged. Inside an S3FileSystem or AioS3FileSystem transaction where the caller catches the error, the object is no longer written when the transaction commits.

WHY

Closes #1014.

AbstractBufferedFile.__exit__() closes the file whatever the exception, and close() commits it. The issue has an offline reproduction on master 16aef64: a callback failure after the first block completed the multipart upload with 5 MiB of an 11 MiB file, and an unreadable local file sent an empty PutObject.

TEST

Tested commit: 49edeea (rebased onto de8cc52); before the rebase d59f880

• just lint: passed.
• uv run --env-file .env pytest -n 8 tests/pyathena/filesystem/: 530 passed (live S3) on 49edeea; 496 on d59f880.
• New offline tests:
  • test_put_file_failed_write: callback RuntimeError, callback KeyboardInterrupt, and an unreadable local file, in and outside a transaction; no PutObject or other request is sent.
  • test_put_file_failed_write_aborts_multipart_upload: a failure after the first part was submitted aborts the upload without completing it, in and outside a transaction.
  • test_commit_failure_and_discard: after commit() fails with an error, discard() does not abort again; after an interrupt, it does.
  • test_finish_multipart_upload_aborts_on_failure[KeyboardInterrupt]: an interrupt while waiting for the parts aborts the upload.
  • test_transaction_put_file_failed_write (aio): a failed write or an unreadable local file inside a transaction does not write the object when the transaction commits; a later put_file() in the same transaction still does.
• The 11 put_file/finish test cases fail with the source changes reverted (tests kept), and pass with them. test_commit_failure_and_discard[KeyboardInterrupt-1] fails when commit() clears its state on BaseException.
• Four existing tests that mocked open() as a context manager now set blocksize on the returned file directly, as put_file() no longer uses it in a with block.
• The unreadable-file cases are skipped when the tests run as root, which can read a file without read permission.
• Not run: interrupting a real upload against S3; the interrupt paths are covered offline only.

🤖 Generated with Claude Code

[laughingman7743]

Self-review round 1 (behavior and implementation): CLEAN

Base 16aef64d10dce92cbf8b8d957c178bef9bd494b6 (merge-base with master), head 5027eba79c4238e0541d7c48deef3de8df0dbf83.

Covered: _write_file_and_close(), put_file(), _finish_multipart_upload(), S3File.commit() (pyathena/filesystem/s3.py), AioS3FileSystem._put_file_in_transaction() (pyathena/filesystem/s3_async.py), and the changed tests.

Checked:

• Failure paths: a failure in the read, write, or callback closes the file with _close_without_commit(); with autocommit=False the file stays in the transaction, and its later commit() sees buffer=None, no parts, and no multipart upload, so it sends nothing. A part-limit ValueError from _upload_chunk() reaches this handler after _upload_chunk() already called _close_without_commit(); the second call is a no-op apart from the executor shutdown, which is idempotent (same as pipe_file()'s _write_and_close() since Write pipe_file() data without committing a failed write #1003).
• mode="create": FileExistsError from self.open() is raised inside with open(lpath), which only closes the local file; test_put_file_create_existing and the aio transaction test still pass.
• Success path: unchanged sequence (read f.blocksize, write, callback, then close()); the transaction still defers the commit.
• _finish_multipart_upload() on BaseException: also covers the multipart copy caller (_copy_object_with_multipart_upload), where aborting on an interrupt is equally correct. After an interrupt it waits for the parts that could not be cancelled before the abort; a second interrupt escapes that wait, and interpreter exit already joins the executor threads.
• commit() widened to BaseException so a later discard() does not abort the already-aborted upload again.
• Tests: each new case was confirmed to fail with the source changes reverted. The multipart test relies on a real write flushing exactly one part before the callback fails; the unreadable-file cases use a real chmod(0) (skipped as root; CI runs on Linux).

Limitations (pre-existing, not changed): an interrupt during close() before _finish_multipart_upload() is entered (e.g. while CreateMultipartUpload or the final part submission runs) can still leave an upload behind; this window also exists for pipe_file() and open() and is outside #1014.

[laughingman7743]

Rebased onto master de8cc52a (#1006, #1012, #1013, #1015 merged since the review base) as 49edeea3e292405f4c9aec69cfba2090d7c2bff0; pushed with --force-with-lease from d59f880b.

• Conflict: only the fsspec import lines in pyathena/filesystem/s3.py; resolved to keep both (Callback from this PR, trailing_sep from master).
• git range-diff 16aef64d..d59f880b de8cc52a..49edeea3: the first commit differs only in that import line; the second is identical.
• Upstream changes checked against this PR's contracts: none touch put_file(), _write_and_close(), _finish_multipart_upload(), S3File.commit()/discard()/_close_without_commit(), or AioS3FileSystem._put_file_in_transaction(). The S3File hunk changes only append mode, and cp_file() now returns True.
• Validation on 49edeea3: just lint passed; tests/pyathena/filesystem/ 530 passed against live S3. AWS CI is rerunning on this head.

[laughingman7743]

Self-review round 2 (claims, callers, operations): FINDINGS (PR text only, repaired)

Base 16aef64d10dce92cbf8b8d957c178bef9bd494b6, head 5027eba79c4238e0541d7c48deef3de8df0dbf83.

Claims checked:

• "AbstractBufferedFile.__exit__() closes the file whatever the exception": fsspec 2026.9.0 spec.py:2337, __exit__ calls self.close() unconditionally.
• "commit() clears its multipart state on BaseException so a later discard() does not abort it again": fsspec 2026.9.0 transaction.py:42-64, Transaction.complete() puts back a file whose commit() raised and then calls discard() on it, so that later discard() does happen.
• 5 MiB of 11 MiB (WHY): matches the issue output, 5242880 of 11534336 bytes.
• "All 11 new test cases fail with the source reverted": 6 + 2 + 1 sync cases and 2 aio cases, run locally on this head.
• Docstrings (put_file(), _write_file_and_close(), _finish_multipart_upload(), commit()) and the inline comment here: consistent with the code. No user docs mention put_file() (searched docs/, README).

Finding (PR text, repaired): the behavior-change note said that a failure before the first block was flushed replaced the object with an empty one. A small file whose callback fails after its write had its written data uploaded, not an empty object. The note now says "the data written so far, or an empty object when nothing was written". It also states that the S3FileSystem transaction case is fixed, which test_put_file_failed_write[*-True] covers.

Callers: the put_file()/_put_file_in_transaction() signatures and returns are unchanged; fsspec put() reaches both. Operationally, a failed multipart put_file() now sends AbortMultipartUpload instead of CompleteMultipartUpload. An interrupt in _finish_multipart_upload() now waits for the parts that could not be cancelled and sends one abort request.

Evidence limits: live S3 run (494 passed) covers the success paths only; the failure and interrupt paths are offline tests with mocked requests.

[laughingman7743]

Independent review (relayed): FINDINGS — finding 1 of 2

Reviewer: Codex CLI 0.160.0 (codex exec --sandbox read-only, model reported gpt-6-astra, ChatGPT login), session 01a101f6-ffc1-7b40-87e4-b17afff5e941. Static review only: no files changed, no tests/builds run, no network. Base 16aef64d10dce92cbf8b8d957c178bef9bd494b6, head 5027eba79c4238e0541d7c48deef3de8df0dbf83, detached snapshot (unchanged afterwards). The prompt held the diff and conventions, without the PR number, PR text, commit message, or prior findings.

Covered: sync put_file, both write helpers, multipart completion/copy, the S3File close/commit/discard lifecycle, aio upload and transaction paths, executor shutdown, fsspec 2026.9.0 buffered-file/transaction behavior, create-mode errors, part limits, repeated cleanup, changed tests, and docstrings/comments.

P2 — Interrupted cleanup loses the upload ID. In an AioS3FileSystem transaction, one part can fail while another remains running. If a single KeyboardInterrupt interrupts the cleanup wait at s3.py:1716, the helper exits before aborting. The widened handler clears the upload ID and futures, so fsspec's subsequent transaction discard() cannot abort the upload; unfinished parts remain. Previously, except Exception preserved that state when cleanup raised KeyboardInterrupt. Preserve the cleanup state until abort handling finishes, and cover an interrupt during cleanup through transaction completion.

Author verification: confirmed. fsspec 2026.9.0 Transaction.complete() (transaction.py:42-66) discards a file whose commit() raised, and this handler made that discard() a no-op.

[laughingman7743]

Repaired in d59f880: S3File.commit() clears the upload state only on Exception again (as before this PR), with a comment that an interrupt may stop the helper before the abort. _finish_multipart_upload() still aborts on BaseException when its wait is not interrupted. New TestS3File.test_commit_failure_and_discard: after an error, discard() sends no second abort; after KeyboardInterrupt, discard() aborts. The KeyboardInterrupt case fails with the BaseException handler.

Self-review of the repair:

• Round 1 (behavior): when the helper did abort before re-raising an interrupt, the state is now kept. A transaction's discard() then sends a second abort, which fails with NoSuchUpload; fsspec 2026.9.0 Transaction.complete() catches that Exception and logs it at debug level (transaction.py:62-66). Outside a transaction, nothing calls discard(). Both outcomes are no worse than before this PR.
• Round 2 (claims): the commit() docstring ("aborted if the completion fails or is interrupted") still holds through the helper. Removed the PR-body claim that commit() clears its state on BaseException, and described the actual behavior.
• Validation: just lint passed; tests/pyathena/filesystem/ 496 passed against live S3 on d59f880.

[laughingman7743]

Independent follow-up review (relayed): CLEAN

Reviewer: Codex CLI 0.160.0 (codex exec --sandbox read-only, model reported gpt-6-astra), session 01a10200-4b1f-7b72-976d-c51c4b2077bd. Static review only. Scope: git range-diff 16aef64d..5027eba7 16aef64d..d59f880b and the repair diff, traced into commit(), _finish_multipart_upload(), discard(), close/flush/_upload_chunk, fsspec 2026.9.0 Transaction.complete(), and AioS3FileSystem transactions. Snapshot at d59f880b915e6710a91037318a2ab42fb4ab5fd1, unchanged afterwards.

CLEAN — both earlier findings are resolved. No repair regressions found.

Helper outcome	Result
Ordinary Exception	Clears upload state and re-raises; transaction discard performs no second abort.
KeyboardInterrupt before abort	Retains upload ID and futures; transaction discard cancels pending parts, waits for running parts, then aborts.
KeyboardInterrupt after abort	Retains state and attempts another abort. A NoSuchUpload response becomes FileNotFoundError, which transaction cleanup catches; remaining files are discarded, transaction state resets, and the original interrupt propagates.

test_commit_failure_and_discard would fail against 5027eba7: its interrupt case would observe zero abort calls instead of one. [...] It isolates the handler; it does not exercise the real helper or transaction's second-abort error handling. The revised put_file test still requires the exact abort request and prohibits completion, with and without a transaction.

Two limitations predate the repair: abort is best effort, so the retained "has been aborted" wording assumes success; and nontransactional close provides no automatic discard if the helper's cleanup itself is interrupted. [...] A direct manual second discard can also surface FileNotFoundError; the transaction specifically suppresses it.

Author: the noted limitations are pre-existing and left as is.

[laughingman7743]

Independent review (relayed): FINDINGS — finding 2 of 2 (reviewer and scope: see the comment on pyathena/filesystem/s3.py:3053)

P2 — The multipart regression test can fail under valid scheduling. The callback raises immediately after the first part is submitted, without ensuring its worker has started. If the future is still pending, discard() correctly cancels it, leaving _upload_part.call_count == 0; assert_called_once() then fails despite correct cleanup. Synchronize the first upload before triggering the callback failure.

Author verification: confirmed. S3File.discard() cancels the parts that have not started (s3.py:3071), so the first part may never run.

[laughingman7743]

Repaired in d59f880: dropped the _upload_part.assert_called_once() assertion. The test still checks the observable outcome: the completion is not called and AbortMultipartUpload is sent once. It still fails on master, where __exit__ reaches _finish_multipart_upload().

[laughingman7743]

Independent review (relayed): pre-existing limitation (reviewer and scope: see the comment on pyathena/filesystem/s3.py:3053)

s3_async.py:218 delegates to asyncio.to_thread; cancelling the awaiting coroutine does not stop an already-running upload, which can still commit. The new handlers cover exceptions inside the worker.

By source tracing, the new tests would detect the original empty/partial-commit defect through observable upload calls. The interrupt test covers an exceptional future, but not interrupted cleanup through S3File.commit().

Author: deferred as pre-existing and out of scope for #1014. Every asyncio.to_thread delegation in AioS3FileSystem behaves this way, not only put_file().

[laughingman7743]

No change. Recorded as pre-existing and outside #1014. The interrupted-cleanup-through-commit() gap is now covered by TestS3File.test_commit_failure_and_discard (d59f880).