From 50399b6329fa0165802f2cd8feb428d88787b13a Mon Sep 17 00:00:00 2001 From: dev_agent Date: Tue, 6 Oct 2026 23:28:11 -0400 Subject: [PATCH 1/6] Add Side quest suggestion batches, deck and Try it / Pass API POST /api/v1/suggestions/batches enqueues a suggestion_batch job that runs suggest_side_quests through the harness, grounds the survivors with recipes.Grounder.GroundMany (dropping those no recipe grounds) and stores each as a dinner (status suggested, origin side_quest) with a pending suggestion holding its reason and cited_regular_ids. Only one batch is pending or running at a time; a POST while one is active returns it. GET /api/v1/suggestions/deck lists pending suggestions oldest first with the dinner's name, photo, difficulty and the reason. POST /api/v1/suggestions/{id}/accept moves the dinner to the Quest log; /decline records the Pass and archives the dinner, whose name every later batch reads as declined. Both are idempotent and 409 on the opposite decision. A decision leaving fewer than 5 in the deck starts a replenish batch unless one is already pending or running. Batches left unfinished by a restart are failed at boot so they never block the next. Task: b4f2558c-227f-4627-852b-be5aa190242a Co-Authored-By: Claude Opus 5.5 --- cmd/server/main.go | 27 +- docs/ARCHITECTURE.md | 41 ++ internal/db/querier.go | 34 ++ internal/db/suggestions.sql.go | 404 ++++++++++++++ internal/server/aisettings_test.go | 2 +- internal/server/dinners_test.go | 2 +- internal/server/login_test.go | 2 +- internal/server/media_test.go | 2 +- internal/server/origin_test.go | 2 +- internal/server/recipesearch_test.go | 2 +- internal/server/server.go | 14 +- internal/server/server_test.go | 48 +- internal/server/settings_test.go | 2 +- internal/server/suggestions.go | 82 +++ internal/server/suggestions_test.go | 189 +++++++ internal/suggest/batch.go | 299 +++++++++++ internal/suggest/suggest.go | 415 +++++++++++++++ internal/suggest/suggest_test.go | 769 +++++++++++++++++++++++++++ migrations/queries/suggestions.sql | 102 ++++ 19 files changed, 2420 insertions(+), 18 deletions(-) create mode 100644 internal/db/suggestions.sql.go create mode 100644 internal/server/suggestions.go create mode 100644 internal/server/suggestions_test.go create mode 100644 internal/suggest/batch.go create mode 100644 internal/suggest/suggest.go create mode 100644 internal/suggest/suggest_test.go create mode 100644 migrations/queries/suggestions.sql diff --git a/cmd/server/main.go b/cmd/server/main.go index 3935c0a..3eed6b6 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -27,6 +27,7 @@ import ( "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/secretbox" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/server" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/store" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/suggest" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/migrations" ) @@ -106,6 +107,10 @@ func run(cfg config.Config, logger *slog.Logger) error { if err != nil { return err } + suggestSvc, err := suggest.NewService(db, grounder, sideQuests(ai, guard), runner, logger) + if err != nil { + return err + } sessions, err := auth.NewSessionManager(db, cfg.SessionIdle, cfg.SessionMax) if err != nil { @@ -123,7 +128,7 @@ func run(cfg config.Config, logger *slog.Logger) error { return err } - router, err := server.NewRouter(logger, version, cfg.TrustedProxy, authFlow, sessions, settings, search, ai, dinnerSvc, runner, photos, frontend.Assets()) + router, err := server.NewRouter(logger, version, cfg.TrustedProxy, authFlow, sessions, settings, search, ai, dinnerSvc, suggestSvc, runner, photos, frontend.Assets()) if err != nil { return err } @@ -145,6 +150,11 @@ func run(cfg config.Config, logger *slog.Logger) error { if _, err := dinnerSvc.FailInterruptedGroundings(ctx); err != nil { return err } + // Likewise a suggestion batch with no job left to run it, so it does + // not block every later batch. + if _, err := suggestSvc.FailInterruptedBatches(ctx); err != nil { + return err + } srv := &http.Server{ Addr: cfg.HTTPAddr, Handler: router, @@ -247,7 +257,8 @@ func newJobRunner(db *store.Store, logger *slog.Logger) (*jobs.Runner, *jobKinds // registerJobKinds registers the framework's job kinds on runner. A feature // service registers its own kinds when it is built, before the runner -// starts (dinners.NewService registers dinners.KindGroundDinner). +// starts (dinners.NewService registers dinners.KindGroundDinner and +// suggest.NewService suggest.KindSuggestionBatch). func registerJobKinds(runner *jobs.Runner) (*jobKinds, error) { builtins, err := jobs.RegisterBuiltins(runner) if err != nil { @@ -268,6 +279,18 @@ func normalizer(ai *aisettings.Service, guard *harness.Guard) recipes.Normalizer } } +// sideQuests runs suggest_side_quests with the AI settings current at each +// run, sharing guard. +func sideQuests(ai *aisettings.Service, guard *harness.Guard) suggest.Suggester { + return func(ctx context.Context, scope string, in skills.SideQuestsRequest) (skills.SideQuests, error) { + cfg, err := ai.HarnessConfig(ctx, guard) + if err != nil { + return skills.SideQuests{}, err + } + return skills.SuggestSideQuests(ctx, cfg, scope, in) + } +} + // logListening logs the startup line. Only the public URL's origin is logged // so no credential-bearing URL component can reach the log. func logListening(logger *slog.Logger, cfg config.Config) { diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index e46c340..4268d6f 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -260,6 +260,47 @@ automatic** (D-021). dinner, is 500, logged with a fixed reason and the dinner id only — never the link, which can carry a personal token, or dependency error text. +- Side quest suggestions (`suggest.Service`). `POST + /api/v1/suggestions/batches` creates a `manual` batch and enqueues its + `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 + 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`, + as many as are still wanted at a time, until 5 are kept or none are + left. A Side quest no recipe grounds is dropped; each round's grounded + 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 + `{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, + D-017), `search_not_configured` or `search_failed`, and the batch is set + `failed`; Side quests stored before the failure stay in the deck. At + boot, after the job runner fails the jobs a previous process left + running, every batch still `pending` or `running` with no job queued or + running is set `failed`, so it does not block the next. + `GET /api/v1/suggestions/deck` returns `{suggestions: [{id, dinner_id, + name, photo_url, difficulty, method, minutes_total, cuisine, reason, + cited_regular_ids, status, dinner_status, decided_at, created_at}]}`: + the `pending` suggestions whose dinner is still `suggested`, oldest + 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 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 + suggestion whose dinner left the deck some other way 409 + `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). - `/health` — liveness. - Everything else serves the embedded PWA with SPA fallback. diff --git a/internal/db/querier.go b/internal/db/querier.go index 0a27a86..6ea2742 100644 --- a/internal/db/querier.go +++ b/internal/db/querier.go @@ -21,6 +21,8 @@ type Querier interface { // Drops references to the runs created before the cutoff, so the purge // can delete them. ClearWeekPlanAIRunsBefore(ctx context.Context, cutoff string) (int64, error) + // How many suggestions ListDeck returns. + CountDeck(ctx context.Context) (int64, error) // How many Regulars the household has; archived dinners are not counted. CountRegularDinners(ctx context.Context) (int64, error) CreateAIRun(ctx context.Context, arg CreateAIRunParams) error @@ -32,6 +34,11 @@ 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 + 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 + // decided is left as it is (0 rows). + DecideSuggestion(ctx context.Context, arg DecideSuggestionParams) (int64, error) DeleteAIRunsBefore(ctx context.Context, cutoff string) (int64, error) DeleteRecipeIngredients(ctx context.Context, recipeID string) error DeleteRecipeSteps(ctx context.Context, recipeID string) error @@ -42,6 +49,11 @@ type Querier interface { // archived dinner is left as it is; archiving stopped its grounding. Run at // 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. + 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 // grounded some other way, renamed or archived meanwhile is left as it is @@ -49,8 +61,14 @@ type Querier interface { FailPendingDinnerGrounding(ctx context.Context, arg FailPendingDinnerGroundingParams) (int64, error) // Fails every job left running, at boot before any worker starts. FailRunningJobs(ctx context.Context, arg FailRunningJobsParams) (int64, error) + // Records a pending or running batch as done or failed; a batch already + // finished is left as it is (0 rows). + FinishSuggestionBatch(ctx context.Context, arg FinishSuggestionBatchParams) (int64, error) // Returns sql.ErrNoRows until the row exists. GetAISettings(ctx context.Context) (AiSetting, error) + // The batch still pending or running, oldest first. Returns sql.ErrNoRows + // when there is none. + GetActiveSuggestionBatch(ctx context.Context) (SuggestionBatch, error) // Returns sql.ErrNoRows until the row exists. GetChatSettings(ctx context.Context) (ChatSetting, error) GetDinner(ctx context.Context, id string) (Dinner, error) @@ -67,6 +85,10 @@ type Querier interface { GetRecipeBySourceURL(ctx context.Context, sourceUrl string) (Recipe, error) // Returns sql.ErrNoRows until the row exists. GetRecipeSearchSettings(ctx context.Context) (RecipeSearchSetting, error) + // A suggestion with its dinner. Returns sql.ErrNoRows for an unknown id. + GetSuggestion(ctx context.Context, id string) (GetSuggestionRow, error) + // The id of the job that runs batch batch_id. + GetSuggestionBatchJobID(ctx context.Context, arg GetSuggestionBatchJobIDParams) (string, error) // Records a dinner's grounding by hand (grounded or manual) and clears any // grounding_error and pasted recipe_url: the recipe chosen replaces the // link. It writes only an unarchived dinner still as it was observed @@ -82,6 +104,11 @@ type Querier interface { // minutes_total or difficulty already set is kept: a pending dinner has // them only when a member edited them while it was grounding. GroundPendingDinner(ctx context.Context, arg GroundPendingDinnerParams) (int64, error) + // 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. + 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. @@ -94,10 +121,17 @@ type Querier interface { // Every dinner a new or renamed dinner must not repeat, with the // source_url of the recipe it is grounded in (NULL while it has none). ListUnarchivedDinners(ctx context.Context) ([]ListUnarchivedDinnersRow, 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 + // (0 rows). + MoveSuggestedDinner(ctx context.Context, arg MoveSuggestedDinnerParams) (int64, error) // Stores a partial result of a running job. PublishJobResult(ctx context.Context, arg PublishJobResultParams) (int64, error) // An unknown id returns sql.ErrNoRows. SetDinnerFavorite(ctx context.Context, arg SetDinnerFavoriteParams) (Dinner, error) + // Marks a pending batch running; a batch in any other status is left as + // it is (0 rows). + StartSuggestionBatch(ctx context.Context, arg StartSuggestionBatchParams) (int64, error) SucceedJob(ctx context.Context, arg SucceedJobParams) (int64, error) // An unknown id returns sql.ErrNoRows. ToggleDinnerFavorite(ctx context.Context, arg ToggleDinnerFavoriteParams) (Dinner, error) diff --git a/internal/db/suggestions.sql.go b/internal/db/suggestions.sql.go new file mode 100644 index 0000000..c0a3dd5 --- /dev/null +++ b/internal/db/suggestions.sql.go @@ -0,0 +1,404 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.31.1 +// source: suggestions.sql + +package db + +import ( + "context" + "database/sql" +) + +const countDeck = `-- name: CountDeck :one +SELECT COUNT(*) +FROM suggestions +JOIN dinners ON dinners.id = suggestions.dinner_id +WHERE suggestions.status = 'pending' AND dinners.status = 'suggested' +` + +// How many suggestions ListDeck returns. +func (q *Queries) CountDeck(ctx context.Context) (int64, error) { + row := q.db.QueryRowContext(ctx, countDeck) + var count int64 + err := row.Scan(&count) + return count, err +} + +const createSuggestion = `-- name: CreateSuggestion :exec +INSERT INTO suggestions ( + id, dinner_id, batch_id, reason, cited_regular_ids, status, created_at, updated_at +) VALUES ( + ?, ?, ?, ?, ?, 'pending', ?, ? +) +` + +type CreateSuggestionParams struct { + ID string + DinnerID string + BatchID string + Reason string + CitedRegularIds string + CreatedAt string + UpdatedAt string +} + +func (q *Queries) CreateSuggestion(ctx context.Context, arg CreateSuggestionParams) error { + _, err := q.db.ExecContext(ctx, createSuggestion, + arg.ID, + arg.DinnerID, + arg.BatchID, + arg.Reason, + arg.CitedRegularIds, + arg.CreatedAt, + arg.UpdatedAt, + ) + return err +} + +const createSuggestionBatch = `-- name: CreateSuggestionBatch :one +INSERT INTO suggestion_batches (id, trigger, status, created_at, updated_at) +VALUES (?, ?, 'pending', ?, ?) +RETURNING id, "trigger", status, ai_run_id, created_at, updated_at +` + +type CreateSuggestionBatchParams struct { + ID string + Trigger string + CreatedAt string + UpdatedAt string +} + +func (q *Queries) CreateSuggestionBatch(ctx context.Context, arg CreateSuggestionBatchParams) (SuggestionBatch, error) { + row := q.db.QueryRowContext(ctx, createSuggestionBatch, + arg.ID, + arg.Trigger, + arg.CreatedAt, + arg.UpdatedAt, + ) + var i SuggestionBatch + err := row.Scan( + &i.ID, + &i.Trigger, + &i.Status, + &i.AiRunID, + &i.CreatedAt, + &i.UpdatedAt, + ) + return i, err +} + +const decideSuggestion = `-- name: DecideSuggestion :execrows +UPDATE suggestions +SET status = ?1, decided_at = ?2, updated_at = ?2 +WHERE id = ?3 AND status = 'pending' +` + +type DecideSuggestionParams struct { + Status string + Now sql.NullString + ID string +} + +// Records the household's decision on a pending suggestion; one already +// decided is left as it is (0 rows). +func (q *Queries) DecideSuggestion(ctx context.Context, arg DecideSuggestionParams) (int64, error) { + result, err := q.db.ExecContext(ctx, decideSuggestion, arg.Status, arg.Now, arg.ID) + if err != nil { + return 0, err + } + return result.RowsAffected() +} + +const failOrphanedSuggestionBatches = `-- name: FailOrphanedSuggestionBatches :execrows +UPDATE suggestion_batches +SET status = 'failed', updated_at = ?1 +WHERE status IN ('pending', 'running') + AND NOT EXISTS ( + SELECT 1 FROM jobs + WHERE jobs.kind = ?2 + AND jobs.status IN ('queued', 'running') + AND json_extract(jobs.payload, '$.batch_id') = suggestion_batches.id + ) +` + +type FailOrphanedSuggestionBatchesParams struct { + UpdatedAt string + Kind string +} + +// 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. +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 { + return 0, err + } + return result.RowsAffected() +} + +const finishSuggestionBatch = `-- name: FinishSuggestionBatch :execrows +UPDATE suggestion_batches +SET status = ?, updated_at = ? +WHERE id = ? AND status IN ('pending', 'running') +` + +type FinishSuggestionBatchParams struct { + Status string + UpdatedAt string + ID string +} + +// Records a pending or running batch as done or failed; a batch already +// finished is left as it is (0 rows). +func (q *Queries) FinishSuggestionBatch(ctx context.Context, arg FinishSuggestionBatchParams) (int64, error) { + result, err := q.db.ExecContext(ctx, finishSuggestionBatch, arg.Status, arg.UpdatedAt, arg.ID) + if err != nil { + return 0, err + } + return result.RowsAffected() +} + +const getActiveSuggestionBatch = `-- name: GetActiveSuggestionBatch :one +SELECT id, "trigger", status, ai_run_id, created_at, updated_at FROM suggestion_batches +WHERE status IN ('pending', 'running') +ORDER BY id +LIMIT 1 +` + +// The batch still pending or running, oldest first. Returns sql.ErrNoRows +// when there is none. +func (q *Queries) GetActiveSuggestionBatch(ctx context.Context) (SuggestionBatch, error) { + row := q.db.QueryRowContext(ctx, getActiveSuggestionBatch) + var i SuggestionBatch + err := row.Scan( + &i.ID, + &i.Trigger, + &i.Status, + &i.AiRunID, + &i.CreatedAt, + &i.UpdatedAt, + ) + return i, err +} + +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 +FROM suggestions +JOIN dinners ON dinners.id = suggestions.dinner_id +WHERE suggestions.id = ? +` + +type GetSuggestionRow struct { + Suggestion Suggestion + Dinner Dinner +} + +// A suggestion with its dinner. Returns sql.ErrNoRows for an unknown id. +func (q *Queries) GetSuggestion(ctx context.Context, id string) (GetSuggestionRow, error) { + row := q.db.QueryRowContext(ctx, getSuggestion, id) + var i GetSuggestionRow + err := row.Scan( + &i.Suggestion.ID, + &i.Suggestion.DinnerID, + &i.Suggestion.BatchID, + &i.Suggestion.Reason, + &i.Suggestion.CitedRegularIds, + &i.Suggestion.Status, + &i.Suggestion.DecidedAt, + &i.Suggestion.CreatedAt, + &i.Suggestion.UpdatedAt, + &i.Dinner.ID, + &i.Dinner.Name, + &i.Dinner.NormalizedName, + &i.Dinner.Status, + &i.Dinner.Origin, + &i.Dinner.Method, + &i.Dinner.MinutesTotal, + &i.Dinner.Difficulty, + &i.Dinner.Cuisine, + &i.Dinner.IsFavorite, + &i.Dinner.RecipeID, + &i.Dinner.PhotoID, + &i.Dinner.GroundingStatus, + &i.Dinner.UpdatedBy, + &i.Dinner.CreatedAt, + &i.Dinner.UpdatedAt, + &i.Dinner.RecipeUrl, + &i.Dinner.GroundingError, + &i.Dinner.GroundingRevision, + ) + return i, err +} + +const getSuggestionBatchJobID = `-- name: GetSuggestionBatchJobID :one +SELECT id FROM jobs +WHERE kind = ?1 + AND json_extract(payload, '$.batch_id') = ?2 +ORDER BY id +LIMIT 1 +` + +type GetSuggestionBatchJobIDParams struct { + Kind string + BatchID string +} + +// The id of the job that runs batch batch_id. +func (q *Queries) GetSuggestionBatchJobID(ctx context.Context, arg GetSuggestionBatchJobIDParams) (string, error) { + row := q.db.QueryRowContext(ctx, getSuggestionBatchJobID, arg.Kind, arg.BatchID) + var id string + err := row.Scan(&id) + return id, err +} + +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 +FROM suggestions +JOIN dinners ON dinners.id = suggestions.dinner_id +WHERE suggestions.status = 'pending' AND dinners.status = 'suggested' +ORDER BY suggestions.created_at, suggestions.id +` + +type ListDeckRow struct { + Suggestion Suggestion + Dinner Dinner +} + +// The pending suggestions whose dinner is still suggested, oldest first, +// with their dinners. +func (q *Queries) ListDeck(ctx context.Context) ([]ListDeckRow, error) { + rows, err := q.db.QueryContext(ctx, listDeck) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListDeckRow + for rows.Next() { + var i ListDeckRow + if err := rows.Scan( + &i.Suggestion.ID, + &i.Suggestion.DinnerID, + &i.Suggestion.BatchID, + &i.Suggestion.Reason, + &i.Suggestion.CitedRegularIds, + &i.Suggestion.Status, + &i.Suggestion.DecidedAt, + &i.Suggestion.CreatedAt, + &i.Suggestion.UpdatedAt, + &i.Dinner.ID, + &i.Dinner.Name, + &i.Dinner.NormalizedName, + &i.Dinner.Status, + &i.Dinner.Origin, + &i.Dinner.Method, + &i.Dinner.MinutesTotal, + &i.Dinner.Difficulty, + &i.Dinner.Cuisine, + &i.Dinner.IsFavorite, + &i.Dinner.RecipeID, + &i.Dinner.PhotoID, + &i.Dinner.GroundingStatus, + &i.Dinner.UpdatedBy, + &i.Dinner.CreatedAt, + &i.Dinner.UpdatedAt, + &i.Dinner.RecipeUrl, + &i.Dinner.GroundingError, + &i.Dinner.GroundingRevision, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const listDeclinedSuggestionNames = `-- name: ListDeclinedSuggestionNames :many +SELECT dinners.name +FROM suggestions +JOIN dinners ON dinners.id = suggestions.dinner_id +WHERE suggestions.status = 'declined' +ORDER BY suggestions.decided_at, suggestions.id +` + +// The name of every declined suggestion's dinner, oldest decision first. +func (q *Queries) ListDeclinedSuggestionNames(ctx context.Context) ([]string, error) { + rows, err := q.db.QueryContext(ctx, listDeclinedSuggestionNames) + if err != nil { + return nil, err + } + defer rows.Close() + var items []string + for rows.Next() { + var name string + if err := rows.Scan(&name); err != nil { + return nil, err + } + items = append(items, name) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const moveSuggestedDinner = `-- name: MoveSuggestedDinner :execrows +UPDATE dinners +SET status = ?1, updated_by = ?2, updated_at = ?3 +WHERE id = ?4 AND status = 'suggested' +` + +type MoveSuggestedDinnerParams struct { + Status string + UpdatedBy sql.NullString + UpdatedAt string + ID string +} + +// Moves a dinner still suggested to status (quest_log or archived), +// recording updated_by; a dinner no longer suggested is left as it is +// (0 rows). +func (q *Queries) MoveSuggestedDinner(ctx context.Context, arg MoveSuggestedDinnerParams) (int64, error) { + result, err := q.db.ExecContext(ctx, moveSuggestedDinner, + arg.Status, + arg.UpdatedBy, + arg.UpdatedAt, + arg.ID, + ) + if err != nil { + return 0, err + } + return result.RowsAffected() +} + +const startSuggestionBatch = `-- name: StartSuggestionBatch :execrows +UPDATE suggestion_batches +SET status = 'running', updated_at = ? +WHERE id = ? AND status = 'pending' +` + +type StartSuggestionBatchParams struct { + UpdatedAt string + ID string +} + +// Marks a pending batch running; a batch in any other status is left as +// it is (0 rows). +func (q *Queries) StartSuggestionBatch(ctx context.Context, arg StartSuggestionBatchParams) (int64, error) { + result, err := q.db.ExecContext(ctx, startSuggestionBatch, arg.UpdatedAt, arg.ID) + if err != nil { + return 0, err + } + return result.RowsAffected() +} diff --git a/internal/server/aisettings_test.go b/internal/server/aisettings_test.go index 4c58c43..bc4dd39 100644 --- a/internal/server/aisettings_test.go +++ b/internal/server/aisettings_test.go @@ -28,7 +28,7 @@ const ( func newAIRouter(t *testing.T) http.Handler { t.Helper() logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - r, err := NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, newTestSettings(t), newTestSearch(t), newTestAI(t), newTestDinners(t), + r, err := NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, newTestSettings(t), newTestSearch(t), newTestAI(t), newTestDinners(t), newTestSuggest(t), newTestRunner(t), newTestPhotos(t), fstest.MapFS{"index.html": {Data: []byte(fixtureIndex)}}) if err != nil { t.Fatal(err) diff --git a/internal/server/dinners_test.go b/internal/server/dinners_test.go index 49e617c..a8c312f 100644 --- a/internal/server/dinners_test.go +++ b/internal/server/dinners_test.go @@ -86,7 +86,7 @@ func newDinnersEnv(t *testing.T) *dinnersEnv { t.Fatal(err) } e.h, err = NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, newTestSettings(t), newTestSearch(t), newTestAI(t), - svc, runner, newTestPhotos(t), fstest.MapFS{"index.html": {Data: []byte(fixtureIndex)}}) + svc, newTestSuggest(t), runner, newTestPhotos(t), fstest.MapFS{"index.html": {Data: []byte(fixtureIndex)}}) if err != nil { t.Fatal(err) } diff --git a/internal/server/login_test.go b/internal/server/login_test.go index b801274..24f6a37 100644 --- a/internal/server/login_test.go +++ b/internal/server/login_test.go @@ -83,7 +83,7 @@ func newLoginRouter(t *testing.T, issuer string) http.Handler { t.Fatal(err) } r, err := NewRouter(logger, "dev", trusted, flow, sessions, newTestSettings(t), - newTestSearch(t), newTestAI(t), newTestDinners(t), newTestRunner(t), newTestPhotos(t), fstest.MapFS{"index.html": {Data: []byte(fixtureIndex)}}) + newTestSearch(t), newTestAI(t), newTestDinners(t), newTestSuggest(t), newTestRunner(t), newTestPhotos(t), fstest.MapFS{"index.html": {Data: []byte(fixtureIndex)}}) if err != nil { t.Fatal(err) } diff --git a/internal/server/media_test.go b/internal/server/media_test.go index e1aed7b..c2ce862 100644 --- a/internal/server/media_test.go +++ b/internal/server/media_test.go @@ -51,7 +51,7 @@ func newMediaRouter(t *testing.T) (r http.Handler, id, orphan string) { t.Fatal(err) } logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - r, err = NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, newTestSettings(t), newTestSearch(t), newTestAI(t), newTestDinners(t), newTestRunner(t), + r, err = NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, newTestSettings(t), newTestSearch(t), newTestAI(t), newTestDinners(t), newTestSuggest(t), newTestRunner(t), photos, fstest.MapFS{"index.html": {Data: []byte(fixtureIndex)}}) if err != nil { t.Fatal(err) diff --git a/internal/server/origin_test.go b/internal/server/origin_test.go index ef97b38..7242e74 100644 --- a/internal/server/origin_test.go +++ b/internal/server/origin_test.go @@ -45,7 +45,7 @@ func newOriginRouter(t *testing.T, logs io.Writer) http.Handler { } logger := slog.New(forwarded.LogHandler(slog.NewJSONHandler(logs, nil))) r, err := NewRouter(logger, "dev", trusted, probeAuth{}, stubSessions{}, newTestSettings(t), - newTestSearch(t), newTestAI(t), newTestDinners(t), newTestRunner(t), newTestPhotos(t), fstest.MapFS{"index.html": {Data: []byte(fixtureIndex)}}) + newTestSearch(t), newTestAI(t), newTestDinners(t), newTestSuggest(t), newTestRunner(t), newTestPhotos(t), fstest.MapFS{"index.html": {Data: []byte(fixtureIndex)}}) if err != nil { t.Fatal(err) } diff --git a/internal/server/recipesearch_test.go b/internal/server/recipesearch_test.go index ecc0d69..5e20f89 100644 --- a/internal/server/recipesearch_test.go +++ b/internal/server/recipesearch_test.go @@ -22,7 +22,7 @@ const ( func newRecipeSearchRouter(t *testing.T, search *recipes.SearchSettingsService) http.Handler { t.Helper() logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - r, err := NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, newTestSettings(t), search, newTestAI(t), newTestDinners(t), newTestRunner(t), + r, err := NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, newTestSettings(t), search, newTestAI(t), newTestDinners(t), newTestSuggest(t), newTestRunner(t), newTestPhotos(t), fstest.MapFS{"index.html": {Data: []byte(fixtureIndex)}}) if err != nil { t.Fatal(err) diff --git a/internal/server/server.go b/internal/server/server.go index 8eaf64c..c81b05e 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -17,6 +17,7 @@ import ( "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" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/suggest" ) // reservedPrefixes are owned by the API router and are never answered with the @@ -38,12 +39,14 @@ type RouteMounter interface { // dinnerSvc serves GET and POST /api/v1/dinners, PATCH and DELETE // /api/v1/dinners/{id}, POST /api/v1/dinners/{id}/favorite, GET // /api/v1/dinners/{id}/recipe-candidates and POST /api/v1/dinners/{id}/ground; -// runner serves +// suggestSvc serves POST /api/v1/suggestions/batches, GET +// /api/v1/suggestions/deck and POST /api/v1/suggestions/{id}/accept and +// /decline; runner serves // POST /api/v1/jobs and GET /api/v1/jobs/{id}; photos serves // GET /media/photos/{id}-{size}.jpg; assets is the web build // served for every other GET. /health, /auth and the web build are public. // Requests to /api/v1 other than GET also need the session's CSRF token in CSRFHeader. -func NewRouter(logger *slog.Logger, version string, trusted forwarded.Trusted, auth RouteMounter, sessions SessionLookup, settings *household.SettingsService, search *recipes.SearchSettingsService, ai *aisettings.Service, dinnerSvc *dinners.Service, runner *jobs.Runner, photos PhotoFiles, assets fs.FS) (http.Handler, error) { +func NewRouter(logger *slog.Logger, version string, trusted forwarded.Trusted, auth RouteMounter, sessions SessionLookup, settings *household.SettingsService, search *recipes.SearchSettingsService, ai *aisettings.Service, dinnerSvc *dinners.Service, suggestSvc *suggest.Service, runner *jobs.Runner, photos PhotoFiles, assets fs.FS) (http.Handler, error) { if settings == nil { return nil, errors.New("server: nil household settings service") } @@ -56,6 +59,9 @@ func NewRouter(logger *slog.Logger, version string, trusted forwarded.Trusted, a if dinnerSvc == nil { return nil, errors.New("server: nil dinners service") } + if suggestSvc == nil { + return nil, errors.New("server: nil suggestions service") + } if runner == nil { return nil, errors.New("server: nil job runner") } @@ -106,6 +112,10 @@ func NewRouter(logger *slog.Logger, version string, trusted forwarded.Trusted, a apiV1.Post("/dinners/{id}/favorite", postDinnerFavorite(logger, dinnerSvc)) apiV1.Get("/dinners/{id}/recipe-candidates", getDinnerCandidates(logger, dinnerSvc)) apiV1.Post("/dinners/{id}/ground", postDinnerGround(logger, dinnerSvc)) + apiV1.Post("/suggestions/batches", postSuggestionBatch(logger, suggestSvc)) + apiV1.Get("/suggestions/deck", getSuggestionDeck(logger, suggestSvc)) + apiV1.Post("/suggestions/{id}/accept", postSuggestionDecision(logger, suggestSvc, true)) + apiV1.Post("/suggestions/{id}/decline", postSuggestionDecision(logger, suggestSvc, false)) apiV1.Post("/jobs", postJob(logger, runner)) apiV1.Get("/jobs/{id}", getJob(logger, runner)) media.Get("/photos/{name}", getPhoto(logger, photos)) diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 5f121ce..05a3807 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -5,6 +5,7 @@ import ( "crypto/rand" "encoding/base64" "encoding/json" + "errors" "io" "log/slog" "maps" @@ -18,6 +19,7 @@ import ( "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/aisettings" "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/dinners" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/household" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/jobs" @@ -25,6 +27,7 @@ import ( "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/recipes/recipestest" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/secretbox" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/store" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/suggest" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/migrations" ) @@ -46,7 +49,7 @@ func newTestRouterWith(t *testing.T, version string, settings *household.Setting "index.html": {Data: []byte(fixtureIndex)}, "assets/index-AbC123.js": {Data: []byte(fixtureAsset)}, } - r, err := NewRouter(logger, version, nil, stubAuth{}, stubSessions{}, settings, newTestSearch(t), newTestAI(t), newTestDinners(t), runner, newTestPhotos(t), assets) + r, err := NewRouter(logger, version, nil, stubAuth{}, stubSessions{}, settings, newTestSearch(t), newTestAI(t), newTestDinners(t), newTestSuggest(t), runner, newTestPhotos(t), assets) if err != nil { t.Fatal(err) } @@ -160,6 +163,28 @@ func newTestDinners(t *testing.T) *dinners.Service { return svc } +// newTestSuggest returns a suggestions service over a migrated temp +// database, with its own unstarted job runner, a grounder that finds +// nothing and a suggester that always fails. +func newTestSuggest(t *testing.T) *suggest.Service { + t.Helper() + st := newTestStore(t) + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + runner, err := jobs.NewRunner(st, logger, 1) + if err != nil { + t.Fatal(err) + } + g := recipestest.NewGrounder(t, st, t.TempDir(), &recipestest.Search{}, &recipestest.Pages{}, &recipestest.Images{}, &llm.Fake{}) + fail := func(context.Context, string, skills.SideQuestsRequest) (skills.SideQuests, error) { + return skills.SideQuests{}, errors.New("no model in this test") + } + svc, err := suggest.NewService(st, g, fail, runner, logger) + if err != nil { + t.Fatal(err) + } + return svc +} + // newTestBox returns a secret box under a random key. func newTestBox(t *testing.T) *secretbox.Box { t.Helper() @@ -191,7 +216,7 @@ func TestNewRouterRequiresAISettings(t *testing.T) { t.Parallel() logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - if _, err := NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, newTestSettings(t), newTestSearch(t), nil, newTestDinners(t), newTestRunner(t), newTestPhotos(t), fstest.MapFS{"index.html": {}}); err == nil { + if _, err := NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, newTestSettings(t), newTestSearch(t), nil, newTestDinners(t), newTestSuggest(t), newTestRunner(t), newTestPhotos(t), fstest.MapFS{"index.html": {}}); err == nil { t.Fatal("NewRouter accepted a nil AI settings service") } } @@ -200,7 +225,7 @@ func TestNewRouterRequiresSearchSettings(t *testing.T) { t.Parallel() logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - if _, err := NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, newTestSettings(t), nil, newTestAI(t), newTestDinners(t), newTestRunner(t), newTestPhotos(t), fstest.MapFS{"index.html": {}}); err == nil { + if _, err := NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, newTestSettings(t), nil, newTestAI(t), newTestDinners(t), newTestSuggest(t), newTestRunner(t), newTestPhotos(t), fstest.MapFS{"index.html": {}}); err == nil { t.Fatal("NewRouter accepted a nil recipe search settings service") } } @@ -209,7 +234,7 @@ func TestNewRouterRequiresPhotoFiles(t *testing.T) { t.Parallel() logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - if _, err := NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, newTestSettings(t), newTestSearch(t), newTestAI(t), newTestDinners(t), newTestRunner(t), nil, fstest.MapFS{"index.html": {}}); err == nil { + if _, err := NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, newTestSettings(t), newTestSearch(t), newTestAI(t), newTestDinners(t), newTestSuggest(t), newTestRunner(t), nil, fstest.MapFS{"index.html": {}}); err == nil { t.Fatal("NewRouter accepted nil photo files") } } @@ -218,7 +243,7 @@ func TestNewRouterRequiresSettings(t *testing.T) { t.Parallel() logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - if _, err := NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, nil, newTestSearch(t), newTestAI(t), newTestDinners(t), newTestRunner(t), newTestPhotos(t), fstest.MapFS{"index.html": {}}); err == nil { + if _, err := NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, nil, newTestSearch(t), newTestAI(t), newTestDinners(t), newTestSuggest(t), newTestRunner(t), newTestPhotos(t), fstest.MapFS{"index.html": {}}); err == nil { t.Fatal("NewRouter accepted a nil settings service") } } @@ -227,16 +252,25 @@ func TestNewRouterRequiresDinners(t *testing.T) { t.Parallel() logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - if _, err := NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, newTestSettings(t), newTestSearch(t), newTestAI(t), nil, newTestRunner(t), newTestPhotos(t), fstest.MapFS{"index.html": {}}); err == nil { + if _, err := NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, newTestSettings(t), newTestSearch(t), newTestAI(t), nil, newTestSuggest(t), newTestRunner(t), newTestPhotos(t), fstest.MapFS{"index.html": {}}); err == nil { t.Fatal("NewRouter accepted a nil dinners service") } } +func TestNewRouterRequiresSuggestions(t *testing.T) { + t.Parallel() + + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + if _, err := NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, newTestSettings(t), newTestSearch(t), newTestAI(t), newTestDinners(t), nil, newTestRunner(t), newTestPhotos(t), fstest.MapFS{"index.html": {}}); err == nil { + t.Fatal("NewRouter accepted a nil suggestions service") + } +} + func TestNewRouterRequiresJobRunner(t *testing.T) { t.Parallel() logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - if _, err := NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, newTestSettings(t), newTestSearch(t), newTestAI(t), newTestDinners(t), nil, newTestPhotos(t), fstest.MapFS{"index.html": {}}); err == nil { + if _, err := NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, newTestSettings(t), newTestSearch(t), newTestAI(t), newTestDinners(t), newTestSuggest(t), nil, newTestPhotos(t), fstest.MapFS{"index.html": {}}); err == nil { t.Fatal("NewRouter accepted a nil job runner") } } diff --git a/internal/server/settings_test.go b/internal/server/settings_test.go index 9570fbc..6586253 100644 --- a/internal/server/settings_test.go +++ b/internal/server/settings_test.go @@ -277,7 +277,7 @@ func TestHouseholdSettingsWithRealSession(t *testing.T) { } logger := slog.New(slog.NewTextHandler(io.Discard, nil)) router, err := NewRouter(logger, "dev", nil, stubAuth{}, sessions, household.NewSettingsService(st), - newTestSearch(t), newTestAI(t), newTestDinners(t), newTestRunner(t), newTestPhotos(t), fstest.MapFS{"index.html": {Data: []byte(fixtureIndex)}}) + newTestSearch(t), newTestAI(t), newTestDinners(t), newTestSuggest(t), newTestRunner(t), newTestPhotos(t), fstest.MapFS{"index.html": {Data: []byte(fixtureIndex)}}) if err != nil { t.Fatal(err) } diff --git a/internal/server/suggestions.go b/internal/server/suggestions.go new file mode 100644 index 0000000..dbe1b84 --- /dev/null +++ b/internal/server/suggestions.go @@ -0,0 +1,82 @@ +package server + +import ( + "errors" + "log/slog" + "net/http" + + "github.com/go-chi/chi/v5" + + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/suggest" +) + +type deck struct { + Suggestions []suggest.Suggestion `json:"suggestions"` +} + +// postSuggestionBatch starts a manual suggestion batch and answers 202 with +// its batch_id and the job_id that GET /jobs/{id} reports on, without +// waiting for the job. When a batch is already pending or running it +// answers 202 with that batch instead of starting another. +func postSuggestionBatch(logger *slog.Logger, svc *suggest.Service) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + b, err := svc.StartBatch(r.Context(), suggest.TriggerManual) + if err != nil { + writeSuggestionError(w, r, logger, "start suggestion batch", err) + return + } + writeJSON(w, r, logger, http.StatusAccepted, b) + } +} + +// getSuggestionDeck answers GET /suggestions/deck with the pending +// suggestions, oldest first. +func getSuggestionDeck(logger *slog.Logger, svc *suggest.Service) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + list, err := svc.Deck(r.Context()) + if err != nil { + writeSuggestionError(w, r, logger, "list suggestion deck", err) + return + } + writeJSON(w, r, logger, http.StatusOK, deck{Suggestions: list}) + } +} + +// postSuggestionDecision records Try it (accept) or Pass (decline) on a +// suggestion, recording the session's actor as its dinner's updated_by. +func postSuggestionDecision(logger *slog.Logger, svc *suggest.Service, accept bool) http.HandlerFunc { + decide, action := svc.Decline, "decline suggestion" + if accept { + decide, action = svc.Accept, "accept suggestion" + } + return func(w http.ResponseWriter, r *http.Request) { + actor, ok := actorOrFail(w, r, logger) + if !ok { + return + } + d, err := decide(r.Context(), chi.URLParam(r, "id"), actor.Subject) + if err != nil { + writeSuggestionError(w, r, logger, action, err) + return + } + writeJSON(w, r, logger, http.StatusOK, d) + } +} + +// writeSuggestionError maps a suggest.Service error to its API error: an +// unknown suggestion is 404, and a decision on one already decided the +// other way, or whose dinner left the deck, is 409. +func writeSuggestionError(w http.ResponseWriter, r *http.Request, logger *slog.Logger, action string, err error) { + var decided *suggest.DecidedError + switch { + case errors.As(err, &decided): + writeError(w, r, logger, http.StatusConflict, "suggestion_decided", decided.Error()) + case errors.Is(err, suggest.ErrDinnerNotSuggested): + writeError(w, r, logger, http.StatusConflict, "dinner_not_suggested", err.Error()) + case errors.Is(err, suggest.ErrNotFound): + writeError(w, r, logger, http.StatusNotFound, "not_found", "suggestion not found") + default: + logger.ErrorContext(r.Context(), action, "error", err) + writeError(w, r, logger, http.StatusInternalServerError, "internal", "internal error") + } +} diff --git a/internal/server/suggestions_test.go b/internal/server/suggestions_test.go new file mode 100644 index 0000000..1674994 --- /dev/null +++ b/internal/server/suggestions_test.go @@ -0,0 +1,189 @@ +package server + +import ( + "context" + "database/sql" + "errors" + "fmt" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "testing" + "testing/fstest" + + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/harness" + "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/jobs" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/recipes/recipestest" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/store" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/suggest" +) + +const suggestionsPath = "/api/v1/suggestions" + +// newSuggestionsRouter returns a router whose suggestions service has an +// unstarted job runner, so batches stay pending, and a deck seeded with +// pending suggestions of dinners named names, in order, whose ids it +// returns. +func newSuggestionsRouter(t *testing.T, names ...string) (http.Handler, *store.Store, []string) { + t.Helper() + st := newTestStore(t) + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + runner, err := jobs.NewRunner(st, logger, 1) + if err != nil { + t.Fatal(err) + } + g := recipestest.NewGrounder(t, st, t.TempDir(), &recipestest.Search{}, &recipestest.Pages{}, &recipestest.Images{}, &llm.Fake{}) + fail := func(context.Context, string, skills.SideQuestsRequest) (skills.SideQuests, error) { + return skills.SideQuests{}, errors.New("no model in this test") + } + svc, err := suggest.NewService(st, g, fail, runner, logger) + if err != nil { + t.Fatal(err) + } + h, err := NewRouter(logger, "dev", nil, stubAuth{}, stubSessions{}, newTestSettings(t), newTestSearch(t), newTestAI(t), + newTestDinners(t), svc, runner, newTestPhotos(t), fstest.MapFS{"index.html": {Data: []byte(fixtureIndex)}}) + if err != nil { + t.Fatal(err) + } + + q := db.New(st.Writer) + ctx := t.Context() + now := store.Now().String() + batch, err := q.CreateSuggestionBatch(ctx, db.CreateSuggestionBatchParams{ID: store.NewID(), Trigger: "manual", CreatedAt: now, UpdatedAt: now}) + if err != nil { + t.Fatal(err) + } + if _, err := q.FinishSuggestionBatch(ctx, db.FinishSuggestionBatchParams{Status: "done", UpdatedAt: now, ID: batch.ID}); err != nil { + t.Fatal(err) + } + var ids []string + for i, name := range names { + d, err := q.CreateDinner(ctx, db.CreateDinnerParams{ + ID: store.NewID(), Name: name, NormalizedName: harness.Normalize(name), Status: "suggested", Origin: "side_quest", + Difficulty: sql.NullString{String: "easy", Valid: true}, GroundingStatus: "grounded", CreatedAt: now, UpdatedAt: now, + }) + if err != nil { + t.Fatal(err) + } + 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: "[]", + CreatedAt: store.Now().String(), UpdatedAt: now, + }); err != nil { + t.Fatal(err) + } + ids = append(ids, id) + } + return h, st, ids +} + +func TestSuggestionDeck(t *testing.T) { + t.Parallel() + h, _, ids := newSuggestionsRouter(t, "Pho", "Ramen") + + rec, body := callDinners(t, h, http.MethodGet, suggestionsPath+"/deck", "") + if rec.Code != http.StatusOK { + t.Fatalf("GET deck = %d: %s", rec.Code, rec.Body) + } + list, _ := body["suggestions"].([]any) + if len(list) != 2 { + t.Fatalf("deck = %v, want 2 suggestions", body) + } + for i, name := range []string{"Pho", "Ramen"} { + s := list[i].(map[string]any) + if s["id"] != ids[i] || s["name"] != name || s["difficulty"] != "easy" || s["reason"] != fmt.Sprintf("Reason %d", i) || s["status"] != "pending" { + t.Errorf("deck[%d] = %v, want %s", i, s, name) + } + if _, ok := s["photo_url"]; !ok { + t.Errorf("deck[%d] has no photo_url key", i) + } + } +} + +func TestSuggestionDecisions(t *testing.T) { + t.Parallel() + h, _, ids := newSuggestionsRouter(t, "Pho", "Ramen", "Laksa", "Bibimbap", "Katsu", "Tonkatsu", "Udon") + + for _, tc := range []struct { + target string + status int + code string + sugStatus string + dinnerStatus string + }{ + {ids[0] + "/accept", http.StatusOK, "", "accepted", "quest_log"}, + {ids[0] + "/accept", http.StatusOK, "", "accepted", "quest_log"}, + {ids[0] + "/decline", http.StatusConflict, "suggestion_decided", "", ""}, + {ids[1] + "/decline", http.StatusOK, "", "declined", "archived"}, + {ids[1] + "/decline", http.StatusOK, "", "declined", "archived"}, + {ids[1] + "/accept", http.StatusConflict, "suggestion_decided", "", ""}, + {"nope/accept", http.StatusNotFound, "not_found", "", ""}, + {"nope/decline", http.StatusNotFound, "not_found", "", ""}, + } { + rec, body := callDinners(t, h, http.MethodPost, suggestionsPath+"/"+tc.target, "") + if rec.Code != tc.status { + t.Fatalf("POST %s = %d, want %d: %s", tc.target, rec.Code, tc.status, rec.Body) + } + if tc.code != "" { + if errorCode(body) != tc.code { + t.Errorf("POST %s error = %v, want %s", tc.target, body, tc.code) + } + continue + } + s, _ := body["suggestion"].(map[string]any) + if s["status"] != tc.sugStatus || s["dinner_status"] != tc.dinnerStatus { + t.Errorf("POST %s = %v, want %s with dinner %s", tc.target, body, tc.sugStatus, tc.dinnerStatus) + } + if body["replenish_job_id"] != nil { + t.Errorf("POST %s replenished with 5 left: %v", tc.target, body["replenish_job_id"]) + } + } + + // A third decision leaves 4: a replenish batch starts. + rec, body := callDinners(t, h, http.MethodPost, suggestionsPath+"/"+ids[2]+"/decline", "") + if rec.Code != http.StatusOK { + t.Fatalf("decline = %d: %s", rec.Code, rec.Body) + } + if id, _ := body["replenish_job_id"].(string); id == "" { + t.Errorf("decline leaving 4 = %v, want a replenish_job_id", body) + } +} + +func TestPostSuggestionBatch(t *testing.T) { + t.Parallel() + h, st, _ := newSuggestionsRouter(t) + + rec, first := callDinners(t, h, http.MethodPost, suggestionsPath+"/batches", "") + if rec.Code != http.StatusAccepted { + t.Fatalf("POST batches = %d: %s", rec.Code, rec.Body) + } + if first["batch_id"] == nil || first["job_id"] == nil || first["trigger"] != "manual" || first["status"] != "pending" { + t.Errorf("POST batches = %v, want a pending manual batch with its job", first) + } + var kind string + if err := st.Reader.QueryRow(`SELECT kind FROM jobs WHERE id = ?`, first["job_id"]).Scan(&kind); err != nil || kind != suggest.KindSuggestionBatch { + t.Errorf("job kind = %q (%v), want %s", kind, err, suggest.KindSuggestionBatch) + } + + rec, again := callDinners(t, h, http.MethodPost, suggestionsPath+"/batches", "") + if rec.Code != http.StatusAccepted || again["batch_id"] != first["batch_id"] || again["job_id"] != first["job_id"] { + t.Errorf("second POST batches = %d %v, want the pending batch %v", rec.Code, again, first) + } +} + +func TestSuggestionWritesNeedCSRF(t *testing.T) { + t.Parallel() + h, _, ids := newSuggestionsRouter(t, "Pho") + for _, target := range []string{"/batches", "/" + ids[0] + "/accept", "/" + ids[0] + "/decline"} { + req := httptest.NewRequest(http.MethodPost, suggestionsPath+target, nil) + req.AddCookie(validSessionCookie) + rec, body := serve(t, h, req) + if rec.Code != http.StatusForbidden || errorCode(body) != "csrf" { + t.Errorf("POST %s without CSRF = %d %v, want 403 csrf", target, rec.Code, body) + } + } +} diff --git a/internal/suggest/batch.go b/internal/suggest/batch.go new file mode 100644 index 0000000..1640b31 --- /dev/null +++ b/internal/suggest/batch.go @@ -0,0 +1,299 @@ +package suggest + +import ( + "context" + "database/sql" + "encoding/json" + "errors" + "fmt" + "time" + + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/harness" + "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/jobs" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/recipes" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/store" +) + +// BatchJob is the KindSuggestionBatch payload: the batch to run. +type BatchJob struct { + BatchID string `json:"batch_id"` +} + +// Batch stages, as the job's result reports them. +const ( + StageSuggesting = "suggesting" + StageGrounding = "grounding" + StageDone = "done" +) + +// batchResult is the KindSuggestionBatch result, published as the batch +// goes: its stage, the suggestions stored so far, and how many Side +// quests were dropped, by suggest_side_quests' rules, because no recipe +// grounds them, or because they repeat a dinner added meanwhile. +type batchResult struct { + BatchID string `json:"batch_id"` + Stage string `json:"stage"` + SuggestionIDs []string `json:"suggestion_ids"` + Dropped int `json:"dropped"` +} + +func (r batchResult) PublicJSON() ([]byte, error) { return json.Marshal(r) } + +// runBatch runs batch p.BatchID: it asks suggest_side_quests for BatchSize +// Side quests given the household context, the deck and every declined +// name, then grounds them in the model's order, at most the number still +// wanted at a time, until BatchSize are kept or none are left. A Side quest +// no recipe grounds is dropped; any other grounding failure fails the +// batch, as does suggest_side_quests failing, with no Side quest made up +// instead. Each round's grounded Side quests are stored before the next +// round starts, so the deck fills as the batch goes. The batch ends done +// or failed; Side quests stored before a failure stay in the deck. +func (s *Service) runBatch(ctx context.Context, p BatchJob, publish func(batchResult) error) (batchResult, error) { + n, err := db.New(s.st.Writer).StartSuggestionBatch(ctx, db.StartSuggestionBatchParams{ + UpdatedAt: store.Now().String(), + ID: p.BatchID, + }) + if err != nil { + return batchResult{}, fmt.Errorf("suggest: start batch %s: %w", p.BatchID, err) + } + if n == 0 { + return batchResult{}, fmt.Errorf("suggest: batch %s is not pending", p.BatchID) + } + res, err := s.fill(ctx, p.BatchID, publish) + write := context.WithoutCancel(ctx) + if err == nil { + s.finishBatch(write, p.BatchID, BatchDone) + return res, nil + } + s.finishBatch(write, p.BatchID, BatchFailed) + if ctx.Err() != nil { + return batchResult{}, ctx.Err() + } + var failure *jobs.Failure + if errors.As(err, &failure) { + // Only the code is logged: the error may quote a model's or + // search provider's response. + s.logger.InfoContext(ctx, "suggestion batch failed", "batch_id", p.BatchID, "reason", failure.Code) + } + return batchResult{}, err +} + +// fill does runBatch's work, returning a *jobs.Failure for a failure the +// household may see. +func (s *Service) fill(ctx context.Context, batchID string, publish func(batchResult) error) (batchResult, error) { + res := batchResult{BatchID: batchID, Stage: StageSuggesting, SuggestionIDs: []string{}} + if err := publish(res); err != nil { + return batchResult{}, err + } + in, err := s.request(ctx) + if err != nil { + return batchResult{}, err + } + out, err := s.suggest(ctx, batchID, in) + if err != nil { + if ctx.Err() != nil { + return batchResult{}, ctx.Err() + } + return batchResult{}, &jobs.Failure{Code: ReasonAIFailed} + } + res.Stage = StageGrounding + res.Dropped = len(out.Dropped) + if err := publish(res); err != nil { + return batchResult{}, err + } + left := out.Suggestions + for len(res.SuggestionIDs) < BatchSize && len(left) > 0 { + round := left[:min(BatchSize-len(res.SuggestionIDs), len(left))] + left = left[len(round):] + grounded, err := s.ground(ctx, round) + if err != nil { + return batchResult{}, err + } + res.Dropped += len(round) - len(grounded) + ids, err := s.store(ctx, batchID, grounded) + if err != nil { + return batchResult{}, err + } + res.Dropped += len(grounded) - len(ids) + res.SuggestionIDs = append(res.SuggestionIDs, ids...) + if err := publish(res); err != nil { + return batchResult{}, err + } + } + res.Stage = StageDone + return res, nil +} + +// request reads what suggest_side_quests is given: the household context, +// the deck's names and every declined name. A household with no Regulars +// fails with ReasonNoRegulars. +func (s *Service) request(ctx context.Context) (skills.SideQuestsRequest, error) { + hctx, err := harness.BuildContext(ctx, s.st, time.Now(), ContextBudget) + if err != nil { + return skills.SideQuestsRequest{}, fmt.Errorf("suggest: %w", err) + } + if len(hctx.Regulars) == 0 { + return skills.SideQuestsRequest{}, &jobs.Failure{Code: ReasonNoRegulars} + } + q := db.New(s.st.Reader) + deck, err := q.ListDeck(ctx) + if err != nil { + return skills.SideQuestsRequest{}, fmt.Errorf("suggest: list deck: %w", err) + } + pending := []string{} + for _, row := range deck { + pending = append(pending, row.Dinner.Name) + } + declined, err := q.ListDeclinedSuggestionNames(ctx) + if err != nil { + return skills.SideQuestsRequest{}, fmt.Errorf("suggest: list declined names: %w", err) + } + return skills.SideQuestsRequest{Context: hctx, Needed: BatchSize, Pending: pending, Declined: declined}, nil +} + +// groundedQuest is a Side quest with the recipe that grounds it. +type groundedQuest struct { + quest skills.SideQuest + grounded recipes.Grounded +} + +// ground grounds quests by their search queries and returns those a +// recipe grounds, in order. A Side quest no recipe grounds is left out; +// any other failure fails the batch. +func (s *Service) ground(ctx context.Context, quests []skills.SideQuest) ([]groundedQuest, error) { + reqs := make([]recipes.GroundRequest, len(quests)) + for i, q := range quests { + reqs[i] = recipes.GroundRequest{DishName: q.Name, Query: q.SearchQuery} + } + results := s.grounder.GroundMany(ctx, reqs) + if err := ctx.Err(); err != nil { + return nil, err + } + var out []groundedQuest + for i, r := range results { + var ungroundable *recipes.ErrUngroundable + switch { + case r.Err == nil: + out = append(out, groundedQuest{quest: quests[i], grounded: r.Grounded}) + case errors.As(r.Err, &ungroundable): + case errors.Is(r.Err, recipes.ErrSearchNotConfigured): + return nil, &jobs.Failure{Code: ReasonSearchNotConfigured} + case errors.Is(r.Err, recipes.ErrSearchFailed): + return nil, &jobs.Failure{Code: ReasonSearchFailed} + case errors.Is(r.Err, recipes.ErrNormalizeFailed): + return nil, &jobs.Failure{Code: ReasonAIFailed} + default: + return nil, fmt.Errorf("suggest: ground %q: %w", quests[i].Name, r.Err) + } + } + return out, nil +} + +// store stores each grounded Side quest of batchID as a dinner with status +// 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. +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 { + q := db.New(tx) + existing, err := q.ListUnarchivedDinners(ctx) + if err != nil { + return fmt.Errorf("suggest: list dinners for dedupe: %w", err) + } + var names []string + regulars := map[string][]string{} + for _, row := range existing { + names = append(names, row.Dinner.Name) + if row.Dinner.Status == dinners.StatusRegular { + regulars[row.Dinner.Name] = append(regulars[row.Dinner.Name], row.Dinner.ID) + } + } + for _, gq := range quests { + cited, ok := citedIDs(gq.quest.CitedRegulars, regulars) + if !ok || repeats(gq.quest.Name, names) { + s.logger.InfoContext(ctx, "suggested dinner dropped: changed while grounding", "batch_id", batchID) + continue + } + citedJSON, err := json.Marshal(cited) + if err != nil { + return fmt.Errorf("suggest: encode cited Regulars: %w", err) + } + now := store.Now().String() + gr := gq.grounded + d, err := q.CreateDinner(ctx, db.CreateDinnerParams{ + ID: store.NewID(), + Name: gq.quest.Name, + NormalizedName: harness.Normalize(gq.quest.Name), + Status: dinners.StatusSuggested, + Origin: "side_quest", + Method: sql.NullString{String: gr.Method, Valid: true}, + MinutesTotal: nullInt(gr.MinutesTotal), + Difficulty: sql.NullString{String: gr.Difficulty, Valid: true}, + Cuisine: sql.NullString{String: gq.quest.Cuisine, Valid: true}, + RecipeID: sql.NullString{String: gr.RecipeID, Valid: true}, + PhotoID: sql.NullString{String: gr.PhotoID, Valid: true}, + GroundingStatus: dinners.GroundingGrounded, + CreatedAt: now, + UpdatedAt: now, + }) + if err != nil { + return fmt.Errorf("suggest: create dinner: %w", err) + } + id := store.NewID() + if err := q.CreateSuggestion(ctx, db.CreateSuggestionParams{ + ID: id, + DinnerID: d.ID, + BatchID: batchID, + Reason: gq.quest.Reason, + CitedRegularIds: string(citedJSON), + CreatedAt: now, + UpdatedAt: now, + }); err != nil { + return fmt.Errorf("suggest: create suggestion: %w", err) + } + names = append(names, d.Name) + ids = append(ids, id) + } + return nil + }) + if err != nil { + return nil, err + } + return ids, nil +} + +// citedIDs returns the ids of the Regulars named cited, false when one +// names no Regular or more than one. +func citedIDs(cited []string, regulars map[string][]string) ([]string, bool) { + ids := make([]string, len(cited)) + for i, name := range cited { + if len(regulars[name]) != 1 { + return nil, false + } + ids[i] = regulars[name][0] + } + return ids, true +} + +// repeats reports whether name is harness.Similar to any of names. +func repeats(name string, names []string) bool { + for _, n := range names { + if harness.Similar(name, n) { + return true + } + } + return false +} + +func nullInt(v *int) sql.NullInt64 { + if v == nil { + return sql.NullInt64{} + } + return sql.NullInt64{Int64: int64(*v), Valid: true} +} diff --git a/internal/suggest/suggest.go b/internal/suggest/suggest.go new file mode 100644 index 0000000..da5a140 --- /dev/null +++ b/internal/suggest/suggest.go @@ -0,0 +1,415 @@ +// Package suggest keeps the household's Side quest deck: batches of new +// dinners the AI suggests, each grounded in a recipe, and the household's +// Try it / Pass decision on each. A batch runs as a job: suggest_side_quests +// proposes Side quests, each is grounded with the recipe grounding pipeline +// (dropping those no recipe grounds), and every one kept is stored as a +// dinner with status suggested and origin side_quest and a pending +// suggestion. Only one batch is pending or running at a time. +package suggest + +import ( + "context" + "database/sql" + "encoding/json" + "errors" + "fmt" + "log/slog" + "slices" + "sync" + + "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/jobs" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/recipes" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/store" +) + +// KindSuggestionBatch is the job that runs a suggestion batch. +const KindSuggestionBatch = "suggestion_batch" + +const ( + // BatchSize is how many Side quests a batch keeps at most. + BatchSize = 5 + // ReplenishBelow is the deck size under which a decision starts a + // replenish batch. + ReplenishBelow = 5 + // ContextBudget is the character budget of the household context + // suggest_side_quests is given (see harness.BuildContext). + ContextBudget = 16000 +) + +// Batch triggers (the suggestion_batches.trigger column). +const ( + TriggerScheduled = "scheduled" + TriggerManual = "manual" + TriggerReplenish = "replenish" +) + +// Batch statuses (the suggestion_batches.status column). +const ( + BatchPending = "pending" + BatchRunning = "running" + BatchDone = "done" + BatchFailed = "failed" +) + +// Suggestion statuses (the suggestions.status column). +const ( + StatusPending = "pending" + StatusAccepted = "accepted" + StatusDeclined = "declined" +) + +// Reasons a batch failed, stored as the KindSuggestionBatch job's failure +// code. Any other failure, such as the database's, is the runner's +// internal_error. +const ( + // ReasonNoRegulars: the household has no Regulars for Side quests to + // be based on. + ReasonNoRegulars = "no_regulars" + // ReasonAIFailed: suggest_side_quests, or normalize_recipe while + // grounding, failed; no Side quest is made up instead (D-017). + ReasonAIFailed = "ai_failed" + // ReasonSearchNotConfigured: recipe search is not set up. + ReasonSearchNotConfigured = "search_not_configured" + // ReasonSearchFailed: the search provider failed. + ReasonSearchFailed = "search_failed" +) + +// Reasons are every batch failure reason. +var Reasons = []string{ReasonNoRegulars, ReasonAIFailed, ReasonSearchNotConfigured, ReasonSearchFailed} + +var triggers = []string{TriggerScheduled, TriggerManual, TriggerReplenish} + +// ErrNotFound is returned for an unknown suggestion id. +var ErrNotFound = errors.New("suggestion not found") + +// ErrDinnerNotSuggested is a decision on a pending suggestion whose dinner +// was archived or moved out of the deck some other way. +var ErrDinnerNotSuggested = errors.New("the suggestion's dinner is no longer suggested") + +// DecidedError is a decision on a suggestion already decided the other +// way. +type DecidedError struct { + // Status is the suggestion's decision: accepted or declined. + Status string +} + +func (e *DecidedError) Error() string { + return "the suggestion was already " + e.Status +} + +// Suggester runs suggest_side_quests on in; scope names the batch. In +// production it is skills.SuggestSideQuests with the household's +// harness.Config. +type Suggester func(ctx context.Context, scope string, in skills.SideQuestsRequest) (skills.SideQuests, error) + +// Service runs suggestion batches and records decisions. +type Service struct { + st *store.Store + grounder *recipes.Grounder + 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 +// with grounder and registers the KindSuggestionBatch job on runner, which +// must not have started yet. +func NewService(st *store.Store, grounder *recipes.Grounder, suggest Suggester, runner *jobs.Runner, logger *slog.Logger) (*Service, error) { + switch { + case st == nil: + return nil, errors.New("suggest: nil store") + case grounder == nil: + return nil, errors.New("suggest: nil grounder") + case suggest == nil: + return nil, errors.New("suggest: nil suggester") + case runner == nil: + return nil, errors.New("suggest: nil job runner") + case logger == nil: + return nil, errors.New("suggest: nil logger") + } + s := &Service{st: st, grounder: grounder, suggest: suggest, logger: logger} + kind, err := jobs.Register(runner, KindSuggestionBatch, s.runBatch, Reasons...) + if err != nil { + return nil, err + } + s.batchJob = kind + return s, nil +} + +// Batch is a suggestion batch with the id of the job running it. +type Batch struct { + ID string `json:"batch_id"` + JobID string `json:"job_id"` + Trigger string `json:"trigger"` + Status string `json:"status"` + // Created says whether this call created the batch rather than found + // it already pending or running. + Created bool `json:"-"` +} + +// StartBatch creates a batch with trigger and enqueues the +// KindSuggestionBatch job that runs it, unless a batch is already pending +// or running, which it returns instead with Created false. +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 + 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 + }) + 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) + } + return b, 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{ + Status: status, + UpdatedAt: store.Now().String(), + ID: id, + }); err != nil { + s.logger.ErrorContext(ctx, "record suggestion batch status", "batch_id", id, "status", status, "error", err) + } +} + +// FailInterruptedBatches fails every batch still pending or running whose +// KindSuggestionBatch job is no longer queued or running, so a batch a +// previous process left unfinished does not block every later one. Call it +// once at boot, after the job runner's Start has failed the jobs that +// process left running. It returns how many it failed. +func (s *Service) FailInterruptedBatches(ctx context.Context) (int64, error) { + n, err := db.New(s.st.Writer).FailOrphanedSuggestionBatches(ctx, db.FailOrphanedSuggestionBatchesParams{ + UpdatedAt: store.Now().String(), + Kind: KindSuggestionBatch, + }) + if err != nil { + return 0, fmt.Errorf("suggest: fail interrupted batches: %w", err) + } + if n > 0 { + s.logger.WarnContext(ctx, "failed suggestion batches interrupted by restart", "count", n) + } + return n, nil +} + +// Suggestion is a Side quest suggestion as the API shows it, with its +// dinner. +type Suggestion struct { + ID string `json:"id"` + DinnerID string `json:"dinner_id"` + Name string `json:"name"` + PhotoURL *string `json:"photo_url"` + Difficulty *string `json:"difficulty"` + Method *string `json:"method"` + MinutesTotal *int64 `json:"minutes_total"` + Cuisine *string `json:"cuisine"` + // Reason is the "Why this?". + Reason string `json:"reason"` + CitedRegularIDs []string `json:"cited_regular_ids"` + Status string `json:"status"` + DinnerStatus string `json:"dinner_status"` + DecidedAt *string `json:"decided_at"` + CreatedAt string `json:"created_at"` +} + +// Deck returns the pending suggestions whose dinner is still suggested, +// oldest first. +func (s *Service) Deck(ctx context.Context) ([]Suggestion, error) { + rows, err := db.New(s.st.Reader).ListDeck(ctx) + if err != nil { + return nil, fmt.Errorf("suggest: list deck: %w", err) + } + out := make([]Suggestion, len(rows)) + for i, row := range rows { + out[i], err = fromRow(row.Suggestion, row.Dinner) + if err != nil { + return nil, err + } + } + return out, nil +} + +// Decision is a suggestion once decided, with the id of the replenish +// batch's job when the decision started one. +type Decision struct { + Suggestion Suggestion `json:"suggestion"` + ReplenishJobID *string `json:"replenish_job_id"` +} + +// Accept records Try it on suggestion id, moving its dinner to the Quest +// log and recording updatedBy on it. +func (s *Service) Accept(ctx context.Context, id, updatedBy string) (Decision, error) { + return s.decide(ctx, id, StatusAccepted, dinners.StatusQuestLog, updatedBy) +} + +// 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. +func (s *Service) Decline(ctx context.Context, id, updatedBy string) (Decision, error) { + return s.decide(ctx, id, StatusDeclined, dinners.StatusArchived, updatedBy) +} + +// decide records decision on suggestion id and moves its dinner to +// 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. +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 + err := s.st.WithTx(ctx, func(tx *sql.Tx) error { + q := db.New(tx) + cur, err := q.GetSuggestion(ctx, id) + if errors.Is(err, sql.ErrNoRows) { + return ErrNotFound + } + if err != nil { + return fmt.Errorf("suggest: read suggestion %s: %w", id, err) + } + switch cur.Suggestion.Status { + case decision: + row = cur + return nil + case StatusPending: + default: + return &DecidedError{Status: cur.Suggestion.Status} + } + if cur.Dinner.Status != dinners.StatusSuggested { + return ErrDinnerNotSuggested + } + now := store.Now().String() + if _, err := q.DecideSuggestion(ctx, db.DecideSuggestionParams{ + Status: decision, + Now: sql.NullString{String: now, Valid: true}, + ID: id, + }); err != nil { + return fmt.Errorf("suggest: decide %s: %w", id, err) + } + if _, err := q.MoveSuggestedDinner(ctx, db.MoveSuggestedDinnerParams{ + Status: dinnerStatus, + UpdatedBy: sql.NullString{String: updatedBy, Valid: true}, + UpdatedAt: now, + ID: cur.Dinner.ID, + }); err != nil { + return fmt.Errorf("suggest: move dinner %s: %w", cur.Dinner.ID, err) + } + row, err = q.GetSuggestion(ctx, id) + if err != nil { + return fmt.Errorf("suggest: read suggestion %s: %w", id, err) + } + changed = true + return nil + }) + if err != nil { + return Decision{}, err + } + 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 + } + return d, nil +} + +func fromRow(sg db.Suggestion, d db.Dinner) (Suggestion, error) { + out := Suggestion{ + ID: sg.ID, + DinnerID: d.ID, + Name: d.Name, + Difficulty: nullString(d.Difficulty), + Method: nullString(d.Method), + Cuisine: nullString(d.Cuisine), + Reason: sg.Reason, + Status: sg.Status, + DinnerStatus: d.Status, + DecidedAt: nullString(sg.DecidedAt), + CreatedAt: sg.CreatedAt, + } + if err := json.Unmarshal([]byte(sg.CitedRegularIds), &out.CitedRegularIDs); err != nil || out.CitedRegularIDs == nil { + return Suggestion{}, fmt.Errorf("suggest: suggestion %s cited_regular_ids is not a JSON array: %q", sg.ID, sg.CitedRegularIds) + } + if d.MinutesTotal.Valid { + out.MinutesTotal = &d.MinutesTotal.Int64 + } + if d.PhotoID.Valid { + u := dinners.PhotoURL(d.PhotoID.String) + out.PhotoURL = &u + } + return out, nil +} + +func nullString(s sql.NullString) *string { + if !s.Valid { + return nil + } + return &s.String +} diff --git a/internal/suggest/suggest_test.go b/internal/suggest/suggest_test.go new file mode 100644 index 0000000..5e80703 --- /dev/null +++ b/internal/suggest/suggest_test.go @@ -0,0 +1,769 @@ +package suggest_test + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "log/slog" + "slices" + "strings" + "sync" + "testing" + "time" + + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/harness" + "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/household" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/jobs" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/recipes" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/recipes/recipestest" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/store" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/suggest" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/migrations" +) + +const member = "member-1" + +type env struct { + svc *suggest.Service + st *store.Store + runner *jobs.Runner + search *recipestest.Search + pages *recipestest.Pages + images *recipestest.Images + // model answers normalize_recipe while grounding; side answers + // suggest_side_quests. + model *llm.Fake + side *llm.Fake + // regulars are the Regulars' ids by name. + regulars map[string]string + + mu sync.Mutex + inputs []skills.SideQuestsRequest +} + +// newEnv returns a suggestions service over a migrated store with the +// Regulars Beef Chili and Chicken Fajitas and an unstarted job runner. It +// suggests through the real harness and suggest_side_quests with the +// scripted e.side, and grounds with fake search, pages and images and the +// scripted e.model. +func newEnv(t *testing.T) *env { + t.Helper() + ctx := t.Context() + dataDir := t.TempDir() + st, err := store.Open(ctx, dataDir) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + if err := st.Close(); err != nil { + t.Error(err) + } + }) + if err := store.Migrate(ctx, st.Writer, migrations.FS); err != nil { + t.Fatal(err) + } + if err := household.SeedSettings(ctx, st, "America/New_York"); err != nil { + t.Fatal(err) + } + e := &env{ + st: st, + search: &recipestest.Search{Results: map[string][]recipes.Candidate{}}, + pages: &recipestest.Pages{Bodies: map[string][]byte{}}, + images: &recipestest.Images{JPEGs: map[string][]byte{}}, + model: &llm.Fake{}, + side: &llm.Fake{}, + regulars: map[string]string{}, + } + for _, name := range []string{"Beef Chili", "Chicken Fajitas"} { + e.regulars[name] = e.dinner(t, name, "regular") + } + g := recipestest.NewGrounder(t, st, dataDir, e.search, e.pages, e.images, e.model) + guard, err := harness.NewGuard(harness.DefaultMaxCalls) + if err != nil { + t.Fatal(err) + } + cfg := harness.Config{ + Settings: llm.Settings{Provider: "anthropic", Model: "claude-test", Temperature: 0.7}, + NewProvider: func(llm.Settings, string) (llm.Provider, error) { return e.side, nil }, + Store: st, + Guard: guard, + } + suggester := func(ctx context.Context, scope string, in skills.SideQuestsRequest) (skills.SideQuests, error) { + e.mu.Lock() + e.inputs = append(e.inputs, in) + e.mu.Unlock() + return skills.SuggestSideQuests(ctx, cfg, scope, in) + } + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + e.runner, err = jobs.NewRunner(st, logger, 1) + if err != nil { + t.Fatal(err) + } + e.svc, err = suggest.NewService(st, g, suggester, e.runner, logger) + if err != nil { + t.Fatal(err) + } + return e +} + +// start starts the env's job runner, stopping it when the test ends. +func (e *env) start(t *testing.T) { + t.Helper() + ctx, cancel := context.WithCancel(context.Background()) + if err := e.runner.Start(ctx); err != nil { + cancel() + t.Fatal(err) + } + t.Cleanup(func() { + cancel() + e.runner.Wait() + }) +} + +// dinner stores a dinner named name with status and returns its id. +func (e *env) dinner(t *testing.T, name, status string) string { + t.Helper() + now := store.Now().String() + origin := "regular" + if status == "suggested" { + origin = "side_quest" + } + d, err := db.New(e.st.Writer).CreateDinner(t.Context(), db.CreateDinnerParams{ + ID: store.NewID(), + Name: name, + NormalizedName: harness.Normalize(name), + Status: status, + Origin: origin, + GroundingStatus: "manual", + CreatedAt: now, + UpdatedAt: now, + }) + if err != nil { + t.Fatal(err) + } + return d.ID +} + +// seedDeck stores a finished batch holding pending suggestions of +// dinners named names, in order, and returns their ids. +func (e *env) seedDeck(t *testing.T, names ...string) []string { + t.Helper() + q := db.New(e.st.Writer) + now := store.Now().String() + b, err := q.CreateSuggestionBatch(t.Context(), db.CreateSuggestionBatchParams{ID: store.NewID(), Trigger: "manual", CreatedAt: now, UpdatedAt: now}) + if err != nil { + t.Fatal(err) + } + if _, err := q.FinishSuggestionBatch(t.Context(), db.FinishSuggestionBatchParams{Status: "done", UpdatedAt: now, ID: b.ID}); err != nil { + t.Fatal(err) + } + var ids []string + for _, name := range names { + id := store.NewID() + if err := q.CreateSuggestion(t.Context(), db.CreateSuggestionParams{ + ID: id, + DinnerID: e.dinner(t, name, "suggested"), + BatchID: b.ID, + Reason: "Like your Beef Chili.", + CitedRegularIds: fmt.Sprintf("[%q]", e.regulars["Beef Chili"]), + CreatedAt: store.Now().String(), + UpdatedAt: now, + }); err != nil { + t.Fatal(err) + } + ids = append(ids, id) + } + return ids +} + +// recipe makes the search for name find one page that grounds it, and +// scripts the normalize_recipe reply grounding it takes. +func (e *env) recipe(t *testing.T, name string) { + t.Helper() + slug := strings.ReplaceAll(strings.ToLower(name), " ", "-") + page, photo := "https://r.example/"+slug, "https://r.example/"+slug+".jpg" + e.search.Results[name] = []recipes.Candidate{{URL: page}} + e.pages.Bodies[page] = recipestest.RecipePage(name, "R Kitchen", photo, 35) + e.images.JPEGs[photo] = recipestest.JPEG(t, 400, 320, uint8(len(e.images.JPEGs)+1)) + e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 35)) +} + +// sideQuest renders one Side quest of a suggest_side_quests reply whose +// search query is its name. +func sideQuest(name, reason string, cited ...string) string { + c, _ := json.Marshal(cited) + r, _ := json.Marshal(reason) + return fmt.Sprintf(`{"name":%q,"cuisine":"Fusion","method":"stovetop","est_minutes":35,"difficulty":"easy","reason":%s,"cited_regulars":%s,"search_query":%q}`, + name, r, c, name) +} + +// reply scripts one suggest_side_quests reply holding quests. +func (e *env) reply(quests ...string) { + js := `{"suggestions":[` + strings.Join(quests, ",") + `]}` + e.side.QueueGenerate(llm.GenerateResult{Response: llm.Response{Text: js, JSON: json.RawMessage(js)}}) +} + +// waitJob waits for job id to finish and returns it. +func (e *env) waitJob(t *testing.T, id string) jobs.Job { + t.Helper() + deadline := time.Now().Add(10 * time.Second) + for { + job, err := e.runner.Get(t.Context(), id) + if err != nil { + t.Fatal(err) + } + if job.Status == jobs.StatusSucceeded || job.Status == jobs.StatusFailed { + return job + } + if time.Now().After(deadline) { + t.Fatalf("job %s still %s", id, job.Status) + } + time.Sleep(5 * time.Millisecond) + } +} + +type batchResult struct { + BatchID string `json:"batch_id"` + Stage string `json:"stage"` + SuggestionIDs []string `json:"suggestion_ids"` + Dropped int `json:"dropped"` +} + +// runBatch starts a manual batch, waits for it to succeed and returns its +// result. +func (e *env) runBatch(t *testing.T) batchResult { + t.Helper() + b, err := e.svc.StartBatch(t.Context(), suggest.TriggerManual) + if err != nil { + t.Fatal(err) + } + if !b.Created { + t.Fatalf("batch %s was already active", b.ID) + } + job := e.waitJob(t, b.JobID) + if job.Status != jobs.StatusSucceeded { + t.Fatalf("batch job %s, error %v", job.Status, *job.Error) + } + var res batchResult + if err := json.Unmarshal(job.Result, &res); err != nil { + t.Fatal(err) + } + if res.BatchID != b.ID || res.Stage != suggest.StageDone { + t.Fatalf("batch result = %+v, want batch %s done", res, b.ID) + } + if got := e.batchStatus(t, b.ID); got != suggest.BatchDone { + t.Fatalf("batch status = %s, want done", got) + } + return res +} + +func (e *env) batchStatus(t *testing.T, id string) string { + t.Helper() + var status string + if err := e.st.Reader.QueryRowContext(t.Context(), `SELECT status FROM suggestion_batches WHERE id = ?`, id).Scan(&status); err != nil { + t.Fatal(err) + } + return status +} + +func (e *env) activeBatches(t *testing.T) int { + t.Helper() + var n int + if err := e.st.Reader.QueryRowContext(t.Context(), `SELECT COUNT(*) FROM suggestion_batches WHERE status IN ('pending', 'running')`).Scan(&n); err != nil { + t.Fatal(err) + } + return n +} + +func (e *env) dinnerStatus(t *testing.T, id string) string { + t.Helper() + row, err := db.New(e.st.Reader).GetDinner(t.Context(), id) + if err != nil { + t.Fatal(err) + } + return row.Status +} + +// deckNames returns the names Deck lists, in order. +func (e *env) deckNames(t *testing.T) []string { + t.Helper() + deck, err := e.svc.Deck(t.Context()) + if err != nil { + t.Fatal(err) + } + names := []string{} + for _, s := range deck { + names = append(names, s.Name) + } + return names +} + +func TestBatchStoresGroundedSideQuests(t *testing.T) { + e := newEnv(t) + e.start(t) + e.recipe(t, "Chicken Shawarma Bowls") + e.recipe(t, "Chili Verde") + e.reply( + sideQuest("Chicken Shawarma Bowls", "Spiced chicken, like your Chicken Fajitas.", "Chicken Fajitas"), + sideQuest("Mystery Stew", "Hearty like your Beef Chili.", "Beef Chili"), // no recipe grounds it + sideQuest("Chili Verde", "Your Beef Chili gone green.", "Beef Chili"), + sideQuest("Lamb Tagine", "Slow-cooked like your Pot Roast.", "Pot Roast"), // cites no Regular + ) + + res := e.runBatch(t) + if len(res.SuggestionIDs) != 2 || res.Dropped != 2 { + t.Fatalf("result = %+v, want 2 suggestions and 2 dropped", res) + } + if g, _, _ := e.model.Remaining(); g != 0 { + t.Errorf("%d normalize_recipe replies unused", g) + } + if q := e.search.Queries(); slices.Contains(q, "Lamb Tagine") { + t.Errorf("searched %q for a Side quest the skill dropped", q) + } + + deck, err := e.svc.Deck(t.Context()) + if err != nil { + t.Fatal(err) + } + if len(deck) != 2 { + t.Fatalf("deck has %d suggestions, want 2", len(deck)) + } + want := []struct{ name, reason, regular string }{ + {"Chicken Shawarma Bowls", "Spiced chicken, like your Chicken Fajitas.", "Chicken Fajitas"}, + {"Chili Verde", "Your Beef Chili gone green.", "Beef Chili"}, + } + for i, w := range want { + s := deck[i] + if s.ID != res.SuggestionIDs[i] || s.Name != w.name || s.Reason != w.reason || s.Status != suggest.StatusPending { + t.Errorf("deck[%d] = %+v, want %s pending with reason %q", i, s, w.name, w.reason) + } + if !slices.Equal(s.CitedRegularIDs, []string{e.regulars[w.regular]}) { + t.Errorf("deck[%d] cites %v, want %s's id", i, s.CitedRegularIDs, w.regular) + } + if s.PhotoURL == nil || !strings.HasPrefix(*s.PhotoURL, "/media/photos/") || s.Difficulty == nil { + t.Errorf("deck[%d] photo %v, difficulty %v; want both", i, s.PhotoURL, s.Difficulty) + } + row, err := db.New(e.st.Reader).GetDinner(t.Context(), s.DinnerID) + if err != nil { + t.Fatal(err) + } + if row.Status != "suggested" || row.Origin != "side_quest" || row.GroundingStatus != "grounded" || !row.RecipeID.Valid || row.Cuisine.String != "Fusion" { + t.Errorf("dinner %s = %+v, want suggested side_quest grounded in a recipe", w.name, row) + } + } + var dinners int + if err := e.st.Reader.QueryRowContext(t.Context(), `SELECT COUNT(*) FROM dinners WHERE name IN ('Mystery Stew', 'Lamb Tagine')`).Scan(&dinners); err != nil { + t.Fatal(err) + } + if dinners != 0 { + t.Errorf("%d dropped Side quests stored as dinners, want none", dinners) + } +} + +func TestBatchKeepsAtMostBatchSize(t *testing.T) { + e := newEnv(t) + e.start(t) + var quests []string + for i := range suggest.BatchSize + 2 { + name := fmt.Sprintf("Fajita Dish %c", 'A'+i) + e.search.Results[name] = nil // ungroundable unless recipe is called + quests = append(quests, sideQuest(name, "Like your Chicken Fajitas.", "Chicken Fajitas")) + } + // The first round grounds BatchSize Side quests; one fails, so the + // second round grounds one more, and the last is never grounded. + for i := range suggest.BatchSize + 1 { + if i != 1 { + e.recipe(t, fmt.Sprintf("Fajita Dish %c", 'A'+i)) + } + } + e.reply(quests...) + + res := e.runBatch(t) + if len(res.SuggestionIDs) != suggest.BatchSize { + t.Fatalf("kept %d, want %d", len(res.SuggestionIDs), suggest.BatchSize) + } + if slices.Contains(e.search.Queries(), fmt.Sprintf("Fajita Dish %c", 'A'+suggest.BatchSize+1)) { + t.Error("grounded a Side quest after the batch was full") + } + names := e.deckNames(t) + if names[0] != "Fajita Dish A" || names[1] != "Fajita Dish C" || names[len(names)-1] != fmt.Sprintf("Fajita Dish %c", 'A'+suggest.BatchSize) { + t.Errorf("deck = %v, want the model's order without B", names) + } +} + +func TestDeckIsOldestFirst(t *testing.T) { + e := newEnv(t) + e.start(t) + e.recipe(t, "Chicken Shawarma Bowls") + e.recipe(t, "Chili Verde") + e.reply( + sideQuest("Chicken Shawarma Bowls", "Spiced chicken, like your Chicken Fajitas.", "Chicken Fajitas"), + sideQuest("Chili Verde", "Your Beef Chili gone green.", "Beef Chili"), + ) + e.runBatch(t) + e.recipe(t, "Beef Bulgogi") + e.reply(sideQuest("Beef Bulgogi", "Sweet beef for Beef Chili fans.", "Beef Chili")) + e.runBatch(t) + + want := []string{"Chicken Shawarma Bowls", "Chili Verde", "Beef Bulgogi"} + if got := e.deckNames(t); !slices.Equal(got, want) { + t.Errorf("deck = %v, want %v", got, want) + } + // The second batch was told the first one's Side quests are pending. + if got := e.inputs[1].Pending; !slices.Equal(got, want[:2]) { + t.Errorf("second batch pending = %v, want %v", got, want[:2]) + } +} + +func TestDeckLeavesOutDinnersNoLongerSuggested(t *testing.T) { + e := newEnv(t) + ids := e.seedDeck(t, "Pho", "Ramen") + sug, err := e.svc.Deck(t.Context()) + if err != nil { + t.Fatal(err) + } + if _, err := db.New(e.st.Writer).ArchiveDinner(t.Context(), db.ArchiveDinnerParams{ID: sug[0].DinnerID, UpdatedAt: store.Now().String()}); err != nil { + t.Fatal(err) + } + if got := e.deckNames(t); !slices.Equal(got, []string{"Ramen"}) { + t.Errorf("deck = %v, want [Ramen]", got) + } + if _, err := e.svc.Accept(t.Context(), ids[0], member); !errors.Is(err, suggest.ErrDinnerNotSuggested) { + t.Errorf("accept a suggestion whose dinner was archived: %v, want ErrDinnerNotSuggested", err) + } +} + +func TestAcceptIsIdempotent(t *testing.T) { + e := newEnv(t) + ids := e.seedDeck(t, "Pho", "Ramen", "Laksa", "Bibimbap", "Katsu", "Tonkatsu") + + first, err := e.svc.Accept(t.Context(), ids[0], member) + if err != nil { + t.Fatal(err) + } + s := first.Suggestion + if s.Status != suggest.StatusAccepted || s.DinnerStatus != "quest_log" || s.DecidedAt == nil { + t.Fatalf("accepted = %+v, want accepted with its dinner in the Quest log", s) + } + row, err := db.New(e.st.Reader).GetDinner(t.Context(), s.DinnerID) + if err != nil { + t.Fatal(err) + } + if row.Status != "quest_log" || row.UpdatedBy.String != member { + t.Errorf("dinner status %s, updated_by %q; want quest_log by %s", row.Status, row.UpdatedBy.String, member) + } + + again, err := e.svc.Accept(t.Context(), ids[0], "member-2") + if err != nil { + t.Fatal(err) + } + if *again.Suggestion.DecidedAt != *s.DecidedAt || again.ReplenishJobID != nil { + t.Errorf("repeat accept = %+v, want the first decision unchanged", again) + } + if row, _ := db.New(e.st.Reader).GetDinner(t.Context(), s.DinnerID); row.UpdatedBy.String != member { + t.Errorf("repeat accept recorded updated_by %q", row.UpdatedBy.String) + } + + var decided *suggest.DecidedError + if _, err := e.svc.Decline(t.Context(), ids[0], member); !errors.As(err, &decided) || decided.Status != suggest.StatusAccepted { + t.Fatalf("decline an accepted suggestion: %v, want *DecidedError accepted", err) + } + if got := e.dinnerStatus(t, s.DinnerID); got != "quest_log" { + t.Errorf("dinner status after refused decline = %s, want quest_log", got) + } + if got := e.deckNames(t); slices.Contains(got, "Pho") { + t.Errorf("deck %v still holds the accepted suggestion", got) + } +} + +func TestDeclineIsIdempotent(t *testing.T) { + e := newEnv(t) + ids := e.seedDeck(t, "Pho", "Ramen", "Laksa", "Bibimbap", "Katsu", "Tonkatsu") + + first, err := e.svc.Decline(t.Context(), ids[1], member) + if err != nil { + t.Fatal(err) + } + s := first.Suggestion + if s.Status != suggest.StatusDeclined || s.DinnerStatus != "archived" || s.DecidedAt == nil { + t.Fatalf("declined = %+v, want declined with its dinner archived", s) + } + again, err := e.svc.Decline(t.Context(), ids[1], member) + if err != nil { + t.Fatal(err) + } + if *again.Suggestion.DecidedAt != *s.DecidedAt || again.ReplenishJobID != nil { + t.Errorf("repeat decline = %+v, want the first decision unchanged", again) + } + var decided *suggest.DecidedError + if _, err := e.svc.Accept(t.Context(), ids[1], member); !errors.As(err, &decided) || decided.Status != suggest.StatusDeclined { + t.Fatalf("accept a declined suggestion: %v, want *DecidedError declined", err) + } + if got := e.dinnerStatus(t, s.DinnerID); got != "archived" { + t.Errorf("dinner status after refused accept = %s, want archived", got) + } + names, err := db.New(e.st.Reader).ListDeclinedSuggestionNames(t.Context()) + if err != nil { + t.Fatal(err) + } + if !slices.Equal(names, []string{"Ramen"}) { + t.Errorf("declined names = %v, want [Ramen]", names) + } +} + +func TestDecideUnknownSuggestion(t *testing.T) { + e := newEnv(t) + if _, err := e.svc.Accept(t.Context(), "nope", member); !errors.Is(err, suggest.ErrNotFound) { + t.Errorf("accept unknown: %v, want ErrNotFound", err) + } + if _, err := e.svc.Decline(t.Context(), "nope", member); !errors.Is(err, suggest.ErrNotFound) { + t.Errorf("decline unknown: %v, want ErrNotFound", err) + } +} + +func TestReplenishWhenDeckFallsBelowFive(t *testing.T) { + e := newEnv(t) // runner not started: a replenish batch stays pending + ids := e.seedDeck(t, "Pho", "Ramen", "Laksa", "Bibimbap", "Katsu", "Tonkatsu") + + d, err := e.svc.Accept(t.Context(), ids[0], member) // 5 left + if err != nil { + t.Fatal(err) + } + if d.ReplenishJobID != nil || e.activeBatches(t) != 0 { + t.Fatalf("replenished with 5 left: job %v, %d active batches", d.ReplenishJobID, e.activeBatches(t)) + } + + d, err = e.svc.Decline(t.Context(), ids[1], member) // 4 left + if err != nil { + t.Fatal(err) + } + if d.ReplenishJobID == nil { + t.Fatal("no replenish batch with 4 left") + } + 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) + } + var trigger string + if err := e.st.Reader.QueryRowContext(t.Context(), `SELECT trigger FROM suggestion_batches WHERE status = 'pending'`).Scan(&trigger); err != nil { + t.Fatal(err) + } + if trigger != suggest.TriggerReplenish { + t.Errorf("batch trigger = %s, want replenish", trigger) + } +} + +func TestReplenishIsSingleFlight(t *testing.T) { + e := newEnv(t) // runner not started: the replenish batch stays pending + ids := e.seedDeck(t, "Pho", "Ramen", "Laksa") + + first, err := e.svc.Decline(t.Context(), ids[0], member) + if err != nil { + t.Fatal(err) + } + if first.ReplenishJobID == nil { + t.Fatal("no replenish batch with 2 left") + } + second, err := e.svc.Accept(t.Context(), ids[1], member) + if err != nil { + t.Fatal(err) + } + if second.ReplenishJobID != nil { + t.Errorf("second decision enqueued replenish job %s while one is pending", *second.ReplenishJobID) + } + manual, err := e.svc.StartBatch(t.Context(), suggest.TriggerManual) + if err != nil { + t.Fatal(err) + } + if manual.Created || manual.JobID != *first.ReplenishJobID || manual.Trigger != suggest.TriggerReplenish { + t.Errorf("manual batch = %+v, want the pending replenish batch with job %s", manual, *first.ReplenishJobID) + } + if n := e.activeBatches(t); n != 1 { + t.Errorf("%d active batches, want 1", n) + } + var jobsQueued int + if err := e.st.Reader.QueryRowContext(t.Context(), `SELECT COUNT(*) FROM jobs WHERE kind = ?`, suggest.KindSuggestionBatch).Scan(&jobsQueued); err != nil { + t.Fatal(err) + } + if jobsQueued != 1 { + t.Errorf("%d batch jobs, want 1", jobsQueued) + } +} + +func TestStartBatchIsSingleFlightUnderConcurrency(t *testing.T) { + e := newEnv(t) + var wg sync.WaitGroup + batches := make([]suggest.Batch, 8) + for i := range batches { + wg.Add(1) + go func() { + defer wg.Done() + b, err := e.svc.StartBatch(context.Background(), suggest.TriggerManual) + if err != nil { + t.Error(err) + } + batches[i] = b + }() + } + wg.Wait() + created := 0 + for _, b := range batches { + if b.Created { + created++ + } + if b.ID != batches[0].ID || b.JobID != batches[0].JobID || b.JobID == "" { + t.Errorf("batch %+v, want %s with job %s", b, batches[0].ID, batches[0].JobID) + } + } + if created != 1 { + t.Errorf("%d calls created a batch, want 1", created) + } +} + +func TestLaterBatchNeverResuggestsDeclined(t *testing.T) { + e := newEnv(t) + e.start(t) + shawarma := sideQuest("Chicken Shawarma Bowls", "Spiced chicken, like your Chicken Fajitas.", "Chicken Fajitas") + e.recipe(t, "Chicken Shawarma Bowls") + e.reply(shawarma) + res := e.runBatch(t) + + // Passing it leaves the deck empty, so a replenish batch runs; the + // model suggests the passed dish again, respelled, with a new one. + 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"), + ) + d, err := e.svc.Decline(t.Context(), res.SuggestionIDs[0], member) + if err != nil { + t.Fatal(err) + } + if d.ReplenishJobID == nil { + t.Fatal("passing the last Side quest started no replenish batch") + } + job := e.waitJob(t, *d.ReplenishJobID) + if job.Status != jobs.StatusSucceeded { + t.Fatalf("replenish job %s: %v", job.Status, *job.Error) + } + var replenished batchResult + if err := json.Unmarshal(job.Result, &replenished); err != nil { + t.Fatal(err) + } + if len(replenished.SuggestionIDs) != 1 || replenished.Dropped != 1 { + t.Errorf("replenish result = %+v, want 1 kept and the passed dish dropped", replenished) + } + 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[1] + 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 passed dish in both", in.Declined, in.Context.Passed) + } +} + +func TestBatchFailsWhenAIFails(t *testing.T) { + e := newEnv(t) + e.start(t) + e.side.QueueGenerate(llm.GenerateResult{Err: errors.New("provider down")}) + + b, err := e.svc.StartBatch(t.Context(), suggest.TriggerManual) + if err != nil { + t.Fatal(err) + } + job := e.waitJob(t, b.JobID) + if job.Status != jobs.StatusFailed || *job.Error != suggest.ReasonAIFailed { + t.Fatalf("job = %s %v, want failed %s", job.Status, job.Error, suggest.ReasonAIFailed) + } + if got := e.batchStatus(t, b.ID); got != suggest.BatchFailed { + t.Errorf("batch status = %s, want failed", got) + } + if got := e.deckNames(t); len(got) != 0 { + t.Errorf("deck = %v after a failed batch, want empty", got) + } + // A failed batch does not block the next one. + next, err := e.svc.StartBatch(t.Context(), suggest.TriggerManual) + if err != nil { + t.Fatal(err) + } + if !next.Created || next.ID == b.ID { + t.Errorf("next batch = %+v, want a new one", next) + } +} + +func TestBatchFailsWhenSearchFails(t *testing.T) { + e := newEnv(t) + e.start(t) + e.search.Err = errors.New("search down") + e.reply(sideQuest("Chili Verde", "Your Beef Chili gone green.", "Beef Chili")) + + b, err := e.svc.StartBatch(t.Context(), suggest.TriggerManual) + if err != nil { + t.Fatal(err) + } + job := e.waitJob(t, b.JobID) + if job.Status != jobs.StatusFailed || *job.Error != suggest.ReasonSearchFailed { + t.Fatalf("job = %s %v, want failed %s", job.Status, job.Error, suggest.ReasonSearchFailed) + } + if got := e.deckNames(t); len(got) != 0 { + t.Errorf("deck = %v, want empty", got) + } +} + +func TestBatchFailsWithNoRegulars(t *testing.T) { + e := newEnv(t) + e.start(t) + for _, id := range e.regulars { + if _, err := db.New(e.st.Writer).ArchiveDinner(t.Context(), db.ArchiveDinnerParams{ID: id, UpdatedAt: store.Now().String()}); err != nil { + t.Fatal(err) + } + } + b, err := e.svc.StartBatch(t.Context(), suggest.TriggerManual) + if err != nil { + t.Fatal(err) + } + job := e.waitJob(t, b.JobID) + if job.Status != jobs.StatusFailed || *job.Error != suggest.ReasonNoRegulars { + t.Fatalf("job = %s %v, want failed %s", job.Status, job.Error, suggest.ReasonNoRegulars) + } + if reqs := e.side.Requests(); len(reqs) != 0 { + t.Errorf("model called %d times with no Regulars", len(reqs)) + } +} + +func TestFailInterruptedBatches(t *testing.T) { + e := newEnv(t) + queued, err := e.svc.StartBatch(t.Context(), suggest.TriggerManual) // its job is still queued + if err != nil { + t.Fatal(err) + } + now := store.Now().String() + orphan, err := db.New(e.st.Writer).CreateSuggestionBatch(t.Context(), db.CreateSuggestionBatchParams{ID: store.NewID(), Trigger: "manual", CreatedAt: now, UpdatedAt: now}) + if err != nil { + t.Fatal(err) + } + n, err := e.svc.FailInterruptedBatches(t.Context()) + if err != nil { + t.Fatal(err) + } + if n != 1 || e.batchStatus(t, orphan.ID) != suggest.BatchFailed || e.batchStatus(t, queued.ID) != suggest.BatchPending { + t.Errorf("failed %d; orphan %s, queued %s; want only the orphan failed", n, e.batchStatus(t, orphan.ID), e.batchStatus(t, queued.ID)) + } +} + +func TestStartBatchRefusesUnknownTrigger(t *testing.T) { + e := newEnv(t) + if _, err := e.svc.StartBatch(t.Context(), "whenever"); err == nil { + t.Fatal("unknown trigger accepted") + } +} diff --git a/migrations/queries/suggestions.sql b/migrations/queries/suggestions.sql new file mode 100644 index 0000000..b493575 --- /dev/null +++ b/migrations/queries/suggestions.sql @@ -0,0 +1,102 @@ +-- name: CreateSuggestionBatch :one +INSERT INTO suggestion_batches (id, trigger, status, created_at, updated_at) +VALUES (?, ?, 'pending', ?, ?) +RETURNING *; + +-- name: GetActiveSuggestionBatch :one +-- The batch still pending or running, oldest first. Returns sql.ErrNoRows +-- when there is none. +SELECT * FROM suggestion_batches +WHERE status IN ('pending', 'running') +ORDER BY id +LIMIT 1; + +-- name: GetSuggestionBatchJobID :one +-- The id of the job that runs batch batch_id. +SELECT id FROM jobs +WHERE kind = sqlc.arg(kind) + AND json_extract(payload, '$.batch_id') = sqlc.arg(batch_id) +ORDER BY id +LIMIT 1; + +-- name: StartSuggestionBatch :execrows +-- Marks a pending batch running; a batch in any other status is left as +-- it is (0 rows). +UPDATE suggestion_batches +SET status = 'running', updated_at = ? +WHERE id = ? AND status = 'pending'; + +-- name: FinishSuggestionBatch :execrows +-- Records a pending or running batch as done or failed; a batch already +-- finished is left as it is (0 rows). +UPDATE suggestion_batches +SET status = ?, updated_at = ? +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. +UPDATE suggestion_batches +SET status = 'failed', updated_at = sqlc.arg(updated_at) +WHERE status IN ('pending', 'running') + AND NOT EXISTS ( + SELECT 1 FROM jobs + WHERE jobs.kind = sqlc.arg(kind) + AND jobs.status IN ('queued', 'running') + AND json_extract(jobs.payload, '$.batch_id') = suggestion_batches.id + ); + +-- name: CreateSuggestion :exec +INSERT INTO suggestions ( + id, dinner_id, batch_id, reason, cited_regular_ids, status, created_at, updated_at +) VALUES ( + ?, ?, ?, ?, ?, 'pending', ?, ? +); + +-- name: GetSuggestion :one +-- A suggestion with its dinner. Returns sql.ErrNoRows for an unknown id. +SELECT sqlc.embed(suggestions), sqlc.embed(dinners) +FROM suggestions +JOIN dinners ON dinners.id = suggestions.dinner_id +WHERE suggestions.id = ?; + +-- name: ListDeck :many +-- The pending suggestions whose dinner is still suggested, oldest first, +-- with their dinners. +SELECT sqlc.embed(suggestions), sqlc.embed(dinners) +FROM suggestions +JOIN dinners ON dinners.id = suggestions.dinner_id +WHERE suggestions.status = 'pending' AND dinners.status = 'suggested' +ORDER BY suggestions.created_at, suggestions.id; + +-- name: CountDeck :one +-- How many suggestions ListDeck returns. +SELECT COUNT(*) +FROM suggestions +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 +FROM suggestions +JOIN dinners ON dinners.id = suggestions.dinner_id +WHERE suggestions.status = 'declined' +ORDER BY suggestions.decided_at, suggestions.id; + +-- name: DecideSuggestion :execrows +-- Records the household's decision on a pending suggestion; one already +-- decided is left as it is (0 rows). +UPDATE suggestions +SET status = sqlc.arg(status), decided_at = sqlc.arg(now), updated_at = sqlc.arg(now) +WHERE id = sqlc.arg(id) AND status = 'pending'; + +-- name: MoveSuggestedDinner :execrows +-- Moves a dinner still suggested to status (quest_log or archived), +-- recording updated_by; a dinner no longer suggested is left as it is +-- (0 rows). +UPDATE dinners +SET status = sqlc.arg(status), updated_by = sqlc.arg(updated_by), updated_at = sqlc.arg(updated_at) +WHERE id = sqlc.arg(id) AND status = 'suggested'; -- 2.55.0 From 4e2773703bc645436c2b5b98896440cc619990eb Mon Sep 17 00:00:00 2001 From: dev_agent Date: Tue, 6 Oct 2026 23:47:45 -0400 Subject: [PATCH 2/6] 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 -- 2.55.0 From a6643aaa941151a3d382bfde05f6554ccb92eef5 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Tue, 6 Oct 2026 23:59:40 -0400 Subject: [PATCH 3/6] Fail the batch job when recording its end fails, and reconcile orphans finishBatch now returns its error, and treats a batch that is no longer running as one, so a batch job never ends succeeded with its batch still running. A batch left pending or running with its job ended (process stopped, or the final status write failed) is failed inside startBatch, before the active batch is read, so it never blocks a manual or replenish batch. This replaces the boot-only FailInterruptedBatches. Document suggestions.name in the architecture schema summary. Co-Authored-By: Claude Opus 5.5 --- cmd/server/main.go | 5 --- docs/ARCHITECTURE.md | 2 +- internal/db/querier.go | 5 ++- internal/db/suggestions.sql.go | 5 ++- internal/suggest/batch.go | 13 ++++--- internal/suggest/suggest.go | 48 +++++++++++++------------ internal/suggest/suggest_test.go | 56 +++++++++++++++++++++++++----- migrations/queries/suggestions.sql | 5 ++- 8 files changed, 88 insertions(+), 51 deletions(-) diff --git a/cmd/server/main.go b/cmd/server/main.go index 3eed6b6..ca0834e 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -150,11 +150,6 @@ func run(cfg config.Config, logger *slog.Logger) error { if _, err := dinnerSvc.FailInterruptedGroundings(ctx); err != nil { return err } - // Likewise a suggestion batch with no job left to run it, so it does - // not block every later batch. - if _, err := suggestSvc.FailInterruptedBatches(ctx); err != nil { - return err - } srv := &http.Server{ Addr: cfg.HTTPAddr, Handler: router, diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index e9605c3..36e46b1 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -98,7 +98,7 @@ Columns ending in `_ct` hold AES-256-GCM ciphertext (via `secretbox`). | `photos` | source_url, local_path, width, height, sha256, fetched_at. | | `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 when a pending dinner is renamed, so only the latest `ground_dinner` job may write it), updated_by. | | `suggestion_batches` | trigger, status, ai_run_id. | -| `suggestions` | dinner_id, batch_id, reason, cited_regular_ids (JSON), status, decided_at. | +| `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. | | `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. | diff --git a/internal/db/querier.go b/internal/db/querier.go index 64ecb77..b6094eb 100644 --- a/internal/db/querier.go +++ b/internal/db/querier.go @@ -52,9 +52,8 @@ 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 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, or recording the batch's end failed, + // before its handler recorded a result. Run before reading the active batch. 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 diff --git a/internal/db/suggestions.sql.go b/internal/db/suggestions.sql.go index 98fc20d..4106a2a 100644 --- a/internal/db/suggestions.sql.go +++ b/internal/db/suggestions.sql.go @@ -132,9 +132,8 @@ type FailOrphanedSuggestionBatchesParams struct { } // Fails every batch still pending or running whose job is no longer queued -// 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. +// or running: the process stopped, or recording the batch's end failed, +// before its handler recorded a result. Run before reading the active batch. 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 { diff --git a/internal/suggest/batch.go b/internal/suggest/batch.go index d3117c8..0492440 100644 --- a/internal/suggest/batch.go +++ b/internal/suggest/batch.go @@ -50,7 +50,8 @@ func (r batchResult) PublicJSON() ([]byte, error) { return json.Marshal(r) } // batch, as does suggest_side_quests failing, with no Side quest made up // instead. Each round's grounded Side quests are stored before the next // round starts, so the deck fills as the batch goes. The batch ends done -// or failed; Side quests stored before a failure stay in the deck. +// or failed; Side quests stored before a failure stay in the deck. When +// recording the batch's end fails, so does the job. func (s *Service) runBatch(ctx context.Context, p BatchJob, publish func(batchResult) error) (batchResult, error) { n, err := db.New(s.st.Writer).StartSuggestionBatch(ctx, db.StartSuggestionBatchParams{ UpdatedAt: store.Now().String(), @@ -65,12 +66,14 @@ func (s *Service) runBatch(ctx context.Context, p BatchJob, publish func(batchRe res, err := s.fill(ctx, p.BatchID, publish) write := context.WithoutCancel(ctx) if err == nil { - s.finishBatch(write, p.BatchID, BatchDone) + if err := s.finishBatch(write, p.BatchID, BatchDone); err != nil { + return batchResult{}, err + } return res, nil } - s.finishBatch(write, p.BatchID, BatchFailed) + finishErr := s.finishBatch(write, p.BatchID, BatchFailed) if ctx.Err() != nil { - return batchResult{}, ctx.Err() + return batchResult{}, errors.Join(ctx.Err(), finishErr) } var failure *jobs.Failure if errors.As(err, &failure) { @@ -78,7 +81,7 @@ func (s *Service) runBatch(ctx context.Context, p BatchJob, publish func(batchRe // search provider's response. s.logger.InfoContext(ctx, "suggestion batch failed", "batch_id", p.BatchID, "reason", failure.Code) } - return batchResult{}, err + return batchResult{}, errors.Join(err, finishErr) } // fill does runBatch's work, returning a *jobs.Failure for a failure the diff --git a/internal/suggest/suggest.go b/internal/suggest/suggest.go index 9ffda32..0b00713 100644 --- a/internal/suggest/suggest.go +++ b/internal/suggest/suggest.go @@ -172,10 +172,23 @@ func (s *Service) StartBatch(ctx context.Context, trigger string) (Batch, error) } // 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 +// it are committed together, so a batch is never seen without its job. A +// batch still pending or running whose job is no longer queued or running +// is failed first, so a batch whose job ended without recording its end +// (the process stopped, or the write failed) never blocks a later one. 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) + n, err := q.FailOrphanedSuggestionBatches(ctx, db.FailOrphanedSuggestionBatchesParams{ + UpdatedAt: store.Now().String(), + Kind: KindSuggestionBatch, + }) + if err != nil { + return Batch{}, fmt.Errorf("suggest: fail orphaned batches: %w", err) + } + if n > 0 { + s.logger.WarnContext(ctx, "failed suggestion batches whose job ended", "count", n) + } cur, err := q.GetActiveSuggestionBatch(ctx) if err == nil { jobID, err := q.GetSuggestionBatchJobID(ctx, db.GetSuggestionBatchJobIDParams{Kind: KindSuggestionBatch, BatchID: cur.ID}) @@ -204,34 +217,23 @@ func (s *Service) startBatch(ctx context.Context, tx *sql.Tx, trigger string) (B 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{ +// finishBatch records running batch id as status. Its job's handler +// returns any error, so the job never ends succeeded with the batch still +// running; the runner keeps only a code, so the error is logged too. +func (s *Service) finishBatch(ctx context.Context, id, status string) error { + n, err := db.New(s.st.Writer).FinishSuggestionBatch(ctx, db.FinishSuggestionBatchParams{ Status: status, UpdatedAt: store.Now().String(), ID: id, - }); err != nil { - s.logger.ErrorContext(ctx, "record suggestion batch status", "batch_id", id, "status", status, "error", err) - } -} - -// FailInterruptedBatches fails every batch still pending or running whose -// KindSuggestionBatch job is no longer queued or running, so a batch a -// previous process left unfinished does not block every later one. Call it -// once at boot, after the job runner's Start has failed the jobs that -// process left running. It returns how many it failed. -func (s *Service) FailInterruptedBatches(ctx context.Context) (int64, error) { - n, err := db.New(s.st.Writer).FailOrphanedSuggestionBatches(ctx, db.FailOrphanedSuggestionBatchesParams{ - UpdatedAt: store.Now().String(), - Kind: KindSuggestionBatch, }) + if err == nil && n != 1 { + err = errors.New("batch is not running") + } if err != nil { - return 0, fmt.Errorf("suggest: fail interrupted batches: %w", err) + s.logger.ErrorContext(ctx, "record suggestion batch status", "batch_id", id, "status", status, "error", err) + return fmt.Errorf("suggest: record batch %s %s: %w", id, status, err) } - if n > 0 { - s.logger.WarnContext(ctx, "failed suggestion batches interrupted by restart", "count", n) - } - return n, nil + return nil } // Suggestion is a Side quest suggestion as the API shows it, with its diff --git a/internal/suggest/suggest_test.go b/internal/suggest/suggest_test.go index 313920d..43eef70 100644 --- a/internal/suggest/suggest_test.go +++ b/internal/suggest/suggest_test.go @@ -891,23 +891,63 @@ func TestBatchFailsWithNoRegulars(t *testing.T) { } } -func TestFailInterruptedBatches(t *testing.T) { +func TestStartBatchFailsOrphanedBatch(t *testing.T) { e := newEnv(t) - queued, err := e.svc.StartBatch(t.Context(), suggest.TriggerManual) // its job is still queued - if err != nil { - t.Fatal(err) - } now := store.Now().String() orphan, err := db.New(e.st.Writer).CreateSuggestionBatch(t.Context(), db.CreateSuggestionBatchParams{ID: store.NewID(), Trigger: "manual", CreatedAt: now, UpdatedAt: now}) if err != nil { t.Fatal(err) } - n, err := e.svc.FailInterruptedBatches(t.Context()) + b, err := e.svc.StartBatch(t.Context(), suggest.TriggerManual) if err != nil { t.Fatal(err) } - if n != 1 || e.batchStatus(t, orphan.ID) != suggest.BatchFailed || e.batchStatus(t, queued.ID) != suggest.BatchPending { - t.Errorf("failed %d; orphan %s, queued %s; want only the orphan failed", n, e.batchStatus(t, orphan.ID), e.batchStatus(t, queued.ID)) + if !b.Created || b.ID == orphan.ID { + t.Fatalf("batch = %+v, want a new one past the orphan", b) + } + if got := e.batchStatus(t, orphan.ID); got != suggest.BatchFailed { + t.Errorf("orphan status = %s, want failed", got) + } + // A batch whose job is still queued is not an orphan. + again, err := e.svc.StartBatch(t.Context(), suggest.TriggerManual) + if err != nil { + t.Fatal(err) + } + if again.Created || again.ID != b.ID || e.batchStatus(t, b.ID) != suggest.BatchPending { + t.Errorf("second batch = %+v, status %s; want batch %s still pending", again, e.batchStatus(t, b.ID), b.ID) + } +} + +func TestBatchJobFailsWhenRecordingItsEndFails(t *testing.T) { + e := newEnv(t) + e.start(t) + e.recipe(t, "Chili Verde") + e.reply(sideQuest("Chili Verde", "Your Beef Chili gone green.", "Beef Chili")) + if _, err := e.st.Writer.ExecContext(t.Context(), `CREATE TRIGGER refuse_done BEFORE UPDATE OF status ON suggestion_batches WHEN NEW.status = 'done' BEGIN SELECT RAISE(ABORT, 'done refused'); END`); err != nil { + t.Fatal(err) + } + + b, err := e.svc.StartBatch(t.Context(), suggest.TriggerManual) + if err != nil { + t.Fatal(err) + } + job := e.waitJob(t, b.JobID) + if job.Status != jobs.StatusFailed { + t.Fatalf("job %s with its batch %s, want failed", job.Status, e.batchStatus(t, b.ID)) + } + + if _, err := e.st.Writer.ExecContext(t.Context(), `DROP TRIGGER refuse_done`); err != nil { + t.Fatal(err) + } + next, err := e.svc.StartBatch(t.Context(), suggest.TriggerManual) + if err != nil { + t.Fatal(err) + } + if !next.Created || next.ID == b.ID { + t.Errorf("next batch = %+v, want a new one", next) + } + if got := e.batchStatus(t, b.ID); got != suggest.BatchFailed { + t.Errorf("batch status = %s with its job failed, want failed", got) } } diff --git a/migrations/queries/suggestions.sql b/migrations/queries/suggestions.sql index c9392e9..3a42470 100644 --- a/migrations/queries/suggestions.sql +++ b/migrations/queries/suggestions.sql @@ -35,9 +35,8 @@ 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 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, or recording the batch's end failed, +-- before its handler recorded a result. Run before reading the active batch. UPDATE suggestion_batches SET status = 'failed', updated_at = sqlc.arg(updated_at) WHERE status IN ('pending', 'running') -- 2.55.0 From d5cd47a775f1fda95a86b240c7ee8453d9a3844d Mon Sep 17 00:00:00 2001 From: dev_agent Date: Wed, 7 Oct 2026 00:12:52 -0400 Subject: [PATCH 4/6] Retry a job's final status write until it lands A worker whose final SucceedJob or FailJob write failed logged it and moved on, leaving the job running. For a suggestion batch whose own end write had also failed, both rows stayed running, so orphan reconciliation kept treating the batch as active and it blocked manual and replenish batches until a restart. The runner now retries the final write with the claim backoff until it lands; on shutdown a failed attempt is left for the next Start to fail as interrupted. Document that orphaned batches are reconciled each time a batch is started, before the active batch is selected, not at boot. Co-Authored-By: Claude Opus 5.5 --- docs/ARCHITECTURE.md | 11 ++-- internal/jobs/jobs.go | 87 ++++++++++++++++++++------------ internal/jobs/jobs_test.go | 33 ++++++++++++ internal/suggest/suggest_test.go | 80 ++++++++++++++++++++++++++++- 4 files changed, 174 insertions(+), 37 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 36e46b1..2e54f05 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -283,10 +283,13 @@ automatic** (D-021). `grounding`, `done`). It fails with `no_regulars`, `ai_failed` (the skill or `normalize_recipe` failed; no Side quest is made up instead, D-017), `search_not_configured` or `search_failed`, and the batch is set - `failed`; Side quests stored before the failure stay in the deck. At - boot, after the job runner fails the jobs a previous process left - running, every batch still `pending` or `running` with no job queued or - running is set `failed`, so it does not block the next. + `failed`; Side quests stored before the failure stay in the deck. If + recording the batch's end fails, its job fails; the job runner retries + a job's final status write with backoff until it lands. Each time a + batch is started, before the active batch is selected, every batch + still `pending` or `running` with no job queued or running (the process + stopped, or recording its end failed) is set `failed`, so it does not + block the next. `GET /api/v1/suggestions/deck` returns `{suggestions: [{id, dinner_id, name, photo_url, difficulty, method, minutes_total, cuisine, reason, cited_regular_ids, status, dinner_status, decided_at, created_at}]}`: diff --git a/internal/jobs/jobs.go b/internal/jobs/jobs.go index 2d2c865..b3eb2a6 100644 --- a/internal/jobs/jobs.go +++ b/internal/jobs/jobs.go @@ -54,12 +54,13 @@ const ( CodeUnknownKind = "unknown_kind" ) -// Claim retry backoff. A worker whose claim fails with a database error -// retries after claimRetryMin, doubling up to claimRetryMax, so a job queued -// when the error clears is claimed without waiting for a wake token. +// Retry backoff. A worker whose claim or final job write fails with a +// database error retries after retryMin, doubling up to retryMax, so a job +// queued when the error clears is claimed without waiting for a wake token, +// and a job the worker has finished is never left running. const ( - claimRetryMin = 50 * time.Millisecond - claimRetryMax = 5 * time.Second + retryMin = 50 * time.Millisecond + retryMax = 5 * time.Second ) // codePattern is the form of every failure code: lower-case snake case. @@ -387,16 +388,16 @@ func (r *Runner) Get(ctx context.Context, id string) (Job, error) { // already queued is not stranded by a transient error. func (r *Runner) work(ctx context.Context) { defer r.wg.Done() - backoff := claimRetryMin + backoff := retryMin for ctx.Err() == nil { job, err := r.claim(ctx, sql.NullString{String: store.Now().String(), Valid: true}) switch { case err == nil: - backoff = claimRetryMin + backoff = retryMin r.run(ctx, job) continue case errors.Is(err, sql.ErrNoRows): - backoff = claimRetryMin + backoff = retryMin select { case <-r.wake: case <-ctx.Done(): @@ -412,7 +413,7 @@ func (r *Runner) work(ctx context.Context) { case <-ctx.Done(): timer.Stop() } - backoff = min(2*backoff, claimRetryMax) + backoff = min(2*backoff, retryMax) } } @@ -420,14 +421,13 @@ func (r *Runner) work(ctx context.Context) { // ctx has been cancelled, so a handler that finishes during shutdown is // recorded. func (r *Runner) run(ctx context.Context, job db.Job) { - write := context.WithoutCancel(ctx) logger := r.logger.With("job_id", job.ID, "kind", job.Kind) q := db.New(r.st.Writer) h, ok := r.handlers[job.Kind] // read-only once started if !ok { logger.ErrorContext(ctx, "job failed", "code", CodeUnknownKind, "reason", "no handler registered for the kind") - r.fail(write, logger, job.ID, CodeUnknownKind) + r.fail(ctx, logger, job.ID, CodeUnknownKind) return } @@ -449,7 +449,7 @@ func (r *Runner) run(ctx context.Context, job db.Job) { result, code, reason := h.run(ctx, job.Payload, publish) if code != "" { if ctx.Err() != nil { - logger.WarnContext(write, "job interrupted by shutdown; it fails at the next start") + logger.WarnContext(context.WithoutCancel(ctx), "job interrupted by shutdown; it fails at the next start") return } level := slog.LevelInfo @@ -457,35 +457,58 @@ func (r *Runner) run(ctx context.Context, job db.Job) { level = slog.LevelError } logger.Log(ctx, level, "job failed", "code", code, "reason", reason) - r.fail(write, logger, job.ID, code) + r.fail(ctx, logger, job.ID, code) return } - n, err := q.SucceedJob(write, db.SucceedJobParams{ - Result: sql.NullString{String: string(result), Valid: true}, - Now: sql.NullString{String: store.Now().String(), Valid: true}, - ID: job.ID, + r.record(ctx, logger, "record job success", func(write context.Context) (int64, error) { + return q.SucceedJob(write, db.SucceedJobParams{ + Result: sql.NullString{String: string(result), Valid: true}, + Now: sql.NullString{String: store.Now().String(), Valid: true}, + ID: job.ID, + }) }) - if err == nil && n != 1 { - err = errors.New("job is not running") - } - if err != nil { - logger.ErrorContext(write, "record job success", "error", err) - } } // fail records the job as failed with code. The record errors it logs come // from the database, never from a handler. func (r *Runner) fail(ctx context.Context, logger *slog.Logger, id, code string) { - n, err := db.New(r.st.Writer).FailJob(ctx, db.FailJobParams{ - Error: sql.NullString{String: code, Valid: true}, - Now: sql.NullString{String: store.Now().String(), Valid: true}, - ID: id, + r.record(ctx, logger, "record job failure", func(write context.Context) (int64, error) { + return db.New(r.st.Writer).FailJob(write, db.FailJobParams{ + Error: sql.NullString{String: code, Valid: true}, + Now: sql.NullString{String: store.Now().String(), Valid: true}, + ID: id, + }) }) - if err == nil && n != 1 { - err = errors.New("job is not running") - } - if err != nil { - logger.ErrorContext(ctx, "record job failure", "error", err) +} + +// record makes a job's final write, which must change the one running job. +// A database error is retried with backoff until the write lands, so a +// worker never moves on from a job it has finished while leaving it +// running. Each attempt is made even when ctx has been cancelled; once it +// is, a failed attempt is not retried and the job stays running, so the +// next Start fails it as interrupted. +func (r *Runner) record(ctx context.Context, logger *slog.Logger, what string, write func(context.Context) (int64, error)) { + backoff := retryMin + for { + n, err := write(context.WithoutCancel(ctx)) + if err == nil { + if n != 1 { + logger.ErrorContext(ctx, what, "error", "job is not running") + } + return + } + if ctx.Err() != nil { + logger.ErrorContext(context.WithoutCancel(ctx), what+" failed during shutdown; it fails at the next start", "error", err) + return + } + logger.ErrorContext(ctx, what+" failed; retrying", "error", err, "retry_in", backoff) + timer := time.NewTimer(backoff) + select { + case <-timer.C: + case <-ctx.Done(): + timer.Stop() + } + backoff = min(2*backoff, retryMax) } } diff --git a/internal/jobs/jobs_test.go b/internal/jobs/jobs_test.go index b1b6752..1727d18 100644 --- a/internal/jobs/jobs_test.go +++ b/internal/jobs/jobs_test.go @@ -609,6 +609,39 @@ func TestClaimErrorIsRetried(t *testing.T) { } } +func TestFinalWriteErrorIsRetried(t *testing.T) { + st := migratedStore(t) + logs := &logBuffer{} + r := newRunnerLogging(t, st, 1, logs) + k := register(t, r, "count", func(ctx context.Context, p countPayload, publish func(countResult) error) (countResult, error) { + return countResult{Done: p.Steps}, nil + }) + if _, err := st.Writer.ExecContext(t.Context(), `CREATE TRIGGER refuse_end BEFORE UPDATE OF status ON jobs WHEN NEW.status IN ('succeeded', 'failed') BEGIN SELECT RAISE(ABORT, 'end refused'); END`); err != nil { + t.Fatal(err) + } + start(t, r) + id := enqueue(t, k, countPayload{Steps: 3}) + + deadline := time.Now().Add(5 * time.Second) + for !strings.Contains(logs.String(), "record job success") { + if time.Now().After(deadline) { + t.Fatal("job success never recorded") + } + time.Sleep(5 * time.Millisecond) + } + if _, err := st.Writer.ExecContext(t.Context(), `DROP TRIGGER refuse_end`); err != nil { + t.Fatal(err) + } + + job := waitFor(t, r, id, finished) + if job.Status != StatusSucceeded || string(job.Result) != `{"done":3}` { + t.Fatalf("job = %+v (result %s), want succeeded with {\"done\":3}", job, job.Result) + } + if out := logs.String(); !strings.Contains(out, "record job success failed; retrying") { + t.Fatalf("final write error not logged as retried: %s", out) + } +} + func TestRunnerEnqueueByKindName(t *testing.T) { t.Parallel() st := migratedStore(t) diff --git a/internal/suggest/suggest_test.go b/internal/suggest/suggest_test.go index 43eef70..7f89c88 100644 --- a/internal/suggest/suggest_test.go +++ b/internal/suggest/suggest_test.go @@ -44,6 +44,9 @@ type env struct { regulars map[string]string grounder *recipes.Grounder + // logs collects the service's and job runner's log output. + logs *logBuffer + mu sync.Mutex inputs []skills.SideQuestsRequest // stub, when set, answers in place of suggest_side_quests. @@ -82,6 +85,7 @@ func newEnv(t *testing.T) *env { model: &llm.Fake{}, side: &llm.Fake{}, regulars: map[string]string{}, + logs: &logBuffer{}, } for _, name := range []string{"Beef Chili", "Chicken Fajitas"} { e.regulars[name] = e.dinner(t, name, "regular") @@ -108,7 +112,7 @@ func newEnv(t *testing.T) *env { } return skills.SuggestSideQuests(ctx, cfg, scope, in) } - logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + logger := slog.New(slog.NewTextHandler(e.logs, nil)) e.runner, err = jobs.NewRunner(st, logger, 1) if err != nil { t.Fatal(err) @@ -120,6 +124,24 @@ func newEnv(t *testing.T) *env { return e } +// logBuffer collects log output written by concurrent goroutines. +type logBuffer struct { + mu sync.Mutex + buf strings.Builder +} + +func (b *logBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.Write(p) +} + +func (b *logBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.String() +} + // start starts the env's job runner, stopping it when the test ends. func (e *env) start(t *testing.T) { t.Helper() @@ -951,6 +973,62 @@ func TestBatchJobFailsWhenRecordingItsEndFails(t *testing.T) { } } +func TestBatchRecoversWhenRecordingItsEndAndItsJobsEndFail(t *testing.T) { + e := newEnv(t) + e.start(t) + e.recipe(t, "Chili Verde") + e.reply(sideQuest("Chili Verde", "Your Beef Chili gone green.", "Beef Chili")) + for _, trigger := range []string{ + `CREATE TRIGGER refuse_done BEFORE UPDATE OF status ON suggestion_batches WHEN NEW.status = 'done' BEGIN SELECT RAISE(ABORT, 'done refused'); END`, + `CREATE TRIGGER refuse_job_end BEFORE UPDATE OF status ON jobs WHEN NEW.status IN ('succeeded', 'failed') BEGIN SELECT RAISE(ABORT, 'job end refused'); END`, + } { + if _, err := e.st.Writer.ExecContext(t.Context(), trigger); err != nil { + t.Fatal(err) + } + } + + b, err := e.svc.StartBatch(t.Context(), suggest.TriggerManual) + if err != nil { + t.Fatal(err) + } + deadline := time.Now().Add(10 * time.Second) + for !strings.Contains(e.logs.String(), "record job failure") { + if time.Now().After(deadline) { + t.Fatalf("job end never recorded; batch %s", e.batchStatus(t, b.ID)) + } + time.Sleep(5 * time.Millisecond) + } + // While neither end is recorded the batch is still the active one. + again, err := e.svc.StartBatch(t.Context(), suggest.TriggerManual) + if err != nil { + t.Fatal(err) + } + if again.Created || again.ID != b.ID { + t.Fatalf("batch = %+v while batch %s is running, want it", again, b.ID) + } + + // The database recovers; the service is not restarted. + for _, trigger := range []string{"refuse_done", "refuse_job_end"} { + if _, err := e.st.Writer.ExecContext(t.Context(), "DROP TRIGGER "+trigger); err != nil { + t.Fatal(err) + } + } + job := e.waitJob(t, b.JobID) + if job.Status != jobs.StatusFailed { + t.Fatalf("job %s, want failed", job.Status) + } + next, err := e.svc.StartBatch(t.Context(), suggest.TriggerManual) + if err != nil { + t.Fatal(err) + } + if !next.Created || next.ID == b.ID { + t.Errorf("next batch = %+v, want a new one", next) + } + if got := e.batchStatus(t, b.ID); got != suggest.BatchFailed { + t.Errorf("batch status = %s with its job failed, want failed", got) + } +} + func TestStartBatchRefusesUnknownTrigger(t *testing.T) { e := newEnv(t) if _, err := e.svc.StartBatch(t.Context(), "whenever"); err == nil { -- 2.55.0 From 76ec60d2198c58b0e7e2295d8f58d39470fd605e Mon Sep 17 00:00:00 2001 From: dev_agent Date: Wed, 7 Oct 2026 00:29:28 -0400 Subject: [PATCH 5/6] Bound each attempt at a job's final write A final write used a context that could never be cancelled, so a worker waiting for a held writer connection kept runner.Wait, and with it shutdown, open indefinitely. Each attempt now has a 5-second deadline; once shutdown begins, a failed or timed-out attempt is not retried and the next start fails the job as interrupted. Co-Authored-By: Claude Opus 5.5 --- docs/ARCHITECTURE.md | 4 ++- internal/jobs/jobs.go | 31 +++++++++++++------- internal/jobs/jobs_test.go | 58 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 11 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 2e54f05..00e82e4 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -285,7 +285,9 @@ automatic** (D-021). D-017), `search_not_configured` or `search_failed`, and the batch is set `failed`; Side quests stored before the failure stay in the deck. If recording the batch's end fails, its job fails; the job runner retries - a job's final status write with backoff until it lands. Each time a + a job's final status write with backoff until it lands, each attempt + bounded at 5 seconds; at shutdown an attempt that fails or times out is + not retried, and the next start fails the job as interrupted. Each time a batch is started, before the active batch is selected, every batch still `pending` or `running` with no job queued or running (the process stopped, or recording its end failed) is set `failed`, so it does not diff --git a/internal/jobs/jobs.go b/internal/jobs/jobs.go index b3eb2a6..db57527 100644 --- a/internal/jobs/jobs.go +++ b/internal/jobs/jobs.go @@ -63,6 +63,11 @@ const ( retryMax = 5 * time.Second ) +// recordTimeout bounds each attempt at a job's final write, so a writer +// connection that stays busy cannot hold a stopping worker, and with it +// shutdown, open. +const recordTimeout = 5 * time.Second + // codePattern is the form of every failure code: lower-case snake case. var codePattern = regexp.MustCompile(`^[a-z][a-z0-9_]{0,63}$`) @@ -132,6 +137,8 @@ type Runner struct { wake chan struct{} // claim claims the oldest queued job, marking it running at now. claim func(ctx context.Context, now sql.NullString) (db.Job, error) + // recordTimeout bounds each attempt at a job's final write. + recordTimeout time.Duration mu sync.Mutex handlers map[string]handler @@ -152,12 +159,13 @@ func NewRunner(st *store.Store, logger *slog.Logger, workers int) (*Runner, erro return nil, fmt.Errorf("jobs: %d workers, want at least 1", workers) } return &Runner{ - st: st, - logger: logger, - workers: workers, - wake: make(chan struct{}, workers), - claim: db.New(st.Writer).ClaimNextJob, - handlers: make(map[string]handler), + st: st, + logger: logger, + workers: workers, + wake: make(chan struct{}, workers), + claim: db.New(st.Writer).ClaimNextJob, + recordTimeout: recordTimeout, + handlers: make(map[string]handler), }, nil } @@ -485,13 +493,16 @@ func (r *Runner) fail(ctx context.Context, logger *slog.Logger, id, code string) // record makes a job's final write, which must change the one running job. // A database error is retried with backoff until the write lands, so a // worker never moves on from a job it has finished while leaving it -// running. Each attempt is made even when ctx has been cancelled; once it -// is, a failed attempt is not retried and the job stays running, so the -// next Start fails it as interrupted. +// running. Each attempt is made even when ctx has been cancelled, and is +// bounded by r.recordTimeout; once ctx is cancelled, a failed or timed-out +// attempt is not retried and the job stays running, so the next Start +// fails it as interrupted. func (r *Runner) record(ctx context.Context, logger *slog.Logger, what string, write func(context.Context) (int64, error)) { backoff := retryMin for { - n, err := write(context.WithoutCancel(ctx)) + attempt, cancel := context.WithTimeout(context.WithoutCancel(ctx), r.recordTimeout) + n, err := write(attempt) + cancel() if err == nil { if n != 1 { logger.ErrorContext(ctx, what, "error", "job is not running") diff --git a/internal/jobs/jobs_test.go b/internal/jobs/jobs_test.go index 1727d18..02e05a3 100644 --- a/internal/jobs/jobs_test.go +++ b/internal/jobs/jobs_test.go @@ -642,6 +642,64 @@ func TestFinalWriteErrorIsRetried(t *testing.T) { } } +func TestBlockedFinalWriteDoesNotHoldShutdown(t *testing.T) { + st := migratedStore(t) + r := newRunner(t, st, 1) + r.recordTimeout = 100 * time.Millisecond + started, finish := make(chan struct{}), make(chan struct{}) + k := register(t, r, "count", func(ctx context.Context, p countPayload, publish func(countResult) error) (countResult, error) { + close(started) + <-finish + return countResult{Done: p.Steps}, nil + }) + stop := start(t, r) + id := enqueue(t, k, countPayload{Steps: 3}) + <-started + + // Hold the only writer connection, so the job's final write waits for it. + conn, err := st.Writer.Conn(t.Context()) + if err != nil { + t.Fatal(err) + } + waits := st.Writer.Stats().WaitCount + close(finish) + deadline := time.Now().Add(5 * time.Second) + for st.Writer.Stats().WaitCount == waits { + if time.Now().After(deadline) { + t.Fatal("final write never waited for the writer") + } + time.Sleep(5 * time.Millisecond) + } + + stopped := make(chan struct{}) + go func() { + stop() + close(stopped) + }() + select { + case <-stopped: + case <-time.After(5 * time.Second): + conn.Close() + <-stopped + t.Fatal("worker waited for a held writer through shutdown") + } + if err := conn.Close(); err != nil { + t.Fatal(err) + } + + if job := waitFor(t, r, id, func(Job) bool { return true }); job.Status != StatusRunning { + t.Fatalf("after shutdown job = %+v, want left running", job) + } + second := newRunner(t, st, 1) + register(t, second, "count", func(context.Context, countPayload, func(countResult) error) (countResult, error) { + return countResult{}, nil + }) + start(t, second) + if job := waitFor(t, second, id, finished); job.Status != StatusFailed || job.Error == nil || *job.Error != CodeInterrupted { + t.Errorf("interrupted job = %+v, want failed with %q", job, CodeInterrupted) + } +} + func TestRunnerEnqueueByKindName(t *testing.T) { t.Parallel() st := migratedStore(t) -- 2.55.0 From f95a31472b8f995a0e2734c33d242b62c12744f5 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Wed, 7 Oct 2026 00:40:52 -0400 Subject: [PATCH 6/6] Bound the suggestion batch's end write like a job's final write runBatch recorded the batch's end with an uncancellable context and no deadline, before its handler returned, so a writer connection that stayed busy could hold the worker, and shutdown, open indefinitely. jobs.FinalWriteContext now gives every final write one bounded attempt context (FinalWriteTimeout, 5 seconds, not cancelled with ctx); the runner's record and finishBatch both use it. On timeout at shutdown the handler returns, the job and batch stay running, the next Start fails the job as interrupted and the next StartBatch fails the orphaned batch. TestBlockedBatchEndDoesNotHoldShutdown holds the writer while a batch reaches finishBatch and asserts the worker stops within the bound and the batch is reconciled after restart. TestBlockedFinalWriteDoesNotHoldShutdown no longer sets a runner field, so it compiles against earlier heads. Co-Authored-By: Claude Opus 5.5 --- docs/ARCHITECTURE.md | 5 +- internal/jobs/jobs.go | 43 +++++++------ internal/jobs/jobs_test.go | 5 +- internal/suggest/batch.go | 9 +-- internal/suggest/suggest.go | 13 ++-- internal/suggest/suggest_test.go | 107 +++++++++++++++++++++++++++++++ 6 files changed, 153 insertions(+), 29 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 00e82e4..3227230 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -287,7 +287,10 @@ automatic** (D-021). recording the batch's end fails, its job fails; the job runner retries a job's final status write with backoff until it lands, each attempt bounded at 5 seconds; at shutdown an attempt that fails or times out is - not retried, and the next start fails the job as interrupted. Each time a + not retried, and the next start fails the job as interrupted. Recording + the batch's end is bounded the same way (one attempt, 5 seconds, made + even during shutdown), so a busy writer cannot hold a stopping worker; + a batch whose end timed out is left `running` with its job. Each time a batch is started, before the active batch is selected, every batch still `pending` or `running` with no job queued or running (the process stopped, or recording its end failed) is set `failed`, so it does not diff --git a/internal/jobs/jobs.go b/internal/jobs/jobs.go index db57527..12cb947 100644 --- a/internal/jobs/jobs.go +++ b/internal/jobs/jobs.go @@ -63,10 +63,20 @@ const ( retryMax = 5 * time.Second ) -// recordTimeout bounds each attempt at a job's final write, so a writer -// connection that stays busy cannot hold a stopping worker, and with it -// shutdown, open. -const recordTimeout = 5 * time.Second +// FinalWriteTimeout bounds each attempt at a job's final write, and at a +// handler's own last write, so a writer connection that stays busy cannot +// hold a stopping worker, and with it shutdown, open. +const FinalWriteTimeout = 5 * time.Second + +// FinalWriteContext returns the context for one attempt at a final write +// of a job: it is not cancelled with ctx, so a job that finishes during +// shutdown is still recorded, but it ends after FinalWriteTimeout. A +// handler makes its own last write, the one recording what the job did, +// with it too, and returns the error when the attempt times out, so its +// worker can stop; the next Start fails the job as interrupted. +func FinalWriteContext(ctx context.Context) (context.Context, context.CancelFunc) { + return context.WithTimeout(context.WithoutCancel(ctx), FinalWriteTimeout) +} // codePattern is the form of every failure code: lower-case snake case. var codePattern = regexp.MustCompile(`^[a-z][a-z0-9_]{0,63}$`) @@ -137,8 +147,6 @@ type Runner struct { wake chan struct{} // claim claims the oldest queued job, marking it running at now. claim func(ctx context.Context, now sql.NullString) (db.Job, error) - // recordTimeout bounds each attempt at a job's final write. - recordTimeout time.Duration mu sync.Mutex handlers map[string]handler @@ -159,13 +167,12 @@ func NewRunner(st *store.Store, logger *slog.Logger, workers int) (*Runner, erro return nil, fmt.Errorf("jobs: %d workers, want at least 1", workers) } return &Runner{ - st: st, - logger: logger, - workers: workers, - wake: make(chan struct{}, workers), - claim: db.New(st.Writer).ClaimNextJob, - recordTimeout: recordTimeout, - handlers: make(map[string]handler), + st: st, + logger: logger, + workers: workers, + wake: make(chan struct{}, workers), + claim: db.New(st.Writer).ClaimNextJob, + handlers: make(map[string]handler), }, nil } @@ -493,14 +500,14 @@ func (r *Runner) fail(ctx context.Context, logger *slog.Logger, id, code string) // record makes a job's final write, which must change the one running job. // A database error is retried with backoff until the write lands, so a // worker never moves on from a job it has finished while leaving it -// running. Each attempt is made even when ctx has been cancelled, and is -// bounded by r.recordTimeout; once ctx is cancelled, a failed or timed-out -// attempt is not retried and the job stays running, so the next Start -// fails it as interrupted. +// running. Each attempt is made with FinalWriteContext, so even when ctx +// has been cancelled, and is bounded by FinalWriteTimeout; once ctx is +// cancelled, a failed or timed-out attempt is not retried and the job +// stays running, so the next Start fails it as interrupted. func (r *Runner) record(ctx context.Context, logger *slog.Logger, what string, write func(context.Context) (int64, error)) { backoff := retryMin for { - attempt, cancel := context.WithTimeout(context.WithoutCancel(ctx), r.recordTimeout) + attempt, cancel := FinalWriteContext(ctx) n, err := write(attempt) cancel() if err == nil { diff --git a/internal/jobs/jobs_test.go b/internal/jobs/jobs_test.go index 02e05a3..ece6b13 100644 --- a/internal/jobs/jobs_test.go +++ b/internal/jobs/jobs_test.go @@ -645,7 +645,6 @@ func TestFinalWriteErrorIsRetried(t *testing.T) { func TestBlockedFinalWriteDoesNotHoldShutdown(t *testing.T) { st := migratedStore(t) r := newRunner(t, st, 1) - r.recordTimeout = 100 * time.Millisecond started, finish := make(chan struct{}), make(chan struct{}) k := register(t, r, "count", func(ctx context.Context, p countPayload, publish func(countResult) error) (countResult, error) { close(started) @@ -676,9 +675,11 @@ func TestBlockedFinalWriteDoesNotHoldShutdown(t *testing.T) { stop() close(stopped) }() + // The final write's attempt ends after 5 seconds; well past that, the + // worker is waiting for the writer with no bound. select { case <-stopped: - case <-time.After(5 * time.Second): + case <-time.After(15 * time.Second): conn.Close() <-stopped t.Fatal("worker waited for a held writer through shutdown") diff --git a/internal/suggest/batch.go b/internal/suggest/batch.go index 0492440..00aeccb 100644 --- a/internal/suggest/batch.go +++ b/internal/suggest/batch.go @@ -51,7 +51,9 @@ func (r batchResult) PublicJSON() ([]byte, error) { return json.Marshal(r) } // instead. Each round's grounded Side quests are stored before the next // round starts, so the deck fills as the batch goes. The batch ends done // or failed; Side quests stored before a failure stay in the deck. When -// recording the batch's end fails, so does the job. +// recording the batch's end fails, so does the job; when it times out at +// shutdown, the job is left running and the batch with it, for the next +// start to fail. func (s *Service) runBatch(ctx context.Context, p BatchJob, publish func(batchResult) error) (batchResult, error) { n, err := db.New(s.st.Writer).StartSuggestionBatch(ctx, db.StartSuggestionBatchParams{ UpdatedAt: store.Now().String(), @@ -64,14 +66,13 @@ func (s *Service) runBatch(ctx context.Context, p BatchJob, publish func(batchRe return batchResult{}, fmt.Errorf("suggest: batch %s is not pending", p.BatchID) } res, err := s.fill(ctx, p.BatchID, publish) - write := context.WithoutCancel(ctx) if err == nil { - if err := s.finishBatch(write, p.BatchID, BatchDone); err != nil { + if err := s.finishBatch(ctx, p.BatchID, BatchDone); err != nil { return batchResult{}, err } return res, nil } - finishErr := s.finishBatch(write, p.BatchID, BatchFailed) + finishErr := s.finishBatch(ctx, p.BatchID, BatchFailed) if ctx.Err() != nil { return batchResult{}, errors.Join(ctx.Err(), finishErr) } diff --git a/internal/suggest/suggest.go b/internal/suggest/suggest.go index 0b00713..efed277 100644 --- a/internal/suggest/suggest.go +++ b/internal/suggest/suggest.go @@ -217,11 +217,16 @@ func (s *Service) startBatch(ctx context.Context, tx *sql.Tx, trigger string) (B return Batch{ID: row.ID, JobID: jobID, Trigger: row.Trigger, Status: row.Status, Created: true}, nil } -// finishBatch records running batch id as status. Its job's handler -// returns any error, so the job never ends succeeded with the batch still -// running; the runner keeps only a code, so the error is logged too. +// finishBatch records running batch id as status, with +// jobs.FinalWriteContext: the write is made even when ctx has been +// cancelled, but a writer that stays busy cannot hold its worker past +// jobs.FinalWriteTimeout. Its job's handler returns any error, so the job +// never ends succeeded with the batch still running; the runner keeps only +// a code, so the error is logged too. func (s *Service) finishBatch(ctx context.Context, id, status string) error { - n, err := db.New(s.st.Writer).FinishSuggestionBatch(ctx, db.FinishSuggestionBatchParams{ + write, cancel := jobs.FinalWriteContext(ctx) + defer cancel() + n, err := db.New(s.st.Writer).FinishSuggestionBatch(write, db.FinishSuggestionBatchParams{ Status: status, UpdatedAt: store.Now().String(), ID: id, diff --git a/internal/suggest/suggest_test.go b/internal/suggest/suggest_test.go index 7f89c88..157f1a5 100644 --- a/internal/suggest/suggest_test.go +++ b/internal/suggest/suggest_test.go @@ -1029,6 +1029,113 @@ func TestBatchRecoversWhenRecordingItsEndAndItsJobsEndFail(t *testing.T) { } } +func TestBlockedBatchEndDoesNotHoldShutdown(t *testing.T) { + e := newEnv(t) + ctx, cancel := context.WithCancel(context.Background()) + if err := e.runner.Start(ctx); err != nil { + cancel() + t.Fatal(err) + } + stopped := make(chan struct{}) + t.Cleanup(func() { + cancel() + <-stopped + }) + e.search.Gate = make(chan struct{}) + e.search.Err = errors.New("search down") + e.reply(sideQuest("Chili Verde", "Your Beef Chili gone green.", "Beef Chili")) + + b, err := e.svc.StartBatch(t.Context(), suggest.TriggerManual) + if err != nil { + t.Fatal(err) + } + deadline := time.Now().Add(10 * time.Second) + for len(e.search.Queries()) == 0 { + if time.Now().After(deadline) { + t.Fatal("batch never searched") + } + time.Sleep(5 * time.Millisecond) + } + + // Hold the only writer connection, so recording the batch's end + // waits for it once the search fails. + conn, err := e.st.Writer.Conn(t.Context()) + if err != nil { + t.Fatal(err) + } + waits := e.st.Writer.Stats().WaitCount + close(e.search.Gate) + for e.st.Writer.Stats().WaitCount == waits { + if time.Now().After(deadline) { + conn.Close() + t.Fatal("batch end never waited for the writer") + } + time.Sleep(5 * time.Millisecond) + } + + go func() { + e.runner.Wait() + close(stopped) + }() + cancel() + // Each final write's attempt ends after jobs.FinalWriteTimeout; well + // past that, the worker is waiting for the writer with no bound. + select { + case <-stopped: + case <-time.After(jobs.FinalWriteTimeout + 10*time.Second): + conn.Close() + <-stopped + t.Fatal("worker waited for a held writer through shutdown") + } + if err := conn.Close(); err != nil { + t.Fatal(err) + } + if got := e.batchStatus(t, b.ID); got != suggest.BatchRunning { + t.Fatalf("after shutdown batch status = %s, want left running", got) + } + + // The next process fails the job as interrupted, and the batch no + // longer blocks the next one. + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + next, err := jobs.NewRunner(e.st, logger, 1) + if err != nil { + t.Fatal(err) + } + refuse := func(context.Context, string, skills.SideQuestsRequest) (skills.SideQuests, error) { + return skills.SideQuests{}, errors.New("not wanted") + } + svc, err := suggest.NewService(e.st, e.grounder, refuse, next, logger) + if err != nil { + t.Fatal(err) + } + nextCtx, nextCancel := context.WithCancel(context.Background()) + if err := next.Start(nextCtx); err != nil { + nextCancel() + t.Fatal(err) + } + t.Cleanup(func() { + nextCancel() + next.Wait() + }) + job, err := next.Get(t.Context(), b.JobID) + if err != nil { + t.Fatal(err) + } + if job.Status != jobs.StatusFailed || job.Error == nil || *job.Error != jobs.CodeInterrupted { + t.Fatalf("interrupted job = %+v, want failed with %q", job, jobs.CodeInterrupted) + } + again, err := svc.StartBatch(t.Context(), suggest.TriggerManual) + if err != nil { + t.Fatal(err) + } + if !again.Created || again.ID == b.ID { + t.Errorf("next batch = %+v, want a new one", again) + } + if got := e.batchStatus(t, b.ID); got != suggest.BatchFailed { + t.Errorf("interrupted batch status = %s, want failed", got) + } +} + func TestStartBatchRefusesUnknownTrigger(t *testing.T) { e := newEnv(t) if _, err := e.svc.StartBatch(t.Context(), "whenever"); err == nil { -- 2.55.0