From 177a21b2661982df3dec5d7efa16c799ec75f35e Mon Sep 17 00:00:00 2001
From: Roy Jacobson <roy@dragonflydb.io>
Date: Mon, 3 Jul 2023 16:46:38 +0200
Subject: [PATCH] Fix a bug and add a timeout test for takeover. (#1512)

---
 src/server/replica.cc                 |  8 ++----
 tests/dragonfly/replication_test.py   | 38 +++++++++++++++++++++++++++
 tests/dragonfly/server_family_test.py |  2 +-
 3 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/src/server/replica.cc b/src/server/replica.cc
index 7083f88b8..c5c73c3de 100644
--- a/src/server/replica.cc
+++ b/src/server/replica.cc
@@ -235,12 +235,8 @@ std::error_code Replica::TakeOver(std::string_view timeout) {
   sock_->proactor()->Await(
       [this, &ec, timeout] { ec = SendNextPhaseRequest(absl::StrCat("TAKEOVER ", timeout)); });
 
-  if (ec) {
-    // TODO: Handle timeout more gracefully.
-    return cntx_.ReportError(ec);
-  }
-  // If we successfully taken over, return and let server_family stop us.
-  return {};
+  // If we successfully taken over, return and let server_family stop the replication.
+  return ec;
 }
 
 void Replica::MainReplicationFb() {
diff --git a/tests/dragonfly/replication_test.py b/tests/dragonfly/replication_test.py
index 1e053b6ab..0730682d4 100644
--- a/tests/dragonfly/replication_test.py
+++ b/tests/dragonfly/replication_test.py
@@ -1179,3 +1179,41 @@ async def test_take_over_seeder(df_local_factory, df_seeder_factory, master_thre
 
         capture = await seeder.capture()
         assert await seeder.compare(capture, port=replica.port)
+
+
+@pytest.mark.asyncio
+async def test_take_over_timeout(df_local_factory, df_seeder_factory):
+    master = df_local_factory.create(proactor_threads=2,
+                                     port=BASE_PORT,
+                                     logtostderr=True)
+    replica = df_local_factory.create(
+        port=BASE_PORT+1, proactor_threads=2)
+    df_local_factory.start_all([master, replica])
+
+    seeder = df_seeder_factory.create(
+        port=master.port, keys=1000, dbcount=5, stop_on_failure=False)
+    async with (
+        master.client() as c_master,
+        replica.client() as c_replica,
+    ):
+        await c_replica.execute_command(f"REPLICAOF localhost {master.port}")
+        await wait_available_async(c_replica)
+
+        async def seed():
+            await seeder.run(target_ops=3000)
+
+        fill_task = asyncio.create_task(seed())
+
+        # Give the seeder a bit of time.
+        await asyncio.sleep(1)
+        try:
+            await c_replica.execute_command(f"REPLTAKEOVER 0.0001")
+        except redis.exceptions.ResponseError as e:
+            assert str(e) == "Couldn't execute takeover"
+        else:
+            assert False, "Takeover should not succeed."
+        seeder.stop()
+        await fill_task
+
+        assert await c_master.execute_command("role") == [b'master', [[b'127.0.0.1', bytes(str(replica.port), 'ascii'), b'stable_sync']]]
+        assert await c_replica.execute_command("role") == [b'replica', b'localhost', bytes(str(master.port), 'ascii'), b'stable_sync']
diff --git a/tests/dragonfly/server_family_test.py b/tests/dragonfly/server_family_test.py
index 5737906ae..88150cb5b 100644
--- a/tests/dragonfly/server_family_test.py
+++ b/tests/dragonfly/server_family_test.py
@@ -40,7 +40,7 @@ change to match the fact that we supporting this operation.
 For now we are expecting to get an error
 '''
 
-@pytest.skip("Skip until we decided on correct behaviour of eval inside multi")
+@pytest.mark.skip("Skip until we decided on correct behaviour of eval inside multi")
 async def test_multi_eval(async_client: aioredis.Redis):
     try:
         pipeline = async_client.pipeline()