graph/db/migration1: fix defer commit/rollback in test tx executor
What the developer wrote
Authored by ziggie
graph/db/migration1: fix defer commit/rollback in test tx executor
The defer closure checked a local err variable for commit/rollback
decisions, but err remained nil after a successful BeginTx. When
txBody failed, the error was returned directly without assigning to
err, so the defer always committed instead of rolling back.
Additionally, since err was not a named return value, the defer's
Commit error assignment was silently swallowed.
Replace the error-prone defer pattern with explicit rollback on
txBody failure and a direct Commit return.
What changed, and why it matters
This commit fixes a bug in a test helper that runs database transactions for LND's graph database migration tests. The old code accidentally committed every transaction, even when the inner test work failed, because of a subtle Go error-handling mistake. The fix makes failed transactions roll back and successful ones commit properly. This is test-only code, so it does not directly affect live Lightning nodes, but it could have caused migration tests to leave partial or incorrect data in the database instead of rolling back as intended.
No urgent production action is needed because the change is in test-only code. Users running LND graph migration tests should update to include this fix so that tests correctly roll back on failure and surface commit errors. Review other transaction helpers in the codebase for the same defer-on-local-err pattern.
Security signals we found
Incorrect transaction lifecycle in database helper
Silent swallowing of commit errors due to non-named return value
Test-only code with no direct production impact
Potential for test state corruption / non-atomic test behavior
Evidence from the diff
The function ExecTx in graph/db/migration1/test_sql.go previously used a deferred closure to decide between Commit and Rollback based on a local err variable. That err was nil after BeginTx succeeded, was never reassigned by txBody’s returned error (because txBody’s error was returned directly), and was not a named return value, so any Commit error assigned inside the defer was discarded. As a result, the defer always took the Commit branch. The patch removes the defer, explicitly rolls back when txBody returns an error, and returns tx.Commit() directly on success.
Changed components
graph/db/migration1/test_sql.gotestBatchedSQLQueries.ExecTxInspect captured patch +7 / −8
diff --git a/graph/db/migration1/test_sql.go b/graph/db/migration1/test_sql.go
index 8034bec..3d157dc 100644
--- a/graph/db/migration1/test_sql.go
+++ b/graph/db/migration1/test_sql.go
@@ -31,16 +31,15 @@ func (t *testBatchedSQLQueries) ExecTx(ctx context.Context,
if err != nil {
return err
}
- defer func() {
- if err != nil {
- _ = tx.Rollback()
- } else {
- err = tx.Commit()
- }
- }()
reset()
queries := sqlc.New(tx)
- return txBody(queries)
+ if err := txBody(queries); err != nil {
+ _ = tx.Rollback()
+
+ return err
+ }
+
+ return tx.Commit()
}
Why this scored 21/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.