From 5f76780a282c6bd592f6c037574d24188de98b98 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 05:15:01 +0000 Subject: [PATCH 1/3] fix(schedule-import): abort commit_schedule on stale watermark (#42) diff-schedule now computes a watermark over the edition's sets and returns it with the plan; the client threads it through unchanged to commit-schedule, which re-validates it inside the commit transaction and aborts (applying nothing) if the edition changed since Analyse, instead of silently applying a stale plan. Closes #42 --- .../scheduleImport/buildCommitPayload.test.ts | 8 ++ .../scheduleImport/buildCommitPayload.ts | 2 + src/services/scheduleImport/types.ts | 1 + .../commit-schedule/commit-schedule.test.ts | 89 ++++++++++++++++++ supabase/functions/commit-schedule/index.ts | 6 ++ supabase/functions/diff-schedule/index.ts | 21 ++++- supabase/functions/diff-schedule/types.ts | 4 + ...260901000000_commit_schedule_watermark.sql | 93 +++++++++++++++++++ 8 files changed, 222 insertions(+), 2 deletions(-) create mode 100644 supabase/migrations/20260901000000_commit_schedule_watermark.sql diff --git a/src/services/scheduleImport/buildCommitPayload.test.ts b/src/services/scheduleImport/buildCommitPayload.test.ts index 708140f3a..5d7940cfc 100644 --- a/src/services/scheduleImport/buildCommitPayload.test.ts +++ b/src/services/scheduleImport/buildCommitPayload.test.ts @@ -3,6 +3,13 @@ import { buildCommitPayload } from "./buildCommitPayload"; import { type DiffResult } from "./types"; describe("buildCommitPayload", () => { + it("passes the diff's watermark through unchanged", () => { + const diff = makeDiff({ watermark: "7:2026-08-30T12:00:00+00:00" }); + + const payload = buildCommitPayload(diff, {}, {}); + expect(payload.watermark).toBe("7:2026-08-30T12:00:00+00:00"); + }); + it("passes through clean artistsToCreate/stagesToCreate untouched", () => { const diff = makeDiff({ cleanOperations: { @@ -167,6 +174,7 @@ describe("buildCommitPayload", () => { function makeDiff(overrides: Partial = {}): DiffResult { return { + watermark: "3:2026-08-01T00:00:00+00:00", summary: { newArtists: 0, newStages: 0, diff --git a/src/services/scheduleImport/buildCommitPayload.ts b/src/services/scheduleImport/buildCommitPayload.ts index fee6fbbdf..b41c014db 100644 --- a/src/services/scheduleImport/buildCommitPayload.ts +++ b/src/services/scheduleImport/buildCommitPayload.ts @@ -10,6 +10,7 @@ export function buildCommitPayload( stageMismatchResolutions: Record, orphanResolutions: Record, ): { + watermark: string; artistsToCreate: { name: string; slug: string }[]; stagesToCreate: { name: string }[]; setsToCreate: SetPayload[]; @@ -33,6 +34,7 @@ export function buildCommitPayload( .map((s) => s.id); return { + watermark: diff.watermark, artistsToCreate: diff.cleanOperations.artistsToCreate, stagesToCreate: [ ...diff.cleanOperations.stagesToCreate, diff --git a/src/services/scheduleImport/types.ts b/src/services/scheduleImport/types.ts index e6ce8560b..2aa1d3fee 100644 --- a/src/services/scheduleImport/types.ts +++ b/src/services/scheduleImport/types.ts @@ -24,6 +24,7 @@ export const setPayloadSchema = z.object({ export type SetPayload = z.infer; export const diffResultSchema = z.object({ + watermark: z.string(), summary: z.object({ newArtists: z.number(), newStages: z.number(), diff --git a/supabase/functions/commit-schedule/commit-schedule.test.ts b/supabase/functions/commit-schedule/commit-schedule.test.ts index 328aef838..6a5538709 100644 --- a/supabase/functions/commit-schedule/commit-schedule.test.ts +++ b/supabase/functions/commit-schedule/commit-schedule.test.ts @@ -39,6 +39,20 @@ async function getTestUserId( return data.user_id; } +// #42: commit_schedule now requires a watermark matching the edition's +// current state. Fetch it fresh (via the same RPC diff-schedule uses) +// right before each call so unrelated setup inserts above don't go stale. +async function getWatermark( + db: ReturnType, + editionId: string, +): Promise { + const { data, error } = await db.rpc("commit_schedule__compute_watermark", { + p_festival_edition_id: editionId, + }); + assertEquals(error, null); + return data as string; +} + Deno.test("commit_schedule: creates new artist and set", async () => { const db = adminClient(); const editionId = await getTestEditionId(db); @@ -49,6 +63,7 @@ Deno.test("commit_schedule: creates new artist and set", async () => { const { data, error } = await db.rpc("commit_schedule", { p_festival_edition_id: editionId, p_user_id: userId, + p_watermark: await getWatermark(db, editionId), p_artists_to_create: [{ name: "Test Artist", slug }], p_stages_to_create: [], p_sets_to_create: [ @@ -112,6 +127,7 @@ Deno.test( const { data, error } = await db.rpc("commit_schedule", { p_festival_edition_id: editionId, p_user_id: userId, + p_watermark: await getWatermark(db, editionId), p_artists_to_create: [], p_stages_to_create: [], p_sets_to_create: [], @@ -165,6 +181,7 @@ Deno.test("commit_schedule: archives orphaned sets", async () => { const { data, error } = await db.rpc("commit_schedule", { p_festival_edition_id: editionId, p_user_id: userId, + p_watermark: await getWatermark(db, editionId), p_artists_to_create: [], p_stages_to_create: [], p_sets_to_create: [], @@ -202,6 +219,7 @@ Deno.test( const { data, error } = await db.rpc("commit_schedule", { p_festival_edition_id: editionId, p_user_id: userId, + p_watermark: await getWatermark(db, editionId), p_artists_to_create: [], p_stages_to_create: [], p_sets_to_create: [ @@ -261,6 +279,7 @@ Deno.test( const { data, error } = await db.rpc("commit_schedule", { p_festival_edition_id: editionId, p_user_id: userId, + p_watermark: await getWatermark(db, editionId), p_artists_to_create: [], p_stages_to_create: [], p_sets_to_create: [ @@ -330,6 +349,7 @@ Deno.test( const { error: overwriteError } = await db.rpc("commit_schedule", { p_festival_edition_id: editionId, p_user_id: userId, + p_watermark: await getWatermark(db, editionId), p_artists_to_create: [], p_stages_to_create: [], p_sets_to_create: [], @@ -349,6 +369,7 @@ Deno.test( const { error: preserveError } = await db.rpc("commit_schedule", { p_festival_edition_id: editionId, p_user_id: userId, + p_watermark: await getWatermark(db, editionId), p_artists_to_create: [], p_stages_to_create: [], p_sets_to_create: [], @@ -384,6 +405,7 @@ Deno.test( const { error } = await db.rpc("commit_schedule", { p_festival_edition_id: editionId, p_user_id: userId, + p_watermark: await getWatermark(db, editionId), p_artists_to_create: [], p_stages_to_create: [], p_sets_to_create: [ @@ -417,3 +439,70 @@ Deno.test( await db.from("artists").delete().eq("slug", slug); }, ); + +Deno.test( + "commit_schedule: aborts and applies nothing when the edition changed since the watermark was computed (#42)", + async () => { + const db = adminClient(); + const editionId = await getTestEditionId(db); + const userId = await getTestUserId(db); + const slug = `test-stale-watermark-${Date.now()}`; + const setName = `Stale Watermark Set ${slug}`; + + const staleWatermark = await getWatermark(db, editionId); + + // Simulate a concurrent edit landing after Analyse: create an unrelated + // set, which changes the edition's watermark. + const { data: concurrentSet } = await db + .from("sets") + .insert({ + festival_edition_id: editionId, + name: "Concurrent Edit", + slug: `concurrent-edit-${Date.now()}`, + created_by: userId, + }) + .select("id") + .single(); + + const { data, error } = await db.rpc("commit_schedule", { + p_festival_edition_id: editionId, + p_user_id: userId, + p_watermark: staleWatermark, + p_artists_to_create: [{ name: "Stale Watermark Artist", slug }], + p_stages_to_create: [], + p_sets_to_create: [ + { + name: setName, + description: null, + stageName: null, + timeStart: null, + timeEnd: null, + artistSlugs: [slug], + }, + ], + p_sets_to_update: [], + p_set_ids_to_archive: [], + }); + + assertExists(error, "expected commit_schedule to reject a stale watermark"); + assertEquals(data, null); + + // Nothing from the rejected commit was applied — same transaction. + const { data: artist } = await db + .from("artists") + .select("id") + .eq("slug", slug) + .maybeSingle(); + assertEquals(artist, null); + + const { data: sets } = await db + .from("sets") + .select("id") + .eq("festival_edition_id", editionId) + .eq("name", setName); + assertEquals(sets?.length, 0); + + // Cleanup + await db.from("sets").delete().eq("id", concurrentSet!.id); + }, +); diff --git a/supabase/functions/commit-schedule/index.ts b/supabase/functions/commit-schedule/index.ts index 5ed2708ff..a1904ad21 100644 --- a/supabase/functions/commit-schedule/index.ts +++ b/supabase/functions/commit-schedule/index.ts @@ -28,6 +28,10 @@ const setPayloadSchema = z.object({ const commitRequestSchema = z.object({ festivalEditionId: z.string().uuid(), + // #42: the watermark diff-schedule returned at Analyse time, round-tripped + // unchanged. commit_schedule re-validates it against the edition's current + // state and aborts if they diverge. + watermark: z.string().min(1), artistsToCreate: z .array(z.object({ name: z.string().min(1), slug: z.string().min(1) })) .default([]), @@ -71,6 +75,7 @@ serve(async (req) => { const { festivalEditionId, + watermark, artistsToCreate, stagesToCreate, setsToCreate, @@ -83,6 +88,7 @@ serve(async (req) => { const { data, error } = await db.rpc("commit_schedule", { p_festival_edition_id: festivalEditionId, p_user_id: auth.userId, + p_watermark: watermark, p_artists_to_create: artistsToCreate, p_stages_to_create: stagesToCreate, p_sets_to_create: setsToCreate, diff --git a/supabase/functions/diff-schedule/index.ts b/supabase/functions/diff-schedule/index.ts index 062986953..d6c5fb143 100644 --- a/supabase/functions/diff-schedule/index.ts +++ b/supabase/functions/diff-schedule/index.ts @@ -1,11 +1,27 @@ import { serve } from "https://deno.land/std@0.168.0/http/server.ts"; import { z } from "https://deno.land/x/zod@v3.22.4/mod.ts"; +import type { SupabaseClient } from "https://esm.sh/@supabase/supabase-js@2"; import { requireAdmin } from "../_shared/auth.ts"; import { buildCorsHeaders } from "../_shared/cors.ts"; import { SET_TYPES } from "../_shared/setTypes.ts"; import { computeDiff } from "./computeDiff.ts"; import { fetchAllRows } from "./fetchAllRows.ts"; +// #42: a watermark over the edition's sets, computed the same way (same SQL +// function) commit_schedule recomputes at Commit time. Round-tripped through +// the client unchanged so commit_schedule can abort if the edition changed +// between Analyse and Commit. +async function fetchWatermark( + db: SupabaseClient, + festivalEditionId: string, +): Promise { + const { data, error } = await db.rpc("commit_schedule__compute_watermark", { + p_festival_edition_id: festivalEditionId, + }); + if (error) throw error; + return data as string; +} + function isValidTimezone(tz: string): boolean { try { new Intl.DateTimeFormat("en-US", { timeZone: tz }); @@ -97,7 +113,7 @@ serve(async (req) => { const db = auth.adminClient; - const [dbStages, dbSets, dbArtists] = await Promise.all([ + const [dbStages, dbSets, dbArtists, watermark] = await Promise.all([ fetchAllRows((from, to) => db .from("stages") @@ -127,11 +143,12 @@ serve(async (req) => { .order("id") .range(from, to), ), + fetchWatermark(db, festivalEditionId), ]); const result = computeDiff(rows, dbStages, dbSets, dbArtists, timezone); - return new Response(JSON.stringify(result), { + return new Response(JSON.stringify({ ...result, watermark }), { headers: { ...corsHeaders, "Content-Type": "application/json" }, }); } catch (error) { diff --git a/supabase/functions/diff-schedule/types.ts b/supabase/functions/diff-schedule/types.ts index 09a32b96e..5186f6021 100644 --- a/supabase/functions/diff-schedule/types.ts +++ b/supabase/functions/diff-schedule/types.ts @@ -46,6 +46,10 @@ export type SetPayload = { }; export type DiffResult = { + // #42: opaque watermark over the edition's sets at Analyse time. Threaded + // back through commit-schedule unchanged so commit_schedule can detect a + // concurrent edit and abort instead of applying a stale plan. + watermark: string; summary: { newArtists: number; newStages: number; diff --git a/supabase/migrations/20260901000000_commit_schedule_watermark.sql b/supabase/migrations/20260901000000_commit_schedule_watermark.sql new file mode 100644 index 000000000..7987ddfb7 --- /dev/null +++ b/supabase/migrations/20260901000000_commit_schedule_watermark.sql @@ -0,0 +1,93 @@ +-- #42: commit_schedule applied a stale diff snapshot with no re-validation +-- at commit time. diff-schedule (Analyse) and commit_schedule (Commit) can +-- be minutes apart with no lock in between, so a concurrent edit made after +-- Analyse was silently overridden by the earlier plan -- most visibly, +-- setIdsToArchive being built from an orphan list that's gone stale. +-- +-- Fix: abort-on-change. diff-schedule computes a watermark over the +-- edition's sets and returns it with the plan; the client threads it +-- through unchanged to commit-schedule; commit_schedule recomputes the same +-- watermark as its first step, inside the transaction, and aborts the whole +-- commit if it doesn't match. No advisory lock needed -- the in-txn re-check +-- makes concurrent commits safe (the second one aborts). The plan is either +-- applied verbatim or rejected whole; setIdsToArchive is never silently +-- recomputed. + +-- Watermark = row count + latest updated_at across ALL of the edition's +-- sets (archived included -- an archive is exactly the kind of change this +-- must catch, and commit_schedule__archive_sets bumps updated_at on +-- archive same as create/update do via the update_sets_updated_at trigger +-- and the sets.updated_at DEFAULT now()). Shared between diff-schedule and +-- commit_schedule so the two sides can never drift out of format. +CREATE OR REPLACE FUNCTION public.commit_schedule__compute_watermark( + p_festival_edition_id UUID +) +RETURNS TEXT +LANGUAGE sql +STABLE +SET search_path = public +AS $$ + SELECT COUNT(*) || ':' || COALESCE(MAX(updated_at)::TEXT, 'none') + FROM sets + WHERE festival_edition_id = p_festival_edition_id; +$$; + +-- Signature is changing (new p_watermark param) -- drop the old overload +-- rather than leaving it reachable without the re-validation. +DROP FUNCTION IF EXISTS public.commit_schedule( + UUID, UUID, JSONB, JSONB, JSONB, JSONB, UUID[] +); + +CREATE OR REPLACE FUNCTION public.commit_schedule( + p_festival_edition_id UUID, + p_user_id UUID, + p_watermark TEXT, -- from diff-schedule's Analyse response, unchanged + p_artists_to_create JSONB, -- [{ name, slug }] + p_stages_to_create JSONB, -- [{ name }] + p_sets_to_create JSONB, -- [{ name, description, stageName, timeStart, timeEnd, artistSlugs }] + p_sets_to_update JSONB, -- [{ id, name, description, stageName, timeStart, timeEnd, artistSlugs }] + p_set_ids_to_archive UUID[] +) +RETURNS JSONB +LANGUAGE plpgsql +SET search_path = public +AS $$ +DECLARE + v_sets_created INT; + v_sets_updated INT; + v_sets_archived INT; + v_current_watermark TEXT; +BEGIN + v_current_watermark := commit_schedule__compute_watermark(p_festival_edition_id); + + IF v_current_watermark IS DISTINCT FROM p_watermark THEN + RAISE EXCEPTION + 'The schedule changed since this review was generated. Start over to re-run Analyse against the latest data -- nothing from this review was applied.'; + END IF; + + PERFORM commit_schedule__upsert_artists(p_artists_to_create, p_user_id); + PERFORM commit_schedule__upsert_stages(p_festival_edition_id, p_stages_to_create); + + v_sets_updated := commit_schedule__update_sets( + p_festival_edition_id, p_sets_to_update + ); + v_sets_created := commit_schedule__create_sets( + p_festival_edition_id, p_user_id, p_sets_to_create + ); + v_sets_archived := commit_schedule__archive_sets( + p_festival_edition_id, p_set_ids_to_archive + ); + + RETURN jsonb_build_object( + 'setsCreated', v_sets_created, + 'setsUpdated', v_sets_updated, + 'setsArchived', v_sets_archived + ); +END; +$$; + +REVOKE EXECUTE ON FUNCTION public.commit_schedule__compute_watermark(UUID) FROM PUBLIC; +REVOKE EXECUTE ON FUNCTION public.commit_schedule(UUID, UUID, TEXT, JSONB, JSONB, JSONB, JSONB, UUID[]) FROM PUBLIC; + +GRANT EXECUTE ON FUNCTION public.commit_schedule__compute_watermark(UUID) TO service_role; +GRANT EXECUTE ON FUNCTION public.commit_schedule(UUID, UUID, TEXT, JSONB, JSONB, JSONB, JSONB, UUID[]) TO service_role; From 43602ac5e68a2330ed88e5ea0d8ad7aca152c9c8 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 05:22:09 +0000 Subject: [PATCH 2/3] fix(schedule-import): distinguish the watermark-mismatch error and fix its ordering - Prefix commit_schedule's watermark-mismatch exception with a stable edition_changed_since_analyse marker, and have the review step key off it to show a dedicated "start over, re-run Analyse" message instead of a generic failure with a doomed Retry button. - Compute diff-schedule's watermark before (not alongside, via Promise.all) the sets read it certifies, so a concurrent edit can no longer land in the gap between the two reads and slip past the Commit-time re-check. --- .../ScheduleImport/DiffReviewStep.test.tsx | 71 +++++++++++++++++++ .../Admin/ScheduleImport/DiffReviewStep.tsx | 22 ++++-- src/services/scheduleImport/types.test.ts | 19 +++++ src/services/scheduleImport/types.ts | 10 +++ .../commit-schedule/commit-schedule.test.ts | 5 ++ supabase/functions/diff-schedule/index.ts | 11 ++- ...260901000000_commit_schedule_watermark.sql | 5 +- 7 files changed, 136 insertions(+), 7 deletions(-) create mode 100644 src/components/Admin/ScheduleImport/DiffReviewStep.test.tsx create mode 100644 src/services/scheduleImport/types.test.ts diff --git a/src/components/Admin/ScheduleImport/DiffReviewStep.test.tsx b/src/components/Admin/ScheduleImport/DiffReviewStep.test.tsx new file mode 100644 index 000000000..1e367a12c --- /dev/null +++ b/src/components/Admin/ScheduleImport/DiffReviewStep.test.tsx @@ -0,0 +1,71 @@ +import { describe, expect, it, vi } from "vitest"; +import { render, screen } from "@testing-library/react"; +import { DiffReviewStep } from "./DiffReviewStep"; +import type { DiffResult } from "@/services/scheduleImport/types"; + +const emptyDiff: DiffResult = { + watermark: "0:none", + summary: { + newArtists: 0, + newStages: 0, + setsMatched: 0, + setsToCreate: 0, + setsOrphaned: 0, + }, + newArtistNames: [], + cleanOperations: { + artistsToCreate: [], + stagesToCreate: [], + setsToCreate: [], + setsToUpdate: [], + }, + conflicts: { stageNameMismatches: [], orphanedSets: [] }, +}; + +function renderStep(commitError: string | null) { + render( + , + ); +} + +describe("DiffReviewStep", () => { + it("shows a dedicated message and disables the primary button when the edition changed", () => { + renderStep( + "edition_changed_since_analyse: The schedule changed since this review was generated.", + ); + + expect( + screen.getByText("The schedule changed since this review"), + ).toBeVisible(); + expect(screen.queryByText("Retry")).not.toBeInTheDocument(); + expect( + screen.getByRole("button", { name: "Commit to database" }), + ).toBeDisabled(); + }); + + it("shows the generic failure message and an enabled Retry for other commit errors", () => { + renderStep("Stage Mainstage not found in edition edition-1"); + + expect( + screen.getByText("Import failed — no changes were saved."), + ).toBeVisible(); + expect( + screen.getByText("Stage Mainstage not found in edition edition-1"), + ).toBeVisible(); + expect(screen.getByRole("button", { name: "Retry" })).toBeEnabled(); + }); +}); diff --git a/src/components/Admin/ScheduleImport/DiffReviewStep.tsx b/src/components/Admin/ScheduleImport/DiffReviewStep.tsx index bad1893ca..a1cfa8358 100644 --- a/src/components/Admin/ScheduleImport/DiffReviewStep.tsx +++ b/src/components/Admin/ScheduleImport/DiffReviewStep.tsx @@ -6,6 +6,7 @@ import { type DiffResult, type StageMismatchResolution, type OrphanResolution, + isEditionChangedError, } from "@/services/scheduleImport/types"; import type { RevealLevel } from "@/lib/scheduleReveal"; import { DiffSummaryBanner } from "./DiffSummaryBanner"; @@ -53,6 +54,8 @@ export function DiffReviewStep({ const setsToArchive = Object.values(orphanResolutions).filter( (r) => r === "archive", ).length; + const editionChanged = + commitError != null && isEditionChangedError(commitError); return ( @@ -90,8 +93,16 @@ export function DiffReviewStep({ {commitError && ( - Import failed — no changes were saved. - {commitError} + + {editionChanged + ? "The schedule changed since this review" + : "Import failed — no changes were saved."} + + + {editionChanged + ? "Someone changed this edition's schedule after Analyse ran. Nothing was applied — click Start over to re-run Analyse against the latest data." + : commitError} + )} @@ -99,13 +110,16 @@ export function DiffReviewStep({ -