Skip to content

fix(migration): guard activateIndex against a missing OpenSearch counterpart (#36360) - #36880

Closed
fabrizzio-dotCMS wants to merge 5 commits into
mainfrom
issue-36360-activate-index-os-guard
Closed

fix(migration): guard activateIndex against a missing OpenSearch counterpart (#36360)#36880
fabrizzio-dotCMS wants to merge 5 commits into
mainfrom
issue-36360-activate-index-os-guard

Conversation

@fabrizzio-dotCMS

Copy link
Copy Markdown
Member

Problem

Activating an old/inactive index is dotCMS's rollback mechanism (revert to a previous reindex). But if that index predates the migration, it never went through the OpenSearch create fan-out, so it has no OpenSearch counterpart. ContentletIndexAPIImpl.activateIndex is phase-aware but only repoints both stores by name (toPhysicalNamecluster_X.<name>.os) — no indexExists, no create, no reconcile. Reactivating such an index during the migration is unguarded:

  • Phase 1: silent divergence (OS points at a .os index that doesn't exist; shadow writes swallowed).
  • Phase 2: the ES read-fallback masks it (logs ERROR per read).
  • Phase 3 (the cliff): ES is decommissioned, no fallback → OS points at a missing index → empty results / errors = "content lost" for the customer.

delete already has a phase-aware guard (assertIndexNotActive); activate had none.

Fix — a narrow guard

During the migration (phases 1/2/3), activateIndex now refuses to activate an index whose OpenSearch counterpart does not exist, throwing DotStateException (→ 400) with a clear message.

  • It is narrow: a rollback to a migration-era index that DOES have its .os copy still works. Only the genuinely-broken case (no OS copy) is blocked.
  • Phase 0 is not guarded (no OpenSearch store).
  • Escape hatch: ALLOW_ACTIVATE_INDEX_WITHOUT_OS_MIRROR=true forces the activation (accepting that OpenSearch is left pointing at a missing index until rebuilt) — mirroring the ALLOW_ACTIVE_INDEX_DELETE override on the delete guard.

Known trade-off (deliberate, "simple first")

A hard block conflicts with rollback: you can't roll back to a pre-migration index during the migration without first rebuilding it (or setting the override). The non-blocking alternative — allow the rollback, repoint ES instantly (stays transparent), and rebuild the OpenSearch copy asynchronously via the existing reindex machinery, with the readiness gate refusing Phase 3 until it's ready — is the intended follow-up. This PR is the cheap, safe first step that stops the silent Phase-3 detonation.

Tests

  • ContentletIndexAPIImplActivateGuardTest (6, surefire): blocks in phases 1/2/3 when the OS counterpart is missing; allows when it's present, in Phase 0, and under the override flag. Set-backed engine fakes control OS existence — no cluster needed.
  • Fixed the shared FakeContentletIndexOperations.indexAPI() (previously threw) to report existence, so the existing phase-2/3 activateIndex tests in ContentletIndexAPIImplPhaseTest (14) keep passing under the new precondition.

CI note: migration integration tests that activate an index in a dual/OS phase now require the OS counterpart to exist (real bootstrap normally provides it). Watch the OpenSearch upgrade / phase suites.

Notes

🤖 Generated with Claude Code

…terpart (#36360)

Activating an old/backup index is the rollback mechanism, but if that index predates the
migration it has no OpenSearch copy. activateIndex only repoints both stores by name (no
create/reconcile), so pointing at a missing .os index silently diverges ES from OS and
detonates at Phase 3 (no ES fallback → empty/error).

Add a narrow guard: during the migration (phases 1/2/3), refuse to activate an index whose
OpenSearch counterpart does not exist (DotStateException → 400). This does NOT block
rollback to a migration-era index that has its .os copy — only the genuinely broken case.
Escape hatch: ALLOW_ACTIVATE_INDEX_WITHOUT_OS_MIRROR=true forces it.

(The non-blocking alternative — allow the rollback and rebuild the OS copy async via the
reindex machinery, with the readiness gate refusing Phase 3 until it's ready — is the
deferred follow-up.)

Tests: ContentletIndexAPIImplActivateGuardTest (6) — blocks in phases 1/2/3 when the OS
counterpart is missing; allows when present, in Phase 0, and under the override. Fixed the
shared FakeContentletIndexOperations.indexAPI() to report existence so the existing
phase-2/3 activate tests in ContentletIndexAPIImplPhaseTest keep passing (14).

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

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

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


Code Review — activateIndex OpenSearch-counterpart guard

  • Read the diff (ContentletIndexAPIImpl, ESIndexResource, both tests)
  • Trace callers / blast radius of activateIndex
  • Check guard placement, phase semantics, error paths
  • Verify test coverage

New Issues

  • 🟡 Medium: ContentletIndexAPIImpl.java:3301Fail-closed existence check conflates "OS unreachable" with "OS counterpart missing." Try.of(() -> operationsOS.indexAPI().indexExists(osCounterpart)).getOrElse(false) treats any exception (connection timeout, cluster red, transient OpenSearch outage) as "the index does not exist" and blocks the activation. Since activateIndex is the rollback mechanism, a transient OS hiccup during Phases 1/2 — where ES is still primary and fully functional — would block a legitimate emergency ES rollback, exactly when an operator most needs it. The ALLOW_ACTIVATE_INDEX_WITHOUT_OS_MIRROR override mitigates it, but only if the operator knows to reach for it mid-incident.
    Assumption: indexExists throws (rather than returning false) on cluster connectivity failures.
    What to verify: the exception behavior of operationsOS.indexAPI().indexExists() on a red/unreachable cluster; consider distinguishing "confirmed absent" from "could not determine" (e.g. only block on a confirmed false, and log-and-allow on a thrown exception).

  • 🟡 Medium: ContentletIndexAPIImpl.java:3280Audit log claims the activation happened before the guard can reject it. The Logger.info(..., "Index activation (" + indexName + ") performed by user...") runs unconditionally, ahead of the guard block at line 3298. When the guard throws, the audit trail records "activation performed" for an activation that was actually blocked, which is misleading for anyone reading logs during an incident. Consider moving the audit line after the guard, or wording it as "requested" until it succeeds.

Notes (non-blocking)

  • The guard does not sit on the normal reindex switchover path — fullReindexSwitchover repoints slots directly and never calls activateIndex, so routine reindexes are unaffected. Good; the blast radius is limited to explicit activate calls (REST modIndex default case, IndexAjaxAction.activateIndex).
  • ESIndexResource.java:620 — switching the log from the raw action query param to the parsed indexAction enum, and adding the sendAdminMessage toast, is a clean improvement and consistent with the existing delete/clear guard handling.
  • Test coverage is solid: phases 1/2/3 blocked-when-missing, allowed-when-present, Phase 0 / no-flag / override all covered. The FakeContentletIndexOperations.indexAPI() fix (returning indexExists == true instead of throwing) correctly keeps the existing phase-2/3 activate tests green under the new precondition.

Both findings are 🟡 Medium — non-blocking. The core guard is correct and the "simple first" tradeoff is clearly documented in the PR.

issue-36360-activate-index-os-guard

…removed (#36360)

Confirms the guard only applies while migration is started (phases 1/2/3): with no phase
configured, MigrationPhase.current() defaults to Phase 0 and activation (rollback) proceeds.
So setting the phase to 0 or removing the flag re-enables unrestricted rollback. 7/7.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fabrizzio-dotCMS and others added 2 commits August 4, 2026 12:10
#36360)

When activate/clear/delete is rejected by a guard (DotStateException), modIndex returned a
400 but the portlet showed nothing. Push the reason as a system message (toast) via
sendAdminMessage so the operator sees it in the UI, not only in the server log. Also log the
resolved indexAction instead of the raw query param, which fixes the misleading
"Rejected 'null'" when the UI omits ?action=activate (relies on the default).

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

The guard's rejection reached the UI toast with internal jargon (OpenSearch, the .os
physical name, "Phase 3", the override config key), which also exposed the migration to a
regular admin. Split it: the full diagnostic (counterpart name + override) goes to the
operator log; the exception message that surfaces in the UI is now friendly and neutral —
"'<index>' can't be set as the active index right now because its search data isn't ready
yet. Run a full reindex and try again." Toast severity softened to WARNING.

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

Per feedback the UI message should be honest, not migration-neutral: state the real reason
(no OpenSearch counterpart) and the real fix (suspend the migration, or run a full reindex).
The physical counterpart name and override key stay in the operator log only.

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

Copy link
Copy Markdown
Member Author

Closing — a hard existence guard on activateIndex (block when the OpenSearch counterpart is missing) is fundamentally incompatible with the transparent-mirror / catch-up design.

activate/deactivate are intentionally pointer-store updates keyed by name, not validated against cluster state — a documented, tested contract (ContentletIndexAPIImplMigrationIntegrationTest: "the OS DB pointer reflects the name passed in, regardless of which index the OS cluster actually holds", "deactivateIndex never validates cluster existence … a pointer-store update driven by the name pattern, not by cluster state"). During catch-up the ES/OS names diverge and the OS physical may not exist yet, so activate must mirror the pointer optimistically.

At activation time, two cases are indistinguishable by a point-in-time existence check:

  • ✅ the OS copy will exist in a moment (normal dual-write catch-up), vs.
  • ❌ the OS copy will never exist (reactivating a pre-migration backup).

Both are "no OS copy right now", so the guard blocks the legitimate catch-up too — breaking normal migration operation. CI confirmed it: 6 ITs in the OpenSearch Upgrade Suite failed for exactly this reason.

The real risk (reactivating an un-mirrored backup → empty search at Phase 3) is a known, accepted characteristic of the optimistic-mirror model; the intended mitigation is the migration-readiness endpoint (#36849) — it detects a MISSING_COUNTERPART and gates the Phase-3 promotion, rather than a hard block on activate.

Finding documented in docs/backend/OPENSEARCH_MIGRATION.md (#36825).

fabrizzio-dotCMS added a commit that referenced this pull request Aug 4, 2026
…tchup) (#36360)

Record the finding from the reverted activate guard (PR #36880): activateIndex/
deactivateIndex are name-driven pointer-store updates that deliberately don't validate
cluster existence — a load-bearing part of the catchup model (ES/OS names diverge, OS may
not be built yet). A hard "block activate when the OS counterpart is missing" guard cannot
tell "not built yet" (legitimate catchup) from "never built" (old backup) at activation
time, so it breaks normal migration operation (6 OpenSearch Upgrade Suite ITs). The
intended mitigation for the un-mirrored-backup risk is the migration-readiness endpoint
(detect MISSING_COUNTERPART + gate Phase-3), not a per-op guard.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area : Backend PR changes Java/Maven backend code

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant