Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions apps/gateway/src/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import { imagesRoute } from "./images/route.js";
import {
buildAnthropicErrorBody,
buildOpenAIErrorBody,
getUpstreamErrorCause,
} from "./lib/error-response.js";
import { mcpHandler, registerMcpOAuthRoutes } from "./mcp/mcp.js";
import { tracingMiddleware } from "./middleware/tracing.js";
Expand Down Expand Up @@ -165,8 +166,19 @@ app.onError((error, c) => {

if (error instanceof HTTPException) {
const status = error.status;

if (status >= 500) {
const upstreamCause = getUpstreamErrorCause(error.cause);

if (upstreamCause) {
// Expected upstream/gateway provider error (e.g. a provider 429)
// forwarded from an internal chat-completions call. It is already
// recorded in the log table with its unifiedFinishReason, so surface
// it as a warning rather than an ERROR-level log.
logger.warn("Provider error", {
status,
message: error.message,
unifiedFinishReason: upstreamCause.unifiedFinishReason,
});
} else if (status >= 500) {
logger.error("HTTP 500 exception", error);
} else {
logger.warn("HTTP client error", { status, message: error.message });
Expand Down
12 changes: 11 additions & 1 deletion apps/gateway/src/images/images.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@ import { createRoute, OpenAPIHono, z } from "@hono/zod-openapi";
import { HTTPException } from "hono/http-exception";

import { app } from "@/app.js";
import { upstreamErrorCause } from "@/lib/error-response.js";

import { parseDataUrl, processImageUrl } from "@llmgateway/actions";
import { logger, toError } from "@llmgateway/logger";

import type { ServerTypes } from "@/vars.js";
import type { Context } from "hono";
import type { ContentfulStatusCode } from "hono/utils/http-status";

const imageGenerationsRequestSchema = z.object({
prompt: z.string().min(1).openapi({
Expand Down Expand Up @@ -338,15 +340,23 @@ async function forwardToChatCompletions(
});
const errorData = await response.text();
let errorMessage = `Image generation failed with status ${response.status}`;
let upstreamFinishReason: string | undefined;
try {
const parsed = JSON.parse(errorData);
errorMessage = parsed?.error?.message ?? parsed?.message ?? errorMessage;
const errorType = parsed?.error?.type ?? parsed?.error?.code;
if (errorType === "upstream_error" || errorType === "gateway_error") {
upstreamFinishReason = errorType;
}
Comment on lines +347 to +350

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Check both error.type and error.code independently when deriving upstream markers.

Line 347 currently uses parsed?.error?.type ?? parsed?.error?.code, so if error.type exists but is non-marker (e.g. "api_error"), a marker in error.code is ignored. That drops cause, and Line 181 in apps/gateway/src/app.ts falls back to 5xx ERROR logging.

Suggested fix
-			const errorType = parsed?.error?.type ?? parsed?.error?.code;
-			if (errorType === "upstream_error" || errorType === "gateway_error") {
-				upstreamFinishReason = errorType;
-			}
+			const errorType = parsed?.error?.type;
+			const errorCode = parsed?.error?.code;
+			if (errorType === "upstream_error" || errorType === "gateway_error") {
+				upstreamFinishReason = errorType;
+			} else if (
+				errorCode === "upstream_error" ||
+				errorCode === "gateway_error"
+			) {
+				upstreamFinishReason = errorCode;
+			}

Also applies to: 357-359

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/gateway/src/images/images.ts` around lines 347 - 350, The code derives
errorType using parsed?.error?.type ?? parsed?.error?.code which ignores
parsed.error.code when parsed.error.type exists but is a non-marker; update the
logic around the errorType assignment (the block that sets upstreamFinishReason)
to check parsed?.error?.type and parsed?.error?.code independently and set
upstreamFinishReason if either equals "upstream_error" or "gateway_error" (do
the same fix for the similar logic later in the file that mirrors this
behavior). Ensure you reference parsed.error.type and parsed.error.code
separately when deciding to assign upstreamFinishReason.

} catch {
// use default message
}

throw new HTTPException(response.status as any, {
throw new HTTPException(response.status as ContentfulStatusCode, {
message: errorMessage,
...(upstreamFinishReason
? { cause: upstreamErrorCause(upstreamFinishReason) }
: {}),
});
}

Expand Down
25 changes: 25 additions & 0 deletions apps/gateway/src/lib/error-response.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import {
buildOpenAIErrorBody,
getAnthropicErrorType,
getOpenAIErrorMeta,
getUpstreamErrorCause,
upstreamErrorCause,
} from "./error-response.js";

describe("error-response", () => {
Expand Down Expand Up @@ -70,4 +72,27 @@ describe("error-response", () => {
expect(getAnthropicErrorType(429)).toBe("rate_limit_error");
expect(getAnthropicErrorType(500)).toBe("api_error");
});

test("round-trips the upstream error cause marker", () => {
const cause = upstreamErrorCause("upstream_error");
expect(cause).toEqual({
upstreamError: true,
unifiedFinishReason: "upstream_error",
});
expect(getUpstreamErrorCause(cause)).toBe(cause);
expect(getUpstreamErrorCause(upstreamErrorCause("gateway_error"))).toEqual({
upstreamError: true,
unifiedFinishReason: "gateway_error",
});
});

test("rejects non-marker causes", () => {
expect(getUpstreamErrorCause(undefined)).toBeNull();
expect(getUpstreamErrorCause(null)).toBeNull();
expect(getUpstreamErrorCause(new Error("boom"))).toBeNull();
expect(getUpstreamErrorCause({ upstreamError: true })).toBeNull();
expect(
getUpstreamErrorCause({ unifiedFinishReason: "upstream_error" }),
).toBeNull();
});
});
31 changes: 31 additions & 0 deletions apps/gateway/src/lib/error-response.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,37 @@ export function buildOpenAIErrorBody(opts: {
};
}

// Marker attached as the `cause` of an HTTPException when an internal
// chat-completions call surfaces an expected upstream/gateway provider error
// (e.g. a provider 429). These are already recorded in the log table with their
// unifiedFinishReason, so the global error handler should surface them as
// warnings rather than ERROR-level logs that trip error alerting.
export interface UpstreamErrorCause {
upstreamError: true;
unifiedFinishReason: string;
}

export function upstreamErrorCause(
unifiedFinishReason: string,
): UpstreamErrorCause {
return { upstreamError: true, unifiedFinishReason };
}

export function getUpstreamErrorCause(
cause: unknown,
): UpstreamErrorCause | null {
if (
cause &&
typeof cause === "object" &&
(cause as Partial<UpstreamErrorCause>).upstreamError === true &&
typeof (cause as Partial<UpstreamErrorCause>).unifiedFinishReason ===
"string"
) {
return cause as UpstreamErrorCause;
}
return null;
Comment on lines +150 to +159
}

export function buildAnthropicErrorBody(opts: {
message: string;
status: number;
Expand Down
Loading