graph/db: let migration test test retry safety
What changed, and why it matters
This commit only changes a test file. It updates the graph database migration test to check whether running the migration twice (a retry) succeeds or fails. The commit message says the actual migration code is not yet retry-safe, and this test change is preparation for a future fix. No production code is changed, so this is not a fix for a live vulnerability by itself.
Treat this as a preparatory test refactor, not a security patch. Monitor the repository for the follow-up commit that actually makes MigrateGraphToSQL retry-safe, and review that production change when it lands.
Security signals we found
Test-only change
Acknowledges retry-safety defect in graph SQL migration
No production code patch included
Temporary test scaffolding for future fix
Evidence from the diff
The diff modifies graph/db/sql_migration_test.go. It adds a new expNotRetrySafety boolean field to test cases, marks several non-empty migration scenarios as currently not retry-safe, and adds a second call to MigrateGraphToSQL in each test to verify behavior. The commit message explicitly states the migration is not retry-safe and that this patch prepares the test for an upcoming fix. No migration logic is altered.
Changed components
graph/db/sql_migration_test.goInspect captured patch +33 / −0
diff --git a/graph/db/sql_migration_test.go b/graph/db/sql_migration_test.go
index 84f7e99..a752be4 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",
@@ -139,6 +155,7 @@ func TestMigrateGraphToSQL(t *testing.T) {
expGraphStats: graphStats{
numNodes: 6,
},
+ expNotRetrySafety: true,
},
{
name: "source node",
@@ -156,6 +173,7 @@ func TestMigrateGraphToSQL(t *testing.T) {
numNodes: 1,
srcNodeSet: true,
},
+ expNotRetrySafety: true,
},
{
name: "channels and policies",
@@ -228,6 +246,7 @@ func TestMigrateGraphToSQL(t *testing.T) {
numChannels: 3,
numPolicies: 3,
},
+ expNotRetrySafety: true,
},
{
name: "prune log",
@@ -338,6 +357,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)
+ }
})
}
}
Why this scored 24/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.