Skip to content

Keep multipart uploads within the 10,000-part limit - #968

Merged
laughingman7743 merged 8 commits into
masterfrom
fix/953-multipart-part-limit
Oct 3, 2026
Merged

laughingman7743 merged 8 commits into
masterfrom
fix/953-multipart-part-limit

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

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. The file's executor is shut down in the error path too, because fsspec does not close an already-closed file again when it is garbage collected. 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().
  • Known sizes (put_file(), pipe_file()): before uploading anything, a new S3FileSystem._check_multipart_upload_size() raises ValueError when the data needs more than 10,000 blocks at the block size in use (size > block_size * 10,000, the documented rule). The message names the minimum block size, max(ceil(size / 10,000), 5 MiB). pipe_file() measures the data in bytes (memoryview(value).nbytes) for this check; its PutObject dispatch keeps len(value) as before. The check runs before exists() for mode="create", before any API call, and before the transaction branch. AioS3FileSystem._put_file/_pipe_file delegate to these methods.
  • put_file() block_size: put_file() accepts block_size and passes it to open(); before, it went to the S3 API as a request parameter and failed.
  • 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, and to check up front where the size is known (put, pipe). A streamed open() write cannot be checked in advance because its total size is unknown (fsspec passes no size for writes). The design of the up-front check was reviewed with claude-fable-5-1.

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).
  • put()/put_file() and pipe()/pipe_file() raise ValueError before uploading anything when the data needs more than 10,000 blocks, naming the minimum block size. pipe_file() also rejects data up to 5 MiB over 10,000 blocks, which a multipart upload could have fit by merging its short last block.
  • put()/put_file() accept block_size.
  • S3File.close() shuts its executor down even when the final flush fails.
  • A multipart copy with a small block_size now uses larger parts instead of failing at 10,000 parts.

Pre-existing and out of scope: put_file() forwards max_workers to the S3 API; pipe_file() with a non-contiguous memoryview larger than a block in items fails in the buffered path, where an exception inside fsspec's pipe_file context manager commits an empty object.

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: a5f0ec7 (rebased onto 6f25850, the merge of #992).

  • 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.
    • TestS3FileSystem::test_check_multipart_upload_size, test_put_file_exceeding_max_parts, test_put_file_block_size, and test_pipe_file_exceeding_max_parts cover the boundary, the minimum block size in the message, no open()/API call on rejection (with an explicit block_size and with default_block_size), and block_size reaching open(). All 8 cases fail without the source change of 6901304; a memoryview case and test_write_exceeding_max_parts_without_close fail without da55411, and test_pipe_file_non_contiguous_memoryview guards the unchanged PutObject dispatch.
    • 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: 219 passed. The 110 failures are the same set as on the base commit (190 passed there): 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

Comment thread pyathena/filesystem/s3.py Outdated
# Abort the upload and close the file without the
# buffered data, so that neither close() nor commit()
# uploads it.
self.discard()

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread pyathena/filesystem/s3.py
The ``(start, end)`` byte ranges, with an exclusive end, that
cover the whole object in order.
"""
block_size = max(block_size, math.ceil(size / self.MULTIPART_UPLOAD_MAX_PARTS))

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread pyathena/filesystem/s3.py Outdated
f"Cannot upload more than {self.fs.MULTIPART_UPLOAD_MAX_PARTS} "
f"parts to s3://{self.bucket}/{self.key} with a block size of "
f"{self.blocksize} bytes. Write the file with a block_size, or "
"a default_block_size of the filesystem, of at least its total "

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 laughingman7743 Oct 3, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread docs/filesystem.md Outdated
[fsspec transaction](https://filesystem-spec.readthedocs.io/en/latest/features.html#transactions),
writes are deferred until the transaction commits and are discarded on rollback.

A multipart upload consists of at most 10,000 parts, one per block, so a write with

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment thread pyathena/filesystem/s3.py
# upload. An abort failure does not mask this error, and
# commit() does not complete the upload afterwards.
self.buffer = None
self.closed = True

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread tests/pyathena/filesystem/test_s3.py Outdated
f.commit()

assert f.closed
assert fs._upload_part_copy.call_count + fs._upload_part.call_count == 3

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread docs/filesystem.md Outdated

A multipart upload consists of at most 10,000 parts, one per block, so a write with
the default block size can upload up to about 48.8 GiB (10,000 × 5 MiB). To write a
larger object, use a block size of at least its size divided by 10,000, either with

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread docs/filesystem.md Outdated
(10,000 × 5 MiB). To upload a larger object, use a block size of at least its size
divided by 10,000, either with the `block_size` argument of `pipe` and `open` or with
the `default_block_size` argument of `S3FileSystem`. A file written with `open` can
take more parts, because each write that fills the buffer uploads the data beyond its

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent follow-up review (relayed) of the repair — reviewer: Codex CLI 0.160.0, model gpt-6-astra, sandbox read-only, session 01a100bf-f35e-7521-ab60-c3e46b71311c. Previous head b8224b1957948918124f30c5359129c5c6fe783e, new head 8b99abbb7612b1594263d626b75f7b79a51d5cc7, base f3ef301d9fec92f1fc53b147bebfef87bfc919ca. Static review only. Covered: the incremental diff and the full paragraph; _upload_chunk, the multipart constants, put_file/pipe_file, append accounting, the overflow abort, and copy sizing; fsspec 2026.9.0 write/flush, buffer reset, close, and pipe delegation. Result: CLEAN — the "at least 5 MiB" condition matches the implementation (e.g., with a 10 MiB block size, a buffered 12 MiB write produces one merged part; 15 MiB produces separate 10 MiB and 5 MiB parts).

Comment thread pyathena/filesystem/s3.py
starts.append(starts[-1] + (size - starts[-1]) // 2)
return list(zip(starts, [*starts[1:], size], strict=True))

def _check_multipart_upload_size(self, path: str, size: int, block_size: int) -> None:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review rounds one and two of the pre-check extension — base f3ef301d9fec92f1fc53b147bebfef87bfc919ca, range 8b99abbb7612b1594263d626b75f7b79a51d5cc7..6901304bea3cb5112903407b4b7d5d78eb50ed38 (a contract expansion requested by the maintainer after consulting claude-fable-5-1). Result: CLEAN.

Round one (behavior): the check runs before parse_path-dependent work, exists() (mode="create"), any API call, and the transaction branch of pipe_file, so nothing is opened or added to transaction.files; sizes up to one block never reach it; an empty file passes; AioS3FileSystem._put_file/_pipe_file delegate to the sync methods; put_file passes block_size to open(), where S3File.__init__ still rejects a write block size below 5 MiB. New tests fail without the source change (8 cases) and pass with it; offline tests/pyathena/filesystem/: 171 passed, the same 108 credential-less integration failures as the base.

Round two (claims, callers): size > block_size * MULTIPART_UPLOAD_MAX_PARTS is exact for put_file (it writes remote.blocksize chunks, so parts = ceil(size / block)) and is the documented rule for pipe_file, which therefore rejects the sizes in (10,000·B, 10,000·B + 5 MiB) that the tail merge would have fit (release-note). The minimum block size in the message is max(ceil(size / 10,000), 5 MiB), so it never suggests a size open() rejects. put(…, block_size=…) previously reached the S3 API as a parameter and failed, so accepting it breaks no caller. Pre-existing and out of scope: put_file still forwards max_workers to the S3 API.

Comment thread pyathena/filesystem/s3.py
# neither close() nor commit() uploads it, and abort the
# upload. An abort failure does not mask this error, and
# commit() does not complete the upload afterwards.
self.buffer = None

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review of the expanded scope (relayed) — reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, sandbox read-only, session 01a100db-fc47-7d52-93c8-beff25916d64. Base f3ef301d9fec92f1fc53b147bebfef87bfc919ca, head 6901304bea3cb5112903407b4b7d5d78eb50ed38 (full diff, detached snapshot; intended behavior and conventions only). Static review only. Result: FINDINGS (2). Covered (reviewer): the supplied diff, sync/async upload paths, fsspec 2026.9.0 lifecycle and transactions, append accounting, merged/split tails, copy-range bounds through 5 TiB, pre-check placement, typing, documentation, and tests.

1. P2 — Mid-stream overflow leaves the executor running (introduced). Outside a context manager, writing past the limit sets closed=True, drops the buffer, and aborts, but never shuts down the executor; commit()/discard() do not, and fsspec's __del__() skips closed files. The tests masked it because their helper always exits a context manager.

Author verification: confirmed (fsspec __del__: if not self.closed: self.close()).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repaired in da55411: the error path calls self._executor.shutdown() after the abort (idempotent with close(); S3AioExecutor.shutdown() is a no-op). New test_write_exceeding_max_parts_without_close (no context manager) fails without the repair and passes with it; the existing failure tests now assert shutdown was called rather than called once. Self-review of the repair (behavior: waits for in-flight parts like close(); claims: fsspec __del__ skips closed files) found nothing further.

Comment thread pyathena/filesystem/s3.py Outdated
``MULTIPART_UPLOAD_MAX_PARTS`` blocks.
"""
block_size = kwargs.get("block_size") or self.default_block_size
self._check_multipart_upload_size(path, len(value), block_size)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review of the expanded scope (relayed), finding 2. P2 — The new pre-check undercounts memoryviews (gap in the added check; the length-based dispatch predates the diff). len(memoryview) counts items: with B = 5 MiB, a four-byte-item view of 10,000·B + 4 bytes passes the check, and its tail merges into part 10,000, so nothing rejects it. Use memoryview.nbytes.

Author verification: confirmed (len(memoryview(array('I', [0] * 4))) is 4, nbytes is 16).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repaired in da55411: pipe_file() computes size = memoryview(value).nbytes once and uses it for both the part limit check and the PutObject dispatch (the dispatch had the same pre-existing undercount). New case memoryview(b"a" * 16).cast("I") in test_pipe_file_exceeding_max_parts fails without the repair and passes with it. Offline tests/pyathena/filesystem/: 173 passed, the same 108 credential-less integration failures as the base; just lint passes.

Comment thread pyathena/filesystem/s3.py Outdated
# The size in bytes; the length of a memoryview counts its items.
size = memoryview(value).nbytes
self._check_multipart_upload_size(path, size, block_size)
if self._intrans or size > min(block_size, self.MULTIPART_UPLOAD_MAX_PART_SIZE):

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent follow-up review (relayed) — reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, sandbox read-only, session 01a100e4-4bf8-7bb1-97b6-fb513b7f26a5. Base f3ef301d9fec92f1fc53b147bebfef87bfc919ca, previous head 6901304bea3cb5112903407b4b7d5d78eb50ed38, new head da55411fa141d3618e7721e3ceed7e580e6c7a67. Static review only. Covered: _upload_chunk/close/commit/discard incl. abort failures and deferred commits; both executors' shutdown idempotency and waiting; fsspec 2026.9.0 write/flush/close/__del__; sync/async pipe_file with bytes/bytearray/memoryview and both dispatch paths; the four named tests. No defects in the executor-shutdown repair. Result: FINDINGS (1).

P2 — Strided multi-byte memoryviews now fail and can overwrite the destination with an empty object (introduced by the incremental diff). With the default block size, memoryview(b"a" * (12 * 2**20)).cast("I")[::2] holds 6 MiB in 1,572,864 items; it used to reach bytes(value) (PutObject), but the byte-based dispatch sends it to the buffered path, whose BytesIO.write raises BufferError; the context manager then closes an empty buffer and commit() touches the object.

Author verification: confirmed (io.BytesIO().write(memoryview(b"a" * 24).cast("I")[::2]) raises BufferError).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repaired in cf3cddf: the PutObject dispatch uses len(value) again, exactly as on the base, and only the part limit check uses memoryview(value).nbytes. New test_pipe_file_non_contiguous_memoryview (4 items, 8 bytes, block_size=6) asserts PutObject with the converted bytes; on da55411 it takes the buffered path instead. Self-review of the repair: the dispatch is identical to the base, so no new behavior. Pre-existing and out of scope (recorded in the PR body): a non-contiguous view larger than a block in items still fails in the buffered path, and an exception inside fsspec's pipe_file context manager commits an empty object. Offline tests/pyathena/filesystem/: 174 passed, the same 108 credential-less integration failures; just lint passes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent follow-up review (relayed) of the repair — reviewer: Codex CLI 0.160.0, model gpt-6-astra, sandbox read-only, session 01a100ea-cdb7-7562-8119-c8e1bb323dd4. Base f3ef301d9fec92f1fc53b147bebfef87bfc919ca, previous head da55411fa141d3618e7721e3ceed7e580e6c7a67, new head cf3cddf712c746ef72f8a9daec94cb851ee367c2. Static review only. Covered: the pipe_file byte-based pre-check and both upload paths for bytes, bytearray, and contiguous/non-contiguous memoryviews; _upload_chunk/close/commit/discard incl. append accounting, abort failure, deferred commit, and cleanup without an explicit close; both executors' shutdown; fsspec 2026.9.0 write/flush/close/__del__; AioS3FileSystem._pipe_file; the named tests and the new regression test. Result: CLEAN — the pre-check stays byte-based while the dispatch restores the base's exact len(value).

@laughingman7743
laughingman7743 marked this pull request as ready for review October 3, 2026 08:45
@laughingman7743
laughingman7743 marked this pull request as draft October 3, 2026 08:49
laughingman7743 and others added 8 commits October 3, 2026 17:50
S3 multipart uploads accept at most 10,000 parts. S3File numbered write
parts without an upper bound, so a write of more than 10,000 blocks
(about 48.8 GiB with the default 5 MiB block size) submitted part 10,001
and failed only when S3 rejected it. Multipart copies with a small
block_size had the same gap.

Writes now raise ValueError before submitting a part beyond the limit,
naming the block size to use. The multipart upload is aborted and the
file is closed without its buffered data, so that neither close() nor a
deferred commit() uploads a partial object. Parts copied from the
existing object in an append count toward the limit.

Multipart copies know the source size, so _get_copy_ranges raises the
split size to the size divided by 10,000 instead.

Closes #953

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
If AbortMultipartUpload failed, its error replaced the part limit
error, the file stayed open so close() retried the abort, and a deferred
commit() completed the parts uploaded so far. Close the file before the
abort, log an abort failure, clear the upload state, and always raise
the part limit error.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
In an append to an object smaller than the maximum part size, the copy
takes one part, so a block size of the total size divided by 10,000 can
still need 10,001 parts. The error now asks for a block size that fits
the parts, including the copied ones, and the docs note that the copied
parts count toward the limit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
S3File.close() skipped the executor shutdown when the final flush
raised, as the part limit error does, and the file was already marked
closed. Shut it down in a finally block.

The part limit failure tests asserted the number of executed part
uploads, which depends on whether the abort cancels queued uploads.
They now record the submitted parts through a wrapped executor and
check its shutdown.

The docs claimed one part per block for every write, but each write
through open() that fills the buffer uploads its data beyond the last
full block as a separate part. Narrow the sizing guidance to put and
pipe.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A remainder smaller than the minimum part size is merged into the
preceding part, so only a remainder of at least 5 MiB adds a part.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
put_file() and pipe_file() know the size of the data, so they now raise
ValueError before uploading anything when it needs more than 10,000
blocks, naming the minimum block size, instead of failing after about
48.8 GiB has been uploaded at the default block size. The check follows
the documented rule (a block size of at least the size divided by
10,000), so pipe_file() also rejects data up to 5 MiB over 10,000
blocks, whose short last block the multipart upload would merge.

put_file() now accepts block_size and passes it to open(); before, it
went to the S3 API as a request parameter.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A write() that reached the part limit closed the file without shutting
down its executor, and fsspec does not close a closed file again when it
is garbage collected, so the executor was left running unless close()
was called. Shut it down in the error path as well.

pipe_file() measured a memoryview with len(), which counts its items,
so a view with multi-byte items passed the part limit check and the
PutObject dispatch with a fraction of its size. Use its size in bytes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Measuring the dispatch in bytes sent a non-contiguous memoryview with
multi-byte items to the buffered path, whose BytesIO cannot write it,
when its item count fit in a block. Only the part limit check needs the
size in bytes; the dispatch keeps using len() as before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@laughingman7743
laughingman7743 force-pushed the fix/953-multipart-part-limit branch from cf3cddf to a5f0ec7 Compare October 3, 2026 08:52
Comment thread docs/filesystem.md
filesystem's `default_block_size`, must be between 5 MiB and 5 GiB, inclusive, the part
size limits of a multipart upload. Otherwise, `open` raises `ValueError`.

A multipart upload consists of at most 10,000 parts. `put` and `pipe` upload one part

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebase onto master after #992 — self-review of the rebase (both perspectives). Old: base f3ef301d9fec92f1fc53b147bebfef87bfc919ca, head cf3cddf712c746ef72f8a9daec94cb851ee367c2. New: base 6f258501a56124f6e29c6d530ce429b702ed2ab3 (merge of #992), head a5f0ec7ab45619d563865e6616bb333d6640511e. Result: CLEAN.

git range-diff f3ef301d..cf3cddf7 6f258501..a5f0ec7a: commits 2, 3, 5, 7, 8 identical; 1, 4, 6 differ only in context. Conflicts resolved by each commit's own delta: docs/filesystem.md keeps #992's block-size range paragraph followed by this PR's 10,000-part paragraph; the test import combines S3AioExecutor (#992) and S3ThreadPoolExecutor (this PR).

Upstream contracts that affect this PR, checked: #992's discard() now waits for running parts before AbortMultipartUpload, so the part-limit error path (close, drop buffer, discard(), shut down the executor) aborts after in-flight parts settle; this also settles the in-flight-parts deferral recorded in round two. #992's S3File.__init__ limits the write block size to 5 MiB–5 GiB; 10,000 × 5 GiB exceeds the 5 TiB object limit, so the messages asking for a larger block_size stay satisfiable for any storable object.

Validation on the rebased head: just lint, just docs lint pass; offline tests/pyathena/filesystem/: 219 passed, and the 110 failures are the same set as on 6f258501 (190 passed there; AWS integration tests stopped without credentials).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review of the rebase (relayed) — reviewer: Codex CLI 0.160.0, model gpt-6-astra, sandbox read-only, session 01a100f7-b3b6-70d3-b0c4-d61bb06fb921. Old f3ef301d..cf3cddf7, new 6f258501a56124f6e29c6d530ce429b702ed2ab3..a5f0ec7ab45619d563865e6616bb333d6640511e; given the range-diff, the full rebased diff, and the upstream history. Static review only. Covered: the conflict resolutions (both docs paragraphs; test imports keep wait, chain, pairwise, and both executors); the lifecycle through __init__ version-ID handling, _upload_chunk, discard (waits for running uploads before the abort), close, and deferred commit; put_file block-size forwarding, the byte-based pipe_file check with the preserved PutObject dispatch, copy-range sizing, messages and docstrings; the tests against the new initializer validation and wait(). Result: CLEAN — no actionable defects or rebase regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

S3File writes fail after 10,000 parts

1 participant