Move the copy requests and the multipart copy plan into S3Core, and abort uploads interrupted during creation (step 3.3 of #1063) - #1074
Conversation
Add S3Core.copy_object(), plan_multipart_copy() with the frozen S3MultipartCopyPlan, list_object_annotations() and copy_object_annotation(). The planner owns the reads of the source (HeadObject, GetObjectTagging, ListObjectAnnotations), the block size validation, the version pinning, the CopyObject directives, the copy-source parameter mapping and the directory-bucket rule, and filters the parameters of each multipart request. The sync and aio multipart copies now only schedule a plan: sync with the executor and _finish_multipart_upload(), aio with to_thread and its unchanged cancellation handling. A HeadObject response without a size now raises ValueError instead of falling back to the cached size. Part of #1063 (sub-step 3). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ng-size error Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… its own sentence Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…xtures Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| _logger.debug(f"Copy object from {source.uri} to {destination.uri}.") | ||
| self.call(self._client.copy_object, **request, **params) | ||
|
|
||
| def plan_multipart_copy( |
There was a problem hiding this comment.
Self-review round 1 (implementation behavior, public contracts, simplicity, regression coverage; plus /code-review high and /simplify), base 6d268b17d984adf2b16acadfd3e8fd447a03a376, initial head 9187b6f203aca5bd2f9bcbe1abdf7c5172e63779 reviewed in full; repairs in 3a184ae2, 567b2161, 3e6d7c53 (head 3e6d7c534fcddeed02878ca78ab6eefb65e4885f).
Inventory: S3Core.copy_object, plan_multipart_copy/S3MultipartCopyPlan, list_object_annotations, copy_object_annotation; sync/aio _copy_file and _copy_object_with_multipart_upload; removed S3FileSystem copy helpers; docs; tests in test_s3_core.py, test_s3.py, test_s3_async.py.
Behavior checked against origin/master (no change on the wire):
- Request sequence and order: HeadObject → GetObjectTagging (COPY tagging, non-directory bucket) → ListObjectAnnotations pages → CreateMultipartUpload → UploadPartCopy×N → Complete/Abort → Get/PutObjectAnnotation per name; small-object and HEAD-fits paths send CopyObject only. Same validation order (block size, directives, then HeadObject).
- Parameter precedence kept per replaced helper: CopyObject and GetObjectAnnotation keep
**request, **params(duplicate →TypeError), ListObjectAnnotations and PutObjectAnnotation keep request-field precedence. - Version pinning (
nullnot pinned, given version kept) and the HEAD-size re-check are covered bytest_plan_multipart_copy_source_version/_fits_single_request, which fail if the pinning is removed.
Result: FINDINGS (docs/precision only), repaired; no behavior defect found.
|
|
||
| class S3Core: | ||
| """Typed S3 operations, one request each, on a boto3 S3 client. | ||
| """Typed S3 operations on a boto3 S3 client. |
There was a problem hiding this comment.
Round 1 finding (repaired in 3a184ae2, 567b2161): the class summary said "one request each" and docs/filesystem.md put the multi-request exception in a sentence that read as if plan_multipart_copy()/copy_object_annotation() were also exempt from retries and error translation; the plan docstring said "the fields after size are empty", which included fits_single_request. All three now state exactly which operations send several requests and which plan fields are empty. The adapters' Raises: now include the missing-size ValueError.
| head = self.head_object(source, **self.operation_params("head_object", source_params)) | ||
| if head.content_length is None: | ||
| raise ValueError(f"HeadObject reported no size for {source.uri}.") | ||
| if not source.version_id and head.version_id and head.version_id != "null": |
There was a problem hiding this comment.
Round 1 /simplify repairs (3e6d7c53): S3Path.with_version_id() instead of rebuilding the path; operation_params("copy_object", ...) instead of a second hand-written lookup of the CopyObject input shape (line 1011, equivalent for empty and non-empty requests); test_plan_multipart_copy uses the shared MULTIPART_COPY_SIZE/MULTIPART_COPY_BLOCK_SIZE/MULTIPART_COPY_KWARGS; the test helper's bucket1 became source_bucket.
Skipped (reasoned):
- Efficiency: none; the per-part
operation_paramsnow runs once per copy instead of once per part, and aio makes one thread hop for the reads instead of two. - A shared private helper for
{Bucket, Key[, VersionId]}: the pattern predates this PR across S3Core; no stray helpers. setxattr()still sends CopyObject through_call; it is a metadata update outside the Move copy, multipart and delete requests into S3Core (step 3 of #1053) #1063 copy scope.
| request_kwargs=kwargs, | ||
| # Filtered again for the completion and the abort, which | ||
| # leaves the plan's parameters of each unchanged. | ||
| request_kwargs={**plan.complete_params, **plan.abort_params}, |
There was a problem hiding this comment.
Round 1 /code-review + /simplify dispositions:
- All four simplify angles and code-review suggested
request_kwargs=kwargshere. Kept as decided on Move copy, multipart and delete requests into S3Core (step 3 of #1053) #1063 (_finish_multipart_uploadsignature unchanged, the plan's completion and abort parameters passed and re-filtered, which leaves them unchanged); the comment now says so. - Plan fields for the annotation parameters /
fits_single_requestas a property /_is_directory_bucketonS3Path: not taken; the plan fields, the CopyObject parameters ofcopy_object_annotationand the planner owning the directory-bucket rule were decided on Move copy, multipart and delete requests into S3Core (step 3 of #1053) #1063. - HEAD without
ContentLength→ValueError: decided on Move copy, multipart and delete requests into S3Core (step 3 of #1053) #1063 (S3 always returns it); documented in the PR body. - Keyword names that collide with the new signatures (
source=,destination=reachingcopy_object/plan_multipart_copy) now raiseTypeErrorinstead of botocore'sParamValidationError;name/version_id/etagare rejected by botocore at CreateMultipartUpload before any annotation copy. Both are invalid parameters; no change.
|
|
||
| create_kwargs, version_id1, head_size = await asyncio.to_thread( | ||
| self._sync_fs._get_multipart_copy_kwargs, bucket1, key1, version_id1, kwargs | ||
| plan = await asyncio.to_thread( |
There was a problem hiding this comment.
Round 1, aio: only the scheduled callables changed. The failed flag, the semaphore, _cleanup_tasks, the shielded completion and abort-unless-completed (#1046) are untouched; the cancellation tests now get a HEAD size of 3/2 default-size parts instead of relying on the removed missing-size fallback, and still assert the same event order. Block-size and directive validation now run in the planner's thread instead of on the loop; same exceptions.
…ct is not read further Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| annotations that CopyObject would. It sends HeadObject, then GetObjectTagging and | ||
| ListObjectAnnotations unless the directives or the source exclude them, and writes | ||
| nothing. If HeadObject reports a size that fits in one CopyObject request, nothing else | ||
| is read, and the plan's `fits_single_request` says to copy with `copy_object()` instead. `copy_object_annotation()` copies one annotation onto the destination after |
There was a problem hiding this comment.
Self-review round 2 (claims, callers, AWS operation, docs, evidence), base 6d268b17d984adf2b16acadfd3e8fd447a03a376, head 2f7a891c77c088c0afb529c5cf5aca4aeff67e6a; full claim audit of the PR body, the changed docstrings/comments, docs/filesystem.md, docs/api/filesystem.rst and the commit messages.
Claims checked and corrected:
- PR body said the fields "after
size" are empty for a single-request plan (that includesfits_single_request) → now namesranges, the parameter fields andannotations. - PR body said only stubbed responses can lack
ContentLength→ narrowed to "a stub or a non-conforming S3-compatible endpoint". - PR body said
cp_file()/_cp_file()callcopy_object(); it is_copy_file(), behindcp_file(),copy()(fsspec →cp_file) andmv()(direct), sync and aio, verified by the call sites. - These docs did not say that a HEAD size fitting one CopyObject stops further reads → added here (
2f7a891c). - An unverified wording in the TEST section about the live annotation listing was replaced by what the script actually did (source without annotations; the listing was sent and succeeded).
Held: wire-request equivalence (round 1 trace); CopyObject single-request limit equals MULTIPART_UPLOAD_MAX_PART_SIZE (5 GiB); the existing cp section (docs/filesystem.md :108-126) and the aio comparison table (docs/aio.md :269) remain accurate; the removed helpers are private and undocumented.
Result: FINDINGS (claims only), corrected; no code change.
| source_params = self._copy_source_params(params) | ||
| _logger.debug(f"Head object to copy: {source.uri}") | ||
| head = self.head_object(source, **self.operation_params("head_object", source_params)) | ||
| if head.content_length is None: |
There was a problem hiding this comment.
Round 2, callers and operators:
- Existing callers: public methods unchanged;
cp_file/copy/mvsignatures and defaults unchanged. The only caller-visible change is thisValueErrorfor a HeadObject withoutContentLength(decided on Move copy, multipart and delete requests into S3Core (step 3 of #1053) #1063; S3 sets it from theContent-Lengthheader). Private_copy_object_with_multipart_upload()now takesS3Paths; nothing outsidepyathena/filesystemcalls it. - AWS operator: each request still goes through
S3Core.callwith the sameRetryConfig, so attempts and elapsed-time bounds per request are unchanged; the request count per copy is unchanged; aio makes oneto_threadhop for the reads instead of two. - Adversarial: a source replaced between HEAD and the part copies is still read from the pinned version; a
nullversion is still not pinned (pre-existing, documented). - Evidence: offline suite identical failure set to master (all AWS); live
-k "cp_file or copy or mv or move or annotation"80 passed on this head; live 10 MiB multipart copy (limit lowered to 5 MiB) through sync and aio on this head: 2 parts, content, ContentType, Metadata and tags copied. Not covered live: an annotation copy (the source had none) and the full AWS suite (CI on Ready).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| ) | ||
| multipart_upload = await asyncio.to_thread( | ||
| self.core.create_multipart_upload, destination, **create_kwargs | ||
| self.core.create_multipart_upload, plan.destination, **plan.create_params |
There was a problem hiding this comment.
Independent review (relayed): Codex CLI codex-cli 0.160.0, model gpt-6.1-sol, session 01a105a9-5ad8-7fd2-ab4d-dd1f293bdca0, codex exec -s read-only on a detached snapshot at 2f7a891c77c088c0afb529c5cf5aca4aeff67e6a, diff 6d268b17d984adf2b16acadfd3e8fd447a03a376..2f7a891c77c088c0afb529c5cf5aca4aeff67e6a. The prompt gave the intended behavior only: no PR number, PR text or prior findings. Review only: no edits, builds, tests or GitHub access. Static review.
Coverage (reviewer): the exact diff and the base copy helpers; request order, filtering, precedence, directives, version pinning and HEAD size handling; sync/aio scheduling, completion, abort, cancellation and cache invalidation; public APIs, docstrings, both docs files, changed tests, simplicity.
Result: FINDINGS. Introduced regressions: none. One pre-existing defect:
P2: a cancellation during
CreateMultipartUpload(here,asyncio.to_thread) can leak an incomplete upload. The coroutine exits before it has the upload ID or enters the cleanup, while the thread can still create the upload, and nothing aborts it. The same window exists at bases3_async.py:639.
Author verification: confirmed at the base. It is the case that PR #1056 explicitly documented as out of scope ("A cancellation during the CreateMultipartUpload request, before the upload ID is known. The sync path does not handle an interrupt there either."). No issue tracks it yet. This refactor does not change the window. Deferred: not fixed here (it would change behavior); raised with the maintainer for a grouped issue.
There was a problem hiding this comment.
Independent follow-up (relayed): Codex gpt-6.1-sol, session 01a105ae-6e5c-7d22-ba68-3a16062a85d9, codex exec -s read-only on a detached snapshot at 1d84d89532c3ebe11efb1ea83ece665b547e8050, reviewing only 2f7a891c77c088c0afb529c5cf5aca4aeff67e6a..1d84d89532c3ebe11efb1ea83ece665b547e8050 (the docs repair). Result: CLEAN. The paragraph stays accurate: cp_file(), copy() and mv() reach the multipart plans in both filesystems, and nothing else changed. Static review.
There was a problem hiding this comment.
Repair: the maintainer asked to include this fix here. Filed as #1076 (sync and aio copy) and fixed in 748e48be6eac69b8a05bfd5a92d49714ff85472d; the S3File writer case is filed separately as #1077.
- sync: the creation runs on the copy's executor; on an interrupt while waiting, the copy waits for it, aborts the created upload through
_abort_multipart_upload(), and re-raises. - aio: the creation is a task awaited through
asyncio.shield(); the Cancelling an async multipart copy leaves its multipart upload behind #1046_abort()first waits for it and returns early if no upload was created. The rest of the cleanup is unchanged.
Self-review of the repair:
- Behavior: normal creation errors still propagate unchanged; a creation that has not started is cancelled; with
max_workers=1the creation and the parts run in order; an aio cancellation before the creation starts still waits and aborts the upload. - Claims: the docstrings and the
docs/filesystem.mdsentence match. Not covered: an interrupt insideexecutor.submit()itself (microseconds) and the event-loop shutdown case from Abort a cancelled async multipart copy after its running parts #1056. Both are stated in the PR body.
Evidence:
- New offline tests (sync SIGINT; aio with a successful and a failing creation) fail without the fix and pass with it, also under
-n 8. - The offline suite matches master's failure set.
- Live: copy subset 83 passed.
- Live An interrupted multipart copy leaves the upload behind when CreateMultipartUpload is in flight #1076 check: a real CreateMultipartUpload was held for 1 s, then the sync copy got SIGINT and the aio copy was cancelled.
ListMultipartUploadswas empty afterwards for both.
The PR went back to Draft until the independent follow-up and CI pass.
There was a problem hiding this comment.
Independent follow-up of the #1076 fix (relayed): Codex gpt-6.1-sol, session 01a105ce-8fac-7260-900b-6effa94b0845, codex exec -s read-only on a detached snapshot, full review of 1d84d89532c3ebe11efb1ea83ece665b547e8050..748e48be. Static review.
Coverage (reviewer): the full five-file diff, the executor and abort contracts, cancellation before the creation task starts, a failing creation, repeated cancellation, exception retrieval, typing, the existing part/completion cleanup, docstrings and docs.
Result: FINDINGS.
- The sync test's fixed delays (0.1 s / 0.5 s) do not establish ordering, and
thread.join()was skipped ifpytest.raisesfailed. Repaired inaa90e742: the test wraps the creation future'sresult()to signal that the copy is waiting, sends SIGINT only then, and installs a SIGINT handler that releases the held creation. The join and the handler restore are infinally. It still fails without the fix (checked by reverse-applying the source change) and passes 6/6 serially and under-n 8. - The aio creation was scheduled before
asyncio.Semaphore(max_workers), outside the cleanup guard, so an invalidmax_workersleft the creation unobserved. Repaired inaa90e742: the creation is now scheduled right before thetry. Codex noted that the base also leaked a successful creation in this case, which the repair removes too.
There was a problem hiding this comment.
Narrow follow-up of 748e48be..aa90e742 (relayed): Codex gpt-6.1-sol, session 01a105d7-da99-7302-b53d-44bb40f79b6d, read-only snapshot at aa90e742. Static review. The aio scheduling move and the docs scoping match the intent.
Result: FINDINGS (test only).
- SIGINT could arrive before the worker started the creation; then
cancel()succeeds and no abort happens, so the assertion fails although the code is correct. - A late SIGINT during teardown could leave the test handler installed or reach a later test.
Repaired in 733cb483:
- The sender waits until the creation has started and the copy is waiting on it.
- The held creation returns only after the test handler has run (up to 30 s).
finallyswaps in an ignoring handler before it joins the sender, then restores the original handler.- The test fails without the fix (31 s, DID NOT RAISE) and passes repeatedly.
There was a problem hiding this comment.
Final follow-up of aa90e742..733cb483 (relayed): Codex gpt-6.1-sol, session 01a105df-20b4-7611-83c1-e8eb910a0f19, read-only snapshot at 733cb483. Static review.
Coverage (reviewer): worker-start and result-wait synchronization, the 30 s creation gate, detection of a synchronous creation or a missing abort, and the cleanup when the sender fails to start, when SIGINT arrives late, on the join, and on the handler restore. Result: CLEAN.
| ListObjectAnnotations unless the directives or the source exclude them, and writes | ||
| nothing. If HeadObject reports a size that fits in one CopyObject request, nothing else | ||
| is read, and the plan's `fits_single_request` says to copy with `copy_object()` | ||
| instead. `copy_object_annotation()` copies one annotation onto the destination after |
There was a problem hiding this comment.
Independent review (relayed): Claude claude-fable-5-1 (Agent tool, model fable, general-purpose, read-only instructions) on the same detached snapshot 2f7a891c77c088c0afb529c5cf5aca4aeff67e6a, diff 6d268b17d984adf2b16acadfd3e8fd447a03a376..2f7a891c77c088c0afb529c5cf5aca4aeff67e6a, with the same prompt. Static review.
Coverage (reviewer): s3_core.py (new plan, operations, private helpers; existing call/operation_params/multipart primitives), s3.py and s3_async.py copy paths including the unchanged _finish_multipart_upload/_abort_multipart_upload and the aio cancellation machinery, s3_path.py, s3_object.py, the exports, all changed tests (including a hand recomputation of the expected plan in test_plan_multipart_copy), tests/pyathena/util.py, docs/filesystem.md, docs/api/filesystem.rst, and a repo-wide grep for leftover helper references.
Result: CLEAN. Verified: request sequence and parameters vs base; create_params equivalence of operation_params("copy_object", ...); the union re-filter in sync yields exactly the per-operation params; precedence matches the replaced helpers; version pinning and the fits-single-request boundary are covered by Stubber tests that fail on stray requests; validation order unchanged; aio cancellation untouched; cache invalidation unchanged; docs consistent.
Non-blocking notes:
- This paragraph had a 164-column line. Repaired in
1d84d895(rewrapped, andmv()is now named withcp_file()andcopy(), verified ats3.py:1453). - Pre-existing and harmless: COPY metadata always sends
Metadata({}for none), as the base did. S3MultipartCopyPlan.sizeis informational; neither filesystem reads it.
Snapshot and PR worktree unchanged after both reviews (git status clean, snapshot HEAD 2f7a891c77c088c0afb529c5cf5aca4aeff67e6a).
There was a problem hiding this comment.
Repair 1d84d89532c3ebe11efb1ea83ece665b547e8050 (rewrap and naming mv()). Author check: both self-review perspectives applied to the hunk; the call paths were verified (mv() → _copy_file() at s3.py :1453, s3_async.py :380). just docs lint passed.
Independent follow-up (relayed): Claude claude-fable-5-1 on the same snapshot, reviewing only 2f7a891c77c088c0afb529c5cf5aca4aeff67e6a..1d84d89532c3ebe11efb1ea83ece665b547e8050. Result: CLEAN. Only the two intended hunks changed (identical under --ignore-all-space apart from the added mv()), the call paths in sync and aio match, the wording is consistent with docs/filesystem.md :236 and :285-286, and the lines are within the paragraph's width. Static review.
There was a problem hiding this comment.
Independent follow-up of the #1076 fix (relayed): Claude claude-fable-5-1 on the same snapshot, full review of 1d84d89532c3ebe11efb1ea83ece665b547e8050..748e48be. Static review.
Coverage (reviewer): both copy paths, _finish_multipart_upload/_abort_multipart_upload, shield semantics against CPython, typing, every sync test that mocks or stubs the creation (unchanged order under the executor), the new tests (including the xdist main-thread execution and the CI runners), and the docs.
Result: FINDINGS (docs only; no code regressions).
- The docs and the sync docstring said that the requests in flight finish first. In the sync path, CompleteMultipartUpload runs on the calling thread, so an interrupt abandons it and the abort follows at once (pre-existing behavior). Repaired in
aa90e742: the claim is now scoped to the creation and the part copies, plus the completion for anAioS3FileSystemcopy. - The creation was scheduled before the
Semaphore, the same as Codex's finding 2. Repaired as above.
After the repairs: lint passed; the offline suite matches master's failure set (691 passed); live copy subset 83 passed; the live #1076 check (a held real CreateMultipartUpload, then SIGINT or cancel) left no uploads; the live 10 MiB multipart copy completed in 2 parts in sync and aio.
There was a problem hiding this comment.
Narrow follow-up of 748e48be..aa90e742 (relayed): Claude claude-fable-5-1, same snapshot. Static review. The aio scheduling move, the sync docstring and the docs scoping are correct. The reviewer traced the test through submit, Future.result and Condition.wait, and found that no SIGINT or handler leaks in any path it traced.
Result: FINDINGS (test only). 1: the same residual race as Codex's finding 1, where the creation is still PENDING when SIGINT is handled. Nit: the handler was installed and the sender started outside the try.
Repaired in 733cb483: a started event gates the sender, as described in the Codex reply. thread.start() now runs inside the try, and join() runs only for a thread that has started. After the repairs, lint passed and the offline suite matches master's failure set.
There was a problem hiding this comment.
Final follow-up of aa90e742..733cb483 (relayed): Claude claude-fable-5-1, same snapshot. Static review.
Coverage (reviewer): the interrupt timing (the sender fires only when the creation is RUNNING and the copy is inside its try); the copy cannot finish before the interrupt; the test fails without the fix (DID NOT RAISE after the 30 s hold, and no SIGINT is sent); the handler and thread lifecycle, where a late SIGINT is consumed by the ignoring handler before the restore and only one signal is ever sent; the failure paths inside the try; the skip guard; the per-instance submit wrapper. Result: CLEAN.
Review complete for this PR; marking Ready after the offline checks pass on 733cb483.
…creation The sync copy now creates the upload on its executor and, on an interrupt while waiting, waits for the creation and aborts the upload that it created. The aio copy runs the creation as its own task behind asyncio.shield(), and the existing cleanup waits for it before deciding whether there is an upload to abort. Closes #1076. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… the interrupt test, and scope the wait claims Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… interrupt from reaching later tests Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WHAT
Sub-step 3 of #1063 (step 3 of #1053): the copy requests and the planning of a multipart copy move into
S3Core.Core (
pyathena/filesystem/s3_core.py, public):copy_object(source, destination, **params)sends CopyObject. As in the replaced_copy_object(), a parameter that repeats a request field raisesTypeError.plan_multipart_copy(source, destination, block_size=None, **params) -> S3MultipartCopyPlanmakes only the reads: HeadObject, GetObjectTagging and ListObjectAnnotations. It validatesblock_sizeand the directives, then returns a frozen plan with these fields:source, pinned to the version that HeadObject reports unless the path has one or it isnull;destination,size(from HeadObject) andranges;create_params,part_params,complete_paramsandabort_params, each filtered for its operation.create_paramscarries the metadata and tags that CopyObject would copy, plus the parameters that CopyObject would reject, so that botocore still rejects them;annotations, empty when the directive, an SSE-C source or a directory bucket excludes them;fits_single_request. When it is true, nothing after HeadObject is read, andranges, the four parameter fields andannotationsare empty.list_object_annotations(path, **params)returns the names across all pages.copy_object_annotation(name, source, destination, version_id, etag, **params)takes the CopyObject parameters of the copy. It maps them to the source's names for GetObjectAnnotation and filters them for PutObjectAnnotation.S3Core.Adapters:
_copy_file()(behindcp_file(),copy()andmv(), sync and aio) callscore.copy_object()directly for an object up to 5 GiB. The sync and aio_copy_object_with_multipart_upload()takeS3Paths, and each now only schedules a plan:_finish_multipart_upload(). The signature of_finish_multipart_upload()is unchanged; it receives the plan's completion and abort parameters, filters them again, and sends the same requests.asyncio.to_threadwithgather/wait. Thefailedflag, the semaphore,_cleanup_tasks, the shielded completion and the rule "abort unless the completion succeeded" are unchanged._copy_object_with_multipart_upload()now takesS3Paths instead ofbucket1/key1/size1/...):S3FileSystem._COPY_METADATA_PARAMS,_copy_object,_is_directory_bucket,_get_copy_source_kwargs,_get_multipart_copy_kwargs,_copies_annotations,_list_object_annotationsand_copy_object_annotation. The duplicated block-size validation in aio is gone as well.docs/filesystem.mddescribes the copy operations, and the API reference listsS3MultipartCopyPlan.Fix (#1076, release note): a multipart copy that is interrupted (sync,
KeyboardInterrupt) or cancelled (aio) while CreateMultipartUpload is in flight now aborts the upload that the request creates. It used to be left behind, incomplete._abort_multipart_upload()), and re-raises.asyncio.shield(). The existing Cancelling an async multipart copy leaves its multipart upload behind #1046 cleanup waits for it first and aborts only if it returned an upload. The rest of the cleanup (parts, completion, repeated cancellation) is unchanged.executor.submit()itself, before the future is returned (a window of microseconds while the worker thread starts), is not covered, nor is the event-loop shutdown case already listed in Abort a cancelled async multipart copy after its running parts #1056. TheS3Filewriter has the same window; it is tracked in An interrupted S3File write leaves the upload behind when CreateMultipartUpload is in flight #1077.Requests on the wire are unchanged. There is one behavior change, decided on #1063: a HeadObject response without
ContentLengthused to fall back to the size frominfo(). It now raisesValueError. S3 returnsContentLengthin every HeadObject response (theContent-Lengthheader), so only a stub or a non-conforming S3-compatible endpoint can reach this case.WHY
Closes #1076. Part of #1063.
#1063 / #1053: the typed core owns the S3 requests and the S3 rules. Scheduling, cache invalidation and fsspec semantics stay in the adapters. This is the third of the four PRs: delete (#1064), multipart primitives (#1069), copy (this PR), then pairing.
TEST
Tested commit: 733cb48 (lint and offline suite). The live runs used aa90e74; 733cb48 changes only one offline test after it. The refactor was also tested on 9187b6f and 2f7a891 with the same results. AWS CI passed on 1d84d89 before the #1076 fix was added.
just lintandjust docs lint: passed.pytest --noconftest -n 8 tests/pyathena/filesystemgave 691 passed, 119 failed. The failure set is identical toorigin/master6d268b1 (664 passed, 119 failed), and all 119 need AWS.test_s3_core.py:copy_objectwith and without a version;nulland a given version, at both sizes;copy_object_annotationwith the mapped source parameters, the destination version and the ETag.The
_get_multipart_copy_kwargsunknown-parameter test moved there.stub_multipart_copytests, the directive, SSE-C and directory-bucket tests, and the aio cancellation tests (Cancelling an async multipart copy leaves its multipart upload behind #1046). Their call shapes changed:S3Patharguments, andcore.copy_objectinstead of_copy_object. Their HeadObject stubs now report a size. The aio cancellation tests set a size of 3 or 2 parts of the default block size instead of relying on the missing-size fallback.test_copy_object_with_multipart_upload_interrupted_creation(sync) deliversSIGINTto the main thread while a mocked creation is in flight.test_copy_object_with_multipart_upload_cancelled_creation[False/True](aio) cancels while the creation is held, with a creation that succeeds and one that fails.All three fail without the fix (checked by reverse-applying the source change) and pass with it, also under
-n 8.The sync test has no timing assumptions. It sends SIGINT only once the creation is running and the copy is waiting for it, and the held creation returns only after the test's SIGINT handler has run. A late signal is ignored before the original handler is restored.
uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/test_s3.py tests/pyathena/filesystem/test_s3_async.py -k "cp_file or copy or mv or move or annotation"gave 83 passed on 748e48b and on aa90e74, including the 3 new offline tests (80 passed on 9187b6f and 2f7a891).SIGINTand the aio copy was cancelled. Afterwards,ListMultipartUploadsunder the test prefix was empty for both, and no destination object existed. None of these makes a multipart copy on S3; the suite has no live test of one.S3Core.MULTIPART_UPLOAD_MAX_PART_SIZEwas lowered to 5 MiB, and a 10 MiB object withContentType,Metadataand a tag was copied throughS3FileSystem.cp_file()andAioS3FileSystem._cp_file(). Both copies completed in 2 parts (ETag-2), with identical content and the source's content type, metadata and tags. The test objects were deleted afterwards. Run on 9187b6f, 2f7a891, 748e48b and aa90e74, with the same result. The source had no annotations: ListObjectAnnotations was sent (defaultCOPYdirective) and did not fail, but no annotation was copied, so the copy of an annotation was exercised only by the Stubber tests.🤖 Generated with Claude Code