Vanillaxi opened a new issue, #1183: URL: https://github.com/apache/incubator-seata-go/issues/1183
### ✅ 验证清单 - [x] 🔍 我已经搜索过 [现有 Issues](https://github.com/apache/incubator-seata-go/issues)，确信这不是重复问题 - [x] 🛠️ 我愿意自己处理这个议题 ### 🚀 Go 版本 go1.24.3 darwin/arm64 ### 📦 Seata-go 版本 master, commit 5980741 ### 💾 操作系统 🍎 macOS ### 📝 Bug 描述 `pkg/datasource/sql/undo/base/undo.go` 的`BaseUndoLogManager.Undo` 的清理逻辑存在错误传播和资源释放问题： 1. 该函数使用命名返回值 `err`，但 `defer` 中的 `stmt.Close()`、`rows.Close()` 和 `tx.Rollback()` 会重新赋值给同一个 `err`。这些清理操作成功返回 `nil` 时，会覆盖原始执行错误，使 `Undo` 将失败返回为成功。 2. 事务回滚又依赖 `err != nil`，如果较晚注册的 `defer` 已把原始错误清成 `nil`，事务回滚会被跳过。 3. `db.Conn(ctx)` 取得的连接没有对应的 `conn.Close()`。包括 `BeginTx` 失败的路径，函数返回后连接仍然处于占用状态。 4. 遇到 `GlobalFinished` 日志或空 SQL Undo 日志列表时，函数直接返回 `nil`，未执行到末尾的提交逻辑，错误条件下的回滚也不会执行。 相关代码：[undo/base/undo.go](https://github.com/apache/incubator-seata-go/blob/598074146bc4142dd33f29d5d2a2049c90ba0f6f/pkg/datasource/sql/undo/base/undo.go#L355-L441) ### 🔄 重现步骤 1. 使用上述(master, commit 5980741)源码, 在包含 `go.mod` 的仓库根目录创建 `undo_cleanup_test.go`，复制下面的完整代码。 2. 在根目录下执行 `go test -count=1 -v -timeout 30s ./undo_cleanup_test.go` 复现直接调用生产代码中的 `BaseUndoLogManager.Undo`，仅使用仓库已有依赖 `go-sqlmock` 模拟数据库 I/O，不需要运行 MySQL、PostgreSQL 或 Seata Server。 <details> <summary>完整复现代码：undo_cleanup_test.go</summary> ```go package investigation_test import ( "context" "database/sql" "errors" "testing" "github.com/DATA-DOG/go-sqlmock" "seata.apache.org/seata-go/v2/pkg/datasource/sql/types" "seata.apache.org/seata-go/v2/pkg/datasource/sql/undo" "seata.apache.org/seata-go/v2/pkg/datasource/sql/undo/base" ) func newUndoDB(t *testing.T) (*sql.DB, sqlmock.Sqlmock) { t.Helper() db, mock, err := sqlmock.New() if err != nil { t.Fatal(err) } db.SetMaxOpenConns(1) t.Cleanup(func() { _ = db.Close() }) return db, mock } func runUndo(db *sql.DB) error { return base.NewBaseUndoLogManager().Undo( context.Background(), types.DBTypeMySQL, "test-xid", 123, db, "test_db", ) } func TestUndoPreservesPrepareError(t *testing.T) { db, mock := newUndoDB(t) failure := errors.New("prepare failed") mock.ExpectBegin() mock.ExpectPrepare("SELECT").WillReturnError(failure) mock.ExpectRollback() err := runUndo(db) t.Logf("Undo returned: %v", err) if !errors.Is(err, failure) { t.Errorf("expected original prepare error, got %v", err) } if err := mock.ExpectationsWereMet(); err != nil { t.Error(err) } } func TestUndoPreservesQueryErrorAndRollsBack(t *testing.T) { db, mock := newUndoDB(t) failure := errors.New("query failed") mock.ExpectBegin() mock.ExpectPrepare("SELECT").ExpectQuery(). WithArgs(int64(123), "test-xid").WillReturnError(failure) mock.ExpectRollback() err := runUndo(db) t.Logf("Undo returned: %v", err) if !errors.Is(err, failure) { t.Errorf("expected original query error, got %v", err) } if err := mock.ExpectationsWereMet(); err != nil { t.Error(err) } } func TestUndoReleasesConnectionAfterBeginFailure(t *testing.T) { db, mock := newUndoDB(t) failure := errors.New("begin failed") mock.ExpectBegin().WillReturnError(failure) if err := runUndo(db); !errors.Is(err, failure) { t.Errorf("expected original begin error, got %v", err) } if err := mock.ExpectationsWereMet(); err != nil { t.Error(err) } inUse := db.Stats().InUse t.Logf("connections still in use: %d", inUse) if inUse != 0 { t.Errorf("expected connection returned to pool, InUse=%d", inUse) } } func TestUndoFinishesTransactionForGlobalFinishedLog(t *testing.T) { db, mock := newUndoDB(t) mock.ExpectBegin() mock.ExpectPrepare("SELECT").ExpectQuery(). WithArgs(int64(123), "test-xid").WillReturnRows( sqlmock.NewRows([]string{"branch_id", "xid", "context", "rollback_info", "log_status"}). AddRow(int64(123), "test-xid", []byte("{}"), []byte("{}"), int32(undo.UndoLogStatusGlobalFinished)), ) // This normal early-return path should finish its transaction. mock.ExpectCommit() if err := runUndo(db); err != nil { t.Errorf("unexpected error: %v", err) } if err := mock.ExpectationsWereMet(); err != nil { t.Error(err) } inUse := db.Stats().InUse t.Logf("connections still in use: %d", inUse) if inUse != 0 { t.Errorf("expected connection returned to pool, InUse=%d", inUse) } } ``` </details> 四个用例分别检查错误传播、失败回滚、连接归还和正常提前返回的事务收尾。`GlobalFinished` 用例沿用仓库现有单测的提交预期；当前实现实际既未提交，也未回滚。 ### ✅ 预期行为 场景 | 正确预期 | | --- | --- | | Prepare 失败，回滚成功 | 返回原始 Prepare 错误 | | Query 失败，语句关闭成功 | 返回原始 Query 错误，并回滚事务 | | BeginTx 失败 | 返回原始错误，并将已取得的连接归还连接池 | | 遇到 GlobalFinished 日志提前返回 | 正常结束事务并归还连接，不执行业务 Undo | 清理操作不应把原始失败覆盖为成功。所有返回路径都应处理已取得的连接和已开启的事务。 ### ❌ 实际行为 场景 | 实际结果 | | --- | --- | | Prepare 失败 | 返回 `nil`，原始错误被成功的回滚覆盖 | | Query 失败 | 返回 `nil`，期望的回滚未执行 | | BeginTx 失败 | 返回原始错误，但 `db.Stats().InUse` 仍为 `1` | | GlobalFinished 提前返回 | 期望的提交未执行，`db.Stats().InUse` 仍为 `1` | 实际测试输出： ```text === RUN TestUndoPreservesPrepareError ERROR: prepare sql fail, err: prepare failed undo_cleanup_test.go:40: Undo returned: <nil> undo_cleanup_test.go:42: expected original prepare error, got <nil> --- FAIL: TestUndoPreservesPrepareError (0.00s) === RUN TestUndoPreservesQueryErrorAndRollsBack ERROR: query sql fail, err: query failed undo_cleanup_test.go:58: Undo returned: <nil> undo_cleanup_test.go:60: expected original query error, got <nil> undo_cleanup_test.go:63: there is a remaining expectation which was not matched: ExpectedRollback => expecting transaction Rollback --- FAIL: TestUndoPreservesQueryErrorAndRollsBack (0.00s) === RUN TestUndoReleasesConnectionAfterBeginFailure undo_cleanup_test.go:79: connections still in use: 1 undo_cleanup_test.go:81: expected connection returned to pool, InUse=1 --- FAIL: TestUndoReleasesConnectionAfterBeginFailure (0.00s) === RUN TestUndoFinishesTransactionForGlobalFinishedLog undo_cleanup_test.go:100: there is a remaining expectation which was not matched: ExpectedCommit => expecting transaction Commit undo_cleanup_test.go:103: connections still in use: 1 undo_cleanup_test.go:105: expected connection returned to pool, InUse=1 --- FAIL: TestUndoFinishesTransactionForGlobalFinishedLog (0.00s) FAIL FAIL command-line-arguments 0.467s FAIL ``` ### 💡 可能的解决方案 1. `rows.Close()`、`stmt.Close()`、`tx.Rollback()` 不再赋值给命名返回值 err。保留原始执行错误，清理失败单独记录日志。 2. 不再通过 `err != nil` 来决定事务回滚 3. `db.Conn(ctx)` 取得连接后，通过注册 `defer conn.Close()` 安排归还 4. 对于 `GlobalFinished `日志或空 SQL Undo 日志列表，把 `return nil` 改成 `return tx.Commit()` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
