Deflake ccov: Contain slot migration test failures with a recovery barrier#4223
Open
rainsupreme wants to merge 4 commits into
Open
Deflake ccov: Contain slot migration test failures with a recovery barrier#4223rainsupreme wants to merge 4 commits into
rainsupreme wants to merge 4 commits into
Conversation
When a test in cluster-migrateslots.tcl fails an assertion mid-migration, it leaks state: the in-flight migration, DEBUG SLOTMIGRATION PREVENT-* flags, and config tweaks (rdb-key-save-delay, repl-timeout). The next test to call CLUSTER MIGRATESLOTS then hits a non-assertion error such as "ERR I am already migrating slot", which the test framework re-raises as an exception that aborts the entire test client, discarding all remaining tests in the run. Issue 2692 root-caused one instance of this cascade; gcov soak runs show it is the general failure amplification mode for this file (6 of 8 soak shards ended early this way). Add a best-effort recovery barrier (cancel in-flight migrations, reset leaked debug/config state, wait for jobs to reach a terminal state and for cluster convergence) and call it after the migration-starting tests observed to seed cascades, including inside the parameterized restart test loop. A failing test now costs one [err] instead of the rest of the run. Signed-off-by: Rain Valentine <rsg000@gmail.com>
The recovery barrier cancels leaked migrations and resets debug/config state, but a test that fails after slots have moved leaves ownership displaced from the layout later tests assume. Their commands then fail with MOVED or "ERR Slots are not served by this node" -- non-assertion errors that abort the whole test client (observed in 2 of 4 guard-mode gcov soak shards). Capture the canonical slot-to-primary layout once at cluster setup and, in the barrier after migrations drain, migrate any displaced ranges back to their canonical owners. Contiguous displaced slots with the same source and destination move as one range. No-op on the happy path beyond a CLUSTER SLOTS comparison; all steps best-effort. Signed-off-by: Rain Valentine <rsg000@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe migration test suite now captures the cluster’s initial slot ownership, detects and drains active migrations, restores displaced slot ranges to their canonical shard primaries, and invokes this recovery barrier across migration-related test flows. ChangesSlot migration recovery
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #4223 +/- ##
============================================
- Coverage 76.83% 76.80% -0.03%
============================================
Files 162 162
Lines 81433 81477 +44
============================================
+ Hits 62565 62577 +12
- Misses 18868 18900 +32 🚀 New features to boost your workflow:
|
![valkey-review-bot[bot]](https://avatars.githubusercontent.com/in/3633497?s=60&v=4){kind=small}
rainsupreme
marked this pull request as ready for review
July 20, 2026 22:17
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/cluster/cluster-migrateslots.tcl`:
- Around line 265-271: Update recover_slot_migration_state to reset each
server’s hz configuration to its normal default alongside rdb-key-save-delay and
repl-timeout, using the existing CONFIG SET pattern. This cleanup must run for
every server even when the “Import with hz set to 1” test exits early, so the
subsequent barrier sees restored cron timing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 56e1fb6a-9529-4044-8606-8d3b6dfb150f
📒 Files selected for processing (1)
tests/unit/cluster/cluster-migrateslots.tcl
The hz-throttled import test lowers hz to 1 on two nodes and only restores it in its final lines, so a mid-test failure leaks the slowed cron tick into every subsequent test. Reset hz to its default alongside the other leaked configs. Signed-off-by: Rain Valentine <rsg000@gmail.com>
The canonical layout only recorded the three capture-time primaries, so slots owned by a replica promoted during a failover test were invisible to the restore path and stayed displaced after the barrier ran. Track ownership at shard granularity instead: record every node's ID and shard membership at capture time, treat a slot as displaced only when it sits on the wrong shard, and migrate it back to whichever member of the canonical shard is currently its primary. Cancelling and draining migrations now also covers all nodes, since after a failover the primary holding a migration job may be a promoted replica. Verified with a forced failure that promotes a replica and leaves a slot displaced onto it: the following barrier migrates the slot back to its canonical shard, where previously it stayed displaced. Full file passes 105/105 unchanged. Signed-off-by: Rain Valentine <rsg000@gmail.com>
Contributor
Author
|
This restorer idea could potentially be generalized to all tests by integrating it with the on-failure hook, in which case it would restore some per-file invariant state. The advantage is that nothing changes when tests pass, but it wouldn't work for tests that depend on state set by a prior test). This refactor would be far less targeted of a fix, so I didn't do it here. |
roshkhatri
self-requested a review
July 21, 2026 21:46
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
1 participant
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.
Summary
When a test in cluster-migrateslots.tcl fails an assertion mid-migration, it leaks state: the in-flight migration, DEBUG SLOTMIGRATION PREVENT-* flags, config tweaks (rdb-key-save-delay, repl-timeout), and displaced slot ownership. The framework recovers from the assertion and continues, but the next code to touch the cluster hits a non-assertion error (
ERR I am already migrating slot,MOVED,ERR Slots are not served by this node), which aborts the entire test client and discards all remaining tests in the run. Issue 2692 fixed one instance of this cascade at the seeding test; this PR addresses the amplification mechanism itself, which is also the dominant way this file destroys Codecov runs (coverage-side data in issue 4153). Leaked config even manufactures downstream "flakes": a failed run of the repl-timeout test leavesrdb-key-save-delay 100000set, injecting 100 seconds of artificial snapshot delay (1000 keys x 100ms) into the restart-test loop that follows.Commit 1 adds
recover_slot_migration_state, a best-effort recovery barrier called after each migration-starting test observed to seed cascades (one call inside the parameterized restart loop covers its 8 variants): cancel in-flight migrations on all primaries, reset the leaked debug flags and configs, wait for all migration jobs to reach a terminal state, and wait for cluster convergence. Every step is catch-wrapped — the barrier runs at block level, where an escaping error would itself abort the client.Commit 2 adds slot ownership restoration, since cancelling a migration does not move slots back: capture the canonical slot-to-primary layout once at cluster setup, and in the barrier — after migrations drain — migrate any displaced ranges back to their canonical owners.
Verification (local)
Full file passes 105/105 with the barrier, identical to baseline — the barrier is a no-op beyond cheap checks when tests pass.
Forced-failure experiments: injecting
failimmediately after MIGRATESLOTS starts in the restart loop costs, with the barrier, 8 [err] with all remaining 97 tests passing; the identical injection without the barrier reproduces the historical cascade — 1 [err], then[exception]: ERR I am already migrating slot, client abort, 24 tests discarded. Forcing a failure that leaves slot ownership displaced costs exactly one [err] with the restore, and causes collateral failures in later tests without it.Soak results (gcov-instrumented builds matching the Codecov job, 50 loops x 4 shards per mode, fork CI)
Run 1 — barrier only vs its base commit (https://github.com/valkey-rainfall/valkey/actions/runs/29466298795): baseline retained 11105 of 21000 [ok] results (53%) with 3 client aborts; barrier retained 17098 (81%) with 2 aborts, and the
already migrating slotabort class dropped to zero.Run 2 — barrier + ownership restore vs base (https://github.com/valkey-rainfall/valkey/actions/runs/29621384929): baseline retained 9062 (43%) with 3 aborts; guarded retained 18304 (87%) with 2 aborts, with two of four guarded shards fully contained — one flaky [err], zero aborts, everything else retained.
Per-iteration flake rates are unchanged between modes in both runs, as expected — this PR reduces the blast radius of a failure, it does not deflake the failing tests themselves (root-cause fixes for the top flakes are planned separately).
Known limitations
The two guarded-mode aborts in run 2 have understood causes, left as future work: (1) the ownership restore is single-shot and silent-on-failure — it can race the just-cancelled migration and give up, so a retry/verify loop would harden it; (2) the barrier covers the observed cascade-seeding tests, not every migration-starting test, so an unguarded test (e.g. "FLUSH on target during import" in run 2) can still seed a cascade — per-test coverage is possible if maintainers prefer it; (3) block-level setup code between tests executes outside any test body and remains exposed to leaked state.
References
Issue #2692 (prior instance of this cascade, fixed per-test)
issue #4153 (coverage-job impact data)
issue #3685 (an open flake in this file that this PR contains but does not fix).