paulcaron16k opened a new issue, #3896:
URL: https://github.com/apache/iceberg-python/issues/3896
### Apache Iceberg version
main (development) — also reproduced on 0.12.0 and 0.11.1.
### Please describe the bug 🐞
**Table commits fail intermittently against a remote-signing catalog,
because the request signer is unregistered while other threads are signing.**
Roughly **3–4% of appends** come back `403 AccessDenied` under PyIceberg's
own concurrent write path. PyIceberg's writer is concurrent by default, so no
unusual usage is needed to reach this.
It surfaces as an opaque
```
PermissionError: Access Denied
```
raised out of `s3fs`, several frames from its cause, with nothing tying it
to signing. Every unsigned request we caught on the wire was a `PUT` of a
**manifest** (`*-m0.avro`) during commit — 5 of 5 and 3 of 3 across two runs —
so what fails is the commit itself, not a stray read.
Environment: Lakekeeper 0.13.1 vending remote signing, MinIO,
`s3.path-style-access=true`.
---
## Root cause
`pyiceberg/io/fsspec.py::_s3` installs the signer like this (lines 234–238
on `main`; the unregister/register pair is 237–238):
```python
fs = S3FileSystem(**s3_fs_kwargs)
for event_name, event_function in register_events.items():
fs.s3.meta.events.unregister(event_name, unique_id=1925)
fs.s3.meta.events.register_last(event_name, event_function,
unique_id=1925)
```
fsspec caches filesystem instances, so `fs` — and the botocore client and
event emitter hanging off it — is **shared by every thread**. Between the
`unregister` and the `register_last` **no signer is installed**, and `_s3()`
has already set `config_kwargs["signature_version"] = UNSIGNED`, so botocore
will not sign in its place. A request signed in that window goes out with **no
`Authorization` header at all**, and the store answers `403 AccessDenied`.
`FsspecFileIO.get_fs` caches per thread and every table gets its own
`FileIO`, so a short workload performs **over a hundred** unregister/register
cycles against that one shared emitter, each racing whatever writes are in
flight.
The response code is the clue that took us longest to read correctly: it is
always `AccessDenied` and never `SignatureDoesNotMatch` — which is what an
**unsigned** request produces, not a mis-signed one.
## Evidence
90 writes per run against a live stack, counting requests that reached the
wire without an `Authorization` header (a `before-send.s3` hook):
| configuration | append failures | requests sent unsigned |
|---|---|---|
| as shipped | 2 / 4 / 5 | 2 / 4 / 5 |
| fsspec instance cache disabled (133 separate clients) | 0 | 0 |
| **signer registered once, never unregistered (one shared client)** | **0**
| **0** |
| `PYICEBERG_MAX_WORKERS=1` | 0 | 0 |
Unsigned-on-wire equals append failures **exactly**, run after run.
The third row is the one that matters: the failures stop **while the client
is still shared**, which separates this from a general concurrency problem.
## Suggested fix
One line, same place — drop the `unregister`:
```python
for event_name, event_function in register_events.items():
fs.s3.meta.events.register_last(event_name, event_function,
unique_id=1925)
```
`HierarchicalEmitter._register_section` already returns early for a
`unique_id` it holds:
```python
if unique_id in self._unique_id_handlers:
# We've already registered a handler using this unique_id
# so we don't need to register it again.
...
return
```
so re-registering an equivalent signer was **already** a no-op, and the
`unregister` only opens the window. Every signer `_s3()` builds derives from
the same `properties`, so which instance stays installed does not matter — only
that one always is.
## Reproduction
Fork and branch: **[paulcaron16k/iceberg-python @
`bug/s3v4restsigner_unregisters_signer_mid_flight`](https://github.com/paulcaron16k/iceberg-python/tree/bug/s3v4restsigner_unregisters_signer_mid_flight)**
— one commit on top of current `main`, adding
`tests/io/test_fsspec_signer_registration.py`. Seven tests, **no credentials
and no network**.
The one that reads the library is
`test_s3_unregisters_the_signer_on_a_client_it_shares`: two `_s3()` calls,
fsspec hands back the same filesystem, and `unregister` is caught on **both** —
the second while a perfectly good signer is installed. **It fails once the
`unregister` is dropped**, which is what ties it to the fix rather than to
incidental structure. The four existing signer tests in
`tests/io/test_fsspec.py` pass before and after.
## A second, independent defect in the same file
Reported here because it is four lines away, but it is **not** the cause of
the above and can be fixed separately.
`pyiceberg/io/fsspec.py:160` applies the signing service's headers with
`add_header`, which **appends**:
```python
for key, value in response_json["headers"].items():
request.headers.add_header(key, ", ".join(value))
```
A signing service echoes back headers it was given as well as those it
computed, so every echoed header ends up on the request twice. Instrumenting a
real workload, **880 of 1615** sign calls ended with a duplicate of a header
named in `SignedHeaders` — `expect`, `x-amz-checksum-crc32`,
`x-amz-sdk-checksum-algorithm` on `PutObject`; `if-match`, `range`,
`x-amz-checksum-mode` on `HeadObject`/`GetObject`.
In fairness: on MinIO this is **latent**. De-duplicating the headers left
the failure count unchanged, and only the registration fix above stopped the
403s. But SigV4 combines repeated headers comma-separated, so a stricter
implementation is entitled to reject them, and the signing service's header set
is authoritative regardless.
It is invisible to the current tests because they assert on
`dict(request.headers)`, and `dict()` collapses repeated keys to one value —
any regression test needs `get_all`.
Suggested fix, replace rather than append:
```python
for key, value in response_json["headers"].items():
if key in request.headers:
del request.headers[key]
request.headers.add_header(key, ", ".join(value))
```
## Related, not proposed here
The fsspec cache key comes from `s3_fs_kwargs`, which does **not** include
the signer's URI or endpoint — so two catalogs sharing an S3 endpoint and
credentials but signing through different services share one filesystem. Today
the last `_s3()` caller's signer wins; with the fix above the first one does.
Neither is correct. The complete answer is for the signer configuration to
participate in the filesystem's identity, but that changes which objects are
cached, so we have left it as a separate decision rather than folding it in.
### Willingness to contribute
- [x] I would be willing to contribute a fix for this bug with guidance from
the Iceberg community
The branch above already carries the tests, and the one-line fix is
validated against a live stack. Happy to open a PR for either or both defects —
guidance welcome on whether you want them split, and on the cache-key question.
Contact: [email protected]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
