diff --git a/core/lib/workarea/elasticsearch/index.rb b/core/lib/workarea/elasticsearch/index.rb index 36787b657..b0375dfe1 100644 --- a/core/lib/workarea/elasticsearch/index.rb +++ b/core/lib/workarea/elasticsearch/index.rb @@ -18,23 +18,40 @@ def exists? Workarea.elasticsearch.indices.exists?(index: name) end + # Create the index, optionally deleting it first when +force+ is true. + # + # The previous implementation used an +unless exists?+ guard which + # introduced a TOCTOU (time-of-check/time-of-use) race condition: the + # cluster state visible to +exists?+ could differ from the state seen by + # the subsequent +create+ call, causing intermittent + # +resource_already_exists_exception+ (400) errors in CI and during rapid + # sequential test runs. When +create!+ raised that exception the index + # remained deleted (because +delete!+ had already run), which caused the + # cascade +index_not_found_exception+ (404) seen in later test operations. + # + # The fix removes the guard and instead rescues the 400 BadRequest that + # Elasticsearch raises when the index already exists. This makes the + # operation atomic from the caller's perspective: if the index was already + # present we simply skip creation, just as the old +unless exists?+ path + # intended – but without the window for a concurrent creation to slip in. + # def create!(force: false) delete! if force - unless exists? - Workarea.elasticsearch.indices.create( - index: name, - body: { - settings: Search::Settings.current.elasticsearch_settings, - mappings: mappings, - aliases: aliases - } - ) - end + Workarea.elasticsearch.indices.create( + index: name, + body: { + settings: Search::Settings.current.elasticsearch_settings, + mappings: mappings, + aliases: aliases + } + ) + rescue ::Elasticsearch::Transport::Transport::Errors::BadRequest => e + raise unless e.message.include?('resource_already_exists_exception') end def delete! - Workarea.elasticsearch.indices.delete(index: name, ignore: 404) + Workarea.elasticsearch.indices.delete(index: name, ignore: [404]) end def while_closed diff --git a/core/test/lib/workarea/elasticsearch/document_test.rb b/core/test/lib/workarea/elasticsearch/document_test.rb index dcd862f57..214642ef4 100644 --- a/core/test/lib/workarea/elasticsearch/document_test.rb +++ b/core/test/lib/workarea/elasticsearch/document_test.rb @@ -143,6 +143,48 @@ def test_scroll ensure Foo.clear_scroll(results['_scroll_id']) end + + # Regression: Index#create! must be idempotent. + # + # The old implementation used `unless exists?` before calling + # `indices.create`. This introduced a TOCTOU race: between the `exists?` + # check and the actual `create` call another request could have created + # (or re-created) the same index, causing a + # `resource_already_exists_exception` (HTTP 400). When that exception + # propagated the index was left in a deleted state (because `delete!` had + # already run), which then caused `index_not_found_exception` (404) for + # any search performed by the same test. + # + # The fix removes the guard and rescues the 400 BadRequest instead, making + # the operation atomic. + def test_create_idempotent_when_index_already_exists + index = Foo.current_index + + # First call – index exists from setup_indexes; should not raise. + assert_nothing_raised { index.create! } + end + + def test_create_force_recreates_index_and_remains_searchable + Foo.save(id: 'before') + Foo.current_index.wait_for_health + + # force: true must delete-and-recreate; previously saved documents are gone + Foo.reset_indexes! + + assert_equal(0, Foo.count, 'expected empty index after force-recreate') + + Foo.save(id: 'after') + assert_equal(1, Foo.count, 'expected new document after force-recreate') + end + + def test_create_force_idempotent_called_twice + # Calling reset_indexes! back-to-back must not raise + # resource_already_exists_exception on the second call. + assert_nothing_raised do + Foo.reset_indexes! + Foo.reset_indexes! + end + end end end end