Skip to content

Await returned awaitables from async retry lifecycle callbacks - #728

Open
harbinresearcher wants to merge 1 commit into
jd:mainfrom
harbinresearcher:codex/fix-awaitable-callbacks
Open

harbinresearcher wants to merge 1 commit into
jd:mainfrom
harbinresearcher:codex/fix-awaitable-callbacks

Conversation

@harbinresearcher

Copy link
Copy Markdown

Problem

AsyncRetrying accepts before, after, and before_sleep callbacks returning Awaitable[None] | None. A regular function returning a coroutine, Task, or Future satisfies this contract, but _utils.wrap_to_async_func only decides whether to await from the callable's declaration. Its synchronous wrapper returns the awaitable as a value without waiting for it.

Consequently, an operation can run before its before callback finishes, or the next attempt can start before after/before_sleep finishes. Exceptions from a callback's Future can also be ignored.

A local reproducer using a regular before function returning asyncio.create_task(...) records ['operation'] when AsyncRetrying returns. Only after explicitly draining the task does it record ['operation', 'before'].

Change

Resolve the actual result of only the three lifecycle callbacks in AsyncRetrying._add_action_func. Call each callback once, await its result if it is awaitable, and leave ordinary synchronous results unchanged. Callback attributes and copying retain their existing behavior.

The shared utility and other internal actions are intentionally unchanged. Successful operation results can themselves be awaitables used as data; they must not be awaited a second time. The regression coverage checks that a pending Future returned by an async operation remains the identical, still-pending object.

This does not change callable detection, sleeping, retry policies, or statistics, and does not depend on #657.

Verification

On Python 3.12.14 / Windows:

  • Unchanged main: 184 tests and 15 subtests passed.
  • New regression cases on the original implementation: 13 failed, covering all three hooks, coroutine/Task results, direct calls/async iteration, and Future exception propagation. The operation-result compatibility test already passes on main.
  • Final full suite: 198 tests and 15 subtests passed.
  • Ruff 0.16.9 lint and formatting passed.
  • Strict Mypy 2.3.1 passed for all 19 configured source files.
  • All 35 Sphinx doctests and the HTML build passed with warnings treated as errors.
  • Reno lint and git diff --check passed.

OpenAI Codex assisted with investigation, implementation, tests, and independent review. Review found and rejected an initial overly broad shared-utility change that would await an operation result twice; the submitted change is callback-scoped and includes a regression for that behavior. All verification above was run locally.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant