Fixes #33587: let hybrid ranking ride along with the query instead of a cluster-global pipeline - #33588
Conversation
… a cluster-global pipeline Creating the hybrid-rrf search pipeline needs cluster:admin/search/pipeline/put, which an index-scoped search role does not have and which the security plugin cannot pattern-scope. On every deployment whose role is confined to its own <clusterAlias>* prefix the PUT 403s. The reindex survives (the orchestrator warns and continues) but the pipeline is never created, so hybrid queries name a pipeline that does not exist and ranking silently degrades — the only signal being one preflight WARN. Two more things were wrong with the stored pipeline even where it is permitted. A search pipeline is cluster-global and hybrid-rrf is a hardcoded name, while its weights come from per-deployment search settings: on a shared cluster each tenant's reindex overwrote the previous tenant's ranking weights. Granting the privilege would have made that worse rather than better. Expose the pipeline as a definition rather than only as a stored object: - buildHybridRrfPipelineDefinition(kw, sem) is now the single source of truth for the RRF body, used both by the PUT and by callers that inline it. A test pins the two byte-identical so the stored and inline forms cannot drift into ranking differently. - SearchRepository.getHybridRrfPipelineDefinition() resolves the effective weights per call and hands back the body to inline. Resolving per call is also why an admin's weight change now applies to the next query instead of waiting for a reindex to re-PUT. OpenSearch builds ad-hoc pipelines through the same processor factories as stored ones — resolvePipeline passes phaseInjectorProcessorFactories, and RRFProcessorFactory ignores the pipeline source — so score-ranker-processor behaves identically inline. Also stop failing the search-settings save when the pipeline refresh fails. SystemResource turned that 403 into a SystemSettingsException, which made search settings unsaveable on exactly the deployments this affects. The weights being saved are the ones hybrid search reads, so they apply either way. The PUT stays as a best-effort legacy path for deployments still reading through ?search_pipeline=hybrid-rrf; it can be removed once no consumer names the stored pipeline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
backend
safe to test
Code Review ✅ Approved🟡 Medium risk Moves hybrid RRF pipeline definition inline with search requests instead of storing it globally, eliminating permission issues on role-scoped deployments and preventing weight conflicts on shared clusters. The inline and stored bodies are byte-identical by test, weights resolve per query to reflect current settings, and the stored PUT remains as a legacy path. No issues found. OptionsDisplay: compact → Counting what did not apply, without listing it. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } catch (Exception e) { | ||
| LOG.error("Failed to update hybrid search pipeline", e); | ||
| throw new SystemSettingsException( | ||
| "Failed to update hybrid search pipeline: " + e.getMessage()); | ||
| // Refreshing the named pipeline is a cluster-scoped write, so it fails outright on a | ||
| // deployment whose search role is confined to its own <clusterAlias>* prefix. Failing | ||
| // the whole settings save on that made search settings unsaveable there. The weights | ||
| // being saved here are the ones hybrid search reads per query, so they take effect | ||
| // regardless of whether the stored pipeline could be refreshed. | ||
| LOG.warn( | ||
| "Saved search settings but could not refresh the named hybrid search pipeline; " | ||
| + "the new weights still apply to hybrid queries", | ||
| e); | ||
| } |
There was a problem hiding this comment.
The added test only compares the extracted definition with the stored PUT body. It does not cover the new behavior where a pipeline refresh fails but the settings update still succeeds, nor does it verify that getHybridRrfPipelineDefinition() uses the configured weights. Without direct tests for these central behaviors, either part of this fix could regress unnoticed.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
✅ Playwright Results — workflow succeededValidated commit ✅ 4471 passed · ❌ 0 failed · 🟡 10 flaky · ⏭️ 1 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 57m 18s ⏱️ Max setup 4m 10s · max shard execution 20m 44s · max shard-job elapsed before upload 24m 8s · reporting 22s 🌐 220.09 requests/attempt · 2.23 app boots/UI scenario · 37.06% common-shard skew Optimization targets still in progress:
🟡 10 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
1 participant
Describe your changes:
Fixes #33587
Creating the
hybrid-rrfsearch pipeline needscluster:admin/search/pipeline/put, which an index-scoped search role does not have and which the security plugin cannot pattern-scope. On every deployment whose role is confined to its own<clusterAlias>*prefix the PUT 403s. The reindex survives —ReindexingOrchestratorwarns and continues — but the pipeline is never created, so Collate's hybrid queries name a pipeline that does not exist and ranking silently degrades. The only signal is one preflight WARN.Two further problems with the stored pipeline even where it is permitted: a search pipeline is a cluster-global object and
hybrid-rrfis a hardcoded name, while its weights come from per-deployment search settings. On a shared cluster each tenant's reindex overwrote the previous tenant's ranking weights. Granting the privilege would have made that worse, not better.What changed
OpenSearchVectorService.buildHybridRrfPipelineDefinition(kw, sem)— the RRF body is now a value, used both by the PUT and by callers that inline it into a search request'ssearch_pipelinefield. A test pins the two byte-identical, so stored and inline deployments cannot drift into ranking differently.SearchRepository.getHybridRrfPipelineDefinition()— resolves the effective weights (search settings, falling back to config) per call and returns the body to inline. Resolving per call is also why an admin's weight change now applies to the next query instead of waiting for a reindex to re-PUT.SystemResource— stop turning a failed pipeline refresh into aSystemSettingsException. That made search settings unsaveable on exactly the deployments this issue affects. The weights being saved are the ones hybrid search reads, so they take effect either way.The PUT stays as a best-effort legacy path for anything still reading through
?search_pipeline=hybrid-rrf; it can be deleted once no consumer names the stored pipeline.Why inline is safe
OpenSearch builds ad-hoc pipelines through the same processor factories as stored ones —
SearchPipelineService.resolvePipelinepassesphaseInjectorProcessorFactoriestoPipelineWithMetrics.create, andRRFProcessorFactoryignores the pipeline source — soscore-ranker-processorbehaves identically inline. Note OpenSearch rejects a request that both names a stored pipeline and carries an inline one, so consumers must send exactly one.Companion change
This PR only exposes the definition. The consumer switch is in Collate (
HybridSearchService), which is what actually restores hybrid search; that PR is blocked on this one merging.Type of change:
High-level design:
N/A — small change.
Tests:
Use cases covered
Unit tests
OpenSearchVectorServiceTest.testInlinePipelineDefinitionIsByteIdenticalToTheStoredPipelineBody— captures the real PUT body and asserts it equalsbuildHybridRrfPipelineDefinition(0.4, 0.6). The pre-existing pipeline-shape tests (weights,rank_constant, noresponse_processors/collapse) still pass unchanged, which is the regression guard on the extraction.Result:
Tests run: 173, Failures: 0, Errors: 0across the touched search suites.Backend integration tests
SystemResourcebehaviour change is failure-path only).Ingestion integration tests
Playwright (UI) tests
Manual testing performed
Verified the inline path against upstream source rather than a live restricted cluster:
SearchPipelineService.resolvePipelinebuilds ad-hoc pipelines with the full factory set including phase-results processors, and rejects requests carrying both a named and an inline pipelineRRFProcessorFactory.createplaces no constraint onProcessor.PipelineContext/ pipeline sourceUI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.🤖 Generated with Claude Code
The implementation appears safe to merge, with non-blocking but important regression-test gaps around the settings-save recovery and effective-weight lookup.
Findings
Summary
This PR extracts the OpenSearch RRF pipeline document for request-level use, resolves effective hybrid weights per call, and makes legacy named-pipeline refresh failures non-fatal when saving search settings.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Admin saves search settings] --> B[Attempt legacy named-pipeline PUT] B -->|Success| C[Persist settings] B -->|Failure: permission or cluster error| D[Warn] D --> C C --> E[Next hybrid query] E --> F[Resolve settings or config weights] F --> G[Build request-level RRF pipeline] G --> H[OpenSearch executes hybrid ranking]Reviews (1) · Last reviewed commit: "Fixes #33587: let hybrid ranking ride al..."