From c2b0f9358397ca36133d6462168a54ea12da7c84 Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Tue, 1 Sep 2026 03:15:08 -0700 Subject: [PATCH] feat: close the last silent-success window on the segment pin: the in-transaction segment_members insert now compares RowsAffected against the deduplicated segment list and rolls the create back with a 400 when a segment was deleted between the service's existence check and the write, plus a live repository test that a contact created with a segment is pinned as an include override while a sibling naming none stays out, and that a create naming a segment the organization does not have fails and leaves no contact behind --- internal/repository/pg_contact.go | 12 ++++- internal/repository/segment_live_test.go | 57 ++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/internal/repository/pg_contact.go b/internal/repository/pg_contact.go index 93d90290..fd0068d3 100644 --- a/internal/repository/pg_contact.go +++ b/internal/repository/pg_contact.go @@ -512,16 +512,24 @@ func (r *contactRepository) Add(ctx context.Context, userID string, orgID uuid.U if len(segs) == 0 { continue } - if _, err := tx.Exec(ctx, ` + tag, err := tx.Exec(ctx, ` INSERT INTO segment_members (segment_id, contact_id, mode) SELECT s.id, $1, 'include' FROM segments s WHERE s.id = ANY($2) AND s.organization_id = $3 ON CONFLICT (segment_id, contact_id) DO UPDATE SET mode = EXCLUDED.mode, created_at = now() - `, ncontacts[i].ID, segs, orgID); err != nil { + `, ncontacts[i].ID, segs, orgID) + if err != nil { db.CaptureError(err, "", nil, "segment_members insert") return nil, errx.InternalError() } + // One row per requested segment, since `segs` is deduplicated and a + // conflicting row still counts as affected. Fewer means a segment was + // deleted between the service's check and this statement, so the + // create is rolled back rather than answered without the membership. + if int(tag.RowsAffected()) != len(segs) { + return nil, errx.New(errx.BadRequest, "a selected segment does not exist") + } } if err := logContactCreated(ctx, tx, orgID, actor, createdIDs); err != nil { diff --git a/internal/repository/segment_live_test.go b/internal/repository/segment_live_test.go index c45ffeac..a68a815f 100644 --- a/internal/repository/segment_live_test.go +++ b/internal/repository/segment_live_test.go @@ -345,3 +345,60 @@ func TestLiveSegmentCampaignLinks(t *testing.T) { t.Errorf("leads after unlink = %d, want 3", got) } } + +// Issue #285: a contact created inside a segment joins it, and the include +// override is written in the same transaction as the contact. A segment that +// vanishes between the service's existence check and this insert must roll the +// whole create back rather than answer with a membership that never happened. +func TestLiveContactAddPinsSegments(t *testing.T) { + f, repo := newSegmentFixture(t) + ctx := context.Background() + handle, pool := liveContactDB(t) + contacts := NewContactRepostory(handle) + + acme, xerr := repo.Create(ctx, f.org, &f.owner, &models.Segment{ + Name: "Pin target", Color: "#0284c7", Match: models.SegmentMatchAll, + Conditions: []models.SegmentCondition{{Field: "company", Operator: "equals", Value: "acme"}}, + }) + if xerr != nil { + t.Fatalf("create segment: %v", xerr) + } + + // A company the conditions do not match, so membership can only come from + // the pin. The second contact names no segment and must stay out. + tag := uuid.New().String()[:6] + made, xerr := contacts.Add(ctx, f.owner.String(), f.org, []models.AddContact{ + {Email: "pinned-" + tag + "@initech.test", FirstName: "Pinned", Company: "initech", Segments: []string{acme.ID.String(), acme.ID.String()}}, + {Email: "loose-" + tag + "@initech.test", FirstName: "Loose", Company: "initech"}, + }) + if xerr != nil || len(made) != 2 { + t.Fatalf("add: %+v, %v", made, xerr) + } + modes, xerr := repo.MemberModes(ctx, acme.ID, []uuid.UUID{made[0].ID, made[1].ID}) + if xerr != nil { + t.Fatalf("modes: %v", xerr) + } + if modes[made[0].ID] != models.SegmentMemberInclude { + t.Errorf("pinned contact mode = %q, want include", modes[made[0].ID]) + } + if _, ok := modes[made[1].ID]; ok { + t.Errorf("contact that named no segment was pinned") + } + + // A segment id that is not in the organization (the state the race leaves + // behind) fails the create instead of dropping the membership silently. + gone := uuid.New().String() + email := "vanished-" + tag + "@initech.test" + if _, xerr := contacts.Add(ctx, f.owner.String(), f.org, []models.AddContact{ + {Email: email, FirstName: "Vanished", Company: "initech", Segments: []string{gone}}, + }); xerr == nil { + t.Fatalf("create with a vanished segment succeeded") + } + var stranded int + if err := pool.QueryRow(ctx, `SELECT COUNT(*) FROM contacts WHERE organization_id = $1 AND email = $2`, f.org, email).Scan(&stranded); err != nil { + t.Fatalf("count: %v", err) + } + if stranded != 0 { + t.Errorf("rolled-back create left %d contacts behind", stranded) + } +}