prosgarz35 commented on code in PR #3200:
URL: https://github.com/apache/james-project/pull/3200#discussion_r4090897559
##########
mailbox/postgres/src/test/java/org/apache/james/mailbox/postgres/DeleteMessageListenerContract.java:
##########
@@ -311,6 +311,27 @@ void deleteMessageShouldCleanUpThreadData() throws
Exception {
});
}
+ @Test
+ void
deleteIfUnreferencedShouldNotDeleteBlobWhenConcurrentReferenceExists() throws
Exception {
+ MessageManager.AppendResult appendResult =
inboxManager.appendMessage(MessageManager.AppendCommand.builder()
+
.build(ClassLoaderUtils.getSystemResourceAsByteArray("eml/emailWithOnlyAttachment.eml")),
session);
+ PostgresMessageId messageId = (PostgresMessageId)
appendResult.getId().getMessageId();
+ BlobId messageBodyBlobId =
postgresMessageDAO.getBodyBlobId(messageId).block();
+
+ // Simulate concurrent reference by copying message to other mailbox
+ mailboxManager.copyMessages(MessageRange.all(), inboxManager.getId(),
otherBoxManager.getId(), session);
+
+ // Attempt conditional delete - should not delete row nor return
blobId because of concurrent reference
+ Optional<BlobId> deletedBlobId =
postgresMessageDAO.deleteIfUnreferenced(messageId).block();
Review Comment:
Thanks for the thorough review, @quantranhong1999! You're completely
right on both points:
1. **Deletion order & retry**: Deleting the database row first indeed
breaks event retryability in `DeleteMessageListener`—if `blobStore.delete()` or
attachment cleanup fails due to transient storage/network issues, subsequent
retries won't be able to retrieve the message representation and remaining
blobs/attachments will become unrecoverable orphans. Keeping the standard James
order (deleting blobs/attachments first and removing the message database row
last) preserves idempotency and allows retries to finish cleanup.
2. **Test scenario**: Agreed, the test simply called
`deleteIfUnreferenced` directly against a copied message without actually
running the listener or reproducing a true concurrent race. Moreover,
`deleteMessageListenerShouldNotDeleteReferencedBlob()` already verifies
that referenced messages/blobs are not deleted when other references exist.
I will revert the premature row deletion and clean this up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
