Skip to content

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

Open
laughingman7743 wants to merge 3 commits into
masterfrom
docs/copy-delete-marker-source
Open

laughingman7743 wants to merge 3 commits into
masterfrom
docs/copy-delete-marker-source

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Oct 26, 2026 •

Copy link
Copy Markdown
Member

WHAT

Documents how a copy whose source names a delete-marker version fails. Before this change, the S3Core docstring and docs/filesystem.md implied FileNotFoundError for it.

  • S3Core class docstring: copy_object() and upload_part_copy() with a source version that is a delete marker raise OSError with EINVAL.
  • S3Core.copy_object() and S3Core.upload_part_copy() Raises: FileNotFoundError for a missing bucket, upload, source object or source version, and OSError (EINVAL) for an invalid request, such as a source version that is a delete marker.
  • docs/filesystem.md (S3Core section): the same for copy_object() and upload_part_copy(), and that the filesystem's cp_file() raises FileNotFoundError for it, as for a missing file. copy() and mv() are not named. With recursive=True, expand_path() drops a version path that does not exist (s3.py:934), so FileNotFoundError is raised only when nothing else matches, and fsspec's copy() defaults to on_error="ignore". With one delete-marker source, both raise FileNotFoundError with or without recursive (checked offline with a scripted S3Core).

No code change. The 400 response carries neither the x-amz-delete-marker header nor an error code specific to delete markers; only its message identifies the case, so it is documented rather than translated.

WHY

Closes #1346. Follow-up of #1338 (part of #1327).

TEST

Tested at d3ae3a9.

  • Live probe on a temporary versioned bucket in us-east-1 (--profile pyathena), at 1a6a0d6; the bucket and every version were deleted afterwards:
    • CopyObject and UploadPartCopy with CopySource.VersionId = a delete marker: 400 InvalidRequest, no x-amz-delete-marker header, "The source of a copy request may not specifically refer to a delete marker by version id." S3Core.copy_object() raised OSError [Errno 22].
    • CopyObject with a permanently removed version ID: 404 NoSuchVersion. CopyObject without a version ID when the current version is a delete marker: 404 NoSuchKey. Both are translated to FileNotFoundError.
    • A second probe on 10/27 covered UploadPartCopy and CopyObject: a permanently removed source version gives 404 NoSuchVersion, a missing source key 404 NoSuchKey, and a key whose current version is a delete marker 404 NoSuchKey with x-amz-delete-marker: true. All three translate to FileNotFoundError. A malformed version ID gives 400 (InvalidRequest for CopyObject), which translates to EINVAL.
    • S3FileSystem.cp_file() of the delete-marker version raised FileNotFoundError from its info() lookup. The aio _copy_file looks up the source with _info() in the same way (s3_async.py:576); that path was read in the code but not run live.
  • just lint: passed. just docs lint: 0 errors.
  • uv run sphinx-build docs <tmp> of the working tree: at d3ae3a9 the new text renders in filesystem.html and api/filesystem.html. 173 warnings, the same count as origin/master. just docs build (sphinx-multiversion) builds only committed refs, so it does not show uncommitted changes.

🤖 Generated with Claude Code

laughingman7743 and others added 3 commits October 27, 2026 00:23
…EINVAL) (GH-1346)

CopyObject and UploadPartCopy whose source names a delete-marker version
are answered with 400 InvalidRequest, without the x-amz-delete-marker
header, so S3Core.copy_object() and upload_part_copy() raise OSError with
EINVAL rather than FileNotFoundError. The filesystem's copies look up the
source first and raise FileNotFoundError.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
plan_multipart_copy() looks up the source with HeadObject first and
raises FileNotFoundError for a delete-marker version, so the class
docstring names copy_object() and upload_part_copy() instead of every
copy.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- The filesystem paragraph names cp_file() only: copy() and mv() follow
  fsspec, which skips a missing source with on_error="ignore" (the
  recursive default) and drops a missing version from expand_path().
- copy_object() and upload_part_copy() list FileNotFoundError for every
  missing resource they can report, measured for a removed source version
  and a missing source key, and EINVAL for an invalid request, which a
  malformed version ID also gives.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread docs/filesystem.md
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
filesystem's `cp_file()` raises `FileNotFoundError` for it, as for a missing file.

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.

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`

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.

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,

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.

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,

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.

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

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.

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

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.

@laughingman7743
laughingman7743 marked this pull request as ready for review October 26, 2026 15:35

This branch has not been deployed

No deployments
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.

Document that S3Core copies of a delete-marker source version raise OSError(EINVAL)

1 participant