Skip to content

Move the copy requests and the multipart copy plan into S3Core, and abort uploads interrupted during creation (step 3.3 of #1063) - #1074

Merged
laughingman7743 merged 9 commits into
masterfrom
refactor/1063-s3-core-copy
Oct 4, 2026
Merged

laughingman7743 merged 9 commits into
masterfrom
refactor/1063-s3-core-copy

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Oct 4, 2026 •

Copy link
Copy Markdown
Member

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 raises TypeError.
  • plan_multipart_copy(source, destination, block_size=None, **params) -> S3MultipartCopyPlan makes only the reads: HeadObject, GetObjectTagging and ListObjectAnnotations. It validates block_size and 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 is null;
    • destination, size (from HeadObject) and ranges;
    • create_params, part_params, complete_params and abort_params, each filtered for its operation. create_params carries 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, and ranges, the four parameter fields and annotations are 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.
  • The planner now owns the copy-source parameter mapping, the directory-bucket rule and the copied metadata fields. They are private on S3Core.

Adapters:

  • _copy_file() (behind cp_file(), copy() and mv(), sync and aio) calls core.copy_object() directly for an object up to 5 GiB. The sync and aio _copy_object_with_multipart_upload() take S3Paths, and each now only schedules a plan:
    • sync: the executor and _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.
    • aio: asyncio.to_thread with gather/wait. The failed flag, the semaphore, _cleanup_tasks, the shielded completion and the rule "abort unless the completion succeeded" are unchanged.
  • Removed private helpers (none is documented; the private _copy_object_with_multipart_upload() now takes S3Paths instead of bucket1/key1/size1/...): S3FileSystem._COPY_METADATA_PARAMS, _copy_object, _is_directory_bucket, _get_copy_source_kwargs, _get_multipart_copy_kwargs, _copies_annotations, _list_object_annotations and _copy_object_annotation. The duplicated block-size validation in aio is gone as well.
  • Docs: docs/filesystem.md describes the copy operations, and the API reference lists S3MultipartCopyPlan.

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.

Requests on the wire are unchanged. There is one behavior change, decided on #1063: a HeadObject response without ContentLength used to fall back to the size from info(). It now raises ValueError. S3 returns ContentLength in every HeadObject response (the Content-Length header), 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 lint and just docs lint: passed.
  • Offline, with dummy credentials: pytest --noconftest -n 8 tests/pyathena/filesystem gave 691 passed, 119 failed. The failure set is identical to origin/master 6d268b1 (664 passed, 119 failed), and all 119 need AWS.
  • New Stubber tests in test_s3_core.py:
    • copy_object with and without a version;
    • keyless paths and versioned destinations for every new operation;
    • a full plan with metadata, tags, two annotation pages and the filtered parameters of each operation;
    • version pinning, including null and a given version, at both sizes;
    • the single-request fit and a HeadObject without a size;
    • parameters that CopyObject would reject;
    • the annotation paging and the request precedence;
    • copy_object_annotation with the mapped source parameters, the destination version and the ETag.
      The _get_multipart_copy_kwargs unknown-parameter test moved there.
  • The existing sync and aio copy tests still pass, including the wire-level stub_multipart_copy tests, 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: S3Path arguments, and core.copy_object instead 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.
  • An interrupted multipart copy leaves the upload behind when CreateMultipartUpload is in flight #1076 regression tests:
    • test_copy_object_with_multipart_upload_interrupted_creation (sync) delivers SIGINT to 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.
  • Live AWS: 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).
  • Live An interrupted multipart copy leaves the upload behind when CreateMultipartUpload is in flight #1076 check, with a temporary script: a real CreateMultipartUpload was made, then held for one second before it returned. During that second, the sync copy got SIGINT and the aio copy was cancelled. Afterwards, ListMultipartUploads under 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.
  • Live multipart copy, with a temporary script outside the repository: S3Core.MULTIPART_UPLOAD_MAX_PART_SIZE was lowered to 5 MiB, and a 10 MiB object with ContentType, Metadata and a tag was copied through S3FileSystem.cp_file() and AioS3FileSystem._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 (default COPY directive) and did not fail, but no annotation was copied, so the copy of an annotation was exercised only by the Stubber tests.
  • Not run locally: the full AWS suite, which runs in CI once the PR is Ready.

🤖 Generated with Claude Code

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>
@laughingman7743 laughingman7743 added this to the 4.0.0 milestone Oct 4, 2026
laughingman7743 and others added 3 commits October 4, 2026 15:20
…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(

@laughingman7743 laughingman7743 Oct 4, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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 (null not pinned, given version kept) and the HEAD-size re-check are covered by test_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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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":

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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_params now 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.

Comment thread pyathena/filesystem/s3.py
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},

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Round 1 /code-review + /simplify dispositions:

  • All four simplify angles and code-review suggested request_kwargs=kwargs here. Kept as decided on Move copy, multipart and delete requests into S3Core (step 3 of #1053) #1063 (_finish_multipart_upload signature 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_request as a property / _is_directory_bucket on S3Path: not taken; the plan fields, the CopyObject parameters of copy_object_annotation and 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= reaching copy_object/plan_multipart_copy) now raise TypeError instead of botocore's ParamValidationError; name/version_id/etag are 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(

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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>
Comment thread docs/filesystem.md Outdated
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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Self-review round 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 includes fits_single_request) → now names ranges, the parameter fields and annotations.
  • 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() call copy_object(); it is _copy_file(), behind cp_file(), copy() (fsspec → cp_file) and mv() (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:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Round 2, callers and operators:

  • Existing callers: public methods unchanged; cp_file/copy/mv signatures and defaults unchanged. The only caller-visible change is this ValueError for a HeadObject without ContentLength (decided on Move copy, multipart and delete requests into S3Core (step 3 of #1053) #1063; S3 sets it from the Content-Length header). Private _copy_object_with_multipart_upload() now takes S3Paths; nothing outside pyathena/filesystem calls it.
  • AWS operator: each request still goes through S3Core.call with the same RetryConfig, so attempts and elapsed-time bounds per request are unchanged; the request count per copy is unchanged; aio makes one to_thread hop for the reads instead of two.
  • Adversarial: a source replaced between HEAD and the part copies is still read from the pinned version; a null version 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>
Comment thread pyathena/filesystem/s3_async.py Outdated
)
multipart_upload = await asyncio.to_thread(
self.core.create_multipart_upload, destination, **create_kwargs
self.core.create_multipart_upload, plan.destination, **plan.create_params

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Independent review (relayed): 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 base s3_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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Independent follow-up (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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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=1 the 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.md sentence match. Not covered: an interrupt inside executor.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:

The PR went back to Draft until the independent follow-up and CI pass.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Independent follow-up 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.

  1. The sync test's fixed delays (0.1 s / 0.5 s) do not establish ordering, and thread.join() was skipped if pytest.raises failed. Repaired in aa90e742: the test wraps the creation future's result() 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 in finally. It still fails without the fix (checked by reverse-applying the source change) and passes 6/6 serially and under -n 8.
  2. The aio creation was scheduled before asyncio.Semaphore(max_workers), outside the cleanup guard, so an invalid max_workers left the creation unobserved. Repaired in aa90e742: the creation is now scheduled right before the try. Codex noted that the base also leaked a successful creation in this case, which the repair removes too.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

  1. 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.
  2. 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).
  • finally swaps 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Comment thread docs/filesystem.md
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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Independent review (relayed): 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:

  1. This paragraph had a 164-column line. Repaired in 1d84d895 (rewrapped, and mv() is now named with cp_file() and copy(), verified at s3.py :1453).
  2. Pre-existing and harmless: COPY metadata always sends Metadata ({} for none), as the base did.
  3. S3MultipartCopyPlan.size is informational; neither filesystem reads it.

Snapshot and PR worktree unchanged after both reviews (git status clean, snapshot HEAD 2f7a891c77c088c0afb529c5cf5aca4aeff67e6a).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Independent follow-up 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).

  1. 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 an AioS3FileSystem copy.
  2. 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@laughingman7743
laughingman7743 marked this pull request as ready for review October 4, 2026 06:55
@laughingman7743
laughingman7743 marked this pull request as draft October 4, 2026 07:23
…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>
@laughingman7743 laughingman7743 changed the title Move the copy requests and the multipart copy plan into S3Core (step 3.3 of #1063) Move the copy requests and the multipart copy plan into S3Core, and abort uploads interrupted during creation (step 3.3 of #1063) Oct 4, 2026
laughingman7743 and others added 2 commits October 4, 2026 16:34
… 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>
@laughingman7743
laughingman7743 marked this pull request as ready for review October 4, 2026 07:48
@laughingman7743
laughingman7743 merged commit 2007620 into master Oct 4, 2026
14 checks passed
@laughingman7743
laughingman7743 deleted the refactor/1063-s3-core-copy branch October 4, 2026 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

An interrupted multipart copy leaves the upload behind when CreateMultipartUpload is in flight

1 participant