fix(retry): let streaming requests reach the retry loop - #47
Open
shoemoney wants to merge 1 commit into
Open
Conversation
_handle_stream_request converted every exception into a normal
("failed", ...) return value, so send_request's except clause never
fired for a streaming request and the loop exited after one attempt.
Re-raise instead; the final-failure return shape is unchanged.
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.
send_requestretries up to--retriestimes (default 10), but streaming requests only everget one attempt.
_handle_stream_requestwraps its whole body in atryand converts every exception into a normalreturn value:
Because
verify.py:259is areturn, that value goes straight back out ofsend_requestand theexcepton line 263 never sees anything. The loop exits on its first iteration.--streamwas effectively dead code until #14 wiredreq["stream"] = Trueatverify.py:212-213,which is why this hasn't bitten before.
Measured
Driving the real
ValidatorRunner.send_requestagainst a local server that always fails, countingHTTP requests that actually arrive:
--retries=10, SDK retries off, non-stream--retries=10, SDK retries off,--stream--retries=10, non-stream--retries=10,--stream--retries=3, SDK defaults (real-world config), non-stream--retries=3, SDK defaults (real-world config),--streamThe SDK's own
max_retries(set atverify.py:95) is a different layer — it applies to both modesequally, and it cannot help the mid-stream case at all, since the 200 has already landed.
Why it matters beyond the retry count
all_request_count(verify.py:255) counts retry attempts and is reported asall_count(
verify.py:484), whichscripts/calculate_batch_metrics.py:255uses as the denominator ofQuery-Success-Rate.Since stream mode never retries, its
all_countequals its request count, while non-stream inflateson a flaky provider. The same provider scores a better Query-Success-Rate under
--streamthanwithout it — so the numbers aren't comparable across modes today.
The change
except Exception as e: logger.error(f"Stream request failed: {e}") - return "failed", {"error": str(e)} + # Re-raise so send_request's retry loop can see the failure. Returning + # here would exit the loop after a single attempt. + raiseraiserather than deleting thetry— it keeps the stream-specific log line, and dropping theblock would mean dedenting ~75 lines for no behavioural gain. The final-failure return shape is
unchanged:
send_requestalready returns"failed", {"error": str(last_error)}at line 276.Not included, deliberately
Spotted while tracing this, left alone:
verify.py:612's help text says"Number of retries on failure (default: 3)"whileverify.py:611isdefault=10. Separateconcern, happy to send it as its own one-word PR.
No test. There is no unit-test suite here —
m3_format_check/needs a live provider endpoint — andadding test infrastructure isn't this PR's concern. The reproduction above is a standalone script
driving the real code path against a local failing server; happy to contribute it, or a proper test,
if you'd like the harness.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.