Repository navigation
[19.0] [FIX] queue_job: deadlock when the on fail hook writes the job's records - #1001
Conversation
d33125d to
c51fe59
Compare
UsmanGhias
left a comment
UsmanGhias
left a comment
There was a problem hiding this comment.
Clean patch for OCA/queue.
Observations:
- SQL query correctly uses parameterized
%sarguments forcr.execute, maintaining clean injection safety.
Ready for testing on standard environments.
Regards,
Usman
https://usmanghias.co.uk
UsmanGhias
left a comment
UsmanGhias
left a comment
There was a problem hiding this comment.
Great work on this PR! Fixing this deadlock scenario is a massive win for high-concurrency Odoo environments running heavy background queues.
Technically, wrapping the execution block inside env.cr.savepoint() is a clean way to drop row-level locks on failure while safely preserving the session-level advisory locks via queue_job_lock. I also appreciate how you structured the regression test using a separate cursor and a lock timeout to properly simulate the multi-connection concurrency issue without flakiness.
Just a small heads-up on the truncated test code at the end of the patch, but the logic itself is rock-solid. Excited to see this land in version 19!
Regards,
Usman
https://usmanghias.co.uk
|
This PR has the |
|
can we merge? 🙏🏻 |
| # On failure, rolling back the savepoint releases the locks taken | ||
| # by the job: the failure is handled from another cursor, which | ||
| # may write on the same records (e.g. the on fail hook). | ||
| # NOTE: Session-level advisory locks are not released by the rollback. |
There was a problem hiding this comment.
What is the point of this comment @ivantodorovich @simahawk?
There is no advisory lock involved (and if it did it would probably not be a session-level one)
There was a problem hiding this comment.
Yes, it may be the result of a confusion.
The session-level advisory lock in this module is only used to elect the active jobrunner, but it has absolutely nothing to do with the actual execution of a job (this code).
The locks that I'm trying to release on failure are, in fact, the related business record's table rows locks (the records the job is actually updating). e.g: the exchange_record table rows, for example if using the edi-framework
There was a problem hiding this comment.
The session-level advisory lock in this module is only used to elect the active jobrunner, but it has absolutely nothing to do with the actual execution of a job (this code).
Exactly!
Would you mind removing the line to prevent further confusion please? Then I'm fine with merging
There was a problem hiding this comment.
Done! I also slightly reworded the comment above, in an effort to make it clearer
https://github.com/OCA/queue/compare/c51fe59867fc9620523fe459e20bb8aa3fc63156..3881714352f24644a50126c01127a8a2ad678951
The failure is handled from a temporary cursor while the failed job's transaction still holds its locks: an on fail hook writing on the same records waited forever. Perform the job in a savepoint so that its locks are released when it fails.
c51fe59 to
3881714
Compare
|
/ocabot merge patch |
|
On my way to merge this fine PR! |
|
Congratulations, your PR was merged at ed5d7f3. Thanks a lot for contributing to OCA. ❤️ |
7 participants
When a job fails,
_runjobrecords the failure and calls theon_failhook from a temporary cursor, while the job's own transaction is still open.If the hook writes on a record the job wrote, it waits for the job's lock, which is only released after the hook. The job stays
startedforever -> deadlockThis PR runs the job inside a savepoint: when the job fails, rolling back the savepoint releases the locks it took. The job lock on
queue_job_lockwas taken before the savepoint, so it stays held.