Skip to content

Document that copies of a delete-marker source version raise OSError(EINVAL) (GH-1346) #1348

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

Open
laughingman7743 wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Document that copies of a delete-marker source version raise OSError(EINVAL) (GH-1346) #1348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5addecd
Document that copies of a delete-marker source version raise OSError(…
laughingman7743 Oct 26, 2026
247bf10
Name the two copy operations in the S3Core docstring (GH-1346)
laughingman7743 Oct 26, 2026
d3ae3a9
Narrow the copy docs to what the copies raise (GH-1346)
laughingman7743 Oct 26, 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
6 changes: 4 additions & 2 deletions docs/filesystem.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,8 +399,10 @@ operation sends one request (one per page for the iterators and
described below, send several, and `generate_presigned_url()` signs a URL locally
without a request. 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
version that it reads, and caches nothing. `copy_object()` and `upload_part_copy()` with
a source version that is a delete marker raise `OSError` with `EINVAL`, while the

@laughingman7743 laughingman7743 Oct 26, 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 two (claims, documentation reader, existing callers, evidence), base 1a6a0d6c3f137d47bbe96e274ecd467e016fcb38, head d3ae3a9ce8b101673332a7af18bc68d49af5ea98. Result: FINDINGS (PR description wording), corrected. No change to tracked files.

Claims checked:

  1. CopyObject and UploadPartCopy with a delete-marker source version get 400 InvalidRequest without the header. Measured live on 10/27 (temporary versioned bucket, deleted afterwards), and S3Core.copy_object() raised OSError [Errno 22] at 1a6a0d6.
  2. FileNotFoundError for a missing bucket, upload, source object or source version. Measured for both operations: removed source version 404 NoSuchVersion, missing source key 404 NoSuchKey. The missing upload and bucket follow the existing NoSuchUpload/NoSuchBucket mappings (s3_errors.py).
  3. EINVAL for an invalid request, such as a delete-marker source. A generic 400 maps to EINVAL (_HTTP_STATUS_CODE_TO_ERRNO, pinned by test_os_error_by_http_status_code[400]). A malformed version ID gives the same (measured CopyObject InvalidRequest), which is why the entry no longer reads as identifying a delete marker.
  4. The filesystem's cp_file() raises FileNotFoundError for it. Measured live (sync). The aio _copy_file calls _info() first (s3_async.py:576); read in the code only.
  5. Why copy()/mv() are not named (PR body). The first wording attributed both to fsspec. mv() is PyAthena's own (s3.py:1356), and the expand_path() drop applies only with recursive=True (s3.py:934). Corrected. I checked offline with a scripted S3Core: one delete-marker source raises FileNotFoundError from copy/mv with and without recursive. With several sources it is dropped silently.
  6. The docs render, with no new warnings. uv run sphinx-build docs <tmp> at d3ae3a9: 173 warnings, the same as origin/master. just docs build builds committed refs only. just docs lint: 0 errors.

Documentation reader: the error-translation table (docs/filesystem.md:239-251) and the S3ClientError docstring are unaffected, since the 400 is not a PyAthena-specific translation. No other page states copy errors.
Existing callers: no behavior change, since the PR touches only docstrings and Markdown.

filesystem's `cp_file()` raises `FileNotFoundError` for it, as for a missing file.

@laughingman7743 laughingman7743 Oct 26, 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 one (behavior of the documented paths, simplicity), base 1a6a0d6c3f137d47bbe96e274ecd467e016fcb38. The initial pass covered heads 5addecda926178912127e3b6049cfbe5f8e91209 and 247bf10687ffcaca9f41d37350fea88cd78f8383 (my own pass, /code-review high, /simplify). Result: FINDINGS, repaired in 247bf106 and d3ae3a9c.

Finding 1 (docs/filesystem.md:403 at 247bf10). The paragraph said that copy() and mv() raise FileNotFoundError for a delete-marker source version. They do not always:

  • fsspec's copy() uses on_error="ignore" by default with recursive=True, so the source is skipped without an error.
  • expand_path() keeps a version path only if exists() (s3.py around 934, s3_async.py around 825), so with several sources the delete-marker version is dropped.

Repair: only cp_file() is named ("as for a missing file"). copy() and mv() then follow fsspec's missing-source handling. /simplify's wording suggestion is folded in.

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
12 changes: 11 additions & 1 deletion pyathena/filesystem/s3_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,9 @@ class S3Core:
:class:`~pyathena.filesystem.s3_errors.S3ClientError`): a missing bucket
or multipart upload, or a missing object or version that an operation
reads, raises ``FileNotFoundError``, and a denied request
``PermissionError``.
``PermissionError``. :meth:`copy_object` and :meth:`upload_part_copy`

@laughingman7743 laughingman7743 Oct 26, 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.

Round one, finding 2 (my pass, at 5addecd). The class docstring said "a copy whose source names a delete-marker version" raises EINVAL. plan_multipart_copy() looks up the source with HeadObject first and raises FileNotFoundError instead. Repair (247bf10): the docstring names copy_object and upload_part_copy. In d3ae3a9c the trailing "as S3 rejects the request as invalid" was cut as a restatement (/simplify).

Not taken: removing the sentence and relying on the method Raises sections (/simplify, altitude). The class docstring's general "a missing version that an operation reads raises FileNotFoundError" is the statement #1346 found contradicted, so it keeps the exception next to it.

with a source version that is a delete marker raise ``OSError`` with
``EINVAL``.
As in S3, deleting a missing key is not an error. Nothing is cached.

Example:
Expand Down Expand Up @@ -1019,6 +1021,10 @@ def upload_part_copy(
Raises:
ValueError: If the upload has no bucket, key, or upload ID, or the
source has no key.
FileNotFoundError: If the upload, or the source object or version,

@laughingman7743 laughingman7743 Oct 26, 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.

Round one, finding 3. upload_part_copy gained the EINVAL entry but no FileNotFoundError entry, unlike copy_object (altitude, /code-review). Before adding it, a live probe on a temporary versioned bucket (deleted afterwards) measured UploadPartCopy: removed source version 404 NoSuchVersion, missing source key 404 NoSuchKey. Repair: FileNotFoundError: If the upload, or the source object or version, does not exist. The missing upload follows the existing NoSuchUpload mapping.

does not exist.
OSError: With ``EINVAL`` for an invalid request, such as a source
version that is a delete marker.
"""
request = self._multipart_upload_request(upload)
if not source.key:
Expand Down Expand Up @@ -1176,6 +1182,10 @@ def copy_object(self, source: S3Path, destination: S3Path, **params) -> None:
Raises:
ValueError: If the source or the destination has no key, or the
destination has a version ID, which a write cannot replace.
FileNotFoundError: If a bucket, or the source object or version,

@laughingman7743 laughingman7743 Oct 26, 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.

Round one, finding 4 (/code-review). Two inaccuracies in the new copy_object Raises entries:

  • FileNotFoundError named only the source, but a missing destination bucket (NoSuchBucket) maps to it too.
  • The EINVAL entry read as if it identified a delete marker, but a malformed version ID gives the same 400 → EINVAL (measured: CopyObject InvalidRequest).

Repair: "If a bucket, or the source object or version, does not exist", and "With EINVAL for an invalid request, such as a source version that is a delete marker". upload_part_copy was changed the same way.

Deferred or rejected:

  • cp_file's Raises lacks FileNotFoundError: this predates the PR, and the new prose no longer claims more than cp_file already does.
  • An error-table row for the 400 (reuse, altitude): the table lists PyAthena's own translations, and this case falls under "Others".
  • copy_object_annotation: not listed, since it uses GetObjectAnnotation and is unmeasured.
  • A test of the documented contract: the generic 400 → EINVAL is pinned by test_os_error_by_http_status_code[400]. A message-specific translation was declined in Document that S3Core copies of a delete-marker source version raise OSError(EINVAL) #1346.

does not exist.
OSError: With ``EINVAL`` for an invalid request, such as a source
version that is a delete marker.
"""

@laughingman7743 laughingman7743 Oct 26, 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 result), static only.

  • Reviewer: Codex CLI 0.164.0, model gpt-6-astra, reasoning effort max, codex exec --sandbox read-only. It did not author the change. Session 01a178d5-cee4-77e3-a1f5-a6c4283b97f4.
  • Scope: base 1a6a0d6c3f137d47bbe96e274ecd467e016fcb38, head d3ae3a9ce8b101673332a7af18bc68d49af5ea98, run on a detached snapshot of the head. The prompt supplied the measured S3 facts as evidence, without the PR description, commit messages, or prior findings. The reviewer was not allowed to edit, build, run tests, or use GitHub or the network.
  • Covered: the full diff (class docstring, both copy-method Raises sections, the user-docs paragraph); s3_errors.py translation; sync lookup, cp_file, _copy_file, copy, mv and expand_path; aio _copy_file; the surrounding filesystem docs, the error translation table, and the Google-style docstring rules.

Result: CLEAN. No findings were introduced, and no pre-existing actionable issue was found in the reviewed context. The reviewer reports that:

  • Both core copy methods raise OSError(EINVAL) for a delete-marker source version, and cp_file() raises FileNotFoundError from its lookup, as does the aio path.
  • Missing keys, removed versions, and an unversioned key whose current version is a delete marker remain FileNotFoundError cases. The malformed-version result fits the "invalid request" wording.
  • The 405 row of the table does not contradict the copies' 400.
  • The added prose states behavior without rationale asides and does not overpromise for copy()/mv().

The snapshot and the PR worktree were unchanged after the review.

if not source.key:
raise ValueError(f"The source has no key: {source.uri}.")
Expand Down
Loading