Start index jobs after the transaction commits, shorten index waits in ITs

[ErykKul]

What this PR does / why we need it:

Fixes the flaky search tests (SearchIT, DataRetrieverApiIT, see #12710 and #12658) and shortens the index waits in the ITs.

Commands call asyncIndexDataset from inside their transaction, and the background job can run before that transaction commits. When it does, the permission doc misses the creator and the index time is never written (the dvObjectToModify is null errors in the server log), so the dataset stays invisible and the tests wait for nothing. This has been possible since indexing on create became async in #9558; the container based CI is fast enough to hit it.

Index jobs, the index time update and role reindexing now fire a CDI event that runs after the transaction has committed (IndexingRequest, IndexingRequestObserver). No callers change, and the 1 s sleep in IndexAsync goes away.

The requests carry ids, not entities. The background job loads the dataset itself, in a transaction of its own, and the permission reindexing loads the definition points by id (for a revoked role the assignment is already gone, so its definition point id is passed on). Sharing the entity with the request thread was a race: both threads read it at the same time, and EclipseLink's unit of work is not thread-safe. That race produced an NPE inside EclipseLink in a CI run of #12740 (AbstractSession.executeDeferredEvents) and in a develop run of 15 September.

On the test side sleepForReindex polls every 100 ms instead of sleeping a fixed 1.5 s, the dev/CI Solr soft commits every 100 ms, and sleepForDatasetIndex fails on timeout instead of warning. When a search cannot be tied to a dataset (a query for * or a text), the fixed 1.5 s wait stays, and the searches in SearchIT that relied on it now wait on their dataset id first.

Which issue(s) this PR closes:

• Closes #

Special notes for your reviewer:

Verified against the Docker dev stack: both classes pass, 0 index-time errors in the server log (CI runs had 25 to 80), SearchIT in 137 s instead of 409 s.

Suggestions on how to test this:

Run SearchIT and DataRetrieverApiIT against the dev stack with -Ddataverse.test.solr.softcommit.millis=100, then check that docker logs dev_dataverse 2>&1 | grep -c 'dvObjectToModify" is null' prints 0.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

No.

Is there a release notes update needed for this change?:

Yes: indexing now starts only after the transaction that changed a dataset or its permissions has committed, so a new dataset no longer risks being missing from search for its creator until the next reindex.

Additional documentation:

The testing guide documents -Ddataverse.test.solr.softcommit.millis.

[coveralls]

coverage: 25.443% (+0.03%) from 25.414% — flaky-tests-fix into develop

[github-actions]

Test Results

406 tests  ±0   391 ✅ ±0   24m 7s ⏱️ - 9m 35s
 55 suites ±0    15 💤 ±0 
 55 files   ±0     0 ❌ ±0 

Results for commit 29c49fa. ± Comparison against base commit 36d1f0f.

♻️ This comment has been updated with latest results.

[pdurbin]

@ErykKul can you please resolve the merge conflicts?

[ErykKul]

@pdurbin conflicts resolved.

[qqmyers]

Thanks for fixing this! Two comments:

1. I ran AI and it said the following. I don't know if it's worth making a change or not, but thought I'd put it here:

Medium — transactional requests carry detached JPA entities across the commit boundary
IndexingRequest stores Dataset, RoleAssignment, and Collection<DvObject> instances, and the observer passes them to asynchronous background methods after the transaction has completed. ([raw.githubusercontent.com](https://raw.githubusercontent.com/IQSS/dataverse/c343893dc4a4ef25227ea57bde1da84090e7c319/src/main/java/edu/harvard/iq/dataverse/search/IndexingRequestObserver.java))

For example:

java
record IndexDataset(Dataset dataset, boolean doNormalSolrDocCleanUp)
record IndexRole(RoleAssignment roleAssignment)
record IndexRoles(Collection<DvObject> dvObjects)
After commit, these objects are detached. If any of them—or a collection passed as IndexRoles—contains lazy state, the asynchronous method can encounter lazy-initialization failures or observe stale entity state. The risk is especially relevant because doIndexDataset() traverses substantial dataset/version/file metadata state.

Recommendation: Prefer request records containing IDs and primitive options:

java
record IndexDataset(Long datasetId, boolean cleanup)
record IndexRole(Long roleAssignmentId)
record IndexRoles(Collection<Long> definitionPointIds)
Then reload entities inside the asynchronous EJB transaction. This also makes the after-commit boundary explicit and avoids retaining persistence-context-owned objects.

This may already have been possible with the previous asynchronous implementation, but the new code makes the post-commit detached-object handoff a central part of the design, so it should be addressed or explicitly tested.

2. It seems to me that we generally use the onSuccess() method in commands as though they run after the main transaction (and so other things besides indexing may have this type of problem). I wonder if (for v6.13+, not a .1 fix) if using the technique here on the whole completeCommand loop through onSuccess() methods would be a better fix or if we can even adjust the transaction annotations/boundaries there to avoid having to monitor them.

[ErykKul]

@qqmyers

On your first point: good hint, but for a different reason. EclipseLink loads lazy relations of detached entities without trouble, so that part is fine. The real problem is that the request thread and the background indexer share the same entity objects, and EclipseLink's unit of work is not thread-safe. That race just failed a CI run on #12740 (an NPE inside EclipseLink) and it shows up in a develop run from 15 September too. Passing ids and loading in the background thread fixes it. One catch: for a revoked role the assignment is already deleted, so the event has to carry the id of the collection or dataset it was on. I'm working on that now, and I'll also make the test waits more careful. A commit is coming.

On your second point: agreed, and I checked where it bites. onSuccess runs after the commit only when the command is submitted from code without a transaction, i.e. the API and the pages. From a bean that already has one, it runs before the commit. Examples:

• Publishing through a workflow: WorkflowServiceBean.start/resume are @Asynchronous with the default transaction and submit FinalizeDatasetPublicationCommand inside it, so the notifications, the post-publish workflow and the async export start before the publish is committed. The export can read the unpublished state.
• Harvesting (ImportServiceBean, REQUIRES_NEW): UpdateHarvestedDatasetCommand.onSuccess writes to Solr before the commit.
• deleteHarvestedDataset (REQUIRES_NEW): DestroyDatasetCommand.onSuccess deletes the Solr docs before the commit.
• Saved search links (submitInNewTransaction): LinkDatasetCommand.onSuccess reindexes the linking collection inside that transaction.

I'd keep this PR to indexing and open an issue for 6.13: run the whole onSuccess loop after the commit, the same way, with an audit of the hooks first (they also send notifications, do storage accounting and start workflows). Changing the transaction attributes alone won't do it: REQUIRES_NEW on the inner engine would break nested commands. And the event here stays useful either way, since some indexing is requested directly from inside execute and from the role service.

[ErykKul]

I am investigating the failed tests.

[sonarqubecloud]

Quality Gate failed

Failed conditions
37.7% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

[github-actions]

📦 Pushed preview images as

ghcr.io/gdcc/dataverse:flaky-tests-fix

ghcr.io/gdcc/configbaker:flaky-tests-fix

🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name.