Make add-on installation resilient to a single failing add-on (upgrade resilience, #5694)

[JonathanGiles]

Note

This is an AI-driven pull request. The analysis, code changes, commit messages and the reproduction below were produced by an AI agent (GitHub Copilot CLI, Claude Opus) working under the direction of, and reviewed by, @JonathanGiles, who takes responsibility for the contribution and has signed off all commits (DCO).

Background

This addresses the resilience problems discussed in #5694 ("openHAB does not start after upgrade to 5.2.0"). That issue collects several distinct failure modes; this PR targets the mechanism where an add-on that fails to download/resolve during an upgrade cascades into a total, self-perpetuating outage (bindings missing, Unable to acquire the state change lock, and ModelRepositoryImpl NullPointerExceptions on every .items/.rules file), matching the maintainer's own suggestions in that thread:

I also think we need to look at FeatureInstaller, because as far as I can see, it never gives up on trying a failed installation, and you can't abort it. This seems to block the installation of other, working, features/bundles.

There's no reason for the [mDNS] bundle activation to wait for all the JmDNS instances to spin up.

What changes

Three small, independent fixes (one commit each):

1. [karaf] FeatureInstaller: isolate failures + bound retries

FeatureInstaller.installFeatures() installed all requested add-ons in a single transactional featuresService.installFeatures(set, …) call. If one add-on could not be resolved/downloaded, the whole batch was aborted and none of the add-ons installed. Worse, any failure set configMapCache = null unconditionally, so the periodic sync job retried the install — and a bundle refresh — every minute, forever, with no way to give up.

• On a group-install failure, the failed add-ons are now retried individually, so one broken add-on can no longer block the working ones.
• The number of automatic retries for a persistently failing set is bounded (MAX_INSTALL_ATTEMPTS), after which it logs an actionable error and stops, instead of looping forever. It retries again when the add-on config changes or the server restarts.

2. [mdns] Start JmDNS asynchronously on activation

MDNSClientImpl.activate() synchronously created a JmDNS instance per interface/address (blocking network I/O) while holding the bundle's OSGi state-change lock, which can stall other bundles' activation and feature installs. The JmDNS instances are now started on a single-threaded executor so activation returns immediately (executor is shut down on deactivate).

3. [model.core] Guard against NPE when no parser is registered yet

ModelRepositoryImpl.validateModel() called Resource.load() on a possibly-null resource returned by resourceSet.createResource() (null when no resource factory is registered yet, e.g. during startup before the parser bundle is active), throwing an NPE for every affected model file. It now returns gracefully and lets addOrRefreshModel() handle the missing parser as it already does elsewhere.

Testing

The FeatureInstaller behaviour was reproduced and diagnosed live in an isolated openHAB 5.2.0 Docker container (never against production), by faithfully modelling "12 add-ons downloaded, 1 failed during upgrade":

• 12 requested bindings pre-seeded into the local mvn cache (resolvable offline) + one known binding (zwave) left uncached, with the online repo blackholed so only the uncached add-on fails to download — exactly the transient-download-failure scenario.

Before (stock 5.2.0), a single failing add-on:

[DEBUG] FeatureInstaller - Installing 'openhab-binding-gree, openhab-binding-teslascope, openhab-binding-unifi, openhab-binding-daikin, openhab-binding-zwave'
[ERROR] FeatureInstaller - Failed installing 'openhab-binding-gree, openhab-binding-teslascope, openhab-binding-unifi, openhab-binding-daikin, openhab-binding-zwave'
        Error downloading mvn:org.openhab.addons.bundles/org.openhab.binding.zwave/5.2.0
        org.apache.karaf.features.internal.util.MultiException: Error:

→ the four perfectly-resolvable bindings were NOT installed solely because zwave could not be downloaded (transactional rollback), and the install + bundle refresh retried every 60 s indefinitely (12:57:34 → 12:58:34 → 12:59:34 …) — the amplifier that turns one download hiccup into the observed meltdown.

With this change, the group failure falls back to individual installs (the four good bindings install, only zwave is reported failed) and the retries are bounded. This was also confirmed with a full before/after run of the fixed FeatureInstaller image (results posted in the comments below).

The [karaf] fix is covered by unit tests (FeatureInstallerTest) that fail against the previous implementation and pass with the fix: one verifies a failing group install falls back to individual installs so the healthy add-ons still install, the other verifies the retries are bounded and stop.

• ✅ All three patched bundles compile against 5.2.0 and main (the touched files are identical on both), and pass spotless:check.
• The mdns and model.core fixes are supported by the stack traces and lock errors in Bindings not installed when upgrading to 5.2 (docker) #5694 and by code analysis.

This is ready for review. Feedback on the approach is welcome, and I'm happy to split it into separate PRs per bundle if preferred.

Signed-off-by: Jonathan Giles jonathan@jonathangiles.net

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/openhab-5-2-release-discussion/169691/36

[JonathanGiles]

Live before/after validation

I reproduced the meltdown from #5694 in an isolated container (5.2.0 image, real add-on install path) and then confirmed the fix resolves it. The repro forces the exact upgrade-time condition: a cache-cleared start where one requested binding cannot be downloaded (its bundle download 404s while the rest resolve fine).

Reproduction setup

• Stock openhab/openhab:5.2.0 image, isolated test env (production untouched).
• addons.cfg requests 13 bindings; 12 are present in the local mvn cache, one (zwave) is not, and the remote repo is blackholed so its bundle download fails — mirroring an upgrade where the add-on repo is briefly unreachable for one artifact.
• Cache cleared before boot so all requested features install in a single batch (the upgrade scenario).

Before the fix (stock 5.2.0)

One un-downloadable add-on aborts the entire batch (transactional installFeatures throws MultiException), so none of the good add-ons install, and the 60s sync job retries the whole failing batch forever:

WARN  Installing 'gree, teslascope, unifi, daikin, zwave'
ERROR Failed installing 'gree, teslascope, unifi, daikin, zwave'   (MultiException: Error downloading …/zwave)
… retried every 60s indefinitely: 12:57:34 → 12:58:34 → 12:59:34 → …

The four healthy bindings never come up, and the unbounded installFeatures + refreshFeatures() retry loop is what starves the rest of the framework (the symptom reported in #5694).

After the fix

Group install still tried first; on group failure it falls back to installing each add-on individually, so a single bad add-on no longer blocks the others. Retries for the genuinely-failing add-on are bounded, then it gives up cleanly instead of looping forever:

WARN  Installing add-ons '…20 features…' as a group failed. Retrying them individually so that a single failing add-on does not block the others.
      Installed 'openhab-binding-gree'
      Installed 'openhab-binding-spotify'
      Installed 'openhab-binding-network'
      Installed 'openhab-binding-sonos'
      Installed 'openhab-binding-daikin'
      Installed 'openhab-binding-shelly'
      Installed 'openhab-binding-mqtt'
      Installed 'openhab-binding-http'
      Installed 'openhab-binding-teslascope'
      Installed 'openhab-binding-astro'
      Installed 'openhab-binding-unifi'
ERROR Failed installing add-ons 'openhab-binding-zwave' (attempt 1 of 5). Will retry.
ERROR Failed installing add-ons 'openhab-binding-zwave' (attempt 2 of 5). Will retry.
ERROR Failed installing add-ons 'openhab-binding-zwave' (attempt 3 of 5). Will retry.
ERROR Failed installing add-ons 'openhab-binding-zwave' (attempt 4 of 5). Will retry.
ERROR Failed installing add-ons 'openhab-binding-zwave' after 5 attempts. Giving up until the add-on configuration changes or the server is restarted.
DEBUG Running scheduled sync job    ← sync still ticks, but no more install/refresh churn
DEBUG Running scheduled sync job    ← the retry storm is over

Result: all 12 healthy bindings install despite zwave being un-downloadable; the failing add-on is retried a bounded number of times and then abandoned without nulling the config cache, so the every-60s install + refreshFeatures() storm that amplified a single failure into a full outage no longer occurs.

Unit tests (red-before / green-after) will follow as a separate commit on this branch.

[Nadahar]

I think the ideas are good, I haven't really reviewed the implementations that much. I think it makes more sense to handle these 3 cases separately:

• mDNS initialization: I thought of using executors to do the initializations as well, however I can see no point in using a single threaded executor, and we might have a suitable thread pool for this anyway. I also think that there must be proper thread-safety implemented here, so that the registrations can take place in parallel, and because it's long over-due anyway. I could look at this.
• FeatureInstaller: This is probably the most tricky one, touching it is "high risk" - yet something needs to be done. It should be discussed exactly how we want it to behave - how many retries - how spread out, how should user-feedback look if it finally gives up. Some of the fix is a given, like not letting the failure of one feature prevent installation of the others.
• ModelRepository: Here it should be considered whether a real failure occurs, or if it's just communicated/logged a bit unfortunate. Perhaps @lolodomo could take a look at that?

[JonathanGiles]

I can create three separate PR tomorrow if you want. If you would rather do it yourself, that's fine too. I think with these fixes it will unblock this class of issue though, so it is probably generally the right thing to get in.

[Nadahar]

I don't know what the others want, but as I see it, the "biggest job" often isn't to write the code, but to figure out exactly how it can best be solved. I think the FeatureInstaller solution requires the most deliberation to figure out how to best handle it.

[JonathanGiles]

Just let me know what you want me to do (if anything) and I can revise / recreate the PRs

[lolodomo]

• ModelRepository: Here it should be considered whether a real failure occurs, or if it's just communicated/logged a bit unfortunate. Perhaps @lolodomo could take a look at that?

Looks not stupid at all.
We just have to be sure that it could not lead to DSL files being loaded without validation.
I could prepare a specific PR for that.

But I see no link with the original problem.
Did someone encounter this potential NPE?

[Nadahar]

But I see no link with the original problem.
Did someone encounter this potential NPE?

I think that was part of the puzzle in some instances. It's hard to be sure of anything here, but it doesn't hurt to make it more robust.

[Nadahar]

I have created a separate PR for the mDNS part: #5708

[lolodomo]

I just take a look to FeatureInstaller changes, it looks not bad, even if 5 retries looks really very big to me (maybe 2 or 3 could be more appropriate ?).
@Nadahar : WDYT about these changes ?
If this part if fine, we can extract it in a separate PR.
I have not looked in details to the tests.

As I understand it, it covers the case the installation of an add-on is faling + the case of an add-on cannot be downloaded (because its id is wrong for example).

[Nadahar]

@lolodomo I've tried to address #5708 first, it's a part of this, since I already had a branch with previously attempted refactoring of the mDNS stuff.

I think FeatureInstaller is very "dangerous" to touch because it will break "everything" if it stops working. I don't think the solutions there are good enough. There is one thing that I think could be "extracted", and that is to make it continue trying to install others even if one fail. That makes a lot of sense, and could help the system progress to a "more working state". But, there's no getting around that these things often aren't "optional". Sure, the system can manage when a binding doesn't install, but is that all it's used for? It might be that FeatureInstaller only handles add-ons, in which case it would be "less dangerous" to touch it, but I don't have the overview to know if that's the case.

My plan was to try to deal with just the part where one failure aborts everything, as soon as I was done with the mDNS. But, I wasn't planning on preventing it from retrying indefinitely, because I think we need another solution in place before we can do that. I've long had an idea that we should have a "registry"/table/collection of failed add-on installations, both "official" and marketplace ones, where they would end up if automatic installation failed. The user would be alerted with a "health warning" that something was there, and you then decide what to do with each add-on - whether to retry installation, or delete them from the list. Once that is in place, I think the "repeating installation" should just be removed. But, that's not something that can be done as a fix quickly.

[wborn]

Thanks for investigating this. I agree with the main FeatureInstaller direction: one unavailable add-on should not prevent unrelated add-ons from installing, and an unchanged failure should not trigger an unbounded refresh/retry loop.

However, I think the current implementation needs some changes before merging:

• The mDNS change overlaps with #5708 and introduces lifecycle races that #5708 already handles more comprehensively.
• A genuine add-on configuration change does not reset the retry budget as the error message promises.
• Karaf installation exceptions are discarded and can be represented as complete success based only on the installed-feature snapshot.
• Falling back to one complete Karaf provisioning operation per add-on can amplify framework-wide failures.
• The original failure cause is hidden at normal log levels.
• The tests do not cover these relevant Karaf and configuration lifecycle cases.

I also recommend separating the FeatureInstaller, mDNS and model-repository changes. They address independent concerns and have different testing and backporting requirements.

This review was AI-assisted, and I checked the findings against the current PR implementation and the Apache Karaf 4.4.11 provisioning code.

[wborn]

This change overlaps directly with #5708, which addresses the same bundle-activation problem as part of a dedicated mDNS thread-safety change.

Submitting the existing start() method to an executor is not sufficient to preserve the current lifecycle semantics. For example, addServiceListener() only adds a listener to the JmDNS instances that already exist. A consumer that registers after activation returns but before start() creates the instances will silently lose that registration, because listeners are not retained and replayed.

Startup can also race with onChanged() and deactivation, potentially creating duplicate instances or publishing a creation result after close() has already run.

#5708 already handles these concerns more comprehensively by retaining listeners and services, coordinating address and service operations, and rejecting or closing results that complete after deactivation.

Could the mDNS commit be removed from this PR and #5708 be referenced for this part of the problem?

[wborn]

The final error says that installation is abandoned until the add-on configuration changes or the server is restarted, but a configuration change does not reset this retry state.

For example:

1. zwave reaches the five-attempt limit.
2. The user changes the add-on configuration.
3. zwave is still among the requested add-ons and fails again.
4. The failed set still equals {zwave}, so the old counter is incremented.
5. The new configuration immediately gives up without receiving a fresh retry budget.

The same problem occurs when an add-on is removed and later re-added.

Could the retry state be associated with the processed configuration revision, or explicitly reset when a genuine configuration update is handled? Periodic retries of the unchanged configuration should retain their counter, while a user configuration change should start a new retry budget.

Please add a test that reaches the limit, processes a changed configuration and verifies that installation is attempted with a fresh retry budget.

[wborn]

Each call to Karaf’s installFeatures() performs a complete provisioning operation: it copies the current state, resolves the requested requirements, creates the download infrastructure and deploys the resulting changes.

The current fallback can therefore turn one failed group operation into one additional complete provisioning operation for every configured add-on. During a repository-wide outage, resolver failure or state-change-lock problem, a configuration containing 40 add-ons could perform one group attempt followed by 40 individual attempts, all failing for the same global reason.

That amplifies load and contention during exactly the failure scenario this change is intended to make safer.

Could the isolation use recursive batch splitting, or otherwise avoid one call per add-on when the failure appears to be framework-wide? Please add a test where every installation request fails for the same global reason and verify that the number of provisioning calls remains bounded.

[wborn]

This now hides the actual Karaf exception at normal log levels. The later error only contains the add-on names and retry count, so users cannot determine whether the cause was a missing Maven artifact, repository or TLS failure, resolver error, bundle-start failure or state-change-lock problem.

The original exception was essential to diagnosing #5694.

There is also an important semantic distinction here: Karaf can throw after it has already persisted the new feature state—for example, when starting one of the affected bundles fails. In that case, the subsequent listInstalledFeatures() call can contain every requested feature, causing this method to return an empty failure set and represent the operation as completely successful even though Karaf reported a provisioning failure.

Could the installation result retain both:

• the exception reported by Karaf; and
• the set of features that appear installed afterwards?

The installed-state snapshot can prevent unnecessary retries, but it should not silently erase the provisioning failure. At minimum, the first and final failures should retain the original cause at an actionable log level.

[wborn]

This posts an installed event for every requested feature found in Karaf’s installed-state snapshot, even when the immediately preceding installFeatures() call threw.

Karaf saves its feature state before some later deployment phases, including bundle startup. A feature can therefore appear installed while its provisioning operation still ended with a MultiException.

Could events be based on a more explicit result that distinguishes:

• installation completed successfully;
• the feature was already installed before this invocation;
• Karaf recorded the feature but subsequently reported a deployment failure?

Without that distinction, this can emit misleading or duplicate installed events after failed provisioning attempts.

A test should cover installFeatures() throwing while listInstalledFeatures() nevertheless contains the requested feature.

[wborn]

This test directly replaces configMapCache through reflection and invokes installFeatures(). It therefore verifies only the internal cache-clearing mechanism, not the documented behavior that a real configuration change restores the retry budget.

The mocked installation behavior also models only an exception that occurs before any feature becomes installed, while Karaf may persist the installed-feature state and throw during a later deployment phase.

Could the tests cover the observable lifecycle instead?

1. A failure reaches the retry limit.
2. A genuine configuration change is processed.
3. The same add-on receives a fresh retry budget.
4. installFeatures() throws after the feature has entered the installed-state snapshot.
5. The exception is not silently treated as complete success and misleading events are not emitted.
6. A framework-wide failure does not trigger one provisioning call per configured add-on.

It would also be preferable to make the retry policy directly testable rather than starting a real scheduler, shutting it down and mutating private component state through reflection.