fix(test): TestBoundedFanOut_SkipsAgentRoutedDeployments race on seenIDs slice

CI race detector flagged TestBoundedFanOut_SkipsAgentRoutedDeployments
on commit 35e18bf (audit fix #9). The test's `work` closure was
appending to a plain []string slice from worker goroutines without
synchronisation:

    var seenIDs []string
    work := func(ctx context.Context, job *domain.Job) error {
        seen.Add(1)
        seenIDs = append(seenIDs, job.ID)  // race
        return nil
    }

atomic.Int64 covered the count assertion but the slice header itself
is the racing memory — race detector caught both the read+write race
on the slice header and the runtime.growslice path on append.

Fix: protect seenIDs with a sync.Mutex. The slice is only used in
the failure-message branch (`t.Errorf` ids=%v formatting), so the
contention is irrelevant to performance — correctness only.

Also locked around the read in the t.Errorf format-args evaluation,
since that read happens AFTER boundedFanOut returns (and Wait()
inside boundedFanOut synchronizes the worker goroutines), but the
explicit Lock/Unlock makes the synchronisation visible without
depending on the implicit happens-before from Wait.

The other five tests in the file (TestBoundedFanOut_CapHolds,
_AllJobsRun, _CtxCancelInterrupts, _FailedJobsCounted,
TestSetRenewalConcurrency_NormalizesNonPositive) only mutate
atomic.Int64 counters from worker goroutines, so they were
already race-clean.

Verified locally: go test -race -count=1 -run
'TestBoundedFanOut|TestSetRenewalConcurrency' ./internal/service/...
green.
This commit is contained in:
shankar0123
2026-05-02 14:34:48 +00:00
parent a22a1be962
commit 475421457f
+13 -1
View File
@@ -16,6 +16,7 @@ import (
"io"
"log/slog"
"strconv"
"sync"
"sync/atomic"
"testing"
"time"
@@ -135,10 +136,19 @@ func TestBoundedFanOut_SkipsAgentRoutedDeployments(t *testing.T) {
}
var seen atomic.Int64
var seenIDs []string
// seenIDs is appended from multiple worker goroutines; the
// surrounding mutex serialises the slice mutation so the
// race detector stays clean. (atomic.Int64 alone isn't
// sufficient: the slice header is the racing memory.)
var (
mu sync.Mutex
seenIDs []string
)
work := func(ctx context.Context, job *domain.Job) error {
seen.Add(1)
mu.Lock()
seenIDs = append(seenIDs, job.ID)
mu.Unlock()
return nil
}
@@ -147,7 +157,9 @@ func TestBoundedFanOut_SkipsAgentRoutedDeployments(t *testing.T) {
}
if got := seen.Load(); got != 2 {
mu.Lock()
t.Errorf("expected 2 jobs to run (renewal + issuance, deployment-with-agent skipped), got %d (ids=%v)", got, seenIDs)
mu.Unlock()
}
}