smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge PR #4266: fix(config): cache failed !command resolutions, bound dynamic fetch timeout (@metaphorics)

Browse Source
This commit is contained in:
can1357
2026-07-05 13:39:10 +02:00
parent f3e372e7bf da86dc9912
commit e4f05ae182
3 changed files with 94 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/CHANGELOG.md
+3
View File
@@ -206,6 +206,9 @@
### Changed
- Load persisted Agent Hub subagents asynchronously to avoid blocking the TUI on synchronous directory walks ([#4239](https://github.com/can1357/oh-my-pi/issues/4239))
### 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
packages/coding-agent/src/config/model-registry.ts
+43 -2
View File
@@ -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<string>(["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<T>(timeoutMs: number, run: () => Promise<T>): Promise<T> {
const { promise: timeoutPromise, reject: timeoutReject } = Promise.withResolvers<never>();
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;
@@ -240,6 +265,13 @@ interface CustomModelsResult {
}
const commandValueCache = new Map<string, string>();
// 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<string, number>();
function isCommandConfigValue(valueConfig: string | undefined): valueConfig is string {
return valueConfig?.startsWith("!") === true;
@@ -248,13 +280,20 @@ function isCommandConfigValue(valueConfig: string | undefined): valueConfig is s
function resolveCommandConfig(command: string): string | undefined {
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) return undefined;
if (trimmed.length === 0) {
commandFailureRetryAt.set(command, Date.now() + COMMAND_FAILURE_RETRY_MS);
return undefined;
}
commandFailureRetryAt.delete(command);
commandValueCache.set(command, trimmed);
return trimmed;
} catch {
commandFailureRetryAt.set(command, Date.now() + COMMAND_FAILURE_RETRY_MS);
return undefined;
}
}
@@ -2134,7 +2173,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<Api>[] = [];
for (const modelDef of modelDefs) {
const overlay = buildCustomModelOverlay(
packages/coding-agent/test/model-registry-command-values.test.ts
+48
View File
@@ -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<Api> = 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");
});
});