Skip to content

test(network/grpc): fix race in Test_grpcConnectionManager_Peers#4276

Open
stevenvegt wants to merge 1 commit into
masterfrom
fix/grpc-peers-test-race
Open

test(network/grpc): fix race in Test_grpcConnectionManager_Peers#4276
stevenvegt wants to merge 1 commit into
masterfrom
fix/grpc-peers-test-race

Conversation

@stevenvegt
Copy link
Copy Markdown
Member

Problem

`Test_grpcConnectionManager_Peers/inbound_stream_triggers_observer` (and the mirror outbound subtest) is flaky on CI. Example failure: PR #4275, run 26294161548.

The failure message:

```
controller.go:97: missing call(s) to *grpc.MockAuthenticator.Authenticate(is equal to did:nuts:test (did.DID), is anything)
```

Root cause

Both subtests set up two peers (cm1 and cm2) with separate `MockAuthenticator` mocks, and expect `Authenticate` to be called on both:

```go
authenticator1.EXPECT().Authenticate(*nodeDID, gomock.Any()).Return(transport.Peer{ID: "1"}, nil)
authenticator2.EXPECT().Authenticate(*nodeDID, gomock.Any()).Return(transport.Peer{ID: "2"}, nil)
```

But the wait condition only watches one side's observer:

```go
test.WaitFor(t, func() (bool, error) {
return capturedPeer.Load() != nil, nil
}, time.Second*2, "waiting for peer 1 observers")
```

Both authentications happen in parallel on opposite ends of the bufconn. When the observed side wins the race, `cm2.Stop()` runs while the other side's `Authenticate` is still in flight, and `t.Cleanup` triggers gomock's `Finish()` before the mock call lands.

Fix

Extend the wait condition to also require the unobserved side's `Peers()` list to be non-empty. A side only adds a remote to its `Peers()` list after its own `Authenticate` returns, so this guarantees both expectations have been satisfied before teardown.

Test plan

  • `go test -count=20 -run "Test_grpcConnectionManager_Peers" ./network/transport/grpc/` passes locally (20/20)
  • CI passes

@qltysh
Copy link
Copy Markdown
Contributor

qltysh Bot commented May 22, 2026

Qlty


Coverage Impact

⬇️ Merging this pull request will decrease total coverage on master by 0.02%.

🚦 See full report on Qlty Cloud »

🛟 Help
  • Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

  • Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

  • File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

    • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

…g down

Test_grpcConnectionManager_Peers/inbound_stream_triggers_observer and the
mirror outbound subtest set Authenticate expectations on both peer's
authenticator mocks but only waited for one side's observer to fire before
calling cm2.Stop(). When the observed side won the race, the other side's
authenticator could still be in flight when t.Cleanup ran gomock's Finish(),
reporting a missing call.

Extend the wait condition to also require the unobserved side's Peers() list
to be populated, which only happens after that side's Authenticate returns.

Assisted-by: AI
@stevenvegt stevenvegt force-pushed the fix/grpc-peers-test-race branch from 06c187b to 68bc22c Compare May 22, 2026 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants