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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
laughingman7743 merged 9 commits into from
Oct 4, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9187b6f
Move the copy requests and the multipart copy plan into S3Core
laughingman7743 Oct 4, 2026
3a184ae
State which core operations send more than one request, and the missi…
laughingman7743 Oct 4, 2026
567b216
Correct which plan fields are empty and split the docs exception into…
laughingman7743 Oct 4, 2026
3e6d7c5
Reuse S3Path.with_version_id, operation_params and the shared copy fi…
laughingman7743 Oct 4, 2026
2f7a891
Say in the docs that a source whose HeadObject size fits one CopyObje…
laughingman7743 Oct 4, 2026
1d84d89
Wrap the copy paragraph of the docs and name mv() among its callers
laughingman7743 Oct 4, 2026
748e48b
Abort a multipart copy's upload when an interrupt arrives during its …
laughingman7743 Oct 4, 2026
aa90e74
Schedule the aio creation right before its cleanup guard, synchronize…
laughingman7743 Oct 4, 2026
733cb48
Interrupt the creation in its test only once it runs, and keep a late…
laughingman7743 Oct 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/api/filesystem.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,9 @@ S3 Core
.. autoclass:: pyathena.filesystem.s3_core.S3DeleteError
:members:

.. autoclass:: pyathena.filesystem.s3_core.S3MultipartCopyPlan
:members:

S3 Objects
----------

Expand Down
29 changes: 23 additions & 6 deletions docs/filesystem.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,10 @@ from that version, which needs `s3:GetObjectVersion` and, to copy the tags,
`s3:GetObjectVersionTagging` on the source. A `null` version is not pinned. The annotations are listed before anything is written and copied after the
upload completes, so the destination exists without them until the last one is
written. If an annotation fails to copy, the error is raised and the destination is
kept. A failed part copy aborts the multipart upload.
kept. A failed part copy aborts the multipart upload. So does an interrupt, or the
cancellation of an `AioS3FileSystem` copy, unless the upload has already completed. A
CreateMultipartUpload request and the part copies in flight finish first, and so does
the CompleteMultipartUpload request of an `AioS3FileSystem` copy.

Paths are normalized as in fsspec, which drops a trailing slash, so `info`, `isfile`,
and `open` treat `s3://YOUR_S3_BUCKET/dir/` as `s3://YOUR_S3_BUCKET/dir`: the object
Expand Down Expand Up @@ -280,12 +283,13 @@ directories below the bucket level) and is always a no-op.
## Typed S3 operations

`S3FileSystem.core` is an `S3Core`, the typed operations that the filesystem sends
its listing, lookup, delete and multipart upload requests with. It can also be built
on a boto3 S3 client. Each operation sends one request (one per page for the
iterators) with the retry policy, raises `FileNotFoundError` for a missing bucket or
its listing, lookup, delete, multipart upload and copy requests with. It can also be
built on a boto3 S3 client. Each operation sends one request (one per page for the
iterators and `list_object_annotations()`); `plan_multipart_copy()` and
`copy_object_annotation()`, described below, send several. The requests are sent with
the retry policy. An operation raises `FileNotFoundError` for a missing bucket or
multipart upload, or for a missing object or version that it reads, and caches
nothing. Requests sent
through `fs.core` do not invalidate the filesystem's cache: call
nothing. Requests sent through `fs.core` do not invalidate the filesystem's cache: call
`fs.invalidate_cache()` after a change, or make it through the filesystem.

```python
Expand Down Expand Up @@ -331,6 +335,19 @@ ranges of the parts that copy it, by the part limits `MULTIPART_UPLOAD_MIN_PART_
`MULTIPART_UPLOAD_MAX_PART_SIZE` (5 GiB) and `MULTIPART_UPLOAD_MAX_PARTS` (10,000) of
`S3Core`.

`copy_object()` copies an object with one CopyObject request, which accepts objects
up to `MULTIPART_UPLOAD_MAX_PART_SIZE`. For a larger object, `plan_multipart_copy()`
reads the source and returns an `S3MultipartCopyPlan`: the version to copy, the byte
ranges of the parts, the parameters of each multipart upload request, and the
annotations to copy, so that the multipart upload writes the metadata, tags and
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

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

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

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

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.

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

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.

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

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.

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

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.

the upload completes, with GetObjectAnnotation and PutObjectAnnotation. The
filesystems' `cp_file()`, `copy()` and `mv()` run these plans.

## Async filesystem

`AioS3FileSystem` provides the same functionality on top of fsspec's
Expand Down
Loading
Loading