fix(data): fire transaction synchronizations, and scope them to their transaction

Two defects in the same mechanism, both silent. 1. HibernateTxManager#commit fired synchronizations *after* its finally block, where cleanupIfIdle() had already called ResourceRegistry.cleanup() and removed the ThreadLocal list holding them. fireSynchronizations() then read a freshly initialized empty list and did nothing. No afterCommit callback had ever run on the Hibernate path: no exception, no log, just silence. rollback() twenty lines below had the order right, which is what makes this an ordering slip rather than a design choice. Found in production, from the far end: an admin write landed in Postgres while the in-memory cache it was registered to refresh never heard about it, so the change only took effect when the process restarted and re-read the database at boot. 2. Synchronizations were a single flat per-thread list, fired from index 0 by whichever transaction completed first. A REQUIRES_NEW inner transaction therefore fired the *suspended* outer transaction's callbacks too — early, with the inner transaction's outcome, for a transaction that might still roll back. Each new transaction now records how many synchronizations were already registered when it began, and fires only its own tail. Both managers get the fix and the same callback ordering: unbind the session or connection first, so a callback that opens its own transaction (a cache reload, an outbox drain) gets a fresh one instead of joining the transaction that just committed, then fire, then clean up. Also fixes JdbcTxStatus rejecting a null connection, which turned the two propagations that deliberately produce a connectionless status — SUPPORTS with no active transaction, and NOT_SUPPORTED — into an NPE inside begin(). The Hibernate manager always allowed it, and resource() already reports the real mistake with a message that names it. Tests: 16 new across the two managers, kept deliberately parallel since the two are interchangeable behind TxManager — synchronization firing, ordering, per-transaction scoping, callbacks opening their own transaction, and the previously untested SUPPORTS/NOT_SUPPORTED/MANDATORY propagations. Nothing covered afterCommit before, which is how both defects shipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-08-12 23:23:47 +00:00
parent 88ac3c3d1f
commit 7299490d0d
10 changed files with 546 additions and 67 deletions
@@ -41,6 +41,9 @@ public class JdbcTxManager implements TxManager {

    private TxStatus beginNew(TxDefinition definition) {
        Connection conn = null;
+        // Anything already registered belongs to an enclosing transaction this one is nested
+        // inside (or suspended over) — see ResourceRegistry#fireSynchronizations.
+        int synchronizationBaseline = ResourceRegistry.synchronizationCount();
        JdbcTxStatus suspended = suspendIfNeeded();
        boolean bound = false;
        try {
@@ -55,7 +58,8 @@ public class JdbcTxManager implements TxManager {
                    true,
                    definition.readOnly(),
                    suspended,
-                    new JdbcTxStatus.RollbackMarker()
+                    new JdbcTxStatus.RollbackMarker(),
+                    synchronizationBaseline
            );
            ResourceRegistry.bind(JDBC_STATUS_KEY, status);
            bound = true;
@@ -76,12 +80,15 @@ public class JdbcTxManager implements TxManager {
        if (definition.readOnly() && !existing.isReadOnly()) {
            throw new TxException("Cannot join read-write tx as read-only");
        }
+        // Baseline 0 is never read: a joined status isn't a new transaction, so commit()/rollback()
+        // hand it straight back to the transaction it joined without firing anything.
        return new JdbcTxStatus(
                existing.connection(),
                false,
                definition.readOnly(),
                null,
-                existing.rollbackMarker()
+                existing.rollbackMarker(),
+                0
        );
    }

@@ -90,7 +97,7 @@ public class JdbcTxManager implements TxManager {
    }

    private TxStatus noOp(TxDefinition definition, JdbcTxStatus suspended) {
-        return new JdbcTxStatus(null, false, definition.readOnly(), suspended, new JdbcTxStatus.RollbackMarker());
+        return new JdbcTxStatus(null, false, definition.readOnly(), suspended, new JdbcTxStatus.RollbackMarker(), 0);
    }

    private JdbcTxStatus suspendIfNeeded() {
@@ -110,18 +117,23 @@ public class JdbcTxManager implements TxManager {
            cleanupIfIdle();
            return;
        }
+        TxOutcome outcome = null;
        try {
            if (s.isRollbackOnly()) {
                s.connection().rollback();
-                ResourceRegistry.fireSynchronizations(TxOutcome.ROLLED_BACK);
-                return;
+                outcome = TxOutcome.ROLLED_BACK;
+            } else {
+                s.connection().commit();
+                outcome = TxOutcome.COMMITTED;
            }
-            s.connection().commit();
-            ResourceRegistry.fireSynchronizations(TxOutcome.COMMITTED);
        } catch (SQLException e) {
            throw new TxException(e);
        } finally {
+            // Unbind the connection before firing, so a callback that opens its own transaction
+            // gets a fresh one instead of joining the connection that just committed — and fire
+            // before cleanupIfIdle(), whose ResourceRegistry.cleanup() drops the pending list.
            cleanupAndResume(s);
+            if (outcome != null) ResourceRegistry.fireSynchronizations(outcome, s.synchronizationBaseline());
            cleanupIfIdle();
        }
    }
@@ -137,11 +149,12 @@ public class JdbcTxManager implements TxManager {
        }
        try {
            s.connection().rollback();
-            ResourceRegistry.fireSynchronizations(TxOutcome.ROLLED_BACK);
        } catch (SQLException e) {
            throw new TxException(e);
        } finally {
+            // Same order as commit() above, for the same two reasons.
            cleanupAndResume(s);
+            ResourceRegistry.fireSynchronizations(TxOutcome.ROLLED_BACK, s.synchronizationBaseline());
            cleanupIfIdle();
        }
    }
@@ -3,7 +3,6 @@ package dev.relism.flash.ext.data.jdbc;
 import dev.relism.flash.ext.data.core.TxStatus;

 import java.sql.Connection;
-import java.util.Objects;

 class JdbcTxStatus implements TxStatus {
    static final class RollbackMarker {
@@ -15,19 +14,27 @@ class JdbcTxStatus implements TxStatus {
    private final boolean readOnly;
    private final JdbcTxStatus suspended;
    private final RollbackMarker rollbackMarker;
+    private final int synchronizationBaseline;

+    // No requireNonNull on the connection: a SUPPORTS-without-a-transaction or a NOT_SUPPORTED
+    // status is deliberately connectionless (see JdbcTxManager#noOp), and rejecting null here
+    // turned both of those propagations into an NPE at begin() — the Hibernate manager has
+    // always allowed it. resource() reports the real mistake, asking a connectionless status for
+    // its connection, where it can name it.
    JdbcTxStatus(
            Connection connection,
            boolean newTransaction,
            boolean readOnly,
            JdbcTxStatus suspended,
-            RollbackMarker rollbackMarker
+            RollbackMarker rollbackMarker,
+            int synchronizationBaseline
    ) {
-        this.connection = Objects.requireNonNull(connection);
+        this.connection = connection;
        this.newTransaction = newTransaction;
        this.readOnly = readOnly;
        this.suspended = suspended;
        this.rollbackMarker = rollbackMarker;
+        this.synchronizationBaseline = synchronizationBaseline;
    }

    @Override public boolean isNewTransaction() { return newTransaction; }
@@ -46,4 +53,7 @@ class JdbcTxStatus implements TxStatus {
    Connection connection() { return connection; }
    JdbcTxStatus suspended() { return suspended; }
    RollbackMarker rollbackMarker() { return rollbackMarker; }
+
+    /** Index into {@code ResourceRegistry}'s synchronization list where this transaction's own callbacks start. */
+    int synchronizationBaseline() { return synchronizationBaseline; }
 }
@@ -0,0 +1,158 @@
+package dev.relism.flash.ext.data.jdbc;
+
+import dev.relism.flash.ext.data.core.*;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+import java.sql.Connection;
+import java.util.ArrayList;
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+/**
+ * Transaction synchronization semantics for the JDBC manager — the same contract {@code
+ * HibernateTxManagerSynchronizationTest} pins down for the Hibernate one, kept deliberately
+ * parallel: the two managers are interchangeable behind {@code TxManager}, so a callback must not
+ * observe a different lifecycle depending on which one is installed.
+ */
+class JdbcTxManagerSynchronizationTest {
+
+    private final JdbcTxManager manager = new JdbcTxManager(new TestDataSource());
+
+    @AfterEach
+    void cleanup() {
+        ResourceRegistry.clear();
+    }
+
+    /** Records which callbacks ran, in order — order is part of the contract, not just the fact they ran. */
+    private static final class Recorder implements TxSynchronization {
+        final List<String> calls = new ArrayList<>();
+
+        @Override public void afterCommit() { calls.add("afterCommit"); }
+        @Override public void afterRollback() { calls.add("afterRollback"); }
+        @Override public void afterCompletion(TxOutcome outcome) { calls.add("afterCompletion:" + outcome); }
+    }
+
+    @Test
+    void afterCommit_fires_on_commit() {
+        Recorder recorder = new Recorder();
+        TxStatus tx = manager.begin(TxDefinition.DEFAULTS);
+        ResourceRegistry.addSynchronization(recorder);
+
+        manager.commit(tx);
+
+        assertEquals(List.of("afterCommit", "afterCompletion:COMMITTED"), recorder.calls);
+    }
+
+    @Test
+    void afterRollback_fires_on_rollback() {
+        Recorder recorder = new Recorder();
+        TxStatus tx = manager.begin(TxDefinition.DEFAULTS);
+        ResourceRegistry.addSynchronization(recorder);
+
+        manager.rollback(tx);
+
+        assertEquals(List.of("afterRollback", "afterCompletion:ROLLED_BACK"), recorder.calls);
+    }
+
+    @Test
+    void commit_of_a_rollback_only_tx_fires_the_rollback_callbacks() {
+        Recorder recorder = new Recorder();
+        TxStatus tx = manager.begin(TxDefinition.DEFAULTS);
+        tx.markRollbackOnly();
+        ResourceRegistry.addSynchronization(recorder);
+
+        manager.commit(tx);
+
+        assertEquals(List.of("afterRollback", "afterCompletion:ROLLED_BACK"), recorder.calls);
+    }
+
+    /** Callbacks run after the committed connection is unbound, so opening a transaction gets a fresh one. */
+    @Test
+    void a_synchronization_may_open_its_own_transaction() {
+        List<Connection> connectionsSeen = new ArrayList<>();
+        List<Boolean> wasNewTransaction = new ArrayList<>();
+
+        TxStatus outer = manager.begin(TxDefinition.DEFAULTS);
+        Connection committedConnection = outer.resource(Connection.class);
+        ResourceRegistry.addSynchronization(new TxSynchronization() {
+            @Override
+            public void afterCommit() {
+                TxStatus own = manager.begin(TxDefinition.DEFAULTS);
+                connectionsSeen.add(own.resource(Connection.class));
+                wasNewTransaction.add(own.isNewTransaction());
+                manager.commit(own);
+            }
+        });
+
+        manager.commit(outer);
+
+        assertEquals(1, connectionsSeen.size(), "the callback must have run");
+        assertEquals(List.of(true), wasNewTransaction, "must start its own transaction, not join the committed one");
+        assertNotSame(committedConnection, connectionsSeen.get(0));
+    }
+
+    @Test
+    void a_joined_commit_defers_synchronizations_to_the_outermost_commit() {
+        Recorder recorder = new Recorder();
+        TxStatus outer = manager.begin(TxDefinition.DEFAULTS);
+        TxStatus inner = manager.begin(TxDefinition.DEFAULTS.withPropagation(TransactionPropagation.REQUIRED));
+        ResourceRegistry.addSynchronization(recorder);
+
+        manager.commit(inner);
+        assertEquals(List.of(), recorder.calls, "the joined commit did not commit anything yet");
+
+        manager.commit(outer);
+        assertEquals(List.of("afterCommit", "afterCompletion:COMMITTED"), recorder.calls);
+    }
+
+    @Test
+    void synchronizations_do_not_leak_into_the_next_transaction() {
+        Recorder recorder = new Recorder();
+        TxStatus first = manager.begin(TxDefinition.DEFAULTS);
+        ResourceRegistry.addSynchronization(recorder);
+        manager.commit(first);
+        recorder.calls.clear();
+
+        TxStatus second = manager.begin(TxDefinition.DEFAULTS);
+        manager.commit(second);
+
+        assertEquals(List.of(), recorder.calls);
+    }
+
+    @Test
+    void a_requires_new_commit_leaves_the_suspended_transactions_synchronizations_alone() {
+        Recorder outerSync = new Recorder();
+        Recorder innerSync = new Recorder();
+
+        TxStatus outer = manager.begin(TxDefinition.DEFAULTS);
+        ResourceRegistry.addSynchronization(outerSync);
+
+        TxStatus inner = manager.begin(TxDefinition.DEFAULTS.withPropagation(TransactionPropagation.REQUIRES_NEW));
+        ResourceRegistry.addSynchronization(innerSync);
+        manager.commit(inner);
+
+        assertEquals(List.of("afterCommit", "afterCompletion:COMMITTED"), innerSync.calls);
+        assertEquals(List.of(), outerSync.calls, "the outer transaction has not committed yet");
+
+        manager.commit(outer);
+        assertEquals(List.of("afterCommit", "afterCompletion:COMMITTED"), outerSync.calls);
+    }
+
+    @Test
+    void a_requires_new_rollback_leaves_the_suspended_transactions_synchronizations_alone() {
+        Recorder outerSync = new Recorder();
+
+        TxStatus outer = manager.begin(TxDefinition.DEFAULTS);
+        ResourceRegistry.addSynchronization(outerSync);
+
+        TxStatus inner = manager.begin(TxDefinition.DEFAULTS.withPropagation(TransactionPropagation.REQUIRES_NEW));
+        manager.rollback(inner);
+
+        assertEquals(List.of(), outerSync.calls, "the outer transaction is still open");
+
+        manager.commit(outer);
+        assertEquals(List.of("afterCommit", "afterCompletion:COMMITTED"), outerSync.calls);
+    }
+}
@@ -4,15 +4,12 @@ import dev.relism.flash.ext.data.core.*;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Test;

-import javax.sql.DataSource;
 import java.sql.Connection;
-import java.sql.DriverManager;
-import java.sql.SQLException;

 import static org.junit.jupiter.api.Assertions.*;

 class JdbcTxManagerTest {
-    private final JdbcTxManager manager = new JdbcTxManager(dataSource());
+    private final JdbcTxManager manager = new JdbcTxManager(new TestDataSource());

    @AfterEach
    void cleanup() {
@@ -53,52 +50,38 @@ class JdbcTxManagerTest {
        manager.rollback(outer);
    }

-    private static DataSource dataSource() {
-        return new DataSource() {
-            @Override
-            public Connection getConnection() throws SQLException {
-                return DriverManager.getConnection("jdbc:h2:mem:tx-jdbc;DB_CLOSE_DELAY=-1");
-            }
+    /**
+     * SUPPORTS without an active transaction yields a connectionless status — it must not be a
+     * transaction, and asking it for a connection must say so rather than NPE. Both propagations
+     * that produce one used to throw {@link NullPointerException} straight out of {@code begin()}.
+     */
+    @Test
+    void supports_without_active_transaction_is_a_connectionless_no_op() {
+        TxStatus s = manager.begin(TxDefinition.DEFAULTS.withPropagation(TransactionPropagation.SUPPORTS));

-            @Override
-            public Connection getConnection(String username, String password) throws SQLException {
-                return DriverManager.getConnection("jdbc:h2:mem:tx-jdbc;DB_CLOSE_DELAY=-1", username, password);
-            }
+        assertFalse(s.isNewTransaction());
+        assertThrows(IllegalStateException.class, () -> s.resource(Connection.class));
+        assertDoesNotThrow(() -> manager.commit(s));
+    }

-            @Override
-            public <T> T unwrap(Class<T> iface) {
-                throw new UnsupportedOperationException();
-            }
+    @Test
+    void not_supported_suspends_the_active_transaction_and_restores_it_on_commit() {
+        TxStatus outer = manager.begin(TxDefinition.DEFAULTS);
+        Connection outerConnection = outer.resource(Connection.class);

-            @Override
-            public boolean isWrapperFor(Class<?> iface) {
-                return false;
-            }
+        TxStatus suspended = manager.begin(TxDefinition.DEFAULTS.withPropagation(TransactionPropagation.NOT_SUPPORTED));
+        assertFalse(suspended.isNewTransaction());
+        assertThrows(IllegalStateException.class, () -> suspended.resource(Connection.class));
+        manager.commit(suspended);

-            @Override
-            public java.io.PrintWriter getLogWriter() {
-                throw new UnsupportedOperationException();
-            }
+        TxStatus rejoined = manager.begin(TxDefinition.DEFAULTS.withPropagation(TransactionPropagation.REQUIRED));
+        assertSame(outerConnection, rejoined.resource(Connection.class), "the suspended transaction must be back");
+        manager.rollback(outer);
+    }

-            @Override
-            public void setLogWriter(java.io.PrintWriter out) {
-                throw new UnsupportedOperationException();
-            }
-
-            @Override
-            public void setLoginTimeout(int seconds) {
-                throw new UnsupportedOperationException();
-            }
-
-            @Override
-            public int getLoginTimeout() {
-                return 0;
-            }
-
-            @Override
-            public java.util.logging.Logger getParentLogger() {
-                throw new UnsupportedOperationException();
-            }
-        };
+    @Test
+    void mandatory_without_active_transaction_is_rejected() {
+        assertThrows(IllegalStateException.class,
+                () -> manager.begin(TxDefinition.DEFAULTS.withPropagation(TransactionPropagation.MANDATORY)));
    }
 }
@@ -0,0 +1,37 @@
+package dev.relism.flash.ext.data.jdbc;
+
+import javax.sql.DataSource;
+import java.io.PrintWriter;
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.SQLException;
+import java.util.logging.Logger;
+
+/**
+ * A bare in-memory H2 {@link DataSource} — one fresh connection per {@code getConnection()}, which
+ * is all {@code JdbcTxManager} needs to exercise real commit/rollback and suspension. Every method
+ * outside the two {@code getConnection} overloads throws: nothing under test calls them, and a
+ * loud failure beats a silent stub if that ever changes.
+ */
+final class TestDataSource implements DataSource {
+
+    static final String URL = "jdbc:h2:mem:tx-jdbc;DB_CLOSE_DELAY=-1";
+
+    @Override
+    public Connection getConnection() throws SQLException {
+        return DriverManager.getConnection(URL);
+    }
+
+    @Override
+    public Connection getConnection(String username, String password) throws SQLException {
+        return DriverManager.getConnection(URL, username, password);
+    }
+
+    @Override public <T> T unwrap(Class<T> iface) { throw new UnsupportedOperationException(); }
+    @Override public boolean isWrapperFor(Class<?> iface) { return false; }
+    @Override public PrintWriter getLogWriter() { throw new UnsupportedOperationException(); }
+    @Override public void setLogWriter(PrintWriter out) { throw new UnsupportedOperationException(); }
+    @Override public void setLoginTimeout(int seconds) { throw new UnsupportedOperationException(); }
+    @Override public int getLoginTimeout() { return 0; }
+    @Override public Logger getParentLogger() { throw new UnsupportedOperationException(); }
+}