Fix flaky tests in IT pipeline

dspace-api/src/main/java/org/dspace/external/CachingOrcidRestConnector.java

@@ -174,7 +174,9 @@ protected String getAccessToken(String clientSecret, String clientId, String OAU
         }
     }
 
-    private InputStream httpGet(String path, String accessToken) throws IOException {
+    // Package/sub-class visible so tests can stub the HTTP layer (see CachingOrcidRestConnectorTest)
+    // and avoid hitting the live ORCID sandbox.
+    protected InputStream httpGet(String path, String accessToken) throws IOException {
         String trimmedPath = path.replaceFirst("^/+", "").replaceFirst("/+$", "");
 
         String fullPath = apiURL + '/' + trimmedPath;

dspace-api/src/test/java/org/dspace/AbstractIntegrationTestWithDatabase.java

@@ -9,7 +9,12 @@
 
 import static org.junit.Assert.fail;
 
+import java.io.File;
+import java.io.PrintWriter;
 import java.sql.SQLException;
+import java.util.ConcurrentModificationException;
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicInteger;
 
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
@@ -168,9 +173,47 @@ public void setUp() throws Exception {
     public void destroy() throws Exception {
         // Cleanup our global context object
         try {
-            AbstractBuilder.cleanupObjects();
-            parentCommunity = null;
-            cleanupContext();
+            // Builders/cleanupContext commit transactions through Hibernate. We have observed a rare,
+            // CI-only intermittent ConcurrentModificationException thrown from
+            // ResourceRegistryStandardImpl#releaseResources (a HashMap.forEach over the JDBC resource
+            // registry) while committing here. That registry is per-Hibernate-Session and explicitly NOT
+            // thread-safe, so the CME means a second thread is touching the same Session concurrently
+            // during cleanup. The exact offending thread has not yet been identified (it does not
+            // reproduce locally), so on CME we (a) capture a full thread dump to pinpoint the colliding
+            // thread the next time it happens in CI, and (b) retry the cleanup so an already-passed test
+            // is not failed by this teardown race. See dumpAllThreadsOnCme().
+            final int maxCleanupAttempts = 3;
+            boolean cleanupComplete = false;
+            for (int cleanupAttempt = 1; cleanupAttempt <= maxCleanupAttempts; cleanupAttempt++) {
+                try {
+                    AbstractBuilder.cleanupObjects();
+                    parentCommunity = null;
+                    cleanupContext();
+                    cleanupComplete = true;
+                    break;
+                } catch (ConcurrentModificationException cme) {
+                    // Capture a full thread dump the instant the CME is caught, so we can see which OTHER
+                    // thread is concurrently inside JDBC/Hibernate code on the same (non-thread-safe) session.
+                    dumpAllThreadsOnCme(cme, cleanupAttempt);
+                    log.warn("Transient Hibernate CME during @After cleanup (concurrent access to the "
+                            + "per-session JDBC resource registry), attempt {}/{}; aborting context, capturing a "
+                            + "thread dump and retrying cleanup.", cleanupAttempt, maxCleanupAttempts, cme);
+                    if (context != null && context.isValid()) {
+                        context.abort();
+                    }
+                    context = null;
+                    parentCommunity = null;
+
+                    if (cleanupAttempt < maxCleanupAttempts) {
+                        context = new Context(Context.Mode.READ_WRITE);
+                        context.turnOffAuthorisationSystem();
+                    }
+                }
+            }
+
+            if (!cleanupComplete) {
+                throw new IllegalStateException("Unable to complete @After DB cleanup after retries.");
+            }
 
             ServiceManager serviceManager = DSpaceServicesFactory.getInstance().getServiceManager();
             // Clear the search core.
@@ -202,6 +245,47 @@ public void destroy() throws Exception {
         }
     }
 
+    // Counter to give each captured dump a unique file name.
+    private static final AtomicInteger CME_DUMP_COUNTER = new AtomicInteger(0);
+
+    /**
+     * Diagnostic helper: when a ConcurrentModificationException is caught during @After cleanup, dump the
+     * stack traces of ALL live threads to a file under target/cme-dumps/ (archived as a CI artifact). The
+     * CME is thrown on the test thread while another thread concurrently mutates the same Hibernate
+     * session's (non-thread-safe) JDBC ResourceRegistry; this dump is meant to reveal that other thread so
+     * the underlying concurrency bug can be fixed at its source.
+     */
+    private static void dumpAllThreadsOnCme(ConcurrentModificationException cme, int attempt) {
+        try {
+            File dir = new File("target/cme-dumps");
+            dir.mkdirs();
+            int idx = CME_DUMP_COUNTER.incrementAndGet();
+            File out = new File(dir, "cme-" + System.currentTimeMillis() + "-" + idx + "-attempt" + attempt + ".txt");
+            try (PrintWriter pw = new PrintWriter(out, "UTF-8")) {
+                pw.println("===== ConcurrentModificationException caught during @After cleanup =====");
+                pw.println("Caught on thread: " + Thread.currentThread().getName());
+                pw.println("Attempt: " + attempt);
+                pw.println();
+                pw.println("----- CME stack -----");
+                cme.printStackTrace(pw);
+                pw.println();
+                pw.println("----- ALL THREAD STACKS at moment of CME -----");
+                for (Map.Entry<Thread, StackTraceElement[]> e : Thread.getAllStackTraces().entrySet()) {
+                    Thread t = e.getKey();
+                    pw.println();
+                    pw.println("\"" + t.getName() + "\" id=" + t.getId() + " state=" + t.getState()
+                            + " daemon=" + t.isDaemon());
+                    for (StackTraceElement ste : e.getValue()) {
+                        pw.println("\tat " + ste);
+                    }
+                }
+            }
+            log.error("CME thread dump written to {}", out.getAbsolutePath());
+        } catch (Exception dumpEx) {
+            log.error("Failed to write CME thread dump", dumpEx);
+        }
+    }
+
     /**
      * Utility method to cleanup a created Context object (to save memory).
      * This can also be used by individual tests to cleanup context objects they create.

dspace-api/src/test/java/org/dspace/external/CachingOrcidRestConnectorTest.java

@@ -13,9 +13,13 @@
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 
+import java.io.IOException;
+import java.io.InputStream;
+
 import org.dspace.AbstractDSpaceTest;
 import org.dspace.external.provider.orcid.xml.ExpandedSearchConverter;
 import org.dspace.utils.DSpace;
@@ -33,8 +37,22 @@ public class CachingOrcidRestConnectorTest extends AbstractDSpaceTest {
     private static final String orcid = "0000-0002-9150-2529";
     private static final String expectedLabel = "Connor, John";
 
+    // Canned ORCID "expanded-search" response (num-found=1725, first result -> "Connor, John").
+    // Used to mock the HTTP layer so the tests don't depend on the live ORCID sandbox.
+    private static final String EXPANDED_SEARCH_XML = "org/dspace/external/orcid-expanded-search.xml";
+
     private CachingOrcidRestConnector sut;
 
+    /**
+     * Load a canned API response from the test classpath as a fresh InputStream.
+     * (A new stream is returned on every call because the connector consumes/closes it.)
+     */
+    private InputStream cannedResponse(String resource) {
+        InputStream is = getClass().getClassLoader().getResourceAsStream(resource);
+        assertNotNull("Missing test resource: " + resource, is);
+        return is;
+    }
+
     @Before
     public void setup() {
         sut = new CachingOrcidRestConnector();
@@ -59,40 +77,54 @@ public void getAccessToken() {
     }
 
     @Test
-    public void getLabel() {
+    public void getLabel() throws Exception {
         sut = Mockito.spy(sut);
         sut.setApiURL("https://pub.sandbox.orcid.org/v3.0");
         //Mock the CachingOrcidRestConnector so that getAccessToken returns sandboxToken
         doReturn(sandboxToken).when(sut).getAccessToken(Mockito.anyString(), Mockito.anyString(), Mockito.anyString());
+        //Mock the HTTP layer with a canned response so we don't depend on the live ORCID sandbox.
+        doReturn(cannedResponse(EXPANDED_SEARCH_XML)).when(sut).httpGet(Mockito.anyString(), Mockito.anyString());
 
         String label = sut.getLabel(orcid);
         assertEquals(expectedLabel, label);
     }
     @Test
-    public void search() {
+    public void search() throws Exception {
         sut = Mockito.spy(sut);
         sut.setApiURL("https://pub.sandbox.orcid.org/v3.0");
         //Mock the CachingOrcidRestConnector so that getAccessToken returns sandboxToken
         doReturn(sandboxToken).when(sut).getAccessToken(Mockito.anyString(), Mockito.anyString(), Mockito.anyString());
+        //Mock the HTTP layer with a canned ORCID expanded-search response. Previously this test hit the live
+        //ORCID sandbox and asserted numFound() > 1000, which flaked whenever the sandbox dataset was reset/shrunk.
+        //Mocking the transport keeps the real parsing + edismax wildcard query-building path under test, but makes
+        //the result deterministic.
+        doReturn(cannedResponse(EXPANDED_SEARCH_XML)).when(sut).httpGet(Mockito.anyString(), Mockito.anyString());
 
         ExpandedSearchConverter.Results search = sut.search("joh", 0, 1);
-        //Should match all Johns also, because edismax with wildcard
-        assertTrue(search.numFound() > 1000);
+        assertTrue("Expected a successful ORCID response, got: " + search, search.isOk());
+        //'joh' is alphabetic, so the connector turns it into an edismax wildcard query ("joh || joh*") that matches
+        //many authors; the canned response carries num-found=1725.
+        assertEquals("Unexpected num-found for the canned ORCID response", 1725L, (long) search.numFound());
+        assertEquals("Connor, John", search.results().get(0).label());
     }
 
     @Test
-    public void search_fail() {
+    public void search_fail() throws Exception {
         sut = Mockito.spy(sut);
         sut.setApiURL("https://pub.sandbox.orcid.org/v3.0");
-        //Mock the CachingOrcidRestConnector so that getAccessToken returns and invalid token
+        //Mock the CachingOrcidRestConnector so that getAccessToken returns an invalid token
         doReturn("FAKE").when(sut).getAccessToken(Mockito.anyString(), Mockito.anyString(),
                 Mockito.anyString());
+        //Simulate the ORCID API rejecting the (fake) token: every httpGet fails. Done via the mocked HTTP layer
+        //so the test is deterministic and doesn't rely on the live sandbox returning a 401.
+        doThrow(new IOException("simulated ORCID auth failure")).when(sut)
+                .httpGet(Mockito.anyString(), Mockito.anyString());
 
         ExpandedSearchConverter.Results search = sut.search("joh", 0, 1);
 
         assertFalse(search.isOk());
 
-        //Further calls fail too, token is stored
+        //Further calls fail too, token is stored (so getAccessToken is only resolved once)
         search = sut.search("joh", 0, 1);
         assertFalse(search.isOk());
 

dspace-api/src/test/resources/org/dspace/external/orcid-expanded-search.xml

@@ -0,0 +1,22 @@
+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
+<!--
+    Canned ORCID "expanded-search" response used by CachingOrcidRestConnectorTest so the test does NOT
+    depend on the live ORCID sandbox (https://pub.sandbox.orcid.org), whose dataset is periodically reset
+    and previously caused intermittent CI failures (e.g. num-found dropping below a hard-coded threshold).
+    Namespace/element names mirror org.orcid.jaxb.model.v3.release.search.expanded.ExpandedSearch.
+    The first result intentionally maps to the label "Connor, John" (family-names, given-names).
+-->
+<expanded-search:expanded-search num-found="1725"
+        xmlns:expanded-search="http://www.orcid.org/ns/expanded-search">
+    <expanded-search:expanded-result>
+        <expanded-search:orcid-id>0000-0002-9150-2529</expanded-search:orcid-id>
+        <expanded-search:given-names>John</expanded-search:given-names>
+        <expanded-search:family-names>Connor</expanded-search:family-names>
+    </expanded-search:expanded-result>
+    <expanded-search:expanded-result>
+        <expanded-search:orcid-id>0000-0002-1208-2352</expanded-search:orcid-id>
+        <expanded-search:given-names>John</expanded-search:given-names>
+        <expanded-search:family-names>Kendrew</expanded-search:family-names>
+        <expanded-search:credit-name>John Kendrew</expanded-search:credit-name>
+    </expanded-search:expanded-result>
+</expanded-search:expanded-search>