Fix server tasks system

This will only occur if you let an index and associated server lying around disabled for some time (or when deleting huge amounts of items). Enabling the server for a short time and then disabling it again should clear the variable. On production sites, just don't leave unused servers lying around and you'll be fine.

If more people agree with you, though, I'd also not be totally against moving the tasks into their own table.

@bibo,

I 100% agree with you. Whilst I don't have the same level of detail that you provided, I can add that we had severe problems on an index with 350,000 items in it. Massive spikes in the cache layer and immense heartache.

As this site is heavily trafficked and highly available, we can't disable the index and enable. Firstly, just index the 350k nodes takes days because we have to do it in limited bursts (again due to the traffic of the site). So that is not an option.

So to make Search API as bullet proof as possible let's pursue this :)

Working for an organization that supports a lot of websites with high traffic and large numbers of documents, we are often seeing this variable becoming way too large and crashing entire sites if they aren't using memcache. Would love to see the architecture change to ensure the reliability of this module for larger sites.

Marking this as major as this is really crashing sites that rely on large scale datasets

I see several possible solutions for this:

• The basic option would be the one proposed here, to move this information to a dedicated table, which would of course eliminate the huge performance impact this can have. The downside is, though, that the 4 MB of probably useless data would still be saved and use up disk space – much better than it being in the cache, but still unnecessary.
• The other option would be to get rid of this whole subsystem, by handling the disabling of a server much like deleting it: all indexes will be removed from the server, and all data indexed on it (for these indexes, unless they're read-only) deleted. We could then trash the entire logic, also making the code in some parts of the module significantly less complex.
  Since disabling a server is never really necessary (except for preventing a Solr server from getting pinged), there is no reason to make it such a light-weight operation, by making sure re-enabling it goes as smoothly as possible (and storing all this data for it). By all your accounts in this issue, the function is currently mainly misused anyways, to permanently get rid of a server. By making disabling more like deleting, we could take that into account.
• A variant for #1, potentially reducing the danger of amassing large amounts of useless data, would be to add a warning mechanism, when disabling a server and/or when visiting the overview with such a disabled server there, that indexes lying on a disabled server will use unnecessary ressources and should be moved away from it.

What would you think the better course of action would be here?

Just had a short chat with Nick Veenhof, in the course of which we realized that the Search API currently doesn't handle the case when a search item gets deleted but the server, though enabled, isn't available (Solr not reachable, etc.). A watchdog message is logged, but the index will stay out of sync for eternity (or until the index is cleared – which might amount to the same for large sites) – which of course isn't good.

So, our proposed solution would be a combination of both options: make disabling the server a more permanent action*, removing the indexes and data from it so we don't have to worry about anything that happens there as long as the server is disabled. And migrate the tasks variable to its own table and use that to track attempted actions that failed (apart from indexing, which the tracking system already handles). In addition to when a server gets enabled, we would then also look at that table during cron runs and see what tasks still need to be done for all enabled servers.

* We also talked about completely dropping support for disabling servers, possibly replacing it with a read-only flag on the server, but this would be an unnecessarily large change – now that we have this functionality, we should keep it, but if most people are using this as an alternative to deleting the server completely, we should take that into account in how we handle it.

One remaining problem is what we should do in the update function with disabled servers that have indexes attached to them. Remove the indexes or not? Keep their tasks or not?
I'll have to see what makes most sense, but thinking about it a bit I guess removing the indexes and making that the sole tasks in the new tasks table would make the most sense.

Another idea here would be to include some function to check the search server for invalid old items and automatically purge them. However, that is another issue, and would probably have to live in the individual backend modules anyways.

Anyways, preparing a patch now.

I totally approve of this change and I'm excited to see what we can do to fix this.

• make disabling the server a more permanent action*, removing the indexes and data from it so we don't have to worry about anything that happens there as long as the server is disabled.
• migrate the tasks variable to its own table and use that to track attempted actions that failed.
• Create an update function to move the task data from the variable to the table.
• look at that table during cron runs and see what tasks still need to be done for all enabled servers.
• Change the tests to use the new task tracking table.

These are the five tasks we need to get done before we've seen this problem fixed. A significant change. If there's anyone else reading this - input is welcome! :)

Change the tests to use the new task tracking table.

There are currently no tests to be updated, I think. But we could of course add some, would definitely make sense.

This would definitely involve some kind of API change – while the search_api_tasks variable isn't exactly exposed and probably not used by anyone, an index's server will now always be enabled, which is different from before. No code should break, though, as checking for the enabled field as before will of course continue to work.

Also, of course, the fact that the service class' delete() method can now throw an exception, and should do so when deleting failed.

This is a first stab at the patch. I haven't tested it yet, nor added any automatic tests, but this is the direction I want to take. The service class can now throw exceptions for some more operations, and the server entity class is set up to automatically catch them and add tasks for them. During cron, the tasks for enabled servers are collected and executed. (The same is done when enabling a server, for that server alone, to clean up on older tasks. However, I'm not even sure that's needed – it would be done on the next cron run anyways. Still, better to be on the safe side. Might otherwise make problems when re-adding an index to a newly re-enabled server.)

The tasks themselves have been moved to a table now, as discussed, and also have come shiny new API functions to deal with them. (For D8 those should probably be methonds on a pluggable task manager service or something, but that's in the future.) The update function removes the indexes from all disabled servers and stores a removeIndex task for each. Those and the clear all tasks are migrated to the new system, all others are ignored since removing the index should take care of them anyways.

Created issues for the Solr (#2126281: Update error handling according to the latest Search API change) and DB (#2126291: Update error handling according to the latest Search API change) service classes to make use of these changes.

Corrected patch, should pass now (only one silly mistake).

Adding new tests now.

Oh, one question though, to any native speaker (or sufficiently confident US-resident Belgian): I've refered in all comments to the tasks that need to be executed as "outstanding tasks". For your ears, does the adjective sound right (pending, unresolved) or weird (prominent, exceptional)?

I first used it without thinking about it and only later realized it might sound weird.
If you think we should use some other adjective, which one would you suggest?

And now even with additional tests.

Lying is a bit weird to use in a server context. Also please add the functionality that now only happens in the cron run to the index now / drush scripts so that tasks are executed when you are indexing the remaining items.

Lying is a bit weird to use in a server context. Also please add the functionality that now only happens in the cron run to the index now / drush scripts so that tasks are executed when you are indexing the remaining items.

OK, here is the patch, with "outstanding" replaced by "pending" and tasks also being executed when indexing items.
I opted here for aborting if the tasks couldn't be executed successfully instead of trying to index items. First off, this saves some time in the likely case that when the tasks fail indexing will fail also. Secondly, if indexing would succeed although some tasks could not be executed, the result might not be desirable. I.e., if we tried to clear the index before, it failed and the deletion of those items is pending, then we want to make absolutely sure we do it before re-indexing any items.
So, your objection regarding also executing the tasks when indexing saved us from adding a new, annoying bug (though it could only have occured under very peculiar circumstances).

I hope this is now ready to go (and that the test bot will say so, too).

Since no-one seems to object, I've now committed this.
Thanks again for your input!