Skip to content

VSB-TUO/ORCID Authority fix - do NOT call context.abort() on a locally created Context #1261

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
milanmajchrak merged 4 commits into from
Feb 26, 2026
Merged

VSB-TUO/ORCID Authority fix - do NOT call context.abort() on a locally created Context #1261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aa27ccc
Fix LazyInitializationException in Bundle.getBitstreams during CLI re…
milanmajchrak Feb 26, 2026
da0ee37
Fix Bundle.getBitstreams() triggering lazy loading in uncacheEntity
milanmajchrak Feb 26, 2026
3df3d45
Fix flaky WorkflowCurationIT by clearing plugin cache before test
milanmajchrak Feb 26, 2026
ebb57f6
Fix LazyInitializationException on DSpaceObject.handles during reinde…
milanmajchrak Feb 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions dspace-api/src/main/java/org/dspace/content/authority/SimpleORCIDAuthority.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,11 +152,10 @@ public String getLabel(String key, String locale) {

private String resolveLocalLabel(String key, String locale) {
Context requestContext = ContextUtil.obtainCurrentRequestContext();
boolean createdContext = (requestContext == null);
Context context = requestContext;

try {
if (createdContext) {
if (context == null) {
context = createReadOnlyContext();
}
if (context == null) {
Expand All @@ -166,11 +165,16 @@ private String resolveLocalLabel(String key, String locale) {
} catch (Exception e) {
log.error("Error resolving local label for authority key '{}'", key, e);
return key;
} finally {
if (createdContext && context != null) {
context.abort();
}
}
// We intentionally do NOT call context.abort() on a locally created Context.
// During CLI operations (e.g. reindexing), the new Context shares the Hibernate
// session (thread-local) with the caller's Context. Calling abort() triggers
// closeDBConnection() → rollback(), which kills the shared transaction and causes
// Hibernate to clear the persistence context — detaching ALL managed entities.
// This leads to LazyInitializationException when the caller later accesses
// lazy-loaded properties (e.g. DSpaceObject.handles).
// Since we only performed read operations, no cleanup is needed.
// The session/transaction lifecycle is managed by the caller's Context.
Comment on lines 153 to +177
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolveLocalLabel() now creates a new read-only Context when there is no current request Context, but never closes it. Because Context/DBConnection are backed by a thread-local Hibernate Session, leaving this Context unclosed can leak an open Session/transaction when no caller Context exists, and it can also trigger Context.finalize() later (which may call abort() and close the shared Session while the caller is still using it). Please reintroduce deterministic cleanup for locally-created Contexts (e.g., only create/close a Context when you can guarantee it owns its Session, or use an isolated Session/DataSource-based lookup for the fallback query so it can be safely closed).

Copilot uses AI. Check for mistakes.
Comment on lines +169 to +177
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new comment claims abort() triggers "closeDBConnection() → rollback()", but Context.abort() explicitly skips rollback for READ_ONLY contexts (it only closes the DB connection). This makes the rationale misleading, and it also contradicts the statement "no cleanup is needed" (a locally-created Context still needs its Session/transaction lifecycle handled somehow). Please update the comment to accurately describe what happens for READ_ONLY Contexts and what cleanup strategy is being used.

Suggested change
// We intentionally do NOT call context.abort() on a locally created Context.
// During CLI operations (e.g. reindexing), the new Context shares the Hibernate
// session (thread-local) with the caller's Context. Calling abort() triggers
// closeDBConnection() → rollback(), which kills the shared transaction and causes
// Hibernate to clear the persistence context — detaching ALL managed entities.
// This leads to LazyInitializationException when the caller later accesses
// lazy-loaded properties (e.g. DSpaceObject.handles).
// Since we only performed read operations, no cleanup is needed.
// The session/transaction lifecycle is managed by the caller's Context.
// We intentionally do NOT call context.abort() here, even when we created a
// local READ_ONLY Context. In CLI-style flows (e.g. reindexing) a newly created
// Context may share the underlying, thread-local Hibernate Session/connection
// with an outer caller Context. For READ_ONLY modes, Context.abort() skips any
// rollback but still closes the DB connection backing that shared Session.
// Closing that connection while the outer Context is still in use can break the
// caller's transaction and leave managed entities in an inconsistent state,
// leading to LazyInitializationException when lazy properties are accessed later.
// We only perform read operations here; the owning infrastructure (web request /
// CLI command) is responsible for managing the Session/transaction lifecycle and
// ultimately closing the associated Context(s).

Copilot uses AI. Check for mistakes.
}

Context createReadOnlyContext() {
Expand Down
8 changes: 7 additions & 1 deletion dspace-api/src/main/java/org/dspace/core/HibernateDBConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,13 @@ public <E extends ReloadableEntity> void uncacheEntity(E entity) throws SQLExcep
} else if (entity instanceof Bundle) {
Bundle bundle = (Bundle) entity;

if (Hibernate.isInitialized(bundle.getBitstreams())) {
// Bundle.getBitstreams() creates a defensive copy via new ArrayList<>(bitstreams)
// which iterates the Hibernate proxy, triggering lazy loading unconditionally.
// Unlike Item.getBundles() which returns the raw proxy, we cannot safely call
// getBitstreams() when the bundle is detached from the session.
// Guard with session.contains(): if the bundle is still managed,
// lazy loading will work; if detached (e.g. after session.clear()), we skip.
if (getSession().contains(bundle)) {
for (Bitstream bitstream : Utils.emptyIfNull(bundle.getBitstreams())) {
uncacheEntity(bitstream);
}
Expand Down
8 changes: 7 additions & 1 deletion dspace-api/src/main/java/org/dspace/discovery/SolrServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,13 @@ public void updateIndex(Context context, boolean force, String type) {
final Iterator<IndexableObject> indexableObjects = indexableObjectService.findAll(context);
while (indexableObjects.hasNext()) {
final IndexableObject indexableObject = indexableObjects.next();
indexContent(context, indexableObject, force);
try {
indexContent(context, indexableObject, force);
} catch (RuntimeException e) {
log.error("Failed to index object {} (type={}): {}",
indexableObject.getUniqueIndexID(), indexableObject.getType(),
e.getMessage(), e);
}
Comment on lines +361 to +367
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catching and swallowing RuntimeException here changes updateIndex() behavior: previously a RuntimeException would bubble up and fail the indexing run, but now the method will continue and still commit at the end, with no way for callers (e.g. IndexClient) to detect partial/failed indexing. Consider tracking failures and surfacing them (e.g., rethrow at end, return a status, or at least log a summary and skip the final commit when failures occurred) so operational tooling can reliably detect unsuccessful indexing runs.

Copilot uses AI. Check for mistakes.
context.uncacheEntity(indexableObject.getIndexedObject());
indexObject++;
if ((indexObject % 100) == 0 && indexableObjectService instanceof ItemIndexFactory) {
Expand Down
9 changes: 9 additions & 0 deletions dspace-api/src/test/java/org/dspace/workflow/WorkflowCurationIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,15 @@
import org.dspace.content.Community;
import org.dspace.content.MetadataValue;
import org.dspace.content.service.ItemService;
import org.dspace.core.LegacyPluginServiceImpl;
import org.dspace.ctask.testing.MarkerTask;
import org.dspace.eperson.EPerson;
import org.dspace.util.DSpaceConfigurationInitializer;
import org.dspace.util.DSpaceKernelInitializer;
import org.dspace.xmlworkflow.storedcomponents.XmlWorkflowItem;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.junit4.SpringRunner;

Expand All @@ -46,6 +48,8 @@ public class WorkflowCurationIT
extends AbstractIntegrationTestWithDatabase {
@Inject
private ItemService itemService;
@Autowired
private LegacyPluginServiceImpl legacyPluginService;

/**
* Basic smoke test of a curation task attached to a workflow step.
Expand All @@ -57,6 +61,11 @@ public void curationTest()
throws Exception {
context.turnOffAuthorisationSystem();

// Reset the named plugin cache to avoid pollution from other tests
// (e.g. CreateMissingIdentifiersIT) that may have run before this one.
// See https://github.com/DSpace/DSpace/issues/8533
legacyPluginService.clearNamedPluginClasses();
Comment on lines 25 to +67
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test reaches into the concrete LegacyPluginServiceImpl via @Autowired just to clear the named-plugin cache. Elsewhere in tests (e.g. CreateMissingIdentifiersIT / CuratorTest) this is done via CoreServiceFactory.getInstance().getPluginService().clearNamedPluginClasses(), which avoids depending on the implementation class and Spring wiring. Please switch to using the PluginService API (or CoreServiceFactory) here to keep the test less brittle and aligned with existing test patterns.

Copilot uses AI. Check for mistakes.

//** GIVEN **

// A submitter;
Expand Down