[code-review] OpenAI streaming connection retry is dead code — missing 'continue' after retrySleep causes immediate fatal error

[topcheer]

File and Lines

internal/provider/openai.go lines 396-409

Problem Description

When createChatCompletionStream fails with a retryable error (502, 503, 504, 429, network timeout), the retry logic correctly sleeps for the backoff delay but then falls through to the "CONNECT FATAL" error path instead of continuing to the next loop iteration. A continue statement is missing after retrySleep.

if isRetryableForContext(ctx, err) && attempt < providerRetryAttempts-1 {
    delay := retryDelay(err, attempt)
    ch <- StreamEvent{Type: StreamEventSystem, Text: ...}  // "Retry 1/20..."
    if sleepErr := retrySleep(ctx, delay); sleepErr != nil { ... }
    // ← MISSING: continue
}
// Falls through unconditionally:
debug.Log("openai", "CONNECT FATAL ...")
ch <- StreamEvent{Type: StreamEventError, Error: fmt.Errorf("openai stream: %w", err)}
return  // goroutine exits, no retry happens

Contrast with the correct implementation in anthropic.go (lines 432-452):

if isRetryableForContext(ctx, err) && attempt < providerRetryAttempts-1 {
    // ...sleep...
    retry = true
    return  // exits closure, not goroutine
}
// After closure:
if retry {
    continue  // ← correctly continues the outer for loop
}

Trigger Scenario

1. Agent sends request to OpenAI API
2. Network experiences a transient failure — API gateway returns 502 Bad Gateway or 503 Service Unavailable
3. createChatCompletionStream fails during connection phase
4. User sees: [Retry 1/20, waiting 1s...] — believes retry is working
5. Immediately sees: fatal error openai stream: ...502... — request fails completely
6. Zero retries actually executed. The for attempt := 0; attempt < 20 loop exits on attempt 0.

Expected vs Actual Behavior

• Expected: Sleep for backoff delay, then loop back and retry the connection (up to 20 attempts)
• Actual: Sleep for backoff delay, then immediately return a fatal error. The retry loop is dead code.

Impact

Critical (P1): Every transient OpenAI API failure (502/503/504/429/network blip) is treated as fatal. The connection-level retry infrastructure exists in code but is completely non-functional. Any momentary API instability terminates the entire agent run.

This is user-visible: the misleading [Retry 1/20...] notification is sent, followed immediately by a fatal error, creating a false impression that retries were exhausted.

Note: Mid-stream disconnection retries (after the connection is established) use a different path (the retry flag at line 415+) and work correctly. Only connection-establishment retries are broken.

Fix

Add continue after the sleep block (between current lines 405 and 406):

if sleepErr := retrySleep(ctx, delay); sleepErr != nil {
    ch <- StreamEvent{Type: StreamEventError, Error: sleepErr}
    streamError = true
    return
}
continue  // ← skip CONNECT FATAL, proceed to next attempt

Verification

Independently verified by subagent (sa-70): confirmed the missing continue via source code analysis, compared with anthropic.go's correct retry pattern, and traced the control flow to confirm the retry loop never reaches iteration 2.

[topcheer]

Fixed in commit e99e949.

Root cause: createChatCompletionStream connection-failure path slept for the backoff delay but had no continue, falling through to the CONNECT FATAL error — the retry loop never reached attempt 2. Mid-stream retries used a different (correct) path, which is why only connection-establishment failures were affected.

Fix: Added continue after retrySleep so transient 502/503/504/429/network errors loop back and retry up to providerRetryAttempts (20), matching anthropic.go's pattern.

Provider + a2a + wailskit package tests all pass.