Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 15 additions & 5 deletions internal/forge/github/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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
Comment thread
rh-hemartin marked this conversation as resolved.
// 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()
}
Expand Down
38 changes: 33 additions & 5 deletions internal/forge/github/github_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
}

Comment thread
rh-hemartin marked this conversation as resolved.
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))
})
}
}
Expand Down
Loading