diff --git a/backend/app/services/doctor_service.py b/backend/app/services/doctor_service.py index 56b6c84c..0a8a6c4b 100644 --- a/backend/app/services/doctor_service.py +++ b/backend/app/services/doctor_service.py @@ -590,15 +590,27 @@ class DoctorService: key = f'backup_unverified.{policy.id}' title = f'Backup verification: {cls._policy_label(policy)}' level = run.effective_verify_level() - if level == 'none': + if level != 'none': + return _check(key, title, 'ok', f'Latest backup verified ({level}).') + + # The repair verifies the REMOTE copy (size + checksum against the + # provider). With no remote copy there is nothing for it to compare, so + # offering the button would be offering a guaranteed failure — the + # exact thing this check family got wrong. Say what would fix it + # instead: an offsite copy, or a restore drill. + if not getattr(run, 'remote_key', None): return _check( key, title, 'warn', - 'The latest backup has not been verified (no integrity check or ' - 'restore drill has run against it yet).', - repairable=True, - repair_ref={'kind': 'backup_verify', 'policy_id': policy.id, - 'run_id': run.id}) - return _check(key, title, 'ok', f'Latest backup verified ({level}).') + 'The latest backup has not been verified, and it has no offsite ' + 'copy to verify against. Send this policy to remote storage, or ' + 'run a restore drill to prove the backup restores.') + return _check( + key, title, 'warn', + 'The latest backup has not been verified (no integrity check or ' + 'restore drill has run against it yet).', + repairable=True, + repair_ref={'kind': 'backup_verify', 'policy_id': policy.id, + 'run_id': run.id}) @classmethod def _setup_checks(cls): @@ -700,11 +712,88 @@ class DoctorService: from app.services import doctor_check_registry results.append({'item': item, **doctor_check_registry.repair( item.get('namespace'), item.get('ref'))}) + elif kind == 'backup_drill': + results.append({'item': item, + **cls._repair_backup_drill(item.get('policy_id'))}) + elif kind == 'backup_verify': + results.append({'item': item, **cls._repair_backup_verify( + item.get('policy_id'), item.get('run_id'))}) else: results.append({'item': item, 'success': False, 'error': f'Unknown repair kind: {kind}'}) return results + @staticmethod + def _load_policy(policy_id): + """Resolve a backup policy for a repair, or ``None``. + + The id arrives from the client echoing back a repair_ref, so it is + looked up rather than trusted — an id for a policy that has since been + deleted must read as "gone", not raise. + """ + try: + from app.models.backup_policy import BackupPolicy + return BackupPolicy.query.filter_by(id=policy_id).first() + except Exception: # noqa: BLE001 + return None + + @classmethod + def _repair_backup_drill(cls, policy_id): + """Run a fresh restore drill for a policy. + + Asynchronous: request_drill enqueues a ``backup.drill.run`` job and + returns it, so the answer carries the job id for the console to link + to rather than pretending the drill is already done. + """ + policy = cls._load_policy(policy_id) + if policy is None: + return {'success': False, + 'error': f'Backup policy {policy_id} no longer exists.'} + from app.services.backup_drill_service import ( + BackupDrillError, BackupDrillService, + ) + try: + job = BackupDrillService.request_drill(policy, trigger='doctor') + except BackupDrillError as e: + # "already drilling" / "no successful backup to drill" are ordinary + # refusals, not faults — surface the reason as-is. + return {'success': False, 'error': str(e)} + except Exception as e: # noqa: BLE001 + return {'success': False, 'error': str(e)} + return {'success': True, 'job_id': getattr(job, 'id', None), + 'detail': 'Restore drill queued.'} + + @classmethod + def _repair_backup_verify(cls, policy_id, run_id): + """Verify a backup run's remote copy. + + Unlike the drill this is synchronous — verify_run does the size and + checksum comparison inline and returns the verdict, so there is no job + id to hand back. + """ + policy = cls._load_policy(policy_id) + if policy is None: + return {'success': False, + 'error': f'Backup policy {policy_id} no longer exists.'} + from app.services.backup_policy_service import ( + BackupPolicyError, BackupPolicyService, + ) + try: + result = BackupPolicyService.verify_run(policy, run_id) + except BackupPolicyError as e: + return {'success': False, 'error': str(e)} + except Exception as e: # noqa: BLE001 + return {'success': False, 'error': str(e)} + if not result.get('verified'): + # The check ran and the copy did NOT match — a real answer, and a + # worse one than "unverified". Do not report it as a fix. + return {'success': False, + 'error': 'Verification ran but the remote copy did not ' + 'match — this backup may not restore.', + 'detail': result.get('detail')} + return {'success': True, 'verified': True, + 'detail': 'Remote copy verified.'} + @classmethod def _restart_service(cls, name): if name not in CORE_SERVICES: diff --git a/backend/tests/test_doctor_backup_repairs.py b/backend/tests/test_doctor_backup_repairs.py new file mode 100644 index 00000000..a64c69dc --- /dev/null +++ b/backend/tests/test_doctor_backup_repairs.py @@ -0,0 +1,215 @@ +"""Plan 72 A.1 — the two doctor repair kinds that could never work. + +``backup_drill_stale.*`` and ``backup_unverified.*`` both emitted +``repairable: True``, but ``DoctorService.repair()`` only knew drift, service, +dns and extension. Every Repair button those checks rendered came back +"Unknown repair kind: backup_drill" — a button that failed 100% of the time. + +The dispatcher now handles both. They are deliberately asymmetric: a drill is +enqueued as a job (so the answer carries a job id), while verification runs +inline and returns a verdict. +""" +from unittest.mock import patch + +import pytest + +from app.services.doctor_service import DoctorService + + +class _Job: + id = 4242 + + +@pytest.fixture +def policy(app): + """A backup policy the repair refs can point at.""" + from app import db + from app.models.backup_policy import BackupPolicy + + row = BackupPolicy(target_type='app', target_id=1) + db.session.add(row) + db.session.commit() + return row + + +# --------------------------------------------------------------------------- # +# The regression itself +# --------------------------------------------------------------------------- # +def test_both_kinds_are_known_to_the_dispatcher(app, policy): + """The whole bug: these two came back 'Unknown repair kind'.""" + with patch('app.services.backup_drill_service.BackupDrillService.request_drill', + return_value=_Job()), \ + patch('app.services.backup_policy_service.BackupPolicyService.verify_run', + return_value={'success': True, 'verified': True}): + results = DoctorService.repair([ + {'kind': 'backup_drill', 'policy_id': policy.id}, + {'kind': 'backup_verify', 'policy_id': policy.id, 'run_id': 7}, + ]) + + assert [r['success'] for r in results] == [True, True] + for r in results: + assert 'Unknown repair kind' not in str(r.get('error') or '') + + +def test_unknown_kind_still_errors_cleanly(app): + results = DoctorService.repair([{'kind': 'nonsense'}]) + + assert results[0]['success'] is False + assert 'Unknown repair kind: nonsense' in results[0]['error'] + + +def test_each_result_echoes_its_input_item(app, policy): + """DoctorPanel pairs answers back to checks by the echoed item.""" + with patch('app.services.backup_drill_service.BackupDrillService.request_drill', + return_value=_Job()): + item = {'kind': 'backup_drill', 'policy_id': policy.id} + results = DoctorService.repair([item]) + + assert results[0]['item'] == item + + +# --------------------------------------------------------------------------- # +# backup_drill — asynchronous, surfaces a job id +# --------------------------------------------------------------------------- # +class TestBackupDrillRepair: + + def test_enqueues_a_drill_and_returns_the_job_id(self, app, policy): + with patch('app.services.backup_drill_service.BackupDrillService.request_drill', + return_value=_Job()) as request: + result = DoctorService.repair( + [{'kind': 'backup_drill', 'policy_id': policy.id}])[0] + + assert result['success'] is True + assert result['job_id'] == 4242 + assert request.call_args.args[0].id == policy.id + + def test_the_drill_is_attributed_to_the_doctor(self, app, policy): + """The drill history shows where a drill came from; a doctor-initiated + one should not masquerade as someone pressing the manual button.""" + with patch('app.services.backup_drill_service.BackupDrillService.request_drill', + return_value=_Job()) as request: + DoctorService.repair([{'kind': 'backup_drill', 'policy_id': policy.id}]) + + assert request.call_args.kwargs['trigger'] == 'doctor' + + def test_a_refusal_is_reported_not_raised(self, app, policy): + """'already drilling' / 'no successful backup' are ordinary answers.""" + from app.services.backup_drill_service import BackupDrillError + + with patch('app.services.backup_drill_service.BackupDrillService.request_drill', + side_effect=BackupDrillError('A restore drill is already in progress.')): + result = DoctorService.repair( + [{'kind': 'backup_drill', 'policy_id': policy.id}])[0] + + assert result['success'] is False + assert 'already in progress' in result['error'] + + def test_an_unexpected_failure_does_not_escape(self, app, policy): + with patch('app.services.backup_drill_service.BackupDrillService.request_drill', + side_effect=RuntimeError('queue exploded')): + result = DoctorService.repair( + [{'kind': 'backup_drill', 'policy_id': policy.id}])[0] + + assert result['success'] is False + assert 'queue exploded' in result['error'] + + def test_a_deleted_policy_reads_as_gone(self, app): + """policy_id comes back from the client echoing a repair_ref; a stale + one must not raise.""" + result = DoctorService.repair( + [{'kind': 'backup_drill', 'policy_id': 999999}])[0] + + assert result['success'] is False + assert 'no longer exists' in result['error'] + + +# --------------------------------------------------------------------------- # +# backup_verify — synchronous, returns a verdict +# --------------------------------------------------------------------------- # +class TestBackupVerifyRepair: + + def _verify(self, policy, **patch_kwargs): + with patch('app.services.backup_policy_service.BackupPolicyService.verify_run', + **patch_kwargs) as verify: + result = DoctorService.repair([{'kind': 'backup_verify', + 'policy_id': policy.id, + 'run_id': 7}])[0] + return result, verify + + def test_a_verified_copy_succeeds(self, app, policy): + result, verify = self._verify( + policy, return_value={'success': True, 'verified': True}) + + assert result['success'] is True + assert result['verified'] is True + assert verify.call_args.args[1] == 7 + + def test_no_job_id_because_it_is_synchronous(self, app, policy): + """Unlike the drill, nothing is queued — claiming a job id would send + the console looking for a job that does not exist.""" + result, _ = self._verify( + policy, return_value={'success': True, 'verified': True}) + + assert 'job_id' not in result + + def test_a_mismatching_copy_is_a_failure_not_a_fix(self, app, policy): + """Verification running and finding a BAD copy is a worse outcome than + 'unverified' — reporting it as a successful repair would be a lie.""" + result, _ = self._verify( + policy, return_value={'success': True, 'verified': False, + 'detail': {'size': 'mismatch'}}) + + assert result['success'] is False + assert 'did not match' in result['error'] + + def test_a_policy_error_is_reported_not_raised(self, app, policy): + from app.services.backup_policy_service import BackupPolicyError + + result, _ = self._verify( + policy, side_effect=BackupPolicyError('Backup not found')) + + assert result['success'] is False + assert 'Backup not found' in result['error'] + + def test_a_deleted_policy_reads_as_gone(self, app): + result = DoctorService.repair( + [{'kind': 'backup_verify', 'policy_id': 999999, 'run_id': 7}])[0] + + assert result['success'] is False + assert 'no longer exists' in result['error'] + + +# --------------------------------------------------------------------------- # +# The other half: don't render a button that cannot work +# --------------------------------------------------------------------------- # +class TestVerifyButtonIsOnlyOfferedWhenItCanWork: + """verify_run compares against the REMOTE copy and raises when there is + none. A local-only backup would therefore render a Repair button that + fails every time — the same defect A.1 exists to remove.""" + + def _check_for(self, app, policy, remote_key): + from app import db + from app.models.backup_run import BackupRun + + run = BackupRun(policy_id=policy.id, kind='full', status='success') + run.remote_key = remote_key + db.session.add(run) + db.session.commit() + return DoctorService._backup_unverified_check(policy) + + def test_local_only_backup_is_not_repairable(self, app, policy): + check = self._check_for(app, policy, None) + + assert check['status'] == 'warn' + assert check['repairable'] is False + assert check['repair_ref'] is None + # and it says what WOULD fix it + assert 'offsite' in check['detail'] or 'restore drill' in check['detail'] + + def test_backup_with_a_remote_copy_is_repairable(self, app, policy): + check = self._check_for(app, policy, 's3://bucket/key') + + assert check['status'] == 'warn' + assert check['repairable'] is True + assert check['repair_ref']['kind'] == 'backup_verify' + assert check['repair_ref']['policy_id'] == policy.id