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 <noreply@anthropic.com>

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

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

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)
 	}
 }