b
' WHERE campaign_id = $1`, campaign); err != nil { + t.Fatalf("html bodies: %v", err) + } + svc := NewService(NewPgStore(pool), sqlChecker{pool: pool}, NewPgVariantStore(q), NewPgBranchStore(pool)) + return branchIT{pool: pool, q: q, svc: svc, ws: ws, campaign: campaign, steps: ids}, pool.Close +} + +func (b branchIT) addStep(t *testing.T, order int32) uuid.UUID { + t.Helper() + st, err := b.q.CreateStep(context.Background(), gen.CreateStepParams{ + WorkspaceID: b.ws, CampaignID: b.campaign, StepOrder: order, Subject: "s", BodyText: "b", + }) + if err != nil { + t.Fatalf("step %d: %v", order, err) + } + return st.ID +} + +func (b branchIT) always(t *testing.T, from uuid.UUID, to *uuid.UUID) { + t.Helper() + if _, err := b.svc.SetBranch(context.Background(), b.ws, b.campaign, BranchInput{StepID: from, Condition: "always", YesStepID: to}); err != nil { + t.Fatalf("always %v -> %v: %v", from, to, err) + } +} + +func TestBranchSaveAndReadBack(t *testing.T) { + b, done := newBranchIT(t, "Branch save") + defer done() + ctx := context.Background() + three := int32(3) + // A custom label that does NOT stop the enrollment: the only kind a branch + // can route on (the builtin human labels all stop the sequence). + label := "soft_yes" + if _, err := b.pool.Exec(ctx, ` + INSERT INTO reply_labels (workspace_id, key, label, color, position, stops_enrollment) + VALUES ($1, $2, 'Soft yes', '#123456', 9, false)`, b.ws, label); err != nil { + t.Fatalf("custom label: %v", err) + } + got, err := b.svc.SetBranch(ctx, b.ws, b.campaign, BranchInput{ + StepID: b.steps[0], Condition: "replied", WithinDays: &three, ReplyLabelKey: &label, + YesStepID: &b.steps[2], NoStepID: &b.steps[1], + }) + if err != nil { + t.Fatalf("SetBranch: %v", err) + } + if got.WorkspaceID != b.ws || got.CampaignID != b.campaign || got.ReplyLabelKey == nil || *got.ReplyLabelKey != label { + t.Fatalf("stored = %+v", got) + } + + // Replace in place: one row per step. + if _, err := b.svc.SetBranch(ctx, b.ws, b.campaign, BranchInput{StepID: b.steps[0], Condition: "always"}); err != nil { + t.Fatalf("replace: %v", err) + } + g, err := b.svc.Graph(ctx, b.ws, b.campaign) + if err != nil { + t.Fatal(err) + } + if len(g.Branches) != 1 || g.Branches[0].Condition != "always" || g.Branches[0].WithinDays != nil || g.Branches[0].YesStepID.Valid { + t.Fatalf("graph branches = %+v", g.Branches) + } + + if err := b.svc.DeleteBranch(ctx, b.ws, b.campaign, b.steps[0], BranchPrecondition{}); err != nil { + t.Fatalf("delete: %v", err) + } + if g, _ := b.svc.Graph(ctx, b.ws, b.campaign); len(g.Branches) != 0 { + t.Fatalf("branch survived delete: %+v", g.Branches) + } +} + +// Against the real seeded taxonomy: every builtin human label stops the +// enrollment, so none of them can be named by a reply branch, while the +// automated ones (which leave the enrollment running) can. +func TestBranchRefusesSeededStoppingLabels(t *testing.T) { + b, done := newBranchIT(t, "Branch seeded labels") + defer done() + ctx := context.Background() + two := int32(2) + for _, key := range []string{"positive", "negative", "neutral", "unknown", "unsubscribe"} { + k := key + _, err := b.svc.SetBranch(ctx, b.ws, b.campaign, BranchInput{ + StepID: b.steps[0], Condition: "replied", WithinDays: &two, ReplyLabelKey: &k, YesStepID: &b.steps[2], + }) + if seqgraph.CodeOf(err) != seqgraph.CodeLabelStopsSequence { + t.Errorf("%s: code %q (%v), want %q", key, seqgraph.CodeOf(err), err, seqgraph.CodeLabelStopsSequence) + } + } + ooo := "out_of_office" + if _, err := b.svc.SetBranch(ctx, b.ws, b.campaign, BranchInput{ + StepID: b.steps[0], Condition: "replied", WithinDays: &two, ReplyLabelKey: &ooo, YesStepID: &b.steps[2], + }); err != nil { + t.Fatalf("an automated (non-stopping) label is routable: %v", err) + } +} + +// Tracking off on the campaign refuses an open branch against the real column. +func TestBranchOpenRefusedWhenTrackingOff(t *testing.T) { + b, done := newBranchIT(t, "Branch tracking off") + defer done() + ctx := context.Background() + if _, err := b.pool.Exec(ctx, `UPDATE campaigns SET tracking_enabled = false WHERE id = $1`, b.campaign); err != nil { + t.Fatal(err) + } + one := int32(1) + _, err := b.svc.SetBranch(ctx, b.ws, b.campaign, BranchInput{StepID: b.steps[0], Condition: "opened", WithinDays: &one}) + if seqgraph.CodeOf(err) != seqgraph.CodeTrackingRequired { + t.Fatalf("code %q (%v), want %q", seqgraph.CodeOf(err), err, seqgraph.CodeTrackingRequired) + } +} + +func TestBranchCycleRefusedAndNothingWritten(t *testing.T) { + b, done := newBranchIT(t, "Branch cycle") + defer done() + _, err := b.svc.SetBranch(context.Background(), b.ws, b.campaign, BranchInput{ + StepID: b.steps[2], Condition: "always", YesStepID: &b.steps[0], + }) + var cyc *seqgraph.CycleError + if !errors.As(err, &cyc) { + t.Fatalf("want CycleError, got %v", err) + } + if g, _ := b.svc.Graph(context.Background(), b.ws, b.campaign); len(g.Branches) != 0 { + t.Fatalf("a refused edit was committed: %+v", g.Branches) + } +} + +// A target in ANOTHER campaign of the same workspace is refused by the service +// with a named error, and by the composite FK if the service is bypassed. +func TestBranchTargetMustBeInSameCampaign(t *testing.T) { + b, done := newBranchIT(t, "Branch target owner") + defer done() + ctx := context.Background() + _, _, others := seedThreeSteps(t, ctx, b.q, "Branch target other") + + _, err := b.svc.SetBranch(ctx, b.ws, b.campaign, BranchInput{StepID: b.steps[0], Condition: "always", YesStepID: &others[0]}) + var te *seqgraph.TargetError + if !errors.As(err, &te) { + t.Fatalf("service: want TargetError, got %v", err) + } + + store := NewPgBranchStore(b.pool) + _, err = store.UpsertBranch(ctx, b.ws, BranchInput{ + CampaignID: b.campaign, StepID: b.steps[0], Condition: "always", YesStepID: &others[0], + }, checkGraph) + if !errors.Is(err, ErrTargetGone) { + t.Fatalf("store backstop: want ErrTargetGone (composite FK), got %v", err) + } +} + +// Every graph write takes the campaign lock on (campaign, workspace) first, so +// a foreign workspace writes nothing — even through the store directly. +func TestBranchWritesAreWorkspacePinned(t *testing.T) { + b, done := newBranchIT(t, "Branch tenant owner") + defer done() + ctx := context.Background() + intruder, err := b.q.CreateWorkspace(ctx, "Branch tenant intruder "+uuid.NewString()) + if err != nil { + t.Fatal(err) + } + + if _, err := b.svc.SetBranch(ctx, intruder.ID, b.campaign, BranchInput{StepID: b.steps[0], Condition: "always"}); !errors.Is(err, ErrCampaignNotFound) { + t.Fatalf("service: want ErrCampaignNotFound, got %v", err) + } + store := NewPgBranchStore(b.pool) + if _, err := store.UpsertBranch(ctx, intruder.ID, BranchInput{CampaignID: b.campaign, StepID: b.steps[0], Condition: "always"}, checkGraph); !errors.Is(err, ErrCampaignNotFound) { + t.Fatalf("store: want ErrCampaignNotFound, got %v", err) + } + if err := store.DeleteBranch(ctx, intruder.ID, b.campaign, b.steps[0], BranchPrecondition{}, checkGraph); !errors.Is(err, ErrCampaignNotFound) { + t.Fatalf("store delete: want ErrCampaignNotFound, got %v", err) + } + if rows, _ := b.q.ListBranchesByCampaign(ctx, gen.ListBranchesByCampaignParams{CampaignID: b.campaign, WorkspaceID: intruder.ID}); len(rows) != 0 { + t.Fatalf("foreign workspace sees %d branches", len(rows)) + } + if g, _ := b.svc.Graph(ctx, b.ws, b.campaign); len(g.Branches) != 0 { + t.Fatalf("intruder wrote into the owner's graph: %+v", g.Branches) + } +} + +// Deleting a target step turns the exits that pointed at it into ends (ON +// DELETE SET NULL on the target column only), and deleting a SOURCE step +// removes its router. +func TestStepDeleteNullsExitsAndDropsRouter(t *testing.T) { + b, done := newBranchIT(t, "Branch step delete") + defer done() + ctx := context.Background() + two := int32(2) + if _, err := b.svc.SetBranch(ctx, b.ws, b.campaign, BranchInput{ + StepID: b.steps[0], Condition: "opened", WithinDays: &two, YesStepID: &b.steps[2], NoStepID: &b.steps[1], + }); err != nil { + t.Fatal(err) + } + b.always(t, b.steps[1], nil) + + if err := b.svc.Delete(ctx, b.ws, b.campaign, b.steps[2]); err != nil { + t.Fatalf("delete target: %v", err) + } + g, err := b.svc.Graph(ctx, b.ws, b.campaign) + if err != nil { + t.Fatal(err) + } + for _, br := range g.Branches { + if br.StepID == b.steps[0] { + if br.YesStepID.Valid || !br.NoStepID.Valid || br.CampaignID != b.campaign { + t.Fatalf("after target delete = %+v, want yes nulled, no kept", br) + } + } + } + + if err := b.svc.Delete(ctx, b.ws, b.campaign, b.steps[1]); err != nil { + t.Fatalf("delete source: %v", err) + } + g, _ = b.svc.Graph(ctx, b.ws, b.campaign) + if len(g.Branches) != 1 || g.Branches[0].StepID != b.steps[0] || g.Branches[0].NoStepID.Valid { + t.Fatalf("after source delete = %+v", g.Branches) + } +} + +// A delete re-links the fall-through around the removed step. Here A -> B (by +// order), B -> D, C -> A, D ends: acyclic. Deleting B makes A fall through to C, +// and C -> A closes a loop, so the delete is refused and rolled back. +func TestStepDeleteRefusedWhenFallThroughClosesLoop(t *testing.T) { + b, done := newBranchIT(t, "Branch delete cycle") + defer done() + ctx := context.Background() + a, bb, c := b.steps[0], b.steps[1], b.steps[2] + d := b.addStep(t, 4) + b.always(t, bb, &d) + b.always(t, c, &a) + + err := b.svc.Delete(ctx, b.ws, b.campaign, bb) + if seqgraph.CodeOf(err) != seqgraph.CodeCycle { + t.Fatalf("want a cycle refusal, got %v", err) + } + if _, err := b.q.GetStep(ctx, gen.GetStepParams{ID: bb, WorkspaceID: b.ws}); err != nil { + t.Fatalf("the refused delete must be rolled back: %v", err) + } +} + +// Reordering moves every fall-through edge. A -> C explicitly with B and C +// falling through is acyclic; the order [C, B, A] makes C -> B -> A by order, +// and A -> C closes the loop. +func TestReorderRefusedWhenFallThroughClosesLoop(t *testing.T) { + b, done := newBranchIT(t, "Branch reorder cycle") + defer done() + ctx := context.Background() + a, bb, c := b.steps[0], b.steps[1], b.steps[2] + b.always(t, a, &c) + + _, err := b.svc.Reorder(ctx, b.ws, b.campaign, []uuid.UUID{c, bb, a}) + if seqgraph.CodeOf(err) != seqgraph.CodeCycle { + t.Fatalf("want a cycle refusal, got %v", err) + } + after, err := b.q.ListStepsByCampaign(ctx, gen.ListStepsByCampaignParams{CampaignID: b.campaign, WorkspaceID: b.ws}) + if err != nil { + t.Fatal(err) + } + if got := orderedIDs(after); !equalIDs(got, b.steps) { + t.Fatalf("refused reorder was committed: %v", got) + } +} + +// The composite FKs make a cross-campaign reference unrepresentable even for a +// raw write that skips every Go check. +func TestBranchSchemaRefusesForeignReferences(t *testing.T) { + b, done := newBranchIT(t, "Branch schema owner") + defer done() + ctx := context.Background() + otherWS, otherCampaign, others := seedThreeSteps(t, ctx, b.q, "Branch schema other") + + cases := map[string]struct { + ws, campaign, step uuid.UUID + yes *uuid.UUID + }{ + "source step of another campaign": {b.ws, b.campaign, others[0], nil}, + "campaign of another workspace": {b.ws, otherCampaign, others[0], nil}, + "target in another campaign": {b.ws, b.campaign, b.steps[0], &others[1]}, + "workspace mismatching campaign": {otherWS, b.campaign, b.steps[0], nil}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + _, err := b.q.UpsertBranch(ctx, gen.UpsertBranchParams{ + StepID: tc.step, WorkspaceID: tc.ws, CampaignID: tc.campaign, Condition: "always", + YesStepID: nullUUID(tc.yes), + }) + if err == nil { + t.Fatal("the schema accepted a foreign reference") + } + }) + } +} diff --git a/internal/app/sequencestep/branch_test.go b/internal/app/sequencestep/branch_test.go new file mode 100644 index 00000000..5feb6fbc --- /dev/null +++ b/internal/app/sequencestep/branch_test.go @@ -0,0 +1,441 @@ +package sequencestep + +import ( + "context" + "errors" + "slices" + "testing" + "time" + + "github.com/google/uuid" + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgtype" + + "github.com/inroad/inroad/internal/platform/db/gen" + "github.com/inroad/inroad/internal/platform/seqgraph" +) + +// fakeBranchStore keeps routers in memory and, like the real store, runs the +// GraphCheck against the graph AS IT WOULD BE COMMITTED, rolling back on error. +// steps is the campaign's step list the check sees. +type fakeBranchStore struct { + steps []gen.SequenceStep + branches map[uuid.UUID]gen.SequenceStepBranch + // labels maps a label key to whether it stops the enrollment; a missing key + // is a label that does not exist. + labels map[string]bool + // trackingOff models a campaign with tracking disabled. + trackingOff bool + upserts int + // expects records every precondition the store was handed, in call order. + expects []BranchPrecondition +} + +func (f *fakeBranchStore) ListBranches(context.Context, uuid.UUID, uuid.UUID) ([]gen.SequenceStepBranch, error) { + return f.list(), nil +} + +func (f *fakeBranchStore) ReplyLabelStops(_ context.Context, _ uuid.UUID, key string) (stops, found bool, err error) { + stops, found = f.labels[key] + return stops, found, nil +} + +func (f *fakeBranchStore) TrackingEnabled(context.Context, uuid.UUID, uuid.UUID) (bool, error) { + return !f.trackingOff, nil +} + +func (f *fakeBranchStore) UpsertBranch(_ context.Context, ws uuid.UUID, in BranchInput, check GraphCheck) (gen.SequenceStepBranch, error) { + f.expects = append(f.expects, in.Expect) + row := gen.SequenceStepBranch{ + StepID: in.StepID, WorkspaceID: ws, CampaignID: in.CampaignID, Condition: in.Condition, + WithinDays: in.WithinDays, ReplyLabelKey: in.ReplyLabelKey, + YesStepID: nullUUID(in.YesStepID), NoStepID: nullUUID(in.NoStepID), + } + prev, had := f.branches[in.StepID] + if f.branches == nil { + f.branches = map[uuid.UUID]gen.SequenceStepBranch{} + } + f.branches[in.StepID] = row + if err := check(f.steps, f.list()); err != nil { + if had { + f.branches[in.StepID] = prev + } else { + delete(f.branches, in.StepID) + } + return gen.SequenceStepBranch{}, err + } + f.upserts++ + return row, nil +} + +func (f *fakeBranchStore) DeleteBranch(_ context.Context, _, _, stepID uuid.UUID, expect BranchPrecondition, check GraphCheck) error { + f.expects = append(f.expects, expect) + prev, had := f.branches[stepID] + delete(f.branches, stepID) + if err := check(f.steps, f.list()); err != nil { + if had { + f.branches[stepID] = prev + } + return err + } + return nil +} + +func (f *fakeBranchStore) list() []gen.SequenceStepBranch { + out := make([]gen.SequenceStepBranch, 0, len(f.branches)) + for _, b := range f.branches { + out = append(out, b) + } + return out +} + +// branchFixture is a campaign with three linear steps and both fakes wired so +// the service's own step lookups and the store's graph check see the same steps. +type branchFixture struct { + svc *Service + branches *fakeBranchStore + variants *fakeVariantStore + steps *stepsByID + campaign uuid.UUID + ws uuid.UUID + step []uuid.UUID +} + +func newBranchFixture(t *testing.T, status string) branchFixture { + t.Helper() + campaign := uuid.New() + var steps []gen.SequenceStep + var ids []uuid.UUID + for i := range 3 { + id := uuid.New() + ids = append(ids, id) + steps = append(steps, gen.SequenceStep{ID: id, CampaignID: campaign, StepOrder: int32(i + 1), BodyHtml: "hi
"}) + } + // "positive" is a builtin label (stops the enrollment); "soft_yes" is a + // custom one that does not. + bs := &fakeBranchStore{steps: steps, labels: map[string]bool{"positive": true, "soft_yes": false}} + store := &stepsByID{steps: steps} + vs := &fakeVariantStore{} + return branchFixture{ + svc: NewService(store, fakeChecker{status: status}, vs, bs), + branches: bs, variants: vs, steps: store, campaign: campaign, ws: uuid.New(), step: ids, + } +} + +// stepsByID is a Store whose Get and List answer from a fixed step set, so +// assertStepInCampaign behaves as it does against the database. +type stepsByID struct { + fakeStore + steps []gen.SequenceStep +} + +func (s *stepsByID) Get(_ context.Context, _, id uuid.UUID) (gen.SequenceStep, error) { + for _, st := range s.steps { + if st.ID == id { + return st, nil + } + } + return gen.SequenceStep{}, errors.New("no rows") +} + +func (s *stepsByID) List(context.Context, uuid.UUID, uuid.UUID) ([]gen.SequenceStep, error) { + return s.steps, nil +} + +func ptr[T any](v T) *T { return &v } + +func TestSetBranchHappyPathOnRunningCampaign(t *testing.T) { + f := newBranchFixture(t, "running") + b, err := f.svc.SetBranch(context.Background(), f.ws, f.campaign, BranchInput{ + StepID: f.step[0], Condition: "opened", WithinDays: ptr(int32(3)), + YesStepID: &f.step[2], NoStepID: &f.step[1], + }) + if err != nil { + t.Fatalf("a branch edit is allowed live: %v", err) + } + if b.CampaignID != f.campaign || b.Condition != "opened" || !b.YesStepID.Valid || b.YesStepID.Bytes != f.step[2] { + t.Fatalf("stored branch = %+v", b) + } +} + +func TestSetBranchRejectsMissingCampaign(t *testing.T) { + f := newBranchFixture(t, "draft") + svc := NewService(&fakeStore{}, fakeChecker{err: pgx.ErrNoRows}, &fakeVariantStore{}, f.branches) + _, err := svc.SetBranch(context.Background(), f.ws, f.campaign, BranchInput{StepID: f.step[0], Condition: "always"}) + if !errors.Is(err, ErrCampaignNotFound) { + t.Fatalf("want ErrCampaignNotFound, got %v", err) + } +} + +// Only a genuine miss is a 404. A database failure looking the campaign up is +// a server error, and all three graph endpoints must say so rather than +// telling the client the campaign does not exist. +func TestGraphEndpointsDoNotReportADatabaseErrorAsNotFound(t *testing.T) { + f := newBranchFixture(t, "draft") + boom := errors.New("connection reset") + svc := NewService(f.steps, fakeChecker{err: boom}, &fakeVariantStore{}, f.branches) + ctx := context.Background() + _, gerr := svc.Graph(ctx, f.ws, f.campaign) + _, serr := svc.SetBranch(ctx, f.ws, f.campaign, BranchInput{StepID: f.step[0], Condition: "always"}) + derr := svc.DeleteBranch(ctx, f.ws, f.campaign, f.step[0], BranchPrecondition{}) + for name, err := range map[string]error{"Graph": gerr, "SetBranch": serr, "DeleteBranch": derr} { + if !errors.Is(err, boom) || errors.Is(err, ErrCampaignNotFound) { + t.Errorf("%s: got %v, want the wrapped database error", name, err) + } + } +} + +// A reply condition may name a label only if replies with it leave the +// enrollment running: a stopping label (every builtin human one) ends the +// sequence before any branch is consulted, so the branch could never fire. +func TestSetBranchReplyLabelMustNotStopTheSequence(t *testing.T) { + ctx := context.Background() + f := newBranchFixture(t, "running") + _, err := f.svc.SetBranch(ctx, f.ws, f.campaign, BranchInput{ + StepID: f.step[0], Condition: "replied", WithinDays: ptr(int32(2)), ReplyLabelKey: ptr("positive"), YesStepID: &f.step[2], + }) + if seqgraph.CodeOf(err) != seqgraph.CodeLabelStopsSequence { + t.Fatalf("stopping label: code %q (%v), want %q", seqgraph.CodeOf(err), err, seqgraph.CodeLabelStopsSequence) + } + if _, err := f.svc.SetBranch(ctx, f.ws, f.campaign, BranchInput{ + StepID: f.step[0], Condition: "not_replied", WithinDays: ptr(int32(2)), ReplyLabelKey: ptr("soft_yes"), YesStepID: &f.step[2], + }); err != nil { + t.Fatalf("a non-stopping label is routable: %v", err) + } +} + +// Open and click conditions need somewhere for an open or click to be recorded: +// tracking on for the campaign, and an HTML body on the step and every variant. +// Reply conditions need neither. +func TestSetBranchOpenClickNeedTracking(t *testing.T) { + ctx := context.Background() + openIn := func(f branchFixture, cond string) BranchInput { + return BranchInput{StepID: f.step[0], Condition: cond, WithinDays: ptr(int32(1)), YesStepID: &f.step[1]} + } + + f := newBranchFixture(t, "running") + f.branches.trackingOff = true + for _, cond := range []string{"opened", "clicked", "not_opened"} { + if _, err := f.svc.SetBranch(ctx, f.ws, f.campaign, openIn(f, cond)); seqgraph.CodeOf(err) != seqgraph.CodeTrackingRequired { + t.Errorf("%s with tracking off: %v", cond, err) + } + } + if _, err := f.svc.SetBranch(ctx, f.ws, f.campaign, openIn(f, "replied")); err != nil { + t.Errorf("replied does not need tracking: %v", err) + } + + f = newBranchFixture(t, "running") + f.steps.steps[0].BodyHtml = "" + if _, err := f.svc.SetBranch(ctx, f.ws, f.campaign, openIn(f, "opened")); seqgraph.CodeOf(err) != seqgraph.CodeTrackingRequired { + t.Errorf("text-only step: %v", err) + } + + f = newBranchFixture(t, "running") + f.variants.variants = []Variant{{ID: uuid.New(), StepID: f.step[0], BodyHTML: "b
"}, {ID: uuid.New(), StepID: f.step[0]}} + if _, err := f.svc.SetBranch(ctx, f.ws, f.campaign, openIn(f, "clicked")); seqgraph.CodeOf(err) != seqgraph.CodeTrackingRequired { + t.Errorf("a text-only variant: %v", err) + } + if f.branches.upserts != 0 { + t.Fatal("a refused open/click branch must not be written") + } +} + +// A step of a different campaign (or tenant — the workspace-pinned Get simply +// does not find it) is not routable through this campaign's URL. +func TestSetBranchRejectsStepFromAnotherCampaign(t *testing.T) { + f := newBranchFixture(t, "draft") + _, err := f.svc.SetBranch(context.Background(), f.ws, f.campaign, BranchInput{StepID: uuid.New(), Condition: "always"}) + if !errors.Is(err, ErrNotFound) { + t.Fatalf("want ErrNotFound, got %v", err) + } + if f.branches.upserts != 0 { + t.Fatal("nothing may be written for a foreign step") + } +} + +func TestSetBranchRejectsTargetOutsideCampaign(t *testing.T) { + f := newBranchFixture(t, "draft") + foreign := uuid.New() + _, err := f.svc.SetBranch(context.Background(), f.ws, f.campaign, BranchInput{ + StepID: f.step[0], Condition: "replied", WithinDays: ptr(int32(2)), NoStepID: &foreign, + }) + var te *seqgraph.TargetError + if !errors.As(err, &te) || te.Target != foreign { + t.Fatalf("want TargetError naming %v, got %v", foreign, err) + } + if f.branches.upserts != 0 { + t.Fatal("an unknown target must be refused before any write") + } +} + +func TestSetBranchRejectsCycle(t *testing.T) { + f := newBranchFixture(t, "running") + // 3 -> 1 closes 1 -> 2 -> 3 (implicit fall-through). + _, err := f.svc.SetBranch(context.Background(), f.ws, f.campaign, BranchInput{ + StepID: f.step[2], Condition: "always", YesStepID: &f.step[0], + }) + var cyc *seqgraph.CycleError + if !errors.As(err, &cyc) { + t.Fatalf("want CycleError, got %v", err) + } + if len(f.branches.branches) != 0 { + t.Fatal("a refused edit must leave the graph as it was") + } +} + +func TestSetBranchShapeErrors(t *testing.T) { + cases := map[string]struct { + in func(f branchFixture) BranchInput + code string + }{ + "unknown condition": {func(f branchFixture) BranchInput { + return BranchInput{StepID: f.step[0], Condition: "bounced", WithinDays: ptr(int32(1))} + }, seqgraph.CodeInvalidCondition}, + "missing window": {func(f branchFixture) BranchInput { + return BranchInput{StepID: f.step[0], Condition: "clicked"} + }, seqgraph.CodeInvalidWithinDays}, + "explicit zero window on always": {func(f branchFixture) BranchInput { + return BranchInput{StepID: f.step[0], Condition: "always", WithinDays: ptr(int32(0))} + }, seqgraph.CodeInvalidWithinDays}, + "no exit on always": {func(f branchFixture) BranchInput { + return BranchInput{StepID: f.step[0], Condition: "always", NoStepID: &f.step[1]} + }, seqgraph.CodeNoExitNotAllowed}, + "label on an open": {func(f branchFixture) BranchInput { + return BranchInput{StepID: f.step[0], Condition: "opened", WithinDays: ptr(int32(1)), ReplyLabelKey: ptr("positive")} + }, seqgraph.CodeLabelNotAllowed}, + "label that stops the sequence": {func(f branchFixture) BranchInput { + return BranchInput{StepID: f.step[0], Condition: "replied", WithinDays: ptr(int32(1)), ReplyLabelKey: ptr("positive")} + }, seqgraph.CodeLabelStopsSequence}, + "label that does not exist": {func(f branchFixture) BranchInput { + return BranchInput{StepID: f.step[0], Condition: "replied", WithinDays: ptr(int32(1)), ReplyLabelKey: ptr("ghost")} + }, seqgraph.CodeLabelNotAllowed}, + "self loop": {func(f branchFixture) BranchInput { + return BranchInput{StepID: f.step[1], Condition: "not_opened", WithinDays: ptr(int32(1)), YesStepID: &f.step[1]} + }, seqgraph.CodeCycle}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + f := newBranchFixture(t, "draft") + _, err := f.svc.SetBranch(context.Background(), f.ws, f.campaign, tc.in(f)) + if got := seqgraph.CodeOf(err); got != tc.code { + t.Fatalf("code = %q (%v), want %q", got, err, tc.code) + } + if f.branches.upserts != 0 { + t.Fatal("a malformed branch must not be written") + } + }) + } +} + +// Removing a router restores the step's fall-through, which is itself an edge: +// here step 2 routes back to step 1, which is only acyclic because step 1 ends +// explicitly. Deleting step 1's router would restore 1 -> 2 and close the loop. +func TestDeleteBranchRefusedWhenFallThroughClosesLoop(t *testing.T) { + f := newBranchFixture(t, "running") + ctx := context.Background() + if _, err := f.svc.SetBranch(ctx, f.ws, f.campaign, BranchInput{StepID: f.step[0], Condition: "always"}); err != nil { + t.Fatalf("1 ends: %v", err) + } + if _, err := f.svc.SetBranch(ctx, f.ws, f.campaign, BranchInput{ + StepID: f.step[1], Condition: "always", YesStepID: &f.step[0], + }); err != nil { + t.Fatalf("2 -> 1 while 1 ends is acyclic: %v", err) + } + err := f.svc.DeleteBranch(ctx, f.ws, f.campaign, f.step[0], BranchPrecondition{}) + if seqgraph.CodeOf(err) != seqgraph.CodeCycle { + t.Fatalf("want a cycle refusal, got %v", err) + } + if _, still := f.branches.branches[f.step[0]]; !still { + t.Fatal("the refused delete must be rolled back") + } +} + +func TestDeleteBranchIsIdempotent(t *testing.T) { + f := newBranchFixture(t, "running") + if err := f.svc.DeleteBranch(context.Background(), f.ws, f.campaign, f.step[0], BranchPrecondition{}); err != nil { + t.Fatalf("deleting a router that does not exist is a no-op: %v", err) + } +} + +func TestGraphReturnsStepsAndBranches(t *testing.T) { + f := newBranchFixture(t, "running") + f.branches.branches = map[uuid.UUID]gen.SequenceStepBranch{ + f.step[0]: {StepID: f.step[0], Condition: "always", YesStepID: pgtype.UUID{Bytes: f.step[2], Valid: true}}, + } + g, err := f.svc.Graph(context.Background(), f.ws, f.campaign) + if err != nil { + t.Fatalf("Graph: %v", err) + } + if len(g.Steps) != 3 || len(g.Branches) != 1 { + t.Fatalf("graph = %+v", g) + } + resp := toGraphResponse(f.campaign, g) + if resp.EntryStepID == nil || *resp.EntryStepID != f.step[0].String() { + t.Fatalf("entry = %v", resp.EntryStepID) + } + if resp.Nodes[0].Branch == nil || resp.Nodes[0].Branch.YesStepID == nil || *resp.Nodes[0].Branch.YesStepID != f.step[2].String() { + t.Fatalf("node 1 branch = %+v", resp.Nodes[0].Branch) + } + // The fall-through is served whether or not a branch overrides it. + if resp.Nodes[0].DefaultNextStepID == nil || *resp.Nodes[0].DefaultNextStepID != f.step[1].String() { + t.Fatalf("node 1 default next = %v", resp.Nodes[0].DefaultNextStepID) + } + if resp.Nodes[2].DefaultNextStepID != nil { + t.Fatalf("the last step has no fall-through, got %v", *resp.Nodes[2].DefaultNextStepID) + } +} + +func TestGraphRejectsMissingCampaign(t *testing.T) { + svc := NewService(&fakeStore{}, fakeChecker{err: pgx.ErrNoRows}, &fakeVariantStore{}, &fakeBranchStore{}) + if _, err := svc.Graph(context.Background(), uuid.New(), uuid.New()); !errors.Is(err, ErrCampaignNotFound) { + t.Fatalf("want ErrCampaignNotFound, got %v", err) + } +} + +// A linear campaign (no routers) validates under every structural edit exactly +// as before branching existed: the check a delete/reorder now commits under +// never refuses a graph with no branches. +func TestCheckGraphAcceptsEveryLinearCampaign(t *testing.T) { + var steps []gen.SequenceStep + for i := range 5 { + steps = append(steps, gen.SequenceStep{ID: uuid.New(), StepOrder: int32(i*2 + 1)}) // gaps too + } + if err := checkGraph(steps, nil); err != nil { + t.Fatalf("linear campaign refused: %v", err) + } + if err := checkGraph(nil, nil); err != nil { + t.Fatalf("empty campaign refused: %v", err) + } +} + +// The precondition is enforced by the store, in the write; the service's job is +// to hand it over untouched on both writes, and to refuse a malformed branch +// before any precondition is consulted. +func TestBranchPreconditionReachesTheStore(t *testing.T) { + ctx := context.Background() + f := newBranchFixture(t, "running") + at := time.Date(2026, 9, 23, 16, 10, 44, 123456000, time.UTC) + + if _, err := f.svc.SetBranch(ctx, f.ws, f.campaign, BranchInput{ + StepID: f.step[0], Condition: "always", Expect: ExpectNoBranch(), + }); err != nil { + t.Fatalf("SetBranch: %v", err) + } + if err := f.svc.DeleteBranch(ctx, f.ws, f.campaign, f.step[0], ExpectBranchAt(at)); err != nil { + t.Fatalf("DeleteBranch: %v", err) + } + want := []BranchPrecondition{ExpectNoBranch(), ExpectBranchAt(at)} + if !slices.Equal(f.branches.expects, want) { + t.Fatalf("store saw %+v, want %+v", f.branches.expects, want) + } + + f = newBranchFixture(t, "running") + if _, err := f.svc.SetBranch(ctx, f.ws, f.campaign, BranchInput{ + StepID: f.step[0], Condition: "bounced", Expect: ExpectBranchAt(at), + }); seqgraph.CodeOf(err) != seqgraph.CodeInvalidCondition { + t.Fatalf("a malformed branch is refused as malformed, got %v", err) + } + if len(f.branches.expects) != 0 { + t.Fatal("a malformed branch must not reach the store") + } +} diff --git a/internal/app/sequencestep/branchhandler.go b/internal/app/sequencestep/branchhandler.go new file mode 100644 index 00000000..146fcaf4 --- /dev/null +++ b/internal/app/sequencestep/branchhandler.go @@ -0,0 +1,332 @@ +package sequencestep + +import ( + "encoding/json" + "errors" + "fmt" + "net/http" + "time" + + "github.com/go-chi/chi/v5" + "github.com/google/uuid" + + "github.com/inroad/inroad/internal/app/auth" + "github.com/inroad/inroad/internal/platform/db/gen" + "github.com/inroad/inroad/internal/platform/httpx" + "github.com/inroad/inroad/internal/platform/seqgraph" +) + +// branchRequest is the PUT body (StepBranchRequest in api/openapi.yaml). A null +// or absent exit ends the path. +type branchRequest struct { + Condition string `json:"condition"` + WithinDays *int32 `json:"within_days"` + ReplyLabelKey *string `json:"reply_label_key"` + YesStepID *uuid.UUID `json:"yes_step_id"` + NoStepID *uuid.UUID `json:"no_step_id"` + // ExpectedUpdatedAt is the optimistic-concurrency precondition, and its + // three states mean three different things: absent = none, null = "expect no + // branch", a timestamp = "expect the branch at this updated_at". + ExpectedUpdatedAt presentField `json:"expected_updated_at"` +} + +// presentField records whether a JSON field was sent at all, and its raw value +// if so. encoding/json calls UnmarshalJSON for an explicit null on a non-pointer +// field, so this is the one way to tell an absent field from a null one. +type presentField struct { + set bool + raw json.RawMessage +} + +func (f *presentField) UnmarshalJSON(b []byte) error { + f.set = true + f.raw = append(f.raw[:0], b...) + return nil +} + +// expectedUpdatedAtParam is the DELETE precondition's query parameter, named +// like the PUT body field it mirrors. +const expectedUpdatedAtParam = "expected_updated_at" + +// bodyPrecondition reads expected_updated_at from a PUT body. +func bodyPrecondition(f presentField) (BranchPrecondition, error) { + if !f.set { + return BranchPrecondition{}, nil + } + if string(f.raw) == "null" { + return ExpectNoBranch(), nil + } + var raw string + if err := json.Unmarshal(f.raw, &raw); err != nil { + return BranchPrecondition{}, errors.New("expected_updated_at must be a timestamp string or null") + } + at, err := parseExpectedUpdatedAt(raw) + if err != nil { + return BranchPrecondition{}, err + } + return ExpectBranchAt(at), nil +} + +// queryPrecondition reads expected_updated_at from a DELETE's query string. +// There is no "expect no branch" form: deleting nothing needs no guard. +func queryPrecondition(r *http.Request) (BranchPrecondition, error) { + q := r.URL.Query() + if !q.Has(expectedUpdatedAtParam) { + return BranchPrecondition{}, nil + } + at, err := parseExpectedUpdatedAt(q.Get(expectedUpdatedAtParam)) + if err != nil { + return BranchPrecondition{}, err + } + return ExpectBranchAt(at), nil +} + +// parseExpectedUpdatedAt accepts an updated_at exactly as toBranchResponse +// serializes it. The column is microsecond-precision, so a value with finer +// digits cannot be one the API returned; it is refused rather than truncated, +// because truncating would let a value that differs from the stored one match. +func parseExpectedUpdatedAt(raw string) (time.Time, error) { + at, err := time.Parse(time.RFC3339Nano, raw) + if err != nil { + return time.Time{}, fmt.Errorf("expected_updated_at must be an RFC3339 timestamp: %w", err) + } + if at.Nanosecond()%int(time.Microsecond) != 0 { + return time.Time{}, errors.New("expected_updated_at has sub-microsecond digits; send updated_at back exactly as it was returned") + } + return at, nil +} + +// branchResponse is the wire shape of one router (StepBranch). +type branchResponse struct { + StepID string `json:"step_id"` + Condition string `json:"condition"` + WithinDays *int32 `json:"within_days"` + ReplyLabelKey *string `json:"reply_label_key"` + YesStepID *string `json:"yes_step_id"` + NoStepID *string `json:"no_step_id"` + UpdatedAt string `json:"updated_at"` +} + +// graphNodeResponse is one step as a graph node (CampaignGraphNode). +type graphNodeResponse struct { + StepID string `json:"step_id"` + StepOrder int32 `json:"step_order"` + // DefaultNextStepID is where the step goes when it has NO branch: the next + // step by step_order, or null for the last step. Served rather than left for + // the client to derive so the canvas draws the same fall-through edge the + // send path follows. + DefaultNextStepID *string `json:"default_next_step_id"` + Branch *branchResponse `json:"branch"` +} + +// graphResponse is GET /campaigns/{id}/graph (CampaignGraph). +type graphResponse struct { + CampaignID string `json:"campaign_id"` + EntryStepID *string `json:"entry_step_id"` + Nodes []graphNodeResponse `json:"nodes"` +} + +// graphErrorResponse is every graph validation failure (BranchValidationError): +// the usual human message plus a stable code, and — for a loop — the steps on +// it, so the canvas can highlight the offending edges. +type graphErrorResponse struct { + Error string `json:"error"` + Code string `json:"code"` + StepIDs []string `json:"step_ids,omitempty"` +} + +// CodeBranchChanged is the BranchValidationError code of a refused +// expected_updated_at precondition (409). +const CodeBranchChanged = "branch_changed" + +// branchChangedResponse is the 409 for a refused precondition: the same error +// shape plus the branch as it is now. current is always present — null means +// the step now has no branch — so it is not omitempty. +type branchChangedResponse struct { + Error string `json:"error"` + Code string `json:"code"` + Current *branchResponse `json:"current"` +} + +// Graph handles GET /campaigns/{id}/graph. +func (h *Handler) Graph(w http.ResponseWriter, r *http.Request) { + ws, ok := auth.WorkspaceID(w, r) + if !ok { + return + } + campaignID, err := uuid.Parse(chi.URLParam(r, "id")) + if err != nil { + httpx.Error(w, http.StatusBadRequest, "bad campaign id") + return + } + g, err := h.svc.Graph(r.Context(), ws, campaignID) + if err != nil { + writeGraphError(w, err, "could not load the sequence graph") + return + } + httpx.JSON(w, http.StatusOK, toGraphResponse(campaignID, g)) +} + +// SetBranch handles PUT /campaigns/{id}/steps/{stepId}/branch. +func (h *Handler) SetBranch(w http.ResponseWriter, r *http.Request) { + ws, campaignID, stepID, ok := campaignAndStepIDs(w, r) + if !ok { + return + } + var req branchRequest + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + httpx.Error(w, http.StatusBadRequest, "invalid json") + return + } + expect, err := bodyPrecondition(req.ExpectedUpdatedAt) + if err != nil { + httpx.Error(w, http.StatusBadRequest, err.Error()) + return + } + // An empty label is "no label", not a label whose key is "". + if req.ReplyLabelKey != nil && *req.ReplyLabelKey == "" { + req.ReplyLabelKey = nil + } + b, err := h.svc.SetBranch(r.Context(), ws, campaignID, BranchInput{ + StepID: stepID, Condition: req.Condition, WithinDays: req.WithinDays, + ReplyLabelKey: req.ReplyLabelKey, YesStepID: req.YesStepID, NoStepID: req.NoStepID, + Expect: expect, + }) + if err != nil { + writeGraphError(w, err, "could not save branch") + return + } + httpx.JSON(w, http.StatusOK, toBranchResponse(b)) +} + +// DeleteBranch handles DELETE /campaigns/{id}/steps/{stepId}/branch. +func (h *Handler) DeleteBranch(w http.ResponseWriter, r *http.Request) { + ws, campaignID, stepID, ok := campaignAndStepIDs(w, r) + if !ok { + return + } + expect, err := queryPrecondition(r) + if err != nil { + httpx.Error(w, http.StatusBadRequest, err.Error()) + return + } + if err := h.svc.DeleteBranch(r.Context(), ws, campaignID, stepID, expect); err != nil { + writeGraphError(w, err, "could not remove branch") + return + } + w.WriteHeader(http.StatusNoContent) +} + +// campaignAndStepIDs reads the pinned workspace and the two path ids, writing +// the error response itself when any is missing or malformed. +func campaignAndStepIDs(w http.ResponseWriter, r *http.Request) (ws, campaignID, stepID uuid.UUID, ok bool) { + ws, ok = auth.WorkspaceID(w, r) + if !ok { + return uuid.Nil, uuid.Nil, uuid.Nil, false + } + campaignID, err := uuid.Parse(chi.URLParam(r, "id")) + if err != nil { + httpx.Error(w, http.StatusBadRequest, "bad campaign id") + return uuid.Nil, uuid.Nil, uuid.Nil, false + } + stepID, err = uuid.Parse(chi.URLParam(r, "stepId")) + if err != nil { + httpx.Error(w, http.StatusBadRequest, "bad step id") + return uuid.Nil, uuid.Nil, uuid.Nil, false + } + return ws, campaignID, stepID, true +} + +// writeGraphError maps every error a graph read or write can return. +// +// A malformed branch (seqgraph shape codes) is 400, like any other invalid +// body. A well-formed edit the GRAPH refuses — an exit to a step outside the +// campaign, or a loop — is 422: the request was understood and is the wrong +// thing to commit. Both carry `code`. A refused expected_updated_at is 409 +// (code branch_changed) with the current branch. +func writeGraphError(w http.ResponseWriter, err error, fallback string) { + var cycle *seqgraph.CycleError + var changed *BranchChangedError + switch code := seqgraph.CodeOf(err); { + case errors.As(err, &changed): + resp := branchChangedResponse{Error: changed.Error(), Code: CodeBranchChanged} + if changed.Current != nil { + current := toBranchResponse(*changed.Current) + resp.Current = ¤t + } + httpx.JSON(w, http.StatusConflict, resp) + case errors.Is(err, ErrCampaignNotFound): + httpx.Error(w, http.StatusNotFound, "campaign not found") + case errors.Is(err, ErrNotFound), errors.Is(err, ErrBranchConflict): + httpx.Error(w, http.StatusNotFound, "step not found") + case errors.Is(err, ErrCampaignNotDraft): + httpx.Error(w, http.StatusConflict, "steps can only be changed structurally while the campaign is draft") + case errors.Is(err, ErrTargetGone): + httpx.JSON(w, http.StatusUnprocessableEntity, graphErrorResponse{Error: err.Error(), Code: seqgraph.CodeUnknownTarget}) + case errors.As(err, &cycle): + httpx.JSON(w, http.StatusUnprocessableEntity, graphErrorResponse{ + Error: err.Error(), Code: code, StepIDs: uuidStrings(cycle.StepIDs), + }) + case code == seqgraph.CodeUnknownTarget || code == seqgraph.CodeUnknownStep: + httpx.JSON(w, http.StatusUnprocessableEntity, graphErrorResponse{Error: err.Error(), Code: code}) + case code != "": + httpx.JSON(w, http.StatusBadRequest, graphErrorResponse{Error: err.Error(), Code: code}) + default: + httpx.Error(w, http.StatusInternalServerError, fallback) + } +} + +func toGraphResponse(campaignID uuid.UUID, g Graph) graphResponse { + model := BuildGraph(g.Steps, g.Branches) + byStep := make(map[uuid.UUID]gen.SequenceStepBranch, len(g.Branches)) + for _, b := range g.Branches { + byStep[b.StepID] = b + } + out := graphResponse{CampaignID: campaignID.String(), Nodes: make([]graphNodeResponse, 0, len(g.Steps))} + if entry, ok := model.Entry(); ok { + out.EntryStepID = uuidString(entry.ID) + } + for _, st := range g.Steps { + node := graphNodeResponse{StepID: st.ID.String(), StepOrder: st.StepOrder} + if next, ok := model.DefaultNext(st.ID); ok { + node.DefaultNextStepID = uuidString(next.ID) + } + if b, ok := byStep[st.ID]; ok { + resp := toBranchResponse(b) + node.Branch = &resp + } + out.Nodes = append(out.Nodes, node) + } + return out +} + +func toBranchResponse(b gen.SequenceStepBranch) branchResponse { + out := branchResponse{ + StepID: b.StepID.String(), Condition: b.Condition, + WithinDays: b.WithinDays, ReplyLabelKey: b.ReplyLabelKey, + // Full precision: updated_at is the concurrency token a client sends + // back as expected_updated_at, and a value rounded to the second would + // never match the stored microseconds. + UpdatedAt: b.UpdatedAt.Time.UTC().Format(time.RFC3339Nano), + } + if b.YesStepID.Valid { + out.YesStepID = uuidString(b.YesStepID.Bytes) + } + if b.NoStepID.Valid { + out.NoStepID = uuidString(b.NoStepID.Bytes) + } + return out +} + +func uuidString(id uuid.UUID) *string { + s := id.String() + return &s +} + +func uuidStrings(ids []uuid.UUID) []string { + out := make([]string, len(ids)) + for i, id := range ids { + out[i] = id.String() + } + return out +} diff --git a/internal/app/sequencestep/branchhandler_test.go b/internal/app/sequencestep/branchhandler_test.go new file mode 100644 index 00000000..80d649e8 --- /dev/null +++ b/internal/app/sequencestep/branchhandler_test.go @@ -0,0 +1,183 @@ +package sequencestep + +import ( + "encoding/json" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + "time" + + "github.com/google/uuid" + "github.com/jackc/pgx/v5/pgtype" + + "github.com/inroad/inroad/internal/platform/db/gen" + "github.com/inroad/inroad/internal/platform/seqgraph" +) + +// The status + body shape the canvas keys its error handling on. +func TestWriteGraphErrorContract(t *testing.T) { + loop := []uuid.UUID{uuid.New(), uuid.New()} + cases := []struct { + name string + err error + status int + code string + stepIDs int + hasError bool + }{ + {"shape", &seqgraph.ShapeError{Code: seqgraph.CodeInvalidWithinDays, Msg: "x"}, http.StatusBadRequest, seqgraph.CodeInvalidWithinDays, 0, true}, + {"cycle", &seqgraph.CycleError{StepIDs: loop}, http.StatusUnprocessableEntity, seqgraph.CodeCycle, 2, true}, + {"wrapped cycle", fmt.Errorf("commit: %w", &seqgraph.CycleError{StepIDs: loop}), http.StatusUnprocessableEntity, seqgraph.CodeCycle, 2, true}, + {"unknown target", &seqgraph.TargetError{StepID: uuid.New(), Target: uuid.New()}, http.StatusUnprocessableEntity, seqgraph.CodeUnknownTarget, 0, true}, + {"target gone", ErrTargetGone, http.StatusUnprocessableEntity, seqgraph.CodeUnknownTarget, 0, true}, + {"campaign missing", ErrCampaignNotFound, http.StatusNotFound, "", 0, true}, + {"step missing", ErrNotFound, http.StatusNotFound, "", 0, true}, + {"not draft", ErrCampaignNotDraft, http.StatusConflict, "", 0, true}, + {"unexpected", errors.New("db down"), http.StatusInternalServerError, "", 0, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + rec := httptest.NewRecorder() + writeGraphError(rec, tc.err, "fallback") + if rec.Code != tc.status { + t.Fatalf("status = %d, want %d", rec.Code, tc.status) + } + var body graphErrorResponse + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("body %q: %v", rec.Body.String(), err) + } + if body.Code != tc.code || len(body.StepIDs) != tc.stepIDs || (tc.hasError && body.Error == "") { + t.Fatalf("body = %+v", body) + } + }) + } +} + +// expected_updated_at has three states in a PUT body, and each means something +// different: confusing absent with null would turn every legacy write into a +// create-only write, and confusing null with absent would drop the guard. +func TestBodyPreconditionDistinguishesAbsentNullAndTimestamp(t *testing.T) { + at := time.Date(2026, 9, 23, 16, 10, 44, 120000000, time.UTC) + cases := []struct { + body string + want BranchPrecondition + }{ + {`{"condition":"always"}`, BranchPrecondition{}}, + {`{"condition":"always","expected_updated_at":null}`, ExpectNoBranch()}, + {`{"condition":"always","expected_updated_at":"2026-09-23T16:10:44.12Z"}`, ExpectBranchAt(at)}, + } + for _, tc := range cases { + var req branchRequest + if err := json.Unmarshal([]byte(tc.body), &req); err != nil { + t.Fatalf("%s: %v", tc.body, err) + } + got, err := bodyPrecondition(req.ExpectedUpdatedAt) + if err != nil { + t.Fatalf("%s: %v", tc.body, err) + } + if got.kind != tc.want.kind || !got.updatedAt.Equal(tc.want.updatedAt) { + t.Fatalf("%s: got %+v, want %+v", tc.body, got, tc.want) + } + } +} + +func TestBodyPreconditionRefusesWhatCannotBeAStoredValue(t *testing.T) { + for _, raw := range []string{ + `12345`, + `"yesterday"`, + `""`, + // Nanoseconds: the column stores microseconds, so this is not a value the + // API returned, and truncating it would let a DIFFERENT value match. + `"2026-09-23T16:10:44.123456789Z"`, + } { + var req branchRequest + if err := json.Unmarshal([]byte(`{"expected_updated_at":`+raw+`}`), &req); err != nil { + t.Fatalf("%s: decode: %v", raw, err) + } + if _, err := bodyPrecondition(req.ExpectedUpdatedAt); err == nil { + t.Errorf("%s: accepted", raw) + } + } +} + +func TestQueryPrecondition(t *testing.T) { + at := time.Date(2026, 9, 23, 16, 10, 44, 123456000, time.UTC) + req := func(query string) *http.Request { + return httptest.NewRequestWithContext(t.Context(), http.MethodDelete, "/campaigns/x/steps/y/branch"+query, http.NoBody) + } + if got, err := queryPrecondition(req("")); err != nil || got != (BranchPrecondition{}) { + t.Fatalf("absent: %+v, %v", got, err) + } + got, err := queryPrecondition(req("?expected_updated_at=" + url.QueryEscape("2026-09-23T16:10:44.123456Z"))) + if err != nil || got.kind != preconditionAt || !got.updatedAt.Equal(at) { + t.Fatalf("timestamp: %+v, %v", got, err) + } + // An offset's '+' must be escaped; unescaped it decodes to a space and the + // value is refused rather than misread. + for _, q := range []string{"?expected_updated_at=", "?expected_updated_at=nope", "?expected_updated_at=2026-09-23T18:10:44+02:00"} { + if _, err := queryPrecondition(req(q)); err == nil { + t.Errorf("%q accepted", q) + } + } +} + +// updated_at is the token a client echoes back, so what the API serializes must +// parse to exactly the stored instant — including microseconds that end in +// zeros (RFC3339Nano trims them) and a whole second (no fraction at all). +func TestUpdatedAtRoundTripsExactly(t *testing.T) { + for _, nanos := range []int{0, 100000000, 123456000, 1000, 999999000} { + stored := time.Date(2026, 9, 23, 16, 10, 44, nanos, time.FixedZone("x", 3600)) + resp := toBranchResponse(gen.SequenceStepBranch{ + StepID: uuid.New(), Condition: "always", UpdatedAt: pgtype.Timestamptz{Time: stored, Valid: true}, + }) + back, err := parseExpectedUpdatedAt(resp.UpdatedAt) + if err != nil { + t.Fatalf("%q does not parse back: %v", resp.UpdatedAt, err) + } + if !back.Equal(stored) { + t.Fatalf("%q round-trips to %v, want %v", resp.UpdatedAt, back, stored) + } + } +} + +// A refused precondition is 409 with the branch that won; when the step has no +// branch any more, current is an explicit null, not a missing key. +func TestWriteGraphErrorBranchChanged(t *testing.T) { + step := uuid.New() + row := gen.SequenceStepBranch{StepID: step, Condition: "always", UpdatedAt: pgtype.Timestamptz{Time: time.Now(), Valid: true}} + cases := []struct { + name string + err error + current bool + }{ + {"replaced", &BranchChangedError{Current: &row}, true}, + {"removed", fmt.Errorf("tx: %w", &BranchChangedError{}), false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + rec := httptest.NewRecorder() + writeGraphError(rec, tc.err, "fallback") + if rec.Code != http.StatusConflict { + t.Fatalf("status = %d, want 409", rec.Code) + } + var body map[string]json.RawMessage + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatal(err) + } + if string(body["code"]) != `"branch_changed"` || len(body["error"]) < 3 { + t.Fatalf("body = %s", rec.Body.String()) + } + raw, present := body["current"] + if !present { + t.Fatalf("current must always be present: %s", rec.Body.String()) + } + if tc.current != (string(raw) != "null") || (tc.current && !strings.Contains(string(raw), step.String())) { + t.Fatalf("current = %s", raw) + } + }) + } +} diff --git a/internal/app/sequencestep/branchprecondition_integration_test.go b/internal/app/sequencestep/branchprecondition_integration_test.go new file mode 100644 index 00000000..554a1e01 --- /dev/null +++ b/internal/app/sequencestep/branchprecondition_integration_test.go @@ -0,0 +1,311 @@ +//go:build integration + +package sequencestep + +import ( + "context" + "errors" + "sync" + "testing" + "time" + + "github.com/google/uuid" + + "github.com/inroad/inroad/internal/platform/db/gen" +) + +// wireToken is updated_at as a client holds it: serialized by the handler and +// parsed back the way the handler parses expected_updated_at. Every test goes +// through it, so a precision loss anywhere on the wire fails here rather than +// only in a browser. +func wireToken(t *testing.T, b gen.SequenceStepBranch) BranchPrecondition { + t.Helper() + at, err := parseExpectedUpdatedAt(toBranchResponse(b).UpdatedAt) + if err != nil { + t.Fatalf("updated_at %q does not parse back: %v", toBranchResponse(b).UpdatedAt, err) + } + return ExpectBranchAt(at) +} + +func changedErr(t *testing.T, err error) *BranchChangedError { + t.Helper() + var changed *BranchChangedError + if !errors.As(err, &changed) { + t.Fatalf("want *BranchChangedError, got %v", err) + } + return changed +} + +func (b branchIT) branchOf(t *testing.T, step uuid.UUID) *gen.SequenceStepBranch { + t.Helper() + g, err := b.svc.Graph(context.Background(), b.ws, b.campaign) + if err != nil { + t.Fatal(err) + } + for _, br := range g.Branches { + if br.StepID == step { + return &br + } + } + return nil +} + +func TestBranchPreconditionMatchingApplies(t *testing.T) { + b, done := newBranchIT(t, "Precondition match") + defer done() + ctx := context.Background() + + created, err := b.svc.SetBranch(ctx, b.ws, b.campaign, BranchInput{ + StepID: b.steps[0], Condition: "always", YesStepID: &b.steps[2], Expect: ExpectNoBranch(), + }) + if err != nil { + t.Fatalf("create-if-absent on a step with no branch: %v", err) + } + replaced, err := b.svc.SetBranch(ctx, b.ws, b.campaign, BranchInput{ + StepID: b.steps[0], Condition: "always", Expect: wireToken(t, created), + }) + if err != nil { + t.Fatalf("replace at the current version: %v", err) + } + if replaced.YesStepID.Valid || !replaced.UpdatedAt.Time.After(created.UpdatedAt.Time) { + t.Fatalf("replaced = %+v (was %v)", replaced, created.UpdatedAt.Time) + } + if err := b.svc.DeleteBranch(ctx, b.ws, b.campaign, b.steps[0], wireToken(t, replaced)); err != nil { + t.Fatalf("delete at the current version: %v", err) + } + if got := b.branchOf(t, b.steps[0]); got != nil { + t.Fatalf("branch survived its delete: %+v", got) + } +} + +// The core promise: a write made against a branch someone else has since +// changed is refused, writes nothing, and says what the branch is now. +func TestBranchPreconditionStaleIsRefusedWithCurrent(t *testing.T) { + b, done := newBranchIT(t, "Precondition stale") + defer done() + ctx := context.Background() + + v1, err := b.svc.SetBranch(ctx, b.ws, b.campaign, BranchInput{StepID: b.steps[0], Condition: "always", YesStepID: &b.steps[2]}) + if err != nil { + t.Fatal(err) + } + stale := wireToken(t, v1) + // Another tab saves first. + v2, err := b.svc.SetBranch(ctx, b.ws, b.campaign, BranchInput{StepID: b.steps[0], Condition: "always", Expect: stale}) + if err != nil { + t.Fatal(err) + } + + _, err = b.svc.SetBranch(ctx, b.ws, b.campaign, BranchInput{ + StepID: b.steps[0], Condition: "always", YesStepID: &b.steps[1], Expect: stale, + }) + changed := changedErr(t, err) + if changed.Current == nil || changed.Current.YesStepID.Valid || !changed.Current.UpdatedAt.Time.Equal(v2.UpdatedAt.Time) { + t.Fatalf("current = %+v, want the second tab's save %+v", changed.Current, v2) + } + if got := b.branchOf(t, b.steps[0]); got == nil || got.YesStepID.Valid || !got.UpdatedAt.Time.Equal(v2.UpdatedAt.Time) { + t.Fatalf("a refused write changed the branch: %+v", got) + } + + // "Expect no branch" against a step that now has one is the same refusal. + _, err = b.svc.SetBranch(ctx, b.ws, b.campaign, BranchInput{StepID: b.steps[0], Condition: "always", Expect: ExpectNoBranch()}) + if changed := changedErr(t, err); changed.Current == nil || changed.Current.StepID != b.steps[0] { + t.Fatalf("current = %+v", changed.Current) + } + + // Expecting a version of a branch that is gone reports current = nil. + if err := b.svc.DeleteBranch(ctx, b.ws, b.campaign, b.steps[0], BranchPrecondition{}); err != nil { + t.Fatal(err) + } + _, err = b.svc.SetBranch(ctx, b.ws, b.campaign, BranchInput{StepID: b.steps[0], Condition: "always", Expect: wireToken(t, v2)}) + if changed := changedErr(t, err); changed.Current != nil { + t.Fatalf("current = %+v, want nil for a removed branch", changed.Current) + } + if got := b.branchOf(t, b.steps[0]); got != nil { + t.Fatalf("an update-if-unchanged recreated a removed branch: %+v", got) + } +} + +// No precondition is exactly the old contract: last writer wins, delete is +// idempotent. API clients and agents that never send the field keep working. +func TestBranchWithoutPreconditionIsLastWriterWins(t *testing.T) { + b, done := newBranchIT(t, "Precondition absent") + defer done() + ctx := context.Background() + b.always(t, b.steps[0], &b.steps[2]) + b.always(t, b.steps[0], nil) + if got := b.branchOf(t, b.steps[0]); got == nil || got.YesStepID.Valid { + t.Fatalf("second unconditional write did not win: %+v", got) + } + for range 2 { + if err := b.svc.DeleteBranch(ctx, b.ws, b.campaign, b.steps[0], BranchPrecondition{}); err != nil { + t.Fatalf("unconditional delete: %v", err) + } + } +} + +// Two editors create a branch on the same empty step at the same moment, each +// expecting none: exactly one wins, and the loser is told what the winner saved. +func TestBranchCreateIfAbsentRace(t *testing.T) { + b, done := newBranchIT(t, "Precondition race") + defer done() + ctx := context.Background() + + targets := []*uuid.UUID{&b.steps[1], &b.steps[2]} + rows := make([]gen.SequenceStepBranch, 2) + errs := make([]error, 2) + start := make(chan struct{}) + var wg sync.WaitGroup + for i := range 2 { + wg.Go(func() { + <-start + rows[i], errs[i] = b.svc.SetBranch(ctx, b.ws, b.campaign, BranchInput{ + StepID: b.steps[0], Condition: "always", YesStepID: targets[i], Expect: ExpectNoBranch(), + }) + }) + } + close(start) + wg.Wait() + + winner, loser := 0, 1 + if errs[0] != nil { + winner, loser = 1, 0 + } + if errs[winner] != nil { + t.Fatalf("both creates failed: %v / %v", errs[0], errs[1]) + } + changed := changedErr(t, errs[loser]) + if changed.Current == nil || changed.Current.YesStepID.Bytes != *targets[winner] { + t.Fatalf("loser's current = %+v, want the winner's branch", changed.Current) + } + if got := b.branchOf(t, b.steps[0]); got == nil || got.YesStepID.Bytes != *targets[winner] || + !got.UpdatedAt.Time.Equal(rows[winner].UpdatedAt.Time) { + t.Fatalf("stored = %+v, want the winner's %+v", got, rows[winner]) + } +} + +func TestBranchDeleteWithStalePreconditionIsRefused(t *testing.T) { + b, done := newBranchIT(t, "Precondition delete") + defer done() + ctx := context.Background() + v1, err := b.svc.SetBranch(ctx, b.ws, b.campaign, BranchInput{StepID: b.steps[0], Condition: "always", YesStepID: &b.steps[2]}) + if err != nil { + t.Fatal(err) + } + stale := wireToken(t, v1) + v2, err := b.svc.SetBranch(ctx, b.ws, b.campaign, BranchInput{StepID: b.steps[0], Condition: "always", Expect: stale}) + if err != nil { + t.Fatal(err) + } + + changed := changedErr(t, b.svc.DeleteBranch(ctx, b.ws, b.campaign, b.steps[0], stale)) + if changed.Current == nil || !changed.Current.UpdatedAt.Time.Equal(v2.UpdatedAt.Time) { + t.Fatalf("current = %+v, want %+v", changed.Current, v2) + } + if b.branchOf(t, b.steps[0]) == nil { + t.Fatal("a refused delete removed the branch") + } + + // Once someone else has removed it, a delete at the old version is refused + // with current = nil: the client learns it is gone rather than being told + // its own delete succeeded. + if err := b.svc.DeleteBranch(ctx, b.ws, b.campaign, b.steps[0], wireToken(t, v2)); err != nil { + t.Fatal(err) + } + if changed := changedErr(t, b.svc.DeleteBranch(ctx, b.ws, b.campaign, b.steps[0], wireToken(t, v2))); changed.Current != nil { + t.Fatalf("current = %+v, want nil", changed.Current) + } +} + +// A precondition changes nothing about tenancy: a foreign workspace is 404 +// whatever it expects, through the service or the store directly, and a +// precondition failure never leaks the owner's branch to it. +func TestBranchPreconditionStaysWorkspacePinned(t *testing.T) { + b, done := newBranchIT(t, "Precondition tenant owner") + defer done() + ctx := context.Background() + owned, err := b.svc.SetBranch(ctx, b.ws, b.campaign, BranchInput{StepID: b.steps[0], Condition: "always"}) + if err != nil { + t.Fatal(err) + } + intruder, err := b.q.CreateWorkspace(ctx, "Precondition tenant intruder "+uuid.NewString()) + if err != nil { + t.Fatal(err) + } + store := NewPgBranchStore(b.pool) + for name, expect := range map[string]BranchPrecondition{ + "none": {}, "absent": ExpectNoBranch(), "at": wireToken(t, owned), + } { + in := BranchInput{CampaignID: b.campaign, StepID: b.steps[0], Condition: "always", YesStepID: &b.steps[2], Expect: expect} + if _, err := b.svc.SetBranch(ctx, intruder.ID, b.campaign, in); !errors.Is(err, ErrCampaignNotFound) { + t.Errorf("%s: service set: want ErrCampaignNotFound, got %v", name, err) + } + if _, err := store.UpsertBranch(ctx, intruder.ID, in, checkGraph); !errors.Is(err, ErrCampaignNotFound) { + t.Errorf("%s: store set: want ErrCampaignNotFound, got %v", name, err) + } + if err := b.svc.DeleteBranch(ctx, intruder.ID, b.campaign, b.steps[0], expect); !errors.Is(err, ErrCampaignNotFound) { + t.Errorf("%s: service delete: want ErrCampaignNotFound, got %v", name, err) + } + if err := store.DeleteBranch(ctx, intruder.ID, b.campaign, b.steps[0], expect, checkGraph); !errors.Is(err, ErrCampaignNotFound) { + t.Errorf("%s: store delete: want ErrCampaignNotFound, got %v", name, err) + } + } + if got := b.branchOf(t, b.steps[0]); got == nil || got.YesStepID.Valid || !got.UpdatedAt.Time.Equal(owned.UpdatedAt.Time) { + t.Fatalf("the intruder changed the owner's branch: %+v", got) + } +} + +// updated_at is a token only if every write moves it. Back-to-back writes in +// one tight loop (well inside a millisecond each on a local database) must each +// see a strictly later value, and so must the ON DELETE SET NULL that deleting +// an exit's target step performs — a write no query of ours makes. +func TestBranchUpdatedAtAdvancesOnEveryWrite(t *testing.T) { + b, done := newBranchIT(t, "Precondition advances") + defer done() + ctx := context.Background() + + prev, err := b.svc.SetBranch(ctx, b.ws, b.campaign, BranchInput{StepID: b.steps[0], Condition: "always", YesStepID: &b.steps[2]}) + if err != nil { + t.Fatal(err) + } + for i := range 20 { + next, err := b.svc.SetBranch(ctx, b.ws, b.campaign, BranchInput{StepID: b.steps[0], Condition: "always", YesStepID: &b.steps[2]}) + if err != nil { + t.Fatal(err) + } + if !next.UpdatedAt.Time.After(prev.UpdatedAt.Time) { + t.Fatalf("write %d: updated_at %v did not advance past %v", i, next.UpdatedAt.Time, prev.UpdatedAt.Time) + } + prev = next + } + + // Same-transaction rewrites share now(); the trigger must still advance. + tx, err := b.pool.Begin(ctx) + if err != nil { + t.Fatal(err) + } + defer func() { _ = tx.Rollback(ctx) }() + var first, second time.Time + for _, dst := range []*time.Time{&first, &second} { + if err := tx.QueryRow(ctx, `UPDATE sequence_step_branches SET condition = 'always' WHERE step_id = $1 RETURNING updated_at`, b.steps[0]).Scan(dst); err != nil { + t.Fatal(err) + } + } + if !second.After(first) || !first.After(prev.UpdatedAt.Time) { + t.Fatalf("in one transaction: %v then %v (after %v)", first, second, prev.UpdatedAt.Time) + } + if err := tx.Rollback(ctx); err != nil { + t.Fatal(err) + } + + if err := b.svc.Delete(ctx, b.ws, b.campaign, b.steps[2]); err != nil { + t.Fatalf("delete target step: %v", err) + } + after := b.branchOf(t, b.steps[0]) + if after == nil || after.YesStepID.Valid || !after.UpdatedAt.Time.After(prev.UpdatedAt.Time) { + t.Fatalf("after its exit's target was deleted: %+v (was %v)", after, prev.UpdatedAt.Time) + } + // So a client holding the pre-delete token is told the branch changed. + _, err = b.svc.SetBranch(ctx, b.ws, b.campaign, BranchInput{StepID: b.steps[0], Condition: "always", Expect: wireToken(t, prev)}) + changedErr(t, err) +} diff --git a/internal/app/sequencestep/branchstore.go b/internal/app/sequencestep/branchstore.go new file mode 100644 index 00000000..d70a936d --- /dev/null +++ b/internal/app/sequencestep/branchstore.go @@ -0,0 +1,329 @@ +package sequencestep + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/google/uuid" + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgconn" + "github.com/jackc/pgx/v5/pgtype" + "github.com/jackc/pgx/v5/pgxpool" + + "github.com/inroad/inroad/internal/platform/db/gen" +) + +// GraphCheck validates a campaign's routing graph as it would be committed: the +// campaign's steps and branches AFTER a mutation, read inside the mutation's own +// transaction. A non-nil error rolls the mutation back and is returned to the +// caller unchanged. +// +// It is a function rather than a rule baked into the store so the store stays +// policy-free: what makes a graph valid is the service's decision +// (internal/platform/seqgraph), the store only guarantees the check sees exactly +// the graph it is about to commit. +type GraphCheck func(steps []gen.SequenceStep, branches []gen.SequenceStepBranch) error + +// BranchInput is one step's router as written. A nil exit ends the path. +type BranchInput struct { + CampaignID uuid.UUID + StepID uuid.UUID + Condition string + WithinDays *int32 + ReplyLabelKey *string + YesStepID *uuid.UUID + NoStepID *uuid.UUID + // Expect is the write's optimistic-concurrency precondition. The zero value + // is none: the write replaces whatever is there, as it always has. + Expect BranchPrecondition +} + +// BranchPrecondition is what a branch write expects to find on the step before +// it applies, so two editors cannot silently overwrite each other. The zero +// value expects nothing (last writer wins); ExpectNoBranch and ExpectBranchAt +// are the two real preconditions. A struct with unexported fields rather than a +// *time.Time so "no precondition" and "expect no branch" cannot be confused. +type BranchPrecondition struct { + kind preconditionKind + updatedAt time.Time +} + +type preconditionKind int + +const ( + preconditionNone preconditionKind = iota + preconditionAbsent + preconditionAt +) + +// ExpectNoBranch applies the write only if the step has no branch: a create +// that must not overwrite a branch someone else just made. +func ExpectNoBranch() BranchPrecondition { return BranchPrecondition{kind: preconditionAbsent} } + +// ExpectBranchAt applies the write only if the step's branch exists and its +// updated_at is exactly at (microsecond precision, as stored). +func ExpectBranchAt(at time.Time) BranchPrecondition { + return BranchPrecondition{kind: preconditionAt, updatedAt: at} +} + +// BranchChangedError is a write whose precondition no longer holds: the branch +// was created, replaced or removed since the client read it. Current is the +// branch as it is now (nil when the step has none), read in the same +// transaction that refused the write, so the client can show what won. +type BranchChangedError struct { + Current *gen.SequenceStepBranch +} + +func (e *BranchChangedError) Error() string { + return "the branch was changed since it was loaded; reload it and try again" +} + +// BranchStore is the persistence seam for routers. A distinct interface from +// Store for the reason VariantStore is one: distinct callers, and it keeps the +// graph rules testable without a database. +type BranchStore interface { + ListBranches(ctx context.Context, ws, campaignID uuid.UUID) ([]gen.SequenceStepBranch, error) + // ReplyLabelStops reports whether the workspace's label with this key stops + // the enrollment; found=false when no such label exists. + ReplyLabelStops(ctx context.Context, ws uuid.UUID, key string) (stops, found bool, err error) + // TrackingEnabled reports whether the campaign records opens and clicks. + TrackingEnabled(ctx context.Context, ws, campaignID uuid.UUID) (bool, error) + // UpsertBranch creates or replaces the router on in.StepID, then runs check + // on the result, in one transaction holding the campaign's graph lock. A + // precondition in in.Expect that does not hold is a *BranchChangedError and + // writes nothing. + UpsertBranch(ctx context.Context, ws uuid.UUID, in BranchInput, check GraphCheck) (gen.SequenceStepBranch, error) + // DeleteBranch removes the router on stepID (a no-op when there is none), + // then runs check, in one transaction holding the campaign's graph lock. + // Removing a router is a graph change too: it restores the step's linear + // fall-through, which can close a loop. A precondition that does not hold is + // a *BranchChangedError and removes nothing. + DeleteBranch(ctx context.Context, ws, campaignID, stepID uuid.UUID, expect BranchPrecondition, check GraphCheck) error +} + +// ErrBranchConflict is an upsert that matched a router row owned by a different +// workspace or campaign. Unreachable through the service (the step is +// ownership-checked first), and reported rather than swallowed so a future +// caller that skips that check fails closed. +var ErrBranchConflict = errors.New("step branch belongs to another campaign") + +// ErrTargetGone is an exit whose target step stopped existing between the +// service's pre-check and the write (a concurrent delete): the composite FK +// refused it. Reported as the same unknown-target failure the pre-check gives. +var ErrTargetGone = errors.New("a branch exit points at a step that is no longer in this campaign") + +// foreignKeyViolation is PostgreSQL's SQLSTATE for a refused foreign key. +const foreignKeyViolation = "23503" + +// PgBranchStore implements BranchStore over sqlc. The pool backs the +// lock-mutate-check transaction. +type PgBranchStore struct { + pool *pgxpool.Pool + q *gen.Queries +} + +func NewPgBranchStore(pool *pgxpool.Pool) *PgBranchStore { + return &PgBranchStore{pool: pool, q: gen.New(pool)} +} + +func (s *PgBranchStore) ListBranches(ctx context.Context, ws, campaignID uuid.UUID) ([]gen.SequenceStepBranch, error) { + return s.q.ListBranchesByCampaign(ctx, gen.ListBranchesByCampaignParams{CampaignID: campaignID, WorkspaceID: ws}) +} + +func (s *PgBranchStore) ReplyLabelStops(ctx context.Context, ws uuid.UUID, key string) (stops, found bool, err error) { + stops, err = s.q.ReplyLabelStopsEnrollment(ctx, gen.ReplyLabelStopsEnrollmentParams{WorkspaceID: ws, Key: key}) + if errors.Is(err, pgx.ErrNoRows) { + return false, false, nil + } + if err != nil { + return false, false, fmt.Errorf("reply label lookup: %w", err) + } + return stops, true, nil +} + +func (s *PgBranchStore) TrackingEnabled(ctx context.Context, ws, campaignID uuid.UUID) (bool, error) { + on, err := s.q.CampaignTrackingEnabled(ctx, gen.CampaignTrackingEnabledParams{ID: campaignID, WorkspaceID: ws}) + if err != nil { + return false, fmt.Errorf("campaign tracking lookup: %w", err) + } + return on, nil +} + +// UpsertBranch enforces in.Expect in the write itself — a conditional INSERT or +// UPDATE, not a read followed by a write — so the precondition and the change +// are one statement, under the graph lock besides. +func (s *PgBranchStore) UpsertBranch(ctx context.Context, ws uuid.UUID, in BranchInput, check GraphCheck) (gen.SequenceStepBranch, error) { + var out gen.SequenceStepBranch + err := withGraphLock(ctx, s.pool, s.q, ws, in.CampaignID, check, func(q *gen.Queries) error { + row, err := writeBranch(ctx, q, ws, in) + if errors.Is(err, pgx.ErrNoRows) { + return refusedWrite(ctx, q, ws, in) + } + var pgErr *pgconn.PgError + if errors.As(err, &pgErr) && pgErr.Code == foreignKeyViolation { + return ErrTargetGone + } + if err != nil { + return fmt.Errorf("upsert branch: %w", err) + } + out = row + return nil + }) + return out, err +} + +// writeBranch runs the one statement in.Expect calls for. Each returns +// pgx.ErrNoRows when it writes nothing. +func writeBranch(ctx context.Context, q *gen.Queries, ws uuid.UUID, in BranchInput) (gen.SequenceStepBranch, error) { + yes, no := nullUUID(in.YesStepID), nullUUID(in.NoStepID) + switch in.Expect.kind { + case preconditionAbsent: + return q.InsertBranchIfAbsent(ctx, gen.InsertBranchIfAbsentParams{ + StepID: in.StepID, WorkspaceID: ws, CampaignID: in.CampaignID, Condition: in.Condition, + WithinDays: in.WithinDays, ReplyLabelKey: in.ReplyLabelKey, YesStepID: yes, NoStepID: no, + }) + case preconditionAt: + return q.UpdateBranchIfUnchanged(ctx, gen.UpdateBranchIfUnchangedParams{ + StepID: in.StepID, WorkspaceID: ws, CampaignID: in.CampaignID, Condition: in.Condition, + WithinDays: in.WithinDays, ReplyLabelKey: in.ReplyLabelKey, YesStepID: yes, NoStepID: no, + ExpectedUpdatedAt: timestamptz(in.Expect.updatedAt), + }) + default: + return q.UpsertBranch(ctx, gen.UpsertBranchParams{ + StepID: in.StepID, WorkspaceID: ws, CampaignID: in.CampaignID, Condition: in.Condition, + WithinDays: in.WithinDays, ReplyLabelKey: in.ReplyLabelKey, YesStepID: yes, NoStepID: no, + }) + } +} + +// refusedWrite explains a write that matched no row. With a precondition that +// is the precondition failing, reported with the branch as it now is. Without +// one — or when "expect no branch" collided with a row this workspace cannot +// see — it is the tenant pin refusing another owner's row. +func refusedWrite(ctx context.Context, q *gen.Queries, ws uuid.UUID, in BranchInput) error { + if in.Expect.kind == preconditionNone { + return ErrBranchConflict + } + current, err := currentBranch(ctx, q, ws, in.CampaignID, in.StepID) + if err != nil { + return err + } + if current == nil && in.Expect.kind == preconditionAbsent { + return ErrBranchConflict + } + return &BranchChangedError{Current: current} +} + +// currentBranch reads one step's router inside the caller's transaction; nil +// when the step has none. +func currentBranch(ctx context.Context, q *gen.Queries, ws, campaignID, stepID uuid.UUID) (*gen.SequenceStepBranch, error) { + row, err := q.GetBranch(ctx, gen.GetBranchParams{StepID: stepID, CampaignID: campaignID, WorkspaceID: ws}) + if errors.Is(err, pgx.ErrNoRows) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("read current branch: %w", err) + } + return &row, nil +} + +// DeleteBranch, like UpsertBranch, puts an "expect this version" precondition in +// the DELETE's own WHERE clause. +func (s *PgBranchStore) DeleteBranch(ctx context.Context, ws, campaignID, stepID uuid.UUID, expect BranchPrecondition, check GraphCheck) error { + return withGraphLock(ctx, s.pool, s.q, ws, campaignID, check, func(q *gen.Queries) error { + switch expect.kind { + case preconditionAt: + n, err := q.DeleteBranchIfUnchanged(ctx, gen.DeleteBranchIfUnchangedParams{ + StepID: stepID, CampaignID: campaignID, WorkspaceID: ws, ExpectedUpdatedAt: timestamptz(expect.updatedAt), + }) + if err != nil { + return fmt.Errorf("delete branch: %w", err) + } + if n == 0 { + return changedSince(ctx, q, ws, campaignID, stepID, false) + } + return nil + case preconditionAbsent: + // Expecting no branch, a delete has nothing to remove; it holds iff + // there is still none, which the graph lock keeps true to commit. + return changedSince(ctx, q, ws, campaignID, stepID, true) + default: + if err := q.DeleteBranch(ctx, gen.DeleteBranchParams{StepID: stepID, CampaignID: campaignID, WorkspaceID: ws}); err != nil { + return fmt.Errorf("delete branch: %w", err) + } + return nil + } + }) +} + +// changedSince reports a refused precondition with the branch as it now is (nil +// when the step has none). absentIsFine is for "expect no branch", where +// finding none means the precondition held after all. +func changedSince(ctx context.Context, q *gen.Queries, ws, campaignID, stepID uuid.UUID, absentIsFine bool) error { + current, err := currentBranch(ctx, q, ws, campaignID, stepID) + if err != nil { + return err + } + if current == nil && absentIsFine { + return nil + } + return &BranchChangedError{Current: current} +} + +// withGraphLock runs mutate and then check in ONE transaction that first takes +// the campaign's graph lock (LockCampaignGraph). Serializing on the campaign is +// what makes the check sound: two edits that are each acyclic alone can close a +// loop together, and without the lock both would pass their own check against a +// graph that no longer exists by the time they commit. Every write is pinned on +// workspace_id by the queries mutate calls; the lock itself matches zero rows for +// a campaign outside ws, which is reported as ErrCampaignNotFound. +func withGraphLock(ctx context.Context, pool *pgxpool.Pool, q *gen.Queries, ws, campaignID uuid.UUID, + check GraphCheck, mutate func(q *gen.Queries) error) error { + tx, err := pool.Begin(ctx) + if err != nil { + return fmt.Errorf("begin graph tx: %w", err) + } + defer func() { _ = tx.Rollback(ctx) }() // no-op once committed + qtx := q.WithTx(tx) + + if _, err := qtx.LockCampaignGraph(ctx, gen.LockCampaignGraphParams{ID: campaignID, WorkspaceID: ws}); err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return ErrCampaignNotFound + } + return fmt.Errorf("lock campaign graph: %w", err) + } + if err := mutate(qtx); err != nil { + return err + } + if check != nil { + steps, err := qtx.ListStepsByCampaign(ctx, gen.ListStepsByCampaignParams{CampaignID: campaignID, WorkspaceID: ws}) + if err != nil { + return fmt.Errorf("list steps: %w", err) + } + branches, err := qtx.ListBranchesByCampaign(ctx, gen.ListBranchesByCampaignParams{CampaignID: campaignID, WorkspaceID: ws}) + if err != nil { + return fmt.Errorf("list branches: %w", err) + } + if err := check(steps, branches); err != nil { + return err + } + } + if err := tx.Commit(ctx); err != nil { + return fmt.Errorf("commit graph tx: %w", err) + } + return nil +} + +// nullUUID converts an optional id into the nullable column value. +func nullUUID(id *uuid.UUID) pgtype.UUID { + if id == nil { + return pgtype.UUID{} + } + return pgtype.UUID{Bytes: *id, Valid: true} +} + +func timestamptz(t time.Time) pgtype.Timestamptz { + return pgtype.Timestamptz{Time: t, Valid: true} +} diff --git a/internal/app/sequencestep/handler.go b/internal/app/sequencestep/handler.go index e782c4c0..a0e09221 100644 --- a/internal/app/sequencestep/handler.go +++ b/internal/app/sequencestep/handler.go @@ -11,6 +11,7 @@ import ( "github.com/inroad/inroad/internal/app/auth" "github.com/inroad/inroad/internal/platform/db/gen" "github.com/inroad/inroad/internal/platform/httpx" + "github.com/inroad/inroad/internal/platform/seqgraph" "github.com/inroad/inroad/internal/platform/validate" ) @@ -131,6 +132,9 @@ func (h *Handler) Delete(w http.ResponseWriter, r *http.Request) { httpx.Error(w, http.StatusConflict, "steps can only be removed while the campaign is draft") case errors.Is(err, ErrNotFound): httpx.Error(w, http.StatusNotFound, "step not found") + case seqgraph.CodeOf(err) != "": + // The re-linked fall-through would close a loop through a branch (422). + writeGraphError(w, err, "could not delete step") case err != nil: httpx.Error(w, http.StatusInternalServerError, "could not delete step") default: @@ -175,6 +179,9 @@ func (h *Handler) Reorder(w http.ResponseWriter, r *http.Request) { httpx.Error(w, http.StatusNotFound, "step not found") case errors.Is(err, ErrInvalidOrder): httpx.Error(w, http.StatusBadRequest, err.Error()) + case seqgraph.CodeOf(err) != "": + // The new order's fall-through would close a loop through a branch (422). + writeGraphError(w, err, "could not reorder steps") case err != nil: httpx.Error(w, http.StatusInternalServerError, "could not reorder steps") default: diff --git a/internal/app/sequencestep/reorder_integration_test.go b/internal/app/sequencestep/reorder_integration_test.go index e571af7c..b4db7583 100644 --- a/internal/app/sequencestep/reorder_integration_test.go +++ b/internal/app/sequencestep/reorder_integration_test.go @@ -4,6 +4,7 @@ package sequencestep import ( "context" + "errors" "testing" "github.com/google/uuid" @@ -85,7 +86,7 @@ func TestReorderPersistsNewOrder(t *testing.T) { // Reverse the order: [3,2,1]. want := []uuid.UUID{ids[2], ids[1], ids[0]} - got, err := store.Reorder(ctx, ws, campaignID, want) + got, err := store.Reorder(ctx, ws, campaignID, want, checkGraph) if err != nil { t.Fatalf("Reorder: %v", err) } @@ -109,8 +110,8 @@ func TestReorderPersistsNewOrder(t *testing.T) { } // TestReorderIsWorkspaceScoped proves the workspace pin: reordering under a -// foreign workspace id touches zero rows (returns an empty list) and leaves the -// owning workspace's step_order untouched. +// foreign workspace id is refused (ErrCampaignNotFound, no rows returned) and +// leaves the owning workspace's step_order untouched. func TestReorderIsWorkspaceScoped(t *testing.T) { ctx := context.Background() if err := db.Migrate(dbtest.DSN(t)); err != nil { @@ -131,9 +132,12 @@ func TestReorderIsWorkspaceScoped(t *testing.T) { } // Intruder attempts to reorder the owner's steps under its own workspace id. - got, err := store.Reorder(ctx, other.ID, campaignID, []uuid.UUID{ids[2], ids[1], ids[0]}) - if err != nil { - t.Fatalf("cross-tenant Reorder: %v", err) + // The graph lock is taken on (campaign, workspace) before any write, so a + // foreign workspace fails there — refused outright rather than, as before the + // lock existed, running every UPDATE against zero rows. + got, err := store.Reorder(ctx, other.ID, campaignID, []uuid.UUID{ids[2], ids[1], ids[0]}, checkGraph) + if !errors.Is(err, ErrCampaignNotFound) { + t.Fatalf("cross-tenant Reorder: want ErrCampaignNotFound, got %v", err) } if len(got) != 0 { t.Fatalf("cross-tenant reorder returned %d rows, want 0", len(got)) diff --git a/internal/app/sequencestep/routes.go b/internal/app/sequencestep/routes.go index 4887df03..9c8f2737 100644 --- a/internal/app/sequencestep/routes.go +++ b/internal/app/sequencestep/routes.go @@ -27,6 +27,15 @@ func (h *Handler) Register(r chi.Router) { r.With(write).Put("/{id}/steps/{stepId}", h.Update) r.With(write).Delete("/{id}/steps/{stepId}", h.Delete) + // Conditional branching. The graph read is the whole campaign's routing in + // one response; a branch is written per step, nested under the step it + // routes out of so the ownership check has the step id without trusting the + // body. Writes take campaigns:write and, like variant writes, are allowed on + // a running campaign — see Service.SetBranch for the mid-flight contract. + r.With(read).Get("/{id}/graph", h.Graph) + r.With(write).Put("/{id}/steps/{stepId}/branch", h.SetBranch) + r.With(write).Delete("/{id}/steps/{stepId}/branch", h.DeleteBranch) + // A/B variants. Nested under the step they belong to: a variant has no // meaning apart from its step, and the nesting is what makes the step id // available for the ownership check without trusting the body. diff --git a/internal/app/sequencestep/service.go b/internal/app/sequencestep/service.go index ff5ecb16..542a754f 100644 --- a/internal/app/sequencestep/service.go +++ b/internal/app/sequencestep/service.go @@ -33,13 +33,15 @@ type Service struct { store Store checker CampaignChecker variants VariantStore + branches BranchStore } // NewService builds the step service. variants is a second, narrow seam for A/B // variants: a distinct responsibility with distinct callers, and keeping it // separate is what lets the weight invariant be tested without a database. -func NewService(store Store, checker CampaignChecker, variants VariantStore) *Service { - return &Service{store: store, checker: checker, variants: variants} +// branches is the third, for the routing graph, for the same reason. +func NewService(store Store, checker CampaignChecker, variants VariantStore, branches BranchStore) *Service { + return &Service{store: store, checker: checker, variants: variants, branches: branches} } // Create appends a step at max(step_order)+1. Structural change → requires the @@ -82,6 +84,11 @@ func (s *Service) Update(ctx context.Context, ws, campaignID uuid.UUID, in Updat // Delete removes a step. Structural change → requires the campaign to be // draft (running/paused/done return 409). +// +// Branch exits that pointed at the deleted step become path ends (the FK nulls +// them), and the linear fall-through re-links around the gap. That re-link can +// close a loop through a branch elsewhere, so the delete commits only if the +// resulting graph validates — refused with a *seqgraph.CycleError otherwise. func (s *Service) Delete(ctx context.Context, ws, campaignID, stepID uuid.UUID) error { if err := s.requireDraft(ctx, ws, campaignID); err != nil { return err @@ -89,7 +96,7 @@ func (s *Service) Delete(ctx context.Context, ws, campaignID, stepID uuid.UUID) if err := s.assertStepInCampaign(ctx, ws, campaignID, stepID); err != nil { return err } - return s.store.Delete(ctx, ws, stepID) + return s.store.Delete(ctx, ws, campaignID, stepID, checkGraph) } // Reorder rewrites the campaign's step_order to match stepIDs' order. @@ -109,7 +116,9 @@ func (s *Service) Reorder(ctx context.Context, ws, campaignID uuid.UUID, stepIDs if err := validatePermutation(current, stepIDs); err != nil { return nil, err } - return s.store.Reorder(ctx, ws, campaignID, stepIDs) + // Reordering moves every linear fall-through edge, so like a delete it is + // committed only if the resulting graph has no loop. + return s.store.Reorder(ctx, ws, campaignID, stepIDs, checkGraph) } // validatePermutation confirms stepIDs contains each of current's ids exactly @@ -154,9 +163,15 @@ func (s *Service) requireDraft(ctx context.Context, ws, campaignID uuid.UUID) er // campaignID; otherwise ErrNotFound (never leaks another campaign's/tenant's // step). func (s *Service) assertStepInCampaign(ctx context.Context, ws, campaignID, stepID uuid.UUID) error { + _, err := s.stepInCampaign(ctx, ws, campaignID, stepID) + return err +} + +// stepInCampaign is assertStepInCampaign for a caller that also needs the step. +func (s *Service) stepInCampaign(ctx context.Context, ws, campaignID, stepID uuid.UUID) (gen.SequenceStep, error) { st, err := s.store.Get(ctx, ws, stepID) if err != nil || st.CampaignID != campaignID { - return ErrNotFound + return gen.SequenceStep{}, ErrNotFound } - return nil + return st, nil } diff --git a/internal/app/sequencestep/service_test.go b/internal/app/sequencestep/service_test.go index 18c89096..f422466c 100644 --- a/internal/app/sequencestep/service_test.go +++ b/internal/app/sequencestep/service_test.go @@ -33,7 +33,7 @@ func (f *fakeStore) Get(context.Context, uuid.UUID, uuid.UUID) (gen.SequenceStep func (f *fakeStore) List(context.Context, uuid.UUID, uuid.UUID) ([]gen.SequenceStep, error) { return f.listSteps, nil } -func (f *fakeStore) Reorder(_ context.Context, _, _ uuid.UUID, stepIDs []uuid.UUID) ([]gen.SequenceStep, error) { +func (f *fakeStore) Reorder(_ context.Context, _, _ uuid.UUID, stepIDs []uuid.UUID, _ GraphCheck) ([]gen.SequenceStep, error) { f.reorderedTo = stepIDs out := make([]gen.SequenceStep, len(stepIDs)) for i, id := range stepIDs { @@ -45,7 +45,10 @@ func (f *fakeStore) Update(_ context.Context, _ uuid.UUID, in UpdateInput) (gen. f.updated = in return gen.SequenceStep{ID: in.StepID, Subject: in.Subject}, nil } -func (f *fakeStore) Delete(_ context.Context, _, id uuid.UUID) error { f.deletedID = id; return nil } +func (f *fakeStore) Delete(_ context.Context, _, _, id uuid.UUID, _ GraphCheck) error { + f.deletedID = id + return nil +} func (f *fakeStore) MaxStepOrder(context.Context, uuid.UUID, uuid.UUID) (int32, error) { return f.maxOrder, nil } @@ -60,7 +63,7 @@ func (c fakeChecker) CampaignStatus(context.Context, uuid.UUID, uuid.UUID) (stri } func TestCreateRejectsNonDraftCampaign(t *testing.T) { - svc := NewService(&fakeStore{}, fakeChecker{status: "running"}, &fakeVariantStore{}) + svc := NewService(&fakeStore{}, fakeChecker{status: "running"}, &fakeVariantStore{}, &fakeBranchStore{}) _, err := svc.Create(context.Background(), uuid.New(), uuid.New(), CreateInput{Subject: "x", BodyText: "y"}) if !errors.Is(err, ErrCampaignNotDraft) { t.Fatalf("want ErrCampaignNotDraft, got %v", err) @@ -68,7 +71,7 @@ func TestCreateRejectsNonDraftCampaign(t *testing.T) { } func TestCreateRejectsMissingCampaign(t *testing.T) { - svc := NewService(&fakeStore{}, fakeChecker{err: errors.New("no rows")}, &fakeVariantStore{}) + svc := NewService(&fakeStore{}, fakeChecker{err: errors.New("no rows")}, &fakeVariantStore{}, &fakeBranchStore{}) _, err := svc.Create(context.Background(), uuid.New(), uuid.New(), CreateInput{Subject: "x", BodyText: "y"}) if !errors.Is(err, ErrCampaignNotFound) { t.Fatalf("want ErrCampaignNotFound, got %v", err) @@ -77,7 +80,7 @@ func TestCreateRejectsMissingCampaign(t *testing.T) { func TestCreateAppendsAtNextOrder(t *testing.T) { store := &fakeStore{maxOrder: 2} - svc := NewService(store, fakeChecker{status: "draft"}, &fakeVariantStore{}) + svc := NewService(store, fakeChecker{status: "draft"}, &fakeVariantStore{}, &fakeBranchStore{}) campaignID := uuid.New() st, err := svc.Create(context.Background(), uuid.New(), campaignID, CreateInput{Subject: "x", BodyText: "y"}) if err != nil { @@ -96,7 +99,7 @@ func TestUpdateAllowedOnRunningCampaign(t *testing.T) { campaignID := uuid.New() stepID := uuid.New() store := &fakeStore{getStep: gen.SequenceStep{ID: stepID, CampaignID: campaignID}} - svc := NewService(store, fakeChecker{status: "running"}, &fakeVariantStore{}) + svc := NewService(store, fakeChecker{status: "running"}, &fakeVariantStore{}, &fakeBranchStore{}) _, err := svc.Update(context.Background(), uuid.New(), campaignID, UpdateInput{StepID: stepID, Subject: "new"}) if err != nil { t.Fatalf("update on running should be allowed (live-reference), got %v", err) @@ -111,7 +114,7 @@ func TestDeleteRejectsRunningCampaign(t *testing.T) { campaignID := uuid.New() stepID := uuid.New() store := &fakeStore{getStep: gen.SequenceStep{ID: stepID, CampaignID: campaignID}} - svc := NewService(store, fakeChecker{status: "running"}, &fakeVariantStore{}) + svc := NewService(store, fakeChecker{status: "running"}, &fakeVariantStore{}, &fakeBranchStore{}) err := svc.Delete(context.Background(), uuid.New(), campaignID, stepID) if !errors.Is(err, ErrCampaignNotDraft) { t.Fatalf("want ErrCampaignNotDraft, got %v", err) @@ -126,7 +129,7 @@ func TestReorderHappyPath(t *testing.T) { store := &fakeStore{listSteps: []gen.SequenceStep{ {ID: a, StepOrder: 1}, {ID: b, StepOrder: 2}, {ID: c, StepOrder: 3}, }} - svc := NewService(store, fakeChecker{status: "draft"}, &fakeVariantStore{}) + svc := NewService(store, fakeChecker{status: "draft"}, &fakeVariantStore{}, &fakeBranchStore{}) newOrder := []uuid.UUID{c, a, b} got, err := svc.Reorder(context.Background(), uuid.New(), campaignID, newOrder) if err != nil { @@ -156,7 +159,7 @@ func TestReorderRejectsNonPermutation(t *testing.T) { for name, ids := range cases { t.Run(name, func(t *testing.T) { store := &fakeStore{listSteps: []gen.SequenceStep{{ID: a, StepOrder: 1}, {ID: b, StepOrder: 2}}} - svc := NewService(store, fakeChecker{status: "draft"}, &fakeVariantStore{}) + svc := NewService(store, fakeChecker{status: "draft"}, &fakeVariantStore{}, &fakeBranchStore{}) _, err := svc.Reorder(context.Background(), uuid.New(), campaignID, ids) if !errors.Is(err, ErrInvalidOrder) { t.Fatalf("want ErrInvalidOrder, got %v", err) @@ -171,7 +174,7 @@ func TestReorderRejectsNonPermutation(t *testing.T) { // Reorder is a structural edit → forbidden on a non-draft campaign (409). func TestReorderRejectsNonDraftCampaign(t *testing.T) { store := &fakeStore{} - svc := NewService(store, fakeChecker{status: "running"}, &fakeVariantStore{}) + svc := NewService(store, fakeChecker{status: "running"}, &fakeVariantStore{}, &fakeBranchStore{}) _, err := svc.Reorder(context.Background(), uuid.New(), uuid.New(), []uuid.UUID{uuid.New()}) if !errors.Is(err, ErrCampaignNotDraft) { t.Fatalf("want ErrCampaignNotDraft, got %v", err) @@ -183,7 +186,7 @@ func TestReorderRejectsNonDraftCampaign(t *testing.T) { // Reorder on a missing campaign is 404, checked before any store read. func TestReorderRejectsMissingCampaign(t *testing.T) { - svc := NewService(&fakeStore{}, fakeChecker{err: errors.New("no rows")}, &fakeVariantStore{}) + svc := NewService(&fakeStore{}, fakeChecker{err: errors.New("no rows")}, &fakeVariantStore{}, &fakeBranchStore{}) _, err := svc.Reorder(context.Background(), uuid.New(), uuid.New(), []uuid.UUID{uuid.New()}) if !errors.Is(err, ErrCampaignNotFound) { t.Fatalf("want ErrCampaignNotFound, got %v", err) @@ -197,7 +200,7 @@ func TestUpdateRejectsStepFromAnotherCampaign(t *testing.T) { otherCampaign := uuid.New() stepID := uuid.New() store := &fakeStore{getStep: gen.SequenceStep{ID: stepID, CampaignID: otherCampaign}} - svc := NewService(store, fakeChecker{status: "draft"}, &fakeVariantStore{}) + svc := NewService(store, fakeChecker{status: "draft"}, &fakeVariantStore{}, &fakeBranchStore{}) _, err := svc.Update(context.Background(), uuid.New(), urlCampaign, UpdateInput{StepID: stepID, Subject: "x"}) if !errors.Is(err, ErrNotFound) { t.Fatalf("want ErrNotFound for mismatched campaign, got %v", err) diff --git a/internal/app/sequencestep/store.go b/internal/app/sequencestep/store.go index 1046deb4..2dc762fc 100644 --- a/internal/app/sequencestep/store.go +++ b/internal/app/sequencestep/store.go @@ -43,13 +43,18 @@ type Store interface { Get(ctx context.Context, ws, id uuid.UUID) (gen.SequenceStep, error) List(ctx context.Context, ws, campaignID uuid.UUID) ([]gen.SequenceStep, error) Update(ctx context.Context, ws uuid.UUID, in UpdateInput) (gen.SequenceStep, error) - Delete(ctx context.Context, ws, id uuid.UUID) error + // Delete removes one step of campaignID, then runs check on the campaign's + // resulting graph, in one transaction holding the campaign's graph lock (a + // delete re-links the linear fall-through around the removed step, which can + // close a loop through a branch). A check error rolls the delete back. + Delete(ctx context.Context, ws, campaignID, id uuid.UUID, check GraphCheck) error MaxStepOrder(ctx context.Context, ws, campaignID uuid.UUID) (int32, error) - // Reorder rewrites step_order to 1..N to match stepIDs' order, in one - // transaction, and returns the campaign's steps in the new order. Every - // write is pinned by campaign_id AND workspace_id. Callers must pre-validate - // that stepIDs is a permutation of the campaign's current step ids. - Reorder(ctx context.Context, ws, campaignID uuid.UUID, stepIDs []uuid.UUID) ([]gen.SequenceStep, error) + // Reorder rewrites step_order to 1..N to match stepIDs' order, then runs + // check on the resulting graph, in one transaction holding the campaign's + // graph lock, and returns the campaign's steps in the new order. Every write + // is pinned by campaign_id AND workspace_id. Callers must pre-validate that + // stepIDs is a permutation of the campaign's current step ids. + Reorder(ctx context.Context, ws, campaignID uuid.UUID, stepIDs []uuid.UUID, check GraphCheck) ([]gen.SequenceStep, error) } // CampaignChecker reports a campaign's status (and existence) within the @@ -87,8 +92,13 @@ func (s *PgStore) Update(ctx context.Context, ws uuid.UUID, in UpdateInput) (gen Subject: in.Subject, BodyText: in.BodyText, BodyHtml: in.BodyHTML, }) } -func (s *PgStore) Delete(ctx context.Context, ws, id uuid.UUID) error { - return s.q.DeleteStep(ctx, gen.DeleteStepParams{ID: id, WorkspaceID: ws}) +func (s *PgStore) Delete(ctx context.Context, ws, campaignID, id uuid.UUID, check GraphCheck) error { + return withGraphLock(ctx, s.pool, s.q, ws, campaignID, check, func(q *gen.Queries) error { + if err := q.DeleteStep(ctx, gen.DeleteStepParams{ID: id, WorkspaceID: ws}); err != nil { + return fmt.Errorf("delete step: %w", err) + } + return nil + }) } func (s *PgStore) MaxStepOrder(ctx context.Context, ws, campaignID uuid.UUID) (int32, error) { return s.q.MaxStepOrder(ctx, gen.MaxStepOrderParams{CampaignID: campaignID, WorkspaceID: ws}) @@ -100,36 +110,38 @@ func (s *PgStore) MaxStepOrder(ctx context.Context, ws, campaignID uuid.UUID) (i // final 1-based position. All writes are pinned by campaign_id AND // workspace_id, so a foreign id would update zero rows (belt-and-braces on top // of the service's permutation check). Either every write commits or none does. -func (s *PgStore) Reorder(ctx context.Context, ws, campaignID uuid.UUID, stepIDs []uuid.UUID) ([]gen.SequenceStep, error) { - tx, err := s.pool.Begin(ctx) - if err != nil { - return nil, fmt.Errorf("begin reorder tx: %w", err) - } - defer func() { _ = tx.Rollback(ctx) }() // no-op once committed - qtx := s.q.WithTx(tx) - - maxOrder, err := qtx.MaxStepOrder(ctx, gen.MaxStepOrderParams{CampaignID: campaignID, WorkspaceID: ws}) - if err != nil { - return nil, fmt.Errorf("max step order: %w", err) - } - if err := qtx.ShiftStepOrders(ctx, gen.ShiftStepOrdersParams{ - CampaignID: campaignID, WorkspaceID: ws, StepOrder: maxOrder, - }); err != nil { - return nil, fmt.Errorf("shift step orders: %w", err) - } - for i, id := range stepIDs { - if err := qtx.SetStepOrder(ctx, gen.SetStepOrderParams{ - ID: id, CampaignID: campaignID, WorkspaceID: ws, StepOrder: int32(i + 1), +// +// The transaction is the graph-lock one (withGraphLock): reordering moves the +// linear fall-through edges, so it is a routing change like any branch edit. A +// campaign outside ws fails the lock and returns ErrCampaignNotFound before any +// write. +func (s *PgStore) Reorder(ctx context.Context, ws, campaignID uuid.UUID, stepIDs []uuid.UUID, check GraphCheck) ([]gen.SequenceStep, error) { + var steps []gen.SequenceStep + err := withGraphLock(ctx, s.pool, s.q, ws, campaignID, check, func(qtx *gen.Queries) error { + maxOrder, err := qtx.MaxStepOrder(ctx, gen.MaxStepOrderParams{CampaignID: campaignID, WorkspaceID: ws}) + if err != nil { + return fmt.Errorf("max step order: %w", err) + } + if err := qtx.ShiftStepOrders(ctx, gen.ShiftStepOrdersParams{ + CampaignID: campaignID, WorkspaceID: ws, StepOrder: maxOrder, }); err != nil { - return nil, fmt.Errorf("set step order: %w", err) + return fmt.Errorf("shift step orders: %w", err) } - } - steps, err := qtx.ListStepsByCampaign(ctx, gen.ListStepsByCampaignParams{CampaignID: campaignID, WorkspaceID: ws}) + for i, id := range stepIDs { + if err := qtx.SetStepOrder(ctx, gen.SetStepOrderParams{ + ID: id, CampaignID: campaignID, WorkspaceID: ws, StepOrder: int32(i + 1), + }); err != nil { + return fmt.Errorf("set step order: %w", err) + } + } + steps, err = qtx.ListStepsByCampaign(ctx, gen.ListStepsByCampaignParams{CampaignID: campaignID, WorkspaceID: ws}) + if err != nil { + return fmt.Errorf("list steps: %w", err) + } + return nil + }) if err != nil { - return nil, fmt.Errorf("list steps: %w", err) - } - if err := tx.Commit(ctx); err != nil { - return nil, fmt.Errorf("commit reorder tx: %w", err) + return nil, err } return steps, nil } diff --git a/internal/app/sequencestep/variant_test.go b/internal/app/sequencestep/variant_test.go index 0b8204a8..6703db2b 100644 --- a/internal/app/sequencestep/variant_test.go +++ b/internal/app/sequencestep/variant_test.go @@ -18,7 +18,7 @@ func variantFixture(baseWeight int32, variants ...Variant) (*Service, *fakeVaria step := gen.SequenceStep{ID: testStepID, StepOrder: 1, Subject: "hi", VariantWeight: baseWeight} store := &fakeStore{getStep: step} vs := &fakeVariantStore{variants: variants, sent: map[uuid.UUID]int64{}} - return NewService(store, fakeChecker{status: "running"}, vs), vs + return NewService(store, fakeChecker{status: "running"}, vs, &fakeBranchStore{}), vs } func variant(label string, weight int32) Variant { diff --git a/internal/coreapi/coreapi.go b/internal/coreapi/coreapi.go index ffb73632..0306f17c 100644 --- a/internal/coreapi/coreapi.go +++ b/internal/coreapi/coreapi.go @@ -295,9 +295,13 @@ type Client interface { // GetStepSendJob loads everything needed to send the enrollment's next due // step (current_step+1): resolved step content, personalization vars, - // threading headers, cap gate, and decrypted transport. Read-only — it - // creates no rows, so a suppressed/capped step leaves no orphan. workspaceID - // is pinned in the SQL WHERE (defense in depth on the enrollment UUID). + // threading headers, cap gate, and decrypted transport. It creates no + // sends row (the claim does), so a suppressed/capped step leaves no orphan. + // It may write enrollment state the control plane decides on its own: a + // pool mailbox pin, and on a branched campaign a routed completion or a + // condition wait (ConditionPending). Each is guarded on status='active' and + // idempotent. workspaceID is pinned in the SQL WHERE (defense in depth on + // the enrollment UUID). GetStepSendJob(ctx context.Context, enrollmentID, workspaceID string) (StepSendJob, error) // ClaimStepSend claims one step-send for delivery (claim-before-send): the // sends row is inserted 'sending' (fresh claim), or a STALE 'sending' lease is @@ -730,6 +734,18 @@ type StepSendJob struct { // whereas a pause is a condition that CLEARS. The enrollment has to wait and // resume, so the worker defers it (see the blocked branch in advance.go). CampaignPaused bool `json:"campaign_paused"` + // ConditionPending means the enrollment's next move waits on a branch + // condition that is not decided yet, or on a routed step that is not due + // yet. The control plane has ALREADY re-stamped next_due_at to RecheckAt; + // the worker's only job is to schedule the next advance for then. + // + // Skip is set alongside it, deliberately: a worker built before this field + // existed decodes it as false and must not treat the otherwise-empty job as + // a send. Such a worker skips instead, and the stamped next_due_at lets the + // sweeper re-drive the enrollment. A current worker checks ConditionPending + // FIRST, so it schedules the recheck rather than leaving it to the sweeper. + ConditionPending bool `json:"condition_pending"` + RecheckAt time.Time `json:"recheck_at"` // NotDueUntil is the enrollment's persisted next_due_at, carried so the // claim can refuse a step that is not due yet. It exists because pushing // next_due_at out (DeferEnrollment, the out-of-office path) cannot cancel diff --git a/internal/coreapi/inprocess/branchroute.go b/internal/coreapi/inprocess/branchroute.go new file mode 100644 index 00000000..d9da46ca --- /dev/null +++ b/internal/coreapi/inprocess/branchroute.go @@ -0,0 +1,442 @@ +package inprocess + +import ( + "context" + "errors" + "fmt" + "log/slog" + "time" + + "github.com/google/uuid" + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgtype" + + "github.com/inroad/inroad/internal/app/sequencestep" + "github.com/inroad/inroad/internal/coreapi" + "github.com/inroad/inroad/internal/platform/cadence" + "github.com/inroad/inroad/internal/platform/db/gen" + "github.com/inroad/inroad/internal/platform/seqgraph" +) + +// Conditional branching on the send path. +// +// # When this code runs +// +// ONLY for a campaign that has at least one branch, or for an enrollment whose +// current wait was set by a condition (awaiting_condition_step == current_step: +// the campaign's last branch was deleted while this contact waited on it). Every +// other enrollment — every linear campaign — takes the original GetNextStep path +// in localStepSendJob, untouched. See usesGraphRouting. +// +// # The route is re-derived, never stored +// +// An enrollment carries no "which branch did I take" state. On every advance the +// route out of the step it last received is recomputed from the CURRENT graph +// and the engagement evidence. That is sound because a verdict is a pure +// function of events inside a fixed window (seqgraph.Evaluate): once decided it +// cannot change, so re-deriving it later lands on the same exit. +// +// # Mid-flight edit rule +// +// Graph edits are allowed on a running campaign, and the rule for an enrollment +// already in flight is: EVERY DECISION IS MADE AGAINST THE GRAPH AS IT IS WHEN +// THE DECISION IS MADE. Concretely, for an enrollment whose last received step +// is S: +// +// - A message already sent is never affected; nothing is re-sent or recalled. +// - A branch added to S after S was sent applies at the next advance, with its +// window measured from S's send (last_sent_at) — so a window that has +// already elapsed decides immediately. +// - A branch on S whose condition, window or exits change while the contact +// waits is evaluated with the NEW definition at the next check. That check +// is at most conditionRecheckInterval away while the condition is OPEN, but +// can be later: rechecks are snapped into the campaign's send window (a +// Friday-evening recheck lands on Monday morning), an out-of-office deferral +// that ends later wins, and once a route is decided the next check is when +// its target falls due — which may be days out. A lengthened window keeps +// waiting; a shortened one decides at the next check. Until the target is +// actually SENT, a decided route is re-derived too, so changing S's exits +// in that interval re-routes the contact to the new exit. +// - Known edge: the recover-forward window. If S's routed target T was +// delivered but the cursor advance to T failed, and S's exit is changed +// BEFORE the retry runs, the retry routes to the NEW target U and sends it +// too — the contact receives both T and U. The window is one failed +// transaction plus an asynq retry (seconds); closing it would mean storing +// the route, which the design deliberately does not do. +// - A branch removed from S while the contact waits returns S to linear +// fall-through, and the successor still waits out its own delay from S's +// send (awaiting_condition_step is what keeps this true once the campaign +// has no branches left). +// - An exit whose target step is deleted becomes an end (the FK nulls it). +// Step deletes are draft-only, so this cannot happen to a running campaign. +// - Once the route out of S has been decided AND its target sent, the +// enrollment's cursor is on the target; later edits to S no longer matter. +const ( + // conditionRecheckInterval bounds how long an undecided condition goes + // unobserved. The route is taken the first time the advance runs after the + // deciding event, and the routed step's delay is measured from the event + // itself, so this only adds latency when that delay is shorter than the + // interval. A reply additionally nudges the enrollment (see + // NudgeEnrollmentAwaitingReply), so the one signal an operator expects to act + // on promptly does not wait for the interval. + conditionRecheckInterval = time.Hour + // routeDueTolerance absorbs the skew between the worker clock that scheduled + // an advance and the database clock that stamped last_sent_at, so an advance + // that fires on time is not bounced for being a few seconds early. + routeDueTolerance = time.Minute +) + +// usesGraphRouting reports whether an enrollment must be routed through the +// branch graph rather than the linear GetNextStep path. +func usesGraphRouting(branches []gen.SequenceStepBranch, b gen.GetStepEnrollmentBundleRow) bool { + return len(branches) > 0 || + (b.AwaitingConditionStep != nil && *b.AwaitingConditionStep == b.CurrentStep && b.CurrentStep > 0) +} + +// routeKind is what the send path does with this advance. +type routeKind int + +const ( + // routeSend: send target now. + routeSend routeKind = iota + // routeWait: nothing to send yet; look again at recheckAt. + routeWait + // routeEnd: the path has ended; complete the enrollment without a send. + routeEnd + // routeSkip: the campaign has no step to send at all (the linear path's + // ErrNoRows → Skip, preserved). + routeSkip +) + +// routeDecision is decideRoute's answer. +type routeDecision struct { + kind routeKind + target seqgraph.Step + recheckAt time.Time + // lastStep and nextDelay describe the route OUT of target once it is sent — + // what AdvanceStepCursor schedules next. They replace the linear path's + // "is there a step after this one" lookup. + lastStep bool + nextDelay int +} + +// routeInput is everything decideRoute reads. It is a value, and evidence is a +// function, so the whole decision is unit-testable without a database. +type routeInput struct { + graph seqgraph.Graph + // cursor is sequence_enrollments.current_step: the step_order last sent, 0 + // before the first send. + cursor int32 + // lastSentAt is when the cursor step was sent — the start of its window and + // the reference point for the successor's delay. + lastSentAt time.Time + now time.Time + window cadence.Window + // key seeds the window's humanization (the enrollment id), as it does for + // every other due-time computation on this path. + key string +} + +// evidenceFunc returns the earliest event of the branch's signal for the cursor +// step inside [start, start+window], or the zero time when there is none. +type evidenceFunc func(cursor seqgraph.Step, b seqgraph.Branch, start time.Time) (time.Time, error) + +// decideRoute routes one advance. See the package-level notes above for the +// rules; the order of the checks is: +// +// 1. No send yet → the entry step, now (the launch stagger scheduled it). +// 2. The route out of the cursor step: end, a fixed next step, or a condition. +// 3. A condition still open → wait until it can next change (bounded). +// 4. A decided route → wait until the target's delay has elapsed from the +// decision AND the campaign's window is open; then send. +func decideRoute(in routeInput, evidence evidenceFunc) (routeDecision, error) { + if in.cursor == 0 { + entry, ok := in.graph.Entry() + if !ok { + return routeDecision{kind: routeSkip}, nil + } + return in.sendDecision(entry), nil + } + if in.lastSentAt.IsZero() { + // current_step > 0 is only ever written together with last_sent_at, so + // this is a corrupted row. Failing loudly beats inventing a window start: + // a start of "now" would re-open the window on every advance and the + // condition would never close. + return routeDecision{}, fmt.Errorf("enrollment %s is past step %d but has no last_sent_at", in.key, in.cursor) + } + cursor, ok := in.graph.StepByOrder(in.cursor) + if !ok { + // The step this contact last received no longer exists. Step deletes are + // draft-only, so a running campaign cannot get here; ending the path is + // the answer that cannot send the wrong message. + return routeDecision{kind: routeEnd}, nil + } + + plan := in.graph.After(cursor.ID) + decidedAt := in.lastSentAt + var target seqgraph.Step + switch plan.Kind { + case seqgraph.PlanEnd: + return routeDecision{kind: routeEnd}, nil + case seqgraph.PlanNext: + target = plan.Next + case seqgraph.PlanAwait: + first, err := evidence(cursor, plan.Branch, in.lastSentAt) + if err != nil { + return routeDecision{}, err + } + verdict := seqgraph.Evaluate(plan.Branch.Condition, in.lastSentAt, plan.Branch.Window(), first, in.now) + if !verdict.Decided { + // Look again when the answer can next change: the deadline, or the + // recheck interval if that comes first. + return routeDecision{kind: routeWait, recheckAt: in.snap(minTime(verdict.At, in.now.Add(conditionRecheckInterval)))}, nil + } + next, ok := in.graph.Step(plan.Branch.Target(verdict)) + if !ok { + return routeDecision{kind: routeEnd}, nil + } + target, decidedAt = next, verdict.At + } + + due := decidedAt.Add(time.Duration(target.DelaySeconds) * time.Second) + if in.now.Before(due.Add(-routeDueTolerance)) || !in.window.Contains(in.now) { + return routeDecision{kind: routeWait, recheckAt: in.snap(maxTime(due, in.now))}, nil + } + return in.sendDecision(target), nil +} + +// sendDecision is a routeSend for target, with the route out of it resolved. +func (in routeInput) sendDecision(target seqgraph.Step) routeDecision { + d := routeDecision{kind: routeSend, target: target} + switch plan := in.graph.After(target.ID); plan.Kind { + case seqgraph.PlanEnd: + d.lastStep = true + case seqgraph.PlanNext: + d.nextDelay = int(plan.Next.DelaySeconds) + case seqgraph.PlanAwait: + // The first look at the condition: the deadline, or one recheck interval + // after the send if that is sooner. + d.nextDelay = int(min(plan.Branch.Window(), conditionRecheckInterval) / time.Second) + } + return d +} + +// snap moves t into the campaign's send window. A wait must END inside the +// window: the advance that wakes up may send immediately, and nothing later on +// this path re-checks the window. +func (in routeInput) snap(t time.Time) time.Time { + if at, err := in.window.Next(t, in.key); err == nil { + return at + } + return t +} + +func maxTime(a, b time.Time) time.Time { + if a.After(b) { + return a + } + return b +} + +func minTime(a, b time.Time) time.Time { + if a.Before(b) { + return a + } + return b +} + +// graphRouteResult is routeGraph's answer: the decision plus the full step row +// to send when there is one. +type graphRouteResult struct { + routeDecision + step gen.SequenceStep +} + +// routeGraph loads the campaign's graph and routes one advance through it. It +// applies the database-backed half of the rules decideRoute cannot see: the +// engagement evidence and the loop backstop. +func (c client) routeGraph(ctx context.Context, ws uuid.UUID, enrollmentID string, b gen.GetStepEnrollmentBundleRow, + branches []gen.SequenceStepBranch, sched cadence.Schedule, now time.Time) (graphRouteResult, error) { + steps, err := c.q.ListStepsByCampaign(ctx, gen.ListStepsByCampaignParams{CampaignID: b.CampaignID, WorkspaceID: ws}) + if err != nil { + return graphRouteResult{}, fmt.Errorf("list steps: %w", err) + } + win, err := sched.Compile() + if err != nil { + return graphRouteResult{}, fmt.Errorf("enrollment %s route: %w", enrollmentID, err) + } + g := sequencestep.BuildGraph(steps, branches) + d, err := decideRoute(routeInput{ + graph: g, cursor: b.CurrentStep, lastSentAt: b.LastSentAt.Time, now: now, window: win, key: enrollmentID, + }, func(cursor seqgraph.Step, br seqgraph.Branch, start time.Time) (time.Time, error) { + return c.firstEvidence(ctx, ws, b, cursor, br, start) + }) + if err != nil { + return graphRouteResult{}, err + } + if d.kind != routeSend { + return graphRouteResult{routeDecision: d}, nil + } + if b.CurrentStep > 0 && g.HasBranches() { + revisit, err := c.revisits(ctx, ws, b, b.CurrentStep, d.target) + if err != nil { + return graphRouteResult{}, err + } + if revisit { + // The save path refuses every loop, under a lock, so this is a + // backstop: without it a loop that got in anyway would recover-forward + // through ClaimAlreadySent step after step, forever, never sending + // and never finishing. End the path loudly instead. + slog.WarnContext(ctx, "sequence_graph_loop_detected", + "enrollment_id", enrollmentID, "campaign_id", b.CampaignID, "step_order", d.target.Order) + return graphRouteResult{routeDecision: routeDecision{kind: routeEnd}}, nil + } + } + for _, st := range steps { + if st.ID == d.target.ID { + return graphRouteResult{routeDecision: d, step: st}, nil + } + } + // Unreachable: target came from this same step list. + return graphRouteResult{}, fmt.Errorf("routed step %s vanished", d.target.ID) +} + +// revisits reports whether target was already sent EARLIER on this contact's +// path. Its deterministic send row existing is not enough — that is also the +// recover-forward case, where target was sent but the cursor advance to it did +// not commit — so the test compares it with the CURRENT step's own send row: a +// recover-forward row for target was created after the current step's, a loop's +// row before it (the loop visited target first). +// +// Both instants are sends.created_at, the same column stamped by the same +// database clock, so there is no skew to absorb and no tolerance — a tolerance +// would blind this to the loop that matters most, zero-delay steps seconds +// apart. (enrollment.last_sent_at would be the wrong reference: a recover-forward +// re-stamps it, so it can land after a genuinely-earlier visit.) +func (c client) revisits(ctx context.Context, ws uuid.UUID, b gen.GetStepEnrollmentBundleRow, cursorOrder int32, target seqgraph.Step) (bool, error) { + targetAt, found, err := c.stepSendCreatedAt(ctx, ws, b, target.Order) + if err != nil || !found { + return false, err + } + cursorAt, found, err := c.stepSendCreatedAt(ctx, ws, b, cursorOrder) + if err != nil || !found { + // No row for the step the contact is on cannot happen past step 0 (the + // cursor only moves after a claim); with nothing to compare against, + // the claim remains the delivery guard. + return false, err + } + return targetAt.Before(cursorAt), nil +} + +// stepSendCreatedAt reads when one step's deterministic send row was created; +// found=false when the row does not exist. +func (c client) stepSendCreatedAt(ctx context.Context, ws uuid.UUID, b gen.GetStepEnrollmentBundleRow, order int32) (time.Time, bool, error) { + created, err := c.q.StepSendCreatedAt(ctx, gen.StepSendCreatedAtParams{ + ID: deriveStepSendID(b.CampaignID, b.ContactID, int(order)), WorkspaceID: ws, + }) + if errors.Is(err, pgx.ErrNoRows) { + return time.Time{}, false, nil + } + if err != nil { + return time.Time{}, false, fmt.Errorf("step send lookup: %w", err) + } + return created.Time, true, nil +} + +// firstEvidence reads the earliest qualifying event for one condition. +// +// Opens and clicks are keyed on the cursor step's OWN deterministic send id, so +// they are "opened THIS step", never an earlier one; they count HUMAN events +// only (invariant 84). Replies span both legs of the conversation: the step went +// out as a sends row, but the answer lives in inbox_messages, matched on the +// enrollment's campaign and contact rather than on any one send. +func (c client) firstEvidence(ctx context.Context, ws uuid.UUID, b gen.GetStepEnrollmentBundleRow, + cursor seqgraph.Step, br seqgraph.Branch, start time.Time) (time.Time, error) { + end := start.Add(br.Window()) + var ( + at pgtype.Timestamptz + err error + ) + switch br.Condition.Signal() { + case seqgraph.SignalOpen, seqgraph.SignalClick: + kind := gen.TrackingEventKindOpen + if br.Condition.Signal() == seqgraph.SignalClick { + kind = gen.TrackingEventKindClick + } + at, err = c.q.FirstHumanTrackingEventAt(ctx, gen.FirstHumanTrackingEventAtParams{ + SendID: deriveStepSendID(b.CampaignID, b.ContactID, int(cursor.Order)), + WorkspaceID: ws, Kind: kind, WindowEnd: pgtype.Timestamptz{Time: end, Valid: true}, + }) + case seqgraph.SignalReply: + at, err = c.q.FirstInboundReplyAt(ctx, gen.FirstInboundReplyAtParams{ + WorkspaceID: ws, + CampaignID: pgtype.UUID{Bytes: b.CampaignID, Valid: true}, + ContactID: pgtype.UUID{Bytes: b.ContactID, Valid: true}, + WindowStart: pgtype.Timestamptz{Time: start, Valid: true}, + WindowEnd: pgtype.Timestamptz{Time: end, Valid: true}, + LabelKey: br.ReplyLabelKey, + }) + default: + return time.Time{}, fmt.Errorf("condition %q has no evidence to read", br.Condition) + } + if errors.Is(err, pgx.ErrNoRows) { + return time.Time{}, nil + } + if err != nil { + return time.Time{}, fmt.Errorf("read %s evidence: %w", br.Condition, err) + } + return at.Time, nil +} + +// routeStep loads the schedule the route needs (a wait must end inside the send +// window) and routes the advance. +func (c client) routeStep(ctx context.Context, ws uuid.UUID, enrollmentID string, b gen.GetStepEnrollmentBundleRow, + branches []gen.SequenceStepBranch) (graphRouteResult, error) { + sched, err := c.loadSchedule(ctx, ws, b.CampaignID, b.Timezone) + if err != nil { + return graphRouteResult{}, err + } + return c.routeGraph(ctx, ws, enrollmentID, b, branches, sched, time.Now()) +} + +// applyRoute carries out a route that sends nothing this advance, and returns +// the job the worker acts on. +// +// These are control-plane writes made while building a job, which the linear +// path does not do. They belong here rather than behind new worker-invoked +// coreapi methods because they are DECISIONS about committed data, not outcomes +// of a delivery: both are idempotent and guarded on status='active', a retried +// or raced advance recomputes the identical decision (the verdict cannot change +// once taken), and the remote transport serves this very function in the +// control plane, so a worker never needs a way to express either write. +func (c client) applyRoute(ctx context.Context, ws, eid uuid.UUID, enrollmentID string, + b gen.GetStepEnrollmentBundleRow, d routeDecision) (coreapi.StepSendJob, error) { + switch d.kind { + case routeEnd: + if err := c.enroll.FinishRoute(ctx, ws, eid); err != nil { + return coreapi.StepSendJob{}, fmt.Errorf("finish routed enrollment: %w", err) + } + return coreapi.StepSendJob{Skip: true}, nil + case routeWait: + // Never pull a due time EARLIER than one already stamped: a later + // next_due_at is an out-of-office deferral, and waking inside the stated + // absence would route — and possibly send — into it. + recheck := d.recheckAt + if b.NextDueAt.Valid && b.NextDueAt.Time.After(recheck) { + recheck = b.NextDueAt.Time + } + if err := c.enroll.AwaitCondition(ctx, ws, eid, recheck); err != nil { + return coreapi.StepSendJob{}, fmt.Errorf("await branch condition: %w", err) + } + return coreapi.StepSendJob{ + EnrollmentID: enrollmentID, WorkspaceID: ws.String(), + ConditionPending: true, RecheckAt: recheck, + // Skip rides along so a worker built before ConditionPending existed + // treats this job as inert instead of claiming a send with an empty + // job; the next_due_at stamped above lets the sweeper re-drive it. + Skip: true, + }, nil + default: + return coreapi.StepSendJob{Skip: true}, nil + } +} diff --git a/internal/coreapi/inprocess/branchroute_test.go b/internal/coreapi/inprocess/branchroute_test.go new file mode 100644 index 00000000..fdf4d094 --- /dev/null +++ b/internal/coreapi/inprocess/branchroute_test.go @@ -0,0 +1,303 @@ +package inprocess + +import ( + "errors" + "testing" + "time" + + "github.com/google/uuid" + + "github.com/inroad/inroad/internal/platform/cadence" + "github.com/inroad/inroad/internal/platform/db/gen" + "github.com/inroad/inroad/internal/platform/seqgraph" +) + +// alwaysOpen is a window that never blocks, so a test isolates the rule it is +// about from send-window snapping. +func alwaysOpen(t *testing.T) cadence.Window { + t.Helper() + var ws []cadence.SendWindow + for d := range 7 { + ws = append(ws, cadence.SendWindow{Weekday: d, StartMinute: 0, EndMinute: 24 * 60}) + } + w, err := cadence.Schedule{Timezone: "UTC", Windows: ws}.Compile() + if err != nil { + t.Fatal(err) + } + return w +} + +// businessHours is Mon–Fri 09:00–17:00 UTC. +func businessHours(t *testing.T) cadence.Window { + t.Helper() + w, err := cadence.DefaultSchedule("UTC").Compile() + if err != nil { + t.Fatal(err) + } + return w +} + +// routeSteps is a three-step campaign; step 3 has a 1h delay, step 2 a 2h one. +func routeSteps() (ids [3]uuid.UUID, steps []seqgraph.Step) { + for i := range ids { + ids[i] = uuid.New() + } + return ids, []seqgraph.Step{ + {ID: ids[0], Order: 1}, + {ID: ids[1], Order: 2, DelaySeconds: 7200}, + {ID: ids[2], Order: 3, DelaySeconds: 3600}, + } +} + +// noEvidence is an evidence source that has seen nothing. +func noEvidence(seqgraph.Step, seqgraph.Branch, time.Time) (time.Time, error) { + return time.Time{}, nil +} + +// eventAt is an evidence source that saw the signal at t. +func eventAt(t time.Time) evidenceFunc { + return func(seqgraph.Step, seqgraph.Branch, time.Time) (time.Time, error) { return t, nil } +} + +// A Wednesday, mid-morning UTC — inside businessHours. +var routeSentAt = time.Date(2026, 9, 23, 10, 0, 0, 0, time.UTC) + +func TestDecideRouteEntryStepSendsNow(t *testing.T) { + ids, steps := routeSteps() + g := seqgraph.New(steps, []seqgraph.Branch{{StepID: ids[0], Condition: seqgraph.Opened, WithinDays: 2, Yes: ids[2], No: ids[1]}}) + d, err := decideRoute(routeInput{graph: g, cursor: 0, now: routeSentAt, window: alwaysOpen(t), key: "e"}, noEvidence) + if err != nil { + t.Fatal(err) + } + if d.kind != routeSend || d.target.ID != ids[0] { + t.Fatalf("entry = %+v", d) + } + // The first look at step 1's condition is one recheck interval out (the + // 2-day window is longer), and the enrollment is not on its last step. + if d.lastStep || d.nextDelay != int(conditionRecheckInterval/time.Second) { + t.Fatalf("after step 1: lastStep=%v nextDelay=%d", d.lastStep, d.nextDelay) + } +} + +func TestDecideRouteNoStepsSkips(t *testing.T) { + d, err := decideRoute(routeInput{graph: seqgraph.New(nil, nil), now: routeSentAt, window: alwaysOpen(t)}, noEvidence) + if err != nil || d.kind != routeSkip { + t.Fatalf("empty campaign = %+v, %v", d, err) + } +} + +func TestDecideRouteConditionPendingWaitsBoundedByRecheck(t *testing.T) { + ids, steps := routeSteps() + g := seqgraph.New(steps, []seqgraph.Branch{{StepID: ids[0], Condition: seqgraph.Opened, WithinDays: 2, Yes: ids[2], No: ids[1]}}) + now := routeSentAt.Add(10 * time.Minute) + d, err := decideRoute(routeInput{graph: g, cursor: 1, lastSentAt: routeSentAt, now: now, window: alwaysOpen(t), key: "e"}, noEvidence) + if err != nil { + t.Fatal(err) + } + if d.kind != routeWait { + t.Fatalf("want wait, got %+v", d) + } + // Snapped into the window, which only ever moves an instant later, by under + // two minutes of humanization. + want := now.Add(conditionRecheckInterval) + if d.recheckAt.Before(want) || d.recheckAt.After(want.Add(2*time.Minute)) { + t.Fatalf("recheck = %v, want ~%v", d.recheckAt, want) + } +} + +// Near the deadline the recheck is the deadline, not a full interval later. +func TestDecideRouteConditionPendingRecheckStopsAtDeadline(t *testing.T) { + ids, steps := routeSteps() + g := seqgraph.New(steps, []seqgraph.Branch{{StepID: ids[0], Condition: seqgraph.Replied, WithinDays: 1, Yes: ids[2], No: ids[1]}}) + deadline := routeSentAt.Add(24 * time.Hour) + now := deadline.Add(-10 * time.Minute) + d, err := decideRoute(routeInput{graph: g, cursor: 1, lastSentAt: routeSentAt, now: now, window: alwaysOpen(t), key: "e"}, noEvidence) + if err != nil { + t.Fatal(err) + } + if d.kind != routeWait || d.recheckAt.Before(deadline) || d.recheckAt.After(deadline.Add(2*time.Minute)) { + t.Fatalf("recheck = %+v, want ~deadline %v", d, deadline) + } +} + +// The positive event routes YES as soon as it is seen, and the target's delay +// counts from the event — not from the send, and not from when we noticed. +func TestDecideRouteEarlyEventRoutesYesWithDelayFromEvent(t *testing.T) { + ids, steps := routeSteps() + g := seqgraph.New(steps, []seqgraph.Branch{{StepID: ids[0], Condition: seqgraph.Opened, WithinDays: 3, Yes: ids[2], No: ids[1]}}) + opened := routeSentAt.Add(30 * time.Minute) + in := routeInput{graph: g, cursor: 1, lastSentAt: routeSentAt, window: alwaysOpen(t), key: "e"} + + // 40 minutes after the open: step 3's 1h delay has not elapsed yet. + in.now = opened.Add(40 * time.Minute) + d, err := decideRoute(in, eventAt(opened)) + if err != nil { + t.Fatal(err) + } + due := opened.Add(time.Hour) + if d.kind != routeWait || d.recheckAt.Before(due) || d.recheckAt.After(due.Add(2*time.Minute)) { + t.Fatalf("before the yes step is due: %+v, want wait until ~%v", d, due) + } + + // Once due: send step 3 (the YES exit). Step 3 has no branch and is last. + in.now = due.Add(time.Minute) + d, err = decideRoute(in, eventAt(opened)) + if err != nil { + t.Fatal(err) + } + if d.kind != routeSend || d.target.ID != ids[2] || !d.lastStep { + t.Fatalf("yes route = %+v", d) + } +} + +func TestDecideRouteWindowCloseRoutesNo(t *testing.T) { + ids, steps := routeSteps() + g := seqgraph.New(steps, []seqgraph.Branch{{StepID: ids[0], Condition: seqgraph.Clicked, WithinDays: 1, Yes: ids[2], No: ids[1]}}) + // Step 2's delay (2h) counts from the deadline. + now := routeSentAt.Add(24*time.Hour + 2*time.Hour + time.Minute) + d, err := decideRoute(routeInput{graph: g, cursor: 1, lastSentAt: routeSentAt, now: now, window: alwaysOpen(t), key: "e"}, noEvidence) + if err != nil { + t.Fatal(err) + } + if d.kind != routeSend || d.target.ID != ids[1] { + t.Fatalf("no route = %+v", d) + } + // Step 2 has no branch: it falls through to step 3 with step 3's delay. + if d.lastStep || d.nextDelay != 3600 { + t.Fatalf("after step 2: lastStep=%v nextDelay=%d", d.lastStep, d.nextDelay) + } +} + +// A negated condition is decided NO by the event, immediately. +func TestDecideRouteNotRepliedDecidedByReply(t *testing.T) { + ids, steps := routeSteps() + g := seqgraph.New(steps, []seqgraph.Branch{{StepID: ids[0], Condition: seqgraph.NotReplied, WithinDays: 5, Yes: ids[1]}}) + replied := routeSentAt.Add(time.Hour) + d, err := decideRoute(routeInput{graph: g, cursor: 1, lastSentAt: routeSentAt, now: replied.Add(time.Minute), + window: alwaysOpen(t), key: "e"}, eventAt(replied)) + if err != nil { + t.Fatal(err) + } + // The NO exit is unset: the path ends, without waiting out the 5 days. + if d.kind != routeEnd { + t.Fatalf("replied on a not_replied branch with no NO exit = %+v", d) + } +} + +func TestDecideRouteAlwaysToEndFinishes(t *testing.T) { + ids, steps := routeSteps() + g := seqgraph.New(steps, []seqgraph.Branch{{StepID: ids[1], Condition: seqgraph.Always}}) + d, err := decideRoute(routeInput{graph: g, cursor: 2, lastSentAt: routeSentAt, now: routeSentAt.Add(time.Hour), + window: alwaysOpen(t), key: "e"}, noEvidence) + if err != nil || d.kind != routeEnd { + t.Fatalf("always -> end = %+v, %v", d, err) + } +} + +// A decided route may not send outside the campaign's window: it waits for the +// window to open, even though the target is overdue. +func TestDecideRouteWaitsForWindow(t *testing.T) { + ids, steps := routeSteps() + g := seqgraph.New(steps, []seqgraph.Branch{{StepID: ids[0], Condition: seqgraph.Opened, WithinDays: 1, Yes: ids[2], No: ids[1]}}) + opened := routeSentAt.Add(time.Hour) + saturday := time.Date(2026, 9, 26, 12, 0, 0, 0, time.UTC) + d, err := decideRoute(routeInput{graph: g, cursor: 1, lastSentAt: routeSentAt, now: saturday, + window: businessHours(t), key: "e"}, eventAt(opened)) + if err != nil { + t.Fatal(err) + } + monday := time.Date(2026, 9, 28, 9, 0, 0, 0, time.UTC) + if d.kind != routeWait || d.recheckAt.Before(monday) || d.recheckAt.After(monday.Add(2*time.Minute)) { + t.Fatalf("weekend route = %+v, want wait until ~%v", d, monday) + } +} + +// The mid-flight rule for a DELETED branch: the enrollment falls through to the +// linear successor, which still honours its own delay from the last send rather +// than going out at the recheck time the condition had scheduled. +func TestDecideRouteBranchRemovedMidWaitHonoursDelay(t *testing.T) { + _, steps := routeSteps() + g := seqgraph.New(steps, nil) // the only branch was deleted + in := routeInput{graph: g, cursor: 1, lastSentAt: routeSentAt, window: alwaysOpen(t), key: "e"} + + in.now = routeSentAt.Add(time.Hour) // a recheck the old condition scheduled + d, err := decideRoute(in, noEvidence) + if err != nil { + t.Fatal(err) + } + due := routeSentAt.Add(2 * time.Hour) // step 2's delay + if d.kind != routeWait || d.recheckAt.Before(due) { + t.Fatalf("fall-through before its delay = %+v, want wait until %v", d, due) + } + + in.now = due + if d, err = decideRoute(in, noEvidence); err != nil || d.kind != routeSend || d.target.Order != 2 { + t.Fatalf("fall-through once due = %+v, %v", d, err) + } +} + +// An advance that fires on schedule is never bounced by the few seconds of skew +// between the worker's clock and the database's last_sent_at. +func TestDecideRouteToleratesClockSkew(t *testing.T) { + ids, steps := routeSteps() + g := seqgraph.New(steps, []seqgraph.Branch{{StepID: ids[0], Condition: seqgraph.Always, Yes: ids[1]}}) + due := routeSentAt.Add(2 * time.Hour) + d, err := decideRoute(routeInput{graph: g, cursor: 1, lastSentAt: routeSentAt, now: due.Add(-5 * time.Second), + window: alwaysOpen(t), key: "e"}, noEvidence) + if err != nil || d.kind != routeSend { + t.Fatalf("5s early = %+v, %v", d, err) + } +} + +func TestDecideRouteDeletedCursorStepEnds(t *testing.T) { + _, steps := routeSteps() + g := seqgraph.New(steps, nil) + d, err := decideRoute(routeInput{graph: g, cursor: 9, lastSentAt: routeSentAt, now: routeSentAt, window: alwaysOpen(t)}, noEvidence) + if err != nil || d.kind != routeEnd { + t.Fatalf("cursor on a vanished step = %+v, %v", d, err) + } +} + +func TestDecideRouteMissingLastSentIsAnError(t *testing.T) { + _, steps := routeSteps() + _, err := decideRoute(routeInput{graph: seqgraph.New(steps, nil), cursor: 1, now: routeSentAt, window: alwaysOpen(t)}, noEvidence) + if err == nil { + t.Fatal("a cursor past step 0 with no last_sent_at must fail loudly") + } +} + +func TestDecideRouteEvidenceErrorPropagates(t *testing.T) { + ids, steps := routeSteps() + g := seqgraph.New(steps, []seqgraph.Branch{{StepID: ids[0], Condition: seqgraph.Opened, WithinDays: 1}}) + boom := errors.New("db down") + _, err := decideRoute(routeInput{graph: g, cursor: 1, lastSentAt: routeSentAt, now: routeSentAt, window: alwaysOpen(t)}, + func(seqgraph.Step, seqgraph.Branch, time.Time) (time.Time, error) { return time.Time{}, boom }) + if !errors.Is(err, boom) { + t.Fatalf("evidence error = %v, want %v", err, boom) + } +} + +// Linear campaigns never reach the graph router: no branches and no condition +// wait means usesGraphRouting is false and localStepSendJob runs the original +// GetNextStep path. That is the whole of the "linear campaigns are unchanged" +// guarantee on the send path, so it is pinned here explicitly. +func TestUsesGraphRoutingOnlyForBranchedOrParkedEnrollments(t *testing.T) { + two, three := int32(2), int32(3) + cases := []struct { + name string + branches []gen.SequenceStepBranch + bundle gen.GetStepEnrollmentBundleRow + want bool + }{ + {"linear, never waited", nil, gen.GetStepEnrollmentBundleRow{CurrentStep: 2}, false}, + {"linear, fresh enrollment", nil, gen.GetStepEnrollmentBundleRow{CurrentStep: 0}, false}, + {"linear, waited at an EARLIER step", nil, gen.GetStepEnrollmentBundleRow{CurrentStep: 3, AwaitingConditionStep: &two}, false}, + {"branch deleted while waiting here", nil, gen.GetStepEnrollmentBundleRow{CurrentStep: 3, AwaitingConditionStep: &three}, true}, + {"campaign has a branch", []gen.SequenceStepBranch{{Condition: "always"}}, gen.GetStepEnrollmentBundleRow{}, true}, + } + for _, tc := range cases { + if got := usesGraphRouting(tc.branches, tc.bundle); got != tc.want { + t.Errorf("%s: usesGraphRouting = %v, want %v", tc.name, got, tc.want) + } + } +} diff --git a/internal/coreapi/inprocess/inboxpoll.go b/internal/coreapi/inprocess/inboxpoll.go index 271dcb7a..617112ff 100644 --- a/internal/coreapi/inprocess/inboxpoll.go +++ b/internal/coreapi/inprocess/inboxpoll.go @@ -191,7 +191,18 @@ func (c client) localRecordReplyClass(ctx context.Context, enrollmentID, workspa if err != nil { return err } - return c.recordReplyClass(ctx, eid, ws, class, source, confidence) + if err := c.recordReplyClass(ctx, eid, ws, class, source, confidence); err != nil { + return err + } + // A reply that did not stop the sequence can still decide a branch + // condition on the step the contact is waiting at ("replied within N + // days"). Pull that enrollment's due time to now so the next sweep routes it + // instead of leaving it for the next hourly recheck. A no-op for every + // enrollment not waiting on a reply condition, and for automated replies, + // which are the ones that defer an enrollment (see the query). + return c.q.NudgeEnrollmentAwaitingReply(ctx, gen.NudgeEnrollmentAwaitingReplyParams{ + ID: eid, WorkspaceID: ws, ReplyClass: class, + }) } // MarkUnsubscribed suppresses the address and, when the reply belongs to an diff --git a/internal/coreapi/inprocess/stepsendjob.go b/internal/coreapi/inprocess/stepsendjob.go index 931f112a..f8220d25 100644 --- a/internal/coreapi/inprocess/stepsendjob.go +++ b/internal/coreapi/inprocess/stepsendjob.go @@ -138,11 +138,13 @@ func decodeCustom(b []byte) map[string]string { // threading computes the In-Reply-To / References headers for the step about to // send. Empty for step 1. For later steps it prefers the immediately-preceding // sent message (proper chain), falling back to the stored thread root. -func (c client) threading(ctx context.Context, order int, campaignID, contactID uuid.UUID, threadRootID string) (inReplyTo, references string) { +func (c client) threading(ctx context.Context, ws uuid.UUID, order int, campaignID, contactID uuid.UUID, threadRootID string) (inReplyTo, references string) { if order <= 1 { return "", "" } - prior, err := c.q.LatestSentForContact(ctx, gen.LatestSentForContactParams{CampaignID: campaignID, ContactID: contactID}) + prior, err := c.q.LatestSentForContact(ctx, gen.LatestSentForContactParams{ + CampaignID: campaignID, ContactID: contactID, WorkspaceID: ws, + }) if err == nil && prior.MessageID != "" { return prior.MessageID, strings.TrimSpace(prior.ReferencesHeader + " " + prior.MessageID) } @@ -196,7 +198,12 @@ func (c client) newLeadLimitReached(ctx context.Context, ws, campaignID uuid.UUI } // localStepSendJob resolves the enrollment's next due step and builds the send -// job. Read-only: creates no rows. workspaceID is pinned in the SQL WHERE +// job. It creates no sends row — the claim does that — so a suppressed or +// capped step leaves no orphan. It is not strictly read-only, though: resolving +// a sender pins a pool mailbox to the enrollment, and on a branched campaign +// routing may complete the enrollment (a path that ends) or park it until a +// condition can next change (see applyRoute). Every such write is guarded on +// status='active' and idempotent. workspaceID is pinned in the SQL WHERE // (defense in depth on the unguessable enrollment UUID). func (c client) localStepSendJob(ctx context.Context, enrollmentID, workspaceID string) (coreapi.StepSendJob, error) { eid, err := uuid.Parse(enrollmentID) @@ -231,17 +238,47 @@ func (c client) localStepSendJob(ctx context.Context, enrollmentID, workspaceID }, nil } - // Resolve the next step by order rather than current_step+1: DeleteStep does - // not renumber, so orders can have gaps (e.g. {1,3}). GetNextStep skips gaps; - // ErrNoRows means the cursor is at/after the last step → done. - step, err := c.q.GetNextStep(ctx, gen.GetNextStepParams{ - CampaignID: b.CampaignID, WorkspaceID: ws, StepOrder: b.CurrentStep, - }) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - return coreapi.StepSendJob{Skip: true}, nil + // A campaign with branches (or an enrollment still parked by one) is routed + // through the graph; every other campaign takes the linear path below exactly + // as it did before branching existed. See branchroute.go. + branches, err := c.q.ListBranchesByCampaign(ctx, gen.ListBranchesByCampaignParams{CampaignID: b.CampaignID, WorkspaceID: ws}) + if err != nil { + return coreapi.StepSendJob{}, fmt.Errorf("list branches: %w", err) + } + var ( + step gen.SequenceStep + route *routeDecision + ) + if usesGraphRouting(branches, b) { + // A campaign that is not running must not have its enrollments moved at + // all — not finished by a path that ends, nor parked by a condition — + // only held, exactly as the linear path holds them below. So on the graph + // path the gate comes BEFORE routing. (On the linear path it stays after + // GetNextStep, where it has always been, so linear behaviour is unchanged.) + if b.CampaignStatus != string(campaign.StatusRunning) { + return c.campaignPausedJob(ctx, ws, enrollmentID, b) + } + routed, err := c.routeStep(ctx, ws, enrollmentID, b, branches) + if err != nil { + return coreapi.StepSendJob{}, err + } + if routed.kind != routeSend { + return c.applyRoute(ctx, ws, eid, enrollmentID, b, routed.routeDecision) + } + step, route = routed.step, &routed.routeDecision + } else { + // Resolve the next step by order rather than current_step+1: DeleteStep + // does not renumber, so orders can have gaps (e.g. {1,3}). GetNextStep + // skips gaps; ErrNoRows means the cursor is at/after the last step → done. + step, err = c.q.GetNextStep(ctx, gen.GetNextStepParams{ + CampaignID: b.CampaignID, WorkspaceID: ws, StepOrder: b.CurrentStep, + }) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return coreapi.StepSendJob{Skip: true}, nil + } + return coreapi.StepSendJob{}, err } - return coreapi.StepSendJob{}, err } nextOrder := int(step.StepOrder) @@ -259,17 +296,7 @@ func (c client) localStepSendJob(ctx context.Context, enrollmentID, workspaceID // comes FIRST because it is already in the bundle, so it costs no query, while // campaignLimitReached runs a COUNT. if b.CampaignStatus != string(campaign.StatusRunning) { - // The schedule travels for the same reason as the daily-limit branch: a - // deferred retry SENDS as soon as it runs, so the worker has to wake inside - // the campaign's window. - sched, serr := c.loadSchedule(ctx, ws, b.CampaignID, b.Timezone) - if serr != nil { - return coreapi.StepSendJob{}, serr - } - return coreapi.StepSendJob{ - EnrollmentID: enrollmentID, WorkspaceID: ws.String(), CampaignPaused: true, - Schedule: sched, - }, nil + return c.campaignPausedJob(ctx, ws, enrollmentID, b) } // The campaign-wide daily limit, stacked on top of the per-mailbox caps: the @@ -329,17 +356,23 @@ func (c client) localStepSendJob(ctx context.Context, enrollmentID, workspaceID } // Is there a step after this one? Its existence decides last-step; its delay - // is the cadence gap to the following send. One query answers both. - after, err := c.q.GetNextStep(ctx, gen.GetNextStepParams{ - CampaignID: b.CampaignID, WorkspaceID: ws, StepOrder: step.StepOrder, - }) - lastStep := errors.Is(err, pgx.ErrNoRows) - if err != nil && !lastStep { - return coreapi.StepSendJob{}, err - } - nextDelay := 0 - if !lastStep { - nextDelay = int(after.DelaySeconds) + // is the cadence gap to the following send. On the linear path one query + // answers both; a routed step already carries the answer from the graph (the + // next step, the end, or the first look at its condition). + lastStep, nextDelay := false, 0 + if route != nil { + lastStep, nextDelay = route.lastStep, route.nextDelay + } else { + after, err := c.q.GetNextStep(ctx, gen.GetNextStepParams{ + CampaignID: b.CampaignID, WorkspaceID: ws, StepOrder: step.StepOrder, + }) + lastStep = errors.Is(err, pgx.ErrNoRows) + if err != nil && !lastStep { + return coreapi.StepSendJob{}, err + } + if !lastStep { + nextDelay = int(after.DelaySeconds) + } } // Thread subject is only needed to build "Re:hello
' }, + TWO_STEPS[1]!, + { id: 'step-3', step_order: 3, delay_seconds: 86400, subject: 'Last call', body_text: 'closing', body_html: '' }, + ] + const { branchWrites } = await mockApi(page, three) + await signIn(page) + await page.goto(`/app/campaigns/${CAMPAIGN_ID}/steps`) + const canvas = page.getByRole('region', { name: 'Sequence flow' }) + await expect(canvas.getByText('Last call')).toBeVisible() + + await canvas.getByRole('button', { name: 'Add a condition after step 1' }).click() + const panel = page.getByRole('complementary', { name: 'Condition after step 1' }) + await panel.getByLabel('If they…').selectOption('opened') + await panel.getByLabel('Within (days)').fill('3') + await panel.getByLabel('Yes — go to').selectOption('step-3') + await panel.getByLabel('No — go to').selectOption('step-2') + await panel.getByRole('button', { name: 'Add condition' }).click() + + await expect.poll(() => branchWrites.at(-1)).toEqual({ + condition: 'opened', + within_days: 3, + reply_label_key: null, + yes_step_id: 'step-3', + no_step_id: 'step-2', + // A new condition is create-only. + expected_updated_at: null, + }) + + // The diamond, in words, right after step 1 — and it took focus. + const condition = canvas.getByRole('button', { name: 'Edit the condition after step 1: Opened within 3 days' }) + await expect(condition).toBeVisible() + await expect(condition).toBeFocused() + await expect(canvas.locator('.react-flow__node[data-id="cond:step-1"] polygon')).toHaveCount(1) + // Labelled exits, each an edge to the step it was set to. + const node = canvas.locator('.react-flow__node[data-id="cond:step-1"]') + await expect(node.getByText('Yes', { exact: true })).toBeVisible() + await expect(node.getByText('No', { exact: true })).toBeVisible() + await expect(canvas.getByTestId('rf__edge-cond:step-1:yes->step-3')).toHaveCount(1) + await expect(canvas.getByTestId('rf__edge-cond:step-1:no->step-2')).toHaveCount(1) + + // Dragging the Yes exit onto step 2 re-routes it — a real pointer drag. + const from = node.locator('.react-flow__handle.source').first() + const to = canvas.locator('.react-flow__node[data-id="step-2"] .react-flow__handle.target') + const [a, b] = await Promise.all([from.boundingBox(), to.boundingBox()]) + if (!a || !b) throw new Error('handles not rendered') + await page.mouse.move(a.x + a.width / 2, a.y + a.height / 2) + await page.mouse.down() + await page.mouse.move(b.x + b.width / 2, b.y + b.height / 2, { steps: 12 }) + await page.mouse.up() + // The drag is conditional on the version the canvas showed — echoed verbatim. + await expect.poll(() => branchWrites.at(-1)).toMatchObject({ + yes_step_id: 'step-2', + no_step_id: 'step-2', + expected_updated_at: '2026-09-23T00:00:00Z', + }) +}) diff --git a/web/src/components/shared/flow/__tests__/flow-layout.test.ts b/web/src/components/shared/flow/__tests__/flow-layout.test.ts index 0bc1b12d..c501d582 100644 --- a/web/src/components/shared/flow/__tests__/flow-layout.test.ts +++ b/web/src/components/shared/flow/__tests__/flow-layout.test.ts @@ -1,5 +1,6 @@ import { renderHook } from '@testing-library/react' -import type { Edge, Node } from '@xyflow/react' +import type { Node, XYPosition } from '@xyflow/react' +import type { FlowEdgeType } from '../flow-edge' import { describe, expect, test } from 'vitest' import { layoutFlow, useFlowLayout } from '../flow-layout' import type { FlowNodeSize } from '../node-registry' @@ -16,17 +17,58 @@ const sizeOf = (type: string | undefined): FlowNodeSize => { } const node = (id: string, type: string): Node => ({ id, type, position: { x: 0, y: 0 }, data: {} }) -const edge = (source: string, target: string, sourceHandle?: string): Edge => ({ - id: `${source}->${target}`, +const edge = (source: string, target: string, sourceHandle?: string): FlowEdgeType => ({ + id: `${source}:${sourceHandle ?? ''}->${target}`, source, target, sourceHandle, }) +type Laid = ReturnTypeb
' }, + { id: 'v-2', step_id: 's-1', label: 'C', weight: 50, subject: '', body_text: '', body_html: 'c
' }, + ]) + } + if (url.endsWith('/variants')) return jsonResponse([]) + if (url.endsWith('/steps/reorder')) { + if (server.reorderFails) return server.reorderFails + const { step_ids } = body as { step_ids: string[] } + server.steps = renumber(step_ids.flatMap((id) => server.steps.filter((step) => step.id === id))) + return jsonResponse(server.steps) + } + if (/\/steps\/[^/]+$/.test(url) && method === 'PUT') return jsonResponse(server.steps[0]) + if (/\/steps\/[^/]+$/.test(url) && method === 'DELETE') { + const id = url.split('/').at(-1) + server.steps = renumber(server.steps.filter((step) => step.id !== id)) + return new Response(null, { status: 204 }) + } + if (url.endsWith('/steps') && method === 'POST') { + const created: FakeStep = { + id: 's-new', + step_order: server.steps.length + 1, + delay_seconds: 0, + subject: (body as { subject?: string }).subject ?? '', + } + server.steps = [...server.steps, created] + return jsonResponse(created) + } + if (url.endsWith('/steps')) { + if (server.listGate) await server.listGate + return jsonResponse(server.steps) + } + if (url.endsWith(`/campaigns/${server.campaign.id}`)) return jsonResponse(server.campaign) + return jsonResponse({ error: `unhandled ${method} ${url}` }, 404) + }), + ) + return server +} + +export function lastRequest( + server: FakeSequenceServer, + predicate: (request: CapturedRequest) => boolean, +): CapturedRequest | undefined { + return [...server.requests].reverse().find(predicate) +} + +/** The bodies sent to paths ending in `suffix` (query strings aside), in order. */ +export function bodiesTo(server: FakeSequenceServer, suffix: string, method?: string): unknown[] { + return requestsTo(server, suffix, method).map((request) => request.body) +} + +/** The requests sent to paths ending in `suffix` (query strings aside), in order. */ +export function requestsTo(server: FakeSequenceServer, suffix: string, method?: string): CapturedRequest[] { + return server.requests.filter( + (request) => new URL(request.url).pathname.endsWith(suffix) && (method === undefined || request.method === method), + ) +} + +/** A promise and the function that settles it, for holding a response back. */ +export function gate(): { promise: PromiseHi
' }, + { id: 's-2', step_order: 2, delay_seconds: 2 * 86400, subject: 'Bump' }, + { id: 's-3', step_order: 3, delay_seconds: 86400, subject: 'Case study' }, + ]) + server.replyLabels = [ + { key: 'interested', label: 'Interested', stops_enrollment: true }, + { key: 'question', label: 'Question', stops_enrollment: false }, + ] + canvasProps.current = undefined +}) + +afterEach(() => { + vi.unstubAllGlobals() + vi.restoreAllMocks() +}) + +function branch(stepId: string, fields: PartialHi
' }, { id: 's-2', step_order: 2, delay_seconds: 259200, subject: '', body_text: 'Following up now' }, { id: 's-3', step_order: 3, delay_seconds: 86400, subject: 'Last call' }, - ] - reorderFails = null - listGate = null + ]) canvasProps.current = undefined - - vi.stubGlobal( - 'fetch', - vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { - const isRequest = input instanceof Request - const url = isRequest ? input.url : typeof input === 'string' ? input : (input as URL).href - const method = (isRequest ? input.method : (init?.method ?? 'GET')).toUpperCase() - const text = isRequest ? await input.clone().text() : typeof init?.body === 'string' ? init.body : '' - const body = text ? (JSON.parse(text) as unknown) : undefined - requests.push({ method, url, body }) - - if (url.endsWith('/steps/s-1/variants')) { - return jsonResponse([ - { id: 'v-1', step_id: 's-1', label: 'B', weight: 50, subject: '', body_text: '', body_html: '' }, - { id: 'v-2', step_id: 's-1', label: 'C', weight: 50, subject: '', body_text: '', body_html: '' }, - ]) - } - if (url.endsWith('/variants')) return jsonResponse([]) - if (url.endsWith('/steps/reorder')) { - if (reorderFails) return reorderFails - const { step_ids } = body as { step_ids: string[] } - steps = renumber(step_ids.flatMap((id) => steps.filter((step) => step.id === id))) - return jsonResponse(steps) - } - if (/\/steps\/[^/]+$/.test(url) && method === 'PUT') return jsonResponse(steps[0]) - if (/\/steps\/[^/]+$/.test(url) && method === 'DELETE') { - const id = url.split('/').at(-1) - steps = renumber(steps.filter((step) => step.id !== id)) - return new Response(null, { status: 204 }) - } - if (url.endsWith('/steps') && method === 'POST') { - const created: Step = { - id: 's-new', - step_order: steps.length + 1, - delay_seconds: 0, - subject: (body as { subject?: string }).subject ?? '', - } - steps = [...steps, created] - return jsonResponse(created) - } - if (listGate) await listGate - return jsonResponse(steps) - }), - ) }) afterEach(() => { @@ -128,10 +64,7 @@ afterEach(() => { vi.restoreAllMocks() }) -function lastRequest(predicate: (r: CapturedRequest) => boolean): CapturedRequest | undefined { - return [...requests].reverse().find(predicate) -} -const reorderBodies = () => requests.filter((r) => r.url.endsWith('/steps/reorder')).map((r) => r.body) +const reorderBodies = () => bodiesTo(server, '/steps/reorder') async function renderCanvas(status = 'draft') { renderWithProviders(+ This sequence has conditions. The list shows steps in order only — switch to Flow to see where each path goes. +
+ )} {isDraft && adding && (