From 6a17dcf4de905aadd3907dda9adb07b74d62877e Mon Sep 17 00:00:00 2001 From: Johann150 Date: Sat, 11 Mar 2023 19:38:56 +0100 Subject: [PATCH] server: refactor to group deletion deliveries The `quiet` argument has been removed from `deleteNote` (or `deleteNotes` respectively) since it was not used anywhere and it does not seem a good idea to not update statistics in some cases. This should also fix an issue where cascade deletions mean that statistics are not properly updated or streaming clients not properly informed of deletions. This case was seemingly not considered before, even though there was some handling for cascade deleted notes. This is going to improve how cascade deletion impacts the delivery queue, because cascade-deleted notes will now be grouped for delivery. Changelog: Fixed --- .../remote/activitypub/kernel/delete/note.ts | 4 +- .../activitypub/kernel/undo/announce.ts | 4 +- .../src/server/api/endpoints/notes/delete.ts | 6 +- .../server/api/endpoints/notes/unrenote.ts | 8 +- packages/backend/src/services/note/delete.ts | 152 ++++++++++-------- 5 files changed, 99 insertions(+), 75 deletions(-) diff --git a/packages/backend/src/remote/activitypub/kernel/delete/note.ts b/packages/backend/src/remote/activitypub/kernel/delete/note.ts index cb9a992ed..9f9a5cea6 100644 --- a/packages/backend/src/remote/activitypub/kernel/delete/note.ts +++ b/packages/backend/src/remote/activitypub/kernel/delete/note.ts @@ -1,5 +1,5 @@ import { CacheableRemoteUser } from '@/models/entities/user.js'; -import { deleteNote } from '@/services/note/delete.js'; +import { deleteNotes } from '@/services/note/delete.js'; import { getApLock } from '@/misc/app-lock.js'; import { deleteMessage } from '@/services/messages/delete.js'; import { DbResolver } from '@/remote/activitypub/db-resolver.js'; @@ -29,7 +29,7 @@ export default async function(actor: CacheableRemoteUser, uri: string): Promise< return 'skip: cant delete other actors note'; } - await deleteNote(actor, note); + await deleteNotes([note], actor); return 'ok: note deleted'; } } finally { diff --git a/packages/backend/src/remote/activitypub/kernel/undo/announce.ts b/packages/backend/src/remote/activitypub/kernel/undo/announce.ts index 232617cb6..78981b542 100644 --- a/packages/backend/src/remote/activitypub/kernel/undo/announce.ts +++ b/packages/backend/src/remote/activitypub/kernel/undo/announce.ts @@ -1,6 +1,6 @@ import { Notes } from '@/models/index.js'; import { CacheableRemoteUser } from '@/models/entities/user.js'; -import { deleteNote } from '@/services/note/delete.js'; +import { deleteNotes } from '@/services/note/delete.js'; import { IAnnounce, getApId } from '@/remote/activitypub/type.js'; export const undoAnnounce = async (actor: CacheableRemoteUser, activity: IAnnounce): Promise => { @@ -13,6 +13,6 @@ export const undoAnnounce = async (actor: CacheableRemoteUser, activity: IAnnoun if (!note) return 'skip: no such Announce'; - await deleteNote(actor, note); + await deleteNotes([note], actor); return 'ok: deleted'; }; diff --git a/packages/backend/src/server/api/endpoints/notes/delete.ts b/packages/backend/src/server/api/endpoints/notes/delete.ts index c87c8a125..ee787b603 100644 --- a/packages/backend/src/server/api/endpoints/notes/delete.ts +++ b/packages/backend/src/server/api/endpoints/notes/delete.ts @@ -1,4 +1,4 @@ -import { deleteNote } from '@/services/note/delete.js'; +import { deleteNotes } from '@/services/note/delete.js'; import { Users } from '@/models/index.js'; import { SECOND, HOUR } from '@/const.js'; import define from '@/server/api/define.js'; @@ -47,6 +47,6 @@ export default define(meta, paramDef, async (ps, user) => { throw new ApiError('ACCESS_DENIED'); } - // Fetch the note owner, since the current user may be an admin or moderator. - await deleteNote(await Users.findOneByOrFail({ id: note.userId }), note); + // Here, we do not provide the current user because it may be an admin/moderator. + await deleteNotes([note]); }); diff --git a/packages/backend/src/server/api/endpoints/notes/unrenote.ts b/packages/backend/src/server/api/endpoints/notes/unrenote.ts index 5cd16a7f8..909835f5f 100644 --- a/packages/backend/src/server/api/endpoints/notes/unrenote.ts +++ b/packages/backend/src/server/api/endpoints/notes/unrenote.ts @@ -1,4 +1,4 @@ -import { deleteNote } from '@/services/note/delete.js'; +import { deleteNotes } from '@/services/note/delete.js'; import { Notes, Users } from '@/models/index.js'; import { SECOND, HOUR } from '@/const.js'; import define from '@/server/api/define.js'; @@ -48,7 +48,7 @@ export default define(meta, paramDef, async (ps, user) => { renoteId: note.id, }); - for (const note of renotes) { - deleteNote(await Users.findOneByOrFail({ id: user.id }), note); - } + if (renotes.length === 0) return; + + await deleteNotes(renotes, user); }); diff --git a/packages/backend/src/services/note/delete.ts b/packages/backend/src/services/note/delete.ts index 448dd85bf..595c28ace 100644 --- a/packages/backend/src/services/note/delete.ts +++ b/packages/backend/src/services/note/delete.ts @@ -14,14 +14,88 @@ import { notesChart, perUserNotesChart, instanceChart } from '@/services/chart/i import { DeliverManager } from '@/remote/activitypub/deliver-manager.js'; import { countSameRenotes } from '@/misc/count-same-renotes.js'; import { registerOrFetchInstanceDoc } from '../register-or-fetch-instance-doc.js'; -import { deliverToRelays } from '../relay.js'; +import { deliverMultipleToRelays } from '../relay.js'; /** - * Delete your note. - * @param user author - * @param note note to be deleted + * Delete several notes of the same user. + * @param notes Array of notes to be deleted. + * @param user Author of the notes. Will be fetched if not provided. */ -export async function deleteNote(user: { id: User['id']; uri: User['uri']; host: User['host']; }, note: Note, quiet = false): Promise { +export async function deleteNotes(notes: Note[], user?: User): Promise { + if (notes.length === 0) return; + + const fetchedUser = user ?? await Users.findOneByOrFail({ id: notes[0].userId }); + + const cascadingNotes = await Promise.all( + notes.map(note => findCascadingNotes(note)) + ).then(res => res.flat()); + + // perform side effects for notes and cascaded notes + await Promise.all( + notes.concat(cascadingNotes) + .map(note => deletionSideEffects(note, fetchedUser)) + ); + + // Compute delivery content for later. + // It is important that this is done before deleting notes from + // the database since we may need some information from parent + // notes that cause this one to be cascade-deleted. + let content = await Promise.all( + notes.concat(cascadingNotes) + // only deliver for local notes that are not local-only + .filter(note => note.userHost == null && !note.localOnly) + .map(async note => { + let renote: Note | null = null; + + // if the deleted note is a renote + if (foundkey.entities.isPureRenote(note)) { + renote = await Notes.findOneBy({ id: note.renoteId }); + } + + return renderActivity(renote + ? renderUndo(renderAnnounce(renote.uri || `${config.url}/notes/${renote.id}`, note), fetchedUser) + : renderDelete(renderTombstone(`${config.url}/notes/${note.id}`), fetchedUser)); + }) + ); + + // Compute addressing information. + // Since we do not send any actual content, we send all note deletions to everyone. + const manager = new DeliverManager(fetchedUser, content); + manager.addFollowersRecipe(); + manager.addEveryone(); + // Check mentioned users, since not all may have a shared inbox. + await Promise.all( + notes.concat(cascadingNotes) + .map(note => getMentionedRemoteUsers(note)) + ) + .then(remoteUsers => { + remoteUsers.flat() + .forEach(remoteUser => manager.addDirectRecipe(remoteUser)) + }); + + // Actually delete notes from the database. + // It is important that this is done before delivering the activities. + // Otherwise there might be a race condition where we tell someone + // the note exists and they can successfully fetch it. + await Notes.delete({ + id: In(notes.map(x => x.id)), + userId: fetchedUser.id, + }); + + // deliver the previously computed content + await Promise.all([ + manager.execute(), + deliverMultipleToRelays(user, content), + ]); +} + +/** + * Perform side effects of deletion, such as updating statistics. + * Does not actually delete the note itself. + * @param note The soon to be deleted note. + * @param user The author of said note. + */ +async function deletionSideEffects(note: Note, user: User): Promise { const deletedAt = new Date(); // If this is the only renote of this note by this user @@ -34,48 +108,18 @@ export async function deleteNote(user: { id: User['id']; uri: User['uri']; host: await Notes.decrement({ id: note.replyId }, 'repliesCount', 1); } - if (!quiet) { - publishNoteStream(note.id, 'deleted', { deletedAt }); + publishNoteStream(note.id, 'deleted', { deletedAt }); - // deliver delete activity of note itself for local posts - if (Users.isLocalUser(user) && !note.localOnly) { - let renote: Note | null = null; + // update statistics + notesChart.update(note, false); + perUserNotesChart.update(user, note, false); - // if deleted note is renote - if (foundkey.entities.isPureRenote(note)) { - renote = await Notes.findOneBy({ id: note.renoteId }); - } - - const content = renderActivity(renote - ? renderUndo(renderAnnounce(renote.uri || `${config.url}/notes/${renote.id}`, note), user) - : renderDelete(renderTombstone(`${config.url}/notes/${note.id}`), user)); - - deliverToConcerned(user, note, content); - } - - // also deliver delete activity to cascaded notes - const cascadingNotes = await findCascadingNotes(note); - for (const cascadingNote of cascadingNotes) { - const content = renderActivity(renderDelete(renderTombstone(`${config.url}/notes/${cascadingNote.id}`), cascadingNote.user)); - deliverToConcerned(cascadingNote.user, cascadingNote, content); - } - - // update statistics - notesChart.update(note, false); - perUserNotesChart.update(user, note, false); - - if (Users.isRemoteUser(user)) { - registerOrFetchInstanceDoc(user.host).then(i => { - Instances.decrement({ id: i.id }, 'notesCount', 1); - instanceChart.updateNote(i.host, note, false); - }); - } + if (Users.isRemoteUser(user)) { + registerOrFetchInstanceDoc(user.host).then(i => { + Instances.decrement({ id: i.id }, 'notesCount', 1); + instanceChart.updateNote(i.host, note, false); + }); } - - await Notes.delete({ - id: note.id, - userId: user.id, - }); } /** @@ -143,23 +187,3 @@ async function getMentionedRemoteUsers(note: Note): Promise { }) as IRemoteUser[]; } -async function deliverToConcerned(user: { id: ILocalUser['id']; host: null; }, note: Note, content: any): Promise { - const manager = new DeliverManager(user, content); - - const remoteUsers = await getMentionedRemoteUsers(note); - for (const remoteUser of remoteUsers) { - manager.addDirectRecipe(remoteUser); - } - - if (['public', 'home', 'followers'].includes(note.visibility)) { - manager.addFollowersRecipe(); - } - - if (['public', 'home'].includes(note.visibility)) { - manager.addEveryone(); - } - - await manager.execute(); - - deliverToRelays(user, content); -}