feat: warn when Live search_after paging uses an unstable sort key#301
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a guardrail to prevent silent data loss when using Elasticsearch search_after deep pagination in Live mode with unstable sort keys (_doc, _score). It introduces a warning at query-build time, wires logging into the query builder via DI, adds tests for the warning behavior, and documents the gotcha for consumers.
Changes:
- Log a warning when Live-mode
SearchAfterPaging()is used with an unstable sort key (_doc/_score), while avoiding warnings in Point-In-Time mode. - Plumb
ILoggerFactoryintoElasticQueryBuilder(andIndex.CreateQueryBuilder) to create theSearchAfterQueryBuilderlogger once. - Add test coverage and documentation clarifying safe/unsafe
search_aftersorting patterns.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Foundatio.Repositories.Elasticsearch.Tests/ReadOnlyRepositoryTests.cs | Adds tests asserting warning/no-warning behavior for Live vs PIT and stable vs unstable sorts. |
| src/Foundatio.Repositories.Elasticsearch/Queries/Builders/SearchAfterQueryBuilder.cs | Implements detection of unstable sort keys in Live search_after paging and logs a warning. |
| src/Foundatio.Repositories.Elasticsearch/Queries/Builders/ElasticQueryBuilder.cs | Adds optional ILoggerFactory support and wires a logger into SearchAfterQueryBuilder during default registration. |
| src/Foundatio.Repositories.Elasticsearch/Configuration/Index.cs | Passes Configuration.LoggerFactory into ElasticQueryBuilder so repository-built query builders can log. |
| docs/guide/querying.md | Updates examples to strongly-typed sorts and documents the _doc/_score + Live search_after pitfall with guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47b0c76f2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
47b0c76 to
284b392
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 284b392d8a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
284b392 to
18383d1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18383d1cef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
18383d1 to
4a0530d
Compare
Sorting by _doc or _score with Live-mode search_after paging is only safe within a Point-In-Time snapshot: index refreshes and segment merges can renumber/reorder these keys mid-paging, silently invalidating the cursor so a subsequent page can come back empty while documents remain (root cause of OUT-17644, where a read-modify-write job paged and wrote the same index using sort: "_doc" and lost ~63k of 73k documents with no error). SearchAfterQueryBuilder.BuildAsync now resolves each sort field once per query build to check both the id-tiebreaker and for these unstable sort keys, logging a warning when found in Live mode (no warning in PointInTime mode, where these keys are stable). The logger is now created once via constructor injection instead of being fetched per query build: ElasticQueryBuilder accepts an optional ILoggerFactory and constructs SearchAfterQueryBuilder with a real logger during RegisterDefaults(); Index.CreateQueryBuilder passes Configuration.LoggerFactory through. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add a warning callout to the Search After Paging section covering the unstable _doc/_score sort key footgun, using strongly-typed sort expressions in the examples and linking to Elasticsearch's own paginate-search-results docs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
4a0530d to
4c195c7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c195c7b1b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| if (warnOnUnstableSort && !alreadyWarned && UnstableSortFields.Contains(fieldName)) | ||
| { | ||
| var logger = (ctx.Options.GetElasticIndex() as IHaveLogger)?.Logger ?? NullLogger.Instance; |
There was a problem hiding this comment.
When a repository is configured with a custom IIndex implementation rather than the built-in Index base class, this cast fails and the warning is sent to NullLogger, even though IIndex already exposes Configuration.LoggerFactory. That makes the new Live search_after guardrail silently disappear for interface-based index implementations; use the interface's logger factory or inject a logger into the query builder instead of depending on IHaveLogger.
Useful? React with 👍 / 👎.
Summary
Root-causes and fixes the class of bug behind OUT-17644: a read-modify-write job paged through ~74k contacts with
sort: "_doc"andSearchAfterPaging()(Live mode), silently stopping after ~10.5k documents with no error, because_doc(Lucene's internal doc id) is only stable within a Point-In-Time -- index refreshes and segment merges renumbered it mid-run and invalidated thesearch_aftercursor.An exhaustive audit of the codebase found no built-in read-modify-write paths (
PatchAllAsync,RemoveAllAsync, etc.) using_doc/_scoresorts today -- they're all id-stable. This PR adds a guardrail so any future or caller-supplied query that does this gets a clear warning instead of a silent data-loss bug.Changes
SearchAfterQueryBuildernow logs a warning when Live-modesearch_afterpaging is combined with an unstable sort key (_docor_score). No warning inPointInTimemode, since a frozen snapshot keeps those keys stable. A single pass over the sort fields resolves each field name once to check both the id-tiebreaker and the unstable-sort condition.ILogger<SearchAfterQueryBuilder>with aNullLoggerfallback), rather than fetched inline on every query build.ElasticQueryBuildernow accepts an optionalILoggerFactoryand wires it up inRegisterDefaults();Index.CreateQueryBuilder()passesConfiguration.LoggerFactorythrough._doc/_scorein Live mode, no warning in PointInTime mode, no warning for an explicit stable sort, and no warning when no explicit sort is given (implicit id tiebreaker only).docs/guide/querying.mdwith strongly-typed sort examples and a link to Elasticsearch's paginate search results docs: https://www.elastic.co/docs/reference/elasticsearch/rest-apis/paginate-search-results#search-afterTesting
dotnet build Foundatio.Repositories.slnx-- 0 errors/warningsFoundatio.Repositories.Elasticsearch.Tests-- 684/684 passedFoundatio.Repositories.Tests-- 152/152 passeddotnet format --verify-no-changes-- cleanBuildworkflow) -- greenCo-authored-by: Copilot 223556219+Copilot@users.noreply.github.com