fix(firmware): Keep bundled release discovery available#6082
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughFirmware seeding now caches decoded bundled releases and stops retrying decode failures. Cache reads in stale-while-revalidate are guarded, and firmware update handling now clears device metadata on hardware lookup errors while keeping the update flow aligned with the current device state. ChangesFirmware data and update flow
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
e3c4275 to
419355c
Compare
jeremiah-k
changed the title
![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 (1)
core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeDatabaseProvider.kt (1)
27-42: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMinor:
dbisn't volatile, and the previous DB is closed immediately on switch.
dbis a plainvarread bywithDb()and written byswitchToNewDatabase(); without@Volatilea reader on a different dispatcher/thread than the writer isn't guaranteed to see the update. Also,switchToNewDatabase()closesprevioussynchronously — if any in-flightwithDb()call captured the old instance concurrently, it would keep using (or get errors from) a closed database.Not exercised by the tests in this diff (usage is sequential), but worth hardening if this fake is reused with real dispatchers/concurrent collectors in future tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeDatabaseProvider.kt` around lines 27 - 42, The FakeDatabaseProvider’s shared db state is not safe across threads, and switchToNewDatabase() closes the old database immediately while withDb() may still be using it. Harden FakeDatabaseProvider by making the db field visibly safe for cross-dispatcher reads (or otherwise synchronizing access) and avoid closing the previous database synchronously until you know no in-flight withDb() calls can still reference it. Keep the fix centered around db, withDb(), and switchToNewDatabase() so the fake remains reliable under concurrent test usage.
🤖 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
`@core/data/src/commonMain/kotlin/org/meshtastic/core/data/repository/FirmwareReleaseRepositoryImpl.kt`:
- Around line 105-134: The bundle seeding logic in ensureSeeded() can still
overwrite newer data because it compares with isBundleNewerFor() outside the
same lock used by singleFlightRefresh(). Update the compare-and-write path so
replaceFirmwareReleases() is guarded by refreshMutex as well, or recheck
freshness under that lock immediately before writing. Keep the fix localized to
FirmwareReleaseRepositoryImpl and use the existing mutexes and
localDataSource.replaceFirmwareReleases() path.
---
Nitpick comments:
In
`@core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeDatabaseProvider.kt`:
- Around line 27-42: The FakeDatabaseProvider’s shared db state is not safe
across threads, and switchToNewDatabase() closes the old database immediately
while withDb() may still be using it. Harden FakeDatabaseProvider by making the
db field visibly safe for cross-dispatcher reads (or otherwise synchronizing
access) and avoid closing the previous database synchronously until you know no
in-flight withDb() calls can still reference it. Keep the fix centered around
db, withDb(), and switchToNewDatabase() so the fake remains reliable under
concurrent test usage.
🪄 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: 7bc7b5e1-ad73-4ab4-b7ec-54fd016231fc
📒 Files selected for processing (5)
core/data/src/commonMain/kotlin/org/meshtastic/core/data/repository/FirmwareReleaseRepositoryImpl.ktcore/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/FirmwareReleaseRepositoryImplTest.ktcore/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeDatabaseProvider.ktfeature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/FirmwareUpdateViewModel.ktfeature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/FirmwareUpdateViewModelTest.kt
419355c to
c9a1ef8
Compare
c9a1ef8 to
faa5edb
Compare
jamesarich
left a comment
jamesarich
left a comment
There was a problem hiding this comment.
Re-reviewed after the force-push. The re-seed fix is solid — bundled releases now re-seed when the active device DB switches (good test), and the bundleDecodeFailed latch still prevents per-collection retry. Three carryovers inline; none are hard blockers, but the first two are worth tightening before merge.
| } else { | ||
| FirmwareUpdateMethod.Usb | ||
| } | ||
| releaseFlow.collectLatest { release -> |
There was a problem hiding this comment.
The description says release collection now starts before hardware resolution, but releaseFlow is a cold flow still collected here — after getDeviceHardware() returns at line 180, in the same coroutine. During a slow api.meshtastic.org hardware lookup the release state is still not populated any earlier. The real fix is the null-guard restructure below; the "collect before resolution" framing (and the construction reorder) is inert — worth correcting the PR body so it doesn't imply a latency win that isn't there.
jamesarich
left a comment
jamesarich
left a comment
There was a problem hiding this comment.
Re-reviewed at 4f9ccd6 — two of the three are fixed:
- ✅ Recovery path clears stale metadata (
clearDeviceMetadata()in both branches ofenterRecoveryModeOrError), withrecovery hardware lookup failure clears stale device metadata. - ✅ Specific unknown-hardware Error no longer clobbered — on hardware-lookup failure the connected path now
return@launches before collecting the release flow, so a throwing cache read can't overwrite it (the new test stubs the cache to throw and asserts the specific error survives).
Two follow-ups:
- PR body now contradicts the code. The
harden release discovery errorscommit stops collecting the release flow when hardware resolution fails (getDeviceHardware(ourNode) ?: return@launch) — the opposite of the Key Changes line "Ensured release state is still collected when hardware lookup fails." And neither head ever started collection before hardware resolution (it's a cold flow collected in the same coroutine). Please update the description to match what the code does. - Same clobber survives on the recovery Ready path. The fix short-circuited only the connected hardware-failure path. After a successful recovery lookup,
getReleaseFlow(type).collectLateststill runs inside the outersafeCatching, andloadFromCache()inStaleWhileRevalidateFlowis still unguarded (onlyfetchis), so a throwing cache read there overwrites the recovery Ready state with a genericDynamicString(e.message). GuardingloadFromCacheat the source would cover both paths.
5f32e79 to
272f328
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@core/data/src/commonMain/kotlin/org/meshtastic/core/data/util/StaleWhileRevalidateFlow.kt`:
- Around line 84-90: The reload path in StaleWhileRevalidateFlow should not emit
null when loadFromCache() fails after a successful fetch. Update the logic
around safeCatching { loadFromCache() } and the subsequent fresh/cached
comparison so that a reload failure falls back to the previous cached value
instead of getOrNull() producing null. Keep the existing logging in the cache
reload block, and make sure the emit call only publishes a real updated value or
preserves the prior cached state.
🪄 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: 4a32c272-5c79-48b7-8f1c-e22be8fec774
📒 Files selected for processing (4)
core/data/src/commonMain/kotlin/org/meshtastic/core/data/repository/FirmwareReleaseRepositoryImpl.ktcore/data/src/commonMain/kotlin/org/meshtastic/core/data/util/StaleWhileRevalidateFlow.ktcore/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/FirmwareReleaseRepositoryImplTest.ktcore/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeDatabaseProvider.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeDatabaseProvider.kt
- core/data/src/commonMain/kotlin/org/meshtastic/core/data/repository/FirmwareReleaseRepositoryImpl.kt
- core/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/FirmwareReleaseRepositoryImplTest.kt
Mark the bundled firmware release seed as checked only after the bundled snapshot path completes successfully. A failed asset decode or local cache update now leaves the seed eligible for a later collection retry instead of suppressing bundled fallback for the rest of the process. Start collecting the selected release flow before resolving device hardware so release cache seeding and selected-release state are not skipped by an unknown hardware lookup. FirmwareUpdateState.Ready remains gated on resolved hardware so update actions are still blocked when the device context is not known. Add focused coverage for retrying a failed bundled seed and for preserving selected release state when hardware lookup fails.
Resolve device hardware once per update refresh before collecting release emissions, keeping release discovery independent from hardware lookup while avoiding repeated repository lookups for each release emission. Clear cached device hardware and firmware version when hardware resolution fails so the firmware screen does not keep displaying stale device metadata beside an error state. Extend the hardware-failure coverage to start from a successful Ready state, then verify selected release preservation, metadata clearing, and the final error state after a failed refresh.
The process-wide seedChecked flag assumed one successful seed meant every Room database was populated. But the active database switches per selected device, so the first seed could populate the default DB and skip the device-specific DB entirely. Replace seedChecked with a cached decoded snapshot (bundledSnapshot) that avoids re-reading the asset file, but re-evaluates the apply/skip decision against the current active database on every collection. When the device DB switches and is empty, the bundle re-applies. Also removes temporary diagnostic logging.
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.
Wrap both loadFromCache calls in staleWhileRevalidateFlow with safeCatching so a cache-read failure logs and returns null instead of throwing into the caller. This prevents a DB error during release cache reads from clobbering a recovery Ready state with a generic error on the recovery path.
…ailure
A transient loadFromCache() failure after a successful fetch was
producing null via getOrNull(), which then emitted null downstream and
wiped valid cached state for consumers. Use getOrElse { cached } so a
reload failure preserves the prior cached value instead.
272f328 to
d019ae0
Compare
|
@jamesarich I pushed a refreshed branch with the release-state cleanup tightened. The description no longer claims release collection starts before hardware resolution. The code now keeps bundled seeding DB-aware, clears stale metadata on hardware lookup failures, and guards cache reads in the stale-while-revalidate flow so a transient cache/DB read failure does not overwrite the more specific firmware update state. |
jamesarich
left a comment
jamesarich
left a comment
There was a problem hiding this comment.
Traced the core logic (no deadlock / correct temp-file lifecycle & validation respectively) — LGTM. Details in review pass.
2 participants
Overview
This PR fixes firmware release discovery when the app needs bundled release metadata.
F-Droid builds do not use the Meshtastic firmware release API, so bundled release metadata needs to remain available for the firmware update screen. The app also uses per-device databases, which means a bundled release snapshot seeded into one active database is not enough for later selected-device databases.
This change keeps bundled release discovery available for the current active database while still preventing unknown hardware from becoming flashable.
Key Changes
Testing
git diff --checkpasses.Migration Notes
Summary by CodeRabbit