diff --git a/.github/workflows/issue_comp_link-issue-to-pr.yml b/.github/workflows/issue_comp_link-issue-to-pr.yml index b53eb27c788..b93c4d3fc96 100644 --- a/.github/workflows/issue_comp_link-issue-to-pr.yml +++ b/.github/workflows/issue_comp_link-issue-to-pr.yml @@ -498,8 +498,45 @@ jobs: current_body=$(echo "$current_pr" | jq -r '.body // ""') - # Check if issue reference already exists (word-boundary match so #10 does not match #100) - if ! echo "$current_body" | grep -qE "(^|[^0-9])#${issue_number}([^0-9]|\$)"; then + # Decide whether the PR already *closes* this issue, not merely whether the + # number appears somewhere. The previous bare "#N" test treated an incidental + # prose mention ("reproducing QA's report on #36222") as an existing link and + # skipped the patch below, so the PR carried no closing reference at all and + # the issue would not close on merge (#36933). + already_linked=false + + # GitHub's own parser is authoritative and also catches markdown-linked refs + # ("fixes [#123](url)") that a regex misses. + owner="${GITHUB_REPOSITORY%/*}" + repo="${GITHUB_REPOSITORY#*/}" + gh_err=$(mktemp) + if closing_issues=$(gh api graphql \ + -F owner="$owner" \ + -F name="$repo" \ + -F num="$pr_number" \ + -f query='query($owner:String!,$name:String!,$num:Int!){repository(owner:$owner,name:$name){pullRequest(number:$num){closingIssuesReferences(first:20){nodes{number}}}}}' \ + --jq '.data.repository.pullRequest.closingIssuesReferences.nodes[].number' 2>"$gh_err"); then + if echo "$closing_issues" | grep -qx "$issue_number"; then + already_linked=true + fi + else + echo "::warning::closingIssuesReferences lookup failed; relying on keyword matching alone:" + cat "$gh_err" + fi + rm -f "$gh_err" + + # Keyword match as well as — not instead of — the API: it covers the window + # right after our own PATCH, where GraphQL may not yet report the new + # reference and a second append would duplicate the line. Accepts the same + # forms as the link detection above: "#N", "owner/repo#N", full issue URL. + # Word-boundary on the number so #10 does not match #100. + closing_kw='(close[ds]?|fix(e[ds])?|resolve[ds]?)(:)?[[:space:]]+' + if echo "$current_body" | grep -qiE "${closing_kw}([a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+)?#${issue_number}([^0-9]|\$)" \ + || echo "$current_body" | grep -qiE "${closing_kw}https://github\.com/[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+/issues/${issue_number}([^0-9]|\$)"; then + already_linked=true + fi + + if [[ "$already_linked" != 'true' ]]; then # Add issue reference to PR body if [[ -n "$current_body" && "$current_body" != "null" ]]; then new_body=$(printf "%s\n\nThis PR fixes: #%s" "$current_body" "$issue_number") @@ -517,7 +554,7 @@ jobs: echo "Added issue #$issue_number reference to PR #$pr_number body" else - echo "Issue #$issue_number already referenced in PR #$pr_number body" + echo "Issue #$issue_number is already a closing reference on PR #$pr_number" fi - name: Add failure comment to PR 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 4e50eafb3ff..c2cd213f492 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 @@ -25,6 +25,7 @@ import com.dotcms.content.index.IndexAPI; import com.dotcms.content.index.IndexAPIImpl; import com.dotcms.content.index.IndexConfigHelper.MigrationPhase; +import com.dotcms.content.index.MigrationHaltReport; import com.dotcms.content.index.opensearch.IndexStartupValidator; import com.dotcms.content.index.opensearch.OSIndexAPIImpl; import com.dotcms.content.index.opensearch.OSIndexAPIImpl.ConnectionFailureKind; @@ -1175,6 +1176,13 @@ boolean handleOsBootstrapFailure(final String workingName, final String liveName + " Any OS index created before the failure is left in the cluster and is reused or" + " repaired by the next bootstrap; it is never registered in the OS index store." + " Fix the cause above and re-enable the migration phase when ready.", e); + + // Keep the classified cause where callers outside the index layer can read it: the log line + // above is for support, this is what the operator who triggered the operation gets told. + MigrationHaltReport.record(new MigrationHaltReport(phase.name(), kind.name(), + kind.remediation(), + operationsOS.toPhysicalName(workingName) + ", " + + operationsOS.toPhysicalName(liveName))); haltMigration(); // haltMigration() writes the phase through Config, which the DB-backed ConfigSystemTable @@ -1341,28 +1349,62 @@ public boolean reindexSwitchover(boolean forceSwitch) throws DotDataException { @WrapInTransaction public synchronized IndexStartResult fullReindexStart() throws DotDataException { if (indexReady() && !isInFullReindex()) { - final User currentUser = Try.of( - () -> PortalUtil.getUser(HttpServletRequestThreadLocal.INSTANCE.getRequest())) - .getOrNull(); - if (currentUser != null) { - Logger.info(this, "Full reindex started by user: " - + currentUser.getUserId() + " (" + currentUser.getEmailAddress() - + ") at " + new java.util.Date()); - } else { - Logger.info(this, "Full reindex started by system user at " + new java.util.Date()); - } + return startFullReindex(); + } - final String ts = ContentletIndexAPI.threadSafeTimestampFormatter - .format(LocalDateTime.now()); - initAndPointReindex(ts); + final boolean migrationWasRunning = isMigrationStarted(); + final IndexStartResult bootstrapResult = initIndex(); - return ImmutableIndexStartResult.builder() - .indexSuffixES(ts) - .indexSuffixOS(isMigrationNotStarted() ? "" : ts) - .build(); + // A dual-phase OpenSearch rejection makes indexReady() false — an index the OS user may not + // even probe reads as missing — so the branch above is skipped and the caller silently gets + // a bootstrap instead of the full reindex it asked for. initIndex() absorbs that rejection + // and halts the migration (handleOsBootstrapFailure), and once dotCMS is ES-only the reindex + // is possible again: run it rather than return a downgraded no-op (issue #36222). + // + // Gated on the halt, not on a bare indexReady() re-check: on a fresh install initIndex() + // legitimately turns indexReady() true by creating the indices, and reindexing brand-new + // empty indices is not this method's contract. + if (migrationWasRunning && !isMigrationStarted() && indexReady() && !isInFullReindex()) { + Logger.warn(this, "The OpenSearch migration was halted while preparing the full reindex." + + " Continuing on Elasticsearch only — see the ERROR above for the cause." + + MigrationHaltReport.last() + .map(report -> " " + report.operatorMessage()).orElse("")); + return startFullReindex(); + } + + return bootstrapResult; + } + + /** + * Creates the reindex slots for every applicable provider and reports the timestamp they share. + * + *

Extracted from {@link #fullReindexStart()} so that the same work runs both on the happy + * path and on the retry that follows an absorbed OpenSearch failure (issue #36222). Callers must + * have checked {@link #indexReady()} and {@link #isInFullReindex()} first.

+ * + * @return the ES suffix always, and the OS suffix only while the migration is running + * @throws DotDataException on persistence or creation failure + */ + private IndexStartResult startFullReindex() throws DotDataException { + final User currentUser = Try.of( + () -> PortalUtil.getUser(HttpServletRequestThreadLocal.INSTANCE.getRequest())) + .getOrNull(); + if (currentUser != null) { + Logger.info(this, "Full reindex started by user: " + + currentUser.getUserId() + " (" + currentUser.getEmailAddress() + + ") at " + new java.util.Date()); } else { - return initIndex(); + Logger.info(this, "Full reindex started by system user at " + new java.util.Date()); } + + final String ts = ContentletIndexAPI.threadSafeTimestampFormatter + .format(LocalDateTime.now()); + initAndPointReindex(ts); + + return ImmutableIndexStartResult.builder() + .indexSuffixES(ts) + .indexSuffixOS(isMigrationNotStarted() ? "" : ts) + .build(); } /** diff --git a/dotCMS/src/main/java/com/dotcms/content/index/MigrationHaltReport.java b/dotCMS/src/main/java/com/dotcms/content/index/MigrationHaltReport.java new file mode 100644 index 00000000000..a0953d7c129 --- /dev/null +++ b/dotCMS/src/main/java/com/dotcms/content/index/MigrationHaltReport.java @@ -0,0 +1,62 @@ +package com.dotcms.content.index; + +import java.util.Optional; +import java.util.concurrent.atomic.AtomicReference; + +/** + * Why the ES → OpenSearch migration last switched itself off. + * + *

When OpenSearch rejects an index operation while it is only the shadow store, the failure is + * absorbed and the migration is halted so that dotCMS keeps serving from Elasticsearch + * (issue #36222). That policy is right, but on its own it is silent: the reason lives in the log and + * nothing reaches the operator who pressed the button. This carrier keeps the classified cause and + * its remediation so callers outside the index layer — the reindex REST resource, which carries no + * vendor imports — can report why the migration stopped, not merely that it did.

+ * + *

Scope: the last report is held per JVM, deliberately matching the scope of the + * halt it describes: {@link IndexConfigHelper.MigrationPhase#reset()} is a runtime-only change, so a + * restart clears both the halted phase and the reason for it. It is not persisted and not + * cluster-wide — each node reports the halt it absorbed itself.

+ * + * @param haltedPhase the phase that was active when the failure was absorbed + * @param cause classified failure kind, e.g. {@code AUTH_FORBIDDEN} + * @param remediation operator-facing description of how to fix that kind of failure + * @param indexNames the physical OpenSearch index names the operation was rejected for + */ +public record MigrationHaltReport(String haltedPhase, String cause, String remediation, + String indexNames) { + + private static final AtomicReference LAST = new AtomicReference<>(); + + /** + * Records {@code report} as the most recent halt, replacing any previous one. + * + * @param report the halt to remember + */ + public static void record(final MigrationHaltReport report) { + LAST.set(report); + } + + /** + * Returns the most recent halt absorbed by this JVM, or empty when the migration has not been + * halted since start-up. + * + * @return the last halt report, if any + */ + public static Optional last() { + return Optional.ofNullable(LAST.get()); + } + + /** + * Returns a single sentence an operator can act on: what happened, that Elasticsearch is still + * serving, why it happened, and what to do about it. + * + * @return the operator-facing message + */ + public String operatorMessage() { + return "The OpenSearch migration was switched off (was " + haltedPhase + ") and dotCMS is" + + " serving from Elasticsearch only. Cause: " + cause + " (" + remediation + ")." + + " Rejected index names: " + indexNames + "." + + " Fix the cause and re-enable the migration phase when ready."; + } +} diff --git a/dotCMS/src/main/java/com/dotcms/rest/api/v1/index/ESIndexResource.java b/dotCMS/src/main/java/com/dotcms/rest/api/v1/index/ESIndexResource.java index c9bf0b08db8..6441ef1df9c 100644 --- a/dotCMS/src/main/java/com/dotcms/rest/api/v1/index/ESIndexResource.java +++ b/dotCMS/src/main/java/com/dotcms/rest/api/v1/index/ESIndexResource.java @@ -8,6 +8,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Optional; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.ws.rs.DELETE; @@ -36,6 +37,8 @@ import com.dotcms.content.elasticsearch.business.IndexType; import com.dotcms.content.elasticsearch.business.IndiciesAPI; import com.dotcms.content.index.IndexAPI; +import com.dotcms.content.index.IndexConfigHelper.MigrationPhase; +import com.dotcms.content.index.MigrationHaltReport; import com.dotcms.content.index.domain.NodeStats; import com.dotcms.content.elasticsearch.util.ESReindexationProcessStatus; import com.dotcms.contenttype.model.type.ContentType; @@ -43,6 +46,7 @@ import com.dotcms.rest.ErrorEntity; import com.dotcms.rest.InitDataObject; import com.dotcms.rest.ResourceResponse; +import com.dotcms.rest.MessageEntity; import com.dotcms.rest.ResponseEntityView; import com.dotcms.rest.WebResource; import com.dotcms.rest.annotation.NoCache; @@ -341,6 +345,10 @@ public Response startReindex(@Context final HttpServletRequest request, @Context System.setProperty("es.index.number_of_shards", String.valueOf(shards)); Logger.info(this, "Running Contentlet Reindex"); + // Captured before the call: a shadow-phase OpenSearch failure is absorbed deep in the index + // layer, which halts the migration to ES-only without any of it reaching this response. + final boolean migrationWasRunning = !MigrationPhase.current().isMigrationNotStarted(); + if(!DOTALL.equals(contentType)) { ContentType type = APILocator.getContentTypeAPI(APILocator.systemUser()).find(contentType); Logger.info(this.getClass(), "Starting reindex of " + type.name()); @@ -351,9 +359,39 @@ public Response startReindex(@Context final HttpServletRequest request, @Context APILocator.getContentletAPI().refreshAllContent(); } + final Optional haltMessage = migrationHaltMessage(migrationWasRunning); + if (haltMessage.isPresent()) { + Logger.warn(this.getClass(), haltMessage.get()); + return Response.ok(new ResponseEntityView<>( + ESReindexationProcessStatus.getProcessIndexationMap(), + List.of(new MessageEntity(haltMessage.get())))).build(); + } + return getReindexationProgress(request, response); } + + /** + * Returns what to tell the operator when the OpenSearch migration switched itself off while this + * request was running, or empty when it did not (issue #36222). + * + *

The reindex itself succeeds on Elasticsearch — that degradation is deliberate — but it must + * not be silent: without this the response is indistinguishable from a run where nothing went + * wrong, and the only trace is a log line nobody is watching.

+ * + * @param migrationWasRunning whether the migration was active before the reindex was triggered + * @return the operator-facing message, if the migration was halted during this request + */ + private static Optional migrationHaltMessage(final boolean migrationWasRunning) { + if (!migrationWasRunning || !MigrationPhase.current().isMigrationNotStarted()) { + return Optional.empty(); + } + return Optional.of(MigrationHaltReport.last() + .map(MigrationHaltReport::operatorMessage) + .orElse("The OpenSearch migration was switched off while this reindex was starting" + + " and dotCMS is serving from Elasticsearch only. See the log for the" + + " cause, and re-enable the migration phase once it is fixed.")); + } @CloseDBIfOpened @DELETE diff --git a/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/OsBootstrapForbiddenIndexTest.java b/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/OsBootstrapForbiddenIndexTest.java index 737ba404d5e..447622c3d5e 100644 --- a/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/OsBootstrapForbiddenIndexTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/OsBootstrapForbiddenIndexTest.java @@ -16,14 +16,19 @@ import com.dotcms.content.index.IndexAPI; import com.dotcms.content.index.IndexAPIImpl; import com.dotcms.content.index.IndexConfigHelper.MigrationPhase; +import com.dotcms.content.index.MigrationHaltReport; import com.dotcms.content.index.VersionedIndices; +import com.dotcms.content.index.VersionedIndicesImpl; +import com.dotcms.content.index.domain.IndexStartResult; import com.dotcms.content.index.opensearch.IndexStartupValidator; +import com.dotcms.content.index.opensearch.OSIndexAPIImpl.ConnectionFailureKind; import com.dotcms.util.IntegrationTestInitService; import com.dotmarketing.business.APILocator; import com.dotmarketing.business.DotStateException; import com.dotmarketing.exception.DotDataException; import com.dotmarketing.util.Config; import com.dotmarketing.util.Logger; +import com.dotmarketing.util.UtilMethods; import io.vavr.control.Try; import java.util.Optional; import javax.enterprise.context.ApplicationScoped; @@ -209,6 +214,79 @@ public void phase1_forbiddenOsCreate_doesNotRegisterOsReindexSlots() throws DotD } } + /** + * Given : Phase 1, a full reindex requested through {@code fullReindexStart()}, and an OS store + * pointing at indices the restricted user may not even probe — so {@code indexReady()} is + * false and the reindex branch is skipped. + * When : the reindex is triggered. + * Then : the OS failure is absorbed, the migration is halted, and the full reindex the caller + * asked for still runs on Elasticsearch — new reindex slots and a real ES suffix. + * + *

Before the fix this returned a bootstrap result with an empty ES suffix and created no index + * at all: the UI showed a progress bar that completed, no error, and nothing had happened. The + * operator had to press Reindex a second time — which worked only because the first press had + * silently switched the migration off (issue #36222, QA round 3).

+ */ + @Test + public void phase1_forbiddenOsCreate_stillRunsTheFullReindexOnElasticsearch() + throws DotDataException { + + setPhase(MigrationPhase.PHASE_1_DUAL_WRITE_ES_READS); + warmUpOsClient(); + Config.setProperty(OS_ENDPOINTS_KEY, new String[]{UNUSED_OS_ENDPOINT}); + + final IndiciesInfo originalEsIndices = APILocator.getIndiciesAPI().loadIndicies(); + final Optional originalOsStore = + APILocator.getVersionedIndicesAPI().loadDefaultVersionedIndices(); + final String missingSuffix = "os_missing_" + System.currentTimeMillis(); + + try { + // An OS store that points at indices which do not exist makes indexReadyOS() false, which + // is exactly what a 403 on the exists-probe produces: unreadable is indistinguishable from + // absent (OSIndexAPIImpl.indexExists swallows and returns false). + APILocator.getVersionedIndicesAPI().saveIndices(VersionedIndicesImpl.builder() + .working(IndexType.WORKING.getPrefix() + "_" + missingSuffix) + .live(IndexType.LIVE.getPrefix() + "_" + missingSuffix) + .version(VersionedIndices.OPENSEARCH_3X) + .build()); + + final IndexStartResult result = newApiWithForbiddenOs().fullReindexStart(); + + assertTrue("The full reindex must still run on Elasticsearch: an empty ES suffix means" + + " the request was silently downgraded to a bootstrap that did nothing", + UtilMethods.isSet(result.indexSuffixES())); + assertEquals("With the migration halted there is no OpenSearch slot to advertise", + "", result.indexSuffixOS()); + + final IndiciesInfo esAfter = APILocator.getIndiciesAPI().loadIndicies(); + assertTrue("The ES reindex slots must be registered for the suffix the reindex reported." + + " Got: " + esAfter.getReindexWorking(), + esAfter.getReindexWorking() != null + && esAfter.getReindexWorking().contains(result.indexSuffixES())); + + assertEquals("The shadow-phase OS failure must still halt the migration (ES-only)", + MigrationPhase.PHASE_0_MIGRATION_NOT_STARTED, MigrationPhase.current()); + + final Optional halt = MigrationHaltReport.last(); + assertTrue("The halt must leave a reason behind: without it the REST layer can say that" + + " the migration stopped but never why", halt.isPresent()); + assertEquals("A 403 on create must be classified as an authorization problem, so the" + + " operator is told to fix the role instead of reindexing again", + ConnectionFailureKind.AUTH_FORBIDDEN.name(), halt.get().cause()); + + Logger.info(this, "✅ Phase 1: forbidden OS create still produced an ES full reindex (" + + result.indexSuffixES() + ") and reported " + halt.get().cause()); + } finally { + Try.run(() -> APILocator.getIndiciesAPI().point(originalEsIndices)); + Try.run(() -> APILocator.getContentletIndexAPI() + .delete(IndexType.REINDEX_WORKING.getPrefix() + "_" + missingSuffix)); + if (originalOsStore.isPresent()) { + Try.run(() -> APILocator.getVersionedIndicesAPI() + .saveIndices(originalOsStore.get())); + } + } + } + // ── helpers ─────────────────────────────────────────────────────────────── /**