From 24529d8fa7ba0e954ff81b48202ed9c69424673f Mon Sep 17 00:00:00 2001
From: Steve Cliff <steve@devcloud.guru>
Date: Mon, 4 May 2026 18:01:35 +0100
Subject: [PATCH] test: lock-protect fakeSender so -race CI passes

The CI runs go test with -race; the agent runner has two pump goroutines
(pumpStdout + pumpStderr) writing through the sender concurrently, and
the unprotected fakeSender slice append raced. The cancel_test had a
local 'safeSender' workaround for the same issue; promote that mutex
onto fakeSender itself so every test in the package is race-clean
without per-test variants.

- fakeSender grows mu sync.Mutex; Send takes/releases. New snapshot()
  helper for tests that want a stable copy.
- cancel_test drops its local safeSender + sync import; uses fakeSender.

Verified: go test -race ./... passes across all packages.
---
 internal/agent/runner/cancel_test.go | 29 +++++-----------------------
 internal/agent/runner/runner_test.go | 26 +++++++++++++++++++++++--
 2 files changed, 29 insertions(+), 26 deletions(-)

diff --git a/internal/agent/runner/cancel_test.go b/internal/agent/runner/cancel_test.go
index cacadd2..2df7e74 100644
--- a/internal/agent/runner/cancel_test.go
+++ b/internal/agent/runner/cancel_test.go
@@ -3,35 +3,16 @@ package runner
 import (
 	"context"
 	"strings"
-	"sync"
 	"testing"
 	"time"
 
 	"gitea.dcglab.co.uk/steve/restic-manager/internal/api"
 )
 
-// safeSender is a thread-safe variant of fakeSender. The cancel test
-// has the runner goroutine sending envelopes while the test goroutine
-// is reading the slice, so we need a mutex.
-type safeSender struct {
-	mu   sync.Mutex
-	envs []api.Envelope
-}
-
-func (s *safeSender) Send(e api.Envelope) error {
-	s.mu.Lock()
-	s.envs = append(s.envs, e)
-	s.mu.Unlock()
-	return nil
-}
-
-func (s *safeSender) snapshot() []api.Envelope {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-	out := make([]api.Envelope, len(s.envs))
-	copy(out, s.envs)
-	return out
-}
+// (fakeSender is defined in runner_test.go; it's already lock-protected
+// because the runner's stdout + stderr pump goroutines call Send
+// concurrently. The original local 'safeSender' here was a workaround
+// from before fakeSender itself grew the mutex.)
 
 // TestRunBackupCanceledMidRunReportsCanceled spawns a backup against
 // a fake restic that sleeps for 30 seconds, cancels the context after
@@ -48,7 +29,7 @@ func TestRunBackupCanceledMidRunReportsCanceled(t *testing.T) {
 	// SIGKILL fallback path firing — slower and noisier.
 	bin := setupScript(t, `exec sleep 30`)
 
-	tx := &safeSender{}
+	tx := &fakeSender{}
 	r := New(Config{ResticBin: bin}, tx, 0)
 
 	ctx, cancel := context.WithCancel(context.Background())
diff --git a/internal/agent/runner/runner_test.go b/internal/agent/runner/runner_test.go
index 77ec8e8..239cdf7 100644
--- a/internal/agent/runner/runner_test.go
+++ b/internal/agent/runner/runner_test.go
@@ -4,20 +4,42 @@ import (
 	"context"
 	"os"
 	"path/filepath"
+	"sync"
 	"testing"
 
 	"gitea.dcglab.co.uk/steve/restic-manager/internal/api"
 	"gitea.dcglab.co.uk/steve/restic-manager/internal/restic"
 )
 
-// fakeSender collects sent envelopes for assertions.
-type fakeSender struct{ envs []api.Envelope }
+// fakeSender collects sent envelopes for assertions. Lock-protected
+// because the runner's pumpStdout / pumpStderr goroutines call Send
+// concurrently — without the mutex, -race in CI flags every test
+// that exercises a Run* method with both pumps active.
+type fakeSender struct {
+	mu   sync.Mutex
+	envs []api.Envelope
+}
 
 func (s *fakeSender) Send(e api.Envelope) error {
+	s.mu.Lock()
 	s.envs = append(s.envs, e)
+	s.mu.Unlock()
 	return nil
 }
 
+// snapshot returns a copy of the captured envelopes safe to read
+// without holding the lock. Tests use this when iterating envs while
+// other goroutines may still be writing — though in practice all
+// runner Run* methods join their pumps before returning, so callers
+// can also read .envs directly post-return.
+func (s *fakeSender) snapshot() []api.Envelope {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	out := make([]api.Envelope, len(s.envs))
+	copy(out, s.envs)
+	return out
+}
+
 // setupScript writes a shell script (without shebang) to a temp dir,
 // names it "restic", makes it executable, and returns the path.
 //