Limit multipart uploads and object versions to the key and keys under it - #989
Conversation
list_multipart_uploads(), clear_multipart_uploads() and object_version_info() sent the path key to S3 as a plain string Prefix, so they also listed, aborted or returned sibling keys that merely start with the same characters (data2/..., data.csv, a.csv.bak). The multipart methods now keep only the uploads to the key itself and to the keys under key/. object_version_info() returns the versions of the key itself when it has any, and otherwise those under key/; a path with a trailing slash always selects the keys under it. Closes #981 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| S3MultipartUpload({**u, "Bucket": bucket}) for u in response.get("Uploads", []) | ||
| S3MultipartUpload({**u, "Bucket": bucket}) | ||
| for u in response.get("Uploads", []) | ||
| if u["Key"] == key or u["Key"].startswith(prefix) |
There was a problem hiding this comment.
Self-review round one (implementation behavior) — base aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe, head 3872f3151c34c1032739f9f3aa89590d43dad052. Result: CLEAN.
Covered: pyathena/filesystem/s3.py (list_multipart_uploads, clear_multipart_uploads, object_version_info), pyathena/filesystem/s3_async.py delegates, docs/filesystem.md, tests/pyathena/filesystem/test_s3.py.
- Multipart filter (this line):
prefixis""for a bucket path, sostartswith("")keeps every upload, andu["Key"] == Noneis never true; the redundantnot keyguard was dropped before the push. Pagination still followsIsTruncated/markers of the raw response, so a page whose uploads are all filtered out does not stop the loop.ListMultipartUploadsalways returnsKeyper upload. - Trailing slash (
data/):Prefix="data/"is sent unchanged and every returned key starts withdata/, so the filter keeps all of them;dataitself is not included. clear_multipart_uploads()aborts exactly the filtered list, so the sibling uploads are no longer touched. The aio methods call the sync ones (s3_async.py:484-520), so no separate async path exists.- Request count is unchanged: one paginated listing with the same
Prefix; existing pagination tests still assert the same request arguments.
| object_versions = [v for v in versions if v.key == key] | ||
| if object_versions: | ||
| return object_versions | ||
| return [v for v in versions if v.key.startswith(prefix)] |
There was a problem hiding this comment.
Self-review round one (implementation behavior), object_version_info() — base aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe, head 3872f3151c34c1032739f9f3aa89590d43dad052.
- Key without trailing slash: versions of the key itself when any entry for it was collected, otherwise the entries under
key/; with a trailing slash the exact-key step is skipped, so adir/folder marker cannot hide the folder's contents. Order of entries is preserved by both filters. - Considered, not changed: with
delete_markers=True, a key that has only delete markers (all versions expired) and also keys underkey/returns the markers, whiledelete_markers=Falsefalls back tokey/. This needs a key that is both an object and a prefix and has no live versions; deciding on markers regardless of the flag would add state for a rare corner, so it is left as documented ("if it has any"). - Tests: the offline fake applies
Prefixas a plain string prefix like S3, covering key, prefix with/without slash, key with slash and bucket, with and without delete markers; 3 cases fail with the fix reverted. The live tests add a real sibling (<path>.bak,<prefix>2/file) and fail on the old code, so the fake is not the only evidence.
There was a problem hiding this comment.
Repair self-review, round-one perspective (3872f315..6a92aa09, base unchanged aa0fc914): the decode is applied only when the caller passed EncodingType="url", so the default path (botocore auto-decodes) is byte-for-byte the same filter as before. zip(..., strict=True) pairs each version with its own key, order is preserved, and the returned S3ObjectVersion.key stays as listed (encoded), matching the old return shape for that option. New offline test fails without the repair; offline subset 12 passed, live -k "object_version_info or read_version_id or multipart_uploads" 15 passed on 6a92aa09. just lint passed. CLEAN.
There was a problem hiding this comment.
Repair self-review, round-one perspective (6a92aa09..3867c355, base unchanged aa0fc914) — maintainer decision: unify the key choice across delete_markers. This supersedes the "considered, not changed" note above.
- Delete markers are now always collected; the key is chosen when it appears among versions or markers (
key in keys), anddelete_markers=Falsedrops markers only after the choice. Corner case:dirwith only a delete marker plusdir/x→True:[dir marker],False:[](previouslydir/x). - Unchanged elsewhere: per-page order (versions, then markers), bucket path (everything, minus markers when
False), trailing-slash paths, URL-encoded matching, request arguments and count (markers come in the same ListObjectVersions response).key in keysis guarded bykey and, so bucket paths never compare againstNone. - Test:
test_object_version_info_chooses_key_with_only_delete_markerscovers both values; theFalsecase fails on6a92aa09. Offline subset 14 passed, live targeted 17 passed,just lintpassed. CLEAN.
| if it has any, and otherwise the versions of the keys under ``key/``. | ||
| A key path with a trailing slash returns the versions of the keys | ||
| under it, and a bucket path returns the versions of all the keys in | ||
| the bucket. Sibling keys that merely start with the same characters |
There was a problem hiding this comment.
Self-review round two (claims and callers) — base aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe, head 3872f3151c34c1032739f9f3aa89590d43dad052. Result: FINDINGS (1, fixed in the PR body).
Claims checked:
- "S3 matches
Prefixas a plain string" (docstring comments, PR WHY): the live tests with real siblings (<prefix>2/file,<path>.bak) fail on the old code, so S3 returned the siblings; not only the fake. - "An aborted multipart upload cannot be resumed" (PR WHY): AbortMultipartUpload invalidates the upload ID, after which no parts can be uploaded with it.
- "The request pattern is unchanged" (PR WHAT): same
Prefix/markers per request; the pagination tests assert identical arguments. - Docstring at this line and the aio summaries: bucket path returns everything (
prefix == ""), trailing slash skips the exact-key step; matches the code. - Release-note example (
clear_multipart_uploads("s3://bucket/tmp")used to cleartmp1/,tmp2/): true for the old raw-prefix behavior.
Finding: the PR body's TEST section cited a targeted live run made before the redundant not key guard was dropped, i.e. not on the published head. Repaired: reran pytest -n 4 tests/pyathena/filesystem/ on 3872f31 (283 passed) and rewrote the TEST section to separate pre-push from head evidence.
Callers: inside pyathena/, only the aio delegates (s3_async.py:499,512,522) and clear_multipart_uploads() call these methods; return types and ordering are unchanged. Existing user code that relied on the raw string-prefix match changes behavior; this is called out as a release-note item.
AWS: no added requests at runtime. The live tests add one CreateMultipartUpload (aborted in finally) and one small PutObject.
There was a problem hiding this comment.
Repair self-review, round-two perspective (3872f315..6a92aa09): claim in the new comment ("botocore decodes the keys only when it sets EncodingType itself") checked against botocore/handlers.py _decode_list_object (EncodingType == 'url' and encoding_type_auto_set) in the locked botocore. unquote_plus matches botocore's own unquote_str and live S3 (a space is listed as +). Pagination with an explicit EncodingType="url" still sends the encoded NextKeyMarker back, which is pre-existing and outside this PR. The PR body's behavior description is unaffected; its TEST section is updated with the repair evidence. CLEAN.
There was a problem hiding this comment.
Repair self-review, round-two perspective (6a92aa09..3867c355): claims rechecked. The docstring now states the single criterion ("selects that key if it has any versions or delete markers ... does not depend on delete_markers, so a key that has only delete markers yields no versions without them"), which matches the code. The PR body WHAT section is rewritten with the rule and the corner-case outcome, and "one listing" is still true because ListObjectVersions returns DeleteMarkers in the same response as Versions. docs/filesystem.md makes no statement about this choice, so it needs no change. Existing callers: only the result for that corner changes (from the folder's versions to []/markers); every other path returns the same list as before. CLEAN.
| print(upload.key, upload.upload_id, upload.initiated) | ||
|
|
||
| # Abort all incomplete uploads under a bucket or key prefix. | ||
| # Abort all incomplete uploads to a key and the keys under it. |
There was a problem hiding this comment.
Self-review round two (documentation reader): the example on the next line uses a trailing slash, so it aborts the uploads under path/to/, as the new comment says. The other examples in this file (list_multipart_uploads on a bucket at line 127, object_version_info on an object path at line 155, ls(..., versions=True)) are unaffected by this change, and no other doc or README references these methods.
With an explicit EncodingType="url", botocore leaves the listed keys URL-encoded, so the key filter compared "a+b" with "a b" and dropped the versions of the requested key. Decode the keys for matching while returning them as listed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| return versions | ||
| # botocore decodes the keys only when it sets EncodingType itself, so | ||
| # the keys of an explicit EncodingType="url" are decoded for matching. | ||
| url_encoded = kwargs.get("EncodingType") == "url" |
There was a problem hiding this comment.
Independent review (relayed) — reviewer: Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max (different model from the Claude author), session 01a100cc-e27a-78a1-b1ba-1ad4e70f68df. Static review only: codex exec --sandbox read-only on a detached snapshot at 3872f3151c34c1032739f9f3aa89590d43dad052 (no .env), base aa0fc9146f4e683d6cdf96b894f963f9dd8f7abe; prompt had the literal diff and intended behavior, no PR text or prior findings. Snapshot and PR worktree verified unchanged afterwards. Result: FINDINGS.
Covered: exact diff, path parsing, trailing slashes, bucket paths, pagination, delete markers, empty results, result models, async delegation, docs, test fakes, live-resource cleanup.
- P2 — explicit URL encoding drops valid versions (
pyathena/filesystem/s3.py:1941at the reviewed head):object_version_info("s3://bucket/a b", EncodingType="url")getsKey="a+b"; botocore decodes only when it setEncodingTypeitself, so both comparisons reject the key and return[](old code returned it).
Verified and fixed in6a92aa09(this line): keys are decoded withunquote_plusfor matching only, returned as listed. Verified botocore's_decode_list_objectcondition (encoding_type_auto_set) and live S3: the raw key fora bisa+b, and the fixed call now returns onlya b's version, nota b.bak. New offline testtest_object_version_info_matches_url_encoded_keysfails without the fix.
Non-actionable observations from the reviewer: the fakes apply S3's plain-prefix matching and distinguish old/new behavior; marker-only precedence differs between delete_markers=False/True (same corner recorded in self-review round one, left as documented); the multipart live test's pre-existing cleanup only covers the sibling in finally (the bucket's AbortIncompleteMultipartUpload lifecycle rule, 1 day, covers a leaked upload).
There was a problem hiding this comment.
Independent follow-up (relayed) — Codex CLI 0.160.0, model gpt-6-astra, session 01a100d5-6dac-70b2-af54-30841cf57a86; static, read-only snapshot at 6a92aa090dc1455185c7a19334683b0f809fac3e, bounded to the repair 3872f3151c34c1032739f9f3aa89590d43dad052..6a92aa090dc1455185c7a19334683b0f809fac3e (single commit, base unchanged). Result: CLEAN — prior finding resolved. Covered: decoding only for explicit EncodingType="url" with defaults unchanged; botocore 1.43.102 _decode_list_object decodes only auto-set encoding via unquote_str = unquote_plus, so the repair matches it; returned objects, encoded keys and order preserved, including delete markers; the new test receives [] before the repair; the aio delegate forwards kwargs unchanged. Snapshot verified clean afterwards.
There was a problem hiding this comment.
Independent follow-up (relayed), unified delete-marker rule — Codex CLI 0.160.0, model gpt-6-astra, session 01a1010c-4834-7a60-9845-28dfef5d761f; static, read-only snapshot at 3867c355146304ef5a35b50db367cd028eca2096, bounded to 6a92aa090dc1455185c7a19334683b0f809fac3e..3867c355146304ef5a35b50db367cd028eca2096 (one commit, base unchanged), prompt with the requirement and diff only. Result: CLEAN — inconsistency resolved. Covered: selection independent of delete_markers and docstring accuracy; exact-key/prefix, bucket, trailing-slash, sibling exclusion and EncodingType="url" behavior unchanged; one paginated listing with order preserved; the new test's False case returns [("dir/x", "v1", False)] on the old code; the aio delegate forwards path, flag and kwargs unchanged. Snapshot verified clean afterwards.
| ) | ||
| fs.pipe(path, b"data") | ||
| # A sibling key that starts with the same characters as the path. | ||
| fs.pipe(f"{path}.bak", b"backup") |
There was a problem hiding this comment.
Independent review (relayed), finding 2 — P3, not changed: the reviewer noted that the new <uuid>.bak object is never removed. Rejected: the staging bucket's lifecycle rule (cloudformation/github_actions_oidc.yaml:329-338: ExpirationInDays: 1, NoncurrentVersionExpiration 1 day, AbortIncompleteMultipartUpload 1 day) expires it, which is what the existing path object of this test (and the other unique-path filesystem tests) already relies on; an explicit delete would add a request per run without changing the outcome.
A key that had only delete markers and was also a folder returned the markers with delete_markers=True but the versions under key/ without them. Decide on the key itself whenever it has any versions or delete markers, and drop the delete markers only after that choice, so both modes select the same key from the same single listing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 participant
WHAT
list_multipart_uploads(),clear_multipart_uploads()andobject_version_info()no longer act on sibling keys that merely start with the same characters as the path key.list_multipart_uploads()keeps only the uploads to the key itself and to the keys underkey/.clear_multipart_uploads()aborts only those, soclear_multipart_uploads("s3://bucket/data")no longer aborts the uploads todata2/other.csvordata.csv.object_version_info()selects the key itself when it has any versions or delete markers, and otherwise the keys underkey/;delete_markers=Falsethen drops the markers from that selection. The choice is made before the markers are dropped, so bothdelete_markersvalues pick the same key: for a name that is both a deleted object (only delete markers left) and a folder,delete_markers=Truereturns the markers anddelete_markers=Falsereturns[], instead of switching to the folder's contents. This keeps the single listing (markers are in the same ListObjectVersions response) and is one rule: "does the key exist in the version history?". A path with a trailing slash always selects the keys under it, and a bucket path still returns every key.object_version_info("s3://bucket/a.csv")no longer returns the versions ofa.csv.bakora.csv/x.AioS3FileSystemdelegates these methods to the sync implementation, so it gets the same behavior; its docstrings anddocs/filesystem.mdare updated.The request pattern is unchanged: one paginated listing with
Prefix=key, filtered on the client. With an explicitEncodingType="url"inobject_version_info()'s**kwargs, botocore leaves the listed keys encoded, so they are decoded for matching and returned as listed.Release note (behavior change): with a key path, these methods now ignore sibling keys that only share the leading characters. Code that relied on the raw string-prefix match (e.g.,
clear_multipart_uploads("s3://bucket/tmp")to cleartmp1/,tmp2/) must call them per prefix or with the bucket path.WHY
Closes #981. S3 matches
Prefixas a plain string, and the methods returned every match. An aborted multipart upload cannot be resumed, soclear_multipart_uploads()could destroy in-progress uploads outside the requested path.TEST
Tested commits: 3872f31; review repair 6a92aa0; unified delete-marker rule 3867c35 (Python 3.13.1).
just lint: passed.--noconftestwith dummy env): newtest_object_version_info_excludes_sibling_keys(key, prefix without/with trailing slash, key with trailing slash, bucket; with and without delete markers) andtest_list_and_clear_multipart_uploads_exclude_sibling_keys, plus the existing pagination/delete-marker tests: 11 passed. With thes3.pychange reverted, 3 of the new cases fail.test_list_and_clear_multipart_uploadsnow creates a sibling upload (<prefix>2/file) and checks that it is neither listed nor aborted;test_object_version_infowrites a<path>.baksibling. With thes3.pychange reverted (on the pre-push tree, which differed only by a redundantnot keyguard), both live tests fail.uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/on 3872f31: 283 passed. An earlier attempt on the pre-push tree aborted with an xdistINTERNALERROR(KeyError: <WorkerController gw0>) before running any test; reruns passed.test_object_version_info_matches_url_encoded_keysfails without the repair; offline subset 12 passed; liveuv run --env-file .env pytest -n 1 tests/pyathena/filesystem/test_s3.py -k "object_version_info or read_version_id or multipart_uploads": 15 passed. A one-off live check confirmed S3 lists the keya basa+bwithEncodingType="url".test_object_version_info_chooses_key_with_only_delete_markers(bothdelete_markersvalues); itsFalsecase fails on 6a92aa0. Offline subset 14 passed; liveuv run --env-file .env pytest -n 1 tests/pyathena/filesystem/test_s3.py -k "object_version_info or read_version_id or multipart_uploads": 17 passed.just lintpassed.🤖 Generated with Claude Code