diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 11098023c..7d0344bc8 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,11 @@ ## [Unreleased] +### Fixed + +- Cache failed `!command` config resolutions and time out extension dynamic model fetches after 15s ([#4237](https://github.com/can1357/oh-my-pi/issues/4237)) + + ## [16.3.0] - 2026-07-02 ### Added diff --git a/packages/coding-agent/src/config/model-registry.ts b/packages/coding-agent/src/config/model-registry.ts index 0b5854dbc..d403b7c17 100644 --- a/packages/coding-agent/src/config/model-registry.ts +++ b/packages/coding-agent/src/config/model-registry.ts @@ -48,6 +48,13 @@ const STARTUP_MODEL_CACHE_PROVIDER_IDS: readonly string[] = [ // packages/ai/src/registry/lm-studio.ts, and packages/ai/src/registry/vllm.ts. const LOCAL_PROVIDER_PLACEHOLDERS = new Set(["llama-cpp-local", "lm-studio-local", "vllm-local"]); +/** + * Hard bound for extension-provided fetchDynamicModels to prevent indefinite hangs + * during runtime provider discovery. Uses a cancellable manual timer (not AbortSignal.timeout) + * so a successful fast path does not leave an armed timeout signal for concurrent GC. + */ +const RUNTIME_DYNAMIC_MODEL_FETCH_TIMEOUT_MS = 15_000; + import type { ApiKeyResolver, FetchImpl } from "@oh-my-pi/pi-ai"; import { registerOAuthProvider, unregisterOAuthProviders } from "@oh-my-pi/pi-ai/oauth"; import type { OAuthCredentials, OAuthLoginCallbacks } from "@oh-my-pi/pi-ai/oauth/types"; @@ -81,6 +88,24 @@ function isDiscoveryBearerApiKey(apiKey: string | undefined | null): apiKey is s return isAuthenticated(apiKey) && !LOCAL_PROVIDER_PLACEHOLDERS.has(apiKey); } +/** + * Wraps an extension-provided fetchDynamicModels call with a hard timeout. + * Uses a cancellable manual timer (not AbortSignal.timeout) so that a fast + * successful path does not leave an armed timeout signal for concurrent GC. + * The inner fetcher does not receive a signal (extension contract has none). + */ +async function withRuntimeDynamicModelsTimeout(timeoutMs: number, run: () => Promise): Promise { + const { promise: timeoutPromise, reject: timeoutReject } = Promise.withResolvers(); + const timer = setTimeout(() => { + timeoutReject(new Error(`fetchDynamicModels timed out after ${timeoutMs}ms`)); + }, timeoutMs); + try { + return await Promise.race([run(), timeoutPromise]); + } finally { + clearTimeout(timer); + } +} + /** Provider override config (baseUrl, headers, apiKey, compat, transport) without custom models */ interface ProviderOverride { baseUrl?: string; @@ -239,22 +264,25 @@ interface CustomModelsResult { found: boolean; } -const commandValueCache = new Map(); +const commandValueCache = new Map(); function isCommandConfigValue(valueConfig: string | undefined): valueConfig is string { return valueConfig?.startsWith("!") === true; } function resolveCommandConfig(command: string): string | undefined { - const cached = commandValueCache.get(command); - if (cached !== undefined) return cached; + if (commandValueCache.has(command)) return commandValueCache.get(command); try { const stdout = execSync(command, { encoding: "utf8", timeout: 10_000, windowsHide: true }); const trimmed = stdout.trim(); - if (trimmed.length === 0) return undefined; + if (trimmed.length === 0) { + commandValueCache.set(command, undefined); + return undefined; + } commandValueCache.set(command, trimmed); return trimmed; } catch { + commandValueCache.set(command, undefined); return undefined; } } @@ -2134,7 +2162,9 @@ export class ModelRegistry { fetchDynamicModels: async () => { const apiKey = await this.#peekApiKeyForProvider(providerName); const resolvedKey = isAuthenticated(apiKey) ? apiKey : undefined; - const modelDefs = await fetcher(resolvedKey); + const modelDefs = await withRuntimeDynamicModelsTimeout(RUNTIME_DYNAMIC_MODEL_FETCH_TIMEOUT_MS, () => + fetcher(resolvedKey), + ); const results: Model[] = []; for (const modelDef of modelDefs) { const overlay = buildCustomModelOverlay( diff --git a/packages/coding-agent/test/model-registry-command-values.test.ts b/packages/coding-agent/test/model-registry-command-values.test.ts index 10b8a6ca4..dde67673b 100644 --- a/packages/coding-agent/test/model-registry-command-values.test.ts +++ b/packages/coding-agent/test/model-registry-command-values.test.ts @@ -2,6 +2,8 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; +import type { Api, Model } from "@oh-my-pi/pi-ai/types"; +import { buildModel } from "@oh-my-pi/pi-catalog/build"; import { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry"; import { AuthStorage } from "@oh-my-pi/pi-coding-agent/session/auth-storage"; import { removeSyncWithRetries, Snowflake } from "@oh-my-pi/pi-utils"; @@ -84,4 +86,50 @@ describe("ModelRegistry command-resolved models.yml values", () => { expect(model?.headers?.["X-Model-Key"]).toBe("cmd-model-header"); expect(model?.headers?.Authorization).toBe("Bearer cmd-api-key"); }); + + test("resolveCommandConfig caches failed executions so they do not retry", async () => { + const counterFile = path.join(tempDir, "counter.txt"); + fs.writeFileSync(counterFile, "0"); + + // Command increments a counter and then fails (exit 1). + const trackingCommand = `node -e "const fs=require('fs'); fs.writeFileSync('${counterFile.replace(/\\/g, "/")}', String(Number(fs.readFileSync('${counterFile.replace(/\\/g, "/")}', 'utf8')) + 1)); process.exit(1);"`; + + fs.writeFileSync( + modelsPath, + JSON.stringify({ + providers: { + "custom-proxy": { + baseUrl: "https://custom-proxy.example.com/v1", + api: "openai-completions", + apiKey: `!${trackingCommand}`, + }, + }, + }), + ); + + // Init triggers the first command resolution. + const registry = new ModelRegistry(authStorage, modelsPath); + + const dummyModel: Model = buildModel({ + id: "foo", + name: "foo", + api: "openai-completions", + provider: "custom-proxy", + baseUrl: "a", + reasoning: false, + input: ["text"], + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 }, + contextWindow: 4096, + maxTokens: 1024, + }); + + // Trigger the fallback resolver which also calls resolveConfigValue. + await registry.getApiKey(dummyModel); + + // Another call to ensure it hits cache multiple times. + await registry.getApiKey(dummyModel); + + // The command should have only run once. + expect(fs.readFileSync(counterFile, "utf8")).toBe("1"); + }); });