Problem/Motivation

Ultimate Cron replaces core's cron service, and with it core's global cron lock. Each cron job gets its own lock instead.

Queues do not. With queue.enabled on, each queue becomes a cron job and is locked. With it off, UltimateCron::run() calls processQueues() directly, and nothing locks it.

So two cron runs that overlap drain the same queue at the same time. If a queue worker loads an entity, changes it and saves it, two workers can do that to the same entity at once, and one overwrites the other.

Steps to reproduce

  1. Leave queue.enabled off, its default.
  2. Start a cron run that lasts longer than the interval between runs.
  3. The next run starts while the first is still going.

Both call processQueues() and claim items from the same queues.

Proposed resolution

Lock the processQueues() call in UltimateCron::run(), and skip it if the lock is taken.

Use a lock name of its own, not core's cron, since it covers queue processing and not the whole run.

Remaining tasks

  • Decide whether the lock timeout should be configurable. The patch uses 900 seconds, the value Cron::run() uses for the same call.
  • Review.

User interface changes

None.

API changes

None.

Data model changes

None.

Issue fork ultimate_cron-3618809

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

herved created an issue. See original summary.

herved opened merge request !90

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Isn't queue processing by itself meant to be conflict-resistant? Drush ships queue runners for example and you on purpose can have more than one running at the same time.

Fair point that core by default handles it with the global lock, but see \Drupal\Core\Cron::processQueue and the claimItem() API, it doesn't just get a queue item, it attempts to claim it and only one can succeed.

I think if you have a queue plugin where different queue items can conflict then that's on you?

herved’s picture

herved commented
Issue summary: View changes
Status: Active » Needs review

CI failing on current 8.x-2.x, from CronJobInstallTest.

herved’s picture

herved commented
Status: Needs review » Needs work
herved’s picture

herved commented

Thanks for the early feedback @berdir

We hit this with TMGMT and proposed a fix there (#3618383: Data loss when the same source is accepted concurrently), by locking the entity while a translation is applied. But that only protects TMGMT from itself. What's left is two different queues from two different modules: TMGMT's queue saves node X to add a translation, and another module's import queue saves the same node X. Both claim their own items fine, both save, and one overwrites the other.
A queue plugin can't really guard against a queue it doesn't know about.

Core takes the cron lock around processQueues(), so two cron runs never do this.
Was dropping that lock intentional, or just a side effect of replacing the cron service?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I don't think it was intentional, but I'm also not sure it's correct to add it. As mentioned, queues are kind of expected to be run in parallel and long-running processes. This wouldn't fix those queues for someone using a different queue runner.

In commerce, I've added the loadForUpdate() (that's essentially loadUnchanged + lock) a while ago to handle such race conditions, mostly for checkout with multiple requests like webhook notifications and the user returning both doing updates. maybe core should have a built-in protection against this that disallows updates on stale entities from static cache, something like a generated hash that updates on save. There likely are issues related to that. we kind of have something like that with the changed-based protection but that's only in the UI and it won't help much with long-running processes. Possibly loadForUpdate() could be pushed into core?

while a queue can't guard against conflicts entirely, it might be useful to document somewhere that queues should by default use loadUnchanged(), that should cover most cases except a race condition during the actual save on multiple processes, but then this won't protect against that either if the other save isn't from the queue? seems like a partial workaround at best?

From the description, I'm not clear whether you are hitting an actual race condition between load + change + save, which is a rather small window or a stale static cache, which would require a third queue item or something to load that entity and keep it in memory (could be a cron job too). It has to be something that doesn't save, because save does clear the memory cache.

herved’s picture

herved commented

What we actually hit: both failures came from two items of the same queue, and we proposed a fix in tmgmt (#3618383: Data loss when the same source is accepted concurrently), a lock on the source plus loading it fresh once the lock is held. The fresh read on its own still leaves the window between the load and the save, which is why the lock is there too.

I filed this issue in ultimate_cron as extra insurance, since we also run another module with its own queue that saves the same nodes. We haven't seen a collision across two queues yet on production, but it looks possible to me.
The patch here only covers the cron path, UltimateCron::run() calling processQueues(). drush queue:run is untouched, so a queue can still be processed by several runners in parallel.

On your question, we hit two different failures:
- One is a plain race: two runs each held the node from before the other committed, and the later save dropped the translation the earlier one had just added, since storage deletes translations that are missing from the entity being saved. That cost us 314 translations across 93 nodes over about six weeks.
- The other one is the cache holding uncommitted rather than stale data, which ends with entity_reference_revisions dropping paragraph references it can't resolve (in entity_reference_revisions_entity_revision_create), 104 references across 16 nodes. Neither is anything this module does, and I have the details and a repro for the second if that's useful.

Setup for context: 45,000 nodes, 23 continuous jobs (one per target language, English source), delivered into the tmgmt_ec_etranslation queue and auto accepted. Cron every 5 minutes, and runs overlap: we saw a node revision written by a run that had started 18 minutes earlier.

herved’s picture

herved commented
Status: Needs work » Needs review

I understand the reservation in #7, but on closer inspection we run 9 cron queues that write content entities and several of them target the same entities/nodes.
loadForUpdate() looks like a great concept, but unfortunately it doesn't exist yet in core, and it would only help if every queue adopted it, including the ones from contrib.
Core's cron lock covered all of them at once, without any queue having to know about the others. That's what we lose here.
So I'm going to apply this patch on our project for now.

Tentatively moving to review.
PS: I opened #3618995: Fix CI now on Drupal 11.4 (CronJobInstallTest failure) for the CI failures

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

I tested MR !90 against a real long-running queue workload and wanted to add a data point here.

Our case is Linkchecker rather than an entity-write race. Its cron worker has a 240-second budget (current default), and cron is triggered every minute(because I'm like that).

This data was LLM compiled with a bunch of data I have been collecting in both production and locally to track down the source of a dreaded and sporatic "Attempting to re-run cron while it is already running." 🐰🕳️

Without this patch, Ultimate Cron allowed the fallback queue-processing phase to overlap:

  • 817 batches / 6,531 URLs
  • maximum 5 simultaneous cron processes
  • maximum 5 simultaneous queue-processing phases / Linkchecker workers
  • backlog drained in 9.74 minutes

All 817 batches were accounted for, so claimItem() did its job; I did not see duplicate queue-item processing or data corruption.

With MR !90 applied, repeating the same workload gave:

  • maximum 2 simultaneous cron processes
  • maximum 1 queue-processing phase / Linkchecker worker
  • 25 competing invocations skipped queue processing cleanly
  • backlog drained in 31.64 minutes
  • all 817 batches / 6,531 URLs still accounted for

So for us the issue is less queue-item correctness and more resource concurrency. The current behaviour can turn one long-running cron queue worker into several concurrent PHP processes doing expensive outbound HTTP work.

I think @berdir's point about explicit queue runners being expected to support parallel execution still stands. What seems different here is the fallback UltimateCron::run()->processQueues() path: it is taking the place of core cron queue processing, but without core's global cron serialization.

MR !90 has been a useful fix for that distinction on our site. It serializes the fallback cron queue phase without preventing someone from intentionally running a queue in parallel through another runner.

I'll collect more data over the weekend. I can see both of your points on this and I am wondering if we can have 🍰 and eat it too...
Hope you found the data interesting, have a nice weekend!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Both core and the MR will log warnings if if you reach that threshold, not against doing that lock, but it seems like a last resort kind of thing.

Seems wrong to me to have a queue being processed for longer than your cron execution frequency, I'd consider opening an issue in link checker to lower that by default and maybe make it configurable. If someone wants to those faster, the queue API provides options to do that. Doing it like this risks delaying other queues and problems like this.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

RE #11 good point. The lock exposed that our 240s default is also worth fixing (in my case). I am co-maintainer of linkchecker, so will open that up there if that is the result of the investigation. More experimenting! 🧪