Spec: silent index delete loss (#37276)

[fabrizzio-dotCMS]

Defect spec for #37276. Implementation follows in PR 2.

This PR fixes: #37276

[claude]

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

---

Code review — PR #37297 (silent index delete loss)

I reviewed the implementation diff (ContentletIndexAPIImpl, ContentletIndexOperationsES/OS, ESContentletAPIImpl, ReindexQueueFactory, IndexPolicyProvider) against origin/main. The design is sound and the durability approach matches the spec. The reviewer-raised gaps (journal collision, provider skip, partial-failure surfacing) are all addressed. A few things worth a second look before merge — none blocking.

New Issues

• 🟡 Medium: ReindexQueueFactory.java:303-305 — the merge(...) fix only guarantees AC-008 ("the older is not applied afterwards") within a single batch. The loser entry is poll()ed off the in-memory queue but never added to contentToIndex, so it is neither applied nor deleted from dist_reindex_journal (only contentToIndex values reach BulkProcessorListener.deleteReindexEntry(successful)). It also won't be re-read until lastIdIndexed resets to 0 when the queue empties (:355, :359-361). On that later pass the winning DELETE row has already been processed and removed, so the stale REINDEX loser is loaded alone and applied.

  • Assumption: For a destroyed identifier this is benign — reindexing an identifier with no DB rows writes no document, so it cannot re-add the orphan. The DELETE always wins for destroy (content is gone, nothing enqueues a higher-id REINDEX for that identifier).
  • What to verify: Confirm no in-scope flow enqueues a REINDEX with a higher id than the DELETE for the same identifier (which would make REINDEX the winner and re-add), and confirm the persisted loser row does not accumulate indefinitely / churn the journal across reset cycles. If the intent of AC-008 is a hard cross-batch guarantee, the loser needs to be deleted or superseded, not just out-voted in the map.

• 🟡 Medium: ContentletIndexAPIImpl.java:3137 — deleteProviders.get(0) assumes router.writeProviders() is never empty. The prior for (ops : writeProviders()) loop degraded to a silent no-op on an empty list; now an empty list throws IndexOutOfBoundsException (an opaque failure, not the descriptive "removal was NOT performed" error). Assumption: phases 0–3 always yield ≥1 provider, so this is unreachable in normal operation. What to verify: that no phase-transition/misconfiguration window can return an empty provider list. If unsure, guard with an explicit empty-check that throws the descriptive message. Fix this →

• 🟡 Medium: ContentletIndexAPIImpl.java:3186 / ESContentletAPIImpl.java:3668 — the new throw-on-empty-primary and the putToIndex partial-failure throw also fire on the unpublish / onlyLive=true path (removeContentFromLiveIndex → removeContentFromIndex(content, true) → removeContentAndProcessDependencies), which the spec explicitly lists as out-of-scope and which journalContentDeletes deliberately does not journal. So a partial/empty failure on that path now throws where it previously continued silently, but with no durable journal record to retry it. This is likely an acceptable/intended visibility improvement, but confirm it doesn't turn a previously-silent unpublish into a hard failure without any recovery path.

Notes (non-blocking)

• ContentletIndexOperationsES.java:207 / ContentletIndexOperationsOS.java — handleBulkResponse throws DotRuntimeException, which is then caught by the surrounding catch (final Exception e) in putToIndex and re-wrapped/re-thrown. Correct end result (the failure still propagates), but the failure is logged twice (Logger.error in handleBulkResponse, then Logger.warnAndDebug in the catch). Minor log noise, not a correctness issue.
• ContentletIndexOperationsOS.handleBulkResponse — if response.errors() is true but no item carries a non-null error(), detail stays empty and it throws DotRuntimeException(""). Edge case; a fallback message would read better in logs.

Resolved (reviewer-raised gaps)

• ✅ ReindexQueueFactory.java:303 — the identifier-keyed HashMap collision (nollymar / fabrizzio) is now resolved deterministically by row id rather than poll order.
• ✅ ContentletIndexAPIImpl.java:3186 — skipped/empty primary provider (loss point L2) now surfaces instead of reading as a completed removal.
• ✅ ContentletIndexOperationsES/OS.handleBulkResponse — per-item bulk failures (loss point L3) now raise to the caller with delete-flavoured wording ("index operation(s)" not "reindexing").
• ✅ ESContentletAPIImpl.java:3668 — deleteVersionByInode-style subset deletes correctly left un-journalled (identifier-wide entry would over-remove languages), with a clear comment.

Tests cover the partial-failure verdict (ES/OS unit tests), the batch-key resolution, and the destroy/skip/phase integration paths — coverage looks appropriate for the behavior added.

---

• Gather context
• Review production code
• Review test coverage
• Check reviewer-raised gaps
• Post review
  · branch 37276-silent-index-delete-loss

[ihoffmann-dot]

Three gaps I found:

• Secondary dedupe-less site at ESContentletAPIImpl.java:3549-3554 (same L1/L2/L3 shape, mentioned in the original issue) isn't addressed as in-scope or out-of-scope here. Please make a call either way.

• F4 (reconciliation check in the Maintenance portlet) from the issue is also silently dropped, not in scope, not in non-goals. Was that intentional?

• AC-003's "enumerate callers that tolerate partial index failures" is called a prerequisite but has no AC or verification step backing it. As written it can slip through planning unnoticed, needs its own AC or explicit task.

[fabrizzio-dotCMS]

All four gaps from review are closed — three in the spec (945d27e), one in code plus spec.

@ihoffmann-dot, your three:

• The :3549-3554 site. Call made, both ways: deleteAllVersionsandBackup is journalled (its version list comes from findAllVersions(identifier), so an identifier-wide entry is correct). Implementing it turned up a third site the issue never mentioned — ContentletAPI#delete(List, User, boolean, boolean) — which is now an explicit non-goal, because it can delete a subset of an identifier's versions and an identifier-wide entry would drop languages that still exist. Both are marked in the code so neither gets "completed" by mistake later.
• F4. Fair hit — I dropped it silently. Now a stated non-goal: AC-007 already makes the residue enumerable with one query, which is the data that portlet view would render, so the view itself is separate work and does not gate this fix.
• AC-003's caller enumeration. You were right that prose in Regression Risk was not enough to survive planning. It is now AC-009, stated as a prerequisite for AC-003 rather than a follow-up.

The claude[bot] review also found something sharper than a spec gap, and it turned out to be real: from Phase 2 onwards PhaseRouter#readProvider serves reads from OpenSearch while writes stay ES-primary / OS-shadow — so a removal lost on the OS leg orphaned a document in the index being queried. That is this defect, in the phase the migration spends the longest in, and neither the durable journal nor the partial-failure escalation covered it, because the shadow listener acked the journal entry on the ES result alone.

Fixed in #37333 by scoping the shadow treatment to who serves reads rather than to dual-write, with AC-010 added here. ADR-0009's intent is preserved — Phase 1 still swallows, and there is a test pinning that so the fix cannot over-reach.

[fabrizzio-dotCMS]

Closing out the two 🟡 findings from the automated review — both were real and both are fixed.

1. Blank exception message in ContentletIndexOperationsOS.handleBulkResponse. Fixed in #37333. The invariant you flagged (errors() implies at least one item with a cause) is almost certainly true, but I did not want to rely on it: a failure with no message is barely better than the silent return it replaced, so there is now a fallback message when no item carried a cause.

2. Phase 2 read/shadow gap. Confirmed, and it was the sharpest thing in either review. PhaseRouter#readProvider:144 serves reads from OpenSearch from Phase 2 onwards while writes stay ES-primary / OS-shadow, so a removal lost on the OS leg orphaned a document in the index actually answering queries — #37276 itself, in the phase the migration spends the longest in.

You suggested it might be intentionally out of scope under ADR-0009 and that a non-goal note would close the gap. I went the other way and fixed it, because the ADR's rationale is "a store nobody reads from must not break a user operation" — and in Phase 2 that premise no longer holds. Scoping the shadow treatment by isReadEnabled() rather than by dual-write preserves the intent and corrects only the stale assumption.

Worth noting the async path mattered more than the sync one you pointed at: createBulkProcessor built a shadow BulkProcessorListener for OS in Phases 1 and 2, and 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. The durable-delete fix would not have covered Phase 2 without this.

Fix in #37333 with ContentletIndexAPIImplPhase2ReadDurabilityTest, including a case pinning that Phase 1 still swallows so the change cannot over-reach. Spec updated with AC-010; the contract's per-phase table and the release note were both stale and are corrected there too.