fix(xmtp_mls): propagate retryable errors from own self-publish reaction handlers#3758
Open
Jr-kenny wants to merge 2 commits into
Open
fix(xmtp_mls): propagate retryable errors from own self-publish reaction handlers#3758Jr-kenny wants to merge 2 commits into
Jr-kenny wants to merge 2 commits into
Conversation
…ion handlers
On the self-publish path, process_own_leave_request_message and
process_own_delete_message discarded their errors: the leave handler matched the
result and dropped Err to a debug! line, and both used let Ok(..) else { return }
reads that swallowed DB failures. Both are called from process_own_message, which
returns a Result, so a retryable storage failure on a client's own leave/delete
reaction was silently dropped instead of rolling the sync back and retrying like
the rest of process_message_inner.
Both handlers now return Result<(), GroupMessageProcessingError> and propagate
read failures with ?. The new resolve_own_reaction helper in process_own_message
branches on is_retryable(): retryable errors propagate so the sync rolls back and
retries, non-retryable errors are logged at warn! and swallowed because the
message is already published and the local reaction is best-effort. The
content_type and missing-row guards stay as benign early returns.
Adds a test that injects a retryable storage error into both handlers and asserts
they surface it rather than silently succeeding.
Closes xmtp#3747
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3758 +/- ##
========================================
Coverage 84.45% 84.46%
========================================
Files 408 408
Lines 59772 59876 +104
========================================
+ Hits 50483 50574 +91
- Misses 9289 9302 +13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The fault injection toggles an env-var flag and wasm targets cannot set environment variables, so the test panics before it can run. Gate it with cfg(not(target_arch = "wasm32")), same as the other test_mocks_helpers based tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3747
Problem
When you do something in a group like leaving it, or deleting one of your own messages, your app first publishes that action to the network, then gets it back and runs a little followup step locally to update your status. Something like "I want to leave the group" (
process_own_leave_request_message) and "I deleted a message" (process_own_delete_message).Those two followups had a bad habit: if something went wrong while they ran, they just quietly threw the error away. One will catch the failure and only write a simple debug line, the other will use a coding shortcut that treats "the database read failed" exactly like "nothing found" and just returns as if all was well.
The problem is that some of these failures are actually temporary, like the database was briefly busy or a little connection shake. Everywhere else in the sync code, a temporary failure causes the system to roll back and try again. However, on these two specific paths, a temporary failure during a leave or delete silently drops. The operation incorrectly appears successful when in fact it failed.
Fix
Both handlers now return
Resultand pass database read errors up with?instead of hiding them. A missing row or a different content type is still a normal early return, since those cases are genuinely fine.A new
resolve_own_reactionhelper inprocess_own_messagedecides what to do with a failure:warn!with the group and message ids and move on, because the message is already published and retrying won't help.The reasoning is documented in comments at the call site so the intent is clear to the next reader.
Testing
Added
test_own_reaction_handlers_surface_retryable_errors. It injects a retryable storage error into both handlers and checks two things: the handlers themselves return the error, and the error survives the full sync path. The end-to-end part publishes a real message, makes the reaction fail, and asserts that the sync reports the failure in its summary without counting the message as processed. It then clears the fault and confirms the same message is processed cleanly on the next sync, proving the rollback left the cursor in the right place.Existing
test_self_removal*and delete-message tests still pass, and clippy and rustfmt are clean.Note
Propagate retryable errors from own leave/delete reaction handlers in
MlsGroupprocess_own_leave_request_messageandprocess_own_delete_messagenow returnResult<(), GroupMessageProcessingError>instead of swallowing errors, so database read failures can trigger sync transaction rollback and retry.resolve_own_reactionhelper in mls_sync.rs maps handler results: retryable errors becomeIntentResolutionError(triggering rollback), non-retryable errors are logged atwarnand skipped, and missing rows are treated as benign no-ops.TEST_MODE_OWN_REACTION_RETRYABLE_ERRORis added in test_mocks_helpers.rs with a regression test that verifies retryable errors surface correctly and that clearing the fault allows a subsequent sync to succeed.Macroscope summarized 66fdd7f.