fix(dfu): Detect legacy bootloaders before secure fallback#6079
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughSecureDfuHandler now detects bootloader protocol with an explicit Unknown outcome, routes uploads through DfuFallbackCoordinator, and returns DFU result objects from upload and connection paths. Added tests cover protocol ordering, fallback gating, and session-attempt behavior. ChangesDFU Fallback and Detection
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
feature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/dfu/DfuFallbackCoordinatorTest.kt (2)
51-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMisleading test name for the assertion made.
Test name says "Unknown tries Legacy then Secure" but the body only verifies Legacy is tried first (success on first call short-circuits fallback), never exercising the Secure path. Consider renaming to something like
Unknown tries Legacy first.🤖 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 `@feature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/dfu/DfuFallbackCoordinatorTest.kt` around lines 51 - 60, The test name in DfuFallbackCoordinatorTest does not match what the assertion actually verifies. Update the test case around DfuFallbackCoordinator.execute so the name reflects the behavior being checked, since it only confirms that BootloaderDetection.Unknown starts with DfuProtocolKind.LEGACY and does not reach Secure on success. Rename the test to something like “Unknown tries Legacy first” to align with the single-path assertion.
63-77: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing coverage for suppressed prior-error chaining.
Per the coordinator's implementation (
priorError?.let { result.error.addSuppressed(it) }), when both protocols fail before engagement, the first error should be attached as a suppressed exception on the final thrown error. None of these fallback tests assert onsuppressedExceptions, so a regression in that chaining wouldn't be caught.✅ Example additional assertion
assertFailsWith<RuntimeException> { coordinator.execute { protocol, _ -> protocols.add(protocol) if (protocol == DfuProtocolKind.LEGACY) { DfuUploadResult.Failure(RuntimeException("connect failed"), protocolEngaged = false) } else { DfuUploadResult.Failure(RuntimeException("also failed"), protocolEngaged = false) } } - } + }.also { thrown -> + assertEquals(1, thrown.suppressedExceptions.size) + assertEquals("connect failed", thrown.suppressedExceptions.first().message) + }Also applies to: 115-129
🤖 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 `@feature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/dfu/DfuFallbackCoordinatorTest.kt` around lines 63 - 77, The fallback tests for DfuFallbackCoordinator do not verify suppressed-error chaining, so add assertions that the final thrown error from execute includes the first failure as a suppressed exception when both protocol attempts fail before engagement. Update the affected test cases in DfuFallbackCoordinatorTest to inspect the thrown RuntimeException’s suppressedExceptions and confirm it contains the earlier error, matching the coordinator’s priorError?.let { result.error.addSuppressed(it) } behavior.
🤖 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.
Inline comments:
In
`@feature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/dfu/SecureDfuHandler.kt`:
- Around line 167-172: sessionAttemptsFor() is not giving the Legacy primary
flow the larger retry budget when detection is Unknown, so the reset-prime
recovery path in runDfuUploadWithRetry() never gets a chance to run. Update the
retry selection in SecureDfuHandler so the Legacy primary case uses
LEGACY_SESSION_ATTEMPTS not only for BootloaderDetection.LegacyObserved but also
for the Unknown Legacy path, while keeping non-primary and non-Legacy cases on
LIMITED_SESSION_ATTEMPTS.
---
Nitpick comments:
In
`@feature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/dfu/DfuFallbackCoordinatorTest.kt`:
- Around line 51-60: The test name in DfuFallbackCoordinatorTest does not match
what the assertion actually verifies. Update the test case around
DfuFallbackCoordinator.execute so the name reflects the behavior being checked,
since it only confirms that BootloaderDetection.Unknown starts with
DfuProtocolKind.LEGACY and does not reach Secure on success. Rename the test to
something like “Unknown tries Legacy first” to align with the single-path
assertion.
- Around line 63-77: The fallback tests for DfuFallbackCoordinator do not verify
suppressed-error chaining, so add assertions that the final thrown error from
execute includes the first failure as a suppressed exception when both protocol
attempts fail before engagement. Update the affected test cases in
DfuFallbackCoordinatorTest to inspect the thrown RuntimeException’s
suppressedExceptions and confirm it contains the earlier error, matching the
coordinator’s priorError?.let { result.error.addSuppressed(it) } behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b5aa7426-d425-4564-9ab8-8df489847ffa
📒 Files selected for processing (2)
feature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/dfu/SecureDfuHandler.ktfeature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/dfu/DfuFallbackCoordinatorTest.kt
f528444 to
3336489
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
feature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/dfu/DfuFallbackCoordinatorTest.kt (1)
96-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: add coverage for the fallback/alternate protocol's session-attempt budget.
Existing tests confirm
LEGACY_SESSION_ATTEMPTS(3) for the Legacy primary case, but no test assertsLIMITED_SESSION_ATTEMPTS(1) for an alternate/fallback protocol attempt (e.g., Secure as fallback underLegacyObserved, or Legacy as fallback underSecureObserved). Not required, but would close a small coverage gap in the retry-budget contract.🤖 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 `@feature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/dfu/DfuFallbackCoordinatorTest.kt` around lines 96 - 116, Add a test that verifies the alternate/fallback protocol uses the limited session-attempt budget in DfuFallbackCoordinator.execute. The current DfuFallbackCoordinatorTest only covers LEGACY_SESSION_ATTEMPTS for LegacyObserved and Unknown, so extend it with a case for a fallback path such as Secure under LegacyObserved or Legacy under SecureObserved, and assert the attempts value is LIMITED_SESSION_ATTEMPTS (1). Use the existing DfuFallbackCoordinator and BootloaderDetection setup to locate the retry-budget behavior.
🤖 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.
Nitpick comments:
In
`@feature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/dfu/DfuFallbackCoordinatorTest.kt`:
- Around line 96-116: Add a test that verifies the alternate/fallback protocol
uses the limited session-attempt budget in DfuFallbackCoordinator.execute. The
current DfuFallbackCoordinatorTest only covers LEGACY_SESSION_ATTEMPTS for
LegacyObserved and Unknown, so extend it with a case for a fallback path such as
Secure under LegacyObserved or Legacy under SecureObserved, and assert the
attempts value is LIMITED_SESSION_ATTEMPTS (1). Use the existing
DfuFallbackCoordinator and BootloaderDetection setup to locate the retry-budget
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c044a72f-7c7a-46ae-844b-e1beebc417d4
📒 Files selected for processing (2)
feature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/dfu/SecureDfuHandler.ktfeature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/ota/dfu/DfuFallbackCoordinatorTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/ota/dfu/SecureDfuHandler.kt
3336489 to
83a2d17
Compare
jamesarich
left a comment
jamesarich
left a comment
There was a problem hiding this comment.
Re-reviewed after the force-push. Two of the bigger items are addressed — nice:
- ✅ Unknown detection now grants the Legacy primary the full 3-attempt budget, so the stale-session reset-prime path can actually run and a wedged legacy bootloader with a missed 1530 ad recovers instead of hard-failing.
- ✅
DfuFallbackCoordinatorTestnow covers the LegacyObserved→Secure ordering, per-leg budgets (listOf(3, 1)), and the suppressed-error shape.
A couple of things still stand (inline), plus a latency note: the 3-attempt fix means an Unknown-detected Secure device (FE59 ad missed) now burns 3× the doomed Legacy connect cycle before the Secure leg is tried — acceptable, but worth a comment so it's a deliberate choice. Not blocking.
| is DfuUploadResult.Failure -> { | ||
| if (result.protocolEngaged || !hasAlternateProtocol) { | ||
| priorError?.let { result.error.addSuppressed(it) } | ||
| throw result.error |
There was a problem hiding this comment.
On exhaustion the thrown error is the alternate leg's, with the primary only addSuppressed — and the UI renders e.message. So a legacy device that fails both legs shows "Failed to connect… via SECURE after 4 attempts", naming the protocol it doesn't speak. Consider throwing the primary (detection-matched) leg's error and suppressing the alternate, or composing a message that names both.
| * is the alternate, attempted only if the primary fails before protocol engagement. | ||
| */ | ||
| private fun BootloaderDetection.orderedProtocols(): List<DfuProtocolKind> = when (this) { | ||
| BootloaderDetection.LegacyObserved -> listOf(DfuProtocolKind.LEGACY, DfuProtocolKind.SECURE) |
There was a problem hiding this comment.
Conclusive detections still carry the opposite protocol as a fallback leg, so a pre-engagement connect flake on the primary triggers a full doomed alternate pass (up to ~140s of scans+retries against a service the device never advertises). The insurance is reasonable for a wrong "conclusive" call, but gating the alternate leg on "primary failed after engaging its DFU service" (vs. never connected) would avoid the worst-case stall on a simple flake.
8c70ff8 to
0ef3798
Compare
|
@jamesarich I pushed a refreshed branch with the fallback behavior tightened. Conclusive Legacy/Secure detections now only try the alternate protocol after the primary protocol has connected, and the surfaced error stays tied to the primary/detection-matched failure. The Unknown path still goes Legacy-first with the full Legacy retry budget so the stale-session reset case remains covered. |
jamesarich
left a comment
jamesarich
left a comment
There was a problem hiding this comment.
Reviewed the fallback state machine closely — the core safety is solid: wrong-protocol attempts fail cleanly at the scan step (each transport scan-filters on its own service UUID, so a Legacy attempt never writes opcodes to a Secure bootloader), the protocolEngaged gate has no suspension-point window where a live session could be torn down, and the NonCancellable cleanup can't leak a half-open transport. Nice.
One reliability concern before this goes in — the Unknown retry budget:
sessionAttemptsFor gives Unknown → Legacy the full LEGACY_SESSION_ATTEMPTS (3) budget, and orderedProtocols puts Legacy first for Unknown. So a genuinely Secure-only device that momentarily reads Unknown (exactly the transient causes your own KDoc lists — scan duty-cycling, adv cache, bootloader not yet re-advertising after the reboot wait) burns 3 doomed Legacy sessions before it ever tries Secure. Each is a connectWithRetry (4 attempts) scanning ~10s for the 1530 service that isn't there — order of minutes of "connecting" with the phone never touching the device. By the time Secure is tried once, the Secure bootloader may well have hit its DFU-inactivity timeout and rebooted back to app, so the flash fails — where the old Unknown→Secure path completed it immediately.
Suggest dropping Unknown→Legacy to LIMITED_SESSION_ATTEMPTS (1) so fallback to Secure happens in tens of seconds instead of minutes. Legacy-first ordering for Unknown is reasonable; it's just the 3× budget on the speculative primary that's expensive on the ambiguous case this PR is meant to help.
Minor: the per-attempt Logger.w { ... } dropped the throwable arg, so retry-loop failures now log only the exception class name, not the stack — a bit harder to field-diagnose. Terminal error is still preserved in the final Failure, so not a blocker.
Replace the brittle single-scan Legacy DFU detection (which treated a missed scan as proof of Secure DFU) with a resilient detection and fallback architecture: - Add BootloaderDetection sealed result type (LegacyObserved, SecureObserved, Unknown) so the inconclusive case stays representable instead of being silently coerced to Secure. - Scan for Legacy DFU service first; if observed, return immediately without scanning for Secure DFU (Legacy wins per orderedProtocols). - Add runDfuUploadWithFallback wrapper that resolves a detection into an ordered protocol list (Legacy first for Unknown) and tries each in turn, falling back to the alternate only on pre-transfer failures. - Guard fallback with TransferProgression flag set immediately before firmware transfer; once streaming begins, failures remain protocol-specific and throw immediately with no fallback. - Reworked fallback loop so the final throw is reachable for last-protocol pre-engagement failures. - Thread protocol kind into connectWithRetry and log the scanned service UUID per connect attempt. - Log detection outcome, fallback decisions, and address-set type using privacy-safe redacted forms (no raw BLE MAC addresses). Preserves: bond-preservation contract, Legacy stale-session (INVALID_STATE) reset handling, per-session NonCancellable GATT cleanup, and Legacy retry semantics (fresh transport + reconnect per retry).
Move DFU protocol ordering and session retry budget selection into a fallback coordinator so SecureDfuHandler delegates protocol fallback decisions instead of owning them directly. Replace the mutable transfer progression flag with an explicit upload result that reports whether a protocol session was engaged before a failure. This keeps alternate-protocol fallback limited to pre-engagement failures while preserving Legacy retry handling, stale-session reset priming, and NonCancellable transport cleanup.
Keep Unknown detection fallback behavior, but stop conclusive Legacy/Secure detections from spending a full alternate-protocol pass when the observed primary service never engages. When both protocol legs fail, throw the primary detection-matched error and attach the alternate failure as suppressed so the UI reports the relevant protocol.
Unknown detection still tries Legacy first, but using the full Legacy retry budget can burn minutes on a Secure-only bootloader before Secure gets a chance to connect. Keep confirmed Legacy on the reset-prime session budget, limit speculative Unknown Legacy probes to one session, and restore throwable logging in retry warnings for field diagnostics.
0ef3798 to
7e7448a
Compare
jamesarich
left a comment
jamesarich
left a comment
There was a problem hiding this comment.
Re-reviewed after the fallback-budget commits. Both points addressed: Unknown→Legacy now bounded to a single speculative probe (1 attempt) before Secure fallback — new Unknown gives Legacy primary 1 session attempt test pins it — so a mis-detected Secure device no longer burns minutes of doomed Legacy scans. Per-attempt session log restored to Logger.w(result.error). Wrong-protocol scan-filter safety, the protocolEngaged gate, and NonCancellable cleanup were already sound. LGTM.
2 participants
Overview
This PR makes BLE DFU protocol detection more reliable when a device reboots into a Legacy DFU bootloader.
Previously, if the app missed the Legacy DFU service during the initial scan, the update flow could continue down the Secure DFU path and keep retrying the wrong service. That made OTA updates fail even though the device was advertising a valid bootloader protocol.
This change keeps bootloader detection explicit, preserves inconclusive scan results, and only falls back to the alternate protocol before a DFU session has actually connected.
Key Changes
Testing
git diff --checkpasses.Migration Notes