Re-enable storage maintenance call

[grigoryk]

In #2765 we added periodic storage maintenance calls that run once a day (or so), during startup.

In #6937 we saw that this interferes with our fennec migration code, which also runs on startup. So, maintenance was disabled.

In #6938 I proposed a solution for this problem, but it doesn't apply now that we're running migration code in a background service.

This issue is to re-enable maintenance call in a way that doesn't interfere with other parts of the browser that may need to acquire higher levels of SQL locks (reserved+).

┆Issue is synchronized with this Jira Task

[boek]

@grigoryk are we tracking this anywhere? Or is this a nice-to-have in the future?

[mcomella]

Perf self triage: this doesn't seem like it'll land in GA so moving to low impact so we don't have to track it.

[mcomella]

This isn't inherently a performance problem so removing it from our board. However, the proposed solution may have perf implications so if that's the case, please let us know. Thanks!

[mcomella]

From the original issue:

@grigoryk indicated that it does not [run maintenance], which will result in worse perf, more disk usage, etc.

Re-adding to our board because this may impact long term app performance.

I'd guess the solution should happen via WorkManager when the app isn't running though if it does happen while the app is running, please don't put it into the startup critical path: it should probably live in the visualCompletenessQueue if it happens during startup.

[stale]

See: #17373 This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[mcomella]

@grigoryk I noticed stalebot closed this but this seems important. What do you think the urgency/priority is on this issue? If high, do you think it's something the team would have cycles to work on?

[mhammond]

Thought about two more issues worth considering:

* I think we'd want to use a dedicated writer for this though, so we don't interrupt/cancel the wrong operation by mistake when using a shared writer.

I don't think using a dedicated writer will be able to work without the risk of the "other" writer seeing "database is locked" errors. I do agree we need to work out how to not interrupt the wrong operation though (and I think Ben was suggesting a way to "scope" the interrupt, but I haven't thought it through)

* We should also make sure we don't run this every time the device is idle. Maybe just once a day?

The call only removes 6 visits each time it is called, so doing it once per day seems unlikely to effectively keep a cap on the number of visits.

[csadilek]

The call only removes 6 visits each time it is called, so doing it once per day seems unlikely to effectively keep a cap on the number of visits.

OK, makes sense! But running it every time the device is idle might also not be ideal for battery consumption. If you all have a suggestion of how often we should ideally run this, please let us know. We could start with a compromise and introduce some upper limit of runs per day otherwise.

[mhammond]

It's certainly true that we are never calling it today, so ramping up slowly should in theory have some small benefit. Another challenge is that a world where this has been in-place and being called regularly (ie, there will usually be just a few records to purge) is quite different to the world we have today (where there will be many as it catches up).

IOW, calling it daily is still better than what we do today, so let's do that! But it does open the question about telemetry etc - how will we ever know when we've found the sweet-spot?

[bendk]

I don't think using a dedicated writer will be able to work without the risk of the "other" writer seeing "database is locked" errors. I do agree we need to work out how to not interrupt the wrong operation though (and I think Ben was suggesting a way to "scope" the interrupt, but I haven't thought it through)

I didn't have a plan for how to stop this, so maybe adding another writer is the simplest way to move forward. My one concern is that the number of connections continues to grow as we add more interruptible operations. If that happens we should probably come up with a different situation.

OK, makes sense! But running it every time the device is idle might also not be ideal for battery consumption. If you all have a suggestion of how often we should ideally run this, please let us know. We could start with a compromise and introduce some upper limit of runs per day otherwise.

I like the idea of starting once per day and increasing based on telemetry. Maybe we count how many devices are still over the size target after running maintenance and try to get that down to some small-ish number.

As to how often maybe we could return a boolean if the DB is still above the size target after running maintenance. If that's false, then just run it once a day (maybe even less if the app isn't used everyday). If it's true, then run it more often, with some upper limit.

[Mugurell]

Saw this next in our backlog but it seems like there are still discussions to take plans and a dependency in AS?
Not sure what the next steps are for Fenix. @csadilek @mhammond @bendk .

[mhammond]

I thought we had a plan written somewhere in one place but can't find it. I believe we agreed:

• We'd start small, running once per day, with a target size of 75 MiB
• We accept that this is not interruptable because we expect it to complete very quickly. If we don't already, it would be fine to interrupt the "writer" connection at shutdown, which would interrupt any executing operations, possibly including this, which is fine.
• Christian suggested that it might make sense to put this behind a feature flag in Nightly.
• We have already landed telemetry in app-services in #5123, so as long we we have a fairly recent app-services we should get telemetry. Our team will monitor that and offer advice for next steps (which will hopefully be to run more often than once per day)

I'm sure @bendk or @csadilek will correct what I got wrong or add what I forgot.

[csadilek]

Thanks @mhammond. Yes, I think this captures it all. The rest of the discussion happened in mozilla/application-services#5115.

@Mugurell Please sync up with @jonalmeida and @kycn though, because I think they had planned to look into this soon, or possibly already started.

[kycn]

Status update:
A PeriodicWorkRequest is introduced in A-C with a 24h repeat interval.
It runs when the following constraints are met: setRequiresBatteryNotLow(true) and setRequiresDeviceIdle(true). Fenix enqueues this request to WorkManager if visualCompletenesQueue is ready.
The related PRs: Fenix and A-C.
The feature is behind a feature flag and is enabled for only Debug and Nightly variants.
@csadilek @jonalmeida @mhammond @bendk

[jonalmeida]

No QA testing is needed here.