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):
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:
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:
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 β 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:
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:
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
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.
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 AccessDeniedunder 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
raised out of
s3fs, several frames from its cause, with nothing tying it to signing. Every unsigned request we caught on the wire was aPUTof 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::_s3installs the signer like this (lines 234β238 onmain; the unregister/register pair is 237β238):fsspec caches filesystem instances, so
fsβ and the botocore client and event emitter hanging off it β is shared by every thread. Between theunregisterand theregister_lastno signer is installed, and_s3()has already setconfig_kwargs["signature_version"] = UNSIGNED, so botocore will not sign in its place. A request signed in that window goes out with noAuthorizationheader at all, and the store answers403 AccessDenied.FsspecFileIO.get_fscaches per thread and every table gets its ownFileIO, 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
AccessDeniedand neverSignatureDoesNotMatchβ 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
Authorizationheader (abefore-send.s3hook):PYICEBERG_MAX_WORKERS=1Unsigned-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:HierarchicalEmitter._register_sectionalready returns early for aunique_idit holds:so re-registering an equivalent signer was already a no-op, and the
unregisteronly opens the window. Every signer_s3()builds derives from the sameproperties, 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β one commit on top of currentmain, addingtests/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, andunregisteris caught on both β the second while a perfectly good signer is installed. It fails once theunregisteris dropped, which is what ties it to the fix rather than to incidental structure. The four existing signer tests intests/io/test_fsspec.pypass 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:160applies the signing service's headers withadd_header, which appends: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-algorithmonPutObject;if-match,range,x-amz-checksum-modeonHeadObject/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), anddict()collapses repeated keys to one value β any regression test needsget_all.Suggested fix, replace rather than append:
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
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.