Only first update processed when multiple updates for single record land in pull queue together

Yes it looks like this probably affects 8.x as well. I have not verified it with actual testing, but the logic looks the same (see MappedObject.php's pull() method and and PullBase.php's updateEntity() method).

From what I can tell this check stems from trying to prevent overwriting newer data on Drupal with older data from Salesforce. Say a record is updated directly on Drupal before an older update that occurred on Salesforce is processed. It would seem that this check tries to prevent the older data from overwriting the newer data.

Seems like a good idea and the more I look at it I'm starting to think the check itself may need adjustment/improvement, rather than what's stored.

Idea:

Imagine we have $entity_updated, which actually reflects the last time the Drupal entity in question was updated/modified (by any route) - not just what's in $smo->entity_updated. Let $smo->entity_updated reflect the last time we, the Salesforce module updated the Drupal entity.

$entity_updated = [...tbd but does not source from $smo->entity_updated...];
// If we can't determine the last time the Drupal entity in question was updated/modified (by any route), just proceed.
// This check is not useful without an accurate $entity_updated timestamp.
if ($entity_updated != NULL) {
  $updated_on_drupal_after_last_pull = ($entity_updated > $smo->entity_updated) ;
  $drupal_update_is_most_recent = ($entity_updated > $sf_object_updated);
  if ($updated_on_drupal_after_last_pull && $drupal_update_is_most_recent) {
    // Skip record processing.  Drupal data is newer than what we're about to pull over it.
  }
}
// Proceed with processing this record.
...

I think this approach could solve the issue and require no additions/changes to existing data structures.

Interesting... So rather than blindly add to the queue, if an unprocessed queue item already exists for a record, simply update that existing item in the queue with current data...

I think given the structure of the queue table we'd be somewhat limited here unless we identify the salesforce ID or mapping object ID (which may not yet exist for new records - tricky) in the queue name - which is what I think you're suggesting. But setting up a queue per SF ID (or record pair - though you wouldn't necessarily know the drupal ID at the point of queuing) seems like it would introduce more complexity in processing which could easily outweigh anything saved by processing only the most recent update for a record.

And I think the problem with the timestamp check would remain. Too often the check fails to find the Drupal entity's actual last modified/updated date and instead falls back to $smo->entity_updated, which I feel is really the root of the problem. Drupal-side record updates that are more recent than the data in the queue could still get wiped out.

So for those reasons I'm still thinking the approach in #3 may be the way to go. It eliminates the bug and improves the timestamp check (to prevent overwriting newer data on Drupal with older data from Salesforce).

But I'd like to add an additional idea here: the most complex (and undocumented) part of the timestamp check outlined in #3 is determining where to source the last modified timestamp for the given Drupal entity. We could try to guess better with more code - which was what I was originally thinking. Or, perhaps better, we could allow users to provide that (select the correct property on the Drupal entity) in the mapping settings. Basically the flip side of this:

With that I think we'd really be in a good spot. A checkbox for turning the timestamp check off entirely would also be nice.

Anyway if you think this is a viable approach I'd be willing to craft a patch for review.

Let me expand on how I think the proposed logic change solves the bug. Here's the logic:

$entity_updated = [...tbd but does not source from $smo->entity_updated...];
// If we can't determine the last time the Drupal entity in question was updated/modified (by any route), just proceed.
// This check is not useful without an accurate $entity_updated timestamp.
if ($entity_updated != NULL) {
  $updated_on_drupal_after_last_pull = ($entity_updated > $smo->entity_updated) ;
  $drupal_update_is_most_recent = ($entity_updated > $sf_object_updated);
  if ($updated_on_drupal_after_last_pull && $drupal_update_is_most_recent) {
    // Skip record processing.  Drupal data is newer than what we're about to pull over it.
  }
}
// Proceed with processing this record.
...

Illustration of how this solves the issue:

1. Say you have a pull mapping set up for Accounts in Salesforce.
2. Say your cron task is set to run hourly, and there are 100,000 items to process in the pull queue.
3. 7:00AM: Cron runs and processes 25,000 items, but there are still 75,000 items left to process in the queue.
4. 7:30AM: "Acme Corporation" Account's name has been updated to "Acme Corp." on Salesforce.
5. 8:00AM: Cron runs and adds the "Acme Corporation" -> "Acme Corp." update to the queue and processes say another 25,000 items - leaving 50,001 items left to process in the queue.
6. 8:30AM: "Acme Corp." Account's name has been updated to from "Acme Corp." to "Acme Inc." on Salesforce.
7. 9:00AM: Cron runs and adds the "Acme Corp." to "Acme Inc." update to the queue and processes say another 25,000 items - leaving 25,002 items left to process in the queue.
8. 10:00AM: Cron runs and processes say another 25,000 items - leaving just two items left to process in the queue (our two updates to Acme).
9. 11:00AM: Cron runs and processes the "Acme Corporation" -> "Acme Corp." update first. As part of this it sets $mapping_object->entity_updated = time(); (11:00AM). The account's name is now "Acme Corp." in Drupal. The second update is now ready for processing ("Acme Corp." to "Acme Inc."). This second update is processed as well because $updated_on_drupal_after_last_pull is FALSE ($entity_updated (11:00AM) is not greater than $smo->entity_updated (11:00AM)).

Result: All data was queued and processed, and we have "Acme Inc." for the Account's name on both Salesforce and Drupal.

Sourcing $entity_updated in the above is what led me to the Drupal entity last modified date property setting idea in the mapping. Takes the guesswork out of it. I think you're right that we could split that part off into a separate issue - and it involves its own discussion surrounding sane defaults, upgrade path, and interface additions. For this issue we could keep the entity's last modified date source unaltered. In other words we'd keep using $entity->updated for the source. The only slight difference to the sourcing there, which also is what fixes the bug, is that we would never source the entity's last modified date from $smo->entity_updated in cases where $entity->updated is not available (in the logic above, $entity_updated would simply be NULL in those cases). In fact the only consequence I can think of surrounding that slight sourcing change (combined with the new record-skip logic) is the bug at hand being squashed.

Anyway that's the patch I'm thinking of writing, pending your buy-in on approach (of course).

D7 patch in the works.

I'll be testing this today to ensure it squashes the bug.

This is the same patch, but against the stable 7.x-3.2 release.

Just confirming that this patch is working perfectly for me in tests.

To test I needed a pile of updates to the same record in the queue. I achieved this by setting 'skip on cron' TRUE in salesforce_pull.module:

/**
 * Implements hook_cron_queue_info().
 */
function salesforce_cron_queue_info() {
  $queues[SALESFORCE_PULL_QUEUE] = array(
    'worker callback' => 'salesforce_pull_process_records',
    // Set to a high max timeout in case pulling in lots of data from SF.
    'time' => 180,
    'skip on cron' => TRUE,
  );
  return $queues;
}

I then updated the name of a record in salesforce to "Update 1" and ran cron. After this I verified the update was sitting in the queue and that the record was unchanged in Drupal. I repeated this until I had three name change updates sitting in the queue ("Update 1", "Update 2", and "Update 3"). So three updates aimed at the same Drupal record, all sitting together in the queue.

I then set 'skip on cron' to FALSE and ran cron. The result was exactly what I expected. All three updates were processed, in order, leaving me with "Update 3" on both Salesforce and Drupal. Bug fixed. If you did this same experiment with the 8.x-3.x code you'd instead see "Update 1" in Drupal, which is incorrect.