Guard plan drafts against swaps and keep confirm on single-flight

Return the draft job id as jobId, as the plan API contract specifies. Add a week_plans revision (migration 0013) that every swap and stored draft raises. A draft_week job reads the revision before draft_week runs and stores its week only while the plan still has it; a plan edited while the AI ran is left as it is and the job fails plan_changed, so a draft never erases the household's swap. A confirmed draft now joins only a confirmed queued or running job, so a job asked for without confirm can no longer fail it with confirm_required after the plan was synced. Test simultaneous drafts, a swap during a draft, a confirmed draft while an unconfirmed one runs, impossible calendar dates and start_by across the end of daylight saving time. Maestro task ea48e650-d510-49cc-8485-0f07174ee731. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-07 06:32:51 -04:00
parent 75d6ad4e19
commit 7e6e49f6b1
12 changed files with 352 additions and 45 deletions
@@ -35,7 +35,8 @@ func (r draftResult) PublicJSON() ([]byte, error) { return json.Marshal(r) }
 // runDraft drafts the week p.WeekStart: draft_week plans it from the
 // Regulars and the Quest log dinners no other current or later plan has,
 // given the household context and last week's dinners, and the draft is
-// stored as the week's plan. A pool under seven dinners fails with
+// stored as the week's plan if the plan is still as it was before
+// draft_week ran. A pool under seven dinners fails with
 // ReasonNotEnoughDinners and draft_week failing with ReasonAIFailed; no
 // week is made up instead.
 func (s *Service) runDraft(ctx context.Context, p DraftJob, _ func(draftResult) error) (draftResult, error) {
@@ -43,6 +44,10 @@ func (s *Service) runDraft(ctx context.Context, p DraftJob, _ func(draftResult)
 	if err != nil {
 		return draftResult{}, fmt.Errorf("plan: draft: %w", err)
 	}
+	base, err := s.revision(ctx, weekStart)
+	if err != nil {
+		return draftResult{}, err
+	}
 	in, err := s.request(ctx, weekStart)
 	if err != nil {
 		return draftResult{}, err
@@ -63,13 +68,29 @@ func (s *Service) runDraft(ctx context.Context, p DraftJob, _ func(draftResult)
 	for _, w := range week.Warnings {
 		s.logger.InfoContext(ctx, "week draft warning", "week_start", p.WeekStart, "warning", w)
 	}
-	planID, err := s.storeDraft(ctx, weekStart, p.Confirm, week)
+	planID, err := s.storeDraft(ctx, weekStart, p.Confirm, base, week)
 	if err != nil {
 		return draftResult{}, err
 	}
 	return draftResult{WeekStart: p.WeekStart, PlanID: planID}, nil
 }

+// noPlan is the revision of a week with no plan.
+const noPlan = -1
+
+// revision returns the revision of the plan of the week starting
+// weekStart, or noPlan when the week has none.
+func (s *Service) revision(ctx context.Context, weekStart store.LocalDate) (int64, error) {
+	cur, err := db.New(s.st.Reader).GetWeekPlan(ctx, weekStart.String())
+	if errors.Is(err, sql.ErrNoRows) {
+		return noPlan, nil
+	}
+	if err != nil {
+		return 0, fmt.Errorf("plan: read week %s: %w", weekStart, err)
+	}
+	return cur.Revision, nil
+}
+
 // request reads what draft_week is given for the week starting weekStart:
 // the household context, the pool, last week's dinners and the dinner
 // time.
@@ -119,8 +140,10 @@ func (s *Service) request(ctx context.Context, weekStart store.LocalDate) (skill
 // event id; a draft plan stays draft and a synced or changed one becomes
 // changed, since Calendar no longer shows it. A plan synced since the
 // draft was asked for without confirm is left as it is, failing with
-// ReasonConfirmRequired.
-func (s *Service) storeDraft(ctx context.Context, weekStart store.LocalDate, confirm bool, week skills.Week) (string, error) {
+// ReasonConfirmRequired, and so is a plan no longer at revision base (the
+// revision before draft_week ran, or noPlan), failing with
+// ReasonPlanChanged.
+func (s *Service) storeDraft(ctx context.Context, weekStart store.LocalDate, confirm bool, base int64, week skills.Week) (string, error) {
 	var planID string
 	err := s.st.WithTx(ctx, func(tx *sql.Tx) error {
 		q := db.New(tx)
@@ -128,6 +151,8 @@ func (s *Service) storeDraft(ctx context.Context, weekStart store.LocalDate, con
 		note := sql.NullString{String: week.Note, Valid: week.Note != ""}
 		cur, err := q.GetWeekPlan(ctx, weekStart.String())
 		switch {
+		case errors.Is(err, sql.ErrNoRows) && base != noPlan:
+			return &jobs.Failure{Code: ReasonPlanChanged}
 		case errors.Is(err, sql.ErrNoRows):
 			planID = store.NewID()
 			if err := q.CreateWeekPlan(ctx, db.CreateWeekPlanParams{
@@ -143,6 +168,8 @@ func (s *Service) storeDraft(ctx context.Context, weekStart store.LocalDate, con
 			return fmt.Errorf("plan: read week %s: %w", weekStart, err)
 		case cur.Status != StatusDraft && !confirm:
 			return &jobs.Failure{Code: ReasonConfirmRequired}
+		case cur.Revision != base:
+			return &jobs.Failure{Code: ReasonPlanChanged}
 		default:
 			planID = cur.ID
 			status := StatusDraft
@@ -2,7 +2,8 @@
 // drafts from the Regulars and the Quest log, and the swaps the household
 // makes on it. A draft runs as a job: draft_week plans seven dinners, one
 // a day from Monday to Sunday, and the job stores them as the week's plan,
-// replacing any draft before it. A plan is draft until Push to Calendar
+// replacing any draft before it unless the household edited the plan
+// while draft_week ran. A plan is draft until Push to Calendar
 // syncs it (Feature 16); an edit of a synced plan makes it changed.
 package plan

@@ -45,10 +46,13 @@ const (
 	// ReasonConfirmRequired: the plan was synced after the draft was
 	// asked for without confirm, so the draft does not replace it.
 	ReasonConfirmRequired = "confirm_required"
+	// ReasonPlanChanged: the plan was edited while draft_week ran, so the
+	// draft does not replace the edit.
+	ReasonPlanChanged = "plan_changed"
 )

 // Reasons are every draft failure reason.
-var Reasons = []string{ReasonNotEnoughDinners, ReasonAIFailed, ReasonConfirmRequired}
+var Reasons = []string{ReasonNotEnoughDinners, ReasonAIFailed, ReasonConfirmRequired, ReasonPlanChanged}

 // ContextBudget is the character budget of the household context
 // draft_week is given (see harness.BuildContext).
@@ -265,15 +269,16 @@ func startBy(zone *time.Location, date store.LocalDate, dinnerTime string, d db.
 // Draft is a draft asked for: the job drafting the week, and whether this
 // call enqueued it rather than found one already queued or running.
 type Draft struct {
-	JobID   string `json:"job_id"`
+	JobID   string `json:"jobId"`
 	Created bool   `json:"-"`
 }

 // StartDraft enqueues the KindDraftWeek job drafting the week starting
 // weekStart, unless one is already queued or running for it, which it
-// returns instead with Created false. Drafting over a synced or changed
-// plan needs confirm, else it fails with ErrConfirmRequired and enqueues
-// nothing.
+// returns instead with Created false. A confirmed draft joins only a
+// confirmed job, so a job asked for without confirm cannot fail it with
+// ReasonConfirmRequired. Drafting over a synced or changed plan needs
+// confirm, else it fails with ErrConfirmRequired and enqueues nothing.
 func (s *Service) StartDraft(ctx context.Context, weekStart store.LocalDate, confirm bool) (Draft, error) {
 	if _, err := household.WeekDays(weekStart); err != nil {
 		return Draft{}, &InvalidWeekError{Value: weekStart.String()}
@@ -289,7 +294,11 @@ func (s *Service) StartDraft(ctx context.Context, weekStart store.LocalDate, con
 		case cur.Status != StatusDraft && !confirm:
 			return ErrConfirmRequired
 		}
-		id, err := q.GetActiveDraftJobID(ctx, db.GetActiveDraftJobIDParams{Kind: KindDraftWeek, WeekStart: weekStart.String()})
+		confirmed := int64(0)
+		if confirm {
+			confirmed = 1
+		}
+		id, err := q.GetActiveDraftJobID(ctx, db.GetActiveDraftJobIDParams{Kind: KindDraftWeek, WeekStart: weekStart.String(), Confirmed: confirmed})
 		if err == nil {
 			d = Draft{JobID: id}
 			return nil
@@ -355,6 +355,60 @@ func TestStartByIsDinnerTimeMinusMinutesForCrockPot(t *testing.T) {
 	}
 }

+func TestStartByAcrossDaylightSavingTimeEnd(t *testing.T) {
+	e := newEnv(t)
+	// New York leaves daylight saving time at 02:00 on Sunday 2026-11-01.
+	// Beef Chili (240 minutes) is planned Monday to Saturday and a 20-hour
+	// crock-pot dinner on Sunday, so its start crosses the change.
+	stew := e.dinner(t, "Overnight Stew", "regular", "crockpot", 1200)
+	q := db.New(e.st.Writer)
+	now := store.Now().String()
+	id := store.NewID()
+	dst := mustDate("2026-10-26")
+	if err := q.CreateWeekPlan(t.Context(), db.CreateWeekPlanParams{ID: id, WeekStart: dst.String(), CreatedAt: now, UpdatedAt: now}); err != nil {
+		t.Fatal(err)
+	}
+	dates := []string{"2026-10-26", "2026-10-27", "2026-10-28", "2026-10-29", "2026-10-30", "2026-10-31", "2026-11-01"}
+	for i, date := range dates {
+		dinner := e.ids["Beef Chili"]
+		if i == 6 {
+			dinner = stew
+		}
+		if err := q.PutPlanDay(t.Context(), db.PutPlanDayParams{ID: store.NewID(), PlanID: id, Date: date,
+			DinnerID: sql.NullString{String: dinner, Valid: true}, CreatedAt: now, UpdatedAt: now}); err != nil {
+			t.Fatal(err)
+		}
+	}
+	p, err := e.svc.Get(t.Context(), dst)
+	if err != nil {
+		t.Fatal(err)
+	}
+	// Saturday's dinner is 17:30 EDT; Sunday's is 17:30 EST, 22:30 UTC, and
+	// twenty hours before it is 22:30 EDT on Saturday.
+	for _, tc := range []struct {
+		day  int
+		want string
+	}{
+		{5, `"2026-10-31T13:30:00-04:00"`},
+		{6, `"2026-10-31T22:30:00-04:00"`},
+	} {
+		b, err := json.Marshal(p.Days[tc.day].StartBy)
+		if err != nil || string(b) != tc.want {
+			t.Errorf("%s start_by = %s, %v; want %s", dates[tc.day], b, err, tc.want)
+		}
+	}
+	// Swapped to Beef Chili, Sunday starts at 13:30 EST.
+	if _, err := e.svc.Swap(t.Context(), dst, "2026-11-01", e.ids["Beef Chili"]); err != nil {
+		t.Fatal(err)
+	}
+	if p, err = e.svc.Get(t.Context(), dst); err != nil {
+		t.Fatal(err)
+	}
+	if b, err := json.Marshal(p.Days[6].StartBy); err != nil || string(b) != `"2026-11-01T13:30:00-05:00"` {
+		t.Errorf("Sunday Beef Chili start_by = %s, %v; want 13:30 EST", b, err)
+	}
+}
+
 func TestDraftGivesLastWeek(t *testing.T) {
 	e := newEnv(t)
 	e.start(t)
@@ -464,6 +518,148 @@ func TestStartDraftIsSingleFlight(t *testing.T) {
 	}
 }

+func TestSimultaneousStartDraftsShareOneJobPerConfirm(t *testing.T) {
+	e := newEnv(t)
+	first, err := e.svc.StartDraft(t.Context(), week, false)
+	if err != nil {
+		t.Fatal(err)
+	}
+	// Twenty simultaneous drafts, half of them confirmed: the unconfirmed
+	// ones join the queued job, and the confirmed ones share one confirmed
+	// job of their own.
+	const n = 20
+	drafts := make([]plan.Draft, n)
+	errs := make([]error, n)
+	var wg sync.WaitGroup
+	for i := range n {
+		wg.Go(func() {
+			drafts[i], errs[i] = e.svc.StartDraft(context.Background(), week, i%2 == 1)
+		})
+	}
+	wg.Wait()
+	confirmedJob := ""
+	created := 0
+	for i, d := range drafts {
+		if errs[i] != nil {
+			t.Fatalf("draft %d: %v", i, errs[i])
+		}
+		if d.Created {
+			created++
+		}
+		if i%2 == 0 {
+			if d.JobID != first.JobID || d.Created {
+				t.Errorf("unconfirmed draft %d = %+v, want it to join %s", i, d, first.JobID)
+			}
+			continue
+		}
+		if confirmedJob == "" {
+			confirmedJob = d.JobID
+		}
+		if d.JobID != confirmedJob || d.JobID == first.JobID {
+			t.Errorf("confirmed draft %d = %+v, want the one confirmed job %s, not %s", i, d, confirmedJob, first.JobID)
+		}
+	}
+	if created != 1 {
+		t.Errorf("%d drafts enqueued a job, want 1", created)
+	}
+	var payload string
+	if err := e.st.Reader.QueryRowContext(t.Context(), `SELECT payload FROM jobs WHERE id = ?`, confirmedJob).Scan(&payload); err != nil {
+		t.Fatal(err)
+	}
+	if !strings.Contains(payload, `"confirm":true`) {
+		t.Errorf("confirmed job payload = %s, want confirm", payload)
+	}
+}
+
+func TestConfirmedDraftDoesNotJoinUnconfirmedJob(t *testing.T) {
+	e := newEnv(t)
+	e.start(t)
+	e.reply("first", fullWeek...)
+	e.mustDraft(t, false)
+
+	// An unconfirmed redraft is running when the plan is synced; the
+	// household then confirms a redraft, which must not join the job that
+	// will fail confirm_required.
+	reversed := slices.Clone(fullWeek)
+	slices.Reverse(reversed)
+	started, release := make(chan struct{}), make(chan struct{})
+	r := e.weekReply("unconfirmed", fullWeek...)
+	r.Started, r.Release = started, release
+	e.model.QueueGenerate(r)
+	e.reply("confirmed", reversed...)
+	unconfirmed, err := e.svc.StartDraft(t.Context(), week, false)
+	if err != nil {
+		t.Fatal(err)
+	}
+	<-started
+	e.setStatus(t, plan.StatusSynced)
+	confirmed, err := e.svc.StartDraft(t.Context(), week, true)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !confirmed.Created || confirmed.JobID == unconfirmed.JobID {
+		t.Fatalf("confirmed draft = %+v, want its own job, not %s", confirmed, unconfirmed.JobID)
+	}
+	close(release)
+	if job := e.waitJob(t, unconfirmed.JobID); job.Status != jobs.StatusFailed || *job.Error != plan.ReasonConfirmRequired {
+		t.Errorf("unconfirmed job %s %v, want failed %s", job.Status, job.Error, plan.ReasonConfirmRequired)
+	}
+	if job := e.waitJob(t, confirmed.JobID); job.Status != jobs.StatusSucceeded {
+		t.Fatalf("confirmed job %s %v, want succeeded", job.Status, job.Error)
+	}
+	p, err := e.svc.Get(t.Context(), week)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if p.Status != plan.StatusChanged || *p.AINote != "confirmed" || !slices.Equal(dayNames(p), reversed) {
+		t.Errorf("plan = %s %q %v, want the confirmed draft, changed", p.Status, *p.AINote, dayNames(p))
+	}
+}
+
+func TestDraftFailsWhenPlanSwappedWhileDrafting(t *testing.T) {
+	e := newEnv(t)
+	e.start(t)
+	e.reply("first", fullWeek...)
+	e.mustDraft(t, false)
+
+	// The household swaps Wednesday while draft_week runs; the draft must
+	// not erase the swap.
+	reversed := slices.Clone(fullWeek)
+	slices.Reverse(reversed)
+	started, release := make(chan struct{}), make(chan struct{})
+	r := e.weekReply("second", reversed...)
+	r.Started, r.Release = started, release
+	e.model.QueueGenerate(r)
+	d, err := e.svc.StartDraft(t.Context(), week, false)
+	if err != nil {
+		t.Fatal(err)
+	}
+	<-started
+	if _, err := e.svc.Swap(t.Context(), week, "2026-10-14", e.ids["Sheet Pan Salmon"]); err != nil {
+		t.Fatal(err)
+	}
+	close(release)
+	job := e.waitJob(t, d.JobID)
+	if job.Status != jobs.StatusFailed || *job.Error != plan.ReasonPlanChanged {
+		t.Fatalf("job %s %v, want failed %s", job.Status, job.Error, plan.ReasonPlanChanged)
+	}
+	p, err := e.svc.Get(t.Context(), week)
+	if err != nil {
+		t.Fatal(err)
+	}
+	want := slices.Clone(fullWeek)
+	want[2] = "Sheet Pan Salmon"
+	if *p.AINote != "first" || !slices.Equal(dayNames(p), want) {
+		t.Errorf("plan = %q %v, want the first draft with the swap %v", *p.AINote, dayNames(p), want)
+	}
+
+	// A draft after the swap replaces the plan.
+	e.reply("third", reversed...)
+	if p := e.mustDraft(t, false); *p.AINote != "third" || !slices.Equal(dayNames(p), reversed) {
+		t.Errorf("redraft after the swap = %q %v, want the third draft", *p.AINote, dayNames(p))
+	}
+}
+
 func TestRedraftOverDraftNeedsNoConfirm(t *testing.T) {
 	e := newEnv(t)
 	e.start(t)
@@ -564,7 +760,8 @@ func TestParseWeekStart(t *testing.T) {
 	if d, err := plan.ParseWeekStart("2026-10-12"); err != nil || d != week {
 		t.Errorf("ParseWeekStart(Monday) = %v, %v", d, err)
 	}
-	for _, s := range []string{"2026-10-13", "2026-10-18", "2026-10-5", "next-week", ""} {
+	// 2026-02-30 and 2027-02-29 would be Mondays were they dates.
+	for _, s := range []string{"2026-10-13", "2026-10-18", "2026-10-5", "2026-02-30", "2027-02-29", "2026-13-05", "next-week", ""} {
 		var invalid *plan.InvalidWeekError
 		if _, err := plan.ParseWeekStart(s); !errors.As(err, &invalid) {
 			t.Errorf("ParseWeekStart(%q) = %v, want *InvalidWeekError", s, err)
@@ -661,6 +858,7 @@ func TestSwapRefusals(t *testing.T) {
 		{"archived dinner", week, "2026-10-12", archived, func(err error) bool { return errors.As(err, &notPlannable) }},
 		{"day of another week", week, "2026-10-19", e.ids["Pad Thai"], func(err error) bool { return errors.As(err, &badDay) }},
 		{"not a date", week, "monday", e.ids["Pad Thai"], func(err error) bool { return errors.As(err, &badDay) }},
+		{"impossible date", week, "2026-10-32", e.ids["Pad Thai"], func(err error) bool { return errors.As(err, &badDay) }},
 	} {
 		if _, err := e.svc.Swap(t.Context(), tc.week, tc.date, tc.id); !tc.wantError(err) {
 			t.Errorf("%s: Swap = %v", tc.name, err)