From ad97ae890df9ae93bf363e4081b000dd4b6e0e78 Mon Sep 17 00:00:00 2001 From: "courtmanr@gmail.com" Date: Tue, 28 Jul 2026 22:33:55 +0100 Subject: [PATCH] Backfill agent-lifecycle governance for the marshal-hook race fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 68e557e9f moved the Docker agent's JSON-marshal hook from a package global to a per-Agent field, which touches internal/dockeragent/agent.go and internal/dockeragent/container_update.go. It meant to take the Contract-Neutral bypass, but a blank line separated the trailer from Co-Authored-By, so git's trailer parser dropped it, CI ran the completion guard without a reason, and Canonical Governance went red on three counts: missing contract docs/release-control/v6/internal/subsystems/agent-lifecycle.md, missing verification artifact for "agent runtime transport trust proof", and missing verification artifact for "Docker container recreate, rollback, durable result, and live network-mode proof". The sibling timer-hook commit 8e5ef365d has the same defect, so this backfill covers both seams. Document the seams in the agent-lifecycle contract's Current State: both newTimerFn and jsonMarshalFn are unexported Agent fields reached through the newTimer and jsonMarshal methods, nil falls back to time.NewTimer and json.Marshal, no package-level hook global remains in internal/dockeragent, and injection happens at construction only so the fields need no mutex. The seams stay internal — no enrollment, transport trust, command admission, acknowledgement, or recreate/rollback semantics move, and they must not be promoted into NewAgent options or any server-facing surface. Pin that shape in the two registered verification artifacts. agent_internal_test.go asserts the field names, types, and unexported-ness, parses every non-test source in the package to fail if either hook returns as a package-level var, proves the nil defaults run the standard library, drives two Agents with different hooks concurrently so the isolation is checked under -race, and proves sendCommandAck marshals through the receiver's own seam. container_update_test.go proves decodeUpdateContainerPayload routes through a.jsonMarshal, that an injected failure never reaches a sibling Agent, and pins the method form at compile time. Guard dry-run over the staged set passes with no Contract-Neutral bypass, and replaying 68e557e9f's file list plus these three files is green, so the original commit would have passed had it carried them. Co-Authored-By: Claude Fable 5 --- .../v6/internal/subsystems/agent-lifecycle.md | 35 +++ internal/dockeragent/agent_internal_test.go | 214 ++++++++++++++++++ internal/dockeragent/container_update_test.go | 86 +++++++ 3 files changed, 335 insertions(+) diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index f53b42e1ef..be58c0bcde 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -2322,6 +2322,41 @@ manual host-local update command only for a timestamped update attempt inside the bounded in-flight window; a missing timestamp or a stale attempt restores the manual recovery handoff. +### Docker agent test seams are per-Agent fields, never package globals + +The Docker / Podman module owns two indirection seams that exist only so tests +can drive timing and encoding failures: the timer constructor used by +`waitForAsyncDelay` and the JSON marshaller used by command acknowledgement +delivery and by update-command payload decoding. Both are unexported fields on +the `Agent` struct — `newTimerFn func(time.Duration) *time.Timer` and +`jsonMarshalFn func(any) ([]byte, error)` — reached through the `newTimer` and +`jsonMarshal` methods, and both fall back to the standard library +(`time.NewTimer`, `json.Marshal`) when the field is nil. No package-level +`newTimerFn` or `jsonMarshalFn` variable exists in `internal/dockeragent`. + +Ownership is per-Agent because the seams are read from goroutines the agent +itself started. `runAsync` work such as command-acknowledgement retry outlives +the test that scheduled it, so a package global swapped by a later test was a +genuine data race between that swap and the leaked goroutine's read, not merely +a test-isolation smell. Making the seam a field the goroutine's own `Agent` +already carries removes the shared location: an injected hook can only ever be +observed by the Agent it was constructed with, and two Agents running +concurrently with different hooks stay independent. The injection point is +construction only — nothing reassigns either field after an Agent is in use, so +the fields need no mutex and take part in no locking order. + +This is an internal test-seam shape with no public contract delta. Agent +enrollment, transport trust, command admission, acknowledgement semantics, and +the Docker recreate/rollback result contract are all unchanged by it; a +production Agent leaves both fields nil and runs the standard-library +implementations. The seams remain unexported and must not be promoted into +`NewAgent` options, agent profiles, or any server-facing configuration surface. +`internal/dockeragent/agent_internal_test.go` proves the field shape, the nil +defaults, the absence of package-level hook globals, and per-Agent isolation +under the race detector; +`internal/dockeragent/container_update_test.go` proves the update-command +decode path routes through the receiver's own marshaller. + ### vSphere system members carry no agent semantics ESXi host rows that now compose under their owning vCenter connection in the diff --git a/internal/dockeragent/agent_internal_test.go b/internal/dockeragent/agent_internal_test.go index 8e6faeb209..43b51b92c0 100644 --- a/internal/dockeragent/agent_internal_test.go +++ b/internal/dockeragent/agent_internal_test.go @@ -1,11 +1,19 @@ package dockeragent import ( + "context" "errors" + "fmt" + "go/ast" + "go/parser" + "go/token" "io" "math" "math/big" + "path/filepath" "reflect" + "strings" + "sync" "testing" "time" @@ -1331,3 +1339,209 @@ func TestDetectHostRemovedError(t *testing.T) { }) } } + +// TestAgentHookSeamsArePerAgentFields pins the shape of the two indirection +// seams the Docker agent exposes for tests: the timer constructor read by +// waitForAsyncDelay and the JSON marshaller read by acknowledgement delivery +// and update-payload decoding. Both used to be package-level vars in deps.go, +// which raced with async goroutines leaked from earlier tests. They are now +// fields on Agent, injected at construction and never reassigned afterwards. +func TestAgentHookSeamsArePerAgentFields(t *testing.T) { + agentType := reflect.TypeOf(Agent{}) + + for _, tc := range []struct { + field string + want reflect.Type + }{ + {field: "newTimerFn", want: reflect.TypeOf((func(time.Duration) *time.Timer)(nil))}, + {field: "jsonMarshalFn", want: reflect.TypeOf((func(any) ([]byte, error))(nil))}, + } { + field, ok := agentType.FieldByName(tc.field) + if !ok { + t.Fatalf("Agent is missing the %s test seam field; the hook must stay per-Agent, not a package global", tc.field) + } + if field.Type != tc.want { + t.Fatalf("Agent.%s has type %s, want %s", tc.field, field.Type, tc.want) + } + if field.PkgPath == "" { + t.Fatalf("Agent.%s must stay unexported; it is an internal test seam, not a configuration surface", tc.field) + } + } +} + +// TestAgentHookSeamsHaveNoPackageGlobals fails if either seam is reintroduced +// as a package-level var in the non-test sources of internal/dockeragent. A +// package global is reachable from every leaked goroutine in the package, which +// is exactly the race the per-Agent fields removed. +func TestAgentHookSeamsHaveNoPackageGlobals(t *testing.T) { + forbidden := map[string]struct{}{ + "newTimerFn": {}, + "jsonMarshalFn": {}, + } + + sources, err := filepath.Glob("*.go") + if err != nil { + t.Fatalf("glob package sources: %v", err) + } + if len(sources) == 0 { + t.Fatal("no package sources found; the global scan would vacuously pass") + } + + scanned := 0 + for _, source := range sources { + if strings.HasSuffix(source, "_test.go") { + continue + } + scanned++ + + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, source, nil, 0) + if err != nil { + t.Fatalf("parse %s: %v", source, err) + } + + for _, decl := range file.Decls { + genDecl, ok := decl.(*ast.GenDecl) + if !ok || genDecl.Tok != token.VAR { + continue + } + for _, spec := range genDecl.Specs { + valueSpec, ok := spec.(*ast.ValueSpec) + if !ok { + continue + } + for _, name := range valueSpec.Names { + if _, bad := forbidden[name.Name]; bad { + t.Errorf( + "%s declares package-level var %s; the timer and JSON-marshal seams must stay Agent fields so async goroutines never read shared state", + source, name.Name, + ) + } + } + } + } + } + + if scanned == 0 { + t.Fatal("scanned no non-test sources; the global scan would vacuously pass") + } +} + +// TestAgentHookSeamsDefaultToStdlib proves a zero-value Agent — the production +// shape, where nothing injects a hook — runs the standard library behind both +// seams. +func TestAgentHookSeamsDefaultToStdlib(t *testing.T) { + agent := &Agent{} + + body, err := agent.jsonMarshal(map[string]string{"k": "v"}) + if err != nil { + t.Fatalf("nil jsonMarshalFn should fall back to json.Marshal: %v", err) + } + if string(body) != `{"k":"v"}` { + t.Fatalf("nil jsonMarshalFn produced %q, want json.Marshal output", body) + } + + timer := agent.newTimer(time.Millisecond) + if timer == nil { + t.Fatal("nil newTimerFn should fall back to time.NewTimer") + } + defer stopTimer(timer) + select { + case <-timer.C: + case <-time.After(5 * time.Second): + t.Fatal("fallback timer never fired") + } +} + +// TestAgentHookSeamsAreIsolatedPerAgent is the regression proof for the race +// the per-Agent seams fixed. Two Agents constructed with different hooks are +// exercised concurrently; with a package global, one Agent's injection would be +// visible to the other and the swap would race the reads. Run under -race. +func TestAgentHookSeamsAreIsolatedPerAgent(t *testing.T) { + marshalErr := errors.New("injected marshal failure") + + failing := &Agent{ + hostID: "failing", + jsonMarshalFn: func(any) ([]byte, error) { + return nil, marshalErr + }, + newTimerFn: func(time.Duration) *time.Timer { + return time.NewTimer(0) + }, + } + plain := &Agent{hostID: "plain"} + + var wg sync.WaitGroup + errs := make(chan error, 64) + + for range 16 { + wg.Add(2) + + go func() { + defer wg.Done() + if _, err := failing.jsonMarshal(struct{}{}); !errors.Is(err, marshalErr) { + errs <- fmt.Errorf("injected agent should use its own marshaller, got %v", err) + } + }() + + go func() { + defer wg.Done() + body, err := plain.jsonMarshal(map[string]int{"n": 1}) + if err != nil { + errs <- fmt.Errorf("sibling agent must not observe the injected marshaller: %v", err) + return + } + if string(body) != `{"n":1}` { + errs <- fmt.Errorf("sibling agent produced %q, want json.Marshal output", body) + } + }() + } + + wg.Wait() + close(errs) + for err := range errs { + t.Error(err) + } + + // The injected timer fires immediately; the sibling's nil field must still + // resolve to a real time.NewTimer rather than the injected constructor. + injected := failing.newTimer(time.Hour) + defer stopTimer(injected) + select { + case <-injected.C: + case <-time.After(5 * time.Second): + t.Fatal("injected newTimerFn was not used by its own Agent") + } + + sibling := plain.newTimer(time.Hour) + defer stopTimer(sibling) + select { + case <-sibling.C: + t.Fatal("sibling agent observed the injected immediate timer; the seam is not per-Agent") + case <-time.After(50 * time.Millisecond): + } +} + +// TestSendCommandAckUsesReceiverMarshaller proves the acknowledgement transport +// path in agent.go marshals through the receiver's own seam, so an injected +// failure stays scoped to the Agent under test. +func TestSendCommandAckUsesReceiverMarshaller(t *testing.T) { + marshalErr := errors.New("injected ack marshal failure") + agent := &Agent{ + hostID: "host1", + jsonMarshalFn: func(any) ([]byte, error) { + return nil, marshalErr + }, + } + + err := agent.sendCommandAck(context.Background(), TargetConfig{URL: "http://example"}, "cmd", "status", "msg") + if err == nil { + t.Fatal("expected the injected marshaller to fail the acknowledgement") + } + if !errors.Is(err, marshalErr) { + t.Fatalf("acknowledgement error %v does not wrap the injected failure; the ack path is not using a.jsonMarshal", err) + } + if !strings.Contains(err.Error(), "marshal command acknowledgement") { + t.Fatalf("acknowledgement error %v lost its marshal context", err) + } +} diff --git a/internal/dockeragent/container_update_test.go b/internal/dockeragent/container_update_test.go index ae7754550b..5835efa576 100644 --- a/internal/dockeragent/container_update_test.go +++ b/internal/dockeragent/container_update_test.go @@ -748,3 +748,89 @@ func TestUpdateContainer_SharedNamespaceCreateConfig(t *testing.T) { } }) } + +// TestDecodeUpdateContainerPayloadUsesReceiverMarshaller pins the update-command +// decode path to the receiver's own JSON seam. decodeUpdateContainerPayload was +// a package function reading a package-level jsonMarshalFn; a test swapping that +// global raced with async goroutines leaked from earlier tests. It is now an +// Agent method reading a per-Agent field injected at construction, so a failure +// injected into one Agent can never be observed by another. +func TestDecodeUpdateContainerPayloadUsesReceiverMarshaller(t *testing.T) { + t.Run("injected marshaller fails only its own Agent", func(t *testing.T) { + marshalErr := errors.New("injected update marshal failure") + failing := &Agent{ + jsonMarshalFn: func(any) ([]byte, error) { + return nil, marshalErr + }, + } + + if _, err := failing.decodeUpdateContainerPayload(map[string]any{"containerId": "abc"}); err == nil { + t.Fatal("expected the injected marshaller to fail the decode") + } else if !errors.Is(err, marshalErr) { + t.Fatalf("decode error %v does not wrap the injected failure; the decode is not using a.jsonMarshal", err) + } else if !strings.Contains(err.Error(), "marshal update command payload") { + t.Fatalf("decode error %v lost its marshal context", err) + } + + plain := &Agent{} + decoded, err := plain.decodeUpdateContainerPayload(map[string]any{"containerId": "abc"}) + if err != nil { + t.Fatalf("sibling Agent must not observe the injected marshaller: %v", err) + } + if decoded.ContainerID != "abc" { + t.Fatalf("sibling Agent decoded %q, want %q", decoded.ContainerID, "abc") + } + }) + + t.Run("concurrent Agents keep their own marshaller", func(t *testing.T) { + marshalErr := errors.New("injected update marshal failure") + failing := &Agent{ + jsonMarshalFn: func(any) ([]byte, error) { + return nil, marshalErr + }, + } + plain := &Agent{} + + var wg sync.WaitGroup + problems := make(chan string, 64) + + for range 16 { + wg.Add(2) + + go func() { + defer wg.Done() + if _, err := failing.decodeUpdateContainerPayload(map[string]any{"containerId": "abc"}); !errors.Is(err, marshalErr) { + problems <- "injected Agent lost its marshaller" + } + }() + + go func() { + defer wg.Done() + decoded, err := plain.decodeUpdateContainerPayload(map[string]any{"containerId": "abc"}) + if err != nil || decoded.ContainerID != "abc" { + problems <- "sibling Agent observed the injected marshaller" + } + }() + } + + wg.Wait() + close(problems) + for problem := range problems { + t.Error(problem) + } + }) + + t.Run("decodeUpdateContainerPayload is an Agent method", func(t *testing.T) { + // A compile-time pin: if the decode is ever hoisted back to a package + // function reading a package global, this assignment stops building. + var decode func(*Agent, map[string]any) (updateContainerCommandPayload, error) = (*Agent).decodeUpdateContainerPayload + + decoded, err := decode(&Agent{}, map[string]any{"containerId": " abc "}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if decoded.ContainerID != "abc" { + t.Fatalf("decoded %q, want %q", decoded.ContainerID, "abc") + } + }) +}