(kvrocks) branch unstable updated: fix(cluster): release forbidden slot range on CLUSTER RESET (#3545)

[hu...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

git-hulk pushed a commit to branch unstable
in repository https://gitbox.apache.org/repos/asf/kvrocks.git


The following commit(s) were added to refs/heads/unstable by this push:
     new 8830e26eb fix(cluster): release forbidden slot range on CLUSTER RESET (#3545)
8830e26eb is described below

commit 8830e26ebd82308741bb04d4f220ef0091c42c22
Author: hulk <hu...@gmail.com>
AuthorDate: Mon Jul 6 11:35:19 2026 +0800

    fix(cluster): release forbidden slot range on CLUSTER RESET (#3545)
    
    After a slot has been migrated away, the source node keeps
    `forbidden_slot_range_` set. That is normally harmless because the
    topology no longer routes the slot to this node, so the forbidden-slot
    check is never reached. But `CLUSTER RESET` clears the topology while
    leaving the migrator's forbidden range intact — a subsequent SETNODES
    that reassigns the slot back to this node then spuriously rejects writes
    with `TRYAGAIN Can't write to slot being migrated`. Explicitly release
    the forbidden slot range in `Cluster::Reset` to fix this.
    
    Assistant-By: Claude Opus 4.7
---
 src/cluster/cluster.cc                             |  8 +++
 .../integration/slotmigrate/slotmigrate_test.go    | 66 ++++++++++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/src/cluster/cluster.cc b/src/cluster/cluster.cc
index d5d95ed33..a2972a6f2 100644
--- a/src/cluster/cluster.cc
+++ b/src/cluster/cluster.cc
@@ -986,6 +986,14 @@ Status Cluster::Reset() {
   migrated_slots_.clear();
   imported_slots_.clear();
 
+  // The migrator's forbidden slot range persists past a successful migration
+  // and is only harmless while slots_nodes_[slot] no longer points at us.
+  // A subsequent SETNODES after reset can reassign that slot back here and
+  // spuriously reject writes with TRYAGAIN, so drop it explicitly.
+  if (srv_->slot_migrator) {
+    srv_->slot_migrator->ReleaseForbiddenSlotRange();
+  }
+
   // unlink the cluster nodes file if exists
   unlink(srv_->GetConfig()->NodesFilePath().data());
   return Status::OK();
diff --git a/tests/gocase/integration/slotmigrate/slotmigrate_test.go b/tests/gocase/integration/slotmigrate/slotmigrate_test.go
index 5a214bb9d..dee732408 100644
--- a/tests/gocase/integration/slotmigrate/slotmigrate_test.go
+++ b/tests/gocase/integration/slotmigrate/slotmigrate_test.go
@@ -1422,3 +1422,69 @@ func TestSlotRangeMigrate(t *testing.T) {
 	})
 
 }
+
+// TestClusterResetPreservesForbiddenSlot reproduces a bug where the source
+// node keeps its `forbidden_slot_range_` set after a slot has been migrated
+// away. Under normal operation this is harmless because the topology has
+// already switched the slot to the destination node, so the forbidden-slot
+// check in CanExecByMySelf is never reached for that slot. However, after
+// `CLUSTER RESET` clears the cluster topology, if the new topology assigns
+// the previously-migrated slot back to this node, writes to that slot are
+// spuriously rejected with `TRYAGAIN Can't write to slot being migrated
+// which is in write forbidden phase`.
+func TestClusterResetPreservesForbiddenSlot(t *testing.T) {
+	ctx := context.Background()
+
+	srv0 := util.StartServer(t, map[string]string{"cluster-enabled": "yes"})
+	defer func() { srv0.Close() }()
+	rdb0 := srv0.NewClient()
+	defer func() { require.NoError(t, rdb0.Close()) }()
+	id0 := "resetforbiddenxxxxxxxxxxxxxxxxxxxxxxxx00"
+	require.NoError(t, rdb0.Do(ctx, "clusterx", "SETNODEID", id0).Err())
+
+	srv1 := util.StartServer(t, map[string]string{"cluster-enabled": "yes"})
+	defer func() { srv1.Close() }()
+	rdb1 := srv1.NewClient()
+	defer func() { require.NoError(t, rdb1.Close()) }()
+	id1 := "resetforbiddenxxxxxxxxxxxxxxxxxxxxxxxx01"
+	require.NoError(t, rdb1.Do(ctx, "clusterx", "SETNODEID", id1).Err())
+
+	setupCluster := func(version int) {
+		clusterNodes := fmt.Sprintf("%s %s %d master - 0-8191\n", id0, srv0.Host(), srv0.Port())
+		clusterNodes += fmt.Sprintf("%s %s %d master - 8192-16383", id1, srv1.Host(), srv1.Port())
+		require.NoError(t, rdb0.Do(ctx, "clusterx", "SETNODES", clusterNodes, strconv.Itoa(version)).Err())
+		require.NoError(t, rdb1.Do(ctx, "clusterx", "SETNODES", clusterNodes, strconv.Itoa(version)).Err())
+	}
+
+	setupCluster(1)
+
+	// Migrate slot 3300 (the slot key "b" hashes to) from srv0 to srv1.
+	slot := 3300
+	require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", slot, id1).Val())
+	waitForMigrateState(t, rdb0, slot, SlotMigrationStateSuccess)
+
+	// Update topology so slot 3300 now belongs to srv1.
+	srcVer, err := rdb0.Do(ctx, "clusterx", "version").Int()
+	require.NoError(t, err)
+	require.NoError(t, rdb0.Do(ctx, "clusterx", "setslot", slot, "node", id1, srcVer+1).Err())
+	dstVer, err := rdb1.Do(ctx, "clusterx", "version").Int()
+	require.NoError(t, err)
+	require.NoError(t, rdb1.Do(ctx, "clusterx", "setslot", slot, "node", id1, dstVer+1).Err())
+
+	// Empty the DB so `CLUSTER RESET` is allowed (it refuses on non-empty DBs).
+	require.NoError(t, rdb0.FlushDB(ctx).Err())
+
+	// CLUSTER RESET HARD — this clears cluster topology, node id, migrated_slots_,
+	// imported_slots_, etc., but does NOT reset the migrator's forbidden_slot_range_.
+	require.Equal(t, "OK", rdb0.Do(ctx, "cluster", "reset", "hard").Val())
+
+	// Recreate the cluster; srv0 now owns slot 3300 again.
+	require.NoError(t, rdb0.Do(ctx, "clusterx", "SETNODEID", id0).Err())
+	setupCluster(2)
+
+	// A write to a key hashing to slot 3300 must succeed. Before the fix this
+	// returns `TRYAGAIN Can't write to slot being migrated which is in write
+	// forbidden phase` because forbidden_slot_range_ still holds {3300, 3300}.
+	key := util.SlotTable[slot]
+	require.NoError(t, rdb0.Set(ctx, key, "1", 0).Err())
+}