From 4990bfbd3cb1efc24991bf2e456bb9354f1d7d13 Mon Sep 17 00:00:00 2001 From: BadgerOps Date: Tue, 24 Feb 2026 19:57:45 -0600 Subject: [PATCH 1/2] perf: reduce download test suite from 92s to <1s MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The download package tests were the CI bottleneck, consuming ~92s of the ~97s unit test step. Three root causes: 1. TestDownloadFileTimeout had a 60s server-side sleep. The client context timed out in 1s, but httptest.Server.Close() blocked waiting for the handler goroutine. Fixed by making the handler respect the request context so it exits immediately on cancellation. 2. TestDownloadFileContextCancellation sent 100 chunks with 100ms sleeps (10s total). Reduced to 50 chunks at 10ms with context-aware select loops. 3. Multiple retry tests (ServerError, ChecksumMismatch, SizeValidation, Retry) used real exponential backoff sleeps (1-3s each). Made the backoff function injectable on Client via a BackoffFunc field, with test helper that sets zero-delay backoff. Also reduced TestPoolContextCancellation and TestPoolConcurrency server delays proportionally. All 13 packages still pass. No production code behavior changes — the BackoffFunc field defaults to the existing calculateBackoffDelay. Co-Authored-By: Claude Opus 4.6 --- CHANGELOG.md | 7 +++ internal/download/client.go | 17 +++++--- internal/download/client_test.go | 74 +++++++++++++++++++++----------- internal/download/pool_test.go | 44 +++++++++---------- 4 files changed, 87 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 348c5fd..8d14152 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,13 @@ All notable changes to this project are documented in this file. +## 0.3.5 - 2026-02-24 + +### Changed + +- Reduced download package test suite runtime from ~92s to ~1s by making retry backoff injectable and eliminating unnecessary wall-clock sleeps in test server handlers. +- Test HTTP handlers now respect request context cancellation so `httptest.Server.Close()` returns immediately instead of blocking on in-flight handlers. + ## 0.3.4 - 2026-02-24 ### Added diff --git a/internal/download/client.go b/internal/download/client.go index 84a5939..411dd51 100644 --- a/internal/download/client.go +++ b/internal/download/client.go @@ -43,11 +43,15 @@ type DownloadResult struct { Duration time.Duration // Total download duration } +// BackoffFunc calculates the delay before a retry attempt. +type BackoffFunc func(attempt int) time.Duration + // Client performs HTTP downloads with retry logic, resumption, and validation. type Client struct { - httpClient *http.Client - logger *slog.Logger - userAgent string + httpClient *http.Client + logger *slog.Logger + userAgent string + backoffFunc BackoffFunc } // NewClient creates a new download client with the given logger. @@ -65,8 +69,9 @@ func NewClient(logger *slog.Logger) *Client { // No overall Timeout — body reads can take as long as needed. // Context cancellation still works for user-initiated cancel. }, - logger: logger, - userAgent: "airgap/1.0", + logger: logger, + userAgent: "airgap/1.0", + backoffFunc: calculateBackoffDelay, } } @@ -159,7 +164,7 @@ func (c *Client) Download(ctx context.Context, opts DownloadOptions) (*DownloadR // Wait before retrying with exponential backoff + jitter if attempt < opts.RetryCount { - delay := calculateBackoffDelay(attempt) + delay := c.backoffFunc(attempt) c.logger.Debug("retrying download", "url", opts.URL, "delay", delay) select { case <-time.After(delay): diff --git a/internal/download/client_test.go b/internal/download/client_test.go index 259e01c..a09dc65 100644 --- a/internal/download/client_test.go +++ b/internal/download/client_test.go @@ -15,10 +15,17 @@ import ( "time" ) +// newTestClient creates a client with zero-delay backoff for fast tests. +func newTestClient(logger *slog.Logger) *Client { + c := NewClient(logger) + c.backoffFunc = func(attempt int) time.Duration { return 0 } + return c +} + // TestNewClient creates client with logger func TestNewClient(t *testing.T) { logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - client := NewClient(logger) + client := newTestClient(logger) if client == nil { t.Fatal("expected client to be non-nil") @@ -49,7 +56,7 @@ func TestDownloadFile(t *testing.T) { destPath := filepath.Join(tmpDir, "testfile.bin") logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - client := NewClient(logger) + client := newTestClient(logger) result, err := client.Download(context.Background(), DownloadOptions{ URL: server.URL, @@ -114,7 +121,7 @@ func TestDownloadFileWithHeaders(t *testing.T) { tmpDir := t.TempDir() destPath := filepath.Join(tmpDir, "header.bin") logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - client := NewClient(logger) + client := newTestClient(logger) result, err := client.Download(context.Background(), DownloadOptions{ URL: server.URL, @@ -154,7 +161,7 @@ func TestDownloadFileWithChecksum(t *testing.T) { destPath := filepath.Join(tmpDir, "testfile_checksum.bin") logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - client := NewClient(logger) + client := newTestClient(logger) result, err := client.Download(context.Background(), DownloadOptions{ URL: server.URL, @@ -195,7 +202,7 @@ func TestDownloadFileChecksumMismatch(t *testing.T) { destPath := filepath.Join(tmpDir, "testfile_bad_checksum.bin") logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - client := NewClient(logger) + client := newTestClient(logger) wrongChecksum := "0000000000000000000000000000000000000000000000000000000000000000" @@ -231,7 +238,7 @@ func TestDownloadFileNotFound(t *testing.T) { destPath := filepath.Join(tmpDir, "testfile_404.bin") logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - client := NewClient(logger) + client := newTestClient(logger) result, err := client.Download(context.Background(), DownloadOptions{ URL: server.URL, @@ -264,7 +271,7 @@ func TestDownloadFileServerError(t *testing.T) { destPath := filepath.Join(tmpDir, "testfile_500.bin") logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - client := NewClient(logger) + client := newTestClient(logger) result, err := client.Download(context.Background(), DownloadOptions{ URL: server.URL, @@ -304,7 +311,7 @@ func TestDownloadFileRetry(t *testing.T) { destPath := filepath.Join(tmpDir, "testfile_retry.bin") logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - client := NewClient(logger) + client := newTestClient(logger) result, err := client.Download(context.Background(), DownloadOptions{ URL: server.URL, @@ -368,7 +375,7 @@ func TestDownloadFileResume(t *testing.T) { } logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - client := NewClient(logger) + client := newTestClient(logger) result, err := client.Download(context.Background(), DownloadOptions{ URL: server.URL, @@ -400,11 +407,17 @@ func TestDownloadFileResume(t *testing.T) { // TestDownloadFileTimeout httptest with delayed response, verify timeout handling func TestDownloadFileTimeout(t *testing.T) { + serverCtx, serverCancel := context.WithCancel(context.Background()) + defer serverCancel() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Simulate a slow server by sleeping longer than the timeout - time.Sleep(60 * time.Second) - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte("This should timeout")) + // Block until either request or server context is done, so + // server.Close() doesn't wait for the full sleep duration. + select { + case <-r.Context().Done(): + case <-serverCtx.Done(): + case <-time.After(30 * time.Second): + } })) defer server.Close() @@ -412,15 +425,16 @@ func TestDownloadFileTimeout(t *testing.T) { destPath := filepath.Join(tmpDir, "testfile_timeout.bin") logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - client := NewClient(logger) + client := newTestClient(logger) // Create a context with a short timeout - ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) defer cancel() result, err := client.Download(ctx, DownloadOptions{ - URL: server.URL, - DestPath: destPath, + URL: server.URL, + DestPath: destPath, + RetryCount: 1, }) if err == nil { @@ -430,16 +444,24 @@ func TestDownloadFileTimeout(t *testing.T) { if result != nil { t.Fatal("expected result to be nil on timeout") } + + // Signal server handler to stop so server.Close() returns quickly. + serverCancel() } // TestDownloadFileContextCancellation verifies that context cancellation stops the download func TestDownloadFileContextCancellation(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Send a large response slowly - for i := 0; i < 100; i++ { - _, _ = w.Write([]byte("chunk")) - w.(http.Flusher).Flush() - time.Sleep(100 * time.Millisecond) + // Send chunks slowly; stop when the request context is cancelled + // so server.Close() returns promptly. + for i := 0; i < 50; i++ { + select { + case <-r.Context().Done(): + return + case <-time.After(10 * time.Millisecond): + _, _ = w.Write([]byte("chunk")) + w.(http.Flusher).Flush() + } } })) defer server.Close() @@ -448,13 +470,13 @@ func TestDownloadFileContextCancellation(t *testing.T) { destPath := filepath.Join(tmpDir, "testfile_cancel.bin") logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - client := NewClient(logger) + client := newTestClient(logger) ctx, cancel := context.WithCancel(context.Background()) // Cancel after a short delay go func() { - time.Sleep(200 * time.Millisecond) + time.Sleep(50 * time.Millisecond) cancel() }() @@ -488,7 +510,7 @@ func TestDownloadFileProgress(t *testing.T) { destPath := filepath.Join(tmpDir, "testfile_progress.bin") logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - client := NewClient(logger) + client := newTestClient(logger) progressCallCount := 0 onProgress := func(bytesDownloaded, totalBytes int64) { @@ -529,7 +551,7 @@ func TestDownloadFileSizeValidation(t *testing.T) { destPath := filepath.Join(tmpDir, "testfile_size.bin") logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - client := NewClient(logger) + client := newTestClient(logger) // Expect a different size expectedSize := int64(len(testContent) + 100) diff --git a/internal/download/pool_test.go b/internal/download/pool_test.go index c7469da..95acfd1 100644 --- a/internal/download/pool_test.go +++ b/internal/download/pool_test.go @@ -20,7 +20,7 @@ import ( // TestNewPool creates pool with given workers func TestNewPool(t *testing.T) { logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - client := NewClient(logger) + client := newTestClient(logger) pool := NewPool(client, 5, logger) @@ -41,7 +41,7 @@ func TestNewPool(t *testing.T) { // TestNewPoolDefaultWorkers verifies pool defaults to 1 worker if workers <= 0 func TestNewPoolDefaultWorkers(t *testing.T) { logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - client := NewClient(logger) + client := newTestClient(logger) pool := NewPool(client, 0, logger) @@ -81,7 +81,7 @@ func TestPoolExecute(t *testing.T) { tmpDir := t.TempDir() logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - client := NewClient(logger) + client := newTestClient(logger) pool := NewPool(client, 3, logger) jobs := []Job{ @@ -148,7 +148,7 @@ func TestPoolConcurrency(t *testing.T) { mu.Unlock() // Simulate some work - time.Sleep(100 * time.Millisecond) + time.Sleep(20 * time.Millisecond) w.Header().Set("Content-Type", "application/octet-stream") w.WriteHeader(http.StatusOK) @@ -159,7 +159,7 @@ func TestPoolConcurrency(t *testing.T) { tmpDir := t.TempDir() logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - client := NewClient(logger) + client := newTestClient(logger) // Create pool with 4 workers pool := NewPool(client, 4, logger) @@ -208,7 +208,7 @@ func TestPoolWithFailures(t *testing.T) { tmpDir := t.TempDir() logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - client := NewClient(logger) + client := newTestClient(logger) pool := NewPool(client, 2, logger) jobs := make([]Job, 6) @@ -259,10 +259,14 @@ func TestPoolWithFailures(t *testing.T) { // TestPoolContextCancellation cancel context mid-execution, verify pool stops func TestPoolContextCancellation(t *testing.T) { slowServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Simulate a slow server - for i := 0; i < 20; i++ { - time.Sleep(100 * time.Millisecond) - _, _ = w.Write([]byte("chunk")) + // Send chunks slowly; respect request context so server.Close() is fast. + for i := 0; i < 50; i++ { + select { + case <-r.Context().Done(): + return + case <-time.After(10 * time.Millisecond): + _, _ = w.Write([]byte("chunk")) + } } })) defer slowServer.Close() @@ -270,7 +274,7 @@ func TestPoolContextCancellation(t *testing.T) { tmpDir := t.TempDir() logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - client := NewClient(logger) + client := newTestClient(logger) pool := NewPool(client, 2, logger) // Create multiple jobs @@ -286,7 +290,7 @@ func TestPoolContextCancellation(t *testing.T) { // Cancel context after a short delay go func() { - time.Sleep(250 * time.Millisecond) + time.Sleep(50 * time.Millisecond) cancel() }() @@ -298,19 +302,13 @@ func TestPoolContextCancellation(t *testing.T) { } // Not all should succeed due to cancellation - successCount := 0 failureCount := 0 - for _, result := range results { - if result.Success { - successCount++ - } else { + if !result.Success { failureCount++ } } - t.Logf("Results after cancellation: %d successes, %d failures", successCount, failureCount) - if failureCount == 0 { t.Fatal("expected some failures due to context cancellation") } @@ -319,7 +317,7 @@ func TestPoolContextCancellation(t *testing.T) { // TestPoolEmptyJobs verifies pool handles empty job list func TestPoolEmptyJobs(t *testing.T) { logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - client := NewClient(logger) + client := newTestClient(logger) pool := NewPool(client, 3, logger) results := pool.Execute(context.Background(), []Job{}) @@ -341,7 +339,7 @@ func TestPoolResultOrder(t *testing.T) { tmpDir := t.TempDir() logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - client := NewClient(logger) + client := newTestClient(logger) pool := NewPool(client, 5, logger) // More workers than jobs // Create jobs with identifiable URLs @@ -390,7 +388,7 @@ func TestPoolWithChecksumValidation(t *testing.T) { tmpDir := t.TempDir() logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - client := NewClient(logger) + client := newTestClient(logger) pool := NewPool(client, 2, logger) // Calculate correct checksum @@ -441,7 +439,7 @@ func TestPoolSingleWorker(t *testing.T) { tmpDir := t.TempDir() logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - client := NewClient(logger) + client := newTestClient(logger) pool := NewPool(client, 1, logger) jobs := make([]Job, 3) From f5abf66c15c2cf4b9f9d79747c1770bc9e3120c1 Mon Sep 17 00:00:00 2001 From: BadgerOps Date: Tue, 24 Feb 2026 20:31:54 -0600 Subject: [PATCH 2/2] chore: Run go fmt --- internal/engine/progress.go | 30 +++++++++++++++--------------- internal/mirror/epel.go | 4 ++-- internal/server/templates.go | 4 ++-- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/internal/engine/progress.go b/internal/engine/progress.go index 20542f6..2e877c6 100644 --- a/internal/engine/progress.go +++ b/internal/engine/progress.go @@ -27,23 +27,23 @@ type FileEvent struct { // SyncProgress is a snapshot of the current sync state, safe for JSON serialization. type SyncProgress struct { - Provider string `json:"provider"` - Phase SyncPhase `json:"phase"` - TotalFiles int `json:"total_files"` - CompletedFiles int `json:"completed_files"` - FailedFiles int `json:"failed_files"` - SkippedFiles int `json:"skipped_files"` - TotalBytes int64 `json:"total_bytes"` - BytesDownloaded int64 `json:"bytes_downloaded"` - Percent float64 `json:"percent"` + Provider string `json:"provider"` + Phase SyncPhase `json:"phase"` + TotalFiles int `json:"total_files"` + CompletedFiles int `json:"completed_files"` + FailedFiles int `json:"failed_files"` + SkippedFiles int `json:"skipped_files"` + TotalBytes int64 `json:"total_bytes"` + BytesDownloaded int64 `json:"bytes_downloaded"` + Percent float64 `json:"percent"` CurrentFiles []FileProgress `json:"current_files,omitempty"` RecentEvents []FileEvent `json:"recent_events,omitempty"` - TotalRetries int `json:"total_retries"` - BytesPerSecond int64 `json:"bytes_per_second"` - ETA string `json:"eta,omitempty"` - StartTime time.Time `json:"start_time"` - Elapsed string `json:"elapsed"` - Message string `json:"message,omitempty"` + TotalRetries int `json:"total_retries"` + BytesPerSecond int64 `json:"bytes_per_second"` + ETA string `json:"eta,omitempty"` + StartTime time.Time `json:"start_time"` + Elapsed string `json:"elapsed"` + Message string `json:"message,omitempty"` } // FileProgress tracks the download state of an individual file. diff --git a/internal/mirror/epel.go b/internal/mirror/epel.go index f42ac4f..8206393 100644 --- a/internal/mirror/epel.go +++ b/internal/mirror/epel.go @@ -14,7 +14,7 @@ var EPELArchitectures = []string{"x86_64", "aarch64", "ppc64le", "s390x"} // metalinkXML structs model the Metalink 3.0 XML format. type metalinkXML struct { - XMLName xml.Name `xml:"metalink"` + XMLName xml.Name `xml:"metalink"` Files metalinkFilesXML `xml:"files"` } @@ -23,7 +23,7 @@ type metalinkFilesXML struct { } type metalinkFileXML struct { - Name string `xml:"name,attr"` + Name string `xml:"name,attr"` Resources metalinkResourcesXML `xml:"resources"` } diff --git a/internal/server/templates.go b/internal/server/templates.go index d2149fb..88e4785 100644 --- a/internal/server/templates.go +++ b/internal/server/templates.go @@ -9,8 +9,8 @@ import ( // initializeTemplateFuncs sets up custom template functions. func initializeTemplateFuncs() template.FuncMap { return template.FuncMap{ - "formatBytes": formatBytes, - "formatTime": formatTime, + "formatBytes": formatBytes, + "formatTime": formatTime, "formatDuration": formatDuration, } }