Skip to content

Answer info() and exists() from cached parent listings and fix bucket lookups - #1006

Merged
laughingman7743 merged 4 commits into
masterfrom
fix/965-980-cached-lookups
Oct 3, 2026
Merged

laughingman7743 merged 4 commits into
masterfrom
fix/965-980-cached-lookups

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

WHAT

Bucket and key lookups in S3FileSystem now use the cached listings consistently.

  • info() and exists() (and fsspec's isdir(), isfile() and size(), which call info()) answer a key path from the cached listing of its parent directory, the (parent, "/") entry that ls() and find(maxdepth=...) cache. A key that is listed is returned without HeadObject. A key that the complete parent listing does not contain raises FileNotFoundError (or exists() returns False) without a request.
    • When the parent listing has both an object and a key prefix of the same name, the object is returned, as the uncached HeadObject-first lookup does.
    • A listing of the path itself is not used. It cannot tell whether an object of the same name exists, and using it would make such an object look like a directory, which open() then rejects.
    • A lookup that contradicts the cached parent listing removes it, so the listing does not answer against a fresher result afterwards. This happens when HeadObject misses an object that the listing still lists (from info()/exists() with refresh=True, ls(path, refresh=True), or the version_aware re-head), and when the ListObjectsV2 check of info() finds a key prefix, which nothing else caches. A HeadObject miss for a path that the listing does not list keeps it.
    • open(path, "a") looks up the existing object with HeadObject (info(path, refresh=True)), because the rewritten object keeps the existing ContentType, Metadata and encryption settings, which a listing entry lacks. This replaces the former exists() + info() pair; an append of an object whose HeadObject result was already cached now sends one more HeadObject.
    • With version_aware, a listed file still gets HeadObject to pin its version, as before. A version-qualified path uses only its own cached entry, because listings describe the current versions.
  • info() and isdir() of a bucket that is not in the cached bucket listing send HeadBucket instead of raising FileNotFoundError. ListBuckets returns only the buckets that the caller owns.
  • info(""), info("/") and info("s3://") return the root directory instead of raising ValueError. parse_path() ran before the root branch, which had never been reached since the filesystem was added (207c8f0), so fsspec's isdir() and size() of the root raised ValueError while exists() returned True.
  • invalidate_cache(""), invalidate_cache("/") and invalidate_cache("s3://") remove the cached bucket listing. Bucket and key paths still keep it.
  • exists() of a bucket returns True when HeadBucket answers 403. makedirs("s3://bucket/prefix", exist_ok=True) and mkdir() of a prefix therefore succeed for such a bucket instead of raising PermissionError.

Reads go through one dircache.get() each, as in #1002. The race in which a fetch running during an invalidation writes back a stale entry is out of scope.

Behavior changes for the release notes:

  • info(), size() and exists() of a key that a cached ls()/find(maxdepth=...) listing of its parent covers no longer send HeadObject. The result is the listed entry, which has no ContentType or Metadata, as ls(detail=True) entries do. The listing's ETag is still used for IfMatch on open().
  • A cached parent listing now also answers info(), exists(), isfile(), size() and open(), not only ls() and find(). An object that something other than this filesystem instance (Athena, another process, another instance) writes after the listing was cached is reported missing until refresh=True or invalidate_cache() is used. s3fs behaves the same way. Writes through this instance invalidate the parent listings as before.
  • exists() returns True for a bucket that HeadBucket denies. Measured on 2026-10-03 with valid credentials, HeadBucket returns 403 for an existing bucket in another account (test, amazon, example) and 404 for a missing one. With invalid credentials, HeadBucket returns 403 for every bucket, so exists() also returns True for a missing bucket in that case. The next request fails with PermissionError.

WHY

Closes #965, closes #980.

TEST

Tested commit: eebe98d.

  • just lint: passed.
  • uv run --env-file .env pytest -q -n 4 tests/pyathena/filesystem: 431 passed (live S3).
  • New offline tests in tests/pyathena/filesystem/test_s3.py. The following fail on master and pass here:
    • test_info_uses_cached_listings
    • test_info_prefers_listed_object_to_prefix_of_same_name
    • test_info_bucket_missing_from_bucket_listing
    • test_invalidate_cache_root_drops_bucket_listing
    • test_exists_bucket_access_denied
    • the updated test_dir_filesystem, whose info() is now answered from the listing
    • test_open_append_keeps_metadata_of_listed_object, test_missing_object_drops_cached_parent_listing (exists/ls/version_aware) and test_refreshed_prefix_drops_cached_parent_listing (fail on 2efc5a1, the first commit of this PR)
    • test_missing_object_keeps_cached_parent_listing_without_it guards the conditional eviction
    • test_info_root (fails on master and on fed2d2a)
  • test_open_append_lookup_failure now makes info() fail instead of exists(), which the append no longer calls.
  • Guard tests that pass on both master and this branch: test_info_does_not_use_listing_of_path, test_info_version_aware_heads_listed_file, test_exists_version_ignores_cached_parent_listing.
  • The HeadBucket status codes above were measured with boto3 against live S3. The 403 path of exists() has offline coverage only, because the CI account has no bucket of another account to test against.

🤖 Generated with Claude Code

info() and exists() looked up only string dircache keys, so the listings
that _ls_dirs() caches under (path, delimiter) never answered them, and
a bucket missing from the cached bucket listing was reported missing
without HeadBucket. Look up the (parent, "/") listing for key paths,
preferring an object to a key prefix of the same name as HeadObject
does, and fall through to HeadBucket for buckets that the listing of
owned buckets does not contain.

invalidate_cache() of the root now removes the cached bucket listing,
and exists() treats a bucket that HeadBucket denies (403) as existing,
so makedirs(exist_ok=True) works for a prefix in such a bucket.

Closes #965, closes #980.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread pyathena/filesystem/s3.py
if version_id:
return None
if key:
parent_cache = self.dircache.get((self._parent(path), "/"))

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 1 (implementation behavior) — base 9b2f033708b81dce7323dfda61e8b2ec95df23da, head 2efc5a10e90a8332d998ba6159a0a673b16c96a7.

Covered: _ls_from_cache(), info(), exists(), invalidate_cache(), the mkdir/rmdir comments, and all tests changed in tests/pyathena/filesystem/test_s3.py. Traced the info() callers that now get listing entries: S3File.__init__ (size, ETag for IfMatch, version pin under version_aware), cat_file() negative ranges (info.key != key guard), cp_file() size, checksum() ETag, modified(). Also checked AioS3FileSystem._info/_exists, which delegate to the sync methods. The cached listing keys come only from ls()/_find(), which pass protocol-stripped paths, so (self._parent(path), "/") matches them. Prefixed and continuation listings are not cached, so a cached parent listing is complete.

Write paths: pipe_file, put_file, touch, cp_file, setxattr, _delete_objects_request, S3File.commit and the aio _put_file/_cp_file all call invalidate_cache(path), which evicts (parent, "/") and (parent, "") up to the bucket. So writes through this instance never leave a parent listing that misses the new object.

Result: FINDINGS (1).

  1. Release-note completeness. Before this change, a stale listing affected only ls()/find(). Now it also answers info()/exists()/isfile()/size()/open(), so an object that another writer adds after the listing was cached is reported missing until refresh=True/invalidate_cache(). This is the intended S3FileSystem.info() does not use cached directory listings #965 behavior and matches s3fs. Repair: added it to the PR body's behavior changes. No code change.

No other findings: the version-qualified guard, the version_aware re-head, the same-name preference (object over prefix) and the bucket fallback each have a test. The exists() 403 branch is covered offline only.

Comment thread pyathena/filesystem/s3.py
if isinstance(caches, list):
cache = next((c for c in caches if c.name == path), None)
matches = [c for c in caches if c.name == path]
# A key can be both an object and a key prefix.

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 2 (claims, callers, operations) — base 9b2f033708b81dce7323dfda61e8b2ec95df23da, head 2efc5a10e90a8332d998ba6159a0a673b16c96a7.

Claims checked:

  • "Tuple keys since v3.15.0": git tag --contains 681e7496 → first tag is v3.15.0. ✔
  • "isdir/isfile/size/du call info()": fsspec 2026.9.0 isdir/isfile/size call info(). du calls find() and isdir(), and reaches info() only through _find's object fallback. Corrected the PR body to name only isdir()/isfile()/size().
  • "s3fs behaves the same way" (stale parent listing): s3fs 2026.9.0 _info uses fsspec _ls_from_cache, which raises FileNotFoundError from a parent listing without the path. ✔ s3fs also answers "directory" from a listing of the path itself, which this PR deliberately does not do.
  • "ListBuckets returns only owned buckets" (docstring/comment): AWS ListBuckets doc, "buckets owned by the authenticated sender". ✔
  • HeadBucket 403/404 statements: measured live with boto3 on 2026-10-03. ✔ "With invalid credentials the next request fails with PermissionError": measured ls/pipe/info → PermissionError after exists() returned True. ✔
  • docs/filesystem.md:101-104 ("info/isfile/open treat dir/ as dir: the object dir if it exists, otherwise the directory"): still holds with a cached parent listing, because the object entry is preferred (this line). It would not have held if the path's own listing were used. No other docs mention the listing cache or exists() of buckets. The 403 → PermissionError table stays true for the other operations.

Adversarial callers:

  • A key with // has a parent with a trailing slash, which never matches a stripped listing key, so it falls through to HeadObject (no false FileNotFoundError).
  • A version-qualified path never uses listings.
  • A folder marker dir/ is listed as a CommonPrefix of the parent, so it does not shadow the object dir.
  • External writers after a cached listing are recorded as a release-note behavior change (round 1).
  • The stale-write race is out of scope by decision.

Evidence: the live tests/pyathena/filesystem run (422 passed) was local at this head. Draft CI ran only check/lint/offline (all pass); AWS jobs run on Ready. The exists() 403 path is offline-only.

Result: FINDINGS (1, PR-text only, repaired). No code change.

A cached listing entry has no ContentType, Metadata or encryption
settings, so an append that looked up the existing object through it
dropped them from the rewritten object. Look the object up with
HeadObject for an append.

A refreshed info() or exists() of a listed key that S3 no longer has
left the parent listing in place, so the next lookup answered from it
again. A refreshed lookup of a key now removes the cached listing of
its parent.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread pyathena/filesystem/s3.py
# The rewritten object keeps the metadata of the existing one,
# which a cached listing entry lacks, so look up the object.
with contextlib.suppress(FileNotFoundError):
append_info = fs.info(path, refresh=True)

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, session 01a10169-e169-7793-aa8c-9e5591e0c2a0. Static review only, run with codex exec -s read-only on a detached snapshot at head 2efc5a10e90a8332d998ba6159a0a673b16c96a7 (merge-base 9b2f033708b81dce7323dfda61e8b2ec95df23da). The prompt included the diff range and the intended behavior, without the PR number, description or self-review records. Afterwards the snapshot was clean and still at the same HEAD.

Covered: info()/exists(), listing cache keys, object/prefix precedence, refresh and invalidation, bucket lookup/403, root paths, trailing/repeated slashes, folder markers, version queries, S3File reads/appends, cat_file, cp_file, checksum, find, DirFileSystem, async delegation, docstrings and tests.

Result: FINDINGS

  1. P2, introduced: append loses metadata after a cached listing. info() now returns a ListObjectsV2 entry, so S3File append (s3_additional_kwargs.update(append_info.to_api_repr())) omitted ContentType/Metadata from the rewritten object.
  2. P2, introduced: a refreshed missing key reappears from its parent cache. After ls("bucket/d") and an external delete, exists(..., refresh=True) is False, but _head_object evicts only the key entry, so the next exists() returns True from (bucket/d, "/").
  3. P2, pre-existing: info("")/info("/")/info("s3://") call parse_path("") before the root branch and raise ValueError (same on the merge-base).

The reviewer noted that the own-listing, version_aware and explicit-version tests also pass on the merge-base; they are compatibility guards.

Verification by the author: all three reproduced (3 also on master). Repairs for 1 and 2 are in 1b26dd1 (this line and the refresh eviction in info()). 3 is outside this PR's scope and is reported to the maintainer instead of being fixed here.

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.

Repair 1b26dd1 — self-review of the repair, both perspectives (range: git range-diff 9b2f0337..2efc5a10 9b2f0337..1b26dd13; only the new commit was added).

Round 1 (behavior):

  • The append lookup is now info(path, refresh=True) with FileNotFoundError suppressed. A missing object gives a plain write, as exists() → False did. A prefix-only path still gets the directory object and then fails in cat(), as on master. Other errors (e.g. PermissionError) still propagate before the base initializer (test_open_append_lookup_failure, now stubbing info). A versioned append is rejected earlier (s3.py:2622).
  • info(refresh=True) of a key without a version evicts (parent, "/") before the lookup. Buckets are untouched (_head_bucket already evicts "" on 404), and version lookups do not touch listings. The internal version_aware re-head sets refresh after this point, so it does not evict.
  • New tests test_open_append_keeps_metadata_of_listed_object and test_refresh_drops_cached_parent_listing fail on 2efc5a1 and pass here.

Round 2 (claims/operations):

  • The info() docstring for refresh and the append comment match the code.
  • Request count: an append sends one HeadObject, the same as master's uncached exists() + cached info(). It sends one more only when a HeadObject result was already cached. Stated in the PR body.
  • An append that is discarded after the lookup has evicted the parent listing needlessly. The cost is one relist, and a committed append invalidates it anyway.

Validation at 1b26dd1: just lint passed; uv run --env-file .env pytest -q -n 4 tests/pyathena/filesystem 424 passed (live S3).

Result: CLEAN. An independent follow-up on the repair is requested next.

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 (relayed): Codex CLI 0.160.0, model gpt-6-astra, session 01a10176-1537-7db2-96ed-5340a7bf6509. Static, read-only review of repair 1b26dd13991f98d42d91a0f72ea89c1654eae10d (git range-diff 9b2f0337..2efc5a10 9b2f0337..1b26dd13). Afterwards the snapshot was clean and still at the same HEAD.

Result: FINDINGS. Both original regressions are fixed and both new tests fail on 2efc5a1. One P2 remains, introduced relative to the merge-base: the parent-listing eviction applied only to info(refresh=True). ls("bucket/d/key", refresh=True) of a deleted object (which calls _head_object() directly) and the version_aware re-head both left ("bucket/d", "/") in place, so a later exists() returned True.

Repair fed2d2a0ec728c4539d38a556635e4cbeb6e7e6c moves the eviction to where the contradiction is found:

  • _head_object() 404 without a version evicts (parent, "/") only if that listing still lists the path (mirrors _head_bucket() with ""). So ls() of a nonexistent prefix keeps a correct parent listing.
  • info()'s ListObjectsV2 check evicts the parent listing when it finds a key prefix, because nothing caches the prefix and the listing may predate it (a refreshed lookup would otherwise be followed by FileNotFoundError from the stale listing).
  • The info(refresh=True) eviction from 1b26dd1 is removed.

Self-review of the repair:

  • Round 1: every key-path _head_object() caller (ls() at s3.py:563, info()) gets the eviction. Version-qualified misses keep listings. The non-refresh info() path reaches HeadObject with a listing cached only for the version_aware re-head, and the prefix branch only after a HeadObject miss.
  • Round 2: the _head_object()/info() docstrings and the PR body now describe the actual eviction points.

Tests: test_missing_object_drops_cached_parent_listing[exists|ls|version_aware] and test_refreshed_prefix_drops_cached_parent_listing fail on 2efc5a1. [ls] and [version_aware] also fail on 1b26dd1. test_missing_object_keeps_cached_parent_listing_without_it guards the conditional eviction. just lint passed; live tests/pyathena/filesystem 428 passed at fed2d2a. A second independent follow-up is requested next.

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.

Second independent follow-up (relayed): Codex CLI 0.160.0, model gpt-6-astra, session 01a1017d-e4f4-7672-b57f-e66cdce4d0c3. Static, read-only review of repair fed2d2a0ec728c4539d38a556635e4cbeb6e7e6c (git range-diff 9b2f0337..1b26dd13 9b2f0337..fed2d2a0). Afterwards the snapshot was clean and still at the same HEAD.

Covered: _head_object(), cache helpers, ls(), info(), exists(), find(), S3File append, AioS3FileSystem delegation and the tests. Also checked name normalization, explicit/null versions, prefix-only paths, eviction scope, request counts and docstrings against the merge-base.

Result: CLEAN.

  • Both stale-listing paths are fixed by the shared eviction in _head_object().
  • A missing object that the listing does not list keeps the listing, and a discovered prefix evicts it.
  • The eviction adds no S3 requests.
  • The new ls/version_aware cases fail on 1b26dd1. The prefix and non-matching-entry tests guard behavior that moved.
  • The fetch/invalidation race stays out of scope as agreed.

Independent review complete for head fed2d2a.

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.

Repair eebe98d099d4328a8529ce6642edd0117bcbb813: pre-existing finding 3 folded in at the maintainer's request. info() now checks the root path before parse_path().

Self-review of the repair:

  • Round 1 (behavior): info("")/("/")/("s3://") return the root directory object (bucket="", name "") without requests, so fsspec isdir() is True, isfile() False and size() 0. Before, the first two raised ValueError. exists() already returned True for the root. Non-root paths are parsed exactly as before. AioS3FileSystem._info delegates to this method. refresh/version_id do not apply to the root.
  • Round 2 (claims): "never reached since 207c8f0" holds. In 207c8f0 and 4fbced8, parse_path() precedes the root check, and PATTERN_PATH requires a bucket of at least one character, so "" and "/" never matched. The docstring now states the root is a directory, and the PR body lists the change.

Tests: test_info_root[|/|s3://] fails on fed2d2a and passes here. just lint passed; live tests/pyathena/filesystem 431 passed at eebe98d. An independent follow-up is requested next.

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 (relayed): Codex CLI 0.160.0, model gpt-6-astra, session 01a101d5-146d-79c0-9d75-de6020255926. Static, read-only review of repair eebe98d099d4328a8529ce6642edd0117bcbb813 (git diff fed2d2a0..eebe98d0, git range-diff 9b2f0337..fed2d2a0 9b2f0337..eebe98d0; the earlier commits are unchanged). Afterwards the snapshot was clean and still at the same HEAD.

Covered: root normalization, S3Object construction, non-root parsing/version/cache paths, and fsspec 2026.9.0 stat/isdir/isfile/size/sizes/checksum/ukey/du. Also AioS3FileSystem._info delegation, DirFileSystem, root exists/ls/find/walk/glob/expand_path, the test, the docstring and the parser history.

Result: CLEAN.

  • The pre-existing defect is resolved, and non-root behavior is unchanged.
  • Root traversal stays consistent with the merge-base: find/wildcard glob/recursive expansion still reject the root. A literal root glob with detail=True now succeeds.
  • The unreachability claim holds: PATTERN_PATH is unchanged since 207c8f0 and requires a non-empty bucket.
  • test_info_root fails without the repair.

Independent review complete for head eebe98d.

A refreshed info() was the only lookup that removed the cached parent
listing, so ls(path, refresh=True) of a deleted object, or the version
aware lookup of a listed entry, left a listing that still answered for
the deleted object. Evict the parent listing where the contradiction is
found instead: when HeadObject misses an object that the listing still
lists, and when the ListObjectsV2 check finds a key prefix, which
nothing else caches.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@laughingman7743
laughingman7743 marked this pull request as ready for review October 3, 2026 11:26
@laughingman7743
laughingman7743 marked this pull request as draft October 3, 2026 12:52
info() parsed the path before its root branch, and parse_path rejects
the root path, so info(""), info("/") and info("s3://") raised
ValueError while exists() returned True for them. fsspec's isdir() and
size() raised it as well. Check the root path before parsing it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@laughingman7743
laughingman7743 marked this pull request as ready for review October 3, 2026 13:00
@laughingman7743
laughingman7743 merged commit 406d090 into master Oct 3, 2026
12 checks passed
@laughingman7743
laughingman7743 deleted the fix/965-980-cached-lookups branch October 3, 2026 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant