Use DatabaseCache for Attachments so they work with additions and deletions inside transactions

api/src/org/labkey/api/data/DbScope.java

@@ -2573,6 +2573,16 @@ private void popCurrentTransaction()
         }
     }
 
+    // Counterpart to popCurrentTransaction(), for callers that need to detach a transaction from its thread
+    // temporarily. See TransactionImpl.commitAndKeepConnection().
+    private void pushCurrentTransaction(TransactionImpl transaction)
+    {
+        synchronized (_transaction)
+        {
+            _transaction.computeIfAbsent(getEffectiveThread(), _ -> new ArrayList<>()).add(transaction);
+        }
+    }
+
     public static class ConnectionSharingCloseable implements AutoCloseable
     {
         private final Thread _asyncThread;
@@ -2804,8 +2814,29 @@ public void commitAndKeepConnection()
             {
                 CommitTaskOption.PRECOMMIT.run(this);
                 getConnection().commit();
-                _caches.clear();
-                CommitTaskOption.POSTCOMMIT.run(this);
+                closeCaches();
+
+                // Detach this transaction from the thread while the POSTCOMMIT tasks run, matching commit(), which
+                // pops before running them. Commit tasks that invalidate a DatabaseCache resolve their target through
+                // getCurrentTransactionImpl(): with this transaction still on the thread they build a fresh
+                // TransactionCache and clear that throwaway private cache, so the shared cache goes on serving
+                // pre-commit values until they expire. Skip the swap if we somehow aren't the innermost transaction,
+                // since popping would then corrupt the thread's transaction stack.
+                boolean detached = this == getCurrentTransactionImpl();
+
+                if (detached)
+                    popCurrentTransaction();
+
+                try
+                {
+                    CommitTaskOption.POSTCOMMIT.run(this);
+                }
+                finally
+                {
+                    if (detached)
+                        pushCurrentTransaction(this);
+                }
+
                 clearCommitTasks();
             }
             catch (SQLException e)
@@ -3239,6 +3270,59 @@ public void tesCommitTaskFailure()
             closeAllConnectionsForCurrentThread();
         }
 
+        @Test
+        public void testCommitAndKeepConnection()
+        {
+            DbScope scope = getLabKeyScope();
+            // TempDatabaseCache's shared cache is temporary, so it stays out of KNOWN_CACHES; close() it below
+            DatabaseCache<String, String> cache = new DatabaseCache.TestCase.TempDatabaseCache<>(scope, 10, "commitAndKeepConnection test");
+
+            try
+            {
+                cache.put("key_1", "value_1");
+                cache.put("key_2", "value_2");
+
+                List<Transaction> transactionsSeenByPostCommitTask = new ArrayList<>();
+
+                try (Transaction t = scope.ensureTransaction())
+                {
+                    t.addCommitTask(() -> transactionsSeenByPostCommitTask.add(scope.getCurrentTransaction()), CommitTaskOption.POSTCOMMIT);
+                    cache.remove("key_1");
+
+                    // DatabaseCache defers removals to the commit, so the shared cache still serves the old value
+                    assertTrue("Shared cache should still hold key_1 before the commit", cache.getKeys().contains("key_1"));
+
+                    t.commitAndKeepConnection();
+
+                    // The deferred removal must land on the shared cache. If this transaction is still on the thread
+                    // while the POSTCOMMIT tasks run, the removal builds a fresh TransactionCache and clears that
+                    // throwaway private cache instead, leaving key_1 in the shared cache until it expires.
+                    assertFalse("commitAndKeepConnection() must invalidate the shared cache", cache.getKeys().contains("key_1"));
+                    assertTrue("commitAndKeepConnection() should leave unrelated keys alone", cache.getKeys().contains("key_2"));
+
+                    // POSTCOMMIT tasks must run detached from the transaction, exactly as they do under commit()
+                    assertEquals("POSTCOMMIT task should have run exactly once", 1, transactionsSeenByPostCommitTask.size());
+                    assertNull("POSTCOMMIT tasks must not see an active transaction", transactionsSeenByPostCommitTask.get(0));
+
+                    // ...and the transaction must be back on the thread, still active and still usable
+                    assertTrue(scope.isTransactionActive());
+                    assertSame("commitAndKeepConnection() must leave the transaction on the thread", t, scope.getCurrentTransaction());
+
+                    cache.remove("key_2");
+                    assertTrue("Removal after commitAndKeepConnection() should be deferred again", cache.getKeys().contains("key_2"));
+
+                    t.commit();
+                }
+
+                assertFalse("commit() must invalidate the shared cache", cache.getKeys().contains("key_2"));
+                assertFalse(scope.isTransactionActive());
+            }
+            finally
+            {
+                cache.close();
+            }
+        }
+
         @Test
         public void testLockReleasedException()
         {

core/src/org/labkey/core/attachment/AttachmentCache.java

@@ -25,6 +25,7 @@
 import org.labkey.api.collections.CsvSet;
 import org.labkey.api.data.Container;
 import org.labkey.api.data.CoreSchema;
+import org.labkey.api.data.DatabaseCache;
 import org.labkey.api.data.SimpleFilter;
 import org.labkey.api.data.Sort;
 import org.labkey.api.data.TableSelector;
@@ -44,7 +45,6 @@
 public class AttachmentCache
 {
     private static final Set<String> ATTACHMENT_COLUMNS = new CsvSet("Parent, Container, DocumentName, DocumentSize, DocumentType, Created, CreatedBy, LastIndexed");
-    private static final Cache<String, Map<String, Attachment>> CACHE = CacheManager.getStringKeyCache(200000, CacheManager.MONTH, "Attachments");
 
     private static final CacheLoader<String, Map<String, Attachment>> LOADER = (key, attachmentParent) ->
     {
@@ -63,6 +63,9 @@ public class AttachmentCache
         return Collections.unmodifiableMap(map);
     };
 
+    // Must be transaction aware: attachments are very often added and deleted inside a transaction
+    private static final Cache<String, Map<String, Attachment>> CACHE = DatabaseCache.get(CoreSchema.getInstance().getScope(), 200000, CacheManager.MONTH, "Attachments", LOADER);
+
 
     static @NotNull Map<String, Attachment> getAttachments(AttachmentParent parent)
     {

core/src/org/labkey/core/attachment/AttachmentServiceImpl.java

@@ -33,6 +33,7 @@
 import org.labkey.api.attachments.AttachmentParent;
 import org.labkey.api.attachments.AttachmentParentType;
 import org.labkey.api.attachments.AttachmentService;
+import org.labkey.api.attachments.ByteArrayAttachmentFile;
 import org.labkey.api.attachments.DocumentWriter;
 import org.labkey.api.attachments.FileAttachmentFile;
 import org.labkey.api.attachments.SpringAttachmentFile;
@@ -91,6 +92,7 @@
 import org.labkey.api.util.GUID;
 import org.labkey.api.util.HtmlString;
 import org.labkey.api.util.HtmlStringBuilder;
+import org.labkey.api.util.JunitUtil;
 import org.labkey.api.util.MimeMap;
 import org.labkey.api.util.PageFlowUtil;
 import org.labkey.api.util.Pair;
@@ -146,6 +148,7 @@
 import java.util.Objects;
 import java.util.Set;
 import java.util.TreeSet;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.stream.Collectors;
 
 public class AttachmentServiceImpl implements AttachmentService
@@ -382,12 +385,33 @@ public void deleteAttachments(AttachmentParent parent)
         deleteAttachments(Collections.singleton(parent));
     }
 
+    /**
+     * Returns a parent's attachments without reading through the AttachmentCache. Bulk deletes call this once per
+     * parent inside a single transaction -- ExperimentServiceImpl.truncateDataClass builds one AttachmentParent per
+     * data class row, and IssueManager one per comment -- so reading through the cache there fills the transaction's
+     * private cache and queues a post-commit reload task for every parent, work that the removal below immediately
+     * discards. AttachmentDirectory parents still take the cached path, which reconciles the database rows against
+     * the file system.
+     */
+    private @NotNull List<Attachment> getAttachmentsForDelete(AttachmentParent parent)
+    {
+        if (parent instanceof AttachmentDirectory)
+            return getAttachments(parent);
+
+        checkSecurityPolicy(parent);
+
+        return new TableSelector(CoreSchema.getInstance().getTableInfoDocuments(),
+                ATTACHMENT_COLUMNS,
+                new SimpleFilter(FieldKey.fromParts("Parent"), parent.getEntityId()),
+                new Sort("+RowId")).getArrayList(Attachment.class);
+    }
+
     @Override
     public void deleteAttachments(Collection<AttachmentParent> parents)
     {
         for (AttachmentParent parent : parents)
         {
-            List<Attachment> atts = getAttachments(parent);
+            List<Attachment> atts = getAttachmentsForDelete(parent);
 
             // No attachments, or perhaps container doesn't match entityid
             if (atts.isEmpty())
@@ -1832,6 +1856,123 @@ private void testFileAttachmentFiles(File file1, File file2, User user) throws I
             assertEquals(originalCount, attachments.size());
         }
 
+        /**
+         * Attachments are routinely added and deleted inside a transaction while other threads -- most notably the
+         * SearchService indexing threads -- read the same parent's attachments. Those readers have no transaction, so
+         * they must see the pre-commit state until the delete commits, and must never be handed a cached snapshot that
+         * outlives the commit. This is the deterministic version of the race that made
+         * DeleteJobAttachmentsApiTest.testMultipleAttachments flaky.
+         */
+        @Test
+        public void testCacheConsistencyAcrossUncommittedDelete() throws Exception
+        {
+            User user = TestContext.get().getUser();
+            AttachmentService svc = AttachmentService.get();
+            AttachmentParent parent = new TestAttachmentParent(JunitUtil.getTestContainer());
+
+            try
+            {
+                svc.addAttachments(parent, List.of(testFile("one.txt"), testFile("two.txt"), testFile("three.txt")), user);
+
+                // Warm the shared cache; this is what threads without a transaction read below
+                assertEquals("Should start with three attachments", 3, svc.getAttachments(parent).size());
+
+                try (DbScope.Transaction tx = CoreSchema.getInstance().getScope().ensureTransaction())
+                {
+                    svc.deleteAttachment(parent, "one.txt", user);
+                    svc.deleteAttachment(parent, "two.txt", user);
+
+                    // The deleting thread sees its own uncommitted deletes
+                    assertEquals(List.of("three.txt"), getNames(svc.getAttachments(parent)));
+
+                    // A thread with no transaction must still see the pre-commit state. Two ways this can break: the
+                    // cache hands it the uncommitted list the line above just loaded, or the cache was evicted
+                    // eagerly and it reloads the pre-delete rows on its own connection and caches them past the
+                    // commit. A transaction-aware cache keeps the transaction's loads private and defers the eviction.
+                    assertEquals("Thread without a transaction must see the pre-commit state",
+                            List.of("one.txt", "two.txt", "three.txt"), getNamesOnNewThread(parent));
+
+                    tx.commit();
+                }
+
+                // The commit must have invalidated the shared cache in both directions
+                assertEquals(List.of("three.txt"), getNames(svc.getAttachments(parent)));
+                assertEquals("Thread without a transaction must see the committed state",
+                        List.of("three.txt"), getNamesOnNewThread(parent));
+            }
+            finally
+            {
+                svc.deleteAttachments(parent);
+            }
+        }
+
+        private static AttachmentFile testFile(String name)
+        {
+            return new ByteArrayAttachmentFile(name, name.getBytes(StringUtilsLabKey.DEFAULT_CHARSET), "text/plain");
+        }
+
+        private static List<String> getNames(Collection<Attachment> attachments)
+        {
+            return attachments.stream().map(Attachment::getName).toList();
+        }
+
+        // Reads the parent's attachments on a thread that has never had a transaction, so it always resolves to the
+        // shared cache
+        private static List<String> getNamesOnNewThread(AttachmentParent parent) throws InterruptedException
+        {
+            AtomicReference<List<String>> names = new AtomicReference<>();
+            AtomicReference<Throwable> failure = new AtomicReference<>();
+
+            Thread thread = new Thread(() -> {
+                try
+                {
+                    names.set(getNames(AttachmentService.get().getAttachments(parent)));
+                }
+                catch (Throwable t)
+                {
+                    failure.set(t);
+                }
+            }, "AttachmentServiceImpl.TestCase reader");
+
+            thread.start();
+            thread.join(30_000);
+
+            if (null != failure.get())
+                throw new RuntimeException("Reader thread failed", failure.get());
+            assertFalse("Reader thread did not finish; it is likely blocked on the open transaction", thread.isAlive());
+
+            return names.get();
+        }
+
+        private static class TestAttachmentParent implements AttachmentParent
+        {
+            private final String _containerId;
+            private final String _entityId = GUID.makeGUID();
+
+            private TestAttachmentParent(Container c)
+            {
+                _containerId = c.getId();
+            }
+
+            @Override
+            public String getEntityId()
+            {
+                return _entityId;
+            }
+
+            @Override
+            public String getContainerId()
+            {
+                return _containerId;
+            }
+
+            @Override
+            public @NotNull AttachmentParentType getAttachmentParentType()
+            {
+                return AttachmentParentType.UNKNOWN;
+            }
+        }
+
         // Tests the ability to extract EntityIds from data class LSIDs
         @Test
         public void testLsidGuidExtraction()