Conversation
![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
Sort on the resolved id sort field
When id is mapped as a text field with a keyword/sort sub-field and HasSortableIdField remains true, IdTiebreakerField computes the sort-safe idSortFieldName, but this appends the raw resolved idField instead. Explicit sorts are normalized through GetSortFieldName, while the automatic tiebreaker is not, so a no-explicit-sort or default search_after query can still sort on id and hit Fielddata is disabled on text fields instead of using the sortable sub-field; append the sort-safe field name here (and in the analogous SearchAfter path).
ℹ️ 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".
…#305) DefaultSortQueryBuilder/SearchAfterQueryBuilder unconditionally appended an id sort for deterministic pagination, but never verified that id was actually sortable server-side or that the model even implements IIdentity. This broke queries against externally-managed indexes (e.g. Logstash-written partitions with no id field) with "all shards failed" or "Fielddata is disabled on text fields" errors, and, more subtly, still failed after removing IIdentity from the model because GetResolvedField() falls back to the literal field name instead of null when nothing is mapped. Both builders now delegate to a shared IdTiebreakerField.TryResolve<T>() helper that skips the tiebreaker when the model doesn't implement IIdentity or when the index opts out via the new IIndex.HasSortableIdField (default true, so existing library-owned indexes are unaffected). DailyIndex/MonthlyIndex also gain an overridable GetIndexMappingFilter() so externally-managed indexes can point the mapping resolver at their real (unversioned) naming pattern, plus a warning log when no index matches the filter instead of failing silently.
2604b7f to
a641c7f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a641c7fd0a
ℹ️ 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".
Tie-break same-date candidates by highest parsed index version before falling back to ordinal name ordering, so a custom mapping filter that matches multiple versions picks v10 over v9. Pin the search-after cursor extension semantics with unit tests, document the all-null-cursor behavior, and reset the paging mode when SearchAfterPaging is disabled.
The all-null and mixed-null cursor cases are already pinned by SearchAfterQueryExtensionsTests; keep the new class focused on the null-reference, empty-array, token, and mode-reset cases it uniquely covers. Document that SearchAfter/SearchBefore always enable search-after paging, even when they clear the cursor.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f9f6f461b
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbc2462993
ℹ️ 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".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25703b89a0
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73c2be7ea4
ℹ️ 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".
Bring the PR branch up to main at a61d541. Use GitHub's conflict-free merge tree for the verified main and PR heads; retain the PR head as the first parent.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reference System.IO.Hashing directly where XxHash64 is used instead of relying on a transitive dependency. Migrate both test assemblies to xUnit's current Parallelization attribute while keeping tests sequential.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Share continuation identity across Live and PIT without changing PIT ownership. Reject reset or replaced sessions before cursor mutation and again after BeforeQuery, while preserving virtual dispatch, same-mode reapplication, and retry behavior.
niemyjski
changed the title
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This branch has not been deployed
1 participant
Fixes #305. Externally managed indexes must not receive an automatic
idsort unless their mapping guarantees a sortable ID. This PR adds an explicit opt-out and hardens Live/PIT cursor traversal, sort reversal, caching, and failure cleanup.September 22 audit status
Current PR head:
e786babd17585db1d3a1ffca4402c5d611972be5. All eight audit commits listed below are on this branch. It includes currentmain(a61d541613528428cc00bc2236602db72948a924) and is zero commits behind, without rewriting history. All seven inline review threads are resolved.On this exact head, the Release build, packaged .NET 8/10 consumers, and complete suites on both Elasticsearch 8.19.15 and 9.5.0 passed: 1,109 tests per server version, zero failures/skips. The documentation-site build, clean-workspace formatting rerun, and normal final-head GitHub checks are still queued. These remain merge gates; the PR is not being declared unconditionally production-ready. Historical results from older revisions are not current-head validation.
Feature and compatibility contract
IdTiebreakerField.TryEnsure<T>(). Models withoutIIdentitynever receive an automatic ID sort.IIndex.HasSortableIdFieldhas a default implementation returningtrue, preserving existing custom implementations. External adapters can set it tofalse; this is an explicit guarantee, not automatic server-mapping detection. Explicit caller sorts remain supported, and sortable subfields such asid.sortare used when resolved.MappingIndexPatternandGetIndexDate()together. The pattern is authoritative; malformed dated matches are ignored, newest-date/version/name ordering is deterministic, and no-match mapping fallbacks log a warning. External adapters are read-only and require the external writer to maintain the umbrella alias for unbounded/large-range queries._shard_docso backward traversal reverses the complete cursor tuple. Cursor direction must be exclusive and cardinality must match the final sort.object[]?cursor return annotations are preserved. Raw null/empty/all-null arrays clear cursors; decoded tokens preserve null sort values.ReverseOrderhelper remains in-place.FindOneAsyncandCountAsyncevaluateBeforeQuerybefore cache decisions, including ordinary cache hits. Scalar PIT search-after remains unsupported.DocumentException. Normal missing-index reads retain empty-result behavior. Failed cursor searches do not become successful terminal pages; cleanup uses the latest PIT ID and never masks the original exception. Caller-owned PITs are not closed automatically.Correctness fixes completed by this audit
Live continuation identity: disabling paging, changing modes, or disabling/re-enabling the same mode invalidates old pages before cursor/page mutation and again after
BeforeQuery.AfterQueryresets also invalidate the returned continuation. Reapplying the current mode preserves the session. VirtualFindAsAsyncdispatch and retry behavior are retained. The outstanding inline review thread is addressed and resolved with regression evidence.Scalar cursor safety:
FindOneAsync,CountAsync, and query-basedExistsAsyncrequest complete Live responses, reject HTTP 200 timeouts/shard failures before returning results, and reject Live+snapshot/async combinations. Existing PIT restrictions remain intact.Owned-PIT cleanup after successful hooks: when a new search's
BeforeQueryresets an existing repository-owned PIT, the abandoned PIT is closed before its state reference is lost. Caller ownership, explicit ownership handoffs, and a replacement session are preserved.Documentation and build: public XML docs, consumer guides, and agent references describe the same behavior. Main-branch dependency upgrade compilation failures are repaired without suppressing warnings or disabling tests.
A session is not a concurrency primitive. Use separate command options/results for independent traversals, await each next page, and keep the query, sorts, and target set unchanged. Live paging is not a snapshot. Explicitly close PITs when abandoning a traversal early; synchronous option-reset methods cannot close a server resource by themselves.
Logical commits
e7fcd142mainwithout rewriting PR history.9ed59304System.IO.Hashingdirectly and migrate both test assemblies to current xUnit parallelization attributes, retaining serial execution.629b706202f88d3671e47fcaad687331380d4601BeforeQueryresets.e786babdValidation evidence
Isolated audit 35765843225 builds the exact commit now on the PR, not the temporary audit branch's source tree. Both server identities and the XML reports were checked independently after downloading the artifacts.
IIndeximplementation, and nullable cursor assignments.git diff --check; clean candidate working treeaa2972336d9947b574465de76f844ec3410a25bematches the tested source.65 new regression cases: 21 Live-session, 36 scalar, and 8 successful-hook ownership cases. Pre-fix proof: Live 16 expected failures / 84 passes; scalar 36 expected failures / 100 passes; owned-PIT cleanup 3 expected failures / 141 passes. All corresponding final tests pass. XML reports and logs are retained in the audit artifacts. The Live pre-fix proof is in 35751059940, baseline job.
The PR contains no
.auditfiles and no.githubchanges. Existing Feedz/GitHub prerelease publishing and tagged release configuration remain unchanged. Temporary runner-local package creation is validation, not a release publication. No PR merge, release tag, or deployment was performed. Temporary verification branches remain isolated while their remaining jobs finish; they are not part of the PR's ancestry.Related-PR reconciliation
The review traced default-ID sorting to #203, the client migration to #216, and unstable Live-sort warnings to #301. Relevant compatibility, sort, and once-per-session warning behavior is retained.
#307: reconcile
DailyIndex.GetLatestIndexMapping()deliberately. Preserve this PR's authoritative external pattern/date hooks and deterministic valid-partition selection, together with #307's authenticated compatibility-wrapper/canonical-alias ownership, open/hidden discovery, and owned-error-partition exclusion. Historical combined-snapshot results do not validate today's branches.#322: reconcile its async resolver with this PR's copy-before-reversal behavior. At
6f74002bf29df9a6ac3f929c78208acecb519c83,ResolveFieldSortAsyncreturns non-field sorts by reference. Combining that implementation unchanged with backward reversal would reintroduce caller-object mutation. Preserve copies for score, document, geographic, and script variants as well asFieldSort.Format, and adapt ID-tiebreaker resolution to the async path. Re-run combined tests after reconciliation; this audit does not claim a current combined-branch pass.Final exact-head checks, maintainer approval, and merge remain separate gates.