[Testing] Fix flaky unit tests

[janezpodhostnik]

Fixes a batch of flaky unit tests. The flakiness fell into a few categories:

• Races between test assertions and async workers — replaced point-in-time asserts
  with waits, registered mock expectations upfront instead of lazily, and stopped
  engines/workers before test teardown.
• Overly tight statistical bounds — the sync spam load tests now use exact
  Binomial(1000, p) quantiles with 1e-9 tail probability, and register loop-invariant
  mock expectations once instead of per iteration (the accumulated expectations made
  call matching quadratic and the tests extremely slow).
• Nondeterministic fixtures — sibling blocks now get guaranteed-unique views,
  resource-limit overrides are guaranteed distinct from defaults, and an exact EVM
  register count assertion (dependent on a random block ID) was relaxed.
• Wall-clock dependence — TestLogProgressNoDataForAWhile now runs under
  testing/synctest with a fake clock.

Also fixes latent data races surfaced while testing: an unsynchronized mock in the
pipeline test utils, a captured err shared across goroutines in the bandwidth rate
limit test, and enables TestRegisterVoteConsumer, which was never running due to a
missing Test prefix.

Tests touched

• consensus/hotstuff/votecollector: TestStateMachine suite (all subtests via
  prepareMockedProcessor); enabled TestRegisterVoteConsumer
• engine/common/follower: TestProcessFinalizedBlock
• engine/common/stop: TestStopControl_OnProcessedBlock
• engine/common/synchronization:
  TestLoad_Process_SyncRequest_HigherThanReceiver_OutsideTolerance_AlwaysReportSpam,
  TestLoad_Process_SyncRequest_HigherThanReceiver_OutsideTolerance_SometimesReportSpam,
  TestLoad_Process_RangeRequest_SometimesReportSpam,
  TestLoad_Process_BatchRequest_SometimesReportSpam
• fvm/evm: TestCadenceOwnedAccountFunctionalities (dryCall, dryCallWithSigAndArgs)
• module/builder/collection: TestBuildOn_WithOrphanedReferenceBlock
• module/executiondatasync/optimistic_sync/pipeline: shared test utils
  (waitForStateUpdates, mockStateProvider) + happy-path/error functional tests
• module/util: TestLogProgressNoDataForAWhile
• network/p2p/builder: TestBuildLibp2pResourceManagerLimits
• network/p2p/inspector/validation:
  TestNewControlMsgValidationInspector_validateClusterPrefixedTopic
• network/test/cohort1: TestUnicastRateLimit_Bandwidth
• state/cluster/badger: TestExtend_WithOrphanedReferenceBlock
• storage/migration: TestCompareKeyValuePairsFromChannels

All touched packages pass go test -race locally (repeated runs).

Known coverage gaps introduced (follow-up PRs planned)

Some fixes deliberately trade assertion strength for determinism. The remaining gaps
will be addressed in follow-up PRs:

• Sync spam load tests: the widened bounds drop the lower bound to 0 for groups
  with p ≤ ~0.02, so those subtests alone can no longer detect an implementation that
  never reports misbehavior (higher-p groups still catch it). Follow-up: extract the
  probabilistic decision into a pure helper and table-test the boundary exactly, then
  slim the statistical groups down to wiring smoke tests.
• EVM register count: assert.Len(..., 13) → NotEmpty can no longer catch a
  write-amplification regression. Follow-up: pin the test env's random block ID (or
  assert a sampled tight range) and restore an exact/bounded count.
• Inspector cluster-prefix test: pinned to a single worker; multi-worker
  dissemination behavior (1..N notifications past the hard threshold) is untested.
  ✅ Addressed in [Testing] Fix data race in cluster-prefix RecordCache #8627 (which also fixes a production data race in RecordCache
  that the new multi-worker test exposed).
• Resource-limit overrides: the new fixture only generates positive values, so
  non-positive overrides ("keep default") are no longer exercised end-to-end through
  BuildLibp2pResourceManagerLimits (unit coverage exists in
  TestApplyResourceLimitOverride). ✅ Addressed in [Testing] Fix data race in cluster-prefix RecordCache #8627.
• TestRegisterVoteConsumer: the enabled test covers the caching state only;
  consumer delivery across the caching→verifying transition is not covered at the
  collector level (cache-level coverage exists in TestVotesCache_RegisterVoteConsumer).
  ✅ Addressed in [Testing] Fix data race in cluster-prefix RecordCache #8627.

---

This PR was produced in collaboration with claude.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

Summary by CodeRabbit

• Tests
  • Improved deterministic handling for asynchronous consensus, synchronization, and stop-control flows (more reliable assertions and event waiting).
  • Reduced flakiness by using controlled timing (fake clock), fixed/unique block views, and safer concurrency primitives in test helpers.
  • Strengthened pipeline state-transition checks to ensure expected “no error” behavior during critical windows.
  • Refined networking and inspection validations to match rate-limiting and single-notification expectations under concurrency.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6752cb2f-b047-40de-a122-d8016b25dd0f

📥 Commits

Reviewing files that changed from the base of the PR and between fc01b9a and bfec14b.

📒 Files selected for processing (3)

• engine/common/follower/compliance_engine_test.go
• engine/common/synchronization/engine_spam_test.go
• network/test/cohort1/network_test.go

🚧 Files skipped from review as they are similar to previous changes (3)

• engine/common/follower/compliance_engine_test.go
• network/test/cohort1/network_test.go
• engine/common/synchronization/engine_spam_test.go

---

📝 Walkthrough

Walkthrough

Changes

Vote collector expectations

Layer / File(s)	Summary
Proposer-vote processing expectations consensus/hotstuff/votecollector/statemachine_test.go	Mocked processor expectations now specify scenario-specific proposer-vote notification counts, including zero and repeated processing cases.
Vote-consumer delivery assertions consensus/hotstuff/votecollector/statemachine_test.go	The consumer-ordering test is renamed and validates notifications for each added vote.

Asynchronous test synchronization

Layer / File(s)	Summary
Pipeline state synchronization module/executiondatasync/optimistic_sync/pipeline/pipeline_test_utils.go	Pipeline test state is stored atomically, state polling prioritizes updates, and a no-error waiting helper is added.
Pipeline transition validation module/executiondatasync/optimistic_sync/pipeline/pipeline_functional_test.go	Functional tests verify error-free transitions before persistence, completion, and cancellation actions.
Spam load-test execution and cleanup engine/common/synchronization/engine_spam_test.go	Load tests pre-register invariant mocks, update quantile ranges, assert per-load message counts, and wait for engine shutdown.
Asynchronous worker and delivery assertions engine/common/stop/stop_control_test.go, network/test/cohort1/network_test.go	Tests wait for asynchronous processed-height updates and message delivery before checking final values.
Serialized inspection processing network/p2p/inspector/validation/control_message_validation_inspector_test.go	The inspection queue uses one worker for deterministic notification assertions.

Deterministic test inputs and assertions

Layer / File(s)	Summary
Unique block-view fixtures engine/common/follower/compliance_engine_test.go, module/builder/collection/builder_test.go, state/cluster/badger/mutator_test.go	Block fixtures now enforce distinct views for finalized, orphaned, or conflicting blocks.
Stable state and cancellation assertions fvm/evm/evm_test.go, storage/migration/validation_test.go	Layout-dependent register checks require non-empty updates, and cancellation cases cancel before channel activity.
Controlled clock execution module/util/log_test.go	The progress test uses testing/synctest and fake-clock sleeping.
Resource override generation network/p2p/builder/libp2pscaler_test.go	Resource-limit overrides are generated as positive values distinct from defaults and sentinel limits.

Estimated code review effort: 4 (Complex) | ~45 minutes

Suggested reviewers: zhangchiqing, peterargue, fxamacker

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: fixing flaky tests across the codebase.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch janez/fix-flaky-unit-tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[coderabbitai]

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@engine/common/follower/compliance_engine_test.go`:
- Line 220: Correct the inline comment on the block.View assignment to state
that newFinalizedBlock.View equals s.finalized.View + 2, distinguishing it from
the child block’s view at s.finalized.View + 1.

In `@engine/common/synchronization/engine_spam_test.go`:
- Around line 350-374: Update the probability-factor comments for each loadGroup
in the synchronization test to use the batch-size formula 0.1 or 0.01 * (n + 1)
/ 64, where n matches the corresponding BlockIDs count (1, 10, 99, or 999/1000
as described). Remove the height-difference expressions and ensure each
comment’s calculated probability matches its batch size before stating the
bounds.

In `@module/executiondatasync/optimistic_sync/pipeline/pipeline_test_utils.go`:
- Around line 96-109: Update waitForStateUpdatesAndNoError to synchronize with
the producer before checking errChan, replacing the non-blocking probe with an
explicit worker-result or phase acknowledgement that confirms pipeline.Run has
reached the relevant completion point. Ensure the helper cannot return before an
immediately produced error is observable, while preserving waitForStateUpdates
behavior for callers that expect concurrent errors.

In `@network/test/cohort1/network_test.go`:
- Around line 550-556: Replace the fixed one-second sleep in the rate-limit test
with deterministic synchronization by calling EnsureNotConnectedBetweenGroups
and EnsureNoStreamCreationBetweenGroups before asserting callCount. Keep the
uint64(2) assertion after both checks so asynchronous pruning and queued
delivery have completed before validating that no third message was delivered.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: dfed4eca-70bc-44f5-9ee9-ccc0146a7b57

📥 Commits

Reviewing files that changed from the base of the PR and between 73f2c26 and fc01b9a.

📒 Files selected for processing (14)

• consensus/hotstuff/votecollector/statemachine_test.go
• engine/common/follower/compliance_engine_test.go
• engine/common/stop/stop_control_test.go
• engine/common/synchronization/engine_spam_test.go
• fvm/evm/evm_test.go
• module/builder/collection/builder_test.go
• module/executiondatasync/optimistic_sync/pipeline/pipeline_functional_test.go
• module/executiondatasync/optimistic_sync/pipeline/pipeline_test_utils.go
• module/util/log_test.go
• network/p2p/builder/libp2pscaler_test.go
• network/p2p/inspector/validation/control_message_validation_inspector_test.go
• network/test/cohort1/network_test.go
• state/cluster/badger/mutator_test.go
• storage/migration/validation_test.go

[Kay-Zee]

LGTM. I went through the full diff and ran everything this touches: all the packages pass go test -race repeatedly on my end (count=5 for the core ones), including the reworked sync spam load tests and the newly enabled TestRegisterVoteConsumer. I also spot-checked the new statistical bounds numerically — all the groups are within 1e-9 per tail of Binomial(1000, p), so the "a correct implementation practically can't flake" claim in the comments checks out.

A few small things, none blocking:

• In the sync load tests, HandleHeight/WithinTolerance are registered without .Maybe(), but they're invoked by the async request workers. AssertExpectations right after the load loop therefore technically depends on a worker having dequeued at least one request by then. In practice that's a non-issue — the test goroutine does a thousand rounds of crypto-RNG and mock matching before it gets there — so I'd leave it. Just mentioning it in case it ever shows up in CI, so nobody has to rediscover the mechanism.
• On the CodeRabbit thread about waitForStateUpdatesAndNoError: I agree with your call to defer. At all three call sites the only thing that can produce an error is the next test action (SetSealed/cancel), so nothing can be sitting on errChan at probe time. The stronger producer-phase synchronization would be nice eventually but isn't buying correctness here.
• The EVM Len(13) → NotEmpty relaxation is a fair trade given the random block ID, and it's disclosed as a known gap with a follow-up planned. Fine by me.

The mock-expectation hoisting out of the load loops is a nice win on its own — quadratic call matching explains a lot about how slow those tests had gotten.

Reviewed in collaboration with an AI agent (kimi).

[Kay-Zee]

Two things I ran into while running this stack — both pre-existing on master, neither caused by these PRs. Passing them along rather than filing, since they're squarely in the territory you've been cleaning up:

1. Real production race in the follower cache. go test -race on TestFollowerHappyPath (untouched by this PR) fires reliably: the compliance engine runs 4 processConnectedBatch workers, which concurrently call follower/cache.(*Cache).Peek. Peek only takes the read lock, but herocache's Get can write — it lazily invalidates stale slots in linkedValueOf (backdata/cache.go:485). Same shape as the RecordCache race you fixed in #8627: a "read" path that secretly mutates. Fix would be Peek taking the write lock (or pushing the invalidation out of the read path). Probably deserves its own issue — happy to file it if you don't want to.

2. TestControlMessageInspection_ValidRpc is independently flaky. GossipSubMessageFixture sets From: RandomBytes(32) per message, and about 0.4% of random 32-byte strings happen to parse as valid peer IDs (I measured 364/100k). When one does, publish-sender validation calls ByPeerID with a peer the mock doesn't expect → unexpected-call failure. Works out to roughly a 1-in-28 failure rate per run; I hit it twice while testing this stack. Good candidate for the next round of flaky-test fixes — giving the fixture a valid peer ID should kill it.

One side effect worth knowing: when that mock failure fires inside an inspector worker goroutine, the FailNow/Goexit kills the worker and can hang later tests in the same binary. That's what produced the scary-looking 10-minute timeout in TestControlMessageValidationInspector_TruncationConfigToggle in my run — it passes 3/3 in isolation. So a single ~4% flake can take down a whole package run and look much worse than it is.

Written in collaboration with an AI agent (kimi).