mirror of https://github.com/garrytan/gstack.git
fix(design/iterate): cap cumulative iterate timeout to single apiTimeoutMs budget
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.
This commit is contained in:
parent
d78eeeed03
commit
917ad34929
|
|
@ -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 <ms>`. 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<void> {
|
|||
// 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<void> {
|
|||
// 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<string[]> {
|
|||
// the production install to a single executable; daemon-client.ts spawns
|
||||
// `<this binary> --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);
|
||||
});
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -32,6 +32,9 @@ export async function iterate(options: IterateOptions): Promise<void> {
|
|||
|
||||
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<void> {
|
|||
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 });
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<void>((_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 =
|
||||
|
|
|
|||
Loading…
Reference in New Issue