diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index 2a44aed083..dd80a73a40 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -199,7 +199,7 @@ func (c *LiveClient) do(ctx context.Context, method, path string, body any) (*ht } // HTTP client timeout (Client.Timeout exceeded): retry // with exponential backoff, same as transient server errors. - if isTimeoutError(err) { + if isTimeoutError(ctx, err) { if attempt == maxRetries-1 { return nil, fmt.Errorf("http %s %s: %w (after %d attempts)", method, path, err, maxRetries) } @@ -289,10 +289,20 @@ func isRetryable(resp *http.Response) (bool, []byte) { } // isTimeoutError reports whether err is an HTTP client timeout (e.g. -// Client.Timeout exceeded) as opposed to a caller-context cancellation. -// Callers must check ctx.Err() first — this function only distinguishes -// timeout transport errors from other transport errors. -func isTimeoutError(err error) bool { +// Client.Timeout exceeded) as opposed to a caller-context cancellation +// or deadline. It checks ctx.Err() internally so callers do not need +// to guard against context errors before calling this function. +// +// The context check is necessary because Go's net/http client timeout +// wraps context.DeadlineExceeded internally, making error-only +// introspection unable to distinguish caller deadlines from transport +// timeouts. Checking the caller's context disambiguates: if ctx.Err() +// is non-nil, the caller's context expired; otherwise, any Timeout() +// error is a transport-level timeout worth retrying. +func isTimeoutError(ctx context.Context, err error) bool { + if ctx.Err() != nil { + return false + } var te interface{ Timeout() bool } return errors.As(err, &te) && te.Timeout() } diff --git a/internal/forge/github/github_test.go b/internal/forge/github/github_test.go index 499105a1ad..4ea5226e75 100644 --- a/internal/forge/github/github_test.go +++ b/internal/forge/github/github_test.go @@ -2934,41 +2934,69 @@ func TestDoDoesNotRetryOnCallerContextCancel(t *testing.T) { } func TestIsTimeoutError(t *testing.T) { + // Create an already-cancelled context for testing the context guard. + cancelledCtx, cancel := context.WithCancel(context.Background()) + cancel() + tests := []struct { name string + ctx context.Context err error want bool }{ { name: "nil error", + ctx: context.Background(), err: nil, want: false, }, { name: "generic error", + ctx: context.Background(), err: fmt.Errorf("connection refused"), want: false, }, { - name: "context.DeadlineExceeded", + name: "context.DeadlineExceeded with active context", + ctx: context.Background(), err: context.DeadlineExceeded, want: true, }, { - name: "wrapped context.DeadlineExceeded", + name: "context.DeadlineExceeded with cancelled context", + ctx: cancelledCtx, + err: context.DeadlineExceeded, + want: false, + }, + { + name: "wrapped context.DeadlineExceeded with cancelled context", + ctx: cancelledCtx, err: fmt.Errorf("request failed: %w", context.DeadlineExceeded), - want: true, + want: false, }, { - name: "context.Canceled is not a timeout", + name: "context.Canceled with active context", + ctx: context.Background(), err: context.Canceled, want: false, }, + { + name: "context.Canceled with cancelled context", + ctx: cancelledCtx, + err: context.Canceled, + want: false, + }, + { + name: "timeout error with cancelled context returns false", + ctx: cancelledCtx, + err: context.DeadlineExceeded, + want: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, isTimeoutError(tt.err)) + assert.Equal(t, tt.want, isTimeoutError(tt.ctx, tt.err)) }) } }