From d4983f9275ba974aade5bd06de9c3d992f385715 Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Mon, 7 Sep 2026 13:18:49 -0600 Subject: [PATCH 1/5] docs(spec): Phase 2 ES read fallback never fires for content search (#37413) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue-resolution spec for #37413. In Phase 2 the documented OpenSearch-> Elasticsearch read fallback never fires for content search: the five read call sites of ESContentFactoryImpl.indexOperationsDelegate() invoke the selected provider directly, so PhaseRouter.read/readChecked — where the fallback lives — is never reached. Root cause was verified against the tree rather than taken from the issue body, which attributes the observed 200/total=0 to the OpenSearchException -> ERROR_HIT branch. A ConnectException is not an OpenSearchException, so that branch is never reached in the reported case. The spec records the verified mechanism instead (count-first cache key, ContentUtils catch(Throwable), ContentHelper resultsSize overwrite) and flags the issue body for correction. Spec only — no implementation. /speckit-plan follows once this is approved. Co-Authored-By: Claude Opus 5 (1M context) --- specs/37413-phase2-read-fallback/spec.md | 249 +++++++++++++++++++++++ 1 file changed, 249 insertions(+) create mode 100644 specs/37413-phase2-read-fallback/spec.md diff --git a/specs/37413-phase2-read-fallback/spec.md b/specs/37413-phase2-read-fallback/spec.md new file mode 100644 index 000000000000..f66e50c141e5 --- /dev/null +++ b/specs/37413-phase2-read-fallback/spec.md @@ -0,0 +1,249 @@ +# Issue Resolution Specification: Phase 2 ES read fallback never fires for content search + +**Feature Branch**: `37413-phase2-read-fallback` + +**Created**: 2026-09-07 + +**Status**: Draft + +**Type**: Issue / Bug Resolution + +**Related GitHub Issue**: #37413 + +**Input**: User description: "37413" + +## Problem Statement *(mandatory)* + +The ES→OpenSearch migration design promises an automatic **read fallback to Elasticsearch in +Phase 2**: OpenSearch serves reads, Elasticsearch is still live and in sync, and if OpenSearch +throws on a read the error is logged at `ERROR` and the read is retried against Elasticsearch. +That fallback is implemented, but the **content read path never reaches it**, so it never fires. + +With OpenSearch unavailable in Phase 2 and Elasticsearch up and holding a complete copy of the +data, `POST /api/content/_search` does not fall back. It returns one of two things: + +- **HTTP 200 with `resultsSize: 0`** — a well-formed empty result. A caller cannot tell this + apart from "this content type has no content", so a live site renders as **missing content** + with no error surfaced anywhere and nothing for 5xx monitoring to catch. +- **HTTP 500** — for content types whose count was not already cached. + +**Severity / Impact**: High. Every customer running Phase 2 loses the documented safety net — +a transient OpenSearch blip, a slow node, or a missing OpenSearch index takes down content +delivery instead of degrading to Elasticsearch. The silent-empty variant is the more dangerous +of the two. Beyond the outage case, this also blocks recommending Phase 2 to customers at all: +Cloud and Support are currently being told "Phase 2 is safe, OpenSearch failures fall back to +Elasticsearch", and for content search that is false today. + +**This is not a regression.** With its read engine down dotCMS has always behaved this way — +Phase 0 with Elasticsearch down does the same thing through the same legacy code. What is +broken is the *promise*, not previously-working behavior. For content search the fallback +wiring was never written. + +## Reproduction *(mandatory)* + +**Environment**: `main` @ `788795e915` (2026-09-03). Local instance in Phase 2 +(`DOT_FEATURE_FLAG_OPEN_SEARCH_PHASE=2`), Elasticsearch 7.10.2 and OpenSearch 3.4.0, real +content indexed and in sync on both engines. + +**Steps to Reproduce**: + +1. Confirm the live phase from `GET /api/v1/jvm` (`DOT_FEATURE_FLAG_OPEN_SEARCH_PHASE` = `2`) + and that `GET /api/v1/index/migration/readiness` reports `readEngine: OpenSearch`. +2. Stop the OpenSearch node (stop the container or block its port). Leave Elasticsearch running + and healthy. +3. Call `POST /api/content/_search` for several content types that **do** have live content: + `{"query":"+contentType:Profile +live:true","limit":7,"offset":1}` +4. **Vary `offset` on every call.** An identical request body is served from the query cache and + returns the pre-outage result, which looks exactly like a working fallback. Only novel, + uncached queries expose the defect — this is how it was nearly missed in QA. +5. Observe the responses, then restart OpenSearch and repeat to confirm the content was present + the whole time. + +**Expected Behavior**: Every query returns the result set Elasticsearch holds, and each fallback +is logged once at `ERROR` naming the failing OpenSearch operation and its cause. + +**Actual Behavior**: + +| Content type | Actual with OpenSearch down | Same call, OpenSearch up | +|---|---|---| +| Profile | **200, `total=0`** | 200, `total=184` | +| Testimonials | **200, `total=0`** | 200, `total=317` | +| JobPosting | **200, `total=0`** | 200, `total=27` | +| Hero | **500** | 200, `total=215` | +| ContactCard | **500** | 200, `total=106` | + +Failures are logged at **`WARN`**, not the `ERROR` the design specifies, so the "early-warning +signal per read" is weaker than documented. + +**Reproducibility**: Always, on demand — provided the query is not already in the query cache +(see step 4). + +## Scope of Investigation *(mandatory)* + +- **Affected area**: Content search / delivery read path (`/api/content/_search`, `ContentletAPI` + index reads, Velocity `$dotcontent.pull`), under the ES→OpenSearch migration phase routing. +- **Suspected surface**: **Both.** The routing gap is in modern code + (`com.dotcms.content.elasticsearch.business.ESContentFactoryImpl`, + `com.dotcms.content.index.PhaseRouter`), but the reason the failure becomes an empty `200` is + legacy swallow behavior in `com.dotcms.rendering.velocity.viewtools.content.util.ContentUtils` + and `com.dotcms.rest.ContentHelper`. +- **Related known decisions**: `docs/backend/OPENSEARCH_MIGRATION.md` lines 105, 402, 676 and 901 + all state the Phase 2 read fallback as designed behavior — the document is the contract being + violated. Phase 3 must keep propagating read failures (no fallback; Elasticsearch is + decommissioned). The plan formally consults `dotCMS/platform-adrs`. + +## Root-Cause Hypothesis + +Two independent layers. Both were verified in the current tree. + +### Layer 1 — the router is never called (the actual defect) + +`ESContentFactoryImpl.java:273` picks an engine with a bare ternary and calls the chosen +provider **directly**: + +```java +ContentFactoryIndexOperations indexOperationsDelegate(){ + return isMigrationComplete() || isReadEnabled() ? indexOperationsOS : indexOperationsES ; +} +``` + +Its five read call sites — `:1352` `search`, `:1607` `indexCount`, `:1616` `searchHits`, +`:1634` `indexSearchScroll`, `:1669` `createScrollQuery` — all invoke the selected provider with +no router in between. `PhaseRouter.read` (`PhaseRouter.java:187-199`) and `readChecked` +(`:298-310`) implement the fallback correctly and are simply unreachable from this path; the +class contains no reference to `PhaseRouter` at all. + +The stack proves it by omission — no `PhaseRouter` frame between +`ESContentFactoryImpl.indexCount` and `ContentFactoryIndexOperationsOS`. The same outage through +`IndexAPIImpl` (the health check) *does* carry `PhaseRouter.read(PhaseRouter.java:192)`. Same +failure, two paths, one routed. + +### Layer 2 — why the same outage yields 200/total=0 for some types and 500 for others + +> This supersedes the explanation in the GitHub issue body, which attributes the `200/total=0` +> to the `OpenSearchException → ERROR_HIT` branch. That is **wrong** for the observed case: a +> `ConnectException` is not an `OpenSearchException`, so it never reaches that branch. The +> issue body needs correcting. + +1. `ContentHelper.pullContent:310` runs the **count first**. A `CountRequest` carries no + offset/limit, so varying the offset does not change the count cache key — content types + queried before the outage had a cached count and did not throw; types not previously queried + (Hero, ContactCard) missed the cache, the count threw, → **500**. +2. `ContentUtils.java:302` has a `catch (Throwable)` that logs a one-line-truncated `WARN` and + returns an empty list, swallowing the search failure entirely. +3. `ContentHelper.java:318` — `if (contentlets.isEmpty() && offset <= resultsSize) { resultsSize = 0; }` + overwrites the real cached count with `0` → **200 / `total=0`**. + +The `ERROR_HIT` branch (`ContentFactoryIndexOperationsOS.java:109-121`) is nonetheless a real +second-order hazard: it converts an `OpenSearchException` into a legitimate-looking empty result, +leaving the router nothing to catch. It is **inherited Elasticsearch behavior**, structurally +identical in `ContentFactoryIndexOperationsES.java:56` and `:151-161` — so any change to it must +be scoped carefully. Cache poisoning is bounded: `shouldQueryCache(exceptionMsg)` only caches +`ERROR_HIT` for `parse_exception` / `search_phase_execution_exception`, so a connection failure +is not cached. + +## Fix Scope & Non-Goals *(mandatory)* + +**In scope**: + +- Route the five read call sites of `ESContentFactoryImpl.indexOperationsDelegate()` through + `PhaseRouter.read` / `readChecked`, following the router pattern already documented in + `PhaseRouter`'s class Javadoc and used by `IndexAPIImpl`. +- Ensure each Phase 2 fallback occurrence is logged at `ERROR` (the router already does this; + confirm no `WARN`-only path remains upstream of it). +- Make the OpenSearch read path in Phase 2 surface a *throwable* failure to the router rather + than an empty result, for the failure classes the router is meant to catch — including a + missing OpenSearch counterpart index. +- Tests: Phase 2 falls back and logs `ERROR`; Phase 3 propagates; the query cache does not mask + a live failure. + +**Explicitly out of scope / non-goals**: + +- **No change to Elasticsearch behavior.** `ContentFactoryIndexOperationsES` keeps its current + `ERROR_HIT` semantics; Phase 0 and Phase 1 read behavior is untouched. +- **No fallback in Phase 3.** Failures must keep propagating there by design. +- **No broad rewrite of the legacy swallow.** `ContentUtils.java:302`'s `catch (Throwable)` and + `ContentHelper.java:318`'s `resultsSize = 0` overwrite are pre-existing and affect all phases, + including pure Elasticsearch. They explain the *symptom*, and fixing Layer 1 removes the + Phase 2 case. Changing them repo-wide is a separate, larger issue — file it, do not fold it in. +- **No `IndexAPI` generic parameterization** — that belongs in its own PR. +- No write-path changes. Write failure semantics per phase are settled elsewhere. +- No new configuration property or feature flag to switch the fallback on and off — it is + documented, unconditional Phase 2 behavior. + +## Regression Risk *(mandatory)* + +- **Blast radius**: `ESContentFactoryImpl`'s five read call sites are the funnel for essentially + all content search — `/api/content/_search`, `ContentletAPI.search`/`searchIndex`/`indexCount`, + Velocity `$dotcontent.pull`, URL maps, Site Search, scroll/pagination consumers, and the + admin UI content browser. Any behavior change here is felt site-wide in every phase, so the + routing change must be a pure pass-through in phases 0, 1 and 3. +- **Callers that depend on empty-instead-of-exception**: making the OpenSearch read throw where + it currently returns `ERROR_HIT` changes the contract for anything that today receives an + empty result. These callers must be enumerated before that change lands, and each confirmed + either unaffected or explicitly handled. If the enumeration turns out to be large, the routing + fix (Layer 1) still stands on its own and can ship without touching `ERROR_HIT`. +- **Scroll and pagination**: `indexSearchScroll` and `createScrollQuery` hold engine-specific + cursor state. A mid-scroll fallback cannot resume an OpenSearch scroll on Elasticsearch — the + plan must decide whether these two sites fall back at all or only propagate, and say so. +- **Backward compatibility**: no API contract, response shape, serialized state, DB schema or + index mapping changes. Not rollback-unsafe. +- **Data considerations**: none — no data repair needed. Content missing from a read during an + outage was never lost; it is in Elasticsearch the whole time. +- **Performance**: a fallback doubles the work for a failing read. Bounded to the outage window, + and the failing engine fails fast, but the plan should confirm no retry storm (one fallback + attempt per read, no nesting). + +## Acceptance & Verification *(mandatory)* + +- **AC-001**: The reproduction above produces the expected behavior — in Phase 2 with + OpenSearch stopped and Elasticsearch healthy, `POST /api/content/_search` returns the same + non-zero result set Elasticsearch holds for **every** content type that has live content. No + `total=0`, no 500. +- **AC-002**: All five read call sites of `indexOperationsDelegate()` (`:1352`, `:1607`, `:1616`, + `:1634`, `:1669`) go through `PhaseRouter.read` / `readChecked` instead of invoking the + selected provider directly — or, for a site the plan excludes (see scroll/pagination above), + the exclusion is documented in code with its reason. +- **AC-003**: Each fallback occurrence is logged at `ERROR`, once per read, naming the failing + OpenSearch operation and the cause — so an outage is visible to log-based monitoring per the + design's early-warning signal. No `WARN`-only swallow remains on the Phase 2 fallback path. +- **AC-004**: Phase 0, 1 and 3 read behavior is byte-for-byte unchanged: 0/1 read from + Elasticsearch with no fallback; 3 reads from OpenSearch and propagates failures. Verified by + test, not by inspection. +- **AC-005**: A missing OpenSearch counterpart index in Phase 2 is served from Elasticsearch + rather than returning empty, matching `OPENSEARCH_MIGRATION.md:402` — this also covers the + reactivated-backup-index scenario. +- **AC-006**: If the `OpenSearchException`/`ERROR_HIT` branch is changed, Elasticsearch's + equivalent branch (`ContentFactoryIndexOperationsES.java:151-161`) is **not** changed, Phase 3 + still propagates, and the enumeration of callers relying on the empty result is recorded in + the plan with each one confirmed unaffected. +- **AC-007**: A repeated identical query does not mask a live OpenSearch failure by serving a + stale pre-outage result from the query cache — the caller either gets fallback results or a + surfaced error, never a silent stale hit presented as current. + +**Verification method**: + +- Integration test — Phase 2 with the OpenSearch provider stubbed to throw: a content search + returns the Elasticsearch result set and logs at `ERROR`. +- Integration test — the same stubbed failure in Phase 3 propagates and does **not** fall back. +- Integration test — phases 0/1 unchanged (AC-004). +- Integration test — the query-cache path (AC-007). +- All of the above live in `dotcms-integration`, are named `*Test` (not `*IT`), and are + **registered in the matching `@SuiteClasses` suite** — an unregistered class compiles, passes + CI green, and never runs. +- Run: `./mvnw verify -pl :dotcms-integration -Dcoreit.test.skip=false -Dit.test=` + (requires the WAR to be installed first, not just compiled). +- Manual re-run of the reproduction on the Phase 2 real-data rig, varying `offset` per call. + +## Assumptions + +- The intended behavior is what `docs/backend/OPENSEARCH_MIGRATION.md` states; no ADR overrides + it. The plan will confirm against `dotCMS/platform-adrs`. +- Elasticsearch being in sync in Phase 2 is a given — Phase 2 is dual-write, so ES is + continuously written. A fallback returning stale-but-present ES data is strictly better than + returning nothing. +- The scroll/pagination question (AC-002 exclusion) is a plan-phase decision, not a + specification gap: either answer satisfies the design as long as it is deliberate and + documented. +- The GitHub issue body will be corrected to match Layer 2 above before implementation, so the + issue and this spec do not disagree on root cause. From f76802744d5966ca5be7cc45894ad0decf917738 Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Thu, 10 Sep 2026 10:16:15 -0600 Subject: [PATCH 2/5] fix(index): route content reads through PhaseRouter so the Phase 2 ES fallback fires (#37413) The Phase 2 Elasticsearch read fallback was implemented and correct; the content read path never reached it. ESContentFactoryImpl selected a provider with a bare ternary and called it directly from its five read call sites, so PhaseRouter.read -- which holds the fallback -- was unreachable from the busiest read path in the product. With OpenSearch down in Phase 2 and Elasticsearch healthy and dual-written, POST /api/content/_search returned either a well-formed 200 with zero results or a 500. The empty-200 variant is the dangerous one: a caller cannot tell it apart from "this content type has no content", so a live site rendered as missing content with nothing for 5xx monitoring to catch. All five read methods on ContentFactoryIndexOperations are declared without checked exceptions, so read() covers every call site and readChecked() is not needed. A connection failure already reaches the provider's generic handler and is rethrown as DotRuntimeException, which read() catches -- so routing alone closes the reported outage. The ternary is removed rather than left beside the router: a second provider-selection mechanism in this class is how the bug happened. The two provider fields and the two phase static imports became dead with it. createScrollQuery is routed for provider selection only and its fallback can never fire. Both implementations just construct a cursor -- no I/O, nothing to throw -- and the requests happen later inside that cursor, outside the router. A mid-scroll fallback is impossible in principle anyway: an OpenSearch scroll id is meaningless to Elasticsearch. The residual gap (a consumer already iterating when OpenSearch dies still fails) is documented at the call site. PhaseRouter gains a read(operation, fn) overload so the fallback log line names what failed and its root cause. The plain read() can only report the cause, because the operation it runs is an opaque lambda -- enough to know something fell back, not enough for monitoring to say what stopped working, which is the early-warning signal the migration design promises. Providers also wrap failures in a generic message, so the actionable text lives further down the cause chain: OS read failed in Phase 2 [indexCount] - falling back to ES. OS index may be stale or unavailable. Cause: An error occurred when executing the Lucene Query / root cause: ConnectException: Connection refused Tests: the per-phase contract is pinned on the router against the real provider interface, because ESContentFactoryImpl cannot be constructed outside a container -- its read methods run through static CacheLocator/APILocator calls before reaching a provider. The wiring proof therefore lives in the integration test, which injects an OpenSearch provider that fails every read and asserts the factory still returns real Elasticsearch data. That is the one thing a unit test on the router cannot show, and it was the entire defect: 13 of the 14 unit tests pass against unmodified code. Phases 0, 1 and 3 are verified unchanged by test rather than by inspection. Co-Authored-By: Claude Opus 5 (1M context) --- .../business/ESContentFactoryImpl.java | 66 ++- .../com/dotcms/content/index/PhaseRouter.java | 63 ++- ...actoryIndexOperationsPhaseRoutingTest.java | 475 ++++++++++++++++++ .../src/test/java/com/dotcms/MainSuite1b.java | 1 + ...SContentFactoryImplPhase2FallbackTest.java | 218 ++++++++ 5 files changed, 802 insertions(+), 21 deletions(-) create mode 100644 dotCMS/src/test/java/com/dotcms/content/index/ContentFactoryIndexOperationsPhaseRoutingTest.java create mode 100644 dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ESContentFactoryImplPhase2FallbackTest.java diff --git a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentFactoryImpl.java b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentFactoryImpl.java index 8b3a7a286d9a..6dfbf9720eb4 100644 --- a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentFactoryImpl.java +++ b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentFactoryImpl.java @@ -1,8 +1,6 @@ package com.dotcms.content.elasticsearch.business; import static com.dotcms.content.elasticsearch.business.ESContentletAPIImpl.MAX_LIMIT; -import static com.dotcms.content.index.IndexConfigHelper.isMigrationComplete; -import static com.dotcms.content.index.IndexConfigHelper.isReadEnabled; import static com.dotcms.variant.VariantAPI.DEFAULT_VARIANT; import static com.dotmarketing.portlets.contentlet.model.Contentlet.AUTO_ASSIGN_WORKFLOW; import static com.dotmarketing.portlets.contentlet.model.Contentlet.TITLE_IMAGE_KEY; @@ -17,6 +15,7 @@ import com.dotcms.content.business.json.ContentletJsonAPI; import com.dotcms.content.business.json.ContentletJsonHelper; import com.dotcms.content.index.ContentFactoryIndexOperations; +import com.dotcms.content.index.PhaseRouter; import com.dotcms.content.index.IndexContentletScroll; import com.dotcms.content.index.domain.SearchHit; import com.dotcms.content.index.domain.SearchHits; @@ -232,8 +231,19 @@ public class ESContentFactoryImpl implements ContentletFactory { private final ContentletCache contentletCache; private final LanguageAPI languageAPI; - private final ContentFactoryIndexOperations indexOperationsES; - private final ContentFactoryIndexOperations indexOperationsOS; + /** + * Phase-aware router for every index read this factory performs. + * + *

These read call sites are the funnel for essentially all content search in the product: + * {@code /api/content/_search}, {@code ContentletAPI} search and count, Velocity + * {@code $dotcontent.pull}, URL maps, Site Search, scroll consumers and the admin content + * browser. They used to pick a provider with a bare ternary and call it directly, which left + * the router's Phase 2 fallback to Elasticsearch unreachable from the busiest read path in + * the product — with OpenSearch down, a search returned a well-formed empty result rather + * than the data Elasticsearch was holding the whole time (issue #37413). The router is now + * the only way a provider is selected here; do not reintroduce a second mechanism.

+ */ + private final PhaseRouter indexRouter; private static final ObjectMapper mapper = DotObjectMapperProvider.getInstance() .getDefaultObjectMapper(); @@ -260,18 +270,28 @@ public String getInode() { * Elastic index. */ public ESContentFactoryImpl() { - this.contentletCache = CacheLocator.getContentletCache(); - this.languageAPI = APILocator.getLanguageAPI(); - this.indexOperationsOS = new ContentFactoryIndexOperationsOS(); - this.indexOperationsES = new ContentFactoryIndexOperationsES(CacheLocator.getESQueryCache()); + this(new ContentFactoryIndexOperationsES(CacheLocator.getESQueryCache()), + new ContentFactoryIndexOperationsOS()); } /** - * Migration-phase-aware Operations delegate - * @return {@link ContentFactoryIndexOperations} + * Constructor that accepts both index-operation providers, so a test can substitute one of + * them — typically an OpenSearch provider that fails on every read, to exercise the Phase 2 + * fallback to Elasticsearch. + * + *

This exists only for testing: production code must use {@link #ESContentFactoryImpl()}, + * which supplies the real providers. It carries no behaviour of its own — provider selection + * for reads is decided by {@link com.dotcms.content.index.PhaseRouter}, not here.

+ * + * @param indexOperationsES the Elasticsearch provider + * @param indexOperationsOS the OpenSearch provider */ - ContentFactoryIndexOperations indexOperationsDelegate(){ - return isMigrationComplete() || isReadEnabled() ? indexOperationsOS : indexOperationsES ; + @VisibleForTesting + ESContentFactoryImpl(final ContentFactoryIndexOperations indexOperationsES, + final ContentFactoryIndexOperations indexOperationsOS) { + this.contentletCache = CacheLocator.getContentletCache(); + this.languageAPI = APILocator.getLanguageAPI(); + this.indexRouter = new PhaseRouter<>(indexOperationsES, indexOperationsOS); } @Override @@ -1349,7 +1369,8 @@ public List findContentlets(final List inodes) throws DotDat public List findContentletsByHost(final String hostId, final int limit, final int offset) { try { - final List inodes = indexOperationsDelegate().search("+conhost:" + hostId, limit, offset); + final List inodes = indexRouter.read("search", + impl -> impl.search("+conhost:" + hostId, limit, offset)); return findContentlets(inodes); } catch (Exception e) { throw new RuntimeException(e.getMessage(), e); @@ -1604,7 +1625,7 @@ public List getRelatedLinks(Contentlet contentlet) throws DotDataException public long indexCount(final String query) { final String qq = LuceneQueryDateTimeFormatter .findAndReplaceQueryDates(translateQuery(query, null).getQuery()); - return indexOperationsDelegate().indexCount(qq); + return indexRouter.read("indexCount", impl -> impl.indexCount(qq)); } @Override @@ -1613,8 +1634,8 @@ public SearchHits indexSearch(final String query, final int limit, final int off final String formattedQuery = LuceneQueryDateTimeFormatter .findAndReplaceQueryDates(translateQuery(query, sortBy).getQuery()); - return indexOperationsDelegate().searchHits( - formattedQuery, limit, offset, sortBy); + return indexRouter.read("searchHits", + impl -> impl.searchHits(formattedQuery, limit, offset, sortBy)); } @@ -1631,7 +1652,8 @@ public SearchHits indexSearch(final String query, final int limit, final int off * @return PaginatedArrayList containing all search results */ PaginatedArrayList indexSearchScroll(final String query, String sortBy) { - return indexOperationsDelegate().indexSearchScroll(query, sortBy, SCROLL_BATCH_SIZE.get()); + return indexRouter.read("indexSearchScroll", + impl -> impl.indexSearchScroll(query, sortBy, SCROLL_BATCH_SIZE.get())); } /** @@ -1666,7 +1688,15 @@ PaginatedArrayList indexSearchScroll(final String query, Strin public IndexContentletScroll createScrollQuery(final String luceneQuery, final User user, final boolean respectFrontendRoles, final int batchSize, final String sortBy) { - return indexOperationsDelegate().createScrollQuery(luceneQuery, user, respectFrontendRoles, batchSize, sortBy); + // Routed for provider selection only: the fallback here can never fire, and that is + // correct rather than an oversight. Both provider implementations just construct a + // cursor -- no I/O, nothing to throw -- and the requests happen later, inside that + // cursor, outside the router. A mid-scroll fallback is impossible in principle anyway: + // an OpenSearch scroll id is meaningless to Elasticsearch, so a half-drained scroll + // cannot be resumed on the other engine. Residual gap: a consumer already iterating a + // scroll when OpenSearch dies still fails (issue #37413, AC-002 documented exclusion). + return indexRouter.read("createScrollQuery", impl -> impl.createScrollQuery( + luceneQuery, user, respectFrontendRoles, batchSize, sortBy)); } /** diff --git a/dotCMS/src/main/java/com/dotcms/content/index/PhaseRouter.java b/dotCMS/src/main/java/com/dotcms/content/index/PhaseRouter.java index 03754b5d5a7a..eaf71ab42287 100644 --- a/dotCMS/src/main/java/com/dotcms/content/index/PhaseRouter.java +++ b/dotCMS/src/main/java/com/dotcms/content/index/PhaseRouter.java @@ -185,19 +185,76 @@ public List writeProviders() { * @return result from the read provider (or ES fallback in Phase 2) */ public R read(final Function fn) { + return read(null, fn); + } + + /** + * Same as {@link #read(Function)}, but names the operation in the fallback log line. + * + *

The plain {@link #read(Function)} can only report the cause, because the operation it + * runs is an opaque lambda. That is enough to know something fell back, but not + * enough for log-based monitoring to say what stopped working — which is the + * early-warning signal the migration design promises. Callers on a read path that matters + * operationally should pass a name.

+ * + * @param operation short name of the operation being routed, e.g. {@code "indexCount"}; + * may be {@code null}, in which case the log line omits it + * @param fn must not throw checked exceptions; use {@link #readChecked} otherwise + */ + public R read(final String operation, final Function fn) { if (!isPhase2()) { return fn.apply(readProvider()); } try { return fn.apply(osImpl); } catch (final RuntimeException e) { - Logger.error(PhaseRouter.class, - "OS read failed in Phase 2 — falling back to ES. " - + "OS index may be stale or unavailable. Cause: " + e.getMessage(), e); + Logger.error(PhaseRouter.class, fallbackMessage(operation, e), e); return fn.apply(esImpl); } } + /** + * Builds the Phase 2 fallback log line. + * + *

Named operation and root cause are both deliberate. This line is the migration + * design's early-warning signal that OS has stopped answering, so it has to say + * what stopped working and why — a monitor that can only see "a read fell + * back" cannot tell a dead node from a single malformed query. The root cause matters + * because providers wrap failures in a generic message: the actionable text + * ("Connection refused", "index_not_found_exception") is further down the chain.

+ */ + private static String fallbackMessage(final String operation, final Throwable failure) { + final StringBuilder message = new StringBuilder("OS read failed in Phase 2"); + if (null != operation && !operation.isBlank()) { + message.append(" [").append(operation).append(']'); + } + message.append(" — falling back to ES. OS index may be stale or unavailable. Cause: ") + .append(failure.getMessage()); + final String rootCause = rootCauseMessage(failure); + if (null != rootCause) { + message.append(" / root cause: ").append(rootCause); + } + return message.toString(); + } + + /** + * Message of the deepest cause, or {@code null} when the failure is not wrapping anything + * that adds information. + */ + private static String rootCauseMessage(final Throwable failure) { + Throwable current = failure; + while (null != current.getCause() && current.getCause() != current) { + current = current.getCause(); + } + if (current == failure) { + return null; + } + final String message = current.getMessage(); + return null == message || message.isBlank() + ? current.getClass().getSimpleName() + : current.getClass().getSimpleName() + ": " + message; + } + /** * Fans a void write out to all current write providers. * diff --git a/dotCMS/src/test/java/com/dotcms/content/index/ContentFactoryIndexOperationsPhaseRoutingTest.java b/dotCMS/src/test/java/com/dotcms/content/index/ContentFactoryIndexOperationsPhaseRoutingTest.java new file mode 100644 index 000000000000..7d9563d6b902 --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/content/index/ContentFactoryIndexOperationsPhaseRoutingTest.java @@ -0,0 +1,475 @@ +package com.dotcms.content.index; + +import static com.dotcms.content.index.IndexConfigHelper.MigrationPhase.FLAG_KEY; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertThrows; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +import com.dotcms.content.index.IndexConfigHelper.MigrationPhase; +import com.dotcms.content.index.domain.SearchHits; +import com.dotcms.content.index.domain.TotalHits; +import com.dotmarketing.common.model.ContentletSearch; +import com.dotmarketing.exception.DotRuntimeException; +import com.dotmarketing.util.Config; +import com.dotmarketing.util.PaginatedArrayList; +import java.util.ArrayList; +import java.util.List; +import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.core.LogEvent; +import org.apache.logging.log4j.core.appender.AbstractAppender; +import org.junit.After; +import org.junit.Test; + +/** + * Unit tests for the per-phase read contract of the five {@link ContentFactoryIndexOperations} + * read operations that {@code ESContentFactoryImpl} funnels essentially all content search + * through (issue #37413). + * + *

What regressed

+ *

{@code ESContentFactoryImpl} picked a provider with a bare ternary and called it directly, + * so {@link PhaseRouter#read} — which holds the Phase 2 Elasticsearch fallback — was unreachable + * from the busiest read path in the product. With OpenSearch down in Phase 2 and Elasticsearch + * healthy and dual-written, {@code POST /api/content/_search} returned either a well-formed + * {@code 200} with zero results (indistinguishable from "this content type has no content") or + * a {@code 500}, instead of the result set Elasticsearch was holding the whole time.

+ * + *

Contract under test

+ * 
+ * Phase │ Provider read │ On OpenSearch failure
+ * ──────┼───────────────┼──────────────────────────────────────────────────────
+ *   0   │ ES            │ n/a — OS is never contacted
+ *   1   │ ES            │ n/a — OS is never contacted
+ *   2   │ OS            │ logged at ERROR, retried against ES, ES result returned
+ *   3   │ OS            │ propagates — ES is decommissioned, there is no fallback
+ *
+ * + *

Why this tests the router and not {@code ESContentFactoryImpl}

+ *

{@code ESContentFactoryImpl} cannot be constructed outside a container: its read methods + * run through static {@code CacheLocator}/{@code APILocator} calls — {@code indexCount} and + * {@code indexSearch} both go through the static {@code translateQuery} — before they reach a + * provider. So the per-phase contract is pinned here, against the real provider interface, and + * the separate question of whether {@code ESContentFactoryImpl} actually consults the router is + * proved by {@code ESContentFactoryImplPhase2FallbackTest} in {@code dotcms-integration}. A unit + * test on the router cannot show which class calls it.

+ * + * @author Fabrizzio Araya + */ +public class ContentFactoryIndexOperationsPhaseRoutingTest { + + /** The failure an unreachable node produces once the provider has wrapped it. */ + private static DotRuntimeException unreachable() { + return new DotRuntimeException("An error occurred when executing the Lucene Query", + new java.net.ConnectException("Connection refused")); + } + + @After + public void clearPhase() { + Config.setProperty(FLAG_KEY, null); + } + + private static void setPhase(final MigrationPhase phase) { + Config.setProperty(FLAG_KEY, String.valueOf(phase.ordinal())); + } + + private static PhaseRouter router( + final ContentFactoryIndexOperations es, final ContentFactoryIndexOperations os) { + return new PhaseRouter<>(es, os); + } + + // ========================================================================= + // indexCount — the operation that produced the 500s, because ContentHelper + // runs the count before the search and an uncached count threw. + // ========================================================================= + + /** + * Given : Phase 2, OpenSearch unreachable, Elasticsearch healthy and holding 184 documents. + * When : indexCount runs through the router. + * Then : the Elasticsearch count is returned — not 0, and not an exception. + */ + @Test + public void indexCount_phase2_osUnreachable_fallsBackToEs() { + setPhase(MigrationPhase.PHASE_2_DUAL_WRITE_OS_READS); + final ContentFactoryIndexOperations es = mock(ContentFactoryIndexOperations.class); + final ContentFactoryIndexOperations os = mock(ContentFactoryIndexOperations.class); + when(os.indexCount(anyString())).thenThrow(unreachable()); + when(es.indexCount(anyString())).thenReturn(184L); + + final long count = router(es, os).read(impl -> impl.indexCount("+contentType:Profile")); + + assertEquals("Phase 2 must serve the count Elasticsearch holds, not 0", 184L, count); + verify(os, times(1)).indexCount(anyString()); + verify(es, times(1)).indexCount(anyString()); + } + + /** + * Given : Phase 3, OpenSearch unreachable. + * When : indexCount runs through the router. + * Then : the failure propagates and Elasticsearch is never contacted — it is decommissioned + * in Phase 3, so a silent fallback there would report data that is genuinely gone. + */ + @Test + public void indexCount_phase3_osUnreachable_propagatesAndNeverTouchesEs() { + setPhase(MigrationPhase.PHASE_3_OPENSEARCH_ONLY); + final ContentFactoryIndexOperations es = mock(ContentFactoryIndexOperations.class); + final ContentFactoryIndexOperations os = mock(ContentFactoryIndexOperations.class); + when(os.indexCount(anyString())).thenThrow(unreachable()); + + assertThrows(DotRuntimeException.class, + () -> router(es, os).read(impl -> impl.indexCount("+contentType:Profile"))); + + verifyNoInteractions(es); + } + + /** + * Given : Phase 0 and Phase 1, where Elasticsearch serves reads. + * When : indexCount runs through the router. + * Then : Elasticsearch answers and OpenSearch is never contacted — the routing change must + * be a pure pass-through outside Phase 2. + */ + @Test + public void indexCount_phases0And1_readEsAndNeverTouchOs() { + for (final MigrationPhase phase : List.of(MigrationPhase.PHASE_0_MIGRATION_NOT_STARTED, + MigrationPhase.PHASE_1_DUAL_WRITE_ES_READS)) { + setPhase(phase); + final ContentFactoryIndexOperations es = mock(ContentFactoryIndexOperations.class); + final ContentFactoryIndexOperations os = mock(ContentFactoryIndexOperations.class); + when(es.indexCount(anyString())).thenReturn(27L); + + final long count = router(es, os).read(impl -> impl.indexCount("+contentType:JobPosting")); + + assertEquals("In " + phase + " the count must come from Elasticsearch", 27L, count); + verifyNoInteractions(os); + } + } + + // ========================================================================= + // searchHits — the operation behind indexSearch, and the one that produced + // the silent 200/total=0 once the legacy layer swallowed the failure. + // ========================================================================= + + /** + * Given : Phase 2, OpenSearch unreachable, Elasticsearch holding real hits. + * When : searchHits runs through the router. + * Then : the Elasticsearch hits are returned — the same object, so nothing substitutes an + * empty result on the way back. + */ + @Test + public void searchHits_phase2_osUnreachable_fallsBackToEs() { + setPhase(MigrationPhase.PHASE_2_DUAL_WRITE_OS_READS); + final ContentFactoryIndexOperations es = mock(ContentFactoryIndexOperations.class); + final ContentFactoryIndexOperations os = mock(ContentFactoryIndexOperations.class); + final SearchHits esHits = SearchHits.builder().totalHits(TotalHits.builder().value(184L).build()).build(); + when(os.searchHits(anyString(), anyInt(), anyInt(), anyString())).thenThrow(unreachable()); + when(es.searchHits(anyString(), anyInt(), anyInt(), anyString())).thenReturn(esHits); + + final SearchHits hits = router(es, os) + .read(impl -> impl.searchHits("+contentType:Profile +live:true", 7, 1, "title asc")); + + assertSame("Phase 2 must return the Elasticsearch hits untouched", esHits, hits); + verify(es, times(1)).searchHits(anyString(), anyInt(), anyInt(), anyString()); + } + + /** + * Given : Phase 3, OpenSearch unreachable. + * When : searchHits runs through the router. + * Then : the failure propagates; Elasticsearch is never contacted. + */ + @Test + public void searchHits_phase3_osUnreachable_propagates() { + setPhase(MigrationPhase.PHASE_3_OPENSEARCH_ONLY); + final ContentFactoryIndexOperations es = mock(ContentFactoryIndexOperations.class); + final ContentFactoryIndexOperations os = mock(ContentFactoryIndexOperations.class); + when(os.searchHits(anyString(), anyInt(), anyInt(), anyString())).thenThrow(unreachable()); + + assertThrows(DotRuntimeException.class, () -> router(es, os) + .read(impl -> impl.searchHits("+contentType:Hero", 10, 0, "title asc"))); + + verifyNoInteractions(es); + } + + // ========================================================================= + // search — inode-only search, behind findContentletsByHost + // ========================================================================= + + /** + * Given : Phase 2, OpenSearch unreachable, Elasticsearch holding two inodes. + * When : search runs through the router. + * Then : the Elasticsearch inodes are returned rather than an empty list. + */ + @Test + public void search_phase2_osUnreachable_fallsBackToEs() { + setPhase(MigrationPhase.PHASE_2_DUAL_WRITE_OS_READS); + final ContentFactoryIndexOperations es = mock(ContentFactoryIndexOperations.class); + final ContentFactoryIndexOperations os = mock(ContentFactoryIndexOperations.class); + when(os.search(anyString(), anyInt(), anyInt())).thenThrow(unreachable()); + when(es.search(anyString(), anyInt(), anyInt())).thenReturn(List.of("inode-a", "inode-b")); + + final List inodes = router(es, os) + .read(impl -> impl.search("+conhost:48190c8c", 10, 0)); + + assertEquals(List.of("inode-a", "inode-b"), inodes); + } + + // ========================================================================= + // indexSearchScroll — drains the whole scroll internally, so a failure means + // the operation produced nothing and can simply be re-run against ES. + // ========================================================================= + + /** + * Given : Phase 2, OpenSearch unreachable, Elasticsearch able to serve the scroll. + * When : indexSearchScroll runs through the router. + * Then : the Elasticsearch result is returned. This site can fall back safely precisely + * because it materialises the entire scroll before returning — there is no cursor + * left half-drained on the failing engine. + */ + @Test + public void indexSearchScroll_phase2_osUnreachable_fallsBackToEs() { + setPhase(MigrationPhase.PHASE_2_DUAL_WRITE_OS_READS); + final ContentFactoryIndexOperations es = mock(ContentFactoryIndexOperations.class); + final ContentFactoryIndexOperations os = mock(ContentFactoryIndexOperations.class); + final PaginatedArrayList esResults = new PaginatedArrayList<>(); + esResults.setTotalResults(317L); + when(os.indexSearchScroll(anyString(), anyString(), anyInt())).thenThrow(unreachable()); + when(es.indexSearchScroll(anyString(), anyString(), anyInt())).thenReturn(esResults); + + final PaginatedArrayList results = router(es, os) + .read(impl -> impl.indexSearchScroll("+contentType:Testimonials", "title asc", 1000)); + + assertEquals(317L, results.getTotalResults()); + verify(es, times(1)).indexSearchScroll(anyString(), anyString(), anyInt()); + } + + /** + * Given : Phase 3, OpenSearch unreachable. + * When : indexSearchScroll runs through the router. + * Then : the failure propagates. + */ + @Test + public void indexSearchScroll_phase3_osUnreachable_propagates() { + setPhase(MigrationPhase.PHASE_3_OPENSEARCH_ONLY); + final ContentFactoryIndexOperations es = mock(ContentFactoryIndexOperations.class); + final ContentFactoryIndexOperations os = mock(ContentFactoryIndexOperations.class); + when(os.indexSearchScroll(anyString(), anyString(), anyInt())).thenThrow(unreachable()); + + assertThrows(DotRuntimeException.class, () -> router(es, os) + .read(impl -> impl.indexSearchScroll("+contentType:Testimonials", "title asc", 1000))); + + verifyNoInteractions(es); + } + + // ========================================================================= + // createScrollQuery — routed for provider selection only. + // ========================================================================= + + /** + * Given : Phase 2 and a working OpenSearch provider. + * When : createScrollQuery runs through the router. + * Then : the OpenSearch cursor is returned and Elasticsearch is never contacted. + * + *

This site is routed so that provider selection has exactly one mechanism in the class, + * but its fallback can never fire and that is correct, not an oversight: the OpenSearch + * implementation is {@code return new OSContentletScrollImpl(...)} — pure construction, no + * I/O, nothing to throw. The requests happen later, inside the returned cursor, outside the + * router. A mid-scroll fallback is impossible in principle anyway, because an OpenSearch + * scroll id is meaningless to Elasticsearch, so a half-drained scroll cannot be resumed on + * the other engine. The residual gap is documented at the call site.

+ */ + @Test + public void createScrollQuery_phase2_selectsOsAndDoesNotTouchEs() { + setPhase(MigrationPhase.PHASE_2_DUAL_WRITE_OS_READS); + final ContentFactoryIndexOperations es = mock(ContentFactoryIndexOperations.class); + final ContentFactoryIndexOperations os = mock(ContentFactoryIndexOperations.class); + final IndexContentletScroll osScroll = mock(IndexContentletScroll.class); + when(os.createScrollQuery(anyString(), any(), anyBoolean(), anyInt(), anyString())) + .thenReturn(osScroll); + + final IndexContentletScroll scroll = router(es, os).read(impl -> + impl.createScrollQuery("+contentType:Profile", null, false, 100, "title asc")); + + assertSame(osScroll, scroll); + verifyNoInteractions(es); + } + + /** + * Given : Phase 1, where Elasticsearch serves reads. + * When : createScrollQuery runs through the router. + * Then : the Elasticsearch cursor is returned and OpenSearch is never contacted. + */ + @Test + public void createScrollQuery_phase1_selectsEs() { + setPhase(MigrationPhase.PHASE_1_DUAL_WRITE_ES_READS); + final ContentFactoryIndexOperations es = mock(ContentFactoryIndexOperations.class); + final ContentFactoryIndexOperations os = mock(ContentFactoryIndexOperations.class); + final IndexContentletScroll esScroll = mock(IndexContentletScroll.class); + when(es.createScrollQuery(anyString(), any(), anyBoolean(), anyInt(), anyString())) + .thenReturn(esScroll); + + final IndexContentletScroll scroll = router(es, os).read(impl -> + impl.createScrollQuery("+contentType:Profile", null, false, 100, "title asc")); + + assertSame(esScroll, scroll); + verifyNoInteractions(os); + } + + // ========================================================================= + // The fallback must be a single attempt — a failing read costs one extra + // call against ES, with no nesting and no retry storm. + // ========================================================================= + + /** + * Given : Phase 2 with both engines failing on the same read. + * When : the read runs through the router. + * Then : the Elasticsearch failure surfaces to the caller, and each provider was called + * exactly once. A two-engine outage is the one case where a caller now sees an error + * where it previously saw an empty result — which is the point of the issue, since + * the silent-empty variant is the dangerous one. + */ + @Test + public void phase2_bothEnginesFail_surfacesErrorAfterExactlyOneAttemptEach() { + setPhase(MigrationPhase.PHASE_2_DUAL_WRITE_OS_READS); + final ContentFactoryIndexOperations es = mock(ContentFactoryIndexOperations.class); + final ContentFactoryIndexOperations os = mock(ContentFactoryIndexOperations.class); + when(os.indexCount(anyString())).thenThrow(unreachable()); + when(es.indexCount(anyString())).thenThrow(unreachable()); + + assertThrows(DotRuntimeException.class, + () -> router(es, os).read(impl -> impl.indexCount("+contentType:Profile"))); + + verify(os, times(1)).indexCount(anyString()); + verify(es, times(1)).indexCount(anyString()); + } + + /** + * Given : Phase 2 and a healthy OpenSearch. + * When : the read succeeds. + * Then : Elasticsearch is never contacted. The fallback must cost nothing on the success + * path — otherwise Phase 2 would double every read in the product. + */ + @Test + public void phase2_osHealthy_neverTouchesEs() { + setPhase(MigrationPhase.PHASE_2_DUAL_WRITE_OS_READS); + final ContentFactoryIndexOperations es = mock(ContentFactoryIndexOperations.class); + final ContentFactoryIndexOperations os = mock(ContentFactoryIndexOperations.class); + when(os.indexCount(anyString())).thenReturn(215L); + + final long count = router(es, os).read(impl -> impl.indexCount("+contentType:Hero")); + + assertEquals(215L, count); + + verify(es, never()).indexCount(anyString()); + } + + // ========================================================================= + // The fallback log line (AC-003). + // + // The pre-fix path logged these failures at WARN, which is why an outage was + // invisible to log-based monitoring even though the design documents it as the + // early-warning signal. Level is therefore part of the contract, not cosmetic: + // a test that only checked "something was logged" would pass against the old + // behaviour. + // ========================================================================= + + /** Captures events published to a single log4j2 logger. */ + private static final class CapturingAppender extends AbstractAppender { + + private final List events = new ArrayList<>(); + + CapturingAppender() { + super("capture-phase-router", null, null, true, null); + } + + @Override + public void append(final LogEvent event) { + events.add(event.toImmutable()); + } + } + + private CapturingAppender attachAppender() { + final CapturingAppender appender = new CapturingAppender(); + appender.start(); + final org.apache.logging.log4j.core.Logger logger = + (org.apache.logging.log4j.core.Logger) LogManager.getLogger(PhaseRouter.class); + logger.addAppender(appender); + return appender; + } + + private void detachAppender(final CapturingAppender appender) { + final org.apache.logging.log4j.core.Logger logger = + (org.apache.logging.log4j.core.Logger) LogManager.getLogger(PhaseRouter.class); + logger.removeAppender(appender); + appender.stop(); + } + + /** + * Given : Phase 2 and an unreachable OpenSearch. + * When : a named read falls back. + * Then : exactly one event is logged, at ERROR, naming both the operation that failed and + * the cause, with the throwable attached. One event per read — not per provider, and + * with no retry storm behind it. + */ + @Test + public void phase2_fallback_logsOnceAtErrorNamingOperationAndCause() { + setPhase(MigrationPhase.PHASE_2_DUAL_WRITE_OS_READS); + final ContentFactoryIndexOperations es = mock(ContentFactoryIndexOperations.class); + final ContentFactoryIndexOperations os = mock(ContentFactoryIndexOperations.class); + when(os.indexCount(anyString())).thenThrow(unreachable()); + when(es.indexCount(anyString())).thenReturn(184L); + + final CapturingAppender appender = attachAppender(); + try { + router(es, os).read("indexCount", impl -> impl.indexCount("+contentType:Profile")); + } finally { + detachAppender(appender); + } + + assertEquals("A fallback must log exactly one event per read", 1, appender.events.size()); + final LogEvent event = appender.events.get(0); + assertEquals("The fallback must be ERROR — WARN is what monitoring was missing", + Level.ERROR, event.getLevel()); + final String message = event.getMessage().getFormattedMessage(); + assertTrue("The log line must name the failing operation, so monitoring can say what " + + "stopped working. Was: " + message, + message.contains("indexCount")); + assertTrue("The log line must carry the cause. Was: " + message, + message.contains("Connection refused")); + assertNotNull("The throwable must be attached so the stack identifies the call site", + event.getThrown()); + } + + /** + * Given : Phase 3 and an unreachable OpenSearch. + * When : the read propagates instead of falling back. + * Then : no fallback event is logged. A fallback log line in Phase 3 would be a false + * signal — there is no fallback there, and Elasticsearch is decommissioned. + */ + @Test + public void phase3_failure_logsNoFallbackEvent() { + setPhase(MigrationPhase.PHASE_3_OPENSEARCH_ONLY); + final ContentFactoryIndexOperations es = mock(ContentFactoryIndexOperations.class); + final ContentFactoryIndexOperations os = mock(ContentFactoryIndexOperations.class); + when(os.indexCount(anyString())).thenThrow(unreachable()); + + final CapturingAppender appender = attachAppender(); + try { + assertThrows(DotRuntimeException.class, () -> router(es, os) + .read("indexCount", impl -> impl.indexCount("+contentType:Profile"))); + } finally { + detachAppender(appender); + } + + assertEquals("Phase 3 must not report a fallback", 0, appender.events.size()); + } +} diff --git a/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java b/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java index f9332c46ce24..773e3d936582 100644 --- a/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java +++ b/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java @@ -96,6 +96,7 @@ com.dotcms.content.elasticsearch.business.ESIndexSpeedTest.class, com.dotcms.content.elasticsearch.business.ES6UpgradeTest.class, com.dotcms.content.elasticsearch.business.ESContentFactoryImplTest.class, + com.dotcms.content.elasticsearch.business.ESContentFactoryImplPhase2FallbackTest.class, com.dotcms.graphql.datafetcher.page.ContentMapDataFetcherTest.class, com.dotcms.graphql.datafetcher.RelationshipFieldDataFetcherTest.class, com.dotcms.rest.StoryBlockMarkdownPopulatorTest.class, diff --git a/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ESContentFactoryImplPhase2FallbackTest.java b/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ESContentFactoryImplPhase2FallbackTest.java new file mode 100644 index 000000000000..b353dcccb92f --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ESContentFactoryImplPhase2FallbackTest.java @@ -0,0 +1,218 @@ +package com.dotcms.content.elasticsearch.business; + +import static com.dotcms.content.index.IndexConfigHelper.MigrationPhase.FLAG_KEY; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import com.dotcms.content.index.ContentFactoryIndexOperations; +import com.dotcms.content.index.IndexConfigHelper.MigrationPhase; +import com.dotcms.content.index.IndexContentletScroll; +import com.dotcms.content.index.domain.SearchHits; +import com.dotcms.contenttype.model.type.ContentType; +import com.dotcms.datagen.ContentTypeDataGen; +import com.dotcms.datagen.ContentletDataGen; +import com.dotcms.util.IntegrationTestInitService; +import com.dotmarketing.business.CacheLocator; +import com.dotmarketing.common.model.ContentletSearch; +import com.dotmarketing.exception.DotRuntimeException; +import com.dotmarketing.portlets.contentlet.model.Contentlet; +import com.dotmarketing.portlets.contentlet.model.IndexPolicy; +import com.dotmarketing.util.Config; +import com.dotmarketing.util.PaginatedArrayList; +import com.liferay.portal.model.User; +import java.util.List; +import org.junit.After; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Integration tests proving that {@link ESContentFactoryImpl} routes its reads through + * {@link com.dotcms.content.index.PhaseRouter}, so the Phase 2 Elasticsearch fallback actually + * fires for content search (#37413). + * + *

What regressed

+ *

The fallback was implemented and correct; the content read path simply never reached it. + * {@code ESContentFactoryImpl} selected a provider with a bare ternary and called it directly + * from five read call sites, so the router was unreachable from the busiest read path in the + * product. In Phase 2 with OpenSearch down and Elasticsearch healthy and dual-written, + * {@code POST /api/content/_search} returned a well-formed {@code 200} with zero results — which + * a caller cannot tell apart from "this content type has no content", so a live site rendered as + * missing content with nothing for 5xx monitoring to catch.

+ * + *

What this test adds over the unit tests

+ *

{@code ContentFactoryIndexOperationsPhaseRoutingTest} pins the per-phase fallback contract + * against the provider interface, but a unit test on the router cannot show which class + * calls it — and that was the entire defect. This test is the wiring proof: it injects an + * OpenSearch provider that fails every read alongside the real Elasticsearch provider, and + * asserts that the factory's own read methods still return real Elasticsearch data. Before the + * fix these calls propagate the injected failure, because nothing catches it.

+ * + * @author Fabrizzio Araya + */ +public class ESContentFactoryImplPhase2FallbackTest { + + private static ContentType contentType; + private static String query; + + @BeforeClass + public static void prepare() throws Exception { + IntegrationTestInitService.getInstance().init(); + contentType = new ContentTypeDataGen().nextPersisted(); + // WAIT_FOR so the document is searchable before the assertions run. + final Contentlet contentlet = new ContentletDataGen(contentType.id()) + .setPolicy(IndexPolicy.WAIT_FOR).nextPersistedAndPublish(); + assertTrue("Test setup must produce a persisted contentlet", + null != contentlet.getInode()); + query = "+contentType:" + contentType.variable() + " +live:true"; + } + + @After + public void clearPhase() { + Config.setProperty(FLAG_KEY, null); + } + + private static void setPhase(final MigrationPhase phase) { + Config.setProperty(FLAG_KEY, String.valueOf(phase.ordinal())); + } + + /** + * An OpenSearch provider standing in for an unreachable node: every read fails the way the + * real provider's generic handler wraps a connection failure. + */ + private static final class UnreachableOpenSearch implements ContentFactoryIndexOperations { + + private static DotRuntimeException down() { + return new DotRuntimeException("An error occurred when executing the Lucene Query", + new java.net.ConnectException("Connection refused")); + } + + @Override + public String inferIndexToHit(final String query) { + throw down(); + } + + @Override + public long indexCount(final String query) { + throw down(); + } + + @Override + public SearchHits searchHits(final String query, final int limit, final int offset, + final String sortBy) { + throw down(); + } + + @Override + public List search(final String query, final int limit, final int offset) { + throw down(); + } + + @Override + public PaginatedArrayList indexSearchScroll(final String query, + final String sortBy, final int scrollBatchSize) { + throw down(); + } + + @Override + public IndexContentletScroll createScrollQuery(final String luceneQuery, final User user, + final boolean respectFrontendRoles, final int batchSize, final String sortBy) { + throw down(); + } + + @Override + public IndexContentletScroll createScrollQuery(final String luceneQuery, final User user, + final boolean respectFrontendRoles, final int batchSize) { + throw down(); + } + } + + /** A factory whose OpenSearch leg is dead and whose Elasticsearch leg is the real one. */ + private static ESContentFactoryImpl factoryWithDeadOpenSearch() { + return new ESContentFactoryImpl( + new ContentFactoryIndexOperationsES(CacheLocator.getESQueryCache()), + new UnreachableOpenSearch()); + } + + /** + * Method to test: {@link ESContentFactoryImpl#indexCount(String)} + * Given Scenario: Phase 2, OpenSearch unreachable, Elasticsearch healthy and holding the + * content. This is the call that produced the 500s, because the count runs + * before the search and an uncached count threw. + * Expected Result: the real Elasticsearch count comes back — not 0, and not an exception. + */ + @Test + public void indexCount_phase2_deadOpenSearch_servesElasticsearchCount() { + setPhase(MigrationPhase.PHASE_2_DUAL_WRITE_OS_READS); + + final long count = factoryWithDeadOpenSearch().indexCount(query); + + assertEquals("Phase 2 must fall back to the count Elasticsearch holds", 1L, count); + } + + /** + * Method to test: {@link ESContentFactoryImpl#indexSearch(String, int, int, String)} + * Given Scenario: Phase 2, OpenSearch unreachable, Elasticsearch healthy. + * Expected Result: the real hits come back. A zero-hit result here is the silent variant of + * the outage — the dangerous one, because it looks like an empty content + * type rather than a failure. + */ + @Test + public void indexSearch_phase2_deadOpenSearch_servesElasticsearchHits() { + setPhase(MigrationPhase.PHASE_2_DUAL_WRITE_OS_READS); + + final SearchHits hits = factoryWithDeadOpenSearch() + .indexSearch(query, 10, 0, "modDate desc"); + + assertEquals("Phase 2 must fall back to the hits Elasticsearch holds", + 1L, hits.getTotalHits().value()); + } + + /** + * Method to test: {@link ESContentFactoryImpl#indexSearchScroll(String, String)} + * Given Scenario: Phase 2, OpenSearch unreachable, Elasticsearch healthy. + * Expected Result: the scroll is served from Elasticsearch. This site can fall back safely + * because it materialises the whole scroll before returning, so a failure + * leaves no cursor half-drained on the failing engine. + */ + @Test + public void indexSearchScroll_phase2_deadOpenSearch_servesElasticsearchResults() { + setPhase(MigrationPhase.PHASE_2_DUAL_WRITE_OS_READS); + + final PaginatedArrayList results = + factoryWithDeadOpenSearch().indexSearchScroll(query, "modDate desc"); + + assertEquals("Phase 2 must fall back to the scroll Elasticsearch can serve", + 1, results.size()); + } + + /** + * Method to test: {@link ESContentFactoryImpl#indexCount(String)} + * Given Scenario: Phase 1, where Elasticsearch serves reads and OpenSearch is only a shadow. + * Expected Result: the count is served without the dead OpenSearch provider being consulted + * at all. The routing change has to be a pure pass-through outside Phase 2 — + * these five call sites carry essentially all content search in the product, + * so any behaviour change here is felt site-wide in every phase. + */ + @Test + public void indexCount_phase1_deadOpenSearchIsNeverConsulted() { + setPhase(MigrationPhase.PHASE_1_DUAL_WRITE_ES_READS); + + final long count = factoryWithDeadOpenSearch().indexCount(query); + + assertEquals("Phase 1 reads Elasticsearch and must not touch OpenSearch", 1L, count); + } + + /** + * Method to test: {@link ESContentFactoryImpl#indexCount(String)} + * Given Scenario: Phase 0, the default — migration not started. + * Expected Result: same as Phase 1. Elasticsearch answers; OpenSearch is never contacted. + */ + @Test + public void indexCount_phase0_deadOpenSearchIsNeverConsulted() { + setPhase(MigrationPhase.PHASE_0_MIGRATION_NOT_STARTED); + + final long count = factoryWithDeadOpenSearch().indexCount(query); + + assertEquals("Phase 0 reads Elasticsearch and must not touch OpenSearch", 1L, count); + } +} From 3e2cab1c48413c5ed291ff8f57453493bd65581b Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Thu, 10 Sep 2026 12:04:26 -0600 Subject: [PATCH 3/5] fix(index): stop the OpenSearch provider from turning a read failure into an empty result in Phase 2 (#37413) Routing the read path through PhaseRouter fixes an unreachable OpenSearch, because a connection failure reaches the provider's generic handler and is rethrown as a runtime exception the router catches. A node that is reachable but answers with an error is a different failure class, and routing does not fix it: the provider absorbs it into a legitimate-looking success. It then reports no failure at all, so the router has nothing to catch and cannot fall back. Four such absorbing paths, all now conditional on Phase 2: - searchHits() index resolution. A catch(Exception) around inferIndexToHit returns an empty SearchHits, and a null index does the same. This one was NOT in the issue's analysis and is the most consequential of the four: it sits on the exact path the issue reports, and it converts ANY failure to determine which index to query into a clean empty result while logging at ERROR and returning success. - cachedIndexSearch() ERROR_HIT sentinel. - cachedIndexCount() -1 sentinel. - indexSearchScroll(), which carries its own separate handler returning an empty list -- also absent from the issue's analysis. This explains the asymmetry the issue described (200/total=0 for some content types, 500 for others) more precisely than the spec does. The spec attributed the empty 200 to the legacy ContentUtils catch(Throwable). There is an earlier swallow, in the provider: the SAME index-resolution failure propagates on the count and scroll paths -- so the router falls back and the caller gets correct data -- while the search path converts it to empty on the spot. The spec's explanation is not wrong about the legacy layer, it just stops one layer short of the provider. Scoping the change to Phase 2 is what makes it safe, and it is why AC-006's enumeration of callers relying on empty-instead-of-throw is unnecessary: in Phase 2 the router catches the raised failure immediately above this class and turns it into a successful Elasticsearch read, so no caller ever observes a new exception type. Phases 0 and 1 do not read from OpenSearch at all; Phase 3 has no Elasticsearch to fall back to, so raising there would expose every such caller with nothing gained. The one genuine change: both engines failing on the same read now surfaces an error instead of a silent empty result, which is the improvement being asked for. ContentFactoryIndexOperationsES is deliberately untouched (AC-006). On the two cached paths the raise happens BEFORE the cache write. The provider caches its sentinel for parse_exception and search_phase_execution_exception; such an entry would be replayed to every later identical query as a successful empty result, outliving the outage and defeating the fallback even after OpenSearch recovers. A test asserts the ordering rather than trusting it. Testing notes. The first version of this test relied on the container having no OpenSearch counterpart indices, which looked like the real customer scenario but was not: it produced dotCMS's own index-resolution failure, thrown before the client is ever called, so it re-exercised the routing fix and index_not_found_exception never appeared once in the run. Its precondition check was too weak -- it verified OpenSearch did not return the right answer, not that it failed for the right reason. Mocking the client is what makes the failure the one AC-005 is about, and the precondition now asserts it. Two gaps, deliberate and recorded rather than silently skipped: - The scroll swallow has no behavioural test. The scroll resolves its client through CDI rather than the injected provider, so a mocked client cannot reach it. The fix covers it; only the proof is missing. - That ContentFactoryIndexOperationsES is unchanged is evidenced by the diff, not by a behavioural test: its client is a static singleton with no injection seam, so no index_not_found can be forced through it. The Phase 0 and Phase 1 tests do show Elasticsearch-served reads behaving identically. Co-Authored-By: Claude Opus 5 (1M context) --- .../ContentFactoryIndexOperationsOS.java | 54 +++- .../src/test/java/com/dotcms/MainSuite1b.java | 1 + ...SContentFactoryImplMissingOsIndexTest.java | 299 ++++++++++++++++++ 3 files changed, 353 insertions(+), 1 deletion(-) create mode 100644 dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ESContentFactoryImplMissingOsIndexTest.java diff --git a/dotCMS/src/main/java/com/dotcms/content/index/opensearch/ContentFactoryIndexOperationsOS.java b/dotCMS/src/main/java/com/dotcms/content/index/opensearch/ContentFactoryIndexOperationsOS.java index 8cd2832390d0..8506fafbae04 100644 --- a/dotCMS/src/main/java/com/dotcms/content/index/opensearch/ContentFactoryIndexOperationsOS.java +++ b/dotCMS/src/main/java/com/dotcms/content/index/opensearch/ContentFactoryIndexOperationsOS.java @@ -84,6 +84,33 @@ private boolean shouldQueryCache(final String exceptionMsg) { exception.contains("search_phase_execution_exception"); } + + /** + * Whether a failure this provider would otherwise absorb must be raised instead. + * + *

These read paths historically convert a failure into a legitimate-looking empty result + * — an empty {@code SearchHits}, the {@code ERROR_HIT} sentinel, {@code -1} for a count, an + * empty scroll list. The provider then reports success, so the phase router sees nothing to + * catch and cannot fall back: with OpenSearch unable to answer in Phase 2, a search returned + * zero results while Elasticsearch held the data the whole time (issue #37413).

+ * + *

The change is scoped to Phase 2 on purpose, and the scoping is what makes it safe. In + * Phase 2 the router catches the raised failure immediately above this class and turns it + * into a successful Elasticsearch read, so no caller ever observes a new exception + * type — which is why no enumeration of callers relying on empty-instead-of-throw + * is needed. In every other phase the absorbing behaviour is untouched: phases 0 and 1 do + * not read from OpenSearch at all, and Phase 3 has no Elasticsearch to fall back to, so + * raising there would expose every such caller with nothing gained.

+ * + *

The one genuine behaviour change: when both engines fail on the same read the caller now + * receives an error rather than a silent empty result. That is the improvement being asked + * for — the silent variant is the dangerous one, because a caller cannot tell it apart from + * "this content type has no content".

+ */ + private static boolean mustRaiseForPhase2Fallback() { + return IndexConfigHelper.isReadEnabled() && !IndexConfigHelper.isMigrationComplete(); + } + /** * If enabled SearchRequests are executed and then cached */ @@ -115,6 +142,13 @@ private HitsMetadata cachedIndexSearch(final SearchRequest searchRequest Logger.warn(this.getClass(), String.format("OS Query: %s", String.valueOf(searchRequest))); Logger.warn(this.getClass(), String.format("Class %s: %s", e.getClass().getName(), exceptionMsg)); Logger.warn(this.getClass(), "----------------------------------------------"); + if (mustRaiseForPhase2Fallback()) { + // Not cached: a sentinel stored here would be replayed to every later identical + // query as a successful empty result, outliving the outage and defeating the + // fallback even after OpenSearch recovers. + throw new DotRuntimeException(String.format( + "OpenSearch search failed for [ %s ]: %s", searchRequest, exceptionMsg), e); + } if(shouldQueryCache(exceptionMsg)) { queryCache.put(searchRequest, ERROR_HIT); } @@ -175,6 +209,11 @@ public Long cachedIndexCount(final CountRequest countRequest) { Logger.warn(this.getClass(), String.format("OS Query: %s", countRequest)); Logger.warn(this.getClass(), String.format("Class %s: %s", e.getClass().getName(), exceptionMsg)); Logger.warn(this.getClass(), "----------------------------------------------"); + if (mustRaiseForPhase2Fallback()) { + // Not cached, for the same reason as the search path above. + throw new DotRuntimeException(String.format( + "OpenSearch count failed for [ %s ]: %s", countRequest, exceptionMsg), e); + } if(shouldQueryCache(exceptionMsg)) { queryCache.put(countRequest, -1L); } @@ -204,10 +243,19 @@ public SearchHits searchHits(String query, int limit, int offset, String sortBy) try { indexToHit = inferIndexToHit(query); if (indexToHit == null) { + if (mustRaiseForPhase2Fallback()) { + throw new DotRuntimeException(String.format( + "Unable to determine which OpenSearch index to query for [ %s ]", + query)); + } return SearchHits.empty(); } - } catch (Exception e) { + } catch (final Exception e) { Logger.error(this, "Can't get indices information.", e); + if (mustRaiseForPhase2Fallback()) { + throw new DotRuntimeException( + "Can't get OpenSearch indices information: " + e.getMessage(), e); + } return SearchHits.empty(); } @@ -469,6 +517,10 @@ public PaginatedArrayList indexSearchScroll(String query, Stri Logger.warn(this.getClass(), String.format("OpenSearch error for query: %s", query)); Logger.warn(this.getClass(), String.format("Class %s: %s", e.getClass().getName(), exceptionMsg)); Logger.warn(this.getClass(), "----------------------------------------------"); + if (mustRaiseForPhase2Fallback()) { + throw new DotRuntimeException(String.format( + "OpenSearch scroll failed for [ %s ]: %s", query, exceptionMsg), e); + } return new PaginatedArrayList<>(); } catch (final IllegalStateException e) { Logger.warnAndDebug(ContentFactoryIndexOperationsOS.class, e); diff --git a/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java b/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java index 773e3d936582..4f1369b8535b 100644 --- a/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java +++ b/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java @@ -97,6 +97,7 @@ com.dotcms.content.elasticsearch.business.ES6UpgradeTest.class, com.dotcms.content.elasticsearch.business.ESContentFactoryImplTest.class, com.dotcms.content.elasticsearch.business.ESContentFactoryImplPhase2FallbackTest.class, + com.dotcms.content.elasticsearch.business.ESContentFactoryImplMissingOsIndexTest.class, com.dotcms.graphql.datafetcher.page.ContentMapDataFetcherTest.class, com.dotcms.graphql.datafetcher.RelationshipFieldDataFetcherTest.class, com.dotcms.rest.StoryBlockMarkdownPopulatorTest.class, diff --git a/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ESContentFactoryImplMissingOsIndexTest.java b/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ESContentFactoryImplMissingOsIndexTest.java new file mode 100644 index 000000000000..638a2c343e13 --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ESContentFactoryImplMissingOsIndexTest.java @@ -0,0 +1,299 @@ +package com.dotcms.content.elasticsearch.business; + +import static com.dotcms.content.index.IndexConfigHelper.MigrationPhase.FLAG_KEY; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.dotcms.content.index.IndexConfigHelper.MigrationPhase; +import com.dotcms.content.index.domain.SearchHits; +import com.dotcms.content.index.opensearch.ContentFactoryIndexOperationsOS; +import com.dotcms.content.index.opensearch.OSClientProvider; +import com.dotcms.content.index.opensearch.OSQueryCache; +import com.dotcms.contenttype.model.type.ContentType; +import com.dotcms.datagen.ContentTypeDataGen; +import com.dotcms.datagen.ContentletDataGen; +import com.dotcms.util.IntegrationTestInitService; +import com.dotmarketing.business.CacheLocator; +import com.dotmarketing.portlets.contentlet.model.Contentlet; +import com.dotmarketing.portlets.contentlet.model.IndexPolicy; +import com.dotmarketing.util.Config; +import java.util.Optional; +import org.junit.After; +import org.junit.BeforeClass; +import org.junit.Test; +import org.opensearch.client.opensearch.OpenSearchClient; +import org.opensearch.client.opensearch._types.ErrorCause; +import org.opensearch.client.opensearch._types.ErrorResponse; +import org.opensearch.client.opensearch._types.OpenSearchException; +import org.opensearch.client.opensearch.core.CountRequest; +import org.opensearch.client.opensearch.core.SearchRequest; + +/** + * Integration tests for the second failure class behind + * #37413: an OpenSearch node that is + * reachable but answers with an error — chiefly a missing counterpart index (AC-005), + * which also covers the reactivated pre-migration backup index scenario. + * + *

Why this is a separate failure class from the routing fix

+ *

Routing the read path through the phase router fixes an unreachable OpenSearch: a + * connection failure reaches the provider's generic handler and is rethrown as a runtime + * exception the router catches. A reachable node answering {@code index_not_found_exception} is + * different. That is an {@code OpenSearchException}, which the provider absorbs into a sentinel — + * an empty {@code ERROR_HIT} result for a search, {@code -1} for a count. The provider then + * reports success, the router sees nothing to catch, and the caller receives a legitimate-looking + * empty result. Routing alone does not fix it.

+ * + *

Why the OpenSearch client is mocked here

+ *

An earlier version of this test relied on the container simply having no OpenSearch + * counterpart indices. That looked like the real customer scenario but was not: the failure it + * produced was dotCMS's own index-name resolution giving up ("Unable to load default versioned + * indices"), which is a {@code DotRuntimeException} thrown before the OpenSearch client + * is ever called — so it exercised the routing fix again rather than the sentinel branch, and + * {@code index_not_found_exception} never appeared once in the run. Mocking the client is what + * makes the failure the one AC-005 is actually about.

+ * + *

Coverage this test cannot give

+ *

The scroll path carries a second, independent swallow of the same kind — its own + * handler that logs a warning and returns an empty list. It cannot be driven from here: the + * scroll resolves its client through CDI rather than through the injected provider, so a mocked + * client does not reach it. The fix still covers it; only the behavioural proof is missing, and + * that gap is deliberate and recorded rather than silently skipped.

+ * + * @author Fabrizzio Araya + */ +public class ESContentFactoryImplMissingOsIndexTest { + + private static String query; + + @BeforeClass + public static void prepare() throws Exception { + IntegrationTestInitService.getInstance().init(); + final ContentType contentType = new ContentTypeDataGen().nextPersisted(); + final Contentlet contentlet = new ContentletDataGen(contentType.id()) + .setPolicy(IndexPolicy.WAIT_FOR).nextPersistedAndPublish(); + assertTrue("Test setup must produce a persisted contentlet", + null != contentlet.getInode()); + query = "+contentType:" + contentType.variable() + " +live:true"; + } + + @After + public void clearPhase() { + Config.setProperty(FLAG_KEY, null); + } + + private static void setPhase(final MigrationPhase phase) { + Config.setProperty(FLAG_KEY, String.valueOf(phase.ordinal())); + } + + /** The 404 a reachable OpenSearch returns when the counterpart index does not exist. */ + private static OpenSearchException indexNotFound() { + return new OpenSearchException(ErrorResponse.of(r -> r + .status(404) + .error(ErrorCause.of(c -> c + .type("index_not_found_exception") + .reason("no such index [working_20260101000000.os]"))))); + } + + /** + * An OpenSearch provider whose node is up and answering, but whose counterpart index is gone. + * The query cache is mocked empty so every call reaches the client rather than a cached hit — + * a cached result would mask the failure exactly the way it masked it in QA. + */ + private static ContentFactoryIndexOperationsOS openSearchMissingIndex() throws Exception { + return openSearchMissingIndex(mock(OSQueryCache.class)); + } + + /** Same, with a caller-supplied cache mock so a test can assert what was written to it. */ + private static ContentFactoryIndexOperationsOS openSearchMissingIndex( + final OSQueryCache queryCache) throws Exception { + final OpenSearchClient client = mock(OpenSearchClient.class); + when(client.search(any(SearchRequest.class), any())).thenThrow(indexNotFound()); + when(client.count(any(CountRequest.class))).thenThrow(indexNotFound()); + + final OSClientProvider clientProvider = mock(OSClientProvider.class); + when(clientProvider.getClient()).thenReturn(client); + + when(queryCache.get(any(SearchRequest.class))).thenReturn(Optional.empty()); + when(queryCache.get(any(CountRequest.class))).thenReturn(Optional.empty()); + + return new ContentFactoryIndexOperationsOS(queryCache, clientProvider); + } + + /** The factory with a real Elasticsearch leg and an OpenSearch leg missing its index. */ + private static ESContentFactoryImpl factory() throws Exception { + return new ESContentFactoryImpl( + new ContentFactoryIndexOperationsES(CacheLocator.getESQueryCache()), + openSearchMissingIndex()); + } + + /** + * Method to test: {@link ContentFactoryIndexOperationsOS#indexCount(String)} + * Given Scenario: the OpenSearch provider is asked directly, with its index missing. + * Expected Result: it does NOT return the right count — it either throws or hands back its + * sentinel. + * + *

This is the precondition the rest of the class depends on, and it asserts the failure + * is real rather than assuming it. Without it a green run could mean "the fallback works" or + * "nothing ever failed", and those must not be indistinguishable — the previous version of + * this class passed for exactly that reason while testing the wrong branch.

+ */ + @Test + public void precondition_openSearchCannotServeTheQuery() throws Exception { + long osCount = Long.MIN_VALUE; + try { + osCount = openSearchMissingIndex().indexCount(query); + } catch (final Exception e) { + return; // throwing is one of the two acceptable outcomes + } + assertNotEquals("OpenSearch answered the query correctly, so the missing-index scenario " + + "is not being exercised and the rest of this class would pass vacuously", + 1L, osCount); + } + + /** + * Method to test: {@link ESContentFactoryImpl#indexSearch(String, int, int, String)} + * Given Scenario: Phase 2, OpenSearch reachable but answering + * {@code index_not_found_exception}, Elasticsearch healthy and holding the + * content. + * Expected Result: the real Elasticsearch hits are served. Zero hits here is the silent + * variant of the outage — the dangerous one, because it is indistinguishable + * from an empty content type. + */ + @Test + public void indexSearch_phase2_missingOsIndex_fallsBackToEs() throws Exception { + setPhase(MigrationPhase.PHASE_2_DUAL_WRITE_OS_READS); + + final SearchHits hits = factory().indexSearch(query, 10, 0, "modDate desc"); + + assertEquals("Phase 2 must serve the hits Elasticsearch holds when the OpenSearch " + + "counterpart index is missing", 1L, hits.getTotalHits().value()); + } + + /** + * Method to test: {@link ESContentFactoryImpl#indexCount(String)} + * Given Scenario: Phase 2, OpenSearch answering {@code index_not_found_exception}. + * Expected Result: the Elasticsearch count is served. A count of 0 is the silent variant; + * -1 is the sentinel leaking to the caller. + */ + @Test + public void indexCount_phase2_missingOsIndex_fallsBackToEs() throws Exception { + setPhase(MigrationPhase.PHASE_2_DUAL_WRITE_OS_READS); + + final long count = factory().indexCount(query); + + assertEquals("Phase 2 must serve the count Elasticsearch holds when the OpenSearch " + + "counterpart index is missing", 1L, count); + } + + /** + * Method to test: {@link ESContentFactoryImpl#indexCount(String)} + * Given Scenario: Phase 3, OpenSearch answering {@code index_not_found_exception}. + * Elasticsearch is decommissioned in Phase 3. + * Expected Result: the caller must NOT receive Elasticsearch data. Either the sentinel or an + * exception is correct; silently serving Elasticsearch results would be a + * worse bug than the one being fixed, because it would report data absent + * from the only live engine. + */ + @Test + public void indexCount_phase3_missingOsIndex_neverFallsBackToEs() throws Exception { + setPhase(MigrationPhase.PHASE_3_OPENSEARCH_ONLY); + + long count = Long.MIN_VALUE; + try { + count = factory().indexCount(query); + } catch (final Exception e) { + return; // propagating is a correct Phase 3 outcome + } + assertNotEquals("Phase 3 must not fall back to Elasticsearch — it is decommissioned " + + "there, so serving its data would hide that OpenSearch has no index", + 1L, count); + } + + /** + * Method to test: {@link ESContentFactoryImpl#indexSearch(String, int, int, String)} + * Given Scenario: Phase 3, OpenSearch answering {@code index_not_found_exception}. + * Expected Result: no Elasticsearch data, by the same reasoning as the count above. + */ + @Test + public void indexSearch_phase3_missingOsIndex_neverFallsBackToEs() throws Exception { + setPhase(MigrationPhase.PHASE_3_OPENSEARCH_ONLY); + + SearchHits hits = null; + try { + hits = factory().indexSearch(query, 10, 0, "modDate desc"); + } catch (final Exception e) { + return; // propagating is a correct Phase 3 outcome + } + assertNotEquals("Phase 3 must not fall back to Elasticsearch", + 1L, hits.getTotalHits().value()); + } + + /** + * Method to test: {@link ESContentFactoryImpl#indexCount(String)} + * Given Scenario: Phase 1, where Elasticsearch serves reads and OpenSearch is a shadow. + * Expected Result: the count is correct and the missing OpenSearch index is irrelevant. The + * change must be a pure pass-through outside Phase 2 — these call sites + * carry essentially all content search, so any behaviour change here is + * felt site-wide in every phase. + */ + @Test + public void indexCount_phase1_missingOsIndexIsIrrelevant() throws Exception { + setPhase(MigrationPhase.PHASE_1_DUAL_WRITE_ES_READS); + + assertEquals("Phase 1 reads Elasticsearch regardless of OpenSearch index state", + 1L, factory().indexCount(query)); + } + + /** + * Method to test: {@link ESContentFactoryImpl#indexCount(String)} + * Given Scenario: Phase 0, the default. + * Expected Result: unchanged. Elasticsearch answers; the OpenSearch leg is never consulted. + */ + @Test + public void indexCount_phase0_missingOsIndexIsIrrelevant() throws Exception { + setPhase(MigrationPhase.PHASE_0_MIGRATION_NOT_STARTED); + + assertEquals("Phase 0 behaviour must be untouched", + 1L, factory().indexCount(query)); + } + + /** + * Method to test: {@link ESContentFactoryImpl#indexSearch(String, int, int, String)} + * Given Scenario: Phase 2, OpenSearch answering with an error, and the fallback firing. + * Expected Result: nothing is written to the OpenSearch query cache. + * + *

This is the half of the query-cache concern that is a real defect rather than a + * reproduction hazard. The provider caches its error sentinel for some failure messages; on + * the Phase 2 path such an entry would be replayed to every later identical query as a + * successful empty result, outliving the outage and defeating the fallback even after + * OpenSearch recovers. The raise therefore has to happen before the cache write, and this + * asserts the order rather than trusting it.

+ * + *

A cache hit during an outage is not a defect and is deliberately not tested + * here: the cached value is a real result from a real earlier query, so serving it is + * correct. It matters only because it hides the failure from a human running the + * reproduction — which cuts both ways, since it can just as easily produce a false + * confirmation that a fix works.

+ */ + @Test + public void phase2_fallback_neverWritesTheErrorSentinelToTheQueryCache() throws Exception { + setPhase(MigrationPhase.PHASE_2_DUAL_WRITE_OS_READS); + final OSQueryCache queryCache = mock(OSQueryCache.class); + + final SearchHits hits = new ESContentFactoryImpl( + new ContentFactoryIndexOperationsES(CacheLocator.getESQueryCache()), + openSearchMissingIndex(queryCache)) + .indexSearch(query, 10, 0, "modDate desc"); + + assertEquals("Precondition: the fallback must have served the Elasticsearch hits", + 1L, hits.getTotalHits().value()); + verify(queryCache, never()).put(any(SearchRequest.class), any()); + verify(queryCache, never()).put(any(CountRequest.class), any()); + } +} From 42b3303fd033d6500da11581afb26c628f513d44 Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Thu, 10 Sep 2026 12:05:49 -0600 Subject: [PATCH 4/5] docs(opensearch): the Phase 2 read fallback, its real limits, and why TC-039 gave a false pass (#37413) The migration design has stated the Phase 2 read fallback as a guarantee in four places. For content search it was not true until #37413: the read path selected a provider with a bare ternary and never reached PhaseRouter, so an OpenSearch outage returned a well-formed 200 with zero results, or a 500. Cloud and Support have been quoting that guarantee to customers. The migration doc now states what actually has to hold for the fallback to fire -- the read must go through the router, AND the provider must raise rather than absorb the failure into a legitimate-looking success -- and, more useful operationally, what still does not fall back: Phase 3, a failure mid-scroll, an OpenSearch that answers successfully with stale data, a repeated identical query served from the cache, and the legacy layer that turns any read failure into an empty 200 in every phase including pure-ES installs. It also records the fallback log line, which now names the operation and root cause. TC-039 in the test plan is the case that should have caught this, and it was written correctly except for one instruction: step 3 said to run "the same search again". An identical request body is served from the query cache and returns the pre-outage result, which is indistinguishable from a working fallback -- so the case was structurally guaranteed to pass whether the fallback worked or not. It now requires varying the query, adds the never-queried-content-type variant that used to fail as a 500 rather than an empty 200, and warns about the second false-pass route: checking only for the absence of an error while the returned total is quietly zero. Co-Authored-By: Claude Opus 5 (1M context) --- docs/backend/OPENSEARCH_MIGRATION.md | 40 +++++++++++++++++++ .../backend/OPENSEARCH_MIGRATION_TEST_PLAN.md | 22 ++++++++-- 2 files changed, 58 insertions(+), 4 deletions(-) diff --git a/docs/backend/OPENSEARCH_MIGRATION.md b/docs/backend/OPENSEARCH_MIGRATION.md index daf868a7b737..756032480f88 100644 --- a/docs/backend/OPENSEARCH_MIGRATION.md +++ b/docs/backend/OPENSEARCH_MIGRATION.md @@ -108,6 +108,46 @@ In Phase 2 OS serves reads but ES is still active. If OS throws an exception on `PhaseRouter` catches it, logs at **ERROR** level, and retries against ES automatically. - The caller receives a correct result from ES + +The fallback log line names the operation and the root cause, so an outage is identifiable from +logs alone: + +``` +OS read failed in Phase 2 [indexCount] — falling back to ES. OS index may be stale or +unavailable. Cause: An error occurred when executing the Lucene Query +/ root cause: ConnectException: Connection refused +``` + +**Content search reached this fallback only from #37413 onward.** Before that fix, +`ESContentFactoryImpl` selected a provider with a bare ternary and called it directly from its +five read call sites, so the router — and therefore the fallback — was unreachable from the +busiest read path in the product. An OpenSearch outage in Phase 2 returned a well-formed `200` +with zero results for content types whose count was already cached, and a `500` for the rest. +The empty result is the dangerous variant: a caller cannot tell it apart from "this content type +has no content". If you are reading this against a build that predates that fix, the guarantee +above did not hold for `/api/content/_search`, `ContentletAPI` search and count, Velocity +`$dotcontent.pull`, URL maps, Site Search or the admin content browser. + +Two conditions are required for the fallback to fire, and both took work: + +1. **The read must go through `PhaseRouter`.** Provider selection in `ESContentFactoryImpl` now + has exactly one mechanism; a second one is what caused #37413. +2. **The OpenSearch provider must actually raise.** In Phase 2, `ContentFactoryIndexOperationsOS` + raises instead of absorbing a failure into a legitimate-looking success — the `ERROR_HIT` + sentinel, `-1` for a count, an empty scroll list, and the index-resolution guard in + `searchHits` that returned an empty result for *any* exception. A provider that reports + success gives the router nothing to catch. Outside Phase 2 the absorbing behaviour is + unchanged, and `ContentFactoryIndexOperationsES` is not touched at all. + +**What still does not fall back**, by design or by limitation: + +| Situation | Behaviour | Why | +|---|---|---| +| Phase 3 read failure | Propagates | ES is decommissioned; a silent fallback would report data that is genuinely absent | +| Failure mid-scroll | Propagates | An OS scroll id is meaningless to ES, so a half-drained scroll cannot be resumed on the other engine. `createScrollQuery` is routed for provider selection only | +| OS answers successfully with stale or incomplete data | No fallback | Indistinguishable from a query that legitimately matches nothing; falling back on every empty result would double the load of most searches | +| A repeated identical query during an outage | Served from the query cache | The cached value is a real earlier result, so serving it is correct — but it makes an outage invisible when reproducing by hand. Vary `offset` per call. The error *sentinel* is never cached on the Phase 2 path, so a poisoned entry cannot outlive the outage | +| Any phase, read failure reaching the legacy layer | Empty `200` | `ContentUtils`' `catch (Throwable)` and `ContentHelper`'s `resultsSize = 0` overwrite predate the migration and affect pure-ES installs too. Tracked separately; this is why the outage was silent rather than loud | - The ERROR log makes the OS failure visible for operators - In Phase 3 there is no fallback — ES is decommissioned and OS failures propagate normally diff --git a/docs/backend/OPENSEARCH_MIGRATION_TEST_PLAN.md b/docs/backend/OPENSEARCH_MIGRATION_TEST_PLAN.md index fdd790dabf07..299f0e5dec0c 100644 --- a/docs/backend/OPENSEARCH_MIGRATION_TEST_PLAN.md +++ b/docs/backend/OPENSEARCH_MIGRATION_TEST_PLAN.md @@ -834,13 +834,27 @@ When the migration starts **successfully** (no shutdown) you instead see an `INF - **Steps:** 1. Confirm content is searchable in Phase 2. 2. Break OpenSearch reads (stop OS or remove the OS working index). - 3. Run the same search again. + 3. Search again — but **change the query on every call** (see the warning below). + 4. Also search a content type you have **not** queried since the server started, so its count is + not cached either. That variant used to fail differently: a `500` instead of an empty `200`. - **Expected Result:** - - The search still returns the correct result (served from ES). - - The log shows an ERROR similar to - `OS read failed in Phase 2 — falling back to ES. OS index may be stale or unavailable. Cause: …` + - The search still returns the correct result (served from ES), for every content type that has + live content. Not zero results, and not a `500`. + - The log shows an ERROR naming the operation and the root cause, similar to + `OS read failed in Phase 2 [indexCount] — falling back to ES. OS index may be stale or + unavailable. Cause: … / root cause: ConnectException: Connection refused` - **Type:** Manual +> ⚠️ **This case gave a false PASS before #37413 and can do so again.** Repeating an *identical* +> search returns the pre-outage result from the query cache, which is indistinguishable from a +> working fallback. Vary the query — a different `offset` on each call is enough — or you are +> testing the cache, not the fallback. A count is cached without `offset` in its key, so also +> exercise a content type never queried since startup. +> +> A second false-pass route: if a *count* is what fails, some paths propagate the error while the +> *search* path used to convert it to an empty result on the spot. Check the returned total, not +> just the absence of an error. + ## TC-040 — Phase 3 does NOT auto-rollback (negative case) - **Objective:** Document that the automatic fallback to ES exists only in Phases 1–2. In Phase 3 (ES From 29e05365c3061764e81b917c4bbc5935337584d1 Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Thu, 10 Sep 2026 13:03:40 -0600 Subject: [PATCH 5/5] fix(index): keep the Lucene query out of the Phase 2 fallback log line (#37413) The four failures this change raises in Phase 2 are caught by PhaseRouter and logged at ERROR via e.getMessage(). Three of the messages embedded the full SearchRequest, CountRequest or query string, which promoted possible customer content -- a Lucene query can carry end-user search terms and field values -- from a pre-existing WARN to a new ERROR line (Constitution Principle III). They now carry the index name and OpenSearch's own reason instead. That is the actionable part: the operation name comes from the router, the index says where it failed, and the reason says why. The request body is still on the WARN block directly above for anyone diagnosing a specific query. Residual and deliberately unchanged: those pre-existing WARN blocks still log the full query in every phase, and ContentFactoryIndexOperationsES does the same. Changing that would alter Phase 0 and Phase 1 logging, which the spec declares out of scope. What this commit fixes is the part the fix introduced. Co-Authored-By: Claude Opus 5 (1M context) --- .../ContentFactoryIndexOperationsOS.java | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/content/index/opensearch/ContentFactoryIndexOperationsOS.java b/dotCMS/src/main/java/com/dotcms/content/index/opensearch/ContentFactoryIndexOperationsOS.java index 8506fafbae04..9347793c286e 100644 --- a/dotCMS/src/main/java/com/dotcms/content/index/opensearch/ContentFactoryIndexOperationsOS.java +++ b/dotCMS/src/main/java/com/dotcms/content/index/opensearch/ContentFactoryIndexOperationsOS.java @@ -146,8 +146,16 @@ private HitsMetadata cachedIndexSearch(final SearchRequest searchRequest // Not cached: a sentinel stored here would be replayed to every later identical // query as a successful empty result, outliving the outage and defeating the // fallback even after OpenSearch recovers. + // Index name and OpenSearch's own reason only. The full SearchRequest is + // deliberately left out: in Phase 2 the router logs this message at ERROR, and a + // Lucene query can carry end-user search terms and field values (Constitution + // Principle III). The request body is still available at DEBUG on the WARN block + // above for anyone diagnosing a specific query. throw new DotRuntimeException(String.format( - "OpenSearch search failed for [ %s ]: %s", searchRequest, exceptionMsg), e); + "OpenSearch search failed on index [%s]: %s", + (searchRequest.index() != null) ? String.join(",", searchRequest.index()) + : "unknown", + exceptionMsg), e); } if(shouldQueryCache(exceptionMsg)) { queryCache.put(searchRequest, ERROR_HIT); @@ -211,8 +219,10 @@ public Long cachedIndexCount(final CountRequest countRequest) { Logger.warn(this.getClass(), "----------------------------------------------"); if (mustRaiseForPhase2Fallback()) { // Not cached, for the same reason as the search path above. + // Index name only -- see the search path above for why the request is omitted. throw new DotRuntimeException(String.format( - "OpenSearch count failed for [ %s ]: %s", countRequest, exceptionMsg), e); + "OpenSearch count failed on index [%s]: %s", + countRequest.index(), exceptionMsg), e); } if(shouldQueryCache(exceptionMsg)) { queryCache.put(countRequest, -1L); @@ -244,9 +254,9 @@ public SearchHits searchHits(String query, int limit, int offset, String sortBy) indexToHit = inferIndexToHit(query); if (indexToHit == null) { if (mustRaiseForPhase2Fallback()) { - throw new DotRuntimeException(String.format( - "Unable to determine which OpenSearch index to query for [ %s ]", - query)); + // The query itself is omitted -- see cachedIndexSearch below. + throw new DotRuntimeException( + "Unable to determine which OpenSearch index to query"); } return SearchHits.empty(); } @@ -518,8 +528,9 @@ public PaginatedArrayList indexSearchScroll(String query, Stri Logger.warn(this.getClass(), String.format("Class %s: %s", e.getClass().getName(), exceptionMsg)); Logger.warn(this.getClass(), "----------------------------------------------"); if (mustRaiseForPhase2Fallback()) { - throw new DotRuntimeException(String.format( - "OpenSearch scroll failed for [ %s ]: %s", query, exceptionMsg), e); + // The Lucene query is omitted here for the same reason. + throw new DotRuntimeException( + "OpenSearch scroll failed: " + exceptionMsg, e); } return new PaginatedArrayList<>(); } catch (final IllegalStateException e) {