[ENG-9176] optimize verify; run verify before archiving only in dry mode (#11563)

Author: antkryt

admin/nodes/views.py

@@ -845,11 +845,12 @@ def get(self, request, *args, **kwargs):
             messages.success(request, f"Registration {registration._id} is archived.")
             return redirect(self.get_success_url())
 
-        addons = set(registration.registered_from.get_addon_names())
-        addons.update(DEFAULT_PERMISSIBLE_ADDONS)
+        addons = set(DEFAULT_PERMISSIBLE_ADDONS)
+        for reg in registration.node_and_primary_descendants():
+            addons.update(reg.registered_from.get_addon_names())
 
         try:
-            archive_status = check(registration, permissible_addons=addons)
+            archive_status = check(registration, permissible_addons=addons, verify_addons=False)
             messages.success(request, archive_status)
         except RegistrationStuckError as exc:
             messages.error(request, str(exc))
@@ -885,17 +886,21 @@ def post(self, request, *args, **kwargs):
         for reg in registration.node_and_primary_descendants():
             addons.update(reg.registered_from.get_addon_names())
 
-        try:
-            verify(registration, permissible_addons=addons, raise_error=True)
-        except ValidationError as exc:
-            messages.error(request, str(exc))
-            return redirect(self.get_success_url())
+        # No need to verify addons during force archive,
+        # because we fetched all permissible addons above
+        verify_addons = False
 
-        dry_mode = force_archive_params.get('dry_mode', False)
-
-        if dry_mode:
-            messages.success(request, f"Registration {registration._id} can be archived.")
+        if force_archive_params.get('dry_mode', False):
+            # For dry mode, verify synchronously to provide immediate feedback
+            try:
+                verify(registration, permissible_addons=addons, verify_addons=verify_addons, raise_error=True)
+                messages.success(request, f"Registration {registration._id} can be archived.")
+            except ValidationError as exc:
+                messages.error(request, str(exc))
+                return redirect(self.get_success_url())
         else:
+            # For actual archiving, skip synchronous verification to avoid 502 timeouts
+            # Verification will be performed asynchronously in the task
             force_archive_task = force_archive.delay(
                 str(registration._id),
                 permissible_addons=list(addons),

osf/management/commands/force_archive.py

@@ -37,7 +37,7 @@
 from framework import sentry
 from framework.exceptions import HTTPError
 from osf import features
-from osf.models import Node, NodeLog, Registration, BaseFileNode
+from osf.models import Node, NodeLog, Registration, BaseFileNode, ArchiveJob
 from osf.models.files import TrashedFileNode
 from osf.utils.requests import get_current_request
 from osf.exceptions import RegistrationStuckRecoverableException, RegistrationStuckBrokenException
@@ -433,18 +433,29 @@ def archive_registrations(*args, **kwargs):
         ARCHIVED.append(reg)
         VERIFIED.remove(reg)
 
-def verify(registration, permissible_addons=DEFAULT_PERMISSIBLE_ADDONS, raise_error=False):
+def verify(registration, permissible_addons=DEFAULT_PERMISSIBLE_ADDONS, verify_addons=True, raise_error=False):
     permissible_addons = set(permissible_addons)
     maybe_suppress_error = contextlib.suppress(ValidationError) if not raise_error else contextlib.nullcontext(enter_result=False)
 
-    for reg in registration.node_and_primary_descendants():
+    # Collect all registrations and prefetch archive_jobs to avoid N+1 queries
+    registrations = list(registration.node_and_primary_descendants())
+    registration_ids = [reg.id for reg in registrations]
+
+    archive_jobs_dict = {
+        job.dst_node_id: job
+        for job in ArchiveJob.objects.filter(dst_node_id__in=registration_ids).only('dst_node_id', 'status')
+    }
+
+    for reg in registrations:
         logger.info(f'Verifying {reg._id}')
-        if reg.archive_job.status == ARCHIVER_SUCCESS:
+        archive_job = archive_jobs_dict.get(reg.id) or getattr(reg, 'archive_job', None)
+        if archive_job and archive_job.status == ARCHIVER_SUCCESS:
             continue
+
         nonignorable_logs = get_logs_to_revert(reg)
         unacceptable_logs = nonignorable_logs.exclude(action__in=LOG_GREYLIST)
         if unacceptable_logs.exists():
-            if len(permissible_addons) == 1 or unacceptable_logs.exclude(action__in=PERMISSIBLE_BLACKLIST):
+            if len(permissible_addons) == 1 or unacceptable_logs.exclude(action__in=PERMISSIBLE_BLACKLIST).exists():
                 message = '{}: Original node {} has unacceptable logs: {}'.format(
                     registration._id,
                     reg.registered_from._id,
@@ -456,30 +467,23 @@ def verify(registration, permissible_addons=DEFAULT_PERMISSIBLE_ADDONS, raise_er
                     raise ValidationError(message)
 
                 return False
-        if nonignorable_logs.filter(action__in=VERIFY_PROVIDER).exists():
-            for log in nonignorable_logs.filter(action__in=VERIFY_PROVIDER):
-                for key in ['source', 'destination']:
-                    if key in log.params:
-                        if log.params[key]['provider'] != 'osfstorage':
-                            message = '{}: {} Only OSFS moves and renames are permissible'.format(
-                                registration._id,
-                                log._id
-                            )
-                            logger.error(message)
-
-                            with maybe_suppress_error:
-                                raise ValidationError(message)
-
-                            return False
-        addons = reg.registered_from.get_addon_names()
-        if set(addons) - set(permissible_addons | {'wiki'}) != set():
-            message = f'{registration._id}: Original node {reg.registered_from._id} has addons: {addons}'
-            logger.error(message)
-
-            with maybe_suppress_error:
-                raise ValidationError(message)
-
-            return False
+
+        verify_provider_logs = nonignorable_logs.filter(action__in=VERIFY_PROVIDER).only('params', '_id')
+        for log in verify_provider_logs:
+            for key in ['source', 'destination']:
+                if key in log.params:
+                    if log.params[key].get('provider') != 'osfstorage':
+                        message = '{}: {} Only OSFS moves and renames are permissible'.format(
+                            registration._id,
+                            log._id
+                        )
+                        logger.error(message)
+
+                        with maybe_suppress_error:
+                            raise ValidationError(message)
+
+                        return False
+
         if nonignorable_logs.exists():
             logger.info('{}: Original node {} has had revertable file operations'.format(
                 registration._id,
@@ -490,6 +494,18 @@ def verify(registration, permissible_addons=DEFAULT_PERMISSIBLE_ADDONS, raise_er
                 registration._id,
                 reg.registered_from._id
             ))
+
+        if verify_addons:
+            addons = reg.registered_from.get_addon_names()
+            if set(addons) - set(permissible_addons | {'wiki'}) != set():
+                message = f'{registration._id}: Original node {reg.registered_from._id} has addons: {addons}'
+                logger.error(message)
+
+                with maybe_suppress_error:
+                    raise ValidationError(message)
+
+                return False
+
     return True
 
 def verify_registrations(registration_ids, permissible_addons):

website/archiver/tasks.py

@@ -375,7 +375,7 @@ def archive_success(self, dst_pk, job_pk):
 
 @celery_app.task(bind=True)
 def force_archive(self, registration_id, permissible_addons, allow_unconfigured=False, skip_collisions=False, delete_collisions=False):
-    from osf.management.commands.force_archive import archive
+    from osf.management.commands.force_archive import archive, verify
 
     create_app_context()
 
@@ -384,6 +384,8 @@ def force_archive(self, registration_id, permissible_addons, allow_unconfigured=
         if not registration or not isinstance(registration, Registration):
             return f'Registration {registration_id} not found'
 
+        verify(registration, permissible_addons=set(permissible_addons), raise_error=True)
+
         archive(
             registration,
             permissible_addons=set(permissible_addons),