Skip to content

ngclient: raise SlowRetrievalError on mid-stream read timeout - #2993

Merged
jku merged 1 commit into
theupdateframework:developfrom
theadsingh:fix/urllib3-stream-read-timeout
Aug 25, 2026
Merged

ngclient: raise SlowRetrievalError on mid-stream read timeout#2993
jku merged 1 commit into
theupdateframework:developfrom
theadsingh:fix/urllib3-stream-read-timeout

Conversation

@theadsingh

Copy link
Copy Markdown

Urllib3Fetcher._chunks only converts a MaxRetryError into SlowRetrievalError. When
the gap timeout expires part way through a response body, urllib3 raises
ReadTimeoutError directly, and that is a TimeoutError but not a MaxRetryError, so it
escapes untouched.

That reaches the public API. fetch() wraps self._fetch(url), but _fetch only builds
the generator, so the wrapper covers connection setup and not streaming. download_file()
then iterates the generator outside any try, so a raw urllib3 error comes out of
download_bytes() and download_file(), both of which document DownloadError.

Against a local server that sends headers plus ten bytes and then stalls, with
socket_timeout=1:

Urllib3Fetcher   -> LEAKED urllib3.exceptions.ReadTimeoutError
RequestsFetcher  -> SlowRetrievalError   (documented)

RequestsFetcher._chunks catches requests.exceptions.Timeout and gets this right, so
the two bundled fetchers disagree on the same condition, and the default one is the one
that leaks.

The existing test_response_read_timeout passes because it mocks
stream.side_effect as a MaxRetryError wrapping a TimeoutError, which is not what
urllib3 raises here. I left that test alone and added the ReadTimeoutError case
alongside it, plus one through download_bytes() so the public contract is pinned rather
than just the generator.

One extra line worth flagging: the existing MaxRetryError branch falls through without
re-raising when the reason is not a timeout, which ends the generator silently and looks
like a complete download to download_file(). I have made that propagate. I could not
construct a case that triggers it, so treat it as tidying rather than a reported bug, and
say the word if you would rather it came out of this PR.

After the change both fetchers raise SlowRetrievalError against the stalling server.
Full suite is 202 passed, and ruff check, ruff format --diff and mypy are clean.

Signed-off-by: Amandeep Singh <mr.ad.iitd@gmail.com>
@theadsingh
theadsingh requested a review from a team as a code owner August 21, 2026 05:36

@jku jku left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks correct to me, thanks.

I think we shouldn't have implemented SlowRetrievalError in the first place (it's not really ever actionable or useful) but since it's in the API I guess we can at least do our best...

@jku

jku commented Aug 25, 2026

Copy link
Copy Markdown
Member

test-with-sslib-main failure on CI is unrelated

@jku
jku merged commit 19ab12b into theupdateframework:develop Aug 25, 2026
16 of 17 checks passed
@jku jku mentioned this pull request Aug 26, 2026
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.

2 participants