Skip to content

feat: add clear_pending_commit for publish failure recovery#192

Merged
erskingardner merged 1 commit into
marmot-protocol:masterfrom
VectorPrivacy:fix/clear-pending-commit-upstream
Feb 23, 2026
Merged

feat: add clear_pending_commit for publish failure recovery#192
erskingardner merged 1 commit into
marmot-protocol:masterfrom
VectorPrivacy:fix/clear-pending-commit-upstream

Conversation

@JSKitty

@JSKitty JSKitty commented Feb 18, 2026

Copy link
Copy Markdown
Contributor

Problem

MDK documents that merge_pending_commit should be called after publishing the Kind:445 message. But if the publish fails (relay down, network error, auth rejection after retries), the pending commit remains in storage permanently. Every subsequent add_members, remove_members, or self_update call fails with:

Can't execute operation because a pending commit exists.

The group is stuck with no recovery path. The only option is to create a new group.

This was discovered while implementing MIP-02/MIP-03 relay-confirmation-before-merge ordering in Vector: VectorPrivacy/Vector#46

Fix

Expose OpenMLS's existing MlsGroup::clear_pending_commit (which already persists the state change via StorageProvider::write_group_state) as a one-line wrapper:

pub fn clear_pending_commit(&self, group_id: &GroupId) -> Result<(), Error>

This returns the group to its pre-commit state — no epoch advance, no member changes. Clients call it when publish exhausts retries, then surface the error to the user who can retry the operation.

Usage

let result = engine.add_members(&group_id, &key_packages)?;

match publish_to_relays(&result.evolution_event).await {
    Ok(_) => engine.merge_pending_commit(&group_id)?,
    Err(_) => engine.clear_pending_commit(&group_id)?,  // rollback
}

Test plan

  • cargo check -p mdk-core
  • Tested in Vector: disable relays → kick member → publish fails → clear_pending_commit called → group remains functional → re-enable relays → retry kick → succeeds

🤖 Generated with Claude Code

This PR exposes OpenMLS's existing MlsGroup::clear_pending_commit via a new MDK wrapper method to enable recovery from failed relay publishes. When a publish fails after creating a pending commit, the group becomes stuck and unable to perform any operations; this method allows clients to roll back the pending commit and retry the failed operation.

What changed:

  • Added MDK::clear_pending_commit(&self, group_id: &GroupId) -> Result<(), Error> public method to mdk-core that loads the MLS group and delegates to OpenMLS's clear_pending_commit, returning the group to its pre-commit state without advancing epoch or applying member changes.
  • Updated CHANGELOG.md to document the new method.

API surface:

  • New public method added to MDK: clear_pending_commit(group_id) allows explicit rollback of uncommitted pending MLS commits.

Testing:

  • Four unit tests added to verify behavior: rollback of failed add-member commit, rollback of failed remove-member commit, no-op behavior when no pending commit exists, and error handling for non-existent groups.

@coderabbitai

coderabbitai Bot commented Feb 18, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉


📝 Walkthrough

Walkthrough

A new public method clear_pending_commit was added to the MDK struct, enabling callers to roll back uncommitted pending MLS commits. The implementation loads an MLS group by ID, invokes OpenMLS's clear_pending_commit method, and handles errors. Comprehensive unit tests verify behavior across success scenarios and error cases.

Changes

Cohort / File(s) Summary
CHANGELOG Documentation
crates/mdk-core/CHANGELOG.md
Added release notes documenting the new clear_pending_commit(group_id) public method.
Core API and Tests
crates/mdk-core/src/groups.rs
Implemented clear_pending_commit(&self, group_id: &GroupId) -> Result<(), Error> method for MDK struct with error mapping. Added four unit tests covering pending commit rollback after failed add/remove operations, no-pending scenarios, and non-existent group error handling.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

mls-protocol

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and accurately summarizes the main feature: adding a clear_pending_commit method to enable recovery from publish failures.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
No Sensitive Identifier Leakage ✅ Passed The pull request adds a new clear_pending_commit method that does not introduce any sensitive identifier leakage. The implementation does not use tracing macros, format!() strings, panic!(), or Debug implementations to expose GroupId, mls_group_id, nostr_group_id, or encryption keys. All test assertions and error messages use generic, non-sensitive language. The error handling uses Error::Provider(e.to_string()) which converts OpenMLS errors without adding group identifiers from the method's parameters.

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

✨ Finishing Touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Add `MDK::clear_pending_commit(group_id)` to allow callers to roll back
an uncommitted pending MLS commit. Without this, a single failed relay
publish permanently blocks all future group operations with "Can't
execute operation because a pending commit exists".

Wraps OpenMLS's `MlsGroup::clear_pending_commit` with MDK's group-
loading and error handling.

Discovered from: VectorPrivacy/Vector#46

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@JSKitty
JSKitty force-pushed the fix/clear-pending-commit-upstream branch from f321a69 to 92ec6a4 Compare February 18, 2026 22:36

@dannym-arx dannym-arx left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, I wonder tho if we should expose clear_pending_commit in the uniffi bindings too?

Don't let that stop the merge tho, can be another PR

@erskingardner
erskingardner merged commit 06a0636 into marmot-protocol:master Feb 23, 2026
15 checks passed
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.

3 participants