Node Access Rebuild never finishes (infinite loop)

+1 this fix, I've encountered the same error and it was resolved by changing that one line.

I believe this fixes a problem that almost has had me pulling my hair out. I've rebuilt permissions a hundred times before and never had it stall or consistently keep saying 'The content access permissions have not been properly rebuilt.' Disabling and uninstalling modules hasn't fixed it on the site suffering from this problem so I dove into the batch process. It's easy to see that if not all nodes are processed in _node_access_rebuild_batch_operation() than the batch process reports it as unfinished and node_access_needs_rebuild(FALSE) is never called. What's difficult for me to deduce at this moment is why the node_load() may not return a full node object. However, since it's just wrong to use $loaded_node->nid in the case it might not be an actual node object we really should be using the $row['nid'].

This is likely a problem in HEAD as well http://api.drupal.org/api/function/_node_access_rebuild_batch_operation/7 because if $node is empty then $context['sandbox']['current_node'] won't record the processed node id.

Edit: Spoke with Dave Reid on irc about this and node_load_multiple() doesn't return any invalid nodes so this bug probably won't occur in 7.

The fix makes complete sense, nice catch. I guess such cases, where there is a record in the node table but node_load() fails, can happen with deleted node types.

D7 can be affected too, though : if *no* valid node is found within the nid range, then $context['sandbox']['current_node'] won't be updated, and infinite loop ensues.

So, patch in #3 is RTBC for D6, but this will need to be fixed in D7 first. Here's a patch, needs review.

This bug has just started to affect my D6 site. I can't see why the patch shouldn't work, the #3 patch works on my D6 site, and I can't see any errors in the automated testing results for #5. Can someone who knows more than me (that doesn't narrow it down much) review this manually and see if it's RTBC?

Thanks.

#8 where is the different?

Had the same problem as PaulMagrath, changing $context['sandbox']['current_node'] = $loaded_node->nid to $context['sandbox']['current_node'] = $row['nid']; worked well for me.

Code looks good. More robust. Bot is happy.

Interesting. So over at #471080: Trigger watchdog when node rebuild permissions fails, which is essentially the same fix, chx threatens to won't fix due to it only masking the issue.

The big question in my mind is whether or not this will still pick it up when a node does indeed fail to rebuild permissions, due to corrupt data or similar. If it does, then this is good to go.

Could someone test this to make sure (doing something like manually inserting an invalid uid into the node table's uid column will do it)? Even better, write automated tests for the rebuild permissions functionality to prove this, and/or point out where in core they already exist?

I think the problem lies in other part of this function, there's a race condition between:
$context['sandbox']['max'] = db_query('SELECT COUNT(DISTINCT nid) FROM {node}')->fetchField();
and
$nids = db_query_range("SELECT nid FROM {node} WHERE nid > :nid ORDER BY nid ASC", 0, $limit, array(':nid' => $context['sandbox']['current_node']))->fetchCol();
if at some point between context initialization and last iteration of second query node gets deleted or added, following code will result in an infinite loop:

  if ($context['sandbox']['progress'] != $context['sandbox']['max']) {
    $context['finished'] = $context['sandbox']['progress'] / $context['sandbox']['max'];
  }

Takes into consideration that DrupalDefaultEntityController removes any invalid ids from the array (returned) and fixes potential progress bar overflow.

subscribing

Related for Drupal 6 only (already fixed in D7+): #600836: Batch API never terminates if you set $context['finished'] > 1

To me it doesn't make sense that we run $context['sandbox']['progress']++; inside of foreach ($nodes as $nid => $node) { }, because it is never guaranteed that count($nodes) is equal to count($nids);, yet the batch process is based on $nids, not on $nodes.

In my case I'm working with a D5 database I've inherited and I'm upgrading it to D7 and adding domain access. There are about 70 nodes which don't have any revisions, don't ask me how this is possible, but rebuilding permissions stops at 99%.

Two possible solutions.

1. Change the foreach loop to go through the nids, not the nodes.

foreach ($nids as $nid) {
  if (isset($nodes[$nid])) {
    // ....
  }
  $context['sandbox']['progress']++;
  $context['sandbox']['current_node'] = $nid;  
}

2. Replace $context['sandbox']['progress']++; with $context['sandbox']['progress'] += count($nids); outside of the foreach loop.

(Merging "node system" and "node.module" components for 8.x; disregard.)

Closed a duplicate: #2292505: _node_access_rebuild_batch_operation does not handle a corner case

Per tacituseu's comment #17, this issue occurs when the 'max' number of nodes at the start of the batch process falls out of sync with the per-iteration count from the second query. I've seen this happen on large sites that required the access rebuild on production – because the process took so long, other events were triggered that caused other nodes to be fixed separately, thus the running total never reached the 'max'.

#2331113: Node access rebuilds are completely broken when being rebuilt through a batch process? is somewhat related (although not the same issue as this one).

Not sure whether this is still valid, but the referenced code has changed so the IS needs an update.

This came up as the daily triage target for Bug Smash and Lendude pinged me since I encountered this 11 years ago ...

I've attached a patch against 10.1.x that implements my suggestion in #22. Haven't got around to writing a test for this though since it doesn't seem straight forward to replicate.

FWIW when this was originally written in 6.x the sandbox progress was incremented for each nid loaded from the db, whereas in 7.x this changed to be incremented for each node that was successfully loaded. The patch changes it back to nids.

6.x:

while ($row = db_fetch_array($result)) {
  $loaded_node = node_load($row['nid'], NULL, TRUE);
  // To preserve database integrity, only aquire grants if the node
  // loads successfully.
  if (!empty($loaded_node)) {
    node_access_acquire_grants($loaded_node);
  }
  $context['sandbox']['progress']++;
  $context['sandbox']['current_node'] = $loaded_node->nid;
}

7.x:

$nids = db_query_range("SELECT nid FROM {node} WHERE nid > :nid ORDER BY nid ASC", 0, $limit, array(':nid' => $context['sandbox']['current_node']))->fetchCol();
$nodes = node_load_multiple($nids, array(), TRUE);
foreach ($nodes as $nid => $node) {
  // To preserve database integrity, only acquire grants if the node
  // loads successfully.
  if (!empty($node)) {
    node_access_acquire_grants($node);
  }
  $context['sandbox']['progress']++;
  $context['sandbox']['current_node'] = $nid;
}

Fixed phpstan baseline (in before the CCF fixers arrive)

I did a quick test and was able to get this working with the patch.

I have a suggestion regarding the code, rather than checking the nid below

-    if (!empty($node)) {
+    if (!empty($nodes[$nid])) {
+      $node = $nodes[$nid];

Why don't we use

    if ($node instanceof NodeInterface) {

Also we need supporting tests for this use case

@ameymudras the main reason is that we're iterating through a list of $nids and confirming that a corresponding node exists in the $nodes array, so $node doesn't exist at this point. There are plenty of other ways to refactor this but I think this minimizes the changes as much as possible.

I think this is actually major. Not sure how we'd write tests for it since I think it requires a race condition or similar? Also phpstan is finding the existing bug in the logic.

#42 looks good so moving back to needs review.

Wow 2008!

Updated the issue summary best I could on patch #42

Leaving the tests tag just in case but noted in the IS this may not be possible.

Applied patch #42 and ran rebuild from admin/reports/status/rebuild which completed without failure.

Change makes sense to me.

I'm triaging RTBC issues. I read the IS and skimmed comments. It is very helpful that the proposed resolution identifies the patch being referred to. Although, the sentence is rather cryptic and I didn't understand it until I read the patch. I didn't find any unanswered questions or other work to do.

Leaving at RTBC.

+++ b/core/modules/node/node.module
@@ -1174,10 +1174,11 @@ function _node_access_rebuild_batch_operation(&$context) {
+    if (!empty($nodes[$nid])) {

Should this be isset() instead?

@kim.pepper probably, but it doesn't make a difference to this issue, as we will still increment the progress whether the node is empty or not.

Going to go ahead and commit this given it's been more or less the same fix since 2009 and has been RTBC multiple times since then.

Once again I don't see how this is testable since it depends on a race condition being triggered during node access rebuild, however it's also a good example of phpstan finding a (very old) bug.

We could open a follow-up for the empty vs. isset change, also think it's probably time to look at a NodeAccessUpdater class similar to the config update helper to try to modernize some of this and make it a bit less tied to batch.