From 8caf7e1b24124cfcdac80d21b42b718ac9b5fe17 Mon Sep 17 00:00:00 2001 From: Houkime <> Date: Tue, 7 Nov 2023 01:00:38 +0000 Subject: [PATCH] fix(backups): do not infinitely retry automatic backup if it errors out --- selfprivacy_api/backup/__init__.py | 38 +++++++++++++++++++++++++----- selfprivacy_api/backup/jobs.py | 10 ++++++++ tests/test_graphql/test_backup.py | 32 +++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 6 deletions(-) diff --git a/selfprivacy_api/backup/__init__.py b/selfprivacy_api/backup/__init__.py index aa11f7f..a5fe066 100644 --- a/selfprivacy_api/backup/__init__.py +++ b/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 diff --git a/selfprivacy_api/backup/jobs.py b/selfprivacy_api/backup/jobs.py index ab4eaca..0aacd86 100644 --- a/selfprivacy_api/backup/jobs.py +++ b/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)) diff --git a/tests/test_graphql/test_backup.py b/tests/test_graphql/test_backup.py index 1903fba..27a2879 100644 --- a/tests/test_graphql/test_backup.py +++ b/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)