fix: prevent MMKV/AsyncStorage split-brain in podcast cache and reading progress (#1953)#2090
Open
vedant7007 wants to merge 1 commit into
Open
fix: prevent MMKV/AsyncStorage split-brain in podcast cache and reading progress (#1953)#2090vedant7007 wants to merge 1 commit into
vedant7007 wants to merge 1 commit into
Conversation
…ng progress (SB2318#1953) when mmkv writes throw, the previous code silently fell through to AsyncStorage but subsequent reads still went to MMKV first — returning stale data and causing state to appear to revert. changes to MMKVUtils.ts: - track a session-level unhealthy flag per store; once mmkv write throws we invalidate the stale mmkv key and route all further reads/writes to AsyncStorage - read side now cross-checks: if mmkv returns nothing but AsyncStorage has data (from a prior failed-write fallback), we use the AsyncStorage value instead of falsely reporting empty - reading progress functions previously called mmkv.set with no try/catch — an mmkv throw would crash the caller. now wrapped, with AsyncStorage fallback under a namespaced prefix so reads can find fallback values - deleteItem and clearMMKV now always attempt AsyncStorage cleanup even if mmkv throws, so a partial failure cannot leave stale data behind - console.log guarded with __DEV__ so error details dont surface in production added MMKVUtils.test.ts covering: - healthy read/write path - write-throws → invalidation + AsyncStorage fallback + subsequent reads return fresh data - mmkv empty but AsyncStorage has data → returns AsyncStorage data - mmkv read throws → falls back to AsyncStorage - reading progress previously-uncaught throw no longer propagates - delete paths hit both stores - module-unavailable path uses AsyncStorage directly
Contributor
|
Thank you @, for creating the PR and contributing to our UltimateHealth project 💗. |
Contributor
Automated Review FeedbackNo major issues were identified during this review. The implementation appears consistent with the repository standards and the modified files were reviewed successfully.
|
Contributor
Author
|
hey @SB2318 heads up on CI:
happy to help debug the lockfile in a separate PR if that's useful. thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #1953
the bug
in MMKVUtils.ts, when an mmkv write threw, the catch block just logged and fell through to AsyncStorage. but on the very next read the code preferred mmkv → returned stale data. so a user could add or remove a downloaded podcast, see it succeed in the UI, restart the app, and the change would silently disappear.
reading progress had a related issue:
setReadingProgressItemcalledmmkv?.set()with no try/catch at all. if mmkv threw, the exception propagated up intosaveProgress→ uncaught.what changed
MMKVUtils.ts
podcastMMKVUnhealthy,readingProgressMMKVUnhealthy). once we detect an mmkv write failure we:setReadingProgressItem,getReadingProgressItem,deleteReadingProgressItemin try/catch. reading progress fallbacks go to AsyncStorage under areading_progress:prefix so we dont collide with anything else in AsyncStoragedeleteItemandclearMMKVnow always attempt AsyncStorage cleanup even if mmkv throws, so a partial failure cant leave stale dataconsole.*with__DEV__— no more log leakage in production buildsMMKVUtils.test.ts (new)
covers the split-brain scenarios end-to-end:
testing done
mmkv.setthrow via the mock — retrieval now returns the just-written data instead of stale mmkv dataReadingProgressService.test.tsstill passes since the public api signatures didnt changechecklist