Skip to content

test(build): fix data race in TestBuildLayer under -race #592

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
aftersnow wants to merge 1 commit into
base: main
Choose a base branch
from
+43 −2
Open

test(build): fix data race in TestBuildLayer under -race #592

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 43 additions & 2 deletions pkg/backend/build/builder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,39 @@ func (s *BuilderTestSuite) TestNewBuilder() {
}
}

// recordingStrategy is a hand-written OutputStrategy used by TestBuildLayer.
//
// BuildLayer hands OutputLayer a live *io.PipeReader whose background tar
// writer goroutine is mid-write. The generated testify mock formats every
// argument with fmt (via mock.Arguments.Diff) when matching the call, which
// reflects into the pipe's internal state concurrently with that writer -- a
// data race reported under `go test -race`. This fake reads the reader like
// the real strategies do, which avoids the reflection and unblocks the writer
// goroutine.
type recordingStrategy struct {
OutputStrategy // OutputConfig/OutputManifest are unused by this test.

desc ocispec.Descriptor
err error

called bool
mediaType string
relPath string
destPath string
size int64
gotReader bool
}

func (r *recordingStrategy) OutputLayer(_ context.Context, mediaType, relPath, destPath, _ string, size int64, reader io.Reader, _ hooks.Hooks) (ocispec.Descriptor, error) {
r.called = true
r.mediaType, r.relPath, r.destPath, r.size = mediaType, relPath, destPath, size
r.gotReader = reader != nil
if reader != nil {
_, _ = io.Copy(io.Discard, reader)
}
return r.desc, r.err
}

func (s *BuilderTestSuite) TestBuildLayer() {
s.Run("successful build layer", func() {
expectedDesc := ocispec.Descriptor{
Expand All @@ -123,11 +156,19 @@ func (s *BuilderTestSuite) TestBuildLayer() {
Size: 100,
}

s.mockOutputStrategy.On("OutputLayer", mock.Anything, "test/media-type.tar", "test-file.txt", "", mock.AnythingOfType("string"), mock.AnythingOfType("int64"), mock.AnythingOfType("*io.PipeReader"), mock.Anything).
Return(expectedDesc, nil)
// Use a hand-written strategy instead of the testify mock: BuildLayer
// passes OutputLayer a live *io.PipeReader, and the mock's argument
// diffing formats it with fmt, racing with the pipe's writer goroutine.
strategy := &recordingStrategy{desc: expectedDesc}
s.builder.strategy = strategy
Comment on lines +162 to +163

@gemini-code-assist gemini-code-assist Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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
}()


desc, err := s.builder.BuildLayer(context.Background(), "test/media-type.tar", s.tempDir, s.tempFile, "", hooks.NewHooks())
s.NoError(err)
s.True(strategy.called)
s.Equal("test/media-type.tar", strategy.mediaType)
s.Equal("test-file.txt", strategy.relPath)
s.Equal("", strategy.destPath)
s.True(strategy.gotReader)
s.Equal(expectedDesc.MediaType, desc.MediaType)
s.Equal(expectedDesc.Digest, desc.Digest)
s.Equal(expectedDesc.Size, desc.Size)
Expand Down