From 83b01584ff9a95dd4367604a9a3d466e1084d2fb Mon Sep 17 00:00:00 2001 From: Deepak Dixit Date: Thu, 25 Jun 2026 13:42:23 +0530 Subject: [PATCH 1/2] Fix zombie XA connection after deadlock by deferring closeTxConnections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closeTxConnections() was called at the top of rollback() and commit(), before ut.rollback()/ut.commit(). When MySQL auto-rolled back an XA branch due to a deadlock (XA_RBDEADLOCK), BTM attempted XA END on the connection during close(), failed silently, and lost track of the XA resource. The connection was then returned to the pool with an unresolved ROLLBACK_ONLY XA branch still open in MySQL. The next thread to acquire that connection received XAER_RMFAIL on XA START. The fix removes the premature closeTxConnections() calls from both methods. clearCurrent() in the finally block already calls closeTxConnections() as a safety net — it now becomes the sole caller, always running after the JTA operation completes and the XA lifecycle (XA END + XA ROLLBACK/COMMIT) has been properly finalized on all enlisted resources. suspend() is unaffected as it intentionally closes connections before suspending. --- .../moqui/impl/context/TransactionFacadeImpl.groovy | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/framework/src/main/groovy/org/moqui/impl/context/TransactionFacadeImpl.groovy b/framework/src/main/groovy/org/moqui/impl/context/TransactionFacadeImpl.groovy index 755251634..2b42b4382 100644 --- a/framework/src/main/groovy/org/moqui/impl/context/TransactionFacadeImpl.groovy +++ b/framework/src/main/groovy/org/moqui/impl/context/TransactionFacadeImpl.groovy @@ -395,7 +395,6 @@ class TransactionFacadeImpl implements TransactionFacade { int status = ut.getStatus() // logger.warn("================ commit TX, currentStatus=${status}") - txStackInfo.closeTxConnections() if (status == Status.STATUS_MARKED_ROLLBACK) { if (txStackInfo.rollbackOnlyInfo != null) { logger.warn("Tried to commit transaction but marked rollback only, doing rollback instead; rollback-only was set here:", txStackInfo.rollbackOnlyInfo.rollbackLocation) @@ -411,6 +410,8 @@ class TransactionFacadeImpl implements TransactionFacade { if (status != Status.STATUS_NO_TRANSACTION) logger.warn((String) "Not committing transaction because status is " + getStatusString(), new Exception("Bad TX status location")) } + // closeTxConnections() is not called here; clearCurrent() in finally handles it after the + // JTA operation completes, so connections are never released before XA END + XA COMMIT/ROLLBACK. } catch (RollbackException e) { if (txStackInfo.rollbackOnlyInfo != null) { logger.warn("Could not commit transaction, was marked rollback-only. The rollback-only was set here: ", txStackInfo.rollbackOnlyInfo.rollbackLocation) @@ -455,7 +456,6 @@ class TransactionFacadeImpl implements TransactionFacade { if (ut == null) throw new IllegalStateException("No transaction manager in place") TxStackInfo txStackInfo = getTxStackInfo() try { - txStackInfo.closeTxConnections() // logger.warn("================ rollback TX, currentStatus=${getStatus()}") if (getStatus() == Status.STATUS_NO_TRANSACTION) { @@ -476,6 +476,12 @@ class TransactionFacadeImpl implements TransactionFacade { } ut.rollback() + // closeTxConnections() is intentionally NOT called here. clearCurrent() in the finally block + // calls it after ut.rollback() completes, ensuring BTM can issue XA END + XA ROLLBACK on all + // enlisted resources while they are still tracked. Calling it before ut.rollback() (the previous + // behavior) could leave XA connections in a zombie ROLLBACK_ONLY state in the pool when a MySQL + // deadlock (XA_RBDEADLOCK) occurs, causing XAER_RMFAIL on the next transaction that acquires + // the same connection. } catch (IllegalStateException e) { throw new TransactionException("Could not rollback transaction", e) } catch (SystemException e) { From fb24de878b588bb8ad162c0f540c82d36d77a3e6 Mon Sep 17 00:00:00 2001 From: Deepak Dixit Date: Tue, 4 Aug 2026 12:45:31 +0530 Subject: [PATCH 2/2] Fixed: XAER_RMFAIL cascade causing connection pool exhaustion during XA transactions Root cause: When MySQL marks an XA branch as rollback-only (e.g. after a deadlock or timeout), Bitronix auto-enlistment fails with XAER_RMFAIL on the next statement call. Because PoolingDataSource does not implement javax.sql.XADataSource, Moqui's getConnection() bypasses enlistConnection() entirely and calls ds.getConnection() directly, returning a ConnectionJavaProxy. When XA START is rejected (XAER_RMFAIL), the connection is never enrolled in the TX, but Bitronix's release() still requeues it to the pool with the dirty XA branch intact. Every subsequent checkout of that connection fails the same way, producing a cascade XAER_RMFAIL errors. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix — ContextJavaUtil.java (ConnectionWrapper): - Added volatile destroyOnClose flag. - Added isXaRmFail() that traverses the exception cause chain checking for XAException.XAER_RMFAIL (JTA spec constant -7). Avoids fragile string matching on exception messages which are locale/driver-version sensitive. - Wrapped createStatement(), prepareStatement(String), and prepareCall(String) with try-catch: sets destroyOnClose = true on XAER_RMFAIL and rethrows. Only the three base variants are wrapped because Bitronix auto-enlistment fires once on the first statement call, and EntityQueryBuilder exclusively uses prepareStatement(String sql). - closeInternal() calls physicallyDestroy() when destroyOnClose is true. physicallyDestroy() uses reflection to invoke JdbcPooledConnection.close() via ConnectionJavaProxy.getPooledConnection(), which unregisters the connection from the Bitronix pool and closes the physical DB connection, preventing the dirty XA branch from being recycled. Falls back to con.close() if reflection fails (non-Bitronix pools). Fix — TransactionFacadeImpl.groovy: - suspend(): removed closeTxConnections() before tm.suspend(). Stashed connections belong to the outer TX's suspended XA branches and must not be returned to pool mid-suspend; they are released when the outer TX resumes and eventually commits or rolls back. - enlistConnection(): added finally { if (con.close() } to destroy the XAConnection on enlistment or connection-retrieval failure for raw XADataSource paths (non-Bitronix), preventing the same dirty-connection cascade on those configurations. --- .../moqui/impl/context/ContextJavaUtil.java | 64 +++++++++++++++++-- .../impl/context/TransactionFacadeImpl.groovy | 18 ++++-- 2 files changed, 73 insertions(+), 9 deletions(-) diff --git a/framework/src/main/groovy/org/moqui/impl/context/ContextJavaUtil.java b/framework/src/main/groovy/org/moqui/impl/context/ContextJavaUtil.java index f3054aec4..64256cb5a 100644 --- a/framework/src/main/groovy/org/moqui/impl/context/ContextJavaUtil.java +++ b/framework/src/main/groovy/org/moqui/impl/context/ContextJavaUtil.java @@ -40,8 +40,11 @@ import jakarta.transaction.Synchronization; import jakarta.transaction.Transaction; +import javax.transaction.xa.XAException; import javax.transaction.xa.XAResource; import java.io.IOException; +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Proxy; import java.math.BigDecimal; import java.math.RoundingMode; import java.sql.*; @@ -471,6 +474,9 @@ public static class ConnectionWrapper implements Connection { protected Connection con; TransactionFacadeImpl tfi; String groupName; + // When XAER_RMFAIL is detected on this connection, mark it for physical destruction on close + // so the dirty XA branch is not returned to the Bitronix pool and recycled indefinitely. + private volatile boolean destroyOnClose = false; public ConnectionWrapper(Connection con, TransactionFacadeImpl tfi, String groupName) { this.con = con; @@ -481,12 +487,62 @@ public ConnectionWrapper(Connection con, TransactionFacadeImpl tfi, String group public String getGroupName() { return groupName; } public void closeInternal() throws SQLException { - con.close(); + if (destroyOnClose) { + // Physical destroy: removes connection from Bitronix pool entirely. + // Prevents XAER_RMFAIL cascade: dirty XA branch stays on pool connection when + // release() is called (XA END no-op + requeue), causing every subsequent checkout + // to fail. JdbcPooledConnection.close() destroys the physical connection instead. + physicallyDestroy(); + } else { + // Normal path: release() back to pool via ConnectionJavaProxy.close(). + con.close(); + } } - @Override public Statement createStatement() throws SQLException { return con.createStatement(); } - @Override public PreparedStatement prepareStatement(String sql) throws SQLException { return con.prepareStatement(sql); } - @Override public CallableStatement prepareCall(String sql) throws SQLException { return con.prepareCall(sql); } + private void physicallyDestroy() { + // con is a JDK Proxy; handler is ConnectionJavaProxy which has getPooledConnection(). + // JdbcPooledConnection.close() unregisters from pool and closes the physical DB connection. + try { + if (Proxy.isProxyClass(con.getClass())) { + InvocationHandler handler = Proxy.getInvocationHandler(con); + Object jdbcPC = handler.getClass().getMethod("getPooledConnection").invoke(handler); + jdbcPC.getClass().getMethod("close").invoke(jdbcPC); + return; + } + } catch (Throwable t) { + logger.warn("Could not physically destroy Bitronix pooled connection for group " + groupName + ", falling back to close()", t); + } + try { con.close(); } catch (Throwable t) { logger.warn("Error on fallback close for group " + groupName, t); } + } + + private static boolean isXaRmFail(Throwable e) { + for (Throwable t = e; t != null; t = t.getCause()) { + if (t instanceof XAException && ((XAException) t).errorCode == XAException.XAER_RMFAIL) return true; + } + return false; + } + + @Override public Statement createStatement() throws SQLException { + try { return con.createStatement(); } + catch (SQLException e) { + if (isXaRmFail(e)) destroyOnClose = true; + throw e; + } + } + @Override public PreparedStatement prepareStatement(String sql) throws SQLException { + try { return con.prepareStatement(sql); } + catch (SQLException e) { + if (isXaRmFail(e)) destroyOnClose = true; + throw e; + } + } + @Override public CallableStatement prepareCall(String sql) throws SQLException { + try { return con.prepareCall(sql); } + catch (SQLException e) { + if (isXaRmFail(e)) destroyOnClose = true; + throw e; + } + } @Override public String nativeSQL(String sql) throws SQLException { return con.nativeSQL(sql); } @Override public void setAutoCommit(boolean autoCommit) throws SQLException { con.setAutoCommit(autoCommit); } @Override public boolean getAutoCommit() throws SQLException { return con.getAutoCommit(); } diff --git a/framework/src/main/groovy/org/moqui/impl/context/TransactionFacadeImpl.groovy b/framework/src/main/groovy/org/moqui/impl/context/TransactionFacadeImpl.groovy index 2b42b4382..914197ddc 100644 --- a/framework/src/main/groovy/org/moqui/impl/context/TransactionFacadeImpl.groovy +++ b/framework/src/main/groovy/org/moqui/impl/context/TransactionFacadeImpl.groovy @@ -538,10 +538,6 @@ class TransactionFacadeImpl implements TransactionFacade { return false } - // close connections before suspend, let the pool reuse them - TxStackInfo txStackInfo = getTxStackInfo() - txStackInfo.closeTxConnections() - Transaction tx = tm.suspend() // only do these after successful suspend pushTxStackInfo(tx, new Exception("Transaction Suspend Location")) @@ -579,12 +575,24 @@ class TransactionFacadeImpl implements TransactionFacade { @Override Connection enlistConnection(XAConnection con) { if (con == null) return null + boolean enlisted = false try { XAResource resource = con.getXAResource() this.enlistResource(resource) - return con.getConnection() + // enlistResource succeeded: XA branch + Connection c = con.getConnection() + enlisted = true + return c } catch (SQLException e) { throw new TransactionException("Could not enlist connection in transaction", e) + } finally { + if (!enlisted) { + // Enlistment or connection retrieval failed. Close and destroy the XAConnection so + // Bitronix removes the physical connection from the pool. This prevents the cascade + // where a dirty XA branch (e.g. XAER_RMFAIL) is returned to the pool and triggers + // XAER_RMFAIL on every subsequent checkout of the same physical connection. + try { con.close() } catch (Throwable t) { logger.warn("Error closing XAConnection after enlist failure: " + t.getMessage()) } + } } }