Merge remote-tracking branch 'origin/farm/1f41837c/timeout-bare-fetches'

packages/coding-agent/CHANGELOG.md

@@ -100,6 +100,7 @@
 
 - Fixed `models.yml` schema validation failures being treated as valid config data, so invalid custom provider files now surface a warning instead of silently dropping all custom providers. ([#4305](https://github.com/can1357/oh-my-pi/issues/4305))
 
+- 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

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);
+	});
+});

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
@@ -240,7 +243,17 @@ async function resolveUpdateTarget(): Promise<UpdateTarget> {
  * Uses npm instead of GitHub API to avoid unauthenticated rate limiting.
  */
 async function getLatestRelease(): Promise<ReleaseInfo> {
-	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}`);
 	}
@@ -833,7 +846,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}`);
 	}

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);
+	});
+});

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<string, string>;
@@ -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<string, unknown>;
 	/** 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) {

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);
+	});
+});

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<SmitheryServerDetails | null> {
+async function fetchServerDetails(
+	path: string,
+	options?: { apiKey?: string; signal?: AbortSignal },
+): Promise<SmitheryServerDetails | null> {
 	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);
 		}

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";
+}