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.
| var findOptions = options.Clone(); | ||
| findOptions.PageLimit(1000); | ||
| // Only the outer batch read may cache results, after all item errors have been resolved. | ||
| findOptions.Cache(false).ReadCache(false).PageLimit(1000); |
There was a problem hiding this comment.
WARNING — undocumented change to default behavior (confidence: high)
This runs for all callers, not just ThrowOnMultiGetErrors(). That contradicts the PR description's statement that default non-strict behavior is unchanged.
Before this PR, for time-series and parent/child repos reaching the fallback:
GetByIdsAsync(ids, o => o.Cache())threwArgumentException("Cache key is required when enabling cache.")fromGetCachedQueryResultAsync, because the cloned options keptCache()without a key.GetByIdsAsync(ids, o => o.Cache("key"))read and wrote a query-resultFindResults<T>underkey. The outerAddDocumentsToCacheAsyncthen storedICollection<FindHit<T>>under the same key (type confusion).
Both are real bug fixes, and I agree with the change. Please:
- Update the PR description and changelog to say the fallback no longer uses caller cache settings in either mode.
- Add one non-strict regression test for
Cache()without a key onDailyLogEventRepositorythat reaches the fallback (it threw before this PR). That locks in the fix.
| findOptions.PageLimit(1000); | ||
| // Only the outer batch read may cache results, after all item errors have been resolved. | ||
| findOptions.Cache(false).ReadCache(false).PageLimit(1000); | ||
| var response = await FindAsync(NewQuery().Id(itemsToFind.Select(id => id.Value)!), findOptions).AnyContext(); |
There was a problem hiding this comment.
WARNING — strict mode doesn't cover fallback-only reads (confidence: ~80%)
Strict mode only checks errors collected from MGET. Two paths can still return a false "missing" with no exception:
- Parent/child IDs without routing are excluded from
itemsForMultiGet(L173) and are only resolved here.ThrowOnMultiGetErrors()has no effect on them. - Partial search failures.
_searchreturns HTTP 200 with_shards.failed > 0(withallow_partial_search_resultsdefaulting to true) ortimed_out: true. I found noShards/TimedOutchecks anywhere insrc/, soFindAsynctreats partial results as complete. IDs that MGET reported asfound:false, or that never went through MGET, are then returned as missing and cached as not-found markers.
For callers using strict mode to avoid deleting on a false miss, (1) is the same data-loss path #313 describes.
Suggestion: when ShouldThrowOnMultiGetErrors() is true, fail on shard failures or timeout in the fallback (for example, have ToFindResults expose shard failures or check the raw response). Alternatively, state explicitly in the XML docs and guides that strict mode does not apply to parent/child reads without routing. A scripted-transport test with a _shards.failed: 1 fallback response would cover it.
| if (itemErrors.Count > 0) | ||
| { | ||
| var stillMissingIds = new HashSet<string>(itemsToFind.Select(id => id.Value)); | ||
| var unresolvedErrors = itemErrors.Where(e => e.Id is null || stillMissingIds.Contains(e.Id.ToString())).ToList(); |
There was a problem hiding this comment.
NIT — dead branch (confidence: ~90%)
On this path GetItemErrors always receives docOperations, and it throws when error.Id doesn't match the expected operation ID, which includes a null ID. So e.Id is null can't be true here. Keeping it is harmless and fail-safe, but a short comment saying it's defensive would stop future readers from thinking null IDs are expected.
| { | ||
| var stillMissingIds = new HashSet<string>(itemsToFind.Select(id => id.Value)); | ||
| var unresolvedErrors = itemErrors.Where(e => e.Id is null || stillMissingIds.Contains(e.Id.ToString())).ToList(); | ||
| if (unresolvedErrors.Count > 0) |
There was a problem hiding this comment.
WARNING — time-series retention makes strict reads fail permanently (confidence: ~85%)
For daily or monthly repos, MGET targets ElasticIndex.GetIndex(id), which is derived from the ObjectId date. After retention deletes that physical index, every strict read of those IDs returns index_not_found_exception. The fallback correctly doesn't find the doc, so this throws on every call.
That's the right default for safety, but callers can't tell "index aged out (the doc is really gone)" from "index temporarily unavailable". A cleanup job that meets one retention-expired ID will stall forever. This is exactly what GetByIdsAsync_WithMixedTimeSeriesItemErrorAndThrowOnMultiGetErrors_ThrowsDocumentException asserts.
Options:
- expose the per-item errors on the exception (see the next comment) so callers can apply their own retention policy, or
- document this outcome explicitly in the configuration guide's strict section, with a recommended caller pattern.
| if (unresolvedErrors.Count > 0) | ||
| { | ||
| string message = String.Join("; ", unresolvedErrors.Select(e => $"id={e.Id}, index={e.Index}, type={e.Error?.Type}, reason={e.Error?.Reason}")); | ||
| throw new DocumentException($"Error getting documents: {message}"); |
There was a problem hiding this comment.
WARNING — no structured error data, and the message has no size limit (confidence: high)
- Callers can only find the failed IDs by parsing the message string. For a safety API, the usual approach is a typed payload. For example, a
MultiGetItemsException : DocumentExceptionexposingIReadOnlyCollection<MultiGetError>orIReadOnlyCollection<string> FailedIds, or at minimumexception.Data["FailedIds"]. Deriving fromDocumentExceptionkeeps it backward compatible. - With a large batch (
PageLimit(1000)fallback; callers commonly pass hundreds or thousands of IDs), a cluster-wide outage produces one very large message. That message is logged and may surface in APIs. Consider capping it (for example, the first N entries plus"and M more") and grouping byindexandtype.
The reason text also contains internal index names. That's fine for logs, but worth noting for consumers that surface exception messages to end users.
| /// </summary> | ||
| /// <exception cref="ArgumentNullException">The response is null.</exception> | ||
| /// <exception cref="DocumentException">The response contains an invalid item.</exception> | ||
| public static IReadOnlyCollection<MultiGetError> GetItemErrors<T>(this MultiGetResponse<T> response, ILogger? logger = null) where T : class |
There was a problem hiding this comment.
NIT — public API surface (confidence: ~70%)
This public overload skips the count and ID correlation checks (operations == null), so it gives weaker guarantees than the path GetByIdsAsync actually uses. It is also a Get* method that throws DocumentException on malformed input. Unless there's a known external consumer (Exceptionless#2267?), consider making it internal and adding it later on request. Once shipped, removing it is a breaking change.
| if (!String.Equals(result.Id, expectedId, StringComparison.Ordinal)) | ||
| throw new DocumentException($"Elasticsearch returned an unexpected multi-get result ID; expected {expectedId}, received {result.Id}."); | ||
|
|
||
| if (result.Found && result.Source is null) |
There was a problem hiding this comment.
WARNING — found:true with no _source is valid in Elasticsearch (confidence: ~75%)
Elasticsearch returns found items without _source when the mapping has _source.enabled: false, when the request uses _source: false, or when it uses stored_fields only. ConfigureMultiGetRequest is protected virtual, so subclasses can configure any of these. For those repositories strict mode would throw on every found document.
The default path can't return such documents anyway (FindHit with a null document is dropped later), so rejecting them is defensible. But the message should say why, and the guide should list it as an unsupported configuration. Otherwise it reads like a cluster fault. DoesNotRequireProjectedSourceIdAsync covers _source: {} but not an absent _source.
| </PropertyGroup> | ||
|
|
||
| <!-- Standalone and design-time builds may not provide a solution name. --> | ||
| <PropertyGroup Condition="'$(SolutionName)' != '' And '$(SolutionName)' != '*Undefined*' And ($(SolutionName.Contains('All')) Or !$(SolutionName.Contains('Foundatio')))"> |
There was a problem hiding this comment.
NIT — behavior change for single-project builds in external workspaces (confidence: ~70%)
Before, SolutionName empty made !''.Contains('Foundatio') true, so standalone builds enabled source references. Now they use packages. That's the stated fix, and the scenario matrix covers it. The side effect: a developer in the combined or Exceptionless workspace who runs dotnet build path/to/Project.csproj (no solution) now silently resolves Foundatio.* packages instead of sibling sources. That can mask local source edits during debugging. Worth one line in the contributor docs: pass -p:ReferenceFoundatioSource=true for single-project builds.
PR Review Summary
Suggested pre-merge plan
|
Summary
Fixes #313. Fixes #328.
Adds opt-in
ThrowOnMultiGetErrors()handling toGetByIdsAsync. Strict mode rejects incomplete or mismatched MGET response items before fallback or document-result caching. Valid item errors produce one aggregatedDocumentExceptiononly if they remain unresolved after any applicable time-series/parent-child search fallback. Ordinaryfound: falseresults and default non-strict behavior remain unchanged.Final-audit fixes
1. Deterministic delete-by-query tests
Replace the four timing-dependent conflict tests from #291 and both background-writer helpers with ten scripted-transport cases and two controlled real-Elasticsearch cases. Retain ordinary live-server deletion coverage.
Scripted cases verify cumulative deletion counts, exact
Retry(n)budgets/defaults, stopping on success, exhausted-budget warnings, one-time/disabled notifications, and request-level failures. Their real client transport supports product verification and synchronous mapping lookup and rejects unexpected requests.Live cases disable scheduled refresh and introduce known unrefreshed updates. A probe must report three version conflicts and zero deletions before the operation under test.
Retry(0)then returns seven deletions with three remaining;Retry(1)returns ten with none remaining. Index settings are restored infinallywith a separate bounded cleanup token.Production delete-by-query logic and retry budgets are unchanged. No background writers, artificial scheduling delays, weakened assertions, skipped tests, or analyzer suppressions are used to obtain a passing run.
2. Strict MGET validation and cache isolation
Validate response item count and metadata IDs against the actual MGET operations in request order. Reject null items, mismatched IDs (including unexpected duplicates), found results without a source, missing results with a source, and malformed error payloads. Seventeen new regression cases cover malformed responses, projections, cache bypass/preservation, and strict/default behavior. Source projections may omit their ID property: correlation uses response metadata.
Preserve fallback consistency propagation from #282 by cloning caller options, then disable internal query-result cache reads/writes with
Cache(false).ReadCache(false). The outer batch read owns document/not-found caching after validation. AddReadCache(bool)while retaining the parameterless API and independent write settings. The inline cache-isolation review finding is resolved with implementation and test evidence.3. Documentation
Align caching/configuration/CRUD/troubleshooting guides, XML comments, and agent references. Correct the caching configuration constructor example. Explain cache ownership, explicit read/write overrides, fallback behavior, and the limits of strict mode.
Additional blockers repaired
uuidentry from 11.1.0 to 11.1.1. No manifest, dependency-major, or unrelated lockfile changes. Verification 35783111978 passed clean installation, zero reported npm vulnerabilities, twelve UUID buffer-boundary assertions, and the documentation build. The final-head audit independently repeated installation, the zero-vulnerability audit, and site build.SolutionNameinference enabled nonexistent sibling project references when the solution name was empty or*Undefined*. Restrict inference to named workspaces and preserve explicit per-dependency overrides. Addbuild/VerifySourceReferences.projwith twelve cases and a permanent read-only CI job. The old configuration demonstrably fails the standalone regression; all repaired cases pass. Standalone provider/test builds work, and the formatter now loads the complete workspace without missing-reference warnings.System.IO.Hashingreference and serial xUnit test-assembly settings.Contract and compatibility
Strict mode is not a freshness guarantee, an atomic snapshot, or proof of absence. Existing positive cache hits remain eligible unless bypassed. Search fallback preserves consistency and soft-delete settings but is not real-time MGET. Never convert
DocumentExceptioninto an empty batch for an irreversible decision. The no-fallback, cache-bypassing adapter in exceptionless/Exceptionless#2267 is not a presumed drop-in replacement candidate.No additional unmerged PR is required: #282 and #291 are already included through
main. Standalone builds use packages when no solution name is available; existing named combined/external workspace defaults and explicit source-reference overrides are retained.Delivered revision
All three audit fixes and both additional repairs are on this PR branch at
561ff3b26d76221e76891976434444518ace51fa, delivered by non-force fast-forwards. A fresh comparison againstmain(a61d541613528428cc00bc2236602db72948a924) reports 20 ahead, 0 behind. No force-push or PR merge was performed.Final repair commits:
62ceb3e909a01ce5829a37b7400627b9235ffbe718b92b5c83f422e97febad7561ff3bTemporary audit-control workflows/scripts remain outside this PR. The only new permanent CI job verifies source-reference settings; the existing build/publishing job is unchanged.
Verified results — September 22, 2026
Exact final-source audit 35784354103 completed successfully. The artifact records source SHA
561ff3b26d76221e76891976434444518ace51fa, distinct from the audit-control workflow SHA.Additional stress and mutation evidence from audit 35779319164, on the unchanged runtime/test implementation at
b92b5c8:The VitePress large-chunk notice is a documentation asset-size advisory, not a compiler or test failure; its warning threshold was not raised. The custom audits created packages without publishing them. No existing publishing behavior was removed or disabled in this PR.
Merge status
The normal exact-head PR Build run 35784695259 has passed the new source-reference-settings job. Its existing
build / buildjob is still queued for a GitHub runner at this update. The exact-source audit above has fully passed, but it is not being represented as completion of that separate required check. Do not bypass the repository's merge checks. The PR remains unmerged.CI commands:
dotnet msbuild build/VerifySourceReferences.proj -target:Verify -verbosity:minimal dotnet build Foundatio.Repositories.slnx --configuration Release dotnet test --solution Foundatio.Repositories.slnx --configuration Release --no-build --report-github