From 5498dccd60d7910b082b5ffb131ea6107ddd8281 Mon Sep 17 00:00:00 2001 From: Rodin Date: Mon, 11 May 2026 05:15:07 -0700 Subject: [PATCH] fix(gitea): address MINOR review findings on retry logic 1. Fix non-deterministic test TestDoGet_RetriesOnTemporaryNetError: - Replace timing-dependent listener approach with mockTransport - mockTransport allows controlled injection of net.OpError failures - Test now makes deterministic assertions: exactly 3 attempts (2 fail + 1 success) - Added SetHTTPClient() method for test transport injection 2. Sanitize error content in retry warning logs: - Added sanitizeErrorForLog() helper that omits response body content - For APIError: logs only 'HTTP ' instead of full body - For other errors: preserves error type information - Addresses security concern about logging server error content at WARN level - Full error with body still returned to caller for proper error handling Both changes have corresponding test coverage. --- gitea/client.go | 22 +++++++- gitea/client_test.go | 126 ++++++++++++++++++++++++++++++------------- 2 files changed, 111 insertions(+), 37 deletions(-) diff --git a/gitea/client.go b/gitea/client.go index 5826958..1cee5b4 100644 --- a/gitea/client.go +++ b/gitea/client.go @@ -72,6 +72,12 @@ func NewClient(baseURL, token string) *Client { } } +// SetHTTPClient sets the underlying HTTP client used for requests. +// This is intended for testing to inject mock transports. +func (c *Client) SetHTTPClient(hc *http.Client) { + c.http = hc +} + // PullRequest holds relevant PR metadata. type PullRequest struct { Title string `json:"title"` @@ -302,6 +308,20 @@ func redactURL(rawURL string) string { return parsed.String() } +// sanitizeErrorForLog returns a loggable version of an error that omits +// potentially sensitive content like response bodies. For APIError, only +// the status code is included; for other errors, the type is preserved. +func sanitizeErrorForLog(err error) string { + if err == nil { + return "" + } + var apiErr *APIError + if errors.As(err, &apiErr) { + return fmt.Sprintf("HTTP %d", apiErr.StatusCode) + } + return err.Error() +} + // doGet performs an HTTP GET request with retry on 5xx errors and temporary // network errors. Retries up to 3 times with exponential backoff (1s, 2s delays // by default; configurable via Client.RetryBackoff for testing). @@ -334,7 +354,7 @@ func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) { "attempt", attempt+1, "url", redactURL(reqURL), "delay", delay.String(), - "lastError", lastErr) + "lastError", sanitizeErrorForLog(lastErr)) timer := time.NewTimer(delay) select { diff --git a/gitea/client_test.go b/gitea/client_test.go index 4578ad4..76156e7 100644 --- a/gitea/client_test.go +++ b/gitea/client_test.go @@ -10,6 +10,7 @@ import ( "net/http" "net/http/httptest" "strings" + "sync/atomic" "syscall" "testing" "time" @@ -891,49 +892,60 @@ func TestDoGet_RespectsContextCancellation(t *testing.T) { } } -func TestDoGet_RetriesOnTemporaryNetError(t *testing.T) { - attempts := 0 - // Create a listener that we can close to simulate connection refused - ln, err := net.Listen("tcp", "127.0.0.1:0") - if err != nil { - t.Fatalf("failed to create listener: %v", err) +// mockTransport is a test helper that returns errors for the first N calls, +// then delegates to a real server. +type mockTransport struct { + failCount int32 // number of failures remaining (atomic) + failErr error // error to return on failure + realServer *httptest.Server + attemptsMade atomic.Int32 // tracks total attempts +} + +func (m *mockTransport) RoundTrip(req *http.Request) (*http.Response, error) { + m.attemptsMade.Add(1) + remaining := atomic.AddInt32(&m.failCount, -1) + if remaining >= 0 { + // Still have failures to return + return nil, m.failErr } - addr := ln.Addr().String() + // Redirect to real server + req.URL.Host = m.realServer.Listener.Addr().String() + req.URL.Scheme = "http" + return http.DefaultTransport.RoundTrip(req) +} - // Close immediately to cause connection refused on first attempts - ln.Close() +func TestDoGet_RetriesOnTemporaryNetError(t *testing.T) { + // Real server that will handle successful requests + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"status":"ok"}`)) + })) + defer server.Close() - // Start a server after a short delay to succeed on retry - go func() { - time.Sleep(5 * time.Millisecond) - newLn, err := net.Listen("tcp", addr) - if err != nil { - // Port might be reused; not critical for this test - return - } - defer newLn.Close() + // Mock transport: fail twice with ECONNREFUSED, then succeed + mt := &mockTransport{ + failCount: 2, + failErr: &net.OpError{Op: "dial", Net: "tcp", Err: syscall.ECONNREFUSED}, + realServer: server, + } - for { - conn, err := newLn.Accept() - if err != nil { - return - } - attempts++ - // Respond with success - conn.Write([]byte("HTTP/1.1 200 OK\r\nContent-Length: 2\r\n\r\nok")) - conn.Close() - } - }() + client := NewClient("http://fake-host/", "test-token") + client.SetHTTPClient(&http.Client{Transport: mt}) + client.RetryBackoff = []time.Duration{1 * time.Millisecond, 1 * time.Millisecond} - client := NewClient("http://"+addr, "test-token") - client.RetryBackoff = []time.Duration{10 * time.Millisecond, 10 * time.Millisecond} + body, err := client.doGet(context.Background(), "http://fake-host/test") + if err != nil { + t.Fatalf("expected success after retries, got error: %v", err) + } + if string(body) != `{"status":"ok"}` { + t.Errorf("body = %q, want %q", string(body), `{"status":"ok"}`) + } - // The request might succeed or fail depending on timing, but the key is - // that we attempt retry on connection refused - _, _ = client.doGet(context.Background(), "http://"+addr+"/test") - - // This test verifies the code path exists; actual retry behavior depends on timing + // Should have made exactly 3 attempts: 2 failures + 1 success + if got := mt.attemptsMade.Load(); got != 3 { + t.Errorf("attempts = %d, want 3 (2 failures + 1 success)", got) + } } func TestIsTemporaryNetError(t *testing.T) { @@ -1037,3 +1049,45 @@ func TestRedactURL(t *testing.T) { }) } } + +func TestSanitizeErrorForLog(t *testing.T) { + tests := []struct { + name string + err error + want string + }{ + { + name: "nil error", + err: nil, + want: "", + }, + { + name: "APIError omits body", + err: &APIError{StatusCode: 500, Body: "internal error: database connection failed"}, + want: "HTTP 500", + }, + { + name: "APIError with large body still only shows status", + err: &APIError{StatusCode: 502, Body: strings.Repeat("x", 1000)}, + want: "HTTP 502", + }, + { + name: "non-API error preserved", + err: fmt.Errorf("connection refused"), + want: "connection refused", + }, + { + name: "wrapped APIError", + err: fmt.Errorf("request failed: %w", &APIError{StatusCode: 503, Body: "service unavailable"}), + want: "HTTP 503", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := sanitizeErrorForLog(tt.err) + if got != tt.want { + t.Errorf("sanitizeErrorForLog() = %q, want %q", got, tt.want) + } + }) + } +}