Skip to content

fix: let streaming failures reach the retry loop - #48

Closed
shoemoney wants to merge 1 commit into
MiniMax-AI:mainfrom
shoemoney:fix/stream-retry-swallowed
Closed

fix: let streaming failures reach the retry loop#48
shoemoney wants to merge 1 commit into
MiniMax-AI:mainfrom
shoemoney:fix/stream-retry-swallowed

Conversation

@shoemoney

@shoemoney shoemoney commented Aug 5, 2026

Copy link
Copy Markdown

The problem

send_request implements retry with exponential backoff by catching exceptions around the provider call (verify.py:252-276):

for retry_attempt in range(self.max_retries):
    try:
        if request.get("stream", False):
            return await self._handle_stream_request(request)
        else:
            response = await self.client.chat.completions.create(...)
            return "success", response.model_dump()
    except Exception as e:
        last_error = e
        if retry_attempt < self.max_retries - 1:
            ...backoff and continue...

_handle_stream_request wraps its own body in a second try/except and converts any failure into a return value:

except Exception as e:
    logger.error(f"Stream request failed: {e}")
    return "failed", {"error": str(e)}

A return is 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.create with a provider stubbed to yield one chunk and then raise, max_retries=3:

Before

[stream]    status='failed'  provider calls=1  (max_retries=3)
[nonstream] status='failed'  provider calls=3  (max_retries=3)

After

[stream]    status='failed'  provider calls=3  (max_retries=3)
[nonstream] status='failed'  provider calls=3  (max_retries=3)

The log confirms the mechanism — before the change, the only line from the stream run is _handle_stream_request:356 - Stream request failed, and send_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 in send_request, which also reports the attempt count.

The diff looks large because removing the try: dedents the body. git diff -w shows the real change: 4 deleted lines.

Noted, not fixed here

--retries is declared with default=10 but its help text reads (default: 3) (verify.py:608-613), so --help understates 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.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

`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.
@shoemoney

Copy link
Copy Markdown
Author

Closing as a duplicate of #47, which was opened first and is the better fix.

#47 re-raises and keeps the existing logger.error line; this PR deleted the handler outright, which drops that log and produces a large whitespace diff for a four-line change. #47's measurements also cover the SDK's own max_retries layer, which this one didn't.

Sorry for the noise — #47 is the one to review.

@shoemoney shoemoney closed this Aug 5, 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.

1 participant