diff --git a/internal/app/warmupcontent/batch.go b/internal/app/warmupcontent/batch.go index 185c2226..e8771b0d 100644 --- a/internal/app/warmupcontent/batch.go +++ b/internal/app/warmupcontent/batch.go @@ -168,29 +168,35 @@ func (s *service) PollBatches(ctx context.Context) error { // pollBatchJob reconciles a single batch job. func (s *service) pollBatchJob(ctx context.Context, job *models.WarmupGenerationJob) error { - status, outputFileID, errorFileID, counts, err := s.gen.GetBatch(ctx, job.BatchID) + state, err := s.gen.GetBatch(ctx, job.BatchID) if err != nil { return err } - job.BatchStatus = status + job.BatchStatus = state.Status - switch status { + switch state.Status { case "completed": - job.BatchOutputFileID = outputFileID + job.BatchOutputFileID = state.OutputFileID // A batch whose requests all failed completes with no output file; the // refusals are in the error file, which has the same JSONL shape. Read // it instead of failing on the empty id, or the reason is never seen. - resultsFileID := outputFileID + resultsFileID := state.OutputFileID if resultsFileID == "" { - resultsFileID = errorFileID + resultsFileID = state.ErrorFileID } - return s.ingestBatch(ctx, job, resultsFileID, counts) + return s.ingestBatch(ctx, job, resultsFileID, state.Counts) case "failed", "expired", "cancelled": now := time.Now() job.Status = "failed" job.FinishedAt = &now if job.Error == "" { - job.Error = fmt.Sprintf("batch %s", status) + // A batch that failed as a whole has no error file, so the provider's + // own message is the only account of why. Recording just the status + // leaves the operator with "batch failed" and nothing to act on. + job.Error = fmt.Sprintf("batch %s", state.Status) + if state.FailureReason != "" { + job.Error += ": " + state.FailureReason + } } return s.repo.UpdateGenerationJob(ctx, job) default: diff --git a/internal/pkg/generation/batch.go b/internal/pkg/generation/batch.go index fcd193c0..8c3c6487 100644 --- a/internal/pkg/generation/batch.go +++ b/internal/pkg/generation/batch.go @@ -112,22 +112,57 @@ func (c *GenerationClient) SubmitBatch(ctx context.Context, requests []BatchRequ return batch.ID, file.ID, nil } -// GetBatch returns the current status, the output file ID (present when at -// least one request succeeded), the error file ID (present when at least one -// failed), and the request counts for a batch. A batch whose requests all -// failed still completes, with output empty and every refusal in the error -// file, so callers must read that one to learn why. -func (c *GenerationClient) GetBatch(ctx context.Context, batchID string) (status, outputFileID, errorFileID string, counts BatchCounts, err error) { +// BatchState is the provider's view of one batch. Grouped rather than returned +// as a widening list of strings, because the two failure surfaces are different +// and a caller has to tell them apart. +type BatchState struct { + Status string + // OutputFileID is present when at least one request succeeded, ErrorFileID + // when at least one failed. A batch whose requests ALL failed still + // completes, with output empty and every refusal in the error file. + OutputFileID string + ErrorFileID string + Counts BatchCounts + // FailureReason is the batch-level error, reported when the batch itself + // never ran: a rejected input file, a quota refusal. It is not a per-request + // refusal, and it arrives with no error file to read it out of, so it is the + // only place that says why. + FailureReason string +} + +// GetBatch returns the provider's current view of a batch. +func (c *GenerationClient) GetBatch(ctx context.Context, batchID string) (BatchState, error) { batch, err := c.client.Batches.Get(ctx, batchID) if err != nil { - return "", "", "", BatchCounts{}, err + return BatchState{}, err } - counts = BatchCounts{ - Completed: int(batch.RequestCounts.Completed), - Failed: int(batch.RequestCounts.Failed), - Total: int(batch.RequestCounts.Total), + return BatchState{ + Status: string(batch.Status), + OutputFileID: batch.OutputFileID, + ErrorFileID: batch.ErrorFileID, + Counts: BatchCounts{ + Completed: int(batch.RequestCounts.Completed), + Failed: int(batch.RequestCounts.Failed), + Total: int(batch.RequestCounts.Total), + }, + FailureReason: batchFailureReason(batch.Errors.Data), + }, nil +} + +// batchFailureReason renders the batch-level errors as one line, naming how many +// more there are rather than pasting all of them into a job row. +func batchFailureReason(errs []openai.BatchError) string { + for _, e := range errs { + msg := strings.TrimSpace(e.Message) + if msg == "" { + continue + } + if len(errs) > 1 { + return fmt.Sprintf("%s (+%d more)", msg, len(errs)-1) + } + return msg } - return string(batch.Status), batch.OutputFileID, batch.ErrorFileID, counts, nil + return "" } // CancelBatch requests cancellation of an in-flight batch. diff --git a/internal/pkg/generation/batch_test.go b/internal/pkg/generation/batch_test.go new file mode 100644 index 00000000..23e1f616 --- /dev/null +++ b/internal/pkg/generation/batch_test.go @@ -0,0 +1,48 @@ +package generation + +import ( + "testing" + + "github.com/openai/openai-go/v2" +) + +// A batch that fails as a whole produces no error file, so the message the +// provider puts on the batch itself is the only account of why. Losing it is +// how "batch failed" ended up being everything an operator was told. +func TestBatchFailureReason(t *testing.T) { + cases := []struct { + name string + errs []openai.BatchError + want string + }{ + {"none", nil, ""}, + { + "single", + []openai.BatchError{{Code: "invalid_request", Message: "Cannot find file file-abc, or organization org-xyz does not have access to it."}}, + "Cannot find file file-abc, or organization org-xyz does not have access to it.", + }, + { + "counts the rest", + []openai.BatchError{ + {Message: "first reason"}, + {Message: "second reason"}, + {Message: "third reason"}, + }, + "first reason (+2 more)", + }, + { + "skips a blank message", + []openai.BatchError{{Code: "empty"}, {Message: " the real one "}}, + "the real one (+1 more)", + }, + {"all blank", []openai.BatchError{{Code: "a"}, {Code: "b"}}, ""}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := batchFailureReason(tc.errs); got != tc.want { + t.Errorf("batchFailureReason() = %q, want %q", got, tc.want) + } + }) + } +}