From 7e6e49f6b1446186a39aabc665bd145cf5945c64 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Wed, 7 Oct 2026 06:32:51 -0400 Subject: [PATCH] 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 --- docs/ARCHITECTURE.md | 12 +- internal/db/models.go | 1 + internal/db/plans.sql.go | 20 ++- internal/db/querier.go | 10 +- internal/plan/draft.go | 35 ++++- internal/plan/plan.go | 23 ++- internal/plan/plan_test.go | 200 ++++++++++++++++++++++++- internal/server/plans.go | 6 +- internal/server/plans_test.go | 62 +++++++- internal/store/migrate_test.go | 8 +- migrations/0013_week_plan_revision.sql | 6 + migrations/queries/plans.sql | 14 +- 12 files changed, 352 insertions(+), 45 deletions(-) create mode 100644 migrations/0013_week_plan_revision.sql diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 4f4febd..985f135 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -99,7 +99,7 @@ Columns ending in `_ct` hold AES-256-GCM ciphertext (via `secretbox`). | `dinners` | The meal record and its lifecycle: name, normalized_name, status (`suggested` / `quest_log` / `tried` / `regular` / `archived`), origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, recipe_url (a pasted recipe link awaiting grounding), photo_id, grounding_status (`pending` / `grounded` / `failed` / `manual`), grounding_error (why grounding failed), grounding_revision (raised whenever a dinner is renamed, so only the latest `ground_dinner` job may write it and no candidate offered before the rename grounds it), updated_by. | | `suggestion_batches` | trigger, status, ai_run_id. | | `suggestions` | dinner_id, batch_id, name (the name the dish was suggested under, kept whatever its dinner is later renamed, so a Pass stays an immutable record every later batch dedupes against), reason, cited_regular_ids (JSON), status, decided_at. | -| `week_plans` | week_start (unique), status (`draft` / `synced` / `changed`), ai_note, ai_run_id, calendar_synced_at. | +| `week_plans` | week_start (unique), status (`draft` / `synced` / `changed`), ai_note, ai_run_id, calendar_synced_at, revision (raised by every swap and stored draft). | | `plan_days` | plan_id, date (unique per plan), dinner_id, is_new, calendar_event_id. | | `rotation_reviews` | dinner_id + plan_date (unique), status, decision (`keep` / `drop` / `not_made`), decided_at, updated_by. | | `shopping_lists` | week_start (unique), plan_id, title, status (`current` / `archived`), sent_to_chat_at, frozen_at. | @@ -344,15 +344,19 @@ automatic** (D-021). dinner time on that date minus `minutes_total`, computed on read, for a crock-pot dinner with known minutes, else null. `POST /api/v1/plans/{weekStart}/draft` takes `{confirm?}` and answers 202 - `{job_id}` for a `draft_week` job, or for the one already queued or - running for the week. The job runs the `draft_week` skill on every Regular + `{jobId}` for a `draft_week` job, or for the one already queued or + running for the week — a confirmed one when the draft is confirmed, so a + confirmed draft never joins an unconfirmed job. The job runs the `draft_week` skill on every Regular and every Quest log dinner no other plan for the current or a later week has, with the household context and last week's dinners, and stores the week: a new plan is `draft`; a redraft keeps each day's `calendar_event_id`, and a `synced` or `changed` plan becomes `changed`. Drafting over a `synced` or `changed` plan needs `{"confirm": true}`, else 409 `confirm_required`; a plan synced while an unconfirmed draft runs is - left as it is and the job fails `confirm_required`. A pool under seven + left as it is and the job fails `confirm_required`. The job reads the + plan's revision before draft_week runs; if the plan was edited (a swap, + or another draft stored) by the time the draft is stored, the plan is + left as it is and the job fails `plan_changed`. A pool under seven dinners fails the job `not_enough_dinners`, draft_week failing `ai_failed`. `PUT /api/v1/plans/{weekStart}/days/{date}` takes `{dinner_id}`, a Regular or Quest log dinner, swaps the day, recomputes diff --git a/internal/db/models.go b/internal/db/models.go index a75716a..6a3998f 100644 --- a/internal/db/models.go +++ b/internal/db/models.go @@ -316,4 +316,5 @@ type WeekPlan struct { CalendarSyncedAt sql.NullString CreatedAt string UpdatedAt string + Revision int64 } diff --git a/internal/db/plans.sql.go b/internal/db/plans.sql.go index dedc1b7..71fb67a 100644 --- a/internal/db/plans.sql.go +++ b/internal/db/plans.sql.go @@ -39,6 +39,7 @@ SELECT id FROM jobs WHERE kind = ?1 AND status IN ('queued', 'running') AND json_extract(payload, '$.week_start') = ?2 + AND (CAST(?3 AS INTEGER) = 0 OR json_extract(payload, '$.confirm') = 1) ORDER BY id LIMIT 1 ` @@ -46,19 +47,21 @@ LIMIT 1 type GetActiveDraftJobIDParams struct { Kind string WeekStart string + Confirmed int64 } // The id of the job of kind queued or running for the week starting -// week_start. Returns sql.ErrNoRows when there is none. +// week_start, one whose payload has confirm when confirmed is 1. Returns +// sql.ErrNoRows when there is none. func (q *Queries) GetActiveDraftJobID(ctx context.Context, arg GetActiveDraftJobIDParams) (string, error) { - row := q.db.QueryRowContext(ctx, getActiveDraftJobID, arg.Kind, arg.WeekStart) + row := q.db.QueryRowContext(ctx, getActiveDraftJobID, arg.Kind, arg.WeekStart, arg.Confirmed) var id string err := row.Scan(&id) return id, err } const getWeekPlan = `-- name: GetWeekPlan :one -SELECT id, week_start, status, ai_note, ai_run_id, calendar_synced_at, created_at, updated_at FROM week_plans +SELECT id, week_start, status, ai_note, ai_run_id, calendar_synced_at, created_at, updated_at, revision FROM week_plans WHERE week_start = ? ` @@ -76,6 +79,7 @@ func (q *Queries) GetWeekPlan(ctx context.Context, weekStart string) (WeekPlan, &i.CalendarSyncedAt, &i.CreatedAt, &i.UpdatedAt, + &i.Revision, ) return i, err } @@ -241,6 +245,7 @@ func (q *Queries) ListWeekPlanDinnerIDs(ctx context.Context, weekStart string) ( const markWeekPlanEdited = `-- name: MarkWeekPlanEdited :exec UPDATE week_plans SET status = CASE WHEN status = 'synced' THEN 'changed' ELSE status END, + revision = revision + 1, updated_at = ? WHERE id = ? ` @@ -250,8 +255,8 @@ type MarkWeekPlanEditedParams struct { ID string } -// Records an edit of plan id: a synced plan becomes changed, any other -// keeps its status. +// Records an edit of plan id, raising its revision: a synced plan becomes +// changed, any other keeps its status. func (q *Queries) MarkWeekPlanEdited(ctx context.Context, arg MarkWeekPlanEditedParams) error { _, err := q.db.ExecContext(ctx, markWeekPlanEdited, arg.UpdatedAt, arg.ID) return err @@ -291,7 +296,7 @@ func (q *Queries) PutPlanDay(ctx context.Context, arg PutPlanDayParams) error { const redraftWeekPlan = `-- name: RedraftWeekPlan :exec UPDATE week_plans -SET status = ?, ai_note = ?, updated_at = ? +SET status = ?, ai_note = ?, revision = revision + 1, updated_at = ? WHERE id = ? ` @@ -302,7 +307,8 @@ type RedraftWeekPlanParams struct { ID string } -// Records a new draft over plan id: its status and note. +// Records a new draft over plan id: its status and note, raising its +// revision. func (q *Queries) RedraftWeekPlan(ctx context.Context, arg RedraftWeekPlanParams) error { _, err := q.db.ExecContext(ctx, redraftWeekPlan, arg.Status, diff --git a/internal/db/querier.go b/internal/db/querier.go index b6c4191..b5abfe1 100644 --- a/internal/db/querier.go +++ b/internal/db/querier.go @@ -69,7 +69,8 @@ type Querier interface { // Returns sql.ErrNoRows until the row exists. GetAISettings(ctx context.Context) (AiSetting, error) // The id of the job of kind queued or running for the week starting - // week_start. Returns sql.ErrNoRows when there is none. + // week_start, one whose payload has confirm when confirmed is 1. Returns + // sql.ErrNoRows when there is none. GetActiveDraftJobID(ctx context.Context, arg GetActiveDraftJobIDParams) (string, error) // The batch still pending or running, oldest first. Returns sql.ErrNoRows // when there is none. @@ -148,8 +149,8 @@ type Querier interface { // The ids of the dinners planned in the week starting week_start, Monday // first. ListWeekPlanDinnerIDs(ctx context.Context, weekStart string) ([]sql.NullString, error) - // Records an edit of plan id: a synced plan becomes changed, any other - // keeps its status. + // Records an edit of plan id, raising its revision: a synced plan becomes + // changed, any other keeps its status. MarkWeekPlanEdited(ctx context.Context, arg MarkWeekPlanEditedParams) error // Moves a dinner still suggested to status (quest_log or archived), // recording updated_by; a dinner no longer suggested is left as it is @@ -160,7 +161,8 @@ type Querier interface { // Plans dinner_id on date of plan_id, keeping the day's calendar event id // when the day exists. PutPlanDay(ctx context.Context, arg PutPlanDayParams) error - // Records a new draft over plan id: its status and note. + // Records a new draft over plan id: its status and note, raising its + // revision. RedraftWeekPlan(ctx context.Context, arg RedraftWeekPlanParams) error // An unknown id returns sql.ErrNoRows. SetDinnerFavorite(ctx context.Context, arg SetDinnerFavoriteParams) (Dinner, error) diff --git a/internal/plan/draft.go b/internal/plan/draft.go index 21fe3c8..f64a569 100644 --- a/internal/plan/draft.go +++ b/internal/plan/draft.go @@ -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 diff --git a/internal/plan/plan.go b/internal/plan/plan.go index b3c8d0e..d0a154c 100644 --- a/internal/plan/plan.go +++ b/internal/plan/plan.go @@ -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 diff --git a/internal/plan/plan_test.go b/internal/plan/plan_test.go index 1d02aa7..c29641f 100644 --- a/internal/plan/plan_test.go +++ b/internal/plan/plan_test.go @@ -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, ¬Plannable) }}, {"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) diff --git a/internal/server/plans.go b/internal/server/plans.go index b2cd605..e6ed165 100644 --- a/internal/server/plans.go +++ b/internal/server/plans.go @@ -43,9 +43,9 @@ func getPlan(logger *slog.Logger, svc *plan.Service) http.HandlerFunc { } // postPlanDraft enqueues the draft of the week and answers 202 with the -// job_id that GET /jobs/{id} reports on, without waiting for the job. When -// a draft of the week is already queued or running it answers 202 with -// that job instead. +// jobId that GET /jobs/{id} reports on, without waiting for the job. When +// a draft of the week is already queued or running, confirmed if this one +// is, it answers 202 with that job instead. func postPlanDraft(logger *slog.Logger, svc *plan.Service) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { week, ok := weekStartOrFail(w, r, logger) diff --git a/internal/server/plans_test.go b/internal/server/plans_test.go index f0577ec..7b314a1 100644 --- a/internal/server/plans_test.go +++ b/internal/server/plans_test.go @@ -3,10 +3,14 @@ package server import ( "context" "database/sql" + "encoding/json" "errors" "io" "log/slog" "net/http" + "net/http/httptest" + "strings" + "sync" "testing" "testing/fstest" @@ -141,6 +145,8 @@ func TestPlanWeekStartMustBeMonday(t *testing.T) { for _, tc := range []struct{ method, target, body string }{ {http.MethodGet, plansPath + "2026-10-13", ""}, {http.MethodGet, plansPath + "not-a-date", ""}, + {http.MethodGet, plansPath + "2026-02-30", ""}, + {http.MethodPost, plansPath + "2027-02-29/draft", "{}"}, {http.MethodPost, plansPath + "2026-10-18/draft", "{}"}, {http.MethodPut, plansPath + "2026-10-14/days/2026-10-14", `{"dinner_id":"` + e.questLog + `"}`}, } { @@ -159,9 +165,9 @@ func TestPostPlanDraft(t *testing.T) { if rec.Code != http.StatusAccepted { t.Fatalf("POST draft = %d: %s", rec.Code, rec.Body) } - jobID, _ := body["job_id"].(string) - if jobID == "" { - t.Fatalf("POST draft = %v, want a job_id", body) + jobID, _ := body["jobId"].(string) + if jobID == "" || len(body) != 1 { + t.Fatalf("POST draft = %v, want only a jobId", body) } rec, job := callDinners(t, e.h, http.MethodGet, "/api/v1/jobs/"+jobID, "") if rec.Code != http.StatusOK || job["kind"] != plan.KindDraftWeek || job["status"] != "queued" { @@ -169,21 +175,64 @@ func TestPostPlanDraft(t *testing.T) { } // A second draft of the week joins the queued one. rec, body = callDinners(t, e.h, http.MethodPost, plansPath+"2026-10-12/draft", `{"confirm":false}`) - if rec.Code != http.StatusAccepted || body["job_id"] != jobID { + if rec.Code != http.StatusAccepted || body["jobId"] != jobID { t.Errorf("second POST draft = %d %v, want 202 with job %s", rec.Code, body, jobID) } // A week with no plan needs no confirm. rec, body = callDinners(t, e.h, http.MethodPost, plansPath+"2026-10-19/draft", "{}") - if rec.Code != http.StatusAccepted || body["job_id"] == jobID { + if rec.Code != http.StatusAccepted || body["jobId"] == jobID { t.Errorf("POST draft of a new week = %d %v, want 202 with its own job", rec.Code, body) } + // A confirmed draft does not join the unconfirmed one, but a second + // confirmed draft joins the first. + rec, body = callDinners(t, e.h, http.MethodPost, plansPath+"2026-10-12/draft", `{"confirm":true}`) + confirmedID, _ := body["jobId"].(string) + if rec.Code != http.StatusAccepted || confirmedID == "" || confirmedID == jobID { + t.Errorf("confirmed POST draft = %d %v, want 202 with its own job", rec.Code, body) + } + rec, body = callDinners(t, e.h, http.MethodPost, plansPath+"2026-10-12/draft", `{"confirm":true}`) + if rec.Code != http.StatusAccepted || body["jobId"] != confirmedID { + t.Errorf("second confirmed POST draft = %d %v, want 202 with job %s", rec.Code, body, confirmedID) + } + rec, body = callDinners(t, e.h, http.MethodPost, plansPath+"2026-10-12/draft", `{"confirm":"yes"}`) if rec.Code != http.StatusBadRequest || errorCode(body) != "invalid_request" { t.Errorf("POST draft with a bad confirm = %d %v, want 400", rec.Code, body) } } +func TestSimultaneousPostPlanDraftsShareOneJob(t *testing.T) { + t.Parallel() + e := newPlansRouter(t) + + const n = 10 + recs := make([]*httptest.ResponseRecorder, n) + var wg sync.WaitGroup + for i := range n { + wg.Go(func() { + recs[i] = httptest.NewRecorder() + e.h.ServeHTTP(recs[i], signedIn(http.MethodPost, plansPath+"2026-10-12/draft", strings.NewReader("{}"))) + }) + } + wg.Wait() + jobIDs := map[any]int{} + for i, rec := range recs { + var body map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil || rec.Code != http.StatusAccepted { + t.Fatalf("POST draft %d = %d %s, want 202", i, rec.Code, rec.Body) + } + jobIDs[body["jobId"]]++ + } + var queued int + if err := e.st.Reader.QueryRowContext(t.Context(), `SELECT COUNT(*) FROM jobs WHERE kind = ?`, plan.KindDraftWeek).Scan(&queued); err != nil { + t.Fatal(err) + } + if len(jobIDs) != 1 || jobIDs[nil] != 0 || queued != 1 { + t.Errorf("jobIds = %v, %d draft jobs; want one jobId for all %d drafts", jobIDs, queued, n) + } +} + func TestPostPlanDraftOverSyncedPlanNeedsConfirm(t *testing.T) { t.Parallel() for _, status := range []string{"synced", "changed"} { @@ -196,7 +245,7 @@ func TestPostPlanDraftOverSyncedPlanNeedsConfirm(t *testing.T) { } } rec, body := callDinners(t, e.h, http.MethodPost, plansPath+"2026-10-12/draft", `{"confirm":true}`) - if rec.Code != http.StatusAccepted || body["job_id"] == nil { + if rec.Code != http.StatusAccepted || body["jobId"] == nil { t.Errorf("%s plan: POST draft with confirm = %d %v, want 202", status, rec.Code, body) } } @@ -228,6 +277,7 @@ func TestPutPlanDay(t *testing.T) { {"2026-10-12/days/2026-10-14", `{"dinner_id":7}`, http.StatusBadRequest, "invalid_request"}, {"2026-10-12/days/2026-10-14", `{"dinner_id":"x","extra":1}`, http.StatusBadRequest, "invalid_request"}, {"2026-10-12/days/2026-10-20", `{"dinner_id":"` + e.questLog + `"}`, http.StatusBadRequest, "invalid_date"}, + {"2026-10-12/days/2026-10-32", `{"dinner_id":"` + e.questLog + `"}`, http.StatusBadRequest, "invalid_date"}, {"2026-10-12/days/2026-10-14", `{"dinner_id":"no-such-dinner"}`, http.StatusNotFound, "not_found"}, {"2026-10-19/days/2026-10-20", `{"dinner_id":"` + e.questLog + `"}`, http.StatusNotFound, "not_found"}, } { diff --git a/internal/store/migrate_test.go b/internal/store/migrate_test.go index 4952f3b..9662138 100644 --- a/internal/store/migrate_test.go +++ b/internal/store/migrate_test.go @@ -176,8 +176,8 @@ func TestMigrateEmbeddedMigrations(t *testing.T) { if err := Migrate(context.Background(), s.Writer, migrations.FS); err != nil { t.Fatal(err) } - if got := versions(migrationRows(t, s)); got != "0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name" { - t.Errorf("versions = %q, want 0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name", got) + if got := versions(migrationRows(t, s)); got != "0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision" { + t.Errorf("versions = %q, want 0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision", got) } } @@ -197,8 +197,8 @@ func TestMigrateBootstrapThenEmbeddedInitApplies(t *testing.T) { if err := Migrate(ctx, s.Writer, migrations.FS); err != nil { t.Fatal(err) } - if got := versions(migrationRows(t, s)); got != "0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name" { - t.Errorf("versions = %q, want 0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name", got) + if got := versions(migrationRows(t, s)); got != "0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision" { + t.Errorf("versions = %q, want 0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision", got) } if !tableExists(t, s, "dinners") { t.Error("table from 0001_init was not created") diff --git a/migrations/0013_week_plan_revision.sql b/migrations/0013_week_plan_revision.sql new file mode 100644 index 0000000..733caeb --- /dev/null +++ b/migrations/0013_week_plan_revision.sql @@ -0,0 +1,6 @@ +-- 0013_week_plan_revision: every edit of a week's plan — a swap or a +-- stored draft — raises its revision. A draft_week job reads the revision +-- before draft_week runs and stores its week only while the plan still +-- has it, so a draft never overwrites an edit made while the AI ran. + +ALTER TABLE week_plans ADD COLUMN revision INTEGER NOT NULL DEFAULT 0; diff --git a/migrations/queries/plans.sql b/migrations/queries/plans.sql index 3d049da..ef18a9c 100644 --- a/migrations/queries/plans.sql +++ b/migrations/queries/plans.sql @@ -18,9 +18,10 @@ INSERT INTO week_plans (id, week_start, status, ai_note, created_at, updated_at) VALUES (?, ?, 'draft', ?, ?, ?); -- name: RedraftWeekPlan :exec --- Records a new draft over plan id: its status and note. +-- Records a new draft over plan id: its status and note, raising its +-- revision. UPDATE week_plans -SET status = ?, ai_note = ?, updated_at = ? +SET status = ?, ai_note = ?, revision = revision + 1, updated_at = ? WHERE id = ?; -- name: PutPlanDay :exec @@ -39,10 +40,11 @@ SET dinner_id = ?, is_new = ?, updated_at = ? WHERE plan_id = ? AND date = ?; -- name: MarkWeekPlanEdited :exec --- Records an edit of plan id: a synced plan becomes changed, any other --- keeps its status. +-- Records an edit of plan id, raising its revision: a synced plan becomes +-- changed, any other keeps its status. UPDATE week_plans SET status = CASE WHEN status = 'synced' THEN 'changed' ELSE status END, + revision = revision + 1, updated_at = ? WHERE id = ?; @@ -72,10 +74,12 @@ ORDER BY plan_days.date; -- name: GetActiveDraftJobID :one -- The id of the job of kind queued or running for the week starting --- week_start. Returns sql.ErrNoRows when there is none. +-- week_start, one whose payload has confirm when confirmed is 1. Returns +-- sql.ErrNoRows when there is none. SELECT id FROM jobs WHERE kind = sqlc.arg(kind) AND status IN ('queued', 'running') AND json_extract(payload, '$.week_start') = sqlc.arg(week_start) + AND (CAST(sqlc.arg(confirmed) AS INTEGER) = 0 OR json_extract(payload, '$.confirm') = 1) ORDER BY id LIMIT 1;