Cache each explicit version of an object separately in info() - #957
Conversation
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>
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>
| 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: |
There was a problem hiding this comment.
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.
- On master, the second
info("bucket/key?versionId=v1")found the cached file entry but compared its namebucket/keywith the suffixed path and returned a directory object (pyathena/filesystem/s3.py:636). ThroughS3File, the secondopen()of the same version on one instance had size 0 and readb""(reproduced live). Fixed by matching onname(path without the suffix); covered bytest_read_null_versionand the second round oftest_info_caches_each_version_separately. - Caching explicit versions under the version-qualified key made a stale
nullversion reachable: writes callinvalidate_cache("bucket/key")only, and an overwrite in an unversioned or suspended bucket replaces thenullversion. Reproduced live asPreconditionFailed(stale ETag inIfMatch) after an overwrite. Repaired in db4970e by not cachingnullunder 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.
There was a problem hiding this comment.
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.
| # 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": |
There was a problem hiding this comment.
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:
nullversions are now headed on everyinfo(); explicitly versioned lookups with theversion_idargument 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 lintpassed;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 thenullexception failed withPreconditionFailed. - Limit: the staging bucket is not versioned, so two distinct versions of one object were exercised only through mocked HeadObject responses.
There was a problem hiding this comment.
Self-review round two — repair (6e263b8c0b70d56281b1a99e6d63c28fd9e20bdf → 46f927a4b32be5a474279f48ec765cfc57966f53).
- Compatibility:
_head_object()signature unchanged; it now also qualifies the cache key when called with theversion_idargument and a plain path (onlyinfo()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.
| 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) |
There was a problem hiding this comment.
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.
- [P2, pre-existing]
rm_file("bucket/key?versionId=v1")invalidates only the qualified key and its parents (pyathena/filesystem/s3.py:1931), so a cachedbucket/keysurvives andexists()/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 underbucket/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. - [P2, pre-existing]
S3File(fs, "bucket/key", version_id="v1")getsself.sizefrom fsspec'sAbstractBufferedFile.__init__(self.details→fs.info(path)without the version) before the version-specific lookup atpyathena/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 withPreconditionFailed), but the size still comes from the plain path. Reachable today only by constructingS3Filedirectly;fs.open(..., version_id=...)raisesTypeErroruntil 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.
There was a problem hiding this comment.
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):
- [P2]
exists(path, version_id=...)drops the version (pyathena/filesystem/s3.py:855callsinfo(path, refresh=refresh)). Out of scope; reported to the maintainer. - [P2]
open(path, version_id=...)raisesTypeError(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. - [P2]
S3Filetakes its size from fsspec's__init__lookup and its ETag from a second lookup (s3.py:2234), so a concurrent overwrite of thenullversion between them can produce a truncated read thatIfMatchdoes not catch. PR Accept version_id in S3FileSystem.open() and cat_file() #958 looks the object up once beforeAbstractBufferedFile.__init__and passessize=, which removes the second lookup. - [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.
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>
WHAT
S3FileSystem.info()now caches each explicitly requested version of an object separately.info()with an explicit version (theversion_idargument 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.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 secondopen()of the same?versionId=path on one instance therefore had size 0 and readb"".Skipping the path's cached entries fixes this as well.
nullversion 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
nullversion.A cached
nullversion would keep the old ETag, and the nextopen()would fail withPreconditionFailedthroughIfMatch.Other versions are immutable, so their cached entries stay valid.
AioS3FileSystem._info()delegates to the synchronousinfo()and gets the same behavior.WHY
Fixes #932.
After
info(p, version_id="v1"),info(p, version_id="v2")returned v1's size, ETag andversion_idfrom 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.e0e85da0):test_info_caches_each_version_separately(offline, with and withoutversion_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): thenullversion is headed on every lookup.test_read_null_version(live S3, unversioned staging bucket): writes an object, reads?versionId=nulltwice, overwrites it and reads twice again. On master, the second read returnsb"". Without thenullexception, the read after the overwrite fails withPreconditionFailed.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
VersionIdinstead.🤖 Generated with Claude Code