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 352bb5b91c2d..8a66fa677abb 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 @@ -3131,7 +3131,13 @@ private void removeContentAndProcessDependencies(final Contentlet contentlet, final DualIndexBulkRequest dualReq = bulkRequest instanceof DualIndexBulkRequest ? (DualIndexBulkRequest) bulkRequest : null; - for (final ContentletIndexOperations ops : router.writeProviders()) { + // writeProviders() is ordered primary-first in every phase (0 → [ES], 1/2 → [ES, OS], + // 3 → [OS]), so element 0 is the provider whose outcome the caller is entitled to. + final List deleteProviders = router.writeProviders(); + final ContentletIndexOperations primary = deleteProviders.get(0); + int primaryDeleteOps = 0; + + for (final ContentletIndexOperations ops : deleteProviders) { final ProviderIndices indices = loadProviderIndicesQuietly(ops); if (indices == null) { continue; @@ -3142,20 +3148,46 @@ private void removeContentAndProcessDependencies(final Contentlet contentlet, } else { providerReq = bulkRequest; } + int opsAdded = 0; if (indices.live != null) { ops.addDeleteOp(providerReq, indices.live, id); + opsAdded++; } if (indices.reindexLive != null) { ops.addDeleteOp(providerReq, indices.reindexLive, id); + opsAdded++; } if (!onlyLive) { if (indices.working != null) { ops.addDeleteOp(providerReq, indices.working, id); + opsAdded++; } if (indices.reindexWorking != null) { ops.addDeleteOp(providerReq, indices.reindexWorking, id); + opsAdded++; } } + if (ops == primary) { + primaryDeleteOps = opsAdded; + } + } + + // The primary contributing no delete operations means the removal did not happen, and + // putToIndex early-returns on an empty batch — so without this check it is + // indistinguishable from a completed removal (#37276, loss point L2). + // + // Counting the operations rather than testing for a null ProviderIndices covers both + // ways the primary can come up empty: its pointers failed to load (loadProviderIndices + // threw), or they loaded but hold no active index at all. The second reads as success + // just as silently as the first, and only the operation count sees both. + // + // A shadow provider keeps warn-and-continue, matching how putToIndex already isolates + // the OS leg under ADR-0009. + if (primaryDeleteOps == 0) { + throw new DotRuntimeException( + "Cannot remove content from the index: the primary provider (" + + primary.getClass().getSimpleName() + ") resolved no active index " + + "for document " + id + ". The removal was NOT performed."); } if (!onlyLive && UtilMethods.isSet(relationships)) { diff --git a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ContentletIndexOperationsES.java b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ContentletIndexOperationsES.java index fd9d7e5fa5ef..ae3f3afa9bd8 100644 --- a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ContentletIndexOperationsES.java +++ b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ContentletIndexOperationsES.java @@ -5,6 +5,7 @@ import static com.dotmarketing.common.reindex.ReindexThread.ELASTICSEARCH_CONCURRENT_REQUESTS; import com.dotcms.content.index.ContentletIndexOperations; +import com.google.common.annotations.VisibleForTesting; import com.dotcms.content.index.IndexAPI; import com.dotcms.content.index.opensearch.ContentletIndexOperationsOS; import com.dotcms.content.index.domain.CreateIndexStatus; @@ -203,11 +204,7 @@ public void putToIndex(final IndexBulkRequest req) { try { final BulkResponse response = RestHighLevelClientProvider.getInstance() .getClient().bulk(bulkRequest, RequestOptions.DEFAULT); - if (response != null && response.hasFailures()) { - Logger.error(this, - "Error reindexing (" + response.getItems().length + ") content(s): " - + response.buildFailureMessage()); - } + handleBulkResponse(response); } catch (final Exception e) { if (ExceptionUtil.causedBy(e, IllegalStateException.class)) { ContentletFactory.rebuildRestHighLevelClientIfNeeded(e); @@ -217,6 +214,33 @@ public void putToIndex(final IndexBulkRequest req) { } } + /** + * Decides what a bulk response means to the caller. + * + *

Extracted from {@link #putToIndex(IndexBulkRequest)} so the policy can be exercised + * without a cluster — the HTTP call is not what is interesting here, the verdict is.

+ * + * @param response the response from the bulk call; {@code null} is tolerated + */ + @VisibleForTesting + void handleBulkResponse(final BulkResponse response) { + if (response != null && response.hasFailures()) { + // A bulk can return normally while rejecting individual items — a saturated write + // queue, an unavailable shard, a version conflict. Logging and returning made those + // indistinguishable from success, so a caller could commit a content deletion whose + // index removal never landed (#37276, loss point L3). Raising lets the journal entry + // be marked failed and retried instead. + // + // The message says "index operations" rather than "reindexing": the batch may well + // have carried removals, and the old wording is why searching production logs for + // failed deletes came back empty. + final String message = "Error applying (" + response.getItems().length + + ") index operation(s): " + response.buildFailureMessage(); + Logger.error(this, message); + throw new DotRuntimeException(message); + } + } + // ========================================================================= // Async bulk-processor write path // ========================================================================= diff --git a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java index f98b62f4f97a..0908350f5ca3 100644 --- a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java @@ -3053,6 +3053,45 @@ private void deleteMultitrees(final Contentlet contentlet, final User user) } } + /** + * Records a durable index-removal intent in {@code dist_reindex_journal} for every identifier + * being destroyed, so that a failed or lost index write is retried instead of silently + * orphaning the document. + * + *

Must be called inside the transaction that deletes the contentlet rows: a rolled-back + * destroy must not leave the index owing a removal for content that still exists.

+ * + *

A journal entry is identifier-wide — the consumer fans out across every language + * and variant of the identifier. That is correct here, because destruction removes every + * version and language by design. It is not transportable to the unpublish/archive + * path, where a removal is per language and an identifier-wide entry would drop languages + * that are still live.

+ * + *

Takes identifiers rather than contentlets on purpose: they are collected before + * the rows are deleted, so this method cannot depend on the state of {@link Contentlet} + * objects that {@code contentFactory.delete} has already processed. The signature is what + * enforces that ordering — a comment would not.

+ * + * @param identifiers identifiers of the contentlets being destroyed, collected before deletion + */ + private void journalContentDeletes(final Set identifiers) { + + if (identifiers == null || identifiers.isEmpty()) { + return; + } + + try { + APILocator.getReindexQueueAPI().addIdentifierDelete(identifiers); + } catch (final DotDataException e) { + // The journal row is the durability guarantee; without it the removal is only as good + // as the in-memory listener. Fail the destroy rather than commit a deletion whose + // index removal nothing is tracking. + throw new DotRuntimeException( + "Unable to journal the index removal for destroyed content: " + e.getMessage(), + e); + } + } + /** * Completely destroys the given list of {@link Contentlet} objects (versions, relationships, * associated contents, binary files) in all of their languages. @@ -3117,8 +3156,25 @@ private boolean destroyContentlets(final List contentlets, final Use this.backupDestroyedContentlets(contentlets, user); + // Collected before the delete so the journal never depends on post-deletion object + // state — see journalContentDeletes. + final Set destroyedIdentifiers = contentletsVersion.stream() + .map(Contentlet::getIdentifier) + .filter(UtilMethods::isSet) + .collect(Collectors.toSet()); + // Delete all the versions of the contentlets to delete this.contentFactory.delete(contentletsVersion); + + // Record the index removal durably, in the SAME transaction that just deleted the rows. + // The commit listener below stays as the low-latency path, but it lives only in memory: + // if it is lost — the JVM stops between commit and execution, the shared pool rejects the + // task, or the bulk comes back with per-item failures — nothing would remember that the + // index still owes a removal, and the document is orphaned permanently. The journal row + // is what ReindexThread retries. This mirrors what the add path already does (see + // ContentletIndexAPIImpl#addContentToIndex, IndexPolicy.DEFER branch). + this.journalContentDeletes(destroyedIdentifiers); + // Remove the contentlets from the search index and cache final Set removedFromIndex = new HashSet<>(); for (final Contentlet contentlet : contentletsVersion) { @@ -3546,8 +3602,21 @@ public void deleteAllVersionsandBackup(List contentlets, User user, contentletInodes.add(element.getInode()); } + // Collected before the delete — see journalContentDeletes. + final Set destroyedIdentifiers = contentletsVersion.stream() + .map(Contentlet::getIdentifier) + .filter(UtilMethods::isSet) + .collect(Collectors.toSet()); + contentFactory.delete(contentletsVersion); + // Same durability requirement as destroyContentlets: the index removal below is deferred + // and in-memory, so record the intent in the journal inside this transaction. Safe here + // because contentletsVersion was built from findAllVersions(identifier) above — every + // version and language of the identifier is going away, which is what an identifier-wide + // journal entry means. + this.journalContentDeletes(destroyedIdentifiers); + for (Contentlet contentlet : perCons) { indexAPI.removeContentFromIndex(contentlet); CacheLocator.getIdentifierCache().removeFromCacheByVersionable(contentlet); @@ -3599,6 +3668,11 @@ public void delete(List contentlets, User user, boolean respectFront contentFactory.delete(contentletsVersion); + // Deliberately NOT journalled (#37276). Unlike destroyContentlets and + // deleteAllVersionsandBackup, contentletsVersion here is just the caller's list — this + // method can delete a subset of an identifier's versions or languages. A journal entry is + // identifier-wide, so recording one would remove index documents for languages that still + // exist. Same reason the unpublish/archive path is excluded. for (Contentlet contentlet : perCons) { indexAPI.removeContentFromIndex(contentlet); CacheLocator.getIdentifierCache().removeFromCacheByVersionable(contentlet); 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 739893e50bbd..8a025399b753 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 @@ -21,6 +21,7 @@ import com.dotmarketing.business.APILocator; import com.dotmarketing.common.reindex.ReindexThread; import com.dotmarketing.exception.DotDataException; +import com.google.common.annotations.VisibleForTesting; import com.dotmarketing.exception.DotRuntimeException; import com.dotmarketing.util.Logger; import com.dotcms.rest.api.v1.DotObjectMapperProvider; @@ -293,23 +294,52 @@ public void putToIndex(final IndexBulkRequest req) { } return b; })); - if (response.errors()) { - for (final BulkResponseItem item : response.items()) { - if (item.error() != null) { - Logger.error(this, - "OS bulk putToIndex error — id=" + item.id() - + " op=" + item.operationType() - + " type=" + item.error().type() - + " reason=" + item.error().reason()); - } - } - } + handleBulkResponse(response); } catch (final Exception e) { Logger.warnAndDebug(ContentletIndexOperationsOS.class, e); throw new DotRuntimeException(e.getMessage(), e); } } + /** + * Decides what a bulk response means to the caller. + * + *

Extracted from {@link #putToIndex(IndexBulkRequest)} so the policy can be exercised + * without a cluster — the HTTP call is not what is interesting here, the verdict is.

+ * + * @param response the response from the bulk call; {@code null} is tolerated + */ + @VisibleForTesting + void handleBulkResponse(final BulkResponse response) { + if (response == null || !response.errors()) { + return; + } + + // Same contract as the Elasticsearch provider (#37276, loss point L3): a bulk that + // returns normally while rejecting items must not read as success to the caller. + // + // This matters most in phase 3, where OpenSearch is the sole provider and there is no + // shadow leg to absorb the loss. In dual-write phases the router already isolates the + // shadow (ContentletIndexAPIImpl#putToIndex), so raising here keeps ADR-0009 intact: + // an OS failure is still swallowed while OS is the shadow, and propagates once primary. + final StringBuilder detail = new StringBuilder(); + for (final BulkResponseItem item : response.items()) { + if (item.error() != null) { + final String itemMessage = "OS bulk index operation error — id=" + item.id() + + " op=" + item.operationType() + + " type=" + item.error().type() + + " reason=" + item.error().reason(); + Logger.error(this, itemMessage); + if (detail.length() > 0) { + detail.append("; "); + } + detail.append(itemMessage); + } + } + + throw new DotRuntimeException(detail.toString()); + } + // ========================================================================= // Async bulk-processor write path // ========================================================================= diff --git a/dotCMS/src/main/java/com/dotmarketing/common/reindex/ReindexQueueFactory.java b/dotCMS/src/main/java/com/dotmarketing/common/reindex/ReindexQueueFactory.java index 03e54c76f543..86725d7155ea 100644 --- a/dotCMS/src/main/java/com/dotmarketing/common/reindex/ReindexQueueFactory.java +++ b/dotCMS/src/main/java/com/dotmarketing/common/reindex/ReindexQueueFactory.java @@ -294,7 +294,15 @@ protected Map findContentToReindex(final int recordsToRetu } for (ReindexEntry entry; (entry = queue.poll()) != null; ) { - contentToIndex.put(entry.getIdentToIndex(), entry); + // One outcome per identifier per batch, resolved by row id rather than by the order + // the entries happened to be polled in. Two entries for the same identifier are + // successive statements about what the index should hold, and only the newest is + // true: a DELETE written after a REINDEX means the content is gone, so applying the + // REINDEX afterwards would re-add a document for content that no longer exists. + // The loser stays in dist_reindex_journal and is collected on a later pass. + contentToIndex.merge(entry.getIdentToIndex(), entry, + (existing, candidate) -> candidate.getId() > existing.getId() + ? candidate : existing); if (contentToIndex.size() >= recordsToReturn) { while (entry.equals(queue.peek())) { // drain duplicate items diff --git a/dotCMS/src/main/java/com/dotmarketing/portlets/contentlet/model/IndexPolicyProvider.java b/dotCMS/src/main/java/com/dotmarketing/portlets/contentlet/model/IndexPolicyProvider.java index 0c5b8ca0518f..f8d82a76281c 100644 --- a/dotCMS/src/main/java/com/dotmarketing/portlets/contentlet/model/IndexPolicyProvider.java +++ b/dotCMS/src/main/java/com/dotmarketing/portlets/contentlet/model/IndexPolicyProvider.java @@ -25,7 +25,13 @@ public static IndexPolicyProvider getInstance() { } // getInstance. /** - * Give the index policy for single content, by default it is {@link IndexPolicy}.WAIT_FOR + * Give the index policy for single content. Defaults to {@link IndexPolicy#DEFER}, + * overridable with {@code INDEX_POLICY_SINGLE_CONTENT}. + * + *

DEFER means the index write is handed to a post-commit listener rather than applied + * inline, which is why content deletion needs a durable journal record to survive a failed + * or lost listener (see #37276).

+ * * @return IndexPolicy */ public IndexPolicy forSingleContent () { @@ -38,7 +44,9 @@ public IndexPolicy forSingleContent () { return this.singleContentIndexPolicy; } /** - * Give the index policy for single content, by default it is {@link IndexPolicy}.WAIT_FOR + * Give the index policy for a content's dependencies. Defaults to + * {@link IndexPolicy#DEFER}, overridable with {@code INDEX_POLICY_DEPENDENCIES}. + * * @return IndexPolicy */ public IndexPolicy forContentDependencies () { diff --git a/dotCMS/src/test/java/com/dotcms/content/elasticsearch/business/ContentletIndexOperationsESPartialFailureTest.java b/dotCMS/src/test/java/com/dotcms/content/elasticsearch/business/ContentletIndexOperationsESPartialFailureTest.java new file mode 100644 index 000000000000..6ca902ef6768 --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/content/elasticsearch/business/ContentletIndexOperationsESPartialFailureTest.java @@ -0,0 +1,107 @@ +package com.dotcms.content.elasticsearch.business; + +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +import org.elasticsearch.action.bulk.BulkItemResponse; +import org.elasticsearch.action.bulk.BulkResponse; +import org.junit.Test; +import org.mockito.Mockito; + +/** + * Unit tests for the failure verdict of + * {@link ContentletIndexOperationsES#handleBulkResponse(BulkResponse)}. + * + *

Covers #37276 AC-003 and AC-004, + * loss point L3.

+ * + *

Why this matters

+ *

A bulk call can return normally while rejecting individual items — a saturated write queue + * ({@code EsRejectedExecutionException}), an unavailable shard, a version conflict. Those took the + * logged branch and the method returned as if everything had been applied, so the caller could not + * distinguish a fully applied batch from one where every item was rejected. On the DEFER path, + * where the refresh policy is NONE and nothing re-reads the document, the loss was invisible.

+ * + *

These tests exercise the verdict alone. The HTTP call is not the interesting part and would + * require a cluster; {@code handleBulkResponse} was extracted from {@code putToIndex} precisely so + * the policy could be asserted without one.

+ */ +public class ContentletIndexOperationsESPartialFailureTest { + + private static final String FAILURE_MESSAGE = + "failure in bulk execution: [0]: index [working_x], id [abc_1_DEFAULT], " + + "message [EsRejectedExecutionException[rejected execution]]"; + + private static ContentletIndexOperationsES operations() { + return new ContentletIndexOperationsES(Mockito.mock(ESIndexAPI.class), + Mockito.mock(MappingOperationsES.class)); + } + + private static BulkResponse responseWithFailures() { + final BulkResponse response = Mockito.mock(BulkResponse.class); + Mockito.when(response.hasFailures()).thenReturn(true); + Mockito.when(response.buildFailureMessage()).thenReturn(FAILURE_MESSAGE); + Mockito.when(response.getItems()).thenReturn(new BulkItemResponse[1]); + return response; + } + + /** + * Given Scenario: A bulk call returns normally but the response reports per-item failures. + * When : handleBulkResponse inspects it. + * Then : the caller is told. Today the failure is logged and the method returns as success, + * which is loss point L3 — the caller commits a delete whose index removal never + * landed. + */ + @Test + public void test_partialFailure_isRaisedToCaller() { + final RuntimeException thrown = assertThrows(RuntimeException.class, + () -> operations().handleBulkResponse(responseWithFailures())); + + assertTrue("The failure detail must survive into the exception, not only the log", + thrown.getMessage() != null && thrown.getMessage().contains("rejected execution")); + } + + /** + * Given Scenario: A clean bulk response. + * When : handleBulkResponse inspects it. + * Then : nothing is raised. The escalation must not turn healthy writes into failures — this + * is the guard that keeps AC-003 from becoming a regression on the add path. + */ + @Test + public void test_cleanResponse_isSilent() { + final BulkResponse response = Mockito.mock(BulkResponse.class); + Mockito.when(response.hasFailures()).thenReturn(false); + + operations().handleBulkResponse(response); + } + + /** + * Given Scenario: A null response, which the original code tolerated. + * When : handleBulkResponse inspects it. + * Then : nothing is raised. Behaviour preserved — this is not the failure being escalated. + */ + @Test + public void test_nullResponse_isSilent() { + operations().handleBulkResponse(null); + } + + /** + * Given Scenario: A failed bulk that carried removals. + * When : the failure message is produced. + * Then : it does not describe the operation as reindexing. + * + *

AC-004. The original message read {@code "Error reindexing"} for every operation type, + * including deletes — which is why searching production logs for delete failures came back + * empty and this defect went unnoticed. The wording is the diagnostic surface, so it is + * asserted rather than left to review.

+ */ + @Test + public void test_failureMessage_doesNotMisreportRemovalsAsReindexing() { + final RuntimeException thrown = assertThrows(RuntimeException.class, + () -> operations().handleBulkResponse(responseWithFailures())); + + assertTrue("A bulk failure must not be reported as 'reindexing' — it may well be a " + + "removal, and that wording is why log searches missed this", + !thrown.getMessage().toLowerCase().contains("error reindexing")); + } +} diff --git a/dotCMS/src/test/java/com/dotcms/content/index/opensearch/ContentletIndexOperationsOSPartialFailureTest.java b/dotCMS/src/test/java/com/dotcms/content/index/opensearch/ContentletIndexOperationsOSPartialFailureTest.java new file mode 100644 index 000000000000..29dd13a3c16a --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/content/index/opensearch/ContentletIndexOperationsOSPartialFailureTest.java @@ -0,0 +1,95 @@ +package com.dotcms.content.index.opensearch; + +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +import java.util.List; +import org.junit.Test; +import org.mockito.Mockito; +import org.opensearch.client.opensearch._types.ErrorCause; +import org.opensearch.client.opensearch.core.BulkResponse; +import org.opensearch.client.opensearch.core.bulk.BulkResponseItem; + +/** + * Unit tests for the failure verdict of + * {@link ContentletIndexOperationsOS#handleBulkResponse(BulkResponse)}. + * + *

Covers #37276 AC-003, loss point L3 + * on the OpenSearch side.

+ * + *

Why OpenSearch matters here as much as Elasticsearch

+ *

The issue and the spec name only the Elasticsearch provider, but the OpenSearch one carries + * the identical defect: {@code response.errors()} is inspected, each failing item is logged, and + * the method returns normally. Fixing only Elasticsearch would leave phase 3 — where + * OpenSearch is the sole provider and there is no shadow leg to absorb a failure — with the + * original defect intact. That is the phase the migration is heading toward.

+ * + *

In dual-write phases the router isolates the shadow leg + * ({@code ContentletIndexAPIImpl#putToIndex}), so raising here does not violate ADR-0009: an + * OpenSearch failure is still swallowed while it is the shadow, and propagates once it is + * primary.

+ */ +public class ContentletIndexOperationsOSPartialFailureTest { + + private static final String REJECTION_REASON = + "rejected execution of coordinating operation, queue capacity exceeded"; + + private static ContentletIndexOperationsOS operations() { + return new ContentletIndexOperationsOS(Mockito.mock(OSClientProvider.class), + Mockito.mock(OSIndexAPIImpl.class), Mockito.mock(MappingOperationsOS.class)); + } + + private static BulkResponse responseWithErrors() { + final ErrorCause cause = Mockito.mock(ErrorCause.class); + Mockito.when(cause.type()).thenReturn("es_rejected_execution_exception"); + Mockito.when(cause.reason()).thenReturn(REJECTION_REASON); + + final BulkResponseItem item = Mockito.mock(BulkResponseItem.class); + Mockito.when(item.id()).thenReturn("abc_1_DEFAULT"); + // operationType() is left unstubbed — it only decorates the message. + Mockito.when(item.error()).thenReturn(cause); + + final BulkResponse response = Mockito.mock(BulkResponse.class); + Mockito.when(response.errors()).thenReturn(true); + Mockito.when(response.items()).thenReturn(List.of(item)); + return response; + } + + /** + * Given Scenario: An OpenSearch bulk returns normally but reports per-item errors. + * When : handleBulkResponse inspects it. + * Then : the caller is told, so a lost removal can be retried instead of being assumed done. + */ + @Test + public void test_partialFailure_isRaisedToCaller() { + final RuntimeException thrown = assertThrows(RuntimeException.class, + () -> operations().handleBulkResponse(responseWithErrors())); + + assertTrue("The rejection reason must reach the caller, not just the log", + thrown.getMessage() != null + && thrown.getMessage().contains("queue capacity exceeded")); + } + + /** + * Given Scenario: A clean OpenSearch bulk response. + * When : handleBulkResponse inspects it. + * Then : nothing is raised. + */ + @Test + public void test_cleanResponse_isSilent() { + final BulkResponse response = Mockito.mock(BulkResponse.class); + Mockito.when(response.errors()).thenReturn(false); + + operations().handleBulkResponse(response); + } + + /** + * Given Scenario: A null response. + * When : handleBulkResponse inspects it. + * Then : nothing is raised. Behaviour preserved. + */ + @Test + public void test_nullResponse_isSilent() { + operations().handleBulkResponse(null); + } +} diff --git a/dotCMS/src/test/java/com/dotmarketing/common/reindex/ReindexQueueFactoryBatchKeyTest.java b/dotCMS/src/test/java/com/dotmarketing/common/reindex/ReindexQueueFactoryBatchKeyTest.java new file mode 100644 index 000000000000..77fa8347ce13 --- /dev/null +++ b/dotCMS/src/test/java/com/dotmarketing/common/reindex/ReindexQueueFactoryBatchKeyTest.java @@ -0,0 +1,215 @@ +package com.dotmarketing.common.reindex; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import com.dotmarketing.common.reindex.ReindexQueueFactory.Priority; +import com.dotmarketing.exception.DotDataException; +import java.util.Map; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +/** + * Unit tests for the batch assembly in {@link ReindexQueueFactory#findContentToReindex(int)}. + * + *

Covers #37276 AC-008: a pending + * removal and a pending reindex for the same identifier must resolve deterministically to the + * newer of the two, and the older must not be applied afterwards.

+ * + *

Why this matters. The batch is a {@code Map} keyed by identifier alone, while + * {@link ReindexEntry} equality includes the delete flag — so the two entries are not equal (the + * duplicate-drain loop does not collapse them) yet they collide on the key, and the later + * {@code poll()} silently overwrites the earlier. Nothing depends on that today because no + * production code enqueues removals; once {@code destroyContentlets} does, the pair becomes the + * normal sequence (content saved, then destroyed).

+ * + *

Two ways the pair can arrive, and both are broken. {@code loadUpLocalQueue} reads + * {@code ORDER BY priority ASC} with no secondary sort key:

+ * + *
    + *
  • Equal priority — the common case. A save enqueues via + * {@code addIdentifierReindex}, which defaults to {@link Priority#NORMAL} (100); a destroy + * enqueues via {@code addIdentifierDelete}, also at {@link Priority#NORMAL}. With equal + * priorities and no tiebreaker, the order the database returns is undefined by + * contract. The same content can be removed correctly in one batch and silently + * re-added in the next, with nothing about the system having changed.
    • + *
  • Unequal priority — deterministically wrong. A full reindex enqueues at + * {@link Priority#REINDEX} (300) and a content-type reindex at {@link Priority#STRUCTURE} + * (200). A removal at {@link Priority#NORMAL} (100) is polled first, so the + * higher-priority reindex is polled second and overwrites it — every time.
    • + *
+ * + *

The tests below pin the unequal-priority case (deterministic, so it can be asserted) and + * the equal-priority case (asserted by id rather than by arrival order, which is the whole + * point — after the fix, arrival order stops mattering).

+ * + *

These are pure unit tests: seeding the local queue means {@code findContentToReindex} never + * reaches {@code loadUpLocalQueue}, so no database is touched.

+ */ +public class ReindexQueueFactoryBatchKeyTest { + + private static final String IDENTIFIER = "a1b2c3d4-0000-0000-0000-00000000cafe"; + private static final String OTHER_IDENTIFIER = "a1b2c3d4-0000-0000-0000-00000000beef"; + + private ReindexQueueFactory factory; + + @Before + public void setUp() { + factory = new ReindexQueueFactory(); + factory.getLocalQueue().clear(); + ReindexQueueFactory.resetLastIdReindexed(); + } + + @After + public void tearDown() { + factory.getLocalQueue().clear(); + ReindexQueueFactory.resetLastIdReindexed(); + } + + private static ReindexEntry entry(final long id, final String identifier, + final int priority, final boolean delete) { + return ReindexEntry.builder() + .id(id) + .identToIndex(identifier) + .priority(priority) + .isDelete(delete) + .build(); + } + + /** + * Given Scenario: unequal priority. A destroy queues a DELETE at NORMAL (100) while a + * full reindex has an older REINDEX pending at REINDEX (300). + * {@code ORDER BY priority ASC} polls the DELETE first, the REINDEX second. + * When : findContentToReindex assembles the batch. + * Then : the DELETE survives, because it is the newer statement about what the index should + * hold. Today the REINDEX wins simply by being polled last — deterministically, on + * every run, for as long as a full reindex overlaps a destroy. + */ + @Test + public void test_newerDelete_survives_olderReindex_forSameIdentifier() throws DotDataException { + factory.getLocalQueue().add(entry(20L, IDENTIFIER, Priority.NORMAL.dbValue(), true)); + factory.getLocalQueue().add(entry(10L, IDENTIFIER, Priority.REINDEX.dbValue(), false)); + + final Map batch = factory.findContentToReindex(50); + + assertEquals("One outcome per identifier per batch", 1, batch.size()); + final ReindexEntry winner = batch.get(IDENTIFIER); + assertTrue("The newer entry (id 20) is the DELETE; it must win", winner.isDelete()); + assertEquals(20L, winner.getId()); + } + + /** + * Given Scenario: the reverse — a REINDEX is the newer entry (id 20) and a DELETE the older + * (id 10). An identifier destroyed and later reused is a reindex, not a + * removal. + * When : findContentToReindex assembles the batch. + * Then : the REINDEX survives. Resolution is by id, not by a rule that deletes always win. + * + *

This one passes today by coincidence — arrival order happens to agree with id here. + * It is kept because after the fix it must pass for the right reason, and because it is what + * stops the fix from being implemented as "a delete always wins".

+ */ + @Test + public void test_newerReindex_survives_olderDelete_forSameIdentifier() throws DotDataException { + factory.getLocalQueue().add(entry(10L, IDENTIFIER, Priority.NORMAL.dbValue(), true)); + factory.getLocalQueue().add(entry(20L, IDENTIFIER, Priority.REINDEX.dbValue(), false)); + + final Map batch = factory.findContentToReindex(50); + + assertEquals(1, batch.size()); + final ReindexEntry winner = batch.get(IDENTIFIER); + assertFalse("The newer entry (id 20) is the REINDEX; it must win", winner.isDelete()); + assertEquals(20L, winner.getId()); + } + + /** + * Given Scenario: equal priority — the case a customer actually hits. A save queues a + * REINDEX at NORMAL (100) and the subsequent destroy queues a DELETE at + * NORMAL (100) too. {@code ORDER BY priority ASC} has no tiebreaker, so the + * order the database returns is undefined; this test pins both + * arrival orders and requires the same outcome from each. + * When : findContentToReindex assembles the batch. + * Then : the newer entry by id wins regardless of the order the entries arrived in. + * + *

This is the test that matters most. The other collision cases are deterministic and + * therefore at least debuggable; this one is not. Today the same content can be removed + * correctly in one batch and silently re-added in the next, with nothing about the system + * having changed — which is exactly the "unexplained drift" shape the field report had.

+ */ + @Test + public void test_equalPriority_newestWins_regardlessOfArrivalOrder() throws DotDataException { + // Arrival order A: the DELETE (newer) is polled first. + factory.getLocalQueue().add(entry(20L, IDENTIFIER, Priority.NORMAL.dbValue(), true)); + factory.getLocalQueue().add(entry(10L, IDENTIFIER, Priority.NORMAL.dbValue(), false)); + + Map batch = factory.findContentToReindex(50); + + assertEquals(1, batch.size()); + assertTrue("Arrival order A: the newer entry (id 20) is the DELETE and must win", + batch.get(IDENTIFIER).isDelete()); + assertEquals(20L, batch.get(IDENTIFIER).getId()); + + // Arrival order B: identical entries, opposite order. The outcome must not change. + factory.getLocalQueue().clear(); + ReindexQueueFactory.resetLastIdReindexed(); + factory.getLocalQueue().add(entry(10L, IDENTIFIER, Priority.NORMAL.dbValue(), false)); + factory.getLocalQueue().add(entry(20L, IDENTIFIER, Priority.NORMAL.dbValue(), true)); + + batch = factory.findContentToReindex(50); + + assertEquals(1, batch.size()); + assertTrue("Arrival order B: same entries, same winner — order must not decide this", + batch.get(IDENTIFIER).isDelete()); + assertEquals(20L, batch.get(IDENTIFIER).getId()); + } + + /** + * Given Scenario: a colliding pair for one identifier plus an unrelated identifier. + * When : findContentToReindex assembles the batch. + * Then : the losing entry is absent from the batch and unrelated identifiers are untouched. + * + *

The losing entry is only dropped from this batch — never from + * {@code dist_reindex_journal}. Rows are removed exclusively by + * {@code deleteReindexEntry} on a successful bulk acknowledgement, and + * {@code findContentToReindex} performs no database write at all. It will be re-picked on a + * later pass and lose to the same newer entry until that one is applied and removed.

+ */ + @Test + public void test_losingEntry_isDroppedFromBatch_notFromJournal() throws DotDataException { + factory.getLocalQueue().add(entry(20L, IDENTIFIER, Priority.NORMAL.dbValue(), true)); + factory.getLocalQueue().add(entry(10L, IDENTIFIER, Priority.REINDEX.dbValue(), false)); + factory.getLocalQueue().add(entry(30L, OTHER_IDENTIFIER, Priority.NORMAL.dbValue(), false)); + + final Map batch = factory.findContentToReindex(50); + + assertEquals("Two identifiers in, two outcomes out", 2, batch.size()); + assertTrue(batch.get(IDENTIFIER).isDelete()); + assertFalse(batch.get(OTHER_IDENTIFIER).isDelete()); + assertEquals("The unrelated identifier is unaffected by the collision", + 30L, batch.get(OTHER_IDENTIFIER).getId()); + } + + /** + * Given Scenario: the same identifier queued three times as identical REINDEX entries, as a + * hot identifier produces during a full reindex. + * When : findContentToReindex assembles the batch. + * Then : they collapse to a single entry. + * + *

This guards the throughput regression rejected in {@code data-model.md}: keying the + * batch by id instead of identifier would turn every redundant entry into its own bulk + * operation on the highest-volume path in the pipeline.

+ */ + @Test + public void test_identicalRepeatedReindexEntries_areStillDeduplicated() throws DotDataException { + factory.getLocalQueue().add(entry(10L, IDENTIFIER, Priority.NORMAL.dbValue(), false)); + factory.getLocalQueue().add(entry(11L, IDENTIFIER, Priority.NORMAL.dbValue(), false)); + factory.getLocalQueue().add(entry(12L, IDENTIFIER, Priority.NORMAL.dbValue(), false)); + + final Map batch = factory.findContentToReindex(50); + + assertEquals("Repeated entries for one identifier collapse to one", 1, batch.size()); + assertFalse(batch.get(IDENTIFIER).isDelete()); + } +} diff --git a/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java b/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java index 521cf43dcb6c..7ee24ed84d69 100644 --- a/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java +++ b/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java @@ -26,6 +26,9 @@ com.dotcms.content.elasticsearch.business.ESSiteSearchAPITest.class, com.dotcms.content.elasticsearch.business.ESMappingAPITest.class, com.dotcms.content.elasticsearch.business.ContentletIndexAPIImplTest.class, + com.dotcms.content.elasticsearch.business.ContentletDestroyIndexRemovalTest.class, + com.dotcms.content.elasticsearch.business.ContentletIndexPartialFailurePhaseTest.class, + com.dotcms.content.elasticsearch.business.ContentletIndexProviderSkipTest.class, com.dotcms.contenttype.test.ContentTypeAPIImplTest.class, com.dotcms.contenttype.test.ContentTypeBuilderTest.class, com.dotcms.contenttype.test.ContentTypeFactoryImplTest.class, diff --git a/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java b/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java index d864203e1850..1e124bc98081 100644 --- a/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java +++ b/dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java @@ -270,6 +270,7 @@ com.dotmarketing.common.reindex.ReindexThreadTest.class, com.dotcms.content.elasticsearch.business.ContentletIndexAPIImplMappingTimeoutIT.class, com.dotmarketing.common.reindex.ReindexAPITest.class, + com.dotmarketing.common.reindex.ReindexDeleteJournalTest.class, CleanUpFieldReferencesJobTest.class, EMAWebInterceptorTest.class, diff --git a/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ContentletDestroyIndexRemovalTest.java b/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ContentletDestroyIndexRemovalTest.java new file mode 100644 index 000000000000..e928ec3f126f --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ContentletDestroyIndexRemovalTest.java @@ -0,0 +1,159 @@ +package com.dotcms.content.elasticsearch.business; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import com.dotcms.contenttype.model.type.ContentType; +import com.dotcms.datagen.ContentTypeDataGen; +import com.dotcms.datagen.ContentletDataGen; +import com.dotcms.datagen.LanguageDataGen; +import com.dotcms.util.IntegrationTestInitService; +import com.dotmarketing.business.APILocator; +import com.dotmarketing.common.db.DotConnect; +import com.dotmarketing.common.reindex.ReindexThread; +import com.dotmarketing.exception.DotDataException; +import com.dotmarketing.portlets.contentlet.model.Contentlet; +import com.dotmarketing.portlets.contentlet.model.IndexPolicy; +import com.dotmarketing.portlets.languagesmanager.model.Language; +import com.dotmarketing.util.Config; +import com.liferay.portal.model.User; +import org.junit.After; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Integration tests for index removal on content destruction. + * + *

Covers #37276:

+ *
    + *
  • AC-001 / AC-002 — a removal that could not be applied when the content was + * destroyed is still applied afterwards, and the index count stops disagreeing with the + * number of contentlets that actually resolve.
    • + *
  • AC-006 — the unpublish/archive path is unchanged: no language that should remain + * live is removed. This is the spec's primary non-goal and the reason the durable-removal + * mechanism must not be extended there.
    • + *
+ */ +public class ContentletDestroyIndexRemovalTest { + + private static User systemUser; + private static ContentType contentType; + + @BeforeClass + public static void prepare() throws Exception { + IntegrationTestInitService.getInstance().init(); + systemUser = APILocator.systemUser(); + contentType = new ContentTypeDataGen().nextPersisted(); + } + + @Before + public void pauseReindex() throws DotDataException { + Config.setProperty("ALLOW_MANUAL_REINDEX_UNPAUSE", true); + ReindexThread.pause(); + new DotConnect().setSQL("delete from dist_reindex_journal").loadResult(); + } + + @After + public void resume() { + Config.setProperty("ALLOW_MANUAL_REINDEX_UNPAUSE", false); + ReindexThread.unpause(); + } + + private long indexCountFor(final String identifier) throws Exception { + return APILocator.getContentletAPI() + .indexCount("+identifier:" + identifier, systemUser, false); + } + + /** + * Method to test: {@link com.dotmarketing.portlets.contentlet.business.ContentletAPI#destroy} + * Given Scenario: Content is destroyed while the reindex journal is paused, standing in for + * an index that cannot accept the write at the moment of the destroy — the + * shape of every reproduction path in the spec (write rejection, process + * stop, provider unavailable). + * Expected Result: The database rows are gone immediately, the pending removal is still owed, + * and once the journal drains the index document is removed. Before the fix + * the removal is lost with the paused in-memory listener and the document + * remains forever. + * + *

Pausing the journal is a deliberate substitution for forcing a bulk rejection: it + * reproduces the property under test — the removal cannot be applied now — without depending + * on index-cluster tuning that would make the test environment-sensitive.

+ */ + @Test + public void test_destroyWithUnavailableIndexWrite_removalIsAppliedOnceItRecovers() + throws Exception { + final Contentlet contentlet = new ContentletDataGen(contentType.id()) + .setPolicy(IndexPolicy.WAIT_FOR).nextPersisted(); + final String identifier = contentlet.getIdentifier(); + + assertEquals("Precondition: the contentlet is in the index", 1, indexCountFor(identifier)); + + new DotConnect().setSQL("delete from dist_reindex_journal").loadResult(); + APILocator.getContentletAPI().destroy(contentlet, systemUser, false); + + assertTrue("The database rows must be gone immediately", + APILocator.getContentletAPI() + .findAllVersions(APILocator.getIdentifierAPI().find(identifier), + systemUser, false).isEmpty()); + + // Let the durable record drive the removal. + Config.setProperty("ALLOW_MANUAL_REINDEX_UNPAUSE", false); + ReindexThread.unpause(); + ReindexThread.startThread(); + + boolean removed = false; + for (int attempt = 0; attempt < 60 && !removed; attempt++) { + removed = indexCountFor(identifier) == 0; + if (!removed) { + Thread.sleep(500); + } + } + + assertTrue("The index document must be removed once the write path recovers — " + + "an inflated count over content that no longer resolves is the field symptom", + removed); + } + + /** + * Method to test: {@link com.dotmarketing.portlets.contentlet.business.ContentletAPI#unpublish} + * Given Scenario: A contentlet with live versions in two languages; one language is + * unpublished. + * Expected Result: Only that language's live document leaves the index. The other stays live. + * + *

AC-006, the regression guard for the spec's primary non-goal. A journal entry is + * identifier-wide, so reusing the durable-removal mechanism on this path would remove every + * language of the identifier. This test is what makes that mistake fail loudly.

+ */ + @Test + public void test_unpublishOneLanguage_leavesOtherLanguagesLive() throws Exception { + final Language secondLanguage = new LanguageDataGen().nextPersisted(); + + final Contentlet defaultLang = new ContentletDataGen(contentType.id()) + .setPolicy(IndexPolicy.WAIT_FOR).nextPersistedAndPublish(); + final String identifier = defaultLang.getIdentifier(); + + final Contentlet otherLang = new ContentletDataGen(contentType.id()) + .languageId(secondLanguage.getId()) + .setProperty("identifier", identifier) + .setPolicy(IndexPolicy.WAIT_FOR).nextPersistedAndPublish(); + + assertEquals("Precondition: both languages are live in the index", 2, + APILocator.getContentletAPI() + .indexCount("+identifier:" + identifier + " +live:true", + systemUser, false)); + + APILocator.getContentletAPI().unpublish(defaultLang, systemUser, false); + + assertEquals("Unpublishing one language must leave the other live — a removal here is " + + "per language, never identifier-wide", 1, + APILocator.getContentletAPI() + .indexCount("+identifier:" + identifier + " +live:true", + systemUser, false)); + assertEquals("The surviving live document must be the language that was not unpublished", + 1, + APILocator.getContentletAPI() + .indexCount("+identifier:" + identifier + " +live:true +languageId:" + + otherLang.getLanguageId(), systemUser, false)); + } +} diff --git a/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ContentletIndexPartialFailurePhaseTest.java b/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ContentletIndexPartialFailurePhaseTest.java new file mode 100644 index 000000000000..ed504fb9ee5c --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ContentletIndexPartialFailurePhaseTest.java @@ -0,0 +1,104 @@ +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.assertTrue; + +import com.dotcms.contenttype.model.type.ContentType; +import com.dotcms.datagen.ContentTypeDataGen; +import com.dotcms.datagen.ContentletDataGen; +import com.dotcms.util.IntegrationTestInitService; +import com.dotmarketing.business.APILocator; +import com.dotmarketing.portlets.contentlet.model.Contentlet; +import com.dotmarketing.portlets.contentlet.model.IndexPolicy; +import com.dotmarketing.util.Config; +import com.liferay.portal.model.User; +import org.junit.After; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Integration tests guarding the blast radius of the partial-bulk-failure escalation + * (#37276, AC-003 and AC-005). + * + *

What is at stake

+ *

Making a partial bulk failure reach the caller affects every index write, not just + * removals. Two things must stay true:

+ *
    + *
  • ADR-0009. In dual-write phases the OpenSearch leg is a shadow: its failures are + * "logged but do not impact operations". The escalation lives in the providers while the + * isolation lives in the router ({@code ContentletIndexAPIImpl#putToIndex}), so a shadow + * failure must still be swallowed. If this test fails, the escalation was put in the router + * by mistake.
    • + *
  • AC-005. Ordinary add, publish and reindex traffic must be unaffected. The + * escalation only changes what happens on a failed bulk, never on a healthy one.
    • + *
+ */ +public class ContentletIndexPartialFailurePhaseTest { + + private static User systemUser; + private static ContentType contentType; + + @BeforeClass + public static void prepare() throws Exception { + IntegrationTestInitService.getInstance().init(); + systemUser = APILocator.systemUser(); + contentType = new ContentTypeDataGen().nextPersisted(); + } + + @After + public void clearPhase() { + Config.setProperty(FLAG_KEY, null); + } + + private static void setPhase(final int ordinal) { + Config.setProperty(FLAG_KEY, String.valueOf(ordinal)); + } + + /** + * Method to test: {@link com.dotmarketing.portlets.contentlet.business.ContentletAPI#checkin} + * Given Scenario: Ordinary content is saved and published in the default phase. + * Expected Result: It succeeds and is searchable. The escalation must not turn healthy writes + * into failures — this is the AC-005 blast-radius check for the change that + * makes partial bulk failures raise. + */ + @Test + public void test_ordinaryWrites_areUnaffectedByTheEscalation() throws Exception { + final Contentlet contentlet = new ContentletDataGen(contentType.id()) + .setPolicy(IndexPolicy.WAIT_FOR).nextPersistedAndPublish(); + + assertEquals("A healthy write must remain a healthy write", 1, + APILocator.getContentletAPI().indexCount( + "+identifier:" + contentlet.getIdentifier(), systemUser, false)); + + // And the working copy round-trips through the reindex path unchanged. + APILocator.getContentletIndexAPI().addContentToIndex(contentlet, false); + assertTrue("Reindexing existing content must not raise", + APILocator.getContentletAPI().indexCount( + "+identifier:" + contentlet.getIdentifier(), systemUser, false) >= 1); + } + + /** + * Method to test: {@code ContentletIndexAPIImpl#putToIndex(IndexBulkRequest)} + * Given Scenario: A dual-write phase (1), where Elasticsearch is primary and OpenSearch is the + * shadow leg. + * Expected Result: A content write succeeds from the caller's point of view even though the + * shadow leg may diverge. ADR-0009: "write failures to 3.x logged but do not + * impact operations." + * + *

This is the test that stops a well-meaning refactor from moving the escalation up into + * the router, which would make every shadow hiccup fail a user-facing save.

+ */ + @Test + public void test_dualWritePhase_shadowFailureDoesNotReachCaller() throws Exception { + setPhase(1); + + final Contentlet contentlet = new ContentletDataGen(contentType.id()) + .setPolicy(IndexPolicy.WAIT_FOR).nextPersisted(); + + assertEquals("In a dual-write phase the primary decides the caller's outcome; a shadow " + + "divergence is logged, never raised", 1, + APILocator.getContentletAPI().indexCount( + "+identifier:" + contentlet.getIdentifier(), systemUser, false)); + } +} diff --git a/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ContentletIndexProviderSkipTest.java b/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ContentletIndexProviderSkipTest.java new file mode 100644 index 000000000000..010b66aed156 --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ContentletIndexProviderSkipTest.java @@ -0,0 +1,125 @@ +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 static org.junit.Assert.assertTrue; + +import com.dotcms.contenttype.model.type.ContentType; +import com.dotcms.datagen.ContentTypeDataGen; +import com.dotcms.datagen.ContentletDataGen; +import com.dotcms.util.IntegrationTestInitService; +import com.dotmarketing.business.APILocator; +import com.dotmarketing.business.CacheLocator; +import com.dotmarketing.common.db.DotConnect; +import com.dotmarketing.portlets.contentlet.model.Contentlet; +import com.dotmarketing.portlets.contentlet.model.IndexPolicy; +import com.dotmarketing.util.Config; +import com.liferay.portal.model.User; +import org.junit.After; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Integration tests for what a skipped index provider means on the removal path. + * + *

Covers #37276 loss point L2.

+ * + *

Why this was invisible

+ *

{@code loadProviderIndicesQuietly} turns any failure to resolve a provider's index pointers + * into {@code null} plus a warning, and the loop moved on. No delete operation was added for that + * provider, and {@code putToIndex} early-returns on an empty batch — so a removal that never + * happened was indistinguishable from one that did.

+ * + *

The distinction that matters is primary versus shadow. {@code writeProviders()} is ordered + * primary-first in every phase (0 → [ES], 1/2 → [ES, OS], 3 → [OS]), so element 0 is the provider + * whose outcome the caller is entitled to. A shadow keeps warn-and-continue, which is what + * ADR-0009 requires of the OpenSearch leg during dual-write.

+ * + *

Why this is an integration test and not a unit test

+ *

The removal path takes a {@link Contentlet}, whose construction pulls in enough of the + * container that a plain unit test cannot reach it — the existing unit tests in this area + * deliberately exercise only the index-name overloads for that reason.

+ */ +public class ContentletIndexProviderSkipTest { + + private static User systemUser; + private static ContentType contentType; + + @BeforeClass + public static void prepare() throws Exception { + IntegrationTestInitService.getInstance().init(); + systemUser = APILocator.systemUser(); + contentType = new ContentTypeDataGen().nextPersisted(); + } + + @After + public void clearPhase() { + Config.setProperty(FLAG_KEY, null); + } + + private static void setPhase(final int ordinal) { + Config.setProperty(FLAG_KEY, String.valueOf(ordinal)); + } + + /** + * Method to test: {@code ContentletIndexAPI#removeContentFromIndex(Contentlet)} + * Given Scenario: The index pointers for the primary provider cannot be resolved while a + * removal is attempted. The pointer record is emptied to force it. + * Expected Result: The caller is told the removal did not happen, rather than the operation + * completing silently with the document still in the index. + * + *

Before the fix this logged a warning and returned normally, so a destroy would commit + * with the index document still in place and nothing recording that it was still owed.

+ */ + @Test + public void test_primaryProviderPointersUnavailable_isNotReportedAsRemoved() throws Exception { + setPhase(0); + + final Contentlet contentlet = new ContentletDataGen(contentType.id()) + .setPolicy(IndexPolicy.FORCE).nextPersisted(); + + // Force the primary to resolve no active index. Deleting the rows is not enough on its + // own — IndiciesAPI reads through IndiciesCache, so the cache must be flushed too or the + // pointers stay visible and the removal proceeds normally. + final IndiciesInfo backup = APILocator.getIndiciesAPI().loadIndicies(); + try { + new DotConnect().setSQL("delete from indicies").loadResult(); + CacheLocator.getIndiciesCache().clearCache(); + + contentlet.setIndexPolicy(IndexPolicy.FORCE); + final RuntimeException thrown = assertThrows(RuntimeException.class, + () -> APILocator.getContentletIndexAPI().removeContentFromIndex(contentlet)); + + assertTrue("The failure must say the removal was not performed, not merely that a " + + "provider was skipped — a warning is what made L2 invisible", + thrown.getMessage() != null && thrown.getMessage().contains("NOT")); + } finally { + // Restore the environment: other tests in the suite depend on these pointers. + APILocator.getIndiciesAPI().point(backup); + CacheLocator.getIndiciesCache().clearCache(); + } + } + + /** + * Method to test: {@code ContentletIndexAPI#removeContentFromIndex(Contentlet)} + * Given Scenario: A dual-write phase where the primary resolves normally. + * Expected Result: The removal succeeds. A shadow provider that cannot resolve its pointers + * must keep warn-and-continue — ADR-0009 — so only the primary can fail a + * removal for the caller. + */ + @Test + public void test_shadowProviderSkip_stillCompletesTheRemoval() throws Exception { + setPhase(1); + + final Contentlet contentlet = new ContentletDataGen(contentType.id()) + .setPolicy(IndexPolicy.FORCE).nextPersisted(); + final String identifier = contentlet.getIdentifier(); + + APILocator.getContentletIndexAPI().removeContentFromIndex(contentlet); + + assertEquals("With the primary healthy, the removal completes regardless of the shadow", + 0, APILocator.getContentletAPI() + .indexCount("+identifier:" + identifier, systemUser, false)); + } +} diff --git a/dotcms-integration/src/test/java/com/dotmarketing/common/reindex/ReindexDeleteJournalTest.java b/dotcms-integration/src/test/java/com/dotmarketing/common/reindex/ReindexDeleteJournalTest.java new file mode 100644 index 000000000000..ebc3287a9756 --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotmarketing/common/reindex/ReindexDeleteJournalTest.java @@ -0,0 +1,214 @@ +package com.dotmarketing.common.reindex; + +import static com.dotmarketing.common.reindex.ReindexQueueFactory.REINDEX_MAX_FAILURE_ATTEMPTS; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +import com.dotcms.contenttype.model.type.ContentType; +import com.dotcms.datagen.ContentTypeDataGen; +import com.dotcms.datagen.ContentletDataGen; +import com.dotcms.util.IntegrationTestInitService; +import com.dotmarketing.business.APILocator; +import com.dotmarketing.common.db.DotConnect; +import com.dotmarketing.common.reindex.ReindexQueueFactory.Priority; +import com.dotmarketing.common.reindex.ReindexQueueFactory.ReindexAction; +import com.dotmarketing.db.HibernateUtil; +import com.dotmarketing.exception.DotDataException; +import com.dotmarketing.portlets.contentlet.model.Contentlet; +import com.dotmarketing.portlets.contentlet.model.IndexPolicy; +import com.dotmarketing.util.Config; +import com.liferay.portal.model.User; +import java.util.List; +import java.util.Map; +import org.junit.After; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Integration tests for the durable record of a content deletion in + * {@code dist_reindex_journal}. + * + *

Covers #37276:

+ *
    + *
  • AC-001 — the pending removal survives as a durable record rather than being lost + * with the in-memory commit listener.
    • + *
  • AC-007 — a removal that exhausts its retry attempts stays discoverable.
    • + *
+ * + *

Destroying content deletes the database rows inside a transaction and then defers the index + * removal to a post-commit listener that records nothing durable. These tests assert the journal + * row that makes that removal survivable — which is the testable substance of AC-001, since the + * JVM-restart path itself cannot be exercised from a test.

+ */ +public class ReindexDeleteJournalTest { + + private static final ReindexQueueFactory factory = new ReindexQueueFactory(); + + private static User systemUser; + private static ContentType contentType; + + @BeforeClass + public static void prepare() throws Exception { + IntegrationTestInitService.getInstance().init(); + systemUser = APILocator.systemUser(); + contentType = new ContentTypeDataGen().nextPersisted(); + } + + @Before + public void pauseReindexAndClearJournal() throws DotDataException { + // The journal must not drain underneath the assertions. + Config.setProperty("ALLOW_MANUAL_REINDEX_UNPAUSE", true); + ReindexThread.pause(); + new DotConnect().setSQL("delete from dist_reindex_journal").loadResult(); + } + + @After + public void resume() { + Config.setProperty("ALLOW_MANUAL_REINDEX_UNPAUSE", false); + ReindexThread.unpause(); + } + + /** Reads the journal rows for one identifier, newest first. */ + private List> journalRowsFor(final String identifier) + throws DotDataException { + return new DotConnect() + .setSQL("select id, ident_to_index, priority, dist_action, index_val " + + "from dist_reindex_journal where ident_to_index = ? order by id desc") + .addParam(identifier) + .loadObjectResults(); + } + + /** + * Reads only the DELETE rows for one identifier. + * + *

A destroy also enqueues REINDEX entries for the content it touches on the way out — + * relationships, categories and permission-driven reindexes all land in the same journal for + * the same identifier. Those are legitimate and unrelated to the removal contract, so the + * assertions here filter to {@code dist_action = DELETE} rather than counting every row. + * (That REINDEX-beside-DELETE pair for one identifier is precisely the batch collision fixed + * alongside this work — see ReindexQueueFactoryBatchKeyTest.)

+ */ + private List> deleteRowsFor(final String identifier) + throws DotDataException { + return new DotConnect() + .setSQL("select id, ident_to_index, priority, dist_action, index_val " + + "from dist_reindex_journal where ident_to_index = ? and dist_action = ? " + + "order by id desc") + .addParam(identifier) + .addParam(ReindexAction.DELETE.ordinal()) + .loadObjectResults(); + } + + private static int intOf(final Map row, final String column) { + return ((Number) row.get(column)).intValue(); + } + + /** + * Method to test: {@link com.dotmarketing.portlets.contentlet.business.ContentletAPI#destroy} + * Given Scenario: A contentlet is created and then destroyed. + * Expected Result: A dist_reindex_journal row exists for its identifier carrying + * dist_action = DELETE, so the removal is owed durably and will be retried + * even if the in-memory commit listener never runs. + */ + @Test + public void test_destroy_writesDurableDeleteEntryToJournal() throws Exception { + final Contentlet contentlet = new ContentletDataGen(contentType.id()) + .setPolicy(IndexPolicy.DEFER).nextPersisted(); + final String identifier = contentlet.getIdentifier(); + + new DotConnect().setSQL("delete from dist_reindex_journal").loadResult(); + + APILocator.getContentletAPI().destroy(contentlet, systemUser, false); + + final List> deletes = deleteRowsFor(identifier); + assertEquals("Destroy must leave exactly one durable removal record. All journal rows for " + + "this identifier: " + journalRowsFor(identifier), + 1, deletes.size()); + } + + /** + * Method to test: {@link com.dotmarketing.portlets.contentlet.business.ContentletAPI#destroy} + * Given Scenario: A destroy is performed inside a transaction that is then rolled back. + * Expected Result: No journal row survives. The removal record must share the fate of the row + * deletion it describes — enqueuing outside the transaction would leave the + * index owing a removal for content that was never actually deleted. + */ + @Test + public void test_rolledBackDestroy_leavesNoJournalEntry() throws Exception { + final Contentlet contentlet = new ContentletDataGen(contentType.id()) + .setPolicy(IndexPolicy.DEFER).nextPersisted(); + final String identifier = contentlet.getIdentifier(); + + new DotConnect().setSQL("delete from dist_reindex_journal").loadResult(); + + try { + HibernateUtil.startTransaction(); + APILocator.getContentletAPI().destroy(contentlet, systemUser, false); + HibernateUtil.rollbackTransaction(); + } finally { + HibernateUtil.closeSessionSilently(); + } + + assertTrue("A rolled-back destroy must leave no removal record", + deleteRowsFor(identifier).isEmpty()); + } + + /** + * Method to test: {@link ReindexQueueFactory#markAsFailed(ReindexEntry, String)} + * Given Scenario: A pending removal fails REINDEX_MAX_FAILURE_ATTEMPTS times. + * Expected Result: The row is still in the journal, parked above ERROR priority, still marked + * as a DELETE, and carrying the last failure cause — so the set of removals + * still owed to the index can be enumerated with one query. + * + *

AC-007. The acceptance here is enumerability, not the retry count: asserting the + * number of attempts would pin the test to a configurable value, while what an operator needs + * is that the residue can be found at all. Exhaustion must not delete the record.

+ */ + @Test + public void test_exhaustedRemoval_staysDiscoverableInJournal() throws Exception { + final Contentlet contentlet = new ContentletDataGen(contentType.id()) + .setPolicy(IndexPolicy.DEFER).nextPersisted(); + final String identifier = contentlet.getIdentifier(); + + new DotConnect().setSQL("delete from dist_reindex_journal").loadResult(); + + APILocator.getContentletAPI().destroy(contentlet, systemUser, false); + + final List> initial = deleteRowsFor(identifier); + assertEquals("Precondition: the destroy left a removal record. All journal rows: " + + journalRowsFor(identifier), 1, initial.size()); + + // Drive the entry through its retry budget. + ReindexEntry entry = ReindexEntry.builder() + .id(((Number) initial.get(0).get("id")).longValue()) + .identToIndex(identifier) + .priority(intOf(initial.get(0), "priority")) + .isDelete(true) + .build(); + + for (int attempt = 0; attempt <= REINDEX_MAX_FAILURE_ATTEMPTS; attempt++) { + factory.markAsFailed(entry, "forced failure " + attempt); + final Map row = deleteRowsFor(identifier).get(0); + entry = ReindexEntry.builder() + .id(((Number) row.get("id")).longValue()) + .identToIndex(identifier) + .priority(intOf(row, "priority")) + .isDelete(true) + .build(); + } + + final List> after = deleteRowsFor(identifier); + assertEquals("Exhaustion must not delete the record", 1, after.size()); + + final Map parked = after.get(0); + assertTrue("An exhausted removal is parked above ERROR priority, where the drain query " + + "no longer reaches it — that is what makes it enumerable residue", + intOf(parked, "priority") > Priority.ERROR.dbValue()); + assertEquals("It must still be a DELETE — the pending work is a removal, not a reindex", + ReindexAction.DELETE.ordinal(), intOf(parked, "dist_action")); + assertNotNull("The last failure cause must be recorded for the operator", + parked.get("index_val")); + } +} 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 new file mode 100644 index 000000000000..03fcfb77225b --- /dev/null +++ b/specs/37276-silent-index-delete-loss/contracts/putToIndex-failure-contract.md @@ -0,0 +1,66 @@ +# Contract change: `putToIndex` partial bulk failure + +`putToIndex` is declared on the public interface `ContentletIndexAPI` (`:159`) and implemented +by the router (`ContentletIndexAPIImpl:2414`) and by each provider +(`ContentletIndexOperationsES:197`, `ContentletIndexOperationsOS:280`). Out-of-tree callers — +plugins, OSGi bundles — can reach the interface method, so this is a contract change and not +an internal refactor. + +## Before + +```java +void putToIndex(IndexBulkRequest bulkRequest); +``` + +Throws only when the bulk **call itself** fails (transport error, client illegal state). A +response carrying **per-item** failures — `EsRejectedExecutionException` from a saturated write +queue, an unavailable shard, a version conflict — is logged and the method returns normally. +The caller cannot distinguish a fully applied batch from one where every item was rejected. + +## After + +Throws when the bulk call fails **or** when the response reports per-item failures. The caller +can no longer mistake a partially or wholly rejected batch for a successful one. + +Unchanged: + +- An empty batch is a no-op and returns without contacting the index. +- The exception type stays `DotRuntimeException` / `DotIndexException` — no new checked + exception, no signature change, so this is source- and binary-compatible. + +## 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. + +| 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** | +| 3 | OS only | n/a | propagates | + +## Impact on callers + +In-tree callers of the router method, all in `ContentletIndexAPIImpl`: + +| Site | Method | Effect of the change | +|------|--------|----------------------| +| `:2364` | `indexContentListNow` (`FORCE`) | A rejected add now raises instead of being silently dropped. | +| `:2372` | `indexContentListWaitFor` (`WAIT_FOR`) | Same. | +| `:2379` | `indexContentListDefer` (`DEFER`) | Same. Reached from the journal drain, so the entry is marked failed and retried — the desired outcome. | +| `:3167` | `removeContentAndProcessDependencies` | The delete loss point (L3). With the journal entry in place the removal is retried. | + +Out-of-tree callers cannot be enumerated from this repository. A caller that today relies on +`putToIndex` returning normally after a partial failure will begin seeing an exception. + +## Migration note for the release + +This is a behavior change that can look like a regression and is not one. An environment that +has been silently losing index writes will start surfacing errors at the moment of the write +rather than as unexplained index drift weeks later. The errors were always happening; only +their visibility changed. + +Operators seeing new `putToIndex` failures after upgrading should treat them as a pre-existing +condition now made visible — typically index write-queue saturation — and not as a fault +introduced by this release. diff --git a/specs/37276-silent-index-delete-loss/data-model.md b/specs/37276-silent-index-delete-loss/data-model.md new file mode 100644 index 000000000000..569423a91d72 --- /dev/null +++ b/specs/37276-silent-index-delete-loss/data-model.md @@ -0,0 +1,107 @@ +# Data Model: Silent index delete loss + +No schema change. This feature uses `dist_reindex_journal` as it already exists; what changes +is which rows get written and how a batch of them is assembled. + +## Entities + +### `dist_reindex_journal` (existing table) + +The durable record of index work owed. Written inside the transaction that changes content; +drained and retried by `ReindexThread` until the index acknowledges, then deleted. + +| Column | Used by this feature | Notes | +|--------|----------------------|-------| +| `id` | yes — ordering | Monotonic per insert. The only reliable ordering signal between two entries for the same identifier. | +| `ident_to_index` | yes — batch key | Contentlet identifier. | +| `inode_to_index` | no | Set to the identifier by the existing enqueue paths. | +| `priority` | yes | Encodes both dispatch priority and retry count (`ReindexEntry.errorCount()` = `priority % 100`). | +| `dist_action` | **yes — the load-bearing field** | `ReindexAction.ordinal()`: `NONE=0`, `REINDEX=1`, `DELETE=2`. Already written by `addIdentifierDelete`, already read back, already consumed by both providers. | +| `index_val` | no | Failure cause, set by `markAsFailed`. | +| `serverid` | no | Cleared on failure so another node can pick it up. | +| `time_entered` | no | | + +**Validation rule that must hold**: a row with `dist_action = DELETE` refers to content that no +longer exists in the database. Any query that assembles or filters journal rows must therefore +not join to `identifier` or `contentlet` — such a join would silently discard exactly these +rows. The current drain query (`ReindexQueueFactory:329-338`) is a plain select and satisfies +this; it must stay that way. + +### `ReindexEntry` (existing immutable value object) + +| Field | In equality? | Notes | +|-------|--------------|-------| +| `identToIndex` | yes | | +| `priority` | yes | | +| `isDelete` | **yes** | `@Value.Default false`. This is why a REINDEX and a DELETE entry for the same identifier are *not* equal. | +| `serverId` | yes | | +| `id` | no — `@Value.Auxiliary` | | +| `lastResult` | no — `@Value.Auxiliary` | | +| `timeEntered` | no — `@Value.Auxiliary` | | + +No change to this type is required. + +## The batch key — the one modelling decision + +`ReindexQueueFactory.findContentToReindex` assembles a batch as +`Map` keyed by `identToIndex` alone. Two entries for the same identifier +that differ only in `isDelete` are unequal (so the duplicate-drain loop below the `put` does +not collapse them) yet collide on the key, and the later `poll()` silently overwrites the +earlier. + +**Decision: collapse to the newest entry per identifier, using `id` as the ordering signal.** + +The rationale is semantic, not mechanical. Two journal entries for one identifier are not +independent work items — they are successive statements about what the index should hold for +that identifier, and only the last one is true. A DELETE written after a REINDEX means the +content is gone; applying the REINDEX afterwards would re-add a document for content that no +longer exists. The reverse order is equally meaningful: content destroyed and an identifier +later reused is a reindex, not a removal. + +So the batch key stays the identifier — one outcome per identifier per batch is correct — and +the collision is resolved deterministically by `id` instead of by poll order. Entries that lose +are dropped from the batch, not from the journal; their rows remain and are collected on a +later pass, where they will lose again to the same newer entry until it is applied and removed. + +| | Before | After | +|---|--------|-------| +| Key | `identToIndex` | `identToIndex` (unchanged) | +| Collision resolution | arbitrary — last `poll()` wins | deterministic — highest `id` wins | +| REINDEX after DELETE | can re-add a deleted document | cannot: DELETE has the higher `id` | +| Losing row | stays in journal, re-picked later | unchanged | + +**Alternatives rejected**: + +- *Key by `identToIndex` + `isDelete`.* Lets both entries into the same batch with no defined + order between them. That does not fix the race; it relocates it into the bulk request, where + ordering depends on iteration order of a `HashMap`. +- *Key by `id`.* Removes deduplication entirely. Every redundant REINDEX for a hot identifier + becomes its own bulk operation — a throughput regression on the full-reindex path, which is + the highest-volume consumer of this batch. + +## State transitions + +``` +content saved → REINDEX row (dist_action = 1) ─┐ +content destroyed → DELETE row (dist_action = 2) ─┤ same ident_to_index + │ + findContentToReindex ──────────────┘ + │ highest id wins + ▼ + ┌─────────────┐ + isDelete ────│ batch entry │──── !isDelete + │ └─────────────┘ │ + ▼ ▼ + appendBulkRemoveRequest* appendBulkRequest* + │ │ + └──────────► bulk to index ◄───────┘ + │ + ack ──────────┴────────── failure + │ │ + deleteReindexEntry(row) markAsFailed(row) + UPDATE priority/index_val + dist_action preserved → retried as a delete +``` + +The failure edge is what the feature buys: the row survives, keeps its `dist_action`, and is +retried. That is the property the in-memory commit listener never had. diff --git a/specs/37276-silent-index-delete-loss/release-note.md b/specs/37276-silent-index-delete-loss/release-note.md new file mode 100644 index 000000000000..6e8597d0941f --- /dev/null +++ b/specs/37276-silent-index-delete-loss/release-note.md @@ -0,0 +1,75 @@ +# Release note — silent index delete loss (#37276) + +## What changed + +Content deletion now records its index removal durably. Previously the removal was handed to an +in-memory post-commit task with no record of the pending work: if that task was lost — the JVM +stopped between commit and execution, the shared pool rejected it, or the bulk write came back +with per-item failures that were logged and treated as success — the index kept a document whose +content no longer existed, and nothing ever retried. + +Deletions are now journalled in `dist_reindex_journal` inside the same transaction that deletes +the rows, and retried by `ReindexThread` until the index acknowledges them — the same guarantee +content additions already had. + +## What operators will notice + +**New errors on index writes that used to be silent.** A bulk write that comes back with per-item +failures (a saturated write queue, an unavailable shard, a version conflict) now raises instead of +being logged and reported as success. + +**This is not a regression.** Those failures were always happening; only their visibility changed. +An environment that has been quietly losing index writes will start surfacing them at the moment +of the write rather than as unexplained index drift weeks later. Treat a new `putToIndex` failure +as a pre-existing condition now made visible — most often index write-queue saturation — and +investigate the index cluster, not this release. + +**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. + +## Existing orphaned documents are not repaired + +This change prevents new divergence. It does not clean up documents already orphaned in an index. +A full reindex remains the remedy for existing drift. + +Because the index document `_id` maps one-to-one onto `contentlet_version_info`'s primary key, +the current orphan count can be measured directly: compare the live-index document count against + +```sql +SELECT count(*) FROM contentlet_version_info WHERE live_inode IS NOT NULL; +``` + +## Removals that exhaust their retries + +A removal that fails `REINDEX_MAX_FAILURE_ATTEMPTS` times is parked in `dist_reindex_journal` +above `ERROR` priority, where the drain query no longer reaches it. It is not retried further and +not deleted — deliberately, so the residue stays enumerable: + +```sql +SELECT ident_to_index, priority, index_val +FROM dist_reindex_journal +WHERE dist_action = 2 AND priority > 400; +``` + +Rows returned by that query are removals the index still owes. A non-empty result means something +is persistently rejecting index writes and warrants investigation; the `index_val` column carries +the last failure cause. + +## Compatibility + +No database schema change, no index mapping change, no REST contract change. `dist_reindex_journal` +already carried a delete action type and both search providers already consumed it, so a +mixed-version cluster during a rolling deploy sees nothing unfamiliar. **Rollback-safe.** + +`putToIndex` is on the public `ContentletIndexAPI` interface. Its signature is unchanged and the +change is source- and binary-compatible, but an out-of-tree plugin that relied on it returning +normally after a partial failure will now see an exception. + +## Paths deliberately unchanged + +Unpublish and archive keep the existing deferred-removal path. A journal entry is +identifier-wide, and a removal there is per language — reusing the mechanism would drop languages +that are still live. The same reasoning excludes +`ContentletAPI#delete(List, User, boolean, boolean)`, which can delete a subset of an +identifier's versions.