fix: give the attempt ceiling its own retryable kind and adapter error - #26
Merged
Conversation
A TimeoutConfig.attempt ceiling firing before the total was classified total_timeout, which is not retryable, and escaped client.get() as a bare stdlib TimeoutError because only an expired deadline became a CallError. The same except-TimeoutError branch also swallowed aiohttp's phase timeouts (its whole timeout family subclasses TimeoutError), so a sock_read timeout was labelled total_timeout and never retried. The engine now asks the cancel scope whether it fired instead of matching on the exception type. An SDK exception goes through the adapter's classifier; a fired ceiling is total_timeout when the deadline is gone and the new attempt_timeout otherwise. attempt_timeout is retryable and trips the breaker by default, like read_timeout, and a call whose last attempt died on the ceiling raises AttemptTimeoutError through the adapter translator: HttpxAttemptTimeoutError is an httpx.TimeoutException, AiohttpAttemptTimeoutError an aiohttp.ServerTimeoutError. The retry gate moved into _retry_delay in both engines so an outcome becomes final in one place. Sync adapters declare attempt_timeout collapsed into read_timeout, so the default retry config has no dead kinds under strict. Closes #24
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TimeoutConfig(attempt=0.4)undertotal=1.0was meant to cut a hung attempt and try again. Instead the first ceiling escapedclient.get()as a bare stdlibTimeoutErrorat 0.4 s, unretried and invisible toexcept httpx.TimeoutException: the engine classified anything raised as aTimeoutErrorastotal_timeout, which is not retryable, and only an expired deadline ever became aCallErrorfor the adapter translator. The sameexcept TimeoutErrorhad a second victim. aiohttp's timeout family subclassesasyncio.TimeoutError(the builtin on 3.11+), so asock_readtimeout never reached the adapter's classifier either: one attempt,total_timeout, no retry.The engine now asks the cancel scope whether it fired (
asyncio.Timeout.expired()) instead of matching on the exception type. An SDK exception,TimeoutErrorsubclass or not, goes throughclassify_error; a fired ceiling istotal_timeoutwhen the deadline is gone and the newFailureKind.ATTEMPT_TIMEOUTotherwise.attempt_timeoutis retryable and trips the breaker by default, next toread_timeout. When the retry gate declines and the last outcome is an attempt timeout,_attemptsraisesAttemptTimeoutError(CallError)carrying the ceiling, chained from theTimeoutError, the way it already raisesDeadlineExceededError, so it leavesrun()through the translator asHttpxAttemptTimeoutError(anhttpx.TimeoutException) orAiohttpAttemptTimeoutError(anaiohttp.ServerTimeoutError). The adapter packages export those two and the root exportsAttemptTimeoutError.To have one place where an outcome becomes final, the retry gate (decide, skip counter, budget spend) moved into
_retry_delay. The sync engine got the identical move so the two loops still read the same; nothing else changes there, since it has no ceiling to fire, and requests/urllib3 get no new error class.Adding a default retryable kind the sync adapters can never emit would have made
dead_retryable_kindsflag every default config on requests and urllib3, andstrictwould have failed those builds. They now declareattempt_timeoutcollapsed intoread_timeout: on a sync runtime the stall a ceiling would have caught arrives as the clamped read phase, the same reasoning as the existingwrite_timeout -> total_timeoutcollapse. The httpx family record is shared by both flavours, so it listsattempt_timeoutinemitswith a note that only the async client produces it, asdeadline_harddoes. A new test compiles the default config against every registered capability record and asserts there are no dead kinds, so this cannot regress quietly.Rejected: reusing
DeadlineExceededErrorfor the ceiling (it is not the deadline, and the label that says which knob fired is the point); leavingattempt_timeoutout of the retry defaults (a ceiling that is never retried is not worth setting next toretry); keepingexcept TimeoutErrorand only relabelling it (leaves aiohttp's read timeouts misclassified).Docs: timeouts guide, retries guide, agents page (
TimeoutConfig,RetryConfig,FailureKind, errors), adapters reference, writing-an-adapter. The changelog belongs to release-please.The repro from #24 (its exact script), before and after:
Three requests reach the origin on the branch: two 0.4 s ceilings, then the third clamped to the 0.17 s the total had left (
attempt_timeout,attempt_timeout,total_timeoutin the attempt metrics; the integration test pins that sequence). The lab's02_retry_multiplies.pyprints the same three lines before and after, as its cases hit the total, not the ceiling:The aiohttp side,
read=0.2against an origin that answers in 1 s with three attempts allowed:Negative control: with master's
engine/aio.pyand the sync capability records swapped back in, the ten new behavioural tests fail (10 failed, 120 deselected); with the branch they pass. Gate:make checkclean (ruff, ruff format, mypy, import-linter);make test819 passed, 42 skipped, coverage 99.60 % against the 97 % floor;uv.lockuntouched.Closes #24