intoduce new async_slow queue for image deletion - #2199
Conversation
|
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
left a comment
There was a problem hiding this comment.
Found three correctness issues in the image-deletion flow.
| return $filesToDelete; | ||
| }); | ||
|
|
||
| foreach ($filesToDelete as $path) { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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.
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.