fix: let streaming failures reach the retry loop - #48
Closed
shoemoney wants to merge 1 commit into
Closed
Conversation
`send_request` implements retry/backoff by catching exceptions around the
provider call. `_handle_stream_request` wraps its own body in a second
try/except and converts any failure into `return "failed", {...}`.
A return is not an exception, so it walks straight past the enclosing
retry loop: the `for retry_attempt in range(self.max_retries)` exits on its
first iteration no matter what `--retries` is set to. Non-stream requests,
whose exceptions propagate normally, get the full budget.
That is the same class of failure the retry loop exists for -- a 429 or 5xx
on connect, or a provider that starts the SSE stream and dies mid-iteration.
Removing the inner handler lets it propagate. The error is still logged and
still returns "failed" once the attempts are exhausted, by the existing code
in send_request.
Counted provider calls for a mid-stream failure with max_retries=3:
before: [stream] 1 attempt [nonstream] 3 attempts
after: [stream] 3 attempts [nonstream] 3 attempts
The diff is mostly the dedent; `git diff -w` shows the four lines that
actually changed.
Author
|
Closing as a duplicate of #47, which was opened first and is the better fix. #47 re-raises and keeps the existing Sorry for the noise — #47 is the one to review. |
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.
The problem
send_requestimplements retry with exponential backoff by catching exceptions around the provider call (verify.py:252-276):_handle_stream_requestwraps its own body in a secondtry/exceptand converts any failure into a return value:A
returnis not an exception, so it walks straight past the enclosing retry loop. The loop exits on its first iteration regardless of--retries. Non-stream requests, whose exceptions propagate normally, get the full budget.The failures this hides are exactly the ones the retry loop exists for: a 429 or 5xx on connect, or a provider that returns 200, starts the SSE stream, and dies partway through iteration.
Reproduction
Counted actual calls into
client.chat.completions.createwith a provider stubbed to yield one chunk and then raise,max_retries=3:Before
After
The log confirms the mechanism — before the change, the only line from the stream run is
_handle_stream_request:356 - Stream request failed, andsend_request's retry warnings never appear.The change
Remove the inner handler and let the exception propagate to the retry loop that already knows what to do with it. The failure is still logged and still returns
"failed"once attempts are exhausted — by the existing code insend_request, which also reports the attempt count.The diff looks large because removing the
try:dedents the body.git diff -wshows the real change: 4 deleted lines.Noted, not fixed here
--retriesis declared withdefault=10but its help text reads(default: 3)(verify.py:608-613), so--helpunderstates the real retry budget by more than 3x. It's a one-line fix but a separate concern, so I left it out of this PR — happy to send it separately, or fold it in if you'd rather have both together.I also didn't add a regression test: there's currently no suite covering
verify.py(m3_format_check/tests response format, not request mechanics), and adding one felt like a bigger decision than this fix should make on its own. The repro above is a few dozen lines and I'm glad to contribute it as a first test if you want it.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.