Model client-state Apps* errors as APIError#6856
Conversation
Follow-up on #6739: with HassioError now logged and captured by Sentry in api_process, a handful of Apps* exceptions raised from AppManager.install/update/rebuild and AppModel._validate_availability surfaced as "unexpected" 400s with a noisy log entry and a Sentry event, even though they are all user/client-state errors (clicked install on an already-installed add-on, "no update available", local and store versions diverged, system architecture/machine/HA version incompatible, etc.). The dominant offender is SUPERVISOR-1JVV ("No update available for app core_mosquitto", ~19k events / ~12k users), but several siblings show the same shape. Map these through properly so the API returns clean, structured 400s: - Add modeled APIError subclasses in exceptions.py for the previously raw raises in apps/manager.py: AppAlreadyInstalledError, AppNotFoundError, AppNotInstalledError, AppNotInStoreError, AppNoUpdateAvailableError, AppRebuildVersionChangedError, AppRebuildImageBasedError. Each gets a stable error_key, a message_template, and an "addon" extra_field. - Add APIError to AppNotSupportedArchitectureError, AppNotSupportedMachineTypeError and AppNotSupportedHomeAssistantVersionError so they behave the same as the other Apps* APIErrors instead of being treated as unexpected. - Pass the app's display name (app.name from the add-on config) instead of the slug to extra_fields wherever an App or AppModel is available at the raise site, so users see "Mosquitto broker" rather than "core_mosquitto" in error messages. Slug is only used as a fallback when no app object exists (install of an unknown slug, update/rebuild of a slug that is not installed). - Update raise sites in apps/manager.py and apps/model.py to use the new typed exceptions and the addon= keyword. These are all runtime states users hit during normal interaction with the apps UI, not Supervisor bugs worth paging on.
There was a problem hiding this comment.
It looks like you're switching to using app name rather then slug in these errors. I get why but I think we should still return the slug for reasons below (both is fine too, just don't skip the slug imo).
Also we should be consistent across all these app errors with whichever we pick, name or slug. Exception of course for ones like AppNotFoundError where all we have is the input slug because its a nonexistent app.
|
|
||
| if slug in self.local: | ||
| raise AppsError(f"App {slug} is already installed", _LOGGER.warning) | ||
| raise AppAlreadyInstalledError(_LOGGER.warning, addon=self.local[slug].name) |
There was a problem hiding this comment.
I think we should use the slug not the name here. Or both if you prefer, but if so we should update all the other App errors to be consistent because we use slug in many of these right now (and even within this PR like for AppNotInstalledError below where we use slug but could get the name).
My reasoning is that we can look up name (and any other information about the app) from the slug later if the frontend wants. We cannot look up app info from the name if that's all we have.
There was a problem hiding this comment.
I've noticed that the Frontend prefixes errors with the App name, which leads to a weird mix of App name and slug in the frontend (see screenshot home-assistant/epics#50 (comment) as an example).
Could we add slug as metadata always and format using the name? 🤔
There was a problem hiding this comment.
Yea that sounds totally reasonable to me. Want to update all of them like that? Or just do these and then another pr for the other unrelated errors?
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Address review feedback from #6856: clients still need the slug to look up additional add-on information (the name is for display only), and we should be consistent about it across the Apps* errors touched by this PR. Every Apps* APIError raised with an App/AppModel available now carries both `addon` (display name, used by the message_template) and `slug` in extra_fields. Raise sites in apps/manager.py and apps/model.py pass both. The two errors raised before an app object exists keep slug-only extra_fields and use {slug} in their message: - AppNotFoundError (install of an unknown slug) - AppNotInstalledError (update/rebuild of a slug not in self.local) Pre-existing Apps* APIErrors outside the scope of this PR (AppUnknownError, AppConfigurationInvalidError, AppBootConfigCannot ChangeError, AppNotRunningError, AppPortConflict, AppNotSupportedWrite StdinError, AppBuild*) will be migrated in a follow-up.
Address review follow-up on #6856: the addon/slug convention was enforced only by hand-rolled __init__s, easy to drift on (forget slug, use a different key, etc.). Promote it to a base class that owns the shape of extra_fields for all App-related API errors. - Add AppAPIError(AppsError, APIError). Its __init__ takes `app: AppModel | App | AppStore | str` and uniformly populates extra_fields with `addon` (display name) and `slug`. Pass a string when no app object exists; only `slug` is set in that case. Extra per-error fields flow through **extra_fields and merge with the defaults. - Convert the new exceptions added in this PR (AppAlreadyInstalledError, AppNotFoundError, AppNotInstalledError, AppNotInStoreError, AppNoUpdateAvailableError, AppRebuildVersionChangedError) into thin subclasses that only declare error_key and message_template -- the __init__ is inherited. - Migrate the AppNotSupported* errors (architecture, machine type, HA version) and AppRebuildImageBasedError to use AppAPIError too; their bespoke per-error fields go through **extra_fields. They keep inheriting AppNotSupportedError so `except AppNotSupportedError` callers (e.g., AppModel._available) still work; MRO routes __init__ through AppAPIError. - Update raise sites in apps/manager.py and apps/model.py to pass `app=<obj-or-slug>` instead of repeating `addon=...` and `slug=...`. Pre-existing App* APIErrors outside this PR's scope (AppUnknownError, AppConfigurationInvalidError, AppBootConfigCannotChangeError, AppNotRunningError, AppPortConflict, AppNotSupportedWriteStdinError, AppBuild*) will be migrated to AppAPIError in a follow-up; the base class is in place for them.
2 participants
Proposed change
Follow-up on #6739. With
HassioErrornow logged and captured by Sentry inapi_process, a handful ofApps*exceptions raised fromAppManager.install/update/rebuildandAppModel._validate_availabilitysurfaced as "unexpected" 400s with a noisy log entry and a Sentry event, even though they are all client-state errors (user clicked install on an already-installed add-on, "no update available", local and store versions diverged, system architecture/machine/Home Assistant version incompatible, image-based app cannot be rebuilt, etc.). The dominant offender is SUPERVISOR-1JVV (No update available for app core_mosquitto, ~19k events / ~12k users on 2026.05.0); several siblings in the samewrap_apicluster show the same shape.Same treatment as #6785 did for
BackupMountDownError: map these through properly so the API returns clean, structured 400s.APIErrorsubclasses inexceptions.pyfor the previously raw raises inapps/manager.py:AppAlreadyInstalledError,AppNotFoundError,AppNotInstalledError,AppNotInStoreError,AppNoUpdateAvailableError,AppRebuildVersionChangedError,AppRebuildImageBasedError. Each gets a stableerror_key, amessage_template, and anaddonextra field.APIErrortoAppNotSupportedArchitectureError,AppNotSupportedMachineTypeErrorandAppNotSupportedHomeAssistantVersionErrorso they behave the same as the rest of theApps*APIErrors instead of being treated as unexpected.app.namefrom the config) instead of the slug inextra_fieldswhenever anApp/AppModelis available at the raise site, so users see e.g. "Mosquitto broker" rather than "core_mosquitto" in error messages. Slug remains the fallback when there is no app object (install of an unknown slug; update/rebuild of a slug that is not installed).apps/manager.pyandapps/model.pyaccordingly.These are all runtime states users hit during normal interaction with the add-ons UI, not Supervisor bugs worth paging on.
Type of change
Additional information
wrap_apiSentry cluster introduced by Log unexpected errors in api_process wrappers #6739; follows the pattern set by Return proper API errors when backup mount is down #6785Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed: