[DadaVinqi]

• I have registered the PR 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:

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

go test ./pkg/datasource/sql/...
# 1773 passed in 23 packages

go test -race ./pkg/datasource/sql \
  -run 'TestXA(ResourceManager|Conn)_|TestDBResource_CheckDBVersion|TestExecBatch(Context|InTxContext)' \
  -count=1
# 83 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?:

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.

[Copilot]

Pull request overview

This PR hardens the XA SQL datasource implementation around phase-two correctness (status direction, idempotency, retry semantics) and physical connection ownership, including MySQL-specific capability probing and phase-two error classification, with substantial regression and concurrency test coverage.

Changes:

• Add directional, cache-aware phase-two handling with optional database-specific terminal/unretryable classification.
• Preserve original phase-one END/PREPARE errors while still performing compensating rollback when needed.
• Introduce synchronized keeper/pool/physical-close ownership rules to prevent premature reuse or loss of retry state; add MySQL xa_detach_on_prepare probing.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/datasource/sql/xa/xa_resource.go	Adds optional XAPhaseTwoErrorClassifier extension interface for phase-two error classification.
pkg/datasource/sql/xa/mysql_xa_connection.go	Implements MySQL phase-two rollback/protocol error classification.
pkg/datasource/sql/xa/mysql_xa_connection_test.go	Adds unit coverage for MySQL phase-two classification behavior.
pkg/datasource/sql/xa_resource_manager.go	Updates phase-two commit/rollback status semantics, cache idempotency, and classifier integration.
pkg/datasource/sql/xa_resource_manager_test.go	Adds table-driven regression tests for phase-two status/caching/idempotency and retry behavior.
pkg/datasource/sql/types/const.go	Corrects/extends MySQL/MariaDB XA-related error code constants.
pkg/datasource/sql/types/const_test.go	Extends tests to validate XA error code constants.
pkg/datasource/sql/db.go	Adds MySQL detach-on-prepare probing and conservative fallback behavior.
pkg/datasource/sql/conn_xa.go	Preserves phase-one errors, synchronizes keeper/pool close ownership, and enforces pool discard for kept branches.
pkg/datasource/sql/conn_xa_test.go	Adds regression/concurrency tests for error preservation, forced-close cleanup, ownership transfer, and session probing.
changes/dev.md	Records the bugfix entry for #1147 in release notes.
changes/dev_zh.md	Records the bugfix entry for #1147 in Chinese release notes.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 77.35192% with 65 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.16%. Comparing base (7c162b6) to head (f1cfb0a).

Files with missing lines	Patch %	Lines
pkg/datasource/sql/xa_resource_manager.go	68.75%	25 Missing and 5 partials ⚠️
pkg/datasource/sql/conn_xa.go	84.42%	12 Missing and 7 partials ⚠️
pkg/datasource/sql/db.go	70.37%	12 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1148      +/-   ##
==========================================
+ Coverage   61.52%   62.16%   +0.63%     
==========================================
  Files         309      309              
  Lines       22625    22845     +220     
==========================================
+ Hits        13920    14201     +281     
+ Misses       7420     7349      -71     
- Partials     1285     1295      +10     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Ethan-Xingyue]

plz fix conflicts～

[DadaVinqi]

Conflicts are resolved in 62cde06 by merging the latest master.

The resolution keeps both sides of the XA hold behavior:

• preserve the per-connection shouldBeHeld decision from fix: preserve XA retry state and connection ownership #1148;
• retain unknown database types conservatively, as introduced by fix: correct XA connection hold fallback #1150;
• keep both the upstream TestXAConn_ShouldBeHeld coverage and the existing ownership/timeout tests.

Validation after the merge:

• go test ./pkg/datasource/sql/... — 1879 passed
• focused race tests — 91 passed
• go vet ./pkg/datasource/sql/... — passed

[Ethan-Xingyue]

plz fix conflicts

[DadaVinqi]

Conflicts resolved.

[DadaVinqi]

I found that IsAlreadyRollbacked was used by both BranchCommit and BranchRollback, so adding XAER_NOTA directly to it would incorrectly make commit-side XAER_NOTA terminal.

I have updated the phase-two classifier to consider the requested operation:

• XAER_NOTA on rollback is treated as an idempotent success;
• XAER_NOTA on commit remains ambiguous and retryable.

I also removed the duplicate local rollback in the timeout path and added regression coverage for the timeout → local rollback → TC rollback → XAER_NOTA sequence described in #676.

[XiaoFeiASK]

plz fix CI and Conflicts.

[DadaVinqi]

Thanks for flagging this! I merged the latest master, resolved the changelog conflicts, and removed the unused import that caused the CI failures. The fix is in 07038ab. GitHub now reports no merge conflicts, and the new CI runs are awaiting maintainer approval.