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.