From 657aaa9c92d38af32d0aa5fc4a724ead1b88c7e4 Mon Sep 17 00:00:00 2001 From: metaphorics <152830360+metaphorics@users.noreply.github.com> Date: Thu, 2 Jul 2026 17:25:49 +0900 Subject: [PATCH 1/2] fix(config): cache failed !command resolutions, bound dynamic fetch timeout resolveCommandConfig used execSync with a 10s timeout and only cached non-empty successful results, so empty/failing/timeout !command values in models.yml re-ran on every API key lookup and blocked the event loop repeatedly. It now probes the cache with has(), stores empty stdout and caught failures as undefined, and returns the cached undefined on later lookups. Extension-registered fetchDynamicModels is now wrapped with a 15s Promise.race timeout so a stuck extension cannot hang model refresh indefinitely. Verified by Bun transpiler syntax check of packages/coding-agent/src/config/model-registry.ts and the model-registry test suites. Closes #4237 --- packages/coding-agent/CHANGELOG.md | 5 ++ .../coding-agent/src/config/model-registry.ts | 40 ++++++++++++++-- .../model-registry-command-values.test.ts | 48 +++++++++++++++++++ 3 files changed, 88 insertions(+), 5 deletions(-) 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"); + }); }); From da86dc991283654c2a5cfd5dcc46234cac98440a Mon Sep 17 00:00:00 2001 From: metaphorics <152830360+metaphorics@users.noreply.github.com> Date: Thu, 2 Jul 2026 17:49:18 +0900 Subject: [PATCH 2/2] fix: address cubic review feedback (iteration 1) TTL-based negative cache (30s) for failed !command config resolutions instead of process-lifetime: transient secret-provider failures recover without restart while still bounding the execSync retry storm to one probe per window. --- .../coding-agent/src/config/model-registry.ts | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/packages/coding-agent/src/config/model-registry.ts b/packages/coding-agent/src/config/model-registry.ts index d403b7c17..9c8c4b6c2 100644 --- a/packages/coding-agent/src/config/model-registry.ts +++ b/packages/coding-agent/src/config/model-registry.ts @@ -264,25 +264,36 @@ interface CustomModelsResult { found: boolean; } -const commandValueCache = new Map(); +const commandValueCache = new Map(); +// Failed `!command` resolutions (non-zero exit, empty stdout) are negative-cached +// with a TTL instead of forever: a transient failure (locked password manager, +// network hiccup) must not disable the key until process restart, but re-running +// the command on every resolution would restore the execSync storm this cache +// exists to prevent. One probe per TTL window bounds both. +const COMMAND_FAILURE_RETRY_MS = 30_000; +const commandFailureRetryAt = new Map(); function isCommandConfigValue(valueConfig: string | undefined): valueConfig is string { return valueConfig?.startsWith("!") === true; } function resolveCommandConfig(command: string): string | undefined { - if (commandValueCache.has(command)) return commandValueCache.get(command); + const cached = commandValueCache.get(command); + if (cached !== undefined) return cached; + const retryAt = commandFailureRetryAt.get(command); + if (retryAt !== undefined && Date.now() < retryAt) return undefined; try { const stdout = execSync(command, { encoding: "utf8", timeout: 10_000, windowsHide: true }); const trimmed = stdout.trim(); if (trimmed.length === 0) { - commandValueCache.set(command, undefined); + commandFailureRetryAt.set(command, Date.now() + COMMAND_FAILURE_RETRY_MS); return undefined; } + commandFailureRetryAt.delete(command); commandValueCache.set(command, trimmed); return trimmed; } catch { - commandValueCache.set(command, undefined); + commandFailureRetryAt.set(command, Date.now() + COMMAND_FAILURE_RETRY_MS); return undefined; } }