test: de-flake scheduled ITs (test-planted handle collisions, leaked bitstream formats, stale Solr searcher)

[milanmajchrak]

Problem description

Scheduled dtq-dev "Run Integration Tests" fails in roughly 1 of 5 runs, each time in a different test. Not a regression: the same SHAs pass most runs. Red runs from the last two weeks: 32009094158, 31883563801, 31653657025 (dspace-api), 31147975599, 31873509292, 30976733926 (dspace-server-webapp). Continues the de-flake series #1321, #1344, #1346. Test-only changes.

Failing test	Symptom	Root cause
SubscribeServiceIT.unsubscribeByAdmin (2 runs), OrcidPublicationDataProviderIT.testSearchWithInvalidOrcidId	NPE in init / "Calling method 'createQuery' is not valid without an active transaction (MARKED_ROLLBACK)"	test-planted handle collides with handle_seq
BitstreamFormatRestRepositoryIT.create*	totalElements expected 95, was 108	13 format rows leaked by PreviewContentServiceImplIT
StatisticsRestRepositoryIT.totalVisitsReport_Item_Visited	views 0 instead of 1	report query hit a stale Solr searcher
WorkspaceItemRestRepositoryIT.patchAddMetadataTest	commons-configuration2 "Cannot determine parent!"	one-shot config-model race, not fixed here (see note on reruns)

Analysis

Test-planted handle collisions (dspace-api). RequiredMetadataIT and ItemHandleCheckerIT create a collection with an explicit handle 123456789/<1000 + Random.nextInt(1000)> (randomized by ufal#1273 to dodge a different collision). Handle rows survive object deletion (unbindHandle only nulls resource_id), and handle_seq climbs to ~1150-1400 by the end of the module run - since #1237 added ~250 mints per run, the sequence reaches the random range. The sequence mint path does no existence check, so when it catches up to the drawn number, the flush of whichever test is minting at that moment dies:

Unique index or primary key violation: "PUBLIC.CONSTRAINT_7E95_INDEX_A ON PUBLIC.HANDLE ... '123456789/1154'"

and its thread-bound session stays MARKED_ROLLBACK. In all three red runs the colliding value equals the lower of the two random draws (1154, 1152, 1127); the two sampled green runs contain no violation.

Leaked bitstream formats (webapp). PreviewContentServiceImplIT.destroy() deleted its custom format through the shared test context, where the delete is never committed. One row leaked per test (13 total), so BitstreamFormatRestRepositoryIT's absolute count assertions (95) failed whenever class scan order put Preview first. The 'Test shortNN' violations visible in logs are the intentional 500 of createAlreadyExisting, present in green runs too.

IT reruns never ran. The IT job passes -Dfailsafe.rerunFailingTestsCount=2, but maven-failsafe-plugin 2.22.x only reads surefire.rerunFailingTestsCount. All failsafe reports of all red runs contain zero rerun entries. The one-line build.yml fix is not in this PR (pushing workflow files needs a token with workflow scope); it will come separately.

Fix (test-only)

• RequiredMetadataIT, ItemHandleCheckerIT: deterministic non-numeric handle suffixes the sequence can never produce (e.g. 123456789/required-metadata-test). Distinct per class and stable across reruns, so the protection from Fixing RequiredMetadataIT ufal/clarin-dspace#1273 is kept. XmlWorkflowFactoryTest gets the same treatment for its 123456789/999.
• PreviewContentServiceImplIT: delete the custom format by id in its own Context after super.destroy().
• BitstreamFormatRestRepositoryIT: short descriptions derived from the test method name instead of Random, id captured on the POST so the finally-block cleans up even when a later assertion fails, count assertions relative to a baseline read at test start.
• StatisticsRestRepositoryIT: commit the statistics core (waitSearcher=true) after posting view events, as topCountriesReport_Community_Visited already did; applied to all view-then-assert tests.

Not included on purpose: hardening the mint path itself (skip suffixes whose handle already exists). That would also cover production minting after handle imports, but it touches HandleServiceImpl; it is prepared with regression tests and can come as a separate PR if wanted.

Verification

All runs in Docker (maven:3.9-eclipse-temurin-11, CI flags):

• dtq-dev with the explicit handle pinned to 123456789/20 + -Dit.test=RequiredMetadataIT,SubscribeServiceIT: fails with the exact CI signature (NPE at SubscribeServiceIT.init:54, unique violation on '123456789/20'). With this branch the planted handle is gone, same selection: green.
• dtq-dev with -Dit.test=PreviewContentServiceImplIT,BitstreamFormatRestRepositoryIT -Dfailsafe.runOrder=reversealphabetical (the CI ordering): 4 failures, 95-vs-108 as in CI. This branch, same command: green.
• This branch: RequiredMetadataIT, ItemHandleCheckerIT, SubscribeServiceIT, XmlWorkflowFactoryTest, StatisticsRestRepositoryIT, PreviewContentServiceImplIT, BitstreamFormatRestRepositoryIT all green; checkstyle green on both modules.

Manual Testing (if applicable)

• Added to testing scenarios

Copilot review

• Requested review from Copilot

[Copilot]

Pull request overview

This PR aims to eliminate intermittent failures in scheduled integration-test runs by addressing three independent sources of flakiness: handle mint collisions, leaked BitstreamFormat test data, and Solr statistics visibility races. It does so via a small production change in handle minting plus several test harness adjustments to make test data and timing deterministic.

Changes:

• Make handle minting skip already-existing handle suffixes (sequence catch-up safety) and add an integration test to cover the behavior.
• Fix BitstreamFormat test flakiness by ensuring cleanup is committed and by removing Random-based uniqueness/count assumptions.
• Stabilize statistics report assertions by forcing a Solr commit (waitSearcher) after posting view events.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
dspace-server-webapp/src/test/java/org/dspace/app/rest/StatisticsRestRepositoryIT.java	Forces a Solr commit after posting view events to avoid stale-searcher reads in report assertions.
dspace-server-webapp/src/test/java/org/dspace/app/rest/PreviewContentServiceImplIT.java	Ensures BitstreamFormat cleanup runs in its own committed Context to prevent leaked rows across tests.
dspace-server-webapp/src/test/java/org/dspace/app/rest/BitstreamFormatRestRepositoryIT.java	Makes test data deterministic (method-name-based shortDescription), captures created IDs for robust cleanup, and asserts counts relative to a baseline.
dspace-api/src/test/java/org/dspace/xmlworkflow/XmlWorkflowFactoryTest.java	Switches to a non-numeric handle suffix to avoid collisions with the numeric handle sequence.
dspace-api/src/test/java/org/dspace/handle/HandleServiceIT.java	Adds IT coverage ensuring sequence-based minting skips existing handle rows (including unbound rows after deletion).
dspace-api/src/test/java/org/dspace/curate/RequiredMetadataIT.java	Replaces randomized numeric handle suffixes with deterministic non-numeric suffixes that cannot collide with the sequence.
dspace-api/src/test/java/org/dspace/curate/ItemHandleCheckerIT.java	Same as above: deterministic non-numeric handle suffixes to prevent sequence collisions.
dspace-api/src/main/java/org/dspace/handle/HandleServiceImpl.java	Updates handle minting to skip already-existing handle IDs instead of assuming sequence uniqueness.
dspace-api/src/main/java/org/dspace/handle/HandleClarinServiceImpl.java	Mirrors the handle minting skip logic for the CLARIN handle service implementation.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

dspace-api/src/main/java/org/dspace/handle/HandleClarinServiceImpl.java:380

• The new regression tests only exercise HandleServiceImpl through collection creation. HandleClarinServiceImplIT never invokes either blank-handle branch that reaches this new loop, so duplicate skipping in this separate production path remains unverified. Add a test that occupies the next sequence suffix, calls createHandle(context, null) or createExternalHandle with a blank handle, and asserts that the occupied suffix is skipped.

        for (int attempt = 0; attempt < MAX_SUFFIX_ATTEMPTS; attempt++) {
            Long handleSuffix = handleDAO.getNextHandleSuffix(context);
            String handleId = handlePrefix + (handlePrefix.endsWith("/") ? "" : "/") + handleSuffix;
            if (handleDAO.findByHandle(context, handleId) == null) {
                return handleId;

dspace-server-webapp/src/test/java/org/dspace/app/rest/BitstreamFormatRestRepositoryIT.java:159

• The ID is still captured only after both POST assertions run. If the POST successfully inserts the format but matchNoEmbeds() fails, andDo is never reached, idRef remains null, and this test leaks the row—the cleanup failure this change is meant to prevent. Register andDo immediately after perform, before any assertions.

                                                  .andDo(result -> idRef
                                                          .set(read(result.getResponse().getContentAsString(),
                                                                  "$.id")))

[milanmajchrak]

Restructured to test-only changes (2 commits): the mint-path hardening in HandleServiceImpl/HandleClarinServiceImpl and its HandleServiceIT regression tests were dropped from this PR and are prepared separately. The earlier resolved review threads refer to that removed code.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (1)

dspace-server-webapp/src/test/java/org/dspace/app/rest/BitstreamFormatRestRepositoryIT.java:159

• Move the ID capture before the HAL assertion. andExpect(...) evaluates immediately, so if matchNoEmbeds() fails, this andDo is never reached, idRef remains null, and the newly created format leaks—the cleanup path this change is intended to guarantee. Keeping the status check first avoids parsing an error response while still capturing the ID before later assertions.

                                                  .andDo(result -> idRef
                                                          .set(read(result.getResponse().getContentAsString(),
                                                                  "$.id")))