fix: improve error handling, url redaction and documentation

AGENTS.md

@@ -15,7 +15,7 @@ and exports Prometheus metrics for monitoring backup status and health.
 - **Logging**: `log/slog` (stdlib, text handler, defaults to stderr)
 - **Log rotation**: `gopkg.in/natefinch/lumberjack.v2`
 - **Testing**: `testing` + `github.com/stretchr/testify/assert`
-- **Build**: GoReleaser, CGO_ENABLED=0, Linux only (amd64/arm64)
+- **Build**: GoReleaser, CGO_ENABLED=0, Linux only (amd64/arm/arm64)
 - Avoid `github.com/sirupsen/logrus` (blocked by depguard linter)
 
 ## Project Structure
@@ -39,9 +39,14 @@ and exports Prometheus metrics for monitoring backup status and health.
 
 ### Configuration (config/config.go)
 - CLI flag parsing: `-c/--config` (required), `-p/--port` (default 9289), `-V/--version`
-- YAML configuration parsing using Koanf
 - Support for multiple backup definitions with URL, username, password, outputFile
 - Logging configuration per logger/LogOptions
+- Use Koanf for all YAML parsing
+- Use spf13/pflag for CLI flag parsing
+- Provide sensible defaults for all config options
+- Validate configuration values at startup
+- Support both short and long CLI flags
+- With Koanf, prefer getting typed values than using the Get() method.
 
 ### Main Application (main.go)
 - Prometheus metrics registry for backup status tracking
@@ -82,14 +87,6 @@ and exports Prometheus metrics for monitoring backup status and health.
 - Logger: singleton via `logger.Get()`.
 - Context: pass `context.Context` to operations that may need cancellation.
 
-### Configuration Management
-- Use Koanf for all YAML parsing
-- Use spf13/pflag for CLI flag parsing
-- Provide sensible defaults for all config options
-- Validate configuration values at startup
-- Support both short and long CLI flags
-- With Koanf, prefer getting typed values than using the Get() method.
-
 ### Environment Variables
 - Use optional environment variables for configuration (e.g., `BRF_BACKUP_DIR`...)
 - Environment variables should override values from the config file
@@ -102,6 +99,7 @@ and exports Prometheus metrics for monitoring backup status and health.
 - Register metrics immediately after creation
 - Assign labels consistently (e.g., "id" for backup identifiers)
 - Document what each metric represents in the Help text
+- `backup_nb` counts every retrieval round (full runs **and** retry batches), not individual files or successful backups
 
 ### Error Handling
 - Use slog for error logging with context
@@ -125,6 +123,17 @@ and exports Prometheus metrics for monitoring backup status and health.
 - Test data files must be placed in the `testdata/` directory
 - Unused testdata files must be removed
 
+## HTTP Client Design
+
+The application creates a new HTTP client per request in `main.go`. This design is intentional and well-suited for this workload because:
+
+1. **Periodic backups**: Backups typically run every 24 hours or so, so connection pooling benefits are minimal
+2. **Different hosts**: Each backup may target a different host, making a per-host pool impractical
+3. **Isolation**: Fresh clients provide clean state per backup operation, avoiding resource leaks
+4. **Simplicity**: Simpler code without complex connection management for edge cases
+
+This approach avoids the complexity of maintaining connection pools while still being performant for the usage pattern.
+
 ## Common Tasks
 
 ### Adding a New Configuration Option

config/config.go

@@ -271,7 +271,7 @@ func (c *Config) readBackups(k *koanf.Koanf, l *logger.Logger) error {
 			return err
 		}
 		c.Backups = append(c.Backups, backup)
-		l.Info("Config: backup url", slog.String("url", backup.URL))
+		l.Info("Config: backup url", slog.String("url", logger.RedactURL(backup.URL)))
 	}
 	return nil
 }

logger/logger.go

@@ -9,6 +9,7 @@ import (
 	"io"
 	"log"
 	"log/slog"
+	"net/url"
 	"os"
 	"strings"
 	"sync"
@@ -166,3 +167,14 @@ func FromCtx(ctx context.Context) *Logger {
 func WithCtx(ctx context.Context, l *Logger) context.Context {
 	return context.WithValue(ctx, ctxKey{}, l)
 }
+
+// RedactURL returns rawURL with any userinfo (username/password) stripped,
+// so it can be logged without leaking credentials.
+func RedactURL(rawURL string) string {
+	u, err := url.Parse(rawURL)
+	if err != nil {
+		return rawURL
+	}
+	u.User = nil
+	return u.String()
+}

logger/logger_test.go

@@ -223,3 +223,32 @@ func TestGet_RaceCondition(t *testing.T) {
 	}
 	wg.Wait()
 }
+
+func TestRedactURL(t *testing.T) {
+	tests := []struct {
+		name   string
+		rawURL string
+		want   string
+	}{
+		{ //nolint:gosec
+			name:   "strips credentials",
+			rawURL: "https://user:secret@example.com/path",
+			want:   "https://example.com/path", //nolint:goconst
+		},
+		{
+			name:   "no credentials unchanged",
+			rawURL: "https://example.com/path",
+			want:   "https://example.com/path",
+		},
+		{
+			name:   "unparseable returned as-is",
+			rawURL: "://not a url",
+			want:   "://not a url",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, RedactURL(tt.rawURL))
+		})
+	}
+}

main.go

@@ -14,6 +14,7 @@ import (
 	"net"
 	"net/http"
 	"os"
+	"os/signal"
 	"runtime"
 	"strconv"
 	"strings"
@@ -87,7 +88,7 @@ func NewMetrics(reg prometheus.Registerer, namespace string) *metrics {
 			prometheus.CounterOpts{
 				Namespace: namespace,
 				Name:      "backup_nb",
-				Help:      "Number of retrievals",
+				Help:      "Number of retrieval rounds, including retries",
 			},
 		),
 	}
@@ -129,6 +130,19 @@ type fsError struct{ error }
 
 func (e *fsError) Unwrap() error { return e.error }
 
+// newClient builds an HTTP client for a single request. It is a variable so
+// tests can substitute a fake transport.
+var newClient = func(timeout time.Duration) *http.Client {
+	return &http.Client{
+		Timeout: timeout,
+		Transport: &http.Transport{
+			DialContext:           (&net.Dialer{Timeout: timeout}).DialContext,
+			TLSHandshakeTimeout:   timeout,
+			ResponseHeaderTimeout: timeout,
+		},
+	}
+}
+
 func fetchURL(
 	ctx context.Context,
 	url, username, password string,
@@ -143,14 +157,7 @@ func fetchURL(
 	// Create a new client per request — these are periodic backups
 	// (typically every 24h), so connection pooling is not beneficial
 	// and each backup may target a different host.
-	client := &http.Client{
-		Timeout: timeout,
-		Transport: &http.Transport{
-			DialContext:           (&net.Dialer{Timeout: timeout}).DialContext,
-			TLSHandshakeTimeout:   timeout,
-			ResponseHeaderTimeout: timeout,
-		},
-	}
+	client := newClient(timeout)
 
 	resp, err := client.Do(req)
 	if err != nil {
@@ -159,25 +166,28 @@ func fetchURL(
 
 	if resp.StatusCode >= http.StatusMultipleChoices {
 		resp.Body.Close()
-		return nil, &httpError{error: errors.New("HTTP status >= 300")}
+		return nil, &httpError{error: fmt.Errorf("unexpected status %d %s", resp.StatusCode, resp.Status)}
 	}
 
 	return resp, nil
 }
 
 func saveToFile(r io.Reader, outputFile string) (int64, error) {
-	f, err := os.Create(outputFile + ".part")
+	partFile := outputFile + ".part"
+	f, err := os.Create(partFile)
 	if err != nil {
 		return 0, &fsError{error: err}
 	}
 	defer f.Close()
 
 	written, err := io.Copy(f, r)
 	if err != nil {
+		_ = os.Remove(partFile)
 		return 0, &fsError{error: err}
 	}
 
-	if err := os.Rename(outputFile+".part", outputFile); err != nil {
+	if err := os.Rename(partFile, outputFile); err != nil {
+		_ = os.Remove(partFile)
 		return 0, &fsError{error: err}
 	}
 
@@ -195,7 +205,7 @@ func backupFile(
 	if err != nil {
 		l.Error("Failed to fetch URL",
 			slog.String("id", id),
-			slog.String("url", url),
+			slog.String("url", logger.RedactURL(url)),
 			slog.Any("error", err),
 		)
 		return 0, err
@@ -367,8 +377,11 @@ func run(ctx context.Context) error {
 }
 
 func main() {
-	if err := run(context.Background()); err != nil {
-		if !errors.Is(err, flag.ErrHelp) {
+	ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt)
+
+	if err := run(ctx); err != nil {
+		cancel()
+		if !errors.Is(err, flag.ErrHelp) && !errors.Is(err, context.Canceled) && !errors.Is(err, context.DeadlineExceeded) {
 			fmt.Fprintf(os.Stderr, "error: %v\n", err)
 			os.Exit(1)
 		}

main_test.go

@@ -76,9 +76,7 @@ func TestRetrieveUrls_TargetIsDirectory(t *testing.T) {
 
 	status := newBackupStatus(cfg.Backups)
 	r := retrieveUrls(t.Context(), cfg, m, status, true)
-	if !assert.FileExists(t, outputFilename+".part") {
-		return
-	}
+	assert.NoFileExists(t, outputFilename+".part")
 
 	assert.True(t, r)
 }
@@ -775,3 +773,34 @@ func TestRunBackupLoop_Retry(t *testing.T) {
 		t.Fatal("run() did not return after cancellation")
 	}
 }
+
+// roundTripFunc adapts a function to an http.RoundTripper for tests.
+type roundTripFunc func(*http.Request) (*http.Response, error)
+
+func (f roundTripFunc) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) }
+
+// failCloseBody is an io.ReadCloser whose Close always returns an error,
+// used to exercise the body-close error path in backupFile.
+type failCloseBody struct{}
+
+func (failCloseBody) Read(_ []byte) (int, error) { return 0, io.EOF }
+
+func (failCloseBody) Close() error { return errors.New("close failed") }
+
+func TestBackupFile_BodyCloseError(t *testing.T) {
+	testutil.UseTempDir(t)
+
+	origNewClient := newClient
+	newClient = func(time.Duration) *http.Client {
+		return &http.Client{
+			Transport: roundTripFunc(func(_ *http.Request) (*http.Response, error) {
+				return &http.Response{StatusCode: http.StatusOK, Body: failCloseBody{}}, nil
+			}),
+		}
+	}
+	t.Cleanup(func() { newClient = origNewClient })
+
+	_, err := backupFile(t.Context(), "test", "http://example.com", "", "", "test.out", 10*time.Second)
+	assert.NoError(t, err)
+	assert.FileExists(t, "test.out")
+}

testutil/testutil.go

@@ -8,19 +8,35 @@ import (
 	"testing"
 )
 
-func UseTempDir(t *testing.T) {
+var (
+	getwd = os.Getwd
+	chdir = os.Chdir
+)
+
+// UseTempDir changes the working directory to a fresh temporary directory for
+// the duration of the test, restoring the original directory on cleanup.
+func UseTempDir(t testing.TB) {
 	t.Helper()
+	if err := useTempDir(t); err != nil {
+		t.Fatal(err)
+	}
+}
+
+// useTempDir performs the directory switch and returns an error instead of
+// aborting, so its failure paths can be tested without failing the test.
+func useTempDir(t testing.TB) error {
 	dir := t.TempDir()
-	oldDir, err := os.Getwd()
+	oldDir, err := getwd()
 	if err != nil {
-		t.Fatal(err)
+		return err
 	}
-	if err := os.Chdir(dir); err != nil {
-		t.Fatal(err)
+	if err := chdir(dir); err != nil {
+		return err
 	}
 	t.Cleanup(func() {
-		if err := os.Chdir(oldDir); err != nil {
+		if err := chdir(oldDir); err != nil {
 			t.Logf("failed to restore working directory: %v", err)
 		}
 	})
+	return nil
 }

testutil/testutil_test.go

@@ -4,6 +4,7 @@
 package testutil
 
 import (
+	"errors"
 	"os"
 	"testing"
 )
@@ -67,3 +68,62 @@ func TestUseTempDir_CleanupLog(t *testing.T) {
 		t.Errorf("working directory was not restored: got %q, want %q", restoredDir, originalDir)
 	}
 }
+
+func TestUseTempDir_ErrorGetwd(t *testing.T) {
+	origGetwd := getwd
+	getwd = func() (string, error) { return "", errors.New("getwd failed") }
+	t.Cleanup(func() { getwd = origGetwd })
+
+	if err := useTempDir(t); err == nil {
+		t.Error("expected error when getwd fails")
+	}
+}
+
+func TestUseTempDir_ErrorChdir(t *testing.T) {
+	origChdir := chdir
+	chdir = func(string) error { return errors.New("chdir failed") }
+	t.Cleanup(func() { chdir = origChdir })
+
+	if err := useTempDir(t); err == nil {
+		t.Error("expected error when chdir fails")
+	}
+}
+
+func TestUseTempDir_ErrorRestore(t *testing.T) {
+	origChdir := chdir
+	calls := 0
+	chdir = func(path string) error {
+		calls++
+		err := origChdir(path)
+		if calls > 1 {
+			return errors.New("restore failed")
+		}
+		return err
+	}
+	t.Cleanup(func() { chdir = origChdir })
+
+	UseTempDir(t)
+}
+
+// fakeTB is a testing.TB whose Fatal records the call instead of aborting,
+// so the failure path of UseTempDir can be exercised without failing a test.
+type fakeTB struct {
+	testing.TB
+	fatalCalled bool
+}
+
+func (f *fakeTB) Fatal(_ ...any) {
+	f.fatalCalled = true
+}
+
+func TestUseTempDir_Fatal(t *testing.T) {
+	origGetwd := getwd
+	getwd = func() (string, error) { return "", errors.New("getwd failed") }
+	t.Cleanup(func() { getwd = origGetwd })
+
+	ft := &fakeTB{TB: t}
+	UseTempDir(ft)
+	if !ft.fatalCalled {
+		t.Error("expected UseTempDir to call Fatal when useTempDir returns an error")
+	}
+}