Fix Dispatchers.IO starvation deadlock in notification send path

[eirsep]

Description

Fixes a permanent Dispatchers.IO starvation deadlock in the notification send path.

SendMessageActionHelper used runBlocking inside code already running on a Dispatchers.IO coroutine (PluginBaseAction launches every transport request on a shared CoroutineScope(Dispatchers.IO)). The parent coroutine parked its dispatcher thread while awaiting child async(Dispatchers.IO) sends that need threads from the same pool. Under sustained concurrent sends to a slow destination (e.g. a webhook that responds slowly or times out), parents eventually occupy all 64 Dispatchers.IO slots; from then on no child can be scheduled and no parent can complete. All Notifications APIs (including GET /_plugins/_notifications/features) hang until the node is restarted. Observed in production on 2.19; code is unchanged on main.

Change: replace all four runBlocking sites in SendMessageActionHelper with structured concurrency — the send-path functions become suspend functions using coroutineScope { ... } + awaitAll(), so waiting parents suspend and release their dispatcher threads instead of parking them. All call chains already terminate in suspend transport-action methods (SendNotificationAction.executeRequest, PublishNotificationAction.executeRequest), so no additional bridging is needed. No behavior change to what is sent, delivery statuses, or parallelism — only how waiting happens (suspension vs. thread parking).

Converted functions:

• sendMessagesInParallel → suspend + coroutineScope (primary deadlock site)
• executeLegacyRequest → suspend, runBlocking wrapper removed
• sendEmailMessage → suspend + coroutineScope (recipient fan-out; previously a nested runBlocking inside a child of the outer one)
• sendMessageToLegacyDestination → suspend + coroutineScope (legacy email branch)
• sendMessageToChannel → suspend (transitively required)

Verification: reproduced the deadlock on an integTest cluster (main) by flooding 200 concurrent feature/test/{config_id} requests at a webhook channel responding after 30s: before this change the node deadlocks permanently (jstack: exactly 64 DefaultDispatcher-worker threads parked in runBlocking at sendMessagesInParallel, zero threads in DestinationHttpClient, no recovery after the webhook is stopped). After this change, all 200 sends complete with correct delivery-failure statuses and all plugin APIs remain responsive throughout; jstack shows no parked parents. Full repro details in the linked issue.

Related Issues

Resolves #1247

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.

[riysaxen-amzn]

Review: ✅ Approve

Well-written fix for a real production deadlock. Categorized findings below:

---

🔴 Critical (0)

None. The fix is correct and complete.

---

🟡 Non-blocking (2)

1. Error propagation behavior change with coroutineScope — Unlike runBlocking, coroutineScope cancels all sibling children when one throws an uncaught exception (structured concurrency semantics). In sendMessagesInParallel, if any async block throws an uncaught exception rather than returning a status object, it would cancel delivery to all other channels. I believe the existing code already catches exceptions internally and returns status objects, but worth confirming that no code path inside sendMessageToChannel can throw uncaught. If it can, wrap each async body in a try-catch that returns a failure status.

2. No backport PR mentioned — Issue says "observed in production on 2.19; code is unchanged on main." Should a 2.x backport be opened, or is this main-only for now?

---

💬 Minor/Nit (2)

3. sendMessageToChannel marked suspend but body has no suspend calls — This is transitively required since it's called from coroutineScope. Correct and idiomatic, but a one-line comment like // suspend required: called from coroutineScope in sendMessagesInParallel would help future readers understand why.

4. No stress/concurrency regression test — The deadlock was reproduced manually with 200 concurrent requests. A follow-up integration test that validates dispatcher health under concurrent load would prevent regressions. Not blocking this PR.

---

Summary

Textbook fix for a well-known Kotlin coroutines anti-pattern (runBlocking inside a dispatcher-bound coroutine). Minimal, mechanical, correct. The coroutineScope + suspend approach is exactly what the kotlinx.coroutines docs prescribe. Verified that the test was updated to wrap the mock in runBlocking for the new suspend signature. Ship it. 🚀

[eirsep]

1. Error propagation behavior change with coroutineScope — Unlike runBlocking, coroutineScope cancels all sibling children when one throws an uncaught exception (structured concurrency semantics). In sendMessagesInParallel, if any async block throws an uncaught exception rather than returning a status object, it would cancel delivery to all other channels. I believe the existing code already catches exceptions internally and returns status objects, but worth confirming that no code path inside sendMessageToChannel can throw uncaught. If it can, wrap each async body in a try-catch that returns a failure status.

runBlocking implements the same structured concurrency semantics: it creates a scope, and a failed async child propagates to the parent Job immediately, cancelling all siblings — then runBlocking rethrows. Failure/cancellation behavior between runBlocking { async {} } and coroutineScope { async {} } is identical

💬 Minor/Nit (2)

3. sendMessageToChannel marked suspend but body has no suspend calls — This is transitively required since it's called from coroutineScope. Correct and idiomatic, but a one-line comment like // suspend required: called from coroutineScope in sendMessagesInParallel would help future readers understand why.

It fails compilation without suspend is this is trivial but added comment.

4. No stress/concurrency regression test — The deadlock was reproduced manually with 200 concurrent requests. A follow-up integration test that validates dispatcher health under concurrent load would prevent regressions. Not blocking this PR.

I acknowledge this but Notifications plugin currently has no real integ test sending a message to a real channel. we need to explore that indepedent of this PR