From b5e87abad2ceec492651ba8ad2abec9af966f7b4 Mon Sep 17 00:00:00 2001 From: Peter van Arkel Date: Thu, 28 May 2026 21:07:59 +0200 Subject: [PATCH] (feature) Require review before incomplete cleanup Require incomplete snapshots to be marked reviewed before the cleanup action can delete them, and show review state in the retention plan UI. Keep cleanup confirmation counts scoped to reviewed incomplete snapshots and add coverage for blocked, reviewed, and deletion flows. --- src/pobsync_backend/retention.py | 51 +++++++++-- .../pobsync_backend/retention_plan.html | 85 ++++++++++++------- .../tests/test_sql_retention.py | 28 ++++++ src/pobsync_backend/tests/test_views.py | 55 +++++++++++- src/pobsync_backend/views.py | 12 ++- 5 files changed, 187 insertions(+), 44 deletions(-) diff --git a/src/pobsync_backend/retention.py b/src/pobsync_backend/retention.py index bc7fd3b..bdb1c39 100644 --- a/src/pobsync_backend/retention.py +++ b/src/pobsync_backend/retention.py @@ -23,7 +23,7 @@ def run_sql_retention_plan(*, host: str, kind: str, protect_bases: bool) -> dict host_config = _enabled_host_config(host) retention = _retention_for_host(host_config) snapshots = _snapshots_for_retention(host_config=host_config, kind=kind) - incomplete_snapshots = _incomplete_snapshots_for_host(host_config) + incomplete_items = _incomplete_snapshot_items_for_host(host_config) plan = build_retention_plan( snapshots=snapshots, @@ -49,10 +49,9 @@ def run_sql_retention_plan(*, host: str, kind: str, protect_bases: bool) -> dict "keep": sorted(keep), "keep_items": [_snapshot_to_item(snapshot, reasons=reasons.get(snapshot.dirname, [])) for snapshot in keep_items], "delete": [_snapshot_to_item(snapshot, reasons=["outside retention policy"]) for snapshot in delete], - "incomplete": [ - _snapshot_to_item(snapshot, reasons=["incomplete snapshot; excluded from retention cleanup"]) - for snapshot in incomplete_snapshots - ], + "incomplete": incomplete_items, + "incomplete_reviewed_count": sum(1 for item in incomplete_items if item["reviewed"]), + "incomplete_unreviewed_count": sum(1 for item in incomplete_items if not item["reviewed"]), "reasons": reasons, } @@ -164,9 +163,15 @@ def run_incomplete_cleanup( def _do_cleanup() -> dict[str, Any]: host_config = _enabled_host_config(host) + unreviewed_count = _unreviewed_incomplete_count(host_config) + if unreviewed_count: + raise ConfigError( + f"Refusing to delete {unreviewed_count} incomplete snapshot(s) that have not been reviewed." + ) + incomplete_list = [ _snapshot_to_item(snapshot, reasons=["manual incomplete cleanup"]) - for snapshot in _incomplete_snapshots_for_host(host_config) + for snapshot in _reviewed_incomplete_snapshots_for_host(host_config) ] if max_delete == 0 and len(incomplete_list) > 0: raise ConfigError("Incomplete cleanup blocked by --max-delete=0") @@ -253,15 +258,39 @@ def _snapshots_for_retention(*, host_config: HostConfig, kind: str) -> list[Snap return [_snapshot_from_record(record) for record in records] -def _incomplete_snapshots_for_host(host_config: HostConfig) -> list[Snapshot]: +def _incomplete_snapshot_items_for_host(host_config: HostConfig) -> list[dict[str, Any]]: records = ( SnapshotRecord.objects.filter(host=host_config, kind=SnapshotRecord.Kind.INCOMPLETE) .select_related("base") .order_by("-started_at", "dirname") ) + return [ + _snapshot_record_to_item(record, reasons=["incomplete snapshot; excluded from retention cleanup"]) + for record in records + ] + + +def _reviewed_incomplete_snapshots_for_host(host_config: HostConfig) -> list[Snapshot]: + records = ( + SnapshotRecord.objects.filter( + host=host_config, + kind=SnapshotRecord.Kind.INCOMPLETE, + reviewed_at__isnull=False, + ) + .select_related("base") + .order_by("-started_at", "dirname") + ) return [_snapshot_from_record(record) for record in records] +def _unreviewed_incomplete_count(host_config: HostConfig) -> int: + return SnapshotRecord.objects.filter( + host=host_config, + kind=SnapshotRecord.Kind.INCOMPLETE, + reviewed_at__isnull=True, + ).count() + + def _snapshot_from_record(record: SnapshotRecord) -> Snapshot: return Snapshot( kind=record.kind, @@ -301,6 +330,14 @@ def _snapshot_to_item(snapshot: Snapshot, *, reasons: list[str]) -> dict[str, An } +def _snapshot_record_to_item(record: SnapshotRecord, *, reasons: list[str]) -> dict[str, Any]: + item = _snapshot_to_item(_snapshot_from_record(record), reasons=reasons) + item["reviewed"] = record.reviewed_at is not None + item["reviewed_at"] = record.reviewed_at.isoformat() if record.reviewed_at else "" + item["reviewed_by"] = record.reviewed_by + return item + + def _snapshot_delete_path(*, path: Path, dirname: str) -> Path: if path.name == "data" and path.parent.name == dirname: return path.parent diff --git a/src/pobsync_backend/templates/pobsync_backend/retention_plan.html b/src/pobsync_backend/templates/pobsync_backend/retention_plan.html index 02fd800..19beef9 100644 --- a/src/pobsync_backend/templates/pobsync_backend/retention_plan.html +++ b/src/pobsync_backend/templates/pobsync_backend/retention_plan.html @@ -45,8 +45,9 @@ snapshots automatically because they can indicate an interrupted backup that should be inspected first.

- After inspection, use the dedicated cleanup form below to delete only incomplete snapshot directories and their - tracking records. Successful scheduled and manual snapshots are not touched by this cleanup. + {{ incomplete_unreviewed_count }} still need review. After inspection, mark them reviewed and use the dedicated + cleanup form below to delete only incomplete snapshot directories and their tracking records. Successful + scheduled and manual snapshots are not touched by this cleanup.

{% endif %} @@ -187,6 +188,7 @@ Dirname Started Status + Review Reason Path @@ -197,6 +199,14 @@ {{ snapshot.dirname }} {{ snapshot.dt }} {{ snapshot.status|default:"" }} + + {% if snapshot.reviewed %} + reviewed + {{ snapshot.reviewed_by|default:"unknown" }} + {% else %} + needs review + {% endif %} + {{ snapshot.reason }} {{ snapshot.path }} @@ -205,40 +215,51 @@

Cleanup Incomplete Snapshots

-

- This deletes only incomplete snapshot directories and their tracking records. Successful manual and scheduled - snapshots are not touched. -

-
- {% csrf_token %} - {{ incomplete_cleanup_form.non_field_errors }} + {% if incomplete_unreviewed_count %} +

+ Cleanup is blocked until all incomplete snapshots are reviewed. This extra step makes it explicit that the + interrupted backup was inspected before deletion. +

+ + {% csrf_token %} + +
+ {% else %} +

+ This deletes only reviewed incomplete snapshot directories and their tracking records. Successful manual and + scheduled snapshots are not touched. +

+
+ {% csrf_token %} + {{ incomplete_cleanup_form.non_field_errors }} -
- {{ incomplete_cleanup_form.max_delete.errors }} - - {{ incomplete_cleanup_form.max_delete }} -
{{ incomplete_cleanup_form.max_delete.help_text }}
-
+
+ {{ incomplete_cleanup_form.max_delete.errors }} + + {{ incomplete_cleanup_form.max_delete }} +
{{ incomplete_cleanup_form.max_delete.help_text }}
+
-
- {{ incomplete_cleanup_form.confirm_host.errors }} - - {{ incomplete_cleanup_form.confirm_host }} -
{{ incomplete_cleanup_form.confirm_host.help_text }}
-
+
+ {{ incomplete_cleanup_form.confirm_host.errors }} + + {{ incomplete_cleanup_form.confirm_host }} +
{{ incomplete_cleanup_form.confirm_host.help_text }}
+
-
- {{ incomplete_cleanup_form.confirm_delete_count.errors }} - - {{ incomplete_cleanup_form.confirm_delete_count }} -
{{ incomplete_cleanup_form.confirm_delete_count.help_text }}
-
+
+ {{ incomplete_cleanup_form.confirm_delete_count.errors }} + + {{ incomplete_cleanup_form.confirm_delete_count }} +
{{ incomplete_cleanup_form.confirm_delete_count.help_text }}
+
-
- - Cancel -
-
+
+ + Cancel +
+ + {% endif %} {% endif %} {% endblock %} diff --git a/src/pobsync_backend/tests/test_sql_retention.py b/src/pobsync_backend/tests/test_sql_retention.py index bfb84ca..4db665f 100644 --- a/src/pobsync_backend/tests/test_sql_retention.py +++ b/src/pobsync_backend/tests/test_sql_retention.py @@ -254,6 +254,8 @@ class SqlRetentionTests(TestCase): path=str(incomplete_dir), status="failed", started_at=datetime(2026, 5, 19, 3, 15, tzinfo=timezone.utc), + reviewed_at=datetime(2026, 5, 19, 4, 15, tzinfo=timezone.utc), + reviewed_by="admin", ) result = run_incomplete_cleanup( @@ -291,6 +293,8 @@ class SqlRetentionTests(TestCase): path=str(incomplete_dir), status="failed", started_at=datetime(2026, 5, 19, 3, 15, tzinfo=timezone.utc), + reviewed_at=datetime(2026, 5, 19, 4, 15, tzinfo=timezone.utc), + reviewed_by="admin", ) result = run_incomplete_cleanup( @@ -305,6 +309,26 @@ class SqlRetentionTests(TestCase): self.assertFalse(SnapshotRecord.objects.filter(pk=record.pk).exists()) self.assertEqual(result["deleted"][0]["dirname"], incomplete_dir.name) + def test_incomplete_cleanup_requires_reviewed_snapshots(self) -> None: + host = HostConfig.objects.create(host="web-01", address="web-01.example.test") + SnapshotRecord.objects.create( + host=host, + kind=SnapshotRecord.Kind.INCOMPLETE, + dirname="20260519-031500Z__BROKEN01", + path=f"/backups/{host.host}/.incomplete/20260519-031500Z__BROKEN01", + status="failed", + started_at=datetime(2026, 5, 19, 3, 15, tzinfo=timezone.utc), + ) + + with self.assertRaisesRegex(ConfigError, "have not been reviewed"): + run_incomplete_cleanup( + prefix=Path("/tmp/pobsync-test"), + host=host.host, + yes=True, + max_delete=1, + acquire_lock=False, + ) + def test_incomplete_cleanup_respects_max_delete(self) -> None: host = HostConfig.objects.create(host="web-01", address="web-01.example.test") SnapshotRecord.objects.create( @@ -314,6 +338,8 @@ class SqlRetentionTests(TestCase): path=f"/backups/{host.host}/.incomplete/20260519-031500Z__BROKEN01", status="failed", started_at=datetime(2026, 5, 19, 3, 15, tzinfo=timezone.utc), + reviewed_at=datetime(2026, 5, 19, 4, 15, tzinfo=timezone.utc), + reviewed_by="admin", ) with self.assertRaisesRegex(ConfigError, "blocked by --max-delete=0"): @@ -334,6 +360,8 @@ class SqlRetentionTests(TestCase): path=f"/backups/{host.host}/scheduled/20260519-031500Z__BROKEN01", status="failed", started_at=datetime(2026, 5, 19, 3, 15, tzinfo=timezone.utc), + reviewed_at=datetime(2026, 5, 19, 4, 15, tzinfo=timezone.utc), + reviewed_by="admin", ) with self.assertRaisesRegex(ConfigError, "unexpected incomplete snapshot path"): diff --git a/src/pobsync_backend/tests/test_views.py b/src/pobsync_backend/tests/test_views.py index 45e5210..324cf96 100644 --- a/src/pobsync_backend/tests/test_views.py +++ b/src/pobsync_backend/tests/test_views.py @@ -2226,9 +2226,33 @@ class ViewTests(TestCase): self.assertContains(response, "Incomplete Snapshots") self.assertContains(response, "20260519-031500Z__BROKEN01") self.assertContains(response, "excluded from retention cleanup") + self.assertContains(response, "needs review") + self.assertContains(response, "Cleanup is blocked until all incomplete snapshots are reviewed.") + self.assertContains(response, "Mark incomplete snapshots reviewed") + self.assertContains(response, "delete only incomplete snapshot directories") + self.assertNotContains(response, "Delete incomplete snapshots") + + def test_retention_plan_offers_incomplete_cleanup_after_review(self) -> None: + self.client.force_login(self.staff_user) + host = HostConfig.objects.create(host="web-01", address="web-01.example.test") + SnapshotRecord.objects.create( + host=host, + kind=SnapshotRecord.Kind.INCOMPLETE, + dirname="20260519-031500Z__BROKEN01", + path=f"/backups/{host.host}/.incomplete/20260519-031500Z__BROKEN01", + status="failed", + started_at=datetime(2026, 5, 19, 3, 15, tzinfo=timezone.utc), + reviewed_at=datetime(2026, 5, 19, 4, 15, tzinfo=timezone.utc), + reviewed_by="admin", + ) + + response = self.client.get(reverse("host_retention_plan", args=[host.host])) + + self.assertEqual(response.status_code, 200) + self.assertContains(response, "reviewed") self.assertContains(response, "Delete incomplete snapshots") self.assertContains(response, "Type 1 to confirm the current number of incomplete snapshots.") - self.assertContains(response, "This deletes only incomplete snapshot directories") + self.assertContains(response, "This deletes only reviewed incomplete snapshot directories") self.assertContains(response, 'class="danger"', html=False) def test_incomplete_cleanup_deletes_incomplete_snapshot_after_confirmation(self) -> None: @@ -2246,6 +2270,8 @@ class ViewTests(TestCase): path=str(incomplete_dir), status="failed", started_at=datetime(2026, 5, 19, 3, 15, tzinfo=timezone.utc), + reviewed_at=datetime(2026, 5, 19, 4, 15, tzinfo=timezone.utc), + reviewed_by="admin", ) with override_settings(POBSYNC_HOME=str(home)): @@ -2291,6 +2317,33 @@ class ViewTests(TestCase): self.assertContains(response, "Incomplete cleanup confirmation is invalid.") self.assertEqual(SnapshotRecord.objects.filter(kind=SnapshotRecord.Kind.INCOMPLETE).count(), 1) + def test_incomplete_cleanup_rejects_unreviewed_snapshots(self) -> None: + self.client.force_login(self.staff_user) + host = HostConfig.objects.create(host="web-01", address="web-01.example.test") + SnapshotRecord.objects.create( + host=host, + kind=SnapshotRecord.Kind.INCOMPLETE, + dirname="20260519-031500Z__BROKEN01", + path=f"/backups/{host.host}/.incomplete/20260519-031500Z__BROKEN01", + status="failed", + started_at=datetime(2026, 5, 19, 3, 15, tzinfo=timezone.utc), + ) + + with TemporaryDirectory() as tmp, override_settings(POBSYNC_HOME=str(Path(tmp) / "home")): + response = self.client.post( + reverse("cleanup_host_incomplete_snapshots", args=[host.host]), + { + "max_delete": "0", + "confirm_host": host.host, + "confirm_delete_count": "0", + }, + follow=True, + ) + + self.assertRedirects(response, reverse("host_retention_plan", args=[host.host])) + self.assertContains(response, "have not been reviewed") + self.assertEqual(SnapshotRecord.objects.filter(kind=SnapshotRecord.Kind.INCOMPLETE).count(), 1) + def test_host_detail_surfaces_retention_warnings(self) -> None: self.client.force_login(self.staff_user) host = HostConfig.objects.create( diff --git a/src/pobsync_backend/views.py b/src/pobsync_backend/views.py index 9870c9e..8bee6ba 100644 --- a/src/pobsync_backend/views.py +++ b/src/pobsync_backend/views.py @@ -933,6 +933,8 @@ def host_retention_plan(request, host: str): scheduled_prune_limit = schedule.prune_max_delete if schedule and schedule.prune else None delete_count = len(plan["delete"]) incomplete_count = len(plan["incomplete"]) + incomplete_reviewed_count = int(plan.get("incomplete_reviewed_count") or 0) + incomplete_unreviewed_count = int(plan.get("incomplete_unreviewed_count") or 0) context = { "host": host_config, "kind": kind, @@ -941,6 +943,8 @@ def host_retention_plan(request, host: str): "schedule": schedule, "scheduled_prune_limit": scheduled_prune_limit, "scheduled_prune_exceeded": scheduled_prune_limit is not None and delete_count > scheduled_prune_limit, + "incomplete_reviewed_count": incomplete_reviewed_count, + "incomplete_unreviewed_count": incomplete_unreviewed_count, "apply_form": RetentionApplyForm( host_name=host_config.host, expected_delete_count=delete_count, @@ -953,10 +957,10 @@ def host_retention_plan(request, host: str): ), "incomplete_cleanup_form": IncompleteCleanupForm( host_name=host_config.host, - expected_delete_count=incomplete_count, + expected_delete_count=incomplete_reviewed_count, initial={ - "max_delete": incomplete_count, - "confirm_delete_count": incomplete_count, + "max_delete": incomplete_reviewed_count, + "confirm_delete_count": incomplete_reviewed_count, }, ), } @@ -1027,7 +1031,7 @@ def cleanup_host_incomplete_snapshots(request, host: str): messages.error(request, str(exc)) return redirect("host_retention_plan", host=host_config.host) - incomplete_count = len(plan.get("incomplete") or []) + incomplete_count = int(plan.get("incomplete_reviewed_count") or 0) form = IncompleteCleanupForm( request.POST, host_name=host_config.host,