Skip to content

Cache each explicit version of an object separately in info() - #957

Merged
laughingman7743 merged 6 commits into
masterfrom
fix/932-info-version-cache
Oct 3, 2026
Merged

laughingman7743 merged 6 commits into
masterfrom
fix/932-info-version-cache

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

WHAT

S3FileSystem.info() now caches each explicitly requested version of an object separately.

  • info() with an explicit version (the version_id argument or a ?versionId= suffix) skips the cached entries of the path and goes to _head_object().
    Cached listings and the entry of the plain path describe the current version.
  • _head_object() caches an explicit version under the version-qualified path (bucket/key?versionId=...), whichever way the version is given.
    Requesting another version heads that version, and a later info(path) without a version no longer returns the explicitly requested version.
  • Before, the second info("s3://bucket/key?versionId=v1") found the cached file entry, did not match the suffixed path against the entry's name, and returned a directory object.
    Through S3File, the second open() of the same ?versionId= path on one instance therefore had size 0 and read b"".
    Skipping the path's cached entries fixes this as well.
  • The null version is not cached under a version-qualified path.
    Writes invalidate only the path without the version, and an overwrite in a bucket without versioning (or with versioning suspended) replaces the null version.
    A cached null version would keep the old ETag, and the next open() would fail with PreconditionFailed through IfMatch.
    Other versions are immutable, so their cached entries stay valid.

AioS3FileSystem._info() delegates to the synchronous info() and gets the same behavior.

WHY

Fixes #932.
After info(p, version_id="v1"), info(p, version_id="v2") returned v1's size, ETag and version_id from the cache.

TEST

Tested commit: 46f927a (the filesystem suite ran on bebe3d7; 46f927a changes only the info() docstring).

  • just lint: passed.
  • uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/: 246 passed.
  • New tests, each failing on master (e0e85da0):
    • test_info_caches_each_version_separately (offline, with and without version_aware): v1, v2, ?versionId=v1, and the plain path each return their own HeadObject result, and a second round is served from the cache. On master, the v2 lookup returns v1's size.
    • test_info_does_not_cache_null_version (offline): the null version is headed on every lookup.
    • test_read_null_version (live S3, unversioned staging bucket): writes an object, reads ?versionId=null twice, overwrites it and reads twice again. On master, the second read returns b"". Without the null exception, the read after the overwrite fails with PreconditionFailed.

Not covered live: the CI test bucket is not versioned, so reading two distinct versions of one object (the issue's reproduction) was not run against S3. The offline test mocks HeadObject responses per VersionId instead.

🤖 Generated with Claude Code

info(path, version_id=...) looked up and stored the HeadObject result
under the path without the version, so a later lookup of another
version, or of the path itself, got the first version's metadata.
Look up an explicit version under its version-qualified path instead,
and compare cached entries by their name without the version suffix,
so a second info() of a ?versionId= path no longer returns a directory.

Fixes #932

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
laughingman7743 and others added 3 commits October 3, 2026 15:09
The second open of a ?versionId= path read the metadata cached by the
first one as a directory and returned no data.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Writes invalidate only the path without the version, and overwriting
an object in a bucket without versioning replaces its null version.
A cached null version would keep the old ETag, and the next read with
IfMatch would fail with PreconditionFailed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread pyathena/filesystem/s3.py Outdated
path = self._strip_protocol(path)
bucket, key, path_version_id = self.parse_path(path)
version_id = path_version_id if path_version_id else kwargs.pop("version_id", None)
if key and version_id and not path_version_id:

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 e0e85da09435257a18c8b8832b3e10ba7840b697, head 6e263b8c0b70d56281b1a99e6d63c28fd9e20bdf.

Covered: info() cache lookup and version resolution, _head_object() cache store, callers S3File.__init__ (info(path, version_id=...)), exists(), ls() (_head_object(path) without a version), AioS3FileSystem._info() (delegates to sync info()), invalidate_cache() interaction, and the new tests.

Result: FINDINGS, repaired in this PR.

  1. On master, the second info("bucket/key?versionId=v1") found the cached file entry but compared its name bucket/key with the suffixed path and returned a directory object (pyathena/filesystem/s3.py:636). Through S3File, the second open() of the same version on one instance had size 0 and read b"" (reproduced live). Fixed by matching on name (path without the suffix); covered by test_read_null_version and the second round of test_info_caches_each_version_separately.
  2. Caching explicit versions under the version-qualified key made a stale null version reachable: writes call invalidate_cache("bucket/key") only, and an overwrite in an unversioned or suspended bucket replaces the null version. Reproduced live as PreconditionFailed (stale ETag in IfMatch) after an overwrite. Repaired in db4970e by not caching null under a version-qualified key (s3.py:372), with the maintainer's approval.

Reasoned deferral (pre-existing, not a regression): rm_file() invalidates the exact spelling of the path, so after rm_file("p?version_id=v") a cached p?versionId=v entry is not popped. On master the explicit version was cached under p, which that rm_file() did not pop either; #931 changes invalidate_cache() for versioned paths.
Listing caches use (path, delimiter) keys, so _ls_from_cache() never returns a list for a version-qualified path; the name comparison in the list branch does not change behavior there.

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 — repair (6e263b8c0b70d56281b1a99e6d63c28fd9e20bdf → 46f927a4b32be5a474279f48ec765cfc57966f53, base e0e85da09435257a18c8b8832b3e10ba7840b697; range-diff: commits 1–4 unchanged, bebe3d7 and 46f927a added).

Trigger: a peer review of the same code (the #936 session) pointed out that info() with a version still consulted _ls_from_cache(). Its repro, a list under the plain parent key bucket/dir, is not produced by any current code path, because _ls_dirs() caches under (path, delimiter) (checked with ls() + find()). Even so, any cached entry of the plain path describes the current version, so a versioned lookup should not consult it.

Repair: info() skips the cache block when a version is given (pyathena/filesystem/s3.py:634); _head_object() keys an explicit version by the version-qualified path whichever way it is given and skips storing null (s3.py:343-350). This removes the name comparison and the now-redundant not version_id in the version_aware stale check.

Re-checked callers: ls() calls _head_object(path) without a version (unchanged); a bucket path with version_id still goes to _head_bucket("bucket") with its own cache; exists() unchanged; the peer's list-cache scenario now returns the versioned HeadObject result. just lint passed; tests/pyathena/filesystem/ 246 passed at bebe3d7. Result: CLEAN.

Comment thread pyathena/filesystem/s3.py Outdated
# Writes invalidate only the path without the version, and an
# overwrite replaces the "null" version of a bucket without
# versioning, so that version is looked up every time.
if path_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.

Self-review round two (compatibility, operations, claims) — base e0e85da09435257a18c8b8832b3e10ba7840b697, head 6e263b8c0b70d56281b1a99e6d63c28fd9e20bdf.

Covered: caller compatibility (_head_object() signature unchanged; S3File, exists(), ls(), async delegation), suspended-versioning buckets in version_aware mode (an unversioned HeadObject pinning null is stored under the plain path, which writes invalidate), AWS request cost, and every factual claim in the PR body and the info() docstring.

Result: CLEAN.

  • Requests: null versions are now headed on every info(); explicitly versioned lookups with the version_id argument are headed once per version instead of reusing another version's entry. The live test adds 2 PutObject, 4 HeadObject and 4 GetObject requests on the unversioned staging bucket.
  • Claims checked against runs at 6e263b8c0b70d56281b1a99e6d63c28fd9e20bdf: just lint passed; tests/pyathena/filesystem/ 246 passed; the three new tests fail on master (assert 1 == 2, assert 0 == 4, assert b'' == b'1'); the run without the null exception failed with PreconditionFailed.
  • Limit: the staging bucket is not versioned, so two distinct versions of one object were exercised only through mocked HeadObject responses.

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 — repair (6e263b8c0b70d56281b1a99e6d63c28fd9e20bdf → 46f927a4b32be5a474279f48ec765cfc57966f53).

  • Compatibility: _head_object() signature unchanged; it now also qualifies the cache key when called with the version_id argument and a plain path (only info() does that).
  • Requests: unchanged from the previous round, apart from a versioned lookup no longer being answered from a cached entry of the plain path.
  • Claims: the PR body WHAT section was rewritten for the new structure (the "matched by name without the suffix" bullet no longer applied), and the info() docstring now says the cached entries of the path are skipped (46f927a). The new tests still fail on master for the same reasons recorded above.
    Result: CLEAN.

Comment thread pyathena/filesystem/s3.py Outdated
if isinstance(caches, list):
cache = next((c for c in caches if c.name == path), None)
elif caches.name == path:
cache = next((c for c in caches if c.name == name), 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.

Independent review (relayed) — reviewer: Codex CLI 0.160.0, model gpt-6-astra (a model other than the author), codex exec -s read-only --ephemeral, session 01a10067-8317-7193-a012-e9bac40bde06. Static review of a detached snapshot at head 6e263b8c0b70d56281b1a99e6d63c28fd9e20bdf, base e0e85da09435257a18c8b8832b3e10ba7840b697; the prompt contained the diff range and intended behavior only, without the PR text, commit messages or prior findings. Snapshot and PR worktree verified unchanged afterwards.

Reviewer coverage: the diff, metadata/cache helpers, exists(), ls(), write/delete invalidation, S3File size/ETag handling, async delegation; both version_aware modes, protocol prefixes, query spellings, trailing slashes, buckets and directory prefixes. Reviewer verdict: FINDINGS — two pre-existing issues, no introduced regression; the added tests fail against the base for cross-version reuse, repeated lookups returning directories, and stale null versions.

  1. [P2, pre-existing] rm_file("bucket/key?versionId=v1") invalidates only the qualified key and its parents (pyathena/filesystem/s3.py:1931), so a cached bucket/key survives and exists()/info() return the deleted object; a ?version_id= spelling also leaves a ?versionId= entry. Author verification: confirmed in the code; this is S3FileSystem keeps the cached object after rm_file() deletes a specific version #931 (fix in progress separately). On master the keyword-version entry was stored under bucket/key, which this delete did not pop either, so this PR does not widen it. Deferred to S3FileSystem keeps the cached object after rm_file() deletes a specific version #931, spelling case passed on.
  2. [P2, pre-existing] S3File(fs, "bucket/key", version_id="v1") gets self.size from fsspec's AbstractBufferedFile.__init__ (self.details → fs.info(path) without the version) before the version-specific lookup at pyathena/filesystem/s3.py:2240, so a version whose size differs from the current one is read with the wrong size. Author verification: confirmed in the code. With this PR the version-specific lookup now returns the correct ETag (on master it returned the cached current version, and the read failed with PreconditionFailed), but the size still comes from the plain path. Reachable today only by constructing S3File directly; fs.open(..., version_id=...) raises TypeError until S3FileSystem.open() and cat_file() raise TypeError when given version_id #936. Deferred to S3FileSystem.open() and cat_file() raise TypeError when given version_id #936, which makes that path public.

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) — reviewer: Codex CLI 0.160.0, model gpt-6-astra, codex exec -s read-only --ephemeral, session 01a10077-22e5-79f3-919a-05179aed5321. Static review of a detached snapshot at 46f927a4b32be5a474279f48ec765cfc57966f53 (previous head 6e263b8c0b70d56281b1a99e6d63c28fd9e20bdf, base e0e85da09435257a18c8b8832b3e10ba7840b697, range-diff with commits 1–4 unchanged). Prompt without PR text, commit messages or prior findings. Snapshot and PR worktree verified unchanged afterwards.

Reviewer verdict on the new commits: they implement the intended cache behavior. Explicit versions are isolated, an explicit null is not stored, lookups without a version (including the version_aware re-lookup) are preserved, ls()'s HeadObject fallback and async info() get the fix, and the tests at test_s3.py:508, :526 and :1716 remain sensitive to the original defects.

Reported issues, all pre-existing at the base (author-verified in the code):

  1. [P2] exists(path, version_id=...) drops the version (pyathena/filesystem/s3.py:855 calls info(path, refresh=refresh)). Out of scope; reported to the maintainer.
  2. [P2] open(path, version_id=...) raises TypeError (s3.py:1989, s3_async.py:369). This is S3FileSystem.open() and cat_file() raise TypeError when given version_id #936, fixed in PR Accept version_id in S3FileSystem.open() and cat_file() #958.
  3. [P2] S3File takes its size from fsspec's __init__ lookup and its ETag from a second lookup (s3.py:2234), so a concurrent overwrite of the null version between them can produce a truncated read that IfMatch does not catch. PR Accept version_id in S3FileSystem.open() and cat_file() #958 looks the object up once before AbstractBufferedFile.__init__ and passes size=, which removes the second lookup.
  4. [P3] info("bucket?versionId=v1") passes the suffix to HeadBucket (s3.py:666). This is a meaningless input for a bucket; out of scope; reported to the maintainer.

No repair is needed in this PR.

@laughingman7743
laughingman7743 marked this pull request as ready for review October 3, 2026 06:26
@laughingman7743
laughingman7743 marked this pull request as draft October 3, 2026 06:29
laughingman7743 and others added 2 commits October 3, 2026 15:31
Cached listings and the entry of the path without a version describe
the current version, so info() with a version skips them, and
_head_object() keys the cache by the version-qualified path whichever
way the version is given. This replaces the name comparison against
the path without its version suffix.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
laughingman7743 added a commit that referenced this pull request Oct 3, 2026
Revert "Look up a version without the cache of the latest version"
(40f8c4c). #957 changes how info() caches explicit versions for #932,
so the overlapping change is dropped here.

This reverts commit 40f8c4c.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@laughingman7743
laughingman7743 marked this pull request as ready for review October 3, 2026 06:43
@laughingman7743
laughingman7743 merged commit b598dc6 into master Oct 3, 2026
12 checks passed
@laughingman7743
laughingman7743 deleted the fix/932-info-version-cache branch October 3, 2026 07:39
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.

S3FileSystem.info(version_id=...) returns cached metadata of another version

1 participant