[1.6.x] Fix #769 - Correctly indicate probe error to user (#802)

Before this change, the only check was whether a test was complete,
not whether it was at all successful. After redirect to the result
page, we then presented the most recent result for each probe,
which could silently return results from the distant past.

There may still be some cases where test error handling is not optimal,
but those should be loud, which means #770 will notify us.

(cherry picked from commit 10ec9b0)

checks/probes.py

@@ -29,6 +29,7 @@
     STATUSES_HTML_CSS_TEXT_MAP,
 )
 from checks.tasks import dispatcher
+from checks.tasks.dispatcher import ProbeTaskResult
 
 if settings.INTERNET_NL_CHECK_SUPPORT_IPV6:
     from checks.tasks import ipv6
@@ -235,25 +236,19 @@ def _verdict_connection(self, total_score):
             verdict = STATUSES_HTML_CSS_TEXT_MAP[STATUS_FAIL]
         return verdict
 
-    def raw_results(self, dname, remote_addr):
+    def raw_results(self, dname, remote_addr) -> ProbeTaskResult:
         """
         Get results from the taskset.
         Start the taskset if not running or cached.
-        Return (done, results)-tuple where:
-            - done=bool
-            - results=Celery JSON output
-
         """
         return dispatcher.check_results(dname, self.taskset, remote_addr, get_results=True)
 
-    def check_results(self, dname, remote_addr):
+    def check_results(self, dname, remote_addr) -> ProbeTaskResult:
         """
         Get just the status of the taskset.
         Start the taskset if not running or cached.
-
         """
-        status, _ = dispatcher.check_results(dname, self.taskset, remote_addr)
-        return status
+        return dispatcher.check_results(dname, self.taskset, remote_addr, get_results=False)
 
     def rated_results(self, dname):
         """

checks/tasks/dispatcher.py

@@ -1,6 +1,9 @@
 # Copyright: 2022, ECP, NLnet Labs and the Internet.nl contributors
 # SPDX-License-Identifier: Apache-2.0
 import time
+from dataclasses import dataclass, field
+from typing import Optional, Any, Dict
+
 from celery import group
 from celery.result import AsyncResult
 from django.core.cache import cache
@@ -22,14 +25,24 @@ def user_limit_exceeded(req_limit_id):
     return current_usage > settings.CLIENT_RATE_LIMIT
 
 
-def check_results(url, checks_registry, remote_addr, get_results=False):
+@dataclass(frozen=True)
+class ProbeTaskResult:
+    done: bool
+    success: Optional[bool] = None
+    results: Dict[Any, Any] = field(default_factory=dict)
+
+
+def check_results(url, checks_registry, remote_addr, get_results=False) -> ProbeTaskResult:
     """
-    Check if results are ready for a task and return the status (True, False).
-    If there are results return them also.
+    Check if results are ready for a task and return the status.
+
+    If get_results is set, fills the results attribute.
+    If the task raised an exception and get_results is set, this call will
+    re-raise the exception. If get_results is not set, this is ignored except
+    that success will be set to False.
 
     If the task is not registered and the user has not passed the task limit,
     start the task.
-
     """
     url = url.lower()
     cache_id = redis_id.dom_task.id.format(url, checks_registry.name)
@@ -44,7 +57,8 @@ def check_results(url, checks_registry, remote_addr, get_results=False):
         req_limit_ttl = redis_id.req_limit.ttl
         if user_limit_exceeded(req_limit_id):
             log.debug("User limit exceeded. Too many requests from this IP. Blocked.")
-            return (False, dict(status="User limit exceeded, try again later"))
+            # TODO: This doesn't seem to have any handling - the task will just time out, no useful error
+            return ProbeTaskResult(done=False, results=dict(status="User limit exceeded, try again later"))
         # Try to aquire lock and start tasks
         elif cache.add(cache_id, False, cache_ttl):
             # Submit test
@@ -62,17 +76,19 @@ def check_results(url, checks_registry, remote_addr, get_results=False):
             task_id = cache.get(cache_id)
 
     log.debug("Trying to retrieve asyncresult from task_id: %s.", task_id)
+    results = {}
     callback = AsyncResult(task_id)
-    if callback.task_id and callback.ready():
-        results = {}
-        if get_results:
-            gets = callback.get()
-            for res in gets:
-                results[res[0]] = res[1]
-        return True, results
+    if callback.task_id and callback.ready() and get_results:
+        gets = callback.get()
+        for res in gets:
+            results[res[0]] = res[1]
     else:
         log.debug("Task is not yet ready.")
-    return False, {}
+    return ProbeTaskResult(
+        done=callback.ready(),
+        success=callback.successful() if callback.ready() else None,
+        results=results,
+    )
 
 
 def submit_task_set(url, checks_registry, req_limit_id=None, error_cb=None):

frontend/js/internetnl.probe.js

@@ -38,10 +38,10 @@ function parseStatuses(json) {
     var retry = false;
     for (var i=0; i<json.length; i++) {
         var obj = json[i];
-        if (obj.done == true) {
-            showResults(obj.name, obj);
-        }
-        else {
+        if (obj.done === true) {
+            if (obj.success === true) { showResults(obj.name, obj); }
+            else { showError(obj.name); }
+        } else {
             retry = true;
         }
     }

interface/views/shared.py

@@ -19,6 +19,8 @@
 from django.utils.translation import ugettext as _
 
 import unbound
+
+from checks.tasks.dispatcher import ProbeTaskResult
 from interface import redis_id
 from internetnl import log
 
@@ -92,18 +94,16 @@ def proberesults(request, probe, dname):
 
     """
     url = dname.lower()
-    done, _ = probe.raw_results(url, get_client_ip(request))
-    if done:
-        results = probe.rated_results(url)
+    task_result = probe.raw_results(url, get_client_ip(request))
+    if task_result.done:
+        return probe.rated_results(url)
     else:
-        results = dict(done=False)
-    return results
+        return dict(done=False)
 
 
-def probestatus(request, probe, dname):
+def probestatus(request, probe, dname) -> ProbeTaskResult:
     """
     Check if a probe has finished.
-
     """
     url = dname.lower()
     return probe.check_results(url, get_client_ip(request))
@@ -116,9 +116,14 @@ def probestatuses(request, dname, probes):
     """
     statuses = []
     for probe in probes:
-        results = dict(name=probe.name)
-        results["done"] = probestatus(request, probe, dname)
-        statuses.append(results)
+        task_result = probestatus(request, probe, dname)
+        statuses.append(
+            {
+                "name": probe.name,
+                "done": task_result.done,
+                "success": task_result.success,
+            }
+        )
     return statuses
 
 
@@ -162,8 +167,8 @@ def process(request, dname, template, probes, pageclass, pagetitle):
     # Also check if every test is done. In case of no-javascript we redirect
     # either to results or the same page.
     for probe in sorted_probes:
-        done, results = probe.raw_results(addr, get_client_ip(request))
-        if done:
+        task_result = probe.raw_results(addr, get_client_ip(request))
+        if task_result.done:
             done_count += 1
     if done_count >= len(sorted_probes):
         no_javascript_redirect = "results"