From 229c50dd28aa71e1c6a897cd4d40e7f97d6f9816 Mon Sep 17 00:00:00 2001 From: Goon Date: Thu, 6 Aug 2026 12:19:24 +0700 Subject: [PATCH] fix(media): scale commit timeout with size and surface structured API errors Large media commits run backend-side ffprobe that streams the whole object from storage (~50s for 300MB), exceeding the fixed 30s request timeout and aborting a succeeding commit with a spurious 408 (asset left in 'uploaded'). Add an optional per-request timeoutMs and pass a size-scaled window to commit. Backend error envelopes wrap the message in { error: { message }, message }, so 'data.error ?? data.message' stringified the object as [object Object] for every structured 4xx. Extract error.message / top-level message instead. Also reuse the asset payload the backend echoes in a 409 (already-committed) body instead of discarding objectKey. --- src/client/http-client.test.ts | 17 ++++++++++++ src/client/http-client.ts | 41 ++++++++++++++++++++++------ src/utils/media-upload.test.ts | 50 +++++++++++++++++++++++++++++++++- src/utils/media-upload.ts | 33 +++++++++++++++++----- 4 files changed, 124 insertions(+), 17 deletions(-) diff --git a/src/client/http-client.test.ts b/src/client/http-client.test.ts index 9905eaf..8790d13 100644 --- a/src/client/http-client.test.ts +++ b/src/client/http-client.test.ts @@ -93,6 +93,23 @@ describe("ApiClient path guard + envelope unwrap", () => { const c = new ApiClient(cfg({ token: "t" })); await expect(c.get("/cms/x")).rejects.toMatchObject({ statusCode: 422, apiMessage: "boom" }); }); + + it("extracts the message from a structured error envelope (not [object Object])", async () => { + // Backend shape: { success:false, error:{ id, code, status, message }, message } + mockFetch( + { success: false, error: { id: "payload_too_large", code: 413, message: "media: payload too large" }, message: "media: payload too large" }, + false, + 413 + ); + const c = new ApiClient(cfg({ token: "t" })); + await expect(c.get("/cms/x")).rejects.toMatchObject({ statusCode: 413, apiMessage: "media: payload too large" }); + }); + + it("falls back to the top-level message when error is not a string", async () => { + mockFetch({ error: {}, message: "top-level detail" }, false, 400); + const c = new ApiClient(cfg({ token: "t" })); + await expect(c.get("/cms/x")).rejects.toMatchObject({ statusCode: 400, apiMessage: "top-level detail" }); + }); }); describe("putAbsoluteBytes", () => { diff --git a/src/client/http-client.ts b/src/client/http-client.ts index 0338709..d033234 100644 --- a/src/client/http-client.ts +++ b/src/client/http-client.ts @@ -11,6 +11,31 @@ function isMcpPath(path: string): boolean { return path === "/mcp" || path.startsWith("/mcp/"); } +// Pull a human-readable message out of an error response body. The backend +// wraps errors as { success:false, error:{ id, code, status, message }, message } +// so a naive `data.error` yields the object itself (rendered "[object Object]"). +// Prefer error.message, then the top-level message, then a bare string body. +function extractApiErrorMessage(data: unknown, statusText: string): string { + if (data && typeof data === "object") { + const d = data as Record; + const err = d.error; + if (typeof err === "string" && err) return err; + if (err && typeof err === "object") { + const m = (err as Record).message; + if (typeof m === "string" && m) return m; + } + if (typeof d.message === "string" && d.message) return d.message; + } + if (typeof data === "string" && data) return data; + return statusText; +} + +// Per-request overrides. `timeoutMs` lets callers widen the abort window for +// requests known to run long server-side (e.g. media commit content-probing). +export interface RequestOptions { + timeoutMs?: number; +} + // HTTP client for AgentBrain API with auth, error handling, and SSE streaming export class ApiClient { private baseUrl: string; @@ -35,8 +60,8 @@ export class ApiClient { return this.request("GET", path, undefined, params); } - async post(path: string, body?: unknown): Promise { - return this.request("POST", path, body); + async post(path: string, body?: unknown, opts?: RequestOptions): Promise { + return this.request("POST", path, body, undefined, opts); } async put(path: string, body?: unknown): Promise { @@ -190,17 +215,18 @@ export class ApiClient { return url.toString(); } - private async request(method: string, path: string, body?: unknown, params?: Record): Promise { + private async request(method: string, path: string, body?: unknown, params?: Record, opts?: RequestOptions): Promise { const url = this.buildUrl(path, params); const headers = { ...this.baseHeaders(), ...this.authHeaders(path) }; const start = Date.now(); + const timeout = opts?.timeoutMs ?? this.timeout; if (this.verbose) { console.error(`${method} ${url}`); } const controller = new AbortController(); - const timeoutId = setTimeout(() => controller.abort(), this.timeout); + const timeoutId = setTimeout(() => controller.abort(), timeout); try { const response = await fetch(url, { @@ -225,10 +251,7 @@ export class ApiClient { } if (!response.ok) { - const msg = (data as Record)?.error ?? - (data as Record)?.message ?? - response.statusText; - throw new ApiError(response.status, String(msg), data); + throw new ApiError(response.status, extractApiErrorMessage(data, response.statusText), data); } // Unwrap envelope if present: { data: T } -> T @@ -239,7 +262,7 @@ export class ApiClient { } catch (err) { if (err instanceof ApiError) throw err; if ((err as Error).name === "AbortError") { - throw new ApiError(408, `Request timed out after ${this.timeout}ms`); + throw new ApiError(408, `Request timed out after ${timeout}ms`); } throw err; } finally { diff --git a/src/utils/media-upload.test.ts b/src/utils/media-upload.test.ts index f8b26d2..5021173 100644 --- a/src/utils/media-upload.test.ts +++ b/src/utils/media-upload.test.ts @@ -2,8 +2,9 @@ import { describe, it, expect, beforeAll, afterAll } from "vitest"; import { writeFile, rm } from "node:fs/promises"; import { join } from "node:path"; import { tmpdir } from "node:os"; -import { detectContentType, detectKind, uploadMedia, MAX_UPLOAD_BYTES } from "./media-upload.js"; +import { detectContentType, detectKind, uploadMedia, MAX_UPLOAD_BYTES, commitTimeoutMs } from "./media-upload.js"; import type { ApiClient } from "../client/http-client.js"; +import { ApiError } from "../client/api-error.js"; describe("detectContentType", () => { it("maps known extensions", () => { @@ -107,6 +108,53 @@ describe("uploadMedia orchestration", () => { const fakeClient = { post: async () => ({}), putAbsoluteBytes: async () => {} } as unknown as ApiClient; await expect(uploadMedia(fakeClient, { filePath: "/no/such/file.pdf" })).rejects.toThrow(/File not found/); }); + + it("passes a size-scaled timeout to the commit request", async () => { + let commitOpts: { timeoutMs?: number } | undefined; + const fakeClient = { + post: async (path: string, _body: unknown, opts?: { timeoutMs?: number }) => { + if (path.endsWith("/presign")) { + return { assetId: "a", putUrl: "https://x/y", expiresAt: "", fields: {} }; + } + commitOpts = opts; + return { assetId: "a", status: "ready", objectKey: "k" }; + }, + putAbsoluteBytes: async () => {}, + } as unknown as ApiClient; + + await uploadMedia(fakeClient, { filePath, computeSha256: false }); + // Small fixture → floor applies; must be well above the default 30s request timeout. + expect(commitOpts?.timeoutMs).toBe(commitTimeoutMs(Buffer.byteLength(fileBody))); + expect(commitOpts?.timeoutMs).toBeGreaterThanOrEqual(60_000); + }); + + it("treats a 409 commit as success, reusing the echoed asset payload", async () => { + const fakeClient = { + post: async (path: string) => { + if (path.endsWith("/presign")) { + return { assetId: "dup", putUrl: "https://x/y", expiresAt: "", fields: {} }; + } + throw new ApiError(409, "already committed", { + data: { assetId: "dup", status: "ready", objectKey: "org/dup.pdf" }, + }); + }, + putAbsoluteBytes: async () => {}, + } as unknown as ApiClient; + + const result = await uploadMedia(fakeClient, { filePath, computeSha256: false }); + expect(result.status).toBe("ready"); + expect(result.objectKey).toBe("org/dup.pdf"); + }); +}); + +describe("commitTimeoutMs", () => { + it("applies a floor for small files", () => { + expect(commitTimeoutMs(1_000)).toBe(60_000); + }); + it("scales with size for large media", () => { + // 300MB → 300 * 1000ms = 300s, comfortably above the observed ~50s probe time. + expect(commitTimeoutMs(300_000_000)).toBe(300_000); + }); }); describe("limits", () => { diff --git a/src/utils/media-upload.ts b/src/utils/media-upload.ts index 22a9561..90ebac5 100644 --- a/src/utils/media-upload.ts +++ b/src/utils/media-upload.ts @@ -12,6 +12,18 @@ export const MAX_UPLOAD_BYTES = 500_000_000; // backend (which HEAD-validates size authoritatively) is rarely worth the I/O. export const SHA256_DEFAULT_MAX_BYTES = 100_000_000; +// Floor for the commit timeout so even small uploads tolerate a slow probe. +const COMMIT_TIMEOUT_FLOOR_MS = 60_000; +// Extra window granted per megabyte to cover backend content-probing that +// streams the object from storage (observed ~50s for a 300MB media asset). +const COMMIT_TIMEOUT_MS_PER_MB = 1_000; + +// Size-scaled abort window for the commit step. Kept generous because it only +// caps a request that returns as soon as the backend finishes probing. +export function commitTimeoutMs(sizeBytes: number): number { + return Math.max(COMMIT_TIMEOUT_FLOOR_MS, Math.ceil(sizeBytes / 1_000_000) * COMMIT_TIMEOUT_MS_PER_MB); +} + // Media asset kind accepted by the presign endpoint. export type MediaKind = "audio" | "video" | "image" | "raw_doc"; @@ -150,15 +162,22 @@ export async function uploadMedia(client: ApiClient, opts: UploadOptions): Promi let commit: CommitResponse; try { - commit = await client.post("/cms/media/uploads/commit", { - assetId: presign.assetId, - sha256, - contentType, - }); + commit = await client.post( + "/cms/media/uploads/commit", + { assetId: presign.assetId, sha256, contentType }, + // Commit runs backend-side content probing (media kinds stream the whole + // object from storage through ffprobe), which scales with file size and + // routinely exceeds the default 30s request timeout for large media. + // Give commit a size-scaled window so a slow-but-succeeding commit is not + // aborted (leaving the asset stuck in "uploaded"). + { timeoutMs: commitTimeoutMs(sizeBytes) } + ); } catch (err) { - // 409 = asset already committed (e.g. a retried commit). Treat as success. + // 409 = asset already committed (e.g. a retried commit). Treat as success, + // reusing the asset payload the backend echoes in the 409 body when present. if (err instanceof ApiError && err.statusCode === 409) { - commit = { assetId: presign.assetId, status: "ready", objectKey: "" }; + const echoed = (err.details as { data?: CommitResponse } | undefined)?.data; + commit = echoed ?? { assetId: presign.assetId, status: "ready", objectKey: "" }; } else { throw err; }