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