diff --git a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ContentletIndexAPIImpl.java b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ContentletIndexAPIImpl.java index 8a66fa677abb..d4309ed9597b 100644 --- a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ContentletIndexAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ContentletIndexAPIImpl.java @@ -2426,16 +2426,32 @@ public void putToIndex(final IndexBulkRequest bulkRequest) { } catch (final Exception e) { esException = new DotRuntimeException(e.getMessage(), e); } + RuntimeException osException = null; try { operationsOS.putToIndex(dual.osReq); } catch (final Exception e) { - Logger.warnAndDebug(this.getClass(), - "OS shadow write failed in putToIndex — " - + "OS index may diverge until next reindex. Cause: " + e.getMessage(), e); + if (isReadEnabled()) { + // Phase 2: OS serves reads (PhaseRouter.readProvider), so a failure here is + // not a shadow divergence — it leaves the index users actually query out of + // sync with the database (#37276). Surface it once ES has been given its + // chance, so the caller can retry rather than assume the write landed. + osException = (e instanceof RuntimeException) + ? (RuntimeException) e + : new DotRuntimeException(e.getMessage(), e); + } else { + Logger.warnAndDebug(this.getClass(), + "OS shadow write failed in putToIndex — " + + "OS index may diverge until next reindex. Cause: " + e.getMessage(), e); + } } + // ES stays authoritative when both legs fail: its exception is the one callers have + // always seen, and demoting it would change behaviour beyond this fix. if (esException != null) { throw esException; } + if (osException != null) { + throw osException; + } } else { // Single-provider phase (0 or 3): forward to the sole active provider. // Failures propagate normally — there is no secondary provider to fall back to. @@ -2495,10 +2511,18 @@ public IndexBulkProcessor createBulkProcessor(final IndexBulkListener bulkListen final boolean isDualWrite = providers.size() > 1; final List entries = new ArrayList<>(); for (final ContentletIndexOperations ops : providers) { - // OS is the shadow index in Phases 1 and 2 (dual-write): it replicates ES writes - // but is not yet the source of truth. In Phase 3 isDualWrite=false, so shadow=false - // and OS becomes the primary — failures propagate normally from that point. - final boolean shadow = isDualWrite && ops == operationsOS; + // OS is the shadow index in Phase 1 only: it replicates ES writes and nothing reads + // from it, so a failure there is genuinely tolerable (ADR-0009). + // + // Phase 2 is different and used to be handled as if it were Phase 1. Reads are served + // by OS from Phase 2 onwards (PhaseRouter.readProvider), so an OS write failure is + // immediately user-visible: a removal lost on the OS leg leaves an orphaned document + // in the very index being queried — the #37276 symptom, in the phase the migration + // spends the longest in. Treating OS as a shadow there also meant the journal entry + // was acked on the ES result alone and never retried. + // + // In Phase 3 isDualWrite=false, so shadow=false and OS is simply the primary. + final boolean shadow = isDualWrite && ops == operationsOS && !isReadEnabled(); // Each provider gets its own listener so counters and log output stay per-provider. // The shadow OS listener never touches the reindex queue or triggers a rebuild. final IndexBulkListener listenerForOps = shadow diff --git a/dotCMS/src/main/java/com/dotcms/content/index/opensearch/ContentletIndexOperationsOS.java b/dotCMS/src/main/java/com/dotcms/content/index/opensearch/ContentletIndexOperationsOS.java index 8a025399b753..59b65ed2cee5 100644 --- a/dotCMS/src/main/java/com/dotcms/content/index/opensearch/ContentletIndexOperationsOS.java +++ b/dotCMS/src/main/java/com/dotcms/content/index/opensearch/ContentletIndexOperationsOS.java @@ -337,7 +337,12 @@ void handleBulkResponse(final BulkResponse response) { } } - throw new DotRuntimeException(detail.toString()); + // errors() is expected to imply at least one item carrying an error, but if that ever + // stops holding we must not hand the caller a blank exception — a failure with no + // message is barely better than the silent return this replaced. + throw new DotRuntimeException(detail.length() > 0 + ? detail.toString() + : "OS bulk reported errors but no item carried an error cause"); } // ========================================================================= diff --git a/dotCMS/src/test/java/com/dotcms/content/elasticsearch/business/ContentletIndexAPIImplPhase2ReadDurabilityTest.java b/dotCMS/src/test/java/com/dotcms/content/elasticsearch/business/ContentletIndexAPIImplPhase2ReadDurabilityTest.java new file mode 100644 index 000000000000..95a379d9f2ef --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/content/elasticsearch/business/ContentletIndexAPIImplPhase2ReadDurabilityTest.java @@ -0,0 +1,141 @@ +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.assertThrows; + +import com.dotcms.content.elasticsearch.business.ContentletIndexAPIImpl.DualIndexBulkRequest; +import com.dotcms.content.elasticsearch.business.ContentletIndexAPIImplPhaseTest.FakeContentletIndexOperations; +import com.dotcms.content.elasticsearch.business.ContentletIndexAPIImplPhaseTest.FakeIndexAPI; +import com.dotcms.content.elasticsearch.business.ContentletIndexAPIImplPhaseTest.FakeIndiciesAPI; +import com.dotcms.content.elasticsearch.business.ContentletIndexAPIImplPhaseTest.FakeVersionedIndicesAPI; +import com.dotcms.content.index.domain.IndexBulkRequest; +import com.dotmarketing.exception.DotRuntimeException; +import com.dotmarketing.util.Config; +import java.util.List; +import org.junit.After; +import org.junit.Test; + +/** + * Unit tests for the durability of the OpenSearch write leg once OpenSearch serves reads. + * + *

Covers the Phase 2 gap found while reviewing + * #37320 for + * #37276.

+ * + *

The gap

+ *

ADR-0009 says a failed write to the shadow store is logged and must not impact operations. + * That is right for Phase 1, where nothing reads from OpenSearch. From Phase 2 + * onwards {@code PhaseRouter#readProvider} serves reads from OpenSearch while writes still + * fan out ES-primary / OS-shadow — so a removal lost on the OS leg left an orphaned document in + * the very index being queried. That is the original defect, in the phase the migration spends + * the longest in.

+ * + *

The fix is scoped by who serves reads, not by dual-write: OS stays fire-and-forget + * while it is invisible, and becomes durable the moment it is readable. ADR-0009's intent is + * preserved; only its Phase 2 assumption is corrected.

+ */ +public class ContentletIndexAPIImplPhase2ReadDurabilityTest { + + @After + public void clearPhase() { + Config.setProperty(FLAG_KEY, null); + } + + private static void setPhase(final int ordinal) { + Config.setProperty(FLAG_KEY, String.valueOf(ordinal)); + } + + /** A bulk request handle with no behaviour — only identity matters here. */ + private static final class StubBulkRequest implements IndexBulkRequest { + @Override + public int size() { + return 1; + } + } + + /** Provider whose {@code putToIndex} either fails or records the call. */ + private static final class RecordingOperations extends FakeContentletIndexOperations { + + private final boolean fail; + int putCalls = 0; + + RecordingOperations(final boolean fail) { + this.fail = fail; + } + + @Override + public void putToIndex(final IndexBulkRequest req) { + putCalls++; + if (fail) { + throw new DotRuntimeException("bulk write rejected"); + } + } + } + + private static ContentletIndexAPIImpl buildApi(final RecordingOperations es, + final RecordingOperations os) { + return new ContentletIndexAPIImpl(es, os, + new FakeIndexAPI(List.of()), new FakeIndiciesAPI(), new FakeVersionedIndicesAPI()); + } + + /** + * Given Scenario: Phase 1 — ES is primary and nothing reads from OpenSearch. The OS leg fails. + * When : putToIndex fans the batch out to both providers. + * Then : the caller is not told. ADR-0009's fire-and-forget shadow policy is preserved where + * it belongs, and this test is what stops the Phase 2 fix from over-reaching into it. + */ + @Test + public void test_phase1_shadowFailure_isStillSwallowed() { + setPhase(1); + final RecordingOperations es = new RecordingOperations(false); + final RecordingOperations os = new RecordingOperations(true); + + buildApi(es, os).putToIndex( + new DualIndexBulkRequest(new StubBulkRequest(), new StubBulkRequest())); + + assertEquals("Both legs must still be attempted", 1, es.putCalls); + assertEquals(1, os.putCalls); + } + + /** + * Given Scenario: Phase 2 — OpenSearch serves reads while writes are still dual. The OS leg + * fails. + * When : putToIndex fans the batch out to both providers. + * Then : the caller IS told, because the failure left the index that answers queries out of + * sync with the database. Swallowing it here is what reproduced #37276 in Phase 2. + */ + @Test + public void test_phase2_osFailure_reachesCaller_becauseOsServesReads() { + setPhase(2); + final RecordingOperations es = new RecordingOperations(false); + final RecordingOperations os = new RecordingOperations(true); + final ContentletIndexAPIImpl api = buildApi(es, os); + + assertThrows(RuntimeException.class, () -> api.putToIndex( + new DualIndexBulkRequest(new StubBulkRequest(), new StubBulkRequest()))); + + assertEquals("ES must still have been written before the OS failure surfaces", + 1, es.putCalls); + } + + /** + * Given Scenario: Phase 2, and BOTH legs fail. + * When : putToIndex runs. + * Then : the ES exception is the one raised. ES stays authoritative for the caller's error; + * demoting it would change behaviour beyond the gap being closed here. + */ + @Test + public void test_phase2_bothFail_esExceptionWins() { + setPhase(2); + final RecordingOperations es = new RecordingOperations(true); + final RecordingOperations os = new RecordingOperations(true); + final ContentletIndexAPIImpl api = buildApi(es, os); + + assertThrows(RuntimeException.class, () -> api.putToIndex( + new DualIndexBulkRequest(new StubBulkRequest(), new StubBulkRequest()))); + + assertEquals("The OS leg is always attempted, even when ES already failed", + 1, os.putCalls); + } +} diff --git a/specs/37276-silent-index-delete-loss/contracts/putToIndex-failure-contract.md b/specs/37276-silent-index-delete-loss/contracts/putToIndex-failure-contract.md index 03fcfb77225b..ac98f865bdcf 100644 --- a/specs/37276-silent-index-delete-loss/contracts/putToIndex-failure-contract.md +++ b/specs/37276-silent-index-delete-loss/contracts/putToIndex-failure-contract.md @@ -30,16 +30,27 @@ Unchanged: ## Behavior per migration phase -The router already isolates the shadow provider, so the new failure surfaces exactly where -ADR-0009 requires and nowhere else. **The escalation is implemented in the providers, not in -the router** — putting it in the router would break the phase 1–2 guarantee below. +**The escalation is implemented in the providers, not in the router.** The router decides +whether a provider's failure is tolerable; the providers only decide whether a bulk response +counts as a failure at all. | Phase | Providers | Partial failure in ES | Partial failure in OS | |-------|-----------|-----------------------|-----------------------| | 0 | ES only | propagates | n/a | -| 1, 2 | ES primary, OS shadow | propagates | logged, swallowed by the router (`ContentletIndexAPIImpl:2429-2435`) — **ADR-0009** | +| 1 | ES primary, OS shadow | propagates | logged, swallowed — **ADR-0009** | +| 2 | ES primary, OS shadow **but OS serves reads** | propagates | **propagates** | | 3 | OS only | n/a | propagates | +Phase 2 is the row that is easy to get wrong. `PhaseRouter#readProvider` serves reads from +OpenSearch from Phase 2 onwards, so an OS write failure there is not a shadow divergence — it +leaves the index that answers queries out of sync with the database, which is the defect +#37276 is about. The shadow treatment is therefore scoped by **who serves reads** +(`isReadEnabled()`), not by whether the phase is dual-write. ADR-0009's intent is preserved: +a store nobody reads from still cannot break a user operation. + +When both legs fail, the ES exception is the one raised — that is what callers have always +seen. + ## Impact on callers In-tree callers of the router method, all in `ContentletIndexAPIImpl`: diff --git a/specs/37276-silent-index-delete-loss/release-note.md b/specs/37276-silent-index-delete-loss/release-note.md index 6e8597d0941f..b02d2c01abcc 100644 --- a/specs/37276-silent-index-delete-loss/release-note.md +++ b/specs/37276-silent-index-delete-loss/release-note.md @@ -24,6 +24,16 @@ of the write rather than as unexplained index drift weeks later. Treat a new `pu as a pre-existing condition now made visible — most often index write-queue saturation — and investigate the index cluster, not this release. +**During the ES→OpenSearch migration, Phase 2 writes can now fail where they previously did +not.** From Phase 2 onwards OpenSearch serves reads while writes still go to both clusters. A +failed OpenSearch write used to be logged and ignored as a shadow divergence — which left +orphaned documents in the very index being queried. It now reaches the caller and, on the +reindex-journal path, marks the entry for retry. + +This affects every write in Phase 2, not only removals. An environment whose OpenSearch cluster +is unhealthy will begin surfacing errors that were previously absorbed. Phase 1 is unchanged: +while nothing reads from OpenSearch, a failed shadow write is still logged and ignored. + **Log wording changed.** Bulk failures previously read `Error reindexing` regardless of the operation, including removals. They now read `Error applying (N) index operation(s)`. Any log alerting or saved search matching the old string needs updating.