Fix flaky tests in IT pipeline

dspace-api/src/main/java/org/dspace/core/AbstractHibernateDAO.java

@@ -306,11 +306,23 @@ public Iterator<T> iterate(Query query) {
         return new AbstractIterator<T> () {
             @Override
             protected T computeNext() {
-                return iter.hasNext() ? iter.next() : endOfData();
-            }
-            @Override
-            public void finalize() {
+                if (iter.hasNext()) {
+                    return iter.next();
+                }
+                // Close the backing ScrollableResults / JDBC cursor as soon as the iteration is exhausted, on
+                // the thread that owns the Hibernate Session (the caller's thread).
+                //
+                // This MUST NOT be done from a finalize() override (as it previously was): finalize() runs on
+                // the GC Finalizer thread, so closing the stream there mutates the Session's per-session,
+                // non-thread-safe JDBC ResourceRegistry (xref) concurrently with the owning thread. That is a
+                // genuine data race which intermittently throws ConcurrentModificationException from
+                // ResourceRegistryStandardImpl.releaseResources during an unrelated commit/rollback (observed
+                // as flaky integration-test failures in AbstractIntegrationTestWithDatabase teardown).
+                //
+                // An iterator abandoned before exhaustion no longer leaks: its open statement is released
+                // safely when the owning Context/Session is closed (releaseResources runs on the owning thread).
                 stream.close();
+                return endOfData();
             }
         };
     }

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/core/AbstractHibernateDAOIteratorIT.java

@@ -0,0 +1,76 @@
+/**
+ * The contents of this file are subject to the license and copyright
+ * detailed in the LICENSE and NOTICE files at the root of the source
+ * tree and available online at
+ *
+ * http://www.dspace.org/license/
+ */
+package org.dspace.core;
+
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.fail;
+
+import java.util.Iterator;
+
+import org.dspace.AbstractIntegrationTestWithDatabase;
+import org.dspace.content.MetadataValue;
+import org.dspace.content.factory.ContentServiceFactory;
+import org.dspace.content.service.MetadataValueService;
+import org.junit.Test;
+
+/**
+ * Regression test for the intermittent {@code ConcurrentModificationException} thrown during
+ * {@code @After} integration-test cleanup and traced to
+ * {@link AbstractHibernateDAO#iterate(javax.persistence.Query)}.
+ *
+ * <p>That method used to close its Hibernate {@code Stream} from a {@code finalize()} override. {@code finalize()}
+ * runs on the GC Finalizer thread, so closing the stream there mutated the owning {@code Session}'s per-session,
+ * non-thread-safe JDBC {@code ResourceRegistry} (xref) concurrently with the thread that owns the session. That
+ * is a genuine data race which intermittently threw {@code ConcurrentModificationException} from
+ * {@code ResourceRegistryStandardImpl.releaseResources} during an unrelated commit/rollback. The fix closes the
+ * stream on the owning thread once the iteration is exhausted. This test guards against reintroducing any
+ * stream-closing finalizer on the returned iterator.</p>
+ */
+public class AbstractHibernateDAOIteratorIT extends AbstractIntegrationTestWithDatabase {
+
+    private final MetadataValueService metadataValueService =
+            ContentServiceFactory.getInstance().getMetadataValueService();
+
+    /**
+     * Verifies that the iterator returned by {@link AbstractHibernateDAO#iterate(javax.persistence.Query)}
+     * (exercised here through {@code MetadataValueService.findByValueLike}) does not close its backing Hibernate
+     * stream from a {@code finalize()} override anywhere in its class hierarchy, and that it still iterates to
+     * exhaustion (closing its cursor on the owning thread) without error. No matching rows are required - the
+     * wrapper iterator is created regardless of the result count.
+     *
+     * @throws Exception passed through.
+     */
+    @Test
+    public void iterateIteratorMustNotCloseStreamFromFinalizer() throws Exception {
+        Iterator<MetadataValue> iterator =
+                metadataValueService.findByValueLike(context, "no-such-metadata-value-" + System.nanoTime());
+        assertNotNull(iterator);
+
+        // The returned iterator - and every class in its hierarchy up to Object - MUST NOT declare a finalize()
+        // override: closing the backing Hibernate Stream from the GC Finalizer thread is exactly the cross-thread
+        // access to the non-thread-safe per-session JDBC ResourceRegistry that caused the flaky
+        // ConcurrentModificationException. Walking the hierarchy also catches a finalizer reintroduced on a
+        // superclass/helper rather than on the anonymous leaf class.
+        for (Class<?> type = iterator.getClass(); type != null && type != Object.class; type = type.getSuperclass()) {
+            try {
+                type.getDeclaredMethod("finalize");
+                fail("AbstractHibernateDAO.iterate() iterator must not declare a finalize() override (found on "
+                        + type.getName() + ") - closing the Hibernate Stream on the GC Finalizer thread races the "
+                        + "owning thread's non-thread-safe JDBC ResourceRegistry and intermittently throws "
+                        + "ConcurrentModificationException.");
+            } catch (NoSuchMethodException expected) {
+                // good: no stream-closing finalizer on this class
+            }
+        }
+
+        // It must still iterate to exhaustion and close its cursor on THIS (the owning) thread without error.
+        while (iterator.hasNext()) {
+            assertNotNull(iterator.next());
+        }
+    }
+}

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>