framework: do not double-execute an async job after returning its queue item by rootnomad · Pull Request #14039 · apache/cloudstack

rootnomad · 2026-09-02T14:51:06Z

Description

In executeQueueItem, when persisting the executing management-server id fails
(the DB-deadlock case the catch block exists for), the queue item is returned to
the queue so it can be retried later. Execution then fell through to
scheduleExecution(job), so the job was dispatched now AND re-dequeued and
dispatched again by the heartbeat, running the same job (VM start/deploy, volume
create, snapshot, etc.) twice concurrently and defeating the sync queue's
serialization.

Return after returning the item so the job runs once, on the retry.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Feature/Enhancement Scale or Bug Severity

Bug Severity

Minor

How Has This Been Tested?

Added a unit test that makes the executing-msid update throw and asserts the
queue item is returned and the job is not also scheduled. Also built the standard
packages and deployed on a KVM advanced zone.

codecov · 2026-09-03T08:04:36Z

Codecov Report

❌ Patch coverage is 76.92308% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.42%. Comparing base (2cd8c5e) to head (b38a0c7).
⚠️ Report is 21 commits behind head on 4.20.

Files with missing lines	Patch %	Lines
...stack/framework/jobs/impl/AsyncJobManagerImpl.java	76.92%	7 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.20   #14039      +/-   ##
============================================
+ Coverage     16.34%   16.42%   +0.07%     
- Complexity    13574    13666      +92     
============================================
  Files          5669     5669              
  Lines        501368   501633     +265     
  Branches      60903    60943      +40     
============================================
+ Hits          81964    82396     +432     
+ Misses       410219   410006     -213     
- Partials       9185     9231      +46

Flag	Coverage Δ
uitests	`4.16% <ø> (+0.01%)`	⬆️
unittests	`17.29% <76.92%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…ue item In executeQueueItem, when persisting the executing MS id fails (the DB-deadlock case the catch block exists for), the queue item is returned to the queue so it can be retried. Execution then fell through to scheduleExecution(job), so the job was dispatched now AND re-dequeued and dispatched again by the heartbeat, running the same job (VM start/deploy, volume create, snapshot, etc.) twice concurrently and defeating the sync queue's serialization. Return after returning the item.

DaanHoogland · 2026-09-04T07:26:56Z

@nagaboinaramgopal the method you are touching has two consecutive try blocks, both of which have nested try block. If you have time, do you feel like cleaning up the code? (does not need to be in the scope of this PR)

rootnomad · 2026-09-05T04:16:27Z

@nagaboinaramgopal the method you are touching has two consecutive try blocks, both of which have nested try block. If you have time, do you feel like cleaning up the code? (does not need to be in the scope of this PR)

sure, will do

Flatten executeQueueItem with an early return for the missing-job case and extract the duplicated return-item-to-queue and clear-executing-msid bookkeeping into helpers, removing the nested try blocks. No behavior change.

shwstppr · 2026-09-07T08:03:57Z

@nagaboinaramgopal do you have reproduction steps for the issue PR is fixing?

rootnomad · 2026-09-07T14:37:48Z

@nagaboinaramgopal do you have reproduction steps for the issue PR is fixing?

Thanks @shwstppr . No linked issue, it came from reading executeQueueItem, and it reproduces deterministically as a fail-before / pass-after test.

The scenario is the DB deadlock path that method already documents in its own TODO comment:

A queued async job is dequeued and executeQueueItem tries _jobDao.update(job.getId(), job) to stamp the executing management-server id.
That update throws, most commonly the DB deadlock the TODO is written to survive.
The catch returns the item to the sync queue with _queueMgr.returnItem(...), so the job will be picked up and run on a later turn.
Before this change, execution then fell through into the next try { scheduleExecution(job); } block and scheduled the same job to run now as well. So the job is both requeued for later and executed now: it runs twice.

The fix returns right after the item is put back on the queue, so a failed dispatch only requeues and does not also execute.

AsyncJobManagerImplExecuteQueueItemTest.executeQueueItemDoesNotScheduleWhenTheJobUpdateFailsAndItemIsReturned reproduces it: it makes the job update throw and verifies the item is returned to the queue and scheduleExecution is not called.

On the unpatched code the test fails, which is the double execution:

executeQueueItemDoesNotScheduleWhenTheJobUpdateFailsAndItemIsReturned  <<< FAILURE!
asyncJobManager.scheduleExecution(...)  was invoked
Tests run: 1, Failures: 1

With the change it passes:

Tests run: 1, Failures: 0

blueorangutan · 2026-09-09T11:12:27Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19172

Explain why a failed executing-msid update returns the item to the queue and what the proper fix would be, as asked in review.

Damans227 · 2026-10-15T14:58:02Z

+            _jobDao.update(job.getId(), job);
+        } catch (Exception e) {
+            logger.warn("Unexpected exception while dispatching job-" + item.getContentId(), e);
+            returnItemToQueue(item);


what happens if putting the item back in the queue also fails? that only logs the error, so the job never runs and just sits there until cleanup fails it about an hour later

Good point. On the requeue itself failing: that logging-only path predates this PR and is the same DB-failure class as the dispatch update, so if the return fails the job is left for the JobCancelThreshold cleanup to fail it on its next pass, as today. I kept a different recovery policy out of this fix to keep it focused, but I'm happy to look at hardening it as a follow up, likely alongside removing the dispatch deadlock at its source, which is the real reason the requeue exists.

can we return early only when putting it back worked? on 4.20 the job still runs if that fails too, now it just sits until cleanup fails it

You're right, and that's on me. My early return strands the job when the requeue itself fails, where the original code at least ran it once by falling through. Fixed in b38a0c7: returnItemToQueue now reports whether the item went back on the queue, and the dispatch-failure path returns only on success, falling through to schedule the job otherwise. So the double-execution stays fixed without losing that safety. Added a test for the requeue-also-fails case.

that covers it, thanks

Damans227 · 2026-10-15T14:58:03Z

+        } catch (RejectedExecutionException e) {
+            logger.warn("Execution for job-" + job.getId() + " is rejected, return it to the queue for next turn");
+            returnItemToQueue(item);
+            clearExecutingMsid(job, item);


should we clear the server id before putting the item back? another server could pick it up in between and we would wipe its id

Good catch, thanks @Damans227, you're right that was a real cross-server race. I moved clearing the executing msid ahead of returning the item in fd11c31, so it is nulled while we still hold the item and another management server cannot dequeue it and have its msid wiped by our update. Added an InOrder test that fails on the old order and passes on the new one.

that covers it, thanks

DaanHoogland · 2026-10-20T11:55:11Z

@rootnomad , can you answer @Damans227 's questions?

…o the queue When scheduleExecution rejects the job, executeQueueItem returned the sync queue item and then cleared the job's executing management-server id. Another management server could dequeue the returned item and stamp its own msid in that window, and the clearing update (a full-row update of the job VO) would then wipe it. Clear the msid while we still hold the item, then return it.

rootnomad · 2026-10-20T14:02:22Z

Thanks @Damans227 for the careful read, and @DaanHoogland. Answered both inline. The first, clearing the executing msid before returning the item, was a real cross-server race, now fixed in fd11c31 with an InOrder test. The second, the requeue itself failing, is a pre-existing path left to the JobCancelThreshold cleanup, so I'd fold that into the deadlock-at-source follow up rather than widen this fix here.

Damans227 · 2026-10-20T14:09:56Z

thanks @rootnomad, CLGTM.

The double-execution fix returned right after returnItem(), but if returnItem itself failed the job was neither requeued nor run and sat until the JobCancelThreshold cleanup. The original code fell through and still ran it once. Return only when the item was put back on the queue successfully, and fall through to schedule it otherwise, so the double-execution stays fixed without stranding the job.

rootnomad force-pushed the fix/asyncjob-double-execution branch from c397396 to 9259d8b Compare September 2, 2026 23:22

DaanHoogland added this to CloudStack Testing Sep 3, 2026

github-project-automation Bot moved this to Backlog in CloudStack Testing Sep 3, 2026

DaanHoogland self-assigned this Sep 3, 2026

rootnomad force-pushed the fix/asyncjob-double-execution branch from 9259d8b to 8c06db5 Compare September 3, 2026 17:18

rootnomad changed the base branch from main to 4.20 September 3, 2026 17:54

framework: simplify executeQueueItem exception handling

bc3cde8

Flatten executeQueueItem with an early return for the missing-job case and extract the duplicated return-item-to-queue and clear-executing-msid bookkeeping into helpers, removing the nested try blocks. No behavior change.

DaanHoogland removed their assignment Sep 7, 2026

DaanHoogland added the status:needs-work label Sep 7, 2026

apache deleted a comment from blueorangutan Sep 9, 2026

DaanHoogland reviewed Sep 9, 2026

View reviewed changes

Comment thread framework/jobs/src/main/java/org/apache/cloudstack/framework/jobs/impl/AsyncJobManagerImpl.java Outdated

framework: move the executeQueueItem deadlock TODO into its javadoc

095d9b3

Explain why a failed executing-msid update returns the item to the queue and what the proper fix would be, as asked in review.

Damans227 reviewed Oct 15, 2026

View reviewed changes

DaanHoogland removed the status:needs-work label Oct 21, 2026

DaanHoogland added the needs-testing label Oct 21, 2026

Conversation

rootnomad commented Sep 2, 2026

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Bug Severity

How Has This Been Tested?

Uh oh!

codecov Bot commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DaanHoogland commented Sep 4, 2026

Uh oh!

rootnomad commented Sep 5, 2026

Uh oh!

shwstppr commented Sep 7, 2026

Uh oh!

rootnomad commented Sep 7, 2026

Uh oh!

blueorangutan commented Sep 9, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Oct 20, 2026

Uh oh!

rootnomad commented Oct 20, 2026

Uh oh!

Damans227 commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Sep 3, 2026 •

edited

Loading

Damans227 commented Oct 20, 2026 •

edited

Loading