🧪 Add regression test for order race condition

This is extremely hard to actually reproduce... I'm sorry.

Basically the idea is that multiple requests can arrive at the same
time (and they will do because of how the frontend is coded!) which kicks
off a number of threads that all start with the same view on the data.

django-ordered-model then performs some UPDATE queries based on that
view of the data and finally a save call to the object itself to
set the order. That last action often corrects a lot of things,
but the UPDATE query causes mayhem.

See also django-ordered-model/django-ordered-model#184.

src/openforms/forms/tests/test_api_form_logic.py

@@ -1,4 +1,9 @@
+import functools
+import types
 from threading import Thread
+from time import sleep
+from unittest import skip
+from unittest.mock import patch
 
 from django.db import close_old_connections
 from django.urls import reverse
@@ -1096,7 +1101,24 @@ def test_cant_have_empty_component_in_mark_step_as_not_applicable(self):
         self.assertEqual("blank", response.json()["invalidParams"][0]["code"])
 
 
+def copy_func(f):
+    """From https://stackoverflow.com/a/13503277"""
+    g = types.FunctionType(
+        f.__code__,
+        f.__globals__,
+        name=f.__name__,
+        argdefs=f.__defaults__,
+        closure=f.__closure__,
+    )
+    g = functools.update_wrapper(g, f)
+    g.__kwdefaults__ = f.__kwdefaults__
+    return g
+
+
 class FormLogicTransactionTests(APITransactionTestCase):
+    @skip(
+        "This test cannot complete/pass when row-level locking is used which is the solution for the problem."
+    )
     def test_reorder_logic_rules(self):
         user = SuperUserFactory.create()
         self.client.force_authenticate(user=user)
@@ -1121,20 +1143,187 @@ def test_reorder_logic_rules(self):
         fl3 = FormLogicFactory.create(order=2, **common_kwargs)
 
         # make a couple of requests in parallel, simulating the UI firing multiple
-        # API calls shortly after each other
+        # API calls shortly after each other.
+        #
+        # django-ordered-model works by:
+        #
+        # 1. first update all the records in an UPDATE query that would be moved around
+        #    by setting the order of a particular record
+        # 2. then set the new order value of the record and save.
+        #
+        # Without transactions:
+        #
+        # There is a race condition here that multiple threads/requests can arrive at
+        # the same time before any UPDATE queries are finished. This causes them to
+        # build update queries based on record.order which is no longer up to date with
+        # the actual state in the database because of one of the updates going through.
+        #
+        # This is not guaranteed - very often it goes right by coincidence, but
+        # sometimes the python code acts on stale data.
+        #
+        # The setup here reproduces that - we have 3 calls to set the order, but we mimick
+        # the following order of database operations (all start with the same view of
+        # data!):
+        #
+        # 1. FL2 -> 0: update other records. This results in FL1: 1, FL2: 1, FL3: 2
+        # 2. FL3 -> 1: update other records. This results in FL1: 2, FL2: 2, FL3: 2
+        # 3. FL2 -> 0: set FL1.order = 0. This results in FL1: 2, FL2: 0, FL3: 2
+        # 4. FL3 -> 1: set FL3.order = 1. This results in FL1: 2, FL2: 0, FL3: 1
+        # 5. FL1 -> 2: update other records: This results in FL1: 1, FL2: 0, FL3: 0
+        # 6. FL1 -> 2: set FL1.order = 2. This results in FL1: 2, FL2: 0, FL3: 0
+        #
+        # Where the expected outcomde would be FL1: 2, FL2: 0, FL3: 1 instead.
+        # Note that 2. and 3. can be interchanged, the end result is the same.
+        #
+        # NOTE: Uncomment the print debugging to see the what happens in which order
+        # because # of the thread orchestration, as this is hard to grasp.
+
+        print(fl1, fl2, fl3)
+
+        # thread-unsafe dict to coordinate thread mock delays
+        shared_state = {}
+
+        # create a real copy of the existing implementation because we'll mock it later
+        # to add delays.
+        real_update = copy_func(FormLogic.objects.all().update)
+
+        def queryset_update(self, **kwargs):
+            """
+            Introduce delays to simulate the race conditions.
+            """
+            # figure out which form_logic we are updating for from the queryset itself
+            if list(self) == [fl2, fl3]:
+                form_logic = fl1
+            elif list(self) == [fl1]:
+                form_logic = fl2
+            elif list(self) == [fl2]:
+                form_logic = fl3
+            else:
+                raise Exception("Unexpected filter query")
+
+            # add a delay so that all threads are definitely looking at the initial
+            # ordering before any update queries are allowed
+            sleep(0.1)
+
+            # FL2 update goes first, this is 1. described above
+            if form_logic == fl2:
+                pass
+            # other updates must wait
+            elif form_logic == fl3:
+                while "FL2_UPDATE_QUERY" not in shared_state:
+                    sleep(0.1)
+            elif form_logic == fl1:
+                # FL1 update query may only run after FL3 record was saved. This is
+                # 4. and 5. above.
+                while "FL3_RECORD_SAVE" not in shared_state:
+                    sleep(0.1)
+
+                # allow some time for the FL3 record save to actually persist
+                sleep(0.1)
+
+            print(shared_state)
+
+            result = real_update(self, **kwargs)
+
+            # track the state
+            if form_logic == fl1:
+                shared_state["FL1_UPDATE_QUERY"] = True
+            elif form_logic == fl2:
+                shared_state["FL2_UPDATE_QUERY"] = True
+            elif form_logic == fl3:
+                shared_state["FL3_UPDATE_QUERY"] = True
+
+            # process wait events AFTER the update
+            if form_logic == fl1:
+                shared_state["FL1_RECORD_SAVE"] = True
+            elif form_logic == fl2:
+                # NOTE: when using select_for_update, a thread needs to complete
+                # before others can run and this while look blocks that, leading
+                # to deadlocks (which don't happen in the application!)
+                # This doesn't make any difference for our test since FL2 from step
+                # 3 onwards always ends up with order 0.
+                # # FL2 may progress once FL3 update is completed, this is 2. above.
+                # # it then continues to the instance save.
+                # while "FL3_UPDATE_QUERY" not in shared_state:
+                #     sleep(0.1)
+                shared_state["FL2_RECORD_SAVE"] = True
+            elif form_logic == fl3:
+                shared_state["FL3_RECORD_SAVE"] = True
+
+            print(shared_state)
+
+            return result
+
+        def _thread(form_logic, new_order):
+            endpoint = reverse(
+                "api:form-logics-detail", kwargs={"uuid": form_logic.uuid}
+            )
+            with patch(
+                "ordered_model.models.OrderedModelQuerySet.update", queryset_update
+            ):
+                response = self.client.patch(endpoint, {"order": new_order})
+
+            close_old_connections()
+            self.assertEqual(response.status_code, status.HTTP_200_OK)
+
+        threads = [
+            Thread(target=_thread, args=(fl2, 0)),
+            Thread(target=_thread, args=(fl3, 1)),
+            Thread(target=_thread, args=(fl1, 2)),
+        ]
+
+        for t in threads:
+            t.start()
+
+        for t in threads:
+            t.join()
+
+        # check that the race conditions are not a problem and the API endpoints
+        # are idempotent
+        fl1.refresh_from_db()
+        fl2.refresh_from_db()
+        fl3.refresh_from_db()
+
+        self.assertEqual(fl1.order, 2)
+        self.assertEqual(fl2.order, 0)
+        self.assertEqual(fl3.order, 1)
+
+    def test_reorder_logic_rules_without_mocking(self):
+        user = SuperUserFactory.create()
+        self.client.force_authenticate(user=user)
+        form = FormFactory.create(
+            generate_minimal_setup=True,
+            formstep__form_definition__configuration={
+                "components": [
+                    {
+                        "type": "textfield",
+                        "key": "component",
+                    }
+                ]
+            },
+        )
+        # create some existing rules, we'll use patch requests to only modify the order
+        common_kwargs = {
+            "json_logic_trigger": {"==": [{"var": "component"}, "1"]},
+            "form": form,
+        }
+        fl1 = FormLogicFactory.create(order=0, **common_kwargs)
+        fl2 = FormLogicFactory.create(order=1, **common_kwargs)
+        fl3 = FormLogicFactory.create(order=2, **common_kwargs)
 
         def _thread(form_logic, new_order):
             endpoint = reverse(
                 "api:form-logics-detail", kwargs={"uuid": form_logic.uuid}
             )
             response = self.client.patch(endpoint, {"order": new_order})
+
             close_old_connections()
-            self.assertEqual(response.status_code, status.HTTP_200_OK, response.data)
+            self.assertEqual(response.status_code, status.HTTP_200_OK)
 
         threads = [
-            Thread(target=_thread, args=(fl1, 1)),
-            Thread(target=_thread, args=(fl3, 0)),
-            Thread(target=_thread, args=(fl2, 2)),
+            Thread(target=_thread, args=(fl2, 0)),
+            Thread(target=_thread, args=(fl3, 1)),
+            Thread(target=_thread, args=(fl1, 2)),
         ]
 
         for t in threads:
@@ -1149,6 +1338,6 @@ def _thread(form_logic, new_order):
         fl2.refresh_from_db()
         fl3.refresh_from_db()
 
-        self.assertEqual(fl1.order, 1)
-        self.assertEqual(fl2.order, 2)
-        self.assertEqual(fl3.order, 0)
+        self.assertEqual(fl1.order, 2)
+        self.assertEqual(fl2.order, 0)
+        self.assertEqual(fl3.order, 1)