From 46f6c73b31968769a940a8ebebbe48a69e6e7112 Mon Sep 17 00:00:00 2001 From: Philip Z Date: Tue, 20 Oct 2026 01:27:05 +0800 Subject: [PATCH] fix: preserve committed outcome after JDBC cleanup failure --- .../teaql/provider/jdbc/JdbcSqlExecutor.java | 46 ++++++- .../JdbcCommittedCleanupSqliteTest.java | 120 ++++++++++++++++++ 2 files changed, 163 insertions(+), 3 deletions(-) create mode 100644 teaql-sqlite/src/test/java/io/teaql/sqlite/JdbcCommittedCleanupSqliteTest.java 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 23a476c4..59016536 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 @@ -14,9 +14,12 @@ import java.util.stream.Stream; import java.util.concurrent.CopyOnWriteArrayList; import java.util.function.Consumer; +import java.util.logging.Logger; public class JdbcSqlExecutor implements SqlExecutionAdapter { + private static final Logger LOG = Logger.getLogger(JdbcSqlExecutor.class.getName()); + private final DataSource dataSource; // A held view retains only this token, not the executor, connection or DataSource. private final Object storageIdentity = new Object(); @@ -427,7 +430,14 @@ public void executeInTransaction(Runnable action) { } return; } - try (Connection connection = openConnection()) { + Connection connection; + try { + connection = openConnection(); + } catch (SQLException failure) { + throw new RuntimeException("JDBC transaction failed", failure); + } + boolean committed = false; + try { boolean autoCommit = connection.getAutoCommit(); connection.setAutoCommit(false); transactionConnection.set(connection); @@ -439,16 +449,46 @@ public void executeInTransaction(Runnable action) { "JDBC transaction is rollback-only after a failed operation"); } connection.commit(); + committed = true; } catch (RuntimeException | Error failure) { - connection.rollback(); + try { + connection.rollback(); + } catch (SQLException | RuntimeException rollbackFailure) { + // Keep the original failure. The JDBC message may contain + // sensitive connection details and is not safe to expose. + failure.addSuppressed(new IllegalStateException("JDBC rollback failed")); + } throw failure; } finally { transactionConnection.remove(); transactionRollbackOnly.remove(); - connection.setAutoCommit(autoCommit); + try { + connection.setAutoCommit(autoCommit); + } catch (SQLException | RuntimeException cleanupFailure) { + // A successful commit is already durable. A cleanup error + // must not make the caller replay that business mutation. + warnTransactionCleanup("autoCommit restoration", committed); + } } } catch (SQLException e) { throw new RuntimeException("JDBC transaction failed", e); + } finally { + try { + connection.close(); + } catch (SQLException | RuntimeException cleanupFailure) { + warnTransactionCleanup("connection close", committed); + } + } + } + + private static void warnTransactionCleanup(String operation, boolean committed) { + try { + LOG.warning(committed + ? "JDBC transaction committed; " + operation + " failed" + : "JDBC transaction unsuccessful; " + operation + " failed"); + } catch (RuntimeException ignored) { + // A user-installed logging handler must not turn a committed write + // back into an apparently retryable transaction failure. } } diff --git a/teaql-sqlite/src/test/java/io/teaql/sqlite/JdbcCommittedCleanupSqliteTest.java b/teaql-sqlite/src/test/java/io/teaql/sqlite/JdbcCommittedCleanupSqliteTest.java new file mode 100644 index 00000000..14e88db0 --- /dev/null +++ b/teaql-sqlite/src/test/java/io/teaql/sqlite/JdbcCommittedCleanupSqliteTest.java @@ -0,0 +1,120 @@ +package io.teaql.sqlite; + +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.ArrayList; +import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.logging.Handler; +import java.util.logging.LogRecord; +import java.util.logging.Logger; +import javax.sql.DataSource; +import org.junit.Test; +import org.sqlite.SQLiteDataSource; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +/** A committed JDBC write must not look like a rolled-back, retryable save. */ +public class JdbcCommittedCleanupSqliteTest { + private static final String DIAGNOSTIC_CANARY = "POST-COMMIT-CLEANUP-SECRET-CANARY"; + private enum Fault { AUTO_COMMIT_RESTORE, CLOSE } + + @Test public void failedAutoCommitRestoreAfterCommitKeepsCommittedOutcome() throws Exception { + verifyCommittedCleanupFailure(Fault.AUTO_COMMIT_RESTORE); + } + + @Test public void failedCloseAfterCommitKeepsCommittedOutcome() throws Exception { + verifyCommittedCleanupFailure(Fault.CLOSE); + } + + @Test public void failedWarningHandlerAfterCommitKeepsCommittedOutcome() throws Exception { + verifyCommittedCleanupFailure(Fault.AUTO_COMMIT_RESTORE, true); + } + + private static void verifyCommittedCleanupFailure(Fault fault) throws Exception { + verifyCommittedCleanupFailure(fault, false); + } + + private static void verifyCommittedCleanupFailure(Fault fault, boolean warningHandlerFails) + throws Exception { + var backing = new SQLiteDataSource(); + backing.setUrl("jdbc:sqlite:" + Files.createTempFile("teaql-committed-cleanup-", ".db")); + try (var connection = backing.getConnection(); var statement = connection.createStatement()) { + statement.execute("CREATE TABLE committed_cleanup_probe (id INTEGER PRIMARY KEY)"); + } + var committed = new AtomicBoolean(); + 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; + return Proxy.newProxyInstance(Connection.class.getClassLoader(), + new Class[] {Connection.class}, (connectionProxy, operation, parameters) -> { + if (operation.getName().equals("setAutoCommit") + && Boolean.TRUE.equals(parameters[0]) && committed.get() + && fault == Fault.AUTO_COMMIT_RESTORE) { + throw new SQLException(DIAGNOSTIC_CANARY); + } + Object result = invoke(actual, operation, parameters); + if (operation.getName().equals("commit")) committed.set(true); + if (operation.getName().equals("close") && committed.get() + && fault == Fault.CLOSE) { + throw new SQLException(DIAGNOSTIC_CANARY); + } + return result; + }); + }); + var executor = new JdbcSqlExecutor(injecting); + List warnings = new ArrayList<>(); + Logger logger = Logger.getLogger(JdbcSqlExecutor.class.getName()); + Handler capture = new Handler() { + @Override public void publish(LogRecord record) { + warnings.add(record.getMessage()); + if (warningHandlerFails) throw new IllegalStateException(DIAGNOSTIC_CANARY); + } + @Override public void flush() { } + @Override public void close() { } + }; + logger.addHandler(capture); + + RuntimeException reported = null; + try { + executor.executeInTransaction(() -> executor.update( + "INSERT INTO committed_cleanup_probe (id) VALUES (?)", new Object[] {1L})); + } catch (RuntimeException failure) { + reported = failure; + } finally { + logger.removeHandler(capture); + } + + assertTrue("the injected failure must occur after the actual commit", committed.get()); + try (var connection = backing.getConnection(); + var statement = connection.prepareStatement("SELECT COUNT(*) FROM committed_cleanup_probe"); + var rows = statement.executeQuery()) { + assertTrue(rows.next()); + assertEquals(1, rows.getInt(1)); + } + assertTrue("cleanup failure needs an operational warning", warnings.stream() + .anyMatch(message -> message.contains("JDBC transaction committed"))); + assertTrue("the warning must not reveal the JDBC exception", warnings.stream() + .noneMatch(message -> message.contains(DIAGNOSTIC_CANARY))); + if (reported != null) { + throw new AssertionError("committed row was reported as a failed transaction", reported); + } + } + + private static Object invoke(Object target, Method method, Object[] args) throws Throwable { + try { + return method.invoke(target, args); + } catch (InvocationTargetException failure) { + throw failure.getCause(); + } + } +}