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 close() currently only calls:

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;
• removes futures when their timer task runs or when the future is otherwise settled;
• 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.

Validation

• branch is based on current upstream main at cf942a40074291290634321ad9fe21e514030b4c;
• branch is 0 commits behind upstream;
• changes are limited to DefaultSleeper and focused lifecycle tests;
• normal timer-driven completion remains unchanged for pending sleeps.

Full repository validation is left to GitHub Actions.

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".

[chatgpt-codex-connector]

Keep the future tracked until it is completed

When the timer fires concurrently with close(), this removes the future from pending and releases the lock before calling future.complete(null). During that gap, close() can acquire the lock, find no pending future, and return while the future is still incomplete, so shutdown does not deterministically settle every outstanding sleep as intended. Complete the future within the same critical section, or otherwise keep it tracked until completion has occurred.

Useful? React with 👍 / 👎.

[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.