Skip to content

intoduce new async_slow queue for image deletion - #2199

Open
blued-gear wants to merge 7 commits into
mainfrom
new/async_slow
Open

intoduce new async_slow queue for image deletion#2199
blued-gear wants to merge 7 commits into
mainfrom
new/async_slow

Conversation

@blued-gear

Copy link
Copy Markdown
Collaborator

On some platforms the deletion of many images when a user gets deleted takes quite a while. To decouple the deletion from the transaction which removes the user, this PR adds a new queue and new message for this job.

@blued-gear
blued-gear marked this pull request as ready for review August 5, 2026 20:54
@blued-gear

Copy link
Copy Markdown
Collaborator Author

I reworked the new code to lower the chance of race conditions between entity and file deletions.

Also, the tests now cover the MessageHandler.

@melroy89 melroy89 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found three correctness issues in the image-deletion flow.

return $filesToDelete;
});

foreach ($filesToDelete as $path) {

@melroy89 melroy89 Sep 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The transaction commits before the loop below removes the files. In that gap, another request can upload the same content and ImageRepository::create() can create or reuse an image pointing to one of these content-addressed paths. This worker will then delete a file that is referenced again. Locking the existing Image rows does not cover the "row does not exist" case either, because there is no row to lock.

The reference check and storage removal need coordination based on the image hash/path that is also used by image creation. For example, the overall shape could be:

$this->imageLock->withHash($hash, function () use ($hash, $path): void {
    $image = $this->imageRepository->findOneBySha256($hash);
    if ($image && $this->imageRepository->isReferenced($image)) {
        return;
    }

    $this->imageManager->remove($path);

    if ($image) {
        $this->entityManager->remove($image);
        $this->entityManager->flush();
    }
});

The important part is that both upload/create and deletion acquire the same per-hash lock and that the lock remains held through the file removal. The exact lock implementation can differ, but ending the database transaction before storage deletion leaves the race open.


foreach ($filesToDelete as $path) {
try {
$this->imageManager->remove($path);

@melroy89 melroy89 Sep 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image::filePath is nullable, and both message factories can therefore put a null value in this batch. When that value reaches ImageManagerInterface::remove(string), PHP throws a TypeError. The current catch (\Exception) does not catch it because TypeError implements Throwable, not Exception. The handler then fails and retries instead of continuing with the remaining images.

A simple guard keeps null paths away from the string-only API:

foreach ($filesToDelete as $path) {
    if (null === $path) {
        continue;
    }

    try {
        $this->imageManager->remove($path);
    } catch (\Exception $e) {
        // existing logging
    }
}

It would also help to stop adding null paths to $filesToDelete in the first place, but the removal boundary should still be safe because the incoming message may already contain null.


// 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));

@melroy89 melroy89 Sep 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user deletion is committed before this message is dispatched. If the transport throws here, Messenger retries DeleteUserMessage, but the retry reaches the $user->isDeleted && null === $user->markedForDeletionAt branch and returns immediately. At that point the original user's image list is no longer available, so no later attempt schedules the cleanup and those files can remain orphaned permanently.

The cleanup intent needs to be saved atomically with the user deletion. One possible shape is to write it to a transactional outbox inside the same database transaction:

$this->entityManager->persist(
    OutboxMessage::from(new DeleteImageV2Message($deleteImagesPayload))
);
$this->entityManager->flush();
$this->entityManager->commit();

A separate worker can then publish that outbox record with retries. If this project already uses a transaction-aware Messenger transport, dispatching through that mechanism inside the transaction would provide the same guarantee. Another valid fix is to make the already-deleted retry path retain enough cleanup data to dispatch the message, but the current replacement user does not contain that data.

@melroy89
melroy89 self-requested a review September 9, 2026 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants