From 5fa5ca10732767b03830b7205fd10994a430771d Mon Sep 17 00:00:00 2001
From: Elle Mouton <elle.mouton@gmail.com>
Date: Fri, 15 Aug 2025 09:00:29 +0200
Subject: [PATCH] graph/db: let migration test test retry safety

Currently, the graph SQL migration is not retry safe. Meaning that if
the source DB exeutes a retry under the hood, this could result in the
migration failing. In preparation for fixing this, we adust the
migration test accordingly.
---
 graph/db/sql_migration_test.go | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/graph/db/sql_migration_test.go b/graph/db/sql_migration_test.go
index 44006a0fd..b1be2a661 100644
--- a/graph/db/sql_migration_test.go
+++ b/graph/db/sql_migration_test.go
@@ -56,6 +56,13 @@ var (
 // for to ensure that our migration from a graph store backed by a KV DB to a
 // SQL database works as expected. At the end of each test, the DBs are compared
 // and expected to have the exact same data in them.
+// This test also ensures that the migration is "retry-safe". This is needed
+// because the migration is hooked up to 2 dbs: the source DB and the
+// destination. The source DB is a db behind the kvdb.Backend interface and
+// the migration makes use of methods on this interface that may be retried
+// under the hood. The migration often does logic inside call-back functions
+// passed to the source DB methods which may be retried, and so we need to
+// ensure that the migration can handle this.
 func TestMigrateGraphToSQL(t *testing.T) {
 	t.Parallel()
 	ctx := context.Background()
@@ -98,6 +105,15 @@ func TestMigrateGraphToSQL(t *testing.T) {
 		write         func(t *testing.T, db *KVStore, object any)
 		objects       []any
 		expGraphStats graphStats
+
+		// expNotRetrySafety is true if we expect an error to occur for
+		// the test if the migration is run twice. In other-words, if
+		// the specific case in question is currently not idempotent.
+		//
+		// NOTE: we want _all_ the cases here to be idempotent, so this
+		// is a temporary field which will be removed once we have
+		// properly made the migration retry-safe.
+		expNotRetrySafety bool
 	}{
 		{
 			name: "empty",
@@ -140,6 +156,7 @@ func TestMigrateGraphToSQL(t *testing.T) {
 			expGraphStats: graphStats{
 				numNodes: 6,
 			},
+			expNotRetrySafety: true,
 		},
 		{
 			name: "source node",
@@ -157,6 +174,7 @@ func TestMigrateGraphToSQL(t *testing.T) {
 				numNodes:   1,
 				srcNodeSet: true,
 			},
+			expNotRetrySafety: true,
 		},
 		{
 			name:  "channels and policies",
@@ -229,6 +247,7 @@ func TestMigrateGraphToSQL(t *testing.T) {
 				numChannels: 3,
 				numPolicies: 3,
 			},
+			expNotRetrySafety: true,
 		},
 		{
 			name: "prune log",
@@ -339,6 +358,20 @@ func TestMigrateGraphToSQL(t *testing.T) {
 
 			// Validate that the two databases are now in sync.
 			assertInSync(t, kvDB, sql, test.expGraphStats)
+
+			// NOTE: for now, not all the cases in the test are
+			// retry safe! The aim is to completely remove this
+			// field once we have made the migration retry-safe.
+			err = MigrateGraphToSQL(ctx, sql.cfg, kvDB.db, sql.db)
+			if !test.expNotRetrySafety {
+				// The migration should be retry-safe, so
+				// running it again should not change the state
+				// of the databases.
+				require.NoError(t, err)
+				assertInSync(t, kvDB, sql, test.expGraphStats)
+			} else {
+				require.Error(t, err)
+			}
 		})
 	}
 }