From 4ba0357d4914b026b812cc3c27f9e4380b5375c6 Mon Sep 17 00:00:00 2001 From: eternal-flame-AD Date: Tue, 19 Nov 2024 21:07:02 -0600 Subject: [PATCH] fix(backend): Atomically mark remote account deletions Signed-off-by: eternal-flame-AD --- .../1732071810971-IndexUserDeleted.js | 16 +++++ .../backend/src/core/DeleteAccountService.ts | 4 ++ packages/backend/src/core/RoleService.ts | 1 + .../src/core/activitypub/ApInboxService.ts | 9 +-- .../activitypub/models/ApPersonService.ts | 6 +- .../DeleteAccountProcessorService.ts | 62 ++++++++++++++++--- .../src/server/api/endpoints/users/show.ts | 1 + 7 files changed, 80 insertions(+), 19 deletions(-) create mode 100644 packages/backend/migration/1732071810971-IndexUserDeleted.js diff --git a/packages/backend/migration/1732071810971-IndexUserDeleted.js b/packages/backend/migration/1732071810971-IndexUserDeleted.js new file mode 100644 index 0000000000..b4c3d714ad --- /dev/null +++ b/packages/backend/migration/1732071810971-IndexUserDeleted.js @@ -0,0 +1,16 @@ +/* + * SPDX-FileCopyrightText: syuilo and misskey-project and yumechi + * SPDX-License-Identifier: AGPL-3.0-only + */ + +export class IndexUserDeleted1732071810971 { + name = 'IndexUserDeleted1732071810971' + + async up(queryRunner) { + await queryRunner.query(`CREATE INDEX IF NOT EXISTS "IDX_199b79e682bdc5ba946f491686" ON "user" ("isDeleted")`); + } + + async down(queryRunner) { + await queryRunner.query(`DROP INDEX IF EXISTS "IDX_199b79e682bdc5ba946f491686"`); + } +} diff --git a/packages/backend/src/core/DeleteAccountService.ts b/packages/backend/src/core/DeleteAccountService.ts index 7f1b8f3efb..a5b0f60fbf 100644 --- a/packages/backend/src/core/DeleteAccountService.ts +++ b/packages/backend/src/core/DeleteAccountService.ts @@ -47,6 +47,10 @@ export class DeleteAccountService { }); } + if (!(await this.usersRepository.update({ id: user.id, isDeleted: false }, { isDeleted: true })).affected) { + return; + } + // 物理削除する前にDelete activityを送信する if (this.userEntityService.isLocalUser(user)) { // 知り得る全SharedInboxにDelete配信 diff --git a/packages/backend/src/core/RoleService.ts b/packages/backend/src/core/RoleService.ts index 5af6b05942..ba4507d206 100644 --- a/packages/backend/src/core/RoleService.ts +++ b/packages/backend/src/core/RoleService.ts @@ -488,6 +488,7 @@ export class RoleService implements OnApplicationShutdown, OnModuleInit { return ids.length > 0 ? await this.usersRepository.findBy({ id: In(ids), + isDeleted: false, }) : []; } diff --git a/packages/backend/src/core/activitypub/ApInboxService.ts b/packages/backend/src/core/activitypub/ApInboxService.ts index f3aa46292e..fccf86cb91 100644 --- a/packages/backend/src/core/activitypub/ApInboxService.ts +++ b/packages/backend/src/core/activitypub/ApInboxService.ts @@ -509,19 +509,12 @@ export class ApInboxService { return `skip: delete actor ${actor.uri} !== ${uri}`; } - const user = await this.usersRepository.findOneBy({ id: actor.id }); - if (user == null) { - return 'skip: actor not found'; - } else if (user.isDeleted) { + if (!(await this.usersRepository.update({ id: actor.id, isDeleted: false }, { isDeleted: true })).affected) { return 'skip: already deleted'; } const job = await this.queueService.createDeleteAccountJob(actor); - await this.usersRepository.update(actor.id, { - isDeleted: true, - }); - this.globalEventService.publishInternalEvent('remoteUserUpdated', { id: actor.id }); return `ok: queued ${job.name} ${job.id}`; diff --git a/packages/backend/src/core/activitypub/models/ApPersonService.ts b/packages/backend/src/core/activitypub/models/ApPersonService.ts index 8c4e40c561..e01b098194 100644 --- a/packages/backend/src/core/activitypub/models/ApPersonService.ts +++ b/packages/backend/src/core/activitypub/models/ApPersonService.ts @@ -557,7 +557,9 @@ export class ApPersonService implements OnModuleInit { if (moving) updates.movedAt = new Date(); // Update user - await this.usersRepository.update(exist.id, updates); + if (!(await this.usersRepository.update({ id: exist.id, isDeleted: false }, updates)).affected) { + return 'skip'; + } if (person.publicKey) { await this.userPublickeysRepository.update({ userId: exist.id }, { @@ -662,7 +664,7 @@ export class ApPersonService implements OnModuleInit { @bindThis public async updateFeatured(userId: MiUser['id'], resolver?: Resolver): Promise { - const user = await this.usersRepository.findOneByOrFail({ id: userId }); + const user = await this.usersRepository.findOneByOrFail({ id: userId, isDeleted: false }); if (!this.userEntityService.isRemoteUser(user)) return; if (!user.featured) return; diff --git a/packages/backend/src/queue/processors/DeleteAccountProcessorService.ts b/packages/backend/src/queue/processors/DeleteAccountProcessorService.ts index 14a53e0c42..05be2e02f0 100644 --- a/packages/backend/src/queue/processors/DeleteAccountProcessorService.ts +++ b/packages/backend/src/queue/processors/DeleteAccountProcessorService.ts @@ -4,9 +4,9 @@ */ import { Inject, Injectable } from '@nestjs/common'; -import { MoreThan } from 'typeorm'; +import { DataSource, MoreThan, QueryFailedError, TypeORMError } from 'typeorm'; import { DI } from '@/di-symbols.js'; -import type { DriveFilesRepository, NotesRepository, UserProfilesRepository, UsersRepository } from '@/models/_.js'; +import { MiUser, type DriveFilesRepository, type NotesRepository, type UserProfilesRepository, type UsersRepository } from '@/models/_.js'; import type Logger from '@/logger.js'; import { DriveService } from '@/core/DriveService.js'; import type { MiDriveFile } from '@/models/DriveFile.js'; @@ -26,6 +26,9 @@ export class DeleteAccountProcessorService { @Inject(DI.usersRepository) private usersRepository: UsersRepository, + @Inject(DI.db) + private db: DataSource, + @Inject(DI.userProfilesRepository) private userProfilesRepository: UserProfilesRepository, @@ -52,6 +55,14 @@ export class DeleteAccountProcessorService { return; } + if (!user.isDeleted) { + this.logger.warn('User is not pre-marked as deleted, this is likely a bug'); + if (process.env.NODE_ENV !== 'production') { + throw new Error('User is not pre-marked as deleted'); // make some noise to make sure tests fail + } + await this.usersRepository.update({ id: user.id }, { isDeleted: true }); + } + { // Delete notes let cursor: MiNote['id'] | null = null; @@ -121,13 +132,46 @@ export class DeleteAccountProcessorService { } } - // soft指定されている場合は物理削除しない - if (job.data.soft) { - // nop - } else { - await this.usersRepository.delete(job.data.user.id); + // Deadlockが発生した場合にリトライする + for (let remaining = 3; remaining > 0; remaining--) { + try { + // soft指定されている場合は物理削除しない + await this.db.transaction(async txn => { + // soft指定してもデータをすべで削除する + await txn.delete(MiUser, user.id); + if (job.data.soft) { + await txn.insert(MiUser, { + ...user, + isRoot: false, + updatedAt: new Date(), + emojis: [], + hideOnlineStatus: true, + followersCount: 0, + followingCount: 0, + avatarUrl: null, + avatarId: null, + notesCount: 0, + inbox: null, + sharedInbox: null, + featured: null, + uri: null, + followersUri: null, + token: null, + isDeleted: true, + }); + } + }); + return 'Account deleted'; + } catch (e) { + // 40P01 = deadlock_detected + // https://www.postgresql.org/docs/current/errcodes-appendix.html + if (remaining > 0 && e instanceof QueryFailedError && e.driverError.code === '40P01') { + this.logger.warn(`Deadlock occurred, retrying after 1s... [${remaining - 1} remaining]`); + await new Promise(resolve => setTimeout(resolve, 1000)); + continue; + } + throw e; + } } - - return 'Account deleted'; } } diff --git a/packages/backend/src/server/api/endpoints/users/show.ts b/packages/backend/src/server/api/endpoints/users/show.ts index 062326e28d..6daed35372 100644 --- a/packages/backend/src/server/api/endpoints/users/show.ts +++ b/packages/backend/src/server/api/endpoints/users/show.ts @@ -106,6 +106,7 @@ export default class extends Endpoint { // eslint- id: In(ps.userIds), } : { id: In(ps.userIds), + isDeleted: false, isSuspended: false, });