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
Here are some automated review suggestions for this pull request.
Reviewed commit: 76b9eaff66
ℹ️ 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".
76b9eaf to
4226b37
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4226b374e5
ℹ️ 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: fd97a54e66
ℹ️ 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: a664a60d98
ℹ️ 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: 92a9507c4a
ℹ️ 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: ba6c9bf746
ℹ️ 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".
This reverts commit ba6c9bf.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?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: 3fcfe71466
ℹ️ 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. |
Explain registered destination reservations and callback cancellation behavior. Correct nonexistent force-configuration and positional progress-callback examples, distinguish best-effort schema resume from compatibility recovery, and avoid a timeless preview label for the server API.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Document registered destination reservations and the actual progress-callback contract. Fix progress-callback call syntax and remove the nonexistent force configuration option. Separate stable alias routing and best-effort schema catch-up from the offline compatibility protocol, including task lifetime, interrupted copies, retained sources, and disk headroom.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Remove an unrelated constructor statement reorder from the compatibility reservation edits. Keep the review diff focused on destination validation.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
niemyjski
left a comment
niemyjski
left a comment
There was a problem hiding this comment.
Review follow-up completed
Pushed 11 commits, merged current main without rewriting history, repaired the baseline build, fixed the registered-destination P1, added 20 regression cases, and corrected the API/public/agent documentation. The destination-collision thread now has implementation and CI evidence and is resolved; a fresh thread listing shows no unresolved threads. The PR description separates current evidence from historical validation and records the #308/#327 integration risks.
Verified: Release solution build on 97a335f with 0 warnings/errors; libraries target .NET 8 and .NET 10; .NET 10 test run reports 1,107 passed / 0 failed / 1 skipped in run 35744890739. All 20 new destination cases are included. Main a61d541 is an ancestor of the head, with 0 commits behind.
Final head: 13f069f has passed its build step in run 35748460623; its test step is still running at this review. Do not substitute the preceding green run for a final-head result.
Release validation still required: the skipped test is the deliberately gated persistent Elasticsearch 7→8→9 sequence. This follow-up did not execute that chain, snapshot restoration, packaged external consumers, or combined current #307/#308/#327 tips. Re-run the persistent sequence and restoration on the intended release candidate, and revalidate any later combined-PR conflict resolution. Normal schema migration remains distinct from this offline compatibility protocol; the docs no longer promise guaranteed crash recovery or lossless concurrent catch-up.
This review does not approve, merge, publish, or certify a production deployment. The code fixes and docs are committed; final CI and the explicit release rehearsal remain gates.
Final-audit findings — validation in progressThe previously pending final-head CI on The deeper audit found additional issues, so the old green build is not final production approval:
Candidates and regression probes are isolated on For release integration, prefer landing #327's normal-schema data-loss fixes first, then reconcile #307 against that updated base. A temporary #327→#307 review stack is useful while both remain open, but merely changing the PR base is not a semantic merge or combined test. #308 should remain independently reviewable unless it gains a real dependency; its shared mapping-discovery conflict still needs an explicit resolution and fresh combined validation. No PR base was retargeted, no PR was merged, and publishing behavior on this PR is unchanged. |
Final-audit evidence updateThe new candidate now has actual regression proof from Windows run 35770100836:
The persistent rehearsal in run 35770775846 built the same candidate cleanly, preserved a real ES7 data volume through ES8, ran the explicit compatibility chain test on ES8 (1 passed, 0 skipped), and completed snapshot restoration at ES7/ES8 and alias read/write checks at ES8. The ES9 leg did not run. The node's captured fatal error identifies an unrelated ES7-created This is concrete evidence for the documented boundary: a Foundatio-owned index preflight is not cluster-wide upgrade certification. System-index readiness still belongs to Elastic's Upgrade Assistant/tooling. Artifact The fixes still need clean-source promotion and completed integration/package/chain gates. Do not merge the temporary audit branch or treat the failed/queued jobs as passed. The original #307 source head remains |
|
|
niemyjski
left a comment
niemyjski
left a comment
There was a problem hiding this comment.
Final-audit fixes are now on this PR in four clean commits: 54856100, 8915594e, 1d76e627, and 9c681f8c. The promoted diff contains only source, regression tests, and documentation; no audit workflow or publishing changes.
Verified evidence for this exact candidate: Release solution builds for net8.0/net10.0 with zero warnings/errors; 37 of 61 new regression cases fail on the original source, all 61 pass with the fixes; all 152 core tests pass; actual local NuGet consumers build with nullable warnings treated as errors and execute on .NET 8 and .NET 10. See run 35771563588 and its candidate patch/bundle artifact. The red/green/core results were also observed independently on Windows.
The corrected paths cover discarded task timeout evidence and coerced/lost counters, natural-prefix error-index provenance, task-404 cleanup authority, and original cancellation when secondary recovery fails. Existing positive task-termination fixtures now require completed:true instead of 404.
Approval remains withheld pending the recorded final-head CI, full Elasticsearch 8/9 suites, clean standalone formatting, and complete persistent-chain rehearsal. The first chain successfully migrated the real ES7-created Foundatio index on ES8 and restored snapshots, then ES9 refused an unrelated 7-created .geoip_databases system index. This is retained as evidence that cluster-wide readiness still requires Elastic's tooling; the isolated repeat disables the optional downloader from initial disposable provisioning, not on a user cluster.
Fresh base comparison: main a61d541 is already included, zero commits behind. For the combined release, prefer landing #327 first and then reconciling #307 against main. A temporary #327→#307 stack is useful for parallel review, not mandatory; retargeting alone does not resolve shared reindex semantics. Keep #308 independently reviewable unless a true dependency is introduced, and test its mapping-discovery reconciliation explicitly. No related PR was merged or retargeted.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c681f8ce1
ℹ️ 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".
| string canonicalAlias = canonicalName.ToString(); | ||
| if (!aliases.HasCanonicalCompatibilityAlias(canonicalAlias)) |
There was a problem hiding this comment.
When a registered VersionedIndex named events coexists with a registered index named events-v2, the sibling's compatibility-upgraded physical index can be reindexed-v9-events-v2 with canonical alias events-v2. Calling events.DeleteAsync() resolves that alias here, accepts the canonical marker, and deletes the sibling's physical index because sibling ownership is checked only in the unwrapped-name branch above. Fresh evidence beyond the previously fixed native-name collision is that this wrapped-name branch never calls IsNativelyClaimedByOtherConfiguredIndex(canonicalName); apply the same arbitration before accepting the canonical alias.
Useful? React with 👍 / 👎.
niemyjski
left a comment
niemyjski
left a comment
There was a problem hiding this comment.
Deeper consistency review: task absence, writer ownership, and destructive-cutover durability
Reviewed #307 at 9c681f8ce11b10509aadd2af9dcbbdcaea33200d, #327 separately at 091b2ba0a6c24806568c42897c1078f0b00dae5c, and Foundatio's actual 13.0.4 dependency tag (d6d686d31be062ba0561eb210e40ad40d79c8fff). These are source-audit findings and failure schedules, not newly executed distributed fault-injection tests. This review does not approve production release.
Confirmed: the destructive cutover does not require restored redundancy
ElasticIndexCompatibilityUpgrader.CreateTargetAsync temporarily sets replicas to zero. RestoreTargetSettingsAsync restores the configured count, but WaitForTargetHealthAsync accepts yellow, with no initializing/relocating shards, before the atomic request deletes the source. Unassigned replicas satisfy that combination. A replacement with only primary copies can therefore become the sole live dataset even when the configured policy requires replicas.
Require target-scoped health and allocated copies consistent with the restored redundancy policy before source deletion. Green is useful only together with the intended replica configuration; green with zero configured replicas is not redundancy. Explicit zero-replica deployments need an explicit risk policy. Make the health/recovery deadline configurable: an unconditional 30-second wait is not a large-index recovery budget. Test a source with replicas, a target whose replicas cannot allocate, and assert no remove_index is dispatched.
Confirmed upstream dependency gap: renewal can return normally after ownership is lost
The repository references Foundatio 13.0.4. At that exact release, CacheLockProvider.RenewAsync returns the task from ReplaceIfEqualAsync(...).AsTask() as non-generic Task, without checking the Boolean outcome. A completed compare-and-replace returning false therefore does not report loss of ownership to callers. The upgrader awaits reindexLock.RenewAsync, so a non-throwing renewal is not evidence the lease remains owned.
Fix and test this in Foundatio, then integrate the dependency deliberately. Test: owner A acquires; its key expires; B acquires; A renews; renewal must report loss and A must not authorize cutover, reset, or block removal. Separately, renewal only on progress callbacks is insufficient protection against long awaits or a stalled callback. Lease renewal must be independent, and lost ownership must revoke mutation authority. A cache lease, or an epoch stored only in a manifest, does not itself fence an already submitted Elasticsearch request.
The 404 change is correct, but proves only one part of the protocol
Pinned Elasticsearch 9.5 task lookup falls back to stored results when the owner is absent or unreachable. Thus task 404 must remain unknown rather than authorize cleanup. Explicit completion means the observed task terminated; successful migration additionally requires the validated response, stable source/target, correct identities, and durable cutover.
The next fault test must cover duplicate/delayed asynchronous submissions. RunCompatibilityReindexAsync sets X-Opaque-Id but does not locally disable inherited transport retries. An opaque ID is correlation, not deduplication. Under a retrying client/proxy, a first submission can be accepted without a received response and a second can return the task ID being tracked. Completion of that one task does not prove all submissions or stale workers are harmless. This is a conditional risk, not a claim that every default client duplicates submissions. Audit client/resilience/proxy launch retries; retain attempt identity and task lineage; do not delete/reuse a target based only on one returned task ID or an empty listing.
Elasticsearch 9.5 adds a reindex-specific cancellation API that follows node-shutdown relocations and can return final state. Consider it through a tested version capability adapter, not as an unconditional replacement on ES8/earlier ES9. Its documented 404 is still ambiguous. Reference: https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-cancel-reindex
Recovery hardening and availability design
Current public Finish recovery uses names, static workflow markers, canonical alias presence, and task absence; it does not recover a durable record of the original index UUIDs and full expected alias manifest. Under the documented exclusion of other writers/managers, this is a narrower protocol than arbitrary concurrent recovery. For stronger unattended recovery, persist source/target UUIDs, attempt identity, expected alias/mapping/settings/transform fingerprints, task lineage, verified-copy evidence, and cutover intent. Use compare-and-set transitions, but do not mistake them for server-enforced fencing of aliases or bulk writes.
A potential safe failback is to quarantine the private destination with an acknowledged dedicated write block, retain it, and resume the intact source. This is valid only if no cutover request can still execute and the attempt/stale controllers cannot later promote or unblock that target. It is NOT permission to delete an uncertain target, nor safe after ambiguous cutover dispatch. This could reduce outage on pre-cutover failures without pretending a missing task terminated, but requires its own adversarial tests before implementation.
Scope and architecture
Keep #307's block-before-copy contract as the simple strict offline mode. A live pre-copy followed by a complete blocked rescan still has O(dataset-size) outage. A genuinely short final outage requires a durable, ordered change stream including deletes, or migration of demonstrably immutable partitions. Do not import #327's unconditional source-unblock lifecycle into this protocol; that branch has separate findings.
Primary operational references: https://www.elastic.co/docs/reference/elasticsearch/index-settings/index-block ; https://www.elastic.co/docs/deploy-manage/production-guidance/optimize-performance/indexing-speed ; https://www.elastic.co/docs/manage-data/migrate/migrate-data-between-elasticsearch-clusters-with-minimal-downtime .
Consistency follow-up — tracked blockers and implementation scopeAt reviewed head Shared ownership and task authority
Compatibility replacement safety
Related #327 blockersTrack scripted delete resurrection, Large-data operating contractDocument strict offline replacement versus a separately designed snapshot/backfill + durable change-capture protocol, including tombstone retention, ordering/idempotence, capacity/backpressure, rollback boundaries, and immutable-partition options. Generic online CDC, arbitrary script reconciliation, and automatic ambiguous-task quarantine must not be represented as implemented without their own tested protocol. Implementation is being worked on in this session in isolated candidate branches with logical commits and regression tests. Exact promoted SHAs, actual test results, and remaining blockers will be recorded here. No release or merge to main is authorized by this work. |
Consistency hardening tracker — implementation and validationThis remains an open release gate, not production approval. Product baseline is #307 Implemented upstream
Implemented in the #307 candidate; full validation/promotion pending
Latest #307 validation: run Cross-PR implementation#327 candidate now implements strict routed item/scroll/bulk evidence, exact reconciliation counts, source UUID + complete primary-shard checkpoint vectors, changed-ID pagination past 1,000 results, scripted-quiescence preflight rejection, retained retired-source fences, no completion fabrication from a QuiesceSource flag, single-attempt launch/cutover and independent lease renewal. Tracker: #327 (comment) . Latest full validation is Still open — do not check off on a green ordinary suite
No production data, main-branch merge, release/publishing policy, force cleanup, history rewrite or PR base retargeting has been performed. |
Consistency remediation tracker — sequential executionThis is the master tracker for #307, #327, and the upstream lock repair. Not a production approval. Each entry is closed only with implementation and exact revision/test evidence. Existing newer work is retained rather than overwritten. Observed progress
Ordered work and remaining gates
Architecture retained: strict offline compatibility replacement stays write-block-first. Low-downtime mutable migration requires a separate durable ordered change stream covering updates/deletes/version fencing. Slicing, throttling, and historical-partition migration do not silently supply that protocol. No production cluster or package-feed configuration is changed as a shortcut; the planned CI-only replica profile leaves the development compose file and publishing configuration intact. |
Sequential step 2: new regression proves request-local retry setting is insufficientRun https://github.com/FoundatioFx/Foundatio.Repositories/actions/runs/35802285678 built the original and proposed code cleanly. All three new transport regressions (502/503/504, three-node pool, global retry limit 2) observed three POST /_reindex submissions, both before and after adding request-local The transport binding implementation computes MaxRetries from the global configuration and pool, ignoring the request-local MaxRetries value; a non-null ForceNode sets the effective limit to zero: https://github.com/elastic/elastic-transport-net/blob/78dc23fe779254d1a06ef52bef44e3941c6fb049/src/Elastic.Transport/Components/Pipeline/BoundConfiguration.cs . This turns the earlier conditional concern into an executed regression and explains why merely setting MaxRetries(0) is not adequate with this dependency. I am correcting the launch to use a single selected node for this one ambiguous side-effecting dispatch, without disabling retries for ordinary reads or changing the shared client globally. The regression gate remains unchanged and must pass before commit promotion. Persisted task intent/404 handling does not account for a duplicate launch whose task ID was never returned. |
Sequential consistency remediation — release blockingI am treating the following as ordered production gates and will close them one at a time with regression evidence before moving to the next:
Each item stays open until the implementation, red/green regression, documentation, and current-head validation agree. I will not mark #307 production-ready merely because the ordinary suite is green. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6a7ec7996
ℹ️ 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".
| catch (OperationCanceledException) | ||
| { | ||
| throw; |
There was a problem hiding this comment.
When the progress callback throws OperationCanceledException or returns a canceled task for the 5% notification, this branch propagates after the source marker and write block have been acknowledged but before CreateTargetAsync is called. Because canResetCurrentAttempt is still false and the source-only topology is classified as ManualIntervention, the source remains write-blocked and the public recovery API cannot clear it. Fresh evidence after the callback-failure fix is that the explicit cancellation branch still bypasses the exception swallowing; mark this locally proven pre-dispatch state as resettable and remove the marker/block before propagating cancellation.
Useful? React with 👍 / 👎.
sunnyrp
left a comment
sunnyrp
left a comment
There was a problem hiding this comment.
PR #307 — Elasticsearch index compatibility upgrades
Verdict: REQUEST CHANGES
Blockers: (1) CI build is red at head b6a7ec79. (2) Two unanswered P1/P2 threads are confirmed at head: sibling-alias deletion → data loss, and a stranded source write block. (3) Deterministic _reindex start rejections leave the source write-blocked with no public recovery path.
Warnings: stale seq_no/primary_term optimistic-concurrency tokens after cutover; final_pipeline can mutate copied docs undetected; no-progress timeout ignores the injected TimeProvider; PR size/scope; CLA pending.
Security | Build | Dependencies | Requirements | Code Review | Data-loss matrix | Pre-merge plan
Security: PASS
No malicious scripts, credential access, or obfuscation found. CodeQL passes. The workflow-permission alert was addressed in 5451d7f7.
Build: FAIL | CI: build / build failed on both runs (35809708683, 35809712324) with 1,170 tests, 1 failed
IndexCompatibilityTests.RunCompatibilityReindexAsync_TransientSubmissionIsNotRetriedAcrossNodes fails at IndexCompatibilityTests.Task.cs:109. Expected ["POST /_reindex"], got HEAD / first.
- Root cause:
StaticNodePoolpings a node (HEAD /) before first use, andForceNodedoes not disable that ping. The stub queued forPOST /_reindexis consumed by the ping instead (see inline). - The PR description's "final-head CI" gate is therefore not met. The listed evidence runs target
9c681f8c, which is two commits behind the current head.
Dependencies: N/A for audit
One System.IO.Hashing reference was added. It is used only for the X-Opaque-Id correlation hash.
Requirements: GAPS
- Stated scope is met in design: offline, operator-invoked; zero compatibility requests in
ConfigureIndexesAsync; fail-closed recovery. - Remaining gates in the PR body are not yet closed at the current head:
Code Review
| # | Sev | Finding | Impact |
|---|---|---|---|
| 1 | BLOCKER | Wrapped-name branch of ValidateCompatibilityDeleteTarget (Index.cs:485-492) never calls IsNativelyClaimedByOtherConfiguredIndex(canonicalName) |
events.DeleteAsync() can delete events-v2's reindexed-v9-events-v2 → data loss (confirms open P1 thread) |
| 2 | BLOCKER | Source is marked and write-blocked (Upgrader.cs:130-131). Any failure before CreateTargetAsync leaves canResetCurrentAttempt == false: refresh, lock renew, or callback cancellation at 5%. Recovery then classifies source-only as ManualIntervention |
Production source stays write-blocked with no public reset API (confirms/extends open P2 thread) |
| 3 | BLOCKER | _reindex start: every non-success response (e.g. deterministic 400/403/404) and every pre-dispatch exception (ping, DNS, cancellation) becomes ElasticReindexTaskUncertainException (Runner.cs:90-101) |
Source write-blocked plus a marked, empty target, classified ManualIntervention; the operator must hand-edit cluster state |
| 4 | WARNING | _reindex resets _seq_no/_primary_term. Foundatio versioned documents carry these as their concurrency token |
Clients holding pre-upgrade versions can hit spurious conflicts, or in rare collisions a lost update (inconsistency). Document it, and advise invalidating cached versions |
| 5 | WARNING | Pipeline("_none") bypasses only default_pipeline. A copied index.final_pipeline still runs during _reindex |
Document bodies can be transformed while counts and mappings still match. Silent data inconsistency; reject or strip final_pipeline for the copy, then restore it |
| 6 | NIT | No-progress timeout uses Stopwatch.StartNew() while delays use the injected TimeProvider (Runner.cs:174) |
The timeout branch cannot be tested with FakeTimeProvider; clocks are inconsistent |
| 7 | WARNING | PR size/shape: +9.8k lines, 90+ commits, many fix-on-fix commits, 98 review threads | Hard to review for regressions in the shared ElasticReindexer path (+297/−248) used by every consumer. Squash-merge, and consider splitting the reindexer refactor |
Data-loss / inconsistency matrix
- Source deletion is irreversible. It happens atomically inside the
_aliasescutover (remove_index,Upgrader.cs:203-206). Count and mapping checks precede it, but there is no snapshot precondition. Require a verified snapshot in the runbook. - Sibling families (#1): physical index of another registered index deleted.
- Stranded write blocks (#2, #3): writers fail until manual intervention. Not data loss, but an outage.
- Concurrency tokens (#4) and final pipeline (#5): silent content or version divergence.
- Cluster-wide readiness: as the PR notes, system indexes such as
.geoip_databasescan still block ES9 startup. Foundatio preflight ≠ Upgrade Assistant.
Existing comments
- Outstanding (confirmed at head): 4075643315 (P1 sibling canonical alias), 4078470116 (P2 5% cancellation). Replied to both with verified details.
- Earlier threads appear addressed by author replies. Not re-verified individually.
Recommended pre-merge plan
- Fix the CI test by adding
.DisablePing(), or stubHEAD /. Decide whether product code should classify pre-dispatch failures as not started. - Fix #1 by applying sibling arbitration in the wrapped branch, with a regression test for
events+events-v2+reindexed-v9-events-v2. - Fix #2 and #3 by tracking "no create/reindex dispatched" locally, and resetting the source marker and block on those paths. Treat deterministic 4xx
_reindexrejections as not started, so the reset path can clean up the empty target. - Address or document #4 and #5 in
docs/guide/index-management.md: pre-flight checks forfinal_pipeline, and a version-token caveat. - Re-run and link, at the final head:
- ordinary CI
- ES 8.19.15 + 9.5.0 full matrix
- standalone format check
- Windows and macOS packaged consumer
- persistent 7.17→8.19→9.5 chain with the ES9 leg actually executing
- Validate the combined #327/#307/#308 stack, or merge the others first and rebase.
- CLA:
license/clais pending. - Squash-merge with a curated message; release as preview first; keep the audit branch unmerged.
- Operator runbook: snapshot → stop writers/index managers →
Inspect→Upgrade→ verify →Recoverif needed, including rollback via snapshot restore.
| new Uri("http://node-2.invalid:9200"), | ||
| new Uri("http://node-3.invalid:9200") | ||
| ]), invoker) | ||
| .MaximumRetries(2); |
There was a problem hiding this comment.
BLOCKER (CI red at head), 95% confidence. StaticNodePool enables pinging, so the transport sends HEAD / to node-1 before the forced POST /_reindex. The ping consumes the only stub, and the Request: "POST /_reindex" assertion in SequenceRequestInvoker fails (CI: Expected "POST /_reindex", Actual "HEAD /").
Minimal fix: add .DisablePing() here, or queue a HEAD / 200 stub first. Keep MaximumRetries(2) so the test still proves ForceNode prevents failover.
| } | ||
| catch (Exception ex) | ||
| { | ||
| throw CreateUncertainStartException(sourceIndex, targetIndex, ex); |
There was a problem hiding this comment.
BLOCKER (fail-closed, but unrecoverable), 85% confidence. Every exception here becomes ElasticReindexTaskUncertainException. That includes failures that provably happen before dispatch: the node ping above, DNS, OperationCanceledException before send, and SelectSingleDispatchNode failure. Lines 95-101 do the same for deterministic 4xx rejections such as 400 action_request_validation_exception or 403.
At this point the source is marked and write-blocked, and the target exists with its marker. onTaskTerminated never fires, so canResetCurrentAttempt stays false. GetRecoveryAction then returns ManualIntervention. Result: the production source stays write-blocked, and no public API can clear it.
Suggest classifying "no request bytes sent" and 4xx (excluding 408/429) as not started, and letting the caller reset. Keep 5xx/timeout/lost-response as uncertain.
| int statusFailures = 0; | ||
| long lastProgress = 0; | ||
| var noProgressTimeout = ElasticReindexer.GetNoProgressTimeout(workItem); | ||
| var noProgressStopwatch = Stopwatch.StartNew(); |
There was a problem hiding this comment.
NIT, 90% confidence. The no-progress window uses the wall-clock Stopwatch, while every delay uses the injected _timeProvider. With a FakeTimeProvider the timeout branch can never fire, so it is untested. Use _timeProvider.GetTimestamp() / GetElapsedTime() for consistency.
| if (batchSize.HasValue) | ||
| source.Size(batchSize.Value); | ||
| }); | ||
| d.Dest(destination => destination.Index(targetIndex).OpType(OpType.Create).Pipeline("_none")); |
There was a problem hiding this comment.
WARNING (silent data inconsistency), 70% confidence; please confirm. Pipeline("_none") suppresses only the destination's index.default_pipeline. _create_from copies index.final_pipeline, which always runs and cannot be bypassed per request. A non-idempotent final pipeline (timestamps, enrich, set) would rewrite every copied _source. Counts and mappings would still match, so verification passes and the original is then deleted.
Suggest failing validation when the source has index.final_pipeline, or stripping it on the target for the copy and restoring it before cutover.
|
|
||
| workflowAttempted = true; | ||
| await AddWorkflowMarkerAsync(sourceIndex, false, cancellationToken).AnyContext(); | ||
| await AddWriteBlockAsync(sourceIndex, cancellationToken).AnyContext(); |
There was a problem hiding this comment.
BLOCKER (stranded write block), 90% confidence. This extends open thread r4078470116. After this block is acknowledged, canResetCurrentAttempt is false until line 138. Every failure in that window leaves the source write-blocked with only the source existing:
RefreshAsyncfailure or cancellationreindexLock.RenewAsync()failure insideReportProgressAsync(5)- callback
OperationCanceledException CreateTargetAsyncrejected with a deterministic 4xx
GetRecoveryAction maps that state to ManualIntervention (Recovery.cs:298-304), and CanResetCurrentAttempt requires TargetExists, so nothing can clear the block.
Suggest a local targetCreateDispatched flag. When it is still false, the process has positive evidence that no create is in flight, so it can safely remove the block and marker before rethrowing.
This branch has not been deployed
4 participants
Status — audit fixes committed; final release validation pending
Current head:
9c681f8ce11b10509aadd2af9dcbbdcaea33200d. Four clean audit-fix commits have been fast-forwarded onto this PR. They have Release-build, red/green regression, core-suite, and packaged-consumer evidence. Production approval is still gated on final-head CI and the remaining full-suite/persistent-chain results.Base
mainremainsa61d541613528428cc00bc2236602db72948a924, already included in this branch. A fresh compare after promotion reports 0 commits behind main. There was no newer base revision to merge. No force-push, merge to main, release, PR-base retargeting, or change to this PR's publishing workflow was performed.Temporary execution workflows remain exclusively on
audit/pr-307-final-validation-20260922; do not merge that audit branch. The four promoted commits change only production source, tests, and documentation.Summary
Add explicit, operator-invoked APIs to inspect and remediate Elasticsearch index-format compatibility independently of schema versioning. This is offline maintenance, not an automatic startup migration. Normal
ConfigureIndexesAsyncperforms zero compatibility requests.reindexed-v{server-major}-{canonical-name}, verify the copy behind write blocks, atomically transfer complete alias definitions, and delete the old physical index.reindexed-v*to normal schema, mapping, maintenance, cleanup, or deletion patterns.Final-audit fixes
548561008915594e1d76e6279c681f8cTask results cannot silently lose failure evidence
The shared reader previously ignored
timed_out, coerced dictionary counters throughConvert.ToInt64, and delegated JSON counter binding to the application serializer. Timeout evidence could be discarded; booleans or fractions could become counters; and application naming policy could lose wire values.The reader now parses all six counters directly and requires nonnegative, in-range integers. Malformed present timeout flags are rejected.
timed_out: trueis rejected before compatibility cutover or normal-schema alias promotion. Tests cover JSON/dictionary payloads, supported integer types, malformed values, custom naming policy, buffered/unbuffered transport, and both callers. This does not supply #327's broader schema-completion guarantees.Natural names cannot bypass error-index provenance
IsPotentialCompatibilityErrorNamenow checks the configured native name before considering a generated wrapper. Recovery therefore requires the persistent.foundatio-reindex-errormarker for naturalreindexed-v…families too. Unit and public inspection regressions cover plain, versioned, daily, monthly, and literalreindexedfamilies, while retaining ordinary names that naturally end in-error.Task 404 is uncertainty, not proof that copying stopped
Task lookup can fall back to stored results when the owning node is unreachable; a missing stored result is not positive termination evidence. See the pinned Elasticsearch task lookup implementation.
After a copy has been dispatched, cleanup now requires a successful task response explicitly reporting
completed: true. A 404 does not invoke the termination callback that authorizes deleting the destination or releasing the source write fence. Tests cover clean/partial cancellation, positive completion, runner cleanup authority, and the full upgrade path. Existing positive-termination fixtures now supply explicit completion rather than treating 404 as success.Secondary recovery errors do not hide cancellation
The upgrader follows the original operation failure through the first aggregate cause, preserving its cancellation token if inspection/reset also fails. Secondary diagnostics remain attached. An independent cleanup-token timeout does not turn a non-cancellation operation failure into apparent caller cancellation. Committed-cutover success paths remain separate.
Validation — observed results and remaining gates
Verified candidate evidence: macOS run 35771563588, which generated the exact four promoted commits:
.githubdelta, and no audit harness files in the promoted tree.10714278739contains the candidate patch/bundle, exact commit IDs, build/test logs, and packaged-consumer results.The same 37-red / 61-green / 152-core results were independently observed on Windows in run 35770100836. That run's consumer restore failed because its audit harness treated the HTTPS package-source argument as a local Windows path; the replacement uses NuGet.Config. This was not a product-test failure.
The original formatter command returned zero but reported optional sibling-project workspace failures. That is not accepted as clean formatter evidence. A standalone folder-mode whitespace verification is included in the refreshed gates.
git diff --checkpassed during candidate construction and staging.9c681f8cWhat the first persistent rehearsal established
Run 35770775846 built the candidate cleanly, preserved a real ES7 data volume through ES8, executed the explicitly gated compatibility test on ES8 (1 passed, 0 skipped), restored snapshots at ES7 and ES8, and passed alias read/write smoke checks at ES8.
The ES9 leg did not execute. Its startup log identifies an unrelated, ES7-created
.geoip_databasessystem index as incompatible. The Foundatio-owned index had already been upgraded. Artifact10714821867, especiallychain-final-server.log, preserves the exact failure and successful earlier phases.This demonstrates why a Foundatio-owned index preflight is not cluster-wide upgrade certification. The replacement isolated rehearsal disables the optional GeoIP downloader from initial disposable-cluster provisioning; it does not delete or mutate production/system data to conceal the failure. Production clusters still require Elastic Upgrade Assistant and system-index readiness checks.
The rehearsal covers a persistent single-node 7.17.29→8.19.15→9.5.0 sequence and same-major snapshot restoration. It is not a rolling multi-node test or proof of arbitrary downgrade safety. Earlier historical chain/combined-snapshot results remain in the discussion; they do not substitute for these current release gates.
Existing review fixes retained
The prior review merged
main, repaired the direct hashing dependency and xUnit 4 namespaces, and added 20 passing destination-reservation cases. All registrations reserve their logical/native/error names and configured/dated aliases, including absent siblings excluded from the upgrade batch; candidates are checked again under the reindex lock. The P1 collision thread was answered and resolved.The public guide, XML remarks, and agent lifecycle reference also distinguish stable alias routing and count checks from guaranteed lossless schema migration, remove nonexistent
force: trueexamples, and use the correct named configuration-level progress callback.The old ordinary CI at
13f069f6is green; the earlier97a335f3run reported 1,107 passed, 0 failed, 1 deliberate manual-chain skip. Those results are historical baseline evidence, not full-suite evidence for the four new commits.Lifecycle and operating contract
Validate the complete requested batch before mutation, then revalidate under
reindex:{logical-name}: registration, ownership, destination reservations, throttles, schema precedence, duplicate lineage, and supported source state.The workflow marks/write-blocks the exact source, refreshes it, creates/marks the deterministic target through
_create_from, and copies usingop_type=create, pipeline_none, one unsliced task, and deterministicX-Opaque-Id. It blocks the target before checking task/shard/count/mapping/settings/alias evidence, restores temporary settings, re-reads source evidence, and atomically deletes the source while transferring original/canonical aliases. It unblocks the target before removing the final workflow marker.Once cutover dispatch starts, automatic reset is forbidden for that attempt. A timeout does not prove the request failed to commit. Public inspection is read-only; public recovery takes the same lock and only finishes a marked committed target.
None,Wait,Finish, andManualInterventionreport distinct observed states. There is no public force-unblock/reset operation. Only the running attempt may clean up acknowledged pre-copy setup or a positively terminated exact task, with intact marked topology and before any cutover dispatch.Ordinary callback errors are logged; cancellation/lock-renewal failures follow evidence-based handling. Batches are not transactional, and cancellation may be reported after a committed cutover. Unsupported system/dot, data-stream, ILM, CCR, closed, non-standard-mode, source-disabled/source-filtered, or pre-blocked sources are rejected.
_create_fromrequires Elasticsearch 8.18+; generated names over 255 UTF-8 bytes are rejected, not truncated. Kibana can choose the same namespace, so the workflows must not run concurrently against one source.Merge order and stacked PRs
Recommended release order: finish and land #327's normal-schema data-loss work first, then merge updated
maininto #307 and deliberately reconcile the shared code. At the audit's last read, #327 was091b2ba0a6c24806568c42897c1078f0b00dae5c; re-read tips before integration.A temporary #327→#307 review stack is useful when both must remain in parallel development. It is not mandatory when #327 can land first. Merely changing the base is insufficient: reconcile full-source copy, completion/error propagation, quiesced reconciliation, alias preservation, compatibility ownership, and strict task evidence, then test the actual combined tree. After the parent lands, retarget/update the child against
main.Keep #308 independently reviewable unless it gains a real dependency. Its shared mapping-discovery conflict must preserve explicit external mapping patterns, canonical ownership, hidden/error filtering, and deterministic ordering. Shared files alone do not require stacking every PR. No related branch was merged or retargeted during this audit.
Production release gate
Require final-head CI, full Elasticsearch 8/9 suites, the completed persistent sequential-major/snapshot rehearsal, clean formatting, and the intended combined release-tree validation before approval.
For deployment: stop writers, queue consumers, maintenance jobs, and alias managers; verify a restorable pre-upgrade snapshot; close the old-major rollback window; run the explicit operation; verify aliases and reads/writes; drain/restart clients before writes resume because cached concurrency tokens belong to deleted indexes; then take a new verified snapshot. Use Elastic Upgrade Assistant for readiness beyond Foundatio-owned indexes.
Green tests are scoped evidence, not a guarantee that every possible bug has been eliminated.