fix(api): scavenger: properly account for touched RRsets, fixes #499

api/desecapi/management/commands/scavenge-unused.py

@@ -4,7 +4,8 @@ from django.conf import settings
 from django.core.mail import get_connection, mail_admins
 from django.core.management import BaseCommand
 from django.db import transaction
-from django.db.models import F, Q
+from django.db.models import F, Max, OuterRef, Subquery
+from django.db.models.functions import Greatest
 from django.test import RequestFactory
 from django.utils import timezone
 from rest_framework.reverse import reverse
@@ -20,15 +21,20 @@ notice_days_warn = 7
 
 class Command(BaseCommand):
     base_queryset = models.Domain.objects.exclude(renewal_state=models.Domain.RenewalState.IMMORTAL)
+    _rrsets_outer_queryset = models.RRset.objects.filter(domain=OuterRef('pk')).values('domain')  # values() is GROUP BY
+    _max_touched = Subquery(_rrsets_outer_queryset.annotate(max_touched=Max('touched')).values('max_touched'))
 
     @classmethod
     def renew_touched_domains(cls):
-        last_published_threshold = timezone.localdate() - datetime.timedelta(days=183)
-        recently_touched_domains = cls.base_queryset.filter(published__date__gte=last_published_threshold,
-                                                            renewal_changed__lt=F('published'))
+        recently_active_domains = cls.base_queryset.annotate(
+            last_active=Greatest(cls._max_touched, 'published')
+        ).filter(
+            last_active__date__gte=timezone.localdate() - datetime.timedelta(days=183),
+            renewal_changed__lt=F('last_active'),
+        )
 
-        print('Renewing domains:', *recently_touched_domains.values_list('name', flat=True))
-        recently_touched_domains.update(renewal_state=models.Domain.RenewalState.FRESH, renewal_changed=F('published'))
+        print('Renewing domains:', *recently_active_domains.values_list('name', flat=True))
+        recently_active_domains.update(renewal_state=models.Domain.RenewalState.FRESH, renewal_changed=F('last_active'))
 
     @classmethod
     def warn_domain_deletion(cls, renewal_state, notice_days, inactive_days):
@@ -65,19 +71,21 @@ class Command(BaseCommand):
 
     @classmethod
     def delete_domains(cls, inactive_days):
-        published_threshold = timezone.localdate() - datetime.timedelta(days=inactive_days)
-        renewal_changed_threshold = timezone.localdate() - datetime.timedelta(days=notice_days_warn)
-        expiry_candidates = cls.base_queryset.filter(renewal_state=models.Domain.RenewalState.WARNED)
-        domains = expiry_candidates.filter(renewal_changed__date__lte=renewal_changed_threshold,
-                                           published__date__lte=published_threshold)
-        for domain in domains:
+        expired_domains = cls.base_queryset.filter(renewal_state=models.Domain.RenewalState.WARNED).annotate(
+            last_active=Greatest(cls._max_touched, 'published')
+        ).filter(
+            renewal_changed__date__lte=timezone.localdate() - datetime.timedelta(days=notice_days_warn),
+            last_active__date__lte=timezone.localdate() - datetime.timedelta(days=inactive_days),
+        )
+
+        for domain in expired_domains:
             with PDNSChangeTracker():
                 domain.delete()
             if not domain.owner.domains.exists():
                 domain.owner.delete()
         # Do one large delegation update
         with PDNSChangeTracker():
-            for domain in domains:
+            for domain in expired_domains:
                 views.DomainViewSet.auto_delegate(domain)
 
     def handle(self, *args, **kwargs):

api/desecapi/migrations/0015_rrset_touched_index.py

@@ -0,0 +1,18 @@
+# Generated by Django 3.1.6 on 2021-02-14 18:04
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('desecapi', '0014_replication'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='rrset',
+            name='touched',
+            field=models.DateTimeField(auto_now=True, db_index=True),
+        ),
+    ]

api/desecapi/models.py

@@ -504,7 +504,7 @@ class RRsetManager(Manager):
 class RRset(ExportModelOperationsMixin('RRset'), models.Model):
     id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False)
     created = models.DateTimeField(auto_now_add=True)
-    touched = models.DateTimeField(auto_now=True)
+    touched = models.DateTimeField(auto_now=True, db_index=True)
     domain = models.ForeignKey(Domain, on_delete=models.CASCADE)
     subname = models.CharField(
         max_length=178,

api/desecapi/tests/test_user_management.py

@@ -946,6 +946,7 @@ class RenewTestCase(UserManagementTestCase, DomainOwnerTestCase):
         for days in [5, 182, 184]:
             domain.published = timezone.now() - timedelta(days=1)
             domain.renewal_changed = timezone.now() - timedelta(days=days)
+            domain.rrset_set.update(touched=domain.renewal_changed)
             for renewal_state in [Domain.RenewalState.FRESH, Domain.RenewalState.NOTIFIED, Domain.RenewalState.WARNED]:
                 domain.renewal_state = renewal_state
                 domain.save()
@@ -957,6 +958,24 @@ class RenewTestCase(UserManagementTestCase, DomainOwnerTestCase):
                 self.assertEqual(Domain.objects.get(pk=domain.pk).published, domain.published)
                 self.assertEqual(len(mail.outbox), 0)
 
+    def test_renew_domain_recently_touched(self):
+        domain = self.my_domains[0]
+        last_active = timezone.now() - timedelta(days=1)
+        for days in [5, 182, 184]:
+            domain.published = timezone.now() - timedelta(days=days)
+            domain.renewal_changed = domain.published
+            domain.rrset_set.update(touched=last_active)
+            for renewal_state in [Domain.RenewalState.FRESH, Domain.RenewalState.NOTIFIED, Domain.RenewalState.WARNED]:
+                domain.renewal_state = renewal_state
+                domain.save()
+
+                self.assertEqual(Domain.objects.get(pk=domain.pk).renewal_state, domain.renewal_state)
+                call_command('scavenge-unused')
+                self.assertEqual(Domain.objects.get(pk=domain.pk).renewal_state, Domain.RenewalState.FRESH)
+                self.assertEqual(Domain.objects.get(pk=domain.pk).renewal_changed, last_active)
+                self.assertEqual(Domain.objects.get(pk=domain.pk).published, domain.published)
+                self.assertEqual(len(mail.outbox), 0)
+
     def test_renew_domain_fresh_182_days(self):
         domain = self.my_domains[0]
         domain.published = timezone.now() - timedelta(days=182)
@@ -975,6 +994,7 @@ class RenewTestCase(UserManagementTestCase, DomainOwnerTestCase):
         domain.renewal_changed = domain.published
         domain.renewal_state = Domain.RenewalState.FRESH
         domain.save()
+        domain.rrset_set.update(touched=domain.published)
 
         self.assertEqual(Domain.objects.get(pk=domain.pk).renewal_state, Domain.RenewalState.FRESH)
         call_command('scavenge-unused')
@@ -1004,6 +1024,7 @@ class RenewTestCase(UserManagementTestCase, DomainOwnerTestCase):
         domain.renewal_state = Domain.RenewalState.NOTIFIED
         domain.renewal_changed = timezone.now() - timedelta(days=21)
         domain.save()
+        domain.rrset_set.update(touched=domain.published)
 
         call_command('scavenge-unused')
         self.assertEqual(Domain.objects.get(pk=domain.pk).renewal_state, Domain.RenewalState.WARNED)
@@ -1035,6 +1056,7 @@ class RenewTestCase(UserManagementTestCase, DomainOwnerTestCase):
             domain.renewal_state = Domain.RenewalState.WARNED
             domain.renewal_changed = timezone.now() - timedelta(days=7)
             domain.save()
+            domain.rrset_set.update(touched=domain.published)
 
             with self.assertPdnsRequests(self.requests_desec_domain_deletion(domain=domain)):
                  call_command('scavenge-unused')
@@ -1047,5 +1069,20 @@ class RenewTestCase(UserManagementTestCase, DomainOwnerTestCase):
             for domain in domains:
                 self.assertLess(Domain.objects.get(pk=domain.pk).renewal_state, Domain.RenewalState.NOTIFIED)
 
+
 class RenewDynTestCase(RenewTestCase):
     DYN = True
+
+
+class RenewNoRRsetTestCase(RenewTestCase):
+
+    def setUp(self):
+        super().setUp()
+        self.my_domains[0].rrset_set.all().delete()
+
+    def test_renew_domain_recently_touched(self):
+        pass
+
+
+class RenewDynNoRRsetTestCase(RenewNoRRsetTestCase):
+    DYN = True