Skip to content

fix(elasticsearch): propagate search failures with index context - #20546

Merged
JinHai-CN merged 2 commits into
infiniflow:mainfrom
quifox:fix/elasticsearch-search-errors
Oct 8, 2026
Merged

fix(elasticsearch): propagate search failures with index context#20546
JinHai-CN merged 2 commits into
infiniflow:mainfrom
quifox:fix/elasticsearch-search-errors

Conversation

@quifox

@quifox quifox commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Ordinary multi-index searches currently discard each index error and return empty or incomplete results as success. Return transport, backend, response-read and decoding errors with the failing index and preserve the underlying error for errors.Is.

Retain empty results for a well-formed index_not_found_exception response, so an unindexed tenant does not turn an empty query into an application error. Other 404 responses remain failures. The Elasticsearch search API documents unavailable-target handling; this change retains the existing missing-index empty case. Successful aggregation and request-body reuse remain covered.

Related: #19736 addresses error propagation within a broader retrieval change. This PR isolates the current searchOneIndex path and preserves empty results for missing indices; the transport/read and multi-index regression coverage can be coordinated with that work.

Validation: bash build.sh --test ./internal/engine/elasticsearch. Transport stubs cover successful and empty searches, mixed populated/missing indices, backend failures, unrelated 404s, malformed and truncated responses, and partial multi-index failures. Real Elasticsearch integration has not been run.

@coderabbitai

coderabbitai Bot commented Oct 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 6ad1818a-6fa7-46b4-934f-c21d714cdbcf
📥 Commits

Reviewing files that changed from the base of the PR and between 08600d6 and 4d15d48.

📒 Files selected for processing (2)
  • internal/engine/elasticsearch/chunk.go
  • internal/engine/elasticsearch/search_errors_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • internal/engine/elasticsearch/chunk.go
  • internal/engine/elasticsearch/search_errors_test.go

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.

📝 Walkthrough

Walkthrough

Search now returns an error when an index search fails, preserves response-body read errors, and treats an index_not_found_exception 404 as an empty result. Tests cover these cases and aggregation across successful indices.

Changes

Elasticsearch Search Error Handling

Layer / File(s) Summary
Search error handling and validation
internal/engine/elasticsearch/chunk.go, internal/engine/elasticsearch/search_errors_test.go		 Search returns an error when an index search fails. searchOneIndex returns response-body read errors and treats an index_not_found_exception 404 as an empty result. Tests cover backend, transport, read, and decode errors, missing indices, and successful result aggregation.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 4d15d

No actionable merge-blocking risk was established; the search error-handling change is ready for normal merge checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 08600

The reviewed request path retains tenant scoping and generic error responses while making incomplete searches fail explicitly. No introduced security issue was established, but broader caller exposure and production behavior remain incompletely verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • observed — An ordinary failure affects the entire requested multi-index result rather than only the failing index's contribution. The observed public dataset caller requests one tenant-derived index with a matching dataset filter; broader internal caller exposure was not exhaustively established.

Trust Boundaries and Controls

  • observed — Backend error bodies and index context can enter the returned engine error, but the observed public handler sends that error through jsonInternalError, which logs details internally and returns a generic server-error message. This blocks requester-facing disclosure on the inspected path.

Resilience and Maintainability Implications

  • observed — The empty-success exception is narrowly classified: unrelated 404s, unreadable error bodies, and malformed JSON remain failures. This preserves failure containment without treating every unavailable response as evidence of an empty tenant index.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly states that the change propagates Elasticsearch search failures and includes useful index context.
Description check ✅ Passed The description includes the required Summary section and explains the problem, intended behavior, missing-index handling, related work, and validation. It also notes that real Elasticsearch integrati…
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

A rabbit checks each index trail,
No partial search results prevail.
A missing index leaves no trace,
While read errors keep their place.
Good hits gather, neat and bright,
Then the rabbit hops from sight.

Comment @coderabbitai help to get the list of available commands.

@quifox

quifox commented Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

Added Go documentation for the search failure contracts and controlled response fixtures in 4d15d48. This update changes comments only. Formatting and diff checks passed, and the regression files remain selected by the default unit-test flags. Functional tests were not rerun for this documentation-only update.

@JinHai-CN
JinHai-CN requested a review from qinling0210 October 8, 2026 05:06
@qinling0210 qinling0210 added the ci Continue Integration label Oct 8, 2026
@JinHai-CN
JinHai-CN merged commit 7a5a337 into infiniflow:main Oct 8, 2026
11 checks passed
@Xavierando Xavierando mentioned this pull request Oct 11, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@qinling0210 qinling0210 qinling0210 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@qinling0210 qinling0210

Labels

ci Continue Integration

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@quifox @qinling0210 @JinHai-CN