A common workflow for projects is to have a read-only production search index, and a local search server that can be used for development. Developers can run drush sapi-sis [search-index] [local-server] to move the index to the local server before enabling writes on it.

The problem is that when you run drush sapi-sis, it rebuilds the original (read-only) search index--in most cases, destroying search data in a production environment (hence the major priority of this bug).

Patch to follow.

CommentFileSizeAuthor
#7 2847092-6--remove_index_read_only_check.patch866 bytesdrunken monkey
#4 search_api-2847092-4.patch950 bytesDane Powell
#2 search_api-2847092-2.patch1014 bytesDane Powell
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dane Powell created an issue. See original summary.

Dane Powell’s picture

Dane Powell CreditAttribution: Dane Powell at Acquia commented
Status: Active » Needs review
FileSize
search_api-2847092-2.patch1014 bytes

The attached patch fixes the issue.

Note, however, that I'm not confident that it's the best fix. It seems like this code should only be acting on the new server in the first place, so there might be a deeper bug here.

I also suspect there are other "leaks" like this one where read-only indexes can be contaminated or destroyed--a more thorough review might be in order.

Dane Powell’s picture

Dane Powell CreditAttribution: Dane Powell at Acquia commented
Status: Needs review » Needs work

Actually I think this is an incomplete fix. It appears that something else in the sapi-sis process can also break the original read-only index.

Dane Powell’s picture

Dane Powell CreditAttribution: Dane Powell at Acquia commented
Status: Needs work » Needs review
FileSize
search_api-2847092-4.patch950 bytes

Okay this makes a lot more sense and seems to fix the problem. I'm not sure what the logic was behind the original behavior here, it seems dangerously destructive.

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy at Acquia commented
Status: Needs review » Reviewed & tested by the community

Reviewed the patch locally, but haven't had a chance to review whether it fully fixes the problem in my production environment. It definitely isn't causing a regression, and the code change looks correct (and much safer than before), so marking RTBC.

This scared the heck out of me when I first noticed—any time a developer would switch to the local index (even though prod was set to read only), somehow our production Solr index would get wiped out... and then we'd get a lot of angry emails pretty quickly, especially since it was usually morning (highest site search activity) when the developers started their work!

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Title: drush sapi-sis destroys read-only indexes » Data of read-only indexes is deleted when they are removed from the server
Project: Search API » Search API Solr
Version: 8.x-1.0-beta3 » 8.x-1.x-dev
Component: General code » Code
Priority: Major » Critical
Status: Reviewed & tested by the community » Needs review

Thanks a lot for reporting this!
You're right, this is a big problem – I even think "critical" priority is in order here.

The bug, however, isn't in the Search API itself – it's in the Solr backend's removeIndex() method, which was incorrectly ported (or not kept up to date) and just ignored the index's "read-only" flag.

The attached patch should fix this, please test!

I'd say this also needs tests, to be on the safe side, but I'll let the Solr D8 maintainer decide that.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
FileSize
2847092-6--remove_index_read_only_check.patch866 bytes
mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner commented

I committed the patch and just wait for travis to pass.
I opened a follow-up: #2848157: Add test for removeIndex()

Dane Powell’s picture

Dane Powell CreditAttribution: Dane Powell at Acquia commented
Status: Needs review » Reviewed & tested by the community

I confirmed that patch in #7 fixes this issue.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner commented
Status: Reviewed & tested by the community » Fixed

Thanks! If you want to help it would be nice to get a test in the follow-up issue :-)

Dane Powell’s picture

Dane Powell CreditAttribution: Dane Powell at Acquia commented

It seems like there are other ways in which production search data can be destroyed... even with this patch applied, we continue to see the production index occasionally being deleted or corrupted as a result of local development. It no longer happens when we switch the index to the local server (this patch fixed that), but it seems to happen later on, possibly as a result of reindexing the now-local index. We haven't been able to clearly reproduce the behavior yet.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner commented

I assume you use config aoverwrites.
Since almost a year I apply this "emergency patch" to all installations to avoid what you describe: #2684977: Config overrides are sometimes ignored.

If that solves your issue it's time to raise #2682369: Fix problems with overridden config entities to critical!

Dane Powell’s picture

Dane Powell CreditAttribution: Dane Powell at Acquia commented

We're hardcoding config in settings.php, we'll take a look at the linked issues, thanks.

Status: Fixed » Closed (fixed)

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