Skip to content

Send the lookup parameters of a file with its lookups - #1024

Merged
laughingman7743 merged 8 commits into
masterfrom
fix/1004-open-lookup-params
Oct 3, 2026
Merged

laughingman7743 merged 8 commits into
masterfrom
fix/1004-open-lookup-params

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

WHAT

The lookups made while opening an S3File now send the file's request parameters that their authorization depends on, and the info() cache keeps the results of such lookups apart for each set of values.

  • info() and exists() accept ExpectedBucketOwner, RequestPayer, SSECustomerAlgorithm, SSECustomerKey, and SSECustomerKeyMD5 as keyword arguments. Each HeadObject, HeadBucket, or ListObjectsV2 request receives those its operation accepts (S3FileSystem._get_operation_kwargs). Other request parameters are ignored, as before.
  • Cache (maintainer choice: the authorization parameters are part of the cache key): a lookup with these parameters uses only a HeadObject/HeadBucket result of a lookup with the same values, cached under (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 the listings_expiry_time of the entry. Each cached result also keeps the time it was cached and expires on its own after listings_expiry_time (maintainer choice), so results of other parameters that keep being refreshed do not extend it. An invalidate_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. SSECustomerKey is 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.
  • Lookups send only these parameters, not everything HeadObject accepts (IfMatch/If*, Range, PartNumber, ChecksumMode, Response*), because the result of the request would depend on them.
  • S3File passes the parameters to the read lookup, the append lookup, the exclusive-create existence check, and the info() of an append of more than 5 GiB. The append reads an existing object smaller than 5 MiB with cat_file() and only these parameters, so the whole object is read even if the file has, e.g., a Range (previously fs.cat(path) without any parameters).
  • Folded in because they have the same defect: pipe_file(mode="create") sends the write's lookup parameters with its existence check, and cat_file() sends them with the info() that resolves negative offsets.
  • Folded in at the maintainer's request (pre-existing, found by the independent review): _head_object caches an explicit version under the ?versionId= spelling, whatever spelling (versionId, versionID, versionid, version_id, or the version_id argument) 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.md documents the parameters and the cache behavior.

Release-note items (behavior changes):

  • open() in rb/ab/xb mode, pipe_file(mode="create"), and info()/exists() with these parameters now send them with their lookups. A mismatched ExpectedBucketOwner now raises PermissionError when the file is opened. Previously the file opened, and the failure came at the first read or at commit (both measured). A missing RequestPayer on a requester-pays bucket should now fail at open in the same way, but this was not measured.
  • Objects encrypted with SSE-C can now be opened when their metadata is not cached.
  • A version looked up with different spellings of ?versionId= is cached once, and a missing version is no longer served from the cache under another spelling.
  • Lookups with these parameters do not use cached listings or the cache entries of lookups without them. This includes open() of a file that has them, for example through the filesystem-level s3_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 mismatched ExpectedBucketOwner (rule from Answer info() and exists() from cached parent listings and fix bucket lookups #1006).
  • Filesystem-level s3_additional_kwargs still go only to the requests of files and writes. A direct fs.info(path) does not receive them.
  • Appending to an SSE-C object of 5 MiB or more copies it with UploadPartCopy, which needs the CopySourceSSECustomer* parameters. The file does not send them, as on master.

WHY

Closes #1004. S3File looked up the object without the file's parameters. For an SSE-C object, the HeadObject reference requires the x-amz-server-side-encryption-customer-* headers, so these opens failed. RequestPayer and ExpectedBucketOwner were 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).
  • New offline tests that use a recording _call with 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 aio TestAioS3File::test_open_lookup_parameters. All 9 failed with the test files copied onto master 33d3a07. test_cache_lookup_concurrent_parameters and the Range assertion of test_open_lookup_parameters[ab] (added after the independent review) fail with the implementation before 5d970c3, and test_cache_lookup_expiry fails on 5d970c3 and 01b8b6a. test_info_version_spellings_share_cache[False|True] fails without 07c1a14. _make_fs in the tests now uses a real DirCache instead of a dict; the set of offline-failing (live-only) tests with dummy credentials is identical before and after.
  • A live script on the CI staging bucket compared this branch with master 406d090, using ExpectedBucketOwner set to the caller's account and to 111122223333:
    • Branch: with the wrong owner, open rb/ab/xb, info, and exists of a missing key raise PermissionError (HeadObject 403 Forbidden). With the right owner, read and append succeed, and exists of a missing key is False.
    • Master: with the wrong owner, all three opens succeed. A read from the rb file fails with PermissionError: Access Denied, and the ab/xb files fail at commit with the same error.
  • Not run: a real SSE-C object. The CI bucket rejects SSE-C uploads ("this bucket has blocked upload requests that specify Server Side Encryption with Customer provided keys"). SSE-C is covered only by the offline tests.

🤖 Generated with Claude Code

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>
Comment thread pyathena/filesystem/s3.py
"""
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:

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): 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 (same IfNoneMatch precedence);
    • the negative-offset info() in cat_file;
    • 404 eviction of the plain and (path, "lookups") entries;
    • versioned and null paths (invalidate_cache evicts the lookups key for every version spelling);
    • version_aware pinning with params;
    • the exists() 403 bucket rule (unchanged).
  • Callers: ls, find, checksum, size, and touch call info/_head_object without params, so they are unchanged. fsspec glob forwards its kwargs to exists/info, and only lookup params are picked from them. AioS3File receives _sync_fs, and AioS3FileSystem._info/_exists forward kwargs.
  • Cache:
    • The DirCache expiry and use_listings_cache=False behavior 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_cache never reads the new key.
  • 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_upload change (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>
Comment thread pyathena/filesystem/s3.py
del self.dircache[key]

@staticmethod
def _get_lookup_kwargs(kwargs: Mapping[str, Any]) -> dict[str, Any]:

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): 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: ExpectedBucketOwner only;
    • ListObjectsV2: ExpectedBucketOwner and RequestPayer.
      The tests assert these exact requests.
  • Wrong ExpectedBucketOwner on master: open succeeds, the rb read fails, and the ab/xb commit fails with PermissionError: Access Denied. This branch: PermissionError at 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 RequestPayer now fails at open: not measured. The PR body now says so.

Corrections:

  1. _get_lookup_kwargs (and the PR body) claimed that IfMatch "would change the cached result". Conditional params change whether the request succeeds, not the metadata. Reworded to "the result would depend on them" and listed PartNumber.
  2. The release-note item limited the listing bypass to info()/exists(). It also applies to open() of a file with these params, e.g. through filesystem-level s3_additional_kwargs, which now sends a HeadObject for a listed path. The PR body now says so.
  3. The docs example used an undefined key. It now uses the YOUR_32_BYTE_KEY placeholder, in the style of YOUR_S3_BUCKET.

Caller and operator review:

  • The new lookup_kwargs parameters of _head_object/_head_bucket are 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.
  • SSECustomerKey is kept only as its SHA-256 digest.

Evidence:

  • The 9 new offline tests failed on master 33d3a07.
  • Live filesystem suite: 511 passed at 19d79c4.
  • Later commits change only tests and wording. At 04e1f7e, just lint and the offline tests were rerun.

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>
Comment thread pyathena/filesystem/s3.py
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)

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, 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.

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 (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_file without 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.

Comment thread pyathena/filesystem/s3.py
self.dircache[path] = file
return
key = (path, _LOOKUPS_CACHE_KEY)
lookups = self.dircache.get(key)

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 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.

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 (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, get returns None and the set is a no-op, so nothing is cached, as before. Readers only call dict.get. invalidate_cache and 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.

Comment thread pyathena/filesystem/s3.py
)
except FileNotFoundError:
self._evict_cache(path)
self._evict_cache((path, _LOOKUPS_CACHE_KEY))

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 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.

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 (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_cache reads the path as spelled. A non-canonical spelling now misses there and falls through to info(), which hits the canonical entry. exists() and info() already skip listings for versions. The null version is still never cached.
  • Round 2 (claims): the _head_object docstring 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.

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 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 and version_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, and null stays uncached.
  • info, exists, ls, and _ls_from_cache: a miss under another spelling falls through to the canonical lookup.
  • version_aware reads and S3File versioned 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>
Comment thread pyathena/filesystem/s3.py
# 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

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): 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_time while another set keeps being refreshed. Also, an invalidate_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.

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 (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__; AioS3FileSystem shares the sync one). With listings_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.

laughingman7743 and others added 2 commits October 3, 2026 23:20
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>
Comment thread pyathena/filesystem/s3.py
# 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

@laughingman7743 laughingman7743 Oct 3, 2026 •

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 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))

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 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.

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 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.

@laughingman7743
laughingman7743 marked this pull request as ready for review October 3, 2026 14:31
@laughingman7743
laughingman7743 marked this pull request as draft October 3, 2026 15:36
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>
@laughingman7743
laughingman7743 marked this pull request as ready for review October 3, 2026 15:42
@laughingman7743
laughingman7743 merged commit a73c6db into master Oct 3, 2026
14 checks passed
@laughingman7743
laughingman7743 deleted the fix/1004-open-lookup-params branch October 3, 2026 16:02
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.

S3File does not send its request parameters with the lookups made while opening

1 participant