[laughingman7743]

WHAT

• Append: before an S3File write submits a part, it now checks the part copies (UploadPartCopy) of an append that have finished, and raises the error of a failed one. The file is then closed without committing it and the upload is aborted, as Stop a multipart write at a part found failed while it waits #1176 does for a failed uploaded part. The part copies are tracked in their own set (_copies_in_flight), so they still do not count toward max_workers, and the check does not depend on the write waiting for a part.
• Sync multipart copy: S3FileSystem._finish_multipart_upload(), used by the multipart copy of cp, now waits with return_when=FIRST_EXCEPTION and raises a failed part from the futures returned as done, before collecting the parts in part-number order. A failed part copy is therefore raised without waiting for the part copies before it, and the part copies queued after it are cancelled before they start. The running ones are still waited for before the abort.
• S3File.commit(): waits the same way before collecting the parts, so a failed part, such as one of the many part copies of a large append, is handled without waiting for the parts before it.
• docs/filesystem.md and the docstrings of _upload_chunk(), commit(), and _finish_multipart_upload() describe the new behavior.

Behavior change for the release notes: in an append, the error of a failed part copy can now be raised by a write(), flush(), or close() that submits a part, instead of only by close()/commit(). This matches the uploaded parts after #1176.

WHY

Closes #1178.

• After Stop a multipart write at a part found failed while it waits #1176, a failed part copy of an append did not stop the write, because the part copies are not in _uploads_in_flight. The write uploaded all of the new data, and close() or commit() then raised the error and aborted the upload.
• The sync multipart copy, and S3File.commit(), collected the parts in part-number order. When a later part failed, the queued parts kept running until the loop reached it, and each one was a billed request. AioS3FileSystem._copy_object_with_multipart_upload() already stops at the first failure.

A done callback on each part was not used for the append. A callback bound to the file would put the file in a reference cycle, so a file dropped without close() would no longer be closed (and committed by fsspec's __del__) when its last reference goes away.

TEST

Tested commit: 5063729 (Python 3.13.1, macOS).

• just format and just lint: passed. mise exec -- markdownlint-cli2 docs/filesystem.md: 0 errors.
• uv run --env-file .env pytest -n 8 tests/pyathena/filesystem/: 1221 passed, 9 skipped, including the live S3 tests.
• New tests, all of which fail on master:
  • TestS3File.test_append_failed_part_copy (max_workers 1 and 4): the part copy of an append is held until the first write() returns and has failed before the second, which raises with or without waiting for a part. Only one uploaded part was submitted, the upload is aborted once, and nothing is completed.
  • TestS3FileSystem.test_finish_multipart_upload_stops_at_failed_part and TestS3File.test_commit_stops_at_failed_part: part 2 has failed while part 1 is still copying. The cleanup starts before part 1 finishes, part 3 is cancelled, and the upload is aborted.
• Changed tests whose fake wait() did not expect the new wait:
  • test_finish_multipart_upload_waits_for_running_parts now asserts both waits.
  • test_write_parts_in_flight and test_open_parts_in_flight now count and steer only the FIRST_COMPLETED waits.
• The write-file test helper sets _copies_in_flight.
• The tests that replace wait(), and the new tests, passed 5 times in a row; test_append_failed_part_copy passed 10 times in a row on 5063729.

Not run:

• An AioS3FileSystem append test. AioS3File shares _upload_chunk(), _initiate_upload(), and commit(), and Stop a multipart write at a part found failed while it waits #1176 covered the wait with both executors.
• just test pyathena as a whole, since no cursor or SQLAlchemy code changed.
• AWS CI, which runs once the PR is Ready.

🤖 Generated with Claude Code

[laughingman7743]

Self-review round one (behavior and implementation)

• Base: 3a5018225e536ba5b0a920465e3f141dae0447e4, head: f135e16b99c518a08bd17a489e03c5fa8105c6c8 (round one started at 8cf11bf0; repairs are in f135e16b).
• Covered:
  • S3File._upload_chunk(), _initiate_upload(), commit(), and discard();
  • S3FileSystem._finish_multipart_upload() and its caller _copy_object_with_multipart_upload();
  • AioS3File, which inherits all of them;
  • the tests that replace wait(), the new tests, and the docs.
• Inputs: /code-review (high) and a /simplify pass (reuse, simplification, efficiency, altitude).
• Result: FINDINGS, repaired or deferred as below.

Repairs in f135e16b:

1. The part copies were checked only inside the max_workers wait branch, so an append that never filled max_workers (default cpu_count() * 5) still uploaded all its data after a failed copy. They are now checked before each part is submitted, and before the wait, so a copy that has already failed does not wait on an upload either (this line).
2. S3File.commit() collected multipart_upload_parts in part-number order. A failed later part, such as one of the many part copies of a large append, waited for the parts before it while the queued ones started. It now waits with FIRST_EXCEPTION as _finish_multipart_upload() does.
3. _finish_multipart_upload() rescanned all futures behind an if pending guard. It now iterates the futures returned as done, as the aio copy does.
4. The done | copied union raised an arbitrary one of two failures. The two checks are now separate.
5. Tests:
   • _make_write_file did not set _copies_in_flight;
   • test_finish_multipart_upload_waits_for_running_parts had been weakened to its last call, and now asserts both waits;
   • test_write_parts_in_flight/test_open_parts_in_flight now steer and count only FIRST_COMPLETED waits, because commit() also waits now.

Deferred or rejected:

• A done callback per part, which would check without polling, was rejected. A callback bound to the file puts the file in a reference cycle, which delays fsspec's close-and-commit of a file dropped without close() (measured for Stop a multipart write at a part found failed while it waits #1176).
• The set comprehension costs O(running copies) per part. The set empties as the copies finish, and a copy part is up to 5 GiB, so the set is small. Not changed.
• Test assertions that overlap the neighbouring uploaded-part test were kept, for symmetry.

[laughingman7743]

Self-review round two (claims, callers, operations, evidence)

• Base: 3a5018225e536ba5b0a920465e3f141dae0447e4, head: f135e16b99c518a08bd17a489e03c5fa8105c6c8.
• Scope: a full pass over the PR title and body, the commit messages, the changed docstrings and comments, this docs paragraph, and the premises of Failed part copies do not stop an append or a sync multipart copy #1178.
• Result: CLEAN after one PR-text correction.

Claims checked:

1. "The check does not depend on the write waiting for a part." The copy check runs at the top of every iteration of iter_parts(), outside the max_workers branch. test_append_failed_part_copy covers max_workers=4, where the second write never waits.
2. "Queued part copies are cancelled before they start; running ones are waited for before the abort." On the raise, _finish_multipart_upload() and commit() (through discard()) cancel the futures they can and wait for the rest. Both new tests assert the cleanup started before part 1 finished and that part 3 was cancelled.
3. "AioS3FileSystem._copy_object_with_multipart_upload() already stops at the first failure." It uses asyncio.wait(tasks, return_when=asyncio.FIRST_EXCEPTION) (pyathena/filesystem/s3_async.py:641).
4. "AioS3File shares _upload_chunk(), _initiate_upload(), and commit()." AioS3File (pyathena/filesystem/s3_async.py:1030) overrides none of them.
5. Docs: "before the write uploads another part" matches claim 1. "A part that fails later raises its error when the file is closed, or when the transaction commits" matches commit(). The cp paragraph ("A failed part copy aborts the multipart upload ... the part copies in flight finish first") is still accurate. No other text in docs/ describes the order of part failures.

Existing callers and operations:

• Interrupts during the new waits go through the same except BaseException cleanup as before.
• A future cancelled by S3AioExecutor's loop shutdown is skipped by the new scans, and still raises CancelledError from the in-order collection, as before.
• wait() over up to 10,000 futures adds one waiter per future, which is O(n) like the previous result() loop.
• Requests after a failed part copy drop from all the remaining parts or copies to the ones already running.

Evidence: local only, Python 3.13.1 on macOS.

• The full tests/pyathena/filesystem/ suite: 1221 passed, 9 skipped, with live S3, on the code of f135e16b.
• The 4 new test cases fail on master.
• The tests that replace wait() passed 5 runs in a row.
• AWS CI has not run (Draft).

Correction: the PR title said "commits at a failed part copy", but commit() stops at any failed part. It now reads "Stop appends, sync multipart copies, and commits at a failed part".

[laughingman7743]

Independent review (relayed result, static review)

• Reviewer: Codex CLI 0.162.1, model gpt-6-astra, reasoning effort high, run with codex exec --sandbox read-only. Session 01a13b27-261d-7221-87e4-9f3948d21b39.
• Base: 3a5018225e536ba5b0a920465e3f141dae0447e4. Head: f135e16b99c518a08bd17a489e03c5fa8105c6c8. Reviewed in a detached worktree at that head. The snapshot and the PR worktree were unchanged afterwards.
• Constraints:
  • No edits, builds, tests, GitHub access, or network access.
  • The prompt carried the literal diff and the intended behavior. It left out the PR number, PR text, commit messages, and prior findings.
• Coverage:
  • the S3File lifecycle methods, the sync multipart copy and its finalization, both executors and their cancellation notification;
  • AioS3File inheritance, compared against the aio copy;
  • fsspec write/flush/close and transactions;
  • part ordering, large part counts, cleanup, interrupts, the changed tests, and the docs.
• The reviewer noted that the patched wait() helpers still tell upload backpressure apart from the commit and cleanup waits.
• Result: FINDINGS, one finding.

P2: race in the append regression test. Location: tests/pyathena/filesystem/test_s3.py:7264 at f135e16b. The first write() submits the part copy in _initiate_upload(). The worker can raise PermissionError before that same write reaches the new copy check in _upload_chunk(). The first write then raises outside pytest.raises, and the test fails even though the implementation is correct. The later wait() does not prevent this. Suggested fix: hold the copy behind an event until the first write returns, then release it, and release it in cleanup too.

Verified against the code: the race is real. With max_workers=1, the worker starts the copy as soon as it is submitted. The test had passed only because of timing. The reviewer found no other actionable defects.

[laughingman7743]

Repair of the independent review finding (f135e16b -> cfd6d3b9 -> 50637290, test only)

• cfd6d3b9: the part copy now waits for an event that the test sets after the first write() returns, so the copy can no longer fail before that write checks the part copies.
• Codex follow-up on f135e16b..cfd6d3b9 (session 01a13b2e-b0eb-7c92-b6ac-c008d86808b1, gpt-6-astra, high, read-only) returned FINDINGS:
  • P2: the copy's release.wait(5) would fail the first write after a 5-second stall.
  • P2: an assertion failure left the executor's threads running.
• 50637290: the copy waits without a timeout. That is safe because a finally block always sets the event. The same block shuts down the executor.
• Codex follow-up on f135e16b..cfd6d3b9..50637290 (session 01a13b34-7e59-7ab0-a7c1-e22440ea597b, same settings): CLEAN. The race is gone for max_workers 1 and 4. Cleanup releases and joins the workers. The test still fails without the copy check. The remaining 5-second timeout on the completion wait() cannot leak threads.

Self-review of the repair:

• Round one: the repair is limited to this test, and no implementation or shared helper changed.
• Round two: the PR body now describes the held copy and names 50637290 as the tested commit.

Validation on 50637290:

• The test passed 10 times in a row.
• With the pyathena/ changes reverted, it fails (2 of 2) without hanging.
• just lint passed.
• The full tests/pyathena/filesystem/ suite passed: 1221 passed, 9 skipped, including live S3.