Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fix-generation-timeout-reason.md
Original file line number Diff line number Diff line change
@@ -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.
46 changes: 45 additions & 1 deletion apps/desktop/src/main/generation-ipc.test.ts
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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();
});
});
18 changes: 18 additions & 0 deletions apps/desktop/src/main/generation-ipc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
34 changes: 25 additions & 9 deletions apps/desktop/src/main/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -737,18 +741,24 @@ 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,
provider: active.model.provider,
modelId: active.model.modelId,
baseUrl: baseUrl ?? '<default>',
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);
Expand Down Expand Up @@ -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 ?? '<default>',
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);
Expand Down
39 changes: 37 additions & 2 deletions apps/desktop/src/renderer/src/components/Settings.test.ts
Original file line number Diff line number Diff line change
@@ -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)),
Expand Down Expand Up @@ -34,7 +39,6 @@ describe('applyLocaleChange', () => {
expect(result).toBe('zh-CN');
});
});

describe('computeModelOptions', () => {
const suffix = '(active, not in provider list)';

Expand Down Expand Up @@ -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]);
});
});
33 changes: 27 additions & 6 deletions apps/desktop/src/renderer/src/components/Settings.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -2109,12 +2132,10 @@ function AdvancedTab() {
<NativeSelect
value={String(prefs.generationTimeoutSec)}
onChange={(v) => 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 }),
}))}
/>
</Row>

Expand Down
Loading