From 8069b6c511eb9edbd06de2306f0c295a842170d4 Mon Sep 17 00:00:00 2001 From: Marc LeBlanc <7050295+marcleblanc2@users.noreply.github.com> Date: Thu, 29 Oct 2026 23:58:36 -0600 Subject: [PATCH 1/2] ci: cache Go modules and build output per job, drop redundant test pass Every CI run compiled the whole dependency tree from scratch on all three OSes, because: - goreleaser-check ran setup-go without ever building anything, so it finished first and saved a 7 KB cache under the key that go-lint and go-test on Linux then restored ('cache hit', never re-saved). - setup-go's built-in cache has no restore-keys, so any go.sum change (every renovate PR) started cold on macOS and Windows too. - go-ci ran 'go test -race' and then 'go test' again, recompiling everything without race instrumentation to rerun the same tests. Now each job owns its cache key (job, OS, arch, Go version, go.sum hash) with a prefix fallback, goreleaser-check no longer installs Go, and the test job runs the race pass only. Amp-Thread-ID: https://ampcode.com/threads/T-01a18b5c-c5bb-7497-82f1-3e9333c8b7e1 Co-authored-by: Amp --- .github/workflows/go-ci.yml | 29 ++++++++++++++++++++++---- .github/workflows/go-lint.yml | 19 ++++++++++++++++- .github/workflows/goreleaser-check.yml | 11 ++-------- 3 files changed, 45 insertions(+), 14 deletions(-) diff --git a/.github/workflows/go-ci.yml b/.github/workflows/go-ci.yml index 5f7d9e63c8..edf042f308 100644 --- a/.github/workflows/go-ci.yml +++ b/.github/workflows/go-ci.yml @@ -17,12 +17,33 @@ 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 }}- + # -race already runs every test; a second plain pass only recompiles + # everything without race instrumentation and reruns the same tests. + - run: go test -race -v ./... # 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: From 1c0e275e0e738de970db4265c5734081322b5125 Mon Sep 17 00:00:00 2001 From: Marc LeBlanc <7050295+marcleblanc2@users.noreply.github.com> Date: Fri, 30 Oct 2026 00:09:20 -0600 Subject: [PATCH 2/2] test: run the batch spec upload tests under -race again The upload tests were tagged //go:build !race in #1082 because testify fmt-prints every recorded mock argument, and printing a bare *io.PipeReader reflects into the pipe's internals while the producer goroutine writes to it. The Windows variant never got the tag, so 'go test -race' has been failing on Windows the whole time, hidden because the step runs under pwsh, which does not fail on a non-zero exit in the middle of a multi-line script; the second non-race pass then went green. The mock client now wraps the body in a RequestBody struct, so fmt prints the pointer address instead of walking the pipe. That removes the need for the !race tag and for the second test pass. uploadFile also starts the writer goroutine only after NewHTTPRequest succeeds; before, an error there leaked a goroutine blocked forever on a pipe nobody reads. Amp-Thread-ID: https://ampcode.com/threads/T-01a18b5c-c5bb-7497-82f1-3e9333c8b7e1 Co-authored-by: Amp --- .github/workflows/go-ci.yml | 5 +++-- internal/api/mock/api.go | 8 +++++++- internal/batches/service/remote.go | 15 ++++++++------- ...te_norace_test.go => remote_upload_test.go} | 18 ++++++++---------- .../batches/service/remote_windows_test.go | 2 +- 5 files changed, 27 insertions(+), 21 deletions(-) rename internal/batches/service/{remote_norace_test.go => remote_upload_test.go} (96%) diff --git a/.github/workflows/go-ci.yml b/.github/workflows/go-ci.yml index edf042f308..063b964d93 100644 --- a/.github/workflows/go-ci.yml +++ b/.github/workflows/go-ci.yml @@ -41,9 +41,10 @@ jobs: 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 }}- - # -race already runs every test; a second plain pass only recompiles - # everything without race instrumentation and reruns the same tests. + # 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/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()