What the developer wrote
Authored by yyforyongyu
sqldb/v2: scope retry rollbacks
Scope the safety-net rollback to each retry attempt instead of
storing one deferred rollback per loop iteration.
This keeps cleanup local to the active attempt while
preserving the existing commit and rollback behavior.
What changed, and why it matters
This change refactors how database transaction retries are cleaned up in LND's SQL layer. Previously, a rollback was scheduled once per loop and could pile up across retries; now each retry attempt gets its own scoped rollback. The commit message frames this as a safety/cleanup improvement, not a reported vulnerability, and the diff shows only a structural refactor with no change to commit or rollback behavior.
Treat as a low-risk maintainability/correctness refactor. Review whether the previous deferred-rollback accumulation could, under any retry path, leave transactions or connections in an unexpected state, but the commit message asserts behavior is preserved. No immediate security response appears warranted.
Security signals we found
Resource cleanup scoped per retry attempt reduces risk of deferred rollback accumulation
No functional change claimed by commit message; refactor only
Rollback remains safe to call on already-closed transactions
No input validation, cryptographic, or authorization changes
Evidence from the diff
The patch moves the transaction-body execution, commit, and deferred rollback into a new helper executeTxAttempt. The key difference is that the deferred tx.Rollback() now lives inside executeTxAttempt, so it is bound to a single attempt’s scope rather than accumulated by the outer retry loop. The commit message explicitly states the goal is to keep cleanup local to the active attempt while preserving existing commit and rollback behavior. No logic changes to error mapping, serialization/deadlock detection, or retry policy are visible.
Changed components
sqldb/v2/interfaces.goExecuteSQLTransactionWithRetryexecuteTxAttemptInspect captured patch +57 / −44
diff --git a/sqldb/v2/interfaces.go b/sqldb/v2/interfaces.go
index a204de5..0bf6da8 100644
--- a/sqldb/v2/interfaces.go
+++ b/sqldb/v2/interfaces.go
@@ -266,6 +266,55 @@ type RollbackTx func(tx Tx) error
// the delay before the next retry.
type OnBackoff func(retry int, delay time.Duration)
+// executeTxAttempt runs a single transaction attempt and reports whether the
+// caller should retry it.
+func executeTxAttempt(tx Tx, txBody TxBody, rollbackTx RollbackTx,
+ waitBeforeRetry func(int) bool, attempt int) (bool, error) {
+
+ // Rollback is safe to call even if the tx is already closed, so if the tx
+ // commits successfully, this is a no-op.
+ defer func() {
+ _ = tx.Rollback()
+ }()
+
+ if bodyErr := txBody(tx); bodyErr != nil {
+ log.Tracef("Error in txBody: %v", bodyErr)
+
+ // Roll back the transaction, then attempt a random backoff and try
+ // again if the error was a serialization error.
+ if err := rollbackTx(tx); err != nil {
+ return false, MapSQLError(err)
+ }
+
+ dbErr := MapSQLError(bodyErr)
+ if IsSerializationOrDeadlockError(dbErr) {
+ return waitBeforeRetry(attempt), dbErr
+ }
+
+ return false, dbErr
+ }
+
+ // Commit transaction.
+ if commitErr := tx.Commit(); commitErr != nil {
+ log.Tracef("Failed to commit tx: %v", commitErr)
+
+ // Roll back the transaction, then attempt a random backoff and try
+ // again if the error was a serialization error.
+ if err := rollbackTx(tx); err != nil {
+ return false, MapSQLError(err)
+ }
+
+ dbErr := MapSQLError(commitErr)
+ if IsSerializationOrDeadlockError(dbErr) {
+ return waitBeforeRetry(attempt), dbErr
+ }
+
+ return false, dbErr
+ }
+
+ return false, nil
+}
+
// ExecuteSQLTransactionWithRetry is a helper function that executes a
// transaction with retry logic. It will retry the transaction if it fails with
// a serialization error. The function will return an error if the transaction
@@ -314,51 +363,15 @@ func ExecuteSQLTransactionWithRetry(ctx context.Context, makeTx MakeTx,
return dbErr
}
- // Rollback is safe to call even if the tx is already closed,
- // so if the tx commits successfully, this is a no-op.
- defer func() {
- _ = tx.Rollback()
- }()
-
- if bodyErr := txBody(tx); bodyErr != nil {
- log.Tracef("Error in txBody: %v", bodyErr)
-
- // Roll back the transaction, then attempt a random
- // backoff and try again if the error was a
- // serialization error.
- if err := rollbackTx(tx); err != nil {
- return MapSQLError(err)
- }
-
- dbErr := MapSQLError(bodyErr)
- if IsSerializationOrDeadlockError(dbErr) {
- if waitBeforeRetry(i) {
- continue
- }
- }
-
- return dbErr
+ retry, err := executeTxAttempt(
+ tx, txBody, rollbackTx, waitBeforeRetry, i,
+ )
+ if retry {
+ // Transient serialization error, discard this attempt and retry.
+ continue
}
-
- // Commit transaction.
- if commitErr := tx.Commit(); commitErr != nil {
- log.Tracef("Failed to commit tx: %v", commitErr)
-
- // Roll back the transaction, then attempt a random
- // backoff and try again if the error was a
- // serialization error.
- if err := rollbackTx(tx); err != nil {
- return MapSQLError(err)
- }
-
- dbErr := MapSQLError(commitErr)
- if IsSerializationOrDeadlockError(dbErr) {
- if waitBeforeRetry(i) {
- continue
- }
- }
-
- return dbErr
+ if err != nil {
+ return err
}
return nil
Why this scored 33/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.