Skip to content

fix(sitesearch): reconcile ES/OS index mirrors on the write path (#36360) - #36825

Open
fabrizzio-dotCMS wants to merge 7 commits into
mainfrom
issue-36360-sitesearch-mirror-reconciliation
Open

fix(sitesearch): reconcile ES/OS index mirrors on the write path (#36360)#36825
fabrizzio-dotCMS wants to merge 7 commits into
mainfrom
issue-36360-sitesearch-mirror-reconciliation

Conversation

@fabrizzio-dotCMS

@fabrizzio-dotCMS fabrizzio-dotCMS commented Jul 30, 2026

Copy link
Copy Markdown
Member

Problem

A Site Search index is one logical index mirrored across both engines (ES + its .os OpenSearch twin). Reads were made phase-aware in #36797, but the write path can still leave the two engines physically out of sync, so Site Search returns stale or empty results once reads move to OpenSearch. This is issue #36360's write-path half (the read half shipped in #36797).

Failure modes (paths that skip the full-crawl happy path)

  1. Forward-only phase change — Phase 0→1/2 does not retroactively build OS twins of indices that already existed in ES.
  2. Phase-0 crawl builds an ES-only index; its OS twin never existed.
  3. Incremental crawl writes documents in place (no createSiteSearchIndex) — two desync paths: (a) if the OS twin is missing, the raw putToIndex lets OpenSearch auto-create it with a dynamic mapping (keywordtext, breaking aggregations) holding only the delta → partial, wrongly-mapped twin; (b) if the twins already drifted, an incremental only appends the new delta to both and never reconciles the difference.
  4. Fire-and-forget shadow create/write — a failed OS create or putToIndex in a dual-write phase is swallowed at WARN. Main source of content drift: the OS twin exists but silently holds fewer docs than ES.
  5. Phase-scoped delete — a delete fanned only to the current phase's write providers leaves a twin on the other engine as an orphan after a rollback.

Fix — self-heal on crawl (not a big-bang rebuild on phase change)

  • SiteSearchAPI#existsOnAllWriteEngines(name) — whether the index exists on every current write engine (ES checks the plain name, OS the .os twin; router aggregates over writeProviders()).
  • SiteSearchAPI#writeMirrorsInSync(name) — existence plus document-count parity across write engines (each leaf counts its own physical index via a match-all rows=0). Catches content drift that existence alone cannot. Count parity is sound here because Site Search is single-writer with immediate refresh and no concurrent crawl on the same index, so copies are quiescent at crawl-planning time.
  • Incremental-crawl gate (SiteSearchJobImpl) — an incremental crawl runs only when writeMirrorsInSync is true; a missing twin OR a count mismatch demotes it to a full rebuild, which recreates identical copies (correct mapping) on every engine and re-points the alias. Self-heals both the missing/partial twin and content drift on the next crawl, and removes the dynamic-mapping auto-create path (FM1–FM4).
  • SiteSearchAPIImpl#deleteIndex sweeps both engines — primary (current read provider) delete is authoritative and propagates; the other engine is best-effort and failure-tolerant (unreachable engine → WARN, never fails the delete), so a rollback leftover is not orphaned even in a single-provider phase (FM5).

Residual limitation (documented)

Reconciliation is on crawl, so the gap is only closed once a crawl runs in a dual-write phase. In the no-crawl window: in Phase 2 a missing twin is covered by the read fallback (OS errors → read ES), a partial-on-create twin can no longer be created, and content drift heals on the next crawl — but a drifted twin read before that crawl still returns incomplete results silently. Phase 3 is the cliff (ES decommissioned, no fallback). Paired operational rule: crawl each index once before promoting phase. A targeted verify/repair job (detect twins missing or count-divergent → rebuild full) that closes the no-crawl window without depending on a crawl is deferred as a follow-up under #36360.

Tests

  • SiteSearchRouterReconciliationTest (unit, mocked leaves, no cluster — runs in surefire): pins existsOnAllWriteEngines aggregation (dual-write vs Phase 0), writeMirrorsInSync (equal counts / count drift / missing twin / Phase 0), and deleteIndex both-engine sweep (authoritative primary, best-effort/tolerant secondary, active-index guard). ✅ 11/11.
  • SiteSearchDualWriteRouterIT: fixes pre-existing OS existence assertions that used the logical (untagged) name while the physical twin is .os-tagged since Align OpenSearch Site Search index naming with the .os + versioned-store migration pattern #36672 (only surfaces on a real separate-cluster run, where this IT is gated); adds reconciliation scenarios (missing OS twin flips existsOnAllWriteEngines; content drift flips writeMirrorsInSync; delete sweeps an orphan twin in a single-provider phase).

Docs

docs/backend/OPENSEARCH_MIGRATION.md — new "Site Search mirror reconciliation (self-heal on crawl)" subsection: failure modes, the two fixes, the no-crawl window, and the operational rule.

Notes

🤖 Generated with Claude Code

@mergify

mergify Bot commented Jul 30, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @fabrizzio-dotCMS's task in 9m 39s —— View job


Review: PR #36825 — sitesearch mirror reconciliation

  • Read the full diff against origin/main
  • Review write-path reconciliation (SiteSearchAPIImpl, routers)
  • Review incremental-crawl gate (SiteSearchJobImpl)
  • Review document-count implementations (ES/OS leaves)
  • Check tests + docs
  • Post review

The design is sound and the follow-up commits address the earlier count-parity / fail-safe feedback well. The documentCount (_count on ES, track_total_hits on OS), the -1 fail-safe treated as out-of-sync, and the extracted incrementalEligible predicate are all correct. Two small items below; neither blocks merge.

New Issues

  • 🟡 Medium: ESSiteSearchAPI.java:49 — the import java.util.stream.Collectors; is now unused. This PR removed its only call site (listIndices() changed .collect(Collectors.toList()).toList() at what was line 114), leaving the import dangling. Harmless at runtime but a leftover; per the repo's progressive-enhancement rule it should be dropped. Fix this →

  • 🟡 Medium: SiteSearchAPIImpl.java:355-380 (deleteIndex) — behavioral change worth a conscious call. The old router.writeChecked(...) fanned the delete to all write providers and always invoked the shadow even when the primary threw (primary exception recorded, shadow still called, then re-thrown). The new sequential form deletes the primary first and, on failure, throws before the secondary.deleteIndex(...) sweep runs. So a transient primary-delete failure in a dual-write phase now leaves the secondary twin untouched, whereas before it would have been deleted.

    This is arguably more consistent (a failed delete leaves the index present on both engines rather than half-deleted), and the exception surfaces so nothing is silently orphaned — but it's a semantic shift from the prior fan-out. If the intent is "sweep the other engine regardless of primary outcome," the secondary sweep should sit in a finally or run before the re-throw. Assumption: leaf deleteIndex is idempotent (confirmed by the code comment), so a retry recovers. What to verify: whether callers reliably retry a failed deleteIndex, or whether best-effort secondary cleanup should happen even when the primary fails.

Notes (non-blocking)

  • The unit test SiteSearchRouterReconciliationTest drives phases 0/1 only. The deleteIndex primary/secondary identity swap (primary == esImpl ? osImpl : esImpl) is only exercised with ES-as-primary; a Phase-2 case (OS primary, ES secondary) would pin the swap in both directions. The IT covers the orphan-sweep behavior, so this is a completeness nit, not a gap.
  • writeMirrorsInSync's cross-engine count-parity soundness rests on the documented single-writer/quiescent-at-planning assumption and on ES vs OS reporting identical doc counts for an in-sync mirror. Both are reasonable and clearly documented in the interface javadoc and OPENSEARCH_MIGRATION.md.

Base automatically changed from issue-36360-sitesearch-alias-os-resolution to main July 30, 2026 22:20
fabrizzio-dotCMS and others added 6 commits July 30, 2026 16:25
… engines (#36360)

A Site Search index is one logical index mirrored across both engines, but paths that
skip the full-crawl happy path can leave the ES index and its OS twin out of sync
(forward-only phase change, Phase-0 ES-only index, incremental crawl over a missing
twin auto-creating it with a dynamic mapping, fire-and-forget shadow-create failure,
phase-scoped delete leaving an orphan).

- Add SiteSearchAPI#existsOnAllWriteEngines(name): true only when the index exists on
  every current write engine. ESSiteSearchAPI checks the plain name, OSSiteSearchAPI the
  .os-tagged name, and SiteSearchAPIImpl aggregates across writeProviders().
- Gate incremental crawls in SiteSearchJobImpl on it: when a write engine is missing the
  index, demote to a full rebuild so the mirror is recreated (correct mapping) on every
  engine instead of auto-created partial. Self-heals on the next crawl.
- SiteSearchAPIImpl#deleteIndex now sweeps BOTH engines: primary (read provider) delete is
  authoritative and propagates; the other engine is best-effort (tolerant of an unreachable
  engine, WARN) so a phase-rollback leftover twin is not orphaned in a single-provider phase.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s assertions (#36360)

- SiteSearchRouterReconciliationTest (unit, mocked leaves, no cluster): pins the routing
  invariants — existsOnAllWriteEngines aggregates across write providers (dual-write vs
  Phase 0), and deleteIndex sweeps both engines with an authoritative primary and a
  best-effort, failure-tolerant secondary, guarding the active index. Runs in surefire.
- SiteSearchDualWriteRouterIT: fix pre-existing OS existence assertions that used the
  logical (untagged) name while the physical OS twin is .os-tagged since #36672 (the IT is
  gated to separate clusters, so this only surfaces on a real dual-cluster run); add
  reconciliation scenarios (missing OS twin flips existsOnAllWriteEngines; delete sweeps an
  orphan twin in a single-provider phase).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…#36360)

Add a subsection covering the write-path desync failure modes, the two fixes
(incremental-crawl gate + delete-both-engines), the no-crawl residual window (Phase-2 read
fallback vs the Phase-3 cliff), and the operational rule to crawl each index once before
promoting phase. Notes the targeted verify/repair job as a deferred follow-up.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… existence (#36360)

Existence alone could not catch content drift: both twins present but holding different
documents (typically an OpenSearch shadow put/create that failed fire-and-forget), which an
incremental crawl would perpetuate by only appending the new delta.

Add SiteSearchAPI#writeMirrorsInSync(name): the index exists on every current write engine
AND their document counts match (each leaf counts its own physical index via a match-all
rows=0; ES the plain name, OS the .os twin). SiteSearchJobImpl now gates the incremental
crawl on this instead of bare existence, so a missing twin OR a count mismatch demotes to a
full rebuild that recreates identical copies on every engine. Count parity is a sound in-sync
test here: Site Search is single-writer with immediate refresh and no concurrent crawl on the
same index, so the copies are quiescent at crawl-planning time. A single write engine (Phases
0/3) has nothing to compare and is trivially in sync.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
)

- SiteSearchRouterReconciliationTest: +4 cases for writeMirrorsInSync (equal counts → in
  sync; count drift → out of sync; missing twin short-circuits before counting; Phase 0
  single engine trivially in sync).
- SiteSearchDualWriteRouterIT: +content-drift scenario — write a document to the OpenSearch
  twin alone, then assert writeMirrorsInSync flips to false.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…te (#36360)

Update the mirror-reconciliation subsection: the incremental gate now checks existence AND
document-count parity, so content drift (not just a missing twin) self-heals on the next
crawl; clarify why count parity is a valid in-sync invariant and refine the no-crawl residual
window and the deferred verify/repair scope (missing OR count-divergent).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@fabrizzio-dotCMS
fabrizzio-dotCMS force-pushed the issue-36360-sitesearch-mirror-reconciliation branch from c6b95e9 to 71b0f17 Compare July 30, 2026 22:26
@github-actions github-actions Bot added Area : Backend PR changes Java/Maven backend code Area : Documentation PR changes documentation files labels Jul 30, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ AI review failed

Claude did not produce a review — the backend call errored before generating any output (provider: anthropic-bedrock, model: global.anthropic.claude-sonnet-5). This usually means the model has no Bedrock access grant in the target account, or the model ID is invalid — not a problem with this PR.

Run: #30587154434

)

Address PR review on the write-path reconciliation:

- HIGH: the count-parity gate compared search().getTotalResults(), whose
  total is capped at 10,000 on the ES 7.x / OpenSearch clients (neither
  search sets track_total_hits). Two large mirrors that had genuinely
  drifted (e.g. ES 15,000 vs OS 12,000) both read back 10000 and compared
  equal, so writeMirrorsInSync returned true and the incremental crawl
  proceeded on divergent copies — the exact content-drift case (FM4) the
  gate exists to catch. Add SiteSearchAPI.documentCount(name): an exact
  count (dedicated _count on ES, size:0 match-all with track_total_hits on
  OS) and use it in the parity loop.

- MEDIUM: a failed count now fails safe. documentCount returns -1 on error
  and writeMirrorsInSync treats -1 as out of sync, so a both-engines-failed
  0==0 is no longer mistaken for "in sync".

- MEDIUM: extract the duplicated 5-term incremental precondition in
  SiteSearchJobImpl into a single incrementalEligible local so the decision
  and the "forcing full rebuild" log cannot drift out of step.

Unit test updated to mock documentCount; adds drift-above-10k and
failed-count fail-safe cases (13/13).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@fabrizzio-dotCMS

Copy link
Copy Markdown
Member Author

Addressed the review feedback in a9b8501:

  • 🟠 High — count-parity blind above 10k. Confirmed: neither leaf's search() set track_total_hits, so getTotalResults() capped at 10,000 and drift on large mirrors (e.g. ES 15k vs OS 12k → both 10000) slipped through. Added SiteSearchAPI.documentCount(name) — an exact count (dedicated _count on ES, size:0 match-all with track_total_hits:true on OS) — and the parity loop now uses it instead of a search total.
  • 🟡 Medium — transient double-failure read as "in sync". documentCount returns -1 on a failed count and writeMirrorsInSync treats -1 as out of sync, so a both-engines-failed 0 == 0 can no longer permit an incremental. Fail-safe is now rebuild.
  • 🟡 Medium — duplicated 5-term predicate. Extracted into a single incrementalEligible local in SiteSearchJobImpl, reused by both the decision and the log guard.

Interface javadoc and OPENSEARCH_MIGRATION.md qualified accordingly. SiteSearchRouterReconciliationTest now mocks documentCount and adds drift-above-10k + failed-count cases — 13/13 green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Backend PR changes Java/Maven backend code Area : Documentation PR changes documentation files

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant