Keep multipart uploads within the 10,000-part limit

[laughingman7743]

WHAT

Keep S3 multipart uploads within the 10,000-part limit.

• Writes (S3File): before submitting a part beyond S3FileSystem.MULTIPART_UPLOAD_MAX_PARTS (10,000), _upload_chunk() aborts the multipart upload and raises ValueError. The error asks for a block_size (or the filesystem's default_block_size) large enough for the object to fit in 10,000 parts. The file is closed and its buffer dropped before the abort, so neither close() nor a later commit() of a deferred write uploads a partial object. An abort failure is logged and does not replace the ValueError. In an append, the parts copied from the existing object count toward the limit. This applies to open(), pipe_file(), and put_file(), and to AioS3File, which inherits _upload_chunk().
• Copies (cp_file(), AioS3FileSystem._cp_file(), append copy): _get_copy_ranges() raises the split size to ceil(size / 10,000) when block_size is smaller. This is at most about 550 MB for an object within the 5 TiB limit.
• S3File.close(): shuts its executor down in a finally block, so a failing final flush, such as the part limit error, no longer skips the shutdown.
• Docs: docs/filesystem.md states the limit, the about 48.8 GiB maximum for put and pipe at the default block size, how to set a larger block size, that a file written with open can take more parts (each write that fills the buffer uploads its data beyond the last full block as a separate part), and that an append's copied parts count toward the limit.

The maintainer chose to fail writes with a clear error rather than grow the part size during the write: the size of a streamed write is not known in advance.

Release-note items:

• A write that needs more than 10,000 parts (more than about 48.8 GiB at the default 5 MiB block size) now raises ValueError and aborts its multipart upload. The error is raised when part 10,001 would be submitted, after the first 10,000 parts were uploaded. Before, part 10,001 was submitted and S3 rejected it (per the API documentation; not reproduced).
• A multipart copy with a small block_size now uses larger parts instead of failing at 10,000 parts.

WHY

Closes #953.
S3 accepts part numbers from 1 to 10,000. S3File numbered write parts with no upper bound. _get_copy_ranges() (from #955) split copies by block_size without checking the number of parts.

TEST

Tested commit: 8b99abb (docs-only change after b8224b1, which the offline tests ran against).

• just format and just lint: pass.
• Offline unit tests, run with a dummy region and no credentials (--noconftest):
  • TestS3FileSystem::test_get_copy_ranges_max_parts uses the real 10,000-part limit and covers exactly 10,000 parts, one byte over, 50 GiB, and 5 TiB.
  • TestS3File::test_write_max_parts, test_write_exceeding_max_parts, and test_write_exceeding_max_parts_abort_failure use a limit of 3 on a mocked filesystem. They cover the limit reached by write() and by close(), an append whose copied part counts toward the limit, an abort failure, and autocommit True/False, including a commit() after the error.
  • The failure tests record the submitted parts through a wrapped executor and check its shutdown, so they do not depend on whether the abort cancels queued uploads (30 repeated runs: all passed).
  • With the source change of 692167d reverted (only the constant kept), the 3 copy cases and the 6 exceeding-write cases fail; the cases that stay within the limit pass. The abort-failure cases fail without ff6de77, and the cases that reach the limit in close() fail without the close() change of b8224b1.
• tests/pyathena/filesystem/ offline: 163 passed. The 108 failures are the same set as on the base commit: AWS integration tests stopped by NoCredentialsError, so no request was sent.
• Not run: live AWS tests. The new behavior needs 10,000 parts (about 48.8 GiB at the default block size), so it was not reproduced on real S3 (cost). The existing S3 write, append, and copy integration tests run in the AWS CI once the PR is Ready.

🤖 Generated with Claude Code

[laughingman7743]

Self-review round one (behavior and implementation) — base f3ef301d9fec92f1fc53b147bebfef87bfc919ca, head 692167df5d21928f908bc2d00aeca573a899d51f. Result: FINDINGS (F1, F2; both repaired in a follow-up commit, see reply).

Covered: S3File._upload_chunk limit check (sync S3File and AioS3File, which inherits it), fsspec 2026.9.0 flush()/close() flow when _upload_chunk raises (mid-stream write() and final close()), deferred commit() and transaction rollback through discard(), append copied parts counting toward the limit, _get_copy_ranges callers (sync/async _copy_object_with_multipart_upload, append path), docs, and the tests.

F1 — an abort failure masks the limit error. pyathena/filesystem/s3.py:2379: self.discard() runs before self.buffer = None / self.closed = True. If AbortMultipartUpload fails, its error propagates instead of the ValueError, the file stays open with its buffer, and close() flushes again, reaching the check and retrying the abort. Reproduced with a mocked filesystem whose _call raises PermissionError: the caller saw PermissionError: abort failed and the abort was called twice.

F2 — after an abort failure, a deferred commit() completes a partial object. Same line: discard() raises before clearing multipart_upload_parts, so with autocommit=False a later commit() (e.g., a transaction that swallowed the error) takes the _finish_multipart_upload branch and completes the first 10,000 parts. Verified by the extended regression test failing without the repair.

Repair: close the file and drop the buffer first, log an abort failure as _finish_multipart_upload does, clear the upload state on that failure as commit() does after its own abort, and always raise the ValueError.

[laughingman7743]

Repaired F1 and F2 in ff6de77: the file is closed and its buffer dropped before discard(); an abort failure is logged and clears multipart_upload/multipart_upload_parts (as commit() does after its own abort); the ValueError is always raised. New test_write_exceeding_max_parts_abort_failure[True/False] fails without the repair (PermissionError for autocommit=True's sibling scenario, and a completed upload after commit() for autocommit=False) and passes with it. just lint passes; offline tests/pyathena/filesystem/: 163 passed, the 108 credential-less integration failures are the same set as on the base.

[laughingman7743]

Self-review round one — copy split size checked, no finding: math.ceil(size / 10_000) is exact for sizes within the 5 TiB object limit (below 2**53), stays ≥ MULTIPART_UPLOAD_MIN_PART_SIZE because the validated block_size is the lower bound, and stays ≤ about 550 MB, so a merged short tail never needs the half split and the count stays ≤ 10,000. The async copy reaches it through self._sync_fs._get_copy_ranges.

[laughingman7743]

Self-review round two (claims, callers, operations) — base f3ef301d9fec92f1fc53b147bebfef87bfc919ca, head ff6de7757fd1a946ddc37c0d8024bb7ba17356ba. Result: FINDINGS (F3; repaired, see reply).

F3 — the block size guidance is not enough for an append. pyathena/filesystem/s3.py:2394 (and the docs formula): "at least its total size divided by 10,000" holds for a plain write, but an append to an object below 5 GiB copies it as one extra part. With block = T/10,000 and an existing object E with E + 5 MiB ≤ block, the appended data takes 9,999 full blocks plus a tail of at least 5 MiB, i.e. 10,000 parts plus the copy = 10,001 (e.g., appending ~200 GiB to a 5 MiB object). Reproduced scaled down (limit 10, existing 4 B, appended 196 B, block 20): ValueError. Larger existing objects keep enough slack (copied parts ≤ E / 5 GiB + 1 versus 10,000·E/T ≥ E / 550 MB). Repair: the message asks for a block size large enough to fit in 10,000 parts, including copied parts; the docs keep the formula for writes and state that an append's copied parts count toward the limit.

[laughingman7743]

Repaired F3 in 41abb14: the error now asks for a block_size (or default_block_size) "large enough for it to fit in 10000 parts, including the parts copied from the existing object in an append", and docs/filesystem.md adds "In an append, the parts copied from the existing object also count toward the limit." just lint, just docs lint, and the offline max_parts tests (18) pass.

[laughingman7743]

Self-review round two — claims checked against evidence:

• 1 to 10,000 part numbers: botocore S3 model docs for UploadPart/UploadPartCopy PartNumber ("a positive integer between 1 and 10,000").
• ~48.8 GiB = 10,000 × 5 MiB at DEFAULT_BLOCK_SIZE; parts are one per block except a merged short tail (_upload_chunk lookahead).
• block_size of pipe reaches open(): fsspec 2026.9.0 pipe → PyAthena pipe_file → fsspec pipe_file → self.open(path, "wb", **kwargs). put/put_file opens without block_size, so default_block_size is the documented way there (also for AioS3FileSystem._put_file, which delegates to the sync put_file).
• cp copies stay within the limit: cp_file → _copy_object_with_multipart_upload (> 5 GiB only) → _get_copy_ranges; same for the async copy.
• Caller compatibility: the new exception is ValueError (release-noted); buffer: BytesIO | None is an annotation only.
• Operations: the error comes after the first 10,000 parts are uploaded (release note updated); the abort is a single request through _call's retry config; a failed abort is logged and the incomplete upload stays listable/clearable (list_multipart_uploads/clear_multipart_uploads).
• Deferred (pre-existing discard() behavior, not introduced here): part uploads already running when the abort is sent may still complete.
• Evidence: local offline tests only; live AWS coverage comes from CI once Ready. PR body corrected (message wording, release note timing, tested commit).

[laughingman7743]

Independent review (relayed) — reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, sandbox read-only, session 01a100b3-237e-7d81-81b1-935b1d6ce6d7. Base f3ef301d9fec92f1fc53b147bebfef87bfc919ca, head 41abb14fde412a6a81dd20d54201db3875cac369, detached snapshot; given the diff, the intended behavior, and repository conventions only (no PR text, commit messages, or prior findings). Static review only: no builds, tests, or network access. Result: FINDINGS (3).

Covered (reviewer): the supplied diff, sync/async write and copy callers, append accounting, fsspec 2026.9.0 buffering and transactions, abort handling, executor lifecycle, typing, documentation, and tests. The reviewer confirmed that the per-part guard covers multi-block writes, merged/split tails, and copied append parts; that clearing the state prevents later publication, including after an abort exception; that copy ranges satisfy count, size, and coverage bounds through 5 TiB; and that no supported flush path exposes None through the cast.

1. P2 — Limit failures can leave the executor running. (reviewer text, condensed) With autocommit=False, write 10,000 full blocks followed by one byte, then call close(). The final flush raises ValueError, so execution never reaches _executor.shutdown() at pyathena/filesystem/s3.py:2286; the file is already marked closed, so __del__ skips cleanup. A direct mid-stream write() failure also closes the file without shutting down its executor. The new failure path introduces this exposure; close() lacking exception-safe shutdown is pre-existing.

Author verification: confirmed for the final-flush path (super().close() raises before shutdown()); a mid-stream failure followed by close() reaches shutdown() because super().close() returns early on a closed file. Pre-existing for other final-flush errors; folded as a contained fix.

[laughingman7743]

Repaired in b8224b1: S3File.close() shuts the executor down in a finally block, so a failing final flush (the part limit error or any other) no longer skips it. The new executor.shutdown.assert_called_once() in test_write_exceeding_max_parts fails for the close()-path cases without the repair and passes with it.

[laughingman7743]

Self-review of the repair b8224b1 (both perspectives, range 41abb14f..b8224b19, same base). Behavior: the try/finally in close() does not change which exception propagates; S3AioExecutor.shutdown() is a no-op and S3ThreadPoolExecutor.shutdown() behaves as on a normal close; a mid-stream error followed by close() reaches shutdown() as before. Claims: put_file writes remote.blocksize chunks (one part per block) and pipe writes once (blocks plus a merged tail), so the docs formula holds for them; the open sentence says "can" because a remainder below the minimum part size is merged. Offline tests/pyathena/filesystem/: 163 passed, the same 108 credential-less integration failures as the base. Result: CLEAN.

[laughingman7743]

Independent review (relayed), finding 2. P2 — Failure tests have scheduling-dependent assertions. (reviewer text, condensed) tests/pyathena/filesystem/test_s3.py:2053 and :2076 use a real thread pool but require exactly three mocked upload calls. If a submitted future is still queued when the fourth part triggers the guard, discard() cancels it before the mock runs, so correct behavior can produce fewer than three calls. Record the submitted part numbers deterministically. These tests also omit executor-shutdown assertions.

Author verification: confirmed (discard() cancels futures that have not started).

[laughingman7743]

Repaired in b8224b1: the failure tests pass a MagicMock(wraps=S3ThreadPoolExecutor(max_workers=1)) executor and assert the submitted part numbers ([1, 2, 3]) and one shutdown instead of the executed upload count. 30 repeated runs of the max_parts tests: 30/30 passed.

[laughingman7743]

Independent review (relayed), finding 3. P3 — The documented block-size formula is insufficient for arbitrary streamed writes. (reviewer text, condensed) For 5,001 writes of 15 MiB each with block_size=10 MiB, the block size exceeds total/10,000, yet each flush emits separate 10 MiB and 5 MiB parts, so the 5,001st write exceeds the limit. Qualify the guidance for write boundaries and partial blocks. The inaccurate guidance is introduced here; the flush behavior is pre-existing.

Author verification: confirmed by _upload_chunk (a remainder of at least the minimum part size after the last full block is not merged). put_file writes in blocksize chunks and pipe writes once, so the formula holds for them.

[laughingman7743]

Repaired in b8224b1: the docs now say put and pipe upload one part per block (so the formula applies to them), and that a file written with open can take more parts because each write that fills the buffer uploads its data beyond the last full block as a separate part. just docs lint passes.

[laughingman7743]

Independent follow-up review (relayed) — reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, sandbox read-only, session 01a100bb-134a-7271-a141-01de05fb7e73. Base f3ef301d9fec92f1fc53b147bebfef87bfc919ca, previous head 41abb14fde412a6a81dd20d54201db3875cac369, new head b8224b1957948918124f30c5359129c5c6fe783e; given the incremental diff, the intended behavior, and the affected contracts only. Static review only. Result: FINDINGS (1).

Covered (reviewer): incremental diff and base-to-head context; S3File.close(), _upload_chunk(), commit/discard; inherited AioS3File.close() and both executors; fsspec 2026.9.0 close/flush/__del__; put_file/pipe_file; both named tests and the docs. No actionable defects in the shutdown repair or the revised test assertions.

P3 — Missing minimum-size condition for separate remainder parts (introduced by the incremental diff). With the default 5 MiB block size, a 6 MiB write submits one 6 MiB part: _upload_chunk() merges the 1 MiB remainder into the preceding block, so "each write that fills the buffer uploads its remainder separately" gives wrong part counts. Qualify it with the minimum part size.

Author verification: confirmed (0 < next_data_size < MULTIPART_UPLOAD_MIN_PART_SIZE merges).

[laughingman7743]

Repaired in 8b99abb: the sentence now ends "as a separate part when that data is at least 5 MiB". Self-review of the wording (claims: matches _upload_chunk's merge condition with MULTIPART_UPLOAD_MIN_PART_SIZE = 5 MiB; behavior: docs only). just docs lint and just lint pass.