fix(core): outlast transient transport faults and restart an interrupted stream - #316
Merged
Merged
Conversation
…ted stream The model client retried a failed request three times inside half a second and reported anything later as the turn's error. A transient fault on the path to an endpoint (a reset or a TLS alert on send, a stream cut mid-reply) recurs for minutes and clears within seconds most times, so those attempts only ever observed the fault and ended turns the next connection would have completed, with the task abandoned mid-way. A stream that died before the reply existed was the same fault at a different moment and was reported as a truncation. The budget is now eight attempts with an exponential backoff from one second to a sixteen second cap, about a minute in all. A stream the network ended before any reply text arrived is started over on a fresh attempt; reasoning deltas are display-only and do not count, and once reply text has streamed a retry would duplicate what the caller already showed, so that case stays a truncation. Streams the client cuts itself (size cap, tool index cap, cancellation) and bodies that fail to decode are never retried. Three attempts in a row that never reach the endpoint give up early, so a local server that is not running still fails in seconds. Every resend is announced through a new `retry` wire event, rendered as a note by the TUI and a stderr line in print mode, and the backoff wait is cancellable. The stdio protocol version moves to 6 for the new event. One CLI fixture streams reply text before its tool call so the interrupted stream it serves is the kind that is not started over.
| cancelled: &crate::state::CancelToken, | ||
| on_delta: &mut impl FnMut(StreamDelta), | ||
| ) -> bool { | ||
| on_delta(StreamDelta::Retry { attempt: attempt + 1, max_attempts: MAX_ATTEMPTS, reason: reason.to_string() }); |
There was a problem hiding this comment.
This emits a retry event before the cancellable backoff completes. If the user cancels during that wait, the next request is never sent even though consumers were told that retry attempt N+1 was occurring. This is a non-blocking protocol accuracy issue that can leave displayed retry status and retry telemetry incorrect. Emit the event after the wait succeeds, or represent it as a scheduled retry.
Artifacts
- Authored integration-test harness starts a local HTTP endpoint, records Retry events, cancels during the announced retry backoff, and asserts the actual request count; it demonstrates that the announcement can outlive the dispatched attempt.
Retry cancellation harness runner
- Authored runner copies the harness into Cargo's temporary integration-test location, runs a selected test with captured output, and removes the temporary test afterward; it provides the reproducible execution command.
- Captured execution of the baseline request sequence `HTTP/1.1 429 Too Many Requests -> HTTP/1.1 200 OK` shows Retry attempt 2/8, `finish_reason=stop`, and two actual HTTP attempts; the announced retry normally dispatches.
- Captured execution of the cancellation case against `HTTP/1.1 429 Too Many Requests` shows Retry attempt 2/8, `finish_reason=cancelled`, and one actual HTTP attempt with exit code 0; cancellation prevents the announced retry from dispatching.
Ran code and verified through T-Rex
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/core/src/client.rs
Line: 777
Comment:
**Announce sent retries**
This emits a retry event before the cancellable backoff completes. If the user cancels during that wait, the next request is never sent even though consumers were told that retry attempt N+1 was occurring. This is a non-blocking protocol accuracy issue that can leave displayed retry status and retry telemetry incorrect. Emit the event after the wait succeeds, or represent it as a scheduled retry.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.…nreached test The retry note left the failed attempt's reasoning tail on screen, so the fresh attempt's reasoning read as a continuation of it. The tail and its caches are cleared when the retry arrives. The unreached-address test now uses a port it bound and released, under a deadline, so a stray listener cannot turn a refusal into a hang. The retry event's docs say it precedes the backoff wait and that a cancel during the wait means the attempt never goes out.
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.
Why
The model client retried a failed request three times inside half a second and reported anything later as the turn's error. A transient fault on the path to an endpoint (a reset or a TLS alert on send, a stream cut mid-reply) recurs for minutes and clears within seconds most times and within a minute at worst, so those attempts only ever observed the fault and ended turns the next connection would have completed, with the task abandoned mid-way. On one full run of the release binary this was the single largest cause of failed turns: one turn in six ended on a TLS alert at send time, and one in ten on a stream that died a few hundred characters into the model's reasoning, before any reply text. A stream that died before the reply existed was the same fault at a different moment and was reported as a truncation.
Summary
retrywire event (attempt,max_attempts,reason), rendered as a note by the TUI and a stderr line in print mode. The stdio protocol version moves to 6 for the new event, per the "any wire change bumps both" rule; the spec, docs, README, golden, and tripwire carry it.read_sse;git diff -w --color-movedshows the three edits (EOF arm, read error arm, theinterruptedflag). 429 stays the only retried status; 5xx policy is unchanged.Test Plan
client.rs:a_stream_interrupted_before_any_reply_text_is_started_over,a_request_dropped_on_send_is_resent_until_a_reply_arrives,a_stream_interrupted_after_reply_text_is_not_retried,an_address_that_never_answers_gives_up_before_the_budget;a_truncated_stream_still_returns_the_tool_calls_it_carriednow spends the whole budget first. With the stream retry disabled and the old budget of three restored, three of these fail as expected.types.rspin theretryenvelope.cargo test --workspace --lockedgreen (602 core, 27 CLI in under 4 s);cargo +1.97.0 clippy --workspace --all-targets --locked -- -D warningsclean.Greptile Summary
This update clears abandoned reasoning before a replacement stream is displayed and makes the unreachable-endpoint test use a released ephemeral port with a deadline. A remaining retry-status accuracy concern is non-blocking.
Confidence Score: 5/5
Safe to merge: no blocking issues remain.
The retry-status issue remains outstanding: a retry notice is emitted before the cancellable backoff completes, so cancellation during that wait can report an attempt that is never sent. This is non-blocking. greptile-apps[bot] resolved the abandoned-reasoning display thread without explanation; the current code clears the attempt-local reasoning state before the retry is rendered. greptile-apps[bot] resolved the unreachable-host test thread without explanation; the current test uses a released ephemeral port and an explicit deadline.
Reviews (2): Last reviewed commit: "fix(tui): drop the failed attempt's reas..." | Re-trigger Greptile