From 46f8468f6d93a06eb6d879da24b837ae8fc5c5d9 Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Tue, 1 Sep 2026 14:43:01 -0600 Subject: [PATCH 1/2] fix(search): make the OpenSearch write leg durable once it serves reads (#37276) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #37320 surfaced a gap that reproduces the original defect in Phase 2. ADR-0009 says a failed write to the shadow store is logged and must not impact operations. That is correct for Phase 1, where nothing reads from OpenSearch. From Phase 2 onwards 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, which is #37276 itself, in the phase the migration spends the longest in. Both paths were affected: - The async path built a shadow BulkProcessorListener for OS in Phases 1 AND 2. Its failures never marked the journal entry failed, so the entry was acked on the ES result alone and the removal was never retried. - The sync path swallowed the OS exception in putToIndex. Scope the shadow treatment by who serves reads rather than by dual-write: OS stays fire-and-forget while nothing reads it, and becomes durable the moment it does. ADR-0009's intent is preserved; only its Phase 2 assumption is corrected. When both legs fail the ES exception still wins — that is what callers have always seen. Also guards against a blank exception message when a bulk reports errors but no item carries a cause (flagged in review). ContentletIndexAPIImplPhase2ReadDurabilityTest covers all three cases, including that Phase 1 still swallows — the guard that keeps this fix from over-reaching into the policy where it is correct. 281 unit tests green. Co-Authored-By: Claude Opus 5 (1M context) --- .../business/ContentletIndexAPIImpl.java | 38 ++++- .../ContentletIndexOperationsOS.java | 7 +- ...tIndexAPIImplPhase2ReadDurabilityTest.java | 141 ++++++++++++++++++ 3 files changed, 178 insertions(+), 8 deletions(-) create mode 100644 dotCMS/src/test/java/com/dotcms/content/elasticsearch/business/ContentletIndexAPIImplPhase2ReadDurabilityTest.java 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 8a66fa677ab..d4309ed9597 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 8a025399b75..59b65ed2cee 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 00000000000..95a379d9f2e --- /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); + } +} From 37162556de465c5ad7f42268d66c5bde05e60855 Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Tue, 1 Sep 2026 14:56:56 -0600 Subject: [PATCH 2/2] docs(search): correct the phase table and release note for the Phase 2 fix (#37276) Both documents were written before the Phase 2 gap was found and now contradict the code. The contract's per-phase table said a partial OpenSearch failure is "logged, swallowed" in phases 1 AND 2. Since OpenSearch serves reads from Phase 2 onwards, that row is now split: Phase 1 swallows, Phase 2 propagates. The rationale is stated inline, because the table is exactly where someone would look before changing this behaviour back. The release note said nothing about it at all, which is the omission that matters most operationally: Phase 2 writes can now fail where they previously did not, for every write and not only removals. Co-Authored-By: Claude Opus 5 (1M context) --- .../contracts/putToIndex-failure-contract.md | 19 +++++++++++++++---- .../release-note.md | 10 ++++++++++ 2 files changed, 25 insertions(+), 4 deletions(-) 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 03fcfb77225..ac98f865bdc 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 6e8597d0941..b02d2c01abc 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.