[Testing] Fix flaky unit tests

consensus/hotstuff/integration/instance_test.go

@@ -326,24 +326,19 @@ func NewInstance(t *testing.T, options ...Option) *Instance {
 	// program the finalizer module behaviour
 	in.finalizer.On("MakeFinal", mock.Anything).Return(
 		func(blockID flow.Identifier) error {
-
-			// as we don't use mocks to assert expectations, but only to
-			// simulate behaviour, we should drop the call data regularly
 			in.updatingBlocks.RLock()
-			block, found := in.headers[blockID]
+			_, found := in.headers[blockID]
 			in.updatingBlocks.RUnlock()
 			if !found {
 				return fmt.Errorf("can't broadcast with unknown parent")
 			}
-			if block.Height%100 == 0 {
-				in.committee.Calls = nil
-				in.builder.Calls = nil
-				in.signer.Calls = nil
-				in.verifier.Calls = nil
-				in.notifier.Calls = nil
-				in.finalizer.Calls = nil
-			}
-
+			// Note: an earlier version of this callback periodically reset the mocks' recorded
+			// call data (`in.verifier.Calls = nil` etc.) to bound memory usage. This is NOT safe:
+			// the mocks are invoked concurrently by the instance's worker goroutines (e.g. vote
+			// and timeout aggregators), which append to Calls under the mock's internal mutex.
+			// Writing the field directly corrupts the slice and crashes the test binary with a
+			// SIGSEGV. The memory bounding is unnecessary anyway: all tests in this package
+			// finalize at most ~100 views, accumulating only a modest number of recorded calls.
 			return nil
 		},
 	)

engine/access/access_test.go

@@ -1121,16 +1121,24 @@ func (suite *Suite) TestGetTransactionResult() {
 		txIdNegative := collectionNegative.Transactions[0].ID()
 		collectionIdNegative := collectionNegative.ID()
 
-		// the transactions should eventually be indexed by the collection indexer
+		// The transactions should eventually be indexed by the collection indexer, AND the
+		// collection-to-block index of the (finalized) positive block should eventually be
+		// written by the finalized-block processor. These are two independent async pipelines
+		// (see the lookup in transactions.Backend), and the API calls below require both to
+		// have completed. Note: blockNegative is deliberately not finalized, so we must not
+		// wait for its collection to be indexed.
 		require.Eventually(suite.T(), func() bool {
 			if _, err := transactions.ByID(txId); err != nil {
 				return false
 			}
 			if _, err := transactions.ByID(txIdNegative); err != nil {
 				return false
 			}
+			if _, err := all.Blocks.ByCollectionID(collectionId); err != nil {
+				return false
+			}
 			return true
-		}, 1*time.Second, 10*time.Millisecond, "transactions never indexed")
+		}, 5*time.Second, 10*time.Millisecond, "transactions and collection-block index never indexed")
 
 		assertTransactionResult := func(
 			resp *accessproto.TransactionResultResponse,

engine/access/ingestion2/engine_test.go

@@ -335,20 +335,25 @@ func (s *Suite) TestOnFinalizedBlockSingle() {
 
 	missingCollectionCount := 4
 	wg := sync.WaitGroup{}
-	wg.Add(missingCollectionCount)
+	// wait for the EntityByID calls AND the subsequent Force call: the engine issues them on its
+	// own worker goroutine, so the test must not finish (and assert the mock expectations) until
+	// Force was actually called
+	wg.Add(missingCollectionCount + 1)
 
 	for _, cg := range block.Payload.Guarantees {
 		s.request.On("EntityByID", cg.CollectionID, mock.Anything).Return().Run(func(args mock.Arguments) {
 			// Ensure the test does not complete its work faster than necessary
 			wg.Done()
 		}).Once()
 	}
-	s.request.On("Force").Return().Once()
+	s.request.On("Force").Return().Run(func(args mock.Arguments) {
+		wg.Done()
+	}).Once()
 
 	// process the block through the finalized callback
 	eng.OnFinalizedBlock(&hotstuffBlock)
 
-	unittest.RequireReturnsBefore(s.T(), wg.Wait, 100*time.Millisecond, "expect to process new block before timeout")
+	unittest.RequireReturnsBefore(s.T(), wg.Wait, time.Second, "expect to process new block before timeout")
 
 	// assert that the block was retrieved and all collections were requested
 	s.headers.AssertExpectations(s.T())
@@ -398,7 +403,10 @@ func (s *Suite) TestOnFinalizedBlockSeveralBlocksAhead() {
 
 	missingCollectionCountPerBlock := 4
 	wg := sync.WaitGroup{}
-	wg.Add(missingCollectionCountPerBlock * newBlocksCount)
+	// wait for the EntityByID calls AND the per-block Force calls: the engine issues them on its
+	// own worker goroutine, so the test must not finish (and assert the mock expectations) until
+	// all Force calls actually happened
+	wg.Add((missingCollectionCountPerBlock + 1) * newBlocksCount)
 
 	// expected all new blocks after last block processed
 	for _, block := range blocks {
@@ -410,7 +418,9 @@ func (s *Suite) TestOnFinalizedBlockSeveralBlocksAhead() {
 				wg.Done()
 			}).Once()
 		}
-		s.request.On("Force").Return().Once()
+		s.request.On("Force").Return().Run(func(args mock.Arguments) {
+			wg.Done()
+		}).Once()
 
 		for _, seal := range block.Payload.Seals {
 			s.results.On("Index", seal.BlockID, seal.ResultID).Return(nil).Once()
@@ -419,7 +429,7 @@ func (s *Suite) TestOnFinalizedBlockSeveralBlocksAhead() {
 
 	eng.OnFinalizedBlock(&hotstuffBlock)
 
-	unittest.RequireReturnsBefore(s.T(), wg.Wait, 100*time.Millisecond, "expect to process all blocks before timeout")
+	unittest.RequireReturnsBefore(s.T(), wg.Wait, time.Second, "expect to process all blocks before timeout")
 
 	expectedEntityByIDCalls := 0
 	expectedIndexCalls := 0

engine/access/rest/websockets/controller_test.go

@@ -824,11 +824,11 @@ func (s *WsControllerSuite) TestSubscribeBlocks() {
 // 2. Configure the WebSocket controller with a rate limit of 2 responses per second.
 // 3. Simulate sending messages to the `multiplexedStream` channel.
 // 4. Collect timestamps of message writes to verify rate-limiting behavior.
-// 5. Assert that all messages are processed and that the delay between messages respects the configured rate limit.
+// 5. Assert that all messages are processed and that the overall pacing respects the configured rate limit.
 //
 // The test ensures that:
 // - The number of messages processed matches the total messages sent.
-// - The delay between consecutive messages falls within the expected range based on the rate limit, with a tolerance of 5ms.
+// - Emitting all messages takes at least (totalMessages-1) rate-limit periods in total.
 func (s *WsControllerSuite) TestRateLimiter() {
 	t := s.T()
 	totalMessages := 5 // Number of messages to simulate.
@@ -876,13 +876,18 @@ func (s *WsControllerSuite) TestRateLimiter() {
 
 	// Calculate the expected delay between messages based on the rate limit.
 	expectedDelay := time.Second / time.Duration(config.MaxResponsesPerSecond)
-	const tolerance = float64(5 * time.Millisecond) // Allow up to 5ms deviation.
 
-	// Step 6: Assert that the delays respect the rate limit with tolerance.
-	for i := 1; i < len(timestamps); i++ {
-		delay := timestamps[i].Sub(timestamps[i-1])
-		assert.InDelta(t, expectedDelay, delay, tolerance, "Messages should respect the rate limit")
-	}
+	// Step 6: Assert that the overall pacing respects the rate limit.
+	// Note: we deliberately do NOT assert individual inter-message delays. The limiter
+	// (token bucket, burst 1) accrues tokens in real time, so on a loaded machine a
+	// scheduling stall can make one gap longer and the following gap correspondingly
+	// shorter — both are correct limiter behavior. The robust invariant is that emitting
+	// n messages takes at least (n-1) rate-limit periods in total.
+	minElapsed := time.Duration(totalMessages-1) * expectedDelay
+	tolerance := 100 * time.Millisecond // measurement jitter between limiter creation and first write
+	elapsed := timestamps[len(timestamps)-1].Sub(timestamps[0])
+	assert.GreaterOrEqual(t, elapsed, minElapsed-tolerance,
+		"Messages should be paced at no more than the configured rate")
 }
 
 // TestConfigureKeepaliveConnection ensures that the WebSocket connection is configured correctly.
@@ -1087,10 +1092,13 @@ func (s *WsControllerSuite) TestKeepaliveRoutine() {
 			}).
 			Times(expectedCalls + 1)
 
+		// Maybe(): with the microsecond ping period the keepalive routine can complete (and shut
+		// the controller down) before the reader routine issues its first ReadJSON call. The
+		// blocking read only keeps the connection open; it is not the property under test.
 		conn.On("ReadJSON", mock.Anything).Return(func(_ interface{}) error {
 			<-done
 			return &websocket.CloseError{Code: websocket.CloseNormalClosure}
-		})
+		}).Maybe()
 
 		factory := dpmock.NewDataProviderFactory(t)
 		controller, err := NewWebSocketController(s.logger, s.wsConfig, conn, factory, s.streamLimiter)
@@ -1146,7 +1154,13 @@ func (s *WsControllerSuite) TestKeepaliveRoutine() {
 		factory := dpmock.NewDataProviderFactory(t)
 		controller, err := NewWebSocketController(s.logger, s.wsConfig, conn, factory, s.streamLimiter)
 		require.NoError(t, err)
-		controller.keepaliveConfig = keepaliveConfig
+		// Use a ping period that cannot elapse during the test: with the suite's microsecond
+		// PingPeriod, both the ticker and the cancelled context are ready at keepalive's first
+		// select, which picks randomly and occasionally produces an unexpected WriteControl call.
+		controller.keepaliveConfig = KeepaliveConfig{
+			PingPeriod: time.Hour,
+			PongWait:   2 * time.Hour,
+		}
 
 		ctx, cancel := context.WithCancel(context.Background())
 		cancel() // Immediately cancel the context