Repository navigation
Conversation
Collect MGET item errors eagerly via a new GetItemErrors extension instead of threading a bool through ToFindHits. GetByIdsAsync now runs the multi-index fallback find before deciding whether to throw, so time-series/parent-child repositories that resolve a document via the fallback no longer fail under ThrowOnMultiGetErrors(). Only ids still unresolved after the fallback raise a single aggregated DocumentException, thrown before any cache write. Also documents the deferred-throw semantics in XML docs, the guide pages, and the skill reference, and switches the closed-index test to AsyncDisposableAction.
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. |
![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: 7702b9d351
ℹ️ 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".
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Final audit status — September 22, 2026The earlier exact-head run for The final audit has prepared six isolated commits covering:
The current validation run is https://github.com/FoundatioFx/Foundatio.Repositories/actions/runs/35771764019. It is queued for a hosted runner, not a passing test result. Its gates include repeated focused tests, deliberate regression mutations, controlled real-server conflicts, the complete suite, changed-file formatting, documentation build, and package creation without publishing. Two analyzer errors in the newly added integration fixture were corrected in this candidate without suppressions. These final-audit changes have not yet been promoted to the PR branch. |
sunnyrp
left a comment
sunnyrp
left a comment
There was a problem hiding this comment.
PR #314 — Fail explicitly on multi-get item errors
Verdict: REQUEST CHANGES
The core strict-MGET design holds up. Validation runs before fallback and before cache writes, positions line up with operations, and unresolved errors are aggregated after fallback. The fallback cache isolation from Codex's earlier finding is fixed and tested. Remaining items are one merge-gate blocker plus contract and release-note gaps that should be settled before this ships in a package.
Blocker
license/clais still PENDING on head561ff3b. 11 of 20 commits are authored bygithub-actions[bot], which cannot sign a CLA. This needs either a squash merge under a human author (withCo-authored-byif wanted) or an explicit maintainer CLA override. Bot authorship also weakens commit provenance for a data-safety change.
Warnings (details inline)
- Default (non-strict) behavior changes, but the PR says it doesn't.
findOptions.Cache(false).ReadCache(false)is unconditional. Before this PR, a non-strictGetByIdsAsync(ids, o => o.Cache())that reached the time-series or parent/child fallback threwArgumentException("Cache key is required..."). WithCache("key"), it wrote aFindResults<T>under a key the outer read later treats asICollection<FindHit<T>>. Both are good fixes, but they are observable behavior changes and need release notes. - Strict mode doesn't cover fallback-only reads. Parent/child IDs without routing never go through MGET. Partial shard failures in the fallback search (HTTP 200 with
_shards.failed > 0) are also not surfaced. In both cases strict mode silently returns "missing". That contradicts #313's stated contract: every item is either found or explicitlyfound:false. - Time-series retention:
index_not_found_exceptionnever resolves. A daily index removed by retention makes strict reads of its IDs throw forever. Callers can't tell "aged out" from "unavailable", so cleanup jobs can get permanently stuck. DocumentExceptioncarries no structured failure data, and the message has no size limit. Callers would have to parse strings to find which IDs failed. With a 1,000-ID batch, the message can be very large, and it gets logged and propagated.- Strict validation rejects
found:truewith no_source. That breaks strict mode for indexes with_sourcedisabled, or for subclasses whoseConfigureMultiGetRequestusesstored_fields/_source: false. - Scope is broader than the minimal fix. One PR bundles strict MGET, the new
ReadCache(bool)API, the #328 test rewrite,common.propsinference changes plus a new CI job, a docs lockfile security patch, theSystem.IO.Hashingdirect reference, and an xUnit attribute migration. Consider splitting, or at least squash-merging with a structured message and per-area release notes.
Nits: a public GetItemErrors overload that skips correlation validation; a dead e.Id is null branch; common.props now uses packages for single-project builds in external workspaces; the docs rewrite changes the documented meaning of Cache(false).
Security | Build | Dependencies | Accessibility | Requirements | Code Review | Data-loss analysis
Security: PASS
No malicious scripts, credential access, or CI tampering. The new source-reference-settings job has contents: read and only runs dotnet msbuild on an in-repo project. Tag-pinned actions match the repo's existing workflows.
Build: CI PASS | Local: NOT RUN
CI is green at 561ff3b: build / build, source-reference-settings, and CodeQL (actions and csharp). The PR reports 887 tests passing. I could not build or test locally during this review, so the test results come only from CI.
Dependencies: AUDITED
System.IO.Hashing 10.0.12(direct): justified, becauseElasticConfiguration.csimportsSystem.IO.Hashingdirectly. It is still a new direct dependency for NuGet consumers and should be listed in release notes.docs/package-lock.jsonuuid11.1.0 → 11.1.1: lockfile only, docs site only, not shipped in packages.
Accessibility: N/A (no UI)
Requirements (#313, #328): MOSTLY ALIGNED
- #313: opt-in strict mode ✅,
found:falsestays non-error ✅, no not-found markers after item errors ✅, closed-index integration test ✅, mixed success/error test ✅. Fallback-only paths are not covered⚠️ (Warning 2). - #328: deterministic scripted transport ✅, controlled live conflicts ✅, cleanup in
finally✅, no weakened assertions ✅.
Data-loss / inconsistency scenarios
| # | Scenario | Mode | Impact | Covered by PR? |
|---|---|---|---|---|
| D1 | MGET item error (closed or unavailable index) is treated as missing, and the caller deletes "orphans" | default | Data loss (the original #313 bug) | No: the default is unchanged; callers must opt in |
| D2 | Parent/child ID without routing, fallback search hits partial shard failures, and the doc comes back missing | strict | Data loss if the caller deletes on a miss | No (Warning 2) |
| D3 | Doc is in a non-date-derived daily index, MGET on the date index returns found:false, and fallback misses it (refresh lag or shard failure) |
strict | Returned as missing and cached as a not-found marker | Documented only ("not proof of absence") |
| D4 | Stale positive cache hit for a doc deleted elsewhere | strict + cache | Returns a deleted doc: stale reads and authorization risk. It is the safe direction for cleanup | Documented; bypass with Cache(false).ReadCache(false) |
| D5 | Read-then-delete race (TOCTOU) | both | Concurrent writes between validation and mutation | Documented; needs caller-side versioning |
| D6 | Daily index removed by retention, so strict reads of its IDs always throw | strict | Liveness, not loss: jobs stall | No (Warning 3) |
| D7 | Default mode with ReadCache(false) and Cache(): an item-errored ID's existing positive entry is overwritten by a not-found marker |
default | Cache churn only. Null markers are filtered on read, so no false "missing" is served | Pre-existing |
| D8 | Caller catches DocumentException and continues with an empty batch |
strict | Recreates D1 | Documented warning only; structured errors would help (Warning 4) |
Existing comments
- Codex P2 (fallback cache isolation): verified fixed at
561ff3bL210, with testsUnresolvedItemErrors_DoNotReadOrWriteFallbackCacheandRecoveredItem_PreservesCallerCachingAndConsistency. I replied in the thread with one new observation (Warning 1). I did not resolve the thread because it contains a human reply.
PR Review Summary
Suggested pre-merge plan
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4f86571d7
ℹ️ 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".
Adds opt-in
ThrowOnMultiGetErrors()so incomplete batch reads fail before document/not-found cache writes. Strict reads validate MGET correlation and reject unresolved item errors, fallback timeouts/shard failures, HTTP 404, and unfinished async results. Fallback cache isolation fixes keyed and unkeyed caching in both modes while preserving consistency and soft-delete options. Fixes #313 and #328.Validation: seven expected regression failures before the latest fixes; final local Release solution build has zero warnings/errors, 113 focused transport tests and 159 core tests pass. Previous head passed all 963 hosted tests; latest-head CI must pass separately.
Remaining merge gate:
license/clapending. No package publication, release or PR merge is requested.Verification and implementation details
Unresolved item failures retain the exact
DocumentExceptiontype.Data["MultiGetErrors"]exposes anIReadOnlyCollection<Elastic.Clients.Elasticsearch.Core.MGet.MultiGetError>backed by a read-only collection of full unresolved errors; recovered errors are excluded. Error objects are not deep-immutable. Display text is constructed within a 4096-character bound with an omission notice, preserving small-message formatting. Other validation exceptions need not include this key.Strict mode remains opt-in. Ordinary missing documents remain ordinary misses; cache freshness and atomicity are not guaranteed. Retention-expired indexes can still throw, and found documents require
_source; projections may omit the model ID. Default async/search behavior remains permissive. Query-result caching is disabled only inside the batch fallback, with outer document caching occurring after validation.The PR also replaces timing-dependent delete-by-query tests with deterministic transport and controlled integration coverage, adds
ReadCache(bool)while retaining its parameterless overload, verifies standalone source-reference settings, and updates the direct System.IO.Hashing dependency and documentation. Production delete-by-query retry budgets are unchanged.Build workflow routing runs unrestricted pull-request validation while restricting push runs to main/master, numeric maintenance branches and v-prefixed tags. Feature branches without PRs no longer run Build, and conflicted PRs need reconciliation before PR CI. Shared workflow jobs/settings remain unchanged; this feature branch's PR validation skips package and release steps.