From 55cde541ea04eff7db8819f0c705831df70f757c Mon Sep 17 00:00:00 2001 From: William Valentin Date: Wed, 18 Feb 2026 11:21:57 -0800 Subject: [PATCH] fix: abort model retries immediately on user cancellation --- docs/plans/state.json | 17 ++++++++- src/backends/native/agent.ts | 6 +++ src/models/retry.test.ts | 28 ++++++++++++++ src/models/retry.ts | 72 +++++++++++++++++++++++++++++++++++- src/models/router.test.ts | 32 ++++++++++++++++ src/models/router.ts | 25 ++++++++++++- 6 files changed, 177 insertions(+), 3 deletions(-) diff --git a/docs/plans/state.json b/docs/plans/state.json index b633c8d..970b845 100644 --- a/docs/plans/state.json +++ b/docs/plans/state.json @@ -5408,10 +5408,25 @@ "docs/plans/state.json" ], "test_status": "pnpm test:run src/config/schema.test.ts src/cli/setup/config.test.ts src/cli/setup/sections.test.ts + pnpm typecheck passing" + }, + "model-retry-cancel-abort-immediacy": { + "status": "completed", + "date": "2026-02-18", + "updated": "2026-02-18", + "summary": "Fixed cancellation responsiveness for model retries: `/cancel` now aborts model-router retry backoff loops immediately via abort-aware retry execution and router abort signaling, preventing long waits through remaining retries/fallback steps.", + "files_modified": [ + "src/models/retry.ts", + "src/models/retry.test.ts", + "src/models/router.ts", + "src/models/router.test.ts", + "src/backends/native/agent.ts", + "docs/plans/state.json" + ], + "test_status": "pnpm test:run src/models/retry.test.ts src/models/router.test.ts src/backends/native/agent.test.ts + pnpm typecheck passing" } }, "overall_progress": { - "total_test_count": 1927, + "total_test_count": 1930, "all_tests_passing": true, "p0_completion": "3/3 (100%)", "p1_completion": "4/4 (100%)", diff --git a/src/backends/native/agent.ts b/src/backends/native/agent.ts index f5f0137..d6e3638 100644 --- a/src/backends/native/agent.ts +++ b/src/backends/native/agent.ts @@ -100,6 +100,9 @@ export class NativeAgent { async process(userMessage: string, attachments?: Attachment[]): Promise { this._cancelRequested = false; + if ('clearAbort' in this.modelClient && typeof this.modelClient.clearAbort === 'function') { + this.modelClient.clearAbort(); + } this._runInProgress = true; // Detect and strip !!think prefix for per-message thinking mode @@ -541,6 +544,9 @@ export class NativeAgent { cancel(): void { if (this._runInProgress) { this._cancelRequested = true; + if ('requestAbort' in this.modelClient && typeof this.modelClient.requestAbort === 'function') { + this.modelClient.requestAbort(); + } } } diff --git a/src/models/retry.test.ts b/src/models/retry.test.ts index ee958b8..70d506c 100644 --- a/src/models/retry.test.ts +++ b/src/models/retry.test.ts @@ -159,6 +159,34 @@ describe('withRetry', () => { expect(fn).toHaveBeenCalledTimes(1); }); + it('aborts before first attempt when shouldAbort is true', async () => { + const fn = vi.fn().mockResolvedValue('never'); + + await expect( + withRetry(fn, fastConfig, 'abort-test', { shouldAbort: () => true }), + ).rejects.toMatchObject({ name: 'AbortError' }); + expect(fn).not.toHaveBeenCalled(); + }); + + it('aborts during backoff sleep when shouldAbort flips true', async () => { + let abort = false; + const fn = vi.fn().mockRejectedValue(new Error('temporary failure')); + + const run = withRetry( + fn, + { ...fastConfig, maxRetries: 3, initialDelayMs: 80, backoffMultiplier: 1, maxDelayMs: 80 }, + 'abort-backoff-test', + { shouldAbort: () => abort }, + ); + + setTimeout(() => { + abort = true; + }, 10); + + await expect(run).rejects.toMatchObject({ name: 'AbortError' }); + expect(fn).toHaveBeenCalledTimes(1); + }); + it('increases delay exponentially between retries', async () => { const timestamps: number[] = []; const config: RetryConfig = { diff --git a/src/models/retry.ts b/src/models/retry.ts index 5fc2fbc..4312803 100644 --- a/src/models/retry.ts +++ b/src/models/retry.ts @@ -13,6 +13,11 @@ export interface RetryConfig { nonRetryablePatterns: string[]; } +export interface RetryExecutionOptions { + /** Abort retry loop before next attempt and during backoff sleeps. */ + shouldAbort?: () => boolean; +} + export const DEFAULT_RETRY_CONFIG: RetryConfig = { maxRetries: 3, initialDelayMs: 1000, @@ -38,15 +43,34 @@ export async function withRetry( fn: () => Promise, config: RetryConfig = DEFAULT_RETRY_CONFIG, label?: string, + options?: RetryExecutionOptions, ): Promise { let lastError: Error | undefined; + const throwAbort = (): never => { + const error = new Error('Operation cancelled by user.'); + error.name = 'AbortError'; + throw error; + }; + for (let attempt = 0; attempt <= config.maxRetries; attempt++) { + if (options?.shouldAbort?.()) { + throwAbort(); + } + try { return await fn(); } catch (error) { lastError = error instanceof Error ? error : new Error(String(error)); + if (lastError.name === 'AbortError') { + throw lastError; + } + + if (options?.shouldAbort?.()) { + throwAbort(); + } + // Don't retry non-retryable errors if (!isRetryable(lastError, config.nonRetryablePatterns)) { throw lastError; @@ -66,9 +90,55 @@ export async function withRetry( `[retry] ${label ?? 'operation'} attempt ${attempt + 1}/${config.maxRetries} failed: ${lastError.message}. Retrying in ${Math.round(jitter)}ms...`, ); - await new Promise(resolve => setTimeout(resolve, jitter)); + await sleepWithAbort(jitter, options?.shouldAbort); } } throw lastError ?? new Error('Retry failed with no error'); } + +async function sleepWithAbort(delayMs: number, shouldAbort?: () => boolean): Promise { + if (!shouldAbort) { + await new Promise((resolve) => setTimeout(resolve, delayMs)); + return; + } + + await new Promise((resolve, reject) => { + const endAt = Date.now() + delayMs; + const checkIntervalMs = Math.min(100, Math.max(20, Math.floor(delayMs / 5))); + let timeout: NodeJS.Timeout | undefined; + let interval: NodeJS.Timeout | undefined; + + const cleanup = () => { + if (timeout) { + clearTimeout(timeout); + } + if (interval) { + clearInterval(interval); + } + }; + + const rejectAbort = () => { + cleanup(); + const error = new Error('Operation cancelled by user.'); + error.name = 'AbortError'; + reject(error); + }; + + if (shouldAbort()) { + rejectAbort(); + return; + } + + timeout = setTimeout(() => { + cleanup(); + resolve(); + }, Math.max(0, endAt - Date.now())); + + interval = setInterval(() => { + if (shouldAbort()) { + rejectAbort(); + } + }, checkIntervalMs); + }); +} diff --git a/src/models/router.test.ts b/src/models/router.test.ts index cbb281e..cf6aee2 100644 --- a/src/models/router.test.ts +++ b/src/models/router.test.ts @@ -476,6 +476,38 @@ describe('setClient and labels', () => { expect(router.isTierStrict('default')).toBe(true); }); + it('requestAbort interrupts retry loop before fallback chain', async () => { + const primary = { + chat: vi.fn().mockRejectedValue(new Error('temporary failure')), + } as unknown as ModelClient; + const fallback = { + chat: vi.fn().mockResolvedValue({ + content: 'fallback', + stopReason: 'end_turn', + usage: { inputTokens: 1, outputTokens: 1 }, + }), + } as unknown as ModelClient; + + const router = new ModelRouter({ + default: primary, + fallbackChain: [fallback], + retryConfig: { + maxRetries: 3, + initialDelayMs: 80, + backoffMultiplier: 1, + maxDelayMs: 80, + nonRetryablePatterns: [], + }, + }); + + const run = router.chat({ messages: [{ role: 'user', content: 'hi' }] }); + setTimeout(() => router.requestAbort(), 10); + + await expect(run).rejects.toMatchObject({ name: 'AbortError' }); + expect(primary.chat).toHaveBeenCalledTimes(1); + expect(fallback.chat).not.toHaveBeenCalled(); + }); + it('setOnTierChange does not replace existing listeners', () => { const router = new ModelRouter({ default: { chat: vi.fn() } as unknown as ModelClient, diff --git a/src/models/router.ts b/src/models/router.ts index 17fc355..a05d1c7 100644 --- a/src/models/router.ts +++ b/src/models/router.ts @@ -28,6 +28,7 @@ export class ModelRouter implements ModelClient { private retryConfig?: RetryConfig; private tierChangeListeners: Array<(tier: ModelTier) => void> = []; private strictTiers: Set = new Set(); + private abortRequested = false; constructor(config: ModelRouterConfig) { this.clients = new Map(); @@ -89,8 +90,11 @@ export class ModelRouter implements ModelClient { // Try primary client (with retry if configured) try { + this.throwIfAborted(); if (this.retryConfig) { - return await withRetry(() => primaryClient.chat(request), this.retryConfig, 'primary model'); + return await withRetry(() => primaryClient.chat(request), this.retryConfig, 'primary model', { + shouldAbort: () => this.abortRequested, + }); } return await primaryClient.chat(request); } catch (error) { @@ -105,6 +109,7 @@ export class ModelRouter implements ModelClient { // Try tier-specific fallbacks first const tierFallbackList = this.tierFallbacks.get(useTier) ?? []; for (let i = 0; i < tierFallbackList.length; i++) { + this.throwIfAborted(); try { const reason = `Primary model failed (${errors[0].message}), using tier fallback #${i + 1}`; logger.debug(reason); @@ -118,6 +123,7 @@ export class ModelRouter implements ModelClient { // Then try global fallback chain for (let i = 0; i < this.fallbackChain.length; i++) { + this.throwIfAborted(); const fallbackClient = this.fallbackChain[i]; try { const reason = `Primary model failed (${errors[0].message}), using global fallback #${i + 1}`; @@ -238,6 +244,14 @@ export class ModelRouter implements ModelClient { return this.strictTiers.has(tier); } + requestAbort(): void { + this.abortRequested = true; + } + + clearAbort(): void { + this.abortRequested = false; + } + getLabel(tier: ModelTier): string { return this.labels.get(tier) ?? 'unknown'; } @@ -249,4 +263,13 @@ export class ModelRouter implements ModelClient { } return result; } + + private throwIfAborted(): void { + if (!this.abortRequested) { + return; + } + const error = new Error('Operation cancelled by user.'); + error.name = 'AbortError'; + throw error; + } }