Use multipart upload objects and fix checksum completion and cleanup races - #1072
Conversation
| "MultipartUpload": { | ||
| "Parts": [{"ETag": p.etag, "PartNumber": p.part_number} for p in parts] | ||
| }, | ||
| "MultipartUpload": {"Parts": [part.to_api_repr() for part in parts]}, |
There was a problem hiding this comment.
Self-review round one (implementation behavior): CLEAN.
Covered all six changed files: response parsing and ten checksum properties, serialization and core request precedence/order, sync completion and aio copy callers, cleanup error classification and executor result draining, and offline/live regression validity. No actionable defect found. NoSuchBucket and unclassified missing-resource errors propagate, and direct core abort behavior is preserved.
Evidence: just lint passed; 169 offline tests passed; 14 new live S3 regressions passed. The broader existing filesystem run was intentionally interrupted after 122 passes to serialize AWS validation and remains incomplete. The strict Sphinx build has the same 195 inherited diagnostics as the unchanged parent. This review verdict does not claim unrun coverage.
There was a problem hiding this comment.
Self-review round one follow-up (full implementation scope): CLEAN.
Covered the complete nine-file diff after the public completion argument and regression coverage expanded: all ten upload/copy checksum fields and None filtering; creation-algorithm selection and default core request shape; sync write/append/copy and async copy callers; request precedence and part order; precise NoSuchUpload classification and draining all scheduled abort results; changed mocks and regression sensitivity; the direct-core documentation.
Resolved the initial default-upload finding by selecting only the creation algorithm, with default and mixed-checksum request assertions. Verified that no algorithm argument preserves the existing direct-core request. Reviewed failure/abort ownership without changing retries or adding requests. The two patches are identical across the master rebase (range-diff); upstream filesystem, fixtures, and dependency contracts are unchanged. Upstream Connection only records whether a session was supplied.
Evidence: rebased just lint and 177 focused offline tests passed. Earlier repaired AWS coverage passed 266 cases, including all eight previously failing default write/append cases and 26 new live regressions; six copy Stubber fixtures had incorrect expected ranges, were corrected, and subsequently passed. The clean rebased affected suite is running. No actionable finding remains in this implementation review; final runtime validation and Ready-triggered CI are still pending.
There was a problem hiding this comment.
Final validation update for the reviewed revision.
No source changes after the full review. Lint and docs lint passed; normal rendered docs succeeded with 195 inherited warnings. All 177 focused offline cases passed. The affected 272-case filesystem selection completed as 268 passes before deliberate AWS serialization and the remaining four CRC32 append/list-abort race cases passed after resumption. No failures remain in that selection.
Published head matches this reviewed commit, the worktree is clean, all Draft/offline CI checks passed, and GitHub reports MERGEABLE. Both self-review rounds and the independent full review are complete; Ready-triggered AWS CI will be checked separately.
There was a problem hiding this comment.
Self-review round one: test-convention repair, CLEAN after correction.
Base: 6d268b1
Previously reviewed head: 534e9a4
Current head: 745664a
The earlier review missed concrete convention mismatches: new module-level tests beside existing test classes, synchronous-only regressions in the async test module, and a separate integration class/fixture with a four-operation branch. These are corrected in the four changed test files.
Core and part-model tests now extend TestS3Core and TestS3MultipartUploadPart. Filesystem regressions extend TestS3FileSystem/TestAioS3FileSystem in their respective modules. Sync copy is an ordinary method; async copy is an async method. Live tests reuse each existing class-scoped fs fixture and separate open, put_file, pipe_file, and append while retaining default/SHA256/CRC32 parametrization, content assertions, upload cleanup, and the controlled list/abort race.
Checked both old commit objects and the full patch-series range-diff: the two production patches are unchanged; the only added commit changes these four test files. Traced the revised tests through their fixtures and unchanged multipart/cleanup callers. No coverage or assertion was intentionally removed. just format and just lint passed; 157 core/object/error cases plus 20 cleanup/copy cases passed. The revised 26 live cases are running; this verdict does not claim their completion.
There was a problem hiding this comment.
Self-review round one, final bounded follow-up: CLEAN.
Base: 6d268b1
Old head: 745664a
Head: a9e7dc8
Verified old objects and range-diff: all three earlier patches are unchanged; only the two filesystem test modules change. Both controlled race tests now assert the patched list/abort callback is called once with the requested path, preventing an unexercised race from passing. The context manager restores the original method before the final list and finally cleanup. Async object/prefix names now match neighboring test_async_* paths while preserving schema/UUID isolation.
Format and lint passed. Both strengthened live races passed at this head. The preceding 177 offline and 26 live results belong to the unchanged test bodies at the immediately preceding revision; checksum algorithms, operations, fixtures, and case counts are preserved. No production change or actionable implementation finding.
| if ( | ||
| isinstance(e, FileNotFoundError) | ||
| and isinstance(cause, botocore.exceptions.ClientError) | ||
| and S3ClientError(cause).code == "NoSuchUpload" |
There was a problem hiding this comment.
Self-review round two (compatibility, operations, and claims): CLEAN.
Audited the complete six-file inventory and PR/docstring claims. The API serialization now explicitly omits None values; existing checksum properties, copy metadata, request precedence, input order, and method signatures are preserved. Verified all ten checksum fields in the minimum supported botocore 1.43.31. Core error chaining permits distinguishing NoSuchUpload from NoSuchBucket; other abort failures propagate after result draining without changing retries or adding S3 requests.
Traced synchronous finishing, asynchronous copy, aio synchronous wrappers, and direct abort/discard callers. Live tests use unique object paths and validate round-trip data plus a controlled list/abort race. The PR's requirement claim was narrowed to the reported SHA256/CRC32 cases; the eight other algorithms have offline coverage only. New API properties appear in the rendered documentation; strict Sphinx diagnostics have the same count as the unchanged parent. Broader filesystem validation and Ready-triggered CI remain pending, so this is not a full-runtime verdict.
There was a problem hiding this comment.
Self-review round two follow-up (full compatibility, operations, and claims scope): CLEAN.
Audited the complete nine-file diff and the revised PR, docstrings, and related filesystem documentation as a separate pass. Claims checked: matching creation-algorithm checksums only; no-argument core completion preserves ETag/part-number requests even when parts contain SDK CRC32; upload and CopyPartResult parsing retains all ten fields; serialization omits None; only chained NoSuchUpload is treated as cleared; missing bucket, permission, unclassified errors, and direct core abort errors remain observable after ordinary abort-result draining.
The new core option is keyword-only and optional; existing positional calls and uppercase AWS parameters retain their behavior and precedence. The private sync finish option follows its existing positional arguments. Traced async copy and synchronous wrappers/open paths to the same creation response. The supported minimum botocore model (1.43.31) accepts all ten fields; the locked runtime is 1.43.102, whose checksum-default implementation and measured default failures justify excluding incidental checksums. This changes no retry policy, pagination, AWS request count, or cleanup prefix scope.
Evidence: 177 rebased offline cases and just lint passed; just docs lint passed; the normal rebased Sphinx build succeeded with 195 inherited warnings. Unique-path live tests verify byte contents and a controlled listed-upload race for both filesystem classes. Live algorithm claims are limited to default/SHA256/CRC32; eight other algorithms are covered only by offline request tests. The earlier eight default runtime failures are resolved; corrected copy Stubber fixtures passed separately. Final rebased AWS coverage is running, and CI is still Draft/offline. No unrun live or CI check is claimed as passed.
There was a problem hiding this comment.
Final validation update for the reviewed revision.
No source changes after the full review. Lint and docs lint passed; normal rendered docs succeeded with 195 inherited warnings. All 177 focused offline cases passed. The affected 272-case filesystem selection completed as 268 passes before deliberate AWS serialization and the remaining four CRC32 append/list-abort race cases passed after resumption. No failures remain in that selection.
Published head matches this reviewed commit, the worktree is clean, all Draft/offline CI checks passed, and GitHub reports MERGEABLE. Both self-review rounds and the independent full review are complete; Ready-triggered AWS CI will be checked separately.
There was a problem hiding this comment.
Ready-triggered CI completed successfully for reviewed head 534e9a4: https://github.com/pyathena-dev/PyAthena/actions/runs/37182904092
Python 3.14 PyAthena suite: 2304 passed, 1 skipped, 7 warnings. All applicable PR checks passed (lint, license headers, offline checks, docs lint/build, and AWS tests). SQLAlchemy/Spark are excluded by this filesystem-only diff's workflow path conditions. The PR is Ready and MERGEABLE; the reviewed worktree remains clean. Recent master changes affect cursor conversion/result paths and add an unrelated query constant in tests/pyathena/util.py; they do not alter the reviewed filesystem, multipart API, or test helper contracts. No source change follows the completed reviews.
There was a problem hiding this comment.
Self-review round two: test-convention repair, CLEAN.
Base: 6d268b1
Previously reviewed head: 534e9a4
Current head: 745664a
Audited the four-file repair and updated description separately from implementation review. Verified the stated class/file conventions directly against neighboring existing tests: TestS3Core, TestS3MultipartUploadPart, TestS3FileSystem and TestAioS3FileSystem. No added module-level test functions, fs_class branching, separate regression class, or replacement fs fixture remain. The live tests use the existing class-scoped fs fixtures and distinct operation methods.
Checked fixture scope and caching, sync versus async entry points, the preserved 2-filesystem x 4-operation x 3-algorithm matrix plus two races, unique object paths, cleanup, and offline error/request assertions. Dummy credentials and single-worker Stubber ordering remain confined to offline cases. Existing live fixture settings replace the special new fixture settings and have now been exercised against AWS.
Evidence at this head: format/lint passed, 157 model/core/error and 20 filesystem offline cases passed, and all 26 revised live cases passed. Production code, documented API behavior, supported dependencies, and request/retry semantics are unchanged by this repair. Corrected PR validation commands for moved tests; the prior 2304-case CI result is not claimed for this new head. Current CI and the independent follow-up remain separate requirements.
There was a problem hiding this comment.
Self-review round two, final bounded follow-up: CLEAN.
Base: 6d268b1
Old head: 745664a
Head: a9e7dc8
Separately audited claims, compatibility, and evidence for the callback/path follow-up. The synchronous mock targets fs.list_multipart_uploads; the asynchronous mock targets the delegated sync_fs method. Both asserts execute after successful clear, and outside-context listing still uses the restored method. Unique async prefixes differ only in naming and retain account/bucket/schema scope and UUID isolation. The earlier class/fixture conventions and 26-case live matrix are preserved.
The two strengthened live races and current format/lint passed. No claim is made that earlier 177/26 counts or earlier full CI were rerun on this head. Updated validation will name their tested revisions and final CI will be collected on this published head. No API, AWS retry, or production contract changes are introduced.
There was a problem hiding this comment.
Final convention-repair validation: complete.
Base: 6d268b1
Head: a9e7dc8
AWS CI: https://github.com/pyathena-dev/PyAthena/actions/runs/37185094919
Python 3.14: 2318 passed, 1 skipped, 13 warnings.
All applicable checks passed, including final AWS tests, lint, license headers, offline tooling, and docs lint/build. SQLAlchemy/Spark are excluded by the current diff's workflow path conditions. PR is Ready and MERGEABLE, at the independently reviewed head; the worktree is clean.
Existing classes/fixtures, separate sync/aio files and entry points, distinct operation methods, async naming, and explicit race-callback assertions are now in place. Both self-review perspectives and the requested Claude Max / claude-opus-5-5 independent follow-ups are recorded above as CLEAN. Earlier local 177/26 results retain their explicit tested-revision boundaries; the current full CI establishes validation on this final head.
| "MultipartUpload": { | ||
| "Parts": [{"ETag": p.etag, "PartNumber": p.part_number} for p in parts] | ||
| }, | ||
| "MultipartUpload": {"Parts": [part.to_api_repr() for part in parts]}, |
There was a problem hiding this comment.
Relayed independent review: FINDINGS (static), Claude Code claude-opus-5-5, Max profile with verified first-party Max authentication (not Enterprise), effort high.
Base: e8f2853
Head: fa673be
Session: 8de20766-061c-4d90-b6a5-7eaa7211bfc6
HIGH: Sending every returned part checksum can break uploads created without ChecksumAlgorithm. The reviewer noted botocore's default UploadPart CRC32, which need not match the algorithm selected at creation. Default writes and mixed copied/uploaded append parts then send unwanted checksum fields during completion. The initial static report treated S3's exact rejection as a hypothesis; the author's existing AWS suite subsequently failed eight default write/append cases with InvalidPart (246 other cases passed). A repair will pass the creation algorithm through completion and send only its matching checksum, with default-write regression coverage.
The reviewer covered the entire six-file diff, sync/aio callers, copy response parsing, public serialization, error chaining, cleanup classification, executors, and test sensitivity. No other actionable defect was reported. The frozen source snapshot and implementation worktree remained unchanged. No builds, tests, GitHub access, edits, or agent-memory access were authorized for the reviewer.
There was a problem hiding this comment.
Relayed independent review follow-up (full expanded scope): CLEAN (static).
Reviewer: Claude Code claude-opus-5-5, Max profile, effort high.
Authentication: verified first-party Claude Max, not Enterprise; API/provider override variables removed. Result metadata confirms canonical model claude-opus-5-5 and firstParty provider.
Base: 6d268b1
Head: 534e9a4
Session: 60c8f64b-12ee-4f61-bce0-3725e42ba81c
Completed the entire nine-file review after both self-review follow-ups. Covered checksum flow through sync copy, file commit/write/append, aio inherited file commit, and async copy; part parsing/serialization; exact translated error causes and ordinary result draining; argument and AWS-parameter compatibility; all changed test fakes/assertions; regression coverage and documentation. No actionable defect found. The initial default-upload finding is resolved by sending only the checksum selected at creation, or no part checksum when that algorithm is None.
Non-defect notes retained: public serialization intentionally omits None-valued keys as documented; completion now uses the already-declared S3MultipartUploadPart contract rather than incidental duck typing; sync-only helper regressions reside beside combined sync/aio cleanup tests in test_s3_async.py. These do not identify a verified behavior defect.
Review limits: source inspection only, with no tests/builds/network/GitHub writes/edits/agent-memory access or delegation. The reviewer could not verify the installed minimum botocore model or actual S3 checksum semantics from the allowed snapshot. The author separately verified the minimum model and measured default/SHA256/CRC32 writes and appends. FULL_OBJECT-specific behavior and the other eight algorithms have no live measurement claimed here.
The exported 286-file tracked-source snapshot plus literal patch (287 hashes) remained unchanged, as did the clean implementation worktree at the reviewed head. Final runtime validation and Ready-triggered CI remain separate requirements.
There was a problem hiding this comment.
Relayed independent test-repair review: CLEAN (static).
Reviewer: Claude Code claude-opus-5-5, verified first-party Max profile (not Enterprise), high effort.
Session: 80487fc6-0965-4262-9210-019a54f60997
Base: 6d268b1
Old head: 534e9a4
Reviewed head: 745664a
Completed a bounded four-test-file follow-up after both self-reviews. Verified moved test counts (44 core/object and 20 cleanup/copy cases, 24 checksum writes plus two live races), discovery and unique method names, existing class-scoped fs fixtures, no per-call checksum option leakage, sync/async entry points, patched cleanup receivers, unique prefixes and finally cleanup, and conventions against neighboring tests. No actionable regression found.
The reviewer noted an inherited sensitivity gap: race tests should explicitly assert the injected list/abort callback was called. It also noted async S3 prefixes should use the neighboring test_async_* naming convention. Both are being corrected in a small follow-up; the optional stored-checksum metadata measurement is not added because strict Stubber algorithm requests and live content/cleanup assertions already cover the reported defect, and no claim of measuring stored checksum metadata is made.
Limits: read-only static review, no execution or AWS/runtime validation. Both original production patches were unchanged. The 292 hashed snapshot/comparison files and the implementation worktree remained unchanged during review. Result metadata confirms claude-opus-5-5 and firstParty; no Enterprise/provider override was used.
There was a problem hiding this comment.
Relayed final bounded independent follow-up: CLEAN (static).
Reviewer: Claude Code claude-opus-5-5, verified first-party Max profile (not Enterprise), high effort.
Session: 768c4e8c-0925-4f27-b6e7-ad3deb26aa27
Base: 6d268b1
Old head: 745664a
Reviewed head: a9e7dc8
Reviewed only the final callback assertions and async S3 path strings, tracing their immediate fixtures, mock receivers, cleanup and callers. Both assertions match the production single positional list call; the saved bound method prevents recursive mock calls, and the mock is restored before final listing and cleanup. The assertion closes the earlier race-bypass sensitivity gap. All async paths now follow neighboring test_async_* naming and preserve schema/UUID isolation and trailing-slash prefix scoping. Formatting and class-based organization match neighboring tests. No actionable defect remains.
The reviewer observed identical sync/async method names, which are conventional here and have distinct module/class pytest node IDs; no change is needed. This was read-only source review, without tests, builds, edits, network/GitHub/agent-memory access or delegation. Actual S3/runtime semantics are not established by the review; the author separately passed both strengthened live races.
Result metadata confirms claude-opus-5-5 and firstParty. The 290 hashed comparison/snapshot files and the clean worktree remained unchanged. The three preceding patches are unchanged in the range-diff. Both self-review perspectives and both independent repair reviews are complete; current CI remains the final separate gate.
6b46db1 to
534e9a4
Compare
a9e7dc8 to
82fbe04
Compare
laughingman7743
changed the title
| } | ||
| _logger.debug(f"Upload part of {upload_id} to {path.uri} as part {part_number}.") | ||
| if upload.checksum_algorithm is not None: | ||
| request["ChecksumAlgorithm"] = upload.checksum_algorithm |
There was a problem hiding this comment.
Self-review round one — behavior, public contracts, simplicity, and regression coverage: CLEAN.
Frozen full diff: 200762088e7b0bac45054f22e32a16aa0ce95dbb..f9e2a831c907408dcba44846c8496d3dfb993e42 (actual PR base: master). All nine changed files were inventoried, with their direct callers and regression tests.
Checked all four upload-object primitive signatures; identity validation and request-field precedence; SDK checksum algorithm inheritance; matching checksum serialization and ChecksumType; model response fields; sync/async multipart copies; buffered writes and all append ranges; direct abort and bulk cleanup error policies. Traced late creation after interruption/cancellation through the #1074 executor/task cleanup paths. Tests stay in existing classes, use real S3MultipartUpload objects, Stubber expectations, and existing live fixtures.
Validation: required format/lint checks passed; 192 offline core/model/error cases and 510 offline filesystem cases passed on implementation-equivalent 82fbe04. The final follow-up only adds runtime CRC32/listing assertions. The same 32 live filesystem checksum cases and two live list/abort races passed at 82fbe04. Direct-core/listing runtime checks on the frozen head are in progress and are not claimed as passed by this static CLEAN verdict.
Scope limits: no SQLAlchemy or Spark changes; algorithms other than default/SHA256/CRC32 have offline request/model coverage. No actionable implementation defect remains.
There was a problem hiding this comment.
Round-one repair self-review: CLEAN. Frozen base/head: 2007620..49a438f. Bounded repair comparison: f9e2a83..49a438f; both objects exist, range-diff confirms the six previous patches are unchanged and the repair affects only s3_core.py, test_s3_core.py, and test_s3.py. Checked request identity retention before constructing S3MultipartUpload, unchanged response upload ID/checksum fields, all four downstream primitives, listing identity, and the sync/async creation/cleanup callers. Access-point alias and ARN Stubber cases now trace creation through upload, copy, completion and NoSuchUpload abort; tests fail when the canonical response bucket replaces the requested endpoint. The file keyword test asserts the real create path and exact upload-object identity, for both key and upload collisions. Format/lint passed; the complete offline core/model/error selection passed 194 cases and the filesystem selection passed 511. Access-point runtime infrastructure was not created; those endpoint cases are offline only. Independent follow-up is pending.
| plan.destination.bucket, | ||
| cast(str, plan.destination.key), | ||
| cast(str, creation.result().upload_id), | ||
| creation.result(), |
There was a problem hiding this comment.
Self-review round two — claim audit, compatibility, AWS effects, and evidence: CLEAN.
Frozen full diff: 200762088e7b0bac45054f22e32a16aa0ce95dbb..f9e2a831c907408dcba44846c8496d3dfb993e42 (actual PR base: master). All nine changed files were inventoried, with their direct callers and regression tests.
Audited the final PR description, changed docs/docstrings/comments, and the maintainer contract separately from round one. The destination, upload ID, and checksum configuration come from one upload object; no per-upload state is cached in S3Core. Old primitive signatures and the separate completion checksum_algorithm option are removed intentionally for the unreleased 4.0.0 API. The decision and release-note item are recorded on #1063.
Checked AWS primary API documentation for UploadPart algorithm consistency, list-entry checksum fields, and completion ChecksumType. Botocore 1.43.31 exposes all required fields. ListMultipartUploads supplies Key/UploadId and optional checksum algorithm/type; the adapter adds Bucket, and abort sends identity only. FULL_OBJECT is retained and forwarded, rather than inferred from incidental part checksums. SDK algorithm support is not overstated by the ten-field serialization coverage.
Reviewed request filtering, SSE-C/requester-pays/conditional writes, source version pinning, retry/error translation, futures/tasks and cancellation ordering, prefix-scoped test cleanup, and cache invalidation boundaries. The #1074 planning/scheduling/cancellation contract is preserved. No new requests, retries, or upload state were added to the primitives.
Evidence boundary: 702 offline cases and 34 targeted live filesystem cases passed on the unchanged implementation; current direct-core/live checksum-value checks remain pending. Docs lint passed and a rendered build succeeded with 205 warnings, without claiming a warning-free build. Current CI and remaining live results are required before delivery. No actionable finding.
There was a problem hiding this comment.
Local validation completed at f9e2a83: the direct-core/listing selection passed all 12 cases in 75.05s. Nine use live AWS (six default/SHA256-COMPOSITE/CRC32-FULL_OBJECT upload/copy cases and three listing/cleanup cases); three are offline sibling-prefix cases. Stored FULL_OBJECT CRC32 matches the checksum calculated over the complete data, for both uploaded and copied first parts. Listing preserves the creation algorithm/type, and clear_multipart_uploads aborts those entries without leaking checksum fields into AbortMultipartUpload. The separate 34 live filesystem cases passed on the unchanged implementation at 82fbe04. Format/lint passed on this head, and current offline CI checks, including rendered docs, all passed. The independent review and Ready-triggered full AWS CI remain pending.
There was a problem hiding this comment.
Round-two repair claim/operational audit: CLEAN. Frozen base/head: 2007620..49a438f. Bounded repair comparison: f9e2a83..49a438f; both objects exist, range-diff confirms the six previous patches are unchanged and the repair affects only s3_core.py, test_s3_core.py, and test_s3.py. Verified the AWS CreateMultipartUpload response Bucket excludes the access-point alias/ARN, so retaining the request endpoint is necessary for subsequent authorization and cleanup. The creation docstring states this identity contract; checksum configuration remains from the service response. No upload state, extra requests, retries or changes to cancellation were added. The stronger assertion now observes the actual creation call and mapping propagation rather than only a hard-coded mock result. Full-object CRC32 and checksum-bearing listing measurements remain valid for the unchanged checksum/cleanup logic; current full CI remains required. No live access-point claim is made. Independent follow-up is pending.
There was a problem hiding this comment.
Final current-head CI — PASS. Frozen review base/head: 2007620..49a438f.
Ready-triggered Test run https://github.com/pyathena-dev/PyAthena/actions/runs/37191012018 completed successfully. The Python 3.14 PyAthena job, including AWS integration tests, reported 2,424 passed, 1 skipped, and 13 warnings in 464.03s. Lint, offline checks, license headers, and documentation lint/build have all completed successfully on this head. SQLAlchemy compliance and Spark suites are intentionally excluded by the filesystem-only path filters in test.yaml; the older Draft run's skipped test job is superseded by this successful Ready run.
Both self-review rounds and repair follow-ups remain CLEAN. The requested independent Claude Max / claude-opus-5-5 review and bounded repair follow-up are recorded separately; the follow-up is CLEAN (static only). Author runtime validation is provided by this current-head CI and the earlier targeted AWS runs. Access-point alias/ARN coverage remains offline only.
The published PR is Ready, OPEN, and MERGEABLE at the reviewed head; the dedicated worktree is clean.
| raise ValueError("The multipart upload has no key.") | ||
| if not upload.upload_id: | ||
| raise ValueError("The multipart upload has no upload ID.") | ||
| return {"Bucket": upload.bucket, "Key": upload.key, "UploadId": upload.upload_id} |
There was a problem hiding this comment.
Relayed independent review — FINDINGS (static only).
Reviewer: Claude Code, claude-opus-5-5, personal Max profile, effort high; first-party Max authentication verified with provider overrides unset. Read/Grep/Glob only on the frozen tracked snapshot and literal full diff. No edits, builds, tests, commands, network, GitHub writes, delegation, PR discussion, commit messages, prior findings, or agent memory. Session 663f96bc-7426-4534-bf25-df8938068135; 31 turns, 252.244s; actual model/provider metadata matches the request. All 284 packet hashes remained unchanged.
Scope: full 2007620..f9e2a83, all nine changed files plus direct callers: primitives/model/serialization; sync/async copy and late creation cleanup; finish/abort; S3File writes/appends/commit/discard; list/bulk cleanup; request filtering, errors, tests, and docs.
[P2] At s3_core.py:677-678, creation constructs S3MultipartUpload directly from the response. Follow-up primitives now use that object's bucket/key instead of the original request path. The CreateMultipartUpload response returns the bucket name rather than an access point alias/ARN. A write or copy authorized only through an access point can create successfully, then send its parts/completion/cleanup to the underlying bucket, get AccessDenied, and leave an incomplete upload. The related changed helper is the inline anchor. The author independently verified the AWS contract: https://docs.aws.amazon.com/AmazonS3/latest/API/API_CreateMultipartUpload.html . Preserve the request bucket/key when building the upload object and test a canonical bucket response through all four follow-up operations.
The second finding is recorded inline on the affected test assertion. Optional checksum-case normalization is deferred: no supported endpoint returning a lower-case algorithm was identified, and AWS documents upper-case response enum values; this is not a confirmed defect. The reviewer did not execute SDK compatibility checks or AWS tests; the author's validation remains separate from this review. The reviewer otherwise found no defect in checksum inheritance/type, field precedence, list-derived aborts, cleanup error classification, copy cancellation, or model serialization. Fixes and independent follow-up are pending; PR remains Draft.
There was a problem hiding this comment.
Author repair at 49a438f: the P2 identity finding is resolved by retaining request Bucket/Key when building the upload, while preserving the response upload ID/checksum fields. Added access-point alias and ARN tests through all four follow-up primitives. The P3 test weakness is resolved by asserting the create call and exact upload identity, parametrized for key/upload keyword collisions. Both self-review perspectives are CLEAN for the bounded repair; 194 core/model/error and 511 offline filesystem cases passed, along with format/lint. AWS access-point runtime was not run. The optional case-normalization item remains deferred because no supported endpoint returning lower-case algorithms was identified; the documented response enums are uppercase. Independent follow-up and current full CI remain pending.
There was a problem hiding this comment.
Relayed independent repair review — CLEAN (static only). Claude Code, actual claude-opus-5-5 / firstParty, verified personal Max profile, effort high, same Read/Grep/Glob-only constraints. Session aec2d3b9-fb86-456f-8be8-6337bbb74a09; 29 turns, 113.756s; no permission denials. All 288 packet hashes and the PR worktree remained unchanged.
Frozen base/head: 2007620..49a438f. Bounded comparison from f9e2a83; patches 1–6 are identical. Covered the three repaired files, upload model and all four primitives, sync/async creation and late cleanup, S3File commit, request filtering, relevant helpers, and docs.
P2 identity defect: RESOLVED. Creation retains request Bucket/Key; UploadId, checksum algorithm/type and other response metadata are preserved. All downstream operations and delayed creation aborts reuse that same upload object. Alias/ARN Stubber cases assert creation and all four following requests, including NoSuchUpload abort, while expecting checksum propagation.
P3 test weakness: RESOLVED. The test checks the actual create path, exact returned-object identity at completion, and mapping propagation for both key/upload keyword collisions. The reviewer confirmed these assertions exercise real operation_params filtering rather than only a hard-coded return value.
No actionable findings in the repaired scope. ARN tests concern direct S3Core callers constructing S3Path(arn, key); filesystem URI parsing is a pre-existing separate surface. The reviewer executed no tests or live S3 calls; author validation remains separate (194 core/model/error and 511 offline filesystem cases passed). Current full AWS CI is required before delivery.
| f.write(b"x" * 8) | ||
|
|
||
| assert fs._finish_multipart_upload.call_args.kwargs["key"] == "key.txt" | ||
| assert fs._finish_multipart_upload.call_args.kwargs["upload"].key == "key.txt" |
There was a problem hiding this comment.
Relayed independent review [P3, test weakness] at test_s3.py:6087: _make_append_fs always returns an upload whose key is key.txt. Checking only the returned object's key no longer proves that S3File created it at its own path rather than the file-level key="other" parameter. A regression in the create destination could pass. Assert the actual create_multipart_upload(S3Path("bucket", "key.txt")) call and exercise the new upload-named parameter collision. Same frozen head/session/constraints as the companion independent-review record; static only.
There was a problem hiding this comment.
Author repair at 49a438f: the P2 identity finding is resolved by retaining request Bucket/Key when building the upload, while preserving the response upload ID/checksum fields. Added access-point alias and ARN tests through all four follow-up primitives. The P3 test weakness is resolved by asserting the create call and exact upload identity, parametrized for key/upload keyword collisions. Both self-review perspectives are CLEAN for the bounded repair; 194 core/model/error and 511 offline filesystem cases passed, along with format/lint. AWS access-point runtime was not run. The optional case-normalization item remains deferred because no supported endpoint returning lower-case algorithms was identified; the documented response enums are uppercase. Independent follow-up and current full CI remain pending.
There was a problem hiding this comment.
Relayed independent repair review — CLEAN (static only). Claude Code, actual claude-opus-5-5 / firstParty, verified personal Max profile, effort high, same Read/Grep/Glob-only constraints. Session aec2d3b9-fb86-456f-8be8-6337bbb74a09; 29 turns, 113.756s; no permission denials. All 288 packet hashes and the PR worktree remained unchanged.
Frozen base/head: 2007620..49a438f. Bounded comparison from f9e2a83; patches 1–6 are identical. Covered the three repaired files, upload model and all four primitives, sync/async creation and late cleanup, S3File commit, request filtering, relevant helpers, and docs.
P2 identity defect: RESOLVED. Creation retains request Bucket/Key; UploadId, checksum algorithm/type and other response metadata are preserved. All downstream operations and delayed creation aborts reuse that same upload object. Alias/ARN Stubber cases assert creation and all four following requests, including NoSuchUpload abort, while expecting checksum propagation.
P3 test weakness: RESOLVED. The test checks the actual create path, exact returned-object identity at completion, and mapping propagation for both key/upload keyword collisions. The reviewer confirmed these assertions exercise real operation_params filtering rather than only a hard-coded return value.
No actionable findings in the repaired scope. ARN tests concern direct S3Core callers constructing S3Path(arn, key); filesystem URI parsing is a pre-existing separate surface. The reviewer executed no tests or live S3 calls; author validation remains separate (194 core/model/error and 511 offline filesystem cases passed). Current full AWS CI is required before delivery.
1 participant
WHAT
Multipart primitives after creation now take the
S3MultipartUploadreturned by creation or listing, replacing separate destination and upload-ID arguments. Creation retains the request bucket/key, including an access point alias or ARN; the response supplies the upload ID and checksum configuration. Follow-up operations derive all of these from the upload object, and the core retains no upload state.upload_part()passes the creation algorithm to the SDK. Completion includes the matching part checksum and the upload'sChecksumType, includingFULL_OBJECT. There is no separatechecksum_algorithmcompletion argument. The part model retains all ten checksum fields supported by botocore 1.43.31.Synchronous/asynchronous copies, buffered writes, appends, finish/abort helpers, and bulk cleanup pass the same upload object. Copy scheduling and cancellation cleanup from #1074 are preserved, including creation requests that finish after cancellation.
clear_multipart_uploads()acceptsNoSuchUploadas already cleared, checks every scheduled abort result, and raises the first other failure. Missing buckets, permission errors, and unclassified missing-resource errors still propagate.WHY
Closes #1070. The previous completion discarded required part checksums, and a listed upload could disappear before cleanup aborted it.
The maintainer decision on #1063 settles the unreleased 4.0.0 primitive API introduced by #1069: keep upload identity and checksum configuration together so callers cannot separately mismatch the destination/ID or omit the creation algorithm. This API change is recorded as a 4.0.0 release-note item on #1063.
AWS requires UploadPart checksums to match creation; a mismatched completion checksum type can produce BadDigest. ListMultipartUploads entries expose both checksum fields and the listing adapter supplies the bucket, so they can be passed directly to abort.
TEST
Current revision:
49a438f170e70832f44adca58fee6dd8945ac9cd, rebased onto master200762088e7b0bac45054f22e32a16aa0ce95dbb(#1074).just format,just lint,git diff --check: passed.uv run --env-file .env pytest --noconftest -n 1 tests/pyathena/filesystem/test_s3_core.py tests/pyathena/filesystem/test_s3_object.py tests/pyathena/filesystem/test_s3_errors.py -q).just docs lint: passed. Rendered Sphinx build succeeded with 205 warnings.82fbe047: 34 passed (uv run --env-file .env pytest -n 1 tests/pyathena/filesystem/test_s3.py tests/pyathena/filesystem/test_s3_async.py -k 'multipart_with_checksum or after_listed_upload_is_aborted or test_multipart_uploads' -q). Covers default/SHA256/CRC32 and CRC32/FULL_OBJECT open, put_file, pipe_file, append, plus cleanup races on both filesystems. These tests are unchanged; this run precedes the final repair to preserve creation's request bucket/key.f9e2a831c907408dcba44846c8496d3dfb993e42: 12 passed (uv run --env-file .env pytest -n 1 tests/pyathena/filesystem/test_s3.py -k 'core_multipart_upload_with_checksum or test_list_and_clear_multipart_uploads' -q). Nine cases use AWS: six direct-core upload/copy cases (default, SHA256/COMPOSITE, CRC32/FULL_OBJECT), including stored whole-object CRC32 assertions, and three checksum-bearing listing/cleanup cases. The selection also includes three offline sibling-prefix cases.claude-opus-5-5, effort high. Its access-point identity and test-assertion findings are repaired, and the independent repair review is CLEAN at the current revision. Access-point alias/ARN coverage is offline only. Live coverage is limited to the algorithms/types above; the remaining checksum fields have offline serialization/request coverage.AI-assisted implementation with Codex.