Fix flaky test_maximum_full_file_count by eliminating race condition#20704
Open
TheLastCicada wants to merge 1 commit intomainfrom
Open
Fix flaky test_maximum_full_file_count by eliminating race condition#20704TheLastCicada wants to merge 1 commit intomainfrom
TheLastCicada wants to merge 1 commit intomainfrom
Conversation
The test fails intermittently on macOS CI because farm_block_with_spend polls the mempool with a blind timeout. When the wallet builds a spend against stale singleton state, the full node rejects it (INVALID_SPEND_BUNDLE, status 3), the mempool never reaches count 1, and the test burns 30 seconds before failing with a misleading timeout. Three fixes: 1. Add check_mempool_spend_count_or_fail() that inspects the transaction's sent_to field on each poll. If any entry has MempoolInclusionStatus.FAILED, raise immediately with the rejection reason instead of waiting for the timeout. 2. Add a singleton readiness check (check_singleton_confirmed) after farm_block_with_spend in the test loop, ensuring the DataLayer wallet has processed the previous block's singleton update before the next batch_update builds a new spend. 3. Change check_mempool_spend_count to use >= instead of == so auto-resends or ancillary transactions don't break the check. Made-with: Cursor
Pull Request Test Coverage Report for Build 23361347167Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Contributor
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose:
Eliminate a race condition that causes
test_maximum_full_file_countto fail intermittently on macOS CI with a misleading 30-second timeout error.Current Behavior:
The test loop (9 iterations of
batch_update→farm_block_with_spend) has a race condition. After farming a block and waiting for wallet sync, the DataLayer wallet's internal singleton state may not have caught up before the nextbatch_updatebuilds a new spend. The wallet creates a spend against stale singleton state → full node rejects it (INVALID_SPEND_BUNDLE, status 3) →check_mempool_spend_countpolls until timeout → test fails after 30 seconds withAssertionError: Timed assertion timed out.New Behavior:
Three fixes:
Fail-fast on spend rejection: New
check_mempool_spend_count_or_fail()helper inspects the transaction'ssent_tofield on each poll iteration. If any entry hasMempoolInclusionStatus.FAILED, it raises aRuntimeErrorimmediately with the rejection reason instead of burning 30 seconds on a doomed timeout.Singleton readiness check: Added
time_out_assert(10, check_singleton_confirmed, True, data_layer, store_id)afterfarm_block_with_spendin the test loop. This ensures the DataLayer wallet has processed the previous block's singleton update before the next iteration callsbatch_update, closing the race condition. (farm_block_check_singletonalready does this;farm_block_with_spenddid not.)Relaxed mempool count check: Changed
check_mempool_spend_countfrom==to>=so auto-resends or ancillary transactions adding extra mempool entries don't break the check.The
farm_block_with_spendfunction signature is unchanged — all ~40 existing callers are unaffected.Testing Notes:
test_maximum_full_file_countpass locally (611s total)Made with Cursor
Note
Low Risk
Low risk: changes are isolated to test helpers and assertions, primarily improving synchronization and error reporting for flaky CI behavior.
Overview
Fixes flakiness in
test_maximum_full_file_countby ensuring eachbatch_updatespend is fully processed before the next iteration runs.The test now fails fast when a transaction is rejected (via
check_mempool_spend_count_or_failinspectingsent_toforMempoolInclusionStatus.FAILED), relaxes mempool size checks from==to>=, and adds an explicitcheck_singleton_confirmedwait afterfarm_block_with_spendin the update loop to avoid racing on stale singleton state.Written by Cursor Bugbot for commit 0c9b1bb. This will update automatically on new commits. Configure here.