fix(search): make the OpenSearch write leg durable once it serves reads (#37276)

[fabrizzio-dotCMS]

Follow-up to #37320 (merged). Addresses the Phase 2 gap raised in review of that PR, plus one defensive fix. Part of #37276.

The gap

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 it stops being true. PhaseRouter#readProvider serves reads from OpenSearch (readProvider():144 — isReadEnabled() ? osImpl : esImpl) 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, and the async one is the reason the durable-delete fix did not cover it:

• Async (journal). createBulkProcessor built a shadow BulkProcessorListener for OS whenever isDualWrite, i.e. Phases 1 and 2. A shadow listener never marks the reindex-queue entry failed, so the journal entry was acked on the ES result alone and the removal was never retried.
• Sync. putToIndex caught the OS exception and logged it as shadow divergence.

The change

Scope the shadow treatment by who serves reads, not by dual-write:

final boolean shadow = isDualWrite && ops == operationsOS && !isReadEnabled();

OpenSearch stays fire-and-forget while nothing reads it, and becomes durable the moment it does. ADR-0009's intent — don't let a store nobody reads break user operations — is preserved; only its Phase 2 assumption that a shadow is not read from is corrected.

When both legs fail, the ES exception is still the one raised. That is what callers have always seen, and demoting it would change behaviour beyond this gap.

Also guards ContentletIndexOperationsOS.handleBulkResponse against throwing a blank-message exception when a bulk reports errors() but no item carries a cause — flagged in the same review. A failure with no message is barely better than the silent return it replaced.

Testing

ContentletIndexAPIImplPhase2ReadDurabilityTest, 3 cases. The first is the important one:

• Phase 1 still swallows the OS failure. This is the guard that stops the fix from over-reaching into the policy where ADR-0009 is correct. Without it, a later refactor could quietly make every shadow hiccup fail a user-facing save.
• Phase 2 surfaces the OS failure, and ES is still written first.
• Both legs failing raises the ES exception.

281 unit tests green. All 9 integration tests from #37320 re-run green against this change (ReindexDeleteJournalTest, ContentletDestroyIndexRemovalTest, ContentletIndexPartialFailurePhaseTest, ContentletIndexProviderSkipTest, one JVM).

Spec

The spec on the base branch was amended in 945d27e to put this in scope with AC-010, alongside the three scope gaps raised separately in review. It had been a decision taken in code without being written down — the same complaint the reviewer had made about the other sites.

Behaviour change worth flagging

This changes failure behaviour for every write in Phase 2, not only removals. An environment whose OpenSearch cluster is unhealthy will begin surfacing errors that were previously absorbed. Same "the defect becoming visible" effect as the partial-failure escalation in #37320, and confined to phases where OpenSearch already serves reads.

🤖 Generated with Claude Code

This PR fixes: #37276

[ihoffmann-dot]

Reviewed the diff in full (ContentletIndexAPIImpl, ContentletIndexOperationsOS, plus the new ContentletIndexAPIImplPhase2ReadDurabilityTest).

The fix is sound. shadow = isDualWrite && ops == operationsOS && !isReadEnabled() correctly scopes shadow treatment by who serves reads, not by dual-write, applied consistently on both the sync and async paths. Exception precedence (ES first, OS second, ES wins if both fail) preserves existing behavior. The blank-message guard is a good small defensive addition.

Test coverage is well-targeted too. Approving ✅

[claude]

Claude finished @fabrizzio-dotCMS's task in 4m 32s —— View job

---

Code Review — PR #37333

• Read the diff (ContentletIndexAPIImpl, ContentletIndexOperationsOS, new test)
• Trace isReadEnabled/isDualWrite/phase semantics
• Trace the async reindex path (createBulkProcessor → shared BulkProcessorListener → reindex journal)
• Post findings

I reviewed both the sync and async legs against the migration-phase model. The sync putToIndex change is correct and well-scoped. The async change, however, has a gap that I think undermines the PR's own stated goal for that path.

New Issues

• 🟠 High: ContentletIndexAPIImpl.java:2525 (async path) — In Phase 2, an ES-success + OS-failure entry is still lost, because ES's success deletes the journal row before OS's failure can requeue it.

  The fix makes OS share the real bulkListener in Phase 2 (shadow=false), so OS results now reach the reindex queue. But both ES and OS processors now share the same listener instance and the same workingRecords map (ReindexThread.java:234-237 creates one batchListener and hands it to createBulkProcessor, which now passes it to both inner processors). The listener's success handler deletes the journal entry (BulkProcessorListener.handleSuccess → deleteReindexEntry, by ident_to_index/id), and its failure handler requeues it (handleFailure → markAsFailed, an UPDATE ... WHERE id=?).

  CompositeBulkProcessor.close() (line 429) iterates writeProviders() in order [ES, OS] (PhaseRouter.writeProviders():168), and ESIndexBulkProcessor.close() calls awaitClose(...) (ContentletIndexOperationsES.java:122) — so ES flushes and its afterBulk callback completes before OS's processor is closed at all. Concretely, for a removal that ES applies but OS rejects:

  1. ES.close() → afterBulk(success) → handleSuccess → deleteReindexEntry(entry) — journal row gone.
  2. OS.close() → afterBulk(failure) → handleFailure → markAsFailed(entry) → UPDATE ... WHERE id=<deleted> → 0 rows, no-op.

  The removal is never retried, and the orphaned document stays in the OS index that Phase 2 actually serves reads from — i.e. exactly the Content deletes are lost silently: index removal is fire-and-forget with no durable record, unlike adds #37276 symptom this PR says it closes on the async path ("the async one is the reason the durable-delete fix did not cover it"). This is deterministic, not a race: the ES leg completes before the OS leg starts. It bites every entry where ES succeeds and OS fails (partial per-item failures, and full OS outages via afterBulk(Throwable) which requeues all of them after ES already deleted them).

  The sync putToIndex path does not have this problem — it surfaces the OS exception directly to the caller. Only the async/reindex path (the one the PR emphasizes) is affected. A fix would need the shared listener to not delete an entry that another provider reported failed in the same batch (e.g. reconcile per-entry across providers, or delete only after all write providers ack success). Fix this →

  What to verify: whether the fix(search): journal content deletes so lost index removals are retried (#37276) #37320 integration tests (ReindexDeleteJournalTest, etc.) actually exercise the Phase 2 ES-success/OS-fail interleaving on the async processor, or only the sync path / a single-provider phase. If they don't, this case is untested and the loss is live.

• 🟡 Medium: ContentletIndexAPIImplPhase2ReadDurabilityTest.java:129 — test_phase2_bothFail_esExceptionWins does not actually prove ES's exception wins. RecordingOperations throws the identical message ("bulk write rejected") for both legs, so the test cannot distinguish an ES-sourced exception from an OS-sourced one; it only asserts some RuntimeException is thrown and OS was attempted. Give the two legs distinct exception messages and assert on the thrown message to make the "ES wins" claim testable.

• 🟡 Medium: ContentletIndexAPIImplPhase2ReadDurabilityTest.java (whole file) — the new coverage is only for the sync putToIndex path. The createBulkProcessor/async change (line 2525) — where the High finding lives — has no unit coverage. The shadow scoping flip is the more consequential half of this PR and is exactly the path that reintroduces the loss above.

The sync-path change (ContentletIndexAPIImpl.java:2429-2454), the exception-precedence choice (ES wins when both fail), and the blank-message guard in ContentletIndexOperationsOS.java:343 are all correct and well-reasoned.

· branch 37276-silent-index-delete-loss-impl

[fabrizzio-dotCMS]

Folded into #37297 — the Phase 2 fix belongs in the PR that carries AC-010, and leaving it stacked meant #37297 read as internally inconsistent: its own spec.md:255 declaring AC-010 while its contract denied it.

Same two commits, fast-forwarded, nothing lost. Closing.