These are left over from the review of #6025, which adds a process-wide web-identity credential provider for the native Iceberg S3 path in native/core/src/cloud/s3/web_identity.rs. We agreed they could land separately so that PR isn't held up on them.
The one with user impact is how readers behave during a refresh. Once the cached credential is inside minTtlSeconds of expiry (300s by default), SharedEntry::credentials sends every caller to refresh_lock. The holder then runs AssumeRoleWithWebIdentity through the SDK's retry loop, which under a throttle is five attempts with up to about 15 seconds of backoff. Every other caller waits behind it, even though still_signable() would give them a credential with minutes left. That covers every native Iceberg file operation in the executor, because iceberg-storage-opendal builds a new operator for each storage call (exists, metadata, read, reader, write and the rest). Each new operator has a fresh reqsign Signer with an empty cache, so each call comes back to our provider.
The commit that fixed the signing dead zone (df2197b on the current branch) also removed the refresh jitter and the refreshJitterSeconds knob, and the PR thread doesn't say why. Before that, each entry added up to 60 seconds of random jitter to minTtlSeconds, so executors that assumed the role in the same startup burst didn't all refresh in the same second an hour later. Now they do. Single-flight keeps that to one STS call per executor, but with the stall above, a throttled refresh would stall every executor at once. The jitter only moves our own refresh earlier, and the provider now reports the real expiry, so bringing it back can't reopen the dead zone. The comment in build_s3_credential_loader in iceberg_common.rs still describes a "shared jittered cache".
Two smaller things in the tests. concurrent_failed_refresh_is_coalesced says every waiter must see the real cause, but it only asserts on web-identity assume-role failed, which credentials() prepends itself and the cooldown replay keeps. It would still pass if the replay dropped the cause. And two comments in iceberg_wiring_reads_s3_prefixed_keys say a bare comet.credential.webIdentity.enabled key never reaches the catalog property bag. It does reach it, because Comet forwards the unfiltered FileIO properties, and it has no effect only because the lookup adds the s3. prefix. The doc comment on the key constants and the user guide already say that correctly.
The work:
- While a refresh is in flight, callers that don't hold
refresh_lock get the cached credential if it is still signable, instead of waiting on STS.
- Bring back a per-entry refresh jitter, or explain in the code why it isn't needed, and fix the "jittered cache" comment either way.
- Make
concurrent_failed_refresh_is_coalesced assert on the stub provider's own error text ("the credential provider was not enabled").
- Fix the two bare-key comments in
iceberg_wiring_reads_s3_prefixed_keys.
Done when a test with a slow, throttled refresh shows the other callers getting the cached credential without waiting, and the other three items are in.
These are left over from the review of #6025, which adds a process-wide web-identity credential provider for the native Iceberg S3 path in
native/core/src/cloud/s3/web_identity.rs. We agreed they could land separately so that PR isn't held up on them.The one with user impact is how readers behave during a refresh. Once the cached credential is inside
minTtlSecondsof expiry (300s by default),SharedEntry::credentialssends every caller torefresh_lock. The holder then runsAssumeRoleWithWebIdentitythrough the SDK's retry loop, which under a throttle is five attempts with up to about 15 seconds of backoff. Every other caller waits behind it, even thoughstill_signable()would give them a credential with minutes left. That covers every native Iceberg file operation in the executor, becauseiceberg-storage-opendalbuilds a new operator for each storage call (exists,metadata,read,reader,writeand the rest). Each new operator has a fresh reqsignSignerwith an empty cache, so each call comes back to our provider.The commit that fixed the signing dead zone (df2197b on the current branch) also removed the refresh jitter and the
refreshJitterSecondsknob, and the PR thread doesn't say why. Before that, each entry added up to 60 seconds of random jitter tominTtlSeconds, so executors that assumed the role in the same startup burst didn't all refresh in the same second an hour later. Now they do. Single-flight keeps that to one STS call per executor, but with the stall above, a throttled refresh would stall every executor at once. The jitter only moves our own refresh earlier, and the provider now reports the real expiry, so bringing it back can't reopen the dead zone. The comment inbuild_s3_credential_loaderiniceberg_common.rsstill describes a "shared jittered cache".Two smaller things in the tests.
concurrent_failed_refresh_is_coalescedsays every waiter must see the real cause, but it only asserts onweb-identity assume-role failed, whichcredentials()prepends itself and the cooldown replay keeps. It would still pass if the replay dropped the cause. And two comments iniceberg_wiring_reads_s3_prefixed_keyssay a barecomet.credential.webIdentity.enabledkey never reaches the catalog property bag. It does reach it, because Comet forwards the unfiltered FileIO properties, and it has no effect only because the lookup adds thes3.prefix. The doc comment on the key constants and the user guide already say that correctly.The work:
refresh_lockget the cached credential if it is still signable, instead of waiting on STS.concurrent_failed_refresh_is_coalescedassert on the stub provider's own error text ("the credential provider was not enabled").iceberg_wiring_reads_s3_prefixed_keys.Done when a test with a slow, throttled refresh shows the other callers getting the cached credential without waiting, and the other three items are in.