From 4e2773703bc645436c2b5b98896440cc619990eb Mon Sep 17 00:00:00 2001 From: dev_agent Date: Tue, 6 Oct 2026 23:47:45 -0400 Subject: [PATCH] Keep Pass history immutable and replenish atomic with decisions Fixes the three review findings on 50399b6 (pull request #102): - A batch now rereads every declined name inside its storing transaction, so a Pass committed after the batch read its input but before it stores can no longer let the passed dish into the deck (SQ-01). - A decision that leaves the deck under five creates the replenish batch and its job in the decision's own transaction, so a decision is never recorded without the replenish it calls for; when they cannot be written, neither is the decision, and a retry decides again (SQ-02). StartBatch commits the batch with its job the same way, replacing the mutex and the fail-on-enqueue cleanup. jobs gains Kind.EnqueueTx and Kind.Wake for this. - Migration 0012 gives each suggestion the name its dish was suggested under; declined names for batches and the household context's passed list read it instead of the dinner's mutable name, so renaming a passed dinner no longer erases it from dedupe (SQ-03). Task: b4f2558c-227f-4627-852b-be5aa190242a Co-Authored-By: Claude Opus 5.5 --- docs/ARCHITECTURE.md | 21 ++-- internal/ai/harness/context_test.go | 6 +- internal/db/household_context.sql.go | 10 +- internal/db/models.go | 1 + internal/db/querier.go | 14 ++- internal/db/suggestions.sql.go | 30 +++-- internal/jobs/jobs.go | 34 ++++- internal/jobs/jobs_test.go | 40 ++++++ internal/server/suggestions_test.go | 2 +- internal/store/migrate_test.go | 84 ++++++++++++- internal/store/repository_test.go | 2 +- internal/store/schema_test.go | 1 + internal/suggest/batch.go | 12 +- internal/suggest/suggest.go | 131 ++++++++++---------- internal/suggest/suggest_test.go | 150 +++++++++++++++++++++++ migrations/0012_suggestion_name.sql | 37 ++++++ migrations/queries/household_context.sql | 10 +- migrations/queries/suggestions.sql | 22 ++-- 18 files changed, 482 insertions(+), 125 deletions(-) create mode 100644 migrations/0012_suggestion_name.sql diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 4268d6f..e9605c3 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -127,7 +127,7 @@ suggested (deck) ──Try it──▶ quest_log ──planned date passes── │ └──────── explicit decision ─────┤ │ ├──▶ regular │ └──▶ archived - └──Pass──▶ archived (name kept for dedupe) + └──Pass──▶ archived (suggested name kept for dedupe) ``` From `tried`, an explicit decision moves the dinner to `regular`, to @@ -265,7 +265,7 @@ automatic** (D-021). `suggestion_batch` job, answering 202 `{batch_id, job_id, trigger, status}` without waiting; while a batch is `pending` or `running` it answers 202 with that batch instead, so at most one runs at a time (the - check and insert share one write transaction). The job reads the + check, the batch and its job share one write transaction). The job reads the household context, the deck's names and every declined name, runs `suggest_side_quests` for 5 Side quests (scope: the batch id), then grounds the survivors in the model's order with `Grounder.GroundMany`, @@ -274,9 +274,11 @@ automatic** (D-021). ones are stored before the next — a dinner with status `suggested`, origin `side_quest`, grounding_status `grounded` with the recipe's recipe_id, photo_id, method, minutes_total and difficulty and the - model's cuisine, and a `pending` suggestion with the reason and - `cited_regular_ids` — unless a dinner it repeats was added, or a Regular - it cites renamed or archived, while it was grounding. The job publishes + model's cuisine, and a `pending` suggestion with the name it is + suggested under, the reason and `cited_regular_ids` — unless, since the + job read its input, a dinner it repeats was added, a suggestion it + repeats passed, or a Regular it cites renamed or archived (checked in + the storing transaction). The job publishes `{batch_id, stage, suggestion_ids, dropped}` as it goes (`suggesting`, `grounding`, `done`). It fails with `no_regulars`, `ai_failed` (the skill or `normalize_recipe` failed; no Side quest is made up instead, @@ -292,7 +294,9 @@ automatic** (D-021). first. `POST /api/v1/suggestions/{id}/accept` (Try it) sets the suggestion `accepted` and moves its dinner to `quest_log`; `POST /api/v1/suggestions/{id}/decline` (Pass) sets it `declined` and archives - the dinner, whose name every later batch reads as declined. Both record + the dinner; every later batch, and the household context's passed list, + reads the suggestion's own name as declined, so renaming the archived + dinner changes nothing. Both record the session's subject as the dinner's `updated_by` and answer 200 `{suggestion, replenish_job_id}`. Repeating a decision changes nothing and answers 200; the opposite decision is 409 `suggestion_decided`, a @@ -300,7 +304,10 @@ automatic** (D-021). `dinner_not_suggested`, and an unknown id 404. A decision that leaves fewer than 5 in the deck starts a `replenish` batch, whose job id it returns, unless a batch is already pending or running - (`replenish_job_id` is then null). + (`replenish_job_id` is then null). The batch and its job are written in + the decision's own transaction, so a decision is never recorded without + the replenish it calls for; when they cannot be written, neither is the + decision, and a retry decides again. - `/health` — liveness. - Everything else serves the embedded PWA with SPA fallback. diff --git a/internal/ai/harness/context_test.go b/internal/ai/harness/context_test.go index b121eb7..ef369b6 100644 --- a/internal/ai/harness/context_test.go +++ b/internal/ai/harness/context_test.go @@ -82,9 +82,9 @@ func (s *seeder) pass(name, decidedAt string) { batch := s.id("batch") s.exec(`INSERT INTO suggestion_batches (id, trigger, status, created_at, updated_at) VALUES (?, 'manual', 'done', ?, ?)`, batch, decidedAt, decidedAt) - s.exec(`INSERT INTO suggestions (id, dinner_id, batch_id, reason, cited_regular_ids, status, - decided_at, created_at, updated_at) VALUES (?, ?, ?, 'why', '[]', 'declined', ?, ?, ?)`, - s.id("suggestion"), s.dinner(name, "archived", "", "", 0, false), batch, decidedAt, decidedAt, decidedAt) + s.exec(`INSERT INTO suggestions (id, dinner_id, batch_id, name, reason, cited_regular_ids, status, + decided_at, created_at, updated_at) VALUES (?, ?, ?, ?, 'why', '[]', 'declined', ?, ?, ?)`, + s.id("suggestion"), s.dinner(name, "archived", "", "", 0, false), batch, name, decidedAt, decidedAt, decidedAt) } // plan inserts the week plan starting weekStart with one dinner per date; diff --git a/internal/db/household_context.sql.go b/internal/db/household_context.sql.go index d924b27..e26740a 100644 --- a/internal/db/household_context.sql.go +++ b/internal/db/household_context.sql.go @@ -54,16 +54,16 @@ func (q *Queries) ListPlannedDinners(ctx context.Context, arg ListPlannedDinners const listRecentDeclinedNames = `-- name: ListRecentDeclinedNames :many -SELECT dinners.name +SELECT name FROM suggestions -JOIN dinners ON dinners.id = suggestions.dinner_id -WHERE suggestions.status = 'declined' -ORDER BY suggestions.decided_at DESC, suggestions.id DESC +WHERE status = 'declined' +ORDER BY decided_at DESC, id DESC LIMIT ? ` // Reads for the AI household context block (internal/ai/harness). -// The names of the most recently declined suggestions, newest first. +// The names the most recently declined suggestions were suggested under, +// newest first; renaming their dinners does not change them. func (q *Queries) ListRecentDeclinedNames(ctx context.Context, limit int64) ([]string, error) { rows, err := q.db.QueryContext(ctx, listRecentDeclinedNames, limit) if err != nil { diff --git a/internal/db/models.go b/internal/db/models.go index b026169..a75716a 100644 --- a/internal/db/models.go +++ b/internal/db/models.go @@ -289,6 +289,7 @@ type Suggestion struct { ID string DinnerID string BatchID string + Name string Reason string CitedRegularIds string Status string diff --git a/internal/db/querier.go b/internal/db/querier.go index 6ea2742..64ecb77 100644 --- a/internal/db/querier.go +++ b/internal/db/querier.go @@ -34,6 +34,8 @@ type Querier interface { CreatePhoto(ctx context.Context, arg CreatePhotoParams) (Photo, error) CreateRecipeIngredient(ctx context.Context, arg CreateRecipeIngredientParams) error CreateRecipeStep(ctx context.Context, arg CreateRecipeStepParams) error + // name is the name the dish is suggested under, kept whatever its dinner is + // later renamed. CreateSuggestion(ctx context.Context, arg CreateSuggestionParams) error CreateSuggestionBatch(ctx context.Context, arg CreateSuggestionBatchParams) (SuggestionBatch, error) // Records the household's decision on a pending suggestion; one already @@ -50,9 +52,9 @@ type Querier interface { // boot, after the job runner fails the jobs a previous process left running. FailOrphanedPendingDinnerGroundings(ctx context.Context, arg FailOrphanedPendingDinnerGroundingsParams) (int64, error) // Fails every batch still pending or running whose job is no longer queued - // or running: the process stopped before enqueuing the job or before its - // handler recorded a result. Run at boot, after the job runner fails the - // jobs a previous process left running. + // or running: the process stopped before its handler recorded a result. + // Run at boot, after the job runner fails the jobs a previous process left + // running. FailOrphanedSuggestionBatches(ctx context.Context, arg FailOrphanedSuggestionBatchesParams) (int64, error) // Records why the background grounding of an unarchived dinner still // pending at the grounding_revision it was grounded for failed; a dinner @@ -107,14 +109,16 @@ type Querier interface { // The pending suggestions whose dinner is still suggested, oldest first, // with their dinners. ListDeck(ctx context.Context) ([]ListDeckRow, error) - // The name of every declined suggestion's dinner, oldest decision first. + // The name every declined suggestion was suggested under, oldest decision + // first; renaming its dinner does not change it. ListDeclinedSuggestionNames(ctx context.Context) ([]string, error) ListDinnersByStatus(ctx context.Context, status string) ([]Dinner, error) // The planned dinners dated from from_date up to but not including // to_date, oldest first. Days with no dinner are left out. ListPlannedDinners(ctx context.Context, arg ListPlannedDinnersParams) ([]ListPlannedDinnersRow, error) // Reads for the AI household context block (internal/ai/harness). - // The names of the most recently declined suggestions, newest first. + // The names the most recently declined suggestions were suggested under, + // newest first; renaming their dinners does not change them. ListRecentDeclinedNames(ctx context.Context, limit int64) ([]string, error) ListRecipeIngredients(ctx context.Context, recipeID string) ([]RecipeIngredient, error) ListRecipeSteps(ctx context.Context, recipeID string) ([]RecipeStep, error) diff --git a/internal/db/suggestions.sql.go b/internal/db/suggestions.sql.go index c0a3dd5..98fc20d 100644 --- a/internal/db/suggestions.sql.go +++ b/internal/db/suggestions.sql.go @@ -27,9 +27,9 @@ func (q *Queries) CountDeck(ctx context.Context) (int64, error) { const createSuggestion = `-- name: CreateSuggestion :exec INSERT INTO suggestions ( - id, dinner_id, batch_id, reason, cited_regular_ids, status, created_at, updated_at + id, dinner_id, batch_id, name, reason, cited_regular_ids, status, created_at, updated_at ) VALUES ( - ?, ?, ?, ?, ?, 'pending', ?, ? + ?, ?, ?, ?, ?, ?, 'pending', ?, ? ) ` @@ -37,17 +37,21 @@ type CreateSuggestionParams struct { ID string DinnerID string BatchID string + Name string Reason string CitedRegularIds string CreatedAt string UpdatedAt string } +// name is the name the dish is suggested under, kept whatever its dinner is +// later renamed. func (q *Queries) CreateSuggestion(ctx context.Context, arg CreateSuggestionParams) error { _, err := q.db.ExecContext(ctx, createSuggestion, arg.ID, arg.DinnerID, arg.BatchID, + arg.Name, arg.Reason, arg.CitedRegularIds, arg.CreatedAt, @@ -128,9 +132,9 @@ type FailOrphanedSuggestionBatchesParams struct { } // Fails every batch still pending or running whose job is no longer queued -// or running: the process stopped before enqueuing the job or before its -// handler recorded a result. Run at boot, after the job runner fails the -// jobs a previous process left running. +// or running: the process stopped before its handler recorded a result. +// Run at boot, after the job runner fails the jobs a previous process left +// running. func (q *Queries) FailOrphanedSuggestionBatches(ctx context.Context, arg FailOrphanedSuggestionBatchesParams) (int64, error) { result, err := q.db.ExecContext(ctx, failOrphanedSuggestionBatches, arg.UpdatedAt, arg.Kind) if err != nil { @@ -185,7 +189,7 @@ func (q *Queries) GetActiveSuggestionBatch(ctx context.Context) (SuggestionBatch } const getSuggestion = `-- name: GetSuggestion :one -SELECT suggestions.id, suggestions.dinner_id, suggestions.batch_id, suggestions.reason, suggestions.cited_regular_ids, suggestions.status, suggestions.decided_at, suggestions.created_at, suggestions.updated_at, dinners.id, dinners.name, dinners.normalized_name, dinners.status, dinners.origin, dinners.method, dinners.minutes_total, dinners.difficulty, dinners.cuisine, dinners.is_favorite, dinners.recipe_id, dinners.photo_id, dinners.grounding_status, dinners.updated_by, dinners.created_at, dinners.updated_at, dinners.recipe_url, dinners.grounding_error, dinners.grounding_revision +SELECT suggestions.id, suggestions.dinner_id, suggestions.batch_id, suggestions.name, suggestions.reason, suggestions.cited_regular_ids, suggestions.status, suggestions.decided_at, suggestions.created_at, suggestions.updated_at, dinners.id, dinners.name, dinners.normalized_name, dinners.status, dinners.origin, dinners.method, dinners.minutes_total, dinners.difficulty, dinners.cuisine, dinners.is_favorite, dinners.recipe_id, dinners.photo_id, dinners.grounding_status, dinners.updated_by, dinners.created_at, dinners.updated_at, dinners.recipe_url, dinners.grounding_error, dinners.grounding_revision FROM suggestions JOIN dinners ON dinners.id = suggestions.dinner_id WHERE suggestions.id = ? @@ -204,6 +208,7 @@ func (q *Queries) GetSuggestion(ctx context.Context, id string) (GetSuggestionRo &i.Suggestion.ID, &i.Suggestion.DinnerID, &i.Suggestion.BatchID, + &i.Suggestion.Name, &i.Suggestion.Reason, &i.Suggestion.CitedRegularIds, &i.Suggestion.Status, @@ -255,7 +260,7 @@ func (q *Queries) GetSuggestionBatchJobID(ctx context.Context, arg GetSuggestion } const listDeck = `-- name: ListDeck :many -SELECT suggestions.id, suggestions.dinner_id, suggestions.batch_id, suggestions.reason, suggestions.cited_regular_ids, suggestions.status, suggestions.decided_at, suggestions.created_at, suggestions.updated_at, dinners.id, dinners.name, dinners.normalized_name, dinners.status, dinners.origin, dinners.method, dinners.minutes_total, dinners.difficulty, dinners.cuisine, dinners.is_favorite, dinners.recipe_id, dinners.photo_id, dinners.grounding_status, dinners.updated_by, dinners.created_at, dinners.updated_at, dinners.recipe_url, dinners.grounding_error, dinners.grounding_revision +SELECT suggestions.id, suggestions.dinner_id, suggestions.batch_id, suggestions.name, suggestions.reason, suggestions.cited_regular_ids, suggestions.status, suggestions.decided_at, suggestions.created_at, suggestions.updated_at, dinners.id, dinners.name, dinners.normalized_name, dinners.status, dinners.origin, dinners.method, dinners.minutes_total, dinners.difficulty, dinners.cuisine, dinners.is_favorite, dinners.recipe_id, dinners.photo_id, dinners.grounding_status, dinners.updated_by, dinners.created_at, dinners.updated_at, dinners.recipe_url, dinners.grounding_error, dinners.grounding_revision FROM suggestions JOIN dinners ON dinners.id = suggestions.dinner_id WHERE suggestions.status = 'pending' AND dinners.status = 'suggested' @@ -282,6 +287,7 @@ func (q *Queries) ListDeck(ctx context.Context) ([]ListDeckRow, error) { &i.Suggestion.ID, &i.Suggestion.DinnerID, &i.Suggestion.BatchID, + &i.Suggestion.Name, &i.Suggestion.Reason, &i.Suggestion.CitedRegularIds, &i.Suggestion.Status, @@ -322,14 +328,14 @@ func (q *Queries) ListDeck(ctx context.Context) ([]ListDeckRow, error) { } const listDeclinedSuggestionNames = `-- name: ListDeclinedSuggestionNames :many -SELECT dinners.name +SELECT name FROM suggestions -JOIN dinners ON dinners.id = suggestions.dinner_id -WHERE suggestions.status = 'declined' -ORDER BY suggestions.decided_at, suggestions.id +WHERE status = 'declined' +ORDER BY decided_at, id ` -// The name of every declined suggestion's dinner, oldest decision first. +// The name every declined suggestion was suggested under, oldest decision +// first; renaming its dinner does not change it. func (q *Queries) ListDeclinedSuggestionNames(ctx context.Context) ([]string, error) { rows, err := q.db.QueryContext(ctx, listDeclinedSuggestionNames) if err != nil { diff --git a/internal/jobs/jobs.go b/internal/jobs/jobs.go index 337addc..2d2c865 100644 --- a/internal/jobs/jobs.go +++ b/internal/jobs/jobs.go @@ -259,6 +259,21 @@ func (k *Kind[P]) Enqueue(ctx context.Context, payload P) (string, error) { return k.r.enqueue(ctx, k.name, data) } +// EnqueueTx stores a queued job with payload in tx and returns its id, so +// the job exists exactly when what else tx writes does. Once tx has +// committed, call Wake so an idle worker runs it. +func (k *Kind[P]) EnqueueTx(ctx context.Context, tx *sql.Tx, payload P) (string, error) { + data, err := json.Marshal(payload) + if err != nil { + return "", fmt.Errorf("jobs: enqueue %s: encode payload: %w", k.name, err) + } + return createJob(ctx, db.New(tx), k.name, data) +} + +// Wake tells an idle worker to look for a job, as Enqueue does itself: +// call it after committing the transaction a job was enqueued in. +func (k *Kind[P]) Wake() { k.r.wakeOne() } + // Enqueue stores a queued job of kind with its JSON payload and returns its // id, as Kind.Enqueue does for a kind known at compile time. It returns // ErrUnknownKind for a kind with no registered handler and @@ -280,9 +295,20 @@ func (r *Runner) Enqueue(ctx context.Context, kind string, payload json.RawMessa // enqueue stores a queued job of kind with its encoded payload and wakes a // worker. func (r *Runner) enqueue(ctx context.Context, kind string, payload []byte) (string, error) { + id, err := createJob(ctx, db.New(r.st.Writer), kind, payload) + if err != nil { + return "", err + } + r.wakeOne() + return id, nil +} + +// createJob stores a queued job of kind with its encoded payload through q +// and returns its id. +func createJob(ctx context.Context, q *db.Queries, kind string, payload []byte) (string, error) { id := store.NewID() now := store.Now().String() - if err := db.New(r.st.Writer).CreateJob(ctx, db.CreateJobParams{ + if err := q.CreateJob(ctx, db.CreateJobParams{ ID: id, Kind: kind, Payload: string(payload), @@ -291,11 +317,15 @@ func (r *Runner) enqueue(ctx context.Context, kind string, payload []byte) (stri }); err != nil { return "", fmt.Errorf("jobs: enqueue %s: %w", kind, err) } + return id, nil +} + +// wakeOne gives an idle worker a token to look for a queued job. +func (r *Runner) wakeOne() { select { case r.wake <- struct{}{}: default: // every worker already has a token and will look again } - return id, nil } // Start fails every job a previous process left running with diff --git a/internal/jobs/jobs_test.go b/internal/jobs/jobs_test.go index 1ab0822..b1b6752 100644 --- a/internal/jobs/jobs_test.go +++ b/internal/jobs/jobs_test.go @@ -187,6 +187,46 @@ func TestJobSucceeds(t *testing.T) { } } +func TestEnqueueTxStoresJobOnlyWithItsTransaction(t *testing.T) { + t.Parallel() + st := migratedStore(t) + r := newRunner(t, st, 1) + count := register(t, r, "count", func(_ context.Context, p countPayload, _ func(countResult) error) (countResult, error) { + return countResult{Done: p.Steps}, nil + }) + start(t, r) + ctx := context.Background() + + var rolledBack string + refused := errors.New("refused") + if err := st.WithTx(ctx, func(tx *sql.Tx) error { + var err error + rolledBack, err = count.EnqueueTx(ctx, tx, countPayload{Steps: 1}) + if err != nil { + return err + } + return refused + }); !errors.Is(err, refused) { + t.Fatalf("WithTx = %v, want the refusal", err) + } + if _, err := r.Get(ctx, rolledBack); !errors.Is(err, ErrNotFound) { + t.Errorf("job of a rolled-back transaction: Get = %v, want ErrNotFound", err) + } + + var id string + if err := st.WithTx(ctx, func(tx *sql.Tx) error { + var err error + id, err = count.EnqueueTx(ctx, tx, countPayload{Steps: 2}) + return err + }); err != nil { + t.Fatal(err) + } + count.Wake() + if job := waitFor(t, r, id, finished); job.Status != StatusSucceeded || string(job.Result) != `{"done":2}` { + t.Errorf("job = %+v (result %s), want succeeded with {\"done\":2}", job, job.Result) + } +} + func TestJobFailsWithFailureCode(t *testing.T) { t.Parallel() st := migratedStore(t) diff --git a/internal/server/suggestions_test.go b/internal/server/suggestions_test.go index 1674994..7f33f3f 100644 --- a/internal/server/suggestions_test.go +++ b/internal/server/suggestions_test.go @@ -71,7 +71,7 @@ func newSuggestionsRouter(t *testing.T, names ...string) (http.Handler, *store.S } id := store.NewID() if err := q.CreateSuggestion(ctx, db.CreateSuggestionParams{ - ID: id, DinnerID: d.ID, BatchID: batch.ID, Reason: fmt.Sprintf("Reason %d", i), CitedRegularIds: "[]", + ID: id, DinnerID: d.ID, BatchID: batch.ID, Name: name, Reason: fmt.Sprintf("Reason %d", i), CitedRegularIds: "[]", CreatedAt: store.Now().String(), UpdatedAt: now, }); err != nil { t.Fatal(err) diff --git a/internal/store/migrate_test.go b/internal/store/migrate_test.go index 94b8ec3..4952f3b 100644 --- a/internal/store/migrate_test.go +++ b/internal/store/migrate_test.go @@ -3,6 +3,7 @@ package store import ( "context" "io/fs" + "reflect" "slices" "strings" "testing" @@ -175,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" { - 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", 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" { + 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) } } @@ -196,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" { - 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", 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" { + 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 !tableExists(t, s, "dinners") { t.Error("table from 0001_init was not created") @@ -557,3 +558,78 @@ func TestMigrateAIRunsStatus(t *testing.T) { }) } } + +// TestMigrateSuggestionName upgrades a database at 0011 holding a pending +// and a declined suggestion and checks 0012 keeps both, gives each its +// dinner's name, and refuses a suggestion with no name. +func TestMigrateSuggestionName(t *testing.T) { + ctx := context.Background() + s := openTestStore(t) + before := fstest.MapFS{} + for _, name := range []string{ + "0000_bootstrap.sql", "0001_init.sql", "0002_household_settings_updated_by.sql", "0003_jobs_status.sql", + "0004_ai_provider_presets.sql", "0005_ai_provider_xai.sql", "0006_ai_runs_status.sql", "0007_recipe_grounding.sql", + "0008_dinner_recipe_url.sql", "0009_dinner_grounding_error.sql", "0010_dinner_grounding_revision.sql", + "0011_ground_dinner_job_revision.sql", + } { + body, err := fs.ReadFile(migrations.FS, name) + if err != nil { + t.Fatal(err) + } + before[name] = &fstest.MapFile{Data: body} + } + if err := Migrate(ctx, s.Writer, before); err != nil { + t.Fatal(err) + } + for _, stmt := range []string{ + `INSERT INTO dinners (id, name, normalized_name, status, origin, is_favorite, grounding_status, created_at, updated_at) + VALUES ('dinner-1', 'Pho', 'pho', 'suggested', 'side_quest', 0, 'grounded', 'c', 'u'), + ('dinner-2', 'Ramen', 'ramen', 'archived', 'side_quest', 0, 'grounded', 'c', 'u')`, + `INSERT INTO suggestion_batches (id, trigger, status, created_at, updated_at) + VALUES ('batch-1', 'manual', 'done', 'c', 'u')`, + `INSERT INTO suggestions (id, dinner_id, batch_id, reason, cited_regular_ids, status, decided_at, created_at, updated_at) + VALUES ('sugg-1', 'dinner-1', 'batch-1', 'why 1', '[]', 'pending', NULL, 'c1', 'u1'), + ('sugg-2', 'dinner-2', 'batch-1', 'why 2', '["dinner-1"]', 'declined', 'd2', 'c2', 'u2')`, + } { + if _, err := s.Writer.Exec(stmt); err != nil { + t.Fatalf("seed: %v", err) + } + } + + if err := Migrate(ctx, s.Writer, migrations.FS); err != nil { + t.Fatal(err) + } + + rows, err := s.Reader.Query(`SELECT id, dinner_id, batch_id, name, reason, cited_regular_ids, status, COALESCE(decided_at, ''), created_at, updated_at + FROM suggestions ORDER BY id`) + if err != nil { + t.Fatal(err) + } + var got [][]string + for rows.Next() { + r := make([]string, 10) + if err := rows.Scan(&r[0], &r[1], &r[2], &r[3], &r[4], &r[5], &r[6], &r[7], &r[8], &r[9]); err != nil { + t.Fatal(err) + } + got = append(got, r) + } + if err := rows.Close(); err != nil { + t.Fatal(err) + } + want := [][]string{ + {"sugg-1", "dinner-1", "batch-1", "Pho", "why 1", "[]", "pending", "", "c1", "u1"}, + {"sugg-2", "dinner-2", "batch-1", "Ramen", "why 2", `["dinner-1"]`, "declined", "d2", "c2", "u2"}, + } + if !reflect.DeepEqual(got, want) { + t.Errorf("suggestions after 0012 =\n %v\nwant\n %v", got, want) + } + if _, err := s.Writer.Exec(`UPDATE suggestions SET name = '' WHERE id = 'sugg-1'`); err == nil || !strings.Contains(err.Error(), "CHECK constraint failed") { + t.Errorf("empty suggestion name: error %v, want CHECK constraint failure", err) + } + if _, err := s.Writer.Exec(`UPDATE suggestions SET dinner_id = 'nope' WHERE id = 'sugg-1'`); err == nil || !strings.Contains(err.Error(), "FOREIGN KEY constraint failed") { + t.Errorf("unknown dinner: error %v, want FOREIGN KEY constraint failure", err) + } + if tableExists(t, s, "suggestions_copy") { + t.Error("suggestions_copy left behind") + } +} diff --git a/internal/store/repository_test.go b/internal/store/repository_test.go index 8ae01b2..58101e8 100644 --- a/internal/store/repository_test.go +++ b/internal/store/repository_test.go @@ -95,7 +95,7 @@ func TestRepositoryInsertReadUpdate(t *testing.T) { parents: []string{"dinners", "suggestion_batches"}, row: repoRow{"suggestions", map[string]any{ "id": "sugg-1", "dinner_id": "dinner-1", "batch_id": "batch-1", - "reason": "You make chili often.", "cited_regular_ids": `["dinner-1"]`, + "name": "White Chicken Chili", "reason": "You make chili often.", "cited_regular_ids": `["dinner-1"]`, "status": "pending", "decided_at": nil, "created_at": repoTime, "updated_at": repoTime, }}, diff --git a/internal/store/schema_test.go b/internal/store/schema_test.go index eb735b8..510a752 100644 --- a/internal/store/schema_test.go +++ b/internal/store/schema_test.go @@ -110,6 +110,7 @@ func testRow(table string, i int) map[string]any { case "suggestions": r["dinner_id"] = rowID("dinners", 0) r["batch_id"] = rowID("suggestion_batches", 0) + r["name"] = fmt.Sprintf("Side quest %d", i) r["reason"] = "Like your chili." r["cited_regular_ids"] = "[]" r["status"] = "pending" diff --git a/internal/suggest/batch.go b/internal/suggest/batch.go index 1640b31..d3117c8 100644 --- a/internal/suggest/batch.go +++ b/internal/suggest/batch.go @@ -196,8 +196,10 @@ func (s *Service) ground(ctx context.Context, quests []skills.SideQuest) ([]grou // suggested and origin side_quest, grounded in its recipe, and a pending // suggestion citing its Regulars by id, all in one transaction, and // returns the suggestion ids. A Side quest is left out when, since -// suggest_side_quests ran, a dinner it repeats was added or a Regular it -// cites renamed or archived. +// suggest_side_quests ran, a dinner it repeats was added, a suggestion it +// repeats passed, or a Regular it cites renamed or archived. Declined +// names are read in the same transaction as the store, so a Pass committed +// before it is never missed. func (s *Service) store(ctx context.Context, batchID string, quests []groundedQuest) ([]string, error) { ids := []string{} err := s.st.WithTx(ctx, func(tx *sql.Tx) error { @@ -206,7 +208,10 @@ func (s *Service) store(ctx context.Context, batchID string, quests []groundedQu if err != nil { return fmt.Errorf("suggest: list dinners for dedupe: %w", err) } - var names []string + names, err := q.ListDeclinedSuggestionNames(ctx) + if err != nil { + return fmt.Errorf("suggest: list declined names for dedupe: %w", err) + } regulars := map[string][]string{} for _, row := range existing { names = append(names, row.Dinner.Name) @@ -250,6 +255,7 @@ func (s *Service) store(ctx context.Context, batchID string, quests []groundedQu ID: id, DinnerID: d.ID, BatchID: batchID, + Name: d.Name, Reason: gq.quest.Reason, CitedRegularIds: string(citedJSON), CreatedAt: now, diff --git a/internal/suggest/suggest.go b/internal/suggest/suggest.go index da5a140..9ffda32 100644 --- a/internal/suggest/suggest.go +++ b/internal/suggest/suggest.go @@ -15,7 +15,6 @@ import ( "fmt" "log/slog" "slices" - "sync" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/skills" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/db" @@ -112,10 +111,6 @@ type Service struct { suggest Suggester logger *slog.Logger batchJob *jobs.Kind[BatchJob] - - // batchMu makes finding or creating the active batch and enqueuing its - // job one step, so a batch is never seen without its job. - batchMu sync.Mutex } // NewService returns a service over st that suggests with suggest, grounds @@ -161,53 +156,54 @@ func (s *Service) StartBatch(ctx context.Context, trigger string) (Batch, error) if !slices.Contains(triggers, trigger) { return Batch{}, fmt.Errorf("suggest: unknown batch trigger %q", trigger) } - s.batchMu.Lock() - defer s.batchMu.Unlock() - var row db.SuggestionBatch - created := false + var b Batch err := s.st.WithTx(ctx, func(tx *sql.Tx) error { - q := db.New(tx) - cur, err := q.GetActiveSuggestionBatch(ctx) - if err == nil { - row = cur - return nil - } - if !errors.Is(err, sql.ErrNoRows) { - return fmt.Errorf("suggest: read active batch: %w", err) - } - now := store.Now().String() - row, err = q.CreateSuggestionBatch(ctx, db.CreateSuggestionBatchParams{ - ID: store.NewID(), - Trigger: trigger, - CreatedAt: now, - UpdatedAt: now, - }) - if err != nil { - return fmt.Errorf("suggest: create batch: %w", err) - } - created = true - return nil + var err error + b, err = s.startBatch(ctx, tx, trigger) + return err }) if err != nil { return Batch{}, err } - b := Batch{ID: row.ID, Trigger: row.Trigger, Status: row.Status, Created: created} - if !created { - b.JobID, err = db.New(s.st.Reader).GetSuggestionBatchJobID(ctx, db.GetSuggestionBatchJobIDParams{Kind: KindSuggestionBatch, BatchID: row.ID}) - if err != nil { - return Batch{}, fmt.Errorf("suggest: read job of batch %s: %w", row.ID, err) - } - return b, nil - } - b.JobID, err = s.batchJob.Enqueue(ctx, BatchJob{BatchID: row.ID}) - if err != nil { - // The batch has no job to run it, so it must not block the next. - s.finishBatch(context.WithoutCancel(ctx), row.ID, BatchFailed) - return Batch{}, fmt.Errorf("suggest: enqueue batch %s: %w", row.ID, err) + if b.Created { + s.batchJob.Wake() } return b, nil } +// startBatch does StartBatch's work in tx: the batch and the job that runs +// it are committed together, so a batch is never seen without its job. The +// caller wakes the job runner once tx has committed and Created is true. +func (s *Service) startBatch(ctx context.Context, tx *sql.Tx, trigger string) (Batch, error) { + q := db.New(tx) + cur, err := q.GetActiveSuggestionBatch(ctx) + if err == nil { + jobID, err := q.GetSuggestionBatchJobID(ctx, db.GetSuggestionBatchJobIDParams{Kind: KindSuggestionBatch, BatchID: cur.ID}) + if err != nil { + return Batch{}, fmt.Errorf("suggest: read job of batch %s: %w", cur.ID, err) + } + return Batch{ID: cur.ID, JobID: jobID, Trigger: cur.Trigger, Status: cur.Status}, nil + } + if !errors.Is(err, sql.ErrNoRows) { + return Batch{}, fmt.Errorf("suggest: read active batch: %w", err) + } + now := store.Now().String() + row, err := q.CreateSuggestionBatch(ctx, db.CreateSuggestionBatchParams{ + ID: store.NewID(), + Trigger: trigger, + CreatedAt: now, + UpdatedAt: now, + }) + if err != nil { + return Batch{}, fmt.Errorf("suggest: create batch: %w", err) + } + jobID, err := s.batchJob.EnqueueTx(ctx, tx, BatchJob{BatchID: row.ID}) + if err != nil { + return Batch{}, fmt.Errorf("suggest: enqueue batch %s: %w", row.ID, err) + } + return Batch{ID: row.ID, JobID: jobID, Trigger: row.Trigger, Status: row.Status, Created: true}, nil +} + // finishBatch records batch id as status, logging a failure to do so. func (s *Service) finishBatch(ctx context.Context, id, status string) { if _, err := db.New(s.st.Writer).FinishSuggestionBatch(ctx, db.FinishSuggestionBatchParams{ @@ -289,9 +285,9 @@ func (s *Service) Accept(ctx context.Context, id, updatedBy string) (Decision, e } // Decline records Pass on suggestion id, archiving its dinner and -// recording updatedBy on it. The archived dinner keeps its name, and the -// declined suggestion is read into every later batch, so the dish is never -// suggested again. +// recording updatedBy on it. The suggestion keeps the name its dish was +// suggested under, whatever the dinner is later renamed, and every later +// batch reads it as declined, so the dish is never suggested again. func (s *Service) Decline(ctx context.Context, id, updatedBy string) (Decision, error) { return s.decide(ctx, id, StatusDeclined, dinners.StatusArchived, updatedBy) } @@ -300,15 +296,15 @@ func (s *Service) Decline(ctx context.Context, id, updatedBy string) (Decision, // dinnerStatus. Repeating a decision changes nothing and returns the // suggestion as it is; the opposite decision is a *DecidedError. A // decision that leaves the deck under ReplenishBelow starts a replenish -// batch unless one is already pending or running. The decision is -// committed before the batch starts, so a failure to start it is returned -// with the decision already made. +// batch unless one is already pending or running, in the decision's own +// transaction: the decision is never recorded without the replenish it +// calls for, and a failure to start the batch records neither. func (s *Service) decide(ctx context.Context, id, decision, dinnerStatus, updatedBy string) (Decision, error) { if updatedBy == "" { return Decision{}, errors.New("suggest: decide: no member to record as updated_by") } var row db.GetSuggestionRow - changed := false + var replenish *Batch err := s.st.WithTx(ctx, func(tx *sql.Tx) error { q := db.New(tx) cur, err := q.GetSuggestion(ctx, id) @@ -349,33 +345,34 @@ func (s *Service) decide(ctx context.Context, id, decision, dinnerStatus, update if err != nil { return fmt.Errorf("suggest: read suggestion %s: %w", id, err) } - changed = true + left, err := q.CountDeck(ctx) + if err != nil { + return fmt.Errorf("suggest: count deck: %w", err) + } + if left >= ReplenishBelow { + return nil + } + b, err := s.startBatch(ctx, tx, TriggerReplenish) + if err != nil { + return err + } + replenish = &b return nil }) if err != nil { return Decision{}, err } + created := replenish != nil && replenish.Created + if created { + s.batchJob.Wake() + } sug, err := fromRow(row.Suggestion, row.Dinner) if err != nil { return Decision{}, err } d := Decision{Suggestion: sug} - if !changed { - return d, nil - } - left, err := db.New(s.st.Reader).CountDeck(ctx) - if err != nil { - return Decision{}, fmt.Errorf("suggest: count deck: %w", err) - } - if left >= ReplenishBelow { - return d, nil - } - b, err := s.StartBatch(ctx, TriggerReplenish) - if err != nil { - return Decision{}, err - } - if b.Created { - d.ReplenishJobID = &b.JobID + if created { + d.ReplenishJobID = &replenish.JobID } return d, nil } diff --git a/internal/suggest/suggest_test.go b/internal/suggest/suggest_test.go index 5e80703..313920d 100644 --- a/internal/suggest/suggest_test.go +++ b/internal/suggest/suggest_test.go @@ -17,6 +17,7 @@ import ( "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/llm" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/skills" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/db" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/dinners" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/household" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/jobs" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/recipes" @@ -41,9 +42,12 @@ type env struct { side *llm.Fake // regulars are the Regulars' ids by name. regulars map[string]string + grounder *recipes.Grounder mu sync.Mutex inputs []skills.SideQuestsRequest + // stub, when set, answers in place of suggest_side_quests. + stub func(in skills.SideQuestsRequest) skills.SideQuests } // newEnv returns a suggestions service over a migrated store with the @@ -83,6 +87,7 @@ func newEnv(t *testing.T) *env { e.regulars[name] = e.dinner(t, name, "regular") } g := recipestest.NewGrounder(t, st, dataDir, e.search, e.pages, e.images, e.model) + e.grounder = g guard, err := harness.NewGuard(harness.DefaultMaxCalls) if err != nil { t.Fatal(err) @@ -96,7 +101,11 @@ func newEnv(t *testing.T) *env { suggester := func(ctx context.Context, scope string, in skills.SideQuestsRequest) (skills.SideQuests, error) { e.mu.Lock() e.inputs = append(e.inputs, in) + stub := e.stub e.mu.Unlock() + if stub != nil { + return stub(in), nil + } return skills.SuggestSideQuests(ctx, cfg, scope, in) } logger := slog.New(slog.NewTextHandler(io.Discard, nil)) @@ -169,6 +178,7 @@ func (e *env) seedDeck(t *testing.T, names ...string) []string { ID: id, DinnerID: e.dinner(t, name, "suggested"), BatchID: b.ID, + Name: name, Reason: "Like your Beef Chili.", CitedRegularIds: fmt.Sprintf("[%q]", e.regulars["Beef Chili"]), CreatedAt: store.Now().String(), @@ -672,6 +682,146 @@ func TestLaterBatchNeverResuggestsDeclined(t *testing.T) { } } +// TestBatchNeverStoresDishPassedWhileRunning passes a Side quest after a +// batch has read its input but before it stores, with suggest_side_quests +// answering from that input: the batch must not store the passed dish. +func TestBatchNeverStoresDishPassedWhileRunning(t *testing.T) { + e := newEnv(t) + e.start(t) + ids := e.seedDeck(t, "Chicken Shawarma Bowls") + e.recipe(t, "Chicken Shawarma Bowls") + e.recipe(t, "Beef Ragu") + held, release := make(chan struct{}), make(chan struct{}) + stub := func(skills.SideQuestsRequest) skills.SideQuests { + close(held) + <-release + quest := func(name, cited string) skills.SideQuest { + return skills.SideQuest{Name: name, Cuisine: "Fusion", Method: "stovetop", EstMinutes: 35, Difficulty: "easy", + Reason: "Like your " + cited + ".", CitedRegulars: []string{cited}, SearchQuery: name} + } + return skills.SideQuests{Suggestions: []skills.SideQuest{ + quest("Chicken Shawarma Bowls", "Chicken Fajitas"), + quest("Beef Ragu", "Beef Chili"), + }} + } + e.mu.Lock() + e.stub = stub + e.mu.Unlock() + + b, err := e.svc.StartBatch(t.Context(), suggest.TriggerManual) + if err != nil { + t.Fatal(err) + } + <-held + if _, err := e.svc.Decline(t.Context(), ids[0], member); err != nil { + close(release) + t.Fatal(err) + } + close(release) + job := e.waitJob(t, b.JobID) + if job.Status != jobs.StatusSucceeded { + t.Fatalf("batch job %s: %v", job.Status, *job.Error) + } + var res batchResult + if err := json.Unmarshal(job.Result, &res); err != nil { + t.Fatal(err) + } + if len(res.SuggestionIDs) != 1 || res.Dropped != 1 { + t.Errorf("batch result = %+v, want 1 kept and the passed dish dropped", res) + } + if got := e.deckNames(t); !slices.Equal(got, []string{"Beef Ragu"}) { + t.Errorf("deck = %v, want [Beef Ragu]", got) + } +} + +// TestLaterBatchNeverResuggestsRenamedPass passes a Side quest, renames +// its archived dinner, and checks the next batch still refuses the name it +// was suggested under. +func TestLaterBatchNeverResuggestsRenamedPass(t *testing.T) { + e := newEnv(t) // runner not started: the replenish batch waits for the rename + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + dinnerSvc, err := dinners.NewService(e.st, e.grounder, e.runner, logger) + if err != nil { + t.Fatal(err) + } + ids := e.seedDeck(t, "Chicken Shawarma Bowls") + d, err := e.svc.Decline(t.Context(), ids[0], member) + if err != nil { + t.Fatal(err) + } + if d.ReplenishJobID == nil { + t.Fatal("passing the last Side quest started no replenish batch") + } + renamed := "Leftover Wrap" + if _, err := dinnerSvc.Update(t.Context(), d.Suggestion.DinnerID, dinners.Patch{Name: &renamed}, member); err != nil { + t.Fatal(err) + } + + // The respelled passed dish would ground, so only the declined names + // keep it out of the deck. + e.recipe(t, "chicken shawarma bowls!") + e.recipe(t, "Beef Ragu") + e.reply( + sideQuest("chicken shawarma bowls!", "Spiced chicken, like your Chicken Fajitas.", "Chicken Fajitas"), + sideQuest("Beef Ragu", "Slow beef like your Beef Chili.", "Beef Chili"), + ) + e.start(t) + job := e.waitJob(t, *d.ReplenishJobID) + if job.Status != jobs.StatusSucceeded { + t.Fatalf("replenish job %s: %v", job.Status, *job.Error) + } + if got := e.deckNames(t); !slices.Equal(got, []string{"Beef Ragu"}) { + t.Errorf("deck = %v, want [Beef Ragu]", got) + } + e.mu.Lock() + in := e.inputs[0] + e.mu.Unlock() + if !slices.Equal(in.Declined, []string{"Chicken Shawarma Bowls"}) || !slices.Equal(in.Context.Passed, []string{"Chicken Shawarma Bowls"}) { + t.Errorf("replenish batch declined %v, passed %v; want the name it was suggested under in both", in.Declined, in.Context.Passed) + } +} + +// TestDecisionNeverCommitsWithoutItsReplenish makes the replenish batch's +// job fail to store: the decision must not be recorded either, so the +// retry decides again and starts the replenish. +func TestDecisionNeverCommitsWithoutItsReplenish(t *testing.T) { + e := newEnv(t) // runner not started: the replenish batch stays pending + ids := e.seedDeck(t, "Pho", "Ramen", "Laksa", "Bibimbap", "Katsu") + if _, err := e.st.Writer.ExecContext(t.Context(), `CREATE TRIGGER refuse_jobs BEFORE INSERT ON jobs BEGIN SELECT RAISE(ABORT, 'jobs refused'); END`); err != nil { + t.Fatal(err) + } + if _, err := e.svc.Decline(t.Context(), ids[0], member); err == nil { + t.Fatal("decline succeeded with no replenish job stored") + } + if got := e.deckNames(t); len(got) != 5 { + t.Errorf("deck after the failed decline = %v, want all 5 still pending", got) + } + if n := e.activeBatches(t); n != 0 { + t.Errorf("%d active batches after the failed decline, want 0", n) + } + + if _, err := e.st.Writer.ExecContext(t.Context(), `DROP TRIGGER refuse_jobs`); err != nil { + t.Fatal(err) + } + d, err := e.svc.Decline(t.Context(), ids[0], member) + if err != nil { + t.Fatal(err) + } + if d.Suggestion.Status != suggest.StatusDeclined || d.ReplenishJobID == nil { + t.Fatalf("retried decline = %+v, want declined with a replenish job", d) + } + job, err := e.runner.Get(t.Context(), *d.ReplenishJobID) + if err != nil { + t.Fatal(err) + } + if job.Kind != suggest.KindSuggestionBatch || job.Status != jobs.StatusQueued { + t.Errorf("replenish job = %+v, want a queued %s", job, suggest.KindSuggestionBatch) + } + if n := e.activeBatches(t); n != 1 { + t.Errorf("%d active batches, want 1", n) + } +} + func TestBatchFailsWhenAIFails(t *testing.T) { e := newEnv(t) e.start(t) diff --git a/migrations/0012_suggestion_name.sql b/migrations/0012_suggestion_name.sql new file mode 100644 index 0000000..d54cc3b --- /dev/null +++ b/migrations/0012_suggestion_name.sql @@ -0,0 +1,37 @@ +-- 0012_suggestion_name: a suggestion keeps the name its dish was suggested +-- under. Its dinner can be renamed, even once archived by a Pass, so the +-- declined names every later batch refuses are read from here, never from +-- the dinner. A suggestion stored before this takes its dinner's name now. +-- SQLite cannot add a NOT NULL column without a default, so the table is +-- rebuilt as 0005 does: copy the rows aside, drop the table, create it +-- again, copy the rows back and drop the copy. Nothing references +-- suggestions. + +CREATE TABLE suggestions_copy AS SELECT * FROM suggestions; + +DROP TABLE suggestions; + +CREATE TABLE suggestions ( + id TEXT NOT NULL PRIMARY KEY, + dinner_id TEXT NOT NULL REFERENCES dinners (id), + batch_id TEXT NOT NULL REFERENCES suggestion_batches (id), + name TEXT NOT NULL CHECK (name <> ''), + reason TEXT NOT NULL, + cited_regular_ids TEXT NOT NULL, -- JSON array of dinner ids + status TEXT NOT NULL CHECK (status IN ('pending', 'accepted', 'declined')), + decided_at TEXT, + created_at TEXT NOT NULL, + updated_at TEXT NOT NULL +); + +INSERT INTO suggestions ( + id, dinner_id, batch_id, name, reason, cited_regular_ids, status, decided_at, created_at, updated_at +) +SELECT + suggestions_copy.id, suggestions_copy.dinner_id, suggestions_copy.batch_id, dinners.name, + suggestions_copy.reason, suggestions_copy.cited_regular_ids, suggestions_copy.status, + suggestions_copy.decided_at, suggestions_copy.created_at, suggestions_copy.updated_at +FROM suggestions_copy +JOIN dinners ON dinners.id = suggestions_copy.dinner_id; + +DROP TABLE suggestions_copy; diff --git a/migrations/queries/household_context.sql b/migrations/queries/household_context.sql index 37ff2d7..fbea66d 100644 --- a/migrations/queries/household_context.sql +++ b/migrations/queries/household_context.sql @@ -1,12 +1,12 @@ -- Reads for the AI household context block (internal/ai/harness). -- name: ListRecentDeclinedNames :many --- The names of the most recently declined suggestions, newest first. -SELECT dinners.name +-- The names the most recently declined suggestions were suggested under, +-- newest first; renaming their dinners does not change them. +SELECT name FROM suggestions -JOIN dinners ON dinners.id = suggestions.dinner_id -WHERE suggestions.status = 'declined' -ORDER BY suggestions.decided_at DESC, suggestions.id DESC +WHERE status = 'declined' +ORDER BY decided_at DESC, id DESC LIMIT ?; -- name: ListPlannedDinners :many diff --git a/migrations/queries/suggestions.sql b/migrations/queries/suggestions.sql index b493575..c9392e9 100644 --- a/migrations/queries/suggestions.sql +++ b/migrations/queries/suggestions.sql @@ -35,9 +35,9 @@ WHERE id = ? AND status IN ('pending', 'running'); -- name: FailOrphanedSuggestionBatches :execrows -- Fails every batch still pending or running whose job is no longer queued --- or running: the process stopped before enqueuing the job or before its --- handler recorded a result. Run at boot, after the job runner fails the --- jobs a previous process left running. +-- or running: the process stopped before its handler recorded a result. +-- Run at boot, after the job runner fails the jobs a previous process left +-- running. UPDATE suggestion_batches SET status = 'failed', updated_at = sqlc.arg(updated_at) WHERE status IN ('pending', 'running') @@ -49,10 +49,12 @@ WHERE status IN ('pending', 'running') ); -- name: CreateSuggestion :exec +-- name is the name the dish is suggested under, kept whatever its dinner is +-- later renamed. INSERT INTO suggestions ( - id, dinner_id, batch_id, reason, cited_regular_ids, status, created_at, updated_at + id, dinner_id, batch_id, name, reason, cited_regular_ids, status, created_at, updated_at ) VALUES ( - ?, ?, ?, ?, ?, 'pending', ?, ? + ?, ?, ?, ?, ?, ?, 'pending', ?, ? ); -- name: GetSuggestion :one @@ -79,12 +81,12 @@ JOIN dinners ON dinners.id = suggestions.dinner_id WHERE suggestions.status = 'pending' AND dinners.status = 'suggested'; -- name: ListDeclinedSuggestionNames :many --- The name of every declined suggestion's dinner, oldest decision first. -SELECT dinners.name +-- The name every declined suggestion was suggested under, oldest decision +-- first; renaming its dinner does not change it. +SELECT name FROM suggestions -JOIN dinners ON dinners.id = suggestions.dinner_id -WHERE suggestions.status = 'declined' -ORDER BY suggestions.decided_at, suggestions.id; +WHERE status = 'declined' +ORDER BY decided_at, id; -- name: DecideSuggestion :execrows -- Records the household's decision on a pending suggestion; one already