From 392483411dc0014c4fd666b9e233a107c204b132 Mon Sep 17 00:00:00 2001 From: karamble Date: Tue, 30 Jun 2026 13:36:44 +0200 Subject: [PATCH] client: don't fail GC part/kill when the GC has unKXd members PartFromGC and KillGroupChat delete the GC inside their db transaction and then call sendToGCMembers to notify the pre-delete member list. When the GC has unKXd members, sendToGCMembers re-reads the GC to find the owner to mediate transitive KX through, but the GC is already gone, so GetGC returns ErrNotFound and the whole part/kill errors out even though it succeeded. Tolerate the GC being gone: stop setting up mediate-ID requests (the message was already sent to the KX'd members) instead of returning the error. Adds a regression test (TestGCPartWithUnkxdMember) that fails without the fix. --- client/client_groupchat.go | 10 ++++++ internal/e2etests/gc_test.go | 59 ++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/client/client_groupchat.go b/client/client_groupchat.go index 65ccb006..2fd181de 100644 --- a/client/client_groupchat.go +++ b/client/client_groupchat.go @@ -588,6 +588,16 @@ func (c *Client) sendToGCMembers(gcID zkidentity.ShortID, // Fetch the group list if needed. if !gotGCInfo { gcl, err := c.db.GetGC(tx, gcID) + if errors.Is(err, clientdb.ErrNotFound) { + // The GC was deleted before notifying members + // (PartFromGC and KillGroupChat delete the GC, + // then send the part/kill to the pre-delete + // member list). With no GC there is no owner to + // mediate transitive KX through, so stop setting + // up mediate-ID requests; the message was already + // sent to the KX'd members. + break + } if err != nil { return err } diff --git a/internal/e2etests/gc_test.go b/internal/e2etests/gc_test.go index bdb19a45..393f3350 100644 --- a/internal/e2etests/gc_test.go +++ b/internal/e2etests/gc_test.go @@ -1159,6 +1159,65 @@ func TestGCUnkxMediateIDRetries(t *testing.T) { assert.ChanNotWritten(t, bobGCUnkxdMemberChan, 100*time.Millisecond) } +// TestGCPartWithUnkxdMember verifies that parting from a GC succeeds even when it +// still contains a member the local client has not KXd with. PartFromGC deletes +// the GC before sending the part to members, so the unkxd-member mediate-ID path +// (which re-reads the GC) must tolerate the GC being gone instead of erroring. +func TestGCPartWithUnkxdMember(t *testing.T) { + t.Parallel() + + tcfg := testScaffoldCfg{} + ts := newTestScaffold(t, tcfg) + alice := ts.newClient("alice") + bob := ts.newClient("bob") + charlie := ts.newClient("charlie") + + // KX Alice to Bob and Charlie (but not Bob to Charlie). + ts.kxUsers(alice, bob) + ts.kxUsers(alice, charlie) + + // Alice creates the GC and adds Bob. + gcID, err := alice.NewGroupChat("gc01") + assert.NilErr(t, err) + assertClientJoinsGC(t, gcID, alice, bob) + + bobNewMembersChan := make(chan clientintf.UserID, 3) + bob.handle(client.OnAddedGCMembersNtfn(func(_ rpc.RMGroupList, uids []clientintf.UserID) { + for _, uid := range uids { + bobNewMembersChan <- uid + } + })) + alicePartChan := make(chan clientintf.UserID, 1) + alice.handle(client.OnGCUserPartedNtfn(func(_ client.GCID, uid client.UserID, _ string, _ bool) { + alicePartChan <- uid + })) + + // Add Charlie but keep the Bob<->Charlie KX from completing, so Charlie + // stays unkxd from Bob's point of view. + assertGoesOffline(t, charlie) + assert.NilErr(t, alice.InviteToGroupChat(gcID, charlie.PublicID())) + assertGoesOffline(t, alice) + + charlieAcceptedChan := charlie.acceptNextGCInvite(gcID) + assertGoesOnline(t, charlie) + assert.NilErrFromChan(t, charlieAcceptedChan) + assertGoesOffline(t, charlie) + + assertGoesOnline(t, alice) + assert.ChanWrittenWithVal(t, bobNewMembersChan, charlie.PublicID()) + + // Wait until Bob would actually attempt a mediate-ID for the unkxd Charlie + // (a recently added member is otherwise skipped) - that is the path that + // re-reads the GC. + time.Sleep(bob.cfg.RecentMediateIDThreshold) + + // Bob parts. The part deletes the GC then notifies members; with Charlie + // unkxd this hits the mediate-ID re-read of the now-deleted GC, which must + // not fail the part. + assert.NilErr(t, bob.PartFromGC(gcID, "leaving")) + assert.ChanWrittenWithVal(t, alicePartChan, bob.PublicID()) +} + // TestGCChangesOwner tests that changing the owner of the GC works as expected. func TestGCChangesOwner(t *testing.T) { t.Parallel()