fix: survive empty, silent, and reasoning-only provider streams - #394
Closed
gnanam1990 wants to merge 4 commits into
Closed
fix: survive empty, silent, and reasoning-only provider streams#394gnanam1990 wants to merge 4 commits into
gnanam1990 wants to merge 4 commits into
Conversation
… persist
A backend can answer a completion request with HTTP 200 and an SSE stream
that carries nothing at all — no text, no tool calls, no reasoning.
Observed live on the ollama cloud relay under load (200 in ~1s, straight
to [DONE]; healthy calls take 11s+): three such responses arrived within
two seconds, the no-output guard struck out, and the run died with the
generic "Agent stopped after 3 turns with no output" — no error, no hint
the PROVIDER failed. On the reporting machine 577 of 1034 sessions (56%)
ended exactly this way, and the same silent handling produced multi-minute
"Working · thinking" hangs in the TUI plan flow.
Distinguish that PROVIDER-empty shape from a behavioral empty turn (the
model streamed reasoning but committed no answer — its trace proves the
backend worked) via a new CollectedStream.ReasoningEmitted signal:
- providerEmptyStream turns are re-issued in-turn with backoff (bounded by
maxEmptyStreamRetries, mirroring the stall-retry pattern) before they may
count as a no-output strike — the condition is transient backend state
and usually recovers. Nothing from the empty stream was forwarded or
committed to history, so the retry is clean. A user-visible notice
("provider returned an empty response — retrying n/m") rides the
reasoning channel like the stall/reconnect notices.
- When every guard strike was provider-empty, the stop answer names the
backend fault and what to do about it, instead of reading as the agent
giving up. Mixed or behavioral strikes keep the existing message, and
IsNoProgressStop recognizes the new answer so session titling and resume
filtering treat it like the other guard stops.
Tests: provider-empty strikes retry 3× each and stop with the provider
message (recognized by IsNoProgressStop, distinct from the generic
marker); a transient empty recovers on the in-turn retry with no extra
turn counted; mixed provider/behavioral strikes keep the generic message;
the existing counter-reset and guard tests now drive BEHAVIORAL empties
(reasoning-only turns) and are byte-identical in outcome.
…ontent` Reasoning/thinking deltas arrive under different keys depending on the backend dialect: DeepSeek-style servers emit `reasoning_content` (already handled), while ollama's OpenAI-compat endpoint, OpenRouter, and most gateways emit `reasoning`. Zero only parsed the former, so on those backends every thinking token was silently dropped: live capture against ollama glm-5.2:cloud showed 28-227 reasoning deltas per turn discarded even in successful runs. The model looked frozen for minutes behind a dead "thinking" spinner, the discarded thinking was never replayed into the next turn's context, and a reasoning-only turn was indistinguishable from a dead provider (it now feeds CollectedStream.ReasoningEmitted, which the agent loop uses to tell backend faults from behavioral empty turns). Emit StreamEventReasoning from whichever key is present; no backend sends both. The regression test replays the exact delta shape captured live from ollama.
…answer Append the last empty attempt's response shape (finish_reason, output/reasoning token counts) to the provider-empty stop message. This failure class previously took a live traffic capture to diagnose — the sessions recorded only the generic guard text, with nothing about what the backend actually returned. Now the session log itself says e.g. "[last attempt: finish_reason=stop, output_tokens=0, reasoning_tokens=0]", which distinguishes an instantly-empty relay response from a truncated or filtered one at a glance.
… timeout Live capture of the reported "stuck for 33 minutes": the model call streamed ~119KB, then the socket went byte-frozen forever while the TUI showed a dead "Working · thinking" spinner. The only bound on that shape was the full 5-minute idle window — per attempt — so one turn could blind- wait 15+ minutes across its stall retries before surfacing anything. Split the watchdog window by stream phase: - BEFORE the first payload the full idle window still applies (slow cloud proxies legitimately withhold output until the upstream model produces its first token — the reason the idle default is 5m). - AFTER any payload has arrived the backend has proven it streams and heartbeats, so the re-arm window tightens to the gap timeout (default 2m, ZERO_STREAM_GAP_TIMEOUT, clamped to the idle window, disabled with it). A byte-frozen socket mid-stream is a dead connection; waiting the full idle for it just multiplies the user's blind wait per retry. Keep-alives reset the gap like they reset idle, so a slow-but-alive stream is never cut — this only fires on true byte silence. Raise the in-turn stall retries from 2 to 3 (4 attempts total): Codex ships stream_max_retries=5 for the same failure class, and opencode's unbounded session retry has looped forever on it — 4 hard-capped attempts with the faster detection bounds a typical mid-stream death at roughly six minutes to recovery-or-honest-error instead of fifteen-plus blind. All providers benefit (openai/anthropic/gemini share the SSE scanner). Tests: gap applies only after the first payload (frozen-after-first-byte aborts at the tightened window; silent-from-the-start still gets the full idle), resolver default/clamp/env/off semantics, and the existing idle, content-stall, and cancel watchdog tests unchanged.
Contributor
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
gnanam1990
marked this pull request as draft
July 2, 2026 15:01
WalkthroughThis PR distinguishes provider-empty streams (no text, tool calls, or reasoning) from behavioral empty/reasoning-only turns in the agent guardrails and loop, adding dedicated retry logic, a provider-specific stop message, and a ChangesProvider-empty guard and retry
Estimated code review effort: 3 (Moderate) | ~30 minutes OpenAI reasoning delta and stream collection
SSE stream gap timeout
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/agent/loop.go (1)
236-240: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard retries on non-empty text only
forwardedVisibleTextflips on everyStreamEventText, including empty deltas. A placeholder""chunk can disable both the stall-retry loop and the provider-empty retry path for the rest of the turn.🛡️ Proposed fix
if options.OnText != nil { - forwardingOpts.OnText = func(s string) { forwardedVisibleText = true; options.OnText(s) } + forwardingOpts.OnText = func(s string) { + if s != "" { + forwardedVisibleText = true + } + options.OnText(s) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agent/loop.go` around lines 236 - 240, The retry gating in CollectOutputLoop is being flipped by empty StreamEventText chunks, which can incorrectly suppress both stall retries and provider-empty retries. Update the OnText handling in CollectOutputLoop so forwardedVisibleText is set only when the received text is non-empty, and keep the existing retry logic keyed off that flag in the surrounding loop.
🧹 Nitpick comments (1)
internal/agent/guardrails_test.go (1)
478-483: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the turns slice from the retry constants instead of a hardcoded
9.
wantRequestsis computed frommaxEmptyTurns * (1 + maxEmptyStreamRetries), but the turns slice is built with a literal loop bound (9). If either constant changes later, this drifts silently or the mock provider runs out of turns mid-test. The same hardcoded-count pattern recurs at line 547 (emptyTurn(), emptyTurn(), emptyTurn()for "1+2 retries").♻️ Proposed fix
- turns := make([][]zeroruntime.StreamEvent, 0, 9) - for i := 0; i < 9; i++ { // 3 strikes × (1 initial + 2 in-turn retries) + wantRequests := maxEmptyTurns * (1 + maxEmptyStreamRetries) + turns := make([][]zeroruntime.StreamEvent, 0, wantRequests) + for i := 0; i < wantRequests; i++ { // strikes × (1 initial + N in-turn retries) turns = append(turns, emptyTurn()) } provider := &mockProvider{turns: turns}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/agent/guardrails_test.go` around lines 478 - 483, The test setup in TestRunRetriesProviderEmptyStreamsThenStopsWithProviderMessage is hardcoding the number of empty turns instead of deriving it from the retry constants. Build the turns slice from maxEmptyTurns and maxEmptyStreamRetries so it always matches wantRequests, and update the later emptyTurn() sequence in the same test file to use the same retry-based count rather than “1+2 retries” literals.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/providers/providerio/providerio_test.go`:
- Around line 379-405: The current test around ScanSSEDataWithContext only uses
io.Pipe, so it does not verify that a transport-backed Read is interrupted when
the timeout fires. Update TestScanSSETightensGapAfterFirstPayload to exercise a
real http.Response.Body (or equivalent transport-backed reader) and confirm the
timeout path actually cancels a blocked read rather than leaving a goroutine
parked. Keep the assertion on ErrStreamIdle and the tightened
ZERO_STREAM_GAP_TIMEOUT behavior, but make the body source and read path match
the production transport-backed case.
---
Outside diff comments:
In `@internal/agent/loop.go`:
- Around line 236-240: The retry gating in CollectOutputLoop is being flipped by
empty StreamEventText chunks, which can incorrectly suppress both stall retries
and provider-empty retries. Update the OnText handling in CollectOutputLoop so
forwardedVisibleText is set only when the received text is non-empty, and keep
the existing retry logic keyed off that flag in the surrounding loop.
---
Nitpick comments:
In `@internal/agent/guardrails_test.go`:
- Around line 478-483: The test setup in
TestRunRetriesProviderEmptyStreamsThenStopsWithProviderMessage is hardcoding the
number of empty turns instead of deriving it from the retry constants. Build the
turns slice from maxEmptyTurns and maxEmptyStreamRetries so it always matches
wantRequests, and update the later emptyTurn() sequence in the same test file to
use the same retry-based count rather than “1+2 retries” literals.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 77ef9fc7-18a0-4d69-bb55-ec6ee54abbbc
📒 Files selected for processing (10)
internal/agent/guardrails.gointernal/agent/guardrails_test.gointernal/agent/loop.gointernal/agent/reconnect.gointernal/providers/openai/provider.gointernal/providers/openai/provider_test.gointernal/providers/openai/types.gointernal/providers/providerio/providerio.gointernal/providers/providerio/providerio_test.gointernal/zeroruntime/helpers.go
Comment on lines
+379
to
+405
| func TestScanSSETightensGapAfterFirstPayload(t *testing.T) { | ||
| t.Setenv("ZERO_STREAM_GAP_TIMEOUT", "80ms") | ||
| pr, pw := io.Pipe() | ||
| defer func() { _ = pw.Close() }() | ||
|
|
||
| go func() { | ||
| _, _ = io.WriteString(pw, "data: first\n\n") | ||
| // then: byte-frozen forever | ||
| }() | ||
|
|
||
| start := time.Now() | ||
| done := make(chan error, 1) | ||
| go func() { | ||
| done <- ScanSSEDataWithContext(context.Background(), func() {}, pr, 2*time.Second, func(string) bool { return true }) | ||
| }() | ||
| select { | ||
| case err := <-done: | ||
| if !errors.Is(err, ErrStreamIdle) { | ||
| t.Fatalf("err = %v, want ErrStreamIdle", err) | ||
| } | ||
| if elapsed := time.Since(start); elapsed > time.Second { | ||
| t.Fatalf("gap abort took %s — the tightened window (80ms) did not apply after the first payload", elapsed) | ||
| } | ||
| case <-time.After(5 * time.Second): | ||
| t.Fatal("stream not aborted") | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the gap-timeout implementation to see how it aborts a blocked Read.
fd providerio.go --exec cat -n {}Repository: Gitlawb/zero
Length of output: 27693
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the surrounding tests and any helpers that exercise ScanSSEDataWithContext.
fd providerio_test.go --exec sh -c 'echo "### {}"; cat -n "{}" | sed -n "1,520p"' \;Repository: Gitlawb/zero
Length of output: 18733
Exercise a transport-backed response body here, not just io.Pipe. The helper does call cancel() on timeout, but this test only proves the timer path; add coverage that reads from a real http.Response.Body (or another transport-backed reader) so a frozen Read is actually interrupted and not left parked in a leaked goroutine.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/providers/providerio/providerio_test.go` around lines 379 - 405, The
current test around ScanSSEDataWithContext only uses io.Pipe, so it does not
verify that a transport-backed Read is interrupted when the timeout fires.
Update TestScanSSETightensGapAfterFirstPayload to exercise a real
http.Response.Body (or equivalent transport-backed reader) and confirm the
timeout path actually cancels a blocked read rather than leaving a goroutine
parked. Keep the assertion on ErrStreamIdle and the tightened
ZERO_STREAM_GAP_TIMEOUT behavior, but make the body source and read path match
the production transport-backed case.
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
1 participant
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.
Problem
"Zero gets stuck": runs dying instantly with the generic no-output message, and multi-minute "Working · thinking" hangs (one report showed 33 minutes). Reported across providers (ollama cloud, OpenAI OAuth), intermittently — 5/10 runs on a bad stretch. On the machine investigated, 577 of 1034 sessions (56%) ended in the no-output guard, every day for ten days.
Root-caused to three stacked failure modes, each verified with live captures:
POST /v1/chat/completionswith HTTP 200 in ~1s and an SSE that goes straight to[DONE]— no content, no tool calls, no usage (server logs show healthy calls take 11s–1m39s). Zero counted each as a silent empty turn: three strikes inside two seconds, run dead, message blaming the agent ("stopped after 3 turns with no output"), nothing recorded about what the backend returned. Reproduced byte-for-byte with a mock backend.reasoning; Zero only parsed DeepSeek'sreasoning_content. Wire capture showed 28–227 reasoning deltas per turn silently discarded even in successful runs — the model looked frozen behind a dead "thinking" spinner, and its thinking was never replayed into the next turn's context.Fixes (one commit per layer)
fix(agent): retry provider-empty streams and name the fault. NewCollectedStream.ReasoningEmitteddistinguishes "backend returned nothing" from "model thought but committed no answer". Provider-empty turns are re-issued in-turn with backoff (bounded, mirroring the stall-retry pattern; nothing was forwarded or committed, so the retry is clean) with a live notice. When every guard strike was provider-empty, the stop answer names the backend fault and suggests switching providers, instead of reading as the agent giving up.IsNoProgressStoprecognizes the new answer (titling/resume filters unchanged).fix(openai): parsereasoningalongsidereasoning_content. Thinking renders live, feeds the discriminator above, and is preserved for replay. The regression test replays the exact delta shape captured from ollama glm-5.2:cloud.feat(agent): wire-level diagnostics in the stop answer ([last attempt: finish_reason=stop, output_tokens=0, reasoning_tokens=0]) so this class is debuggable from the session log — this investigation needed a live traffic capture because nothing about the response shape was recorded.fix(providerio): phase-split watchdog + more bounded retries. Before the first payload the full 5-minute idle window still applies (slow cloud proxies legitimately withhold output until the first upstream token). After any payload arrives, the re-arm window tightens to a gap timeout (default 2m,ZERO_STREAM_GAP_TIMEOUT, clamped to idle, disabled with it) — a byte-frozen socket mid-stream is a dead connection, and keep-alives still reset the window so slow-but-alive streams are never cut. In-turn stall retries go 2→3 (4 attempts): Codex shipsstream_max_retries=5for this class, while opencode's unbounded retry has looped forever on it — 4 hard-capped attempts with fast detection bounds a typical mid-stream death at ~6 minutes to recovery-or-honest-error instead of 15+ blind. All providers share the SSE scanner and benefit.Testing
IsNoProgressStop, carrying diagnostics); a transient empty recovers in-turn with no extra turn counted; mixed provider/behavioral strikes keep the generic message; ollama-stylereasoningdeltas emit reasoning events; gap timeout applies only after the first payload; resolver default/clamp/env/off semantics.go test ./...— 73/73 packages.Summary by CodeRabbit
New Features
Bug Fixes