From e61ab1f164a9246a16681ef4bef77003d5046af4 Mon Sep 17 00:00:00 2001 From: Mark Street Date: Sun, 7 Jun 2026 10:58:42 +0100 Subject: [PATCH] Reduce chance of removing a false orphan --- backend/coreapp/housekeeping.py | 42 +++++++++++---- backend/coreapp/tests/test_housekeeping.py | 63 ++++++++++++++++++++-- 2 files changed, 92 insertions(+), 13 deletions(-) diff --git a/backend/coreapp/housekeeping.py b/backend/coreapp/housekeeping.py index b29c1b241..e195bc2ab 100644 --- a/backend/coreapp/housekeeping.py +++ b/backend/coreapp/housekeeping.py @@ -1,4 +1,6 @@ import datetime +import time +from collections.abc import Callable from typing import Protocol from django.apps import apps @@ -14,6 +16,7 @@ def get_model(model_name: str) -> type[DjangoModel]: DEFAULT_DELETE_BATCH_SIZE = 1000 +ORPHAN_DELETE_CONFIRMATION_DELAY_SECONDS = 3.0 def perform_delete( @@ -36,6 +39,21 @@ def perform_delete( return deleted +def confirmed_orphan_queryset( + build_queryset: Callable[[], QuerySet[Model]], + delay_seconds: float = ORPHAN_DELETE_CONFIRMATION_DELAY_SECONDS, +) -> QuerySet[Model]: + first_pass_ids = set(build_queryset().values_list("pk", flat=True)) + if first_pass_ids and delay_seconds > 0: + time.sleep(delay_seconds) + + second_pass = build_queryset() + second_pass_ids = set( + second_pass.filter(pk__in=first_pass_ids).values_list("pk", flat=True) + ) + return second_pass.filter(pk__in=second_pass_ids) + + def remove_ownerless_scratches( cutoff_datetime: datetime.datetime, dry_run: bool = False ) -> int: @@ -65,9 +83,11 @@ def remove_orphan_contexts( Context = get_model("Context") Scratch = get_model("Scratch") - to_delete = Context.objects.annotate( - has_scratch=Exists(Scratch.objects.filter(context_fk=OuterRef("pk"))) - ).filter(has_scratch=False) + to_delete = confirmed_orphan_queryset( + lambda: Context.objects.annotate( + has_scratch=Exists(Scratch.objects.filter(context_fk=OuterRef("pk"))) + ).filter(has_scratch=False) + ) return perform_delete(to_delete, dry_run=dry_run) @@ -78,9 +98,11 @@ def remove_orphan_assemblies( Assembly = get_model("Assembly") Scratch = get_model("Scratch") - to_delete = Assembly.objects.annotate( - has_scratch=Exists(Scratch.objects.filter(target_assembly=OuterRef("pk"))) - ).filter(has_scratch=False) + to_delete = confirmed_orphan_queryset( + lambda: Assembly.objects.annotate( + has_scratch=Exists(Scratch.objects.filter(target_assembly=OuterRef("pk"))) + ).filter(has_scratch=False) + ) return perform_delete(to_delete, dry_run=dry_run) @@ -91,9 +113,11 @@ def remove_orphan_asms( Asm = get_model("Asm") Assembly = get_model("Assembly") - to_delete = Asm.objects.annotate( - has_assembly=Exists(Assembly.objects.filter(source_asm=OuterRef("pk"))) - ).filter(has_assembly=False) + to_delete = confirmed_orphan_queryset( + lambda: Asm.objects.annotate( + has_assembly=Exists(Assembly.objects.filter(source_asm=OuterRef("pk"))) + ).filter(has_assembly=False) + ) return perform_delete(to_delete, dry_run=dry_run) diff --git a/backend/coreapp/tests/test_housekeeping.py b/backend/coreapp/tests/test_housekeeping.py index 794bef533..c4f1d6fa1 100644 --- a/backend/coreapp/tests/test_housekeeping.py +++ b/backend/coreapp/tests/test_housekeeping.py @@ -1,4 +1,5 @@ import datetime +from unittest.mock import patch from django.contrib.auth.models import User from django.test import TestCase @@ -130,7 +131,8 @@ def test_removes_scratchless_anonymous_profiles_created_before_cutoff(self) -> N self.assertTrue(Profile.objects.filter(pk=new_scratchless_profile.pk).exists()) self.assertTrue(Profile.objects.filter(pk=self.foo.pk).exists()) - def test_removes_orphan_contexts(self) -> None: + @patch("coreapp.housekeeping.time.sleep", return_value=None) + def test_removes_orphan_contexts(self, _sleep: object) -> None: orphan_context = Context.get_or_create_from_text("orphan context") used_context = Context.get_or_create_from_text("used context") assert orphan_context is not None @@ -144,7 +146,25 @@ def test_removes_orphan_contexts(self) -> None: self.assertFalse(Context.objects.filter(pk=self.context.pk).exists()) self.assertTrue(Context.objects.filter(pk=used_context.pk).exists()) - def test_removes_orphan_assemblies(self) -> None: + def test_keeps_context_referenced_between_orphan_checks( + self, + ) -> None: + pending_context = Context.get_or_create_from_text("pending context") + assert pending_context is not None + + def create_referencing_scratch(_delay: float) -> None: + self.create_scratch(owner=self.foo, context=pending_context) + + with patch( + "coreapp.housekeeping.time.sleep", side_effect=create_referencing_scratch + ): + deleted = remove_orphan_contexts(self.cutoff_datetime) + + self.assertEqual(deleted, 1) + self.assertTrue(Context.objects.filter(pk=pending_context.pk).exists()) + + @patch("coreapp.housekeeping.time.sleep", return_value=None) + def test_removes_orphan_assemblies(self, _sleep: object) -> None: orphan_assembly = self.create_assembly("orphan") live_assembly = self.create_assembly("live") self.create_scratch(owner=self.foo).target_assembly = live_assembly @@ -157,7 +177,25 @@ def test_removes_orphan_assemblies(self) -> None: self.assertFalse(Assembly.objects.filter(pk=self.assembly.pk).exists()) self.assertTrue(Assembly.objects.filter(pk=live_assembly.pk).exists()) - def test_removes_orphan_asms(self) -> None: + def test_keeps_assembly_referenced_between_orphan_checks(self) -> None: + pending_assembly = self.create_assembly("pending") + + def create_referencing_scratch(_delay: float) -> None: + self.create_scratch(owner=self.foo) + Scratch.objects.filter(owner=self.foo).update( + target_assembly=pending_assembly + ) + + with patch( + "coreapp.housekeeping.time.sleep", side_effect=create_referencing_scratch + ): + deleted = remove_orphan_assemblies(self.cutoff_datetime) + + self.assertEqual(deleted, 1) + self.assertTrue(Assembly.objects.filter(pk=pending_assembly.pk).exists()) + + @patch("coreapp.housekeeping.time.sleep", return_value=None) + def test_removes_orphan_asms(self, _sleep: object) -> None: orphan_asm = Asm.objects.create(hash="orphan-asm", data="jr $ra\nnop") used_asm = Asm.objects.create(hash="used-asm", data="jr $ra\nnop") self.create_assembly("used", asm=used_asm) @@ -168,7 +206,24 @@ def test_removes_orphan_asms(self) -> None: self.assertFalse(Asm.objects.filter(pk=orphan_asm.pk).exists()) self.assertTrue(Asm.objects.filter(pk=used_asm.pk).exists()) - def test_orphan_asm_cleanup_follows_orphan_assembly_cleanup(self) -> None: + def test_keeps_asm_referenced_between_orphan_checks(self) -> None: + pending_asm = Asm.objects.create(hash="pending-asm", data="jr $ra\nnop") + + def create_referencing_assembly(_delay: float) -> None: + self.create_assembly("pending", asm=pending_asm) + + with patch( + "coreapp.housekeeping.time.sleep", side_effect=create_referencing_assembly + ): + deleted = remove_orphan_asms(self.cutoff_datetime) + + self.assertEqual(deleted, 0) + self.assertTrue(Asm.objects.filter(pk=pending_asm.pk).exists()) + + @patch("coreapp.housekeeping.time.sleep", return_value=None) + def test_orphan_asm_cleanup_follows_orphan_assembly_cleanup( + self, _sleep: object + ) -> None: orphan_assembly = self.create_assembly("orphan") source_asm = orphan_assembly.source_asm assert source_asm is not None