[keyserver] Remove holders in account deleters
ClosedPublic
Actions

Authored by bartek on Sep 30 2024, 3:16 AM.

Details

Reviewers

tomek
ashoat

Commits

rCOMMc5cb285b30cf: [keyserver] Remove holders in account deleters

Summary

Address ENG-9353.
Before calling DELETE FROM uploads, we need to select the extra column first and remove blob holders if present in the column JSON.

Depends on D13513

Test Plan

Enabled blob-hosted user avatars. Created an account, set image avatar, then deleted the account and watched the logs. Similiarly to D13513, a blob was uploaded and then its holder was removed

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bartek created this revision.Sep 30 2024, 3:16 AM

bartek held this revision as a draft.

Harbormaster completed remote builds in B31922: Diff 44667.Sep 30 2024, 3:33 AM

bartek published this revision for review.Sep 30 2024, 4:28 AM

bartek edited the summary of this revision. (Show Details)

bartek edited the test plan for this revision. (Show Details)

bartek added inline comments.

keyserver/src/deleters/account-deleters.js
84 ↗	(On Diff #44667)	Same concern as in https://phab.comm.dev/D13512?id=44665#inline-77250

ashoat requested changes to this revision.Sep 30 2024, 5:45 AM

ashoat added inline comments.

keyserver/src/deleters/account-deleters.js
84 ↗	(On Diff #44667)	This one is a bit more complicated than the others. I don't think we should fail the account deletion if the holders fail to be deleted. Instead, I think we should mark the uploads as "unassigned" by wiping `container` and `user_container`. Then later, the delete will be retried by `deleteUnassignedUploads`. So I'm suggesting: Remove `DELETE u, i FROM uploads u` from the transaction above. Introduce a separate function (or async IIFE) called `deleteUploadsForUser` that first fetches all of the uploads for a user, then deletes their holders, and then deletes the uploads Call that new function with `ignorePromiseRejections`

This revision now requires changes to proceed.Sep 30 2024, 5:45 AM

Move upload deletion into ignorePromiseRejections

bartek added inline comments.Oct 1 2024, 5:48 AM

keyserver/src/deleters/account-deleters.js
41–49 ↗	(On Diff #44749)	I'm not sure about this part. I tried to achieve this: Instead, I think we should mark the uploads as "unassigned" by wiping `container` and `user_container`. Then later, the delete will be retried by `deleteUnassignedUploads`.