You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by "prosgarz35 (via GitHub)" <gi...@apache.org> on 2026/09/24 05:09:01 UTC
[PR] Postgres: Prevent data loss race condition in DeleteMessageListener [james-project]
prosgarz35 opened a new pull request, #3200:
URL: https://github.com/apache/james-project/pull/3200
## Motivation & Problem Statement
In the PostgreSQL storage implementation (`postgres-app` / `mailbox-postgres`), message bodies and attachments are deduplicated and stored in a shared `BlobStore` (e.g., S3, FileBlobStore), while message records are maintained
across two relational tables:
1. `message_mailbox` (holds mailbox-specific entries, UIDs, flags, and references to `message_id`)
2. `message` (holds deduplicated metadata and the pointer `body_blob_id` pointing to the physical payload in `BlobStore`).
When messages are expunged or a mailbox is deleted, the asynchronous `DeleteMessageListener` cleans up unreferenced messages. Previously, the deletion lifecycle followed this sequence:
1. SELECT 1 FROM message_mailbox WHERE message_id = ? (via isUnreferenced)
2. If unreferenced:
a. blobStore.delete(bodyBlobId) <-- Physical S3/BlobStore deletion FIRST!
b. attachmentDAO.deleteBlobs(...)
c. threadDAO.deleteSome(...)
d. messageDAO.deleteByMessageId(...) <-- Database deletion LAST
### The Data Loss Race Condition
Because the initial check (`isUnreferenced`) and the final database deletion were non-atomic and separated by network I/O to the `BlobStore`:
1. **Concurrent Message Duplication / Copying**: A message in Folder A is expunged while concurrently being copied (`IMAP COPY`, `JMAP message move/import`, or concurrent multi-recipient delivery with deduplication) into Folder B.
2. `DeleteMessageListener` checks `isUnreferenced()`: if executed before Folder B's record is committed, it returns `true`.
3. The listener immediately proceeds to **permanently delete the body and attachment blobs in `BlobStore`**.
4. Once the new `message_mailbox` record for Folder B commits, the foreign key constraint on `message` prevents `deleteByMessageId` from completing (or it fails with a foreign key violation), leaving an active record in Folder B.
5. **Impact**: The user sees the email listed in Folder B, but opening or fetching the email body fails with `BlobNotFoundException` / `IOException`. **The email content is permanently lost.**
---
## Solution: Atomic Database-First Conditional Delete (CAS)
This PR addresses the race condition by enforcing a strict **Database-First Atomic Conditional Deletion** policy:
1. **Atomic CAS Query in `PostgresMessageDAO`**:
Introduced `deleteIfUnreferenced(PostgresMessageId messageId)`:
```sql
DELETE FROM message
WHERE message_id = ?
AND NOT EXISTS (
SELECT 1
FROM message_mailbox
WHERE message_mailbox.message_id = message.message_id
)
RETURNING body_blob_id;
• This conditional deletion is atomic within PostgreSQL.
• If concurrent references exist or are added in message_mailbox, the DELETE query returns 0 rows and leaves the row untouched.
• If and only if no references exist, the row is safely removed and returns the body_blob_id.
2. Safe BlobStore Teardown in DeleteMessageListener:
• DeleteMessageListener now relies solely on deleteIfUnreferenced.
• If PostgreSQL confirms the row was deleted (returning Optional<BlobId>), the listener safely proceeds to delete the body and attachment blobs from BlobStore.
• If the database indicates the row is still referenced, the listener short-circuits immediately. The physical payload in BlobStore is never touched.
──────
## Changes
• PostgresMessageDAO: Added deleteIfUnreferenced(PostgresMessageId) utilizing atomic NOT EXISTS subquery with RETURNING body_blob_id.
• DeleteMessageListener:
• Refactored handleMessageDeletion to execute deleteIfUnreferenced before touching external storage.
• Removed separate, non-atomic isUnreferenced query and deprecated deleteBodyBlob.
• DeleteMessageListenerContract: Added test deleteIfUnreferencedShouldNotDeleteBlobWhenConcurrentReferenceExists verifying that concurrent references prevent both database row deletion and blob destruction.
──────
## Verification & Compatibility
• Verified backward-compatibility and zero schema migration needed (uses existing indices on message_mailbox(message_id)).
• Full Checkstyle compliance (0 violations).
• Unit & contract test compilation passing with Maven (mvn test-compile -pl :apache-james-mailbox-postgres).
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] Postgres: Prevent data loss race condition in DeleteMessageListener [james-project]
Posted by "quantranhong1999 (via GitHub)" <gi...@apache.org>.
quantranhong1999 commented on code in PR #3200:
URL: https://github.com/apache/james-project/pull/3200#discussion_r4090506861
##########
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:
Hmm, I am not sure if this actually reproduces your reported race condition issue.
##########
mailbox/postgres/src/main/java/org/apache/james/mailbox/postgres/DeleteMessageListener.java:
##########
@@ -173,11 +168,19 @@ private Mono<Void> handleMessageDeletion(PostgresMessageDAO postgresMessageDAO,
return postgresMessageDAO.retrieveMessage(messageId)
.flatMap(messageRepresentation -> dispatchMessageContentDeletionEvent(mailboxId, owner, mailboxACL, flags, messageRepresentation, mailboxPath)
.thenReturn(messageId))
- .filterWhen(msgId -> isUnreferenced(msgId, postgresMailboxMessageDAO))
- .flatMap(msgId -> deleteBodyBlob(msgId, postgresMessageDAO)
- .then(deleteAttachmentIfEnabled(messageId, attachmentDAO))
- .then(threadDAO.deleteSome(owner, messageId))
- .then(postgresMessageDAO.deleteByMessageId(messageId)));
+ .flatMap(msgId -> deleteMessageDataIfUnreferenced(msgId, postgresMessageDAO, attachmentDAO, threadDAO, owner));
+ }
+
+ private Mono<Void> deleteMessageDataIfUnreferenced(PostgresMessageId messageId,
+ PostgresMessageDAO postgresMessageDAO,
+ PostgresAttachmentDAO attachmentDAO,
+ PostgresThreadDAO threadDAO,
+ Username owner) {
+ return postgresMessageDAO.deleteIfUnreferenced(messageId)
+ .flatMap(optionalBlobId -> optionalBlobId.map(blobId -> Mono.from(blobStore.delete(blobStore.getDefaultBucketName(), blobId))
+ .then(deleteAttachmentIfEnabled(messageId, attachmentDAO))
+ .then(threadDAO.deleteSome(owner, messageId)))
+ .orElse(Mono.empty()));
Review Comment:
Concern here: we delete the messageId first; therefore, if blob cleanup fails later, for example. We can not retry and clean up the remaining data easily, as the referenced messageId is gone.
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] Postgres: Prevent data loss race condition in DeleteMessageListener [james-project]
Posted by "prosgarz35 (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] Postgres: Prevent data loss race condition in DeleteMessageListener [james-project]
Posted by "prosgarz35 (via GitHub)" <gi...@apache.org>.
prosgarz35 closed pull request #3200: Postgres: Prevent data loss race condition in DeleteMessageListener
URL: https://github.com/apache/james-project/pull/3200
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org