Invalidate the object path when a version is deleted

[laughingman7743]

WHAT

S3FileSystem.invalidate_cache() now handles a version-qualified path (bucket/key?versionId=...):

• It drops the cached entries of the version-qualified path itself, under every query spelling that parse_path accepts (versionId, versionID, versionid, version_id), including their listing keys.
• It then invalidates the object path without the version query (bucket/key) and its parents, instead of starting the parent walk from the version-qualified path.

Cached entries of other versions of the same object (bucket/key?versionId=v2) are kept, because deleting one version does not change another.
Unversioned paths pop the same keys as before.

The fix applies to every caller that passes a version-qualified path, including rm_file() and setxattr(); the latter copies the given version over the object and creates a new current version, so it left the same stale entry.
AioS3FileSystem delegates to the same method.

WHY

Closes #931.

_head_object() caches the result for bucket/key under that path.
rm_file("bucket/key?versionId=v1") called invalidate_cache() with the version-qualified path, whose parent is bucket, so the bucket/key entry was never popped.
After that, exists("bucket/key") stayed True when the deleted version was the only one, and info("bucket/key") kept returning the deleted version's metadata when it was the latest.

_head_object() also caches a version-qualified path as given, so deleting bucket/key?version_id=v1 left an entry cached as bucket/key?versionId=v1, and exists() of that path kept returning the deleted version.
This existed before as well; it is folded in here because it is the same invalidation.

This is a bug fix for the next release notes.

TEST

Tested commit: 02b34d6.

• just lint: passed.
• uv run --env-file .env pytest -n 4 tests/pyathena/filesystem/ -q: 248 passed (live S3).
• New offline regression tests, confirmed to fail on master:
  • test_invalidate_cache_version_drops_object_path (parametrized): the version-qualified entry and its listing keys, the object path, and the parent listings are dropped, for an s3:// string, a Path, a directory marker path (bucket/a/dir/?versionId=v1), and a deletion under a different query spelling than the cached one; another version's entry is kept.
  • test_rm_file_version_invalidates_object_path: rm_file() sends VersionId, and exists() of the object path then asks S3 instead of answering from the cache.
• No live test of a versioned delete: deleting an explicit VersionId needs s3:DeleteObjectVersion, which the CI role in cloudformation/github_actions_oidc.yaml does not grant, and the CI bucket is unversioned.
• No async test added: AioS3FileSystem._rm_file() and invalidate_cache() delegate to the synchronous filesystem.

🤖 Generated with Claude Code

[laughingman7743]

Self-review round 1 (implementation behavior): FINDINGS (1, repaired)

Scope: base e0e85da, head b0178a5 (full diff: pyathena/filesystem/s3.py, tests/pyathena/filesystem/test_s3.py).

Covered:

• Every invalidate_cache() caller in s3.py and s3_async.py (rm_file, rm, touch, cp_file, put_file, pipe_file, setxattr, mkdir/rmdir, S3File commit, aio _rm/_cp_file/invalidate_cache). Unversioned paths keep the old behavior: the extra pop hits the same key the loop pops.
• Cache keys written by _head_object (path as given, including ?versionId=), _head_bucket, _ls_buckets ("", still never popped), and _ls_dirs ((path, delimiter)).
• Other versions' entries (bucket/key?versionId=v2) stay cached; versions are immutable.
• Tests: the live test reproduces the issue on the unversioned CI bucket (null version), and both new tests failed without the fix.

Finding: at b0178a5 the query was removed after _strip_protocol, so bucket/a/dir/?versionId=v1 (a directory marker version) became bucket/a/dir/; _parent() of that is bucket/a, so the bucket/a/dir entry was never popped. Before this PR it was popped as the parent of the versioned path, so this was a small regression introduced here.

Repair (9ae84dd): remove the query first, then _strip_protocol the result; added a parametrized case for the trailing-slash path, which fails on b0178a5 and passes now. just lint and the affected filesystem tests (-k "rm or invalidate or version or xattr", 37 passed, live S3) pass.

[laughingman7743]

Self-review round 2 (claims, callers, operations): FINDINGS (2, repaired)

Scope: base e0e85da, head 9ae84dd (full pass over the PR body, docstring, comments, commit messages, and the issue premise).

Claims checked:

• Issue premise: _parent("bucket/key?versionId=v1") is bucket, so the bucket/key HeadObject entry survived; confirmed live by the new integration test failing on master (exists() returned True).
• info() returns the cached entry whether or not it carries a pinned version, so the stale-metadata case holds for both version_aware settings.
• setxattr() with a versioned path copies that version onto the same key (copy_object, s3.py setxattr), creating a new current version, then invalidated only the versioned path: same stale entry, fixed by the same change.
• Aio: _rm_file runs the sync rm_file, and invalidate_cache delegates (s3_async.py:570-578).
• docs/filesystem.md has no statement about cache invalidation that this change makes obsolete.
• Existing callers: signature and path=None behavior unchanged; unversioned paths pop the same keys as before.
• AWS operator: the only traffic change is one fresh HeadObject/listing on the next lookup of the object path after a versioned delete or setxattr, which is the intended freshness.

Findings:

1. The comment said S3 keys cannot contain ?; they can. The safe premise is that parse_path rejects ? in keys (PATTERN_PATH key group [^?]+), and every cache writer (_head_object, _ls_dirs, _head_bucket) calls parse_path first, so no cache key carries a ? other than the versionId query. Repaired the comment (67042cf); no behavior change.
2. The PR body's TEST section named b0178a5 and its results. Re-ran on 67042cf: just lint passed, tests/pyathena/filesystem/ 245 passed (live S3); updated the body.

Limit: no versioned bucket in the test account, so the "latest of several versions" case is covered only offline.

[laughingman7743]

Independent review (relayed): FINDINGS (3, all verified and repaired)

Reviewer: Codex CLI 0.160.0 (codex exec -s read-only, model gpt-6-astra), session 01a10067-c804-7a22-a106-889abb5d36fb. Static review only: no edits, builds, tests, or GitHub access. Input: the literal diff, a detached snapshot worktree at the head, and the intended behavior; no PR number, description, commit messages, or prior findings.
Scope: base e0e85da, head 67042cf. Snapshot and PR worktree unchanged during the review.

Covered (reviewer): all sync/async invalidation callers; HeadObject/listing cache keys and info()/exists() consumers; protocol prefixes, trailing slashes, directory markers, bucket/root paths, version-query variants; fsspec 2026.9.0 _strip_protocol/_parent; test assertions and fixture permissions; comments/docstrings.

Findings (all introduced by this PR):

1. P2 s3.py:1925 (at 67042cf): .split() ran on the raw argument, so a path-like input (Path("bucket/a/key?versionId=v1")) raised AttributeError; fsspec's _strip_protocol stringifies it. Verified: AbstractFileSystem._strip_protocol calls stringify_path.
2. P2 s3.py:1922: the version-qualified path's own (path, delimiter) listing keys were no longer popped, e.g. after ls("bucket/dir?versionId=v1") (_ls_dirs caches under (path, delimiter) with the path as given). Master popped them.
3. P2 test_s3.py:1715: the live test deletes an explicit VersionId (null), which needs s3:DeleteObjectVersion; cloudformation/github_actions_oidc.yaml grants only s3:DeleteObject, so the test would fail in CI with PermissionError. It passed locally only because the local credentials are broader.

Repairs (a74c00e):

• One walk starting from the normalized path: each step pops the string and tuple keys, then moves to the path without the version query if there is one, else to _parent(). Fixes 1 and 2.
• Replaced the live test with offline test_rm_file_version_invalidates_object_path (rm_file sends VersionId, then exists() falls through to HeadObject/listing instead of the cache). Added Path and (cache_key, "/") cases to the invalidation test. No IAM change.
• Validation: the new cases fail with s3.py from 67042cf (Path, tuple keys) and from master (the issue itself, including the rm_file test) and pass now; just lint passed; tests/pyathena/filesystem/ 246 passed (live S3).

The repair still needs both self-review perspectives and an independent follow-up.

[laughingman7743]

Self-review of the repairs, both perspectives (range-diff 67042cf..02b34d6 on the same base e0e85da; commits a74c00e and 02b34d6): CLEAN

Round-1 perspective (behavior):

• Unversioned paths: query is empty, so cache_paths == [path] and the walk is identical to master.
• Versioned paths: the walk pops the given spelling and the other three spellings PATTERN_PATH accepts (version(Id|ID|id|_id)), each with its (path, "/")/(path, "") listing keys, then continues from _strip_protocol(base), which has no ?, so the loop terminates.
• _strip_protocol runs first, so path-like inputs are stringified as in fsspec; a directory marker keeps its slash in the rebuilt spellings (base is not stripped) and continues from bucket/a/dir.
• A version ID containing = keeps everything after the first =, matching the greedy (?P<version_id>.+).
• 02b34d6 also covers a pre-existing gap reported from the S3FileSystem.info(version_id=...) returns cached metadata of another version #932 review (maintainer approved folding it in): _head_object caches the spelling as given, so deleting under another spelling left the deleted version cached.

Round-2 perspective (claims, callers, operations):

• Docstring now states the spelling behavior; the comment's parse_path premise still holds (key group [^?]+).
• Cache writers all call parse_path first, so a non-version ? query cannot be a cache key; for such an input the extra pops only over-invalidate.
• No new S3 traffic beyond the next lookup of the invalidated paths. The live test was removed because the CI role lacks s3:DeleteObjectVersion; no IAM change proposed.
• PR body rewritten for the final behavior and tested commit 02b34d6: just lint passed, tests/pyathena/filesystem/ 248 passed (live S3); the new spelling cases fail with s3.py from a74c00e and pass now.

Limit: PATTERN_PATH and the spelling tuple list the same four names separately. An independent follow-up review of the repairs comes next.

[laughingman7743]

Independent follow-up review (relayed): CLEAN

Reviewer: Codex CLI 0.160.0 (codex exec -s read-only, model gpt-6-astra), session 01a10076-9a9b-7020-be9d-6d7156dc6027. Static review only: no edits, builds, tests, or GitHub access.
Scope: changes since the reviewed head, git diff 67042cf44ef9c4eae1b65d3c3c1aabadaeacae84..02b34d6689c481d96dcf11584d7ebea358503b2a (base e0e85da unchanged), plus the full current diff for context, on a detached snapshot at 02b34d6. Snapshot and PR worktree unchanged afterwards. Input: diffs and intended behavior only, no PR text, commit messages, or prior findings.

Covered (reviewer): all invalidation callers in s3.py/s3_async.py incl. buffered commits and async delegation; HeadObject/listing keys, all four query spellings, parent eviction, later exists()/info(); protocol prefixes, path-like inputs, trailing slashes, directory markers, bucket/root paths, special characters in version IDs, loop termination; test meaningfulness, comments/docstrings, CI role permissions.

Result: no actionable regressions in 67042cf4..02b34d66; unversioned behavior matches the base; the replacement deletion test needs no extra permissions.