Skip to content

[18.0][IMP] queue_job: do not use separate cursor to write error result - #1016

Open
florentx wants to merge 2 commits into
OCA:18.0from
camptocamp:18.0-queue_job-savepoint
Open

[18.0][IMP] queue_job: do not use separate cursor to write error result#1016
florentx wants to merge 2 commits into
OCA:18.0from
camptocamp:18.0-queue_job-savepoint

Conversation

@florentx

@florentx florentx commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

This is a simplification.

This is needed for the change proposed in:

Changes:

  • use a savepoint for job.perform() so that cursor can be re-used in case of OperationalError
  • do not use separate cursor for saving error result on the "queue.job" record
    -use the same cursor, which is in good shape thanks to the savepoint
  • more performant: no need of multiple cursors (except for the allow_commit=True use case)

@OCA-git-bot

OCA-git-bot commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

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

amh-mw
amh-mw approved these changes Oct 21, 2026
View reviewed changes

@amh-mw amh-mw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review only, LGTM. This reminds me of workarounds I was using locally prior to the implementation of allow_commit.

Comment thread queue_job/controllers/main.py
traceback_txt = buff.getvalue()
with StringIO() as buff:
traceback.print_exc(file=buff)
traceback_txt = buff.getvalue()

@amh-mw amh-mw Oct 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with the earlier reclamation of memory here; just calling it out since it is not germaine to the primary intent of this pull request.

@florentx
florentx force-pushed the 18.0-queue_job-savepoint branch 2 times, most recently from 39ed622 to b64a51f Compare October 21, 2026 20:02
@OCA-git-bot

OCA-git-bot commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@sbidoul

sbidoul commented Oct 26, 2026

Copy link
Copy Markdown
Member

Sounds like a good simplification.

One question: there are some operations done in _try_perform_job that are done after the savepoint (including a flush_all). What happens if one of these fail and make the cursor inoperable?

@florentx

florentx commented Oct 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Sounds like a good simplification.

One question: there are some operations done in _try_perform_job that are done after the savepoint (including a flush_all). What happens if one of these fail and make the cursor inoperable?

It seems that it is a valid concern.
Some error might occur during the flush, when it recomputes fields.
So I moved it inside the job.perform() method itself, so it is inside the savepoint block.

Other lines are not likely to trigger a concurrency error:

ivantodorovich
ivantodorovich reviewed Oct 26, 2026
View reviewed changes
Comment thread queue_job/job.py
Comment thread queue_job/job.py Outdated
@ivantodorovich @florentx
[FIX] queue_job: deadlock when the on fail hook writes the job's records
90156f8 
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.
@florentx
florentx force-pushed the 18.0-queue_job-savepoint branch from fa1b124 to 38baa61 Compare October 26, 2026 13:38
ivantodorovich
ivantodorovich reviewed Oct 26, 2026
View reviewed changes
Comment thread queue_job/controllers/main.py
# that the stored computed fields are part of the 'exec_time'.
with env.cr.savepoint():
job.perform()
job.perform()

@ivantodorovich ivantodorovich Oct 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this now removed comment to the perfom definition?
I think it's important that we keep track WHY we chose to use a savepoint

Or keep it here but rephrase it so that it mentions that perform will run in a savepoint

@florentx florentx Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the comment.

@florentx
florentx force-pushed the 18.0-queue_job-savepoint branch from 38baa61 to dc13981 Compare October 26, 2026 14:14
This was referenced Oct 26, 2026
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@amh-mw amh-mw amh-mw approved these changes
@ivantodorovich ivantodorovich ivantodorovich approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approved mod:queue_job Module queue_job ready to merge series:18.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@florentx @OCA-git-bot @sbidoul @amh-mw @ivantodorovich