From dba8d214f2e1d5af8b9f4871b27ea766dcadcd58 Mon Sep 17 00:00:00 2001 From: Yves Mettier Date: Mon, 13 Jul 2026 00:14:56 +0200 Subject: [PATCH 01/11] fix: cancel context before exit and add signal handling --- main.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/main.go b/main.go index 379ccf8..0cca02a 100644 --- a/main.go +++ b/main.go @@ -14,6 +14,7 @@ import ( "net" "net/http" "os" + "os/signal" "runtime" "strconv" "strings" @@ -367,8 +368,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) } From f08015417ca025ec9a310414cbdfb7f9055d4342 Mon Sep 17 00:00:00 2001 From: Yves Mettier Date: Mon, 13 Jul 2026 00:22:51 +0200 Subject: [PATCH 02/11] docs: add HTTP client design explanation --- AGENTS.md | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8666f48..826709f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -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 @@ -125,6 +122,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 From 0fb93208ca992be08a89dcb4bcba0338e429dfd0 Mon Sep 17 00:00:00 2001 From: Yves Mettier Date: Mon, 13 Jul 2026 00:33:24 +0200 Subject: [PATCH 03/11] fix: include http status code in fetch error --- main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main.go b/main.go index 0cca02a..5dc9590 100644 --- a/main.go +++ b/main.go @@ -160,7 +160,7 @@ 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 From 3d5080c62b8759ea2d5c8ea90db127efa071d37f Mon Sep 17 00:00:00 2001 From: Yves Mettier Date: Mon, 13 Jul 2026 00:36:44 +0200 Subject: [PATCH 04/11] fix: clean up stale .part file on backup write failure --- main.go | 7 +++++-- main_test.go | 4 +--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/main.go b/main.go index 5dc9590..0a50538 100644 --- a/main.go +++ b/main.go @@ -167,7 +167,8 @@ func fetchURL( } 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} } @@ -175,10 +176,12 @@ func saveToFile(r io.Reader, outputFile string) (int64, error) { 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} } diff --git a/main_test.go b/main_test.go index a949867..0ccd909 100644 --- a/main_test.go +++ b/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) } From 58e1e8fbc46d69f0cb7f37a025e5011fa43e97bc Mon Sep 17 00:00:00 2001 From: Yves Mettier Date: Mon, 13 Jul 2026 00:43:17 +0200 Subject: [PATCH 05/11] fix: redact credentials from logged urls --- config/config.go | 2 +- logger/logger.go | 12 ++++++++++++ main.go | 2 +- 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/config/config.go b/config/config.go index 8730ff4..50b16b3 100644 --- a/config/config.go +++ b/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 } diff --git a/logger/logger.go b/logger/logger.go index 7f90218..a9c7f67 100644 --- a/logger/logger.go +++ b/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() +} diff --git a/main.go b/main.go index 0a50538..4e3ae3f 100644 --- a/main.go +++ b/main.go @@ -199,7 +199,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 From afee0662aa84350143f445277ab0ee839c44b525 Mon Sep 17 00:00:00 2001 From: Yves Mettier Date: Mon, 13 Jul 2026 00:46:10 +0200 Subject: [PATCH 06/11] docs: document arm (32-bit) in supported build platforms --- AGENTS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 826709f..17723cf 100644 --- a/AGENTS.md +++ b/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 From bf5a384427a96fdb89af26f3a0387bdf93b50a93 Mon Sep 17 00:00:00 2001 From: Yves Mettier Date: Mon, 13 Jul 2026 00:48:16 +0200 Subject: [PATCH 07/11] docs: clarify backup_nb counts retrieval rounds including retries --- AGENTS.md | 1 + main.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 17723cf..1490095 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -99,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 diff --git a/main.go b/main.go index 4e3ae3f..7d8b2c6 100644 --- a/main.go +++ b/main.go @@ -88,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", }, ), } From 46893807976a347c2613d640a9ecb2f4e17d7dd7 Mon Sep 17 00:00:00 2001 From: Yves Mettier Date: Mon, 13 Jul 2026 00:55:08 +0200 Subject: [PATCH 08/11] test: add coverage for logger.RedactURL --- logger/logger_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/logger/logger_test.go b/logger/logger_test.go index 385ffe5..c4768b0 100644 --- a/logger/logger_test.go +++ b/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)) + }) + } +} From 9b11134c0b334d236c6062fd3a5ace7be85e2c60 Mon Sep 17 00:00:00 2001 From: Yves Mettier Date: Mon, 13 Jul 2026 01:03:55 +0200 Subject: [PATCH 09/11] test: cover testutil error paths and reach 92% coverage --- testutil/testutil.go | 26 +++++++++++++++++++++----- testutil/testutil_test.go | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 5 deletions(-) diff --git a/testutil/testutil.go b/testutil/testutil.go index 89249b7..1e663f4 100644 --- a/testutil/testutil.go +++ b/testutil/testutil.go @@ -8,19 +8,35 @@ import ( "testing" ) +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.T) { 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 } diff --git a/testutil/testutil_test.go b/testutil/testutil_test.go index 51632bc..b836b0f 100644 --- a/testutil/testutil_test.go +++ b/testutil/testutil_test.go @@ -4,6 +4,7 @@ package testutil import ( + "errors" "os" "testing" ) @@ -67,3 +68,39 @@ 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) +} From bdac697d28f6cd7868d94166efc5587177628445 Mon Sep 17 00:00:00 2001 From: Yves Mettier Date: Mon, 13 Jul 2026 01:07:03 +0200 Subject: [PATCH 10/11] test: reach 100% coverage for testutil with fake tb --- testutil/testutil.go | 2 +- testutil/testutil_test.go | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/testutil/testutil.go b/testutil/testutil.go index 1e663f4..9b9651b 100644 --- a/testutil/testutil.go +++ b/testutil/testutil.go @@ -15,7 +15,7 @@ var ( // 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.T) { +func UseTempDir(t testing.TB) { t.Helper() if err := useTempDir(t); err != nil { t.Fatal(err) diff --git a/testutil/testutil_test.go b/testutil/testutil_test.go index b836b0f..273f47c 100644 --- a/testutil/testutil_test.go +++ b/testutil/testutil_test.go @@ -104,3 +104,26 @@ func TestUseTempDir_ErrorRestore(t *testing.T) { 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") + } +} From f2d91ecb2abbe41ca4489c2bc17707015ea94d9e Mon Sep 17 00:00:00 2001 From: Yves Mettier Date: Mon, 13 Jul 2026 01:16:01 +0200 Subject: [PATCH 11/11] test: cover backupFile body-close error path --- main.go | 22 ++++++++++++++-------- main_test.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 8 deletions(-) diff --git a/main.go b/main.go index 7d8b2c6..d32276f 100644 --- a/main.go +++ b/main.go @@ -130,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, @@ -144,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 { diff --git a/main_test.go b/main_test.go index 0ccd909..8b144e0 100644 --- a/main_test.go +++ b/main_test.go @@ -773,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") +}