diff --git a/.skills/code-review/SKILL.md b/.skills/code-review/SKILL.md index e499058488..e58934add9 100644 --- a/.skills/code-review/SKILL.md +++ b/.skills/code-review/SKILL.md @@ -55,7 +55,24 @@ When reviewing code, meticulously verify the following categories. Flag any devi - [ ] **Libraries:** Verify usage of `Turbine` for Flow testing, `Kotest` for property-based testing, and `Mokkery` for mocking. - [ ] **Robolectric Configuration:** Check that Compose UI tests running via Robolectric on JVM are pinned to `@Config(sdk = [34])` to prevent SDK 35 compatibility issues. -### 8. ProGuard / R8 Rules +### 8. Logging & Crash Reporting +Kermit is the only logging API, and on the **google** flavor its writers fan every call out to **both** Firebase Crashlytics and Datadog RUM (`androidApp/src/google/.../GooglePlatformAnalytics.kt`). Log level is therefore a *reporting* decision, not just a verbosity one. + +**The rule: `Logger.e` means "a defect someone can fix". Everything else is `Logger.w` or below.** + +- [ ] **Severity gates reporting:** `Severity.Error`/`Assert` become a Crashlytics non-fatal (`shouldReportAsException`, which exempts `CancellationException` and any `ExpectedCondition` in the cause chain) **and** a Datadog RUM error (`shouldDowngradeForDatadog`, which exempts only `ExpectedCondition`). `Warn` and below never report in either sink, with no exceptions. Attaching a throwable at warn level is free and keeps the stack trace in the logs, so demoting costs nothing. +- [ ] **Don't "unify" the two cancellation rules.** Crashlytics drops `CancellationException` because it is a crash-triage tool; Datadog keeps it because a cancellation logged at *error* means a call site swallowed it instead of rethrowing — broken structured concurrency, and a real bug. That asymmetry is the detector that found #6468. Likewise, neither rule unwraps the cause chain for cancellation: coroutine machinery attaches cancellations as the cause of unrelated genuine failures, and unwrapping would silently drop those reports. +- [ ] **`Logger.e` with no throwable still reports.** Crashlytics synthesises an `Exception(message)`; Datadog raises a RUM error from the level alone. `Logger.e { "…" }` is *not* a cheap log line. +- [ ] **Expected conditions must not be reported.** Bluetooth off, a permission not granted, location services off, a deliberate disconnect, a peer/broker protocol violation, a handled retry, a guard that is doing its job — these are environment states, not bugs. Reporting them buries real regressions during release triage. +- [ ] **Use the `ExpectedCondition` seam** (`core/common/src/commonMain/.../log/ExpectedCondition.kt`): + - Exception type that *only ever* means "the environment said no" → implement `ExpectedCondition` and give it a stable, low-cardinality `expectedConditionLabel` (e.g. `ble-scan-bluetooth-disabled`). `BleScanStartException` is the reference example. + - Exception type shared between expected and genuine failures → leave the type alone and log that call site at `Logger.w`. + - Both sinks consult `shouldReportAsException(severity, throwable)`, so an `ExpectedCondition` is suppressed even if some call site logs it at error. Treat that as a backstop, not a licence to log expected states at error. +- [ ] **Prefer a rate over an exception.** For conditions worth *watching* but not *fixing* (watchdog fired, reconnect attempt failed), emit a warn log with a stable label and track its rate in the log backend. Do not manufacture a throwable just to get a stack trace. +- [ ] **Third-party log bridges:** adapters that forward another library's logs into Kermit must downgrade that library's "error" level — its errors are usually operational. See `core/ble/.../KermitLogEngine.kt` (Kable). +- [ ] **New `Logger.e` in a PR:** ask what the on-call engineer would *do* about it. If the answer is "nothing, that's just the user's phone", it is a `Logger.w`. + +### 9. ProGuard / R8 Rules - [ ] **New Dependencies:** If a new reflection-heavy dependency is added (DI, serialization, JNI, ServiceLoader), verify keep rules exist in **both** `androidApp/proguard-rules.pro` (R8) and `desktopApp/proguard-rules.pro` (ProGuard). The two files must stay aligned. - [ ] **Release Smoke-Test:** For dependency or ProGuard rule changes, verify `assembleRelease` and `./gradlew :desktopApp:runRelease` succeed. diff --git a/.skills/compose-ui/strings-index.txt b/.skills/compose-ui/strings-index.txt index 33ae74fb9b..e63d403ff6 100644 --- a/.skills/compose-ui/strings-index.txt +++ b/.skills/compose-ui/strings-index.txt @@ -131,6 +131,7 @@ bluetooth_feature_config_description bluetooth_feature_discovery bluetooth_feature_discovery_description bluetooth_permission +bluetooth_scan_location_services_disabled bluetooth_scan_missing_permission bluetooth_scan_start_failed bluetooth_scan_too_frequent diff --git a/androidApp/src/google/kotlin/org/meshtastic/app/analytics/GooglePlatformAnalytics.kt b/androidApp/src/google/kotlin/org/meshtastic/app/analytics/GooglePlatformAnalytics.kt index 85f48aa0fb..32989f3f5c 100644 --- a/androidApp/src/google/kotlin/org/meshtastic/app/analytics/GooglePlatformAnalytics.kt +++ b/androidApp/src/google/kotlin/org/meshtastic/app/analytics/GooglePlatformAnalytics.kt @@ -52,11 +52,12 @@ import com.google.firebase.crashlytics.crashlytics import com.google.firebase.crashlytics.setCustomKeys import com.google.firebase.initialize import io.opentelemetry.api.GlobalOpenTelemetry -import kotlinx.coroutines.CancellationException import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.onEach import org.koin.core.annotation.Single import org.meshtastic.app.BuildConfig +import org.meshtastic.core.common.log.shouldDowngradeForDatadog +import org.meshtastic.core.common.log.shouldReportAsException import org.meshtastic.core.repository.AnalyticsPrefs import org.meshtastic.core.repository.DataPair import org.meshtastic.core.repository.PlatformAnalytics @@ -301,21 +302,18 @@ class GooglePlatformAnalytics(private val context: Context, private val analytic // Add the log to the Crashlytics log buffer so it appears in reports Firebase.crashlytics.log("$severity/$tag: $message") - // Filter out normal coroutine cancellations - if (throwable is CancellationException) return + // Cancellations and expected conditions stay breadcrumbs only — see shouldReportAsException. + if (!shouldReportAsException(severity, throwable)) return - // Only record non-fatal exceptions for actual Errors (Severity.Error or Severity.Assert) - if (severity >= Severity.Error) { - if (throwable != null) { - Firebase.crashlytics.recordException(throwable) - } else { - Firebase.crashlytics.setCustomKeys { - key(KEY_PRIORITY, severity.ordinal) - key(KEY_TAG, tag) - key(KEY_MESSAGE, message) - } - Firebase.crashlytics.recordException(Exception(message)) + if (throwable != null) { + Firebase.crashlytics.recordException(throwable) + } else { + Firebase.crashlytics.setCustomKeys { + key(KEY_PRIORITY, severity.ordinal) + key(KEY_TAG, tag) + key(KEY_MESSAGE, message) } + Firebase.crashlytics.recordException(Exception(message)) } } } @@ -323,8 +321,13 @@ class GooglePlatformAnalytics(private val context: Context, private val analytic private inner class DatadogLogWriter : LogWriter() { override fun log(severity: Severity, message: String, tag: String, throwable: Throwable?) { val logger = datadogLogger ?: return + // The Datadog SDK turns any log at ERROR or above into a RUM error purely from the level — it has no + // per-call opt-out — so downgrading to WARN is the only way to keep an expected condition out of RUM + // error tracking while still emitting the log line. Note this deliberately keeps CancellationException + // at error here even though Crashlytics drops it; see shouldDowngradeForDatadog. + val effectiveSeverity = if (shouldDowngradeForDatadog(severity, throwable)) Severity.Warn else severity val datadogPriority = - when (severity) { + when (effectiveSeverity) { Severity.Verbose -> android.util.Log.VERBOSE Severity.Debug -> android.util.Log.DEBUG Severity.Info -> android.util.Log.INFO diff --git a/core/ble/build.gradle.kts b/core/ble/build.gradle.kts index df2eeff200..3fb444e884 100644 --- a/core/ble/build.gradle.kts +++ b/core/ble/build.gradle.kts @@ -25,7 +25,8 @@ kotlin { sourceSets { commonMain.dependencies { - implementation(projects.core.common) + // api: BleScanStartException implements core.common's ExpectedCondition in its public supertype list. + api(projects.core.common) implementation(projects.core.di) implementation(projects.core.model) diff --git a/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/BleScanStartException.kt b/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/BleScanStartException.kt index 5979a6e0c2..15ca0155a0 100644 --- a/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/BleScanStartException.kt +++ b/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/BleScanStartException.kt @@ -16,24 +16,55 @@ */ package org.meshtastic.core.ble +import org.meshtastic.core.common.log.ExpectedCondition import kotlin.time.Duration -/** Known reasons a BLE discovery scan failed before Android registered the scanner. */ -enum class BleScanStartFailureReason(val androidCode: String, val description: String) { +/** + * Known reasons a BLE discovery scan failed before Android registered the scanner. + * + * Every reason here is an environment state — a permission not granted, a radio switched off, an OS quota — so + * [BleScanStartException] is an [ExpectedCondition] and is never reported as a crash. + * + * @property androidCode the platform-level code or constant this maps to, used in log lines. + * @property description a human-readable explanation for log lines. + * @property label the stable, low-cardinality [ExpectedCondition.expectedConditionLabel] for rate tracking. + */ +enum class BleScanStartFailureReason(val androidCode: String, val description: String, val label: String) { ApplicationRegistrationFailed( androidCode = "SCAN_FAILED_APPLICATION_REGISTRATION_FAILED(2)", description = "Android could not register the app for BLE scanning", + label = "ble-scan-registration-failed", ), MissingScanPermission( androidCode = "MISSING_SCAN_PERMISSION", description = "A runtime permission required for BLE scanning is not granted", + label = "ble-scan-missing-permission", ), ScanningTooFrequently( androidCode = "SCAN_FAILED_SCANNING_TOO_FREQUENTLY(6)", description = "Android rejected a BLE scan because the app reached its scan-start quota", + label = "ble-scan-too-frequently", + ), + BluetoothDisabled( + androidCode = "BLUETOOTH_DISABLED", + description = "Bluetooth is switched off", + label = "ble-scan-bluetooth-disabled", + ), + LocationServicesDisabled( + androidCode = "LOCATION_SERVICES_DISABLED", + description = "Location services are off, and this Android version requires them for BLE scanning", + label = "ble-scan-location-services-disabled", ), } -/** A discovery scan-start failure. No advertisements can be delivered until a future scan starts successfully. */ +/** + * A discovery scan-start failure. No advertisements can be delivered until a future scan starts successfully. + * + * This is an [ExpectedCondition]: every [BleScanStartFailureReason] describes the environment refusing the scan, not a + * defect in the app, so callers should surface it to the user and log it at warn level rather than reporting it. + */ class BleScanStartException(val reason: BleScanStartFailureReason, cause: Throwable, val retryAfter: Duration? = null) : - IllegalStateException("BLE scan could not start: ${reason.androidCode}", cause) + IllegalStateException("BLE scan could not start: ${reason.androidCode}", cause), + ExpectedCondition { + override val expectedConditionLabel: String = reason.label +} diff --git a/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/KableBleScanner.kt b/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/KableBleScanner.kt index 4a433a758a..a96d4dafb9 100644 --- a/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/KableBleScanner.kt +++ b/core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/KableBleScanner.kt @@ -18,6 +18,8 @@ package org.meshtastic.core.ble import com.juul.kable.Advertisement import com.juul.kable.Scanner +import com.juul.kable.UnmetRequirementException +import com.juul.kable.UnmetRequirementReason import kotlinx.coroutines.CancellationException import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.channelFlow @@ -71,6 +73,10 @@ open class KableBleScanner(private val loggingConfig: BleLoggingConfig) : BleSca return scanner.advertisements.map(Advertisement::toScanResult) } + // ThrowsCount: three deliberate rethrow paths, one per exception family Kable can surface here — cancellation + // (must propagate untouched), UnmetRequirementException (an IOException) and IllegalStateException. They have no + // common supertype below Exception, so merging them would mean catching Exception broadly instead. + @Suppress("ThrowsCount") override fun scan(timeout: Duration, serviceUuid: Uuid?, address: String?): Flow { val filter = resolveKableScanFilter(serviceUuid = serviceUuid, address = address) @@ -91,6 +97,12 @@ open class KableBleScanner(private val loggingConfig: BleLoggingConfig) : BleSca } } catch (ex: CancellationException) { throw ex + } catch (ex: UnmetRequirementException) { + // Kable models "Bluetooth is off" and "location services are off" as an IOException, so these + // never matched the IllegalStateException branch below and escaped raw to ViewModel-level + // catch-alls that log at error — the top source of Crashlytics/RUM noise. Map them to the + // typed, non-reported BleScanStartException instead. + throw ex.asBleScanStartException() } catch (ex: IllegalStateException) { throw ex.asBleScanStartExceptionOrNull() ?: ex } @@ -99,6 +111,23 @@ open class KableBleScanner(private val loggingConfig: BleLoggingConfig) : BleSca } } +/** + * Maps Kable's typed [UnmetRequirementReason] onto the matching [BleScanStartFailureReason]. + * + * Kable's reason enum is exhaustive over the preconditions it checks, so this needs no message matching — unlike the + * [IllegalStateException] paths below, which Kable only distinguishes by message text. + * + * Kept `internal` and separate from the exception so it stays unit-testable: [UnmetRequirementException] has an + * `internal` constructor in Kable and cannot be instantiated from our tests. + */ +internal fun UnmetRequirementReason.toBleScanStartFailureReason(): BleScanStartFailureReason = when (this) { + UnmetRequirementReason.BluetoothDisabled -> BleScanStartFailureReason.BluetoothDisabled + UnmetRequirementReason.LocationServicesDisabled -> BleScanStartFailureReason.LocationServicesDisabled +} + +private fun UnmetRequirementException.asBleScanStartException(): BleScanStartException = + BleScanStartException(reason.toBleScanStartFailureReason(), this) + private fun Throwable.asBleScanStartExceptionOrNull(): BleScanStartException? { var current: Throwable? = this var depth = 0 diff --git a/core/ble/src/commonTest/kotlin/org/meshtastic/core/ble/BleScanStartExceptionTest.kt b/core/ble/src/commonTest/kotlin/org/meshtastic/core/ble/BleScanStartExceptionTest.kt new file mode 100644 index 0000000000..4743d32557 --- /dev/null +++ b/core/ble/src/commonTest/kotlin/org/meshtastic/core/ble/BleScanStartExceptionTest.kt @@ -0,0 +1,84 @@ +/* + * Copyright (c) 2026 Meshtastic LLC + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.meshtastic.core.ble + +import co.touchlab.kermit.Severity +import com.juul.kable.UnmetRequirementReason +import org.meshtastic.core.common.log.isExpectedCondition +import org.meshtastic.core.common.log.shouldReportAsException +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * A BLE scan refused by the environment must never reach a crash reporter. + * + * Bluetooth off, location services off and a missing scan permission were four of the loudest Crashlytics issues during + * 2.8.0 triage, all of them non-actionable. + */ +class BleScanStartExceptionTest { + + @Test + fun `every scan-start failure is an expected condition`() { + BleScanStartFailureReason.entries.forEach { reason -> + val exception = BleScanStartException(reason, cause = IllegalStateException("cause")) + assertTrue(exception.isExpectedCondition(), "$reason must be an expected condition") + assertEquals(reason.label, exception.expectedConditionLabel) + } + } + + @Test + fun `scan-start failures are never reported even when logged at error`() { + BleScanStartFailureReason.entries.forEach { reason -> + val exception = BleScanStartException(reason, cause = IllegalStateException("cause")) + assertFalse( + shouldReportAsException(Severity.Error, exception), + "$reason must not be recorded as a non-fatal", + ) + } + } + + @Test + fun `labels are unique and low cardinality`() { + val labels = BleScanStartFailureReason.entries.map { it.label } + assertEquals(labels.size, labels.toSet().size, "labels must be unique to be usable as rate keys") + labels.forEach { label -> + assertTrue(label.isNotBlank(), "label must not be blank") + assertEquals(label.lowercase(), label, "label '$label' must be lowercase for stable grouping") + } + } + + // Kable's UnmetRequirementException has an internal constructor, so the reason mapping is verified directly. + @Test + fun `kable unmet-requirement reasons map onto scan-start reasons`() { + assertEquals( + BleScanStartFailureReason.BluetoothDisabled, + UnmetRequirementReason.BluetoothDisabled.toBleScanStartFailureReason(), + ) + assertEquals( + BleScanStartFailureReason.LocationServicesDisabled, + UnmetRequirementReason.LocationServicesDisabled.toBleScanStartFailureReason(), + ) + } + + @Test + fun `every kable unmet-requirement reason is mapped`() { + // Guards against a Kable upgrade adding a reason that silently falls through to error-level logging. + UnmetRequirementReason.entries.forEach { it.toBleScanStartFailureReason() } + } +} diff --git a/core/common/build.gradle.kts b/core/common/build.gradle.kts index 36c0b4b474..edc318faff 100644 --- a/core/common/build.gradle.kts +++ b/core/common/build.gradle.kts @@ -31,7 +31,8 @@ kotlin { api(libs.kotlinx.datetime) api(libs.okio) api(libs.uri.kmp) - implementation(libs.kermit) + // api: `shouldReportAsException` exposes Kermit's Severity in its signature. + api(libs.kermit) } androidMain.dependencies { api(libs.androidx.core.ktx) } } diff --git a/core/common/src/commonMain/kotlin/org/meshtastic/core/common/log/ExpectedCondition.kt b/core/common/src/commonMain/kotlin/org/meshtastic/core/common/log/ExpectedCondition.kt new file mode 100644 index 0000000000..a8c47077e4 --- /dev/null +++ b/core/common/src/commonMain/kotlin/org/meshtastic/core/common/log/ExpectedCondition.kt @@ -0,0 +1,107 @@ +/* + * Copyright (c) 2026 Meshtastic LLC + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.meshtastic.core.common.log + +import co.touchlab.kermit.Severity +import kotlinx.coroutines.CancellationException + +/** + * Marks a [Throwable] as an *expected condition*: a state the app is designed to encounter and recover from, caused by + * the environment rather than by a defect in this code. + * + * Bluetooth switched off, a runtime permission the user has not granted, location services disabled, and a deliberate + * disconnect are all expected conditions. They deserve a log line — and are worth watching as a *rate* — but they are + * not bugs anyone can act on, so they must never be recorded as exceptions in Crashlytics or Datadog RUM. Reporting + * them buries genuine regressions during release triage. + * + * Implement this on exception types whose very existence means "the environment said no". Where an exception type is + * shared between expected and genuine failures, keep the type clean and simply log the expected call site at + * [Severity.Warn] instead. An [ExpectedCondition] is suppressed by both sinks — see [shouldReportAsException] + * (Crashlytics) and [shouldDowngradeForDatadog] (Datadog), which agree here but deliberately differ on cancellation. + */ +interface ExpectedCondition { + /** + * Stable, low-cardinality label naming which condition occurred, e.g. `ble-bluetooth-disabled`. This is what makes + * the condition countable as a rate in the log backend instead of an error, so keep it free of addresses, node ids, + * timings and any other per-user detail. + */ + val expectedConditionLabel: String +} + +/** Cause chains are walked with a depth cap so a malformed or self-referential chain cannot spin. */ +private const val MAX_CAUSE_DEPTH = 10 + +/** + * Returns the [ExpectedCondition.expectedConditionLabel] of the first [ExpectedCondition] in this throwable's cause + * chain, or `null` when nothing in the chain is an expected condition. + * + * The chain is walked because transport and coroutine machinery routinely wraps the original cause. + */ +fun Throwable.expectedConditionLabel(): String? { + var current: Throwable? = this + var depth = 0 + while (current != null && depth < MAX_CAUSE_DEPTH) { + (current as? ExpectedCondition)?.let { + return it.expectedConditionLabel + } + current = current.cause + depth++ + } + return null +} + +/** Returns `true` when this throwable, or anything in its cause chain, is an [ExpectedCondition]. */ +fun Throwable.isExpectedCondition(): Boolean = expectedConditionLabel() != null + +/** + * Whether a log line should become a **Crashlytics** non-fatal. + * + * A log below [Severity.Error] is never reported by either sink, so warn-level logging remains the simplest way to + * record an expected condition without reporting it. + * + * The two sinks deliberately differ on [CancellationException] — see [shouldDowngradeForDatadog]. + */ +fun shouldReportAsException(severity: Severity, throwable: Throwable?): Boolean = when { + severity < Severity.Error -> false + + // Top-level only, matching the long-standing Crashlytics behaviour. Deliberately NOT a cause-chain walk: + // coroutine machinery routinely attaches a cancellation as the cause of an unrelated genuine failure, and + // unwrapping here would silently drop those reports. An ExpectedCondition marker is an explicit statement by + // the author about the nature of the condition, so that one is safe to unwrap; an incidental cancellation + // buried in a cause chain is not. + throwable is CancellationException -> false + + throwable?.isExpectedCondition() == true -> false + + else -> true +} + +/** + * Whether the **Datadog** writer must downgrade an error-level log to `WARN`. + * + * Datadog has no per-call opt-out: its SDK turns *any* log at `ERROR` or above into a RUM error, throwable or not. + * Downgrading the emitted level is the only way to keep something out of RUM error tracking while still emitting the + * log line. + * + * This intentionally does **not** mirror [shouldReportAsException] for [CancellationException]. Crashlytics filters + * cancellations because it is a crash-triage tool; Datadog keeps them because a cancellation logged at error level + * means some call site *swallowed* it instead of rethrowing — broken structured concurrency, and a real bug. That + * asymmetry is what surfaced the swallowed-cancellation defects fixed in #6468, so it is load-bearing: do not "unify" + * the two rules. + */ +fun shouldDowngradeForDatadog(severity: Severity, throwable: Throwable?): Boolean = + severity >= Severity.Error && throwable?.isExpectedCondition() == true diff --git a/core/common/src/commonTest/kotlin/org/meshtastic/core/common/log/ExpectedConditionTest.kt b/core/common/src/commonTest/kotlin/org/meshtastic/core/common/log/ExpectedConditionTest.kt new file mode 100644 index 0000000000..e3f74cbc72 --- /dev/null +++ b/core/common/src/commonTest/kotlin/org/meshtastic/core/common/log/ExpectedConditionTest.kt @@ -0,0 +1,155 @@ +/* + * Copyright (c) 2026 Meshtastic LLC + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.meshtastic.core.common.log + +import co.touchlab.kermit.Severity +import kotlinx.coroutines.CancellationException +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertNull +import kotlin.test.assertTrue + +/** + * Locks down the reporting contract that both Crashlytics and Datadog RUM consult. + * + * Regressions here are silent and expensive: they either bury real crashes under expected-condition noise, or stop a + * genuine defect from being reported at all. + */ +class ExpectedConditionTest { + + private class TestExpected(override val expectedConditionLabel: String = "test-expected") : + IllegalStateException("expected"), + ExpectedCondition + + // ── shouldReportAsException ────────────────────────────────────────────────────── + + @Test + fun `severities below Error are never reported`() { + val severities = listOf(Severity.Verbose, Severity.Debug, Severity.Info, Severity.Warn) + severities.forEach { severity -> + assertFalse( + shouldReportAsException(severity, RuntimeException("boom")), + "$severity with a throwable must not be reported", + ) + assertFalse(shouldReportAsException(severity, null), "$severity without a throwable must not be reported") + } + } + + @Test + fun `genuine errors are still reported`() { + assertTrue(shouldReportAsException(Severity.Error, RuntimeException("boom"))) + assertTrue(shouldReportAsException(Severity.Assert, RuntimeException("boom"))) + } + + @Test + fun `error without a throwable is still reported`() { + // Crashlytics synthesises an Exception(message) for these, so the gate must stay open. + assertTrue(shouldReportAsException(Severity.Error, null)) + } + + @Test + fun `cancellation is never reported`() { + assertFalse(shouldReportAsException(Severity.Error, CancellationException("cancelled"))) + assertFalse(shouldReportAsException(Severity.Assert, CancellationException("cancelled"))) + } + + @Test + fun `expected conditions are never reported even at error severity`() { + assertFalse(shouldReportAsException(Severity.Error, TestExpected())) + assertFalse(shouldReportAsException(Severity.Assert, TestExpected())) + } + + @Test + fun `expected condition wrapped in a plain exception is not reported`() { + val wrapped = RuntimeException("transport failed", TestExpected()) + assertFalse(shouldReportAsException(Severity.Error, wrapped)) + } + + @Test + fun `a genuine failure carrying a cancellation cause is still reported`() { + // Coroutine machinery routinely attaches a cancellation as the cause of an unrelated real failure. + // Unwrapping the chain for CancellationException would silently drop these reports. + val wrapped = RuntimeException("write failed", CancellationException("scope closed")) + assertTrue(shouldReportAsException(Severity.Error, wrapped)) + } + + // ── Datadog downgrade rule (deliberately differs on cancellation) ──────────────── + + @Test + fun `datadog downgrades expected conditions`() { + assertTrue(shouldDowngradeForDatadog(Severity.Error, TestExpected())) + assertTrue(shouldDowngradeForDatadog(Severity.Assert, RuntimeException("outer", TestExpected()))) + } + + @Test + fun `datadog keeps cancellation at error even though crashlytics drops it`() { + // Load-bearing asymmetry: a cancellation logged at error means a call site swallowed it instead of + // rethrowing — broken structured concurrency, and a real bug. This is the signal that surfaced #6468. + val cancellation = CancellationException("cancelled") + assertFalse(shouldReportAsException(Severity.Error, cancellation)) + assertFalse(shouldDowngradeForDatadog(Severity.Error, cancellation)) + } + + @Test + fun `datadog leaves genuine errors and sub-error severities alone`() { + assertFalse(shouldDowngradeForDatadog(Severity.Error, RuntimeException("boom"))) + assertFalse(shouldDowngradeForDatadog(Severity.Error, null)) + assertFalse(shouldDowngradeForDatadog(Severity.Warn, TestExpected())) + } + + // ── cause-chain walking ────────────────────────────────────────────────────────── + + @Test + fun `label is read from the throwable itself`() { + assertEquals("test-expected", TestExpected().expectedConditionLabel()) + assertTrue(TestExpected().isExpectedCondition()) + } + + @Test + fun `label is read through the cause chain`() { + val wrapped = RuntimeException("outer", IllegalStateException("middle", TestExpected("ble-bluetooth-disabled"))) + assertEquals("ble-bluetooth-disabled", wrapped.expectedConditionLabel()) + assertTrue(wrapped.isExpectedCondition()) + } + + @Test + fun `plain throwable has no label`() { + val plain = RuntimeException("outer", IllegalStateException("inner")) + assertNull(plain.expectedConditionLabel()) + assertFalse(plain.isExpectedCondition()) + } + + @Test + fun `cause chain walking is depth limited`() { + // A chain longer than the cap must terminate rather than spin, even though the marker is out of reach. + var deep: Throwable = TestExpected() + repeat(50) { deep = RuntimeException("layer", deep) } + assertNull(deep.expectedConditionLabel()) + assertFalse(deep.isExpectedCondition()) + } + + @Test + fun `self referential cause chain terminates`() { + val selfReferencing = + object : RuntimeException("loop") { + override val cause: Throwable + get() = this + } + assertNull(selfReferencing.expectedConditionLabel()) + } +} diff --git a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshConnectionManagerImpl.kt b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshConnectionManagerImpl.kt index 814cf39fa0..59e426e5ef 100644 --- a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshConnectionManagerImpl.kt +++ b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MeshConnectionManagerImpl.kt @@ -620,7 +620,11 @@ class MeshConnectionManagerImpl( if (serviceRepository.connectionState.value !is ConnectionState.Connecting) { return@handledLaunch } - Logger.e { + // Warn, not error: the watchdog firing is the recovery mechanism working, and the cause is a + // stalled radio or link rather than a defect here. A throwable-less Logger.e still synthesises a + // non-fatal in Crashlytics and a RUM error, which made this one of the loudest issues in triage. + // Track it as a rate over this log line instead. + Logger.w { "Fast-handshake watchdog expired after progress stalled — requesting forced transport restart" } runSiblingHandshakeRecovery() diff --git a/core/network/src/commonMain/kotlin/org/meshtastic/core/network/repository/MQTTRepositoryImpl.kt b/core/network/src/commonMain/kotlin/org/meshtastic/core/network/repository/MQTTRepositoryImpl.kt index ada456a58e..83c01c6b1e 100644 --- a/core/network/src/commonMain/kotlin/org/meshtastic/core/network/repository/MQTTRepositoryImpl.kt +++ b/core/network/src/commonMain/kotlin/org/meshtastic/core/network/repository/MQTTRepositoryImpl.kt @@ -31,6 +31,7 @@ import kotlinx.coroutines.flow.first import kotlinx.coroutines.launch import kotlinx.coroutines.sync.Semaphore import kotlinx.coroutines.sync.withPermit +import kotlinx.io.IOException import kotlinx.serialization.ExperimentalSerializationApi import kotlinx.serialization.SerializationException import kotlinx.serialization.json.Json @@ -208,7 +209,19 @@ class MQTTRepositoryImpl( } else -> { - Logger.e(result.exceptionOrNull()) { "MQTT connect failed, retrying in ${reconnectDelay}ms" } + val failure = result.exceptionOrNull() + // Broker- and network-side failures are what this retry loop exists to absorb — an + // unreachable host, a TLS problem, a dropped connection, or a broker that violates the + // MQTT 5 spec (e.g. the topic-alias limit). None are defects in this app, and reporting + // every retry as a non-fatal drowned real regressions. + // + // Anything else landing here is unexpected — a fault in our own connect/subscribe setup + // rather than the peer's — so it keeps reporting. + if (failure.isExpectedMqttRetryFailure()) { + Logger.w(failure) { "MQTT connect failed, retrying in ${reconnectDelay}ms" } + } else { + Logger.e(failure) { "MQTT connect failed unexpectedly, retrying in ${reconnectDelay}ms" } + } delay(reconnectDelay) reconnectDelay = (reconnectDelay * RECONNECT_BACKOFF_MULTIPLIER).coerceAtMost(MAX_RECONNECT_DELAY_MS) @@ -307,6 +320,24 @@ class MQTTRepositoryImpl( } } +/** + * `true` when an MQTT connect/subscribe failure is one the retry loop is designed to absorb, rather than a defect worth + * reporting to Crashlytics/Datadog. + * + * Covers the MQTT client's own sealed error hierarchy — a lost connection, and protocol violations by the broker such + * as an inbound topic alias above the advertised maximum — plus transport-level I/O failures (unreachable host, TLS, + * socket reset). All are peer- or network-side and not actionable by this app. + * + * Deliberately narrow: an unexpected exception escaping our own client/state setup is a real bug and must keep + * reporting, so anything outside these families returns `false`. + */ +private fun Throwable?.isExpectedMqttRetryFailure(): Boolean = when (this) { + null -> false + is MqttException -> true + is IOException -> true + else -> false +} + internal data class MqttClientSetup( val ownerId: String, val mqttConfig: ModuleConfig.MQTTConfig?, diff --git a/core/resources/src/commonMain/composeResources/values/strings.xml b/core/resources/src/commonMain/composeResources/values/strings.xml index fea80c3548..ab1e581230 100644 --- a/core/resources/src/commonMain/composeResources/values/strings.xml +++ b/core/resources/src/commonMain/composeResources/values/strings.xml @@ -149,6 +149,7 @@ Discovery Find and identify Meshtastic devices near you. Bluetooth + Location services are off. Turn them on to scan for nearby devices. Bluetooth scan needs permission. Grant the Nearby devices (or Location) permission to find devices. Bluetooth scan couldn't start. Try again, or toggle Bluetooth if the problem continues. diff --git a/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/ViewModelExtensions.kt b/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/ViewModelExtensions.kt index b27e7391d3..eee65e6ca1 100644 --- a/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/ViewModelExtensions.kt +++ b/core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/ViewModelExtensions.kt @@ -33,6 +33,7 @@ import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.onStart import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.launch +import org.meshtastic.core.common.log.expectedConditionLabel import org.meshtastic.core.resources.Res import org.meshtastic.core.resources.UiText import org.meshtastic.core.resources.unknown_error @@ -122,7 +123,14 @@ fun safeLaunch( throw e } catch (e: Exception) { val label = tag ?: "safeLaunch" - Logger.e(e) { "[$label] Unhandled exception" } + // Expected conditions (Bluetooth off, permission not granted, …) still reach the user as an error + // event, but are logged at warn so neither Crashlytics nor Datadog RUM records them as a defect. + val expectedLabel = e.expectedConditionLabel() + if (expectedLabel != null) { + Logger.w(e) { "[$label] Expected condition: $expectedLabel" } + } else { + Logger.e(e) { "[$label] Unhandled exception" } + } val message = e.message?.let { UiText.DynamicString(it) } ?: UiText.Resource(Res.string.unknown_error) errorEvents?.tryEmit(message) } diff --git a/feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ScannerViewModel.kt b/feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ScannerViewModel.kt index 3d64a55473..898524ebe0 100644 --- a/feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ScannerViewModel.kt +++ b/feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ScannerViewModel.kt @@ -59,6 +59,8 @@ import org.meshtastic.core.repository.RadioPrefs import org.meshtastic.core.repository.ServiceRepository import org.meshtastic.core.repository.UiPrefs import org.meshtastic.core.resources.Res +import org.meshtastic.core.resources.bluetooth_disabled +import org.meshtastic.core.resources.bluetooth_scan_location_services_disabled import org.meshtastic.core.resources.bluetooth_scan_missing_permission import org.meshtastic.core.resources.bluetooth_scan_start_failed import org.meshtastic.core.resources.bluetooth_scan_too_frequent @@ -76,8 +78,49 @@ internal val BLE_SCAN_START_FAILURE_RETRY_COOLDOWN = 15.seconds private const val BLE_SCAN_START_FAILURE_MESSAGE_FALLBACK = "Bluetooth scan couldn't start. Try again, or toggle Bluetooth if the problem continues." -private fun effectiveBleScanRetryCooldown(retryAfter: Duration?): Duration = - maxOf(BLE_SCAN_START_FAILURE_RETRY_COOLDOWN, retryAfter ?: Duration.ZERO) +/** + * How long to block scan restarts after a scan-start failure. + * + * A cooldown only helps where retrying too soon is itself the problem — Android's scan-start quota, or a registration + * failure that needs time to settle. [BleScanStartFailureReason.BluetoothDisabled] and + * [BleScanStartFailureReason.LocationServicesDisabled] instead clear the moment the user flips a system toggle, so + * holding the scan button dead for 15s after they have fixed it would just look broken. + */ +private fun effectiveBleScanRetryCooldown(reason: BleScanStartFailureReason, retryAfter: Duration?): Duration = + when (reason) { + BleScanStartFailureReason.BluetoothDisabled, + BleScanStartFailureReason.LocationServicesDisabled, + -> retryAfter ?: Duration.ZERO + + BleScanStartFailureReason.ApplicationRegistrationFailed, + BleScanStartFailureReason.MissingScanPermission, + BleScanStartFailureReason.ScanningTooFrequently, + -> + maxOf(BLE_SCAN_START_FAILURE_RETRY_COOLDOWN, retryAfter ?: Duration.ZERO) + } + +/** + * Last-resort English copy used only when compose-resources cannot resolve the translated string. + * + * Kept in step with the `bluetooth_scan_*` resources so the degraded path still names the actual problem instead of + * falling back to generic "scan couldn't start" advice for every reason. + */ +private fun untranslatedScanStartFailureMessage(reason: BleScanStartFailureReason, retryCooldownSeconds: Long): String = + when (reason) { + BleScanStartFailureReason.ScanningTooFrequently -> { + val unit = if (retryCooldownSeconds == 1L) "second" else "seconds" + "Bluetooth scan limit reached. Try again in $retryCooldownSeconds $unit." + } + + BleScanStartFailureReason.BluetoothDisabled -> "Bluetooth is off. Turn it on to scan for nearby devices." + + BleScanStartFailureReason.LocationServicesDisabled -> + "Location services are off. Turn them on to scan for nearby devices." + + BleScanStartFailureReason.ApplicationRegistrationFailed, + BleScanStartFailureReason.MissingScanPermission, + -> BLE_SCAN_START_FAILURE_MESSAGE_FALLBACK + } private fun Duration.roundedUpWholeSeconds(): Long { val completeSeconds = inWholeSeconds @@ -403,8 +446,8 @@ open class ScannerViewModel( scanJob = null _isBleScanning.value = false uiPrefs.setBleAutoScan(false) - val retryCooldown = effectiveBleScanRetryCooldown(exception.retryAfter) - startBleScanRetryCooldown(retryCooldown) + val retryCooldown = effectiveBleScanRetryCooldown(exception.reason, exception.retryAfter) + if (retryCooldown > Duration.ZERO) startBleScanRetryCooldown(retryCooldown) Logger.w(exception) { "BLE scan could not start: ${exception.reason.androidCode} (${exception.reason.description})" @@ -425,16 +468,14 @@ open class ScannerViewModel( retryCooldownSeconds.coerceAtMost(Int.MAX_VALUE.toLong()).toInt(), retryCooldownSeconds, ) + + BleScanStartFailureReason.BluetoothDisabled -> getStringSuspend(Res.string.bluetooth_disabled) + + BleScanStartFailureReason.LocationServicesDisabled -> + getStringSuspend(Res.string.bluetooth_scan_location_services_disabled) } } - .getOrDefault( - if (exception.reason == BleScanStartFailureReason.ScanningTooFrequently) { - val unit = if (retryCooldownSeconds == 1L) "second" else "seconds" - "Bluetooth scan limit reached. Try again in $retryCooldownSeconds $unit." - } else { - BLE_SCAN_START_FAILURE_MESSAGE_FALLBACK - }, - ) + .getOrDefault(untranslatedScanStartFailureMessage(exception.reason, retryCooldownSeconds)) serviceRepository.setErrorMessage(text = errorMessage, severity = Severity.Warn) } diff --git a/feature/connections/src/commonTest/kotlin/org/meshtastic/feature/connections/ScannerViewModelTest.kt b/feature/connections/src/commonTest/kotlin/org/meshtastic/feature/connections/ScannerViewModelTest.kt index 2e222cdc2a..44db8cf3a5 100644 --- a/feature/connections/src/commonTest/kotlin/org/meshtastic/feature/connections/ScannerViewModelTest.kt +++ b/feature/connections/src/commonTest/kotlin/org/meshtastic/feature/connections/ScannerViewModelTest.kt @@ -152,6 +152,53 @@ class ScannerViewModelTest { assertEquals(2, scanAttempts) } + @Test + fun `bluetooth-disabled failure allows an immediate retry once the user re-enables it`() = runTest { + // No cooldown for preconditions the user clears with a system toggle — a dead scan button right after they + // switched Bluetooth back on reads as a broken app. + var scanAttempts = 0 + every { bleScanner.scan(any(), any()) } returns + flow { + scanAttempts += 1 + throw BleScanStartException( + reason = BleScanStartFailureReason.BluetoothDisabled, + cause = IllegalStateException("Bluetooth disabled"), + ) + } + + viewModel.startBleScan() + assertEquals(1, scanAttempts) + assertEquals(false, viewModel.isBleScanning.value) + assertEquals("Bluetooth is off. Turn it on to scan for nearby devices.", serviceRepository.errorMessage.value) + + // Immediately retryable — no waiting on the scheduler. + viewModel.startBleScan() + assertEquals(2, scanAttempts) + } + + @Test + fun `location-services-disabled failure allows an immediate retry`() = runTest { + var scanAttempts = 0 + every { bleScanner.scan(any(), any()) } returns + flow { + scanAttempts += 1 + throw BleScanStartException( + reason = BleScanStartFailureReason.LocationServicesDisabled, + cause = IllegalStateException("Location services are required for scanning but are disabled"), + ) + } + + viewModel.startBleScan() + assertEquals(1, scanAttempts) + assertEquals( + "Location services are off. Turn them on to scan for nearby devices.", + serviceRepository.errorMessage.value, + ) + + viewModel.startBleScan() + assertEquals(2, scanAttempts) + } + @Test fun `scan quota failure honors retry-after cooldown`() = runTest { var scanAttempts = 0