Handle non-existing backup file (#5590)

* Make the API return 404 for non-existing backup files

* Introduce BackupFileNotFoundError exception

* Return 404 on full restore as well

* Fix remaining API tests

* Improve error handling in delete

* Fix pytest

* Fix tests and change error handling to agreed logic

---------

Co-authored-by: Mike Degatano <michael.degatano@gmail.com>

supervisor/api/backups.py

@@ -457,7 +457,7 @@ class APIBackups(CoreSysAttributes):
         else:
             self._validate_cloud_backup_location(request, backup.location)
 
-        return self.sys_backups.remove(backup, locations=locations)
+        self.sys_backups.remove(backup, locations=locations)
 
     @api_process
     async def download(self, request: web.Request):

supervisor/api/utils.py

@@ -22,7 +22,7 @@ from ..const import (
     RESULT_OK,
 )
 from ..coresys import CoreSys
-from ..exceptions import APIError, APIForbidden, DockerAPIError, HassioError
+from ..exceptions import APIError, BackupFileNotFoundError, DockerAPIError, HassioError
 from ..utils import check_exception_chain, get_message_from_exception_chain
 from ..utils.json import json_dumps, json_loads as json_loads_util
 from ..utils.log_format import format_message
@@ -62,8 +62,12 @@ def api_process(method):
         """Return API information."""
         try:
             answer = await method(api, *args, **kwargs)
-        except (APIError, APIForbidden, HassioError) as err:
+        except BackupFileNotFoundError as err:
-            return api_return_error(error=err)
+            return api_return_error(err, status=404)
+        except APIError as err:
+            return api_return_error(err, status=err.status, job_id=err.job_id)
+        except HassioError as err:
+            return api_return_error(err)
 
         if isinstance(answer, (dict, list)):
             return api_return_ok(data=answer)
@@ -102,6 +106,13 @@ def api_process_raw(content, *, error_type=None):
             """Return api information."""
             try:
                 msg_data = await method(api, *args, **kwargs)
+            except APIError as err:
+                return api_return_error(
+                    err,
+                    error_type=error_type or const.CONTENT_TYPE_BINARY,
+                    status=err.status,
+                    job_id=err.job_id,
+                )
             except HassioError as err:
                 return api_return_error(
                     err, error_type=error_type or const.CONTENT_TYPE_BINARY
@@ -121,6 +132,8 @@ def api_return_error(
     error: Exception | None = None,
     message: str | None = None,
     error_type: str | None = None,
+    status: int = 400,
+    job_id: str | None = None,
 ) -> web.Response:
     """Return an API error message."""
     if error and not message:
@@ -130,10 +143,6 @@ def api_return_error(
     if not message:
         message = "Unknown error, see supervisor"
 
-    status = 400
-    if is_api_error := isinstance(error, APIError):
-        status = error.status
-
     match error_type:
         case const.CONTENT_TYPE_TEXT:
             return web.Response(body=message, content_type=error_type, status=status)
@@ -146,8 +155,8 @@ def api_return_error(
                 JSON_RESULT: RESULT_ERROR,
                 JSON_MESSAGE: message,
             }
-            if is_api_error and error.job_id:
+            if job_id:
-                result[JSON_JOB_ID] = error.job_id
+                result[JSON_JOB_ID] = job_id
 
     return web.json_response(
         result,

supervisor/backups/backup.py

@@ -52,7 +52,12 @@ from ..const import (
     CRYPTO_AES128,
 )
 from ..coresys import CoreSys
-from ..exceptions import AddonsError, BackupError, BackupInvalidError
+from ..exceptions import (
+    AddonsError,
+    BackupError,
+    BackupFileNotFoundError,
+    BackupInvalidError,
+)
 from ..jobs.const import JOB_GROUP_BACKUP
 from ..jobs.decorator import Job
 from ..jobs.job_group import JobGroup
@@ -513,7 +518,7 @@ class Backup(JobGroup):
             else self.all_locations[location][ATTR_PATH]
         )
         if not backup_tarfile.is_file():
-            raise BackupError(
+            raise BackupFileNotFoundError(
                 f"Cannot open backup at {backup_tarfile.as_posix()}, file does not exist!",
                 _LOGGER.error,
             )

supervisor/backups/manager.py

@@ -22,6 +22,7 @@ from ..dbus.const import UnitActiveState
 from ..exceptions import (
     BackupDataDiskBadMessageError,
     BackupError,
+    BackupFileNotFoundError,
     BackupInvalidError,
     BackupJobError,
     BackupMountDownError,
@@ -279,7 +280,7 @@ class BackupManager(FileConfiguration, JobGroup):
         self,
         backup: Backup,
         locations: list[LOCATION_TYPE] | None = None,
-    ) -> bool:
+    ):
         """Remove a backup."""
         targets = (
             [
@@ -292,24 +293,28 @@ class BackupManager(FileConfiguration, JobGroup):
             else list(backup.all_locations.keys())
         )
         for location in targets:
+            backup_tarfile = backup.all_locations[location][ATTR_PATH]
             try:
-                backup.all_locations[location][ATTR_PATH].unlink()
+                backup_tarfile.unlink()
                 del backup.all_locations[location]
+            except FileNotFoundError as err:
+                raise BackupFileNotFoundError(
+                    f"Cannot delete backup at {backup_tarfile.as_posix()}, file does not exist!",
+                    _LOGGER.error,
+                ) from err
             except OSError as err:
+                msg = f"Could delete backup at {backup_tarfile.as_posix()}: {err!s}"
                 if err.errno == errno.EBADMSG and location in {
                     None,
                     LOCATION_CLOUD_BACKUP,
                 }:
                     self.sys_resolution.unhealthy = UnhealthyReason.OSERROR_BAD_MESSAGE
-                _LOGGER.error("Can't remove backup %s: %s", backup.slug, err)
+                raise BackupError(msg, _LOGGER.error) from err
-                return False
 
         # If backup has been removed from all locations, remove it from cache
         if not backup.all_locations:
             del self._backups[backup.slug]
 
-        return True
-
     async def _copy_to_additional_locations(
         self,
         backup: Backup,

supervisor/exceptions.py

@@ -659,6 +659,10 @@ class BackupJobError(BackupError, JobException):
     """Raise on Backup job error."""
 
 
+class BackupFileNotFoundError(BackupError):
+    """Raise if the backup file hasn't been found."""
+
+
 # Security
 
 

supervisor/misc/tasks.py

@@ -9,7 +9,12 @@ from ..addons.const import ADDON_UPDATE_CONDITIONS
 from ..backups.const import LOCATION_CLOUD_BACKUP
 from ..const import AddonState
 from ..coresys import CoreSysAttributes
-from ..exceptions import AddonsError, HomeAssistantError, ObserverError
+from ..exceptions import (
+    AddonsError,
+    BackupFileNotFoundError,
+    HomeAssistantError,
+    ObserverError,
+)
 from ..homeassistant.const import LANDINGPAGE
 from ..jobs.decorator import Job, JobCondition, JobExecutionLimit
 from ..plugins.const import PLUGIN_UPDATE_CONDITIONS
@@ -364,4 +369,7 @@ class Tasks(CoreSysAttributes):
             and datetime.fromisoformat(backup.date) < utcnow() - OLD_BACKUP_THRESHOLD
         ]
         for backup in old_backups:
-            self.sys_backups.remove(backup, [LOCATION_CLOUD_BACKUP])
+            try:
+                self.sys_backups.remove(backup, [LOCATION_CLOUD_BACKUP])
+            except BackupFileNotFoundError as err:
+                _LOGGER.debug("Can't remove backup %s: %s", backup.slug, err)

supervisor/resolution/fixups/system_clear_full_backup.py

@@ -2,6 +2,8 @@
 
 import logging
 
+from supervisor.exceptions import BackupFileNotFoundError
+
 from ...backups.const import BackupType
 from ...coresys import CoreSys
 from ..const import MINIMUM_FULL_BACKUPS, ContextType, IssueType, SuggestionType
@@ -31,7 +33,10 @@ class FixupSystemClearFullBackup(FixupBase):
         for backup in sorted(full_backups, key=lambda x: x.date)[
             : -1 * MINIMUM_FULL_BACKUPS
         ]:
-            self.sys_backups.remove(backup)
+            try:
+                self.sys_backups.remove(backup)
+            except BackupFileNotFoundError as err:
+                _LOGGER.debug("Can't remove backup %s: %s", backup.slug, err)
 
     @property
     def suggestion(self) -> SuggestionType:

tests/api/test_backups.py

@@ -503,6 +503,13 @@ async def test_restore_immediate_errors(
     assert resp.status == 400
     assert "No Home Assistant" in (await resp.json())["message"]
 
+    resp = await api_client.post(
+        f"/backups/{mock_partial_backup.slug}/restore/partial",
+        json={"background": True, "folders": ["ssl"]},
+    )
+    assert resp.status == 404
+    assert "file does not exist" in (await resp.json())["message"]
+
 
 @pytest.mark.parametrize(
     ("folder", "location"), [("backup", None), ("core/backup", ".cloud_backup")]
@@ -808,6 +815,28 @@ async def test_remove_backup_from_location(api_client: TestClient, coresys: Core
     assert backup.all_locations == {None: {"path": location_1, "protected": False}}
 
 
+@pytest.mark.usefixtures("tmp_supervisor_data")
+async def test_remove_backup_file_not_found(api_client: TestClient, coresys: CoreSys):
+    """Test removing a backup from one location of multiple."""
+    backup_file = get_fixture_path("backup_example.tar")
+    location = Path(copy(backup_file, coresys.config.path_backup))
+
+    await coresys.backups.reload()
+    assert (backup := coresys.backups.get("7fed74c8"))
+    assert backup.all_locations == {
+        None: {"path": location, "protected": False},
+    }
+
+    location.unlink()
+    resp = await api_client.delete("/backups/7fed74c8")
+    assert resp.status == 404
+    body = await resp.json()
+    assert (
+        body["message"]
+        == f"Cannot delete backup at {str(location)}, file does not exist!"
+    )
+
+
 @pytest.mark.parametrize("local_location", ["", ".local"])
 async def test_download_backup_from_location(
     api_client: TestClient,
@@ -909,7 +938,7 @@ async def test_restore_backup_from_location(
         f"/backups/{backup.slug}/restore/partial",
         json={"location": local_location, "folders": ["share"]},
     )
-    assert resp.status == 400
+    assert resp.status == 404
     body = await resp.json()
     assert (
         body["message"]

tests/backups/test_manager.py

@@ -25,6 +25,7 @@ from supervisor.docker.homeassistant import DockerHomeAssistant
 from supervisor.docker.monitor import DockerContainerStateEvent
 from supervisor.exceptions import (
     BackupError,
+    BackupFileNotFoundError,
     BackupInvalidError,
     BackupJobError,
     BackupMountDownError,
@@ -1769,11 +1770,13 @@ async def test_backup_remove_error(
     tar_file_mock.unlink.side_effect = (err := OSError())
 
     err.errno = errno.EBUSY
-    assert coresys.backups.remove(backup) is False
+    with pytest.raises(BackupError):
+        coresys.backups.remove(backup)
     assert coresys.core.healthy is True
 
     err.errno = errno.EBADMSG
-    assert coresys.backups.remove(backup) is False
+    with pytest.raises(BackupError):
+        coresys.backups.remove(backup)
     assert coresys.core.healthy is healthy_expected
 
 
@@ -2072,3 +2075,25 @@ async def test_addon_backup_excludes(coresys: CoreSys, install_addon_example: Ad
     assert not test2.exists()
     assert test_dir.is_dir()
     assert test3.exists()
+
+
+@pytest.mark.usefixtures("tmp_supervisor_data", "path_extern")
+async def test_remove_non_existing_backup_raises(
+    coresys: CoreSys,
+):
+    """Test removing a backup error."""
+    location: LOCATION_TYPE = None
+    backup_base_path = coresys.backups._get_base_path(location)  # pylint: disable=protected-access
+    backup_base_path.mkdir(exist_ok=True)
+    copy(get_fixture_path("backup_example.tar"), backup_base_path)
+
+    await coresys.backups.reload(location=location, filename="backup_example.tar")
+    assert (backup := coresys.backups.get("7fed74c8"))
+
+    assert None in backup.all_locations
+    backup.all_locations[None]["path"] = (tar_file_mock := MagicMock())
+    tar_file_mock.unlink.side_effect = (err := FileNotFoundError())
+    err.errno = errno.ENOENT
+
+    with pytest.raises(BackupFileNotFoundError):
+        coresys.backups.remove(backup)