diff --git a/.github/workflows/go-ci.yml b/.github/workflows/go-ci.yml index 5f7d9e63c8..063b964d93 100644 --- a/.github/workflows/go-ci.yml +++ b/.github/workflows/go-ci.yml @@ -17,12 +17,34 @@ jobs: run: echo "go-version=$(grep 'golang' mise.toml | cut -d'"' -f2)" >> $GITHUB_OUTPUT shell: bash - name: Set up Go - uses: sourcegraph/actions/go-setup@main + uses: actions/setup-go@v7 with: go-version: ${{ steps.mise.outputs.go-version }} - - run: | - go test -race -v ./... - go test -v ./... + # setup-go's built-in cache keys on go.sum alone, has no fallback + # when go.sum changes, and shares one key across every job on the + # same OS, so the first job to finish (even one that compiled + # nothing) decides what everyone else restores. See the explicit + # cache step below instead. + cache: false + - name: Find Go cache directories + id: go-cache + shell: bash + run: | + echo "build=$(go env GOCACHE)" >> "$GITHUB_OUTPUT" + echo "modules=$(go env GOMODCACHE)" >> "$GITHUB_OUTPUT" + - name: Cache Go modules and build output + uses: actions/cache@v6 + with: + path: | + ${{ steps.go-cache.outputs.modules }} + ${{ steps.go-cache.outputs.build }} + key: ${{ github.job }}-${{ runner.os }}-${{ runner.arch }}-go${{ steps.mise.outputs.go-version }}-${{ hashFiles('**/go.sum') }} + restore-keys: | + ${{ github.job }}-${{ runner.os }}-${{ runner.arch }}-go${{ steps.mise.outputs.go-version }}- + # bash on every OS: the default pwsh on Windows does not fail the step + # when a command in a multi-line script exits non-zero. + - run: go test -race -v ./... + shell: bash # This job provides a stable name for branch protection rules # It will only pass if all Go CI matrix tests above pass diff --git a/.github/workflows/go-lint.yml b/.github/workflows/go-lint.yml index 2a851b2a1b..735a96e43a 100644 --- a/.github/workflows/go-lint.yml +++ b/.github/workflows/go-lint.yml @@ -14,7 +14,24 @@ jobs: id: mise run: echo "go-version=$(grep 'golang' mise.toml | cut -d'"' -f2)" >> $GITHUB_OUTPUT - name: Set up Go - uses: sourcegraph/actions/go-setup@main + uses: actions/setup-go@v7 with: go-version: ${{ steps.mise.outputs.go-version }} + # See go-ci.yml for why the built-in cache is off. + cache: false + - name: Find Go cache directories + id: go-cache + run: | + echo "build=$(go env GOCACHE)" >> "$GITHUB_OUTPUT" + echo "modules=$(go env GOMODCACHE)" >> "$GITHUB_OUTPUT" + - name: Cache Go modules, build output, and golangci-lint analysis + uses: actions/cache@v6 + with: + path: | + ${{ steps.go-cache.outputs.modules }} + ${{ steps.go-cache.outputs.build }} + ~/.cache/golangci-lint + key: ${{ github.job }}-${{ runner.os }}-${{ runner.arch }}-go${{ steps.mise.outputs.go-version }}-${{ hashFiles('**/go.sum') }} + restore-keys: | + ${{ github.job }}-${{ runner.os }}-${{ runner.arch }}-go${{ steps.mise.outputs.go-version }}- - run: ./dev/go-lint.sh diff --git a/.github/workflows/goreleaser-check.yml b/.github/workflows/goreleaser-check.yml index 780a6ba76b..76a266fd2a 100644 --- a/.github/workflows/goreleaser-check.yml +++ b/.github/workflows/goreleaser-check.yml @@ -10,15 +10,8 @@ jobs: steps: - name: Checkout uses: actions/checkout@v4 - with: - fetch-depth: 0 - - name: Get Go version from mise - id: mise - run: echo "go-version=$(grep 'golang' mise.toml | cut -d'"' -f2)" >> $GITHUB_OUTPUT - - name: Set up Go - uses: sourcegraph/actions/go-setup@main - with: - go-version: ${{ steps.mise.outputs.go-version }} + # `goreleaser check` only parses the config; it needs neither Go nor + # git history. - name: Check GoReleaser config uses: goreleaser/goreleaser-action@v5 with: diff --git a/internal/api/mock/api.go b/internal/api/mock/api.go index 710b11aff6..f162408abc 100644 --- a/internal/api/mock/api.go +++ b/internal/api/mock/api.go @@ -34,8 +34,14 @@ func (m *Client) NewGzippedQuery(query string) api.Request { return args.Get(0).(api.Request) } +// RequestBody wraps the body handed to NewHTTPRequest. testify fmt-prints every +// recorded argument, and printing a bare *io.PipeReader reflects into the +// pipe's internals, racing with the goroutine writing to it. Behind a struct +// field, fmt prints the pointer's address instead. +type RequestBody struct{ io.Reader } + func (m *Client) NewHTTPRequest(ctx context.Context, method, path string, body io.Reader) (*http.Request, error) { - args := m.Called(ctx, method, path, body) + args := m.Called(ctx, method, path, RequestBody{body}) var obj *http.Request if args.Get(0) != nil { obj = args.Get(0).(*http.Request) diff --git a/internal/batches/service/remote.go b/internal/batches/service/remote.go index 89b3cd56c8..d8d0782ac4 100644 --- a/internal/batches/service/remote.go +++ b/internal/batches/service/remote.go @@ -175,8 +175,15 @@ func (svc *Service) uploadFile(ctx context.Context, workingDir, filePath, batchS pipeReader, pipeWriter := io.Pipe() multipartWriter := multipart.NewWriter(pipeWriter) + request, err := svc.client.NewHTTPRequest(ctx, http.MethodPost, fmt.Sprintf(".api/files/batch-changes/%s", batchSpecID), pipeReader) + if err != nil { + return err + } + request.Header.Set("Content-Type", multipartWriter.FormDataContentType()) + // Write in a separate goroutine to properly chunk the file content. Writing to the pipe lets us not have - // to put the whole file in memory. + // to put the whole file in memory. Started only once the request exists, so a NewHTTPRequest error + // doesn't leak a goroutine blocked on a pipe nobody reads. go func() { defer pipeWriter.Close() defer multipartWriter.Close() @@ -186,12 +193,6 @@ func (svc *Service) uploadFile(ctx context.Context, workingDir, filePath, batchS } }() - request, err := svc.client.NewHTTPRequest(ctx, http.MethodPost, fmt.Sprintf(".api/files/batch-changes/%s", batchSpecID), pipeReader) - if err != nil { - return err - } - request.Header.Set("Content-Type", multipartWriter.FormDataContentType()) - resp, err := svc.client.Do(request) if err != nil { // Errors passed to pipeWriter.CloseWithError come through here. diff --git a/internal/batches/service/remote_norace_test.go b/internal/batches/service/remote_upload_test.go similarity index 96% rename from internal/batches/service/remote_norace_test.go rename to internal/batches/service/remote_upload_test.go index c397175135..307f464c52 100644 --- a/internal/batches/service/remote_norace_test.go +++ b/internal/batches/service/remote_upload_test.go @@ -1,5 +1,3 @@ -//go:build !race - package service_test import ( @@ -48,7 +46,7 @@ func TestService_UploadBatchSpecWorkspaceFiles(t *testing.T) { req := httptest.NewRequest(http.MethodPost, "http://fake.com/.api/files/batch-changes/123", nil) client.On("NewHTTPRequest", mock.Anything, http.MethodPost, ".api/files/batch-changes/123", mock.Anything). Run(func(args mock.Arguments) { - req.Body = args[3].(*io.PipeReader) + req.Body = io.NopCloser(args[3].(mockclient.RequestBody)) }). Return(req, nil). Once() @@ -90,7 +88,7 @@ func TestService_UploadBatchSpecWorkspaceFiles(t *testing.T) { req := httptest.NewRequest(http.MethodPost, "http://fake.com/.api/files/batch-changes/123", nil) client.On("NewHTTPRequest", mock.Anything, http.MethodPost, ".api/files/batch-changes/123", mock.Anything). Run(func(args mock.Arguments) { - req.Body = args[3].(*io.PipeReader) + req.Body = io.NopCloser(args[3].(mockclient.RequestBody)) }). Return(req, nil). Once() @@ -132,7 +130,7 @@ func TestService_UploadBatchSpecWorkspaceFiles(t *testing.T) { req := httptest.NewRequest(http.MethodPost, "http://fake.com/.api/files/batch-changes/123", nil) client.On("NewHTTPRequest", mock.Anything, http.MethodPost, ".api/files/batch-changes/123", mock.Anything). Run(func(args mock.Arguments) { - req.Body = args[3].(*io.PipeReader) + req.Body = io.NopCloser(args[3].(mockclient.RequestBody)) }). Return(req, nil). Twice() @@ -180,7 +178,7 @@ func TestService_UploadBatchSpecWorkspaceFiles(t *testing.T) { req := httptest.NewRequest(http.MethodPost, "http://fake.com/.api/files/batch-changes/123", nil) client.On("NewHTTPRequest", mock.Anything, http.MethodPost, ".api/files/batch-changes/123", mock.Anything). Run(func(args mock.Arguments) { - req.Body = args[3].(*io.PipeReader) + req.Body = io.NopCloser(args[3].(mockclient.RequestBody)) }). Return(req, nil). Twice() @@ -229,7 +227,7 @@ func TestService_UploadBatchSpecWorkspaceFiles(t *testing.T) { req := httptest.NewRequest(http.MethodPost, "http://fake.com/.api/files/batch-changes/123", nil) client.On("NewHTTPRequest", mock.Anything, http.MethodPost, ".api/files/batch-changes/123", mock.Anything). Run(func(args mock.Arguments) { - req.Body = args[3].(*io.PipeReader) + req.Body = io.NopCloser(args[3].(mockclient.RequestBody)) }). Return(req, nil). Once() @@ -281,7 +279,7 @@ func TestService_UploadBatchSpecWorkspaceFiles(t *testing.T) { req := httptest.NewRequest(http.MethodPost, "http://fake.com/.api/files/batch-changes/123", nil) client.On("NewHTTPRequest", mock.Anything, http.MethodPost, ".api/files/batch-changes/123", mock.Anything). Run(func(args mock.Arguments) { - req.Body = args[3].(*io.PipeReader) + req.Body = io.NopCloser(args[3].(mockclient.RequestBody)) }). Return(req, nil). Times(3) @@ -362,7 +360,7 @@ func TestService_UploadBatchSpecWorkspaceFiles(t *testing.T) { req := httptest.NewRequest(http.MethodPost, "http://fake.com/.api/files/batch-changes/123", nil) client.On("NewHTTPRequest", mock.Anything, http.MethodPost, ".api/files/batch-changes/123", mock.Anything). Run(func(args mock.Arguments) { - req.Body = args[3].(*io.PipeReader) + req.Body = io.NopCloser(args[3].(mockclient.RequestBody)) }). Return(req, nil). Once() @@ -400,7 +398,7 @@ func TestService_UploadBatchSpecWorkspaceFiles(t *testing.T) { req := httptest.NewRequest(http.MethodPost, "http://fake.com/.api/files/batch-changes/123", nil) client.On("NewHTTPRequest", mock.Anything, http.MethodPost, ".api/files/batch-changes/123", mock.Anything). Run(func(args mock.Arguments) { - req.Body = args[3].(*io.PipeReader) + req.Body = io.NopCloser(args[3].(mockclient.RequestBody)) }). Return(req, nil). Once() diff --git a/internal/batches/service/remote_windows_test.go b/internal/batches/service/remote_windows_test.go index 9cbdc07182..649864f7d4 100644 --- a/internal/batches/service/remote_windows_test.go +++ b/internal/batches/service/remote_windows_test.go @@ -47,7 +47,7 @@ func TestService_UploadBatchSpecWorkspaceFiles_Windows_Path(t *testing.T) { req := httptest.NewRequest(http.MethodPost, "http://fake.com/.api/files/batch-changes/123", nil) client.On("NewHTTPRequest", mock.Anything, http.MethodPost, ".api/files/batch-changes/123", mock.Anything). Run(func(args mock.Arguments) { - req.Body = args[3].(*io.PipeReader) + req.Body = io.NopCloser(args[3].(mockclient.RequestBody)) }). Return(req, nil). Once()