diff --git a/teaql-provider-jdbc/src/main/java/io/teaql/provider/jdbc/JdbcSqlExecutor.java b/teaql-provider-jdbc/src/main/java/io/teaql/provider/jdbc/JdbcSqlExecutor.java index e945a2c7..295a4143 100644 --- a/teaql-provider-jdbc/src/main/java/io/teaql/provider/jdbc/JdbcSqlExecutor.java +++ b/teaql-provider-jdbc/src/main/java/io/teaql/provider/jdbc/JdbcSqlExecutor.java @@ -509,10 +509,43 @@ public void executeInTransaction(Runnable action) { } catch (SQLException e) { throw new RuntimeException("JDBC transaction failed", e); } finally { - try { - connection.close(); - } catch (SQLException | RuntimeException | Error cleanupFailure) { - warnTransactionCleanup("connection close", committed, outcomeUnknown); + if (rollbackUnconfirmed) { + // JDBC leaves close() with an active transaction implementation-defined. + // A pool may even commit when close() returns the connection, so never + // return an unconfirmed transaction to it. A one-shot rollback fault + // can recover on retry, allowing a confirmed rollback before close. + boolean rollbackConfirmed = false; + try { + connection.rollback(); + rollbackConfirmed = true; + } catch (SQLException | RuntimeException | Error cleanupFailure) { + warnTransactionCleanup("rollback retry before connection disposal", committed, outcomeUnknown); + } + if (rollbackConfirmed) { + try { + connection.close(); + } catch (SQLException | RuntimeException | Error cleanupFailure) { + warnTransactionCleanup("connection close after confirmed rollback", committed, outcomeUnknown); + } + } else { + try { + connection.abort(Runnable::run); + // Some drivers incorrectly implement abort() as a no-op. + if (!connection.isClosed()) { + warnTransactionCleanup("connection abort completion", committed, outcomeUnknown); + } + } catch (SQLException | RuntimeException | Error cleanupFailure) { + // Do not fall back to close(): correctness takes precedence + // over reclaiming a still-active transaction. + warnTransactionCleanup("connection abort", committed, outcomeUnknown); + } + } + } else { + try { + connection.close(); + } catch (SQLException | RuntimeException | Error cleanupFailure) { + warnTransactionCleanup("connection close", committed, outcomeUnknown); + } } } } diff --git a/teaql-sqlite/src/test/java/io/teaql/sqlite/JdbcRollbackCloseSqliteTest.java b/teaql-sqlite/src/test/java/io/teaql/sqlite/JdbcRollbackCloseSqliteTest.java new file mode 100644 index 00000000..f9cd6b9b --- /dev/null +++ b/teaql-sqlite/src/test/java/io/teaql/sqlite/JdbcRollbackCloseSqliteTest.java @@ -0,0 +1,117 @@ +package io.teaql.sqlite; + +import io.teaql.core.TransactionOutcomeUnknownException; +import io.teaql.provider.jdbc.JdbcSqlExecutor; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; +import java.nio.file.Files; +import java.sql.Connection; +import java.sql.SQLException; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; +import javax.sql.DataSource; +import org.junit.Test; +import org.sqlite.SQLiteDataSource; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +/** An unconfirmed rollback must not be followed by a close that can commit pending writes. */ +public class JdbcRollbackCloseSqliteTest { + private enum AbortBehavior { CLOSE, THROW, NO_OP } + + @Test public void failedRollbackDoesNotCommitPendingWriteOnClose() throws Exception { + verifyFailedRollback(AbortBehavior.CLOSE); + } + + @Test public void failedAbortDoesNotFallBackToUnsafeClose() throws Exception { + verifyFailedRollback(AbortBehavior.THROW); + } + + @Test public void noopAbortDoesNotFallBackToUnsafeClose() throws Exception { + verifyFailedRollback(AbortBehavior.NO_OP); + } + + private static void verifyFailedRollback(AbortBehavior abortBehavior) throws Exception { + var backing = new SQLiteDataSource(); + backing.setUrl("jdbc:sqlite:" + Files.createTempFile("teaql-rollback-close-", ".db")); + try (var connection = backing.getConnection(); var statement = connection.createStatement()) { + statement.execute("CREATE TABLE rollback_close_probe (id INTEGER PRIMARY KEY)"); + } + + var rollbackFailed = new AtomicBoolean(); + var committedOnClose = new AtomicBoolean(); + var aborted = new AtomicBoolean(); + var opened = new AtomicReference(); + DataSource injecting = (DataSource) Proxy.newProxyInstance( + DataSource.class.getClassLoader(), new Class[] {DataSource.class}, + (proxy, method, args) -> { + Object value = invoke(backing, method, args); + if (!method.getName().equals("getConnection")) return value; + Connection actual = (Connection) value; + opened.set(actual); + return Proxy.newProxyInstance(Connection.class.getClassLoader(), + new Class[] {Connection.class}, (connectionProxy, operation, parameters) -> { + if (operation.getName().equals("rollback")) { + rollbackFailed.set(true); + throw new SQLException("injected rollback failure"); + } + if (operation.getName().equals("abort")) { + aborted.set(true); + if (abortBehavior == AbortBehavior.THROW) { + throw new SQLException("injected abort failure"); + } + if (abortBehavior == AbortBehavior.NO_OP) return null; + actual.rollback(); + actual.close(); + return null; + } + if (operation.getName().equals("close") && rollbackFailed.get()) { + // JDBC leaves active-transaction close behavior implementation-defined. + // Model a driver/pool that commits when close returns the connection. + actual.commit(); + committedOnClose.set(true); + } + return invoke(actual, operation, parameters); + }); + }); + var executor = new JdbcSqlExecutor(injecting); + + try { + assertThrows(TransactionOutcomeUnknownException.class, () -> + executor.executeInTransaction(() -> { + executor.update("INSERT INTO rollback_close_probe (id) VALUES (?)", new Object[] {1L}); + throw new IllegalStateException("abort this transaction"); + })); + + assertTrue("the rollback failure must be injected", rollbackFailed.get()); + assertFalse("an unsafe close must not commit pending writes", committedOnClose.get()); + assertTrue("unconfirmed transactions must invalidate the connection", aborted.get()); + } finally { + // A failed/no-op abort leaves the backing connection open; release it + // explicitly so the test does not strand SQLite's write lock. + Connection actual = opened.get(); + if (actual != null && !actual.isClosed()) { + actual.rollback(); + actual.close(); + } + } + try (var connection = backing.getConnection(); + var statement = connection.prepareStatement("SELECT COUNT(*) FROM rollback_close_probe"); + var rows = statement.executeQuery()) { + assertTrue(rows.next()); + assertEquals(0, rows.getInt(1)); + } + } + + private static Object invoke(Object target, Method method, Object[] args) throws Throwable { + try { + return method.invoke(target, args); + } catch (InvocationTargetException failure) { + throw failure.getCause(); + } + } +}