Repository navigation
fix(logs): keep recognized bare upstream error classes in synthesized terminals #6889
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,154 @@ | ||
| import { | ||
| CYBER_POLICY_ERROR_CODE, | ||
| httpStatusFromTerminalError, | ||
| isCyberPolicyCode, | ||
| isCyberPolicyMessage, | ||
| isRateLimitOrQuotaFailureMessage, | ||
| recognizedUpstreamError, | ||
| } from "../lib/errors"; | ||
|
|
||
| /** First bounded upstream diagnostics shared by request contexts and final rows. */ | ||
| export interface UpstreamErrorDiagnostics { | ||
| upstreamErrorCode?: string; | ||
| upstreamErrorType?: string; | ||
| upstreamRequestId?: string; | ||
| } | ||
|
|
||
| interface TerminalStatusContext extends UpstreamErrorDiagnostics { | ||
| terminalHttpStatus?: number; | ||
| terminalErrorCode?: typeof CYBER_POLICY_ERROR_CODE; | ||
| } | ||
|
|
||
| export interface UpstreamErrorPayload { | ||
| type?: unknown; | ||
| code?: unknown; | ||
| message?: unknown; | ||
| error?: { type?: unknown; code?: unknown; message?: unknown }; | ||
| last_error?: { type?: unknown; code?: unknown; message?: unknown }; | ||
| response?: { | ||
| error?: { type?: unknown; code?: unknown; message?: unknown }; | ||
| incomplete_details?: { code?: unknown; message?: unknown; reason?: unknown }; | ||
| }; | ||
| } | ||
|
|
||
| // Bare error evidence is provisional: only a genuine terminal owns account-health status. | ||
| const provisionalStatuses = new WeakMap<TerminalStatusContext, number>(); | ||
| const genuineTerminals = new WeakSet<TerminalStatusContext>(); | ||
|
|
||
| const UPSTREAM_DIAGNOSTIC_TOKEN = /^[A-Za-z0-9_.:-]{1,128}$/; | ||
|
|
||
| function noteBoundedDiagnostic( | ||
| logCtx: TerminalStatusContext, | ||
| field: "upstreamErrorCode" | "upstreamErrorType" | "upstreamRequestId", | ||
| value: unknown, | ||
| ): void { | ||
| if (logCtx[field] !== undefined) return; | ||
| if (typeof value !== "string" || !UPSTREAM_DIAGNOSTIC_TOKEN.test(value)) return; | ||
| logCtx[field] = value; | ||
| const status = logCtx.terminalHttpStatus; | ||
| if (status === undefined || status >= 500) console.warn(`[opencodex] upstream failure${status ? ` status=${status}` : ""}${logCtx.upstreamErrorCode ? ` code=${logCtx.upstreamErrorCode}` : ""}${logCtx.upstreamRequestId ? ` request_id=${logCtx.upstreamRequestId}` : ""}`); | ||
| } | ||
|
|
||
| export function noteUpstreamRequestId(logCtx: TerminalStatusContext, headers: Headers): void { | ||
| noteBoundedDiagnostic(logCtx, "upstreamRequestId", headers.get("openai-request-id") ?? headers.get("x-request-id")); | ||
| } | ||
|
|
||
| export function captureUpstreamTerminalDiagnostics(logCtx: TerminalStatusContext, json: UpstreamErrorPayload): void { | ||
| captureTerminalHttpStatus(logCtx, json); | ||
| noteBoundedDiagnostic(logCtx, "upstreamErrorCode", | ||
| json.error?.code ?? json.last_error?.code ?? json.response?.error?.code ?? json.code); | ||
| noteBoundedDiagnostic(logCtx, "upstreamErrorType", | ||
| json.error?.type ?? json.last_error?.type ?? json.response?.error?.type); | ||
| } | ||
|
|
||
| export function upstreamDiagnosticLogFields(logCtx: UpstreamErrorDiagnostics): UpstreamErrorDiagnostics { | ||
| return { | ||
| ...(logCtx.upstreamErrorCode ? { upstreamErrorCode: logCtx.upstreamErrorCode } : {}), | ||
| ...(logCtx.upstreamErrorType ? { upstreamErrorType: logCtx.upstreamErrorType } : {}), | ||
| ...(logCtx.upstreamRequestId ? { upstreamRequestId: logCtx.upstreamRequestId } : {}), | ||
| }; | ||
| } | ||
|
|
||
| function captureTerminalHttpStatus( | ||
| logCtx: TerminalStatusContext, | ||
| json: UpstreamErrorPayload, | ||
| ): void { | ||
| const type = json.type; | ||
| const terminal = type === "response.failed" || type === "response.completed" || type === "response.incomplete"; | ||
| if (!terminal && type !== "error") return; | ||
| if (terminal && provisionalStatuses.has(logCtx)) { | ||
| const provisional = provisionalStatuses.get(logCtx); | ||
| provisionalStatuses.delete(logCtx); | ||
| if (logCtx.terminalHttpStatus === provisional) { | ||
| delete logCtx.terminalHttpStatus; | ||
| delete logCtx.terminalErrorCode; | ||
| } | ||
| } | ||
| if (logCtx.terminalHttpStatus !== undefined || (type === "error" && genuineTerminals.has(logCtx))) return; | ||
| if (terminal) genuineTerminals.add(logCtx); | ||
| const noteStatus = (status: number): void => { | ||
| logCtx.terminalHttpStatus = status; | ||
| if (type === "error") provisionalStatuses.set(logCtx, status); | ||
| }; | ||
| // A completed terminal only retires provisional bare-error evidence; it records no status. | ||
| if (type === "response.completed") return; | ||
| const responseError = json.response?.error; | ||
| const responseDetails = json.response?.incomplete_details; | ||
| const candidates: Array<{ type?: unknown; code?: unknown; message?: unknown } | undefined> = [ | ||
| json.error, json.last_error, responseError, responseDetails, json, | ||
| ]; | ||
| const policy = candidates.some(candidate => ( | ||
| candidate?.code === null || typeof candidate?.code === "string" | ||
| ) && isCyberPolicyCode(candidate.code as string | null | undefined)) | ||
| || candidates.some(candidate => ( | ||
| typeof candidate?.message === "string" | ||
| && candidate.message.trim().length > 0 | ||
| && isCyberPolicyMessage(candidate.message) | ||
| )); | ||
| if (policy) { | ||
| logCtx.terminalErrorCode = CYBER_POLICY_ERROR_CODE; | ||
| noteStatus(400); | ||
| return; | ||
| } | ||
| // A quota terminal can carry only a structured reason, without an error message. | ||
| // Keep this separate from normal output limits and from the policy precedence above. | ||
| const quotaTag = (value: unknown): boolean => value === "usage_limit_reached" | ||
| || value === "rate_limit_exceeded" || value === "insufficient_quota"; | ||
| const structuredRefusal = candidates.some(candidate => [400, 401, 403, 499].includes( | ||
| httpStatusFromTerminalError({ | ||
| type: typeof candidate?.type === "string" ? candidate.type : undefined, | ||
| code: typeof candidate?.code === "string" ? candidate.code : undefined, | ||
| }), | ||
| )); | ||
| const ordinaryIncompleteReason = typeof responseDetails?.reason === "string" | ||
| && ["max_output_tokens", "content_filter", "steered", "upstream_stall_timeout", "adapter_eof"].includes(responseDetails.reason); | ||
| if (type === "response.incomplete" && !structuredRefusal && (quotaTag(responseDetails?.reason) || candidates.some(candidate => | ||
| quotaTag(candidate?.code) | ||
| || quotaTag(candidate?.type) || candidate?.type === "rate_limit_error" | ||
| || (!ordinaryIncompleteReason && typeof candidate?.message === "string" && isRateLimitOrQuotaFailureMessage(candidate.message)) | ||
| ))) { | ||
| // The shared quota classifier also accepts a numeric HTTP status as its message. | ||
| // Preserve explicit payment-required evidence rather than relabeling it as 429. | ||
| noteStatus(candidates.some(candidate => typeof candidate?.message === "string" | ||
| && Number(candidate.message.trim()) === 402) ? 402 : 429); | ||
| return; | ||
| } | ||
| if (type === "error") { | ||
| const structured = recognizedUpstreamError({ | ||
| code: candidates.find(candidate => typeof candidate?.code === "string")?.code, | ||
| type: candidates.slice(0, -1).find(candidate => typeof candidate?.type === "string")?.type, | ||
| }); | ||
| // Unrecognized bare errors leave the status to explicit event fields (combo preflight). | ||
| if (structured) noteStatus(structured.httpStatus); | ||
|
Comment on lines
+136
to
+142
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: python3 - <<'PY'
from pathlib import Path
for name, start, end in [
("src/server/request-log-terminal-status.ts", 72, 150),
("src/server/responses/combo-stream-preflight.ts", 35, 75),
("src/server/responses/combo-stream-preflight.ts", 228, 250),
]:
path = Path(name)
print(f"\n--- {name}:{start}-{end} ---")
for no, line in enumerate(path.read_text().splitlines(), 1):
if start <= no <= end:
print(f"{no:4} {line}")
PYRepository: lidge-jun/opencodex Length of output: 7759 🏁 Script executed: python3 - <<'PY'
from pathlib import Path
path = Path("src/server/request-log-terminal-status.ts")
for no, line in enumerate(path.read_text().splitlines(), 1):
if 1 <= no <= 72 or 145 <= no <= 190:
print(f"{no:4} {line}")
PYRepository: lidge-jun/opencodex Length of output: 4048 Preserve explicit When a bare error contains Only record the recognized provisional status when the bare event has no valid explicit status. Suggested fix export interface UpstreamErrorPayload {
type?: unknown;
code?: unknown;
message?: unknown;
- error?: { type?: unknown; code?: unknown; message?: unknown };
+ status?: unknown;
+ status_code?: unknown;
+ http_status?: unknown;
+ error?: {
+ type?: unknown;
+ code?: unknown;
+ message?: unknown;
+ status?: unknown;
+ status_code?: unknown;
+ http_status?: unknown;
+ };
last_error?: { type?: unknown; code?: unknown; message?: unknown };
response?: {
error?: { type?: unknown; code?: unknown; message?: unknown };
@@
if (type === "error") {
const structured = recognizedUpstreamError({
code: candidates.find(candidate => typeof candidate?.code === "string")?.code,
type: candidates.slice(0, -1).find(candidate => typeof candidate?.type === "string")?.type,
});
+ const hasExplicitStatus = [
+ json.status,
+ json.status_code,
+ json.http_status,
+ json.error?.status,
+ json.error?.status_code,
+ json.error?.http_status,
+ ].some(value => {
+ const status = typeof value === "number" && Number.isInteger(value)
+ ? value
+ : typeof value === "string" && /^\d{3}$/.test(value.trim())
+ ? Number(value)
+ : undefined;
+ return status !== undefined && status >= 400 && status <= 599;
+ });
// Unrecognized bare errors leave the status to explicit event fields (combo preflight).
- if (structured) noteStatus(structured.httpStatus);
+ if (structured && !hasExplicitStatus) noteStatus(structured.httpStatus);
return;
}🤖 Prompt for AI Agents |
||
| return; | ||
| } | ||
| if (type !== "response.failed" || !responseError || typeof responseError !== "object") return; | ||
| const responseCode = responseError.code === null || typeof responseError.code === "string" | ||
| ? responseError.code | ||
| : undefined; | ||
| noteStatus(httpStatusFromTerminalError({ | ||
| type: typeof responseError.type === "string" ? responseError.type : undefined, | ||
| code: responseCode, | ||
| message: typeof responseError.message === "string" ? responseError.message : undefined, | ||
| })); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When an upstream emits a valid bare error containing only a recognized type/code, such as
{"type":"error","error":{"type":"rate_limit_error","code":"rate_limit_exceeded"}}, these new arguments never carry that classification:boundedBareUpstreamErrorreturnsundefinedwhenever both the message and refusal code are absent, soupstreamError()remains undefined and EOF takes theadapterEofIncompleteFramebranch instead. Both relay modes consequently expose a generic incomplete terminal rather than the intended 429/503 failed terminal; treat a recognized type/code as sufficient evidence and synthesize a bounded fallback message.Useful? React with 👍 / 👎.