[ivantodorovich]

• Cancellation now cascades. Cancelling a job used to cancel only its direct dependents: in A → B → C, cancelling A left C in wait_dependencies forever.
  Test: test_button_cancel_dependencies_cascade
• A job is cancelled as soon as one of its dependencies is cancelled. It used to be cancelled only when all of them were, so a job with one cancelled and one done (or running) dependency waited forever.
  Tests: test_button_cancel_dependencies_other_parent_done, test_button_done_dependencies_other_parent_cancelled
• Cancelled dependents get a date_cancelled, the same as the cancelled job. It was left empty, so the autovacuum never removed them.
  Tests: test_button_cancel_dependencies, test_button_cancel_dependencies_cascade
• Cancelled dependents get a result telling why they were cancelled.
  Tests: test_button_cancel_dependencies_cascade, test_button_cancel_dependencies_selected
• Cancelling a job along with its dependent cancels the dependent once. It used to be cancelled a second time as part of the selection, overwriting its date and result.
  Test: test_button_cancel_dependencies_selected

_get_common_dependent_jobs_query is replaced by the searchable dependency_ids / dependent_ids computed fields on queue.job, used to enqueue and cancel dependent jobs through the ORM.
Test: test_search_dependencies

[OCA-git-bot]

Hi @sbidoul, @guewen,
some modules you are maintaining are being modified, check this out!

[florentx]

I would use same date_cancelled as parent job, and do it a little bit differently:

--- a/queue_job/job.py
+++ b/queue_job/job.py
@@ -545,10 +545,10 @@ class Job:
             finally:
                 self._env = env
 
-    def _get_common_dependent_jobs_query(self):
-        return """
+    def _get_common_dependent_jobs_query(self, sql_set_clause):
+        return f"""
             UPDATE queue_job
-            SET state = %s
+            SET {sql_set_clause}
             FROM (
             SELECT child.id, array_agg(parent.state) as parent_states
             FROM queue_job job
@@ -579,13 +579,15 @@ class Job:
         return any(self.__reverse_depends_on_uuids)
 
     def enqueue_waiting(self):
-        sql = self._get_common_dependent_jobs_query()
+        sql = self._get_common_dependent_jobs_query("state = %s")
         self.env.cr.execute(sql, (PENDING, self.uuid, DONE, WAIT_DEPENDENCIES))
         self.env["queue.job"].invalidate_model(["state"])
 
     def cancel_dependent_jobs(self):
-        sql = self._get_common_dependent_jobs_query()
-        self.env.cr.execute(sql, (CANCELLED, self.uuid, CANCELLED, WAIT_DEPENDENCIES))
+        assert self.state == CANCELLED, "Job is not cancelled"
+        sql = self._get_common_dependent_jobs_query("state = %s, date_cancelled = %s")
+        sql_args = (CANCELLED, self.date_cancelled, self.uuid, CANCELLED, WAIT_DEPENDENCIES)
+        self.env.cr.execute(sql, sql_args)
         self.env["queue.job"].invalidate_model(["state"])
 
     def store(self):

[ivantodorovich]

Hi @florentx

I digged a bit deeper and found other issues with the current implementation, all described in the PR description and covered by new unit tests. So I decided to refactor and use the orm and computed fields instead of this query.

[divad1196]

sudo() needed for multicompany ?

[ivantodorovich]

This method run in the env of the job's user, which may not have access to queue.job.
db_record does the same for the same reason:

• queue/queue_job/job.py

  Line 373 in 556bbed

  model = env["queue.job"].sudo()

[divad1196]

I thought that sudo() was missing here in fact.
But I didn't know that db_records called db_records_from_uuids which did the sudo().

So all good

[divad1196]

This is changing:
Before: We cancel a job only if all its parents were cancelled
After: We cancel all the dependents immediately

This has an impact when a jobs X depends on 2 other jobs Y and Z.

Lost benefit

Before: if we cancel Y and requeue it before cancelling Z, then X was not cancelled
After: X is cancelled when Y is cancelled.

This can be useful when things are stuck IMO

Fixed bug

If Y was cancelled but Z ran, then X would never be cancelled and never run -> stuck

2 options

Accept the loss

If we are fine losing the benefit, we can go this way

Keep both benefits

We should cancel X once:

• All parents have been handled (cancelled, done)
• at least one parent is cancelled

[ivantodorovich]

Fixing the bug is precisely my intention here.
Why would you cancel before requeuing? I don't think we loose anything here

[divad1196]

For the potential reason:

• cancelled by mistake
• batch cancel + selective requeue
• attempt to unstuck a job ?

IMO, it's not a big loss, we can go with that,
but it's worth mentioning that the behavior is changing if someone is wondering.

[divad1196]

Good to add the recursion.

NOTE: the The Power of 10 would suggest to define a max number of iteration, in particular when we don't ensure no cycle ever happen.

How deep do we expect to go? If considerably deep: SQL recursioin would be better.

[ivantodorovich]

It always ends. Each pass cancels every job it keeps, and only waiting jobs are kept, so each job is handled at most once. That holds even with a cycle, e.g: a.on_done(b); b.on_done(a), cancelling one cancels the other, then the loop stops.
The depth is the graph's depth, so I wouldn't enforce a maximum. Anyways, in practice, it's never that deep.

[divad1196]

In theory yes,
but nobody writes a recursion thinking it will never end and all it cost is to replace while dependents with

for _ in range(MAX_DEPTH):
    if not dependents:
        break

I won't push on this if you want to keep it.