Skip to content

test: deterministic fixes for two flaky unit tests (#1024)#1026

Merged
oskarszoon merged 3 commits into
bsv-blockchain:mainfrom
oskarszoon:fix/1024-flaky-tests
Jun 4, 2026
Merged

test: deterministic fixes for two flaky unit tests (#1024)#1026
oskarszoon merged 3 commits into
bsv-blockchain:mainfrom
oskarszoon:fix/1024-flaky-tests

Conversation

@oskarszoon

Copy link
Copy Markdown
Contributor

Closes #1024.

Two unit tests flaked under full-suite CI load; both pass in isolation. Test-only fix — no product code changes (git diff touches only *_test.go).

Root causes

services/validatorTestValidateTransactionBatch_DuplicateOutpointCreatesConflicting (expected 36, got 4)
ValidateTransactionBatch validates the two txs concurrently; both spend the same outpoint through one shared sqlitememory store. The sqlitememory DSN (util/sql.go:218) sets no busy_timeout and leaves the pool unbounded, so under CI CPU starvation concurrent writes hit "database is locked"; the spend batch's 3 short retries exhaust → StorageError, and the validator collapses any non-UTXO spend error into ERR_PROCESSING (Validator.go:723) instead of the conflict path (ERR_TX_CONFLICTING). The #1017 batcher attribution in the issue is at most a contributing timing factor — the root vulnerability predates it.

services/legacy/netsyncTestSyncManager_createUtxos_ChunkFailureCancelsSiblings (4 not <= 1)
Racy by construction, unrelated to #1017 (no batcher on this path). errgroup cancels mergeCtx only after the failing worker returns, so the surviving worker keeps passing its loop-top mergeCtx.Err() check and draining iterations until cancellation propagates. The product short-circuit is correct; the test's "≤1 in flight" assumption is not.

Fixes (test-only)

  • validator: utxoStore.RawDB().SetMaxOpenConns(1) — serialise the dev-mode SQLite pool so concurrent batch validations can't surface a transient lock. The concurrent batch path is still exercised; only DB access serialises. Production uses Aerospike.
  • netsync: replace the 2-worker race with a deterministic single-worker test. The first SetMinedMulti cancels the parent ctx and returns success; because one worker runs sequentially, iteration 2's loop-top check always observes the cancel. Asserts zero post-trigger calls. Removing the short-circuit fails the count assertion.

Out of scope (flagged follow-ups, not bundled)

  • Validator.go:723 collapses transient/storage spend errors into ERR_PROCESSING, discarding conflict semantics — worth a separate issue.
  • sqlitememory has no busy_timeout and an unbounded pool; other concurrent SQL-backed tests share the same latent flakiness.

Test plan

  • go test -race -count=50 each test individually — pass
  • netsync mutation check: removing the mergeCtx.Err() short-circuit fails the test at the count assertion (expected 0, got 1)
  • validator flake reproduced on base under -race -count=80 -cpu=8 (got 4); fix holds 360 iterations
  • go test -race ./services/legacy/netsync/ ./services/validator/ — both green
  • go vet clean

@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

🤖 Claude Code Review

Status: Complete

Current Review:
No issues found. Test-only changes are sound and properly address the root causes of CI flakiness.

Summary:

  • netsync fix: Converted 2-worker racy test to deterministic single-worker design. The test correctly validates the mergeCtx.Err() short-circuit at handle_block.go:990. The new approach eliminates timing dependencies.
  • validator fix: SetMaxOpenConns(1) serializes SQLite access to prevent "database is locked" transient errors. The fix is minimal, production uses Aerospike (unaffected), and the concurrent validation path remains exercised.
  • Documentation: In-test comments accurately describe root causes and mutation-testing methodology. The PR description's "Out of scope" section appropriately flags follow-up work without scope creep.

@oskarszoon oskarszoon requested review from ordishs and sugh01 June 2, 2026 20:18
@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Benchmark Comparison Report

Baseline: main (unknown)

Current: PR-1026 (c2bfb60)

Summary

  • Regressions: 0
  • Improvements: 0
  • Unchanged: 144
  • Significance level: p < 0.05
All benchmark results (sec/op)
Benchmark Baseline Current Change p-value
_NewBlockFromBytes-4 1.727µ 1.729µ ~ 1.000
SplitSyncedParentMap_SetIfNotExists/256_buckets-4 59.27n 59.87n ~ 0.200
SplitSyncedParentMap_SetIfNotExists/16_buckets-4 59.21n 59.28n ~ 0.700
SplitSyncedParentMap_SetIfNotExists/1_bucket-4 59.46n 59.19n ~ 0.400
SplitSyncedParentMap_ConcurrentSetIfNotExists/256_buckets... 34.28n 33.49n ~ 0.200
SplitSyncedParentMap_ConcurrentSetIfNotExists/16_buckets_... 57.28n 58.23n ~ 0.100
SplitSyncedParentMap_ConcurrentSetIfNotExists/1_bucket_pa... 148.4n 152.5n ~ 0.100
MiningCandidate_Stringify_Short-4 248.0n 248.5n ~ 0.100
MiningCandidate_Stringify_Long-4 1.760µ 1.761µ ~ 1.000
MiningSolution_Stringify-4 905.2n 900.2n ~ 0.100
BlockInfo_MarshalJSON-4 1.724µ 1.726µ ~ 0.400
NewFromBytes-4 132.7n 133.1n ~ 0.800
AddTxBatchColumnar_Validation-4 2.637µ 2.599µ ~ 0.200
OffsetValidationLoop-4 598.4n 596.7n ~ 1.000
Mine_EasyDifficulty-4 61.37µ 61.42µ ~ 1.000
Mine_WithAddress-4 7.512µ 7.074µ ~ 0.400
BlockAssembler_AddTx-4 0.02770n 0.02851n ~ 0.700
AddNode-4 10.61 10.54 ~ 1.000
AddNodeWithMap-4 11.04 11.43 ~ 0.100
DiskTxMap_SetIfNotExists-4 3.652µ 3.620µ ~ 1.000
DiskTxMap_SetIfNotExists_Parallel-4 3.413µ 3.366µ ~ 1.000
DiskTxMap_ExistenceOnly-4 322.9n 329.6n ~ 0.700
Queue-4 190.6n 190.0n ~ 0.800
AtomicPointer-4 4.369n 4.626n ~ 0.400
ReorgOptimizations/DedupFilterPipeline/Old/10K-4 863.9µ 912.1µ ~ 0.400
ReorgOptimizations/DedupFilterPipeline/New/10K-4 825.7µ 811.1µ ~ 0.400
ReorgOptimizations/AllMarkFalse/Old/10K-4 108.1µ 120.0µ ~ 0.100
ReorgOptimizations/AllMarkFalse/New/10K-4 62.41µ 62.51µ ~ 0.700
ReorgOptimizations/HashSlicePool/Old/10K-4 62.29µ 69.98µ ~ 0.100
ReorgOptimizations/HashSlicePool/New/10K-4 12.12µ 12.25µ ~ 0.700
ReorgOptimizations/NodeFlags/Old/10K-4 4.757µ 5.451µ ~ 0.200
ReorgOptimizations/NodeFlags/New/10K-4 1.622µ 1.634µ ~ 0.400
ReorgOptimizations/DedupFilterPipeline/Old/100K-4 9.628m 9.601m ~ 1.000
ReorgOptimizations/DedupFilterPipeline/New/100K-4 10.653m 9.856m ~ 0.100
ReorgOptimizations/AllMarkFalse/Old/100K-4 1.109m 1.148m ~ 0.100
ReorgOptimizations/AllMarkFalse/New/100K-4 688.5µ 687.4µ ~ 0.400
ReorgOptimizations/HashSlicePool/Old/100K-4 670.5µ 613.3µ ~ 0.100
ReorgOptimizations/HashSlicePool/New/100K-4 308.4µ 311.0µ ~ 0.700
ReorgOptimizations/NodeFlags/Old/100K-4 53.32µ 51.70µ ~ 0.400
ReorgOptimizations/NodeFlags/New/100K-4 18.63µ 18.12µ ~ 0.100
TxMapSetIfNotExists-4 53.10n 53.08n ~ 1.000
TxMapSetIfNotExistsDuplicate-4 39.88n 39.98n ~ 0.300
ChannelSendReceive-4 598.7n 607.2n ~ 0.200
DirectSubtreeAdd/4_per_subtree-4 56.11n 60.40n ~ 0.100
DirectSubtreeAdd/64_per_subtree-4 29.28n 28.98n ~ 0.100
DirectSubtreeAdd/256_per_subtree-4 28.23n 27.81n ~ 0.700
DirectSubtreeAdd/1024_per_subtree-4 26.51n 26.50n ~ 1.000
DirectSubtreeAdd/2048_per_subtree-4 26.16n 26.15n ~ 1.000
SubtreeProcessorAdd/4_per_subtree-4 313.1n 304.0n ~ 0.700
SubtreeProcessorAdd/64_per_subtree-4 296.0n 295.5n ~ 0.800
SubtreeProcessorAdd/256_per_subtree-4 292.3n 302.8n ~ 0.200
SubtreeProcessorAdd/1024_per_subtree-4 287.9n 290.4n ~ 0.400
SubtreeProcessorAdd/2048_per_subtree-4 290.4n 294.9n ~ 0.500
SubtreeProcessorRotate/4_per_subtree-4 288.7n 292.1n ~ 0.700
SubtreeProcessorRotate/64_per_subtree-4 289.1n 285.8n ~ 0.400
SubtreeProcessorRotate/256_per_subtree-4 288.9n 284.9n ~ 0.700
SubtreeProcessorRotate/1024_per_subtree-4 291.5n 298.1n ~ 0.400
SubtreeNodeAddOnly/4_per_subtree-4 55.53n 55.14n ~ 0.200
SubtreeNodeAddOnly/64_per_subtree-4 36.16n 36.08n ~ 0.200
SubtreeNodeAddOnly/256_per_subtree-4 35.07n 35.13n ~ 0.800
SubtreeNodeAddOnly/1024_per_subtree-4 34.47n 34.41n ~ 0.600
SubtreeCreationOnly/4_per_subtree-4 111.7n 110.7n ~ 1.000
SubtreeCreationOnly/64_per_subtree-4 355.4n 347.9n ~ 0.100
SubtreeCreationOnly/256_per_subtree-4 1.254µ 1.228µ ~ 0.100
SubtreeCreationOnly/1024_per_subtree-4 3.864µ 3.854µ ~ 0.200
SubtreeCreationOnly/2048_per_subtree-4 6.996µ 6.791µ ~ 0.100
SubtreeProcessorOverheadBreakdown/64_per_subtree-4 317.5n 294.0n ~ 0.100
SubtreeProcessorOverheadBreakdown/1024_per_subtree-4 314.5n 284.0n ~ 0.100
ParallelGetAndSetIfNotExists/1k_nodes-4 2.018m 2.023m ~ 1.000
ParallelGetAndSetIfNotExists/10k_nodes-4 5.286m 5.322m ~ 0.700
ParallelGetAndSetIfNotExists/50k_nodes-4 7.704m 7.257m ~ 0.100
ParallelGetAndSetIfNotExists/100k_nodes-4 10.058m 9.787m ~ 0.100
SequentialGetAndSetIfNotExists/1k_nodes-4 1.772m 1.779m ~ 0.700
SequentialGetAndSetIfNotExists/10k_nodes-4 4.462m 4.501m ~ 1.000
SequentialGetAndSetIfNotExists/50k_nodes-4 14.05m 13.74m ~ 0.400
SequentialGetAndSetIfNotExists/100k_nodes-4 26.52m 25.25m ~ 0.100
ProcessOwnBlockSubtreeNodesParallel/1k_nodes-4 2.045m 2.061m ~ 0.400
ProcessOwnBlockSubtreeNodesParallel/10k_nodes-4 8.325m 8.416m ~ 0.700
ProcessOwnBlockSubtreeNodesParallel/100k_nodes-4 13.27m 13.42m ~ 0.700
ProcessOwnBlockSubtreeNodesSequential/1k_nodes-4 1.794m 1.814m ~ 0.400
ProcessOwnBlockSubtreeNodesSequential/10k_nodes-4 7.993m 8.376m ~ 0.100
ProcessOwnBlockSubtreeNodesSequential/100k_nodes-4 43.75m 44.86m ~ 0.100
CalcBlockWork-4 509.9n 511.7n ~ 0.400
CalculateWork-4 705.8n 703.4n ~ 1.000
BuildBlockLocatorString_Helpers/Size_10-4 1.660µ 1.528µ ~ 1.000
BuildBlockLocatorString_Helpers/Size_100-4 12.85µ 12.93µ ~ 1.000
BuildBlockLocatorString_Helpers/Size_1000-4 126.3µ 126.7µ ~ 0.100
CatchupWithHeaderCache-4 104.2m 104.1m ~ 0.400
SubtreeSizes/10k_tx_4_per_subtree-4 1.343m 1.356m ~ 0.700
SubtreeSizes/10k_tx_16_per_subtree-4 319.9µ 323.3µ ~ 0.700
SubtreeSizes/10k_tx_64_per_subtree-4 76.48µ 76.69µ ~ 1.000
SubtreeSizes/10k_tx_256_per_subtree-4 19.02µ 19.18µ ~ 0.100
SubtreeSizes/10k_tx_512_per_subtree-4 9.565µ 9.636µ ~ 0.400
SubtreeSizes/10k_tx_1024_per_subtree-4 4.667µ 4.797µ ~ 0.100
SubtreeSizes/10k_tx_2k_per_subtree-4 2.363µ 2.404µ ~ 0.100
BlockSizeScaling/10k_tx_64_per_subtree-4 76.23µ 75.53µ ~ 0.400
BlockSizeScaling/10k_tx_256_per_subtree-4 19.31µ 19.39µ ~ 0.400
BlockSizeScaling/10k_tx_1024_per_subtree-4 4.762µ 4.764µ ~ 1.000
BlockSizeScaling/50k_tx_64_per_subtree-4 401.2µ 391.9µ ~ 0.700
BlockSizeScaling/50k_tx_256_per_subtree-4 96.03µ 95.16µ ~ 0.400
BlockSizeScaling/50k_tx_1024_per_subtree-4 23.67µ 23.57µ ~ 0.700
SubtreeAllocations/small_subtrees_exists_check-4 160.5µ 157.8µ ~ 0.200
SubtreeAllocations/small_subtrees_data_fetch-4 171.2µ 171.2µ ~ 1.000
SubtreeAllocations/small_subtrees_full_validation-4 331.0µ 329.8µ ~ 0.700
SubtreeAllocations/medium_subtrees_exists_check-4 9.285µ 9.345µ ~ 1.000
SubtreeAllocations/medium_subtrees_data_fetch-4 10.15µ 10.18µ ~ 0.700
SubtreeAllocations/medium_subtrees_full_validation-4 19.37µ 19.40µ ~ 1.000
SubtreeAllocations/large_subtrees_exists_check-4 2.246µ 2.214µ ~ 0.700
SubtreeAllocations/large_subtrees_data_fetch-4 2.467µ 2.465µ ~ 1.000
SubtreeAllocations/large_subtrees_full_validation-4 4.906µ 4.827µ ~ 0.100
_BufferPoolAllocation/16KB-4 3.875µ 5.533µ ~ 0.200
_BufferPoolAllocation/32KB-4 10.708µ 8.303µ ~ 0.100
_BufferPoolAllocation/64KB-4 18.39µ 16.61µ ~ 0.100
_BufferPoolAllocation/128KB-4 36.84µ 34.23µ ~ 0.100
_BufferPoolAllocation/512KB-4 135.4µ 124.6µ ~ 0.700
_BufferPoolConcurrent/32KB-4 22.55µ 20.22µ ~ 0.100
_BufferPoolConcurrent/64KB-4 30.61µ 32.50µ ~ 0.700
_BufferPoolConcurrent/512KB-4 148.2µ 152.9µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/16KB-4 720.4µ 687.6µ ~ 0.700
_SubtreeDeserializationWithBufferSizes/32KB-4 713.6µ 689.3µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/64KB-4 710.5µ 668.8µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/128KB-4 715.4µ 683.8µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/512KB-4 620.2µ 634.3µ ~ 0.700
_SubtreeDataDeserializationWithBufferSizes/16KB-4 37.22m 36.96m ~ 0.700
_SubtreeDataDeserializationWithBufferSizes/32KB-4 36.64m 36.86m ~ 0.700
_SubtreeDataDeserializationWithBufferSizes/64KB-4 36.65m 36.85m ~ 0.700
_SubtreeDataDeserializationWithBufferSizes/128KB-4 36.45m 36.95m ~ 0.100
_SubtreeDataDeserializationWithBufferSizes/512KB-4 36.69m 36.82m ~ 1.000
_PooledVsNonPooled/Pooled-4 832.4n 675.6n ~ 0.100
_PooledVsNonPooled/NonPooled-4 8.800µ 8.245µ ~ 0.100
_MemoryFootprint/Current_512KB_32concurrent-4 7.290µ 7.104µ ~ 0.100
_MemoryFootprint/Proposed_32KB_32concurrent-4 9.369µ 10.353µ ~ 0.100
_MemoryFootprint/Alternative_64KB_32concurrent-4 9.106µ 10.058µ ~ 0.100
_prepareTxsPerLevel-4 398.4m 405.1m ~ 0.700
_prepareTxsPerLevelOrdered-4 3.535m 3.950m ~ 0.400
_prepareTxsPerLevel_Comparison/Original-4 396.8m 398.2m ~ 0.400
_prepareTxsPerLevel_Comparison/Optimized-4 3.502m 3.583m ~ 0.100
StoreBlock_Sequential/BelowCSVHeight-4 340.7µ 331.5µ ~ 0.100
StoreBlock_Sequential/AboveCSVHeight-4 334.0µ 336.8µ ~ 0.100
GetUtxoHashes-4 281.4n 284.5n ~ 1.000
GetUtxoHashes_ManyOutputs-4 50.86µ 46.02µ ~ 0.100
_NewMetaDataFromBytes-4 213.8n 214.5n ~ 0.700
_Bytes-4 397.7n 400.9n ~ 0.700
_MetaBytes-4 139.3n 140.0n ~ 0.700

Threshold: >10% with p < 0.05 | Generated: 2026-06-03 07:13 UTC

@ordishs ordishs left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — test-only change, well-reasoned, and verified independently:

  • go test -race -count=10 on both tests: green
  • Mutation check (remove the mergeCtx.Err() short-circuit): netsync test correctly fails with expected 0, got 1, confirming it guards the regression
  • Diff scope is test-only; root causes (Validator.go:723 collapsing spend errors into ERR_PROCESSING, and sqlitememory lacking busy_timeout/bounded pool) are appropriately flagged out of scope — please file those as follow-ups.

One thing to fix before merge — linting:

golangci-lint fails on the doc comment:

services/legacy/netsync/handle_block_test.go:1333:1: File is not properly formatted (gci)

The space-indented continuation lines in the Short-circuit intact: / Short-circuit removed: comment block (~lines 1330-1336) get reinterpreted by gofmt as code blocks. gofmt -w (or gofumpt) on that file resolves it — easiest fix is to let the formatter rewrite those lines, or de-indent the continuations so they read as plain prose. Once gofmt -l reports clean, CI lint will pass.

@sonarqubecloud

sonarqubecloud Bot commented Jun 3, 2026

Copy link
Copy Markdown

@oskarszoon oskarszoon merged commit 1bd1c97 into bsv-blockchain:main Jun 4, 2026
34 checks passed
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.

Flaky unit tests under full-suite CI load: validator DuplicateOutpoint + netsync ChunkFailureCancelsSiblings

3 participants