[sharathkanaka]

Description

Why: This is part of supporting system index snapshot restore for alerting, and the third of three alerting PRs for it:

1. Fix doc-level monitors resuming after a restore Reset stale last run context for doc-level monitors after restore #2287 .
2. Fix doc-level monitors failing when their saved query index doesn't exist after a restore Use the current query index for doc-level monitors after restore #2309 .
3. This PR: fix alert, finding and comment history writes failing after the history indices are restored.

Problem: Alerting writes alert, finding and comment history through a write alias that names one index. If the new cluster runs a monitor before the history indices are restored, it creates its own history index with that alias. Restoring the old cluster's history then brings the alias back on the restored index too, because a restore keeps aliases. The alias now names two indices and neither is marked as the write index, so every write through it fails: findings and completed alerts are not saved, comments fail, and rollover fails. Monitors keep running, so nothing shows the problem.

Fix: Alerting checks each history write alias in two places. Right after a restore, through onRestore from core's RestoreListenerPlugin (opensearch-project/OpenSearch#23295), it checks the aliases on indices the restore brought back. On every history rollover run (every rollover period, 12 hours by default) it checks them again, so an alias the first check could not fix gets fixed later.

When an alias names more than one index and none has is_write_index: true, alerting keeps the alias only on the index with the highest rollover counter (the restored ...-000002 over the new ...-1), so the next rollover creates ...-000003, a name no index has yet. An index with is_write_index: false never keeps the alias, because someone marked it as not to be written to. If every index has false, alerting changes nothing and logs an error. The other indices stay searchable, because alerting searches history by index pattern. The repair runs as alerting's plugin identity.

Limits:

• Aliases named by a monitor's custom data sources are not repaired.
• If a restored history index has the same name as one this cluster already created (for example, both created on the same day), the restore itself is rejected.
• A history index restored with rename_pattern isn't repaired, because alerting can't change an index outside its own patterns.
• If the index skipped for is_write_index: false has the higher rollover counter and the same date as the index that keeps the alias, rollover fails that day, because the next name it picks already exists. It works again once the date changes.
• Restoring with include_aliases: false avoids the problem without this fix.

Tests: 18 unit tests in WriteAliasRepairTests for choosing the index that keeps the alias, the is_write_index cases, an alias on one index, an alias outside the repair list, the restore and rollover entry points, and the logged result (including a request that isn't acknowledged). 3 integration tests in AlertIndicesIT: a restore of all three history indices onto a cluster that has its own, after which each alias names only the restored index and a rollover creates ...-000003; a restored index with is_write_index: false, after which this cluster's own index keeps the alias and writes through it work; and an alias broken without a restore, which the rollover run repairs. The restore tests skip on a cluster with no path.repo.

Related Issues

Resolves #2313

Check List

• New functionality includes testing.
• New functionality has been documented.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit bfffd69)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Blocking on cluster-state thread rolloverAndDeleteAlertHistoryIndices and rolloverAndDeleteFindingHistoryIndices now call repairWriteAliases, which uses runBlocking to synchronously wait for an IndicesAliasesRequest to complete. These rollover methods are invoked from the cluster state listener path (clusterChanged -> scheduled rollover). Blocking a scheduler/cluster-state thread on a cluster-manager admin call can cause thread starvation or deadlocks if the admin call ends up requiring the same thread, and goes against OpenSearch's guidance against blocking cluster-state threads. Consider dispatching the repair asynchronously or verifying the invoking thread pool is safe to block. /** Repairs each alias in [aliases] that names more than one index, so a repair the restore missed happens here. Blocks until done. */ private fun repairWriteAliases(vararg aliases: String) { val repair = writeAliasRepair ?: return runBlocking { aliases.forEach { repair.repair(it) } } } private fun rolloverAndDeleteAlertHistoryIndices() { repairWriteAliases(ALERT_HISTORY_WRITE_INDEX) if (alertHistoryEnabled) rolloverAlertHistoryIndex() deleteOldIndices("History", ALERT_HISTORY_ALL) } private fun rolloverAndDeleteFindingHistoryIndices() { repairWriteAliases(FINDING_HISTORY_WRITE_INDEX) if (findingHistoryEnabled) rolloverFindingHistoryIndex() deleteOldIndices("Finding", FINDING_HISTORY_ALL) Rollover counter parsing edge case rolloverCounter uses substringAfterLast('-') and toLongOrNull(). For an index name without any - (e.g. comments), substringAfterLast('-') returns the entire string, which may parse as a Long if the whole name happens to be numeric (e.g. a user-created index named 12345), unexpectedly beating properly-suffixed rollover indices. The intent per the comment is "-1 when there is none"; consider explicitly checking for the presence of - before parsing. private fun rolloverCounter(index: String): Long = index.substringAfterLast('-').toLongOrNull() ?: -1

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to bfffd69

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Guard against uninitialized lateinit field writeAliasRepair is declared lateinit and only initialized inside createComponents. If onRestore is ever invoked before createComponents completes (e.g. during early node startup or in tests), this will throw UninitializedPropertyAccessException and the restore listener will log a confusing error. Guard against this by checking initialization and skipping the repair silently when not ready. alerting/src/main/kotlin/org/opensearch/alerting/AlertingPlugin.kt [449] // Core calls this on its generic thread pool and logs anything it throws, so the repair can block that thread until it is done -override fun onRestore(context: RestoreContext) = runBlocking { writeAliasRepair.repair(context) } +override fun onRestore(context: RestoreContext) { + if (!::writeAliasRepair.isInitialized) return + runBlocking { writeAliasRepair.repair(context) } +} Suggestion importance[1-10]: 5 __ Why: Reasonable defensive check against UninitializedPropertyAccessException if onRestore runs before createComponents, though this scenario is unlikely in normal plugin lifecycle.	Low
	Clarify misleading warning log message The log message says "none is the write index", but repairAlias can also be reached when one index does have is_write_index=true — wait, actually indicesToDetach returns empty in that case, so this branch is unreachable. However, the message is still misleading because it is emitted whenever more than one index exists without a true write index, including the normal post-restore case. Consider clarifying this message to accurately describe the condition (e.g. mentioning that the restore or external action added the alias to another index). alerting/src/main/kotlin/org/opensearch/alerting/util/WriteAliasRepair.kt [87] val detach = indicesToDetach(aliasedIndices) if (detach.isEmpty()) return val keep = aliasedIndices.first { it.index !in detach }.index -log.warn("Write alias [$alias] names ${aliasedIndices.size} indices and none is the write index. Removing it from $detach.") +log.warn( + "Write alias [$alias] names ${aliasedIndices.size} indices (${aliasedIndices.map { it.index }.sorted()}) and no index " + + "is explicitly marked as the write index. Keeping [$keep] and removing the alias from $detach." +) Suggestion importance[1-10]: 3 __ Why: Minor log message clarification. The existing message is slightly misleading but not incorrect in practice; the improvement is cosmetic.	Low

---

Previous suggestions

Suggestions up to commit 2695db7

Category	Suggestion	Impact
General	Log uncaught coroutine exceptions The launched coroutine has no exception handler, so any unexpected throwable escapes into the uncaught handler and will not be logged with context. Add a CoroutineExceptionHandler (or wrap the block in try/catch) so unexpected failures during restore reconciliation are logged rather than silently swallowed by the SupervisorJob. alerting/src/main/kotlin/org/opensearch/alerting/restore/AlertingRestoreReconciler.kt [26-34] class AlertingRestoreReconciler(client: Client, clusterService: ClusterService) { - private val scope = CoroutineScope(SupervisorJob() + Dispatchers.IO) + private val log = org.apache.logging.log4j.LogManager.getLogger(AlertingRestoreReconciler::class.java) + private val errorHandler = kotlinx.coroutines.CoroutineExceptionHandler { _, e -> + log.error("Unexpected error during alerting restore reconciliation", e) + } + private val scope = CoroutineScope(SupervisorJob() + Dispatchers.IO + errorHandler) private val writeAliasRepair = WriteAliasRepair(client, clusterService) fun onRestore(context: RestoreContext) { scope.launch { writeAliasRepair.repair(context) } } } Suggestion importance[1-10]: 5 __ Why: WriteAliasRepair.repair already catches per-alias exceptions, but adding a top-level exception handler is still a reasonable safety net for logging unexpected failures.	Low
	Guard alias lookup against non-alias abstractions metadata.indicesLookup[alias] returns an IndexAbstraction whose type could be a concrete index when an index shares a name with the alias target. Accessing .indices is fine for aliases, but it.aliases[alias] can NPE if aliases is null for the index metadata entry. Guard against a null aliases map to avoid an NPE aborting the repair for this alias. alerting/src/main/kotlin/org/opensearch/alerting/util/WriteAliasRepair.kt [53-56] private suspend fun repairAlias(metadata: Metadata, restored: Set<String>, alias: String) { - val aliasedIndices = metadata.indicesLookup[alias]?.indices - ?.map { AliasedIndex(it.index.name, it.creationDate, it.aliases[alias]?.writeIndex()) } ?: return + val abstraction = metadata.indicesLookup[alias] ?: return + if (abstraction.type != org.opensearch.cluster.metadata.IndexAbstraction.Type.ALIAS) return + val aliasedIndices = abstraction.indices + .map { AliasedIndex(it.index.name, it.creationDate, it.aliases?.get(alias)?.writeIndex()) } if (aliasedIndices.none { it.index in restored }) return Suggestion importance[1-10]: 3 __ Why: The existing code already uses safe-call (?.) on indicesLookup[alias] and aliases[alias], and repair already catches exceptions per alias. The suggested extra guards are minor defensive improvements.	Low

Suggestions up to commit c24d036

Category	Suggestion	Impact
Possible issue	Fix counter parsing when delimiter missing substringAfterLast('-') returns the whole string when no - exists, so an all-numeric index name like "12345" would be parsed as a valid counter instead of returning -1. Guard against this by checking that the delimiter was actually present before parsing. alerting/src/main/kotlin/org/opensearch/alerting/util/WriteAliasRepair.kt [93] -private fun rolloverCounter(index: String): Long = index.substringAfterLast('-').toLongOrNull() ?: -1 +private fun rolloverCounter(index: String): Long { + val idx = index.lastIndexOf('-') + return if (idx < 0) -1 else index.substring(idx + 1).toLongOrNull() ?: -1 +} Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that substringAfterLast('-') returns the full string when no - is present, which could cause an all-numeric index name to parse as a valid counter instead of -1. However, alerting index names realistically always contain -, so the practical impact is minor.	Low

[riysaxen-amzn]

Reviewed c24d036 against core's merged RestoreListenerPlugin (OpenSearch#23295) and AlertIndices.kt at the base. The mechanism is right: core calls onRestore on the cluster manager after the cleanup state is published, so the metadata read in repair already has the restored aliases, and the repair is idempotent for the concurrent/duplicate calls the contract allows. Keeping the highest rollover counter is the correct pick since RolloverRequest(index, null) derives the next name from the current write index. Downstream code handles the choice well: retention protects the write index by alias presence (so the detached -1 ages out normally), and updateIndexMapping compares against lastUpdated*HistoryIndex, so the restored index gets put-mapped to the current mapping on the next initialize. Good side effect worth mentioning in the description.

Non-blocking:

1. Core's javadoc says delivery is best-effort (a cluster manager failover right after cleanup can drop the call) and that state which must be correct anyway needs a check where the plugin reads it. Today rolloverIndex.onFailure only logs "not roll over failed". Running the same indicesToDetach check there (same is_write_index guard) would make a missed notification self-heal at the next rollover period. Fine as a follow-up.
2. AlertingRestoreReconciler: core already forks each plugin call onto the generic pool, so the Dispatchers.IO scope is a second hop with a scope that is never cancelled, and the comment "so the restore is not held up" describes what core does, not the coroutine. "before any monitor runs" is also not guaranteed; a sweeper run can land in the window between restore completion and alias removal (transient, next run recovers). Either drop the scope or adjust the comment.
3. Limits to add to the description: (a) same-day same-name collision is a different failure, the restore itself is rejected, and only rename_pattern helps; (b) include_aliases: false on the restore avoids the problem with no code and is the only option before 3.10; (c) this needs core 3.10 (@ExperimentalApi, no backport labels on #23295), so it is not backportable; (d) a history index with a failed shard is left out of restoredIndices and is not repaired.
4. Related Issues is NA; please link one.

The bot's rolloverCounter suggestion cannot apply to these alias names; safe to ignore.

CI: the "Build Alerting" ubuntu failures are the same 52 unit tests in the same classes as main at 18be177, and the run completed 7 more tests with no new failures, so WriteAliasRepairTests passed. Every "Build and test", security-enabled and BWC lane died at Initialize containers on the ECR toomanyrequests quota, so AlertIndicesIT has not run anywhere yet. I have rerun the failed jobs on all four workflows; the security-enabled *IT lane is the one that exercises the plugin identity changing an alias on a system index, so please wait for that before merging.

Approving.

[sharathkanaka]

@riysaxen-amzn

1. Sure I can cover that gap when we get an exception , but I think the code will get complex. I will send a follow up PR.
2. Repair is a suspend function since it waits on the alias request, so it needs a coroutine. WIll update the javadoc to say that, and will also "before any monitor runs" since it is not guaranteed.
3. Will add limits in description.
4. Yeah sure will open a ISsue

[github-actions]

Persistent review updated to latest commit 2695db7

[lezzago]

I checked these against the PR head (2695db7) with integration tests, with and without the security plugin. Details and repro steps are inline.

[lezzago]

The repair runs once and nothing tries again. Core calls onRestore once, on the cluster-manager, after the restore's cleanup task is published. If this request fails, comes back with isAcknowledged == false (the response is thrown away here), or the cluster-manager changes before the callback runs, the alias stays on two indices for good. Rollover then keeps failing on it too.

I reproduced the failure case (see the security comment on line 64): the request was refused, the log shows a single attempt, and 20 s later the alias still named both indices and POST <alias>/_doc still returned 400.

Could the check also run from somewhere that repeats, such as AlertIndices/CommentsIndices onClusterManager or the periodic rollover? Checking isAcknowledged and logging a successful repair would also help operators: today they see only the "Removing it from [...]" warning.

[sharathkanaka]

Thanks.
I'll check isAcknowledged. If it's false, I'll re-read the alias right away rather than wait for the next rollover. If the alias names one index, I'll log success; if it still names two, I'll log an error.

Makes sense to have a fall back for rechecking. I'll also run the same repair from the history rollover in AlertIndices and CommentsIndices, taht way, repair that never ran or failed gets fixed on the next rollover.

[lezzago]

With the security plugin installed, the plugin identity can be refused this alias removal when the index to detach doesn't match alerting's system index patterns. That happens with rename_pattern, the workaround the PR description recommends.

Repro (-Dsecurity=true -Dhttps=true): snapshot .opendistro-alerting-alert-history-<today>-1, delete it, create the same -1 locally (newer creation date), then restore with "rename_pattern": "(.+)", "rename_replacement": "restored_$1". The counters tie, the newer local index wins, so the repair tries to remove the alias from restored_.opendistro-alerting-alert-history-<today>-1:

WARN  WriteAliasRepair Write alias [.opendistro-alerting-alert-history-write] names 2 indices and none is the write index. Removing it from [restored_.opendistro-alerting-alert-history-2026.10.29-1].
ERROR WriteAliasRepair Failed to repair write alias [.opendistro-alerting-alert-history-write] after restoring snapshot [...]
OpenSearchSecurityException[no permissions for [indices:admin/aliases] and User [name=plugin:org.opensearch.alerting.AlertingPlugin, backend_roles=[], requestedTenant=null]]

The alias stays on both indices and writes return 400. The same test without security repairs the alias correctly. The security plugin only lets a plugin identity act on requests that touch nothing but its own registered system indices; restored_... matches none of .opendistro-alerting-alert*, .opensearch-alerting-finding* or .opensearch-alerting-comments*.

[sharathkanaka]

Thanks for catching this. I will drop the suggestion of rename_pattern from the description and add under Limits that a renamed history index isn't repaired. In the docs (another PR I will be sending) for restoring alerting will say not to rename these indices or set include_aliases: false if needed.

[lezzago]

any { it.isWriteIndex != null } treats an explicit is_write_index=false as a layout someone chose, but core resolves no write index for [false, null]. The write index is the one with TRUE, or a single index whose flag is unset.

Repro: create ...-000002 with the alias set to {"is_write_index": false}, snapshot it, delete it, create the local -1 with the alias unset, then restore. The repair leaves the alias on both indices and POST <alias>/_doc returns 400 "no write index". Reproduced with and without security.

Alerting never sets the flag itself, so this needs an operator to have set it. Still, == true alone isn't enough as a fix: keeping only the index that has false would still leave no write index, so the repair would also need to set is_write_index: true on the index it keeps (or skip only when some index has true).

[sharathkanaka]

Makes sesne, it will skip whenfalse is set. I'll skip the repair only when an index has is_write_index: true. Also. is_write_index: false is an explicit choice, I won't write to that index. I'll skip the repair only when an index has true, leave indices with false out when choosing which index keeps the alias, and remove the alias from the rest. If every index has false, I'll skip and log an error.

[lezzago]

Gradle test clusters already set path.repo to the node's own repo directory, so this line doesn't change anything: the new IT passes with it removed.

The new IT does fail on clusters without path.repo, which is how opensearch-build runs plugin integ tests (./gradlew integTest -Dtests.rest.cluster=... against a distribution; alerting's entry in the test manifest sets no path.repo, unlike index-management and k-NN). With path.repo: [] the test fails at its first step:

repository_exception: [restore-test-repo] location [restore-test-repo] doesn't match any of the locations specified by path.repo because this setting is empty

Options: add path.repo to alerting's additional-cluster-configs in opensearch-build, or have the test skip when the repository can't be registered.

[sharathkanaka]

The line is needed for -PnumNodes=3, each node gets its own repo directory, so fs repository verification fails without it (the IT fails on 3 nodes without the line and passes with it).
I'll keep it and update the comment.

For opensearch-build, that makes sens. For now (to cut for 3.10) I'll make the restore ITs skip when the cluster has no path.repo, and raise a PR there to add path.repo: /tmp to alerting's test config, like index-management and k-NN.

[lezzago]

Minor: core already calls onRestore on the generic thread pool, and its callPlugin wrapper logs failures with the restore UUID and snapshot. Launching into this scope returns right away, so that handling is skipped: anything thrown outside the per-alias try goes to the thread's uncaught handler instead. The scope also has no exception handler and is never cancelled on node close. Running the repair synchronously in the callback (for example runBlocking) would keep core's error handling and drop the extra scope.

[sharathkanaka]

RIght, will update thanks

[github-actions]

Persistent review updated to latest commit bfffd69

[lezzago]

Nit, non-blocking: when the index left out for is_write_index: false has the higher rollover counter, the index that keeps the alias can't roll over that day.

Repro (same day, with and without security): create <...-{now/d}-000002> with the alias set to {"is_write_index": false}, snapshot it, delete it, create the local <...-{now/d}-1>, then restore. The repair keeps -1 as intended and writes work, but POST <alias>/_rollover targets -000002, which already exists:

resource_already_exists_exception: index [.opendistro-alerting-alert-history-2026.10.30-000002] already exists

It recovers once the date changes, and it needs an operator to have set the flag, so I don't think it should hold the PR. Rollover always names the new index from the write index's own counter, so changing which index keeps the alias can't avoid this. A fix would have to be in the rollover itself, for example passing an explicit new index name one past the highest counter among the history indices. Documenting it under Limits is also fine. The new is_write_index: false IT only checks a write; adding a rollover check there would cover it.

[sharathkanaka]

True that the fix is for the plugin to pick the next rollover name from the existing history indices. For this PR, I'll document it under Limits and add info in the alerting restore docs that this case can make rollover fail until the date changes.

[codecov]

Codecov Report

❌ Patch coverage is 86.44068% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.82%. Comparing base (778e7ce) to head (bfffd69).
⚠️ Report is 255 commits behind head on main.

Files with missing lines	Patch %	Lines
...n/org/opensearch/alerting/util/WriteAliasRepair.kt	90.24%	2 Missing and 2 partials ⚠️
...rg/opensearch/alerting/comments/CommentsIndices.kt	40.00%	3 Missing ⚠️
...lin/org/opensearch/alerting/alerts/AlertIndices.kt	83.33%	0 Missing and 1 partial ⚠️

❌ Your project check has failed because the head coverage (67.82%) is below the target coverage (70.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2312      +/-   ##
============================================
+ Coverage     67.72%   67.82%   +0.09%     
- Complexity      105     1815    +1710     
============================================
  Files           160      170      +10     
  Lines         10343    14322    +3979     
  Branches       1522     2090     +568     
============================================
+ Hits           7005     9714    +2709     
- Misses         2672     3729    +1057     
- Partials        666      879     +213     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.