Cron queue gets processed every time cron is called, regardless of whether it's already being processed elsewhere

Actually, I don't think this is really a problem. Cron queue jobs have to be written so that they are independent and can be done in a queue -- meaning that they sit there and wait for some process to come along, read them from the queue, and do them. There are several methods of processing the queue (cron, a Drush command, contrib modules, etc.). Each method follows the same steps: "grab" a job, process it, and continue until some limit is reached.

So, it shouldn't matter if the queue is already being processed by some other method (or a previous cron run). Each job needs to be independent, and only one processor can grab a given job that is in the queue.

I'd be inclined to mark this as "works as designed".

I think this patch is a good idea. +1. I haven't tested it either, but moving the queue operations inside the cron lock check seems like a good idea. Presumably the maintainers of the cron system will tell us why it was done the other way previously?

Sorry, but I think this is an unnecessary patch that breaks how the system is intended to work. The cron queue is supposed to work differently from standard cron jobs, and moving queue operations inside the cron lock removes one of the key differences. As stated in the cron queue documentation:

While there can be only one hook_cron() process running at the same time, there can be any number of processes defined here running. Because of this, long running tasks are much better suited for this API. Items queued in hook_cron() might be processed in the same cron run if there are not many items in the queue, otherwise it might take several requests, which can be run in parallel.

Unless this behavior is causing an actual bug (and the bug is not due to a poorly designed queue item), then I don't see why it should be altered. I'm changing to works as designed.

Sorry but 6 years later this bug just bit me on a site I'm maintaining and it has nothing to do with a poorly designed queue. The issue is Drupal's automated_cron is set to keep calling cron over and over again on every page load. If you have a long running queue (1 minute say) and you have a lot of traffic, you can easily max out your servers with tons of queues running simultaneously causing the site to crash.

As this is a standard setup we should provide more protection. If the developer wants their queues to be scheduled to run on top of each other, they could easily find other solutions that are more reasonably configured.

I'm submitting a 8.7.x patch to move the call into the lock to at least provide some protection.

#13 looks good and fixes an issue

I agree with #4, queues are designed to be processed in parallel if there is a lot of work to be done.

For sites that need more control over this they should explicitly call cron from an external cron job (instead of relying on the automatic cron that runs after page loads) or use https://www.drupal.org/project/ultimate_cron

Hmm I also agree with #4 and #16 however I'm not sure it's an argument against making this change.

If you have dedicated jobs to run down queues, putting the cron queue running inside the cron lock won't prevent them running simultaneously, it'll only stop more than one cron run from trying to process the queues at once.

After thinking about this some more, this indeed doesn't harm sites that process queues separately, but it does help the case where cron is run regularly and prevents sites with occasionally-busy queues from running out of resources and trying to process more than one queue at a time.

Running tests against 9.5.x and 10.1.x...

I debated the need for a CR but I think the fact that the warning $this->logger->warning('Attempting to re-run cron while it is already running.'); exists means that people should not be depending on the current behaviour - as they'd be getting loads of warnings.

I've credited everyone who contributed to the discussion even if they thought it was works as designed.

Removing credit for @pmagunia - the patch in #5 still applies to 10.x and 9.x so a new MR was unnecessary.

10.1 failure is real.

The cron unit test needed some updating to match the new behaviour.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Tested on 10.1.x with a standard install
Applied patch and verified cron still runs from the UI and from an external URL

But think the tests need tweaking. I applied the test changes and removed the update to Cron.php and the tests still pass.

The tests should still pass; however they made assumptions before that are now incorrect if we only update \Drupal\Core\Cron, so the test needed some tweaking in order for it to pass again (see the fail in #5).

Ah thank you for that explanation. In that case the changes look good to me.

Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x, thanks!

Tangentially related: #3318964: automated_cron should not run cron when visiting update.php — that's s another way that cron gets executed but shouldn't. 😅