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.