fix(index): Phase 2 ES read fallback now fires for content search (#37413)

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
 

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

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.
+     *
+     * <p>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.</p>
+     */
+    private final PhaseRouter<ContentFactoryIndexOperations> 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.
+     *
+     * <p>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.</p>
+     *
+     * @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<Contentlet> findContentlets(final List<String> inodes) throws DotDat
 	public List<Contentlet> findContentletsByHost(final String hostId, final int limit,
             final int offset) {
 		try {
-            final List<String> inodes = indexOperationsDelegate().search("+conhost:" + hostId, limit, offset);
+            final List<String> 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<Link> 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<ContentletSearch> 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<ContentletSearch> 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));
     }
 
     /**

dotCMS/src/main/java/com/dotcms/content/index/PhaseRouter.java

@@ -185,19 +185,76 @@ public List<T> writeProviders() {
      * @return    result from the read provider (or ES fallback in Phase 2)
      */
     public <R> R read(final Function<T, R> fn) {
+        return read(null, fn);
+    }
+
+    /**
+     * Same as {@link #read(Function)}, but names the operation in the fallback log line.
+     *
+     * <p>The plain {@link #read(Function)} can only report the cause, because the operation it
+     * runs is an opaque lambda. That is enough to know <em>something</em> fell back, but not
+     * enough for log-based monitoring to say <em>what</em> stopped working — which is the
+     * early-warning signal the migration design promises. Callers on a read path that matters
+     * operationally should pass a name.</p>
+     *
+     * @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> R read(final String operation, final Function<T, R> 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.
+     *
+     * <p>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
+     * <em>what</em> stopped working and <em>why</em> — 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.</p>
+     */
+    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.
      *