test(build): fix data race in TestBuildLayer under -race#592
Conversation
BuildLayer hands OutputLayer a live *io.PipeReader backed by a background tar writer goroutine. The generated testify mock formats every argument with fmt (mock.Arguments.Diff) to match the call, which reflects into the pipe's internal mutex concurrently with the writer -- a data race under `go test -race`. No matcher avoids it: Diff formats the actual argument before any matcher logic runs. Replace the testify mock in the "successful build layer" subtest with a small hand-written OutputStrategy that reads the reader (as the real local/remote strategies do). This avoids reflecting over the live pipe and unblocks the writer goroutine. Production is unchanged. Claude-Session: https://claude.ai/code/session_01HHaMSeWpe3apQEx7n4UvRg Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
There was a problem hiding this comment.
Code Review
This pull request replaces a testify mock with a hand-written recordingStrategy in builder_test.go to resolve a data race caused by concurrent formatting of a live *io.PipeReader. The feedback recommends restoring the original s.builder.strategy at the end of the subtest to prevent test pollution, and initializing the embedded OutputStrategy interface to avoid potential nil pointer dereference panics.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| strategy := &recordingStrategy{desc: expectedDesc} | ||
| s.builder.strategy = strategy |
There was a problem hiding this comment.
Overwriting the shared suite-level s.builder.strategy field inside a subtest without restoring it can lead to test pollution and unexpected side effects in subsequent subtests or other tests.
Additionally, since recordingStrategy embeds the OutputStrategy interface anonymously, leaving it uninitialized (nil) means any unexpected calls to other methods (like OutputConfig or OutputManifest) will cause a nil pointer dereference panic.
To make the test more robust and isolated, initialize the embedded OutputStrategy with s.mockOutputStrategy and use defer to restore the original strategy after the subtest completes.
| strategy := &recordingStrategy{desc: expectedDesc} | |
| s.builder.strategy = strategy | |
| strategy := &recordingStrategy{ | |
| OutputStrategy: s.mockOutputStrategy, | |
| desc: expectedDesc, | |
| } | |
| s.builder.strategy = strategy | |
| defer func() { | |
| s.builder.strategy = s.mockOutputStrategy | |
| }() |
Summary
Fix a data race in
TestBuilderSuite/TestBuildLayerreported bygo test -race ./pkg/backend/build/.... Test-only change; production code is untouched.Root cause
BuildLayerhandsOutputLayera live*io.PipeReaderwhose backing tar-writer goroutine (archiver.Tar) is mid-write. The subtest uses the generated testify mock forOutputStrategy, and testify'smock.Arguments.Diffformats every actual argument withfmt(fmt.Sprintf("(%[1]T=%[1]v)", actual)) to match the call. Formatting the*io.PipeReaderreflects into the pipe's internalsync.Mutexstate concurrently with the writer goroutine'sLock()— the reported race.No mock matcher avoids it:
Diffformats the actual argument before any matcher logic runs, somock.Anything/AnythingOfType/MatchedBymake no difference.Fix
Replace the testify mock in the "successful build layer" subtest with a small hand-written
OutputStrategythat reads the reader (exactly as the reallocalOutput/remoteOutputstrategies do). This avoids reflecting over the live pipe and unblocks the writer goroutine, while still asserting the call arguments and returned descriptor.Notes
remoteOutput.OutputLayerrelies on the reader's concrete*io.PipeReadertype (to drain it when a blob already exists), so wrapping the reader was deliberately avoided.maintoday and is not currently caught by CI (the test targets don't pass-race).Test plan
go test -race -count=20 -run TestBuilderSuite/TestBuildLayer ./pkg/backend/build/...— clean (was failing).go test -race ./...— all packages clean.go vet ./.../gofmtclean.