Problem/Motivation

If you set up a field to use queue, there is a race condition where the field can be edited before cron runs.

Steps to reproduce

- set up a field to use queue
- create a node and set the field to refer to taxonomy term 1
- delete taxonomy term 1, which queues deletion of the node's delta 0 item
- edit the node so that the delta 0 item now refers to taxonomy term 2
- run cron
- observe that the node no longer refers to taxonomy term 2

Proposed resolution

When queueing a field item for deletion, make note of its target id. When processing the queue, check the field item's target id, to make sure it hasn't changed since the item was queued for deletion.

User interface changes

None.

API changes

The signature of entity_reference_purger_add_to_queue() changes, but only to add an optional $target_id argument, so BC is preserved.

Data model changes

The queue item structure changes, again to add an optional 'target_id' item.

Comments

ksenzee created an issue. See original summary.

ksenzee’s picture

Status: Active » Needs review
StatusFileSize
new5.28 KB
new8.54 KB

Here's a patch that fixes the issue, with tests.

The last submitted patch, 2: 3346341-race-condition-TEST-ONLY-FAIL.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

dalin’s picture

Priority: Major » Critical

Setting to critical since this results in unexpected data loss.

Question about the patch: why skip if the entity ID is no longer is in the expected position? Shouldn't we search for the expected ID and remove it regardless of its position? We could use
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21TypedData...

dalin’s picture

Status: Needs review » Needs work
mrshowerman’s picture

Status: Needs work » Needs review
StatusFileSize
new9.06 KB
new2.49 KB

Addressing #4 while trying to stay backwards compatible (i.e., process queue items without target ID using the "old way").
Also adding a sanity check from my patch in #3348326: Exceptions are not being caught.

dgroene’s picture

StatusFileSize
new10.18 KB

Rerolled for version beta5 and accounting for revisions.

gnikolovski’s picture

Assigned: Unassigned » gnikolovski

  • gnikolovski committed 791bf00c on 1.0.x
    Issue #3346341 by ksenzee, mrshowerman, dgroene, dalin, gnikolovski:...
gnikolovski’s picture

Status: Needs review » Fixed

Fixed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.