fix: two tail mislabeled-500 sources (games cursor + orchestrator 4xx mapping) (#2513)

* fix(games): wrap getPlayerHistory cursor in parseDateTimeBestEffort

The judgment-history pagination built its upper bound as a bare ClickHouse
string literal — `createdAt < '<iso>'` — while the lower bound on the line above
correctly wraps it: `createdAt >= parseDateTimeBestEffort('<iso>')`. ClickHouse
can't implicitly coerce an ISO-8601 string (with the `T` / milliseconds / `Z`) to
DateTime, so the bare comparison threw `Cannot convert string ... to type
DateTime` and 500'd every page-2+ history fetch (any request carrying a cursor).

Surfaced as ~0.04/s of trpc/games.newOrder.getHistory 500s in the per-route
counter (civitai_app_http_errors_total). Fix mirrors the lower bound.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(orchestrator): map unhandled 4xx workflow errors to 4xx, not 500

submitWorkflow's error switch handled 400/401/403/500 explicitly and re-threw
everything else as a raw error, which tRPC maps to INTERNAL_SERVER_ERROR (500).
So an orchestrator client/validation rejection returned with any other 4xx
status (e.g. "<resource> is not enabled for generation. Please contact …") was
mislabeled as the app's own 500 on generate/whatIf.

Fix: in the default branch, surface an unhandled 4xx as a 4xx
(throwBadRequestError with the orchestrator message). Genuine upstream 5xx and
status-less failures still fall through to a server error (correctly counted).

Surfaced as ~0.10/s of trpc/orchestrator.whatIfFromGraph + generateFromGraph
500s in the per-route counter. Covers both (both route through submitWorkflow).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(orchestrator): map 429 to TOO_MANY_REQUESTS, not flattened BAD_REQUEST

Audit follow-up on the 4xx-mapping fix: flattening every unhandled 4xx to
BAD_REQUEST loses 429's rate-limit semantics AND defeats the tRPC onError
Axiom-skip (it skips TOO_MANY_REQUESTS but not BAD_REQUEST), so a 429 storm from
the orchestrator would resume hammering Axiom + the event loop — the exact thing
that skip exists to prevent. Add an explicit `case 429 → throwRateLimitError`;
other unhandled 4xx still map to BAD_REQUEST, 5xx still fall through to 500.
(Latent today — no orchestrator 429 observed in 3d of prod Loki — but cheap
insurance against the footgun.)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(orchestrator): apply the 4xx/429 mapping to queryWorkflows + getWorkflow too

Audit follow-up: the sibling read-path switches had the same latent mislabel as
submitWorkflow — an unhandled 4xx (notably a 404 on a deleted/not-owned workflow,
reached via the orchestrator/blocks/comics routers) hit `default: throw error`
→ tRPC INTERNAL_SERVER_ERROR (500). Port the same `case 429 → throwRateLimitError`
+ unhandled-4xx → throwBadRequestError fallthrough; 5xx/status-less stay 500.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(orchestrator): preserve 404 (not-found) on the workflow read paths

Audit follow-up: queryWorkflows/getWorkflow flattened an unhandled 404 (a
deleted/not-owned workflowId) to BAD_REQUEST via the generic 4xx fallthrough.
Add an explicit `case 404 → throwNotFoundError` before it so callers can still
distinguish "gone" from "malformed". (Strictly better than the prior 500 either
way; this just restores the precise status.) submitWorkflow unaffected — a 404
on a submit stays a BAD_REQUEST.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Zachary Lowden
2026-06-13 15:25:55 -05:00
committed by GitHub
parent 99d7dbc3e1
commit 96b73cfead
2 changed files with 43 additions and 2 deletions
@@ -1878,8 +1878,12 @@ export async function getPlayerHistory({
`userId = ${playerId}`,
`createdAt >= parseDateTimeBestEffort('${player.startAt.toISOString()}')`,
];
// cursor is now guaranteed to be a Date (schema coerces); safe to ISO-format.
if (cursor) AND.push(`createdAt < '${cursor.toISOString()}'`);
// cursor is a Date (schema coerces). Wrap in parseDateTimeBestEffort() exactly
// like the lower bound above — ClickHouse can't implicitly coerce an ISO-8601
// string (with the `T`/ms/`Z`) to DateTime, so a bare `createdAt < '<iso>'`
// threw "Cannot convert string ... to type DateTime" and 500'd every page-2+
// history fetch (whenever a cursor is present).
if (cursor) AND.push(`createdAt < parseDateTimeBestEffort('${cursor.toISOString()}')`);
const HAVING = [];
if (status?.length) HAVING.push(`status IN ('${status.join("','")}')`);
@@ -30,6 +30,8 @@ import {
throwBadRequestError,
throwInsufficientFundsError,
throwInternalServerError,
throwNotFoundError,
throwRateLimitError,
} from '~/server/utils/errorHandling';
export async function queryWorkflows({
@@ -59,9 +61,20 @@ export async function queryWorkflows({
throw throwAuthorizationError(error.detail);
case 403:
throw throwInsufficientFundsError(error.detail);
case 429:
throw throwRateLimitError(error.detail);
case 404:
// Preserve not-found semantics on the read paths (a deleted/not-owned
// workflowId) rather than flattening to BAD_REQUEST below.
throw throwNotFoundError(error.detail);
default:
if (error.detail?.startsWith('<!DOCTYPE'))
throw throwInternalServerError('Generation services down');
// An unhandled 4xx is a client/validation fault (e.g. a 404 on a deleted or
// not-owned workflow) — surface as 4xx, not a re-thrown raw error that tRPC
// maps to 500. Genuine 5xx / status-less failures stay a server error.
if (typeof error.status === 'number' && error.status >= 400 && error.status < 500)
throw throwBadRequestError(error.detail);
throw error;
}
}
@@ -85,9 +98,20 @@ export async function getWorkflow({
throw throwAuthorizationError(error.detail);
case 403:
throw throwInsufficientFundsError(error.detail);
case 429:
throw throwRateLimitError(error.detail);
case 404:
// Preserve not-found semantics on the read paths (a deleted/not-owned
// workflowId) rather than flattening to BAD_REQUEST below.
throw throwNotFoundError(error.detail);
default:
if (error.detail?.startsWith('<!DOCTYPE'))
throw throwInternalServerError('Generation services down');
// An unhandled 4xx is a client/validation fault (e.g. a 404 on a deleted or
// not-owned workflow) — surface as 4xx, not a re-thrown raw error that tRPC
// maps to 500. Genuine 5xx / status-less failures stay a server error.
if (typeof error.status === 'number' && error.status >= 400 && error.status < 500)
throw throwBadRequestError(error.detail);
throw error;
}
}
@@ -159,11 +183,24 @@ export async function submitWorkflow({
throw throwAuthorizationError(message);
case 403:
throw throwInsufficientFundsError(message);
case 429:
// Preserve rate-limit semantics: TOO_MANY_REQUESTS (not a flattened
// BAD_REQUEST), so the client can back off AND the tRPC onError Axiom-skip
// for TOO_MANY_REQUESTS keeps a 429 storm off the event loop.
throw throwRateLimitError(message);
case 500:
throw throwInternalServerError(message);
default:
if (message?.startsWith('<!DOCTYPE'))
throw throwInternalServerError('Generation services down');
// An unhandled 4xx from the orchestrator is a client/validation fault
// (e.g. "<resource> is not enabled for generation. Please contact …"),
// not a server error. Surface it as a 4xx instead of re-throwing a raw
// error that tRPC maps to INTERNAL_SERVER_ERROR (500) — that misclassified
// generate/whatIf validation rejections as the app's own 500s. Genuine
// upstream 5xx / status-less failures still fall through to a server error.
if (typeof response.status === 'number' && response.status >= 400 && response.status < 500)
throw throwBadRequestError(message);
throw error;
}
}