Add credentials refresh in more locations#332
Conversation
Secret refresh can be triggered by multiple sub-requests within one statement; make SQLLogicTests assert presence rather than exact row count. Follow-up: consider per-statement/per-secret refresh rate limiting (deferred).
|
I built It adds (1) a deterministic sqllogictest covering the open/glob/range-GET/write paths (passes on your branch, fails on One thing to flag for the |
duckdb#332 made ExecuteWithRefresh take the handle's S3AuthParams by reference and TryRefreshCredentials() mutate its credential strings in place. Multiple threads share one auth_params (parallel scan range GETs on one S3FileHandle; multipart part PUTs on detached threads sharing one S3HTTPInput), so on credential expiry concurrent refreshes raced on the std::string fields (UB) and fired redundant STS calls. Make the refresh single-flight and synchronized: - S3AuthParams gains a copy-safe shared_ptr<mutex> + a generation counter and a refresh_last_succeeded flag (default-initialized so every value-init path has a non-null lock). shared_ptr (not a bare mutex) keeps S3AuthParams copyable. - TryRefreshCredentials writes the credential fields only under the lock. RefreshCredentialsLocked advances the generation on every completed attempt and records success; the (captured_generation) overload reuses a peer's completed result instead of firing a duplicate STS call, while a later request still gets to retry. - ExecuteWithRefresh snapshots auth_params together with its generation under the lock and signs against the snapshot, so signing never tears against a refresh and the request knows exactly which generation it used. The snapshot also covers the request lambda's own reads (e.g. oauth2_bearer_token). A region correction made on the snapshot (glob 301 path) is propagated back to the shared object. - GetHTTPError and S3FileHandle::CreateClient read region/endpoint via Snapshot() so the error/client-creation paths don't race a concurrent refresh. Initialize's direct callers keep the no-arg TryRefreshCredentials() (single-threaded open). The any>=400 refresh trigger is unchanged. Existing secret-refresh and upload tests pass; the fixed build is ThreadSanitizer- clean on a concurrent parquet-scan refresh workload. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Follow-up: I've added the single-flight / thread-safety piece for the shared |
3 participants
Supersedes #165.
Rebased
auto_refresh_fixonto currentmain.TODO follow-ups:
RemoveFilesrefresh support