[19.0] [FIX] queue_job: deadlock when the on fail hook writes the job's records

queue_job/controllers/main.py

@@ -98,10 +98,14 @@ def _try_perform_job(cls, env, job):
         # TODO refactor, the relation between env and job.env is not clear
         assert env.cr is job.env.cr
         with _prevent_commit(env.cr):
-            job.perform()
-            # Triggers any stored computed fields before calling 'set_done'
-            # so that will be part of the 'exec_time'
-            env.flush_all()
+            # On failure, rolling back the savepoint releases the row locks
+            # the job took on the records it updated: the failure is handled
+            # from another cursor, which may write on the same records (e.g.
+            # the on fail hook).
+            # On success, leaving the savepoint flushes, before 'set_done', so
+            # that the stored computed fields are part of the 'exec_time'.
+            with env.cr.savepoint():
+                job.perform()
             job.set_done()
             job.store()
             env.flush_all()

queue_job/tests/test_run_rob_controller.py

@@ -1,4 +1,5 @@
 # License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl).
+from contextlib import closing
 from unittest.mock import patch
 
 from odoo.tests.common import TransactionCase
@@ -50,6 +51,52 @@ def test_runjob_on_fail(self):
             self.assertEqual(job.state, "failed")
             self.assertEqual(mocked_hook.call_count, 1)
 
+    @mute_logger("odoo.addons.queue_job.controllers.main")
+    def test_runjob_on_fail_no_deadlock(self):
+        """The on fail hook can write on the records the failed job wrote.
+
+        The hook runs on another cursor, while the failed job's transaction
+        is still open. Without rolling back the job's changes first, the hook
+        waits for the job's lock, which is only released after the hook.
+        """
+        function = self.env.ref("queue_job.job_function_queue_job__test_job")
+        function.on_fail_method = "_test_on_fail"
+        # Use a committed record, so that the other cursor can see it
+        partner = self.env.ref("base.partner_admin")
+        job = self.env["queue.job"].with_delay()._test_job()
+
+        # The job writes on the partner (locking it), then fails
+        def failing_job():
+            partner.name = "Written by job"
+            partner.flush_recordset()
+            raise JobError("Job failed")
+
+        # The hook writes on the same partner from another cursor. Its changes
+        # are rolled back on close, and the lock timeout avoids waiting forever.
+        def on_fail(**kwargs):
+            with closing(self.registry.cursor()) as other_cr:
+                other_cr.execute("SET LOCAL lock_timeout = '2s'")
+                other_cr.execute(
+                    "UPDATE res_partner SET name = 'Written by hook' WHERE id = %s",
+                    [partner.id],
+                )
+
+        with (
+            self.assertRaises(JobError),
+            patch(
+                "odoo.addons.queue_job.models.queue_job.QueueJob._test_job",
+                side_effect=failing_job,
+            ),
+            patch(
+                "odoo.addons.queue_job.models.queue_job.QueueJob._test_on_fail",
+                side_effect=on_fail,
+            ) as mocked_hook,
+            patch("odoo.addons.queue_job.job.Job.in_temporary_env") as mocked_temp_env,
+        ):
+            mocked_temp_env.return_value.__enter__.return_value = self.env
+            RunJobController._runjob(self.env, job)
+        self.assertEqual(mocked_hook.call_count, 1)
+
     def test_runjob_on_fail_not_configured(self):
         job = self.env["queue.job"].with_delay()._test_job(failure_rate=1)
         with (