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

[fabrizzio-dotCMS]

Fixes #37413. Spec approved in #37438 — this PR branches off that spec branch, so it carries the spec commit too.

What was broken

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 — which holds the fallback — was unreachable from the busiest read path in the product: /api/content/_search, ContentletAPI search and count, Velocity $dotcontent.pull, URL maps, Site Search, scroll consumers and the admin content browser.

With OpenSearch down in Phase 2 and Elasticsearch healthy and dual-written, a content search 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", so a live site rendered as missing content with nothing for 5xx monitoring to catch.

This blocked more than the outage case. Cloud and Support have been telling customers "Phase 2 is safe, OpenSearch failures fall back to Elasticsearch" — quoting OPENSEARCH_MIGRATION.md, which states it in four places. For content search that was false.

Two failure classes, not one

Routing alone fixes an unreachable OpenSearch: 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 answering with an error is different, and routing does not fix it. The provider absorbs the failure into a legitimate-looking success, reports no failure, and the router has nothing to catch. Four such paths, all now conditional on Phase 2:

Path	Absorbed into
searchHits index resolution	empty SearchHits — a catch (Exception) around inferIndexToHit, plus a null-index guard
cachedIndexSearch	the ERROR_HIT sentinel
cachedIndexCount	-1
indexSearchScroll	an empty list, via its own separate handler

Two of those four were not in the issue's analysis — the index-resolution guard and the scroll handler. The first is the most consequential: it sits on the exact path the issue reports and converts any failure to determine which index to query into a clean empty result.

It also explains the reported asymmetry more precisely than the spec does. The spec attributes the empty 200 to the legacy ContentUtils catch (Throwable). That is true but stops one layer short — there is an earlier swallow in the provider, and 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 converted it to empty on the spot.

Why the Phase 2 scoping is what makes this safe

AC-006 asked for an enumeration of every caller that depends on receiving an empty result rather than an exception, warning that if the list were large the provider change should be dropped.

Scoping the raise to Phase 2 makes the enumeration unnecessary: in Phase 2 the router catches the new exception immediately above the provider 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 behaviour change: both engines failing on the same read now surfaces an error instead of a silent empty result. That is the improvement being asked for.

ContentFactoryIndexOperationsES is deliberately untouched.

The log line

The fallback is the migration design's early-warning signal, so it now names the operation and the root cause. Before, it could only report the wrapper's message — enough to know something fell back, not what stopped working.

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

The raised messages carry the index name and OpenSearch's own reason, not the query body: in Phase 2 the router logs the message at ERROR, and a Lucene query can carry end-user search terms (Constitution Principle III). Residual, unchanged: the provider's pre-existing WARN blocks still log the full query, in every phase, and the Elasticsearch provider does the same. Changing that would alter Phase 0/1 logging, which the spec declares out of scope.

Cache poisoning

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

• Unit, 14 new (ContentFactoryIndexOperationsPhaseRoutingTest, in no suite): all five read operations × all four phases, on the router against the real provider interface; ERROR level and one event per read; both-engines-fail surfaces after exactly one attempt each; a healthy Phase 2 never touches Elasticsearch.
• Integration, 13 new (both registered in MainSuite1b): ESContentFactoryImplPhase2FallbackTest proves the wiring by injecting an OpenSearch provider that fails every read; ESContentFactoryImplMissingOsIndexTest covers the reachable-but-erroring class with a mocked client raising index_not_found_exception.
• Regression: 54 unit tests green across the router, both providers and the Phase 2 read-durability tests.

Phases 0, 1 and 3 are verified unchanged by test, not by inspection — these call sites carry essentially all content search, so a behaviour change here is felt site-wide in every phase.

Two notes on how the tests got there, because both are traps worth knowing:

The unit tests do not go red for this bug. 13 of the 14 pass against unmodified code, because the router was never broken — the defect was that nobody called it. Only the integration test can catch that, and it is why the wiring proof is not optional.

The first version of the missing-index test passed for the wrong reason. It relied on the container simply having no OpenSearch counterpart indices, which looked like the customer scenario but produced dotCMS's own index-resolution failure instead — 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 only checked that OpenSearch did not return the right answer, not that it failed for the right reason. The class now mocks the client and asserts the precondition explicitly.

Reviewer decisions I need

1. AC-006's enumeration was not produced; the reasoning above is offered in its place. Accept it or ask for the list.
2. AC-007's cache-hit clause says a caller must never receive "a silent stale hit presented as current". A cache hit during an outage still does exactly that, and this PR treats it as correct rather than a defect — the cached value is a real result from a real earlier query, and the provider cannot distinguish an outage from a quiet period. The error sentinel half is fixed and tested.

Two signed-off test gaps

• 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 test: its client is a static singleton with no injection seam, so no index_not_found can be forced through it. The Phase 0/1 tests do show Elasticsearch-served reads behaving identically.

Docs

OPENSEARCH_MIGRATION.md now records what must hold for the fallback to fire, and — more useful operationally — what still does not fall back: Phase 3, a failure mid-scroll, an OpenSearch answering successfully with stale data, a repeated identical query served from cache, and the legacy layer that turns any read failure into an empty 200 in every phase including pure-Elasticsearch installs.

TC-039 in the test plan is the QA case that should have caught this, and it was structurally guaranteed to pass. Step 3 told the tester to run "the same search again" — an identical request body is served from the query cache and returns the pre-outage result, indistinguishable from a working fallback. It now requires varying the query, adds the never-queried-content-type variant that used to fail as a 500, and warns about checking the returned total rather than just the absence of an error.

Not in scope

• The legacy swallow in ContentUtils and ContentHelper that turns any read failure into an empty 200 in every phase — pre-existing, affects pure-Elasticsearch installs, and is what made this outage silent rather than loud. Being filed separately.
• No fallback in Phase 3, by design.
• No IndexAPI<F> generic parameterization.
• No write-path changes, no new config property or feature flag — the fallback is documented, unconditional Phase 2 behaviour.

Still to do before merge

Manual validation on the Phase 2 migration environment, varying offset on every call, plus confirming Phase 3 still propagates there. Not yet done.

[claude]

Claude finished @fabrizzio-dotCMS's task in 2m 13s —— View job

---

Review: Phase 2 ES read fallback (#37413)

• Read the diff and changed source files
• Review PhaseRouter fallback logic
• Review ContentFactoryIndexOperationsOS raise-in-Phase-2 changes
• Review ESContentFactoryImpl routing wiring
• Check tests + docs
• Post review

The core change is sound: all five read call sites now go through PhaseRouter.read(...), indexOperationsDelegate() is fully removed (no stragglers), and mustRaiseForPhase2Fallback() uses isReadEnabled() && !isMigrationComplete() — identical to PhaseRouter.isPhase2(), so the raise and the catch are gated on the same condition. Cache-write-after-raise ordering is correct on both cached paths. The @VisibleForTesting two-arg constructor is a clean injection seam.

New Issues

• 🟡 Medium: dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentFactoryImpl.java:258 — In searchHits, the null-index guard throw new DotRuntimeException("Unable to determine which OpenSearch index to query") sits inside the same try block whose catch (final Exception e) is at line 263. DotRuntimeException is a RuntimeException (⊂ Exception), so this throw is immediately recaught one line below and rethrown as "Can't get OpenSearch indices information: Unable to determine which OpenSearch index to query", logged under the misleading Logger.error(this, "Can't get indices information.", e).

  • Functionally still correct — the fallback fires either way because mustRaiseForPhase2Fallback() is true in the catch too — so this is not blocking. But the distinct diagnostic message from the null-index branch never reaches the log, and a benign "no matching index" case is now emitted as an ERROR-level stack trace on the exact path the PR calls "the most consequential." Consider moving the null-index raise outside/after the try, or having the catch differentiate, so the two conditions log distinctly. Fix this →

• 🟡 Medium: dotCMS/src/main/java/com/dotcms/content/index/opensearch/ContentFactoryIndexOperationsOS.java:154,158 — The raised search/count messages interpolate exceptionMsg (OpenSearch's own reason), and in Phase 2 the router logs this message at ERROR. The PR states these carry "OpenSearch's own reason, not the query body" (Constitution Principle III).

  • Assumption: for a parse_exception/search_phase_execution_exception, OpenSearch's reason string frequently embeds a fragment of the failing query (e.g. Failed to parse query [<terms>]). If so, end-user search terms can still land in the ERROR line via exceptionMsg, partially defeating the stated intent.
  • What to verify: trigger a parse_exception against OpenSearch in Phase 2 and inspect the emitted OS read failed in Phase 2 [...] ERROR line for query-term leakage. If confirmed, sanitize or truncate exceptionMsg for the raised message (the full detail already remains at WARN/DEBUG). Same message shape is used on both the search and count paths.

Notes (non-blocking)

• The two signed-off test gaps (scroll swallow resolves its client via CDI, not the injected provider; ContentFactoryIndexOperationsES has no injection seam) and the AC-006/AC-007 reviewer decisions are all clearly documented in the PR body — flagging only that they remain open for a human reviewer's sign-off, not as defects.
• MainSuite1b correctly registers both new integration tests, so they will run in CI. The 14-unit-test class is intentionally in no suite (per PR body) — note it therefore won't gate CI, which matches your stated reasoning that only the integration test can catch the wiring defect.

No critical or high-severity issues. The two mediums above are worth addressing but do not block merge.
• branch 37413-impl-phase2-read-fallback

[ihoffmann-dot]

✅ Approving

Solid analysis, matches what the diff actually does.

• Root cause is correctly identified and fixed at both layers.
• AC-006 and AC-007 aren't satisfied literally as written, but both deviations are argued, not glossed over.
• Test coverage is strong: 14 unit tests against the router contract, 13 integration tests proving the actual wiring, both registered in MainSuite1b.
• All scope decisions, residual risks and out-of-scope items are documented in the PR and in OPENSEARCH_MIGRATION.md, nothing here is a silent assumption.