[16.0][IMP] queue_job: remove dead jobs requeuer cron and automatically requeue dead jobs

[AnizR]

Goal

Automatically re-queue jobs that have been started but whose worker have been killed.
And rely on Odoo's limit_time_cpu and limit_time_real for job execution.

Technical explanation

Everything relies on a new table queue_job_locks which contains ids of jobs that have been started.
When a job is executed, its row queue_job_locks is locked.

If a row is in queue_job_locks with a state='started' but not locked, it is either:

• a job killed
• a job not yet started (happens in a very small time frame)

Using this information, we can re-queue these jobs.

Why not lock directly in the `queue_job' table?

This was tried in #423 but it wasn't working when a job was raising an error.
It seems that the row was locked and it tried to write on the row to set as failed before committing.

Improve current behavior

Re-queue jobs that have been killed but increment their 'retries' to avoid having a job that is always get killed in infinite re-queuing.

[OCA-git-bot]

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

[sbidoul]

This looks very promising!

A few thoughts:

• There is a Caveat comment in runner.py that needs updating.
• This should handle the enqueued state too, which is the state when the runner has decided that a jobs needs to run but then /queue_job/runjob controller has not set it started yet. This state normally exists for a very short time, but I have seen situations where the workers are overloaded and take time to accept /queue_job/runjob requests then die, leaving jobs in enqueued state forever.
• Since there are two race conditions between enqueued and started, and between started and the time the job transaction actually starts with the lock, I wonder if we should not introduce a small elapsed time condition (10s?) in reset_dead_jobs. May be based on date_enqueued.

[AnizR]

This looks very promising!

A few thoughts:

* There is a Caveat comment in `runner.py` that needs updating.

* This should handle the `enqueued` state too, which is the state when the runner has decided that a jobs needs to run but then `/queue_job/runjob` controller has not set it started yet. This state normally exists for a very short time, but I have seen situations where the workers are overloaded and take time to accept `/queue_job/runjob` requests then die, leaving jobs in `enqueued` state forever.

* Since there are two race conditions between `enqueued` and `started`, and between `started` and the time the job transaction actually starts with the lock, I wonder if we should not introduce a small elapsed time condition (10s?) in `reset_dead_jobs`. May be based on `date_enqueued`.

Thank you for your suggestions. I have implemented the necessary corrections.
While I have not encountered a case where a job is left in the 'enqueued' state, I have made the appropriate correction to ensure the code is more robust.

[sbidoul]

A few more minor comments.

[sbidoul]

Why adding this? This sounds unrelated.

[AnizR]

The check that max_retries is greater than retry was done only when a RetryableJobError was raised during the execution of the job.

Since I am increasing the retry when re-queuing a job that was killed, I need to do the check before peforming the job, not only when raising an exception.

Moreover, I think that it doesn't make sense to perform the check only when an exception has been raised 🤔

[guewen]

Thanks for tackling this old issue, I like this elegant solution. It should be quite optimized also. Congrats for this work

[amh-mw]

Should this become

<delete id="ir_cron_queue_job_garbage_collector" model="ir.cron"/>

to clean up upgrades / avoid filling cron logs with errors since requeue_stuck_jobs method is gone?

[AnizR]

I have archived the cron in the pre-migration.py. Therefore, there won't be any error.
I always prefer to archive (set active to false) rather than deleting.

[sbidoul]

@AnizR I think we can also remove the set_job_pending function, as the new mechanism will take care of that part.

[AnizR]

@AnizR I think we can also remove the set_job_pending function, as the new mechanism will take care of that part.

Yes, jobs that have not been started will be re-queued by my new mechanism.
Removing this part will provide a more "uniform" process of re-queuing jobs.

[adrienpeiffer]

LGTM. Thanks for that.

[adrienpeiffer]

@guewen Could you merge this one ?

[adrienpeiffer]

@guewen We're finally going to do a few more tests before we merge this. We'll keep you informed

[florian-dacosta]

Review LGTM
I also did some tests with job exceeding the time_cpu_limit and it works as expected.

[hparfr]

Thanks for addressing this problem.

I'm asking for more comments to give explicit intent.

And I have a question about: why all these joins instead of use id or uuid directly.

[rousseldenis]

Some comments

[rousseldenis]

@AnizR Could you check last @hparfr comments ?

[AnizR]

Thank you for all your valuable feedback!

I appreciate the time you've taken to review this PR.
I will rework the code based on your suggestions and push an update as soon as possible.

[sbidoul]

@AnizR another feedback I got today is that the new queue_job_locks table got deleted by database_cleanup. Maybe we should create a model for it with a proper foreign key, even if we don't actually use the ORM to manipulate it?

[AnizR]

I think that I've addressed every remarks. Thank you all!

I've managed to add some tests on my newly introduced mechanism.
I was forced to introduce some commit() in my tests since I am relying on rows and locks in the db.

[sbidoul]

@AnizR I realized that the AbstractModel technique to create the lock table would not prevent it to be purged by database_cleanup. So I made the change to use a regular model on the train back from the sprint. Could you check?

[pedrobaeza]

You can add the exception in the database_cleanup module as well, as we have done with some columns, although it's true that if there's another way to not require that manual list, it's better.

[AnizR]

@AnizR I realized that the AbstractModel technique to create the lock table would not prevent it to be purged by database_cleanup. So I made the change to use a regular model on the train back from the sprint. Could you check?

I did a small test to simulate re-queuing of 'dead jobs'.
It still works correctly, thanks!

[hparfr]

Comments added as I requested, thanks @AnizR

[rousseldenis]

@AnizR I realized that the AbstractModel technique to create the lock table would not prevent it to be purged by database_cleanup. So I made the change to use a regular model on the train back from the sprint. Could you check?

I did a small test to simulate re-queuing of 'dead jobs'. It still works correctly, thanks!

We'll do tests today