Spec: Phase 2 ES read fallback never fires for content search (#37413)

[fabrizzio-dotCMS]

What this is

PR 1 of 2 in the Spec-Kit flow — carries spec.md alone, no implementation. Needs a dev
approval (not a merge) before /speckit-plan runs. Implementation lands in PR 2, branched
off this one.

Spec: specs/37413-phase2-read-fallback/spec.md

The defect in one line

In Phase 2, if OpenSearch goes down, content search does not fall back to Elasticsearch as the
migration design promises — because it never passes through the PhaseRouter that implements
that fallback — and returns 200 with zero results or a 500, while Elasticsearch holds a
complete, in-sync copy the whole time.

Severity High: this is the safety net Cloud and Support are currently being told Phase 2
has. For content search that wiring was never written, so it also blocks recommending Phase 2.

Not a regression. With its read engine down dotCMS has always behaved this way — Phase 0
with ES down does the same thing through the same legacy code. What is broken is the promise.

Root cause — verified against the tree, and a correction to the issue

Layer 1 (the actual defect). ESContentFactoryImpl.java:273 picks the engine with a bare
ternary and calls the chosen provider directly. Its five read call sites — :1352 search,
:1607 indexCount, :1616 searchHits, :1634 indexSearchScroll, :1669
createScrollQuery — have no router in between; grep PhaseRouter in that file returns
nothing. PhaseRouter.read (:187-199) and readChecked (:298-310) implement the fallback
correctly and are simply unreachable from this path. The stack proves it by omission: no
PhaseRouter frame, where the same outage through IndexAPIImpl does carry one.

Layer 2 — corrects the issue body. #37413 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 real
mechanism, all three confirmed in the current tree:

1. ContentHelper.java:310 counts first, and a CountRequest carries no offset/limit — so
   varying the offset does not change the count cache key. Types queried before the outage had
   a cached count; types not previously queried threw → 500.
2. ContentUtils.java:302 catch (Throwable) logs a one-line-truncated WARN and returns an
   empty list, swallowing the search failure.
3. ContentHelper.java:318 if (contentlets.isEmpty() && offset <= resultsSize) { resultsSize = 0; }
   overwrites the real cached count with 0 → 200 / total=0.

I will correct the issue body to match before implementation so the two do not disagree.

Two decisions deliberately deferred to /speckit-plan

Not clarification gaps — either answer satisfies the design, as long as it is deliberate and
documented:

1. Do the two scroll sites fall back at all? indexSearchScroll / createScrollQuery hold
   engine-specific cursor state; a half-consumed OpenSearch scroll cannot resume on
   Elasticsearch. Likely propagate-only, documented in code. (Regression Risk, AC-002)
2. Is ERROR_HIT touched in the implementation PR, or does the routing fix ship alone?
   Making the OpenSearch read throw where it now returns an empty result changes the contract
   for every caller that depends on receiving empty instead of an exception. Those callers must
   be enumerated first; if the list is large, Layer 1 stands on its own. (Fix Scope, AC-006)

Explicit non-goals

• No change to Elasticsearch behavior — ContentFactoryIndexOperationsES keeps its ERROR_HIT
  semantics; phases 0 and 1 untouched.
• No fallback in Phase 3 — failures must keep propagating by design.
• No repo-wide rewrite of the legacy swallow. ContentUtils:302 and ContentHelper:318 are
  pre-existing and affect all phases including pure Elasticsearch; fixing Layer 1 removes the
  Phase 2 case. Separate issue, not folded in.
• No IndexAPI<F> generic parameterization — its own PR.
• No new config property or feature flag; the fallback is unconditional documented Phase 2
  behavior.

Reproduction note worth reading

The bug hides from QA. Re-running the same query body returns 200 with correct totals —
the query cache serving pre-outage results, which looks exactly like a working fallback. Only
novel, uncached queries expose it, which is why the repro varies offset on every call.

Design being violated

docs/backend/OPENSEARCH_MIGRATION.md lines 105, 402, 676 and 901 all state the Phase 2 read
fallback as designed behavior.

Review focus

The spec, not code — there is none yet. Specifically: is the fix scope bounded correctly, and
are the two deferred decisions the right things to defer?

🤖 Generated with Claude Code

This PR fixes: #37413

[claude]

Claude finished @fabrizzio-dotCMS's task in 1m 40s —— View job

---

Spec review — Phase 2 ES read fallback (#37413)

I verified every code reference the spec cites against the current tree. All of them are exact, which is unusual and worth calling out:

Claim	Verified
ESContentFactoryImpl:273 bare ternary delegate	✅ exact
PhaseRouter appears 0× in ESContentFactoryImpl	✅ grep returns nothing
Five read call sites :1352 :1607 :1616 :1634 :1669	✅ all match
PhaseRouter.read :187-199, readChecked :298-310, both log ERROR then fall back to esImpl	✅ exact
ContentHelper:310 count-first / :318 resultsSize = 0 overwrite	✅ exact
ContentUtils:302 catch (Throwable) → Logger.warn (one-line-truncated)	✅ exact

Layer 1 is proven. The routing gap is real and the fallback machinery genuinely exists and is genuinely unreachable from this path.

Answering the two questions you asked reviewers to focus on:

Are the two deferred decisions the right things to defer? — Yes, both.

• Scroll fallback (indexSearchScroll/createScrollQuery): engine-specific cursor state means this is a genuine design trade-off, not an unstated requirement. Correct to defer to /speckit-plan. AC-002's "document the exclusion in code with its reason" is the right guardrail.
• ERROR_HIT scope: this hinges on enumerating callers that depend on empty-instead-of-exception — investigative work that belongs in planning. The escape hatch ("Layer 1 stands on its own if the list is large") keeps the spec shippable either way. Correct to defer.

Is the fix scope bounded correctly? — Mostly, with three tensions the plan should resolve:

• 🟡 AC-007 vs. the caching design. AC-007 requires that a repeated identical query "does not mask a live OpenSearch failure by serving a stale pre-outage result... never a silent stale hit presented as current." But a query-cache hit means no read reaches OpenSearch, so there is no exception for the router to catch — Layer 1 cannot satisfy this. And per your own Assumptions ("stale-but-present ES data is strictly better than returning nothing"), serving cached pre-outage data during an outage is arguably the desired outcome, not a defect. As written, AC-007 either demands work outside the stated in-scope list (the query cache isn't in it) or contradicts the reproduction's own framing (step 4 treats the cache hit as correct data that merely "looks like" fallback). Recommend the plan either narrow AC-007 to "uncached queries" or explicitly justify touching the cache.

• 🟡 AC-003 vs. the out-of-scope legacy swallow. AC-003 requires "no WARN-only swallow remains on the Phase 2 fallback path," but ContentUtils:302's catch (Throwable) → WARN is explicitly out of scope. On the successful fallback path (OS throws, ES succeeds) the router logs ERROR and returns results — fine. But when both engines fail, control still returns through ContentUtils:302, which swallows to WARN + empty list. AC-003's guarantee therefore only holds when ES succeeds. The plan should state that the both-engines-down case still hits the (out-of-scope) legacy swallow, so AC-003 isn't read as a promise the fix doesn't keep.

• 🟡 In-scope bullet vs. deferred decision Test Branch and Commit #2. The In-Scope list commits firmly to "Make the OpenSearch read path... surface a throwable failure to the router rather than an empty result." But deferred decision Test Branch and Commit #2 / AC-006 treats that same ERROR_HIT change as optional (may ship Layer 1 alone). A reader can't tell whether making OS throw is committed or contingent. Recommend moving the "surface a throwable" bullet under a "contingent on AC-006 enumeration" heading so In-Scope and AC-006 don't disagree.

Minor

• 🟡 Line-number brittleness. The spec pins exact line numbers (:1352, :1607, …) that are correct today but will drift as ESContentFactoryImpl changes before PR 2 lands. Consider anchoring on method names (indexOperationsDelegate()'s callers) as the durable reference, with line numbers as "as of 788795e915."

None of these block spec approval — they're refinements for /speckit-plan to absorb, and two of them (AC-003, AC-006 tension) are exactly the kind of thing convergence should catch later. The root-cause analysis is sound, the Layer 2 correction to the issue body is correct (ConnectException is not an OpenSearchException, so the ERROR_HIT branch genuinely can't be the mechanism for the observed connection outage), and the non-goals are drawn in the right places.

· 37413-phase2-read-fallback

[ihoffmann-dot]

Review:

Spec's solid, root cause checks out against the tree. One thing I'd want resolved before approving: AC-005 requires the missing-index case to fall back too, but that only works if we touch the ERROR_HIT branch, which the PR itself treats as optional, droppable if the caller list gets too big. I think we should pick one: either AC-005 becomes conditional, or that change stops being optional.