JCU/fix: prevent orphaned items - abort leaked request Context (backport #1352 to jcu)#1358
JCU/fix: prevent orphaned items - abort leaked request Context (backport #1352 to jcu)#1358jr-rk wants to merge 2 commits into
Conversation
…ackport dc57ffd / #852) Backport the behavioral fix from dc57ffd ("Transaction bug - close context in finally block", PR #852) which is on dtq-dev/customer/lindat but missing from customer/zcu-data. On the request error path a slow submission upload could leave its Hibernate session bound to the Tomcat thread with a dirty, stale Item: the upload throws before reaching context.commit(), and DSpaceRequestContextFilter assigns its `context` local only AFTER chain.doFilter, so on an exception the local stays null and its finally skips the abort -> the session leaks. A later request on that thread then flushes the stale Item as a full-row UPDATE (Item has no @DynamicUpdate/@Version), reverting owning_collection->NULL and in_archive->false while collection2item + handle survive -> the item becomes an unsearchable orphan (e.g. handle 20.500.14592/107). Wrap chain.doFilter in the outer StatelessAuthenticationFilter with a try/finally that reads the Context from the request attribute and abort()s it if still valid, cleaning up the leaked context the inner filter missed. No-op on the normal success path. Only the StatelessAuthenticationFilter hunk is ported; the diagnostic/CI files in dc57ffd are omitted (they conflict and are not needed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit ea6116e)
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR backports a defensive cleanup to prevent orphaned items caused by leaked, thread-bound Hibernate sessions on the request error path. It wraps the request filter chain in StatelessAuthenticationFilter with a try/finally and aborts any still-valid Context stored on the request to ensure transactions/sessions don’t leak across requests.
Changes:
- Wrap
chain.doFilter(req, res)intry/finallywithinStatelessAuthenticationFilter. - In
finally, attempt to abort a still-validContextretrieved from the request attribute (ContextUtil.DSPACE_CONTEXT) to clean up leaked request contexts.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
This is fixed by Vanilla. |
Problem
On this customer branch a submitted item can silently become orphaned —
owning_collection = NULL,in_archive = false, while itscollection2itemmapping and handle survive — so it vanishes from search/listings, has no breadcrumb, and "Edit Item" throws HTTP 500. Tracking issue: #1353. Source: #1352 (and originaldc57ffded8/ #852).Root cause
On the request error path a slow submission upload throws before
context.commit().DSpaceRequestContextFilterassigns itscontextlocal only afterchain.doFilter(...), so on an exception the local staysnulland itsfinallyskips the abort → the Hibernate session leaks, still bound to the Tomcat thread holding a dirty, stale Item. A later request on that thread flushes the stale Item as a full-rowUPDATE(Item has no@DynamicUpdate/@Version), revertingowning_collection → NULL/in_archive → false.Change set
Backport of the single behavioral hunk from
dc57ffded8(as shipped tocustomer/zcu-datain #1352, commitea6116ebec1): the outerStatelessAuthenticationFilternow wrapschain.doFilter(...)intry/finallyand, in thefinally, reads the Context fromreq.getAttribute(ContextUtil.DSPACE_CONTEXT)andabort()s it when still valid — cleaning up the leaked context the inner filter missed. No-op on the success path (request already committed → context no longer valid). One file, +17/-1; no new imports (Context/ContextUtil/logalready present). Diagnostic/CI files fromdc57ffded8intentionally omitted.Test evidence
Risk & rollback
Very low: 17-line change, no schema/API change, success path is a no-op; identical to code already merged on
dtq-dev/customer/zcu-data/customer/vsb-tuo/customer/lindat. Rollback = revert this single commit; no data migration.Notes / assumptions
Behavioral-only backport. Defense-in-depth follow-ups are tracked separately in #1353 (null-local fix in
DSpaceRequestContextFilter;@DynamicUpdate/@VersiononItem/DSpaceObject; read-path 500 guard #1348).