You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@seata.apache.org by "tsln1998 (via GitHub)" <gi...@apache.org> on 2026/09/22 14:52:13 UTC
[PR] fix(sql): prevent XA rollback nil pointer panic after cleanup [incubator-seata-go]
tsln1998 opened a new pull request, #1197:
URL: https://github.com/apache/incubator-seata-go/pull/1197
Fix a nil pointer panic in the XA rollback error path and add regression coverage for rollback after context cancellation and physical connection cleanup.
## Problem
When a request is canceled, the local transaction and Seata XA rollback paths can overlap. The XA `END` or `ROLLBACK` operation may fail after the connection has already been closed. The existing rollback implementation cleaned the XA branch context and then called `rollbackErrorHandle`, which dereferenced `xaBranchXid` after it had been cleared.
This caused an API process panic instead of returning an observable rollback error. The previous implementation also returned immediately when `XA END` failed with an unclassified error, so it could skip the following `XA ROLLBACK` attempt and leave the branch cleanup incomplete.
## Changes
- Capture the branch ID before XA branch cleanup clears `xaBranchXid`.
- Continue the rollback attempt after a non-terminal `XA END` error.
- Preserve both `XA END` and `XA ROLLBACK` failures with `errors.Join`.
- Perform branch context cleanup once before constructing the final error.
- Return a wrapped, branch-specific rollback error without dereferencing cleared state.
- Add regression coverage proving that `XAConn.Close` delegates to the physical connection.
- Add regression coverage for canceled rollback, repeated rollback, and the no-panic guarantee.
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix(sql): prevent XA rollback nil pointer panic after cleanup [incubator-seata-go]
Posted by "tsln1998 (via GitHub)" <gi...@apache.org>.
tsln1998 commented on code in PR #1197:
URL: https://github.com/apache/incubator-seata-go/pull/1197#discussion_r4102989616
##########
pkg/datasource/sql/conn_xa.go:
##########
@@ -512,24 +518,31 @@ func (c *XAConn) Rollback(ctx context.Context) error {
log.Infof("XA branch already ended, continuing with rollback for xid: %s", c.txCtx.XID)
// Already ended, continue with rollback
} else {
- return c.rollbackErrorHandle()
+ rollbackErr = err
}
}
// Then perform XA rollback
- if c.XaRollback(ctx, c.xaBranchXid) != nil {
- c.cleanXABranchContext()
- return c.rollbackErrorHandle()
+ if err := c.XaRollback(ctx, c.xaBranchXid); err != nil {
Review Comment:
`XAConn.Rollback` now calls `c.invalidate()` when `XaRollback` fails. `TestXAConn_Rollback_FailureDiscardsPooledConnection` uses `database/sql` and an ordinary driver error, then verifies physical connection close and replacement by a new connection. It covers explicit and cancellation-triggered rollback failures for both held and non-held connections. These cases failed on the previous PR head and pass with the fix.
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix(sql): prevent XA rollback nil pointer panic after cleanup [incubator-seata-go]
Posted by "XiaoFeiASK (via GitHub)" <gi...@apache.org>.
XiaoFeiASK commented on code in PR #1197:
URL: https://github.com/apache/incubator-seata-go/pull/1197#discussion_r4291090519
##########
pkg/datasource/sql/conn_xa.go:
##########
@@ -512,24 +517,33 @@ func (c *XAConn) Rollback(ctx context.Context) error {
log.Infof("XA branch already ended, continuing with rollback for xid: %s", c.txCtx.XID)
// Already ended, continue with rollback
} else {
- return c.rollbackErrorHandle()
+ rollbackErr = err
}
}
// Then perform XA rollback
- if c.XaRollback(ctx, c.xaBranchXid) != nil {
- c.cleanXABranchContext()
- return c.rollbackErrorHandle()
+ if err := c.XaRollback(ctx, c.xaBranchXid); err != nil {
+ // The branch may still be open; do not return this connection to the pool.
+ c.invalidate()
+ if rollbackErr != nil {
+ rollbackErr = errors.Join(rollbackErr, err)
+ } else {
+ rollbackErr = err
+ }
}
c.rollBacked = true
Review Comment:
`c.rollBacked = true` is set even when `XA ROLLBACK` fails, which causes subsequent `Rollback()` calls to skip the rollback logic and incorrectly return `nil`. The `rollBacked` flag should only be set when the rollback succeeds. Consider moving it into an `else` block.
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix(sql): prevent XA rollback nil pointer panic after cleanup [incubator-seata-go]
Posted by "tsln1998 (via GitHub)" <gi...@apache.org>.
tsln1998 commented on code in PR #1197:
URL: https://github.com/apache/incubator-seata-go/pull/1197#discussion_r4102994933
##########
pkg/datasource/sql/conn_xa.go:
##########
@@ -512,24 +518,31 @@ func (c *XAConn) Rollback(ctx context.Context) error {
log.Infof("XA branch already ended, continuing with rollback for xid: %s", c.txCtx.XID)
// Already ended, continue with rollback
} else {
- return c.rollbackErrorHandle()
+ rollbackErr = err
}
}
// Then perform XA rollback
- if c.XaRollback(ctx, c.xaBranchXid) != nil {
- c.cleanXABranchContext()
- return c.rollbackErrorHandle()
+ if err := c.XaRollback(ctx, c.xaBranchXid); err != nil {
+ if rollbackErr != nil {
+ rollbackErr = errors.Join(rollbackErr, err)
+ } else {
+ rollbackErr = err
+ }
}
c.rollBacked = true
}
c.cleanXABranchContext()
+ if rollbackErr != nil {
Review Comment:
Yes, preserving the unclassified `XA END` error is intentional in this PR. A non-nil return here means that a step in the rollback sequence failed; it does not necessarily mean the branch is still open.
When `XA END` fails but `XA ROLLBACK` succeeds, the branch cleanup completes and the END error remains available to the caller. The connection is not invalidated solely because END failed. If ROLLBACK also fails, both causes are preserved with `errors.Join`, and the connection is invalidated.
`TestXAConn_Rollback_ErrorHandling` now covers both outcomes, including MySQL error 1614. Treating `XA_RB*` codes as an already-ended state would change the database-specific classification policy; this PR leaves that policy unchanged.
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix(sql): prevent XA rollback nil pointer panic after cleanup [incubator-seata-go]
Posted by "tsln1998 (via GitHub)" <gi...@apache.org>.
tsln1998 commented on code in PR #1197:
URL: https://github.com/apache/incubator-seata-go/pull/1197#discussion_r4103028940
##########
pkg/datasource/sql/conn_xa_test.go:
##########
@@ -605,6 +638,45 @@ func TestXAConn_Rollback_PreparedBranchStillRollsBack(t *testing.T) {
"XA ROLLBACK must run so a PREPARED branch releases its locks")
}
+func TestXAConn_RollbackErrorAfterContextCancellationDoesNotPanic(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ defer func() {
+ simulateExecContextError = nil
+ ctrl.Finish()
+ CleanTxHooks()
+ }()
+
+ xaConn, mockMgr := newMockXAConn(t, ctrl, 123)
+ mockMgr.EXPECT().BranchReport(gomock.Any(), gomock.Any()).AnyTimes().Return(nil)
+
+ ctx, cancel := context.WithCancel(tm.InitSeataContext(context.Background()))
+ tm.SetXID(ctx, uuid.New().String())
+
+ tx, err := xaConn.BeginTx(ctx, driver.TxOptions{})
+ assert.NoError(t, err)
+ cancel()
+
+ simulateExecContextError = func(query string) error {
+ upper := strings.ToUpper(strings.TrimSpace(query))
+ switch {
+ case strings.HasPrefix(upper, "XA ROLLBACK"):
+ return errors.New("conn closed")
+ case strings.HasPrefix(upper, "XA "):
+ return nil
+ }
+ return nil
+ }
+
+ assert.NotPanics(t, func() {
+ err := tx.Rollback()
+ if assert.Error(t, err) {
+ assert.Contains(t, err.Error(), "rollback xa branch")
+ assert.Contains(t, err.Error(), "123")
Review Comment:
Done in f2ca7db
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix(sql): prevent XA rollback nil pointer panic after cleanup [incubator-seata-go]
Posted by "tsln1998 (via GitHub)" <gi...@apache.org>.
tsln1998 commented on code in PR #1197:
URL: https://github.com/apache/incubator-seata-go/pull/1197#discussion_r4103030219
##########
pkg/datasource/sql/conn_xa_test.go:
##########
@@ -605,6 +638,45 @@ func TestXAConn_Rollback_PreparedBranchStillRollsBack(t *testing.T) {
"XA ROLLBACK must run so a PREPARED branch releases its locks")
}
+func TestXAConn_RollbackErrorAfterContextCancellationDoesNotPanic(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ defer func() {
+ simulateExecContextError = nil
+ ctrl.Finish()
+ CleanTxHooks()
+ }()
+
+ xaConn, mockMgr := newMockXAConn(t, ctrl, 123)
+ mockMgr.EXPECT().BranchReport(gomock.Any(), gomock.Any()).AnyTimes().Return(nil)
+
+ ctx, cancel := context.WithCancel(tm.InitSeataContext(context.Background()))
+ tm.SetXID(ctx, uuid.New().String())
+
+ tx, err := xaConn.BeginTx(ctx, driver.TxOptions{})
+ assert.NoError(t, err)
Review Comment:
Done in f2ca7db
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix(sql): prevent XA rollback nil pointer panic after cleanup [incubator-seata-go]
Posted by "tsln1998 (via GitHub)" <gi...@apache.org>.
tsln1998 commented on PR #1197:
URL: https://github.com/apache/incubator-seata-go/pull/1197#issuecomment-5829980395
Thanks for the detailed review, @Ethan-Xingyue. I've pushed the code and test updates in f2ca7dba93716138eddcf1a8a2137d1a8279fc91:
- Invalidate connections when XA rollback fails. Tests through `database/sql` verify physical close and replacement for explicit and cancellation-triggered rollback failures, covering held and non-held connections.
- Cover unclassified END errors, including MySQL error 1614, and verify END/ROLLBACK order, the captured branch ID, and `errors.Is` for both joined causes.
- Reset `rollBacked` when a new branch starts. A single-connection pool test verifies that two distinct branches each execute END and ROLLBACK.
- Address the test naming, setup assertions, variable placement, and helper placement comments, and add both English and Chinese changelog entries.
- Update the PR template with the changes checkbox and release note, and link #1147 as related. This PR addresses only part of XA failure handling, so #1147 should remain open.
I've explained the END-failure/ROLLBACK-success behavior in the relevant thread: the unclassified END error remains observable, and only a ROLLBACK failure invalidates the connection.
Repository-wide race tests and `go vet` pass locally with Go 1.20.14, matching the CI toolchain. `git diff --check` is clean, and local statement coverage for `XAConn.Rollback` is 95.2%, including the previously uncovered error paths. The pool tests use mocked physical drivers.
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix(sql): prevent XA rollback nil pointer panic after cleanup [incubator-seata-go]
Posted by "tsln1998 (via GitHub)" <gi...@apache.org>.
tsln1998 commented on code in PR #1197:
URL: https://github.com/apache/incubator-seata-go/pull/1197#discussion_r4102999434
##########
pkg/datasource/sql/conn_xa_test.go:
##########
@@ -605,6 +638,45 @@ func TestXAConn_Rollback_PreparedBranchStillRollsBack(t *testing.T) {
"XA ROLLBACK must run so a PREPARED branch releases its locks")
}
+func TestXAConn_RollbackErrorAfterContextCancellationDoesNotPanic(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ defer func() {
+ simulateExecContextError = nil
+ ctrl.Finish()
+ CleanTxHooks()
+ }()
+
+ xaConn, mockMgr := newMockXAConn(t, ctrl, 123)
+ mockMgr.EXPECT().BranchReport(gomock.Any(), gomock.Any()).AnyTimes().Return(nil)
+
+ ctx, cancel := context.WithCancel(tm.InitSeataContext(context.Background()))
+ tm.SetXID(ctx, uuid.New().String())
+
+ tx, err := xaConn.BeginTx(ctx, driver.TxOptions{})
+ assert.NoError(t, err)
+ cancel()
Review Comment:
In f2ca7db, `TestXAConn_Rollback_ErrorHandling` covers injected rollback errors without the ineffective cancellation. Separate cases in `TestXAConn_Rollback_FailureDiscardsPooledConnection` call `db.BeginTx(ctx, nil)`, cancel that context, and wait on a channel for `database/sql`'s automatic rollback and physical connection close. Those cases use per-connection mocks rather than the global error hook.
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix(sql): prevent XA rollback nil pointer panic after cleanup [incubator-seata-go]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1197:
URL: https://github.com/apache/incubator-seata-go/pull/1197#issuecomment-5788671772
## [Codecov](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1197?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `69.23077%` with `4 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 61.56%. Comparing base ([`0f3ebc2`](https://app.codecov.io/gh/apache/incubator-seata-go/commit/0f3ebc2bea56c12b2a1d290b8d8443ee76e4ce03?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e4e055d`](https://app.codecov.io/gh/apache/incubator-seata-go/commit/e4e055d92af14743a4c6d6ac04d5212fcdab29b0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1197?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [pkg/datasource/sql/conn\_xa.go](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1197?src=pr&el=tree&filepath=pkg%2Fdatasource%2Fsql%2Fconn_xa.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL2RhdGFzb3VyY2Uvc3FsL2Nvbm5feGEuZ28=) | 69.23% | [2 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1197?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #1197 +/- ##
=======================================
Coverage 61.55% 61.56%
=======================================
Files 309 309
Lines 22625 22632 +7
=======================================
+ Hits 13927 13933 +6
+ Misses 7416 7414 -2
- Partials 1282 1285 +3
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1197?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix(sql): prevent XA rollback nil pointer panic after cleanup [incubator-seata-go]
Posted by "tsln1998 (via GitHub)" <gi...@apache.org>.
tsln1998 commented on code in PR #1197:
URL: https://github.com/apache/incubator-seata-go/pull/1197#discussion_r4102991122
##########
pkg/datasource/sql/conn_xa.go:
##########
@@ -512,24 +518,31 @@ func (c *XAConn) Rollback(ctx context.Context) error {
log.Infof("XA branch already ended, continuing with rollback for xid: %s", c.txCtx.XID)
// Already ended, continue with rollback
} else {
- return c.rollbackErrorHandle()
+ rollbackErr = err
Review Comment:
Added `TestXAConn_Rollback_ErrorHandling` in f2ca7db. It injects MySQL error 1614 from `XA END`, verifies that `XA ROLLBACK` is still issued in order, and covers both successful and failed rollback. When both steps fail, `errors.Is` finds both original errors through the returned branch-specific error. Both previously uncovered paths are now exercised.
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix(sql): prevent XA rollback nil pointer panic after cleanup [incubator-seata-go]
Posted by "Ethan-Xingyue (via GitHub)" <gi...@apache.org>.
Ethan-Xingyue commented on code in PR #1197:
URL: https://github.com/apache/incubator-seata-go/pull/1197#discussion_r4080163346
##########
pkg/datasource/sql/conn_xa.go:
##########
@@ -512,24 +518,31 @@ func (c *XAConn) Rollback(ctx context.Context) error {
log.Infof("XA branch already ended, continuing with rollback for xid: %s", c.txCtx.XID)
// Already ended, continue with rollback
} else {
- return c.rollbackErrorHandle()
+ rollbackErr = err
}
}
// Then perform XA rollback
- if c.XaRollback(ctx, c.xaBranchXid) != nil {
- c.cleanXABranchContext()
- return c.rollbackErrorHandle()
+ if err := c.XaRollback(ctx, c.xaBranchXid); err != nil {
Review Comment:
When `XaRollback` fails, the branch context is cleared and `XATx.Rollback` reports `PhaseOne_Failed` (the TC then drops the branch without a phase-2 rollback), but unless the cause is `driver.ErrBadConn` or the driver closed the socket, `database/sql` puts the physical connection back into the pool with the XA transaction still open. For example, in an explicit XA tx on MySQL whose ctx is canceled just as a statement reaches the driver, `c.Rollback(ctx)` sends neither `XA END` nor `XA ROLLBACK` (both fail with `context.Canceled`) but still clears the context, so the follow-up `XATx.Rollback` with `context.Background()`, which finishes the rollback on master, is refused with "should NOT rollback on an inactive session". Please call `c.invalidate()` here, as `ATTx.finishCommitFailure` does when its rollback fails, so the connection is discarded and the server rolls the branch back on disconnect.
##########
pkg/datasource/sql/conn_xa.go:
##########
@@ -512,24 +518,31 @@ func (c *XAConn) Rollback(ctx context.Context) error {
log.Infof("XA branch already ended, continuing with rollback for xid: %s", c.txCtx.XID)
// Already ended, continue with rollback
} else {
- return c.rollbackErrorHandle()
+ rollbackErr = err
}
}
// Then perform XA rollback
- if c.XaRollback(ctx, c.xaBranchXid) != nil {
- c.cleanXABranchContext()
- return c.rollbackErrorHandle()
+ if err := c.XaRollback(ctx, c.xaBranchXid); err != nil {
+ if rollbackErr != nil {
+ rollbackErr = errors.Join(rollbackErr, err)
+ } else {
+ rollbackErr = err
+ }
}
c.rollBacked = true
Review Comment:
Pre-existing since #875 and widened here: `rollBacked` is only reset in `Close`/`CloseForce`, but `database/sql` reuses the same `XAConn` for the lifetime of the physical connection, so after any branch rolls back on it, the next branch's `Rollback` skips `XA END`/`XA ROLLBACK`, returns nil and reports `PhaseOne_Failed`, leaving that XA transaction open; with this PR the flag is also set when the rollback failed. Because `cleanXABranchContext()` clears `xaActive` right after, the flag never guards a retry within the same branch, so resetting it in `BeginTx` next to `c.xaActive = true` (or removing it) plus a two-branch test would fix it. Not blocking: fixing it here is the smallest change since this PR moves this line; otherwise please open a follow-up issue.
##########
pkg/datasource/sql/conn_xa.go:
##########
@@ -496,6 +496,9 @@ func (c *XAConn) cleanXABranchContext() {
}
func (c *XAConn) Rollback(ctx context.Context) error {
+ var rollbackErr error
+ var branchID uint64
Review Comment:
Nit: declare these where they are first set (`branchID := c.xaBranchXid.GetBranchId()` at L511, `var rollbackErr error` right before `if !c.rollBacked`) to keep their scope minimal.
##########
pkg/datasource/sql/conn_xa_test.go:
##########
@@ -605,6 +638,45 @@ func TestXAConn_Rollback_PreparedBranchStillRollsBack(t *testing.T) {
"XA ROLLBACK must run so a PREPARED branch releases its locks")
}
+func TestXAConn_RollbackErrorAfterContextCancellationDoesNotPanic(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ defer func() {
+ simulateExecContextError = nil
+ ctrl.Finish()
+ CleanTxHooks()
+ }()
+
+ xaConn, mockMgr := newMockXAConn(t, ctrl, 123)
+ mockMgr.EXPECT().BranchReport(gomock.Any(), gomock.Any()).AnyTimes().Return(nil)
+
+ ctx, cancel := context.WithCancel(tm.InitSeataContext(context.Background()))
+ tm.SetXID(ctx, uuid.New().String())
+
+ tx, err := xaConn.BeginTx(ctx, driver.TxOptions{})
+ assert.NoError(t, err)
+ cancel()
+
+ simulateExecContextError = func(query string) error {
+ upper := strings.ToUpper(strings.TrimSpace(query))
+ switch {
+ case strings.HasPrefix(upper, "XA ROLLBACK"):
+ return errors.New("conn closed")
+ case strings.HasPrefix(upper, "XA "):
+ return nil
+ }
+ return nil
+ }
+
+ assert.NotPanics(t, func() {
+ err := tx.Rollback()
+ if assert.Error(t, err) {
+ assert.Contains(t, err.Error(), "rollback xa branch")
+ assert.Contains(t, err.Error(), "123")
Review Comment:
Nit: the XID is a random UUID and may contain `123` itself; assert on `- [123]` and check `errors.Is` against a hoisted injected error so the test pins both the captured branch ID and the new `%w` wrapping.
##########
pkg/datasource/sql/conn_xa.go:
##########
@@ -512,24 +518,31 @@ func (c *XAConn) Rollback(ctx context.Context) error {
log.Infof("XA branch already ended, continuing with rollback for xid: %s", c.txCtx.XID)
// Already ended, continue with rollback
} else {
- return c.rollbackErrorHandle()
+ rollbackErr = err
Review Comment:
This is the PR's main behavior change (keep going to `XA ROLLBACK` after an unclassified `XA END` error), but neither this line nor the `errors.Join` branch at L528 is covered by a test. Please add a case where `XA END` fails with such an error (MySQL returns `ER_XA_RBDEADLOCK`, 1614, after a deadlock, and the branch must still be rolled back) and assert that `XA ROLLBACK` is still issued and that `errors.Is` finds both errors.
##########
pkg/datasource/sql/conn_xa.go:
##########
@@ -512,24 +518,31 @@ func (c *XAConn) Rollback(ctx context.Context) error {
log.Infof("XA branch already ended, continuing with rollback for xid: %s", c.txCtx.XID)
// Already ended, continue with rollback
} else {
- return c.rollbackErrorHandle()
+ rollbackErr = err
}
}
// Then perform XA rollback
- if c.XaRollback(ctx, c.xaBranchXid) != nil {
- c.cleanXABranchContext()
- return c.rollbackErrorHandle()
+ if err := c.XaRollback(ctx, c.xaBranchXid); err != nil {
+ if rollbackErr != nil {
+ rollbackErr = errors.Join(rollbackErr, err)
+ } else {
+ rollbackErr = err
+ }
}
c.rollBacked = true
}
c.cleanXABranchContext()
+ if rollbackErr != nil {
Review Comment:
Question: when `XA END` fails but `XA ROLLBACK` succeeds (e.g. `ER_XA_RBDEADLOCK`, where MySQL expects exactly this sequence), the branch is fully rolled back, yet `Rollback` returns an error and `tx.Rollback()` callers see a failed rollback. Is that intended? If not, a follow-up could treat the `XA_RB*` codes (1402/1613/1614) as already-ended in `MysqlXAErrorClassifier`, or only log the `XA END` error when `XA ROLLBACK` succeeds; not blocking.
##########
pkg/datasource/sql/conn_xa_test.go:
##########
@@ -605,6 +638,45 @@ func TestXAConn_Rollback_PreparedBranchStillRollsBack(t *testing.T) {
"XA ROLLBACK must run so a PREPARED branch releases its locks")
}
+func TestXAConn_RollbackErrorAfterContextCancellationDoesNotPanic(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ defer func() {
+ simulateExecContextError = nil
+ ctrl.Finish()
+ CleanTxHooks()
+ }()
+
+ xaConn, mockMgr := newMockXAConn(t, ctrl, 123)
+ mockMgr.EXPECT().BranchReport(gomock.Any(), gomock.Any()).AnyTimes().Return(nil)
+
+ ctx, cancel := context.WithCancel(tm.InitSeataContext(context.Background()))
+ tm.SetXID(ctx, uuid.New().String())
+
+ tx, err := xaConn.BeginTx(ctx, driver.TxOptions{})
+ assert.NoError(t, err)
+ cancel()
Review Comment:
`cancel()` does not affect this test: `XATx.Rollback` always passes `context.Background()` and the tx was not opened through `database/sql`, so the failure comes only from `simulateExecContextError`. Either drop it and rename the test (e.g. `TestXAConn_Rollback_XARollbackFailureDoesNotPanic`), or open the tx with `db.BeginTx(ctx, nil)` and cancel it so the rollback-on-cancel path is actually exercised; not blocking.
##########
pkg/datasource/sql/conn_xa_test.go:
##########
@@ -301,6 +311,29 @@ func TestXAConn_ShouldBeHeld(t *testing.T) {
}
}
+func TestXAConn_CloseDelegatesPreparedConnectionToPhysicalConnection(t *testing.T) {
Review Comment:
Nit: nothing in this fixture is prepared or invalidated (it is a plain, non-held PostgreSQL `XAConn`), so the test name and the assertion message overstate what is covered; e.g. `TestXAConn_Close_ClosesPhysicalConnWhenNotHeld`.
##########
pkg/datasource/sql/conn_xa_test.go:
##########
@@ -605,6 +638,45 @@ func TestXAConn_Rollback_PreparedBranchStillRollsBack(t *testing.T) {
"XA ROLLBACK must run so a PREPARED branch releases its locks")
}
+func TestXAConn_RollbackErrorAfterContextCancellationDoesNotPanic(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ defer func() {
+ simulateExecContextError = nil
+ ctrl.Finish()
+ CleanTxHooks()
+ }()
+
+ xaConn, mockMgr := newMockXAConn(t, ctrl, 123)
+ mockMgr.EXPECT().BranchReport(gomock.Any(), gomock.Any()).AnyTimes().Return(nil)
+
+ ctx, cancel := context.WithCancel(tm.InitSeataContext(context.Background()))
+ tm.SetXID(ctx, uuid.New().String())
+
+ tx, err := xaConn.BeginTx(ctx, driver.TxOptions{})
+ assert.NoError(t, err)
Review Comment:
Nit: use `require.NoError` (already used in this package, e.g. `batch_test.go`) so a failed `BeginTx` stops the test here instead of showing up as a nil-pointer panic inside `NotPanics`.
##########
pkg/datasource/sql/conn_xa_test.go:
##########
@@ -130,6 +130,16 @@ type fakePreparedStmt struct {
query string
}
+type closeTrackingDriverConn struct {
Review Comment:
Nit: this type is inserted between `fakePreparedStmt` and its methods; please move it below them or next to the `Close` test that uses it.
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix(sql): prevent XA rollback nil pointer panic after cleanup [incubator-seata-go]
Posted by "tsln1998 (via GitHub)" <gi...@apache.org>.
tsln1998 commented on code in PR #1197:
URL: https://github.com/apache/incubator-seata-go/pull/1197#discussion_r4102993510
##########
pkg/datasource/sql/conn_xa.go:
##########
@@ -512,24 +518,31 @@ func (c *XAConn) Rollback(ctx context.Context) error {
log.Infof("XA branch already ended, continuing with rollback for xid: %s", c.txCtx.XID)
// Already ended, continue with rollback
} else {
- return c.rollbackErrorHandle()
+ rollbackErr = err
}
}
// Then perform XA rollback
- if c.XaRollback(ctx, c.xaBranchXid) != nil {
- c.cleanXABranchContext()
- return c.rollbackErrorHandle()
+ if err := c.XaRollback(ctx, c.xaBranchXid); err != nil {
+ if rollbackErr != nil {
+ rollbackErr = errors.Join(rollbackErr, err)
+ } else {
+ rollbackErr = err
+ }
}
c.rollBacked = true
Review Comment:
Fixed in f2ca7db by resetting `rollBacked` next to `xaActive = true` after a new branch starts successfully. `TestXAConn_Rollback_ConsecutivePooledBranches` limits `database/sql` to one connection and runs two distinct branch IDs on it. Ordered expectations require START, END, and ROLLBACK for each branch; the second branch failed this test before the fix.
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] fix(sql): prevent XA rollback nil pointer panic after cleanup [incubator-seata-go]
Posted by "thunguo (via GitHub)" <gi...@apache.org>.
thunguo commented on PR #1197:
URL: https://github.com/apache/incubator-seata-go/pull/1197#issuecomment-6382938823
Plz resolve the comments and conflicts.
--
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org