From 9406425e677c8ffd3cd7b339a434c50a1f0c4183 Mon Sep 17 00:00:00 2001 From: bram-atmire Date: Sat, 18 Apr 2026 09:53:27 +0200 Subject: [PATCH 1/3] Contain IT teardown failures so one flaky cleanup doesn't cascade Wrap each builder's cleanup() in its own try/catch in AbstractBuilderCleanupUtil so a single failure (e.g. the intermittent Hibernate ConcurrentModificationException in ResourceRegistryStandardImpl.releaseResources) no longer aborts cleanup of the remaining builders. The first failure is rethrown with any subsequent failures attached via addSuppressed. Restructure AbstractIntegrationTestWithDatabase.destroy() to run the shared-state resets (Solr cores, authority cache, QA events, config service, builder cache) in a finally block, so that state is always reset between tests even when builder cleanup throws. Previously those resets sat after the try/catch, so any teardown exception would skip them and poison every subsequent test in the class. Fixes #12324 --- .../AbstractIntegrationTestWithDatabase.java | 65 ++++++++++++++----- .../util/AbstractBuilderCleanupUtil.java | 27 +++++++- 2 files changed, 74 insertions(+), 18 deletions(-) diff --git a/dspace-api/src/test/java/org/dspace/AbstractIntegrationTestWithDatabase.java b/dspace-api/src/test/java/org/dspace/AbstractIntegrationTestWithDatabase.java index d5bb6ab8a4c8..84c03317b00c 100644 --- a/dspace-api/src/test/java/org/dspace/AbstractIntegrationTestWithDatabase.java +++ b/dspace-api/src/test/java/org/dspace/AbstractIntegrationTestWithDatabase.java @@ -166,30 +166,63 @@ public void setUp() throws Exception { */ @After public void destroy() throws Exception { - // Cleanup our global context object + // Contain the blast radius of teardown failures: the shared static state (Solr cores, authority + // cache, configuration, builder cache) must be reset regardless of whether builder cleanup threw. + // Otherwise a single flake in one test's teardown poisons every subsequent test in the class. + Exception primaryFailure = null; try { AbstractBuilder.cleanupObjects(); parentCommunity = null; cleanupContext(); + } catch (Exception e) { + primaryFailure = new RuntimeException("Error cleaning up builder objects & context object", e); + } finally { + try { + resetSharedState(); + } catch (Exception e) { + if (primaryFailure == null) { + primaryFailure = e; + } else { + primaryFailure.addSuppressed(e); + } + } + } + if (primaryFailure != null) { + throw primaryFailure; + } + } - ServiceManager serviceManager = DSpaceServicesFactory.getInstance().getServiceManager(); - // Clear the search core. - MockSolrSearchCore searchService = serviceManager - .getServiceByName(null, MockSolrSearchCore.class); - searchService.reset(); - // Clear the statistics core. - serviceManager - .getServiceByName(SolrStatisticsCore.class.getName(), MockSolrStatisticsCore.class) - .reset(); + /** + * Reset all shared static state between tests: Solr cores, authority cache, QA events, configuration + * service, and the builder cache. Called from {@link #destroy()} inside a finally block so this always + * runs, even if earlier teardown steps failed. + * + * @throws Exception if reloading configuration or resetting the builder cache fails + */ + private void resetSharedState() throws Exception { + ServiceManager serviceManager = DSpaceServicesFactory.getInstance().getServiceManager(); - MockSolrLoggerServiceImpl statisticsService = serviceManager - .getServiceByName("solrLoggerService", MockSolrLoggerServiceImpl.class); - statisticsService.reset(); + // Clear the search core. + MockSolrSearchCore searchService = serviceManager + .getServiceByName(null, MockSolrSearchCore.class); + searchService.reset(); - MockAuthoritySolrServiceImpl authorityService = serviceManager - .getServiceByName(AuthoritySearchService.class.getName(), MockAuthoritySolrServiceImpl.class); - authorityService.reset(); + // Clear the statistics core. + serviceManager + .getServiceByName(SolrStatisticsCore.class.getName(), MockSolrStatisticsCore.class) + .reset(); + // Reset the statistics logger service + MockSolrLoggerServiceImpl loggerService = serviceManager + .getServiceByName("solrLoggerService", MockSolrLoggerServiceImpl.class); + loggerService.reset(); + + // Clear the authority core. + MockAuthoritySolrServiceImpl authorityService = serviceManager + .getServiceByName(AuthoritySearchService.class.getName(), MockAuthoritySolrServiceImpl.class); + authorityService.reset(); + + try { // Reload our ConfigurationService (to reset configs to defaults again) DSpaceServicesFactory.getInstance().getConfigurationService().reloadConfig(); diff --git a/dspace-api/src/test/java/org/dspace/builder/util/AbstractBuilderCleanupUtil.java b/dspace-api/src/test/java/org/dspace/builder/util/AbstractBuilderCleanupUtil.java index 18f1f82c290a..d41b9768f3e1 100644 --- a/dspace-api/src/test/java/org/dspace/builder/util/AbstractBuilderCleanupUtil.java +++ b/dspace-api/src/test/java/org/dspace/builder/util/AbstractBuilderCleanupUtil.java @@ -12,6 +12,8 @@ import java.util.List; import java.util.Map; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.dspace.builder.AbstractBuilder; import org.dspace.builder.BitstreamBuilder; import org.dspace.builder.BitstreamFormatBuilder; @@ -48,6 +50,8 @@ */ public class AbstractBuilderCleanupUtil { + private static final Logger log = LogManager.getLogger(AbstractBuilderCleanupUtil.class); + private final LinkedHashMap> map = new LinkedHashMap<>(); @@ -104,15 +108,34 @@ public void addToMap(AbstractBuilder abstractBuilder) { /** * This method takes care of iterating over all the AbstractBuilders in the predefined order and calls * the cleanup method to delete the objects from the database. - * @throws Exception If something goes wrong + *

+ * Each builder is cleaned up inside its own try/catch so that a single failure (e.g. the intermittent + * Hibernate {@link java.util.ConcurrentModificationException} in resource registry cleanup) does not + * abort cleanup of the remaining builders. The first failure is rethrown at the end with any subsequent + * failures attached as suppressed exceptions, so nothing is silently swallowed. + *

+ * @throws Exception If one or more builders fail to clean up */ public void cleanupBuilders() throws Exception { + Exception firstFailure = null; for (Map.Entry> entry : map.entrySet()) { List list = entry.getValue(); for (AbstractBuilder abstractBuilder : list) { - abstractBuilder.cleanup(); + try { + abstractBuilder.cleanup(); + } catch (Exception e) { + log.error("Error cleaning up builder {}", abstractBuilder.getClass().getName(), e); + if (firstFailure == null) { + firstFailure = e; + } else { + firstFailure.addSuppressed(e); + } + } } } + if (firstFailure != null) { + throw firstFailure; + } } /** From f679b668734d3f781120448a7f79027b80abc322 Mon Sep 17 00:00:00 2001 From: Milan Kuchtiak Date: Wed, 22 Jul 2026 13:21:45 +0200 Subject: [PATCH 2/3] Copilot comments --- .../AbstractIntegrationTestWithDatabase.java | 32 ++++++++++++------- 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/dspace-api/src/test/java/org/dspace/AbstractIntegrationTestWithDatabase.java b/dspace-api/src/test/java/org/dspace/AbstractIntegrationTestWithDatabase.java index 84c03317b00c..ec85f5bbaf16 100644 --- a/dspace-api/src/test/java/org/dspace/AbstractIntegrationTestWithDatabase.java +++ b/dspace-api/src/test/java/org/dspace/AbstractIntegrationTestWithDatabase.java @@ -170,21 +170,29 @@ public void destroy() throws Exception { // cache, configuration, builder cache) must be reset regardless of whether builder cleanup threw. // Otherwise a single flake in one test's teardown poisons every subsequent test in the class. Exception primaryFailure = null; + try { AbstractBuilder.cleanupObjects(); - parentCommunity = null; + } catch (Exception e) { + primaryFailure = new RuntimeException("Error cleaning up builder objects", e); + } + parentCommunity = null; + try { cleanupContext(); } catch (Exception e) { - primaryFailure = new RuntimeException("Error cleaning up builder objects & context object", e); - } finally { - try { - resetSharedState(); - } catch (Exception e) { - if (primaryFailure == null) { - primaryFailure = e; - } else { - primaryFailure.addSuppressed(e); - } + if (primaryFailure == null) { + primaryFailure = new RuntimeException("Error cleaning up context object", e); + } else { + primaryFailure.addSuppressed(e); + } + } + try { + resetSharedState(); + } catch (Exception e) { + if (primaryFailure == null) { + primaryFailure = e; + } else { + primaryFailure.addSuppressed(e); } } if (primaryFailure != null) { @@ -193,7 +201,7 @@ public void destroy() throws Exception { } /** - * Reset all shared static state between tests: Solr cores, authority cache, QA events, configuration + * Reset all shared static state between tests: Solr cores, authority cache, configuration * service, and the builder cache. Called from {@link #destroy()} inside a finally block so this always * runs, even if earlier teardown steps failed. * From 09f17c33604db39de8f86f9b61dbb90f04c6fa37 Mon Sep 17 00:00:00 2001 From: Milan Kuchtiak Date: Wed, 22 Jul 2026 15:25:21 +0200 Subject: [PATCH 3/3] context.abort() doesn't need to be called from ITDSpaceAIP#destroy, as it is called from super.destroy() --- .../src/test/java/org/dspace/content/packager/ITDSpaceAIP.java | 1 - 1 file changed, 1 deletion(-) diff --git a/dspace-api/src/test/java/org/dspace/content/packager/ITDSpaceAIP.java b/dspace-api/src/test/java/org/dspace/content/packager/ITDSpaceAIP.java index c5a73ed539a7..ae9f33270999 100644 --- a/dspace-api/src/test/java/org/dspace/content/packager/ITDSpaceAIP.java +++ b/dspace-api/src/test/java/org/dspace/content/packager/ITDSpaceAIP.java @@ -310,7 +310,6 @@ public void init() { @After @Override public void destroy() { - context.abort(); super.destroy(); }