fix_: bad node config migration #21952
Conversation
Jenkins BuildsClick to see older builds (8)
|
qfrank
force-pushed
the
fix/node_config_migration
branch
from
1f2e11e to
70768b6
Compare
There was a problem hiding this comment.
@qfrank, I followed the steps from this PR description in a real device with Android 13. I got locked out as expected and this PR did fix the migration bug!
In order to test this PR I had to manually generate a release build for Android https://ci.infra.status.im/job/status-mobile/job/platforms/job/android/2084/ because I installed 1.10.1 and 2.30.0 using release builds, so the upgrade had to be a release build.
I'm wondering about the safest way for @3esmit to test the release build in a way that doesn't risk making things worse for him. Probably the database mutation caused by the call to SaveConfigWithTx is fine if he needs to downgrade to his current build after testing this PR's release build, correct?
qfrank
force-pushed
the
fix/node_config_migration
branch
from
70768b6 to
0531a80
Compare
qfrank
force-pushed
the
fix/node_config_migration
branch
from
0531a80 to
37ef6f3
Compare
|
@qfrank Thank you for this PR, great work! While waiting for reviews and approvals of both this PR and the corresponding go PR, I’ve started some initial testing. I’ve identified that the latest Status v1 version resulting in the issue after upgrading to Status v2 is v1.18.0. The issue does not appear to reproduce when upgrading from versions >v1.18.0, though this doesn’t entirely rule out the possibility, as we’ve previously seen it occur after an upgrade from v1.20.6. However, those cases were random, and we don’t yet have clear steps to reproduce them. In contrast, upgrading from versions ≤v1.18.0 consistently leads to the issue after upgrading to Status v2 (100% reproducibility). Interestingly, if a user previously upgraded from v1.18.0 or earlier to v1.19.0 or later, they can successfully log in after upgrading to any Status v2 version (e.g., v2.30, v2.31, or v2.32). In such cases, the issue does not reproduce. Based on current results, it looks like this PR resolves the issue. Below is a table summarizing the upgrade flows I’ve tested so far:
@qfrank Please ping me once these PRs receive approval, and I’ll proceed with further testing. Ideally, both the go and mobile PRs should have at least two approving reviews before moving forward. cc @ilmotta |
|
Awesome work @pavloburykh. Interesting findings! I approved the PR now. Our original plan with @qfrank was to provide a release build to @3esmit without having to necessarily force him to wait until 2.33 goes out, especially because we don't actually know if this PR will solve the problem he's facing. But things aren't that straightforward with Apple, or maybe it is and I don't know. I believe @3esmit is locked out in an iPhone, where he cannot install a manual release build from this PR to upgrade and test the fix like any APK would do in Android. I thought TestFlight builds couldn't upgrade production builds from App Store and I'm not even sure if it's safe regarding data loss. @siddarthkay do you know if there's a way for a user to safely "upgrade" from the production build from App Store using TestFlight? |
|
I have the backup of the database, I can test it without fear. If anything goes wrong, I can restore (the whole iPhone, including Status app private data). |
Performed additional testing including upgrade of keycard user, user with enabled biometrics. Checked:
No issues from my side. I still need to trigger full e2e run (currently pipeline is broken) and in case there are no unexpected failures there will be green light from QA side. Will keep you updated. @qfrank as I have in fact finished testing the PR prior to additional reviews of status-go PR please let me know in case there will be any additional commits so I perform re-testing. Thank you. |
96% of end-end tests have passedFailed tests (1)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (54)Click to expandClass TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestFallbackMultipleDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestWalletMultipleDevice:
|
UPDATE: failed e2e is not PR related. PR is good to merge in case there will be no additional changes. |
Thank you for your feedback! The interesting thing is reasonable. In short, the trap of node config migration was introduced from v1.19.0 @pavloburykh |
|
I’d say we should not support such cases in general. Testing upgrades from a previous version while skipping several intermediate versions is not something I would include in our testing scope, as the possible combinations are almost endless. Amazing work, @qfrank and @3esmit, on identifying this issue, but it’s an invalid user path, and I believe it shouldn’t be supported. Ideally, we should consider addressing this at the program level—if feasible—by detecting the installed version of Status and prohibiting installation if it’s older than the latest previous version. cc @ilmotta |
@churik Maybe I misunderstood your suggestion. This would force the user to upgrade one version at a time. The point of using migrations the way we do (which is a conventional solution, nothing special about Status) is that the user can skip releases as much as they want. Status app is meant to be upgradable from any version A to B (where B > A). In other words, if the user skips releases B, C, and D, and if each of these releases had a corresponding migration, when they migrate from A to E, all migrations for B, C, and D will be applied in order and the end result should be the same as if the user upgraded one version at a time. Therefore, if we prevent the user from skipping versions we wouldn't be helping them. I hope this makes sense. We may prefer to not test certain combinations of app upgrades and I very much agree with you on that, but this is simply a limitation of our capacity, not about the software. |
For details, pls see description in status-go PR
General test steps:
debugso we can track the logfailed to open database: table node_config has no column named connector_enabledAffected Platform: