diff --git a/flashduty.go b/flashduty.go index fe6061c..5e5f739 100644 --- a/flashduty.go +++ b/flashduty.go @@ -196,7 +196,14 @@ func (c *Client) doMethod(ctx context.Context, method, path string, body, out an resp.Raw = raw return resp, nil } - return resp, fmt.Errorf("flashduty: malformed response (http %d, request_id %s): %w", httpResp.StatusCode, resp.RequestID, err) + // A non-2xx status with a body that isn't valid envelope JSON never + // came from the Flashduty API itself — it always answers in JSON. + // It's an intermediary (gateway/load balancer/proxy) cutting the + // request off, most often on its own timeout, and returning an + // HTML/plaintext error page instead. Report that distinctly so it + // isn't mistaken for a Flashduty API contract violation. + return resp, fmt.Errorf("flashduty: HTTP %d returned by an intermediary, not the Flashduty API (non-JSON body, %s) — the request likely exceeded a gateway/proxy timeout; retry, or split long-running requests into smaller batches. body: %s", + httpResp.StatusCode, requestIDNote(resp.RequestID), bodySnippet(raw)) } } if env.RequestID != "" { @@ -226,6 +233,30 @@ func (c *Client) doMethod(ctx context.Context, method, path string, body, out an return resp, nil } +// errorBodySnippetSize bounds how much of a non-JSON error body is echoed +// back in an error message. +const errorBodySnippetSize = 120 + +// bodySnippet renders up to the first errorBodySnippetSize bytes of a +// non-JSON response body, quoted, so the underlying error page (e.g. an ALB +// or nginx timeout page) is visible in the error message. +func bodySnippet(raw []byte) string { + if len(raw) > errorBodySnippetSize { + raw = raw[:errorBodySnippetSize] + } + return fmt.Sprintf("%q", raw) +} + +// requestIDNote renders the request id for an error message, or notes its +// absence — an empty Flashcat-Request-Id header is itself evidence that the +// response didn't come from the Flashduty API. +func requestIDNote(requestID string) string { + if requestID == "" { + return "no request id" + } + return "request id " + requestID +} + // isFailureCode reports whether an envelope error.code denotes a real failure. // Empty, "0", and "OK" (the ErrorCodeOK success sentinel) are not failures. func isFailureCode(code string) bool { diff --git a/flashduty_test.go b/flashduty_test.go index 5c38afb..895e25e 100644 --- a/flashduty_test.go +++ b/flashduty_test.go @@ -182,6 +182,66 @@ func TestDoTreatsOKCodeAsSuccess(t *testing.T) { } } +// TestDoReportsIntermediaryOnNon2xxNonJSONBody guards a prod incident: an ALB +// timed out a long-running request and returned an HTML 504 page with no +// Flashcat-Request-Id header. The error must name the status, say the +// response came from an intermediary rather than the Flashduty API, note the +// missing request id, hint at retry/splitting the batch, and echo a body +// snippet — not the old "malformed response" wording, which reads like an +// API bug. +func TestDoReportsIntermediaryOnNon2xxNonJSONBody(t *testing.T) { + c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusGatewayTimeout) + _, _ = io.WriteString(w, "504 Gateway Time-out") + }) + _, err := c.do(context.Background(), "/incident/list", map[string]any{}, nil) + if err == nil { + t.Fatal("expected an error") + } + msg := err.Error() + for _, want := range []string{"HTTP 504", "intermediary", "no request id", "gateway/proxy timeout", "504 Gateway Time-out"} { + if !strings.Contains(msg, want) { + t.Fatalf("error message = %q, want it to contain %q", msg, want) + } + } + if strings.Contains(msg, "malformed response") { + t.Fatalf("error message = %q, must not use the old malformed-response wording", msg) + } +} + +// TestDoReportsIntermediaryWithRequestID covers the case where the +// intermediary (or an upstream hop before it) did forward a request id. +func TestDoReportsIntermediaryWithRequestID(t *testing.T) { + c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Flashcat-Request-Id", "RID-504") + w.WriteHeader(http.StatusGatewayTimeout) + _, _ = io.WriteString(w, "timeout") + }) + _, err := c.do(context.Background(), "/incident/list", map[string]any{}, nil) + if err == nil || !strings.Contains(err.Error(), "request id RID-504") { + t.Fatalf("expected error to include the forwarded request id, got %v", err) + } +} + +// TestDoMapsNon2xxJSONEnvelopeUnaffectedByGatewaySplit confirms a real 504 +// bearing a normal JSON error envelope is completely unaffected by the new +// non-JSON/intermediary branch: it still maps to *ErrorResponse as before. +func TestDoMapsNon2xxJSONEnvelopeUnaffectedByGatewaySplit(t *testing.T) { + c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Flashcat-Request-Id", "RID4") + w.WriteHeader(http.StatusGatewayTimeout) + _, _ = io.WriteString(w, `{"request_id":"RID4","error":{"code":"timeout","message":"upstream took too long"}}`) + }) + _, err := c.do(context.Background(), "/incident/list", map[string]any{}, nil) + var apiErr *ErrorResponse + if !errors.As(err, &apiErr) || apiErr.Code != "timeout" || apiErr.Response.StatusCode != http.StatusGatewayTimeout || apiErr.RequestID != "RID4" { + t.Fatalf("expected mapped ErrorResponse for JSON envelope, got %v", err) + } +} + +// TestDoExposesNonJSONBodyAsRaw also guards the 2xx branch against the +// non-2xx/intermediary-error split above: a non-JSON body on success must +// keep returning Response.Raw with no error, never the new gateway message. func TestDoExposesNonJSONBodyAsRaw(t *testing.T) { const csv = "id,title\n1,boom\n2,bam\n" c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {