Conversation
|
@codex review |
![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: 8bf2e76007
ℹ️ 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. |
ejsmith
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e58c1a208
ℹ️ 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 |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@ejsmith is any work needed here in regards to FoundatioFx/Foundatio.Parsers#253 being merged |
Reference System.IO.Hashing directly where ElasticConfiguration uses it instead of relying on a transitive dependency. Migrate both test assemblies to xUnit's Parallelization attribute while retaining serialized test execution.
…ution Both regression cases fail on the parent commit because CreateFieldSort drops Format. Copy it alongside the other settings so date sort values retain the caller's requested representation, including for search-after cursors.
…ticsearch The suite asserts node-wide scroll counts, so do not start unrelated Kibana background work on its test cluster. Wait for the Elasticsearch health check and propagate startup failures rather than inheriting the masked compose exit.
Document snapshot reload/cancellation behavior consistently across the guide and agent references. Replace the nonexistent force parameter with explicit index selection and explain queue-independent configuration. Remove the incorrect claim that a no-op script backfills newly mapped fields.
niemyjski
left a comment
niemyjski
left a comment
There was a problem hiding this comment.
Production-readiness audit — September 22, 2026
Pushed six focused commits on this PR branch; no force-push or changes to main:
| Commit | Change |
|---|---|
| 57747cf | Merge current main without rewriting history |
| 8e18177 | Add the directly used System.IO.Hashing dependency and migrate both test assemblies to the supported xUnit parallelization attribute, preserving serialized execution |
| f453ab7 | Add synchronous/asynchronous sort-preservation regressions |
| 6995197 | Preserve FieldSort.Format when resolving field sorts |
| 839a095 | Start only Elasticsearch for integration tests, wait for health, and propagate startup failures |
| 6f74002 | Correct resolver-cache, configuration API, cancellation, and backfill documentation |
Findings addressed
- The rebased dependency graph did not compile: System.IO.Hashing was missing and both test assemblies used an obsolete xUnit attribute. The corrected solution builds with zero warnings and zero errors.
- Both resolver paths discarded
FieldSort.Format. The two new regression cases were first observed failing in run 35742899062, specifically because Format was null. The shared copy helper now retains Format along with every other current FieldSort setting; tests also check order, non-field variants, and input preservation. - A PIT test asserted node-wide scroll counts while CI also started Kibana. CI now runs against an Elasticsearch-only cluster. No production paging code or test assertions were relaxed.
- Documentation incorrectly described a 60-second refresh timer, a nonexistent
ConfigureIndexesAsync(force: true)overload, and a write-skipping script as a backfill. Corrected the guide and agent references and distinguished server mappings, resolver invalidation, and document re-indexing.
Verification
Full hosted run 35744362455 at 839a095 passed 864/864 tests, 0 failures, 0 skipped. The Release solution build included library targets net8.0/net10.0 and sample projects; tests executed on net10.0 with the repository's Elasticsearch 9.5.0 service. This audit did not independently execute an Elasticsearch 8 server matrix or a documentation-site build. The final documentation-only head 6f74002 has a separate CI run 35747910079; its result must also be checked before merge.
Fresh comparison reports 0 commits behind main (a61d541). Both existing review threads remain resolved. I rechecked the nonblocking structured-builder paths and resolver creation/disposal ownership covered by those threads.
Related work and remaining release gate
Replying to the question about parser #253 being merged: yes, the remaining dependency work is to adopt and validate a stable parser package containing that change. FoundatioFx/Foundatio.Parsers#253 merged on September 11, but this branch still intentionally pins 8.0.3-preview.scalable-query-optimization-fix.0.24 from parser commit 1f01e52. The latest GitHub stable release verified is v8.0.2 (July 30), which predates #253. Do not downgrade to that package or remove the draft gate merely because #253 is merged.
Repositories #292 established lightweight names/aliases discovery; that contract is preserved. #308 overlaps sort-copying/paging, and #307 overlaps time-series partition discovery. Their relevant context was reviewed, but those separate feature branches were not merged into this PR. Preserve the Format fix and async resolution when integrating overlapping changes.
Release checklist: select the stable parser release containing #253, update the package pin, rerun the full package-based suite on the final merged base, and then remove the draft gate. No merge, package publication, or production deployment was performed.
This branch has not been deployed
2 participants
Summary
Daily and monthly mapping loads used synchronous Elasticsearch calls, and structured search builders resolved fields synchronously. Cold or missing-field lookups could therefore block request threads even in
FindAsync.Typed and untyped time-series resolvers now share asynchronous mapping loads and cancel them when the owning index is disposed. Sort, field-condition, include/exclude, date-range, and paging builders await mapping resolution. Async helpers preserve boosts, all field-sort settings (including
Format), and non-field sort variants. Existing synchronous helpers and protected GET/multi-GET configuration hooks remain compatible.Discovery still requests names and aliases only, then fetches one newest-partition mapping. Partition selection avoids temporary
IndexInfoobjects, and trace diagnostics are formatted only when enabled. Resolver ownership handles disposal racing first initialization without creating an unused resolver during disposal.Audit follow-up
Six focused commits bring the branch onto current
main, repair dependency/xUnit build compatibility, add red/green sort-format regressions, preserveFieldSort.Format, isolate the integration cluster from Kibana background work, and correct mapping lifecycle documentation. CI now waits for Elasticsearch health and does not mask service-start failures. No paging assertions were weakened.The guide and agent references distinguish server mapping changes, resolver invalidation, and re-indexing existing documents. They document snapshot/cooldown and caller-versus-owner cancellation behavior, replace the nonexistent
ConfigureIndexesAsync(force: true)overload with explicit index selection, and remove the claim that skipping a write backfills a mapping.Verification
6f74002bf29df9a6ac3f929c78208acecb519c83; verified 0 commits behind main ata61d541613528428cc00bc2236602db72948a924.839a095: 864 tests passed, 0 failed, 0 skipped. Release solution build: 0 warnings, 0 errors; libraries built for net8.0 and net10.0, tests executed on net10.0 using Elasticsearch 9.5.0.See the September 22 audit review for commits, findings, and the answer to the outstanding dependency question.
Related work
Parsers #253 supplies the asynchronous resolver APIs. Repositories #292 established lightweight discovery; that contract is retained. #307 and #308 overlap partition discovery and sort/paging behavior respectively; their relevant context was reviewed, but those separate feature branches were not merged into this PR.
Release gate — keep draft
Parsers #253 merged on September 11, 2026. This PR still pins
8.0.3-preview.scalable-query-optimization-fix.0.24from parser commit1f01e52for package-based builds and CI. The latest GitHub stable parser release verified is v8.0.2 (July 30), which predates #253.Before merging for release, adopt a verified stable parser package containing #253 and rerun the full package-based suite on the final merged base. Do not downgrade to v8.0.2 or remove this gate solely because the upstream PR merged. No merge, release publication, or deployment was performed.