[fix] Reconcile stuck "modified" config status on checksum request #1330

When a device applies a new configuration but fails to report its
status back (e.g. due to a transient HTTP 502 from the controller),
the config remains in "modified" state on the server forever. The
device's agent will compare its local checksum with the remote
checksum on the next polling cycle and find them identical, so it
will not re-download or re-report. Without manual intervention the
status stays "modified" indefinitely.

Detect this condition on the DeviceChecksumView: if the config has
been in "modified" state longer than a 5 minute grace period and the
device is actively polling (proven by the very checksum request we
are handling), set the status to "applied".

Implementation notes:

- Use the cached device object's config status as a fast path: if
  the cached status is not "modified" we return immediately with
  zero extra database queries, preserving the existing zero-query
  guarantee of the cached checksum path.
- A second fast path checks the cached "modified" timestamp: if the
  grace period has not yet elapsed based on the cached timestamp
  alone, we skip the DB query entirely.
- Only when both fast paths pass do we re-query Config fresh from
  the database inside a transaction with select_for_update to avoid
  race conditions with concurrent requests.
- The re-query uses .only() on the fields we need to keep it cheap.
- A 5 minute grace period avoids fighting an in-flight apply that
  has not had time to report yet.
- The entire method is wrapped in try/except to ensure reconciliation
  never fails the checksum endpoint.

Closes #1330

Author: MichaelUray

openwisp_controller/config/controller/views.py

@@ -8,6 +8,7 @@
 from django.core.exceptions import FieldDoesNotExist, ValidationError
 from django.db import transaction
 from django.db.models import Q
+from django.utils import timezone
 from django.utils.decorators import method_decorator
 from django.utils.translation import gettext_lazy as _
 from django.views.decorators.csrf import csrf_exempt
@@ -144,6 +145,13 @@ class DeviceChecksumView(UpdateLastIpMixin, GetDeviceView):
     returns device's configuration checksum
     """
 
+    # Grace period before reconciling a "modified" status.
+    # If the config has been in "modified" state for longer than this,
+    # and the device is actively requesting its checksum (proving it's
+    # online and polling), we assume a previous report_status call was
+    # lost due to a transient error and reconcile the status to "applied".
+    _STATUS_RECONCILE_GRACE_SECONDS = 300  # 5 minutes
+
     def get(self, request, pk):
         device = self.get_device()
         bad_request = forbid_unallowed(request, "GET", "key", device.key)
@@ -156,10 +164,90 @@ def get(self, request, pk):
         checksum_requested.send(
             sender=device.__class__, instance=device, request=request
         )
+        self._reconcile_modified_status(device)
         return ControllerResponse(
             device.config.get_cached_checksum(), content_type="text/plain"
         )
 
+    @staticmethod
+    def _reconcile_modified_status(device):
+        """
+        Reconciles config status for devices stuck in "modified" state.
+
+        When a device applies a new configuration but fails to report its
+        status back (e.g., due to a transient HTTP error like 502), the
+        config remains in "modified" state on the controller even though
+        the device already has the current configuration.
+
+        The device's agent will compare its local checksum with the
+        remote checksum on the next polling cycle and find they match,
+        so it won't re-download or re-report. The status stays "modified"
+        indefinitely.
+
+        This method detects this condition: if the config has been in
+        "modified" state for longer than the grace period and the device
+        is actively polling (proven by this very checksum request), we
+        set the status to "applied".
+
+        Fast path: the cached device object (from cache_memoize) is used
+        first to check the status. Only when the cached status indicates
+        "modified" do we re-query the database for the fresh state. This
+        keeps the checksum endpoint zero-query for the common case where
+        the status is already "applied".
+        """
+        # Best-effort: never let reconciliation fail the checksum endpoint.
+        try:
+            # Fast path 1: if the cached device's config is not "modified",
+            # there is nothing to reconcile.
+            try:
+                cached_config = device.config
+            except Config.DoesNotExist:
+                return
+            if cached_config is None or cached_config.status != "modified":
+                return
+
+            # Fast path 2: even if cached status is "modified", the cached
+            # `modified` timestamp gives a lower bound on the real elapsed
+            # time. If that lower bound is below the grace period, the real
+            # value cannot be above it either, so skip the DB query entirely.
+            grace = DeviceChecksumView._STATUS_RECONCILE_GRACE_SECONDS
+            cached_elapsed = (timezone.now() - cached_config.modified).total_seconds()
+            if cached_elapsed < grace:
+                return
+
+            # Slow path: re-read config fresh from the database because
+            # cache_memoize has a 30-day TTL and may be serving a stale
+            # "modified" status that was already reconciled earlier.
+            # Use select_for_update inside a transaction to avoid race
+            # conditions with concurrent requests.
+            with transaction.atomic():
+                try:
+                    config = (
+                        Config.objects.select_for_update()
+                        .only("id", "status", "modified")
+                        .get(device=device)
+                    )
+                except Config.DoesNotExist:
+                    return
+                if config.status != "modified":
+                    return
+                elapsed = (timezone.now() - config.modified).total_seconds()
+                if elapsed < grace:
+                    return
+                config.set_status_applied()
+            logger.info(
+                "Reconciled config status for device %s: was 'modified' "
+                "for %d seconds with device actively polling, "
+                "setting to 'applied'.",
+                device,
+                int(elapsed),
+            )
+        except Exception:
+            logger.exception(
+                "Failed to reconcile config status for device %s",
+                device,
+            )
+
     @cache_memoize(
         timeout=Config._CHECKSUM_CACHE_TIMEOUT, args_rewrite=get_device_args_rewrite
     )

openwisp_controller/config/tests/test_controller.py

@@ -270,6 +270,66 @@ def test_device_checksum_requested_signal_is_emitted(self):
                 request=response.wsgi_request,
             )
 
+    def test_device_checksum_reconciles_modified_status(self):
+        """
+        When a device with status "modified" requests its checksum,
+        and enough time has passed (grace period), the status should
+        be automatically reconciled to "applied".
+        """
+        from datetime import timedelta
+
+        from django.utils import timezone
+
+        d = self._create_device_config()
+        c = d.config
+        c.set_status_modified()
+        self.assertEqual(c.status, "modified")
+        url = reverse("controller:device_checksum", args=[d.pk])
+
+        # First request within grace period: status should stay "modified"
+        response = self.client.get(url, {"key": d.key})
+        self.assertEqual(response.status_code, 200)
+        c.refresh_from_db()
+        self.assertEqual(c.status, "modified")
+
+        # Simulate that grace period has elapsed by backdating the
+        # modified timestamp.
+        Config.objects.filter(pk=c.pk).update(
+            modified=timezone.now()
+            - timedelta(
+                seconds=DeviceChecksumView._STATUS_RECONCILE_GRACE_SECONDS + 300
+            )
+        )
+
+        # Second request after grace period: status should be reconciled
+        # to "applied"
+        response = self.client.get(url, {"key": d.key})
+        self.assertEqual(response.status_code, 200)
+        c.refresh_from_db()
+        self.assertEqual(c.status, "applied")
+
+    def test_device_checksum_no_reconcile_for_applied(self):
+        """Status "applied" should not be changed."""
+        d = self._create_device_config()
+        c = d.config
+        c.set_status_applied()
+        url = reverse("controller:device_checksum", args=[d.pk])
+        response = self.client.get(url, {"key": d.key})
+        self.assertEqual(response.status_code, 200)
+        c.refresh_from_db()
+        self.assertEqual(c.status, "applied")
+
+    def test_device_checksum_no_reconcile_within_grace_period(self):
+        """Status should not be reconciled if within the grace period."""
+        d = self._create_device_config()
+        c = d.config
+        c.set_status_modified()
+        url = reverse("controller:device_checksum", args=[d.pk])
+        response = self.client.get(url, {"key": d.key})
+        self.assertEqual(response.status_code, 200)
+        c.refresh_from_db()
+        self.assertEqual(c.status, "modified")
+
     def test_device_checksum_bad_uuid(self):
         d = self._create_device_config()
         pk = "{}-wrong".format(d.pk)