1763626 - Delete obsolete attachments automatically

Status: RESOLVED FIXED

(Firefox :: Remote Settings Client, enhancement)

Description

[Mathieu Leplatre [:leplatrem]]

The approach that we recommend in the docs is to delete attachments in the "sync" event callback.

This is not fully reliable. For example, if the browser crashes or the computer shuts down while the “sync” event listener executes, then then we can have some stale attachments downloaded in the profile that won’t match any record on the server.

The goal of the bug is to regularly scan / compare / delete stale download attachments. During the 24H timer for example.

Comment 1

[Rob Wu [:robwu]]

The first point of bug 1634127 refers to this kind of issue in non-crash cases (e.g. implemention errors).

An example of an implementation issue (not deleting stale dara) with the IndexDB-based backend was fixed at https://phabricator.services.mozilla.com/D141980?id=560833#inline-790005

Comment 2

[Rob Wu [:robwu]]

From https://bugzilla.mozilla.org/show_bug.cgi?id=1795710#c7:

:leplatrem: Could it make sense to add helpers / options to the RemoteSettingsClient or downloader to support the intended behavior of keeping attachments in sync with the stored records? I.e. delete attachments when no record references it any more. That would support the use case from this bug.

In Bug 1763626, we want to remove downloaded attachments that don't match any record (probably comparing their hash or something similar...). Seeing this bug, I realize its priority has to be raised and its implementation planned.

When attachmentId is used, the attachment should not automatically be deleted, because the client user claims responsibility for maintaining the attachment.

When record.id is automatically used, we should ideally delete the attachment when the record doesn't exist, once in a while?

Comment 3

[Mathieu Leplatre [:leplatrem]]

When attachmentId is used, the attachment should not automatically be deleted, because the client user claims responsibility for maintaining the attachment.

I don't think that this is an obvious assumption.

If that is something specific you need for the blocklist, we could add a flag to disable pruning explicitly.

Otherwise, the algorithm is pretty straightforward:

const allRecords = await client.db.list();
const currentRecordsIDs = new Set(allRecords.map(r => r.id));

const allAttachments = await client.attachments.listAll();
const attachmentsToDelete = allAttachments.reduce((entry, acc) => {
   if (!currentRecordsIDs.has(entry.record.id)) {
     acc.push(entry.attachmentId);
     return acc;
   }
}, []);

await client.attachments.deleteAll(attachmentsToDelete);

I propose that we execute this on the 24H timer, and not on push notifications.

Comment 4

[Rob Wu [:robwu]]

(In reply to Mathieu Leplatre [:leplatrem] from comment #3)

When attachmentId is used, the attachment should not automatically be deleted, because the client user claims responsibility for maintaining the attachment.

I don't think that this is an obvious assumption.

If that is something specific you need for the blocklist, we could add a flag to disable pruning explicitly.

Sounds good.

Otherwise, the algorithm is pretty straightforward:
... code in comment 3
I propose that we execute this on the 24H timer, and not on push notifications.

Can this be one atomic transaction? Otherwise there is the chance of broken attachments when the cleanup happens in the middle of some other logic.

I don't know how expensive the deletion logic is going to be, but if the task is relatively expensive, then it may make sense to look into ways of minimizing that query.

Comment 5

[Mathieu Leplatre [:leplatrem]]

Attachment: Bug 1763626 - Delete obsolete attachments r?robwu

Comment 6

[Mathieu Leplatre [:leplatrem]]

Attachment: WIP: Bug 1763626 - Delete obsolete attachments

Depends on D160244

Comment 7

[Mathieu Leplatre [:leplatrem]]

Attachment: WIP: Bug 1763626 - Delete obsolete attachments

Depends on D160245

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eda416dffe89
Delete obsolete attachments r=robwu

Comment 9

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/eda416dffe89