Skip to content

[Testing] Fix flaky unit tests - #8634

Open
janezpodhostnik wants to merge 6 commits into
janez/fix-flaky-tests-production-racesfrom
janez/fix-flaky-unit-tests-batch-2
Open

[Testing] Fix flaky unit tests#8634
janezpodhostnik wants to merge 6 commits into
janez/fix-flaky-tests-production-racesfrom
janez/fix-flaky-unit-tests-batch-2

Conversation

@janezpodhostnik

@janezpodhostnik janezpodhostnik commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Test-only fixes for 40+ flaky unit tests found by repeatedly running the full suite (500+ runs total; failure rate went from 13/20 runs on master to 2/100 with these fixes and #8633). Continues #8626. No code changes. Bugs found by the same campaign are fixed separately in #8633.

Changes

  • Fix async registration and teardown races: register mock expectations and WaitGroups before triggering the code path that uses them, and join spawned goroutines before test completion (execution ingestion, access ingestion2, state stream handlers, epochmgr, indexer, worker pool, hotstuff integration harness, alsp manager).
  • Tolerate benign async calls in strict mocks: periodic metrics and polling loops, gRPC Serve/Stop shutdown races, in-flight unicasts arriving during network teardown, and per-height collection lookups whose call counts are timing-dependent (pruner, connection gater, rpc connection, transaction streaming, cluster-prefix inspector, cohort network tests).
  • Relax too-tight real-time bounds: 10ms-500ms Ready/Done/Eventually windows raised to generous bounds that only limit the failure case (follower cache, matching, sealing, forest, jobqueue, meshengine, websockets, scoring registry).
  • Remove low-probability randomness failures: 1/256 key and uuid-partition collisions and 1/n! shuffle identity permutations replaced with deterministic constructions or partition-aware expected values (merkle proofs, epoch setup, EVM computation metering, allocation profiler).
  • Make shared test utilities robust: temp-dir removal retries when a deliberately abandoned background write races cleanup (WAL compactor), and the network test fixture tolerates the specific unicast-without-subscription violation that in-flight messages produce during teardown.
  • Fix test-code data races found while verifying with -race (echo engine map access, transaction stream suite block pointers, shared gRPC serve error variable).

Each fix verified with 20-100x stress runs of the affected test, most with -race, plus full-package runs and lint.

Related: #8626, #8633


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

@janezpodhostnik
janezpodhostnik requested a review from a team as a code owner August 3, 2026 13:56
@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: bfd13293-3069-45f5-941e-7e84c594e53a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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

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

…-races' into janez/fix-flaky-unit-tests-batch-2
@claude

claude Bot commented Aug 7, 2026

Copy link
Copy Markdown

No concerns from this review — 2 nits, nothing blocking.

The only fvm/** change in this PR is the 11-line test-only edit at fvm/evm/evm_test.go:4108-4117. No production FVM code is touched, so none of the usual FVM risk classes apply: no metering-scope change, no error-taxonomy change, no HCU-relevant behavior change, and nothing affecting execution results or SPoCK.

Verified correct (subtle bits worth recording)

  • The partition derivation matches production exactly. sha256.Sum256(blockID[:])[0] == 0 is precisely environment.uuidPartition(blockID, txnIndex) (fvm/environment/uuids.go:100-101) specialized to txnIndex == 0, and this subtest does run at index 0 (fvm/evm/evm_test.go:4103). The index reaches the generator unchanged via params.TxIndex (fvm/environment/facade_env.go:130). If the subtest ever moves to a non-zero index the branch silently becomes wrong, but as written it is right.
  • Same header on both sides. ctx.BlockHeader is the header the generator hashes — RunWithNewEnvironment sets fvm.WithBlockHeader(block1.ToHeader()) — so the test cannot hash a different block than execution used.
  • No determinism concern. The partition is a pure function of the block ID, so execution stays deterministic per block. The randomness being papered over lives entirely in unittest.BlockFixture(), i.e. in the test harness.
  • The neighbouring exact-computation assertions are not latently flaky for the same reason. fvm/evm/evm_test.go:4003, :4023, :4640 and :4662 all assert on dryRun/dryCall-only transactions, which do not mutate EVM state and therefore generate no UUIDs. Only this subtest calls EVM.run, which is why it alone is partition-sensitive. Scoping the fix to this one assertion is correct.

One caveat on my own verification: this review job cannot execute tests, so I could not independently reproduce the 79 value. The reasoning above establishes that the branch condition is right; the constant rests on the author's forced-partition run.

Nits

  1. fvm/evm/evm_test.go:4114 — the 79 branch is effectively unreachable in CI (p = 1/256), so a wrong or stale constant there would go undetected. Concretely: if 79 is off by one, the test passes ~99.6% of runs and reintroduces exactly the flake this PR is fixing, at a rate low enough that it would be months before anyone correlates it. The stronger fix is to remove the randomness rather than branch on it — pin a deterministic block header for this subtest (or add a RunWithNewEnvironment variant that takes one) and assert a single exact value. That also makes the number reviewable rather than trust-based.
  2. The partition-0 test is now duplicated. assertUpdatedRegisterCount (fvm/evm/evm_test.go:7207) and the new inline check both open-code sha256.Sum256(blockID[:])[0] == 0, and each independently bakes in the txnIndex == 0 assumption. Since environment.uuidPartition is unexported, a small shared test helper (e.g. usesLegacyUUIDPartition(ctx)) would state that assumption once and give a single place to update if the partitioning function ever changes.

Pre-existing (informational, not this PR's problem)

The same transaction costs ~5 more computation units when it lands in a block whose ID selects UUID partition 0, because that partition reuses the legacy uuid register instead of a fresh uuid_N one (model/flow/ledger.go:45-56). This is deterministic per block, so it is not a consensus concern — but it does mean identical transactions are metered slightly differently depending on which block includes them. Inherent to the partitioning design; noting it only because this PR is the first place it is written down explicitly.

@janezpodhostnik

Copy link
Copy Markdown
Contributor Author

@claude comments addressed in #8637

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