Retry scanner webhook deliveries instead of a fixed grace period

docs/topics/development/scanner_pipeline.md

@@ -108,13 +108,55 @@ field:
 The `results` field should contain the same data structure as a synchronous
 response would return.
 
+(skipping-an-event)=
 #### Skipping an event
 
 Scanners can use the `204 No Content` HTTP status code to indicate that they
 intentionally skipped the event (e.g., the event is not relevant for this
 scanner). No results will be stored for the scanner result associated with this
 event.
 
+(scanner-delivery-retries)=
+### Delivery retries
+
+Some events block auto-approval: a version submitted while a scanner is
+subscribed to `during_validation` or `on_version_created` is not auto-approved
+until that scanner is done with it. A [scanner result](#scanner-results) counts
+as done when the scanner [skipped the event](#skipping-an-event) or sent its
+`matchedRules`, [synchronously](#synchronous-response) or
+[asynchronously](#asynchronous-scanning).
+
+A scanner can fail to send its results back, e.g., because of an outage. Rather
+than waiting forever, AMO delivers the webhook again, with a delay that doubles
+after each attempt:
+
+| Delivery | Sent                                    |
+| -------- | --------------------------------------- |
+| 1        | when the event occurs                   |
+| 2        | 1 hour after the previous delivery      |
+| 3        | 2 hours after the previous delivery     |
+| 4        | 4 hours after the previous delivery     |
+| 5        | 8 hours after the previous delivery     |
+
+That is `WEBHOOK_MAX_DELIVERY_ATTEMPTS` deliveries over 15 hours, the initial
+call included. The first delay is configurable in seconds through the
+`scanner-webhook-retry-initial-delay` config key. Retries are scheduled by the
+`auto_approve` command (except in `--dry-run` mode) and performed by the
+`retry_webhook_deliveries_on_version` task.
+
+A retry sends the same payload as the initial call, to the same
+`scanner_result_url`, so a scanner that eventually answers a retry does not need
+to do anything differently. Every delivery increments `delivery_attempts` on the
+scanner result, which is visible in the admin; the counter is incremented before
+the call, so failed calls count too.
+
+AMO gives up on a scanner for a given version when the attempts are exhausted,
+when the payload can no longer be rebuilt (e.g., the uploaded file a
+`during_validation` payload points to is gone), or when no scanner result was
+ever created for the event. The version is then flagged for human review with
+the `WAITING_ON_SCANNERS` reason, so it leaves auto-approval and reaches the
+reviewer queue instead of remaining stuck.
+
 (scanner-annotations)=
 ### Annotations
 

src/olympia/constants/config_keys.py

@@ -60,6 +60,11 @@ def dump(self, value):
 
 REVIEWERS_MOTD = ConfigKey('reviewers_review_motd')
 
+# Delay in seconds before the first webhook retry, doubled at each attempt.
+SCANNER_WEBHOOK_RETRY_INITIAL_DELAY = IntConfigKey(
+    'scanner-webhook-retry-initial-delay', 60 * 60
+)
+
 SITE_NOTICE = ConfigKey('site_notice')
 
 SUBMIT_NOTIFICATION_WARNING = ConfigKey('submit_notification_warning')

src/olympia/constants/reviewers.py

@@ -80,5 +80,3 @@ class HELD_DECISION_CHOICES(StrEnumChoices):
 
 HELD_DECISION_CHOICES.add_subset('ADDON', ('YES', 'CANCEL'))
 HELD_DECISION_CHOICES.add_subset('OTHER', ('YES', 'NO'))
-
-WAIT_ON_SCANNERS_TIMEOUT = 7200  # seconds (2 hours)

src/olympia/constants/scanners.py

@@ -94,6 +94,9 @@
     WEBHOOK_ON_VERSION_CREATED,
 ]
 
+# Max number of webhook deliveries for a single scanner result.
+WEBHOOK_MAX_DELIVERY_ATTEMPTS = 5
+
 # Special rule name used as fallback when a scanner has no better rule to
 # associate with an annotation.
 ANNOTATIONS_RULE_NAME = 'ANNOTATIONS'

src/olympia/lib/settings_base.py

@@ -831,6 +831,7 @@ def get_language_url_map():
     'olympia.devhub.tasks.validate_upload': {'queue': 'devhub'},
     'olympia.files.tasks.repack_fileupload': {'queue': 'devhub'},
     'olympia.scanners.tasks.call_webhooks_during_validation': {'queue': 'devhub'},
+    'olympia.scanners.tasks.retry_webhook_deliveries_on_version': {'queue': 'devhub'},
     'olympia.scanners.tasks.run_narc_on_version': {'queue': 'devhub'},
     'olympia.scanners.tasks.run_yara': {'queue': 'devhub'},
     'olympia.versions.tasks.call_webhooks_on_source_code_uploaded': {'queue': 'devhub'},

src/olympia/reviewers/management/commands/auto_approve.py

@@ -1,5 +1,4 @@
 from collections import Counter
-from datetime import datetime, timedelta
 
 from django.conf import settings
 from django.core.management.base import BaseCommand
@@ -15,7 +14,6 @@
 from olympia.abuse.tasks import report_decision_to_cinder_and_notify
 from olympia.amo.decorators import use_primary_db
 from olympia.constants.abuse import DECISION_ACTIONS
-from olympia.constants.reviewers import WAIT_ON_SCANNERS_TIMEOUT
 from olympia.files.utils import lock
 from olympia.lib.crypto.signing import SigningError
 from olympia.reviewers.models import (
@@ -27,7 +25,10 @@
 )
 from olympia.reviewers.utils import ReviewHelper
 from olympia.scanners.models import ScannerResult
-from olympia.scanners.tasks import run_narc_on_version
+from olympia.scanners.tasks import (
+    retry_webhook_deliveries_on_version,
+    run_narc_on_version,
+)
 from olympia.versions.models import Version
 
 
@@ -142,13 +143,20 @@ def process(self, version_id):
                     summary is not None
                     and summary.scanner_actions_executed is not False
                 )
+                is_waiting_on_scanners = (
+                    AutoApprovalSummary.check_is_waiting_on_scanners(version)
+                )
+                if is_waiting_on_scanners and not self.dry_run:
+                    # In case a scanner never sent its results back.
+                    retry_webhook_deliveries_on_version.delay(version.pk)
+
                 if already_executed:
                     log.info(
                         'Not running run_actions() on version %s because it '
                         'has already been executed',
                         version.pk,
                     )
-                elif AutoApprovalSummary.check_is_waiting_on_scanners(version):
+                elif is_waiting_on_scanners:
                     log.info(
                         'Not running run_actions() on version %s because it '
                         'is still waiting on scanners',
@@ -281,7 +289,6 @@ def disapprove(self, version):
             'has_auto_approval_disabled': (
                 NeedsHumanReview.REASONS.AUTO_APPROVAL_DISABLED
             ),
-            'is_waiting_on_scanners': NeedsHumanReview.REASONS.WAITING_ON_SCANNERS,
         }
         # For the specific reasons that cause an add-on to be added to the
         # (human) review queue, we add the corresponding NeedsHumanReview flag
@@ -296,18 +303,6 @@ def disapprove(self, version):
             has_decision_waiting_for_2nd_level_approval = (
                 version.contentdecision_set.awaiting_action().exists()
             )
-            # AutoApprovalSummary is created on the very first `auto_approve`
-            # run. When we're still waiting on scanners after a long time, it
-            # might mean a scanner has had an issue and will likely never send
-            # its results. That's why we should NHR the version.
-            should_still_wait_on_scanners = (
-                version.autoapprovalsummary.is_waiting_on_scanners
-                and datetime.now()
-                <= (
-                    version.autoapprovalsummary.created
-                    + timedelta(seconds=WAIT_ON_SCANNERS_TIMEOUT)
-                )
-            )
             already_has_same_active_nhr = version.needshumanreview_set.filter(
                 reason=reason, is_active=True
             ).exists()
@@ -316,7 +311,6 @@ def disapprove(self, version):
                 and not version.pending_rejection
                 and not has_decision_waiting_for_2nd_level_approval
                 and not already_has_same_active_nhr
-                and not should_still_wait_on_scanners
             ):
                 NeedsHumanReview.objects.create(version=version, reason=reason)
 

src/olympia/reviewers/tests/test_commands.py

@@ -615,23 +615,14 @@ def test_disapproves_has_auto_approval_disabled(self):
         assert nhr.reason == NeedsHumanReview.REASONS.AUTO_APPROVAL_DISABLED
         assert nhr.is_active
 
-    def test_disapproves_is_waiting_on_scanners_after_grace_period(self):
-        summary = AutoApprovalSummary(is_waiting_on_scanners=True)
-        summary.created = datetime.now() - timedelta(hours=3)
-        self.version.autoapprovalsummary = summary
-        command = auto_approve.Command()
-        command.disapprove(self.version)
-        nhr = self.version.needshumanreview_set.get()
-        assert nhr.reason == NeedsHumanReview.REASONS.WAITING_ON_SCANNERS
-        assert nhr.is_active
-
-    def test_disapproves_is_waiting_on_scanners_within_grace_period(self):
-        summary = AutoApprovalSummary(is_waiting_on_scanners=True)
-        summary.created = datetime.now() - timedelta(hours=1)
-        self.version.autoapprovalsummary = summary
+    def test_does_not_disapprove_is_waiting_on_scanners(self):
+        # We only flag the version once we have given up, in the scanners task.
+        self.version.autoapprovalsummary = AutoApprovalSummary(
+            is_waiting_on_scanners=True
+        )
         command = auto_approve.Command()
         command.disapprove(self.version)
-        assert not self.version.needshumanreview_set.filter(is_active=True).exists()
+        assert not self.version.needshumanreview_set.exists()
 
     def test_disapproves_is_promoted_but_decision_waiting_for_2nd_level_exists(self):
         self.version.autoapprovalsummary = AutoApprovalSummary(
@@ -1095,6 +1086,40 @@ def test_only_executes_run_actions_once_after_waiting_on_scanners(
 
         assert not run_actions_mock.called
 
+    @mock.patch(
+        'olympia.reviewers.management.commands.auto_approve.'
+        'retry_webhook_deliveries_on_version'
+    )
+    def test_retries_webhook_deliveries(self, retry_mock):
+        # Keep the version out of auto-approval so that it remains a candidate.
+        AddonReviewerFlags.objects.create(addon=self.addon, auto_approval_disabled=True)
+        scanner_result = self.create_pending_webhook_scanner_result()
+
+        call_command('auto_approve')
+
+        retry_mock.delay.assert_called_with(self.version.pk)
+
+        # Nothing to retry once the scanner has sent its results.
+        retry_mock.reset_mock()
+        scanner_result.update(results={'matchedRules': []})
+
+        call_command('auto_approve')
+
+        assert not retry_mock.delay.called
+
+    @mock.patch(
+        'olympia.reviewers.management.commands.auto_approve.'
+        'retry_webhook_deliveries_on_version'
+    )
+    def test_does_not_retry_webhook_deliveries_in_dry_run(self, retry_mock):
+        # Keep the version out of auto-approval so that it remains a candidate.
+        AddonReviewerFlags.objects.create(addon=self.addon, auto_approval_disabled=True)
+        self.create_pending_webhook_scanner_result()
+
+        call_command('auto_approve', '--dry-run')
+
+        assert not retry_mock.delay.called
+
     @mock.patch.object(ScannerResult, 'run_actions')
     def test_langpack_does_not_wait_on_scanners(self, run_actions_mock):
         self.addon.update(type=amo.ADDON_LPAPP)

src/olympia/scanners/admin.py

@@ -605,6 +605,7 @@ class ScannerResultAdmin(AbstractScannerResultAdminMixin, AMOModelAdmin):
         'guid',
         'formatted_scanner',
         'created',
+        'delivery_attempts',
         formatted_matched_rules_with_files_and_data,
         'formatted_results',
         'activity_log',

src/olympia/scanners/migrations/0090_scannerresult_delivery_attempts.py

@@ -0,0 +1,22 @@
+# Generated by Django 5.2.16 on 2026-09-02 09:55
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ('scanners', '0089_alter_scannerwebhook_name'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='scannerresult',
+            name='delivery_attempts',
+            field=models.PositiveSmallIntegerField(
+                default=0,
+                help_text=(
+                    'Number of times the webhook has been called for this result'
+                ),
+            ),
+        ),
+    ]

src/olympia/scanners/models.py

@@ -440,6 +440,10 @@ class ScannerResult(AbstractScannerResult):
         'ScannerRule', through='ScannerMatch', related_name='results'
     )
     has_matches = models.BooleanField(null=True)
+    delivery_attempts = models.PositiveSmallIntegerField(
+        default=0,
+        help_text='Number of times the webhook has been called for this result',
+    )
 
     class Meta(AbstractScannerResult.Meta):
         db_table = 'scanners_results'
@@ -461,6 +465,12 @@ def rule_model(self):
     def webhook(self):
         return self.webhook_event.webhook if self.webhook_event else None
 
+    @property
+    def is_complete(self):
+        """Whether the scanner is done with this result: `results` is None
+        (event skipped) or has `matchedRules`."""
+        return self.results is None or 'matchedRules' in self.results
+
     def get_rules_queryset(self):
         # See: https://github.com/mozilla/addons-server/issues/13143
         return super().get_rules_queryset().filter(is_active=True)