From f14455550af9c76231cb4e96849ab533677f127a Mon Sep 17 00:00:00 2001 From: Dominik Schmidt Date: Thu, 9 Jul 2026 01:27:26 +0200 Subject: [PATCH] fix(search): address max-review findings - the additive warnings advertise --all-spaces --force-rescan; a plain walk skips unchanged documents and never backfills the new fields - Apply checks index existence first again, so a pre-provisioned index needs no create privilege and odd create-error shapes (string error bodies, cluster blocks) cannot fail a healthy startup; Create on 404 keeps the typed already-exists swallow as the creation-race backstop - number_of_replicas drift is not breaking, it is runtime-tunable and needs no rebuild - bleve returns the classification alongside post-persist errors and the server warns before the error check, so the one-time additive warning is not lost when close or reopen fails - a golden fixture pins the marshaled bleve mapping so a dependency bump that changes marshaling fails in CI instead of refusing every installation in the field --- services/search/pkg/bleve/index.go | 9 +- services/search/pkg/bleve/index_test.go | 19 + .../pkg/bleve/testdata/mapping.golden.json | 677 ++++++++++++++++++ services/search/pkg/command/server.go | 11 +- services/search/pkg/opensearch/index.go | 41 +- services/search/pkg/opensearch/index_test.go | 14 + 6 files changed, 750 insertions(+), 21 deletions(-) create mode 100644 services/search/pkg/bleve/testdata/mapping.golden.json diff --git a/services/search/pkg/bleve/index.go b/services/search/pkg/bleve/index.go index 1e4e908c0f..f541b1941a 100644 --- a/services/search/pkg/bleve/index.go +++ b/services/search/pkg/bleve/index.go @@ -67,16 +67,19 @@ func NewIndex(root string) (bleve.Index, searchmapping.Classification, error) { // Safe: everything else is identical and the new fields hold no data. // Reopen so the live mapping picks the change up; otherwise the fields // get indexed dynamically and flip to breaking on the next start. + // return the classification even on errors: the mapping may already be + // persisted, so the next start classifies equal and the caller's + // warning is the only chance to surface the new fields if err := index.SetInternal([]byte("_mapping"), codeB); err != nil { _ = index.Close() - return nil, searchmapping.Classification{}, fmt.Errorf("failed to store the updated index mapping: %w", err) + return nil, classification, fmt.Errorf("failed to store the updated index mapping: %w", err) } if err := index.Close(); err != nil { - return nil, searchmapping.Classification{}, err + return nil, classification, err } index, err = bleve.OpenUsing(destination, openRuntimeConfig) if err != nil { - return nil, searchmapping.Classification{}, err + return nil, classification, err } } diff --git a/services/search/pkg/bleve/index_test.go b/services/search/pkg/bleve/index_test.go index 32d285d9f0..ae7ddf0ded 100644 --- a/services/search/pkg/bleve/index_test.go +++ b/services/search/pkg/bleve/index_test.go @@ -1,7 +1,9 @@ package bleve_test import ( + "encoding/json" "fmt" + "os" "path/filepath" bleveSearch "github.com/blevesearch/bleve/v2" @@ -179,4 +181,21 @@ var _ = Describe("NewMapping", func() { Expect(ok).To(BeTrue()) Expect(impl.Validate()).To(Succeed()) }) + + // A diff here means existing indexes will classify as breaking (schema or + // bleve marshaling changed); update the golden file only deliberately. + It("matches the committed golden mapping", func() { + m, err := bleve.NewMapping() + Expect(err).ToNot(HaveOccurred()) + b, err := json.Marshal(m) + Expect(err).ToNot(HaveOccurred()) + var got, golden map[string]any + Expect(json.Unmarshal(b, &got)).To(Succeed()) + + goldenB, err := os.ReadFile("testdata/mapping.golden.json") + Expect(err).ToNot(HaveOccurred()) + Expect(json.Unmarshal(goldenB, &golden)).To(Succeed()) + + Expect(got).To(Equal(golden)) + }) }) diff --git a/services/search/pkg/bleve/testdata/mapping.golden.json b/services/search/pkg/bleve/testdata/mapping.golden.json new file mode 100644 index 0000000000..f9c598a05b --- /dev/null +++ b/services/search/pkg/bleve/testdata/mapping.golden.json @@ -0,0 +1,677 @@ +{ + "default_mapping": { + "enabled": true, + "dynamic": true, + "properties": { + "Content": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "fulltext", + "store": true, + "index": true, + "include_term_vectors": true, + "docvalues": true + } + ] + }, + "Deleted": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "boolean", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "Favorites": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "lowercaseKeyword", + "store": true, + "index": true, + "include_term_vectors": true, + "docvalues": true + } + ] + }, + "Hidden": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "boolean", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "ID": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "MimeType": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "Mtime": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "datetime", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "Name": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "lowercaseKeyword", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "ParentID": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "Path": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "RootID": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "Size": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "Tags": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "analyzer": "lowercaseKeyword", + "store": true, + "index": true, + "include_term_vectors": true, + "docvalues": true + } + ] + }, + "Title": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "Type": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "audio": { + "enabled": true, + "dynamic": true, + "properties": { + "album": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "albumArtist": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "artist": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "bitrate": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "composers": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "copyright": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "disc": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "discCount": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "duration": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "genre": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "hasDrm": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "boolean", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "isVariableBitrate": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "boolean", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "title": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "track": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "trackCount": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "year": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + } + } + }, + "image": { + "enabled": true, + "dynamic": true, + "properties": { + "height": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "width": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + } + } + }, + "location": { + "enabled": true, + "dynamic": true, + "properties": { + "altitude": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "latitude": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "longitude": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + } + } + }, + "location_geopoint": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "geopoint", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "photo": { + "enabled": true, + "dynamic": true, + "properties": { + "cameraMake": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "cameraModel": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "text", + "store": true, + "index": true, + "include_term_vectors": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "exposureDenominator": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "exposureNumerator": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "fNumber": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "focalLength": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "iso": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "orientation": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "number", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + }, + "takenDateTime": { + "enabled": true, + "dynamic": true, + "fields": [ + { + "type": "datetime", + "store": true, + "index": true, + "include_in_all": true, + "docvalues": true + } + ] + } + } + } + } + }, + "type_field": "_type", + "default_type": "_default", + "default_analyzer": "keyword", + "default_datetime_parser": "dateTimeOptional", + "default_field": "_all", + "store_dynamic": true, + "index_dynamic": true, + "docvalues_dynamic": true, + "analysis": { + "analyzers": { + "fulltext": { + "token_filters": [ + "to_lower", + "stemmer_porter" + ], + "tokenizer": "unicode", + "type": "custom" + }, + "lowercaseKeyword": { + "token_filters": [ + "to_lower" + ], + "tokenizer": "single", + "type": "custom" + } + } + } +} diff --git a/services/search/pkg/command/server.go b/services/search/pkg/command/server.go index 4086e64cd8..59fb16befa 100644 --- a/services/search/pkg/command/server.go +++ b/services/search/pkg/command/server.go @@ -74,14 +74,15 @@ func Server(cfg *config.Config) *cobra.Command { switch cfg.Engine.Type { case "bleve": idx, classification, err := bleve.NewIndex(cfg.Engine.Bleve.Datapath) - if err != nil { - return err - } - + // warn before the error check: the new mapping may already be + // persisted, then later startups classify equal and stay silent if classification.Verdict == searchmapping.VerdictAdditive { logger.Warn(). Strs("fields", classification.NewFields). - Msgf("the bleve index at %s was built with an older schema; the new fields were added to the index schema, but documents indexed before the upgrade do not contain them and queries on these fields will miss those documents until they are re-indexed; to re-index everything run: opencloud search index --all-spaces", cfg.Engine.Bleve.Datapath) + Msgf("the bleve index at %s was built with an older schema; the new fields were added to the index schema, but documents indexed before the upgrade do not contain them and queries on these fields will miss those documents until they are re-indexed; to re-index everything run: opencloud search index --all-spaces --force-rescan", cfg.Engine.Bleve.Datapath) + } + if err != nil { + return err } defer func() { diff --git a/services/search/pkg/opensearch/index.go b/services/search/pkg/opensearch/index.go index bc7fe53f0a..a457bc446a 100644 --- a/services/search/pkg/opensearch/index.go +++ b/services/search/pkg/opensearch/index.go @@ -113,22 +113,34 @@ func (m IndexManager) Apply(ctx context.Context, name string, client *opensearch return fmt.Errorf("failed to marshal index %s: %w", name, err) } - createResp, err := client.Indices.Create(ctx, opensearchgoAPI.IndicesCreateReq{ - Index: name, - Body: bytes.NewReader(localIndexB), + // Exists first: a pre-provisioned index must not require create privileges + indicesExistsResp, err := client.Indices.Exists(ctx, opensearchgoAPI.IndicesExistsReq{ + Indices: []string{name}, }) - var createErr *opensearchgo.StructError switch { - case err == nil && createResp.Acknowledged: - return nil - case err == nil: - return fmt.Errorf("failed to create index %s: not acknowledged", name) - case !errors.As(err, &createErr) || createErr.Err.Type != "resource_already_exists_exception": - // transport errors, disk-full etc. stay plain fatal, the restart policy retries - return fmt.Errorf("failed to create index %s: %w", name, err) + case indicesExistsResp != nil && indicesExistsResp.StatusCode == 404: + createResp, createErr := client.Indices.Create(ctx, opensearchgoAPI.IndicesCreateReq{ + Index: name, + Body: bytes.NewReader(localIndexB), + }) + var structErr *opensearchgo.StructError + switch { + case createErr == nil && createResp.Acknowledged: + return nil + case createErr == nil: + return fmt.Errorf("failed to create index %s: not acknowledged", name) + case !errors.As(createErr, &structErr) || structErr.Err.Type != "resource_already_exists_exception": + // transport errors, disk-full etc. stay plain fatal, the restart policy retries + return fmt.Errorf("failed to create index %s: %w", name, createErr) + } + // lost the creation race to another instance, compare against its index + case err != nil: + return fmt.Errorf("failed to check if index %s exists: %w", name, err) + case indicesExistsResp == nil: + return fmt.Errorf("indicesExistsResp is nil for index %s", name) } - // the index already exists: compare settings and classify the mapping diff + // the index exists: compare settings and classify the mapping diff resp, err := client.Indices.Get(ctx, opensearchgoAPI.IndicesGetReq{ Indices: []string{name}, }) @@ -150,6 +162,9 @@ func (m IndexManager) Apply(ctx context.Context, name string, client *opensearch var reasons []string for k := range localIndexJson.Get("settings").Map() { + if k == "number_of_replicas" { + continue // runtime-tunable via PUT _settings, drift needs no rebuild + } lv := localIndexJson.Get("settings." + k).Raw rv := remoteIndexJson.Get("settings.index." + k).Raw if !jsonEqual(lv, rv) { @@ -188,7 +203,7 @@ func (m IndexManager) Apply(ctx context.Context, name string, client *opensearch return fmt.Errorf("failed to update mapping of index %s: not acknowledged", name) } - logger.Warn().Strs("fields", classification.NewFields).Str("index", name).Msg("extended the search index mapping with new fields; documents indexed before the upgrade do not contain them and queries on these fields will miss those documents until they are re-indexed; to re-index everything run: opencloud search index --all-spaces") + logger.Warn().Strs("fields", classification.NewFields).Str("index", name).Msg("extended the search index mapping with new fields; documents indexed before the upgrade do not contain them and queries on these fields will miss those documents until they are re-indexed; to re-index everything run: opencloud search index --all-spaces --force-rescan") return nil } diff --git a/services/search/pkg/opensearch/index_test.go b/services/search/pkg/opensearch/index_test.go index 34d267e288..f9ae3e3199 100644 --- a/services/search/pkg/opensearch/index_test.go +++ b/services/search/pkg/opensearch/index_test.go @@ -80,6 +80,20 @@ func TestIndexManager(t *testing.T) { require.ErrorIs(t, indexManager.Apply(t.Context(), indexName, tc.Client(), log.NopLogger()), opensearch.ErrManualActionRequired) }) + t.Run("tolerates replica drift", func(t *testing.T) { + indexManager := opensearch.IndexManagerLatest + indexName := "opencloud-test-resource" + + tc := opensearchtest.NewDefaultTestClient(t, defaultConfig.Engine.OpenSearch.Client) + tc.Require.IndicesReset([]string{indexName}) + + body, err := sjson.Set(indexManager.String(), "settings.number_of_replicas", "2") + require.NoError(t, err) + tc.Require.IndicesCreate(indexName, strings.NewReader(body)) + + require.NoError(t, indexManager.Apply(t.Context(), indexName, tc.Client(), log.NopLogger())) + }) + t.Run("is idempotent", func(t *testing.T) { indexManager := opensearch.IndexManagerLatest indexName := "opencloud-test-resource"