Send the lookup parameters of a file with its lookups - #1024
Conversation
S3File looked up the object while opening without the file's request parameters, so an object encrypted with a customer-provided key could not be opened, and RequestPayer and ExpectedBucketOwner were not sent with the lookups. info() and exists() now accept ExpectedBucketOwner, RequestPayer, and the SSECustomer* parameters, send them with the HeadObject, HeadBucket, and ListObjectsV2 requests that accept them, and use only the cached results of lookups with the same values. S3File, pipe_file(mode="create"), and cat_file() pass them to their lookups, and the append reads the existing object with the file's GetObject parameters. Closes #1004 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| """ | ||
| return {k: v for k, v in kwargs.items() if k in _LOOKUP_REQUEST_PARAMETERS} | ||
|
|
||
| def _get_cached_lookup(self, path: str, lookup_kwargs: Mapping[str, Any]) -> S3Object | None: |
There was a problem hiding this comment.
Self-review round 1 (implementation behavior): FINDINGS (1 simplification, repaired)
Scope: base 33d3a07, head 19d79c4. Full diff: pyathena/filesystem/s3.py, docs/filesystem.md, tests/pyathena/filesystem/test_s3.py, and tests/pyathena/filesystem/test_s3_async.py.
Covered:
- Behavior and failure paths:
- read, append, and exclusive-create lookups;
- the append > 5 GiB
info()in_initiate_upload; - the
pipe_file(mode="create")reorder (sameIfNoneMatchprecedence); - the negative-offset
info()incat_file; - 404 eviction of the plain and
(path, "lookups")entries; - versioned and
nullpaths (invalidate_cacheevicts the lookups key for every version spelling); version_awarepinning with params;- the
exists()403 bucket rule (unchanged).
- Callers:
ls,find,checksum,size, andtouchcallinfo/_head_objectwithout params, so they are unchanged. fsspecglobforwards its kwargs toexists/info, and only lookup params are picked from them.AioS3Filereceives_sync_fs, andAioS3FileSystem._info/_existsforward kwargs. - Cache:
- The
DirCacheexpiry anduse_listings_cache=Falsebehavior apply to the new tuple key. - Copy-on-write dict update: a racing writer can only lose an entry, which costs a later cache miss.
_ls_from_cachenever reads the new key.
- The
fs.cat(path)→fs.cat_file(path, ...)in append: same result for a single path, without fsspec's glob expansion.
Finding (simplification): the new _record_lookups test helper duplicated _record_requests.
Repair: fa36dc1 merges them (_record_requests(..., exists=True) answers HeadObject/GetObject). The targeted offline tests and just lint pass.
Limitations:
- The
_initiate_uploadchange (append > 5 GiB) has no test. It hits the cache entry of the open-time lookup. - SSE-C is not exercised live, because the CI bucket blocks SSE-C uploads.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| del self.dircache[key] | ||
|
|
||
| @staticmethod | ||
| def _get_lookup_kwargs(kwargs: Mapping[str, Any]) -> dict[str, Any]: |
There was a problem hiding this comment.
Self-review round 2 (claims, callers, operations): FINDINGS (3 wording, corrected)
Scope: base 33d3a07, head fa36dc1, full claim audit. The corrections are in 04e1f7e and the PR body.
Claims checked:
- HeadObject needs the SSE-C headers to retrieve an SSE-C object's metadata. Confirmed by the botocore HeadObject documentation (uv.lock botocore).
- "each request receives those its operation accepts": the botocore input shapes give
- HeadObject: all 5 params;
- HeadBucket:
ExpectedBucketOwneronly; - ListObjectsV2:
ExpectedBucketOwnerandRequestPayer.
The tests assert these exact requests.
- Wrong
ExpectedBucketOwneron master: open succeeds, the rb read fails, and the ab/xb commit fails withPermissionError: Access Denied. This branch:PermissionErrorat open. Both measured live on the CI bucket. - SSE-C works end to end: not measured, because the bucket blocks SSE-C uploads. The PR body says so.
- A missing
RequestPayernow fails at open: not measured. The PR body now says so.
Corrections:
_get_lookup_kwargs(and the PR body) claimed thatIfMatch"would change the cached result". Conditional params change whether the request succeeds, not the metadata. Reworded to "the result would depend on them" and listedPartNumber.- The release-note item limited the listing bypass to
info()/exists(). It also applies toopen()of a file with these params, e.g. through filesystem-levels3_additional_kwargs, which now sends a HeadObject for a listed path. The PR body now says so. - The docs example used an undefined
key. It now uses theYOUR_32_BYTE_KEYplaceholder, in the style ofYOUR_S3_BUCKET.
Caller and operator review:
- The new
lookup_kwargsparameters of_head_object/_head_bucketare trailing keywords, so positional callers are unchanged. info()/exists()without lookup params take the same path as before.- No retry change.
- Extra requests happen only for lookups with params: one HeadObject per path, parameter set, and cache lifetime.
SSECustomerKeyis kept only as its SHA-256 digest.
Evidence:
The append read the existing object with every GetObject parameter of the file, so a Range among them truncated the object. It now sends only the lookup parameters. A lookup cached with some parameters wrote back a copy of the dictionary, which could replace a result cached in the meantime for other parameters; the dictionary is now updated in place. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| append_data = fs.cat(path) | ||
| # from the buffer. Only the lookup parameters are sent, so | ||
| # that the whole object is read. | ||
| append_data = fs.cat_file(path, **lookup_kwargs) |
There was a problem hiding this comment.
Independent review (relayed): Codex CLI 0.160.0, model gpt-6-astra, reasoning effort high, --sandbox read-only, session 01a1020e-557b-73b3-87a2-0a69ad35783c
This was a static review: no tests, builds, edits, or network. The review was based on base 33d3a07 and head 04e1f7e, in a detached snapshot that Codex confirmed unchanged afterwards. The prompt contained no PR number, PR description, or prior findings.
Covered: request filtering; read/append/exclusive-create opens; pipe_file and ranged cat_file; cache isolation, invalidation, missing-object eviction, versions, and concurrency; async delegation; fsspec integrations; the changed tests, docstrings, and docs/filesystem.md.
Verdict: FINDINGS. 2 were introduced by the diff and 1 is pre-existing.
Finding 1 (P2, introduced): append can silently truncate existing data. The append preload forwarded every accepted GetObject parameter, including Range. Scenario: an existing object b"abcd", fs.open(path, "ab", Range="bytes=0-0"), and a write of b"X". The preload reads only b"a", and the commit writes b"aX" instead of b"abcdX". Previously fs.cat(path) read the whole object.
Verified by the author: confirmed. cat_file without start/end sends no Range of its own, so the file's Range reached GetObject.
There was a problem hiding this comment.
Repair (5d970c3)
The append preload is now fs.cat_file(path, **lookup_kwargs). It sends only the authorization parameters, so the whole existing object is read. test_open_lookup_parameters now opens with Range="bytes=0-0" and asserts that the append's GetObject has no Range. The test fails on 04e1f7e.
Self-review of the repair:
- Round 1 (behavior): GetObject accepts all 5 lookup parameters (botocore shape).
cat_filewithout a range takes no other path. The read and the other append requests are unchanged. - Round 2 (claims): the PR body said the append uses "the file's GetObject parameters". It now says "only these parameters".
Validation: just lint passed, and the live filesystem suite passed (512 tests) at 5d970c3.
| self.dircache[path] = file | ||
| return | ||
| key = (path, _LOOKUPS_CACHE_KEY) | ||
| lookups = self.dircache.get(key) |
There was a problem hiding this comment.
Independent review (relayed, Codex gpt-6-astra, same session as above)
Finding 2 (P2, introduced): concurrent cache updates can undo a completed refresh for another parameter set. Variant A holds stale metadata. Thread B copies the dictionary while caching a lookup with other parameters. Thread A then completes info(path, refresh=True, **params_a) and stores fresh metadata. B writes back its earlier snapshot, which restores A's stale result. This also affects async callers through asyncio.to_thread.
Verified by the author: confirmed. The old copy-on-write in _cache_lookup writes back {**old, new_id: file}. test_cache_lookup_concurrent_parameters reproduces the interleaving deterministically, and it fails on 04e1f7e.
There was a problem hiding this comment.
Repair (5d970c3)
_cache_lookup now updates the (path, "lookups") dictionary in place. It creates the dictionary only when none is cached, so an update for one parameter set never writes back another set's older entry. If two threads create the dictionary at once, one entry is lost, which only causes a later cache miss. test_cache_lookup_concurrent_parameters interleaves a refresh into the update. It fails on 04e1f7e and passes now.
Self-review of the repair:
- Round 1 (behavior): with
use_listings_cache=False,getreturns None and the set is a no-op, so nothing is cached, as before. Readers only calldict.get.invalidate_cacheand the 404 path still evict the whole key. - Round 2 (claims): with
listings_expiry_time, entries added in place expire with the dictionary, which can be earlier. The PR body now states this.
Validation: just lint passed, and the live filesystem suite passed (512 tests) at 5d970c3.
| ) | ||
| except FileNotFoundError: | ||
| self._evict_cache(path) | ||
| self._evict_cache((path, _LOOKUPS_CACHE_KEY)) |
There was a problem hiding this comment.
Independent review (relayed, Codex gpt-6-astra, same session as above)
Finding 3 (P2, pre-existing at the merge-base): a missing-version eviction leaves the alternate query spellings cached. Scenario: cache bucket/key?versionId=v1, delete that version externally, then refresh bucket/key?version_id=v1. The 404 evicts only the second spelling, so a later lookup with versionId still returns the deleted version. The new parameterized cache inherits this.
Author's assessment: confirmed pre-existing at pyathena/filesystem/s3.py:441 (_head_object evicts only the path as spelled). This PR does not change it; it is out of scope. The maintainer will decide whether to file an issue.
There was a problem hiding this comment.
Repair (07c1a14), folded in at the maintainer's request
_head_object now always caches an explicit version under {path without query}?versionId={version_id}, whatever spelling or argument looked it up. One version has one entry, plus one (path, "lookups") entry for parameterized lookups, so a 404 evicts it for every spelling. invalidate_cache still evicts every spelling. parse_path rejects ? in keys, so partition('?') splits off only the version query.
test_info_version_spellings_share_cache[False|True] covers lookups without and with parameters. It checks that 3 spellings share 1 HeadObject, and that after a 404 under ?version_id= the ?versionId= spelling raises FileNotFoundError. It fails without the repair.
Self-review of the repair:
- Round 1 (behavior):
_ls_from_cachereads the path as spelled. A non-canonical spelling now misses there and falls through toinfo(), which hits the canonical entry.exists()andinfo()already skip listings for versions. Thenullversion is still never cached. - Round 2 (claims): the
_head_objectdocstring now says the key is spelled?versionId=. The PR body has a WHAT item and a release-note item for it.
Validation: just lint passed. With dummy credentials, the set of offline-failing (live-only) tests is identical to before. The live filesystem suite passed (515 tests) at 07c1a14. An independent follow-up is requested.
There was a problem hiding this comment.
Independent follow-up review 4 (relayed): Codex CLI 0.160.0, gpt-6-astra, effort high, --sandbox read-only, session 01a1026c-748a-7110-9e4b-0b5a76b91d6c
Scope: the repair b175d8b..07c1a14 (merge-base unchanged at 33d3a07). The snapshot was confirmed at 07c1a14.
Covered:
_head_object: all four spellings andversion_id=map to one canonical key before the cache read, the insert, and the 404 eviction. Both the plain and the(path, "lookups")entries are evicted, andnullstays uncached.info,exists,ls, and_ls_from_cache: a miss under another spelling falls through to the canonical lookup.version_awarereads andS3Fileversioned opens.invalidate_cache.AioS3FileSystem, which shares the sync dircache.- The new regression test.
Verdict: CLEAN. Without the repair, both test parameterizations fail at the alternate-spelling lookup.
This was a static review only.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| # another thread has cached since for other parameters. | ||
| lookups[self._get_lookup_cache_id(lookup_kwargs)] = file | ||
| # Set again so that the expiry time of the entry is renewed. | ||
| self.dircache[key] = lookups |
There was a problem hiding this comment.
Independent follow-up review (relayed): Codex CLI 0.160.0, gpt-6-astra, effort high, --sandbox read-only, session 01a10217-df08-7bc1-a33a-d79cb1a2313c
This was a static review of the repair 04e1f7e..5d970c3 (merge-base unchanged at 33d3a07). The snapshot was confirmed unchanged afterwards.
Covered: S3File read/append/exclusive-create, cat_file, info/exists, AioS3File through the sync filesystem, parameter-specific caching, invalidation/404 eviction, and the DirCache settings.
Verdict: FINDINGS.
- Both original findings are resolved. Static inspection confirmed that both regression tests would fail without their repairs.
- New P2: updating the lookup map in place bypassed
DirCache.__setitem__, so a refresh no longer renewed the expiry. Scenario:listings_expiry_time=60, cache at t=0, refresh at t=59. A lookup at t=61 evicts the fresh result and sends another HEAD.
Author verification: confirmed.
Repair (01b8b6a): the dictionary is now set again after the in-place update. test_cache_lookup_renews_expiry uses a real DirCache and a patched clock. It fails on 5d970c3 and passes now. test_cache_lookup_concurrent_parameters still passes. just lint passed, and the live filesystem suite passed (513 tests) at 01b8b6a.
Self-review of the repair:
- Round 1: the expiry is per path. Caching one parameter set renews the other sets' entries for that path, so one of them can outlive
listings_expiry_timewhile another set keeps being refreshed. Also, aninvalidate_cache()that runs between the read and the set can be undone. This is the same class of race as the in-flight stale write on the plain path entry. - Round 2: the PR body claimed entries "expire with the dictionary, i.e. possibly earlier". It now describes the per-path expiry and the race.
The per-path expiry is a design trade-off, and the maintainer will decide on it. A further independent follow-up is pending that decision.
There was a problem hiding this comment.
Repair (cc9d5bd): per-parameter-set expiry. This is the maintainer's choice over keeping per-path expiry.
Each value in the (path, "lookups") dictionary is now (time cached, S3Object). _get_cached_lookup treats a result older than dircache.listings_expiry_time as missing. The dictionary is still set again on each cache write, so the DirCache entry itself is renewed (the Codex follow-up finding).
test_cache_lookup_expiry uses a real DirCache and a patched clock. It checks that a refresh at t=59 survives at t=61, and that one set's result expires at t=120 while another set keeps renewing the path entry. It fails on 5d970c3 and on 01b8b6a.
_make_fs now builds a real DirCache. With dummy credentials, the set of failing (live-only) tests is identical before and after (112).
Self-review of the repair:
- Round 1: production dircaches are always
DirCache(AbstractFileSystem.__init__;AioS3FileSystemshares the sync one). Withlistings_expiry_time=None, nothing changes. An expired result is replaced by the next cache write. The invalidate-vs-write race noted above is unchanged. - Round 2: the PR body's expiry description is updated.
Validation: just lint passed, and the live filesystem suite passed (513 tests) at cc9d5bd. A further independent follow-up is now requested.
Caching the lookup of one set of parameters renews the dircache entry of the path, which extended the other sets' results beyond listings_expiry_time. Each result now keeps the time it was cached and expires on its own. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| # another thread has cached since for other parameters. | ||
| lookups[self._get_lookup_cache_id(lookup_kwargs)] = (time.time(), file) | ||
| # Set again so that the expiry time of the entry is renewed. | ||
| self.dircache[key] = lookups |
There was a problem hiding this comment.
Independent follow-up review 2 (relayed): Codex CLI 0.160.0, gpt-6-astra, effort high, --sandbox read-only, session 01a10226-52e7-7e80-91bb-0470da79a7e1
This was a static review of the repair 5d970c3..cc9d5bd (merge-base unchanged at 33d3a07). The snapshot was confirmed clean.
Covered: the repair diff; fsspec 2026.9.0 DirCache; disabled caching; expiry None/0/positive; max_paths; invalidation and 404 eviction; concurrent updates; async sharing; the changed tests and _make_fs consumers.
Result: the renewal defect is fixed, and expiry applies to each parameter set on its own. Disabled caching, non-expiring entries, zero expiry, and max_paths keep the DirCache behavior. The new expiry test fails without the repair.
Verdict: FINDINGS.
Finding 1 (P2): the set-back can resurrect evicted results. Thread A reads dictionary D while caching set Q. Thread B invalidates the path and caches fresh P in a new dictionary. A then sets D back, which restores the stale P result. A concurrent 404 eviction can be undone the same way.
Author verification: confirmed, but it requires 3 overlapping operations on the same path and filesystem instance, all within the microseconds between A's get and its set.
Decision: won't fix, no code change (the maintainer questioned whether this case occurs in practice; the author recommends won't fix, and the maintainer can object). This is the same class as the in-flight stale write on the plain path entry, which #1002 left as won't fix. That race has a much larger window (a HeadObject round trip). This one has no new exposure in practice.
| {"Bucket": "bucket", "Key": "key", **other_key}, | ||
| ] | ||
| # The cache does not keep the customer-provided keys. | ||
| assert "k" * 32 not in repr(dict(fs.dircache)) |
There was a problem hiding this comment.
Independent follow-up review 2 (relayed, same session)
Finding 2 (P3): after _make_fs switched to DirCache, repr(fs.dircache) no longer shows the contents. As a result, the assertions that the cache does not keep a plaintext SSECustomerKey would pass even after a regression.
Author verification: confirmed. "k" * 32 in repr(DirCache) is False even when the key is stored, while repr(dict(...)) shows it.
Repair (b175d8b): the assertions now inspect repr(dict(fs.dircache)). This is a test-only change. test_info_lookup_parameters_cache and just lint pass.
There was a problem hiding this comment.
Independent follow-up review 3 (relayed): Codex CLI 0.160.0, gpt-6-astra, effort high, --sandbox read-only, session 01a1022b-6faf-7b52-8223-eb16d83f66b3
Scope: the repair cc9d5bd..b175d8b (test-only). The review covered the repaired assertions together with _cache_lookup/_get_lookup_cache_id.
Verdict: CLEAN. repr(dict(fs.dircache)) recursively shows the nested dictionary keys and tuples, so a plaintext SSECustomerKey would fail the assertions.
This was a static review only.
A version looked up with ?version_id= and with ?versionId= was cached twice, and a missing version evicted only the spelling that looked it up, so the other spelling kept returning the deleted version. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WHAT
The lookups made while opening an
S3Filenow send the file's request parameters that their authorization depends on, and theinfo()cache keeps the results of such lookups apart for each set of values.info()andexists()acceptExpectedBucketOwner,RequestPayer,SSECustomerAlgorithm,SSECustomerKey, andSSECustomerKeyMD5as keyword arguments. Each HeadObject, HeadBucket, or ListObjectsV2 request receives those its operation accepts (S3FileSystem._get_operation_kwargs). Other request parameters are ignored, as before.(path, "lookups")as a dictionary keyed by the sorted parameters. The dictionary is updated in place and then set again, so a concurrent update for other parameters cannot write back an older copy, and caching renews thelistings_expiry_timeof the entry. Each cached result also keeps the time it was cached and expires on its own afterlistings_expiry_time(maintainer choice), so results of other parameters that keep being refreshed do not extend it. Aninvalidate_cache()of the path that runs between the read and the set of the dictionary can be undone, which is the same class of race as the stale write of an in-flight request on the plain path entry.SSECustomerKeyis stored as its SHA-256 digest, so the cache does not keep the key. Lookups with these parameters do not use cached listings or the plain path entry. Lookups without them work as before.invalidate_cache()and a 404 from HeadObject/HeadBucket evict these entries together with the plain path entry.IfMatch/If*,Range,PartNumber,ChecksumMode,Response*), because the result of the request would depend on them.S3Filepasses the parameters to the read lookup, the append lookup, the exclusive-create existence check, and theinfo()of an append of more than 5 GiB. The append reads an existing object smaller than 5 MiB withcat_file()and only these parameters, so the whole object is read even if the file has, e.g., aRange(previouslyfs.cat(path)without any parameters).pipe_file(mode="create")sends the write's lookup parameters with its existence check, andcat_file()sends them with theinfo()that resolves negative offsets._head_objectcaches an explicit version under the?versionId=spelling, whatever spelling (versionId,versionID,versionid,version_id, or theversion_idargument) looked it up. Previously each spelling had its own entry, and a missing version (404) evicted only the spelling that looked it up, so another spelling kept returning the deleted version. This also applies to the new(path, "lookups")entries.docs/filesystem.mddocuments the parameters and the cache behavior.Release-note items (behavior changes):
open()inrb/ab/xbmode,pipe_file(mode="create"), andinfo()/exists()with these parameters now send them with their lookups. A mismatchedExpectedBucketOwnernow raisesPermissionErrorwhen the file is opened. Previously the file opened, and the failure came at the first read or at commit (both measured). A missingRequestPayeron a requester-pays bucket should now fail at open in the same way, but this was not measured.?versionId=is cached once, and a missing version is no longer served from the cache under another spelling.open()of a file that has them, for example through the filesystem-levels3_additional_kwargs. The first such lookup of a listed path therefore sends a HeadObject request.Unchanged and out of scope:
exists()of a bucket still returns True when HeadBucket answers 403, which includes a mismatchedExpectedBucketOwner(rule from Answer info() and exists() from cached parent listings and fix bucket lookups #1006).s3_additional_kwargsstill go only to the requests of files and writes. A directfs.info(path)does not receive them.CopySourceSSECustomer*parameters. The file does not send them, as on master.WHY
Closes #1004.
S3Filelooked up the object without the file's parameters. For an SSE-C object, the HeadObject reference requires thex-amz-server-side-encryption-customer-*headers, so these opens failed.RequestPayerandExpectedBucketOwnerwere not sent with the lookups either.TEST
Tested commit 07c1a14.
just lint: passed.uv run --env-file .env pytest -n 2 tests/pyathena/filesystem/: 515 passed (real S3)._callwith the botocore service model:test_open_lookup_parameters[rb|ab|xb],test_info_lookup_parameters_cache,test_info_lookup_parameters_missing_object,test_exists_bucket_lookup_parameters,test_pipe_file_create_lookup_parameters,test_cat_file_range_lookup_parameters, and the aioTestAioS3File::test_open_lookup_parameters. All 9 failed with the test files copied onto master 33d3a07.test_cache_lookup_concurrent_parametersand theRangeassertion oftest_open_lookup_parameters[ab](added after the independent review) fail with the implementation before 5d970c3, andtest_cache_lookup_expiryfails on 5d970c3 and 01b8b6a.test_info_version_spellings_share_cache[False|True]fails without 07c1a14._make_fsin the tests now uses a realDirCacheinstead of a dict; the set of offline-failing (live-only) tests with dummy credentials is identical before and after.ExpectedBucketOwnerset to the caller's account and to111122223333:openrb/ab/xb,info, andexistsof a missing key raisePermissionError(HeadObject 403 Forbidden). With the right owner, read and append succeed, andexistsof a missing key is False.PermissionError: Access Denied, and the ab/xb files fail at commit with the same error.🤖 Generated with Claude Code