fix(#733): do not let an uninstalled bundle poison factory resolution

[oscerd]

Fixes #733

What

OsgiFactoryFinder.getResource walks bundleContext.getBundles() and returns
the first bundle with a matching factory descriptor.

getBundles() returns a snapshot, and per the OSGi spec Bundle.getEntry()
throws IllegalStateException once a bundle has been uninstalled. So a
feature:uninstall or bundle:update running concurrently with a resolution
can make the scan throw on a bundle that has nothing to do with the factory
being looked up.

What turns that transient race into a lasting failure is the caching in Camel.
findClass calls getResource from inside
DefaultFactoryFinder.addToClassMap, whose mapping function does:

} catch (Exception e) {
    classesNotFoundExceptions.put(key, e);
    throw RuntimeCamelException.wrapRuntimeException(e);
}

and on entry addToClassMap rethrows the cached exception for that key.
IllegalStateException is an Exception, so the key stays broken for the life
of the context, until clear().

How

• Skip bundles already in state Bundle.UNINSTALLED.
• Treat an IllegalStateException from getEntry as "this bundle has no such
  entry" and continue, rather than letting it fail the whole scan. This covers
  the bundle that is uninstalled between the state check and the call.
• Log at DEBUG which bundle supplied the descriptor. With several providers
  installed, that is what lets an operator tell which one is actually in use —
  the "observable" half of what OsgiFactoryFinder: first-match-wins bundle scan is order-dependent and silently sticky #733 asks for.

Selection is unchanged. The scan still breaks at the first match, so
install order still decides and there is no added cost on a path that is walked
once per key.

Why no WARN on duplicate providers

An earlier revision of this PR dropped the break and warned when more than one
bundle provided the same descriptor. That is not worth it:

• the default camel-core feature installs both camel-xml-io and
  camel-xml-jaxb, and both ship
  META-INF/services/org/apache/camel/modelxml-dumper, with a different
  implementation class (LwModelToXMLDumper vs JaxbModelToXMLDumper). A stock
  install would warn on every startup. The ambiguity is inherited from
  flat-classpath Camel, where classpath order picks the winner the same way.
• it made every key cost a full sweep of all installed bundles (~300 in a
  typical Camel Karaf), instead of stopping at the winner.
• it widened the IllegalStateException exposure above from "bundles ahead of
  the winner" to "every installed bundle".

What this does not fix

The rolling-upgrade half of #733. Once a key resolves, addToClassMap
short-circuits on classMap and getResource is never called again for it, so
a patched bundle installed afterwards is still silently ignored by an
already-running context — and no amount of logging inside getResource can see
that, because it is not reached. Catching it needs cache invalidation on bundle
events, in the spirit of how OsgiTypeConverter revalidates its delegate on
service changes. That is a separate change with its own blast radius; #733 stays
open for it.

Tests

OsgiFactoryFinderTest: no provider, single provider, first-match-wins with
several providers (pinning the selection the rest of the resolution path depends
on), an UNINSTALLED bundle never being asked for an entry, and a bundle
uninstalled mid-scan not failing the lookup.

Tests run: 43, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS

Note

BundleEntry goes from private static class to package-private. getResource
is public and returns it, so private was never actually restricting
anything; this makes the type nameable from tests in the same package.

---

Claude Code on behalf of JB Onofré

[jbonofre]

bundleContext.getBundles() returns a snapshot, and per the OSGi spec Bundle.getEntry() throws IllegalStateException if this bundle has been uninstalled. Before this change, the loop broke at the winner, so only bundles ahead of it were called, and since the descriptors live in early-installed bundles, that was typically a handful.

Now every one of the ~300 installed bundles is called on every key resolution, so the exposure widens by about two orders of magnitude.

What turns that from a transient error into a lasting one is the caching in Camel. DefaultFactoryFinder.addToClassMap:

} catch (Exception e) {
    classesNotFoundExceptions.put(key, e);
    throw RuntimeCamelException.wrapRuntimeException(e);
}

and on entry it rethrows the cached exception for that key.

IllegalStateException is an Exception, and getResource is called from inside this lambda (see findClass, line 50). So a feature:uninstall or bundle:update racing a resolution, including the rolling upgrade this PR is about (leaves that key throwing for the life of the context, until clear()).

I think we should skip bundles in state Bundle.UNINSTALLED and wrap the getEntry call in a try/catch (IllegalStateException) and continue (or keep the break resolution and do the duplicate detection in a separate pass that cannot fail the lookup).

[jbonofre]

Addressed, and this is now the point of the PR. The scan skips Bundle.UNINSTALLED and catches IllegalStateException from getEntry (needed as well as the state check, for the bundle uninstalled between the two).

Confirmed against the 4.18.1 bytecode: the mapping function in addToClassMap catches Exception, does classesNotFoundExceptions.put(key, e) and rethrows, and on entry addToClassMap rethrows the cached exception for that key. So yes — one race and the key is dead until clear().

Two tests cover it: an UNINSTALLED bundle is never asked for an entry, and a bundle that throws mid-scan does not fail the lookup for the bundles after it.

[jbonofre]

Dropping the break makes scan on all bundles. For performance reason, I would keep it.

[jbonofre]

Agreed, the break is back. Selection and scan cost are unchanged from main.

Worth recording why it matters more than it looks: misses cache too (classesNotFound), so without the break every key — hit or miss — costs a full sweep of all installed bundles, ~300 in a typical Camel Karaf install.

[jbonofre]

The default camel-core feature installs both camel-xml-io and camel-xml-jaxb, and both bundles ship META-INF/services/org/apache/camel/modelxml-dumper with different implementation classes.

It's not a big deal in OSGi as we control the loading, so not sure this warn is useful. We could also fix the duplication in the camel-core feature (we don't need the two bundles).

[jbonofre]

Confirmed, and it is a guaranteed false positive rather than a possible one — the two bundles ship different implementations:

• camel-xml-io → class=org.apache.camel.xml.LwModelToXMLDumper
• camel-xml-jaxb → class=org.apache.camel.xml.jaxb.JaxbModelToXMLDumper

and both are in the default camel-core feature (camel-features.xml:285 and :288). So a stock install would warn at every startup.

Dropped the WARN. Note the same ambiguity exists in flat-classpath Camel, where classpath order picks the winner — it is upstream, not a Karaf artifact.

Trimming one of the two bundles from the camel-core feature is a separate discussion; since they provide different dumpers it is a behaviour change, not just a dedup.

[jbonofre]

#733 is about a rolling upgrade: a patched bundle is installed on top on the old one and silently ignored.

That sequence never reaches this branch here:

1. camel-foo/x.y.z installed, context starts, findClass("some-factory") scans, find one provider, caches the class in the classMap: no ambiguity, no WARN.
2. Users installs camel-foo/x.y.f
3. The running context keep using x.y.z class. addToClassMap short-circuits on classMap, so getResource is never called again for that key. And the finder that is actually ignoring the patched bundle never warns.

The WARN only appears in contexts create after both bundles are installed. The same blind spot applies during Karaf startup, where features install progressively and an early-starting context can scan before all providers exist.

So the stated goal ("it is what tells us whether the ambiguity happens in practice") is not met for the #733 scenario. Catching it needs a BundleListener or cache invalidation on bundle events, in the spirit of how OsgiTypeConverter in validates its delegate on service changes.

[jbonofre]

I have not tried to patch around it: no logging inside getResource can see that sequence, because step 3 never reaches getResource at all. Confirmed in the bytecode: classMap.computeIfAbsent, so a resolved key never re-scans.

So the PR no longer claims to detect it. What it keeps is the cheap, honest part: a DEBUG line naming the bundle that supplied each descriptor, which answers "which one is actually in use" for a context that resolves after the fact.

I have left #733 open for the sticky-cache half. Doing it properly means invalidating on bundle events the way OsgiTypeConverter revalidates its delegate: with the wrinkle that OsgiFactoryFinder has no dispose hook today (it is created per resource path by OsgiFactoryFinderResolver and never torn down), so a BundleListener registered from it would leak. That wants its own PR.

[jbonofre]

@oscerd I pushed to this branch directly rather than waiting on a round trip: sorry for stepping on your PR, revert me if you disagree with any of it. Kept you as the commit author. It is rebased on current main as well.

Summary of what changed relative to your revision:

• Kept the break. Selection semantics and the cost of the scan are exactly as before.
• Dropped the duplicate-provider WARN. It fires on a stock install: the default camel-core feature installs both camel-xml-io and camel-xml-jaxb and both ship META-INF/services/org/apache/camel/modelxml-dumper, with different implementation classes. And it could not see the OsgiFactoryFinder: first-match-wins bundle scan is order-dependent and silently sticky #733 rolling-upgrade sequence anyway, since addToClassMap short-circuits on classMap and never calls getResource again for a resolved key.
• Kept and centred the robustness fix your getEntry loop exposed: skip Bundle.UNINSTALLED, and catch IllegalStateException from getEntry. That one is a real bug on its own — a feature:uninstall racing a lookup poisons the key for the life of the context via classesNotFoundExceptions — and it is now what the PR is about.
• DEBUG instead of WARN for which bundle supplied the descriptor. That is the "observable" half of OsgiFactoryFinder: first-match-wins bundle scan is order-dependent and silently sticky #733 without the noise.
• Tests extended to 6 cases, including the two uninstalled-bundle races. Also fixed the missing newline at EOF and the stray blank lines.

I am leaving #733 open: the sticky-cache half is untouched here and needs invalidation on bundle events, along the lines of what OsgiTypeConverter does with its delegate. Happy to look at that separately if you want to take it.

[oscerd]

Looks good to me