DadaVinqi opened a new issue, #1147: URL: https://github.com/apache/incubator-seata-go/issues/1147
## ✅ Verification Checklist - [x] I searched the existing issues and confirmed this is not a duplicate. - [x] I am willing to fix this bug. ## 🔧 Environment Information - **Go version:** go1.26.5 - **Seata-Go version:** `master@9992f1b7ae8fafb8fd6e8850e6977c0545c28c97` - **Operating system:** macOS arm64 ## 📝 Bug Description Several XA failure paths on the current master return or record a successful terminal state even though the database operation failed. Some paths also lose the original error or release the held XA connection before the TC can retry. The problems are concentrated in `pkg/datasource/sql/xa_resource_manager.go` and `pkg/datasource/sql/conn_xa.go`: 1. `BranchCommit` returns `PhasetwoRollbackFailedUnretryable` when obtaining the phase-two connection fails. This is the wrong operation direction. 2. A database commit error writes `PhasetwoCommitted` to `branchStatusCache`; a rollback error writes `PhasetwoRollbacked`. 3. Commit and rollback errors are returned as unretryable even when they are temporary connection or RM failures. 4. `XAConn.XaCommit` and `XAConn.XaRollback` release the keeper entry even when the database operation fails, so a retry can lose the owner connection. 5. `XAConn.commitErrorHandle` returns `nil` when END/PREPARE fails but the compensating rollback succeeds. The caller can then report phase one as successful even though prepare failed. 6. `XAConn.CloseForce` returns immediately when the physical close fails, leaving the keeper entry and branch state uncleared. 7. `XAConn.termination` reads the cached branch status but only checks the cache lookup error; committed/rollbacked terminal values are ignored. Potential impact: - TC may stop retrying a branch that is still pending. - A failed phase-two operation may be remembered as committed or rollbacked. - The RM may release the only usable owner connection before a retry. - A phase-one prepare failure may be reported as success. - Keeper entries can leak after connection-close failures. Related issue [#709](https://github.com/apache/incubator-seata-go/issues/709) covered a different early-rollback reporting race and was closed by #717. The failure paths above remain reproducible on the current master. ## 🔄 Steps to Reproduce ### Phase-two status/cache 1. Create an `XAResourceManager` with a `DBResource` whose held `XAConn` returns an error from `XAResource.Commit`. 2. Call `BranchCommit`. 3. Observe that the method returns `PhasetwoCommitFailedUnretryable`, writes `PhasetwoCommitted` to the cache, and releases the keeper entry. 4. Repeat with `XAResource.Rollback`; the failed rollback is cached as `PhasetwoRollbacked`. 5. Call `BranchCommit` with a missing resource and observe that it returns a rollback-failure status. ### Swallowed prepare error 1. Use an `XAConn` whose `XA END` succeeds, `XA PREPARE` returns an error, and compensating `XA ROLLBACK` succeeds. 2. Call `XAConn.Commit`. 3. Observe that the method returns `nil`, losing the prepare error. ### Keeper cleanup 1. Put an `XAConn` in the resource keeper. 2. Make the underlying `driver.Conn.Close` return an error. 3. Call `CloseForce`. 4. Observe that the keeper entry is still present. I have local regression tests that fail on `master@9992f1b` and pass after the proposed fixes. ## ✅ Expected Behavior - Commit failures return commit-failure status; rollback failures return rollback-failure status. - Temporary connection/RM errors remain retryable. - A terminal status is cached only after the database operation succeeds or the database-specific classifier proves the same terminal outcome. - Keeper ownership is retained while the operation is retryable. - END/PREPARE errors are preserved even when compensating rollback succeeds. - Forced close always clears keeper and local branch state, while still returning the physical close error. - Cached committed/rollbacked states are handled explicitly by `termination`. ## ❌ Actual Behavior - Failed operations can be cached as successful terminal states. - Commit resource lookup returns a rollback-failure status. - Retryable failures are reported as unretryable. - Keeper entries are released on database errors and leaked on close errors. - Prepare failures can be swallowed. - Cached terminal values are ignored. ## 💡 Possible Solution - Correct the commit/rollback status direction and default temporary failures to retryable. - Write `branchStatusCache` only after confirmed success. - Release keeper entries only after confirmed same-direction terminal outcome. - Pass the original END/PREPARE cause into `commitErrorHandle` and preserve it; combine rollback errors with `errors.Join`. - Run keeper and local-state cleanup regardless of the physical close result. - Make `termination` reject cached committed/rollbacked terminal states. - Add table-driven regression tests for success, retryable failure, missing resources, error preservation, keeper retention, and close cleanup. -- 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]
