Skip to content

Data Race when running attestation tests #10438

Closed
#10439
Closed
Data Race when running attestation tests#10438
#10439
Labels
tech-debtA chore that addresses technical debt

Description

@williammartin
williammartin
opened on Feb 13, 2025

Description

In https://github.com/cli/cli/actions/runs/13301543234/job/37143755142?pr=10430, there is a data race when running the tests:

==================
WARNING: DATA RACE
Read at 0x00c000440ae0 by goroutine 72:
  github.com/cli/cli/v2/pkg/cmd/attestation/api.(*failAfterNCallsHttpClient).Get()
      /Users/runner/work/cli/cli/pkg/cmd/attestation/api/mock_httpClient_test.go:69 +0x68
  github.com/cli/cli/v2/pkg/cmd/attestation/api.(*LiveClient).getBundle.func1()
      /Users/runner/work/cli/cli/pkg/cmd/attestation/api/client.go:194 +0x78
  github.com/cenkalti/backoff/v4.RetryNotifyWithTimer.Operation.withEmptyData.func1()
      /Users/runner/go/pkg/mod/github.com/cenkalti/backoff/[email protected]/retry.go:18 +0x30
  github.com/cenkalti/backoff/v4.doRetryNotify[go.shape.struct {}]()
      /Users/runner/go/pkg/mod/github.com/cenkalti/backoff/[email protected]/retry.go:88 +0x15c
  github.com/cenkalti/backoff/v4.RetryNotifyWithTimer()
      /Users/runner/go/pkg/mod/github.com/cenkalti/backoff/[email protected]/retry.go:61 +0x80
  github.com/cenkalti/backoff/v4.RetryNotify()
      /Users/runner/go/pkg/mod/github.com/cenkalti/backoff/[email protected]/retry.go:49 +0x1b0
  github.com/cenkalti/backoff/v4.Retry()
      /Users/runner/go/pkg/mod/github.com/cenkalti/backoff/[email protected]/retry.go:38 +0x190
  github.com/cli/cli/v2/pkg/cmd/attestation/api.(*LiveClient).getBundle()
      /Users/runner/work/cli/cli/pkg/cmd/attestation/api/client.go:193 +0x138
  github.com/cli/cli/v2/pkg/cmd/attestation/api.(*LiveClient).fetchBundleFromAttestations.func1()
      /Users/runner/work/cli/cli/pkg/cmd/attestation/api/client.go:169 +0x1f0
  golang.org/x/sync/errgroup.(*Group).Go.func1()
      /Users/runner/go/pkg/mod/golang.org/x/[email protected]/errgroup/errgroup.go:78 +0x7c

Previous write at 0x00c000440ae0 by goroutine 73:
  github.com/cli/cli/v2/pkg/cmd/attestation/api.(*failAfterNCallsHttpClient).Get()
      /Users/runner/work/cli/cli/pkg/cmd/attestation/api/mock_httpClient_test.go:69 +0x7c
  github.com/cli/cli/v2/pkg/cmd/attestation/api.(*LiveClient).getBundle.func1()
      /Users/runner/work/cli/cli/pkg/cmd/attestation/api/client.go:194 +0x78
  github.com/cenkalti/backoff/v4.RetryNotifyWithTimer.Operation.withEmptyData.func1()
      /Users/runner/go/pkg/mod/github.com/cenkalti/backoff/[email protected]/retry.go:18 +0x30
  github.com/cenkalti/backoff/v4.doRetryNotify[go.shape.struct {}]()
      /Users/runner/go/pkg/mod/github.com/cenkalti/backoff/[email protected]/retry.go:88 +0x15c
  github.com/cenkalti/backoff/v4.RetryNotifyWithTimer()
      /Users/runner/go/pkg/mod/github.com/cenkalti/backoff/[email protected]/retry.go:61 +0x80
  github.com/cenkalti/backoff/v4.RetryNotify()
      /Users/runner/go/pkg/mod/github.com/cenkalti/backoff/[email protected]/retry.go:49 +0x1b0
  github.com/cenkalti/backoff/v4.Retry()
      /Users/runner/go/pkg/mod/github.com/cenkalti/backoff/[email protected]/retry.go:38 +0x190
  github.com/cli/cli/v2/pkg/cmd/attestation/api.(*LiveClient).getBundle()
      /Users/runner/work/cli/cli/pkg/cmd/attestation/api/client.go:193 +0x138
  github.com/cli/cli/v2/pkg/cmd/attestation/api.(*LiveClient).fetchBundleFromAttestations.func1()
      /Users/runner/work/cli/cli/pkg/cmd/attestation/api/client.go:169 +0x1f0
  golang.org/x/sync/errgroup.(*Group).Go.func1()
      /Users/runner/go/pkg/mod/golang.org/x/[email protected]/errgroup/errgroup.go:78 +0x7c

Goroutine 72 (running) created at:
  golang.org/x/sync/errgroup.(*Group).Go()
      /Users/runner/go/pkg/mod/golang.org/x/[email protected]/errgroup/errgroup.go:75 +0x10c
  github.com/cli/cli/v2/pkg/cmd/attestation/api.(*LiveClient).fetchBundleFromAttestations()
      /Users/runner/work/cli/cli/pkg/cmd/attestation/api/client.go:1[54](https://github.com/cli/cli/actions/runs/13301543234/job/37143755142?pr=10430#step:5:55) +0xb4
  github.com/cli/cli/v2/pkg/cmd/attestation/api.TestFetchBundleFromAttestations_FailOnTheSecondAttestation()
      /Users/runner/work/cli/cli/pkg/cmd/attestation/api/client_test.go:215 +0x2e0
  testing.tRunner()
      /Users/runner/go/pkg/mod/golang.org/[email protected]/src/testing/testing.go:1690 +0x184
  testing.(*T).Run.gowrap1()
      /Users/runner/go/pkg/mod/golang.org/[email protected]/src/testing/testing.go:1743 +0x40

Goroutine 73 (running) created at:
  golang.org/x/sync/errgroup.(*Group).Go()
      /Users/runner/go/pkg/mod/golang.org/x/[email protected]/errgroup/errgroup.go:75 +0x10c
  github.com/cli/cli/v2/pkg/cmd/attestation/api.(*LiveClient).fetchBundleFromAttestations()
      /Users/runner/work/cli/cli/pkg/cmd/attestation/api/client.go:154 +0xb4
  github.com/cli/cli/v2/pkg/cmd/attestation/api.TestFetchBundleFromAttestations_FailOnTheSecondAttestation()
      /Users/runner/work/cli/cli/pkg/cmd/attestation/api/client_test.go:215 +0x2e0
  testing.tRunner()
      /Users/runner/go/pkg/mod/golang.org/[email protected]/src/testing/testing.go:1690 +0x184
  testing.(*T).Run.gowrap1()
      /Users/runner/go/pkg/mod/golang.org/[email protected]/src/testing/testing.go:1743 +0x40
==================
--- FAIL: TestFetchBundleFromAttestations_FailOnTheSecondAttestation (0.[63](https://github.com/cli/cli/actions/runs/13301543234/job/37143755142?pr=10430#step:5:64)s)
    testing.go:1399: race detected during execution of test

Activity

  1. williammartin
    added
    tech-debtA chore that addresses technical debt
    on Feb 13, 2025

  2. williammartin commented on Feb 13, 2025

    @williammartin
    williammartin
    on Feb 13, 2025
    MemberAuthor

    I think we need a mutex here:

    cli/pkg/cmd/attestation/api/mock_httpClient_test.go

    Lines 59 to 66 in cdb44f8

    type failAfterNCallsHttpClient struct {
    mock.Mock
    FailOnCallN int
    FailOnAllSubsequentCalls bool
    NumCalls int
    }
    func (m *failAfterNCallsHttpClient) Get(url string) (*http.Response, error) {

    Otherwise the client concurrency will race here:

    cli/pkg/cmd/attestation/api/client.go

    Line 154 in cdb44f8

    g.Go(func() error {

    I haven't managed to reproduce the failure locally, but it seems pretty likely.

  3. codysoyland commented on Feb 13, 2025

    @codysoyland
    codysoyland
    on Feb 13, 2025
    Contributor

    I agree, need to protect NumCalls++ with a mutex/atomic. I'll make a PR.

  4. williammartin commented on Feb 13, 2025

    @williammartin
    williammartin
    on Feb 13, 2025
    MemberAuthor

    I eventually saw a failure using:

    go test -v -count=128 -run 'TestFetchBundleFromAttestations_FailOnTheSecondAttestation' -race ./pkg/cmd/attestation/api/...
  5. williammartin
    closed this as completedin #10439on Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    tech-debtA chore that addresses technical debt

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions