Skip to content

Conversation

@dannym-arx
Copy link
Contributor

@dannym-arx dannym-arx commented Feb 2, 2026

This PR fixes issues #154 and #159 by making re-arrived previously failed MLS messages non-crashing: when a previously failed or epoch-invalidated message is seen again and its MLS group ID cannot be extracted, processing now returns a graceful PreviouslyFailed result instead of producing an error/panic. It also preserves the prior behavior of returning Unprocessable when the group ID can be extracted, and updates tests and UniFFI bindings to expose the new result variant.

What changed:

  • Added MessageProcessingResult::PreviouslyFailed to mdk-core to represent a previously failed message that cannot be reprocessed and may lack an extractable MLS group ID.
  • Updated dispatch/deduplication logic in crates/mdk-core/src/messages/process.rs to return Unprocessable { mls_group_id } when a group ID can be extracted from a previously failed/epoch-invalidated event, or PreviouslyFailed when it cannot.
  • Added mapping of the new variant to the UniFFI FFI as ProcessMessageResult::PreviouslyFailed in crates/mdk-uniffi so foreign-language clients receive the new outcome.
  • Updated tests in crates/mdk-core/src/messages/error_handling.rs to expect Ok(PreviouslyFailed) in relevant deduplication/failure scenarios and added coverage for sanitize_error_reason mappings and malformed message cases.
  • Changelog entries updated in crates/mdk-core and crates/mdk-uniffi describing the new PreviouslyFailed outcome.

Security impact:

  • None: no cryptographic algorithm, key handling, SQLCipher, or file-permission changes; changes are control-flow/error-handling only and do not expose additional sensitive identifiers.

Protocol changes:

  • None to MLS protocol semantics; this is an implementation-level change to how previously failed messages are reported to callers (no changes to MLS wire formats or RFC 9420 behaviors).

API surface:

  • Breaking/visible API change: public enum variant added: MessageProcessingResult::PreviouslyFailed in mdk-core.
  • FFI/API change: public UniFFI enum variant added: ProcessMessageResult::PreviouslyFailed in mdk-uniffi to expose the new outcome to bindings.
  • Debug impl for MessageProcessingResult updated to handle the new variant.
  • No storage schema changes.

Testing:

  • Multiple unit tests were updated to expect the PreviouslyFailed outcome where appropriate and new tests were added to exercise sanitize_error_reason and malformed/corrupted message deduplication behavior.

@coderabbitai
Copy link

coderabbitai bot commented Feb 2, 2026

📝 Walkthrough

Walkthrough

Adds a new PreviouslyFailed variant to message processing results (core and UniFFI) and updates processing paths/tests to return this variant when a previously-failed message is retried but no MLS group ID can be extracted.

Changes

Cohort / File(s) Summary
Core Message Processing Enum
crates/mdk-core/src/messages/mod.rs
Added public PreviouslyFailed variant to MessageProcessingResult and updated Debug impl to handle it.
Process Logic Updates
crates/mdk-core/src/messages/process.rs
Changed control flow to return PreviouslyFailed when a previously-failed or epoch-invalidated message cannot yield an MLS group ID; preserved returning Unprocessable when group ID is present.
Test & Error Handling
crates/mdk-core/src/messages/error_handling.rs
Updated many tests to expect PreviouslyFailed instead of Err for repeated failures; added tests for sanitize_error_reason mapping across variants and retry/propagation behaviors.
UniFFI Wrapper
crates/mdk-uniffi/src/lib.rs
Mapped core MessageProcessingResult::PreviouslyFailed to new public ProcessMessageResult::PreviouslyFailed variant in the UniFFI API.
Changelogs
crates/mdk-core/CHANGELOG.md, crates/mdk-uniffi/CHANGELOG.md
Documented the addition of the PreviouslyFailed/ProcessMessageResult::PreviouslyFailed variant and related behavior changes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

breaking-change

Suggested reviewers

  • mubarakcoded
  • erskingardner
🚥 Pre-merge checks | ✅ 3 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title references issue numbers (#154, #159) but provides no meaningful description of what the pull request actually changes or fixes. Consider revising the title to describe the actual change, such as 'Add PreviouslyFailed variant to handle message processing crashes' or similar, to make the changeset clear to reviewers scanning history.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
No Sensitive Identifier Leakage ✅ Passed Pull request demonstrates excellent security practices with no sensitive identifier leakage patterns detected in tracing macros, Debug trait implementations, format strings, or error handling.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-previously-failed-message-crash

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link

github-actions bot commented Feb 2, 2026

❌ Coverage: 90.77% → 90.76% (-0.01%)

@github-actions
Copy link

github-actions bot commented Feb 2, 2026

✅ Coverage: 90.77% → 90.78% (+0.01%)

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@crates/mdk-core/CHANGELOG.md`:
- Line 28: Update the CHANGELOG entry for the "**`PreviouslyFailed` Result
Variant**" so the PR link at the end references `#165` instead of `#160`; locate the
line containing "MessageProcessingResult::PreviouslyFailed" and replace the PR
number in the parenthetical link from "(`#160`...)" to "(`#165`...)" so the entry
ends with the correct PR reference.

In `@crates/mdk-uniffi/CHANGELOG.md`:
- Line 44: Update the CHANGELOG entry for the "PreviouslyFailed Result Variant"
(the line mentioning ProcessMessageResult.PreviouslyFailed / "PreviouslyFailed
Result Variant") to reference PR `#165` instead of `#160`, and verify whether the
"fixes [`#153`]" issue reference is correct for this crate—if not, replace it with
the correct issue number; ensure the PR link and issue references in that single
entry reflect the PR that actually modified this crate.

@github-actions
Copy link

github-actions bot commented Feb 2, 2026

✅ Coverage: 90.77% → 90.78% (+0.01%)

@jgmontoya jgmontoya self-requested a review February 2, 2026 16:29
Copy link
Contributor

@jgmontoya jgmontoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@mubarakcoded mubarakcoded left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@dannym-arx dannym-arx merged commit 1ad7322 into master Feb 2, 2026
12 checks passed
@dannym-arx dannym-arx deleted the fix-previously-failed-message-crash branch February 2, 2026 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants