From 9b0d2c1f9bb2df779cc5db1d1f42d85c404a2e8e Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Tue, 25 Aug 2026 17:23:48 -0700 Subject: [PATCH 1/3] Use DatabaseCache for Attachments so they work with additions and deletions inside transactions --- core/src/org/labkey/core/attachment/AttachmentCache.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/core/src/org/labkey/core/attachment/AttachmentCache.java b/core/src/org/labkey/core/attachment/AttachmentCache.java index 559c7ea12d2..9884ab33851 100644 --- a/core/src/org/labkey/core/attachment/AttachmentCache.java +++ b/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 ATTACHMENT_COLUMNS = new CsvSet("Parent, Container, DocumentName, DocumentSize, DocumentType, Created, CreatedBy, LastIndexed"); - private static final Cache> CACHE = CacheManager.getStringKeyCache(200000, CacheManager.MONTH, "Attachments"); private static final CacheLoader> 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> CACHE = DatabaseCache.get(CoreSchema.getInstance().getScope(), 200000, CacheManager.MONTH, "Attachments", LOADER); + static @NotNull Map getAttachments(AttachmentParent parent) { From 9543c702dc56c9a9a7f5ee0cae78e5b03da1b799 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Tue, 25 Aug 2026 19:38:38 -0700 Subject: [PATCH 2/3] Add unit test and a non-cache getAttachmentsForDelete --- .../attachment/AttachmentServiceImpl.java | 143 +++++++++++++++++- 1 file changed, 142 insertions(+), 1 deletion(-) diff --git a/core/src/org/labkey/core/attachment/AttachmentServiceImpl.java b/core/src/org/labkey/core/attachment/AttachmentServiceImpl.java index 59f3aa8182e..3676d4a890e 100644 --- a/core/src/org/labkey/core/attachment/AttachmentServiceImpl.java +++ b/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 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 parents) { for (AttachmentParent parent : parents) { - List atts = getAttachments(parent); + List 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 getNames(Collection 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 getNamesOnNewThread(AttachmentParent parent) throws InterruptedException + { + AtomicReference> names = new AtomicReference<>(); + AtomicReference 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() From f3786cec3f607199ed7eb7c1492689f62072b011 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Tue, 25 Aug 2026 19:39:35 -0700 Subject: [PATCH 3/3] Make updates to commitAndKeepConnection so it doesn't end up with the same stale cache problem --- api/src/org/labkey/api/data/DbScope.java | 88 +++++++++++++++++++++++- 1 file changed, 86 insertions(+), 2 deletions(-) diff --git a/api/src/org/labkey/api/data/DbScope.java b/api/src/org/labkey/api/data/DbScope.java index 834b2b6192c..e267527be09 100644 --- a/api/src/org/labkey/api/data/DbScope.java +++ b/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 cache = new DatabaseCache.TestCase.TempDatabaseCache<>(scope, 10, "commitAndKeepConnection test"); + + try + { + cache.put("key_1", "value_1"); + cache.put("key_2", "value_2"); + + List 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() {