fix(backups): do not infinitely retry automatic backup if it errors out

selfprivacy_api/backup/__init__.py

@@ -1,7 +1,8 @@
 """
 This module contains the controller class for backups.
 """
-from datetime import datetime, timedelta
+from datetime import datetime, timedelta, timezone
+import time
 import os
 from os import statvfs
 from typing import Callable, List, Optional
@@ -37,6 +38,7 @@ from selfprivacy_api.backup.providers import get_provider
 from selfprivacy_api.backup.storage import Storage
 from selfprivacy_api.backup.jobs import (
     get_backup_job,
+    get_backup_fail,
     add_backup_job,
     get_restore_job,
     add_restore_job,
@@ -292,9 +294,9 @@ class Backups:
     def back_up(
         service: Service, reason: BackupReason = BackupReason.EXPLICIT
     ) -> Snapshot:
-        """The top-level function to back up a service"""
-        folders = service.get_folders()
-        service_name = service.get_id()
+        """The top-level function to back up a service
+        If it fails for any reason at all, it should both mark job as
+        errored and re-raise an error"""
 
         job = get_backup_job(service)
         if job is None:
@@ -302,6 +304,10 @@ class Backups:
         Jobs.update(job, status=JobStatus.RUNNING)
 
         try:
+            if service.can_be_backed_up() is False:
+                raise ValueError("cannot backup a non-backuppable service")
+            folders = service.get_folders()
+            service_name = service.get_id()
             service.pre_backup()
             snapshot = Backups.provider().backupper.start_backup(
                 folders,
@@ -692,23 +698,43 @@ class Backups:
         """Get a timezone-aware time of the last backup of a service"""
         return Storage.get_last_backup_time(service.get_id())
 
+    @staticmethod
+    def get_last_backup_error_time(service: Service) -> Optional[datetime]:
+        """Get a timezone-aware time of the last backup of a service"""
+        job = get_backup_fail(service)
+        if job is not None:
+            datetime_created = job.created_at
+            if datetime_created.tzinfo is None:
+                # assume it is in localtime
+                offset = timedelta(seconds=time.localtime().tm_gmtoff)
+                datetime_created = datetime_created - offset
+                return datetime.combine(datetime_created.date(), datetime_created.time(),timezone.utc)
+            return datetime_created
+        return None
+
     @staticmethod
     def is_time_to_backup_service(service: Service, time: datetime):
         """Returns True if it is time to back up a service"""
         period = Backups.autobackup_period_minutes()
-        service_id = service.get_id()
         if not service.can_be_backed_up():
             return False
         if period is None:
             return False
 
-        last_backup = Storage.get_last_backup_time(service_id)
+        last_error = Backups.get_last_backup_error_time(service)
+
+        if last_error is not None:
+            if time < last_error + timedelta(seconds=AUTOBACKUP_JOB_EXPIRATION_SECONDS):
+                return False
+
+        last_backup = Backups.get_last_backed_up(service)
         if last_backup is None:
             # queue a backup immediately if there are no previous backups
             return True
 
         if time > last_backup + timedelta(minutes=period):
             return True
+
         return False
 
     # Helpers

selfprivacy_api/backup/jobs.py

@@ -80,9 +80,19 @@ def get_job_by_type(type_id: str) -> Optional[Job]:
             return job
 
 
+def get_failed_job_by_type(type_id: str) -> Optional[Job]:
+    for job in Jobs.get_jobs():
+        if job.type_id == type_id and job.status == JobStatus.ERROR:
+            return job
+
+
 def get_backup_job(service: Service) -> Optional[Job]:
     return get_job_by_type(backup_job_type(service))
 
 
+def get_backup_fail(service: Service) -> Optional[Job]:
+    return get_failed_job_by_type(backup_job_type(service))
+
+
 def get_restore_job(service: Service) -> Optional[Job]:
     return get_job_by_type(restore_job_type(service))

tests/test_graphql/test_backup.py

@@ -14,6 +14,8 @@ import secrets
 
 import tempfile
 
+from selfprivacy_api.utils.huey import huey
+
 import selfprivacy_api.services as services
 from selfprivacy_api.services import Service, get_all_services
 from selfprivacy_api.services.service import ServiceStatus
@@ -119,6 +121,10 @@ def dummy_service(tmpdir, backups, raw_dummy_service) -> Service:
     # register our service
     services.services.append(service)
 
+    # make sure we are in immediate mode because this thing is non pickleable to store on queue.
+    huey.immediate = True
+    assert huey.immediate is True
+
     assert get_service_by_id(service.get_id()) is not None
     yield service
 
@@ -996,6 +1002,32 @@ def test_autobackup_timing(backups, dummy_service):
     assert Backups.is_time_to_backup_service(dummy_service, future)
 
 
+def test_backup_unbackuppable(backups, dummy_service):
+    dummy_service.set_backuppable(False)
+    assert dummy_service.can_be_backed_up() is False
+    with pytest.raises(ValueError):
+        Backups.back_up(dummy_service)
+
+
+def test_failed_autoback_prevents_more_autobackup(backups, dummy_service):
+    backup_period = 13  # minutes
+    now = datetime.now(timezone.utc)
+
+    Backups.set_autobackup_period_minutes(backup_period)
+    assert Backups.is_time_to_backup_service(dummy_service, now)
+
+    # artificially making an errored out backup job
+    dummy_service.set_backuppable(False)
+    with pytest.raises(ValueError):
+        Backups.back_up(dummy_service)
+    dummy_service.set_backuppable(True)
+
+    assert Backups.get_last_backed_up(dummy_service) is None
+    assert Backups.get_last_backup_error_time(dummy_service) is not None
+
+    assert Backups.is_time_to_backup_service(dummy_service, now) is False
+
+
 # Storage
 def test_snapshots_caching(backups, dummy_service):
     Backups.back_up(dummy_service)