fix(service): stop blocking getString on Dispatchers.Default-reachable notification paths

[jamesarich]

Why

Under CMP 1.12.x, compose-resources loads strings on its own dispatcher-less scope (= Dispatchers.Default) instead of inline on the caller — the same AsyncCache change that broke tests and forced the #6664 revert. That turns our blocking getString shim (runBlocking { composeGetString(...) }) into a pool-starvation hazard: each Default-pool caller parks a worker waiting for a load that itself needs a Default worker.

Measured on a CMP 1.12.0-rc01 tree (Robolectric, 10 cores, real Android resource reader):

Concurrent blocking getString calls on Dispatchers.Default	Result
2 / 4 / 8	complete
16	pool wedges permanently — every subsequent getString in the process hangs forever, main thread included (jstack: DefaultDispatcher workers parked on BlockingCoroutine)

The failure mode is a hang, not slowness, and it is unrecoverable. ServiceScope is CoroutineScope(dispatchers.default + SupervisorJob()) and runs the whole incoming-packet/notification pipeline, so a message burst is exactly the trigger shape. Latency numbers for reference: first-ever cold getString ≈ 198 ms on the calling thread; later cold keys 0.8–14.6 ms; warm 0.7 ms.

main is currently on CMP 1.11.1 (revert #6664), where loads resolve inline and the shim is benign — this PR is a hard prerequisite for re-landing the CMP 1.12 bump (#6666 is the test-side groundwork; this is the production-side counterpart). The change is equally correct on 1.11.1, so it can land now.

🛠️ Changes

• MeshNotificationManagerImpl: showGroupSummary, createConversationNotification, reply/mark-as-read actions, and refreshConversationAfterReply now resolve strings via getStringSuspend. cancelMessageNotification becomes suspend on the MeshNotificationManager interface (all production callers were already inside coroutines; desktop impl and test fakes updated).
• AndroidNotificationManager.ensureChannelsInitialized: now suspend behind a Mutex. The mutex is load-bearing — once the guard can suspend, the old boolean check races and a notification could post before its channel exists.
• ConversationShortcutPublisher.ensureConversationShortcut: now suspend.
• Deliberately NOT changed (documented at the declaration): initChannels() stays eager and blocking on Main — MeshServiceOrchestrator.start() posts the foreground-service notification synchronously right after it, so lazy-gating channel creation would post the FGS notification before its channel exists (watchdog-kill risk). Main is not a Default worker, so the one-time ~198 ms block cannot wedge. Also left as follow-ups (latency-only, never Default-reachable): fdroid MapView, Glance widget, desktopApp notifications, buildDmShortcut (by lazy on IO).

🧪 Testing Performed

• spotlessCheck + detekt + assembleDebug + kmpSmokeCompile + test + allTests: the only test failures are byte-identical to the base commit without this change (stash-compared with --rerun-tasks) — i.e. the pre-existing CMP 1.12 test breakage that revert(deps): back out CMP 1.12.0-rc01 until its test regressions are fixed #6664 reverted away, not regressions. On post-revert main those failures don't exist.
• All tests covering the changed paths pass: ReplyReceiverTest, ConversationShortcutPublisherTest, MeshNotificationManagerImplConversationTest conversation flows.
• Measurement methodology: threshold sweep run one test method per JVM, because a wedged Default pool poisons every later test in the process (it surfaces as an unrelated hang elsewhere). The rigs are deliberately not committed — the numbers and the jstack fingerprint are recorded here and in the commit message. Do not gate CI on a saturation repro.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved notification reliability when multiple notifications are created or updated concurrently.
  • Ensured notification channels and localized labels are prepared asynchronously before notifications are displayed.
  • Improved conversation shortcut labels when a custom name is unavailable.
  • Ensured message cancellation updates notification group summaries correctly.

[coderabbitai]

📝 Walkthrough

Walkthrough

The notification manager contract now supports suspendable cancellation. Android notification channels, shortcuts, summaries, conversations, and actions use suspendable string-resource loading. Channel initialization is serialized with a mutex, and tests use suspend-aware verification.

Changes

Notification asynchronous contract

Layer / File(s)	Summary
Suspend cancellation contract core/repository/..., core/testing/..., androidApp/src/test/..., desktopApp/..., core/service/src/androidHostTest/...	cancelMessageNotification is now suspendable across the shared interface, implementations, fakes, and reply receiver tests.
Serialized channel and shortcut initialization core/service/src/androidMain/kotlin/org/meshtastic/core/service/AndroidNotificationManager.kt, core/service/src/androidMain/kotlin/org/meshtastic/core/service/ConversationShortcutPublisher.kt	Channel setup uses a mutex and suspendable string lookup. Conversation shortcut fallback labels use suspendable resource lookup.
Suspendable notification rendering core/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.kt	Group summaries, conversation notifications, refresh flows, reply actions, and mark-as-read actions now use suspendable resource lookup.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Mergeability Score: 🟠 High · up to a6b97

The change can still mark notification channels ready after setup fails and can repost an outdated group summary after the last conversation notification is removed. These concrete correctness issues may cause missing or incorrect notifications, so the PR is not ready to merge until they are addressed.

Sequence Diagram(s)

sequenceDiagram
  participant AndroidNotificationManager
  participant getStringSuspend
  participant NotificationChannel
  AndroidNotificationManager->>getStringSuspend: Resolve channel name
  getStringSuspend-->>AndroidNotificationManager: Return localized name
  AndroidNotificationManager->>NotificationChannel: Create channel

Loading

Possibly related PRs

• meshtastic/Meshtastic-Android#6666: Related asynchronous Compose resource loading and coroutine-aware notification tests.

Suggested labels: bugfix

🚥 Pre-merge checks | ✅ 6

✅ Passed checks (6 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: replacing blocking getString calls on notification paths reachable from Dispatchers.Default.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Sibling Call Sites And Presence Semantics	✅ Passed	The HEAD^→HEAD diff changes suspend signatures, mutex use, resource lookup, and tests; it adds no nullable fields, zero guards, or presence checks, so this check is inapplicable.
Tests Prove The Path, Not The End State	✅ Passed	The only changed test updates mock verification to verifySuspend; it verifies send, read, refresh, and cancellation side effects, with no backing-store, size-only, or emission-order assertion.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

core/service/src/androidMain/kotlin/org/meshtastic/core/service/AndroidNotificationManager.kt (1)

68-94: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Set channelsInitialized only after channel creation succeeds.

Line 70 marks initialization complete before getStringSuspend and createNotificationChannels finish. If either operation throws, later dispatch calls skip initialization and can post notifications without their required channel. Keep the flag false until all channel work completes so a later dispatch retries.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@core/service/src/androidMain/kotlin/org/meshtastic/core/service/AndroidNotificationManager.kt`
around lines 68 - 94, Update ensureChannelsInitialized so channelsInitialized is
set to true only after createChannel and
notificationManager.createNotificationChannels complete successfully, including
legacy-channel cleanup. Leave it false when any channel setup operation throws,
allowing a later dispatch to retry initialization.

core/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.kt (1)

554-655: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Serialize group-summary rebuilds.

showGroupSummary reads activeNotifications before suspendable string resolution. If cancellation clears the final child and summary while this call is suspended, this call can resume and repost a summary from the stale snapshot. Guard summary rebuilds with one Mutex, including the empty-summary cancellation path.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@core/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.kt`
around lines 554 - 655, Serialize all showGroupSummary executions with a
dedicated Mutex, acquiring it before reading
notificationManager.activeNotifications and holding it through string
resolution, summary posting, and the empty-summary cancellation return path.
Update showGroupSummary and its callers as needed so concurrent
cancellation-triggered rebuilds cannot repost a summary from stale state.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In
`@core/service/src/androidMain/kotlin/org/meshtastic/core/service/AndroidNotificationManager.kt`:
- Around line 68-94: Update ensureChannelsInitialized so channelsInitialized is
set to true only after createChannel and
notificationManager.createNotificationChannels complete successfully, including
legacy-channel cleanup. Leave it false when any channel setup operation throws,
allowing a later dispatch to retry initialization.

In
`@core/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.kt`:
- Around line 554-655: Serialize all showGroupSummary executions with a
dedicated Mutex, acquiring it before reading
notificationManager.activeNotifications and holding it through string
resolution, summary posting, and the empty-summary cancellation return path.
Update showGroupSummary and its callers as needed so concurrent
cancellation-triggered rebuilds cannot repost a summary from stale state.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 10f27b80-75b8-4613-9555-6935b9d42ec6

📥 Commits

Reviewing files that changed from the base of the PR and between d8361cc and a6b9713.

📒 Files selected for processing (8)

• androidApp/src/test/kotlin/org/meshtastic/app/service/Fakes.kt
• core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/MeshNotificationManager.kt
• core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/ReplyReceiverTest.kt
• core/service/src/androidMain/kotlin/org/meshtastic/core/service/AndroidNotificationManager.kt
• core/service/src/androidMain/kotlin/org/meshtastic/core/service/ConversationShortcutPublisher.kt
• core/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.kt
• core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeMeshNotificationManager.kt
• desktopApp/src/main/kotlin/org/meshtastic/desktop/notification/DesktopMeshNotificationManager.kt