fix(cgka-engine): phantom committed_from from failed commit staging poisons fork detection#889
Open
dannym-arx wants to merge 2 commits into
Open
fix(cgka-engine): phantom committed_from from failed commit staging poisons fork detection#889dannym-arx wants to merge 2 commits into
dannym-arx wants to merge 2 commits into
Conversation
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.
…ted_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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
WalkthroughChangesFork detection staging fix
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Ready to review this PR? Stage has broken it down into 4 individual chapters for you: 
Chapters generated by Stage for commit cb279f5 on Jul 16, 2026 7:02am UTC. |
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
1 participant
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.
Four send paths (invite, remove-members, capability upgrade, group-data update) recorded "we committed from this epoch" while commit staging could still fail. When staging failed, the cleanup guard cleared the staged OpenMLS commit, but the committed_from entry stayed behind. A marmot that later advanced past that epoch through a peer's commit (settled via convergence, so it holds no fork-recovery snapshot of its own) then mis-routed a legitimate same-epoch sibling commit: the stale entry blocked convergence entry, the WrongEpoch fork branch found no recovery snapshot, and ingest failed closed with ForkedEpoch. The group got stuck in Recovering and rejected further sends.
begin_pending already records committed_from atomically with the pending-publish transition, so the four manual calls were redundant on success and harmful on failure. This PR removes them and deletes the now-unused EpochManager::record_committed_from.
Tests:
Verified with just fast-ci, cargo test -p cgka-engine, and cargo test -p cgka-conformance-simulator.
Summary by CodeRabbit
Bug Fixes
Documentation