refactor(auth): use injected executor for token refresh by wgtmac · Pull Request #1033 · apache/iceberg-cpp

wgtmac · 2026-10-17T16:55:10Z

Replace the auth-specific token refresh scheduler with an internal utility that shares one process-lifetime timer and submits due work through a caller-owned Executor.

Accept borrowed executors through RestCatalog::Make(config, client_factory, auth_executor, metrics_executor) and the optional final argument of AuthSession::MakeOAuth2. Without an auth executor, background refresh is disabled.
Keep cancellation and shutdown scoped to each scheduler. A single session control mutex serializes task installation and closing; HTTP and dispatch retries retain separate budgets.
Keep the shared timer alive during static destruction. Its process lifetime removes the need for global shutdown, self-ownership and join/detach layers.
Use file-local executor mocks and standard futures in tests. No new shared test helper, and no changes to the existing ThreadExecutor.

Validation:

REST tests: 484 passed, 1 existing skip. Scheduler tests: 6 passed, including static-destruction, cancellation/rejection, callback teardown and active-submit shutdown regressions.
Scheduler suite passed 20 shuffled ASAN/UBSAN repetitions; four key OAuth/catalog tests passed 30 shuffled repetitions.
Core/REST shared libraries built successfully. Repository clang-tidy checks (LLVM 21) and pre-commit passed.

Copilot

🟡 Changes recommended

Unresolved linking, concurrency, scheduler lifetime, and refresh-dispatch issues block approval.

5 open issues

Prevent dangling scheduler references in sessions · New
Synchronize scheduled task handle access · New
Export scheduler classes for cross-library linking · New
Forward auth refresh executor through SigV4 manager · New
Handle refresh task submission failures · New

What changed in this PR

Moves OAuth token-refresh scheduling into a reusable internal utility that dispatches work through an injected executor.

Changes:

Replaces the auth-specific scheduler with ScheduledExecutor.
Adds optional executor injection; background refresh is disabled when omitted.
Migrates scheduler tests and updates build integration.

File	Description
src/iceberg/util/scheduled_executor.cc	Implements delayed dispatch and cancellation.
src/iceberg/util/scheduled_executor_internal.h	Defines scheduler and task handles.
src/iceberg/test/auth_manager_test.cc	Migrates scheduler tests.
src/iceberg/cpp20_compatibility_internal.h	Removes the deleted scheduler include.
src/iceberg/CMakeLists.txt	Builds the new scheduler.
src/iceberg/catalog/rest/rest_catalog.h	Exposes optional auth executor injection.
src/iceberg/catalog/rest/rest_catalog.cc	Configures the auth manager’s executor.
src/iceberg/catalog/rest/CMakeLists.txt	Removes the old scheduler source.
src/iceberg/catalog/rest/auth/token_refresh_scheduler.h	Deletes the old scheduler interface.
src/iceberg/catalog/rest/auth/token_refresh_scheduler.cc	Deletes the old scheduler implementation.
src/iceberg/catalog/rest/auth/auth_session.h	Documents refresh executor requirements.
src/iceberg/catalog/rest/auth/auth_session.cc	Passes the scheduler into session configuration.
src/iceberg/catalog/rest/auth/auth_session_internal.h	Adapts refresh scheduling and cancellation.
src/iceberg/catalog/rest/auth/auth_manager.h	Adds the executor configuration hook.
src/iceberg/catalog/rest/auth/auth_manager.cc	Creates and supplies the scheduler.

_{Review effort: Balanced}

Copilot

🟡 Changes recommended

Scheduling and teardown races can stop refresh or terminate the process, while rejected submissions can cause a busy loop.

3 open issues

1 previously missed issue

In code that hasn't changed since last review

Destruction order can invalidate callback state before worker joins

src/iceberg/test/auth_manager_test.cc:704

Shutdown() now joins only the timer thread, not executor workers. A worker can set fired and pause before cv.notify_one(); the waiting thread can observe fired and finish the test, destroying cv before ThreadExecutor's destructor joins that worker. Declare the callback state before the executor so its workers are joined before their captured objects are destroyed.

5 resolved since last review

_{Review effort: Balanced}

Copilot

🔵 Needs a closer look

The asynchronous scheduling and shared-lifetime teardown changes need human validation before approval.

3 open issues

_{Review effort: Balanced}

Copilot

🔵 Needs a closer look

Executor rejection recovery resets HTTP retry state, allowing failing token requests to bypass the retry limit indefinitely.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Rejected refresh retries reset attempts and bypass the retry limit

src/iceberg/catalog/rest/auth/auth_session_internal.h:242

When submission of an HTTP retry is rejected, this handler reschedules through ScheduleRefreshAfter(), whose callback calls DoRefresh() and resets the HTTP attempt counter to zero. Alternating accepted and rejected submissions while token requests fail can therefore bypass kMaxRetries indefinitely. Retry dispatch of the pending DoRefreshAttempt, preserving its attempt, backoff, and refresh_started_at, while retaining the bounded dispatch-retry backoff.

3 resolved since last review

_{Review effort: Balanced}

…ndles

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

5 open issues

This reorders parameters of an exported/public factory (Make), which is a source-breaking change… · New
Cancellation can be lost in the race where Run() pops a due task (erasing from scheduled_) and… · New
scheduled_ is not updated when a cancelled task is discarded here. This leaves stale IDs in… · New
Moving out of std::priority_queue::top() via const_cast is brittle and easy to misreason about… · New
New behavior is introduced for rejection handling (on_rejected) but the migrated tests in… · New

_{Review effort: Lite}

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

11 open issues

scheduled_ is not updated when a cancelled task is discarded here. This leaves stale IDs in…
Cancellation can be lost in the race where Run() pops a due task (erasing from scheduled_) and…
This reorders parameters of an exported/public factory (Make), which is a source-breaking change…
This changes the parameter ordering of a public exported factory (ICEBERG_REST_EXPORT), which is… · New
This introduces locals that are never used (inserted, and cancelled is only written then… · New
This introduces locals that are never used (inserted, and cancelled is only written then… · New
The previous TokenRefreshScheduler::Instance() was intentionally leaked to avoid… · New
The previous TokenRefreshScheduler::Instance() was intentionally leaked to avoid… · New
ScheduledTask is currently copyable (implicit copy ctor/assign). Copying will produce multiple… · New
New behavior is introduced for rejection handling (on_rejected) but the migrated tests in…
Moving out of std::priority_queue::top() via const_cast is brittle and easy to misreason about…

_{Review effort: Lite}

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

14 open issues

scheduled_ is not updated when a cancelled task is discarded here. This leaves stale IDs in…
Cancellation can be lost in the race where Run() pops a due task (erasing from scheduled_) and…
This reorders parameters of an exported/public factory (Make), which is a source-breaking change…
This reorders RestCatalog::Make parameters and removes the prior `(config, metrics_executor,… · New
Shutdown() is called from both ScheduledExecutor::~ScheduledExecutor() and… · New
ScheduledTask::Cancel() mutates state_/id_ without synchronization. If a ScheduledTask… · New
ScheduledTask is currently copyable (implicit copy ctor/assign). Copying will produce multiple…
The previous TokenRefreshScheduler::Instance() was intentionally leaked to avoid…
The previous TokenRefreshScheduler::Instance() was intentionally leaked to avoid…
This introduces locals that are never used (inserted, and cancelled is only written then…
This introduces locals that are never used (inserted, and cancelled is only written then…
This changes the parameter ordering of a public exported factory (ICEBERG_REST_EXPORT), which is…
New behavior is introduced for rejection handling (on_rejected) but the migrated tests in…
Moving out of std::priority_queue::top() via const_cast is brittle and easy to misreason about…

_{Review effort: Lite}

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

18 open issues

scheduled_ is not updated when a cancelled task is discarded here. This leaves stale IDs in…
Cancellation can be lost in the race where Run() pops a due task (erasing from scheduled_) and…
This reorders parameters of an exported/public factory (Make), which is a source-breaking change…
This changes the exported RestCatalog::Make signature by reordering parameters and adding… · New
The structured binding element inserted is unused. With common warning sets (e.g., `-Wall… · New
cancelled is intentionally used only for its lifetime (to release captures outside the lock), but… · New
ScheduledTask is implicitly copyable, but IsValid() only checks that the handle still has an id… · New
ScheduledTask::Cancel() mutates state_/id_ without synchronization. If a ScheduledTask…
Shutdown() is called from both ScheduledExecutor::~ScheduledExecutor() and…
This reorders RestCatalog::Make parameters and removes the prior `(config, metrics_executor,…
ScheduledTask is currently copyable (implicit copy ctor/assign). Copying will produce multiple…
The previous TokenRefreshScheduler::Instance() was intentionally leaked to avoid…
The previous TokenRefreshScheduler::Instance() was intentionally leaked to avoid…
This introduces locals that are never used (inserted, and cancelled is only written then…
This introduces locals that are never used (inserted, and cancelled is only written then…
This changes the parameter ordering of a public exported factory (ICEBERG_REST_EXPORT), which is…
New behavior is introduced for rejection handling (on_rejected) but the migrated tests in…
Moving out of std::priority_queue::top() via const_cast is brittle and easy to misreason about…

_{Review effort: Lite}

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

21 open issues

scheduled_ is not updated when a cancelled task is discarded here. This leaves stale IDs in…
Cancellation can be lost in the race where Run() pops a due task (erasing from scheduled_) and…
This reorders parameters of an exported/public factory (Make), which is a source-breaking change…
This change reorders RestCatalog::Make parameters (moving metrics_executor from position 2 to… · New
SharedTimerState::Schedule doesn’t check shutdown_. If Shutdown() has been called, Run()… · New
ScheduledTask is implicitly copyable, but IsValid() only checks that the handle still has an id…
cancelled is intentionally used only for its lifetime (to release captures outside the lock), but…
The structured binding element inserted is unused. With common warning sets (e.g., `-Wall…
This changes the exported RestCatalog::Make signature by reordering parameters and adding…
ScheduledTask::Cancel() mutates state_/id_ without synchronization. If a ScheduledTask…
Shutdown() is called from both ScheduledExecutor::~ScheduledExecutor() and…
This reorders RestCatalog::Make parameters and removes the prior `(config, metrics_executor,…
ScheduledTask is currently copyable (implicit copy ctor/assign). Copying will produce multiple…
The previous TokenRefreshScheduler::Instance() was intentionally leaked to avoid…
The previous TokenRefreshScheduler::Instance() was intentionally leaked to avoid…
This introduces locals that are never used (inserted, and cancelled is only written then…
This introduces locals that are never used (inserted, and cancelled is only written then…
This changes the parameter ordering of a public exported factory (ICEBERG_REST_EXPORT), which is…
New behavior is introduced for rejection handling (on_rejected) but the migrated tests in…
Moving out of std::priority_queue::top() via const_cast is brittle and easy to misreason about…

And 1 more that still need to be addressed.

_{Review effort: Lite}

Copilot

🔵 Needs a closer look

Shared-timer teardown, cross-thread shutdown, and borrowed-executor lifetimes warrant final human concurrency review.

No open issues 🎉

21 resolved since last review

scheduled_ is not updated when a cancelled task is discarded here. This leaves stale IDs in…
Cancellation can be lost in the race where Run() pops a due task (erasing from scheduled_) and…
This reorders parameters of an exported/public factory (Make), which is a source-breaking change…
SharedTimerState::Schedule doesn’t check shutdown_. If Shutdown() has been called, Run()…
This change reorders RestCatalog::Make parameters (moving metrics_executor from position 2 to…
ScheduledTask is implicitly copyable, but IsValid() only checks that the handle still has an id…
cancelled is intentionally used only for its lifetime (to release captures outside the lock), but…
The structured binding element inserted is unused. With common warning sets (e.g., `-Wall…
This changes the exported RestCatalog::Make signature by reordering parameters and adding…
ScheduledTask::Cancel() mutates state_/id_ without synchronization. If a ScheduledTask…
Shutdown() is called from both ScheduledExecutor::~ScheduledExecutor() and…
This reorders RestCatalog::Make parameters and removes the prior `(config, metrics_executor,…
ScheduledTask is currently copyable (implicit copy ctor/assign). Copying will produce multiple…
The previous TokenRefreshScheduler::Instance() was intentionally leaked to avoid…
The previous TokenRefreshScheduler::Instance() was intentionally leaked to avoid…
This introduces locals that are never used (inserted, and cancelled is only written then…
This introduces locals that are never used (inserted, and cancelled is only written then…
This changes the parameter ordering of a public exported factory (ICEBERG_REST_EXPORT), which is…
New behavior is introduced for rejection handling (on_rejected) but the migrated tests in…
Moving out of std::priority_queue::top() via const_cast is brittle and easy to misreason about…

And 1 more resolved.

_{Review effort: Balanced}

Copilot

🔵 Needs a closer look

Shared-timer lifetime, borrowed executors, and concurrent shutdown guarantees warrant final human concurrency review.

No open issues 🎉

_{Review effort: Balanced}

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

3 open issues

This changes the public RestCatalog::Make parameter order in a way that breaks existing… · New
inserted is never used. With common warning levels (and especially with -Werror), this may fail… · New
cancelled is not referenced, which can trigger an unused-variable warning. If the intent is to… · New

_{Review effort: Lite}

  static Result<std::shared_ptr<RestCatalog>> Make(const RestCatalogProperties& config,
-                                                   Executor* metrics_executor = nullptr,
-                                                   ClientFactory client_factory = {});
+                                                   ClientFactory client_factory = {},
+                                                   Executor* auth_executor = nullptr,
+                                                   Executor* metrics_executor = nullptr);


+    auto [it, inserted] = tasks_.emplace(
+        std::pair{deadline, id}, Task{.state = std::move(state),
+                                      .task = std::move(task),
+                                      .on_submit_failure = std::move(on_submit_failure)});


+    std::optional<Task> cancelled;
+    {
+      std::lock_guard lock(mutex_);
+      auto it = by_id_.find(id);
+      if (it == by_id_.end()) return;
+      cancelled.emplace(std::move(it->second->second));
+      tasks_.erase(it->second);
+      by_id_.erase(it);
+    }
+    cv_.notify_one();
+    // Release callback captures outside the queue lock.


Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

6 open issues

inserted is never used. If the project builds with -Werror (common in CI), this can fail… · New
This changes the public RestCatalog::Make parameter order in a way that breaks existing…
This changes the positional argument order for a public factory (metrics_executor moved from 2nd… · New
cancelled is not referenced, which can trigger an unused-variable warning. If the intent is to…
inserted is never used. With common warning levels (and especially with -Werror), this may fail…
submissions is mutated on the timer thread and read on the test thread. While current waits on… · New

_{Review effort: Lite}

+    std::lock_guard lock(mutex_);
+    auto id = next_id_++;
+    auto [it, inserted] = tasks_.emplace(std::pair{deadline, id},
+                                         Task{.state = std::move(state),
+                                              .task = std::move(task),
+                                              .on_submission = std::move(on_submission)});
+    by_id_.emplace(id, it);
+    cv_.notify_one();


  static Result<std::shared_ptr<RestCatalog>> Make(const RestCatalogProperties& config,
-                                                   Executor* metrics_executor = nullptr,
-                                                   ClientFactory client_factory = {});
+                                                   ClientFactory client_factory = {},
+                                                   Executor* auth_executor = nullptr,
+                                                   Executor* metrics_executor = nullptr);


+class InlineExecutor : public Executor {
+ public:
+  Status Submit(ExecutorTask task) override {
+    ++submissions;
+    std::move(task)();
+    return {};
+  }
+  int submissions = 0;
+};


refactor(auth): use injected executor for token refresh

643b6cc

Copilot AI balanced review requested due to automatic review settings October 17, 2026 16:55

Copilot started reviewing on behalf of wgtmac October 17, 2026 16:55

View session

Copilot AI reviewed Oct 17, 2026

View reviewed changes

fix(auth): address token refresh scheduler review

6f9beca

Copilot AI balanced review requested due to automatic review settings October 18, 2026 01:29

Copilot started reviewing on behalf of wgtmac October 18, 2026 01:30

View session

Copilot AI reviewed Oct 18, 2026

View reviewed changes

Comment thread src/iceberg/catalog/rest/auth/auth_session_internal.h Outdated

Comment thread src/iceberg/util/scheduled_executor.cc Outdated

Comment thread src/iceberg/catalog/rest/auth/auth_session_internal.h Outdated

style(auth): simplify OAuth2 manager factory

f00c3b7

Copilot AI balanced review requested due to automatic review settings October 18, 2026 01:37

Copilot started reviewing on behalf of wgtmac October 18, 2026 01:37

View session

Copilot AI reviewed Oct 18, 2026

View reviewed changes

fix(auth): harden scheduled refresh lifecycle

4e26134

Copilot AI balanced review requested due to automatic review settings October 18, 2026 02:07

Copilot started reviewing on behalf of wgtmac October 18, 2026 02:08

View session

Copilot AI reviewed Oct 18, 2026

View reviewed changes

wgtmac marked this pull request as ready for review October 19, 2026 07:37

style(util): use braced return for scheduled task

b1d3b4d

Copilot AI balanced review requested due to automatic review settings October 19, 2026 15:06

refactor(rest): reorder catalog factory arguments and clarify task ha…

6a497d7

…ndles

Copilot AI reviewed Oct 19, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 19, 2026 15:23

refactor(util): share one timer across scheduled executors

7b2aca9

Copilot AI reviewed Oct 19, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 19, 2026 15:42

Copilot AI reviewed Oct 19, 2026

View reviewed changes

Comment thread src/iceberg/catalog/rest/rest_catalog.h

Comment thread src/iceberg/util/scheduled_executor.cc

Comment thread src/iceberg/util/scheduled_executor_internal.h Outdated

refactor(auth): simplify refresh state and strengthen behavior tests

7809564

Copilot AI balanced review requested due to automatic review settings October 19, 2026 16:05

Copilot AI reviewed Oct 19, 2026

View reviewed changes

Comment thread src/iceberg/catalog/rest/rest_catalog.h

Comment thread src/iceberg/util/scheduled_executor.cc Outdated

Comment thread src/iceberg/util/scheduled_executor.cc

Comment thread src/iceberg/util/scheduled_executor_internal.h Outdated

Copilot AI balanced review requested due to automatic review settings October 19, 2026 16:26

Copilot AI reviewed Oct 19, 2026

View reviewed changes

Comment thread src/iceberg/catalog/rest/rest_catalog.h

Comment thread src/iceberg/util/scheduled_executor.cc

Comment thread src/iceberg/util/scheduled_executor.cc

shangxinli reviewed Oct 19, 2026

View reviewed changes

Comment thread src/iceberg/catalog/rest/auth/auth_session_internal.h Outdated

Copilot stopped reviewing on behalf of wgtmac due to an error October 19, 2026 17:06
The job was not acquired by a runner even after multiple attempts.

Copilot stopped reviewing on behalf of wgtmac due to an error October 19, 2026 17:17
The job was not acquired by a runner even after multiple attempts.

Copilot stopped reviewing on behalf of wgtmac due to an error October 19, 2026 17:22
The job was not acquired by a runner even after multiple attempts.

Copilot stopped reviewing on behalf of wgtmac due to an error October 19, 2026 17:30
The job was not acquired by a runner even after multiple attempts.

Copilot stopped reviewing on behalf of wgtmac due to an error October 19, 2026 17:32
The job was not acquired by a runner even after multiple attempts.

fix(util): preserve shared timer through static destruction

bb30175

Copilot AI balanced review requested due to automatic review settings October 20, 2026 01:51

Copilot started reviewing on behalf of wgtmac October 20, 2026 01:56

View session

Copilot AI reviewed Oct 20, 2026

View reviewed changes

refactor(util): trim timer ownership and test scaffolding

b065223

Copilot AI balanced review requested due to automatic review settings October 20, 2026 02:15

Copilot started reviewing on behalf of wgtmac October 20, 2026 02:25

View session

Copilot AI reviewed Oct 20, 2026

View reviewed changes

refactor(auth): clarify scheduled task and submission callback names

d7ea151

Copilot AI balanced review requested due to automatic review settings October 20, 2026 03:19

refactor(util): report both task submission outcomes

73db201

Copilot AI reviewed Oct 20, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 20, 2026 03:36

Copilot stopped reviewing on behalf of wgtmac due to an error October 20, 2026 03:40
The job was not acquired by a runner even after multiple attempts.

Copilot AI reviewed Oct 20, 2026

View reviewed changes

Copilot stopped reviewing on behalf of wgtmac due to an error October 20, 2026 04:03
The job was not acquired by a runner even after multiple attempts.

Conversation

wgtmac commented Oct 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

wgtmac commented Oct 17, 2026 •

edited

Loading