Skip to content

Commit 83962c2

Browse files
committed
fix(firmware): harden release discovery errors
Guard bundled release compare-and-write behind the refresh mutex so an older bundled snapshot cannot race and replace fresher API data. Stop collecting release flows after hardware resolution fails, clear stale firmware metadata on connected and recovery errors, and cover both paths in view-model tests.
1 parent 156c71c commit 83962c2

3 files changed

Lines changed: 59 additions & 20 deletions

File tree

core/data/src/commonMain/kotlin/org/meshtastic/core/data/repository/FirmwareReleaseRepositoryImpl.kt

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -122,14 +122,17 @@ open class FirmwareReleaseRepositoryImpl(
122122

123123
val bundled = bundledSnapshot?.releases ?: return
124124

125-
// Re-evaluate against the current active DB on every call — it may have switched devices.
126-
val toApply =
127-
listOf(FirmwareReleaseType.STABLE to bundled.stable, FirmwareReleaseType.ALPHA to bundled.alpha)
128-
.filter { (type, releases) -> isBundleNewerFor(type, releases) }
129-
.toMap()
130-
if (toApply.isNotEmpty()) {
131-
Logger.i { "FirmwareReleaseRepository: applying bundled snapshot for ${toApply.keys}" }
132-
localDataSource.replaceFirmwareReleases(toApply)
125+
refreshMutex.withLock {
126+
// Re-evaluate against the current active DB under the same lock as network refresh so an older bundled
127+
// snapshot cannot overwrite fresher data that just arrived from the API.
128+
val toApply =
129+
listOf(FirmwareReleaseType.STABLE to bundled.stable, FirmwareReleaseType.ALPHA to bundled.alpha)
130+
.filter { (type, releases) -> isBundleNewerFor(type, releases) }
131+
.toMap()
132+
if (toApply.isNotEmpty()) {
133+
Logger.i { "FirmwareReleaseRepository: applying bundled snapshot for ${toApply.keys}" }
134+
localDataSource.replaceFirmwareReleases(toApply)
135+
}
133136
}
134137
}
135138
}

feature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/FirmwareUpdateViewModel.kt

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -172,22 +172,18 @@ class FirmwareUpdateViewModel(
172172
enterRecoveryModeOrError()
173173
return@launch
174174
}
175+
val deviceHardware = getDeviceHardware(ourNode) ?: return@launch
176+
_deviceHardware.value = deviceHardware
177+
_currentFirmwareVersion.value = ourNode.firmwareVersion
178+
175179
val releaseFlow =
176180
if (_selectedReleaseType.value == FirmwareReleaseType.LOCAL) {
177181
flowOf(null)
178182
} else {
179183
firmwareReleaseRepository.getReleaseFlow(_selectedReleaseType.value)
180184
}
181-
182-
val deviceHardware = getDeviceHardware(ourNode)
183-
if (deviceHardware != null) {
184-
_deviceHardware.value = deviceHardware
185-
_currentFirmwareVersion.value = ourNode.firmwareVersion
186-
}
187-
188185
releaseFlow.collectLatest { release ->
189186
_selectedRelease.value = release
190-
if (deviceHardware == null) return@collectLatest
191187

192188
val dismissed = bootloaderWarningDataSource.isDismissed(address)
193189
val firmwareUpdateMethod =
@@ -247,12 +243,14 @@ class FirmwareUpdateViewModel(
247243
private suspend fun enterRecoveryModeOrError() {
248244
val recovery = firmwareRecoveryDataSource.pending.first()
249245
if (recovery == null) {
246+
clearDeviceMetadata()
250247
_state.value = FirmwareUpdateState.Error(UiText.Resource(Res.string.firmware_update_no_device))
251248
return
252249
}
253250
pendingRecovery = recovery
254251
val hardware =
255252
deviceHardwareRepository.getDeviceHardwareByModel(recovery.hwModel, recovery.pioEnv).getOrElse {
253+
clearDeviceMetadata()
256254
_state.value =
257255
FirmwareUpdateState.Error(
258256
UiText.Resource(Res.string.firmware_update_unknown_hardware, recovery.hwModel),
@@ -587,6 +585,7 @@ class FirmwareUpdateViewModel(
587585
}
588586

589587
private fun clearDeviceMetadata() {
588+
_selectedRelease.value = null
590589
_deviceHardware.value = null
591590
_currentFirmwareVersion.value = null
592591
}

feature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/FirmwareUpdateViewModelTest.kt

Lines changed: 41 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import dev.mokkery.mock
2626
import kotlinx.coroutines.Dispatchers
2727
import kotlinx.coroutines.ExperimentalCoroutinesApi
2828
import kotlinx.coroutines.flow.MutableStateFlow
29+
import kotlinx.coroutines.flow.flow
2930
import kotlinx.coroutines.flow.flowOf
3031
import kotlinx.coroutines.test.StandardTestDispatcher
3132
import kotlinx.coroutines.test.advanceUntilIdle
@@ -44,6 +45,7 @@ import org.meshtastic.core.repository.RadioPrefs
4445
import org.meshtastic.core.resources.Res
4546
import org.meshtastic.core.resources.UiText
4647
import org.meshtastic.core.resources.firmware_update_battery_low
48+
import org.meshtastic.core.resources.firmware_update_unknown_hardware
4749
import org.meshtastic.core.testing.FakeNodeRepository
4850
import org.meshtastic.core.testing.FakeRadioController
4951
import org.meshtastic.core.testing.TestDataFactory
@@ -311,21 +313,23 @@ class FirmwareUpdateViewModelTest {
311313
fun `checkForUpdates sets error when hardware lookup fails`() = runTest {
312314
advanceUntilIdle()
313315
assertIs<FirmwareUpdateState.Ready>(viewModel.state.value)
316+
assertEquals("1.0.0", viewModel.selectedRelease.value?.title)
314317
assertIs<DeviceHardware>(viewModel.deviceHardware.value)
315318
assertEquals("0.9.0", viewModel.currentFirmwareVersion.value)
316319

317-
val release = FirmwareRelease(id = "2", title = "2.0.0", zipUrl = "url", releaseNotes = "notes")
318-
every { firmwareReleaseRepository.stableRelease } returns flowOf(release)
320+
every { firmwareReleaseRepository.stableRelease } returns flow { error("cache read failed") }
319321
everySuspend { deviceHardwareRepository.getDeviceHardwareByModel(any(), any()) } returns
320322
Result.failure(IllegalStateException("Unknown hardware"))
321323

322324
viewModel.checkForUpdates()
323325
advanceUntilIdle()
324326

325-
assertEquals(release, viewModel.selectedRelease.value)
327+
assertEquals(null, viewModel.selectedRelease.value)
326328
assertEquals(null, viewModel.deviceHardware.value)
327329
assertEquals(null, viewModel.currentFirmwareVersion.value)
328-
assertIs<FirmwareUpdateState.Error>(viewModel.state.value)
330+
val errorState = assertIs<FirmwareUpdateState.Error>(viewModel.state.value)
331+
val error = assertIs<UiText.Resource>(errorState.error)
332+
assertEquals(Res.string.firmware_update_unknown_hardware, error.res)
329333
}
330334

331335
@Test
@@ -439,4 +443,37 @@ class FirmwareUpdateViewModelTest {
439443
assertEquals("1234abcd", state.address) // fullAddress.drop(1)
440444
assertIs<FirmwareUpdateMethod.Ble>(state.updateMethod)
441445
}
446+
447+
@Test
448+
fun `recovery hardware lookup failure clears stale device metadata`() = runTest {
449+
advanceUntilIdle()
450+
assertIs<FirmwareUpdateState.Ready>(viewModel.state.value)
451+
assertEquals("1.0.0", viewModel.selectedRelease.value?.title)
452+
assertIs<DeviceHardware>(viewModel.deviceHardware.value)
453+
assertEquals("0.9.0", viewModel.currentFirmwareVersion.value)
454+
455+
every { radioPrefs.devAddr } returns MutableStateFlow(null)
456+
every { firmwareRecoveryDataSource.pending } returns
457+
flowOf(
458+
PendingFirmwareRecovery(
459+
fullAddress = "x1234abcd",
460+
hwModel = 999,
461+
pioEnv = "unknown",
462+
releaseType = "STABLE",
463+
deviceName = "Stale Node",
464+
),
465+
)
466+
everySuspend { deviceHardwareRepository.getDeviceHardwareByModel(any(), any()) } returns
467+
Result.failure(IllegalStateException("Unknown hardware"))
468+
469+
viewModel.checkForUpdates()
470+
advanceUntilIdle()
471+
472+
assertEquals(null, viewModel.selectedRelease.value)
473+
assertEquals(null, viewModel.deviceHardware.value)
474+
assertEquals(null, viewModel.currentFirmwareVersion.value)
475+
val errorState = assertIs<FirmwareUpdateState.Error>(viewModel.state.value)
476+
val error = assertIs<UiText.Resource>(errorState.error)
477+
assertEquals(Res.string.firmware_update_unknown_hardware, error.res)
478+
}
442479
}

0 commit comments

Comments
 (0)