diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 64c3c57ae..e4db8e7d2 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -19,6 +19,7 @@ ### Fixed +- Fixed stalled network calls in `omp update`, Hindsight recall, and Smithery registry lookup by adding fetch timeouts. ([#4229](https://github.com/can1357/oh-my-pi/issues/4229)) - Fixed stuttering/latency in speech by running synthesis chunks through the player gaplessly - Fixed race condition causing EPIPE errors and broken pipes during speech playback - Fixed interrupted speech audio by ensuring segments queue and drain in order diff --git a/packages/coding-agent/src/cli/update-cli.test.ts b/packages/coding-agent/src/cli/update-cli.test.ts new file mode 100644 index 000000000..e48b71ceb --- /dev/null +++ b/packages/coding-agent/src/cli/update-cli.test.ts @@ -0,0 +1,28 @@ +import { afterEach, describe, expect, it, vi } from "bun:test"; +import { runUpdateCommand } from "./update-cli"; + +type FetchInput = string | URL | Request; +type FetchInit = RequestInit | BunFetchRequestInit; + +describe("runUpdateCommand fetch cancellation", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("checks release metadata with a timeout signal", async () => { + let requestSignal: AbortSignal | undefined; + vi.spyOn(console, "log").mockImplementation(() => {}); + const fetchStub = Object.assign( + async (_input: FetchInput, init?: FetchInit) => { + requestSignal = init?.signal ?? undefined; + return Response.json({ version: "999.0.0" }); + }, + { preconnect: globalThis.fetch.preconnect }, + ); + vi.spyOn(globalThis, "fetch").mockImplementation(fetchStub); + + await runUpdateCommand({ force: false, check: true }); + + expect(requestSignal).toBeInstanceOf(AbortSignal); + }); +}); diff --git a/packages/coding-agent/src/cli/update-cli.ts b/packages/coding-agent/src/cli/update-cli.ts index 8c21d7d42..b90c00100 100644 --- a/packages/coding-agent/src/cli/update-cli.ts +++ b/packages/coding-agent/src/cli/update-cli.ts @@ -12,6 +12,7 @@ import { $which, APP_NAME, isEnoent, VERSION } from "@oh-my-pi/pi-utils"; import { $ } from "bun"; import chalk from "chalk"; import { theme } from "../modes/theme/theme"; +import { isTimeoutError, withTimeoutSignal } from "../utils/fetch-timeout"; const REPO = "can1357/oh-my-pi"; const PACKAGE = "@oh-my-pi/pi-coding-agent"; @@ -29,6 +30,8 @@ const MISE_TOOL = "github:can1357/oh-my-pi"; * See #1686. */ const NPM_REGISTRY = "https://registry.npmjs.org/"; +const RELEASE_METADATA_TIMEOUT_MS = 30_000; +const BINARY_DOWNLOAD_TIMEOUT_MS = 15 * 60_000; /** * Core native addon package. Bumped in lock-step with {@link PACKAGE} so the @@ -239,7 +242,17 @@ async function resolveUpdateTarget(): Promise { * Uses npm instead of GitHub API to avoid unauthenticated rate limiting. */ async function getLatestRelease(): Promise { - const response = await fetch(`${NPM_REGISTRY}${PACKAGE}/latest`); + let response: Response; + try { + response = await fetch(`${NPM_REGISTRY}${PACKAGE}/latest`, { + signal: withTimeoutSignal(RELEASE_METADATA_TIMEOUT_MS), + }); + } catch (err) { + if (isTimeoutError(err)) { + throw new Error("Timed out fetching release info after 30s", { cause: err }); + } + throw err; + } if (!response.ok) { throw new Error(`Failed to fetch release info: ${response.statusText}`); } @@ -832,7 +845,18 @@ async function updateViaBinaryAt(targetPath: string, expectedVersion: string): P const backupPath = `${targetPath}.${Date.now()}.${process.pid}.bak`; console.log(chalk.dim(`Downloading ${binaryName}…`)); - const response = await fetch(url, { redirect: "follow" }); + let response: Response; + try { + response = await fetch(url, { + redirect: "follow", + signal: withTimeoutSignal(BINARY_DOWNLOAD_TIMEOUT_MS), + }); + } catch (err) { + if (isTimeoutError(err)) { + throw new Error("Timed out downloading release binary after 15 minutes", { cause: err }); + } + throw err; + } if (!response.ok || !response.body) { throw new Error(`Download failed: ${response.statusText}`); } diff --git a/packages/coding-agent/src/hindsight/client.test.ts b/packages/coding-agent/src/hindsight/client.test.ts new file mode 100644 index 000000000..746bbb018 --- /dev/null +++ b/packages/coding-agent/src/hindsight/client.test.ts @@ -0,0 +1,33 @@ +import { afterEach, describe, expect, it, vi } from "bun:test"; +import { HindsightApi } from "./client"; + +type FetchInput = string | URL | Request; +type FetchInit = RequestInit | BunFetchRequestInit; + +describe("HindsightApi fetch cancellation", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("combines caller cancellation with the request timeout", async () => { + let requestSignal: AbortSignal | undefined; + const fetchStub = Object.assign( + async (_input: FetchInput, init?: FetchInit) => { + requestSignal = init?.signal ?? undefined; + return Response.json({ results: [] }); + }, + { preconnect: globalThis.fetch.preconnect }, + ); + vi.spyOn(globalThis, "fetch").mockImplementation(fetchStub); + + const caller = new AbortController(); + const client = new HindsightApi({ baseUrl: "https://hindsight.example" }); + await client.recall("bank", "query", { signal: caller.signal }); + + expect(requestSignal).toBeInstanceOf(AbortSignal); + expect(requestSignal).not.toBe(caller.signal); + caller.abort(new Error("caller aborted")); + expect(requestSignal?.aborted).toBe(true); + expect(requestSignal?.reason).toBe(caller.signal.reason); + }); +}); diff --git a/packages/coding-agent/src/hindsight/client.ts b/packages/coding-agent/src/hindsight/client.ts index 853ae786d..697d2e2b8 100644 --- a/packages/coding-agent/src/hindsight/client.ts +++ b/packages/coding-agent/src/hindsight/client.ts @@ -8,10 +8,12 @@ * tests to spy on. */ +import { isTimeoutError, withTimeoutSignal } from "../utils/fetch-timeout"; import type { HindsightConfig } from "./config"; const USER_AGENT = "oh-my-pi-coding-agent"; const DEFAULT_USER_AGENT = USER_AGENT; +const HINDSIGHT_REQUEST_TIMEOUT_MS = 30_000; export type Budget = "low" | "mid" | "high" | string; export type TagsMatch = "any" | "all" | "any_strict" | "all_strict"; @@ -24,6 +26,11 @@ export interface HindsightApiOptions { userAgent?: string; } +/** Caller cancellation shared by Hindsight request option bags. */ +export interface HindsightRequestOptions { + signal?: AbortSignal; +} + export interface RecallResult { id?: string; text: string; @@ -77,7 +84,7 @@ export interface MemoryItemInput { updateMode?: UpdateMode; } -export interface RetainOptions { +export interface RetainOptions extends HindsightRequestOptions { timestamp?: Date | string; context?: string; metadata?: Record; @@ -87,7 +94,7 @@ export interface RetainOptions { updateMode?: UpdateMode; } -export interface RetainBatchOptions { +export interface RetainBatchOptions extends HindsightRequestOptions { /** Document id applied to every item that doesn't carry its own. */ documentId?: string; /** Tags attached to the resulting document(s), not individual items. */ @@ -95,7 +102,7 @@ export interface RetainBatchOptions { async?: boolean; } -export interface RecallOptions { +export interface RecallOptions extends HindsightRequestOptions { types?: string[]; maxTokens?: number; budget?: Budget; @@ -103,19 +110,19 @@ export interface RecallOptions { tagsMatch?: TagsMatch; } -export interface ReflectOptions { +export interface ReflectOptions extends HindsightRequestOptions { context?: string; budget?: Budget; tags?: string[]; tagsMatch?: TagsMatch; } -export interface CreateBankOptions { +export interface CreateBankOptions extends HindsightRequestOptions { reflectMission?: string; retainMission?: string; } -export interface ListMemoriesOptions { +export interface ListMemoriesOptions extends HindsightRequestOptions { limit?: number; offset?: number; type?: string; @@ -123,12 +130,12 @@ export interface ListMemoriesOptions { consolidationState?: ConsolidationState; } -export interface ListDocumentsOptions { +export interface ListDocumentsOptions extends HindsightRequestOptions { limit?: number; offset?: number; } -export interface UpdateDocumentOptions { +export interface UpdateDocumentOptions extends HindsightRequestOptions { tags?: string[]; } @@ -166,7 +173,7 @@ export interface MentalModelHistoryEntry { [key: string]: unknown; } -export interface CreateMentalModelOptions { +export interface CreateMentalModelOptions extends HindsightRequestOptions { id?: string; tags?: string[]; maxTokens?: number; @@ -183,11 +190,11 @@ export interface RefreshMentalModelResponse { [key: string]: unknown; } -export interface ListMentalModelsOptions { +export interface ListMentalModelsOptions extends HindsightRequestOptions { detail?: MentalModelDetail; } -export interface GetMentalModelOptions { +export interface GetMentalModelOptions extends HindsightRequestOptions { detail?: MentalModelDetail; } @@ -208,6 +215,7 @@ interface RequestOptions { query?: Record; /** Return null instead of throwing on a 404 response. */ allow404?: boolean; + signal?: AbortSignal; } export class HindsightApi { @@ -240,7 +248,10 @@ export class HindsightApi { "POST", `/v1/default/banks/${encodeURIComponent(bankId)}/memories`, "retain", - { body: { items: [item], async: options?.async } }, + { + body: { items: [item], async: options?.async }, + signal: options?.signal, + }, ); } @@ -270,6 +281,7 @@ export class HindsightApi { document_tags: options?.documentTags, async: options?.async, }, + signal: options?.signal, }, ); } @@ -288,6 +300,7 @@ export class HindsightApi { tags: options?.tags, tags_match: options?.tagsMatch, }, + signal: options?.signal, }, ); } @@ -305,6 +318,7 @@ export class HindsightApi { tags: options?.tags, tags_match: options?.tagsMatch, }, + signal: options?.signal, }, ); } @@ -319,6 +333,7 @@ export class HindsightApi { reflect_mission: options.reflectMission, retain_mission: options.retainMission, }, + signal: options.signal, }, ); } @@ -340,6 +355,7 @@ export class HindsightApi { limit: options?.limit, offset: options?.offset, }, + signal: options?.signal, }, ); } @@ -350,7 +366,7 @@ export class HindsightApi { "GET", `/v1/default/banks/${encodeURIComponent(bankId)}/documents`, "listDocuments", - { query: { limit: options?.limit, offset: options?.offset } }, + { query: { limit: options?.limit, offset: options?.offset }, signal: options?.signal }, ); } @@ -370,7 +386,7 @@ export class HindsightApi { "PATCH", `/v1/default/banks/${encodeURIComponent(bankId)}/documents/${encodeURIComponent(documentId)}`, "updateDocument", - { body: { tags: options.tags } }, + { body: { tags: options.tags }, signal: options.signal }, ); } @@ -399,7 +415,7 @@ export class HindsightApi { "GET", `/v1/default/banks/${encodeURIComponent(bankId)}/mental-models`, "listMentalModels", - { query: { detail: options?.detail ?? "content" } }, + { query: { detail: options?.detail ?? "content" }, signal: options?.signal }, ); } @@ -413,7 +429,7 @@ export class HindsightApi { "GET", `/v1/default/banks/${encodeURIComponent(bankId)}/mental-models/${encodeURIComponent(mentalModelId)}`, "getMentalModel", - { query: { detail: options?.detail ?? "content" }, allow404: true }, + { query: { detail: options?.detail ?? "content" }, allow404: true, signal: options?.signal }, ); } @@ -441,6 +457,7 @@ export class HindsightApi { max_tokens: options?.maxTokens, trigger: options?.trigger, }, + signal: options?.signal, }, ); } @@ -489,7 +506,11 @@ export class HindsightApi { if (qs) url += `?${qs}`; } - const init: RequestInit = { method, headers: this.#headers }; + const init: RequestInit = { + method, + headers: this.#headers, + signal: withTimeoutSignal(HINDSIGHT_REQUEST_TIMEOUT_MS, opts?.signal), + }; if (opts?.body !== undefined) { init.body = JSON.stringify(pruneUndefined(opts.body)); } @@ -498,11 +519,10 @@ export class HindsightApi { try { response = await fetch(url, init); } catch (err) { - throw new HindsightError( - `${operation} request failed: ${err instanceof Error ? err.message : String(err)}`, - undefined, - err, - ); + const message = isTimeoutError(err) + ? `${operation} request timed out after 30s` + : `${operation} request failed: ${err instanceof Error ? err.message : String(err)}`; + throw new HindsightError(message, undefined, err); } if (opts?.allow404 && response.status === 404) { diff --git a/packages/coding-agent/src/mcp/smithery-registry.test.ts b/packages/coding-agent/src/mcp/smithery-registry.test.ts new file mode 100644 index 000000000..2ac607157 --- /dev/null +++ b/packages/coding-agent/src/mcp/smithery-registry.test.ts @@ -0,0 +1,51 @@ +import { afterEach, describe, expect, it, vi } from "bun:test"; +import { searchSmitheryRegistry } from "./smithery-registry"; + +type FetchInput = string | URL | Request; +type FetchInit = RequestInit | BunFetchRequestInit; + +describe("searchSmitheryRegistry fetch cancellation", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("adds timeout signals to search and detail requests", async () => { + const signals: AbortSignal[] = []; + const fetchStub = Object.assign( + async (input: FetchInput, init?: FetchInit) => { + if (init?.signal instanceof AbortSignal) signals.push(init.signal); + const url = String(input); + if (url.includes("?")) { + return Response.json({ + servers: [ + { + id: "srv_1", + namespace: "smithery-ai", + slug: "filesystem", + qualifiedName: "@smithery-ai/filesystem", + displayName: "Filesystem", + description: "File access", + useCount: 1, + }, + ], + }); + } + return Response.json({ + qualifiedName: "@smithery-ai/filesystem", + displayName: "Filesystem", + description: "File access", + connections: [{ type: "http", deploymentUrl: "https://mcp.example" }], + tools: [], + }); + }, + { preconnect: globalThis.fetch.preconnect }, + ); + vi.spyOn(globalThis, "fetch").mockImplementation(fetchStub); + + const results = await searchSmitheryRegistry("filesystem", { limit: 1 }); + + expect(results[0]?.name).toBe("smithery-ai/filesystem"); + expect(signals).toHaveLength(2); + expect(signals.every(signal => signal instanceof AbortSignal)).toBe(true); + }); +}); diff --git a/packages/coding-agent/src/mcp/smithery-registry.ts b/packages/coding-agent/src/mcp/smithery-registry.ts index c923fdb92..4b7861de6 100644 --- a/packages/coding-agent/src/mcp/smithery-registry.ts +++ b/packages/coding-agent/src/mcp/smithery-registry.ts @@ -1,7 +1,9 @@ import { logger } from "@oh-my-pi/pi-utils"; +import { isTimeoutError, withTimeoutSignal } from "../utils/fetch-timeout"; import type { MCPServerConfig } from "./types"; const SMITHERY_REGISTRY_BASE_URL = "https://registry.smithery.ai"; +const SMITHERY_REGISTRY_TIMEOUT_MS = 10_000; type SmitherySearchEntry = { id?: string; @@ -106,6 +108,7 @@ export interface SmitherySearchOptions { limit?: number; apiKey?: string; includeSemantic?: boolean; + signal?: AbortSignal; } export class SmitheryRegistryError extends Error { @@ -307,13 +310,17 @@ function createConfig( }; } -async function fetchServerDetails(path: string, options?: { apiKey?: string }): Promise { +async function fetchServerDetails( + path: string, + options?: { apiKey?: string; signal?: AbortSignal }, +): Promise { const headers = new Headers(); if (options?.apiKey) { headers.set("Authorization", `Bearer ${options.apiKey}`); } const response = await fetch(`${SMITHERY_REGISTRY_BASE_URL}/servers/${path}`, { headers, + signal: withTimeoutSignal(SMITHERY_REGISTRY_TIMEOUT_MS, options?.signal), }); if (!response.ok) return null; return (await response.json()) as SmitheryServerDetails; @@ -411,7 +418,18 @@ export async function searchSmitheryRegistry( url.searchParams.set("q", query); url.searchParams.set("pageSize", String(pageSize)); if (page > 1) url.searchParams.set("page", String(page)); - const response = await fetch(url.toString(), { headers }); + let response: Response; + try { + response = await fetch(url.toString(), { + headers, + signal: withTimeoutSignal(SMITHERY_REGISTRY_TIMEOUT_MS, options?.signal), + }); + } catch (err) { + if (isTimeoutError(err)) { + throw new SmitheryRegistryError("Smithery search timed out after 10s", 0); + } + throw err; + } if (!response.ok) { throw new SmitheryRegistryError(`Smithery search failed with status ${response.status}`, response.status); } diff --git a/packages/coding-agent/src/utils/fetch-timeout.ts b/packages/coding-agent/src/utils/fetch-timeout.ts new file mode 100644 index 000000000..784b5c2f5 --- /dev/null +++ b/packages/coding-agent/src/utils/fetch-timeout.ts @@ -0,0 +1,10 @@ +/** Create an abort signal that fires after a timeout and preserves caller cancellation. */ +export function withTimeoutSignal(timeoutMs: number, signal?: AbortSignal): AbortSignal { + const timeoutSignal = AbortSignal.timeout(timeoutMs); + return signal ? AbortSignal.any([signal, timeoutSignal]) : timeoutSignal; +} + +/** Detect a timeout raised by an abortable fetch. */ +export function isTimeoutError(error: unknown): boolean { + return error instanceof Error && error.name === "TimeoutError"; +}