Skip to content

Commit 06a0636

Browse files
JSKittyclaude
andauthored
feat: add clear_pending_commit to rollback failed publish attempts (#192)
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>
1 parent d9f3727 commit 06a0636

2 files changed

Lines changed: 160 additions & 0 deletions

File tree

crates/mdk-core/CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@
2929

3030
### Added
3131

32+
- **`clear_pending_commit` method**: Added `MDK::clear_pending_commit(group_id)` to allow callers to roll back an uncommitted pending MLS commit. This is essential for recovering from failed relay publishes — without it, a single failed publish permanently blocks all future group operations with "pending commit exists" errors. Wraps OpenMLS's `MlsGroup::clear_pending_commit` with MDK's group-loading and error handling. ([#192](https://github.com/marmot-protocol/mdk/pull/192))
33+
3234
### Fixed
3335

3436
### Removed

crates/mdk-core/src/groups.rs

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1280,6 +1280,25 @@ where
12801280
})
12811281
}
12821282

1283+
/// Clear (rollback) a pending commit without merging it.
1284+
///
1285+
/// This should be called when the Kind:445 publish fails after creating a commit
1286+
/// via `add_members`, `remove_members`, or `self_update`. Without clearing, the
1287+
/// pending commit blocks all future group operations with "Can't execute operation
1288+
/// because a pending commit exists".
1289+
///
1290+
/// The group returns to its pre-commit state — no epoch advance, no member changes.
1291+
///
1292+
/// # Arguments
1293+
/// * `group_id` - the MlsGroup GroupId value
1294+
pub fn clear_pending_commit(&self, group_id: &GroupId) -> Result<(), Error> {
1295+
let mut mls_group = self.load_mls_group(group_id)?.ok_or(Error::GroupNotFound)?;
1296+
mls_group
1297+
.clear_pending_commit(self.provider.storage())
1298+
.map_err(|e| Error::Provider(e.to_string()))?;
1299+
Ok(())
1300+
}
1301+
12831302
/// Merge any pending commits.
12841303
/// This should be called AFTER publishing the Kind:445 message that contains a commit message to mitigate race conditions
12851304
///
@@ -4773,4 +4792,143 @@ mod tests {
47734792
let result = alice_mdk.pending_member_changes(&fake_group_id);
47744793
assert!(result.is_err(), "Should error for non-existent group");
47754794
}
4795+
4796+
/// Tests that `clear_pending_commit` rolls back a pending add-member commit,
4797+
/// allowing subsequent group operations to succeed.
4798+
#[test]
4799+
fn test_clear_pending_commit_after_failed_add() {
4800+
let mdk = create_test_mdk();
4801+
let (creator, members, admins) = create_test_group_members();
4802+
let group_id = create_test_group(&mdk, &creator, &members, &admins);
4803+
4804+
let initial_members = mdk.get_members(&group_id).expect("get members");
4805+
let initial_count = initial_members.len();
4806+
4807+
// Add a new member — creates a pending commit but do NOT merge
4808+
// (simulates a failed relay publish)
4809+
let new_member = Keys::generate();
4810+
let kp_event = create_key_package_event(&mdk, &new_member);
4811+
let _add_result = mdk
4812+
.add_members(&group_id, &[kp_event])
4813+
.expect("add_members should succeed");
4814+
4815+
// A second add_members should fail because there is already a pending commit
4816+
let another_member = Keys::generate();
4817+
let kp_event2 = create_key_package_event(&mdk, &another_member);
4818+
let err = mdk.add_members(&group_id, &[kp_event2]);
4819+
assert!(err.is_err(), "Should fail due to existing pending commit");
4820+
4821+
// Clear the pending commit (simulates cleanup after failed publish)
4822+
mdk.clear_pending_commit(&group_id)
4823+
.expect("clear_pending_commit should succeed");
4824+
4825+
// Verify the member was NOT added
4826+
let after_clear = mdk.get_members(&group_id).expect("get members");
4827+
assert_eq!(
4828+
after_clear.len(),
4829+
initial_count,
4830+
"Member count should be unchanged after clearing pending commit"
4831+
);
4832+
assert!(
4833+
!after_clear.contains(&new_member.public_key()),
4834+
"New member should not be in group after clearing pending commit"
4835+
);
4836+
4837+
// Verify the group is usable again — a new operation should succeed
4838+
let kp_event3 = create_key_package_event(&mdk, &another_member);
4839+
mdk.add_members(&group_id, &[kp_event3])
4840+
.expect("add_members should succeed after clearing pending commit");
4841+
mdk.merge_pending_commit(&group_id)
4842+
.expect("merge should succeed after clearing pending commit");
4843+
4844+
let final_members = mdk.get_members(&group_id).expect("get members");
4845+
assert_eq!(
4846+
final_members.len(),
4847+
initial_count + 1,
4848+
"Member should be added after clearing stale commit and retrying"
4849+
);
4850+
assert!(
4851+
final_members.contains(&another_member.public_key()),
4852+
"New member should be in group after successful retry"
4853+
);
4854+
}
4855+
4856+
/// Tests that `clear_pending_commit` rolls back a pending remove-member commit.
4857+
#[test]
4858+
fn test_clear_pending_commit_after_failed_remove() {
4859+
let mdk = create_test_mdk();
4860+
let (creator, members, admins) = create_test_group_members();
4861+
let group_id = create_test_group(&mdk, &creator, &members, &admins);
4862+
4863+
let initial_members = mdk.get_members(&group_id).expect("get members");
4864+
let initial_count = initial_members.len();
4865+
let member_to_remove = members[0].public_key();
4866+
4867+
// Remove a member — creates a pending commit but do NOT merge
4868+
let _remove_result = mdk
4869+
.remove_members(&group_id, &[member_to_remove])
4870+
.expect("remove_members should succeed");
4871+
4872+
// Clear the pending commit
4873+
mdk.clear_pending_commit(&group_id)
4874+
.expect("clear_pending_commit should succeed");
4875+
4876+
// Verify the member was NOT removed
4877+
let after_clear = mdk.get_members(&group_id).expect("get members");
4878+
assert_eq!(
4879+
after_clear.len(),
4880+
initial_count,
4881+
"Member count should be unchanged after clearing pending remove commit"
4882+
);
4883+
assert!(
4884+
after_clear.contains(&member_to_remove),
4885+
"Member should still be in group after clearing pending remove commit"
4886+
);
4887+
4888+
// Verify the group is usable again — retry the removal
4889+
mdk.remove_members(&group_id, &[member_to_remove])
4890+
.expect("remove_members should succeed after clearing pending commit");
4891+
mdk.merge_pending_commit(&group_id)
4892+
.expect("merge should succeed after clearing pending commit");
4893+
4894+
let final_members = mdk.get_members(&group_id).expect("get members");
4895+
assert_eq!(
4896+
final_members.len(),
4897+
initial_count - 1,
4898+
"Member should be removed after clearing stale commit and retrying"
4899+
);
4900+
assert!(
4901+
!final_members.contains(&member_to_remove),
4902+
"Removed member should not be in group after successful retry"
4903+
);
4904+
}
4905+
4906+
/// Tests that `clear_pending_commit` is a no-op when there is no pending commit.
4907+
#[test]
4908+
fn test_clear_pending_commit_no_pending() {
4909+
let mdk = create_test_mdk();
4910+
let (creator, members, admins) = create_test_group_members();
4911+
let group_id = create_test_group(&mdk, &creator, &members, &admins);
4912+
4913+
// Clearing when there's nothing pending should succeed (no-op)
4914+
mdk.clear_pending_commit(&group_id)
4915+
.expect("clear_pending_commit should succeed even with no pending commit");
4916+
4917+
// Group should still be functional
4918+
let member_count = mdk.get_members(&group_id).expect("get members").len();
4919+
assert!(member_count > 0, "Group should still have members");
4920+
}
4921+
4922+
/// Tests that `clear_pending_commit` returns an error for a non-existent group.
4923+
#[test]
4924+
fn test_clear_pending_commit_group_not_found() {
4925+
let mdk = create_test_mdk();
4926+
let fake_group_id = mdk_storage_traits::GroupId::from_slice(&[0u8; 16]);
4927+
4928+
let result = mdk.clear_pending_commit(&fake_group_id);
4929+
assert!(
4930+
result.is_err(),
4931+
"clear_pending_commit should error for non-existent group"
4932+
);
4933+
}
47764934
}

0 commit comments

Comments
 (0)