diff --git a/api/openapi.yaml b/api/openapi.yaml index 1d5bbb92..fe515f1a 100644 --- a/api/openapi.yaml +++ b/api/openapi.yaml @@ -2316,6 +2316,16 @@ paths: a step - including a loop closed by another step's linear fall-through - and with 422 (code unknown_target) when an exit names a step outside this campaign. + + + Optimistic concurrency. Without expected_updated_at the write replaces + whatever branch the step has (last writer wins). With it, the write + applies only if the step is still as the client last saw it - null + means "the step has no branch" (create-only), a timestamp means "the + branch's updated_at is still exactly this". Otherwise nothing is written + and the response is 409 (code branch_changed) carrying the branch as it + is now in current (null if the step has none). The precondition is + checked after the body is validated. requestBody: required: true content: { application/json: { schema: { $ref: '#/components/schemas/StepBranchRequest' } } } @@ -2327,6 +2337,9 @@ paths: '401': { description: Unauthorized } '403': { description: "Insufficient scope (campaigns:write)" } '404': { description: Campaign or step not found } + '409': + description: "expected_updated_at no longer holds - the branch was created, replaced or removed since it was read (code branch_changed, with current)" + content: { application/json: { schema: { $ref: '#/components/schemas/BranchValidationError' } } } '422': description: "The graph refuses the edit (codes cycle, unknown_target)" content: { application/json: { schema: { $ref: '#/components/schemas/BranchValidationError' } } } @@ -2336,15 +2349,32 @@ paths: security: [{ bearerAuth: [] }] description: >- Remove the step's branch, returning it to linear fall-through (the next - step by step_order). Idempotent. Allowed on a running campaign; a contact - waiting on the removed condition proceeds to the fall-through step, which - still waits out its own delay_seconds from this step's send. Refused with - 422 (code cycle) if the restored fall-through would close a loop. + step by step_order). Idempotent without expected_updated_at. Allowed on a + running campaign; a contact waiting on the removed condition proceeds to + the fall-through step, which still waits out its own delay_seconds from + this step's send. Refused with 422 (code cycle) if the restored + fall-through would close a loop. + + + With expected_updated_at the branch is removed only if its updated_at + is still exactly that value; otherwise 409 (code branch_changed) with + the branch as it is now in current - null when someone else already + removed it, which a client may treat as done. + parameters: + - name: expected_updated_at + in: query + required: false + description: "The branch's updated_at exactly as this API returned it (URL-encoded). Omit for an unconditional delete." + schema: { type: string, format: date-time } responses: '204': { description: Removed (or there was none) } + '400': { description: "expected_updated_at is not a timestamp this API could have returned" } '401': { description: Unauthorized } '403': { description: "Insufficient scope (campaigns:write)" } '404': { description: Campaign or step not found } + '409': + description: "expected_updated_at no longer holds (code branch_changed, with current)" + content: { application/json: { schema: { $ref: '#/components/schemas/BranchValidationError' } } } '422': description: The restored fall-through would close a loop (code cycle) content: { application/json: { schema: { $ref: '#/components/schemas/BranchValidationError' } } } @@ -5172,7 +5202,7 @@ components: reply_label_key: { type: string, nullable: true, description: "replied / not_replied only: count only replies classified with this reply label key (always a label that does not stop the sequence)" } yes_step_id: { type: string, format: uuid, nullable: true, description: "Where a true condition (or always) goes; null ends the path" } no_step_id: { type: string, format: uuid, nullable: true, description: "Where a false condition goes; null ends the path; always null for always" } - updated_at: { type: string, format: date-time } + updated_at: { type: string, format: date-time, description: "When the branch last changed, at microsecond precision. Also its concurrency token - send it back verbatim as expected_updated_at (re-formatting through a millisecond clock such as a JS Date loses digits and never matches). Advances on every change, including an exit nulled because its target step was deleted." } StepBranchRequest: type: object required: [condition] @@ -5182,6 +5212,7 @@ components: reply_label_key: { type: string, nullable: true, description: "Optional, replied / not_replied only. Must name a reply label key in the workspace whose stops_enrollment is false (a stopping label ends the sequence before any branch runs). Empty string is treated as null." } yes_step_id: { type: string, format: uuid, nullable: true, description: "A step of the same campaign, not this step; null or absent ends the path" } no_step_id: { type: string, format: uuid, nullable: true, description: "As yes_step_id; must be null or absent for always" } + expected_updated_at: { type: string, format: date-time, nullable: true, description: "Optional precondition. Absent - no check (last writer wins). null - apply only if the step has no branch. A timestamp - apply only if the branch's updated_at is still exactly this (the StepBranch.updated_at value verbatim). A failed check is 409 code branch_changed." } CampaignGraphNode: type: object required: [step_id, step_order, default_next_step_id, branch] @@ -5208,11 +5239,16 @@ components: error: { type: string, description: Human-readable message } code: type: string - enum: [invalid_condition, invalid_within_days, invalid_reply_label, reply_label_stops_sequence, tracking_required, no_exit_not_allowed, unknown_step, unknown_target, cycle] + enum: [invalid_condition, invalid_within_days, invalid_reply_label, reply_label_stops_sequence, tracking_required, no_exit_not_allowed, unknown_step, unknown_target, cycle, branch_changed] step_ids: type: array items: { type: string, format: uuid } description: "code cycle only: the steps on the loop, in path order" + current: + allOf: [{ $ref: '#/components/schemas/StepBranch' }] + type: object + nullable: true + description: "code branch_changed only, and always present then: the step's branch as it is now, or null if it has none" CreateCampaignRequest: type: object required: [name, mailbox_id, list_id, subject] diff --git a/docs/src/content/docs/security.md b/docs/src/content/docs/security.md index 4ef54093..b19489a6 100644 --- a/docs/src/content/docs/security.md +++ b/docs/src/content/docs/security.md @@ -2305,6 +2305,24 @@ write history that never happened. inside the transaction. The send path has a runtime loop backstop that ends the path rather than recovering-forward forever. + **Write preconditions do not widen tenancy.** A branch PUT/DELETE may carry + `expected_updated_at` (optimistic concurrency, so two editors cannot + silently overwrite each other). It is enforced in the write's own `WHERE` + clause (`InsertBranchIfAbsent`, `UpdateBranchIfUnchanged`, + `DeleteBranchIfUnchanged`) under the same graph lock, and the lock still + runs first: a foreign workspace gets the same 404 whatever it expects, and + never reaches the 409. The 409's `current` branch is read with the same + `(step_id, campaign_id, workspace_id)` pin as every other branch read, so it + can only ever show the caller its own workspace's row. The token is + `updated_at`, advanced by a trigger on every insert and update (including + the `ON DELETE SET NULL` of an exit) to a strictly later value than the + row's previous one, so a live row's token cannot repeat and let a stale + write through. (A branch deleted and re-created starts from the database + clock; only a clock stepping back to the exact microsecond of the deleted + row's token could collide.) It is a lost-update guard between + cooperating editors, not an access control: a client that omits it gets + last-writer-wins, exactly as before. + **A branch never overrides reply-label automation.** A reply whose label stops the enrollment still stops it; compliance dispatch (invariants 20, 45) is untouched. A branch only routes enrollments the labels leave active, and diff --git a/internal/app/sequencestep/branch.go b/internal/app/sequencestep/branch.go index 963e6ed9..53b6cd6c 100644 --- a/internal/app/sequencestep/branch.go +++ b/internal/app/sequencestep/branch.go @@ -63,6 +63,10 @@ func (s *Service) Graph(ctx context.Context, ws, campaignID uuid.UUID) (Graph, e // inside the store's transaction, under the campaign's graph lock, against the // graph that is actually being committed — the only place a loop formed by two // concurrent edits can be caught. +// +// in.Expect, when set, is enforced by that same write (see +// BranchStore.UpsertBranch). It is checked after validation, so a request that +// is both stale and malformed is told it is malformed. func (s *Service) SetBranch(ctx context.Context, ws, campaignID uuid.UUID, in BranchInput) (gen.SequenceStepBranch, error) { if err := s.requireCampaign(ctx, ws, campaignID); err != nil { return gen.SequenceStepBranch{}, err @@ -188,16 +192,17 @@ func (s *Service) checkTrackable(ctx context.Context, ws, campaignID uuid.UUID, } // DeleteBranch removes the router on one step, returning it to linear -// fall-through. Idempotent. Allowed live, for the reason SetBranch is; refused -// with a CycleError when the restored fall-through would close a loop. -func (s *Service) DeleteBranch(ctx context.Context, ws, campaignID, stepID uuid.UUID) error { +// fall-through. Idempotent without a precondition. Allowed live, for the reason +// SetBranch is; refused with a CycleError when the restored fall-through would +// close a loop, and with a *BranchChangedError when expect no longer holds. +func (s *Service) DeleteBranch(ctx context.Context, ws, campaignID, stepID uuid.UUID, expect BranchPrecondition) error { if err := s.requireCampaign(ctx, ws, campaignID); err != nil { return err } if _, err := s.stepInCampaign(ctx, ws, campaignID, stepID); err != nil { return err } - return s.branches.DeleteBranch(ctx, ws, campaignID, stepID, checkGraph) + return s.branches.DeleteBranch(ctx, ws, campaignID, stepID, expect, checkGraph) } // checkGraph is the one GraphCheck every graph-changing write commits under. diff --git a/internal/app/sequencestep/branch_integration_test.go b/internal/app/sequencestep/branch_integration_test.go index 3768a61d..cf347bf0 100644 --- a/internal/app/sequencestep/branch_integration_test.go +++ b/internal/app/sequencestep/branch_integration_test.go @@ -110,7 +110,7 @@ func TestBranchSaveAndReadBack(t *testing.T) { t.Fatalf("graph branches = %+v", g.Branches) } - if err := b.svc.DeleteBranch(ctx, b.ws, b.campaign, b.steps[0]); err != nil { + 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 { @@ -214,7 +214,7 @@ func TestBranchWritesAreWorkspacePinned(t *testing.T) { 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], checkGraph); !errors.Is(err, ErrCampaignNotFound) { + 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 { diff --git a/internal/app/sequencestep/branch_test.go b/internal/app/sequencestep/branch_test.go index 119c71e5..5feb6fbc 100644 --- a/internal/app/sequencestep/branch_test.go +++ b/internal/app/sequencestep/branch_test.go @@ -3,7 +3,9 @@ package sequencestep import ( "context" "errors" + "slices" "testing" + "time" "github.com/google/uuid" "github.com/jackc/pgx/v5" @@ -25,6 +27,8 @@ type fakeBranchStore struct { // 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) { @@ -41,6 +45,7 @@ func (f *fakeBranchStore) TrackingEnabled(context.Context, uuid.UUID, uuid.UUID) } 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, @@ -63,7 +68,8 @@ func (f *fakeBranchStore) UpsertBranch(_ context.Context, ws uuid.UUID, in Branc return row, nil } -func (f *fakeBranchStore) DeleteBranch(_ context.Context, _, _, stepID uuid.UUID, check GraphCheck) error { +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 { @@ -171,7 +177,7 @@ func TestGraphEndpointsDoNotReportADatabaseErrorAsNotFound(t *testing.T) { 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]) + 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) @@ -335,7 +341,7 @@ func TestDeleteBranchRefusedWhenFallThroughClosesLoop(t *testing.T) { }); 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]) + 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) } @@ -346,7 +352,7 @@ func TestDeleteBranchRefusedWhenFallThroughClosesLoop(t *testing.T) { func TestDeleteBranchIsIdempotent(t *testing.T) { f := newBranchFixture(t, "running") - if err := f.svc.DeleteBranch(context.Background(), f.ws, f.campaign, f.step[0]); err != nil { + 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) } } @@ -401,3 +407,35 @@ func TestCheckGraphAcceptsEveryLinearCampaign(t *testing.T) { 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 index 1ca8501d..146fcaf4 100644 --- a/internal/app/sequencestep/branchhandler.go +++ b/internal/app/sequencestep/branchhandler.go @@ -3,6 +3,7 @@ package sequencestep import ( "encoding/json" "errors" + "fmt" "net/http" "time" @@ -23,6 +24,76 @@ type branchRequest struct { 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). @@ -64,6 +135,19 @@ type graphErrorResponse struct { 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) @@ -94,6 +178,11 @@ func (h *Handler) SetBranch(w http.ResponseWriter, r *http.Request) { 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 @@ -101,6 +190,7 @@ func (h *Handler) SetBranch(w http.ResponseWriter, r *http.Request) { 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") @@ -115,7 +205,12 @@ func (h *Handler) DeleteBranch(w http.ResponseWriter, r *http.Request) { if !ok { return } - if err := h.svc.DeleteBranch(r.Context(), ws, campaignID, stepID); err != nil { + 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 } @@ -147,10 +242,19 @@ func campaignAndStepIDs(w http.ResponseWriter, r *http.Request) (ws, campaignID, // 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`. +// 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): @@ -200,7 +304,10 @@ func toBranchResponse(b gen.SequenceStepBranch) branchResponse { out := branchResponse{ StepID: b.StepID.String(), Condition: b.Condition, WithinDays: b.WithinDays, ReplyLabelKey: b.ReplyLabelKey, - UpdatedAt: b.UpdatedAt.Time.UTC().Format(time.RFC3339), + // 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) diff --git a/internal/app/sequencestep/branchhandler_test.go b/internal/app/sequencestep/branchhandler_test.go index 60fba6b7..80d649e8 100644 --- a/internal/app/sequencestep/branchhandler_test.go +++ b/internal/app/sequencestep/branchhandler_test.go @@ -6,10 +6,15 @@ import ( "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" ) @@ -51,3 +56,128 @@ func TestWriteGraphErrorContract(t *testing.T) { }) } } + +// 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 index 4d5a606c..d70a936d 100644 --- a/internal/app/sequencestep/branchstore.go +++ b/internal/app/sequencestep/branchstore.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "time" "github.com/google/uuid" "github.com/jackc/pgx/v5" @@ -34,6 +35,49 @@ type BranchInput struct { 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 @@ -47,13 +91,16 @@ type BranchStore interface { // 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. + // 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. - DeleteBranch(ctx context.Context, ws, campaignID, stepID uuid.UUID, check GraphCheck) error + // 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 @@ -104,16 +151,15 @@ func (s *PgBranchStore) TrackingEnabled(ctx context.Context, ws, campaignID uuid 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 := q.UpsertBranch(ctx, gen.UpsertBranchParams{ - 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), - }) + row, err := writeBranch(ctx, q, ws, in) if errors.Is(err, pgx.ErrNoRows) { - return ErrBranchConflict + return refusedWrite(ctx, q, ws, in) } var pgErr *pgconn.PgError if errors.As(err, &pgErr) && pgErr.Code == foreignKeyViolation { @@ -128,15 +174,104 @@ func (s *PgBranchStore) UpsertBranch(ctx context.Context, ws uuid.UUID, in Branc return out, err } -func (s *PgBranchStore) DeleteBranch(ctx context.Context, ws, campaignID, stepID uuid.UUID, check GraphCheck) error { +// 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 { - if err := q.DeleteBranch(ctx, gen.DeleteBranchParams{StepID: stepID, CampaignID: campaignID, WorkspaceID: ws}); err != nil { - return fmt.Errorf("delete branch: %w", err) + 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 } - 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 @@ -188,3 +323,7 @@ func nullUUID(id *uuid.UUID) 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/platform/db/branchmigrations_integration_test.go b/internal/platform/db/branchmigrations_integration_test.go index 85b7d973..0087d0ab 100644 --- a/internal/platform/db/branchmigrations_integration_test.go +++ b/internal/platform/db/branchmigrations_integration_test.go @@ -20,6 +20,18 @@ const ( preBranchVersion = 20260921111415 ) +// triggerExists reports whether the trigger that makes updated_at the branch's +// concurrency token (20260923161044) is installed. +func triggerExists(t *testing.T, ctx context.Context, pool *pgxpool.Pool) bool { + t.Helper() + var n int + if err := pool.QueryRow(ctx, `SELECT count(*) FROM pg_trigger + WHERE tgname = 'sequence_step_branches_advance_updated_at'`).Scan(&n); err != nil { + t.Fatalf("trigger lookup: %v", err) + } + return n == 1 +} + // indexState reports whether the named index exists and, if so, whether it is // VALID — a failed CREATE INDEX CONCURRENTLY leaves an invalid index behind // rather than rolling back, and IF NOT EXISTS would skip it on every re-run. @@ -56,11 +68,17 @@ func TestInboxThreadsCampaignContactIndexIsValid(t *testing.T) { if exists, valid := indexState(t, ctx, pool, idx); !exists || !valid { t.Fatalf("%s after a fresh migrate: exists=%v valid=%v", idx, exists, valid) } + if !triggerExists(t, ctx, pool) { + t.Fatal("updated_at trigger missing after a fresh migrate") + } // Down past both branching migrations, then up again. if err := db.MigrateTo(dsn, branchIndexVersion); err != nil { t.Fatalf("migrate to %d: %v", branchIndexVersion, err) } + if triggerExists(t, ctx, pool) { + t.Fatal("updated_at trigger survived its own down migration") + } if err := db.MigrateTo(dsn, branchTablesVersion); err != nil { t.Fatalf("roll back the index migration: %v", err) } @@ -86,4 +104,7 @@ func TestInboxThreadsCampaignContactIndexIsValid(t *testing.T) { if exists, valid := indexState(t, ctx, pool, idx); !exists || !valid { t.Fatalf("%s after up/down/up: exists=%v valid=%v", idx, exists, valid) } + if !triggerExists(t, ctx, pool) { + t.Fatal("updated_at trigger missing after up/down/up") + } } diff --git a/internal/platform/db/gen/stepbranch.sql.go b/internal/platform/db/gen/stepbranch.sql.go index 318ba18a..c0a1590c 100644 --- a/internal/platform/db/gen/stepbranch.sql.go +++ b/internal/platform/db/gen/stepbranch.sql.go @@ -48,6 +48,34 @@ func (q *Queries) DeleteBranch(ctx context.Context, arg DeleteBranchParams) erro return err } +const deleteBranchIfUnchanged = `-- name: DeleteBranchIfUnchanged :execrows +DELETE FROM sequence_step_branches +WHERE step_id = $1 AND campaign_id = $2 AND workspace_id = $3 + AND updated_at = $4::timestamptz +` + +type DeleteBranchIfUnchangedParams struct { + StepID uuid.UUID `json:"step_id"` + CampaignID uuid.UUID `json:"campaign_id"` + WorkspaceID uuid.UUID `json:"workspace_id"` + ExpectedUpdatedAt pgtype.Timestamptz `json:"expected_updated_at"` +} + +// Remove a step's router only if its updated_at still equals the value the +// client last read. Zero rows means it changed or was already removed. +func (q *Queries) DeleteBranchIfUnchanged(ctx context.Context, arg DeleteBranchIfUnchangedParams) (int64, error) { + result, err := q.db.Exec(ctx, deleteBranchIfUnchanged, + arg.StepID, + arg.CampaignID, + arg.WorkspaceID, + arg.ExpectedUpdatedAt, + ) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + const firstHumanTrackingEventAt = `-- name: FirstHumanTrackingEventAt :one SELECT created_at FROM tracking_events WHERE send_id = $1 AND workspace_id = $2 AND kind = $3 AND NOT is_machine @@ -129,6 +157,88 @@ func (q *Queries) FirstInboundReplyAt(ctx context.Context, arg FirstInboundReply return created_at, err } +const getBranch = `-- name: GetBranch :one +SELECT step_id, workspace_id, campaign_id, condition, within_days, reply_label_key, yes_step_id, no_step_id, created_at, updated_at FROM sequence_step_branches +WHERE step_id = $1 AND campaign_id = $2 AND workspace_id = $3 +` + +type GetBranchParams struct { + StepID uuid.UUID `json:"step_id"` + CampaignID uuid.UUID `json:"campaign_id"` + WorkspaceID uuid.UUID `json:"workspace_id"` +} + +// One step's router, workspace- and campaign-pinned. Read inside a refused +// precondition's transaction, so the 409 carries the branch that won. +func (q *Queries) GetBranch(ctx context.Context, arg GetBranchParams) (SequenceStepBranch, error) { + row := q.db.QueryRow(ctx, getBranch, arg.StepID, arg.CampaignID, arg.WorkspaceID) + var i SequenceStepBranch + err := row.Scan( + &i.StepID, + &i.WorkspaceID, + &i.CampaignID, + &i.Condition, + &i.WithinDays, + &i.ReplyLabelKey, + &i.YesStepID, + &i.NoStepID, + &i.CreatedAt, + &i.UpdatedAt, + ) + return i, err +} + +const insertBranchIfAbsent = `-- name: InsertBranchIfAbsent :one +INSERT INTO sequence_step_branches (step_id, workspace_id, campaign_id, condition, within_days, + reply_label_key, yes_step_id, no_step_id) +VALUES ($1, $2, $3, $4, $5, $6, + $7, $8) +ON CONFLICT (step_id) DO NOTHING +RETURNING step_id, workspace_id, campaign_id, condition, within_days, reply_label_key, yes_step_id, no_step_id, created_at, updated_at +` + +type InsertBranchIfAbsentParams struct { + StepID uuid.UUID `json:"step_id"` + WorkspaceID uuid.UUID `json:"workspace_id"` + CampaignID uuid.UUID `json:"campaign_id"` + Condition string `json:"condition"` + WithinDays *int32 `json:"within_days"` + ReplyLabelKey *string `json:"reply_label_key"` + YesStepID pgtype.UUID `json:"yes_step_id"` + NoStepID pgtype.UUID `json:"no_step_id"` +} + +// Create the router on one step only if the step has none: the "expect no +// branch" precondition. No row returned means one already exists (the caller +// reads it with GetBranch to report it). Atomic on its own via the primary key, +// so two concurrent creates cannot both succeed even without the graph lock. +func (q *Queries) InsertBranchIfAbsent(ctx context.Context, arg InsertBranchIfAbsentParams) (SequenceStepBranch, error) { + row := q.db.QueryRow(ctx, insertBranchIfAbsent, + arg.StepID, + arg.WorkspaceID, + arg.CampaignID, + arg.Condition, + arg.WithinDays, + arg.ReplyLabelKey, + arg.YesStepID, + arg.NoStepID, + ) + var i SequenceStepBranch + err := row.Scan( + &i.StepID, + &i.WorkspaceID, + &i.CampaignID, + &i.Condition, + &i.WithinDays, + &i.ReplyLabelKey, + &i.YesStepID, + &i.NoStepID, + &i.CreatedAt, + &i.UpdatedAt, + ) + return i, err +} + const listBranchesByCampaign = `-- name: ListBranchesByCampaign :many SELECT step_id, workspace_id, campaign_id, condition, within_days, reply_label_key, yes_step_id, no_step_id, created_at, updated_at FROM sequence_step_branches WHERE campaign_id = $1 AND workspace_id = $2 @@ -237,6 +347,62 @@ func (q *Queries) StepSendCreatedAt(ctx context.Context, arg StepSendCreatedAtPa return created_at, err } +const updateBranchIfUnchanged = `-- name: UpdateBranchIfUnchanged :one +UPDATE sequence_step_branches +SET condition = $1, within_days = $2, + reply_label_key = $3, yes_step_id = $4, + no_step_id = $5 +WHERE step_id = $6 AND workspace_id = $7 + AND campaign_id = $8 + AND updated_at = $9::timestamptz +RETURNING step_id, workspace_id, campaign_id, condition, within_days, reply_label_key, yes_step_id, no_step_id, created_at, updated_at +` + +type UpdateBranchIfUnchangedParams struct { + Condition string `json:"condition"` + WithinDays *int32 `json:"within_days"` + ReplyLabelKey *string `json:"reply_label_key"` + YesStepID pgtype.UUID `json:"yes_step_id"` + NoStepID pgtype.UUID `json:"no_step_id"` + StepID uuid.UUID `json:"step_id"` + WorkspaceID uuid.UUID `json:"workspace_id"` + CampaignID uuid.UUID `json:"campaign_id"` + ExpectedUpdatedAt pgtype.Timestamptz `json:"expected_updated_at"` +} + +// Replace the router on one step only if its updated_at still equals the value +// the client last read: the "expect this version" precondition. No row returned +// means the branch changed or is gone. Compared at full (microsecond) precision; +// the trigger guarantees every write moves the value. Pinned on workspace_id and +// campaign_id. +func (q *Queries) UpdateBranchIfUnchanged(ctx context.Context, arg UpdateBranchIfUnchangedParams) (SequenceStepBranch, error) { + row := q.db.QueryRow(ctx, updateBranchIfUnchanged, + arg.Condition, + arg.WithinDays, + arg.ReplyLabelKey, + arg.YesStepID, + arg.NoStepID, + arg.StepID, + arg.WorkspaceID, + arg.CampaignID, + arg.ExpectedUpdatedAt, + ) + var i SequenceStepBranch + err := row.Scan( + &i.StepID, + &i.WorkspaceID, + &i.CampaignID, + &i.Condition, + &i.WithinDays, + &i.ReplyLabelKey, + &i.YesStepID, + &i.NoStepID, + &i.CreatedAt, + &i.UpdatedAt, + ) + return i, err +} + const upsertBranch = `-- name: UpsertBranch :one INSERT INTO sequence_step_branches (step_id, workspace_id, campaign_id, condition, within_days, reply_label_key, yes_step_id, no_step_id) @@ -245,7 +411,7 @@ VALUES ($1, $2, $3, $4, $5, $6, ON CONFLICT (step_id) DO UPDATE SET condition = EXCLUDED.condition, within_days = EXCLUDED.within_days, reply_label_key = EXCLUDED.reply_label_key, yes_step_id = EXCLUDED.yes_step_id, - no_step_id = EXCLUDED.no_step_id, updated_at = now() + no_step_id = EXCLUDED.no_step_id WHERE sequence_step_branches.workspace_id = EXCLUDED.workspace_id AND sequence_step_branches.campaign_id = EXCLUDED.campaign_id RETURNING step_id, workspace_id, campaign_id, condition, within_days, reply_label_key, yes_step_id, no_step_id, created_at, updated_at @@ -267,7 +433,10 @@ type UpsertBranchParams struct { // (source or target) that is not in that campaign, so a foreign id cannot be // smuggled in even if the service's own check were skipped. The ON CONFLICT // update is pinned on workspace_id as well: a step id belonging to another tenant -// updates nothing and returns no row. +// updates nothing and returns no row. updated_at is not written here: a trigger +// advances it on every insert and update (migration +// 20260923161044_step_branch_updated_at_advances), because it is the branch's +// concurrency token. func (q *Queries) UpsertBranch(ctx context.Context, arg UpsertBranchParams) (SequenceStepBranch, error) { row := q.db.QueryRow(ctx, upsertBranch, arg.StepID, diff --git a/internal/platform/db/migrations/20260923161044_step_branch_updated_at_advances.down.sql b/internal/platform/db/migrations/20260923161044_step_branch_updated_at_advances.down.sql new file mode 100644 index 00000000..5bec8975 --- /dev/null +++ b/internal/platform/db/migrations/20260923161044_step_branch_updated_at_advances.down.sql @@ -0,0 +1,2 @@ +DROP TRIGGER sequence_step_branches_advance_updated_at ON sequence_step_branches; +DROP FUNCTION sequence_step_branches_advance_updated_at(); diff --git a/internal/platform/db/migrations/20260923161044_step_branch_updated_at_advances.up.sql b/internal/platform/db/migrations/20260923161044_step_branch_updated_at_advances.up.sql new file mode 100644 index 00000000..d2e93895 --- /dev/null +++ b/internal/platform/db/migrations/20260923161044_step_branch_updated_at_advances.up.sql @@ -0,0 +1,36 @@ +-- sequence_step_branches.updated_at becomes the branch's concurrency token: a +-- PUT/DELETE may carry expected_updated_at and applies only if the stored value +-- still equals it (internal/app/sequencestep/branchstore.go). A token has to +-- change on EVERY write, and a plain `updated_at = now()` in the upsert does not +-- guarantee that: +-- * now() is the TRANSACTION start, taken before the graph lock is acquired, +-- so a transaction that began first but got the lock second writes an OLDER +-- value than the one it replaced; +-- * two writes can share a microsecond, or the clock can step backwards, and +-- the token would then repeat — a stale client's precondition would match; +-- * ON DELETE SET NULL (yes_step_id / no_step_id) rewrites a row when its +-- target step is deleted, and no query of ours runs to bump the column. +-- +-- So the database owns the column, for every insert and every update including +-- the referential action: an insert takes clock_timestamp() (the statement's +-- real time, not the transaction's), and an update takes the later of +-- clock_timestamp() and one microsecond past the old value — strictly advancing +-- per row regardless of the clock. A timestamp rather than a version integer so +-- a branch deleted and re-created does not restart at a value a stale client +-- still holds, and so the already-served updated_at field is the token rather +-- than a second field saying the same thing. +CREATE FUNCTION sequence_step_branches_advance_updated_at() RETURNS trigger +LANGUAGE plpgsql AS $$ +BEGIN + IF TG_OP = 'UPDATE' THEN + NEW.updated_at := GREATEST(clock_timestamp(), OLD.updated_at + interval '1 microsecond'); + ELSE + NEW.updated_at := clock_timestamp(); + END IF; + RETURN NEW; +END; +$$; + +CREATE TRIGGER sequence_step_branches_advance_updated_at +BEFORE INSERT OR UPDATE ON sequence_step_branches +FOR EACH ROW EXECUTE FUNCTION sequence_step_branches_advance_updated_at(); diff --git a/internal/platform/db/queries/stepbranch.sql b/internal/platform/db/queries/stepbranch.sql index 5f4c48c3..67bc0a56 100644 --- a/internal/platform/db/queries/stepbranch.sql +++ b/internal/platform/db/queries/stepbranch.sql @@ -12,7 +12,10 @@ ORDER BY step_id; -- (source or target) that is not in that campaign, so a foreign id cannot be -- smuggled in even if the service's own check were skipped. The ON CONFLICT -- update is pinned on workspace_id as well: a step id belonging to another tenant --- updates nothing and returns no row. +-- updates nothing and returns no row. updated_at is not written here: a trigger +-- advances it on every insert and update (migration +-- 20260923161044_step_branch_updated_at_advances), because it is the branch's +-- concurrency token. INSERT INTO sequence_step_branches (step_id, workspace_id, campaign_id, condition, within_days, reply_label_key, yes_step_id, no_step_id) VALUES ($1, $2, $3, $4, sqlc.narg(within_days), sqlc.narg(reply_label_key), @@ -20,17 +23,57 @@ VALUES ($1, $2, $3, $4, sqlc.narg(within_days), sqlc.narg(reply_label_key), ON CONFLICT (step_id) DO UPDATE SET condition = EXCLUDED.condition, within_days = EXCLUDED.within_days, reply_label_key = EXCLUDED.reply_label_key, yes_step_id = EXCLUDED.yes_step_id, - no_step_id = EXCLUDED.no_step_id, updated_at = now() + no_step_id = EXCLUDED.no_step_id WHERE sequence_step_branches.workspace_id = EXCLUDED.workspace_id AND sequence_step_branches.campaign_id = EXCLUDED.campaign_id RETURNING *; +-- name: InsertBranchIfAbsent :one +-- Create the router on one step only if the step has none: the "expect no +-- branch" precondition. No row returned means one already exists (the caller +-- reads it with GetBranch to report it). Atomic on its own via the primary key, +-- so two concurrent creates cannot both succeed even without the graph lock. +INSERT INTO sequence_step_branches (step_id, workspace_id, campaign_id, condition, within_days, + reply_label_key, yes_step_id, no_step_id) +VALUES ($1, $2, $3, $4, sqlc.narg(within_days), sqlc.narg(reply_label_key), + sqlc.narg(yes_step_id), sqlc.narg(no_step_id)) +ON CONFLICT (step_id) DO NOTHING +RETURNING *; + +-- name: UpdateBranchIfUnchanged :one +-- Replace the router on one step only if its updated_at still equals the value +-- the client last read: the "expect this version" precondition. No row returned +-- means the branch changed or is gone. Compared at full (microsecond) precision; +-- the trigger guarantees every write moves the value. Pinned on workspace_id and +-- campaign_id. +UPDATE sequence_step_branches +SET condition = sqlc.arg(condition), within_days = sqlc.narg(within_days), + reply_label_key = sqlc.narg(reply_label_key), yes_step_id = sqlc.narg(yes_step_id), + no_step_id = sqlc.narg(no_step_id) +WHERE step_id = sqlc.arg(step_id) AND workspace_id = sqlc.arg(workspace_id) + AND campaign_id = sqlc.arg(campaign_id) + AND updated_at = sqlc.arg(expected_updated_at)::timestamptz +RETURNING *; + +-- name: GetBranch :one +-- One step's router, workspace- and campaign-pinned. Read inside a refused +-- precondition's transaction, so the 409 carries the branch that won. +SELECT * FROM sequence_step_branches +WHERE step_id = $1 AND campaign_id = $2 AND workspace_id = $3; + -- name: DeleteBranch :exec -- Remove a step's router, returning it to linear fall-through. Pinned on -- workspace_id and campaign_id. DELETE FROM sequence_step_branches WHERE step_id = $1 AND campaign_id = $2 AND workspace_id = $3; +-- name: DeleteBranchIfUnchanged :execrows +-- Remove a step's router only if its updated_at still equals the value the +-- client last read. Zero rows means it changed or was already removed. +DELETE FROM sequence_step_branches +WHERE step_id = $1 AND campaign_id = $2 AND workspace_id = $3 + AND updated_at = sqlc.arg(expected_updated_at)::timestamptz; + -- name: LockCampaignGraph :one -- Serializes every write that changes a campaign's routing graph (branch -- upsert/delete, step delete, reorder), so the save-time cycle check sees the diff --git a/web/src/store/api.ts b/web/src/store/api.ts index 2265288a..7725d412 100644 --- a/web/src/store/api.ts +++ b/web/src/store/api.ts @@ -1017,6 +1017,9 @@ const injectedRtkApi = api.injectEndpoints({ query: (queryArg) => ({ url: `/campaigns/${queryArg.id}/steps/${queryArg.stepId}/branch`, method: "DELETE", + params: { + expected_updated_at: queryArg.expectedUpdatedAt, + }, }), }), launchCampaign: build.mutation< @@ -2547,6 +2550,8 @@ export type DeleteStepBranchApiResponse = unknown; export type DeleteStepBranchApiArg = { id: string; stepId: string; + /** The branch's updated_at exactly as this API returned it (URL-encoded). Omit for an unconditional delete. */ + expectedUpdatedAt?: string; }; export type LaunchCampaignApiResponse = /** status 200 Enrollment + queue counts */ { @@ -4417,6 +4422,23 @@ export type StepRequest = { body_text?: string; body_html?: string; }; +export type StepBranchCondition = + "always" | "opened" | "clicked" | "replied" | "not_opened" | "not_replied"; +export type StepBranch = { + /** The step this branch routes out of */ + step_id: string; + condition: StepBranchCondition; + /** Evaluation window in days after the step's send; null exactly when condition is always */ + within_days: number | null; + /** replied / not_replied only: count only replies classified with this reply label key (always a label that does not stop the sequence) */ + reply_label_key: string | null; + /** Where a true condition (or always) goes; null ends the path */ + yes_step_id: string | null; + /** Where a false condition goes; null ends the path; always null for always */ + no_step_id: string | null; + /** When the branch last changed, at microsecond precision. Also its concurrency token - send it back verbatim as expected_updated_at (re-formatting through a millisecond clock such as a JS Date loses digits and never matches). Advances on every change, including an exit nulled because its target step was deleted. */ + updated_at: string; +}; export type BranchValidationError = { /** Human-readable message */ error: string; @@ -4429,9 +4451,12 @@ export type BranchValidationError = { | "no_exit_not_allowed" | "unknown_step" | "unknown_target" - | "cycle"; + | "cycle" + | "branch_changed"; /** code cycle only: the steps on the loop, in path order */ step_ids?: string[]; + /** code branch_changed only, and always present then: the step's branch as it is now, or null if it has none */ + current?: StepBranch | null; }; export type StepVariant = { id: string; @@ -4459,22 +4484,6 @@ export type ReorderStepsRequest = { /** the FULL ordered list of the campaign's step ids, in the desired order */ step_ids: string[]; }; -export type StepBranchCondition = - "always" | "opened" | "clicked" | "replied" | "not_opened" | "not_replied"; -export type StepBranch = { - /** The step this branch routes out of */ - step_id: string; - condition: StepBranchCondition; - /** Evaluation window in days after the step's send; null exactly when condition is always */ - within_days: number | null; - /** replied / not_replied only: count only replies classified with this reply label key (always a label that does not stop the sequence) */ - reply_label_key: string | null; - /** Where a true condition (or always) goes; null ends the path */ - yes_step_id: string | null; - /** Where a false condition goes; null ends the path; always null for always */ - no_step_id: string | null; - updated_at: string; -}; export type CampaignGraphNode = { step_id: string; step_order: number; @@ -4499,6 +4508,8 @@ export type StepBranchRequest = { yes_step_id?: string | null; /** As yes_step_id; must be null or absent for always */ no_step_id?: string | null; + /** Optional precondition. Absent - no check (last writer wins). null - apply only if the step has no branch. A timestamp - apply only if the branch's updated_at is still exactly this (the StepBranch.updated_at value verbatim). A failed check is 409 code branch_changed. */ + expected_updated_at?: string | null; }; export type CampaignPreflightCheck = { /** `personalization_tokens` FAILS (does not warn) when a step contains a `{{...}}` placeholder nothing will substitute, which is harsher than the neighbouring content checks on purpose: an empty body is visible the moment an operator looks at it, whereas a bad token produces an email that looks fine in the editor and arrives reading "Hi {{firstname}}" or "Hi ,". A token nothing resolves is always a typo or a since-archived field, never an intent. */