Skip to content

execution/commitment: fix flaky BranchCache unwind tests - #22272

Closed
yperbasis wants to merge 4 commits into
mainfrom
yperbasis/fix-branchcache-unwind-test-flake
Closed

yperbasis wants to merge 4 commits into
mainfrom
yperbasis/fix-branchcache-unwind-test-flake

Conversation

@yperbasis

Copy link
Copy Markdown
Member

Problem

TestBranchCache_Unwind_DropsStaleAboveFloorLazily and TestBranchCache_Unwind_FloorBoundary are flaky. They failed on the race-tests / tests-linux (ubuntu-latest, execution-other, parallel) job of a merge-queue run, which failed ci-gate and dequeued PR #22264 (an unrelated db/state change that happened to share the batch).

--- FAIL: TestBranchCache_Unwind_DropsStaleAboveFloorLazily   branch_cache_test.go:173  "Should be true"
--- FAIL: TestBranchCache_Unwind_FloorBoundary                branch_cache_test.go:211  "Should be true"

Root cause

Both tests built the cache with NewBranchCache(100). The tail is a ShardedLRU split into branchCacheTailShards buckets, so a nominal capacity of 100 leaves most shards room for a single entry (per-shard cap 1–2). Both tests use the same two keys {0xa0,0xb0} and {0xa0,0xb1}. When the per-process maphash seed lands those two keys in the same capacity-1 shard, the second Put evicts the first and the require.True(ok, "…must survive…") assertion fails.

maphash is seeded per process (and mixes the runtime's per-process hash key), so the collision is non-deterministic across runs — which is why the tests usually pass locally and only occasionally fail in CI, and why both fail together (same key pair, same unlucky seed).

Verified by forcing a colliding seed in-process: with cap=100 the survivor is evicted; with DefaultBranchCacheTailCapacity it is retained.

Fix

Size the tail via DefaultBranchCacheTailCapacity (per-shard cap ≈ 195), as the sibling TestBranchCache_ShardedTailUnwindAcrossShards already does, so LRU eviction can't interfere with the txN/epoch invalidation these tests actually exercise. Capacity does not affect the unwind assertions themselves, so the tested behavior is unchanged.

The single-key unwind tests (AcrossAllTiers, CurrentEpochSurvives, FrozenSurvives) are not affected — one key always fits in its shard — so they're left as-is.

Testing

  • go test ./execution/commitment/ -run TestBranchCache -count=1 passes.
  • Reproduced the eviction deterministically under a forced colliding seed and confirmed the default capacity fixes it.

Not TDD-first: this is a flaky-test fix where the failure is a per-process probabilistic collision, not a deterministic behavior change; the mechanism was reproduced by pinning a colliding seed in-process.

TestBranchCache_Unwind_DropsStaleAboveFloorLazily and
TestBranchCache_Unwind_FloorBoundary built the cache with NewBranchCache(100).
The tail is sharded into branchCacheTailShards buckets, so a nominal capacity of
100 leaves most shards room for a single entry. Both tests use the two keys
{0xa0,0xb0} and {0xa0,0xb1}; when the per-process maphash seed lands them in the
same capacity-1 shard, the second Put evicts the first and the "must survive"
assertion fails. maphash is seeded per process, so the failure is
non-deterministic — it surfaced on a merge-queue race run and dequeued a PR.

Size the tail via DefaultBranchCacheTailCapacity (as the sibling
TestBranchCache_ShardedTailUnwindAcrossShards already does) so per-shard
capacity can't evict the keys under test.
@yperbasis
yperbasis requested a review from Copilot July 6, 2026 15:06
@yperbasis
yperbasis marked this pull request as ready for review July 6, 2026 15:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This pull request fixes flakiness in BranchCache unwind tests caused by probabilistic shard collisions in the sharded LRU tail when using a small nominal capacity, which can lead to unexpected evictions depending on the per-process hash seed.

Changes:

  • Introduces a dedicated helper (newUnwindTestCache) to construct a BranchCache with DefaultBranchCacheTailCapacity for unwind-focused tests.
  • Updates the two flaky unwind tests to use the helper instead of NewBranchCache(100), preventing LRU eviction from affecting the assertions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@yperbasis
yperbasis enabled auto-merge July 6, 2026 15:08
@yperbasis
yperbasis requested a review from sudeepdino008 July 7, 2026 10:19
@yperbasis
yperbasis requested a review from AskAlexSharov July 7, 2026 18:42
@awskii

awskii commented Jul 8, 2026

Copy link
Copy Markdown
Member

On the current branch (after the #22154 merge) the tail is freelru v0.16.0, and NewBranchCache(100) does not produce the per-shard-cap-1 layout the description assumes.

newTailShards(100) calls NewShardedWithSize(256, 100, 125). freelru rounds the size up to 128, then collapses shards via for shards > size/16 { shards /= 16 } (size/16 = 8): 256 → 16 → 1. So the tail is a single LRU of capacity 100. Verified by probe: two distinct keys forced into the same shard both survive, and the tail saturates at Len=100.

At cap=100 the two test keys are 2 tail entries in one 100-slot shard, so neither can evict the other under any maphash seed — the seed-dependent collision can't occur. DefaultBranchCacheTailCapacity gives start cap 512 → NewShardedWithSize(256, 512, 640) → 16 shards × 32, also non-evicting. The change is safe but a no-op for the described mechanism.

Timeline: the fix commit (07-06) predates #22154 (07-07), which replaced the tail with freelru. Before #22154 the tail sharded 256-ways at cap 100 (per-shard cap 1), so the original analysis held then. #22154 is the merge-base here, so on the merged branch the flake is already neutralized.

Not a bug — a larger capacity is strictly safe, and keeping it as defensive hygiene (it matches TestBranchCache_ShardedTailUnwindAcrossShards) is reasonable. But the "per-shard cap 1–2" root cause no longer matches freelru v0.16.0. Worth confirming the two tests still flake on current main before merging this as the fix, and updating the description if they don't.

@awskii

awskii commented Jul 8, 2026

Copy link
Copy Markdown
Member

#22154 already made these two tests deterministic. The fix is a no-op on current main, so there's nothing left to fix.

@yperbasis yperbasis closed this Jul 8, 2026
auto-merge was automatically disabled July 8, 2026 07:47

Pull request was closed

@awskii
awskii deleted the yperbasis/fix-branchcache-unwind-test-flake branch July 31, 2026 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants