Skip to content

fix: deadline and timeout with retry #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
christiangda merged 1 commit into from
Mar 7, 2026
Merged

fix: deadline and timeout with retry #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions http_retrier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,16 @@ func (r *retryTransport) RoundTrip(req *http.Request) (*http.Response, error) {
}
}

// Do not retry if the error is due to context cancellation or deadline exceeded.
// When http.Client.Timeout fires, it cancels the request context. Since this
// context is shared across all retry attempts, subsequent retries would fail
// immediately. Return the original error to avoid misleading "retry cancelled" messages.
if err != nil {
if ctx := req.Context(); ctx != nil && ctx.Err() != nil {
return nil, err
}
Comment on lines +134 to +136
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ctx != nil check in the condition ctx := req.Context(); ctx != nil && ctx.Err() != nil is redundant. Per the Go documentation, (*http.Request).Context() always returns a non-nil context — it returns context.Background() if no context was set. The same redundant pattern is already present at line 167 (pre-existing code), so this is consistent, but the new code introduces the same unnecessary nil check. Simplifying to just ctx.Err() != nil would be clearer.

Copilot uses AI. Check for mistakes.
}

// Check if we should retry
if attempt < r.MaxRetries {
delay := retryStrategy(attempt)
Expand Down
50 changes: 50 additions & 0 deletions http_retrier_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1047,6 +1047,56 @@ func TestRetryTransport_ContextCancellation(t *testing.T) {
}
}

func TestRetryTransport_NoRetryOnContextDeadlineExceeded(t *testing.T) {
var attempts int32 = 0

mockRT := &mockRoundTripper{
roundTripFunc: func(req *http.Request) (*http.Response, error) {
atomic.AddInt32(&attempts, 1)
// Simulate the error that http.Client produces when Client.Timeout fires
return nil, fmt.Errorf("Post \"http://localhost:11434/api/generate\": context deadline exceeded (Client.Timeout exceeded while awaiting headers)")
},
}

retryRT := &retryTransport{
Transport: mockRT,
MaxRetries: 5,
RetryStrategy: FixedDelay(1 * time.Millisecond),
}

// Create a request with an already-expired context to simulate Client.Timeout behavior
ctx, cancel := context.WithCancel(context.Background())
cancel() // Cancel immediately to simulate expired deadline
Comment on lines +1067 to +1069
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is named TestRetryTransport_NoRetryOnContextDeadlineExceeded, but the context used in the test is created with context.WithCancel and immediately cancelled — which produces context.Canceled, not context.DeadlineExceeded. The comment on line 1068–1069 also says "simulate expired deadline", but a cancelled context simulates cancellation, not a deadline being exceeded.

While the production fix is correct for both cases (it checks ctx.Err() != nil regardless of the specific error type), the test name and setup are misleading. Consider using context.WithDeadline (with a past time) or context.WithTimeout(ctx, 0) to accurately simulate a deadline-exceeded context, or rename the test to TestRetryTransport_NoRetryOnContextDone to reflect that it covers both cancellation and deadline cases.

Suggested change
// Create a request with an already-expired context to simulate Client.Timeout behavior
ctx, cancel := context.WithCancel(context.Background())
cancel() // Cancel immediately to simulate expired deadline
// Create a request with an already-expired deadline to simulate Client.Timeout behavior
ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(-time.Second))
defer cancel()

Copilot uses AI. Check for mistakes.

req, err := http.NewRequestWithContext(ctx, "POST", "http://localhost:11434/api/generate", nil)
if err != nil {
t.Fatalf("Failed to create request: %v", err)
}

start := time.Now()
_, err = retryRT.RoundTrip(req)
elapsed := time.Since(start)

if err == nil {
t.Fatal("Expected an error, got nil")
}

// Should have made exactly 1 attempt — no retries when context is done
if atomic.LoadInt32(&attempts) != 1 {
t.Errorf("Expected exactly 1 attempt (no retries on context deadline), got %d", atomic.LoadInt32(&attempts))
}

// Should return quickly without waiting for retry delays
if elapsed > 500*time.Millisecond {
t.Errorf("Expected immediate return, but took %v", elapsed)
}

// The error should be the original transport error, not wrapped as "retry cancelled"
if strings.Contains(err.Error(), "retry cancelled") {
t.Errorf("Error should not contain 'retry cancelled', got: %v", err)
}
}

func TestRetryTransport_ContextCancelledDuringRetryDelay(t *testing.T) {
var attempts int32 = 0

Expand Down