From 0e4a696c2f31abb00a05af659d4969bc0e18216c Mon Sep 17 00:00:00 2001 From: joao-crm Date: Sat, 29 Aug 2026 01:12:10 +0000 Subject: [PATCH] feat: record the provider's own reason when a warmup generation batch fails as a whole, instead of leaving the job saying only "batch failed": a batch that never ran has no error file to read the refusal out of, so the message OpenAI puts on the batch itself is the only account of why, and GetBatch now returns it as part of a BatchState the poller writes into the job row --- internal/app/warmupcontent/batch.go | 22 ++++++---- internal/pkg/generation/batch.go | 59 +++++++++++++++++++++------ internal/pkg/generation/batch_test.go | 48 ++++++++++++++++++++++ 3 files changed, 109 insertions(+), 20 deletions(-) create mode 100644 internal/pkg/generation/batch_test.go 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) + } + }) + } +}