Skip to content

Read a negative cat_file() start without an end as a suffix range - #998

Merged
laughingman7743 merged 4 commits into
masterfrom
fix/994-suffix-range
Oct 3, 2026
Merged

laughingman7743 merged 4 commits into
masterfrom
fix/994-suffix-range

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

WHAT

  • S3FileSystem.cat_file() sends a negative start without an end as a suffix range (bytes=-N) instead of resolving it against the size from info().
    S3 returns the last N bytes, or the whole object when it is shorter, so no info() lookup is needed. s3fs sends the same range.
    Other negative offsets (a negative end, or a negative start with an end) are still resolved against info(), as before.
  • cat_file() raises FileNotFoundError for a path without a key (s3://bucket) before any request.
  • When cat_file() resolves a range with info() (a negative end, a negative start with an end, or an empty range), it raises FileNotFoundError unless info() returns a file with the same key. info() of dir/ describes dir, whose size must not be used for dir/.
  • S3File._format_ranges() formats (start, None) with a negative start as bytes=-N.
  • docs/filesystem.md states how keys ending in / behave:
    • info, isfile and open normalize the trailing slash: dir/ is the object dir if it exists, and otherwise the directory dir. Reading dir/ reads dir or raises FileNotFoundError, and writing dir/ writes dir.
    • A ?versionId= suffix keeps the slash and refers to the object, for cat_file ranges too.
    • find, and ls of the directory, list the object as a file.
    • cat_file uses the key as written, and the doc lists the ranges that read it.

Behavior changes for the release notes:

  • cat_file(path, start=-N) (and cat_ranges() with such a range) no longer calls info().
    It reads the current object even when the cached size is stale, and it reads an object whose key ends in /. Before, it returned the whole object for such a key, and with Clamp byte ranges to the object in cat_file() and S3File reads #987, FileNotFoundError.
    A prefix or missing key raises FileNotFoundError from GetObject, as before.
  • cat_file("s3://bucket") raises FileNotFoundError instead of botocore's ParamValidationError.
  • For a key ending in /, ranges resolved with info() raise FileNotFoundError. Before, when an object without the slash also existed, they used that object's size and could return wrong bytes.

WHY

Fixes #994.
Maintainer decision for #994: keep keys ending in / as directories, following fsspec and s3fs, and consistent with #989 (#981) and #990 (#974). Make the minimal change: read start=-N with a suffix range as s3fs does, and document the limitation. ls() is unchanged; it lists such keys as files, as s3fs does.

TEST

Tested commit: fbd2d0b (Python 3.13.1, fsspec 2026.9.0), based on 92c9e3e6. The head da5dedc changes only the docs (just docs lint passed).

  • just lint, just docs lint: passed. just docs build: succeeded with no warnings from docs/filesystem.md.
  • uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/: 378 passed.
  • Offline tests in TestS3FileSystem. The GetObject fake now answers suffix ranges like S3.
    • test_cat_file_range adds (-10, None) and (-100, None), and asserts Range=bytes=-N with no info() call for a negative start without an end.
    • test_cat_file_range_stale_size adds start=-5 with a stale cached size.
    • New test_cat_file_suffix_range_key_ending_in_slash: cat_file("s3://bucket/dir/", start=-2) reads b"bc" with Key="dir/" while info() reports a directory.
    • test_cat_file_range_directory replaces (-5, None) with (-5, 3), which still goes through info().
    • test_format_ranges adds (-8, None).
    • New test_cat_file_bucket: a bucket path raises FileNotFoundError without a request, with and without a range.
    • New test_cat_file_range_key_ending_in_slash: (-2, -1), (0, -1) and (5, 5) on dir/ raise when info() returns the file dir.
    • 6 of these fail on master (ae2d2a04).
  • Live S3:
    • Raw GetObject: bytes=-2 on a 10-byte object returned 206 b"89"; bytes=-10 and bytes=-100 returned the whole object; bytes=-5 on a 0-byte object returned 200 b""; bytes=-2 on a key slash/ returned b"bc"; a missing key returned NoSuchKey.
    • On this branch, for keys only/ and slash/ (with slash/child): cat_file(start=-2) returned b"bc"; cat_file() and [0:1] read the object; (-2, 3), (0, -1), (5, 5) and open() raised FileNotFoundError; find and ls("…/slash/") listed the key as a file, and ls of the parent listed only and slash as directories. The docs paragraph states these results.
    • The S3File reads and cat_file() return the whole object for empty or out-of-bounds ranges #970 range cases still return slice results.
    • With both (10 bytes) and both/ (b"abc"), cat_file("…/both/", -2, -1) and (5, 5) raise FileNotFoundError (master: b""). A bucket path raises FileNotFoundError with and without a range. open("…/w/", "wb") writes w. …/only/?versionId=null: info = file, isfile = True, open reads b"abc", cat_file(0, -1) = b"ab".

🤖 Generated with Claude Code

laughingman7743 and others added 2 commits October 3, 2026 18:24
cat_file(path, start=-N) resolved the offset against the size from
info(), which reports a key ending in "/" as a directory of size 0 and
may serve a stale cached size. A negative start without an end is now
sent as a suffix range (bytes=-N), which S3 resolves without the size,
as s3fs does. The filesystem docs state how keys ending in "/" behave:
they stay directories, following fsspec's path normalization.

Closes #994.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread docs/filesystem.md Outdated
`s3://YOUR_S3_BUCKET/dir` are the same path. An object whose key ends in a slash, such
as a folder marker, is therefore a directory for `info`, `isfile`, and `open`, and
`open` raises `FileNotFoundError` for it. `find`, and `ls` of that directory, list the
object as a file entry. `cat_file` reads it without a range, with a non-empty range of

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 (implementation behavior). Base 92c9e3e67180eb3d52ef7b85579a24540a30fd64, head 91d74a700872a9e1086f0d9a7a982d17015c8387. The branch was rebased onto #988 before this round; #988 does not touch cat_file() or _format_ranges(). Result: FINDINGS (1, repaired in 91d74a7).

Covered:

  • The cat_file() branch for a negative start with no end. It skips info() and the directory check, and the GetObject error is still translated: a prefix or missing key raises FileNotFoundError from NoSuchKey.
  • The other negative and empty-range paths are unchanged.
  • InvalidRange handling: a suffix range on a 0-byte object returns 200 b"" (live), so the handler is not involved.
  • Version IDs are passed through.
  • _format_ranges() callers: _get_object(), and the copy ranges, which are always non-negative.
  • _get_object()'s empty-range guard (end is None) and its return value: the start is used only by _fetch_range(), which never sends suffix ranges.
  • Async delegation and cat_ranges() via cat_file().
  • Tests: the fake implements S3's suffix semantics (data[-N:], the whole object when shorter), and 6 cases fail on master.

Finding: the docs said "ls and find list it as a file entry". Live, ls of the parent lists only/ as the directory only; only ls of the directory itself and find return the only/ file entry. Repaired in the docs and the PR body.

Validation on 91d74a7:

  • just lint and just docs lint: passed.
  • uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/: 372 passed.

Comment thread pyathena/filesystem/s3.py
# S3 would return the whole object for an empty range.
return b""
ranges = (start, end)
if start is not None and start < 0 and end 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.

Self-review round two (claims, callers, operations). Base 92c9e3e67180eb3d52ef7b85579a24540a30fd64, head 91d74a700872a9e1086f0d9a7a982d17015c8387. Result: CLEAN.

Claims checked:

  • "S3 returns the last N bytes, or the whole object when it is shorter": live raw GetObject returned bytes=-2 → 206 b"89", bytes=-10 and bytes=-100 → the whole object, bytes=-5 on a 0-byte object → 200 b"", and a missing key → NoSuchKey 404.
  • "s3fs sends the same range": s3fs 2026.9.0 _process_limits sets start="" and end=-start for a negative start with no end (bytes=-N). Live, s3fs read b"bc" from slash/.
  • The docs paragraph: on live S3, info reports a directory, isfile is False, and open raises FileNotFoundError. find and ls of the directory list the key as a file. cat_file reads it without a range, with [0:1] and with start=-2, and raises for (-2, 3), (0, -1) and (5, 5).
  • The release-note claim that this used to return the whole object (and, with Clamp byte ranges to the object in cat_file() and S3File reads #987, FileNotFoundError): measured in the Objects whose keys end in '/' are listed as files but cannot be opened or read with negative offsets #994 reproduction on aa0fc914 and 02f8ebdd.

Callers and operations: a negative start without an end now sends one GetObject instead of info() (HeadObject, or ListObjectsV2 for a missing key, unless cached) plus GetObject. Requests decrease, retries are unchanged, and a stale cached size no longer affects the result.

A bucket path sent Key=None to GetObject, so the suffix range raised
botocore's ParamValidationError instead of FileNotFoundError, as did a
read without a range before. info() of a key ending in "/" describes the
key without the slash, so a range resolved against it could use another
object's size; such ranges now raise FileNotFoundError. The docs state
which methods normalize the trailing slash and how cat_file() reads the
key as written.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread pyathena/filesystem/s3.py
# empty range sends no GetObject request that would report a
# missing object.
info = self.info(path, version_id=version_id)
if info.get("type") == S3ObjectType.S3_OBJECT_TYPE_DIRECTORY or info.key != key:

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 0.160.0, model gpt-6-astra, reasoning effort max, codex exec -s read-only, session 01a10116-8794-7263-8855-6c44ece03ab5. It ran on a detached snapshot of head 91d74a700872a9e1086f0d9a7a982d17015c8387 against base 92c9e3e67180eb3d52ef7b85579a24540a30fd64. The prompt contained the literal diff and the repository conventions, without PR text, commit messages or prior findings. This is a static review. Afterwards, the snapshot and the PR worktree were unchanged. Result: FINDINGS.

Reviewer output, verbatim:

Covered the exact range, cat_file, inherited cat_ranges, async forwarding, _get_object, buffered reads, multipart-copy callers, version IDs, prefixes, missing/empty objects, slash-ending keys, docstrings, filesystem documentation, and relevant tests.

FINDINGS

  1. P2 — Regression: bucket-only suffix reads leak an SDK exception.
    pyathena/filesystem/s3.py:1527
    For an accessible bucket, fs.cat_file("s3://bucket/", start=-1) previously obtained directory metadata and raised FileNotFoundError. The new branch bypasses that check and sends Key=None to boto3, producing ParamValidationError. Neither exception handler translates it. This also affects async and cat_ranges callers. Reject bucket-only paths before issuing GetObject; the changed directory tests omit this case.

  2. P2 — Introduced documentation error: trailing-slash behavior is overstated.
    docs/filesystem.md:101
    The paths are not universally equivalent: with only dir/ containing b"abc", cat_file("s3://bucket/dir/", start=-2) returns b"bc", while the slashless call raises FileNotFoundError. Also, info/isfile/read-mode open can access the actual slash-ending object through dir/?versionId=v, because the slash before the query survives normalization. Write-mode open(".../dir/", "wb") can create or overwrite dir instead of raising. Qualify the paragraph by method, mode, and version syntax.

  3. P2 — Pre-existing: bounded negative ranges can use another key’s size.
    pyathena/filesystem/s3.py:1538
    Suppose dir contains ten bytes and dir/ contains b"abc". Calling cat_file("s3://bucket/dir/", start=-2, end=-1) obtains dir’s size through slash-stripping info(), then requests bytes=8-8 from dir/. Its InvalidRange becomes b"", whereas the requested slice is b"b". This logic exists at the merge-base. Metadata lookup must identify the same literal key as GetObject.

The suffix header itself matches RFC 9110 and the inspected fsspec/s3fs implementation. Version precedence and buffered-read/multipart-copy callers show no further regression. Ordinary marker entries are listed as files by ls/find, as documented.

The new tests would catch the old stale-size and slash-marker suffix failures. Mocking info() means they do not establish the documentation’s classification claims; empty-object suffix handling also lacks direct coverage.

I read fsspec 2026.9.0 in the supplied .venv and s3fs 2026.9.0 in the local uv cache. Those comparisons used source, not recollection. Static review only: no edits, builds, tests, GitHub, or network access.

Author verification on live S3 (master 92c9e3e6 vs 91d74a70):

  • 1: cat_file("s3://bucket/", start=-1) raised FileNotFoundError on master and ParamValidationError on the branch. Confirmed regression. Without a range, both raise ParamValidationError, which is pre-existing.
  • 2: cat_file("…/only/", start=-2) returned b"bc", while "…/only" raised FileNotFoundError. open("…/w/", "wb") created the key w. "…/only/?versionId=null" gave info = file of size 3, isfile = True and open = b"abc". Confirmed docs overstatement.
  • 3: with both (10 bytes) and both/ (b"abc"), cat_file("…/both/", -2, -1) returned b"" on master and on the branch. Confirmed, pre-existing.

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.

Repaired in fbd2d0b:

  • 1: cat_file() raises FileNotFoundError for a path without a key before any request, with or without a range. Release note: an unranged cat_file("s3://bucket") used to raise ParamValidationError.
  • 3: when info() is used (a negative end, a negative start with an end, or an empty range), cat_file() raises FileNotFoundError unless the returned entry is a file with the same key. info() of dir/ describes dir, so its size is no longer applied to dir/. A ?versionId= path keeps its slash and matches, as cat_file("…/only/?versionId=null", 0, -1) returning b"ab" shows. The redundant cast(str, key) was dropped.
  • 2: the docs paragraph now says that info, isfile and open normalize the trailing slash. Reading raises FileNotFoundError, and writing dir/ writes dir. A ?versionId= suffix keeps the slash. find, and ls of the directory, list the object as a file. cat_file uses the key as written, reads it without a range, with non-empty non-negative ranges or with a suffix range, and raises for other ranges. Every statement was checked on live S3.

Validation on fbd2d0b:

  • just lint and just docs lint: passed.
  • uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/: 378 passed.
  • New test_cat_file_bucket (3 cases) and test_cat_file_range_key_ending_in_slash (3 cases) fail on 91d74a70.
  • Live S3: the bucket path raises FileNotFoundError with and without a range, and both/ raises FileNotFoundError for (-2, -1) and (5, 5). The only//slash/ results match the docs.

Self-review of this repair:

  • Round one (behavior): the key check runs only on the info() path, so suffix ranges and non-negative ranges are unchanged. A file entry from a listing cache carries the listed key, which equals the parsed key for normal paths. The directory check is kept for plain prefixes, whose key equals the directory's key.
  • Round two (claims): the commit message, the cat_file() Raises: text ("If the path has no key or the key does not exist") and the docs statements were checked against the live results above. The PR body is updated with the new release note.

open() and info() of dir/ refer to the object dir when it exists, and a
?versionId= path reads any range of the key ending in a slash.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread docs/filesystem.md
`dir/` for reading reads the object `dir` or raises `FileNotFoundError`, and opening it
for writing writes the object `dir`. A path with a `?versionId=` suffix keeps the slash
and refers to the object. `find`, and `ls` of the directory, list the object as a file
entry. `cat_file` uses the key as written. Without a `?versionId=` suffix, it reads

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 review (relayed): Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, codex exec -s read-only, session 01a10125-db3d-7510-b0ae-fc80f31122fc. The scope was the repair delta 91d74a700872a9e1086f0d9a7a982d17015c8387..fbd2d0bc3aafa1185a7b3150879fe3ee6111e0a7 (the base 92c9e3e67180eb3d52ef7b85579a24540a30fd64 did not move), with the affected contracts. This is a static review. The snapshot and the PR worktree were unchanged. Result: FINDINGS (docs only).

Reviewer output, verbatim:

Covered: the exact delta; path parsing, range branches, metadata/cache keys, version handling, prefixes, fsspec callers, async forwarding, open reads/writes, find/ls, and test helpers.

FINDINGS — two documentation issues; no code regression found.

  1. P3 — introduced documentation overstatement: docs/filesystem.md:109 says other ranges raise FileNotFoundError without excluding version-qualified paths. For an existing ten-byte dir/ version, cat_file("s3://bucket/dir/?versionId=VERSION", start=-2, end=-1) successfully reads one byte; (5, 5) returns b"". info() preserves that key, so the new check passes. Qualify this restriction as applying to trailing-slash paths without a version suffix.

  2. P3 — pre-existing claim retained: docs/filesystem.md:103 assumes that a trailing-slash path always becomes a directory and fails when opened for reading. When both dir and dir/ exist, normalization makes info() return the file dir, isfile() return True, and open(..., "rb") read dir. The object lookup and new test’s setup establish this scenario. State that the directory/read-failure behavior assumes no object exists at the normalized key.

The implementation satisfies the requested guards. Ordinary keys, cached/version-aware entries, prefixes, suffix ranges, and non-empty non-negative ranges retain their prior behavior.

Static tracing indicates all six new test cases fail before and pass after for the intended reasons: bucket cases previously reached GetObject; mismatched-key cases previously returned b"" using the unrelated metadata.

No edits, builds, tests, or network access. HEAD remains fbd2d0bc…, with a clean working tree.

Author verification on live S3:

  • 1: cat_file("…/only/?versionId=null", -2, -1) returned b"e", and (5, 5) returned b"". Confirmed.
  • 2: with both and both/, info("…/both/") returned a file of size 10, isfile returned True, and open().read() returned b"0123456789". Confirmed.

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.

Repaired in da5dedc (docs only):

  • info, isfile and open treat dir/ as "the object dir if it exists, and otherwise the directory dir". Opening dir/ for reading reads dir or raises FileNotFoundError.
  • The cat_file range restriction now applies "without a ?versionId= suffix".
  • just docs lint: passed. Every statement in the paragraph matches the live results recorded in this thread and the earlier ones.
  • Self-review of the repair: round one found no behavior changes (docs only). In round two, each sentence was checked against a live result: normalization and existing dir (both/), the directory and read failure (only/), writing w/ → w, ?versionId=null access, find/ls listing, and cat_file ranges with and without a version suffix.

Comment thread docs/filesystem.md
limit raises `ValueError` and aborts its multipart upload. Multipart copies with `cp`
use parts large enough to stay within the limit.

Paths are normalized as in fsspec, which drops a trailing slash, so `info`, `isfile`,

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 narrow follow-up review (relayed): Codex CLI 0.160.0, model gpt-6-astra, reasoning effort max, codex exec -s read-only, session 01a1012b-b219-74d0-8553-c9578ca54d61. The scope was the docs delta fbd2d0bc3aafa1185a7b3150879fe3ee6111e0a7..da5dedcfabdb057511dae7b935f28e17b6d50d57 (the base 92c9e3e67180eb3d52ef7b85579a24540a30fd64 did not move), with each statement checked against the code paths. This is a static review. The snapshot and the PR worktree were unchanged. Result: CLEAN.

Reviewer output, verbatim:

Surfaces covered, assuming dir/ exists and checking both with and without an object at dir:

Surface Verified behavior
info, isfile Unsuffixed dir/ resolves to dir: file/True when that object exists; directory/False otherwise.
open Reading uses dir or raises FileNotFoundError; writing targets dir.
?versionId=V Preserves the slash key for metadata and reads. Version-qualified writes raise ValueError.
find, ls of the directory Include the slash-key object as a file entry, including when dir also exists.
cat_file, no suffix Whole-object reads, non-empty non-negative ranges—including open-ended ranges—and negative start without end read the slash key directly.
Other unsuffixed ranges Empty/reversed ranges, negative end, and negative start with an explicit end raise FileNotFoundError. The key-mismatch guard also rejects an existing dir object.
Version-qualified cat_file Metadata retains the slash key, allowing negative-offset normalization and empty results. Ranges starting at/past EOF return empty bytes.

CLEAN — no actionable findings in the rewritten paragraph.

Static source review at da5dedcf, including inherited methods in local fsspec 2026.9.0, matching the lockfile. No edits, builds, tests, or network access.

@laughingman7743
laughingman7743 marked this pull request as ready for review October 3, 2026 09:55
@laughingman7743
laughingman7743 merged commit 55af09a into master Oct 3, 2026
14 checks passed
@laughingman7743
laughingman7743 deleted the fix/994-suffix-range branch October 3, 2026 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Objects whose keys end in '/' are listed as files but cannot be opened or read with negative offsets

1 participant