TOMEE-4650 - Guard undeploy against an already-closed EntityManagerFactory

[jungm]

Fixes TOMEE-4650.

Scope reduced per review — this PR is now only the close() guard. The JpaCDIExtension work has been dropped from this branch, preserved on claude/tomee-4650-jpa-cdi-extension, and tracked as TOMEE-4661.

What changed

When an application closes a container-managed EntityManagerFactory itself, TomEE's undeploy path called close() on it again, and Assembler.destroyApplication then failed with "Attempting to execute an operation on a closed EntityManagerFactory". The test method itself passed; only the undeploy step after it errored.

ReloadableEntityManagerFactory.close() now checks isOpen() before delegating. The check reads the non-lazy delegate field rather than delegate(), so it does not force initialisation of a persistence unit that was never used.

This covers the entityManagerFactoryCloseExceptions TCK vehicles (ClientPmservletTest / ClientPuservletTest).

Tests

ReloadableEntityManagerFactoryCloseTest verifies a second close() is a no-op. Confirmed it fails against the unpatched code (2 close calls instead of 1).

Also re-ran ProducedExtendedEmTest and ResourceLocalCdiEmTest — both green.

Follow-up

The Jakarta Persistence 3.2 CDI qualifier-bean half of the original ticket is not in this PR. The review correctly identified that it caused an AmbiguousResolutionException against the @Produces EntityManager pattern; I reproduced that failure in ProducedExtendedEmTest before splitting. TOMEE-4661 carries the full review findings and will come back as its own PR.

The TCK exclusion removals in apache/tomee-tck also remain outstanding: persistence-javatest.txt can be cleared once this merges; persistence-servlet.txt must wait for TOMEE-4661.

🤖 Generated with Claude Code

[rzo1]

Could you split this? There are two unrelated changes here and they're in very different states.

Part 1 — the ReloadableEntityManagerFactory.close() guard — I'd merge today. It's small,
correct, uses the non-lazy delegate field rather than delegate() so it doesn't force
initialisation, and the test genuinely fails without it. That's the part that fixes the reported
undeploy noise.

Part 2 — JpaCDIExtension — needs rework. It's modelled on ConcurrencyCDIExtension, which is
the right reference, but several of the guards that make that class safe were dropped in the copy.

Blocking:

• For every PU without <qualifier>, validateAndCreateQualifiers returns {@Any, @Default} and
  registerBeans adds EntityManagerFactory and EntityManager beans with those qualifiers, with
  no getBeans() check. Beans added via AfterBeanDiscovery.addBean() are ordinary enabled beans,
  not built-ins, so nothing prefers an application producer over them — that's an
  AmbiguousResolutionException against the @Produces EntityManager pattern, which is about as
  common as CDI/JPA patterns get.

  Two tests in this very module should now fail deployment: ProducedExtendedEmTest
  (EntityManagerProducer.produceEm() + @Inject EntityManager in A, PU cdi-em-extended) and
  ResourceLocalCdiEmTest (EMFProducer.em() + @Inject EntityManager in PersistManager, PU
  rl-unit). Neither is @Ignored, and the extension does run in them —
  OptimizedLoaderService.loadExtensions adds it unconditionally at :130 and OpenEJBLifecycle
  sets CURRENT_APP_INFO at :190 before deployer.deploy().

  ConcurrencyCDIExtension.registerDefaultBeanIfMissing (:359) is exactly the guard that's missing —
  it takes the BeanManager and skips when beanManager.getBeans(type, Default.Literal.INSTANCE)
  is non-empty. OpenEJBLifecycle.addInternalBeans (:244-258) uses the same idiom.

• The loop for (final PersistenceUnitInfo unitInfo : appInfo.persistenceUnits) has no dedup of
  qualifier sets and no filter on unitInfo.webappName. So two unqualified PUs in one app register
  duplicate @Default beans, and in an EAR a webapp gets beans for its siblings' PUs —
  TomcatWebAppBuilder:1454 sets CURRENT_APP_INFO to the whole EAR's AppInfo in the per-webapp
  !webAppAlone branch, while the same method correctly filters on unitInfo.webappName at :1425
  for EMF creation. ConcurrencyCDIExtension scopes this with isVisibleInCurrentApp(resource, currentAppIds)
  (:102); there's no equivalent here.

• The annotation proxy breaks the Annotation equals/hashCode contract: equals is
  annotationType.isInstance(args[0]) and hashCode is annotationType.hashCode(). The spec
  mandates 0 for a marker annotation, and equals ignoring member values makes it asymmetric with a
  real annotation instance — so a qualifier with members can select the wrong PU. The correct
  implementation is ~150 lines away in ConcurrencyCDIExtension (annotationEquals :235,
  annotationHashCode :254, annotationToString) and was replaced here by two one-liners. Please
  reuse it rather than reimplementing.

  Related omission: ConcurrencyCDIExtension.validateAndCreateQualifiers (:184-195) rejects
  qualifiers with members lacking defaults and members lacking @Nonbinding, via two
  addDefinitionError calls. JpaCDIExtension.validateAndCreateQualifiers stops at the @Qualifier
  check. So @Qualifier @interface Unit { String value(); } gives getDefaultValue() == null, the
  proxy returns null from value(), and you get either an NPE inside OWB or a bean that can never
  be matched — with no diagnostic. The javadoc on createAnnotation asserts "the CDI qualifier
  rules guarantee [a default] to exist"; that guarantee doesn't exist.

Also:

• jakarta.persistence.qualifiers and jakarta.persistence.scope aren't spec properties. I unpacked
  jakarta.persistence-api-3.2.0.jar — neither string appears anywhere in the jar, and
  PersistenceConfiguration declares constants for every standard override property (JDBC_,
  LOCK_TIMEOUT, QUERY_TIMEOUT, SCHEMAGEN_, VALIDATION_*, CACHE_MODE) with nothing for these two.
  persistence_3_2.xsd defines only the <qualifier>/<scope> elements. Please don't mint new
  property names under the jakarta.* namespace — openejb.* is the right prefix for a
  TomEE-specific carrier.

• The SchemaManager bean can only ever inject null: addUtilityBean(..., SchemaManager.class, EntityManagerFactory::getSchemaManager)
  on the reloadable EMF. Either wire it properly or drop it.

• The jakarta.transaction-absent fallback is unreachable — openejb-core has a hard dependency on it
  (cdi/transactional/TransactionContext imports TransactionManager/TransactionScoped directly
  and extends AbstractContext(TransactionScoped.class)); the module can't load without it. Its only
  possible effect is a silent scope change, and the javadoc describes reflective member access that
  isn't happening.

• <scope> isn't validated as an actual CDI scope, unlike the <qualifier> right above it. A typo
  there fails much later and much less clearly.

• The JNDI prefix is hardcoded rather than using JndiConstants.PERSISTENCE_UNIT_NAMING_CONTEXT.

• qualifierSelectsTheMatchingPersistenceUnit is tautological — it would pass against a stub.

Question rather than a finding: produceWith(instance -> lookupEntityManagerFactory(unitInfo.id).createEntityManager())
hands out the provider EM unwrapped by JtaEntityManager, so it bypasses TomEE's JTA integration —
and resolveEntityManagerScope never consults unitInfo.transactionType (populated at
AppInfoBuilder:685), so a RESOURCE_LOCAL unit gets TransactionScoped. TransactionContext.isActive()
(:50-60) returns false with no JTA transaction, so that EM is unusable outside one. Is
TransactionScoped mandated unconditionally by the platform spec here, with apps expected to declare
<scope> for RESOURCE_LOCAL? The 3.2 XSD (:117) documents <scope> with no stated default, so I
couldn't settle it from the schema alone.

Low-priority, only reachable via the JPA JMX operations: the @Dependent
CriteriaBuilder/Metamodel/Cache/PersistenceUnitUtil beans call the accessor once at
instantiation, so they capture the current delegate. ReloadableEntityManagerFactory.reload()
(:383-389) replaces the delegate without closing the old one, so after a reload an
@ApplicationScoped bean holds a CriteriaBuilder bound to the superseded EMF while
@Inject EntityManagerFactory follows the new one. Resolving through the EMF per call, or a
documented limitation, would avoid the inconsistency.

[rzo1]

see review above

[jungm]

Thanks — split done, and you were right on the blocking point.

I reproduced the AmbiguousResolutionException before touching anything: with the extension in place, ProducedExtendedEmTest fails deployment with There is more than one Bean with type jakarta.persistence.EntityManager Qualifiers: [@Default]. That's a regression I should have caught, and it's the clearest argument for splitting.

This PR is now only the close() guard — two lines plus its test. ProducedExtendedEmTest and ResourceLocalCdiEmTest are green again.

The extension is preserved on claude/tomee-4650-jpa-cdi-extension and will return as its own PR. Taking your points: the missing registerDefaultBeanIfMissing-style getBeans() guard, isVisibleInCurrentApp-style app scoping plus the webappName filter and qualifier-set dedup, reusing ConcurrencyCDIExtension's annotationEquals/annotationHashCode rather than my one-liners, and its full validateAndCreateQualifiers including the defaults/@Nonbinding checks.

On the smaller items — you're right that jakarta.persistence.qualifiers/.scope aren't spec properties. I took them from the Platform spec's "Additional EntityManagerFactory Properties" table in CDI-JPA.adoc, but that's a platform-spec table, not something the API jar defines, and minting jakarta.* names was the wrong call regardless; I'll move them to openejb.*. Also agreed on JndiConstants, dropping the unreachable jakarta.transaction fallback, validating <scope>, the dead SchemaManager wiring, and that qualifierSelectsTheMatchingPersistenceUnit is tautological.

On your question about TransactionScoped and RESOURCE_LOCAL: the platform spec states the EntityManager bean's scope is the <scope> element "or jakarta.transaction.TransactionScoped if no scope is explicitly specified", with no carve-out for RESOURCE_LOCAL — so unconditional as written. But that yields an unusable bean for a RESOURCE_LOCAL unit, which reads like a spec gap rather than an intended outcome. I'll consult unitInfo.transactionType and raise it upstream rather than guess. The unwrapped-EM point stands too — it should go through JtaEntityManager.

[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: small and mostly right. Assembler.destroyApplication (~line 2552) is the only production caller of ReloadableEntityManagerFactory.close() in the tree, and the previous behaviour meant an application-initiated close made undeploy throw — which also skipped persistenceClassLoaderHandler.destroy(...) and remf.unregister(), leaking the transformer registration and the MBean. Guarding fixes that, and the delegate isOpen() semantics hold for OpenJPA (!DelegatingBrokerFactory.isClosed()), EclipseLink and Hibernate, so no distribution-specific breakage. There is no second half in tomee-catalina: TomcatWebAppBuilder only calls overrideClassLoader/createDelegate, never close. The new test is non-vacuous — it fails against unpatched code.

My reservation is placement, not intent.

1. The guard is on the public EMF contract rather than the one call site (minor)

ReloadableEntityManagerFactory is not an internal-only helper: it is the object bound at openejb/PersistenceUnit/<id> (Assembler:931), and JndiEncBuilder:396-406 binds a plain IntraVmJndiReference to that same name for every persistenceUnitRef with no wrapper, while TomcatJndiBuilder.mergeRef(PersistenceUnitReferenceInfo) (~:607) does setResource(resource, factory) with the raw REMF. So an application injecting @PersistenceUnit EntityManagerFactory really does hold this instance. (Contrast persistenceContextRefs, which are wrapped in JtaEntityManager — only the EMF ref exposes the REMF.)

jakarta.persistence.EntityManagerFactory#close() is specified to throw IllegalStateException when already closed, and every provider TomEE ships honours that. After this patch, that exception is gone for application callers too, not just for Assembler.destroyApplication.

Since the problem exists at exactly one production call site, the containable fix is there — if (remf.isOpen()) { remf.close(); }, or a try/catch around that single call — which restores the undeploy path without changing the contract of a public EntityManagerFactory implementation.

Two things the refutation pass corrected in the original write-up, so nobody over-weights this: closing a container-managed EMF from application code is itself illegal per the EE/JPA contract, so the swallowed exception only occurs on already-illegal usage. And the TCK argument in the PR body cannot be evaluated from this repo — there is no JPA TCK harness under tck/ (bval, cdi, concurrency, data, jsonb, jsonp, security, jax-rs, microprofile) and no persistence-javatest.txt in the tree — so "the exclusion can be cleared once this merges" is unverifiable here. If that exclusion lives elsewhere, it would be good to confirm the vehicle does not assert the IllegalStateException this patch now swallows.

2. The patch removes only one of several ways undeploy skips destroy()/unregister() (minor)

Assembler.java:2547-2557 wraps remf.close(), persistenceClassLoaderHandler.destroy(unitInfo.id) and remf.unregister() in a single try/catch (Throwable). Any failure inside a provider's close() — not just the already-closed IllegalStateException this PR guards — still jumps to the catch and leaks the classloader transformer registration and the JMX MBean for that persistence unit.

Giving remf.close() its own try/catch, or reordering so destroy()/unregister() always run, is what actually makes undeploy robust — and it is the same one-call-site edit that avoids touching the public close() contract in finding 1. Worth doing both in one go.

3. close() leaves the closed delegate in place (nit, pre-existing)

ReloadableEntityManagerFactory.close() never nulls delegate nor marks the factory closed, so delegate() still returns the non-null closed instance afterwards and createEntityManager()/getProperties()/getCache() hand callers a closed provider EMF that fails deep inside the provider rather than at the boundary. Pre-existing rather than introduced here, but it is the same object-lifecycle gap the PR is papering over — clearing the field in close() would make the new guard unnecessary.

Checked and dismissed

• TOCTOU on the volatile delegate field (three reads in if (delegate != null && delegate.isOpen()) { delegate.close(); }). Pre-patch close() already did two independent reads, so the "read non-null → createDelegate() swaps → close the new delegate" interleaving was already possible; the extra isOpen() read changes nothing about which instance can be closed. The named concurrent caller does not fit either: TomcatWebAppBuilder:1423-1435 only calls createDelegate() when unitInfo.webappName.equals(webAppInfo.moduleId), i.e. for the one webapp that owns that PU during its own deployment. Capturing the field in a local is a fine tidy-up, but it is not a defect this PR creates.
• The new test's quality. It is non-vacuous (2 close calls vs 1 against unpatched code) and well-formed — EntityManagerFactoryCallable's 5-arg constructor, PersistenceUnitInfoImpl.setLazilyInitialized and Reflections.set all match, and lazilyInitialized keeps the constructor from building a real EMF, so it exercises precisely the changed branch. Demanding an Assembler.destroyApplication integration test for a two-token guard would be disproportionate.