From 8180a7619d1ac3712e992c2b22dd40c34e382560 Mon Sep 17 00:00:00 2001 From: blued_gear Date: Wed, 5 Aug 2026 20:31:14 +0000 Subject: [PATCH 1/5] add the async_slow queue to separate long-running tasks from other messages --- compose.yaml | 2 +- config/packages/messenger.yaml | 18 ++++++ .../02-admin/01-installation/01-bare_metal.md | 2 +- docs/02-admin/04-running-mbin/04-messenger.md | 25 ++++---- .../Contracts/AsyncSlowMessageInterface.php | 9 +++ src/Message/DeleteImageMessage.php | 3 + src/Message/DeleteImageV2Message.php | 28 +++++++++ src/MessageHandler/DeleteImageV2Handler.php | 60 +++++++++++++++++++ src/MessageHandler/DeleteUserHandler.php | 17 +++--- src/Repository/ImageRepository.php | 17 ++++++ src/Service/UserManager.php | 37 +++++++----- 11 files changed, 181 insertions(+), 37 deletions(-) create mode 100644 src/Message/Contracts/AsyncSlowMessageInterface.php create mode 100644 src/Message/DeleteImageV2Message.php create mode 100644 src/MessageHandler/DeleteImageV2Handler.php diff --git a/compose.yaml b/compose.yaml index 623683f87e..864d3019b3 100644 --- a/compose.yaml +++ b/compose.yaml @@ -32,7 +32,7 @@ services: context: . target: prod restart: unless-stopped - command: bin/console messenger:consume scheduler_default old async outbox deliver inbox resolve receive failed --time-limit=3600 + command: bin/console messenger:consume scheduler_default old async async_slow outbox deliver inbox resolve receive failed --time-limit=3600 healthcheck: test: ['CMD-SHELL', "ps aux | grep 'messenger[:]consume' || exit 1"] env_file: .env diff --git a/config/packages/messenger.yaml b/config/packages/messenger.yaml index afd82f2ff8..b8e0ed9bc1 100644 --- a/config/packages/messenger.yaml +++ b/config/packages/messenger.yaml @@ -23,6 +23,23 @@ framework: max_delay: 76800000 jitter: 0 serializer: messenger.transport.symfony_serializer + async_slow: + dsn: "%env(MESSENGER_TRANSPORT_DSN)%" + options: + queues: + async_slow: + arguments: + x-queue-version: 2 + x-queue-type: 'classic' + exchange: + name: async_slow + retry_strategy: + max_retries: 5 + delay: 300000 + multiplier: 4 + max_delay: 76800000 + jitter: 0 + serializer: messenger.transport.symfony_serializer inbox: dsn: "%env(MESSENGER_TRANSPORT_DSN)%" options: @@ -137,6 +154,7 @@ framework: routing: # Route your messages to the transports App\Message\Contracts\AsyncMessageInterface: async + App\Message\Contracts\AsyncSlowMessageInterface: async_slow App\Message\Contracts\ActivityPubInboxInterface: inbox App\Message\Contracts\ActivityPubInboxReceiveInterface: receive App\Message\Contracts\ActivityPubOutboxDeliverInterface: deliver diff --git a/docs/02-admin/01-installation/01-bare_metal.md b/docs/02-admin/01-installation/01-bare_metal.md index af7739a19b..af84a6d6b3 100644 --- a/docs/02-admin/01-installation/01-bare_metal.md +++ b/docs/02-admin/01-installation/01-bare_metal.md @@ -576,7 +576,7 @@ With the following content: ```ini [program:messenger] -command=php /var/www/mbin/bin/console messenger:consume scheduler_default old async outbox deliver inbox resolve receive failed --time-limit=3600 +command=php /var/www/mbin/bin/console messenger:consume scheduler_default old async async_slow outbox deliver inbox resolve receive failed --time-limit=3600 #stdout_logfile=NONE #redirect_stderr=true user=www-data diff --git a/docs/02-admin/04-running-mbin/04-messenger.md b/docs/02-admin/04-running-mbin/04-messenger.md index 431602ac54..8fceb6aabb 100644 --- a/docs/02-admin/04-running-mbin/04-messenger.md +++ b/docs/02-admin/04-running-mbin/04-messenger.md @@ -3,21 +3,22 @@ The symphony messengers are background workers for a lot of different task, the biggest one being handling all the ActivityPub traffic. We have a few different queues: -1. `receive` [RabbitMQ]: everything any remote instance sends to us will first end up in this queue. - When processing it will be determined what kind of message it is (creation of a thread, a new comment, etc.) -2. `inbox` [RabbitMQ]: messages from `receive` with the determined kind of incoming message will end up here and the necessary actions will be executed. - This is the place where the thread or comment will actually be created -3. `outbox` [RabbitMQ]: when a user creates a thread or a comment, a message will be created and send to the outbox queue +1. `receive` [RabbitMQ]: Everything any remote instance sends to us will first end up in this queue. + When processing it will be determined what kind of message it is (creation of a thread, a new comment, etc.). +2. `inbox` [RabbitMQ]: Messages from `receive` with the determined kind of incoming message will end up here and the necessary actions will be executed. + This is the place where the thread or comment will actually be created. +3. `outbox` [RabbitMQ]: When a user creates a thread or a comment, a message will be created and send to the `outbox` queue to build the ActivityPub object that will be sent to remote instances. After the object is built and the inbox addresses of all the remote instances who are interested in the message are gathered, - we will create a `DeliverMessage` for every one of them, which will be sent to the `deliver` queue -4. `deliver` [RabbitMQ]: Actually sending out the ActivityPub objects to other instances + we will create a `DeliverMessage` for every one of them, which will be sent to the `deliver` queue. +4. `deliver` [RabbitMQ]: Actually sending out the ActivityPub objects to other instances. 5. `resolve` [RabbitMQ]: Resolving dependencies or ActivityPub actors. - For example if your instance gets a like message for a post that is not on your instance a message resolving that dependency will be dispatched to this queue -6. `async` [RabbitMQ]: messages in async are local actions that are relevant to this instance, e.g. creating notifications, fetching embedded images, etc. -7. `old` [RabbitMQ]: the standard messages queue that existed before. This exists solely for compatibility purposes and might be removed later on -8. `failed` [PostgreSQL]: jobs from the other queues that have been retried, but failed. They get retried a few times again, before they end up in -9. `dead` [PostgreSQL]: dead jobs that will not be retried + For example if your instance gets a like message for a post that is not on your instance a message resolving that dependency will be dispatched to this queue. +6. `async` [RabbitMQ]: Messages in `async` are local actions that are relevant to this instance, e.g. creating notifications, fetching embedded images, etc. +7. `async_slow` [RabbitMQ]: Similar to the `async` queue, but for messages which might take a long time (up to hours) to process. It is recommended to run a separate messenger for this queue. +8. `old` [RabbitMQ]: The standard messages queue that existed before. This exists solely for compatibility purposes and might be removed later on. +9. `failed` [PostgreSQL]: Jobs from the other queues that have been retried, but failed. They get retried a few times again, before they end up in `dead`. +10. `dead` [PostgreSQL]: Dead jobs that will not be retried. We need the `dead` queue so that messages that throw a `UnrecoverableMessageHandlingException`, which is used to indicate that a message should not be retried and go straight to the supplied failure queue diff --git a/src/Message/Contracts/AsyncSlowMessageInterface.php b/src/Message/Contracts/AsyncSlowMessageInterface.php new file mode 100644 index 0000000000..ebe4fc5878 --- /dev/null +++ b/src/Message/Contracts/AsyncSlowMessageInterface.php @@ -0,0 +1,9 @@ + $images a list of mappings where the key is the sha256 (hex) of the image entity and the value is its filepath + */ + public function __construct( + public array $images, + ) { + } + + public static function fromImage(Image $img): self + { + return new self([bin2hex($img->sha256) => $img->filePath]); + } +} diff --git a/src/MessageHandler/DeleteImageV2Handler.php b/src/MessageHandler/DeleteImageV2Handler.php new file mode 100644 index 0000000000..059e4f2871 --- /dev/null +++ b/src/MessageHandler/DeleteImageV2Handler.php @@ -0,0 +1,60 @@ +entityManager, $kernel); + } + + public function __invoke(DeleteImageV2Message $message): void + { + $this->workWrapper($message); + } + + public function doWork(MessageInterface $message): void + { + if (!$message instanceof DeleteImageV2Message) { + throw new \LogicException(); + } + + // querying each image individually is inefficient but lowers the probability of a race-condition + foreach ($message->images as $hash => $filepath) { + $img = $this->imageRepository->findOneBySha256(hex2bin($hash)); + if (null !== $img) { + if (!$this->imageRepository->isImageReferenced($img)) { + // this should lock the row and prevent concurrent re-referencing + $this->entityManager->remove($img); + $this->entityManager->flush(); + + $deleteFile = true; + } else { + $deleteFile = false; + } + } else { + $deleteFile = true; + } + + if ($deleteFile && null !== $filepath) { + $this->imageManager->remove($filepath); + } + } + } +} diff --git a/src/MessageHandler/DeleteUserHandler.php b/src/MessageHandler/DeleteUserHandler.php index 4184cee4ed..d41ae5a0c4 100644 --- a/src/MessageHandler/DeleteUserHandler.php +++ b/src/MessageHandler/DeleteUserHandler.php @@ -8,6 +8,7 @@ use App\Entity\User; use App\Message\ActivityPub\Outbox\DeliverMessage; use App\Message\Contracts\MessageInterface; +use App\Message\DeleteImageV2Message; use App\Message\DeleteUserMessage; use App\Service\ActivityPub\ActivityJsonBuilder; use App\Service\ActivityPub\Wrapper\DeleteWrapper; @@ -83,13 +84,11 @@ public function doWork(MessageInterface $message): void } catch (\Exception|\Error $e) { $this->logger->error("[ClearDeletedUserHandler::__invoke] Couldn't delete the cover of {user} at '{path}': {message}", ['user' => $user->username, 'path' => $user->cover?->filePath, 'message' => \get_class($e).': '.$e->getMessage()]); } - $filePathsOfUser = $this->userManager->getAllImageFilePathsOfUser($user); - foreach ($filePathsOfUser as $path) { - try { - $this->imageManager->remove($path); - } catch (\Exception|\Error $e) { - $this->logger->error("[ClearDeletedUserHandler::__invoke] Couldn't delete image of {user} at '{path}': {message}", ['user' => $user->username, 'path' => $path, 'message' => \get_class($e).': '.$e->getMessage()]); - } + + $imagesOfUser = $this->userManager->getAllImagesShaAndFilepathOfUser($user); + $deleteImagesPayload = []; + foreach ($imagesOfUser as $row) { + $deleteImagesPayload[$row[0]] = $row[1]; } $this->entityManager->beginTransaction(); @@ -122,6 +121,10 @@ public function doWork(MessageInterface $message): void throw $e; } + + // dispatch at end or else reference-check would keep images + // because of the reference check this call can be safely placed outside the try{} + $this->bus->dispatch(new DeleteImageV2Message($deleteImagesPayload)); } private function sendDeleteMessages(array $targetInboxes, User $deletedUser): void diff --git a/src/Repository/ImageRepository.php b/src/Repository/ImageRepository.php index 453712c57d..da9fd94bf4 100644 --- a/src/Repository/ImageRepository.php +++ b/src/Repository/ImageRepository.php @@ -305,4 +305,21 @@ public function redownloadImagesIfNecessary(array $images): void } $this->getEntityManager()->flush(); } + + public function isImageReferenced(Image $image): bool + { + $resp = $this->getEntityManager()->getConnection()->executeQuery(' + SELECT 1 as result WHERE + EXISTS (SELECT 1 FROM entry WHERE image_id = :img) + OR EXISTS (SELECT 1 FROM entry_comment WHERE image_id = :img) + OR EXISTS (SELECT 1 FROM post WHERE image_id = :img) + OR EXISTS (SELECT 1 FROM post_comment WHERE image_id = :img) + OR EXISTS (SELECT 1 FROM "user" WHERE avatar_id = :img OR cover_id = :img) + OR EXISTS (SELECT 1 FROM magazine WHERE icon_id = :img OR banner_id = :img) + OR EXISTS (SELECT 1 FROM oauth2_client WHERE image_id = :img); + ', ['img' => $image->getId()]); + $res = $resp->fetchAssociative(); + + return false !== $res && 1 === $res['result']; + } } diff --git a/src/Service/UserManager.php b/src/Service/UserManager.php index f5a64d05ba..e6a63edbc3 100644 --- a/src/Service/UserManager.php +++ b/src/Service/UserManager.php @@ -19,7 +19,7 @@ use App\Exception\UserCannotBeBanned; use App\Factory\UserFactory; use App\Message\ClearDeletedUserMessage; -use App\Message\DeleteImageMessage; +use App\Message\DeleteImageV2Message; use App\Message\DeleteUserMessage; use App\Message\Notification\SentNewSignupNotificationMessage; use App\Message\UserCreatedMessage; @@ -269,11 +269,11 @@ public function edit(User $user, UserDto $dto): User } if ($oldAvatar && $user->avatar !== $oldAvatar) { - $this->bus->dispatch(new DeleteImageMessage($oldAvatar->getId())); + $this->bus->dispatch(DeleteImageV2Message::fromImage($oldAvatar)); } if ($oldCover && $user->cover !== $oldCover) { - $this->bus->dispatch(new DeleteImageMessage($oldCover->getId())); + $this->bus->dispatch(DeleteImageV2Message::fromImage($oldCover)); } if ($mailUpdated) { @@ -352,14 +352,14 @@ public function detachAvatar(User $user): void return; } - $image = $user->avatar->getId(); + $imageRmMessage = DeleteImageV2Message::fromImage($user->avatar); $user->avatar = null; $this->entityManager->persist($user); $this->entityManager->flush(); - $this->bus->dispatch(new DeleteImageMessage($image)); + $this->bus->dispatch($imageRmMessage); } public function detachCover(User $user): void @@ -368,14 +368,14 @@ public function detachCover(User $user): void return; } - $image = $user->cover->getId(); + $imageRmMessage = DeleteImageV2Message::fromImage($user->cover); $user->cover = null; $this->entityManager->persist($user); $this->entityManager->flush(); - $this->bus->dispatch(new DeleteImageMessage($image)); + $this->bus->dispatch($imageRmMessage); } /** @@ -791,49 +791,54 @@ public function findAllKnownInboxesNotBannedNotDead(): array } /** - * This method will return all image paths that the user **owns**, + * This method will return the sha256 (hex) and filepath of all images that the user **owns**, * meaning that that image belongs only to posts from the user and not to anybody else's. * - * @return string[] + * @return array [0 => string sha256, 1 => ?string filePath] */ - public function getAllImageFilePathsOfUser(User $user): array + public function getAllImagesShaAndFilepathOfUser(User $user): array { $sql = ' - SELECT i1.file_path FROM entry e INNER JOIN image i1 ON e.image_id = i1.id + SELECT i1.sha256, i1.file_path FROM entry e INNER JOIN image i1 ON e.image_id = i1.id WHERE e.user_id = :userId AND i1.file_path IS NOT NULL AND NOT EXISTS (SELECT id FROM entry e2 WHERE e2.user_id <> :userId AND e2.image_id = i1.id) AND NOT EXISTS (SELECT id FROM post p2 WHERE p2.user_id <> :userId AND p2.image_id = i1.id) AND NOT EXISTS (SELECT id FROM entry_comment ec2 WHERE ec2.user_id <> :userId AND ec2.image_id = i1.id) AND NOT EXISTS (SELECT id FROM post_comment pc2 WHERE pc2.user_id <> :userId AND pc2.image_id = i1.id) UNION DISTINCT - SELECT i2.file_path FROM post p INNER JOIN image i2 ON p.image_id = i2.id + SELECT i2.sha256, i2.file_path FROM post p INNER JOIN image i2 ON p.image_id = i2.id WHERE p.user_id = :userId AND i2.file_path IS NOT NULL AND NOT EXISTS (SELECT id FROM entry e2 WHERE e2.user_id <> :userId AND e2.image_id = i2.id) AND NOT EXISTS (SELECT id FROM post p2 WHERE p2.user_id <> :userId AND p2.image_id = i2.id) AND NOT EXISTS (SELECT id FROM entry_comment ec2 WHERE ec2.user_id <> :userId AND ec2.image_id = i2.id) AND NOT EXISTS (SELECT id FROM post_comment pc2 WHERE pc2.user_id <> :userId AND pc2.image_id = i2.id) UNION DISTINCT - SELECT i3.file_path FROM entry_comment ec INNER JOIN image i3 ON ec.image_id = i3.id + SELECT i3.sha256, i3.file_path FROM entry_comment ec INNER JOIN image i3 ON ec.image_id = i3.id WHERE ec.user_id = :userId AND i3.file_path IS NOT NULL AND NOT EXISTS (SELECT id FROM entry e2 WHERE e2.user_id <> :userId AND e2.image_id = i3.id) AND NOT EXISTS (SELECT id FROM post p2 WHERE p2.user_id <> :userId AND p2.image_id = i3.id) AND NOT EXISTS (SELECT id FROM entry_comment ec2 WHERE ec2.user_id <> :userId AND ec2.image_id = i3.id) AND NOT EXISTS (SELECT id FROM post_comment pc2 WHERE pc2.user_id <> :userId AND pc2.image_id = i3.id) UNION DISTINCT - SELECT i4.file_path FROM post_comment pc INNER JOIN image i4 ON pc.image_id = i4.id + SELECT i4.sha256, i4.file_path FROM post_comment pc INNER JOIN image i4 ON pc.image_id = i4.id WHERE pc.user_id = :userId AND i4.file_path IS NOT NULL AND NOT EXISTS (SELECT id FROM entry e2 WHERE e2.user_id <> :userId AND e2.image_id = i4.id) AND NOT EXISTS (SELECT id FROM post p2 WHERE p2.user_id <> :userId AND p2.image_id = i4.id) AND NOT EXISTS (SELECT id FROM entry_comment ec2 WHERE ec2.user_id <> :userId AND ec2.image_id = i4.id) AND NOT EXISTS (SELECT id FROM post_comment pc2 WHERE pc2.user_id <> :userId AND pc2.image_id = i4.id) '; + $rsm = new ResultSetMapping(); - $rsm->addScalarResult('file_path', 0); + $rsm->addScalarResult('sha256', 0); + $rsm->addScalarResult('file_path', 1); $result = $this->entityManager->createNativeQuery($sql, $rsm) ->setParameter(':userId', $user->getId()) ->getScalarResult(); - return array_filter(array_map(fn ($row) => $row[0], $result)); + return array_map(fn ($row) => [ + bin2hex(stream_get_contents($row[0])), + $row[1], + ], array_filter($result)); } } From e720933ed03ca5356c81fc243d6f5ab84ded92dc Mon Sep 17 00:00:00 2001 From: blued_gear Date: Wed, 5 Aug 2026 20:51:36 +0000 Subject: [PATCH 2/5] add tests --- .../Repository/ImageRepositoryTest.php | 135 ++++++++++++++++++ 1 file changed, 135 insertions(+) create mode 100644 tests/Functional/Service/Repository/ImageRepositoryTest.php diff --git a/tests/Functional/Service/Repository/ImageRepositoryTest.php b/tests/Functional/Service/Repository/ImageRepositoryTest.php new file mode 100644 index 0000000000..77fbd96de6 --- /dev/null +++ b/tests/Functional/Service/Repository/ImageRepositoryTest.php @@ -0,0 +1,135 @@ +createImage('test.png'); + + self::assertFalse($this->imageRepository->isImageReferenced($img)); + } + + public function testIsImageReferencedEntry() + { + $img = $this->createImage('test.png'); + + $entry = $this->getEntryByTitle('entry'); + $entry->image = $img; + $this->entityManager->persist($entry); + $this->entityManager->flush(); + + self::assertTrue($this->imageRepository->isImageReferenced($img)); + } + + public function testIsImageReferencedEntryComment() + { + $img = $this->createImage('test.png'); + + $entry = $this->getEntryByTitle('entry'); + $comment = $this->createEntryComment('image', $entry); + $comment->image = $img; + $this->entityManager->persist($comment); + $this->entityManager->flush(); + + self::assertTrue($this->imageRepository->isImageReferenced($img)); + } + + public function testIsImageReferencedPost() + { + $img = $this->createImage('test.png'); + + $post = $this->createPost('post'); + $post->image = $img; + $this->entityManager->persist($post); + $this->entityManager->flush(); + + self::assertTrue($this->imageRepository->isImageReferenced($img)); + } + + public function testIsImageReferencedPostComment() + { + $img = $this->createImage('test.png'); + + $post = $this->createPost('post'); + $comment = $this->createPostComment('image', $post); + $comment->image = $img; + $this->entityManager->persist($comment); + $this->entityManager->flush(); + + self::assertTrue($this->imageRepository->isImageReferenced($img)); + } + + public function testIsImageReferencedUserAvatar() + { + $img = $this->createImage('test.png'); + + $user = $this->getUserByUsername('someone'); + $user->avatar = $img; + $this->entityManager->persist($user); + $this->entityManager->flush(); + + self::assertTrue($this->imageRepository->isImageReferenced($img)); + } + + public function testIsImageReferencedUserCover() + { + $img = $this->createImage('test.png'); + + $user = $this->getUserByUsername('someone'); + $user->cover = $img; + $this->entityManager->persist($user); + $this->entityManager->flush(); + + self::assertTrue($this->imageRepository->isImageReferenced($img)); + } + + public function testIsImageReferencedMagazineIcon() + { + $img = $this->createImage('test.png'); + + $magazine = $this->getMagazineByName('somemagazine'); + $magazine->icon = $img; + $this->entityManager->persist($magazine); + $this->entityManager->flush(); + + self::assertTrue($this->imageRepository->isImageReferenced($img)); + } + + public function testIsImageReferencedMagazineBanner() + { + $img = $this->createImage('test.png'); + + $magazine = $this->getMagazineByName('somemagazine'); + $magazine->banner = $img; + $this->entityManager->persist($magazine); + $this->entityManager->flush(); + + self::assertTrue($this->imageRepository->isImageReferenced($img)); + } + + public function testIsImageReferencedOAuthClient() + { + $img = $this->createImage('test.png'); + + /** @var ClientManagerInterface $manager */ + $manager = self::getContainer()->get(ClientManagerInterface::class); + $client = new Client('dummy client', 'dummyclient', 'testsecret'); + $client->setDescription('An OAuth2 client for testIsImageReferencedOAuthClient'); + $client->setContactEmail('testIsImageReferencedOAuthClient@kbin.test'); + $client->setGrants(new Grant('authorization_code'), new Grant('refresh_token')); + $client->setRedirectUris(new RedirectUri('https://localhost:3001')); + $client->setImage($img); + $manager->save($client); + + self::assertTrue($this->imageRepository->isImageReferenced($img)); + } +} From da717baaae9c73b2d75fabfa8901cc42517a5c1d Mon Sep 17 00:00:00 2001 From: blued_gear Date: Thu, 6 Aug 2026 20:51:59 +0000 Subject: [PATCH 3/5] improved approach --- src/Message/DeleteImageV2Message.php | 9 + src/MessageHandler/DeleteImageV2Handler.php | 82 +++++--- src/Repository/ImageRepository.php | 72 +++++-- .../DeleteImageV2HandlerTest.php | 145 +++++++++++++++ .../ImageRepositoryReferenceCheckTest.php | 175 ++++++++++++++++++ .../Repository/ImageRepositoryTest.php | 144 ++------------ tests/WebTestCase.php | 2 +- 7 files changed, 467 insertions(+), 162 deletions(-) create mode 100644 tests/Functional/Service/MessageHandlers/DeleteImageV2HandlerTest.php create mode 100644 tests/Functional/Service/Repository/ImageRepositoryReferenceCheckTest.php diff --git a/src/Message/DeleteImageV2Message.php b/src/Message/DeleteImageV2Message.php index 0523e85e3b..9ff7934d5a 100644 --- a/src/Message/DeleteImageV2Message.php +++ b/src/Message/DeleteImageV2Message.php @@ -25,4 +25,13 @@ public static function fromImage(Image $img): self { return new self([bin2hex($img->sha256) => $img->filePath]); } + + public static function fromImages(array $imgs): self + { + $content = []; + foreach ($imgs as $img) { + $content[bin2hex($img->sha256)] = $img->filePath; + } + return new self($content); + } } diff --git a/src/MessageHandler/DeleteImageV2Handler.php b/src/MessageHandler/DeleteImageV2Handler.php index 059e4f2871..2ec476c9c1 100644 --- a/src/MessageHandler/DeleteImageV2Handler.php +++ b/src/MessageHandler/DeleteImageV2Handler.php @@ -4,56 +4,92 @@ namespace App\MessageHandler; -use App\Message\Contracts\MessageInterface; +use App\Entity\Image; use App\Message\DeleteImageV2Message; use App\Repository\ImageRepository; use App\Service\ImageManagerInterface; use Doctrine\ORM\EntityManagerInterface; +use Psr\Log\LoggerInterface; use Symfony\Component\HttpKernel\KernelInterface; use Symfony\Component\Messenger\Attribute\AsMessageHandler; #[AsMessageHandler] -class DeleteImageV2Handler extends MbinMessageHandler +readonly class DeleteImageV2Handler { + private int $chunkSize; + public function __construct( KernelInterface $kernel, private EntityManagerInterface $entityManager, private ImageRepository $imageRepository, private ImageManagerInterface $imageManager, + private LoggerInterface $logger, ) { - parent::__construct($this->entityManager, $kernel); + $this->chunkSize = ('test' !== $kernel->getEnvironment()) ? 256 : 2; } public function __invoke(DeleteImageV2Message $message): void { - $this->workWrapper($message); + try { + /* + * Split workload into chunks to balance overhead with probability of race-conditions. A race-condition can look like the following: + * 1. one image in a chunk is detected as an orphan + * 2. entity is deleted and transaction commited + * 3. new content is created with the same image (resulting in the same file path) while $filesToDelete is iterated + * 4. the file is deleted; the new image entity created by the new content now has an invalid path + */ + $batches = array_chunk($message->images, $this->chunkSize, true); + foreach ($batches as $batch) { + $this->processBatch($batch); + } + } finally { + gc_collect_cycles(); + } } - public function doWork(MessageInterface $message): void + private function processBatch(array $batch): void { - if (!$message instanceof DeleteImageV2Message) { - throw new \LogicException(); - } + $conn = $this->entityManager->getConnection(); + $conn->getNativeConnection(); // calls connect() internally - // querying each image individually is inefficient but lowers the probability of a race-condition - foreach ($message->images as $hash => $filepath) { - $img = $this->imageRepository->findOneBySha256(hex2bin($hash)); - if (null !== $img) { - if (!$this->imageRepository->isImageReferenced($img)) { - // this should lock the row and prevent concurrent re-referencing - $this->entityManager->remove($img); - $this->entityManager->flush(); + /** @var string[] $filesToDelete */ + $filesToDelete = $conn->transactional(function () use ($batch) { + $filesToDelete = []; + $hashes = array_map(fn ($str) => hex2bin($str), array_keys($batch)); + $images = $this->imageRepository->findMultipleBySha256AndLock($hashes); - $deleteFile = true; - } else { - $deleteFile = false; + // images which not exist in DB can be deleted + foreach ($batch as $hash => $filepath) { + $hashBin = hex2bin($hash); + if (!array_any($images, fn ($img) => $img->sha256 === $hashBin)) { + $filesToDelete[] = $filepath; } - } else { - $deleteFile = true; } - if ($deleteFile && null !== $filepath) { - $this->imageManager->remove($filepath); + // images which are not referenced can be deleted + $referenced = $this->imageRepository->areImagesReferenced($images); + foreach ($referenced as $imgId => $isReferenced) { + if (!$isReferenced) { + $img = array_find($images, fn ($img) => $img->getId() === $imgId); + \assert($img instanceof Image); + + $filesToDelete[] = $img->filePath; + $this->entityManager->remove($img); + } + } + + $this->entityManager->flush(); + return $filesToDelete; + }); + + foreach ($filesToDelete as $path) { + try { + $this->imageManager->remove($path); + } catch (\Exception $e) { + $this->logger->error('[DeleteImageV2Handler]: an error occurred when deleting an image file: {type} - {message}', [ + 'message' => $e->getMessage(), + 'type' => \get_class($e), + ]); } } } diff --git a/src/Repository/ImageRepository.php b/src/Repository/ImageRepository.php index da9fd94bf4..6e1085c06e 100644 --- a/src/Repository/ImageRepository.php +++ b/src/Repository/ImageRepository.php @@ -13,8 +13,11 @@ use App\Pagination\Transformation\ContentPopulationTransformer; use App\Service\ImageManagerInterface; use App\Utils\ImageOrigin; +use App\Utils\SqlHelpers; use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository; +use Doctrine\DBAL\ArrayParameterType; use Doctrine\DBAL\Exception; +use Doctrine\DBAL\LockMode; use Doctrine\Persistence\ManagerRegistry; use kornrunner\Blurhash\Blurhash; use Psr\EventDispatcher\EventDispatcherInterface; @@ -40,6 +43,21 @@ public function __construct( parent::__construct($registry, Image::class); } + /** + * @param array $hashes array of hashes (binary) + * + * @return Image[] + */ + public function findMultipleBySha256AndLock(array $hashes): array + { + return $this->createQueryBuilder('i') + ->where('i.sha256 IN (:hashes)') + ->setParameter('hashes', $hashes, ArrayParameterType::BINARY) + ->getQuery() + ->setLockMode(LockMode::PESSIMISTIC_READ) + ->getResult(); + } + /** * Process and store an uploaded image. * @@ -306,20 +324,46 @@ public function redownloadImagesIfNecessary(array $images): void $this->getEntityManager()->flush(); } - public function isImageReferenced(Image $image): bool + /** + * @param Image[] $images + * + * @return array image id => is referenced + */ + public function areImagesReferenced(array $images): array { - $resp = $this->getEntityManager()->getConnection()->executeQuery(' - SELECT 1 as result WHERE - EXISTS (SELECT 1 FROM entry WHERE image_id = :img) - OR EXISTS (SELECT 1 FROM entry_comment WHERE image_id = :img) - OR EXISTS (SELECT 1 FROM post WHERE image_id = :img) - OR EXISTS (SELECT 1 FROM post_comment WHERE image_id = :img) - OR EXISTS (SELECT 1 FROM "user" WHERE avatar_id = :img OR cover_id = :img) - OR EXISTS (SELECT 1 FROM magazine WHERE icon_id = :img OR banner_id = :img) - OR EXISTS (SELECT 1 FROM oauth2_client WHERE image_id = :img); - ', ['img' => $image->getId()]); - $res = $resp->fetchAssociative(); - - return false !== $res && 1 === $res['result']; + $sql = ' + SELECT i.id, (CASE WHEN + e.image_id IS NOT NULL + OR ec.image_id IS NOT NULL + OR p.image_id IS NOT NULL + OR pc.image_id IS NOT NULL + OR u.avatar_id IS NOT NULL + OR u.cover_id IS NOT NULL + OR m.icon_id IS NOT NULL + OR m.banner_id IS NOT NULL + OR oc.image_id IS NOT NULL + THEN TRUE ELSE FALSE END) AS referenced FROM image i + LEFT OUTER JOIN entry e ON i.id = e.image_id + LEFT OUTER JOIN entry_comment ec ON i.id = ec.image_id + LEFT OUTER JOIN post p ON i.id = p.image_id + LEFT OUTER JOIN post_comment pc ON i.id = pc.image_id + LEFT OUTER JOIN "user" u ON i.id = u.avatar_id OR i.id = u.cover_id + LEFT OUTER JOIN magazine m ON i.id = m.icon_id OR i.id = m.banner_id + LEFT OUTER JOIN oauth2_client oc ON i.id = oc.image_id + WHERE i.id IN (:ids); + '; + + $sql = SqlHelpers::rewriteArrayParameters(['ids' => array_map(fn ($img) => $img->getId(), $images)], $sql); + $stmt = $this->getEntityManager()->getConnection()->prepare($sql['sql']); + foreach ($sql['parameters'] as $param => $value) { + $stmt->bindValue($param, $value, SqlHelpers::getSqlType($value)); + } + + $rows = $stmt->executeQuery()->fetchAllAssociative(); + $ret = []; + foreach ($rows as $row) { + $ret[$row['id']] = $row['referenced']; + } + return $ret; } } diff --git a/tests/Functional/Service/MessageHandlers/DeleteImageV2HandlerTest.php b/tests/Functional/Service/MessageHandlers/DeleteImageV2HandlerTest.php new file mode 100644 index 0000000000..2db5acec3b --- /dev/null +++ b/tests/Functional/Service/MessageHandlers/DeleteImageV2HandlerTest.php @@ -0,0 +1,145 @@ +handler = $this->getService(DeleteImageV2Handler::class); + } + + public function testCanDeleteAllImages(): void + { + $img1 = $this->createTestImage('test1.png'); + $img2 = $this->createTestImage('test2.png'); + $img3 = $this->createTestImage('test3.png'); + $message = DeleteImageV2Message::fromImages([$img1, $img2, $img3]); + + try { + ($this->handler)($message); + + self::assertNull($this->imageRepository->findOneBySha256($img1->sha256)); + self::assertNull($this->imageRepository->findOneBySha256($img2->sha256)); + self::assertNull($this->imageRepository->findOneBySha256($img3->sha256)); + self::assertFalse(file_exists($this->imageFilePath($img1->filePath))); + self::assertFalse(file_exists($this->imageFilePath($img2->filePath))); + self::assertFalse(file_exists($this->imageFilePath($img3->filePath))); + } finally { + $this->removeImageFile($img1->filePath); + $this->removeImageFile($img2->filePath); + $this->removeImageFile($img3->filePath); + } + } + + public function testCanSkipReferencedImages(): void + { + $img1 = $this->createTestImage('test1.png'); + $img2 = $this->createTestImage('test2.png'); + $img3 = $this->createTestImage('test3.png'); + $message = DeleteImageV2Message::fromImages([$img1, $img2, $img3]); + + try { + $entry = $this->getEntryByTitle('entry'); + $entry->image = $img2; + $this->entityManager->persist($entry); + $this->entityManager->flush(); + + ($this->handler)($message); + + self::assertNull($this->imageRepository->findOneBySha256($img1->sha256)); + self::assertNotNull($this->imageRepository->findOneBySha256($img2->sha256)); + self::assertNull($this->imageRepository->findOneBySha256($img3->sha256)); + self::assertFalse(file_exists($this->imageFilePath($img1->filePath))); + self::assertTrue(file_exists($this->imageFilePath($img2->filePath))); + self::assertFalse(file_exists($this->imageFilePath($img3->filePath))); + self::assertEquals($img2->getId(), $this->imageRepository->findOneBySha256($img2->sha256)->getId()); + } finally { + $this->removeImageFile($img1->filePath); + $this->removeImageFile($img2->filePath); + $this->removeImageFile($img3->filePath); + } + } + + public function testCanDeleteOrphansImages(): void + { + $img1 = $this->createTestImage('test1.png'); + $img2 = $this->createTestImage('test2.png'); + $img3 = $this->createTestImage('test3.png'); + $message = DeleteImageV2Message::fromImages([$img1, $img2, $img3]); + + try { + $entry = $this->getEntryByTitle('entry'); + $entry->image = $img2; + $this->entityManager->persist($entry); + $this->entityManager->flush(); + + $this->entityManager->remove($img1); + $this->entityManager->remove($img3); + $this->entityManager->flush(); + + ($this->handler)($message); + + self::assertNull($this->imageRepository->findOneBySha256($img1->sha256)); + self::assertNotNull($this->imageRepository->findOneBySha256($img2->sha256)); + self::assertNull($this->imageRepository->findOneBySha256($img3->sha256)); + self::assertFalse(file_exists($this->imageFilePath($img1->filePath))); + self::assertTrue(file_exists($this->imageFilePath($img2->filePath))); + self::assertFalse(file_exists($this->imageFilePath($img3->filePath))); + self::assertEquals($img2->getId(), $this->imageRepository->findOneBySha256($img2->sha256)->getId()); + } finally { + $this->removeImageFile($img1->filePath); + $this->removeImageFile($img2->filePath); + $this->removeImageFile($img3->filePath); + } + } + + private function createTestImage($fileName): Image + { + if (!file_exists($this->imageUploadTmpDir)) { + if (!mkdir($this->imageUploadTmpDir)) { + throw new \Exception('The copy dir could not be created'); + } + } + + $filePathName = $fileName.'.'.bin2hex(random_bytes(32)); + $f = fopen($this->imageFilePath($filePathName), 'w'); + if (!$f) { + throw new \Exception('The dummy file could not be created'); + } + fclose($f); + + $image = new Image( + $fileName, + $filePathName, + hash('sha256', $fileName), + 100, + 100, + null, + ); + $this->entityManager->persist($image); + $this->entityManager->flush(); + + return $image; + } + + private function removeImageFile(string $filename): void + { + @unlink($this->imageFilePath($filename)); + } + + private function imageFilePath(string $filename): string + { + return $this->imageUploadTmpDir.'../../../public/media/'.$filename; + } +} diff --git a/tests/Functional/Service/Repository/ImageRepositoryReferenceCheckTest.php b/tests/Functional/Service/Repository/ImageRepositoryReferenceCheckTest.php new file mode 100644 index 0000000000..9c796761d8 --- /dev/null +++ b/tests/Functional/Service/Repository/ImageRepositoryReferenceCheckTest.php @@ -0,0 +1,175 @@ +createImage('test1.png'); + $img2 = $this->createImage('test2.png'); + + $result = $this->imageRepository->areImagesReferenced([$img1, $img2]); + self::assertCount(2, $result); + self::assertFalse($result[$img1->getId()]); + self::assertFalse($result[$img2->getId()]); + } + + public function testIsImageReferencedEntry() + { + $img1 = $this->createImage('test1.png'); + $img2 = $this->createImage('test2.png'); + + $entry = $this->getEntryByTitle('entry'); + $entry->image = $img1; + $this->entityManager->persist($entry); + $this->entityManager->flush(); + + $result = $this->imageRepository->areImagesReferenced([$img1, $img2]); + self::assertCount(2, $result); + self::assertTrue($result[$img1->getId()]); + self::assertFalse($result[$img2->getId()]); + } + + public function testIsImageReferencedEntryComment() + { + $img1 = $this->createImage('test1.png'); + $img2 = $this->createImage('test2.png'); + + $entry = $this->getEntryByTitle('entry'); + $comment = $this->createEntryComment('image', $entry); + $comment->image = $img1; + $this->entityManager->persist($comment); + $this->entityManager->flush(); + + $result = $this->imageRepository->areImagesReferenced([$img1, $img2]); + self::assertCount(2, $result); + self::assertTrue($result[$img1->getId()]); + self::assertFalse($result[$img2->getId()]); + } + + public function testIsImageReferencedPost() + { + $img1 = $this->createImage('test1.png'); + $img2 = $this->createImage('test2.png'); + + $post = $this->createPost('post'); + $post->image = $img1; + $this->entityManager->persist($post); + $this->entityManager->flush(); + + $result = $this->imageRepository->areImagesReferenced([$img1, $img2]); + self::assertCount(2, $result); + self::assertTrue($result[$img1->getId()]); + self::assertFalse($result[$img2->getId()]); + } + + public function testIsImageReferencedPostComment() + { + $img1 = $this->createImage('test1.png'); + $img2 = $this->createImage('test2.png'); + + $post = $this->createPost('post'); + $comment = $this->createPostComment('image', $post); + $comment->image = $img1; + $this->entityManager->persist($comment); + $this->entityManager->flush(); + + $result = $this->imageRepository->areImagesReferenced([$img1, $img2]); + self::assertCount(2, $result); + self::assertTrue($result[$img1->getId()]); + self::assertFalse($result[$img2->getId()]); + } + + public function testIsImageReferencedUserAvatar() + { + $img1 = $this->createImage('test1.png'); + $img2 = $this->createImage('test2.png'); + + $user = $this->getUserByUsername('someone'); + $user->avatar = $img1; + $this->entityManager->persist($user); + $this->entityManager->flush(); + + $result = $this->imageRepository->areImagesReferenced([$img1, $img2]); + self::assertCount(2, $result); + self::assertTrue($result[$img1->getId()]); + self::assertFalse($result[$img2->getId()]); + } + + public function testIsImageReferencedUserCover() + { + $img1 = $this->createImage('test1.png'); + $img2 = $this->createImage('test2.png'); + + $user = $this->getUserByUsername('someone'); + $user->cover = $img1; + $this->entityManager->persist($user); + $this->entityManager->flush(); + + $result = $this->imageRepository->areImagesReferenced([$img1, $img2]); + self::assertCount(2, $result); + self::assertTrue($result[$img1->getId()]); + self::assertFalse($result[$img2->getId()]); + } + + public function testIsImageReferencedMagazineIcon() + { + $img1 = $this->createImage('test1.png'); + $img2 = $this->createImage('test2.png'); + + $magazine = $this->getMagazineByName('somemagazine'); + $magazine->icon = $img1; + $this->entityManager->persist($magazine); + $this->entityManager->flush(); + + $result = $this->imageRepository->areImagesReferenced([$img1, $img2]); + self::assertCount(2, $result); + self::assertTrue($result[$img1->getId()]); + self::assertFalse($result[$img2->getId()]); + } + + public function testIsImageReferencedMagazineBanner() + { + $img1 = $this->createImage('test1.png'); + $img2 = $this->createImage('test2.png'); + + $magazine = $this->getMagazineByName('somemagazine'); + $magazine->banner = $img1; + $this->entityManager->persist($magazine); + $this->entityManager->flush(); + + $result = $this->imageRepository->areImagesReferenced([$img1, $img2]); + self::assertCount(2, $result); + self::assertTrue($result[$img1->getId()]); + self::assertFalse($result[$img2->getId()]); + } + + public function testIsImageReferencedOAuthClient() + { + $img1 = $this->createImage('test1.png'); + $img2 = $this->createImage('test2.png'); + + /** @var ClientManagerInterface $manager */ + $manager = self::getContainer()->get(ClientManagerInterface::class); + $client = new Client('dummy client', 'dummyclient', 'testsecret'); + $client->setDescription('An OAuth2 client for testIsImageReferencedOAuthClient'); + $client->setContactEmail('testIsImageReferencedOAuthClient@kbin.test'); + $client->setGrants(new Grant('authorization_code'), new Grant('refresh_token')); + $client->setRedirectUris(new RedirectUri('https://localhost:3001')); + $client->setImage($img1); + $manager->save($client); + + $result = $this->imageRepository->areImagesReferenced([$img1, $img2]); + self::assertCount(2, $result); + self::assertTrue($result[$img1->getId()]); + self::assertFalse($result[$img2->getId()]); + } +} diff --git a/tests/Functional/Service/Repository/ImageRepositoryTest.php b/tests/Functional/Service/Repository/ImageRepositoryTest.php index 77fbd96de6..728728d52e 100644 --- a/tests/Functional/Service/Repository/ImageRepositoryTest.php +++ b/tests/Functional/Service/Repository/ImageRepositoryTest.php @@ -3,133 +3,29 @@ namespace App\Tests\Functional\Service\Repository; -use App\Entity\Client; use App\Tests\WebTestCase; -use League\Bundle\OAuth2ServerBundle\Manager\ClientManagerInterface; -use League\Bundle\OAuth2ServerBundle\ValueObject\Grant; -use League\Bundle\OAuth2ServerBundle\ValueObject\RedirectUri; class ImageRepositoryTest extends WebTestCase { - public function testIsImageReferencedEntryNone() - { - $img = $this->createImage('test.png'); - - self::assertFalse($this->imageRepository->isImageReferenced($img)); - } - - public function testIsImageReferencedEntry() - { - $img = $this->createImage('test.png'); - - $entry = $this->getEntryByTitle('entry'); - $entry->image = $img; - $this->entityManager->persist($entry); - $this->entityManager->flush(); - - self::assertTrue($this->imageRepository->isImageReferenced($img)); - } - - public function testIsImageReferencedEntryComment() - { - $img = $this->createImage('test.png'); - - $entry = $this->getEntryByTitle('entry'); - $comment = $this->createEntryComment('image', $entry); - $comment->image = $img; - $this->entityManager->persist($comment); - $this->entityManager->flush(); - - self::assertTrue($this->imageRepository->isImageReferenced($img)); - } - - public function testIsImageReferencedPost() - { - $img = $this->createImage('test.png'); - - $post = $this->createPost('post'); - $post->image = $img; - $this->entityManager->persist($post); - $this->entityManager->flush(); - - self::assertTrue($this->imageRepository->isImageReferenced($img)); - } - - public function testIsImageReferencedPostComment() - { - $img = $this->createImage('test.png'); - - $post = $this->createPost('post'); - $comment = $this->createPostComment('image', $post); - $comment->image = $img; - $this->entityManager->persist($comment); - $this->entityManager->flush(); - - self::assertTrue($this->imageRepository->isImageReferenced($img)); - } - - public function testIsImageReferencedUserAvatar() - { - $img = $this->createImage('test.png'); - - $user = $this->getUserByUsername('someone'); - $user->avatar = $img; - $this->entityManager->persist($user); - $this->entityManager->flush(); - - self::assertTrue($this->imageRepository->isImageReferenced($img)); - } - - public function testIsImageReferencedUserCover() - { - $img = $this->createImage('test.png'); - - $user = $this->getUserByUsername('someone'); - $user->cover = $img; - $this->entityManager->persist($user); - $this->entityManager->flush(); - - self::assertTrue($this->imageRepository->isImageReferenced($img)); - } - - public function testIsImageReferencedMagazineIcon() - { - $img = $this->createImage('test.png'); - - $magazine = $this->getMagazineByName('somemagazine'); - $magazine->icon = $img; - $this->entityManager->persist($magazine); - $this->entityManager->flush(); - - self::assertTrue($this->imageRepository->isImageReferenced($img)); - } - - public function testIsImageReferencedMagazineBanner() - { - $img = $this->createImage('test.png'); - - $magazine = $this->getMagazineByName('somemagazine'); - $magazine->banner = $img; - $this->entityManager->persist($magazine); - $this->entityManager->flush(); - - self::assertTrue($this->imageRepository->isImageReferenced($img)); - } - - public function testIsImageReferencedOAuthClient() - { - $img = $this->createImage('test.png'); - - /** @var ClientManagerInterface $manager */ - $manager = self::getContainer()->get(ClientManagerInterface::class); - $client = new Client('dummy client', 'dummyclient', 'testsecret'); - $client->setDescription('An OAuth2 client for testIsImageReferencedOAuthClient'); - $client->setContactEmail('testIsImageReferencedOAuthClient@kbin.test'); - $client->setGrants(new Grant('authorization_code'), new Grant('refresh_token')); - $client->setRedirectUris(new RedirectUri('https://localhost:3001')); - $client->setImage($img); - $manager->save($client); - - self::assertTrue($this->imageRepository->isImageReferenced($img)); + public function testFindMultipleBySha256AndLock(): void + { + $img1 = $this->createImage('test1.png'); + $img2 = $this->createImage('test2.png'); + $this->createImage('test3.png'); + + $this->entityManager->wrapInTransaction(function () use ($img1, $img2) { + $result = $this->imageRepository->findMultipleBySha256AndLock([ + $img1->sha256, + $img2->sha256, + hex2bin('0000000000000000000000000000000000000000000000000000000000000000'), + ]); + + self::assertCount(2, $result); + self::assertContains($img1, $result); + self::assertContains($img2, $result); + + $locks = $this->entityManager->getConnection()->fetchAllAssociative('SELECT * FROM pg_locks l JOIN pg_stat_all_tables t ON l.relation = t.relid WHERE t.relname = \'image\' AND l.granted = TRUE AND l.mode = \'RowShareLock\''); + self::assertCount(1, $locks); + }); } } diff --git a/tests/WebTestCase.php b/tests/WebTestCase.php index 6fef1207da..07bf4edde2 100644 --- a/tests/WebTestCase.php +++ b/tests/WebTestCase.php @@ -288,7 +288,7 @@ public function setUp(): void * * @return T */ - private function getService(string $className) + protected function getService(string $className) { return $this->getContainer()->get($className); } From f1cbdc5025526a822f4f03c52dd4c331b1999200 Mon Sep 17 00:00:00 2001 From: Melroy van den Berg Date: Fri, 7 Aug 2026 21:14:23 +0200 Subject: [PATCH 4/5] trigger cicd From bce137eba3a88c096ec593575a63730f625b2532 Mon Sep 17 00:00:00 2001 From: blued_gear Date: Sat, 8 Aug 2026 16:00:21 +0000 Subject: [PATCH 5/5] fix tests --- tests/ActivityPubTestCase.php | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/tests/ActivityPubTestCase.php b/tests/ActivityPubTestCase.php index d42f5aaa62..732483f7f4 100644 --- a/tests/ActivityPubTestCase.php +++ b/tests/ActivityPubTestCase.php @@ -89,18 +89,6 @@ public function setUp(): void $this->apMarkdownConverter = $this->getService(MarkdownConverter::class); } - /** - * @template T - * - * @param class-string $className - * - * @return T - */ - private function getService(string $className) - { - return $this->getContainer()->get($className); - } - protected function getDefaultUuid(): Uuid { return new Uuid('00000000-0000-0000-0000-000000000000');