Tolerate unloading a never-loaded config entry in EntityComponent - #176594
Tolerate unloading a never-loaded config entry in EntityComponent#176594honzup wants to merge 2 commits into
Conversation
When a forwarded platform setup fails, config_entries discards the
forwarded setup result and the owning entry stays LOADED while the
platform is untracked in EntityComponent. The next unload then raised
ValueError("Config entry was never loaded!"), which config_entries
converts into a failed unload, wedging the entry into the
non-recoverable FAILED_UNLOAD state until restart.
There is nothing to unload in this situation, so log a warning naming
the entry and platform domain and report success instead of raising.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DERobfZZh3npFRbwpdxzz2
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
There was a problem hiding this comment.
Pull request overview
Prevents failed unload states when an entity platform was never loaded.
Changes:
- Treat missing platforms as already unloaded and log a warning.
- Update unit coverage for the new behavior.
Outstanding: Restore the complete PR template and address the CLA request.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
homeassistant/helpers/entity_component.py |
Tolerates unloading absent platforms. |
tests/helpers/test_entity_component.py |
Verifies success and warning logging. |
|
|
||
| if (platform := self._platforms.pop(key, None)) is None: | ||
| raise ValueError("Config entry was never loaded!") | ||
| self.logger.warning( |
nick-tgcs
left a comment
There was a problem hiding this comment.
Traced this through EntityComponent. async_setup_entry always registers the platform in self._platforms before setup, so _platforms.pop(key) returning None in async_unload_entry means setup for this component and entry never ran, which is exactly the discarded-failure case described. In that state there is no EntityPlatform and no entities registered through this component, so async_reset() would have nothing to do and returning True is accurate rather than lenient. Downgrading to a warning avoids the ValueError being converted into the non-recoverable FAILED_UNLOAD that forces a restart, and the message names the entry, domain, and likely cause. The replacement test asserts both the True return and the warning text, a clear improvement over the previous stub. One note for maintainers: this handles the symptom, while the root cause is that _async_forward_entry_setups_locked discards per-platform setup results, so the entry reports LOADED while a platform silently failed. Is that tracked separately? Either way this change looks correct and low risk. LGTM.
Thanks for tracing it through so thoroughly. The root cause is described in #176593 (the discarded forwarded-setup results are what create the phantom platform in the first place), but it isn't currently tracked as its own issue - this PR deliberately fixes only the unload side, since the missing registration on its own is harmless: it's the unload raising on it that turns a stale internal record into a wedged entry that only a restart can recover. I'll file a separate issue for the forward-setup side so it's tracked properly. Fixing that one is more architectural - a silently failed forwarded platform arguably shouldn't leave the entry reporting LOADED at all, but whether the right behaviour is marking the entry setup_error, scheduling a platform retry, or raising a repair issue deserves its own discussion rather than being bundled in here. |
|
Filed the forward-setup root cause as #176652. |
|
@joostlek thanks for merging #176595. While tracing that one I found the same wedge a level down, which is what this PR fixes. An entry reaches that state because It's the general case behind the Tibber symptom. The same failure is reported in #166228 (tibber The change is 21 lines - the raise becomes a warning naming the entry and domain, and the method returns It's been open since 16 July without a core review. It's in |
Breaking change
Proposed change
EntityComponent.async_unload_entryraisesValueError("Config entry was never loaded!")when the entry is not inself._platforms. In practice this situation arises when a forwarded platform's setup silently failed earlier:_async_forward_entry_setups_lockedgathers per-platform setup tasks and discards their results, so an integration's entry can beLOADEDwhile one of its platforms (e.g.notify) was never registered with the correspondingEntityComponent.The next unload of that entry then hits the
ValueError, whichconfig_entriesconverts into a failed unload — wedging the whole entry into the non-recoverableFAILED_UNLOADstate. The user's only way out is a full Home Assistant restart, for a situation where there was nothing to unload in the first place.Real-world reports of exactly this chain:
Error unloading entry Tibber for notify→ValueError: Config entry was never loaded!→FAILED_UNLOAD→ restart requiredassist_satellite, sameValueError; the reporter explicitly identified this method as not handling already-/never-loaded entriessensorplatform, same failure in 2023.3This change makes
async_unload_entrylog a warning (naming the entry and domain, and pointing at a possible earlier platform setup failure) and returnTruewhen the entry was never registered — there is noEntityPlatformstate to reset, so treating it as already-unloaded is accurate and lets the entry unload/reload normally instead of requiring a restart. Theasync_setup_entryside (including its "has already been setup" guard) is unchanged.Test evidence:
pytest tests/helpers/test_entity_component.py→ 30 passed (the previoustest_unload_entry_fails_if_never_loadedis replaced bytest_unload_entry_tolerates_never_loaded, assertingTrueplus the warning in caplog; the new test fails with the oldValueErrorwhen the helper is reverted). Sanity run oftests/components/tibber/on this branch: 92 passed.Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: