TOMEE-4652 - roll back UserTransaction left over by a request

[jungm]

TOMEE-4652

A servlet or JSP that leaves a bean-managed UserTransaction incomplete leaks that transaction to the next request served on the same pooled Tomcat exec thread. The victim request then sees a bogus transaction status — either missing an expected IllegalStateException or getting a NotSupportedException: Nested Transactions are not supported on its own begin(). Which tests fail depends on which request lands on which thread, which is why the Transactions 2.0 TCK web vehicles (servlet + JSP) fail non-deterministically.

Root cause

Geronimo's TransactionManagerImpl keeps the thread-to-transaction association (and the per-thread transaction timeout) in ThreadLocals that are only cleared by commit() / rollback(). EJBs are wrapped by container interceptors that restore the thread state at the end of the call; plain servlets have no equivalent, and OpenEJBValve's request-teardown finally block cleaned up only the security context. Since Tomcat pools its worker threads, the association survives into the next request.

Fix

• TransactionCleanup (new) — rolls back and unassociates any transaction still active on the thread at request end, and resets the per-thread transaction timeout (which leaks the same way, since Geronimo only clears it inside begin()). If the rollback itself fails it falls back to suspend() so the association never survives the request.
• Invoked from the request-teardown finally in OpenEJBValve (sync path) and OpenEJBSecurityListener.asyncExit() (async complete/error/timeout).
• CoreUserTransaction.resetError(null) now remove()s the ERROR ThreadLocal instead of set(null), so pooled threads don't keep an empty entry pinned. Separate hygiene issue, not the TCK cause.

Testing

UserTransactionLeakTest forces two sequential requests onto a single exec thread (maxThreads=1) and asserts both actually shared the thread (so it can't pass vacuously), that the second request sees STATUS_NO_TRANSACTION, and that it can still run a transaction of its own.

Verified red/green: with the cleanup call removed the test fails with expected:<[STATUS_NO_TRANSACTION]> but was:<[leaked status 0]> and a follow-up NotSupportedException: Nested Transactions are not supported — matching the issue exactly; with the fix it passes. tomee-catalina and tomee-embedded suites are green.

Notes for reviewers

• The full Jakarta Transactions 2.0 TCK was not run here. To confirm end to end, remove the three excluded areas from runner-standalone/exclusions/transactions.txt in the apache/tomee-tck harness and rerun the 49-test baseline.
• Pre-existing failures unrelated to this change exist on main in StatefulBeanManagedTest, InterfaceTransactionTest, and TransactionPropagationTest (confirmed identical on a clean checkout).

🤖 Generated with Claude Code

[rzo1]

Verified the premise against the Geronimo 4.0.0 sources — unassociate() and begin()
are the only places threadTx and transactionTimeoutMilliseconds get cleared, so a web
request really can strand both on a pooled exec thread. Rolling back at request teardown
is the right fix and matches TxBeanManaged.commit(). The single-exec-thread test is a
genuinely good reproduction, and the guard that fails loudly if thread reuse didn't happen
is the right instinct.

The placement rationale is wrong, though, and it should be corrected in the javadoc
because it changes what applications observe:

• The description says the valve runs after ServletRequestListener#requestDestroyed.
  It's the opposite. I disassembled tomcat-catalina 11.0.23: StandardContextValve has no
  fireRequest* call at all. StandardHostValve.invoke fires fireRequestInitEvent at
  offset 72, invokes the Context pipeline (where OpenEJBValve and therefore clean()
  live) at offset 125, and fires fireRequestDestroyEvent only at offset 327 — after the
  pipeline returns.

  So an application ServletRequestListener implementing a tx-per-request pattern and
  committing in requestDestroyed now finds the transaction already rolled back, gets a
  WARN on every single request, and an IllegalStateException from its own commit().
  Filter- and servlet-based patterns are unaffected, so this doesn't invalidate the fix —
  but pre-empting requestDestroyed is a real behavioural change and belongs in the
  javadoc.

• Third uncovered path, more concrete than the async ones: StandardHostValve.invoke
  calls throwable()/status() at offsets 195/298/307, after the pipeline, and
  custom() dispatches the error page through ApplicationDispatcher.include(), not a
  Pipeline. So <error-page> servlets and JSPs run after clean() — they leak exactly as
  before this PR, and they now also run with the request's transaction already rolled back
  and the caller identity already cleared. If you want that covered,
  OpenEJBSecurityListener.RequestCapturer on the Host pipeline
  (TomcatWebAppBuilder:317) wraps all of it.

Smaller:

• In OpenEJBValve, TransactionCleanup.clean() is skipped if listener.exit() throws.
  The async path already gets this right with a nested finally — please mirror it here.
• setTransactionTimeout(0) pins the very ThreadLocal entry that the
  CoreUserTransaction.resetError hunk in this same PR argues against pinning. Worth a
  comment explaining why the tradeoff differs, or reading the timeout first and only
  resetting when non-zero.
• The timeout reset is never asserted. Leaker sets setTransactionTimeout(120) with a
  comment saying it must not leak, and then nothing checks it — the branch is untested.
• Unconditional rollback() logs an ERROR when the leftover transaction is no longer
  rollback-able (already rolled back / marked for rollback by a timeout reaper). Check
  getStatus() against STATUS_ROLLEDBACK/STATUS_ROLLING_BACK first, or log at debug.

Merge precondition rather than a code comment: please re-run the Jakarta Transactions TCK
and drop the corresponding exclusions in apache/tomee-tck with this, since that's the
harness that surfaced it.

[jungm]

Thanks — the ordering correction was right, and I verified it independently before acting on it (javap on tomcat-catalina 11.0.23: StandardContextValve has zero fireRequest* calls; StandardHostValve.invoke has fireRequestInitEvent at 72, the Context pipeline at 113, throwable()/status() at 195/298/307, and fireRequestDestroyEvent at 327).

Rather than only fix the javadoc, I moved the cleanup to OpenEJBSecurityListener.RequestCapturer on the Host pipeline, which wraps all of StandardHostValve#invoke. That covers requestDestroyed, the throwable()/status() paths, and <error-page> servlets/JSPs, and it stops pre-empting applications that complete their transaction in requestDestroyed. The javadoc now describes the real ordering. This also removes the OpenEJBValve call site entirely, so the "skipped if listener.exit() throws" hole goes away with it.

Also addressed:

• Already-rolled-back status: rollback() now checks STATUS_ROLLEDBACK/STATUS_ROLLING_BACK and logs at debug in that case, warning otherwise.
• Timeout reset pinning: kept unconditional, with a comment explaining why the tradeoff differs from resetError. Reading the timeout first isn't available — Geronimo exposes no getter, only the package-private getTransactionTimeoutMilliseconds(long). And setTransactionTimeout(0) stores a null value, which can't pin a webapp classloader the way a stored exception's stack trace can.
• Timeout never asserted: you were right that the branch was untested. Committer now reads the effective timeout of its own transaction and the test distinguishes the leaked 120s from the 600s default. Worth noting the first version of this assertion was a false green — the field is an absolute deadline (duration + currentTime), not a duration, so comparing it to 120000 could never match. Fixed to compare the remaining duration, and confirmed it fails when the cleanup is removed.

On the merge precondition: I have not run the Jakarta Transactions TCK, so the exclusions in apache/tomee-tck are untouched and this shouldn't merge on my testing alone. tomee-catalina and tomee-embedded suites are green.

🤖 Addressed by Claude Code

[rzo1]

Please run a CI full build for this.

[jungm]

https://ci-builds.apache.org/job/Tomee/view/tomee-11.x/job/pull-request-manual-with-tck/56/

[rzo1]

Currently short on time, so AI review only: I ran an adversarial review pass over this PR, followed by a second pass whose job was to refute the first one's findings against the actual code. Everything below survived that second pass (two findings did not and are listed at the end as explicitly dismissed). Take it as input, not as a verdict — I have not run the build.

Overall: the idea is right and the root-cause analysis is accurate. Geronimo's TransactionManagerImpl keeps threadTx/transactionTimeoutMilliseconds in ThreadLocals that only commit()/rollback()/suspend() clear, servlets have no interceptor to restore thread state, and Tomcat pools its exec threads — so a BMT servlet does leak its transaction into the next request. Hooking cleanup into OpenEJBSecurityListener.RequestCapturer is a deliberate and well-argued placement, the resetError ThreadLocal remove() is correct, and UserTransactionLeakTest is a genuine red/green regression test rather than a vacuous one.

Three things worth addressing, none of which is a regression of existing behaviour.

1. Rollback runs under Catalina's TCCL, after CDI request-context teardown (minor, worth fixing before merge)

TransactionCleanup.clean() is called from RequestCapturer.invoke's finally block, and that valve is added to the Host pipeline (TomcatWebAppBuilder.java:317), so it wraps StandardHostValve. StandardHostValve.invoke does Context.bind(MY_CLASSLOADER) at entry, fires requestInit/requestDestroy inside that region, and Context.unbind(...) in every exit path — and StandardContext.unbind sets the TCCL unconditionally. So by the time clean() runs, the TCCL is Catalina's loader and OWB's request context has already been destroyed.

transactionManager.rollback() is not passive: TransactionImpl.rollback() runs afterCompletion() over interposedSyncList + syncList, i.e. application and JPA-provider code. Any ServiceLoader.load(...), Class.forName(name, true, TCCL) or CDI.current() in there resolves against the wrong loader or throws ContextNotActiveException. It is also inconsistent with the async path, where AsyncContextImpl.fireOnComplete() does context.bind(null) around the listeners.

Severity is minor rather than major because this only executes when the application has already leaked a transaction, and the pre-patch behaviour (afterCompletion never running, or running on some later request's thread) is strictly worse. But the fix is cheap — the valve has the Request:

final Context ctx = request.getContext();
final ClassLoader old = ctx == null ? null : ctx.bind(false, null);
try {
    TransactionCleanup.clean();
} finally {
    if (ctx != null) ctx.unbind(false, old);
}

Note this does not restore the CDI request scope, only the classloader.

2. AsyncContext.start(Runnable) is still uncovered (minor)

OpenEJBValve.invoke only registers the OpenEJBSecurityListener as an AsyncListener in the else branch guarded by request.isAsync() && getAsyncContextInternal() != null. On the request that calls startAsync() from inside the servlet, the valve has already run and isAsync() was false, so no listener is ever attached and onComplete/onError/onTimeout — hence the new asyncExit() → clean() — never fire for it. (onStartAsync only fires on already-registered listeners.)

The case that matters is AsyncContext.start(Runnable): AsyncContextImpl.start issues ActionCode.ASYNC_RUN, which AsyncStateMachine submits to the connector endpoint's executor — a pooled exec thread. Application code runs there, can begin() a UserTransaction, and nothing unassociates it. That is the exact TOMEE-4652 symptom, still reproducible after this patch.

This is missing coverage of a corner case that was equally broken before, not something the patch introduces — so a follow-up is fine. But the PR description's claim that asyncExit() covers async complete/error/timeout should be corrected. (The asyncExit() hook is not dead weight, to be fair: when complete() is called from a non-container thread, completion is processed on a connector thread that need not have passed through RequestCapturer.)

3. asyncExit() contradicts the class javadoc's own rationale (minor)

TransactionCleanup's javadoc argues the Host-pipeline placement is deliberate so cleanup happens after ServletRequestListener.requestDestroyed, specifically so an app that completes its transaction in requestDestroyed is not pre-empted. The second call site does the opposite: AsyncContextImpl.fireOnComplete() binds the CL, fires the AsyncListeners (→ asyncExit() → clean()), and only then calls Context.fireRequestDestroyEvent. So on the async completion path a transaction is rolled back before requestDestroyed gets a chance to commit it — exactly the pre-emption the javadoc says was avoided. Either qualify the javadoc or move the async hook.

4. clean() skips the stale association it claims to remove (nit)

The guard is transaction != null && transaction.getStatus() != Status.STATUS_NO_TRANSACTION. In TransactionImpl, a completed rollback (and every commit path) ends with status = STATUS_NO_TRANSACTION, not STATUS_ROLLEDBACK. So a transaction completed by calling Transaction.commit()/rollback() directly on the Transaction object — legal JTA, and the only way an association can survive at all, since TransactionManagerImpl.commit()/rollback() always unassociate() in a finally — presents as threadTx != null && status == NO_TRANSACTION and is skipped. That contradicts the javadoc ("Restores the calling thread to a state with no transaction associated to it"), and the orphaned entry stays in associatedTransactions forever once the next begin() overwrites threadTx, holding its syncList/resources (and thus the webapp classloader) and drifting the JMX active-transaction count.

Dropping the guard is safe: TransactionManagerImpl.rollback() unassociates in its finally even when tx.rollback() throws IllegalStateException, and the catch/suspend() fallback covers the rest.

Two incidental corrections while in there: the STATUS_ROLLEDBACK/STATUS_ROLLING_BACK branch in the private rollback() helper is unreachable for a thread-associated Geronimo transaction, and the comment about "a transaction the reaper already finished" does not describe reality — timeoutTimer.schedule is commented out in this Geronimo version, so there is no reaper.

5. Test parses the response before checking it succeeded (nit)

In transactionDoesNotLeakToNextRequest, victim.substring(0, victim.indexOf(" on ")) runs on the raw response. If StatusReporter hits its catch block the body is failed: … with no " on " marker, so indexOf returns -1 and the test dies with StringIndexOutOfBoundsException instead of an assertion naming the actual response. threadOf() already guards this correctly with assertTrue(marker > 0).

Checked and dismissed

• catch (Throwable) without ExceptionUtils.handleThrowable — that is a Tomcat-internal convention, not this codebase's. Of the tomee-catalina classes catching Throwable, exactly one (MinimumErrorReportValve, which extends a Tomcat class) uses it. Rethrowing an OOME out of a teardown finally lands in StandardHostValve's own catch (Throwable) anyway.
• The reflective TransactionImpl.timeout read being fragile w.r.t. a configured defaultTransactionTimeout — does not apply: the test builds its own embedded Container and never touches the transaction manager, so it gets service-jar.xml's 10-minute default. The reflection itself is acknowledged in a comment and fails loudly; the proposed alternative (assert a later transaction "does not time out") would be slow and flaky.