From 68e557e9f05eccf06287181cd5f70836fb52aedf Mon Sep 17 00:00:00 2001 From: "courtmanr@gmail.com" Date: Tue, 28 Jul 2026 17:16:01 +0100 Subject: [PATCH] Fix Docker agent test JSON-marshal race by making the hook per-Agent Same class of race fixed for newTimerFn in 8e5ef365d: tests swapped the package-level jsonMarshalFn hook while async goroutines leaked from earlier tests (sendCommandAck ack retries via runAsync) could still be reading it, tripping the race detector. Replace the global with a per-Agent jsonMarshalFn seam (nil defaults to json.Marshal), make the decode payload helpers Agent methods so they use it, and inject the failing marshaller into the tests that previously swapped the global. Verified with go test ./internal/dockeragent/ -race -count=20. Contract-Neutral: test seam refactor to fix data race, no public contract delta Co-Authored-By: Claude Fable 5 --- internal/dockeragent/agent.go | 10 ++++++- internal/dockeragent/agent_http_test.go | 11 ++++---- internal/dockeragent/container_update.go | 6 ++-- internal/dockeragent/deps.go | 2 -- internal/dockeragent/update_all.go | 6 ++-- .../update_all_decode_branchcov0723am_test.go | 28 +++++++++---------- 6 files changed, 35 insertions(+), 28 deletions(-) diff --git a/internal/dockeragent/agent.go b/internal/dockeragent/agent.go index fa8329bbb..fe2087331 100644 --- a/internal/dockeragent/agent.go +++ b/internal/dockeragent/agent.go @@ -135,6 +135,7 @@ type Agent struct { manualCheckResults map[string]manualUpdateCheckResult manualCheckCollect func(context.Context) (agentsdocker.Report, error) // test seam for bounded manual checks newTimerFn func(time.Duration) *time.Timer // test seam; per-Agent so async goroutines never read a shared global (nil = time.NewTimer) + jsonMarshalFn func(any) ([]byte, error) // test seam; per-Agent so async goroutines never read a shared global (nil = json.Marshal) backgroundMu sync.Mutex // protects updateCheckRunning, cleanupTaskRunning updateCheckRunning bool cleanupTaskRunning bool @@ -819,6 +820,13 @@ func (a *Agent) newTimer(delay time.Duration) *time.Timer { return time.NewTimer(delay) } +func (a *Agent) jsonMarshal(v any) ([]byte, error) { + if a.jsonMarshalFn != nil { + return a.jsonMarshalFn(v) + } + return json.Marshal(v) +} + func (a *Agent) waitForAsyncDelay(delay time.Duration) bool { if delay <= 0 { return true @@ -1524,7 +1532,7 @@ func (a *Agent) sendCommandAckWithPayload(ctx context.Context, target TargetConf Payload: payload, } - body, err := jsonMarshalFn(ackPayload) + body, err := a.jsonMarshal(ackPayload) if err != nil { return fmt.Errorf("marshal command acknowledgement: %w", err) } diff --git a/internal/dockeragent/agent_http_test.go b/internal/dockeragent/agent_http_test.go index 76e2c545a..c16984ebc 100644 --- a/internal/dockeragent/agent_http_test.go +++ b/internal/dockeragent/agent_http_test.go @@ -364,11 +364,12 @@ func TestSendCommandAck(t *testing.T) { }) t.Run("marshal error", func(t *testing.T) { - swap(t, &jsonMarshalFn, func(any) ([]byte, error) { - return nil, errors.New("marshal failed") - }) - - agent := &Agent{hostID: "host1"} + agent := &Agent{ + hostID: "host1", + jsonMarshalFn: func(any) ([]byte, error) { + return nil, errors.New("marshal failed") + }, + } if err := agent.sendCommandAck(context.Background(), TargetConfig{URL: "http://example"}, "cmd", "status", "msg"); err == nil { t.Fatal("expected error") } diff --git a/internal/dockeragent/container_update.go b/internal/dockeragent/container_update.go index aa4b27e73..0dd94897d 100644 --- a/internal/dockeragent/container_update.go +++ b/internal/dockeragent/container_update.go @@ -38,10 +38,10 @@ type updateContainerCommandPayload struct { ContainerID string `json:"containerId"` } -func decodeUpdateContainerPayload(payload map[string]any) (updateContainerCommandPayload, error) { +func (a *Agent) decodeUpdateContainerPayload(payload map[string]any) (updateContainerCommandPayload, error) { var commandPayload updateContainerCommandPayload - body, err := jsonMarshalFn(payload) + body, err := a.jsonMarshal(payload) if err != nil { return commandPayload, fmt.Errorf("marshal update command payload: %w", err) } @@ -189,7 +189,7 @@ func daemonGeneratedHostname(hostname, containerID string) bool { // handleUpdateContainerCommand handles the update_container command from Pulse. func (a *Agent) handleUpdateContainerCommand(ctx context.Context, target TargetConfig, command agentsdocker.Command) error { - commandPayload, err := decodeUpdateContainerPayload(command.Payload) + commandPayload, err := a.decodeUpdateContainerPayload(command.Payload) if err != nil { a.logger.Error().Err(err).Msg("Update command missing or invalid containerId in payload") if err := a.sendCommandAck(ctx, target, command.ID, agentsdocker.CommandStatusFailed, "Missing containerId in payload"); err != nil { diff --git a/internal/dockeragent/deps.go b/internal/dockeragent/deps.go index f2277d8b8..4da1f391a 100644 --- a/internal/dockeragent/deps.go +++ b/internal/dockeragent/deps.go @@ -3,7 +3,6 @@ package dockeragent import ( "context" "crypto/rand" - "encoding/json" "io" "os" "os/exec" @@ -22,7 +21,6 @@ var ( randomDurationFn = randomDuration nowFn = time.Now sleepFn = time.Sleep - jsonMarshalFn = json.Marshal normalizeTargetsFn = normalizeTargets buildRuntimeCandidatesFn = buildRuntimeCandidates tryRuntimeCandidateFn = tryRuntimeCandidate diff --git a/internal/dockeragent/update_all.go b/internal/dockeragent/update_all.go index cf695aa6a..6989821b3 100644 --- a/internal/dockeragent/update_all.go +++ b/internal/dockeragent/update_all.go @@ -14,10 +14,10 @@ type updateAllCommandPayload struct { ContainerIDs []string `json:"containerIds"` } -func decodeUpdateAllPayload(payload map[string]any) (updateAllCommandPayload, error) { +func (a *Agent) decodeUpdateAllPayload(payload map[string]any) (updateAllCommandPayload, error) { var commandPayload updateAllCommandPayload - body, err := jsonMarshalFn(payload) + body, err := a.jsonMarshal(payload) if err != nil { return commandPayload, fmt.Errorf("marshal update_all command payload: %w", err) } @@ -54,7 +54,7 @@ func decodeUpdateAllPayload(payload map[string]any) (updateAllCommandPayload, er // handleUpdateAllCommand handles the update_all command from Pulse. // It updates each container sequentially to avoid overloading the runtime and registry. func (a *Agent) handleUpdateAllCommand(ctx context.Context, target TargetConfig, command agentsdocker.Command) error { - commandPayload, err := decodeUpdateAllPayload(command.Payload) + commandPayload, err := a.decodeUpdateAllPayload(command.Payload) if err != nil { a.logger.Error().Err(err).Msg("Update-all command missing or invalid containerIds in payload") if err := a.sendCommandAck(ctx, target, command.ID, agentsdocker.CommandStatusFailed, "Missing containerIds in payload"); err != nil { diff --git a/internal/dockeragent/update_all_decode_branchcov0723am_test.go b/internal/dockeragent/update_all_decode_branchcov0723am_test.go index 523ae9437..5533b97cb 100644 --- a/internal/dockeragent/update_all_decode_branchcov0723am_test.go +++ b/internal/dockeragent/update_all_decode_branchcov0723am_test.go @@ -11,7 +11,7 @@ import ( func TestBranchcov0723AmDecodeUpdateAllPayload(t *testing.T) { t.Run("nil map yields missing-containerIds error and zero-value struct", func(t *testing.T) { - got, err := decodeUpdateAllPayload(nil) + got, err := (&Agent{}).decodeUpdateAllPayload(nil) if err == nil { t.Fatal("expected error for nil payload") } @@ -24,7 +24,7 @@ func TestBranchcov0723AmDecodeUpdateAllPayload(t *testing.T) { }) t.Run("empty map yields missing-containerIds error", func(t *testing.T) { - got, err := decodeUpdateAllPayload(map[string]any{}) + got, err := (&Agent{}).decodeUpdateAllPayload(map[string]any{}) if err == nil { t.Fatal("expected error for empty payload") } @@ -42,7 +42,7 @@ func TestBranchcov0723AmDecodeUpdateAllPayload(t *testing.T) { // defaulting and reports "missing containerIds" for both. The // returned structs do differ (nil vs a non-nil empty slice), which // simply reflects what encoding/json left on the field. - absent, errAbsent := decodeUpdateAllPayload(map[string]any{"other": "x"}) + absent, errAbsent := (&Agent{}).decodeUpdateAllPayload(map[string]any{"other": "x"}) if errAbsent == nil { t.Fatal("expected error when containerIds is absent") } @@ -53,7 +53,7 @@ func TestBranchcov0723AmDecodeUpdateAllPayload(t *testing.T) { t.Fatalf("absent field should leave ContainerIDs nil, got %+v", absent.ContainerIDs) } - emptySlice, errEmpty := decodeUpdateAllPayload(map[string]any{"containerIds": []any{}}) + emptySlice, errEmpty := (&Agent{}).decodeUpdateAllPayload(map[string]any{"containerIds": []any{}}) if errEmpty == nil { t.Fatal("expected error for empty containerIds slice") } @@ -72,7 +72,7 @@ func TestBranchcov0723AmDecodeUpdateAllPayload(t *testing.T) { // struct still carries the RAW decoded slice (the `= normalized` // assignment only runs on success). raw := []any{"", " ", "\t"} - got, err := decodeUpdateAllPayload(map[string]any{"containerIds": raw}) + got, err := (&Agent{}).decodeUpdateAllPayload(map[string]any{"containerIds": raw}) if err == nil { t.Fatal("expected error") } @@ -86,11 +86,11 @@ func TestBranchcov0723AmDecodeUpdateAllPayload(t *testing.T) { t.Run("marshal error is wrapped", func(t *testing.T) { marshalErr := errors.New("boom") - swap(t, &jsonMarshalFn, func(any) ([]byte, error) { + agent := &Agent{jsonMarshalFn: func(any) ([]byte, error) { return nil, marshalErr - }) + }} - got, err := decodeUpdateAllPayload(map[string]any{"containerIds": []any{"a"}}) + got, err := agent.decodeUpdateAllPayload(map[string]any{"containerIds": []any{"a"}}) if err == nil { t.Fatal("expected error") } @@ -106,7 +106,7 @@ func TestBranchcov0723AmDecodeUpdateAllPayload(t *testing.T) { }) t.Run("wrong field type yields decode error", func(t *testing.T) { - got, err := decodeUpdateAllPayload(map[string]any{"containerIds": 123}) + got, err := (&Agent{}).decodeUpdateAllPayload(map[string]any{"containerIds": 123}) if err == nil { t.Fatal("expected error") } @@ -122,7 +122,7 @@ func TestBranchcov0723AmDecodeUpdateAllPayload(t *testing.T) { }) t.Run("valid single id", func(t *testing.T) { - got, err := decodeUpdateAllPayload(map[string]any{"containerIds": []any{"a"}}) + got, err := (&Agent{}).decodeUpdateAllPayload(map[string]any{"containerIds": []any{"a"}}) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -132,7 +132,7 @@ func TestBranchcov0723AmDecodeUpdateAllPayload(t *testing.T) { }) t.Run("multiple ids preserve input order", func(t *testing.T) { - got, err := decodeUpdateAllPayload(map[string]any{"containerIds": []any{"b", "a"}}) + got, err := (&Agent{}).decodeUpdateAllPayload(map[string]any{"containerIds": []any{"b", "a"}}) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -142,7 +142,7 @@ func TestBranchcov0723AmDecodeUpdateAllPayload(t *testing.T) { }) t.Run("surrounding whitespace is trimmed", func(t *testing.T) { - got, err := decodeUpdateAllPayload(map[string]any{"containerIds": []any{" a "}}) + got, err := (&Agent{}).decodeUpdateAllPayload(map[string]any{"containerIds": []any{" a "}}) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -152,7 +152,7 @@ func TestBranchcov0723AmDecodeUpdateAllPayload(t *testing.T) { }) t.Run("exact duplicates are deduped", func(t *testing.T) { - got, err := decodeUpdateAllPayload(map[string]any{"containerIds": []any{"a", "a"}}) + got, err := (&Agent{}).decodeUpdateAllPayload(map[string]any{"containerIds": []any{"a", "a"}}) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -164,7 +164,7 @@ func TestBranchcov0723AmDecodeUpdateAllPayload(t *testing.T) { t.Run("duplicates after trim are deduped", func(t *testing.T) { // Verifies the source orders trim-before-dedupe: " a " collapses to // "a" and is then deduped against the literal "a". - got, err := decodeUpdateAllPayload(map[string]any{"containerIds": []any{" a ", "a"}}) + got, err := (&Agent{}).decodeUpdateAllPayload(map[string]any{"containerIds": []any{" a ", "a"}}) if err != nil { t.Fatalf("unexpected error: %v", err) }