From 9b6aa1fd728d72446c99f5578b19986aeb9e2cf8 Mon Sep 17 00:00:00 2001 From: Danny Morabito Date: Thu, 16 Jul 2026 11:10:12 +0900 Subject: [PATCH 1/2] fix(cgka-engine): record committed_from only inside begin_pending do_send_invite, do_send_remove_members, do_upgrade_group_capabilities, and do_update_group_data called record_committed_from before (or apart from) staging the commit. When staging failed after that point, the cleanup guard cleared the OpenMLS pending commit but nothing pruned the phantom "we committed from this epoch" entry. Once the client later advanced past that epoch via a peer's commit settled through convergence (so no fork-recovery incumbent of its own exists), a legitimate same-epoch sibling commit was mis-routed: the phantom entry blocked convergence entry, the WrongEpoch fork branch found no recovery snapshot, and ingest failed closed with ForkedEpoch, sticking the group in Recovering. begin_pending already records committed_from atomically with the state transition (the Sm1 discipline), so the four manual call sites were redundant on success and harmful on failure. Remove them and delete the now-unused EpochManager::record_committed_from (its doc comment claimed the auto-committer used it; the auto-committer goes through begin_pending). Regression test reproduces the full chain: failed duplicate-member invite, peer-driven advance via convergence, then a sibling commit for the past epoch must classify instead of failing closed, and the group must remain usable. --- crates/cgka-engine/src/epoch_manager.rs | 9 -- .../cgka-engine/src/message_processor/send.rs | 14 +- crates/cgka-engine/src/update_group_data.rs | 5 +- crates/cgka-engine/src/upgrade.rs | 5 +- crates/cgka-engine/tests/fork_detection.rs | 128 ++++++++++++++++++ 5 files changed, 142 insertions(+), 19 deletions(-) diff --git a/crates/cgka-engine/src/epoch_manager.rs b/crates/cgka-engine/src/epoch_manager.rs index 6dda71a9..2033cc19 100644 --- a/crates/cgka-engine/src/epoch_manager.rs +++ b/crates/cgka-engine/src/epoch_manager.rs @@ -243,15 +243,6 @@ impl EpochManager { Ok((group_id, prior_epoch)) } - /// Record a committed-from epoch outside the begin_pending path (used - /// by the auto-committer, which doesn't go through PendingPublish). - pub(crate) fn record_committed_from(&mut self, group_id: &GroupId, epoch: EpochId) { - self.committed_from - .entry(group_id.clone()) - .or_default() - .insert(epoch); - } - pub(crate) fn prune_committed_from_before( &mut self, group_id: &GroupId, diff --git a/crates/cgka-engine/src/message_processor/send.rs b/crates/cgka-engine/src/message_processor/send.rs index 2ecec76e..04aabde6 100644 --- a/crates/cgka-engine/src/message_processor/send.rs +++ b/crates/cgka-engine/src/message_processor/send.rs @@ -124,8 +124,14 @@ impl Engine { parsed_kps.push(parsed); } - // Record the pre-commit epoch so a later same-epoch commit can be - // compared against this locally produced branch. + // The pre-commit epoch is the commit origin for fork detection. + // `committed_from` bookkeeping is deliberately NOT recorded here: it + // happens atomically inside `begin_pending` below, after staging + // succeeds. Recording it earlier left a phantom "we committed from + // this epoch" entry behind when staging failed (the cleanup guard + // clears the OpenMLS commit but nothing prunes `committed_from`), + // which later mis-routed legitimate sibling commits into + // fail-closed fork recovery. let pre_commit_epoch = EpochId(mls_group.epoch().as_u64()); // Arm the cleanup guard before creating the snapshot so the snapshot is // released on early return / cancellation even before a pending commit @@ -142,8 +148,6 @@ impl Engine { pre_commit_epoch, "pre_invite_commit", ); - self.epoch_manager - .record_committed_from(&group_id, pre_commit_epoch); let pre_commit_ctx = crate::group_lifecycle::build_group_context_snapshot(&mls_group, &provider)?; @@ -464,8 +468,6 @@ impl Engine { )?; let commit_priority = crate::app_components::commit_ordering_priority_for_staged(staged_commit); - self.epoch_manager - .record_committed_from(&group_id, pre_commit_epoch); let commit_bytes = commit_out .tls_serialize_detached() diff --git a/crates/cgka-engine/src/update_group_data.rs b/crates/cgka-engine/src/update_group_data.rs index 0b89b683..de528b2b 100644 --- a/crates/cgka-engine/src/update_group_data.rs +++ b/crates/cgka-engine/src/update_group_data.rs @@ -210,8 +210,9 @@ impl Engine { pre_commit_epoch, "pre_update_group_data_commit", ); - self.epoch_manager - .record_committed_from(&group_id, pre_commit_epoch); + // `committed_from` is recorded atomically by `begin_pending` after + // staging succeeds — never here, where a staging failure would leave + // a phantom entry behind (see do_send_invite). let pre_commit_ctx = crate::group_lifecycle::build_group_context_snapshot(&mls_group, &provider)?; diff --git a/crates/cgka-engine/src/upgrade.rs b/crates/cgka-engine/src/upgrade.rs index eb728563..ab036c0d 100644 --- a/crates/cgka-engine/src/upgrade.rs +++ b/crates/cgka-engine/src/upgrade.rs @@ -158,8 +158,9 @@ impl Engine { pre_commit_epoch, "pre_upgrade_commit", ); - self.epoch_manager - .record_committed_from(group_id, pre_commit_epoch); + // `committed_from` is recorded atomically by `begin_pending` after + // staging succeeds — never here, where a staging failure would leave + // a phantom entry behind (see do_send_invite). let pre_commit_ctx = crate::group_lifecycle::build_group_context_snapshot(&mls_group, &provider)?; diff --git a/crates/cgka-engine/tests/fork_detection.rs b/crates/cgka-engine/tests/fork_detection.rs index 6dea5853..5175d548 100644 --- a/crates/cgka-engine/tests/fork_detection.rs +++ b/crates/cgka-engine/tests/fork_detection.rs @@ -687,3 +687,131 @@ async fn stale_commit_without_own_commit_is_classified_as_already_at_epoch_not_f } )); } + +#[tokio::test] +async fn failed_invite_staging_does_not_poison_fork_detection() { + // Regression: `do_send_invite` used to record `committed_from` BEFORE + // staging the commit. When staging failed, the cleanup guard cleared the + // OpenMLS pending commit but nothing pruned the phantom "we committed + // from this epoch" entry. Once Alice later advanced past that epoch via a + // PEER's commit (settled through convergence — so no fork-recovery + // incumbent of her own exists), a legitimate same-epoch sibling commit + // was mis-routed: the phantom blocked convergence entry, the WrongEpoch + // fork branch found no recovery snapshot, and ingest failed closed with + // ForkedEpoch, sticking the group in Recovering. `committed_from` is now + // recorded only inside `begin_pending`, atomically with the transition. + let (mut alice, _alice_storage) = build_client_with_storage(b"phantom-alice"); + let mut bob = build_client(b"phantom-bob"); + let mut carol = build_client(b"phantom-carol"); + let (mut dave, dave_storage) = build_client_with_storage(b"phantom-dave"); + let dave_id = MemberId::new(pad32(b"phantom-dave")); + + let bob_kp = bob.fresh_key_package().await.unwrap(); + let dave_kp = dave.fresh_key_package().await.unwrap(); + let (group_id, create) = alice + .create_group(CreateGroupRequest { + name: "".into(), + description: "".into(), + members: vec![bob_kp, dave_kp], + required_features: vec![], + app_components: vec![], + // Bob must be an admin so his sibling-epoch invite below passes + // the MIP-03 committer guard. + initial_admins: vec![MemberId::new(pad32(b"phantom-bob"))], + }) + .await + .unwrap(); + let (bob_welcome, dave_welcome) = match create { + SendResult::GroupCreated { + pending, + mut welcomes, + } => { + alice.confirm_published(pending).await.unwrap(); + let dave_welcome = welcomes.remove(1); + (welcomes.remove(0), dave_welcome) + } + _ => unreachable!(), + }; + bob.join_welcome(bob_welcome).await.unwrap(); + dave.join_welcome(dave_welcome).await.unwrap(); + + // Dave produces (but never publishes/applies) a sibling commit from the + // current epoch — the legitimate same-epoch race Alice must classify + // later. + let dave_sibling_commit = raw_self_update_commit(&dave_storage, &dave_id, &group_id); + + // Alice's invite fails DURING staging: the duplicate-Bob KeyPackage is + // signed with Bob's existing leaf signature key, which OpenMLS rejects + // inside `add_members` — after capability validation, before + // `begin_pending`. + let duplicate_bob_kp = bob.fresh_key_package().await.unwrap(); + let failed = alice + .send(SendIntent::Invite { + group_id: group_id.clone(), + key_packages: vec![duplicate_bob_kp], + }) + .await; + assert!( + failed.is_err(), + "duplicate-member invite must fail during staging" + ); + + // Bob commits from the same epoch (invites Carol). Alice advances by + // settling Bob's commit through convergence — a peer-driven advance, so + // she has no fork-recovery incumbent of her own for the source epoch. + let carol_kp = carol.fresh_key_package().await.unwrap(); + let invite = bob + .send(SendIntent::Invite { + group_id: group_id.clone(), + key_packages: vec![carol_kp], + }) + .await + .unwrap(); + let (bob_commit, pending) = match invite { + SendResult::GroupEvolution { msg, pending, .. } => (msg, pending), + _ => unreachable!(), + }; + bob.confirm_published(pending).await.unwrap(); + let routed_bob_commit = TransportMessage { + envelope: TransportEnvelope::GroupMessage { + transport_group_id: group_id.as_slice().to_vec(), + }, + ..bob_commit + }; + alice + .buffer_openmls_convergence_message(&group_id, routed_bob_commit, 1_000) + .expect("bob commit buffered"); + alice + .converge_stored_openmls_messages(&group_id, 3_000) + .expect("bob commit settles"); + assert_eq!(alice.epoch(&group_id).unwrap(), EpochId(2)); + + // Dave's sibling commit for the now-past epoch arrives. Alice never + // published a commit from that epoch, so ingest must classify it (stale + // losing branch, or a reorg onto the winning branch) — NOT take the + // fork-recovery path, which with the phantom entry and no tracked + // snapshot failed closed with ForkedEpoch and left the group stuck in + // Recovering. + alice + .ingest(dave_sibling_commit) + .await + .expect("sibling commit after failed staging must not fail closed"); + + // The group is still operational: a fresh (valid) invite from Alice is + // accepted — staged immediately or queued behind unresolved convergence + // input. In Recovering it would error instead (`begin_pending` rejects + // non-Stable states). + let mut erin = build_client(b"phantom-erin"); + let erin_kp = erin.fresh_key_package().await.unwrap(); + let recovery_probe = alice + .send(SendIntent::Invite { + group_id: group_id.clone(), + key_packages: vec![erin_kp], + }) + .await + .expect("group must remain usable after failed staging + sibling commit"); + assert!(matches!( + recovery_probe, + SendResult::GroupEvolution { .. } | SendResult::Queued { .. } + )); +} From cb279f5dacdec169baab9af5b40466dac26f46bc Mon Sep 17 00:00:00 2001 From: Danny Morabito Date: Thu, 16 Jul 2026 11:27:08 +0900 Subject: [PATCH 2/2] test(cgka-conformance-simulator): harness scenario for phantom committed_from Mirror the cgka-engine regression test failed_invite_staging_does_not_poison_fork_detection at the multi-client bus level: Alice fails an invite during commit staging, advances to epoch 2 by settling Bob's commit through convergence, and a partitioned Dave later delivers a sibling commit from the same source epoch. The sibling must be adjudicated deterministically instead of failing closed with ForkedEpoch, and the group must stay usable for a follow-up invite. Adds HarnessClient::try_invite so scenarios can drive a deliberately failing invite without panicking (invite() now delegates to it), and registers the scenario in SCENARIOS.md. Verified the scenario fails with Err(ForkedEpoch) against the pre-fix engine. --- .../cgka-conformance-simulator/SCENARIOS.md | 11 ++ .../cgka-conformance-simulator/src/client.rs | 20 ++- .../tests/canonical_scenarios.rs | 118 ++++++++++++++++++ 3 files changed, 145 insertions(+), 4 deletions(-) diff --git a/crates/cgka-conformance-simulator/SCENARIOS.md b/crates/cgka-conformance-simulator/SCENARIOS.md index 56038030..1a6170fc 100644 --- a/crates/cgka-conformance-simulator/SCENARIOS.md +++ b/crates/cgka-conformance-simulator/SCENARIOS.md @@ -190,6 +190,17 @@ These are real simulator scenarios that are still tied to Rust harness details. set. - Reason: the test inspects recovery observations and exact branch ordering keys. +### `failed_invite_staging_does_not_poison_fork_detection_via_harness` + +- Setup: Alice's invite fails during commit staging (duplicate-member KeyPackage). Alice then advances to epoch 2 by + settling Bob's commit through convergence, and a partitioned Dave later delivers a sibling commit from the same + source epoch. +- Expected: the sibling commit is adjudicated deterministically (stale losing branch or reorg onto the winner) — never + a fail-closed `ForkedEpoch` — and Alice's group stays usable for a follow-up invite. +- Reason: regression guard for the phantom `committed_from` bookkeeping bug (fixed by recording it only inside + `begin_pending`); mirrors the cgka-engine test `failed_invite_staging_does_not_poison_fork_detection` at the + multi-client bus level. + ### `convergence-e2e-group-events/v1` - Setup: Alice creates a four-member group. Alice and Bob race invite commits and each branch has an app payload. Carol diff --git a/crates/cgka-conformance-simulator/src/client.rs b/crates/cgka-conformance-simulator/src/client.rs index 09505332..80b71dc0 100644 --- a/crates/cgka-conformance-simulator/src/client.rs +++ b/crates/cgka-conformance-simulator/src/client.rs @@ -567,6 +567,17 @@ impl HarnessClient { /// Invite new members to the default group. pub async fn invite(&mut self, kps: Vec) -> PendingStateRef { + self.try_invite(kps).await.expect("send invite") + } + + /// Invite new members to the default group, surfacing the engine error + /// instead of panicking. Nothing reaches the bus on failure. Used by + /// scenarios that deliberately fail an invite during commit staging + /// (e.g. the phantom committed-from regression). + pub async fn try_invite( + &mut self, + kps: Vec, + ) -> Result { let gid = self.default_group.clone().expect("group"); let res = self .engine @@ -574,8 +585,7 @@ impl HarnessClient { group_id: gid.clone(), key_packages: kps, }) - .await - .expect("send invite"); + .await?; match res { SendResult::GroupEvolution { msg, @@ -589,9 +599,11 @@ impl HarnessClient { self.bus.send(self.bus_id, w); } self.bus.send(self.bus_id, route(msg, &gid)); - pending + Ok(pending) } - other => panic!("expected GroupEvolution, got {other:?}"), + other => Err(EngineError::Other(format!( + "expected GroupEvolution, got {other:?}" + ))), } } diff --git a/crates/cgka-conformance-simulator/tests/canonical_scenarios.rs b/crates/cgka-conformance-simulator/tests/canonical_scenarios.rs index 4743fa38..5e4507bd 100644 --- a/crates/cgka-conformance-simulator/tests/canonical_scenarios.rs +++ b/crates/cgka-conformance-simulator/tests/canonical_scenarios.rs @@ -2041,3 +2041,121 @@ async fn harness_captures_and_asserts_convergence_decision() { "carol should observe the committer-decided convergence decision: {failures:#?}" ); } + +#[tokio::test] +async fn failed_invite_staging_does_not_poison_fork_detection_via_harness() { + // Harness mirror of the cgka-engine regression test + // `failed_invite_staging_does_not_poison_fork_detection`: a send-path + // staging failure must leave no phantom "we committed from this epoch" + // bookkeeping behind. Historically it did, and once the client advanced + // past that epoch via a PEER's commit (settled through convergence, so + // no fork-recovery incumbent of its own exists), a legitimate sibling + // commit for the poisoned epoch failed closed with ForkedEpoch and stuck + // the group in Recovering. + let bus = TransportBus::ordered(); + let mut alice = ClientBuilder::new(pad32(b"phantom-alice")) + .registry(selfremove_registry()) + .attach(&bus); + let mut bob = ClientBuilder::new(pad32(b"phantom-bob")) + .registry(selfremove_registry()) + .attach(&bus); + let mut carol = ClientBuilder::new(pad32(b"phantom-carol")) + .registry(selfremove_registry()) + .attach(&bus); + let mut dave = ClientBuilder::new(pad32(b"phantom-dave")) + .registry(selfremove_registry()) + .attach(&bus); + let mut erin = ClientBuilder::new(pad32(b"phantom-erin")) + .registry(selfremove_registry()) + .attach(&bus); + + // Bob and Dave are admins: each later commits an invite from epoch 1. + let bob_kp = bob.fresh_key_package().await; + let dave_kp = dave.fresh_key_package().await; + let (_group_id, pending) = alice + .create_group_with_admins( + "phantom-committed-from", + vec![bob_kp, dave_kp], + vec![], + vec![bob.member_id(), dave.member_id()], + ) + .await; + alice.confirm(pending).await; + bus.deliver_all(); + bob.tick().await; + dave.tick().await; + assert_eq!(alice.epoch().0, 1); + assert_eq!(bob.epoch().0, 1); + assert_eq!(dave.epoch().0, 1); + + // Alice's invite fails DURING commit staging: the duplicate-Bob + // KeyPackage carries Bob's existing leaf signature key, which OpenMLS + // rejects inside add_members — after capability validation, before the + // pending-publish state transition. Nothing reaches the bus. + let duplicate_bob_kp = bob.fresh_key_package().await; + let failed = alice.try_invite(vec![duplicate_bob_kp]).await; + assert!( + failed.is_err(), + "duplicate-member invite must fail during staging" + ); + + // Partition Dave away so he stays at epoch 1 and never sees Bob's + // commit. Bob invites Carol; Alice advances to epoch 2 purely by + // settling Bob's commit through convergence (peer-driven advance — no + // own commit, no fork-recovery incumbent at epoch 1). + bus.set_partition(Some(vec![alice.bus_id, bob.bus_id, carol.bus_id])); + let carol_kp = carol.fresh_key_package().await; + let bob_pending = bob.invite(vec![carol_kp]).await; + bob.confirm(bob_pending).await; + bus.deliver_all(); + carol.tick().await; + let alice_outcomes = alice.tick().await; + assert_eq!( + alice.epoch().0, + 2, + "alice must settle bob's commit via convergence: {alice_outcomes:?}" + ); + + // Heal the partition. Dave — still at epoch 1 — commits a sibling + // invite from the epoch Alice's failed staging attempt touched. + bus.set_partition(None); + let erin_kp = erin.fresh_key_package().await; + let dave_pending = dave.invite(vec![erin_kp]).await; + dave.confirm(dave_pending).await; + bus.deliver_all(); + + // Alice must classify the sibling (stale losing branch, or a + // deterministic reorg onto the winning branch) — never fail closed with + // ForkedEpoch, which left the group stuck in Recovering. + let alice_outcomes = alice.tick().await; + let alice_forked = alice_outcomes + .iter() + .any(|o| matches!(o, Err(cgka_traits::EngineError::ForkedEpoch { .. }))); + assert!( + !alice_forked, + "sibling commit after failed staging must not fail closed: {alice_outcomes:?}" + ); + assert_eq!(alice.epoch().0, 2); + + // Whichever branch won deterministically, alice holds exactly one of + // the two invitees and the group remains operational. + let members = alice.members(); + let has_carol = members.iter().any(|m| m.id == carol.member_id()); + let has_erin = members.iter().any(|m| m.id == erin.member_id()); + assert_ne!( + has_carol, has_erin, + "exactly one branch must win: {members:?}" + ); + // Usability probe: a fresh valid invite from Alice must stage normally. + // A group stuck in Recovering rejects the send outright. + let mut frank = ClientBuilder::new(pad32(b"phantom-frank")) + .registry(selfremove_registry()) + .attach(&bus); + let frank_kp = frank.fresh_key_package().await; + let probe_pending = alice + .try_invite(vec![frank_kp]) + .await + .expect("group must remain usable after failed staging + sibling commit"); + alice.confirm(probe_pending).await; + assert_eq!(alice.epoch().0, 3); +}