Keep the existing object when appending within a larger block size

[laughingman7743]

WHAT

Appending to an existing S3 object with S3FileSystem.open(path, "ab", block_size=...) now always keeps the existing bytes.

• S3File.append_block is set only when the existing object is at least MULTIPART_UPLOAD_MIN_PART_SIZE (5 MiB), that is, when the object is copied with UploadPartCopy. Smaller objects are read into the write buffer as before and are no longer also copied.
• When append_block is set, _initiate_upload() and _upload_chunk() always use a multipart upload, whatever the block size. The copied object becomes part 1 (≥ 5 MiB), and the appended bytes become the last part, which may be any size.
• The S3File.__init__ docstring now describes this.
• S3File.discard() aborts the multipart upload with Bucket, Key, UploadId, and only the abort-compatible RequestPayer / ExpectedBucketOwner from s3_additional_kwargs. Previously it forwarded s3_additional_kwargs, which in append mode holds the existing object's metadata (StorageClass, ContentType, …). Botocore rejects those for AbortMultipartUpload, so rolling back a transaction that appended to an object ≥ 5 MiB raised ParamValidationError and left the upload behind. That was already the case with the default block size, and the routing change above extends it to larger block sizes.

AioS3File inherits S3File, so AioS3FileSystem gets the same fix.

WHY

Fixes #921.

Before this change, the append paths had three defects. All three were reproduced before the fix (see TEST):

1. S3File append mode drops an existing object of 5 MiB or more when block_size is larger #921: an existing object ≥ 5 MiB, a block_size above 5 MiB, and a final size below block_size. The one-shot PutObject path ran with a buffer that held only the appended bytes, so the existing content was lost without an error.
2. The same setup with nothing written truncated the object to 0 bytes.
3. An existing object < 5 MiB, followed by an append that crossed the block size (reachable with the default 5 MiB block size). The object was in the buffer and copied as part 1. Its bytes were uploaded twice, and CompleteMultipartUpload failed with EntityTooSmall because part 1 was below the minimum part size.

The append logic dates from #539 (v3.8.x), so released versions are affected. It will be backported to the 3.x maintenance branch together with other fixes later.

TEST

Tested commit: 09f34c3 (merge-base 9b74767).

• just lint: passed (ruff, ruff format, mypy, cfn-lint, license headers).
• New offline tests TestS3File::test_append (mocked filesystem, 4-byte minimum part size). They open a real S3File in ab mode, write, and close through fsspec's flush(), then rebuild the stored object from the mocked PutObject / UploadPart / UploadPartCopy calls. With 9b74767's s3.py, 3 of 4 cases fail (b'bb' != b'aaaaaabb', b'' != b'aaaaaa', b'aaaabbbb…' != b'aabbbb…'); with the fix, they pass.
• New offline test TestS3File::test_append_discard: the abort request carries a per-file RequestPayer / ExpectedBucketOwner but no object metadata. It fails on 9b74767/e26b3980 (metadata forwarded) and on 33907e1 (RequestPayer dropped).
• New integration test TestS3FileSystem::test_append_with_block_size against real S3, with two cases: a 6 MiB object + 5 B with block_size=16 MiB, and a 1 KiB object + 5 MiB with the default block size. It checks the size and the bytes at the ends and around the boundary instead of reading the object back. On 9b74767's s3.py: assert 5 == (6291456 + 5) and EntityTooSmall.
• New integration test TestS3FileSystem::test_append_transaction_rollback against real S3: a 6 MiB object, then a 5-byte append inside a rolled-back transaction, with the default and 16 MiB block sizes. It asserts that the ETag, last-modified time, and size are unchanged and that no multipart upload is left. With the routing change but without the discard() change, both cases fail with ParamValidationError. On 9b74767 only the default-block case fails, because the base never starts a multipart upload for the 16 MiB case. The uploads leaked by those runs were aborted afterwards.
• On 09f34c3: uv run --env-file .env pytest -n 1 tests/pyathena/filesystem/test_s3.py tests/pyathena/filesystem/test_s3_async.py -k "append or TestS3File or transaction": 120 passed (the new tests, the existing sync/async test_append, and the write transaction/rollback tests).
• AWS cost: the new integration tests upload about 23 MiB per run (18 MiB of setup objects plus the 5 MiB append), and uploads to S3 are free. Copies are server-side. Reads from S3 to the CI runner are a few bytes plus the 1 KiB object. Objects and incomplete uploads expire after 1 day under the bucket lifecycle rule.
• Not run locally: the full just test pyathena suite; AWS CI runs it when the PR is marked Ready. No async-specific integration case was added, because AioS3File overrides none of the changed methods.

🤖 Generated with Claude Code

[laughingman7743]

Self-review round 1 (implementation behavior): CLEAN

Scope: git diff 9b74767cf65e0ef513338fb2bb2da678cdfbcd2a..e26b3980ee627d23c71fbc6a80b34b5a08be64b7 (pyathena/filesystem/s3.py, tests/pyathena/filesystem/test_s3.py).

Covered:

• Behavior and failure paths. fsspec flush() calls _initiate_upload() once, and the non-final _upload_chunk() runs only with buffer >= blocksize. So append_block changes only the final flush below the block size. Autocommit: CreateMultipartUpload + UploadPartCopy (part 1) + UploadPart (last, any size) + Complete. Transaction (autocommit=False): parts are uploaded and commit() completes the upload; discard() aborts it (it previously did a PutObject of the appended bytes only). An empty append to an object ≥ 5 MiB now rewrites it via the copy part instead of truncating it, which matches the existing default-block-size behavior. Failures in create/copy/part keep the existing paths (fsspec closes the file; _finish_multipart_upload aborts).
• Data boundaries. Existing size == 5 MiB → copy part of exactly the minimum (valid). Existing < 5 MiB → buffered, never copied, so no duplication. 0-byte existing + empty append → touch() path unchanged. Part numbering continues from len(multipart_upload_parts), which includes the copy parts.
• Callers. append_block is read only in S3File (s3.py:2216, :2226, :2269). AioS3File inherits it unchanged. _open(block_size=None) resolves to default_block_size (s3.py:1925).
• Tests. TestS3File::test_append drives the real __init__ → write → close → fsspec flush and rebuilds the object from the mocked S3 calls. It failed on 9b74767 for the three defective cases. The integration test failed on real S3 before the fix (5 == 6291461, EntityTooSmall).

Out of scope (pre-existing, unchanged): the ranged copy for existing objects > 5 GiB (s3.py:2227) can leave a trailing copy part < 5 MiB, and _upload_chunk can emit a non-last part < 5 MiB after a single write larger than two blocks. Both are to be verified separately before filing issues.

[laughingman7743]

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

Scope: git diff 9b74767cf65e0ef513338fb2bb2da678cdfbcd2a..e26b3980ee627d23c71fbc6a80b34b5a08be64b7, plus the PR body and the commit message.

Claims checked:

• "Dates from Support for writing with s3 file system #539 (v3.8.x)": append_block was introduced by 4fbced8 (Support for writing with s3 file system #539); the first containing tag is v3.8.0. ✔
• Docstring (this line): an object ≥ MULTIPART_UPLOAD_MIN_PART_SIZE is copied with UploadPartCopy as the first parts, whatever the block size. This matches s3.py:2200/:2216/:2269. ✔ (It replaces Document every public API in pyathena/ and check docstrings with ruff #919's "once a multipart upload starts", which described the old behavior.)
• "Truncated to zero bytes when nothing was appended" and "duplicated … EntityTooSmall": measured. Offline, the fake gives b'' and b'aaaa…'; on real S3, CompleteMultipartUpload returned EntityTooSmall. ✔
• "Defect 3 reachable with the default 5 MiB block size": the integration case uses block_size=None → default_block_size (s3.py:1925). ✔
• "AioS3FileSystem gets the same fix": AioS3File (s3_async.py:547) overrides no methods. ✔
• Finding: the PR body said the defects were reproduced on 9b74767, but the reproduction ran on 775874c, before the rebase onto Document every public API in pyathena/ and check docstrings with ruff #919. Repaired: I reran the offline reproduction with 9b74767's s3.py (3 of 4 cases fail as described) and the append tests on e26b398 (114 passed). The body now states which commit each result comes from.

Caller and operator view:

• append_block is an unprefixed attribute, but no caller in the repository reads it. It now means "copied server-side", the same meaning s3fs gives it.
• AWS requests: the affected case (an existing object ≥ 5 MiB and a block size above 5 MiB) now issues Create + UploadPartCopy + UploadPart + Complete instead of one PutObject. The copy is server-side, there is no download, and this is the request pattern the default block size already used. botocore/application retries are unchanged.
• Concurrency (pre-existing, not widened in kind): UploadPartCopy copies the latest version without x-amz-copy-source-if-match, so a concurrent overwrite between info() and the copy is not detected. Recorded, not changed here.
• Docs: no user documentation mentions append mode (docs/ searched).

[laughingman7743]

Independent review (relayed): FINDINGS

Reviewer: OpenAI Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, --sandbox read-only, session 01a0ffc3-775d-7563-8d4e-8633fdd8ef6e. Static review only; no tests, builds, or network. The snapshot was a detached checkout of e26b398. The scope was git diff 9b74767cf65e0ef513338fb2bb2da678cdfbcd2a..e26b3980ee627d23c71fbc6a80b34b5a08be64b7. The prompt left out the PR, the commit messages, and the self-review records. The snapshot and the PR worktree were unchanged afterwards.

Covered: existing object absent / 0 / < 5 MiB / == 5 MiB / > 5 MiB / > 5 GiB; empty, short, threshold-crossing, and successive appends; block-size boundaries; part ordering; autocommit, deferred commit, discard, metadata forwarding; AioS3File; whether the tests fail on the base; the accuracy of the changed comments and docstring.

Regression (P2): "Existing object 6 MiB, append 1 byte, block size 16 MiB, autocommit=False, then close and discard → the new condition creates a multipart upload. discard() (s3.py:2375 at e26b398) forwards object metadata, including the automatically populated StorageClass, to AbortMultipartUpload. Botocore rejects these unsupported parameters, leaving the upload un-aborted … Previously this combination created no multipart upload, so discard succeeded."

Pre-existing, not made worse (P2 each):

1. A ranged copy of an existing object of 5 GiB + 1 B leaves an intermediate 1-byte copy part (s3.py:2230).
2. With max_workers=1, _get_ranges() returns a single 6 GiB copy range above the 5 GiB part limit (s3.py:2473).
3. After a single write of 10 MiB + 1 B with a 5 MiB block, _upload_chunk uploads parts of 5 MiB, 5 MiB, and 1 B, and a later part makes the 1-byte part invalid (s3.py:2280).
4. A block size above 5 GiB is accepted and produces a part above the 5 GiB limit (s3.py:2280).

Test notes: "both added integration cases and three of the four unit cases would fail before the change … The unit reconstruction does not validate S3 part limits or completion contents. Append transactions and discard remain uncovered."

[laughingman7743]

Repair: 33907e1 (git range-diff: e26b398 unchanged + 1 new commit on the same merge-base 9b74767)

Regression: verified and fixed. Botocore rejects the extra parameters (Unknown parameter in input: "StorageClass" … must be one of: Bucket, Key, UploadId, RequestPayer, ExpectedBucketOwner, IfMatchInitiatedTime). S3File.discard() now aborts with only Bucket/Key/UploadId, like _finish_multipart_upload() (s3.py:1339) and clear_multipart_uploads() (s3.py:1835). RequestPayer is still merged by _call() (s3.py:2078). This failure was pre-existing with the default block size (an existing object ≥ 5 MiB always took the multipart path) and was widened by this PR, so the fix is folded in here.

New tests:

• TestS3File::test_append_discard checks the exact abort request offline.
• TestS3FileSystem::test_append_transaction_rollback runs on real S3 with a 6 MiB object and block sizes None / 16 MiB. It asserts the object is unchanged and that list_multipart_uploads(path) is empty. Without the repair, both integration cases fail with ParamValidationError; I aborted the 2 uploads that run leaked. With it, 120 passed (append/transaction/TestS3File, sync+async).

Self-review of the repair:

• Behavior: discard() is reached from Transaction.complete(commit=False) and from commit() when tell() == 0; no multipart upload exists in the second case. In "wb" mode, a user s3_additional_kwargs such as ServerSideEncryption would have made the abort fail the same way, and it now succeeds. ExpectedBucketOwner in s3_additional_kwargs is no longer sent on abort, matching the other abort paths. An abort is still scoped by the upload ID.
• Claims: the commit message, the PR body (WHAT/TEST updated), and the code comment state only the measured behavior above.

Pre-existing items 1–4: deferred. None of them is made worse by this PR, and each needs its own reproduction; items 1, 2, and 4 involve objects or parts ≥ 5 GiB. I will verify them and show them to the maintainer before filing issues.

[laughingman7743]

Independent follow-up review (relayed): FINDINGS

Reviewer: OpenAI Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, --sandbox read-only, session 01a0ffce-37fc-7161-a1b8-a88bde11c58f. Static only. Scope: git diff e26b3980ee627d23c71fbc6a80b34b5a08be64b7..33907e1a90674d5227578eb0425bd705e0660b71 on a detached checkout of 33907e1; the checkout was unchanged afterwards.

Covered: append metadata, multipart init, close, and discard for 5 MiB and 16 MiB blocks; all three abort call sites and _call; both new tests (both fail without the repair).

1. Introduced regression (P2), s3.py:2372: "A non-owner using \"wb\", autocommit=False, and s3_additional_kwargs={\"RequestPayer\": \"requester\"} without filesystem-level requester_pays=True can create a multipart upload … Previously, discard() also forwarded the payer acknowledgement; now its abort omits it, receives an authorization error, and leaves the upload behind."
2. Pre-existing (P2), s3.py:2359: if a part or completion fails and the helper's abort also fails, commit() clears multipart_upload, so a later discard() cannot abort the upload.

Repair: 2f0a1c3.

• (1) Verified and fixed. discard() now forwards only the parameters that AbortMultipartUpload accepts from s3_additional_kwargs (RequestPayer, ExpectedBucketOwner) and still excludes object metadata. test_append_discard now passes both parameters plus the append metadata and asserts the exact request. It fails on e26b398 (metadata forwarded) and on 33907e1 (RequestPayer dropped), and passes on 2f0a1c3. just lint passed, and the append/transaction/TestS3File tests passed (120, sync+async, real S3).
• (2) Deferred as pre-existing: this PR doesn't change commit()'s failure path. It will be verified separately with the other out-of-scope items.

Self-review of the repair. Behavior: this restores the pre-PR forwarding of RequestPayer/ExpectedBucketOwner, and both are valid AbortMultipartUpload members per botocore. If RequestPayer is set both per file and via requester_pays=True, _call receives a duplicate keyword; that is pre-existing and also affects every other request that forwards s3_additional_kwargs. Claims: the PR body (WHAT, TEST) and the code comment were updated to match.

[laughingman7743]

Independent follow-up review (relayed): CLEAN

Reviewer: OpenAI Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, --sandbox read-only, session 01a0ffd7-dfdf-7410-bfcd-4d0e9cb40be4. Static only. Scope: git diff 33907e1a90674d5227578eb0425bd705e0660b71..2f0a1c3ac26bb6a9bddefabc0dbf63c541f60cd4 on a detached checkout of 2f0a1c3, with the full PR diff for reference; the checkout was unchanged afterwards.

Covered: discard() filtering and its comment against the AbortMultipartUpload input shape; append metadata, sync/async file creation, multipart init, close/rollback, and commit-failure cleanup; _call() requester-pays handling; the updated unit test against both earlier implementations.

Result: "no actionable regression … The changed call supplies Bucket, Key, and UploadId, preserves RequestPayer and ExpectedBucketOwner, and excludes rejected object parameters … The code comment is accurate." The test fails on 33907e1 and on the original discard().

Pre-existing, unchanged by this PR (deferred, to be verified separately):

• s3.py:2078: filesystem requester_pays=True plus a per-file RequestPayer gives duplicate keywords → TypeError, already at multipart initiation.
• s3.py:1339: _finish_multipart_upload()'s cleanup abort omits a per-file RequestPayer.

[laughingman7743]

Self-review of 09f34c3 (test transfer reduction), rounds 1 + 2: CLEAN

Scope: git diff 2f0a1c3ac26bb6a9bddefabc0dbf63c541f60cd4..09f34c39c6306a4fcd919499f4b9ed701d62776a (tests only). Reason: the maintainer asked to keep S3 cost down. The full read-back moved about 25 MiB per run from S3 to the GitHub runner, outside AWS, which is billed as egress.

Behavior and test quality:

• test_append_with_block_size asserts the HEAD size plus [0,1) == b"a", [size-1, size+1) == b"ab", and the last byte == b"b". Truncation (S3File append mode drops an existing object of 5 MiB or more when block_size is larger #921, size 5) and duplication (size mismatch) are caught by the size check. Wrong order is caught by the boundary check. The case with the existing object read into the buffer uses 1 KiB (< MULTIPART_UPLOAD_MIN_PART_SIZE) + 5 MiB, which still crosses the default 5 MiB block.
• test_append_transaction_rollback compares ETag, last-modified time, and size before and after. A committed append changes the size, and a multipart-completed object gets a -N ETag.
• Measured on 9b74767's s3.py: 3 of 4 cases fail (assert 5 == (6291456 + 5), EntityTooSmall, ParamValidationError). The 16 MiB rollback case passes there because the base never starts a multipart upload. On 09f34c3, 120 passed (append/transaction/TestS3File, sync+async). I aborted the 1 upload that the base run leaked.

Claims and operations: the PR body's TEST section now states the per-run transfer: about 23 MiB uploaded (free), a few bytes + 1 KiB read, server-side copies, and the 1-day lifecycle expiry of objects and incomplete uploads (cloudformation, AbortIncompleteMultipartUpload: 1). The comments say only what the assertions check.

[laughingman7743]

Independent follow-up review (relayed): CLEAN

Reviewer: OpenAI Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, --sandbox read-only, session 01a0fff1-f877-7a60-8083-9f4372282ca1. Static only. Scope: git diff 2f0a1c3ac26bb6a9bddefabc0dbf63c541f60cd4..09f34c39c6306a4fcd919499f4b9ed701d62776a on a detached checkout of 09f34c3; the checkout was unchanged afterwards.

Covered: both changed tests and the append/commit/discard paths; detection of the original defects (replacement, duplication, rollback ParamValidationError and leftover uploads); ordering at the boundary and the ends; info(refresh=True) bypassing the cache, and the ETag/last_modified/size mappings; cat_file exclusive end and start=-1; multipart listing under the unique prefix; the 1 KiB case still taking the buffered path; the accuracy of the comments.

Result: "All three named regressions remain detectable." Stated limit: "The append samples do not prove full interior equality: same-size corruption confined to unsampled bytes could escape."

Author note on the limit: accepted as the cost trade-off. Full-content equality is still asserted offline by TestS3File::test_append, which rebuilds the stored object from every PutObject / UploadPart / UploadPartCopy call.