Skip to content

fix(python): raise on mid-stream body errors instead of silent EOF#482

Open
barjin wants to merge 2 commits into
masterfrom
fix/python-stream-error-propagation
Open

fix(python): raise on mid-stream body errors instead of silent EOF#482
barjin wants to merge 2 commits into
masterfrom
fix/python-stream-error-propagation

Conversation

@barjin

@barjin barjin commented Jun 12, 2026

Copy link
Copy Markdown
Member

Mid-stream body errors (connection reset, truncated chunked transfer) were mapped to StopIteration/StopAsyncIteration, which signal normal end-of-iteration — so for/async for ended silently and callers processed partial bodies as complete (a silent data-integrity bug, #475). Both sync and async iterators now propagate the classified ImpitError as a real exception and set the consumed/closed flags consistently with the clean-EOF branch.

Streamed truncation surfaces in reqwest as a Decode-kinded error rather than Body, so the existing unexpected-EOF classification missed it and fell through to the catch-all HTTPError. The guard is widened to cover is_decode(), so truncated streams now raise RemoteProtocolError, matching httpx.

Verified against a vanilla server that truncates the body mid-response; added sync + async regression tests.

Closes #475

A mid-stream body error (connection reset, truncated chunked transfer) was
mapped to StopIteration/StopAsyncIteration, which signals normal end of
iteration. The `async for`/`for` loop ended silently and the caller processed
a partial body believing it was complete - a silent data-integrity bug. Now
the classified ImpitError is propagated as a real exception, and the
consumed/closed flags are set consistently with the clean-EOF branch.

Streamed body truncation surfaces in reqwest as a `Decode`-kinded error rather
than `Body`, so the existing unexpected-EOF classification missed it and fell
through to the catch-all HTTPError. Widen the guard to cover `is_decode()` so
truncated streams are reported as RemoteProtocolError, matching httpx.
@github-actions github-actions Bot added this to the 142nd sprint - Tooling team milestone Jun 12, 2026
@github-actions github-actions Bot added t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics. labels Jun 12, 2026
@barjin barjin requested a review from Pijukatel June 16, 2026 08:45

@Pijukatel Pijukatel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could this potentially cause something similar to this in Python?

Worth double-checking that we will not get some new client problems due to weird API behavior that was previously silently ignored

@barjin

barjin commented Jun 16, 2026

Copy link
Copy Markdown
Member Author

Thank you @Pijukatel , that's a good point there.

According to my tests, the behaviour actually differs between ActorClient and AsyncActorClient in apify-client-python.

While ActorClient (synchronous) spawns a thread, AsyncActorClient spawns an asyncio Task .

From what I noticed, the async Task will rethrow the new Impit error (causing the whole process to fail), while the Thread will only log the error, but return correctly on t.join() and allow the process to continue as expected.

I'd leave the final decision on the client maintainers, but imo both the Async and sync clients should handle exceptions the same (i.e., ignore in this case?). Wdyt?

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

Labels

t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

(py) Mid-stream body error raises StopAsyncIteration, so truncated streams look like clean EOF

3 participants