From 74662817004a1d76b1886a4009f4ea110b6d4ae4 Mon Sep 17 00:00:00 2001
From: Shayon Mukherjee <shayonj@gmail.com>
Date: Mon, 18 Nov 2024 15:49:19 -0500
Subject: [PATCH] Do not mark switchover complete if subscription drop fails

---
 lib/pg_easy_replicate/orchestrate.rb       | 10 ++--
 spec/pg_easy_replicate/orchestrate_spec.rb | 68 +++++++++++++++++-----
 2 files changed, 57 insertions(+), 21 deletions(-)

diff --git a/lib/pg_easy_replicate/orchestrate.rb b/lib/pg_easy_replicate/orchestrate.rb
index 394ceb8..c471da8 100644
--- a/lib/pg_easy_replicate/orchestrate.rb
+++ b/lib/pg_easy_replicate/orchestrate.rb
@@ -239,6 +239,11 @@ def switchover(
         revoke_connections_on_source_db(group_name)
         wait_for_remaining_catchup(group_name)
         refresh_sequences(conn_string: target_conn, schema: group[:schema_name])
+
+        drop_subscription(
+          group_name: group_name,
+          target_conn_string: target_conn,
+        )
         mark_switchover_complete(group_name)
 
         unless skip_vacuum_analyze
@@ -248,11 +253,6 @@ def switchover(
             schema: group[:schema_name],
           )
         end
-
-        drop_subscription(
-          group_name: group_name,
-          target_conn_string: target_conn,
-        )
       rescue => e
         restore_connections_on_source_db(group_name)
         abort_with("Switchover failed: #{e.message}")
diff --git a/spec/pg_easy_replicate/orchestrate_spec.rb b/spec/pg_easy_replicate/orchestrate_spec.rb
index 2a34b8c..b24e716 100644
--- a/spec/pg_easy_replicate/orchestrate_spec.rb
+++ b/spec/pg_easy_replicate/orchestrate_spec.rb
@@ -209,9 +209,7 @@
   end
 
   describe ".drop_subscription" do
-    before do
-      PgEasyReplicate.bootstrap({ group_name: "cluster1" })
-    end
+    before { PgEasyReplicate.bootstrap({ group_name: "cluster1" }) }
 
     after do
       PgEasyReplicate.cleanup({ everything: true, group_name: "cluster1" })
@@ -234,7 +232,8 @@
     it "raises an error when user does not have sufficient privileges" do
       # Use a limited user for this test case
       restricted_user = "no_sup"
-      restricted_user_target_connection_url = target_connection_url(restricted_user)
+      restricted_user_target_connection_url =
+        target_connection_url(restricted_user)
 
       described_class.create_subscription(
         group_name: "cluster1",
@@ -250,7 +249,9 @@
       end.to raise_error(RuntimeError) { |e|
         expect(e.message).to include("Unable to drop subscription")
       }
-      expect(pg_subscriptions(connection_url: target_connection_url)).not_to eq([])
+      expect(pg_subscriptions(connection_url: target_connection_url)).not_to eq(
+        [],
+      )
     end
   end
 
@@ -704,6 +705,49 @@
         { last_analyze: nil, last_vacuum: nil, relname: "spatial_ref_sys" },
       )
     end
+
+    it "does not mark switchover complete if subscription drop fails" do
+      conn1 =
+        PgEasyReplicate::Query.connect(
+          connection_url: connection_url,
+          schema: test_schema,
+        )
+      conn1[:items].insert(name: "Foo1")
+      expect(conn1[:items].first[:name]).to eq("Foo1")
+
+      conn2 =
+        PgEasyReplicate::Query.connect(
+          connection_url: target_connection_url,
+          schema: test_schema,
+        )
+      expect(conn2[:items].first).to be_nil
+
+      ENV["SECONDARY_SOURCE_DB_URL"] = docker_compose_source_connection_url
+      described_class.start_sync(
+        group_name: "cluster1",
+        schema_name: test_schema,
+        recreate_indices_post_copy: true,
+      )
+
+      allow(described_class).to receive(:drop_subscription).and_raise(
+        "Subscription drop failed",
+      )
+
+      expect do
+        described_class.switchover(
+          group_name: "cluster1",
+          source_conn_string: connection_url,
+          target_conn_string: target_connection_url,
+          skip_vacuum_analyze: true,
+        )
+      end.to raise_error("Switchover failed: Subscription drop failed")
+
+      described_class.restore_connections_on_source_db("cluster1")
+
+      expect(PgEasyReplicate::Group.find("cluster1")).to include(
+        switchover_completed_at: nil,
+      )
+    end
   end
 
   # Note: Hard to test for special roles that act as superuser which aren't superuser, like rds_superuser
@@ -769,17 +813,9 @@
         )
       end.to raise_error(/Starting sync failed: PG::InsufficientPrivilege/)
 
-      # expect(PgEasyReplicate::Group.find("cluster1")).to include(
-      #   switchover_completed_at: nil,
-      #   created_at: kind_of(Time),
-      #   name: "cluster1",
-      #   schema_name: "pger_test",
-      #   id: kind_of(Integer),
-      #   started_at: kind_of(Time),
-      #   updated_at: kind_of(Time),
-      #   failed_at: nil,
-      #   table_names: nil,
-      # )
+      expect(PgEasyReplicate::Group.find("cluster1")).to include(
+        switchover_completed_at: nil,
+      )
 
       # conn1[:items].insert(name: "Foo2")