[FCM] Recovery logic for a corrupt database#15573
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
7e4798d to
3cd2386
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces important recovery logic for a corrupt database, which is a great improvement. The implementation in FIRMessagingRmqManager.m looks mostly good, but I have a few suggestions for improvement regarding error handling and code structure. Additionally, I have a question about the unit test testInitWhenDatabaseIsBrokenThenDatabaseIsDeleted. This test seems to expect an assertion and that the database file is ultimately deleted, which appears to contradict the goal of successfully recovering and recreating the database. Perhaps I'm misunderstanding the test's scenario, and I'd appreciate some clarification.
|
The QS failures are a tooling issue. I think it has to do with when a PR gets force pushed to. Investigating a fix for that. It should be non-blocking for this PR. |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
| "Will delete and try to recreate it.", | ||
| path); | ||
| NSError *removeError; | ||
| if (![[NSFileManager defaultManager] removeItemAtPath:path error:&removeError]) { |
There was a problem hiding this comment.
Do we know the exact error code SQLite returns when the database is corrupt? Deleting the database for all error codes seems a bit extreme.
There was a problem hiding this comment.
According to #14880 , the error code is 14
Could not create RMQ database at path /var/mobile/Containers/Data/Application/372D4296-3F35-46FD-B3C9-0A20877B8378/Library/Application Support/Google/FirebaseMessaging/rmq2.sqlite, error: 14 - unable to open database file
How about updating the code to only delete the file if the result is SQLITE_CANTOPEN?
https://sqlite.org/rescode.html
(14) SQLITE_CANTOPEN
The SQLITE_CANTOPEN result code indicates that SQLite was unable to open a file. The file in question might be a primary database file or one of several temporary disk files.
There was a problem hiding this comment.
Sorry about the quick merge. Addressing feedback in #15678
This commit is the result of the following commands: flutter pub upgrade --major-versions firebase_messaging firebase_core pod update --project-directory=ios/ pod update --project-directory=macos/ Changelogs: https://pub.dev/packages/firebase_core/changelog#440 https://pub.dev/packages/firebase_messaging/changelog#1611 package:firebase_messaging on iOS, adds support for projects migrated to use SceneDelegate. Another change is a fix for duplicate notifications sent to the onMessage handlers, but we do not use those on iOS so we are unaffected by it. Other notable changes include bump to Firebase Android BoM (34.4.0 to 34.7.0) and Firebase iOS SDK (12.4.0 to 12.8.0), changelog for those are at: https://firebase.google.com/support/release-notes/android https://firebase.google.com/support/release-notes/ios For Android SDK, no notable changes in the FCM component. For iOS SDK, there was one bug fix for a potential crash due to a race condition with Realm DB: firebase/firebase-ios-sdk#14880 (comment) but we do not use Realm DB so it doesn't affect us. But either way the fix involves recovering from a corrupted database rather than a crash so it is a welcome change: firebase/firebase-ios-sdk#15573
This commit is the result of the following commands: flutter pub upgrade --major-versions firebase_messaging firebase_core pod update --project-directory=ios/ pod update --project-directory=macos/ Changelogs: https://pub.dev/packages/firebase_core/changelog#440 https://pub.dev/packages/firebase_messaging/changelog#1611 package:firebase_messaging on iOS, adds support for projects migrated to use SceneDelegate. Another change is a fix for duplicate notifications sent to the onMessage handlers, but we do not use those on iOS so we are unaffected by it. Other notable changes include bump to Firebase Android BoM (34.4.0 to 34.7.0) and Firebase iOS SDK (12.4.0 to 12.8.0), changelog for those are at: https://firebase.google.com/support/release-notes/android https://firebase.google.com/support/release-notes/ios For Android SDK, no notable changes in the FCM component. For iOS SDK, there was one bug fix for a potential crash due to a race condition with Realm DB: firebase/firebase-ios-sdk#14880 (comment) but we do not use Realm DB so it doesn't affect us. But either way the fix involves recovering from a corrupted database rather than a crash so it is a welcome change: firebase/firebase-ios-sdk#15573
This commit is the result of the following commands: flutter pub upgrade --major-versions firebase_messaging firebase_core pod update --project-directory=ios/ pod update --project-directory=macos/ Changelogs: https://pub.dev/packages/firebase_core/changelog#440 https://pub.dev/packages/firebase_messaging/changelog#1611 package:firebase_messaging on iOS, adds support for projects migrated to use SceneDelegate. Another change is a fix for duplicate notifications sent to the onMessage handlers, but we do not use those on iOS so we are unaffected by it. Other notable changes include bump to Firebase Android BoM (34.4.0 to 34.7.0) and Firebase iOS SDK (12.4.0 to 12.8.0), changelog for those are at: https://firebase.google.com/support/release-notes/android https://firebase.google.com/support/release-notes/ios For Android SDK, no notable changes in the FCM component. For iOS SDK, there was one bug fix for a potential crash due to a race condition with Realm DB: firebase/firebase-ios-sdk#14880 (comment) but we do not use Realm DB so it doesn't affect us. But either way the fix involves recovering from a corrupted database rather than a crash so it is a welcome change: firebase/firebase-ios-sdk#15573
This commit is the result of the following commands: flutter pub upgrade --major-versions firebase_messaging firebase_core pod update --project-directory=ios/ pod update --project-directory=macos/ Changelogs: https://pub.dev/packages/firebase_core/changelog#440 https://pub.dev/packages/firebase_messaging/changelog#1611 package:firebase_messaging on iOS, adds support for projects migrated to use SceneDelegate. Another change is a fix for duplicate notifications sent to the onMessage handlers, but we do not use those on iOS so we are unaffected by it. Other notable changes include bump to Firebase Android BoM (34.4.0 to 34.7.0) and Firebase iOS SDK (12.4.0 to 12.8.0), changelog for those are at: https://firebase.google.com/support/release-notes/android https://firebase.google.com/support/release-notes/ios For Android SDK, no notable changes in the FCM component. For iOS SDK, there was one bug fix for a potential crash due to a race condition with Realm DB: firebase/firebase-ios-sdk#14880 (comment) but we do not use Realm DB so it doesn't affect us. But either way the fix involves recovering from a corrupted database rather than a crash so it is a welcome change: firebase/firebase-ios-sdk#15573
This commit is the result of the following commands: flutter pub upgrade --major-versions firebase_messaging firebase_core pod update --project-directory=ios/ pod update --project-directory=macos/ Changelogs: https://pub.dev/packages/firebase_core/changelog#440 https://pub.dev/packages/firebase_messaging/changelog#1611 package:firebase_messaging on iOS, adds support for projects migrated to use SceneDelegate. Another change is a fix for duplicate notifications sent to the onMessage handlers, but we do not use those on iOS so we are unaffected by it. Other notable changes include bump to Firebase Android BoM (34.4.0 to 34.7.0) and Firebase iOS SDK (12.4.0 to 12.8.0), changelog for those are at: https://firebase.google.com/support/release-notes/android https://firebase.google.com/support/release-notes/ios For Android SDK, no notable changes in the FCM component. For iOS SDK, there was one bug fix for a potential crash due to a race condition with Realm DB: firebase/firebase-ios-sdk#14880 (comment) but we do not use Realm DB so it doesn't affect us. But either way the fix involves recovering from a corrupted database rather than a crash so it is a welcome change: firebase/firebase-ios-sdk#15573
This commit is the result of the following commands: flutter pub upgrade --major-versions firebase_messaging firebase_core pod update --project-directory=ios/ pod update --project-directory=macos/ Changelogs: https://pub.dev/packages/firebase_core/changelog#440 https://pub.dev/packages/firebase_messaging/changelog#1611 package:firebase_messaging on iOS, adds support for projects migrated to use SceneDelegate. Another change is a fix for duplicate notifications sent to the onMessage handlers, but we do not use those on iOS so we are unaffected by it. Other notable changes include bump to Firebase Android BoM (34.4.0 to 34.7.0) and Firebase iOS SDK (12.4.0 to 12.8.0), changelog for those are at: https://firebase.google.com/support/release-notes/android https://firebase.google.com/support/release-notes/ios For Android SDK, no notable changes in the FCM component. For iOS SDK, there was one bug fix for a potential crash due to a race condition with Realm DB: firebase/firebase-ios-sdk#14880 (comment) but we do not use Realm DB so it doesn't affect us. But either way the fix involves recovering from a corrupted database rather than a crash so it is a welcome change: firebase/firebase-ios-sdk#15573
This commit is the result of the following commands: flutter pub upgrade --major-versions firebase_messaging firebase_core pod update --project-directory=ios/ pod update --project-directory=macos/ Changelogs: https://pub.dev/packages/firebase_core/changelog#440 https://pub.dev/packages/firebase_messaging/changelog#1611 package:firebase_messaging on iOS, adds support for projects migrated to use SceneDelegate. Another change is a fix for duplicate notifications sent to the onMessage handlers, but we do not use those on iOS so we are unaffected by it. Other notable changes include bump to Firebase Android BoM (34.4.0 to 34.7.0) and Firebase iOS SDK (12.4.0 to 12.8.0), changelog for those are at: https://firebase.google.com/support/release-notes/android https://firebase.google.com/support/release-notes/ios For Android SDK, no notable changes in the FCM component. For iOS SDK, there was one bug fix for a potential crash due to a race condition with Realm DB: firebase/firebase-ios-sdk#14880 (comment) but we do not use Realm DB so it doesn't affect us. But either way the fix involves recovering from a corrupted database rather than a crash so it is a welcome change: firebase/firebase-ios-sdk#15573
This commit is the result of the following commands: flutter pub upgrade --major-versions firebase_messaging firebase_core pod update --project-directory=ios/ pod update --project-directory=macos/ Changelogs: https://pub.dev/packages/firebase_core/changelog#440 https://pub.dev/packages/firebase_messaging/changelog#1611 package:firebase_messaging on iOS, adds support for projects migrated to use SceneDelegate. Another change is a fix for duplicate notifications sent to the onMessage handlers, but we do not use those on iOS so we are unaffected by it. Other notable changes include bump to Firebase Android BoM (34.4.0 to 34.7.0) and Firebase iOS SDK (12.4.0 to 12.8.0), changelog for those are at: https://firebase.google.com/support/release-notes/android https://firebase.google.com/support/release-notes/ios For Android SDK, no notable changes in the FCM component. For iOS SDK, there was one bug fix for a potential crash due to a race condition with Realm DB: firebase/firebase-ios-sdk#14880 (comment) but we do not use Realm DB so it doesn't affect us. But either way the fix involves recovering from a corrupted database rather than a crash so it is a welcome change: firebase/firebase-ios-sdk#15573
Fix #14880. More context in discussion at #15559