From 917ad3492979861b2199f76dee3fdf8fd19bd4db Mon Sep 17 00:00:00 2001 From: RagavRida Date: Sat, 16 May 2026 00:17:20 +0530 Subject: [PATCH] fix(design/iterate): cap cumulative iterate timeout to single apiTimeoutMs budget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit callWithThreading + callFresh each received the full apiTimeoutMs, so the worst-case wait on --api-timeout 300000 was 600s (2×300s). Now a shared deadline is set at iteration start; callFresh receives the remaining budget after threading fails or times out, and if the budget is already exhausted at fallback entry a clear Timeout (Xs) error is thrown immediately. --- design/src/cli.ts | 54 ++++++++++++++++++++++----------- design/src/iterate.ts | 11 ++++++- design/src/variants.ts | 3 +- design/test/api-timeout.test.ts | 53 +++++++++++++++++++++++++++++++- 4 files changed, 101 insertions(+), 20 deletions(-) diff --git a/design/src/cli.ts b/design/src/cli.ts index 2ce4af0db..5876b6e85 100644 --- a/design/src/cli.ts +++ b/design/src/cli.ts @@ -25,13 +25,27 @@ import { evolve } from "./evolve"; import { generateDesignToCodePrompt } from "./design-to-code"; import { serve } from "./serve"; import { gallery } from "./gallery"; +import { spawn as nodeSpawn } from "child_process"; import { - daemonStatus as daemonStatusClient, + daemonStatus, + shutdownDaemon, ensureDaemon, publishBoard, - shutdownDaemon, } from "./daemon-client"; -import { spawn as nodeSpawn } from "child_process"; + +/** + * Parse + validate `--api-timeout `. Throws on bad input so the caller can + * surface a clear error rather than passing NaN/0/negative to setTimeout (which + * would fire the AbortController immediately and instant-abort the request). + */ +export function parseApiTimeoutMs(raw: string | undefined): number | undefined { + if (!raw) return undefined; + const parsed = parseInt(raw, 10); + if (!Number.isFinite(parsed) || parsed <= 0) { + throw new Error(`--api-timeout must be a positive integer (ms), got: ${raw}`); + } + return parsed; +} function parseArgs(argv: string[]): { command: string; @@ -133,9 +147,13 @@ async function main(): Promise { // Per-request timeout for OpenAI image-generation calls. Distinct from the // existing `--timeout` flag, which controls the `compare --serve` / `serve` // HTTP listener. See issue #1519. - const apiTimeoutMs = flags["api-timeout"] - ? parseInt(flags["api-timeout"] as string) - : undefined; + let apiTimeoutMs: number | undefined; + try { + apiTimeoutMs = parseApiTimeoutMs(flags["api-timeout"] as string | undefined); + } catch (err: any) { + console.error(err.message); + process.exit(1); + } switch (command) { case "generate": @@ -301,7 +319,7 @@ async function main(): Promise { // Sub-commands: `$D daemon status` and `$D daemon stop [--force]`. const sub = positionals[0] || "status"; if (sub === "status") { - const s = await daemonStatusClient(); + const s = await daemonStatus(); if (!s.running) { console.log(JSON.stringify({ running: false }, null, 2)); process.exit(0); @@ -419,14 +437,16 @@ async function resolveImagePaths(input: string): Promise { // the production install to a single executable; daemon-client.ts spawns // ` --daemon-mode` (or `bun run cli.ts --daemon-mode` in dev) // rather than relying on a separate daemon.ts file at a known path. -if (process.argv.includes("--daemon-mode")) { - const { start } = await import("./daemon"); - start(); - // start() binds Bun.serve and registers signal handlers; this branch - // never falls through to main(). Process stays alive on the bound port. -} else { - main().catch((err) => { - console.error(err.message || err); - process.exit(1); - }); +if (import.meta.main) { + if (process.argv.includes("--daemon-mode")) { + const { start } = await import("./daemon"); + start(); + // start() binds Bun.serve and registers signal handlers; this branch + // never falls through to main(). Process stays alive on the bound port. + } else { + main().catch((err) => { + console.error(err.message || err); + process.exit(1); + }); + } } diff --git a/design/src/iterate.ts b/design/src/iterate.ts index 2ba767eeb..ff05d8d54 100644 --- a/design/src/iterate.ts +++ b/design/src/iterate.ts @@ -32,6 +32,9 @@ export async function iterate(options: IterateOptions): Promise { const startTime = Date.now(); const apiTimeoutMs = options.apiTimeoutMs ?? DEFAULT_IMAGE_GEN_TIMEOUT_MS; + // Single deadline shared across threading + fallback so cumulative wait is + // bounded by apiTimeoutMs rather than 2×apiTimeoutMs. + const deadline = startTime + apiTimeoutMs; // Try multi-turn with previous_response_id first let success = false; @@ -48,13 +51,19 @@ export async function iterate(options: IterateOptions): Promise { console.error(` Threading failed: ${err.message}`); console.error(" Falling back to re-generation with accumulated feedback..."); + const remaining = deadline - Date.now(); + if (remaining <= 0) { + const secs = (apiTimeoutMs / 1000).toFixed(apiTimeoutMs % 1000 === 0 ? 0 : 1); + throw new Error(`Timeout (${secs}s)`); + } + // Fallback: re-generate with original brief + all feedback const accumulatedPrompt = buildAccumulatedPrompt( session.originalBrief, [...session.feedbackHistory, options.feedback] ); - const result = await callFresh(apiKey, accumulatedPrompt, apiTimeoutMs); + const result = await callFresh(apiKey, accumulatedPrompt, remaining); responseId = result.responseId; fs.mkdirSync(path.dirname(options.output), { recursive: true }); diff --git a/design/src/variants.ts b/design/src/variants.ts index bf42ad386..a8343c153 100644 --- a/design/src/variants.ts +++ b/design/src/variants.ts @@ -128,7 +128,8 @@ export async function generateVariant( } catch (err: any) { clearTimeout(timeout); if (err.name === "AbortError") { - return { path: outputPath, success: false, error: `Timeout (${timeoutMs}ms)` }; + const secs = (timeoutMs / 1000).toFixed(timeoutMs % 1000 === 0 ? 0 : 1); + return { path: outputPath, success: false, error: `Timeout (${secs}s)` }; } lastError = err.message; } diff --git a/design/test/api-timeout.test.ts b/design/test/api-timeout.test.ts index b77bf264e..c29cf44ca 100644 --- a/design/test/api-timeout.test.ts +++ b/design/test/api-timeout.test.ts @@ -12,6 +12,7 @@ import os from "os"; import path from "path"; import { generateVariant } from "../src/variants"; import { DEFAULT_IMAGE_GEN_TIMEOUT_MS } from "../src/constants"; +import { parseApiTimeoutMs } from "../src/cli"; describe("DEFAULT_IMAGE_GEN_TIMEOUT_MS", () => { test("default is 300_000ms (5min) — see issue #1519", () => { @@ -19,6 +20,34 @@ describe("DEFAULT_IMAGE_GEN_TIMEOUT_MS", () => { }); }); +describe("parseApiTimeoutMs", () => { + test("undefined input returns undefined (use default)", () => { + expect(parseApiTimeoutMs(undefined)).toBeUndefined(); + }); + + test("valid positive integer returns the parsed number", () => { + expect(parseApiTimeoutMs("300000")).toBe(300000); + expect(parseApiTimeoutMs("1")).toBe(1); + }); + + test("non-numeric input throws (would otherwise be NaN -> instant abort)", () => { + expect(() => parseApiTimeoutMs("abc")).toThrow(/positive integer/); + }); + + test("zero throws (would otherwise fire setTimeout immediately)", () => { + expect(() => parseApiTimeoutMs("0")).toThrow(/positive integer/); + }); + + test("negative throws (would otherwise fire setTimeout immediately)", () => { + expect(() => parseApiTimeoutMs("-1")).toThrow(/positive integer/); + expect(() => parseApiTimeoutMs("-300000")).toThrow(/positive integer/); + }); + + test("trailing garbage is ignored (parseInt semantics; matches other flags)", () => { + expect(parseApiTimeoutMs("300000foo")).toBe(300000); + }); +}); + describe("generateVariant timeoutMs override", () => { let tmpDir: string; let outputPath: string; @@ -54,11 +83,33 @@ describe("generateVariant timeoutMs override", () => { const elapsed = Date.now() - t0; expect(result.success).toBe(false); - expect(result.error).toBe("Timeout (200ms)"); + expect(result.error).toBe("Timeout (0.2s)"); // Was aborted by the 200ms timer, not by exponential-backoff retry chain expect(elapsed).toBeLessThan(2_000); }); + test("formats whole-second timeouts without decimals", async () => { + const fetchFn = (async (_input: any, init?: any) => { + const signal = init?.signal as AbortSignal | undefined; + await new Promise((_resolve, reject) => { + if (!signal) return; + signal.addEventListener("abort", () => { + const err: any = new Error("aborted"); + err.name = "AbortError"; + reject(err); + }); + }); + return new Response("never reached"); + }) as typeof globalThis.fetch; + + const result = await generateVariant( + "fake-key", "prompt", outputPath, "1024x1024", "high", fetchFn, 1000, + ); + + expect(result.success).toBe(false); + expect(result.error).toBe("Timeout (1s)"); + }); + test("default timeoutMs is the shared constant when omitted", async () => { // 1x1 transparent PNG, base64 const TINY_PNG_BASE64 =