kv: fix the flaky two-sample ceiling in the TSO shadow migration test - #1236
kv: fix the flaky two-sample ceiling in the TSO shadow migration test#1236bootjp wants to merge 1 commit into
Conversation
TestShadowAndCutoverAllocatorsSerializeMigrationOnGroupZero took its two HLC ceilings from two separate time.Now() calls. When those calls straddle a millisecond boundary, legacyClock's ceiling lands 1 ms above tsoClock's, and the test fails with tso shadow validation: reserve local TSO window: tso minimum N with batch 1 rolls to physical P beyond committed ceiling P-1 HLC.fencedNowMillis returns the ceiling as "now" whenever the ceiling is in the future, so every timestamp legacyClock issues sits exactly at legacyClock's ceiling. The shadow path hands that value to the group-0 allocator as a minimum, and rejectMinimumWindowBeyondCeiling correctly refuses a minimum above tsoClock's committed ceiling. The rejection is right; the fixture was wrong. Production wires one *HLC -- coordinate.Clock() -- into both the dedicated allocator and the legacy path (main.go configureDedicatedCoordinatorTSO), so the two ceilings are the same value by construction and cannot drift. Taking both from one sample makes the test model that. Measured: ~15-22 failures per 20000 runs before, 0 after. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough移行テストで単一の Changesクロック上限値の共有
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Bug fix · Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to This PR removes the timing flake without changing production behavior, so it is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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. Comment |
1 participant
TestShadowAndCutoverAllocatorsSerializeMigrationOnGroupZerois flaky onmainat roughly 0.08%. It took downtest (ubuntu-latest)on #1234, which touches onlyinternal/encryption— the failure is unrelated to that PR.The failure
The minimum's physical half is exactly
ceiling + 1.Mechanism
The test builds two independent HLCs and sets each ceiling from its own
time.Now()call:HLC.fencedNowMillisreturns the ceiling as "now" whenever the ceiling is in the future (if nowMs < ceiling { return ceiling, ceiling, nil }), so every timestamplegacyClockissues sits exactly atlegacyClock's ceiling — logical 0, physical == ceiling. The shadow path passes that timestamp into the group-0 allocator as a minimum, whererejectMinimumWindowBeyondCeilingcompares it againsttsoClock's committed ceiling.When the two
time.Now()calls fall on either side of a millisecond tick — the work between them isNewTSOStateMachine, the engine fixture,newTestRaftTSOAllocator,NewHLC—legacyClock's ceiling is 1 ms higher and the minimum is 1 ms over the TSO ceiling.This is a fixture bug, not a production bug
rejectMinimumWindowBeyondCeilingis correct to refuse: its doc comment explains thatObserveis permanent, so admitting a minimum past the ceiling poisons every later fenced reservation withErrCeilingExpired. I am not weakening it.Production cannot reach this state.
configureDedicatedCoordinatorTSOinmain.gowires one*HLC—coordinate.Clock()— into all three places:kv.NewRaftTSOAllocator(tsoGroup, coordinate.Clock(), ...)dedicatedTSORoutingOptions(coordinate.Clock(), observer)kv.NewTSORuntimeController(kv.TSORuntimeControllerConfig{Clock: coordinate.Clock(), ...})So the legacy path and the dedicated allocator read the same ceiling field by construction. Two HLCs with independently-set ceilings is a shape only the test creates.
Fix
Take both ceilings from one wall-clock sample, and record why in a comment — the next editor would otherwise naturally write
time.Now()twice again. The two distinct clock objects are kept, since the neighbouring tests uselegacy.Current()to assert the legacy clock is not sampled post-cutover.Risk
None to production: test-only change, no production file touched.
Test evidence
...UnixMilli() + 1), which yields the identical error:tso minimum 117252367897853952 with batch 1 rolls to physical 1789129148832 beyond committed ceiling 1789129148831.go test ./kv/ -run TestShadowAndCutoverAllocatorsSerializeMigrationOnGroupZero -count=20000:okon 20000, repeatedgo test ./kv/ -race -count=1—ok github.com/bootjp/elastickv/kv 17.622sgolangci-lint --config=.golangci.yaml run ./kv/...— 0 issuesSibling sweep
Three tests call
SetPhysicalCeiling(time.Now()...)more than once:TestShadowAndCutoverAllocatorsSerializeMigrationOnGroupZeroTestRaftTSOAllocatorPhaseDReservationsFollowWallClockJumpTestLocalTSOAllocatorRecoversExpiredHLCCeilingA scan of all
kv/*_test.gofor tests constructing twoNewHLC()and taking two ceiling samples returns only the one fixed here.Self-review
kvpackage passes under-race.time.Now()call.rejectMinimumWindowBeyondCeilingkeeps refusing out-of-range minimums, which is what the guard is for.@codex review
@claude review
Summary by CodeRabbit