fix(#8177): sample the matching bootstrap baseline only after the setup commit reached the disk - #8844
Conversation
…up commit reached the disk The matching-peer tests fingerprinted an open database right after committing, while the async flush thread could still be writing that commit's pages. The state machine's recomputation then hashed different bytes and took the mismatch arm. Drain the flush first, and pin the mechanism deterministically with a held flush. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughThe changes add a helper that waits for database pages to flush before computing a bootstrap fingerprint. Two existing matching-baseline tests use it. New tests compare fingerprints sampled during and after a held page flush and check the resulting bootstrap state. ChangesBootstrap fingerprint tests
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to This PR only stabilizes flaky bootstrap fingerprint tests and does not change production behavior. No actionable merge-blocking risk was found. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Coverage variation | ✅ -5.91% coverage variation |
| Diff coverage | ✅ ∅ diff coverage |
Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (e01f4f1) 205671 174005 84.60% Head commit (987868d) 238445 (+32774) 187637 (+13632) 78.69% (-5.91%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>
Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#8844) 0 0 ∅ (not applicable) Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
ReviewOverall: a clean, well-scoped test-side fix. The root-cause analysis (probe table, 5/200 differing digests) is convincing and the scope split with #8843 for production callers is sensible. No blockers from my side. What I checked
Suggestions (non-blocking)
Test coverageGood: new regression class plus the two modified tests, and the 83-test sibling run listed in the PR body. Since these are test-only changes, no new Security / performanceN/A for test-only changes; production behaviour is untouched and tracked in #8843. LGTM from a correctness standpoint; items above are optional polish. 🤖 Generated with Claude Code |
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
ReviewOverall: a good, well-scoped test-side fix. The root-cause evidence is convincing, the helper is small, and the regression class pins the mechanism deterministically with What I checked
Suggestions (all non-blocking)
Production follow-upAgreed that touching Test coverageThe new class covers both sides of the race (in-flight sample takes the mismatch arm; settled sample matches, installs nothing, no unreconciled mark, reason Looks good to merge once CI is green. |
…ut sampling where no sample is wanted Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Review of #8844Test-only fix for the flaky What looks good
Minor suggestions (non-blocking)
Test coverageAdequate for a flake fix: the mechanism is pinned and the fixed path is checked. I did not run the suite locally; the PR description lists the sibling tests run (83 tests, 0 failures). Nothing here blocks merge. |
…and which test is its control Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
ReviewOverall this is a solid, well-scoped test-side fix. I found nothing that blocks the merge. What I checked
Minor suggestions (non-blocking)
Test coverage: good. There is a deterministic reproduction of the race plus a control, and the two flaky tests now sample from the settled copy. I did not run the tests locally. Please make sure CI is green on the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8844 +/- ##
============================================
- Coverage 76.35% 76.35% -0.01%
- Complexity 3780 3781 +1
============================================
Files 2101 2102 +1
Lines 205671 205851 +180
Branches 43315 43375 +60
============================================
+ Hits 157043 157169 +126
- Misses 31708 31741 +33
- Partials 16920 16941 +21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Closes #8177
Issue7519BootstrapWindowGateTest.aPeerThatMatchesTheBaselineIsNeverHeldOutOfTheServicesampled the baseline withBootstrapFingerprint.compute(dir)right aftersetUpcommitted a transaction, and the state machine recomputed it from the same directory a moment later. A commit hands its pages to the asynchronous flush thread, so the two reads could land on either side of the flush, hash different bytes, and send a peer whose copy IS the baseline down the mismatch arm. The fix is test-side: a small helper (SettledBootstrapFingerprint.of(db)) drains the flush queue (waitAllPagesOfDatabaseAreFlushed, assertedtrue) before fingerprinting, and the two tests with this shape use it. A new regression class makes the race deterministic by holding the flush withPageManager.suspendFlushAndExecutearound the commit.Root cause evidence (scratch probe, not committed)
waitAllPagesOfDatabaseAreFlushed, fingerprintsuspendFlushAndExecute, fingerprint inside, then after resume + drainTest plan
Issue8177BootstrapFingerprintInFlightPagesTest(new): a baseline sampled while the commit is in flight takes the mismatch arm (pins the mechanism); its control, sampled from the settled copy after the same held commit, matches, installs nothing, marks nothing, reasonnullIssue7519BootstrapWindowGateTest(7/7),Issue8368BootstrapPassWindowTest(18/18)Issue7011BootstrapSourceRaceTest,ArcadeStateMachineBootstrap{Mismatch,Divergence,BaselinePersistence}Test,ArcadeStateMachineAppliedIndexPerDatabaseTest,ArcadeStateMachineDeferredDropTest,Issue8651StaleEntryAfterRaftInstallBoundaryTest,Issue8368FollowerHeldDuringBootstrapPassIT- 83 tests, 0 failuresCompleteness
Invariant: a test that hands the state machine "this peer's own fingerprint" samples the same on-disk bytes the state machine recomputes.
Sweep -
grep -rln "BootstrapFingerprint.compute" ha-raft/src/test server/src/test engine/src/test:Issue7519BootstrapWindowGateTest(reported)setUp)Issue8368BootstrapPassWindowTest.matchingBaseline()setUp)bootstrapWindowReason()isnull)Issue7011BootstrapSourceRaceTest:186originatedLocally=true, the source arm accepts any copy withlocalLastTxId >= baselineArcadeStateMachineBootstrapDivergenceTest,ArcadeStateMachineBootstrapMismatchTest,ArcadeStateMachineAppliedIndexPerDatabaseTest,ArcadeStateMachineBootstrapBaselinePersistenceTest,Issue8651StaleEntryAfterRaftInstallBoundaryTestArcadeStateMachineDeferredDropTest:190RaftBootstrapFromLocalDatabaseIT,RaftBootstrapFingerprintMismatchSameLsnIT,engine/.../BootstrapFingerprintTestProduction callers (
ArcadeStateMachine.readLocalBootstrapState,BootstrapElection.computeLocalStates,PostBootstrapStateHandler) have the same shape and are not changed here: filed as #8843. Waiting for the flush there is not free (waitAllPagesOfDatabaseAreFlushedkeeps waiting while progress is observed, so on a database under sustained writes it blocks as long as the writes do), and it would run on the Raft apply thread and an HTTP handler.Modified existing tests: two lines (the sample) in
Issue7519BootstrapWindowGateTestandIssue8368BootstrapPassWindowTest. The flaky line IS the defect, so it cannot be fixed by adding tests only. No assertion was changed or loosened.Known gaps
Residual risk
The fix removes the race from the two tests that sample after a commit. It does not change production behaviour (see #8843).
Adversarial pass
Not run: this session had no subagent tool available to spawn the independent reviewer. Noted per the workflow's error table; not a gate.
Review cycles
11d6fde1e7108d8b92a9e366c3388dcommitWhileTheFlushIsHeld()987868d71dDeferred items
SettledBootstrapFingerprint.ofin those five tests too is a one-line change each and would remove the need for the argument entirely." (cycle 1) - skipped: those tests do not commit before sampling (0/200 in the probe), and the workflow forbids modifying existing tests beyond the defect itself.SettledBootstrapFingerprint" (cycles 2, 3) - skipped: a production-side drain would not make the helper redundant, because the test's own sample is taken before the state machine runs, and a naive sample there still reads the pre-flush bytes.isNotEqualToalready fails loudly on a vacuous pass, and the cycle-2 message now names the cause.Final state: clean-approval (cycle 4 of 4).
🤖 Generated with Claude Code
Summary by CodeRabbit