fix(gitea): improve retry logic precision for net.OpError
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 46s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m11s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m23s

Address review feedback on isTemporaryNetError being too broad:

1. RetryBackoff field: Added doc comment clarifying it must be
   configured before the first request (addresses concurrency concern).

2. isTemporaryNetError: Now inspects the underlying syscall error
   instead of treating all net.OpError as retriable. Only retries on:
   - ECONNREFUSED (connection refused)
   - ECONNRESET (connection reset)
   - ENETUNREACH (network unreachable)
   - EHOSTUNREACH (host unreachable)
   - ETIMEDOUT (connection timed out)

   Permanent errors like EACCES, EPERM are no longer retried.

3. DNS errors: Changed from Temporary() to IsTimeout, since
   "no such host" is permanent and shouldn't be retried.

4. Empty backoff slice: Added comment explaining that retry without
   delay is intentional when caller explicitly configures it.

Addresses MINOR findings from sonnet-review-bot and gpt-review-bot.
This commit is contained in:
Rodin
2026-05-11 04:32:15 -07:00
parent 090ae3848c
commit ac53ecfa5d
2 changed files with 86 additions and 12 deletions
+42 -3
View File
@@ -10,6 +10,7 @@ import (
"net/http"
"net/http/httptest"
"strings"
"syscall"
"testing"
"time"
)
@@ -943,9 +944,20 @@ func TestIsTemporaryNetError(t *testing.T) {
}{
{"nil error", nil, false},
{"plain error", fmt.Errorf("some error"), false},
{"OpError", &net.OpError{Op: "dial", Err: fmt.Errorf("connection refused")}, true},
{"temporary DNSError", &net.DNSError{IsTemporary: true}, true},
{"non-temporary DNSError", &net.DNSError{IsTemporary: false}, false},
// OpError with retriable syscall errors
{"OpError ECONNREFUSED", &net.OpError{Op: "dial", Err: syscall.ECONNREFUSED}, true},
{"OpError ECONNRESET", &net.OpError{Op: "read", Err: syscall.ECONNRESET}, true},
{"OpError ENETUNREACH", &net.OpError{Op: "dial", Err: syscall.ENETUNREACH}, true},
{"OpError EHOSTUNREACH", &net.OpError{Op: "dial", Err: syscall.EHOSTUNREACH}, true},
{"OpError ETIMEDOUT", &net.OpError{Op: "dial", Err: syscall.ETIMEDOUT}, true},
// OpError with permanent syscall errors — should NOT retry
{"OpError EACCES", &net.OpError{Op: "dial", Err: syscall.EACCES}, false},
{"OpError EPERM", &net.OpError{Op: "dial", Err: syscall.EPERM}, false},
// OpError with unknown inner error — conservative retry
{"OpError unknown inner", &net.OpError{Op: "dial", Err: fmt.Errorf("unknown")}, true},
// DNS errors
{"DNS timeout", &net.DNSError{IsTimeout: true}, true},
{"DNS no such host", &net.DNSError{IsTimeout: false, Name: "bad.host"}, false},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
@@ -956,3 +968,30 @@ func TestIsTemporaryNetError(t *testing.T) {
})
}
}
func TestIsRetriableSyscallError(t *testing.T) {
tests := []struct {
name string
err error
want bool
}{
{"nil", nil, false},
{"ECONNREFUSED", syscall.ECONNREFUSED, true},
{"ECONNRESET", syscall.ECONNRESET, true},
{"ENETUNREACH", syscall.ENETUNREACH, true},
{"EHOSTUNREACH", syscall.EHOSTUNREACH, true},
{"ETIMEDOUT", syscall.ETIMEDOUT, true},
{"EACCES (permanent)", syscall.EACCES, false},
{"EPERM (permanent)", syscall.EPERM, false},
{"ENOENT (permanent)", syscall.ENOENT, false},
{"unknown error", fmt.Errorf("something"), true}, // conservative retry
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := isRetriableSyscallError(tt.err)
if got != tt.want {
t.Errorf("isRetriableSyscallError(%v) = %v, want %v", tt.err, got, tt.want)
}
})
}
}