diff --git a/.changeset/fix-generation-timeout-reason.md b/.changeset/fix-generation-timeout-reason.md new file mode 100644 index 00000000..3a8bf34e --- /dev/null +++ b/.changeset/fix-generation-timeout-reason.md @@ -0,0 +1,5 @@ +--- +"@open-codesign/desktop": patch +--- + +Fix: preserve the generation-timeout reason so long runs no longer surface a bare "Request was aborted." Provider SDKs rewrite aborted fetches into a generic message that drops `signal.reason`; the generate IPC now re-surfaces the stashed `GENERATION_TIMEOUT` CodesignError (with configured seconds + Settings path) when the controller was aborted by our own timer. Settings → Advanced → Generation timeout also gains 10m / 20m / 30m / 1h / 2h choices so the default 1200s and longer full-PDP runs can actually be configured without the dropdown silently downgrading the stored value. diff --git a/apps/desktop/src/main/generation-ipc.test.ts b/apps/desktop/src/main/generation-ipc.test.ts index 25b10391..73213df7 100644 --- a/apps/desktop/src/main/generation-ipc.test.ts +++ b/apps/desktop/src/main/generation-ipc.test.ts @@ -1,6 +1,10 @@ import { CancelGenerationPayloadV1, CodesignError } from '@open-codesign/shared'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { armGenerationTimeout, cancelGenerationRequest } from './generation-ipc'; +import { + armGenerationTimeout, + cancelGenerationRequest, + extractGenerationTimeoutError, +} from './generation-ipc'; function makeController() { return { abort: vi.fn() } as unknown as AbortController; @@ -182,3 +186,43 @@ describe('armGenerationTimeout', () => { expect(controller.signal.aborted).toBe(false); }); }); + +describe('extractGenerationTimeoutError', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + afterEach(() => { + vi.useRealTimers(); + }); + + it('returns the CodesignError stashed by armGenerationTimeout so the SDK-rewritten AbortError can be upgraded back to GENERATION_TIMEOUT', async () => { + const controller = new AbortController(); + const logger = { warn: vi.fn() }; + + await armGenerationTimeout('gen-1', controller, async () => 3, logger); + vi.advanceTimersByTime(3000); + + const recovered = extractGenerationTimeoutError(controller.signal); + expect(recovered).toBeInstanceOf(CodesignError); + expect(recovered?.code).toBe('GENERATION_TIMEOUT'); + expect(recovered?.message).toContain('3s'); + expect(recovered?.message).toContain('Settings'); + }); + + it('returns null when the controller was aborted by a user-initiated cancel (no reason set)', () => { + const controller = new AbortController(); + controller.abort(); + expect(extractGenerationTimeoutError(controller.signal)).toBeNull(); + }); + + it('returns null when the signal has not been aborted', () => { + const controller = new AbortController(); + expect(extractGenerationTimeoutError(controller.signal)).toBeNull(); + }); + + it('returns null when the abort reason is some other CodesignError (not a timeout)', () => { + const controller = new AbortController(); + controller.abort(new CodesignError('something else', 'PROVIDER_ABORTED')); + expect(extractGenerationTimeoutError(controller.signal)).toBeNull(); + }); +}); diff --git a/apps/desktop/src/main/generation-ipc.ts b/apps/desktop/src/main/generation-ipc.ts index 8a806532..d90deb74 100644 --- a/apps/desktop/src/main/generation-ipc.ts +++ b/apps/desktop/src/main/generation-ipc.ts @@ -82,3 +82,21 @@ export async function armGenerationTimeout( }, ms); return () => clearTimeout(handle); } + +/** + * Provider SDKs (Anthropic / OpenAI) catch an aborted fetch and rethrow their + * own generic `'Request was aborted.'` error, discarding `signal.reason`. When + * the caught error came from our own timeout abort, we want the richer message + * (with the configured seconds and the Settings path) to surface to the user. + * + * Returns the `CodesignError` we stashed on `signal.reason`, or `null` when the + * abort was caused by something else (user-initiated cancel, upstream error). + */ +export function extractGenerationTimeoutError(signal: AbortSignal): CodesignError | null { + if (!signal.aborted) return null; + const reason = signal.reason; + if (reason instanceof CodesignError && reason.code === ERROR_CODES.GENERATION_TIMEOUT) { + return reason; + } + return null; +} diff --git a/apps/desktop/src/main/index.ts b/apps/desktop/src/main/index.ts index 148bc866..d6dbe8c4 100644 --- a/apps/desktop/src/main/index.ts +++ b/apps/desktop/src/main/index.ts @@ -43,7 +43,11 @@ import { registerDiagnosticsIpc } from './diagnostics-ipc'; import { makeRuntimeVerifier } from './done-verify'; import { BrowserWindow, app, clipboard, dialog, ipcMain, shell } from './electron-runtime'; import { registerExporterIpc } from './exporter-ipc'; -import { armGenerationTimeout, cancelGenerationRequest } from './generation-ipc'; +import { + armGenerationTimeout, + cancelGenerationRequest, + extractGenerationTimeoutError, +} from './generation-ipc'; import { maybeAbortIfRunningFromDmg } from './install-check'; import { registerLocaleIpc } from './locale-ipc'; import { getLogPath, getLogger, initLogger } from './logger'; @@ -737,6 +741,12 @@ function registerIpcHandlers(db: Database | null): void { errAsRec['upstream_wire'] = active.wire; } } + // The SDK catches our AbortController and rethrows a generic + // `'Request was aborted.'` that drops signal.reason. Prefer the + // CodesignError we stashed on the signal so the user sees the + // configured timeout + Settings path instead of an opaque message. + const timeoutErr = extractGenerationTimeoutError(controller.signal); + const rethrow = timeoutErr ?? err; logIpc.error('generate.fail', { generationId: id, ms: Date.now() - t0, @@ -744,11 +754,11 @@ function registerIpcHandlers(db: Database | null): void { modelId: active.model.modelId, baseUrl: baseUrl ?? '', status: upstreamStatus, - message: err instanceof Error ? err.message : String(err), - code: err instanceof CodesignError ? err.code : undefined, + message: rethrow instanceof Error ? rethrow.message : String(rethrow), + code: rethrow instanceof CodesignError ? rethrow.code : undefined, }); - recordFinalError('generate', id, err); - throw err; + recordFinalError('generate', id, rethrow); + throw rethrow; } finally { clearTimeoutGuard(); inFlight.delete(id); @@ -846,17 +856,23 @@ function registerIpcHandlers(db: Database | null): void { }); return result; } catch (err) { + // The SDK catches our AbortController and rethrows a generic + // `'Request was aborted.'` that drops signal.reason. Prefer the + // CodesignError we stashed on the signal so the user sees the + // configured timeout + Settings path instead of an opaque message. + const timeoutErr = extractGenerationTimeoutError(controller.signal); + const rethrow = timeoutErr ?? err; logIpc.error('generate.fail', { generationId: id, ms: Date.now() - t0, provider: active.model.provider, modelId: active.model.modelId, baseUrl: baseUrl ?? '', - message: err instanceof Error ? err.message : String(err), - code: err instanceof CodesignError ? err.code : undefined, + message: rethrow instanceof Error ? rethrow.message : String(rethrow), + code: rethrow instanceof CodesignError ? rethrow.code : undefined, }); - recordFinalError('generate', id, err); - throw err; + recordFinalError('generate', id, rethrow); + throw rethrow; } finally { clearTimeoutGuard(); inFlight.delete(id); diff --git a/apps/desktop/src/renderer/src/components/Settings.test.ts b/apps/desktop/src/renderer/src/components/Settings.test.ts index f60551ac..ed7f382f 100644 --- a/apps/desktop/src/renderer/src/components/Settings.test.ts +++ b/apps/desktop/src/renderer/src/components/Settings.test.ts @@ -1,5 +1,10 @@ import { describe, expect, it, vi } from 'vitest'; -import { applyLocaleChange, computeModelOptions } from './Settings'; +import { + TIMEOUT_OPTION_SECONDS, + applyLocaleChange, + computeModelOptions, + resolveTimeoutOptions, +} from './Settings'; vi.mock('@open-codesign/i18n', () => ({ setLocale: vi.fn((locale: string) => Promise.resolve(locale)), @@ -34,7 +39,6 @@ describe('applyLocaleChange', () => { expect(result).toBe('zh-CN'); }); }); - describe('computeModelOptions', () => { const suffix = '(active, not in provider list)'; @@ -88,3 +92,34 @@ describe('computeModelOptions', () => { ]); }); }); + +describe('resolveTimeoutOptions', () => { + it('covers the default 1200s stored value and long-generation 30m / 1h / 2h choices so users can configure what they need without hitting the old 300s ceiling', () => { + expect(TIMEOUT_OPTION_SECONDS).toContain(1200); + expect(TIMEOUT_OPTION_SECONDS).toContain(1800); + expect(TIMEOUT_OPTION_SECONDS).toContain(3600); + expect(TIMEOUT_OPTION_SECONDS).toContain(7200); + }); + + it('returns the canonical options unchanged when the stored value is already present', () => { + const options = resolveTimeoutOptions(1200); + expect(options).toEqual([...TIMEOUT_OPTION_SECONDS]); + }); + + it("merges a stored value that is not in the canonical list and keeps the list sorted so the select shows the user's existing choice instead of silently downgrading on save", () => { + const options = resolveTimeoutOptions(900); + expect(options).toContain(900); + expect(options).toEqual([...options].sort((a, b) => a - b)); + // Canonical entries are preserved. + for (const sec of TIMEOUT_OPTION_SECONDS) { + expect(options).toContain(sec); + } + }); + + it('ignores non-positive or non-finite stored values rather than injecting bogus options', () => { + expect(resolveTimeoutOptions(0)).toEqual([...TIMEOUT_OPTION_SECONDS]); + expect(resolveTimeoutOptions(-1)).toEqual([...TIMEOUT_OPTION_SECONDS]); + expect(resolveTimeoutOptions(Number.NaN)).toEqual([...TIMEOUT_OPTION_SECONDS]); + expect(resolveTimeoutOptions(Number.POSITIVE_INFINITY)).toEqual([...TIMEOUT_OPTION_SECONDS]); + }); +}); diff --git a/apps/desktop/src/renderer/src/components/Settings.tsx b/apps/desktop/src/renderer/src/components/Settings.tsx index 522567dc..9bc3f1c7 100644 --- a/apps/desktop/src/renderer/src/components/Settings.tsx +++ b/apps/desktop/src/renderer/src/components/Settings.tsx @@ -1644,6 +1644,29 @@ export function computeModelOptions(input: { return base; } +/** + * Canonical timeout choices shown in Settings → Advanced. The default prefs + * value is 1200s (20 min); long generations (full PDP runs) need 30-60 min, + * so the dropdown tops out at 2h. The old 60-300s ceiling silently clamped + * the stored value when the UI couldn't represent it. + */ +export const TIMEOUT_OPTION_SECONDS = [60, 120, 180, 300, 600, 1200, 1800, 3600, 7200] as const; + +/** + * Returns the canonical timeout list with `currentSec` merged in when it is a + * positive finite value that isn't already present. Prevents the select from + * rendering with no selection when the user (or an earlier build) stored a + * custom value — and prevents a silent downgrade on the next save. + */ +export function resolveTimeoutOptions(currentSec: number): number[] { + const base: number[] = [...TIMEOUT_OPTION_SECONDS]; + if (Number.isFinite(currentSec) && currentSec > 0 && !base.includes(currentSec)) { + base.push(currentSec); + base.sort((a, b) => a - b); + } + return base; +} + function AppearanceTab() { const t = useT(); const theme = useCodesignStore((s) => s.theme); @@ -2109,12 +2132,10 @@ function AdvancedTab() { void updatePref({ generationTimeoutSec: Number(v) })} - options={[ - { value: '60', label: t('settings.advanced.timeoutSeconds', { value: 60 }) }, - { value: '120', label: t('settings.advanced.timeoutSeconds', { value: 120 }) }, - { value: '180', label: t('settings.advanced.timeoutSeconds', { value: 180 }) }, - { value: '300', label: t('settings.advanced.timeoutSeconds', { value: 300 }) }, - ]} + options={resolveTimeoutOptions(prefs.generationTimeoutSec).map((sec) => ({ + value: String(sec), + label: t('settings.advanced.timeoutSeconds', { value: sec }), + }))} />