fix: settle pending async sleeps on close

[sylvesterkaczmarek]

Summary

Make DefaultSleeper.close() settle all pending sleepAsync() futures instead of abandoning them when its backing Timer is cancelled.

Fixes #889.

Problem

DefaultSleeper.sleepAsync() completes its returned future from a scheduled TimerTask, while the old close() only called:

timer.cancel()

Timer.cancel() discards scheduled tasks that have not run. Their futures remain incomplete forever.

In the SDK this can strand an async retry chain if the client is closed while RetryingHttpClient is waiting on sleeper.sleepAsync(backoffDuration).

A second edge case is sleepAsync() after close: Timer.schedule() throws synchronously because the timer has already been cancelled.

Fix

DefaultSleeper now:

• tracks pending async sleep futures under a small lock;
• keeps a timer-fired future tracked until its completion state has been set, preventing close() from missing an incomplete future during the timer/close handoff;
• removes futures when they settle;
• marks itself closed and cancels the timer exactly once;
• cancels every still-pending future during close;
• returns an already-cancelled future for post-close sleepAsync() calls instead of throwing synchronously.

The synchronous sleep() path is unchanged.

Regression coverage

Added tests verifying:

1. closing the sleeper cancels a long pending async sleep immediately;
2. a normally completed async sleep stays successfully completed after close;
3. repeated close is safe;
4. sleepAsync() after close returns a cancelled future without throwing from the method call.

Review follow-up

The timer-fire-versus-close race identified in review is addressed by completing the future while holding the same lock used by close(), while leaving removal to the existing whenComplete callback. Therefore close() cannot observe the future as absent from pending before it is already settled.

No test-only production hook was added solely to force that narrow handoff. The existing lifecycle tests remain focused on externally observable behavior.

Validation

• changes remain limited to DefaultSleeper and focused lifecycle tests;
• normal timer-driven completion remains unchanged for pending sleeps;
• the follow-up race fix is commit 1bd8c8d664e495b7676cef8c42f270de190403d0;
• GitHub Actions on the fork require approval (action_required) rather than reporting a test failure.

Risk

Low. The behavioral change is limited to sleeper shutdown: pending async waits now terminate deterministically instead of becoming permanently incomplete.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 28db2acd5e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[jbeckwith-oai]

Thanks for putting this together — this is a thoughtful, tightly scoped fix for a real lifecycle bug, and the focused tests are a good start.

I confirmed the underlying problem is worth fixing. DefaultSleeper.sleepAsync() has only one normal completion path: its scheduled TimerTask. Timer.cancel() discards queued tasks without running them, so the old close() could permanently orphan their CompletableFutures. RetryingHttpClient.executeAsync() composes the next attempt from that sleep future, and ClientOptions.close() closes the same sleeper. Closing a client during async retry backoff can therefore leave the externally returned request future incomplete indefinitely.

I am requesting one correctness change before merge:

In TimerTask.run(), the new implementation removes future from pending while holding lock, releases the lock, and only then calls future.complete(null). A legal interleaving is:

1. The timer thread removes the future and is descheduled before completing it.
2. close() acquires the lock, sees no pending future, cancels the timer, and returns.
3. The timer thread resumes and completes the future, potentially starting a retry after the client has closed.

That means close() can still return while an outstanding sleep future is incomplete, contrary to this PR's deterministic-settlement guarantee.

Please keep the future tracked until its completion state has been set, so close() can only miss futures that are already done. One small approach is to check that the future is still tracked without removing it, call complete, and let the existing whenComplete callback perform the removal. Please also add deterministic coverage for the timer-fire-versus-close handoff if it can be done without production-only hooks.

A lightweight RetryingHttpClient regression test would also be valuable: enter a long async backoff with the real DefaultSleeper, close the retry client, assert the returned request future settles promptly, and verify that no second request is issued. I view that as useful cross-layer evidence rather than a separate blocker.

Once the remove-before-complete race is addressed, the rest of the lifecycle handling looks sound: scheduling and close are serialized, repeated close is safe, externally settled futures are untracked, and post-close calls return settled futures.

[sylvesterkaczmarek]

Thanks for putting this together — this is a thoughtful, tightly scoped fix for a real lifecycle bug, and the focused tests are a good start.

I confirmed the underlying problem is worth fixing. DefaultSleeper.sleepAsync() has only one normal completion path: its scheduled TimerTask. Timer.cancel() discards queued tasks without running them, so the old close() could permanently orphan their CompletableFutures. RetryingHttpClient.executeAsync() composes the next attempt from that sleep future, and ClientOptions.close() closes the same sleeper. Closing a client during async retry backoff can therefore leave the externally returned request future incomplete indefinitely.

I am requesting one correctness change before merge:

In TimerTask.run(), the new implementation removes future from pending while holding lock, releases the lock, and only then calls future.complete(null). A legal interleaving is:

1. The timer thread removes the future and is descheduled before completing it.
2. close() acquires the lock, sees no pending future, cancels the timer, and returns.
3. The timer thread resumes and completes the future, potentially starting a retry after the client has closed.

That means close() can still return while an outstanding sleep future is incomplete, contrary to this PR's deterministic-settlement guarantee.

Please keep the future tracked until its completion state has been set, so close() can only miss futures that are already done. One small approach is to check that the future is still tracked without removing it, call complete, and let the existing whenComplete callback perform the removal. Please also add deterministic coverage for the timer-fire-versus-close handoff if it can be done without production-only hooks.

A lightweight RetryingHttpClient regression test would also be valuable: enter a long async backoff with the real DefaultSleeper, close the retry client, assert the returned request future settles promptly, and verify that no second request is issued. I view that as useful cross-layer evidence rather than a separate blocker.

Once the remove-before-complete race is addressed, the rest of the lifecycle handling looks sound: scheduling and close are serialized, repeated close is safe, externally settled futures are untracked, and post-close calls return settled futures.

Thanks for catching this. I've addressed the remove-before-complete race in 1bd8c8d664e495b7676cef8c42f270de190403d0.

The timer now keeps the future tracked while completing it under the same lock used by close(). The existing whenComplete callback removes it only after its completion state has been set, so close() can no longer miss an incomplete future during that handoff.

I did not add a production-only synchronization hook solely to force that narrow interleaving in a deterministic test. The existing lifecycle tests remain unchanged, and I've updated the PR description to document the concurrency guarantee and follow-up.