Skip to content

[Testing] Fix data race in cluster-prefix RecordCache - #8627

Open
janezpodhostnik wants to merge 1 commit into
masterfrom
janez/flaky-test-followup-coverage
Open

[Testing] Fix data race in cluster-prefix RecordCache#8627
janezpodhostnik wants to merge 1 commit into
masterfrom
janez/flaky-test-followup-coverage

Conversation

@janezpodhostnik

@janezpodhostnik janezpodhostnik commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #8626, which fixed flaky tests by (among other things) weakening some
assertions. This PR restores that coverage — and fixes a production data race the new
coverage immediately exposed.

Data race fix in RecordCache (production code)

network/p2p/inspector/internal/cache/cache.go: ReceivedClusterPrefixedMessage and
GetWithInit read .Gauge from the record pointer returned by AdjustWithInit after
the backend lock is released. Records are mutated in place, so this read races with
concurrent adjustments of the same record. The inspector runs 5 inspection workers by
default, so this race is live on production nodes; #8626 pinning the cluster-prefix test
to a single worker had hidden it from the test suite.

Fix: capture the gauge inside the locked adjust closure instead of dereferencing the
returned pointer.

New/extended tests

  • network/p2p/inspector/validation: new multi-worker companion subtest of
    TestNewControlMsgValidationInspector_validateClusterPrefixedTopic — with the default
    worker count, between 1 and one-per-message notifications may be disseminated past the
    hard threshold (the tracker increment and threshold check are not atomic). This is the
    test that surfaced the race above.
  • consensus/hotstuff/votecollector: new
    TestRegisterVoteConsumer_CachingToVerifyingTransition — a vote consumer keeps
    receiving votes, in arrival order, across the caching→verifying transition.
  • network/p2p/builder: negative-value cases in TestApplyResourceLimitOverride and new
    TestBuildLibp2pResourceManagerLimits_NonPositiveOverridesKeepDefaults — non-positive
    overrides keep the scaled defaults and are never passed through to rcmgr, where negative
    values carry special meaning (-1 unlimited, -2 block-all).

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


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 July 29, 2026 14:14
@janezpodhostnik janezpodhostnik changed the title Fix data race in cluster-prefix RecordCache; add follow-up test coverage [Testing] Fix data race in cluster-prefix RecordCache Jul 29, 2026
@janezpodhostnik janezpodhostnik self-assigned this Jul 29, 2026
@coderabbitai

coderabbitai Bot commented Jul 29, 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: 39441ba9-cc74-4034-9108-080c1c5d1d4f

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.

@janezpodhostnik
janezpodhostnik requested a review from a team July 30, 2026 10:31
Base automatically changed from janez/fix-flaky-unit-tests to master July 31, 2026 18:22
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.

3 participants