mirror of
https://github.com/meshtastic/Meshtastic-Android.git
synced 2026-09-12 21:30:02 -04:00
fix(logging): stop reporting expected conditions as crashes (#6470)
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
2d63db8dd4
commit
5235b8fc51
16 files changed
+598
-37
No files matched your search
@@ -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.
|
||||
|
||||
|
||||
Generated
+1
@@ -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
|
||||
|
||||
+18
-15
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
@@ -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
|
||||
}
|
||||
@@ -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<BleDevice> {
|
||||
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
|
||||
|
||||
@@ -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 <https://www.gnu.org/licenses/>.
|
||||
*/
|
||||
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() }
|
||||
}
|
||||
}
|
||||
@@ -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) }
|
||||
}
|
||||
|
||||
+107
@@ -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 <https://www.gnu.org/licenses/>.
|
||||
*/
|
||||
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
|
||||
+155
@@ -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 <https://www.gnu.org/licenses/>.
|
||||
*/
|
||||
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())
|
||||
}
|
||||
}
|
||||
+5
-1
@@ -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()
|
||||
|
||||
+32
-1
@@ -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?,
|
||||
|
||||
@@ -149,6 +149,7 @@
|
||||
<string name="bluetooth_feature_discovery">Discovery</string>
|
||||
<string name="bluetooth_feature_discovery_description">Find and identify Meshtastic devices near you.</string>
|
||||
<string name="bluetooth_permission">Bluetooth</string>
|
||||
<string name="bluetooth_scan_location_services_disabled">Location services are off. Turn them on to scan for nearby devices.</string>
|
||||
<string name="bluetooth_scan_missing_permission">Bluetooth scan needs permission. Grant the Nearby devices (or Location) permission to find devices.</string>
|
||||
<string name="bluetooth_scan_start_failed">Bluetooth scan couldn't start. Try again, or toggle Bluetooth if the problem continues.</string>
|
||||
<plurals name="bluetooth_scan_too_frequent">
|
||||
|
||||
+9
-1
@@ -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)
|
||||
}
|
||||
|
||||
+53
-12
@@ -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)
|
||||
}
|
||||
|
||||
|
||||
+47
@@ -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
|
||||
|
||||
Reference in new issue
Block a user