Page MenuHomePhabricator

[lib] Create a compaction after schema-changing migration
Changes PlannedPublic

Authored by tomek on May 15 2024, 5:49 AM.
Tags
None
Referenced Files
F2028066: D12050.id40291.diff
Mon, Jun 17, 10:31 PM
F2028065: D12050.id40212.diff
Mon, Jun 17, 10:31 PM
F2027997: D12050.id.diff
Mon, Jun 17, 10:29 PM
Unknown Object (File)
Wed, Jun 12, 12:40 PM
Unknown Object (File)
Sat, Jun 8, 1:58 PM
Unknown Object (File)
Sat, Jun 8, 8:18 AM
Unknown Object (File)
Fri, Jun 7, 10:17 PM
Unknown Object (File)
Fri, Jun 7, 9:48 PM
Subscribers

Details

Reviewers
kamil
marcin
varun
Summary

Check if any of migrations changes schema and create a compaction if that happened.

This diff introduces a new flag that will be set to true when all the primary devices will be backed up (the switch from dev menu is removed).

https://linear.app/comm/issue/ENG-7014/create-new-compaction-after-sqlite-migration

Depends on D12049

Test Plan

Enable backup, set the flag to true, run a migration that changes schema and check in the backup service if the backup was created.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested review of this revision.May 15 2024, 6:05 AM
native/backup/use-client-backup.js
49 ↗(On Diff #40212)

It's possible for a SIWE account to not have backup secrets. What happens in that case?

native/backup/use-client-backup.js
49 ↗(On Diff #40212)

In this diff, I'm moving this code from one place to another and adding a place where I call it but behind a flag. I don't know what will happen if the secrets aren't there - this code was introduced in https://phab.comm.dev/D11716 so I think @marcin has more context.

native/backup/use-client-backup.js
49 ↗(On Diff #40212)

Yes it is possible if user logs in but decides to skip backup message generation. We decided to use alert store to remind user to generate backup message every 24 hours if they decide to skip.

What happens in that case?

Now that backup feature works via buttons (for staff users only) it will just show alert informing that backup secrets are missing. Once I complete my backup and app lifecycle goal upload button will be deprecated and SIWE user will jest see the reminder on regular basis.

So createBackup will not be used when we actually ship this? Should it be renamed to be more clear, eg. createTestBackup?

lib/utils/migration-utils.js
270 ↗(On Diff #40291)

This check alone is not enough - we need to check if we are running on a primary device. Do we currently have a way to check for this information?

native/backup/use-client-backup.js
49 ↗(On Diff #40291)

Important question - is this code going to execute after succesfull log in? If s that it is not going to work correctly for SIWE since SIWE user must create new backup message after log in and the screen where is happens is rendered base on information stored in Redux.

native/backup/use-client-backup.js
49 ↗(On Diff #40291)

"If s" should be "If so".

marcin added inline comments.
lib/utils/migration-utils.js
270 ↗(On Diff #40291)

I talked to @bartek and @kamil about this and you should check AuxUserStore to get information whether backup should be created. Potentially we have to expose it as a function in config.js to returns false on web and checks redux on native.

lib/utils/migration-utils.js
270 ↗(On Diff #40291)

@kamil when is AuxUserStore going to be populated by identity service? Is it correct for @tomek to use it here to check if migrations are running on primary device?

lib/utils/migration-utils.js
270 ↗(On Diff #40291)

After Device list updates are disseminated to all peersAuxUserStore (deviceList to be precise) should be automatically updated after receiving updates from other peers (including own devices).

Checking if we're primary device should be easier, it's the primary device's responsibility to sign and broadcast the device list updates so when this code is running we should have the newest knowledge of being/not being the primary device, but unfortunately, this mechanism is not yet well defined or designed. (cc. @bartek)

This code (createAsyncMigrate) will be executed on app start (no matter if logged in/out). How about creating compaction after e.g. primary restore?

This code (createAsyncMigrate) will be executed on app start (no matter if logged in/out). How about creating compaction after e.g. primary restore?

If this code is executed on log-out compaction will not be created.

How about creating compaction after e.g. primary restore?

This will be implemented as a part of backup handler lifecycle update project (my current May project).

The main concern here is when running migration on logged-in device and we have to check for being primary device.

Soo we will remove backupEnabledByDefault and replace it with proper check for being primary device.

lib/utils/migration-utils.js
270 ↗(On Diff #40291)
This revision is now accepted and ready to land.Fri, May 24, 8:27 AM
native/backup/use-client-backup.js
43 ↗(On Diff #40291)

Don't forget to rename to createTestBackup

tomek planned changes to this revision.Tue, Jun 4, 5:33 AM