DadaVinqi opened a new pull request, #1148: URL: https://github.com/apache/incubator-seata-go/pull/1148
- [x] I have registered the PR [changes](https://github.com/apache/incubator-seata-go/tree/master/changes). **What this PR does**: This PR fixes XA failure-path correctness, retry semantics, idempotency, and physical connection ownership. ## Problem and Root Cause ### 1. Incorrect phase-two status and terminal cache When XA phase two fails, the current implementation can return or record an incorrect terminal result: - `BranchCommit` returns a rollback-failure status when resource or connection acquisition fails. - Commit failure writes `PhasetwoCommitted` to `branchStatusCache`. - Rollback failure writes `PhasetwoRollbacked` to the cache. - Temporary database and connection failures are returned as unretryable. - A repeated callback does not consume the cached terminal state and accesses the database again. The root cause is that phase-two error handling does not distinguish commit and rollback directions, retryable and unretryable errors, confirmed terminal outcomes and ambiguous errors, or first execution and repeated callbacks. The cache is also updated from failure branches instead of only after a confirmed terminal result. ### 2. Phase-one errors can be lost When `XA END` or `XA PREPARE` fails, `commitErrorHandle` performs a compensating rollback. If that rollback succeeds, the previous implementation returns `nil`, because the local error variable only receives the rollback result. The original END/PREPARE error is therefore lost, and the caller can subsequently treat phase one as successful. ### 3. Retryable failures lose their owner connection `XAConn.XaCommit` and `XAConn.XaRollback` release the keeper entry regardless of the database result. For databases or configurations that still require the original physical connection, a temporary error therefore removes the only usable owner before the TC retries. The root cause is that database execution and connection ownership completion are handled independently. The code does not model whether phase two actually reached a terminal state. ### 4. Held connections can still be reused by `database/sql` Keeping an `XAConn` in `DBResource.keeper` does not automatically remove its physical connection from the `database/sql` pool. `ResetSession` previously returned `nil`, so the pool could reuse a prepared connection while the same `XAConn` was still referenced by the keeper. At the same time, pool discard, phase-two completion, and timeout close could operate on the same physical connection without a shared ownership guard. This can cause prepared connection reuse, overwritten branch state, phase-two/timeout races, duplicate closes, and keeper entries pointing to unusable connections. ### 5. Forced close can leak keeper state `XAConn.CloseForce` returns immediately when the underlying physical close returns an error. As a result, the keeper entry and local branch state are not cleaned up. ### 6. Cached terminal states are ignored `XAConn.termination` reads `branchStatusCache`, but only checks whether the cache lookup itself returned an error. A successfully retrieved `PhasetwoCommitted` or `PhasetwoRollbacked` value is ignored. ### 7. MySQL detach capability is determined incompletely MySQL 8.0.29 introduced cross-connection phase two, but it also depends on the effective session variable `@@session.xa_detach_on_prepare`. The previous implementation only checked the server version. Because this is a session variable, checking it once on an initialization connection is also insufficient: the actual XA physical connection may have a different value. ### 8. The existing error classifier is not sufficient for phase two `XAErrorClassifier.IsAlreadyEnded` only helps the XA END path. It cannot distinguish already committed, already rollbacked, deterministic protocol errors, ambiguous `XAER_NOTA`, or temporary connection and RM failures. ## Solution ### 1. Make terminal-state publication directional and idempotent Phase-two handling now: - returns commit-failure status for commit-path failures; - returns rollback-failure status for rollback-path failures; - treats unknown connection and RM errors as retryable; - writes terminal cache entries only after confirmed success; - checks the terminal cache before accessing the database; - returns cached same-direction outcomes as idempotent success; - returns cached opposite-direction outcomes as unretryable conflicts. The terminal cache is published before releasing the keeper, so a concurrent retry cannot observe neither the owner nor the terminal result. ### 2. Preserve the original phase-one error `commitErrorHandle` now receives the original failure explicitly. If compensating rollback succeeds, the original END/PREPARE error is returned. If rollback also fails, both errors are retained with `errors.Join`. ### 3. Release keeper ownership only after confirmed completion `XaCommit` and `XaRollback` release the keeper only when the database operation succeeds or the database-specific classifier proves the exact same terminal direction. Retryable errors keep the owner connection available for TC retry. Opposite terminal outcomes are recorded with their actual status and returned as unretryable conflicts. ### 4. Transfer pending connections out of the pool safely Held XA connections now have explicit synchronized ownership state: - `ResetSession` returns `driver.ErrBadConn` for a pending keeper-owned branch; - `database/sql` discards the wrapper instead of reusing it; - `XAConn.Close` does not close the physical connection while keeper owns it; - phase two or timeout cleanup becomes the physical owner; - a mutex serializes pool discard, phase two, and forced close; - a `physicalClosed` guard prevents duplicate closes. If phase two finishes before pool discard, the connection can remain reusable. If the pool already discarded it, phase-two completion closes it exactly once. ### 5. Always clean up after forced close `CloseForce` now performs keeper and branch-state cleanup regardless of the physical close result. The original close error is still returned. ### 6. Handle cached terminal states explicitly `termination` now rejects branches cached as `PhasetwoCommitted` or `PhasetwoRollbacked` and releases any remaining keeper ownership. ### 7. Probe MySQL detach capability on the actual XA connection For MySQL 8.0.29 and later: 1. The initialization probe marks the resource as requiring per-connection capability detection. 2. Every actual XA physical connection reads `@@session.xa_detach_on_prepare`. 3. When enabled, phase two may use a new connection. 4. When disabled, the original physical connection is retained. 5. If the probe fails, the implementation conservatively retains the owner. ### 8. Add optional phase-two error classification This PR introduces the optional `XAPhaseTwoErrorClassifier`: ```go type XAPhaseTwoErrorClassifier interface { IsAlreadyCommitted(err error) bool IsAlreadyRollbacked(err error) bool IsUnretryable(err error) bool } ``` The MySQL implementation keeps `XAER_NOTA` ambiguous and retryable, recognizes XA rollback terminal codes, treats `XAER_INVAL` and `XAER_OUTSIDE` as unretryable, and leaves unknown RM and connection failures retryable. ## Implementation Details ### `pkg/datasource/sql/xa_resource_manager.go` - Correct commit/rollback failure direction. - Consume cached terminal status before database access. - Publish terminal status before releasing keeper ownership. - Handle same-direction retries as idempotent success. - Handle opposite terminal states as unretryable conflicts. - Apply database-specific phase-two classification. - Force cleanup for deterministic unretryable errors. - Preserve wrapped connection acquisition errors. ### `pkg/datasource/sql/conn_xa.go` - Preserve END/PREPARE errors through `commitErrorHandle`. - Use `errors.Join` when compensating rollback also fails. - Retain keeper entries on retryable phase-two errors. - Add synchronized keeper and physical connection ownership. - Return `driver.ErrBadConn` for pool reuse of held branches. - Prevent duplicate physical closes. - Serialize phase two and timeout cleanup. - Release keeper on XA START failure. - Always clean keeper state in `CloseForce`. - Handle cached terminal states in `termination`. ### `pkg/datasource/sql/db.go` - Keep MySQL owner connections conservatively by default. - Detect MySQL 8.0.29+ detach capability. - Mark applicable resources for per-connection session probing. - Parse `xa_detach_on_prepare` from driver-native result types. - Fall back to owner retention on probe failure. ### `pkg/datasource/sql/xa/xa_resource.go` - Add the optional `XAPhaseTwoErrorClassifier`. - Keep the existing `XAResource` and `XAResourceFactory` contracts unchanged. ### `pkg/datasource/sql/xa/mysql_xa_connection.go` - Implement MySQL phase-two terminal and protocol-error classification. - Keep `XAER_NOTA` unclassified because it does not prove a terminal direction. ### `pkg/datasource/sql/types/const.go` Correct and extend MySQL/MariaDB XA error constants for `XAER_NOTA`, `XAER_INVAL`, `XAER_RMFAIL`, `XAER_OUTSIDE`, `XAER_RMERR`, `XA_RBROLLBACK`, `XA_RBTIMEOUT`, and `XA_RBDEADLOCK`. ### Regression Tests Added coverage for: - correct commit/rollback status direction; - terminal cache publication; - repeated same-direction callbacks; - opposite-direction callbacks; - retryable and unretryable errors; - confirmed committed/rollbacked outcomes; - original Prepare error preservation; - forced-close cleanup; - XA START cleanup; - pool-to-keeper ownership transfer; - phase-two/timeout close concurrency; - single physical close ownership; - MySQL version and detach combinations; - per-session detach changes; - conservative fallback on capability probe failure; - MySQL phase-two error classification. **Which issue(s) this PR fixes**: Fixes #1147 **Special notes for your reviewer**: - `branchStatusCache` remains a process-local idempotency optimization and is not used as durable recovery storage. - `XAER_NOTA` remains retryable because it cannot prove whether a branch committed, rolled back, or never existed. - The existing `XAResource` interface is unchanged. - Connection retention is conservative whenever detach capability cannot be proven. - Most added lines are regression and concurrency tests for previously uncovered failure paths. ## Test Plan ```bash go test ./pkg/datasource/sql/... # 1652 passed in 23 packages go test -race ./pkg/datasource/sql \ -run 'TestXA(ResourceManager|Conn)_|TestDBResource_CheckDBVersion' \ -count=1 # 62 passed in 1 package go test ./pkg/datasource/sql/xa ./pkg/datasource/sql/types -count=1 # 590 passed in 2 packages go vet ./pkg/datasource/sql/... git diff --check ``` On Go 1.26.5 / macOS arm64, the repository-wide test command reaches an unrelated pre-existing SIGBUS in a gomonkey-based gRPC test. The modified SQL datasource packages and focused race suites pass. **Does this PR introduce a user-facing change?**: ```release-note Fix XA failure handling so retryable phase-two operations preserve their connection ownership, repeated callbacks remain idempotent, and failed branches are no longer reported or cached as successful. ``` Made with [Cursor](https://cursor.com) -- 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]
