Keep a multipart upload whose abort fails so that discard() retries it

[laughingman7743]

WHAT

When the abort of a multipart upload fails, S3File now keeps the upload so that a later discard() (for example, a transaction rollback) retries the abort. This covers both places where the upload ID used to be dropped.

A failed completion in commit() (#945)

• S3FileSystem._finish_multipart_upload() takes abort: bool = True. With abort=False, it re-raises the original error without cancelling the parts or aborting the upload. The multipart copy caller keeps the default.
• commit() passes abort=False and calls discard() itself on any failure, including an interrupt. discard() already waits for the running parts, aborts, and clears the upload ID only after a successful abort. A failed abort is logged with the upload ID, as the helper logged it, and the original completion error still propagates.

A failed write in _close_without_commit() (used by pipe_file()/put_file() and by a write that exceeds the part limit)

• _close_without_commit() no longer clears the upload after a failed or interrupted abort, and it logs the upload ID.
• It used to clear the upload so that a deferred commit() could not complete a failed write. commit() now returns early for a file whose buffer _close_without_commit() dropped, so the kept upload is never completed. The two existing buffer is not None checks inside commit() became that single guard.

Behavior changes:

• If an abort fails (or is interrupted) after a failed completion or a failed write, multipart_upload and the part futures stay set, and a later discard() sends AbortMultipartUpload again. Previously, the upload ID was dropped and the incomplete upload stayed until a lifecycle rule or clear_multipart_uploads() removed it. commit() of a failed write still does nothing, so after a transaction commit, only the logged upload ID points to the upload.
• After an interrupted completion whose abort succeeds, the upload ID is now cleared, so a later discard() no longer sends a second AbortMultipartUpload for the upload that was already aborted.
• commit() of a failed write no longer invalidates the cache of the path. Nothing was written.

The branch is rebased onto master after #1036, which extracted the abort into _abort_multipart_upload(). The abort check comes before that call.

WHY

Fixes #945. Supersedes #949.

TEST

Tested commit 56bfa0b (rebased onto 8c9d676).

• just lint: passed (ruff, ruff format, mypy, cfn-lint, license headers).
• Live, against the CI test account: uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/: 643 passed.
• The changed tests fail on the source without the corresponding change:
  • test_commit_failure_and_discard (4 cases) and test_finish_multipart_upload_without_abort, checked on f9eedea against 28ec68d.
  • test_write_exceeding_max_parts_abort_failure (2 cases) and test_write_exceeding_max_parts_abort_interrupted, checked on 56bfa0b against ebf798f.
• test_commit_failure_and_interrupted_abort also passes on the original source: an interrupted abort in commit() already kept the upload there. It guards that behavior under the new commit().
• Abort failures are simulated with mocks. No live abort failure was produced.

🤖 Generated with Claude Code

[laughingman7743]

Self-review round one (implementation behavior) — FINDINGS (repaired)

Base 28ec68d9f70b5bc2fb447f6a4fb745ee0a467683, reviewed head f9eedeae41d7864839ea2eec75f21d8074486100, repair head 819d7d1adc6f4bb6d5734d82307b9286e1f543ee.

Covered: _finish_multipart_upload() (new abort flag) and both callers (cp_file multipart copy keeps the default; S3File.commit() passes abort=False), S3File.commit()/discard()/_close_without_commit(), AioS3File (inherits commit()/discard(), no override), fsspec Transaction.complete() (2026.9.0), and the changed tests.

• Failure path equivalence: discard() cancels the pending parts, waits for the running ones, and sends abort_multipart_upload with _get_operation_kwargs("abort_multipart_upload", s3_additional_kwargs). That matches what the helper sent with request_kwargs=self.s3_additional_kwargs, including the event-loop-thread case (cancelled parts are not waited for).
• The bare raise after the inner except Exception re-raises the original completion error, which match="complete failed" asserts.
• An abort interrupted inside discard() propagates before the state is cleared, so the upload is kept, as before.
• Finding (repaired in 819d7d1): the comment said a transaction calls discard() after a failed commit(). fsspec is unpinned, and only recent Transaction.complete() re-queues the failed file for discard(), so the comment now says "such as a transaction rollback".
• Not changed (pre-existing, intentional): _close_without_commit() clears the upload after a failed abort so that a deferred commit() cannot complete a failed write (s3.py:3337).
• Tests: the 4 test_commit_failure_and_discard cases and test_finish_multipart_upload_without_abort fail with the source change reverted. Offline run of the affected tests: 42 passed after the repair.

[laughingman7743]

Round one on the repair (5e09af9b..721d24a1, tests only): CLEAN. test_commit_failure_and_discard now also asserts that the part futures are kept after a failed abort. The new test_commit_failure_and_interrupted_abort drives the real _finish_multipart_upload() with abort=False, makes the first abort_multipart_upload raise KeyboardInterrupt, and asserts that the interrupt propagates, the upload and parts are kept, and discard() retries the abort and clears the state. No source changed. Offline: 69 passed.

[laughingman7743]

Self-review round two (claims, compatibility, operations) — FINDINGS (repaired)

Base 28ec68d9f70b5bc2fb447f6a4fb745ee0a467683, reviewed head 819d7d1adc6f4bb6d5734d82307b9286e1f543ee, repair head 5e09af9bbac5504612108c3bbecb7718420c6200.

Claims checked:

• "With abort=False the helper re-raises without cancelling or aborting": if not abort: raise comes before the cancel/wait/abort; asserted by test_finish_multipart_upload_without_abort.
• "The multipart copy keeps the default": cp_file's call (s3.py:1777) passes no abort.
• "discard() clears only after a successful abort": the clearing follows _call() with no finally, so a raising abort skips it.
• "Previously, an interrupted completion was aborted twice": the old helper aborted on BaseException, and old commit() caught only Exception, so it kept the state for a second abort in discard(). The PR body does not claim what S3 returns for that second abort.
• "_close_without_commit() clears on purpose": its docstring states that commit() must not complete a failed write afterwards. Keeping the upload there would let commit() complete it.
• Docs: docs/filesystem.md (transactions and incomplete-upload management) is still accurate.
• Existing caller: _finish_multipart_upload() is private, and the new keyword has a compatible default. AioS3File inherits the methods.

Findings (repaired in 5e09af9):

1. Operator: the new commit() log line dropped the upload ID that the helper logged ("Failed to abort multipart upload {upload_id} to s3://…"). On a non-transactional close(), nothing calls discard() again, so the log is the only pointer for a manual cleanup. The ID is restored here, and the test asserts it.
2. The PR body claimed a conflict with Copy metadata, tags and annotations in multipart copies #1036. git merge-tree merges the two heads without conflicts, so the body is corrected.

Limitation: no live S3 run; abort failures are simulated with mocks. AWS CI comes after Ready.

[laughingman7743]

Round two on the repair (5e09af9b..721d24a1): CLEAN after a PR text update. The TEST section now names 721d24a1 and 69 passing tests, and states that the interrupted-abort test also passes on the original source, so it is a guard rather than regression evidence. The commit() comment's claim that the upload is kept if the abort is interrupted is now backed by that test. No other claims changed.

[laughingman7743]

Independent review (relayed): Codex CLI 0.160.0, model gpt-6-astra (OpenAI), sandbox read-only, --ephemeral, session 01a102a7-5eca-7790-96c6-8aeb2d1b75b6. Static review only.

Base 28ec68d9f70b5bc2fb447f6a4fb745ee0a467683, head 5e09af9bbac5504612108c3bbecb7718420c6200, on a detached snapshot worktree. The prompt had no PR number, PR text, commit messages, or prior findings, and asked for no edits, tests, or GitHub access. Both the snapshot and the PR worktree were unchanged afterwards.

Result: FINDINGS, both pre-existing, not introduced by this diff:

Reviewer output (verbatim)

Covered commit(), discard(), _close_without_commit(), autocommit close, both helper callers, async delegation/inheritance, executor cancellation, repository transaction integration, request filtering, logging, subclasses, and regression tests.

FINDINGS — two pre-existing gaps remain; neither is introduced by this diff.

1. P2 — An abort-time interrupt replaces the original completion error. s3.py:3533
   CompleteMultipartUpload raises OSError, then discard() raises KeyboardInterrupt while waiting or aborting → commit() propagates KeyboardInterrupt, bypassing the original-error re-raise and abort-failure logging. The upload state survives, but the stated original-error guarantee does not. The previous helper had the same limitation. Preserving that guarantee requires handling cleanup BaseException separately before re-raising the completion error.

2. P2 — _close_without_commit() still destroys failed-abort retry state. s3.py:3355
   A multipart write exceeds the part limit, and its abort raises PermissionError → the finally block clears the upload ID and futures anyway. A later discard() makes no abort request, leaving the upload behind. This unchanged path falls outside the repaired completion branch. Retain cleanup state while separately preventing the failed write from being committed.

The changed completion path otherwise retains both fields after ordinary abort failures and clears them after successful abort. Abort request filtering and field precedence are unchanged, including RequestPayer, ExpectedBucketOwner, and SSE-C filtering. The default helper behavior remains compatible with the sync copy caller; AioS3File inherits the repair.

By inspection, the ordinary completion-error/abort-failure test would fail on the original code’s upload-state assertion. It checks retry requests and logging, but does not assert retained futures before retry or cover interruption during abort. Reusing discard() with an explicit helper opt-out is a simple design consistent with the surrounding code.

Static review only: no edits, builds, tests, or GitHub access. Transaction assessment used tracked integration code and tests; fsspec internals are not tracked here.

Author disposition (verified against the code):

1. Interrupt during the abort replaces the original error (s3.py:3533): kept as is. An interrupt must propagate, not be swallowed. The completion error stays as __context__, and the upload is kept for discard(). The old helper behaved the same way.
2. _close_without_commit() still clears the upload after a failed abort (s3.py:3337): deferred as out of scope. The clearing is what stops a deferred commit() from completing a failed write. Keeping the upload ID while blocking the commit needs a separate design and will be proposed as a follow-up issue.
3. Test gaps the reviewer noted (part futures kept before retry; interrupted abort not covered): repaired in 721d24a1. test_commit_failure_and_interrupted_abort passes on the original source too, so it guards existing behavior rather than proving a regression.

[laughingman7743]

Independent follow-up (relayed): Codex CLI 0.160.0, model gpt-6-astra, sandbox read-only, --ephemeral, session 01a102b1-a9af-7113-864f-ecb713634492. Static review of git diff 5e09af9bbac5504612108c3bbecb7718420c6200..721d24a1fa1aca199a38224b6a95d9e425c0d2e6 (tests only), on a detached snapshot that stayed unchanged.

Result: CLEAN. Reviewer summary: the new assertion checks that the part futures are kept before the retry, across both completion errors and successful and failed aborts. The new test drives completion failure, then an interrupted abort, then the propagated KeyboardInterrupt, the kept state, a successful discard() retry, and the cleared state. _finish_multipart_upload delegates to the production code, and commit()/discard() are real, so premature clearing, a swallowed interrupt, or a missing retry would be caught. No spurious-pass issue was found.

[laughingman7743]

Self-review round one, expanded scope (implementation behavior): CLEAN

Range ebf798f6dd06f394a84c1cf054a0a7561dd9ef41..56bfa0b4f42b71476008cc81fc13f80b8f7ae07d (expanded contract, so this is a full pass over _close_without_commit()/commit() and their callers). Branch base is ec5323ea after the rebase; git range-diff showed the earlier four commits unchanged.

Covered: _close_without_commit() and its three callers (_write_and_close() for sync/aio pipe_file, _write_file_and_close() for sync/aio put_file, and _upload_chunk() at the part limit), commit(), discard(), AioS3File (no overrides), S3AioExecutor.shutdown() (no-op), and fsspec close()/flush().

• buffer is None happens only after _close_without_commit(): fsspec only assigns io.BytesIO() to the buffer of a write-mode file. So the guard affects only failed writes. The two removed inner checks were for the same state.
• The cast in the except is safe: discard() raises only from _call(), inside if self.multipart_upload, and it does not clear on failure.
• Retry: discard() on the closed file uses neither the buffer nor the executor; it waits for uncancellable parts and aborts again. close() on the closed file returns early.
• An interrupted abort propagates through finally (executor shut down) with the upload kept. The callers re-raise in any case.
• Design choice: commit() of a failed write stays a no-op rather than retrying the abort, so a transaction commit does not newly raise abort errors. Rollback (discard()) retries.
• Tests: the two part-limit abort tests now assert the kept upload, the logged upload ID, and an identical second abort request from discard(), followed by cleared state. They fail on ebf798f6. Live tests/pyathena/filesystem/: 643 passed.

[laughingman7743]

Correction: after the rebase, the branch base (merge base with master) is 8c9d676b77e86a7069dc1a47cd7a54d82a09c802 (#1035 merge), not ec5323ea. The review conclusions are unchanged.

[laughingman7743]

Self-review round two, expanded scope (claims, compatibility, operations): FINDINGS (PR text, repaired)

Range ebf798f6dd06f394a84c1cf054a0a7561dd9ef41..56bfa0b4f42b71476008cc81fc13f80b8f7ae07d, full claim pass over the updated PR body, docstrings, comments, and commit message.

• "Used by pipe_file()/put_file() and the part-limit write": confirmed by grep (s3.py:2266, s3.py:2527, s3_async.py:208, s3_async.py:280, _upload_chunk).
• "commit() of a failed write does nothing / no longer invalidates the cache": the early return comes before invalidate_cache(). Nothing was written, so no listing changes.
• "After a transaction commit, only the logged upload ID points to the upload": fsspec Transaction.complete(commit=True) calls commit(), which returns, so the file is not discarded. Rollback calls discard().
• Docs: docs/filesystem.md:104 ("raises ValueError and aborts its multipart upload") is still true; the upload is only kept when that abort fails. The incomplete-upload section (:214) still applies.
• Finding: the PR body still described Copy metadata, tags and annotations in multipart copies #1036 as a pending overlap, but Copy metadata, tags and annotations in multipart copies #1036 is merged and in this branch's base. The abort check precedes _abort_multipart_upload() (s3.py:2338). The body is corrected.
• Operator: both abort-failure logs now carry the upload ID, so a leaked upload can be found or aborted by ID.
• Limitation: abort failures are mocked; the live run covers the success and rollback paths.

[laughingman7743]

Independent review, expanded scope (relayed): Codex CLI 0.160.0, model gpt-6-astra (OpenAI), sandbox read-only, --ephemeral, session 01a1044e-d11e-7791-839c-4b62ee10550d. Static review only.

Range 8c9d676b77e86a7069dc1a47cd7a54d82a09c802..56bfa0b4f42b71476008cc81fc13f80b8f7ae07d, full pass, on a detached snapshot that stayed unchanged. The prompt had no PR number, PR text, commit messages, or prior findings. The reviewer was pointed to the installed fsspec source to check the buffer lifecycle.

Result: FINDINGS, one pre-existing item and no regressions:

Reviewer output (verbatim)

Covered the named upload/cleanup methods, sync and async callers, executor cancellation/waiting, event-loop-thread discard, fsspec transactions, autocommit close, xb, abort parameters, logging, and test assertions against the base revision. Inspected the installed fsspec buffer lifecycle: normal write-mode files retain a BytesIO, so the new buffer is None guard does not skip legitimate commits.

FINDINGS

• P2 — Pre-existing mismatch with the stated error-preservation contract: s3.py:3930, also s3.py:3704. If completion raises RuntimeError and abort then raises KeyboardInterrupt, except Exception misses the abort interruption. The upload state survives, but KeyboardInterrupt replaces the original propagated error and no abort-failure message with the upload ID is logged. The same applies to cleanup after a failed write. This behavior already exists in the base; the new test_s3.py:5612 explicitly expects it. Preserving the original error across abort interruptions requires changing these handlers and that expectation.

No other actionable defects or introduced regressions found. The retention/retry assertions would fail against the original implementation by source inspection; the interrupted-abort commit test preserves existing behavior. The changes remain straightforward.

Static review only; no builds, tests, or GitHub access.

Author disposition: this is the same pre-existing interrupt behavior the first independent review raised. It is kept on purpose: an interrupt during the abort must propagate rather than be swallowed or re-ordered behind the completion error, the original error remains its __context__, and the upload is kept for discard(). No change. The reviewer confirmed that the buffer is None guard does not skip legitimate commits (fsspec keeps a BytesIO for normal write-mode files).