add message deletion

[mchenani]

Add message deletion across core, DB, and bindings to represent, validate, persist, and surface DeletedMessage content

Introduce ContentType::DeleteMessage, a DeletedBy model, and deletion records; validate deletions in enrichment, persist via message_deletions, and expose deleted state in Node, WASM, and FFI bindings. Add group APIs to send and process delete messages and filter deletion entries from lists.

📍Where to Start

Start with deletion handling in MlsGroup: review process_delete_message in xmtp_mls/src/groups/mls_sync.rs, then delete_message in xmtp_mls/src/groups/mod.rs, followed by enrichment logic in xmtp_mls/src/messages/enrichment.rs.

---

📊 Macroscope summarized 5d3df84. 16 files reviewed, 12 issues evaluated, 10 issues filtered, 2 comments posted

🗂️ Filtered Issues

db_tools/src/tasks/migrations.rs — 0 comments posted, 2 evaluated, 2 filtered

• line 73: The migration_status function assumes that values in the applied slice are already purely numeric versions, but this assumption may be incorrect. The old code filtered both name and each applied version a to numeric characters before comparing. The new code only filters name via extract_version() and compares directly with a == &name_version. If db.applied_migrations() returns versions containing non-numeric characters (e.g., dashes like 2025-11-15-232503), the comparison will always fail and migrations will incorrectly be reported as [pending] when they are actually applied. [ Low confidence ]
• line 184: The migration_numeric_version function silently returns 0 via unwrap_or(0) when parsing fails. If a migration name doesn't parse to a valid u64 (e.g., version string is too long or malformed), multiple migrations could get the same sort key of 0, resulting in incorrect sorting order and potentially causing test failures or unexpected rollback behavior in test_migration_status_applied_and_pending. [ Low confidence ]

xmtp_db/src/encrypted_store/group_message.rs — 1 comment posted, 2 evaluated, 1 filtered

• line 270: ContentType::Unknown is marked as deletable (true on line 270), but unknown content types represent messages whose type is not recognized by the current client. This could be a newer content type from updated clients. Allowing deletion of unknown content types risks unintentional data loss when a user attempts to delete what appears to be an unrecognized message but is actually meaningful content that should be preserved. [ Low confidence ]

xmtp_mls/src/groups/mls_sync.rs — 1 comment posted, 5 evaluated, 4 filtered

• line 1364: When the original message doesn't exist yet (out-of-order delivery), is_sender is set to false via unwrap_or(false) at line 1367. This means non-super-admin users cannot delete their own messages if the deletion message arrives before the original message. The code at lines 1409-1415 suggests out-of-order handling is expected to work, but the authorization logic at line 1386 only allows super admins to create deletion records when the original message is missing. Regular users' valid deletions will be silently rejected as unauthorized. [ Already posted ]
• line 1381: The is_super_admin_without_lock call at line 1381-1382 uses unwrap_or(false) to handle errors, which silently treats any error (e.g., storage/database issues) as "not a super admin". This could cause legitimate super admin deletion attempts to be rejected as unauthorized if GroupMutableMetadataError occurs, with only a generic "Unauthorized deletion attempt" warning logged that doesn't indicate the actual cause was an error reading metadata. [ Low confidence ]
• line 3351: The error conversion in the match statement discards the actual error from process_delete_message and replaces it with GroupError::InvalidGroupMembership. If process_delete_message returns an error for a legitimate reason other than malformed content (e.g., storage error), the test would fail with a misleading error type, making debugging difficult. The original error should be propagated or logged. [ Low confidence ]
• line 3416: Same issue as in the previous test - the error conversion replaces any error from process_delete_message with GroupError::InvalidGroupMembership, losing the actual error information and potentially masking different failure modes. [ Low confidence ]

xmtp_mls/src/groups/mod.rs — 0 comments posted, 3 evaluated, 3 filtered

• line 704: TOCTOU race condition: The check at line 704 (is_message_deleted) and the store at line 764 are not atomic. Two concurrent calls to delete_message for the same message_id could both pass the is_message_deleted check, resulting in duplicate deletion messages being sent to the network. While the comment mentions store_or_ignore handles idempotent insertion during sync, the duplicate network messages are still sent. [ Previously rejected ]
• line 764: If send_message_optimistic at line 746 succeeds but deletion.store(&conn) at line 764 fails, the function returns an error to the caller even though the deletion message was already sent to the network. The caller may interpret this as a complete failure and retry, potentially causing confusion or duplicate attempts. The comment acknowledges crash recovery but doesn't address the case where store throws an error (e.g., database constraint violation). [ Previously rejected ]
• line 887: The new find_enriched_messages method does not call filter_out_hidden_message_types_from_query before querying messages, unlike the existing find_messages_v2 method which filters hidden message types. This inconsistency could expose internal/hidden message types to callers that should not be visible. [ Already posted ]

[graphite-app]

How to use the Graphite Merge Queue

Add the label mergequeue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

[claude]

Claude finished @mchenani's task —— View job

---

Code Review Complete ✓

Review Checklist

• Read core implementation files (mls_sync.rs, mod.rs, enrichment.rs, message_deletion.rs)
• Review test coverage
• Analyze code quality and identify issues not already mentioned
• Post review feedback

---

Review Date: 2026-01-05

Summary

This is a well-implemented feature with comprehensive test coverage and thoughtful error handling. The code quality is generally high, with good documentation and clear separation of concerns. I've identified a few issues that haven't been mentioned in previous reviews:

Issues Found

1. Missing Hidden Message Type Filtering in find_enriched_messages (xmtp_mls/src/groups/mod.rs:887)

The find_enriched_messages method doesn't apply the same hidden message type filtering that find_messages_v2 does. This could expose DeleteMessage, Reaction, and ReadReceipt messages to callers.

Impact: Medium - Could expose internal message types to UI layers
Already mentioned: Yes, by macroscopeapp

// Current code (line 887):
let messages = conn.get_group_messages(&self.group_id, args)?;

// Should be:
let filtered_args = filter_out_hidden_message_types_from_query(args);
let messages = conn.get_group_messages(&self.group_id, &filtered_args)?;

2. Potential HashMap Collision in get_deletions (xmtp_mls/src/messages/enrichment.rs:293-298)

If multiple deletion records exist for the same message (e.g., concurrent deletions by sender and admin), only the last one is retained in the HashMap, silently discarding earlier records.

Impact: Low - Edge case, but could affect deletion attribution
Already mentioned: Yes, by macroscopeapp

3. Out-of-Order Deletion Authorization Gap (xmtp_mls/src/groups/mls_sync.rs:1364-1367)

When a deletion arrives before the target message, is_sender defaults to false. This means a legitimate self-deletion could be rejected if it arrives out of order. While the deletion record is still stored for later validation, the MessageDeleted event won't fire.

Impact: Low - Self-corrects when message arrives, but event timing is off
Already mentioned: Yes, by macroscopeapp

4. Test Coverage Observation

The test coverage is excellent (737 lines of tests in test_delete_message.rs), covering:

• Basic sender deletion ✓
• Admin deletion ✓
• Authorization failures ✓
• Non-deletable messages ✓
• Out-of-order scenarios ✓
• Enrichment with deletions ✓
• Reply chains with deleted messages ✓
• Concurrent deletions ✓
• Cross-group prevention ✓

Missing coverage areas:

• DeleteMessage codec error paths (though covered by unit tests in delete_message.rs)
• Binding-level deletion payload exposure (mentioned by macroscopeapp)

Positive Observations

1. Excellent documentation - The error handling strategy in process_delete_message (lines 1286-1298 in mls_sync.rs) is well-documented, explaining why invalid deletions return Ok(()) instead of propagating errors.

2. Thoughtful design choices:

   • Hex-encoded message_id in wire format for readability
   • store_or_ignore for idempotent insertion during sync
   • Deletion validation centralized in is_deletion_valid function
   • Proper handling of cross-group deletion attempts

3. Good test quality - Tests use descriptive names, have clear setup/teardown, and test edge cases thoroughly.

4. Security considerations - Authorization checks at both send-time and process-time, with proper admin status validation.

Recommendations

Since the main issues have already been identified by other reviewers, I recommend:

1. Address the find_enriched_messages filtering issue as it's user-facing
2. Consider documenting the HashMap collision behavior or using a different data structure if multiple deletions per message should be tracked
3. The out-of-order scenario is acceptable given the current design, but consider documenting this behavior

Performance Considerations

The implementation looks performant:

• Batch queries for deletions (line 235 in enrichment.rs)
• Proper indexing on deleted_message_id and group_id (migration file)
• Efficient HashMap lookups during enrichment

Security Assessment

No security concerns beyond what's already been identified. The implementation properly:

• Validates authorization at multiple layers
• Prevents cross-group deletions
• Checks admin status at processing time
• Prevents deletion of system messages

---

[codecov]

Codecov Report

❌ Patch coverage is 86.52118% with 105 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.41%. Comparing base (eb31b6f) to head (5d3df84).

Files with missing lines	Patch %	Lines
xmtp_db/src/encrypted_store/message_deletion.rs	91.05%	23 Missing ⚠️
bindings_node/src/content_types/deleted_message.rs	0.00%	17 Missing ⚠️
xmtp_mls/src/groups/mls_sync.rs	92.05%	17 Missing ⚠️
bindings_ffi/src/message.rs	0.00%	13 Missing ⚠️
xmtp_mls/src/messages/enrichment.rs	86.86%	13 Missing ⚠️
..._node/src/content_types/decoded_message_content.rs	0.00%	8 Missing ⚠️
bindings_ffi/src/mls.rs	0.00%	4 Missing ⚠️
xmtp_mls/src/groups/error.rs	0.00%	4 Missing ⚠️
xmtp_content_types/src/delete_message.rs	91.89%	3 Missing ⚠️
xmtp_db/src/encrypted_store/group_message.rs	85.71%	2 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2772      +/-   ##
==========================================
+ Coverage   74.24%   74.41%   +0.16%     
==========================================
  Files         413      416       +3     
  Lines       53201    53953     +752     
==========================================
+ Hits        39501    40147     +646     
- Misses      13700    13806     +106     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cameronvoell]

This PR addresses #2912. See XIP 76 for more detail: https://github.com/xmtp/XIPs/blob/main/XIPs/xip-76-delete-messages.md

[rygine]

this would appear as just Deleted a message in the message feed, which i don't think is very useful. i actually don't think we need to have a fallback for this message type as i'm not sure it would provide any value. WDYT?

[mchenani]

I was thinking that would be a good solution for backward compatibility. Wdyt?

[rygine]

i'm just looking at it from a user perspective. if i saw "Deleted a message" in the message feed, i would be confused.

[rygine]

it's OK to have no fallback. the default behavior in that case is to display nothing.

[mchenani]

Fair enough. Will remove it.

[rygine]

please rename this file to deleted_message.rs

[rygine]

this is not necessary, right?

[macroscopeapp]

If original_msg_opt is None, cross‑group and deletability checks are skipped, so a super admin can still record a deletion for any ID. Consider returning early when the target message is missing (and only proceeding when it exists and belongs to this group) before authorization and storage.

Suggested change

	// Validate message belongs to this group (prevent cross-group deletion)
	if original_msg_opt.is_none() {
	tracing::warn!("Deletion for unknown message {}, skipping", delete_msg.message_id);
	return Ok(());
	}

🚀 Want me to fix this? Reply ex: "fix it for me".

[macroscopeapp]

ContentType::DeleteMessage exists, but TryFrom<ContentTypeSave> for ContentType doesn’t handle a ContentTypeSave::DeleteMessage. Consider adding the variant to ContentTypeSave and mapping it to ContentType::DeleteMessage, or document why DeleteMessage is intentionally not persisted.

🚀 Want me to fix this? Reply ex: "fix it for me".