Merge PR #4168: fix(inspect-image): bounded per-request timeout on the vision-model call (@roboomp)

# Conflicts: # packages/coding-agent/src/config/settings-schema.ts
2026-07-27 04:59:19 +02:00
parent c4a1fabb8f d2d5e40bb2
commit e2ed58d4d5
4 changed files with 160 additions and 22 deletions
@@ -1462,6 +1462,14 @@
 - Fixed `models.yml` remote compaction schema support for V2 streaming endpoint fields. ([#4146](https://github.com/can1357/oh-my-pi/issues/4146))
 - Fixed the SSH tool to reject `cwd` values of `~` and `~/...` before sending guaranteed-bad quoted tilde paths to remote POSIX shells. ([#4002](https://github.com/can1357/oh-my-pi/issues/4002))

+### Added
+
+- Added a configurable per-request timeout for the `inspect_image` tool (`inspect_image.timeoutMs`, default 5 minutes; set to 0 to disable) so a stalled vision-model provider fails fast with a clear error instead of blocking until manual abort ([#4165](https://github.com/can1357/oh-my-pi/issues/4165)).
+
+### Fixed
+
+- Fixed `inspect_image` blocking indefinitely when the vision-model API stalls by combining the caller's abort signal with an `AbortSignal.timeout()` and surfacing a distinct timeout `ToolError` (separate from user-triggered abort) ([#4165](https://github.com/can1357/oh-my-pi/issues/4165)).
+
 ## [16.2.12] - 2026-07-01

 ### Breaking Changes
@@ -3905,6 +3905,25 @@ export const SETTINGS_SCHEMA = {
 		},
 	},

+	"inspect_image.timeoutMs": {
+		type: "number",
+		default: 300_000,
+		ui: {
+			tab: "tools",
+			group: "Execution",
+			label: "Inspect Image Timeout",
+			description:
+				"Per-request timeout for the inspect_image vision-model call, in milliseconds. A stalled provider fails fast with a timeout error instead of blocking until manual abort. Set to 0 to disable the timeout.",
+			options: [
+				{ value: "0", label: "Disabled" },
+				{ value: "60000", label: "1 minute" },
+				{ value: "120000", label: "2 minutes" },
+				{ value: "180000", label: "3 minutes" },
+				{ value: "300000", label: "5 minutes" },
+			],
+		},
+	},
+
 	"checkpoint.enabled": {
 		type: "boolean",
 		default: false,
@@ -1,6 +1,13 @@
 import type { AgentTool, AgentToolContext, AgentToolResult, AgentToolUpdateCallback } from "@oh-my-pi/pi-agent-core";
 import { instrumentedCompleteSimple, resolveTelemetry } from "@oh-my-pi/pi-agent-core";
-import { type Api, completeSimple, type ImageContent, type Model, type ToolExample } from "@oh-my-pi/pi-ai";
+import {
+	type Api,
+	type AssistantMessage,
+	completeSimple,
+	type ImageContent,
+	type Model,
+	type ToolExample,
+} from "@oh-my-pi/pi-ai";
 import { prompt } from "@oh-my-pi/pi-utils";
 import { type } from "arktype";
 import { extractTextContent } from "../commit/utils";
@@ -212,32 +219,55 @@ export class InspectImageTool implements AgentTool<typeof inspectImageSchema, In
 		}

 		const telemetry = resolveTelemetry(this.session.getTelemetry?.(), this.session.getSessionId?.() ?? undefined);
-		const response = await instrumentedCompleteSimple(
-			model,
-			{
-				systemPrompt: [prompt.render(inspectImageSystemPromptTemplate)],
-				messages: [
-					{
-						role: "user",
-						content: [
-							{ type: "image", data: imageInput.data, mimeType: imageInput.mimeType },
-							{ type: "text", text: params.question },
-						],
-						timestamp: Date.now(),
-					},
-				],
-			},
-			{
-				apiKey: modelRegistry.resolver(model, this.session.getSessionId?.() ?? undefined),
-				signal,
-			},
-			{ telemetry, oneshotKind: "inspect_image", completeImpl: this.completeImageRequest },
-		);
+		const timeoutMs = this.session.settings.get("inspect_image.timeoutMs");
+		const hasTimeout = typeof timeoutMs === "number" && Number.isFinite(timeoutMs) && timeoutMs > 0;
+		const timeoutSignal = hasTimeout ? AbortSignal.timeout(timeoutMs) : undefined;
+		const effectiveSignal = timeoutSignal
+			? signal
+				? AbortSignal.any([signal, timeoutSignal])
+				: timeoutSignal
+			: signal;
+		const timedOut = (): boolean => Boolean(timeoutSignal?.aborted) && !signal?.aborted;
+		const formatTimeoutMessage = (): string => {
+			const seconds = timeoutMs % 1000 === 0 ? `${timeoutMs / 1000}` : (timeoutMs / 1000).toFixed(1);
+			return `inspect_image request timed out after ${seconds}s. Increase inspect_image.timeoutMs (currently ${timeoutMs}ms; 0 disables) or check the vision model provider.`;
+		};
+
+		let response: AssistantMessage;
+		try {
+			response = await instrumentedCompleteSimple(
+				model,
+				{
+					systemPrompt: [prompt.render(inspectImageSystemPromptTemplate)],
+					messages: [
+						{
+							role: "user",
+							content: [
+								{ type: "image", data: imageInput.data, mimeType: imageInput.mimeType },
+								{ type: "text", text: params.question },
+							],
+							timestamp: Date.now(),
+						},
+					],
+				},
+				{
+					apiKey: modelRegistry.resolver(model, this.session.getSessionId?.() ?? undefined),
+					signal: effectiveSignal,
+				},
+				{ telemetry, oneshotKind: "inspect_image", completeImpl: this.completeImageRequest },
+			);
+		} catch (error) {
+			if (error instanceof Error && (error.name === "AbortError" || error.name === "TimeoutError")) {
+				if (timedOut()) throw new ToolError(formatTimeoutMessage());
+			}
+			throw error;
+		}

 		if (response.stopReason === "error") {
 			throw new ToolError(response.errorMessage ?? "inspect_image request failed.");
 		}
 		if (response.stopReason === "aborted") {
+			if (timedOut()) throw new ToolError(formatTimeoutMessage());
 			throw new ToolError("inspect_image request aborted.");
 		}

@@ -121,6 +121,39 @@ function createCompleteSimpleForbiddenStub(): CompleteSimpleStub {
 	return { calls, fn };
 }

+function createCompleteSimpleHangingStub(): CompleteSimpleStub {
+	const calls: unknown[][] = [];
+	const fn = (async (...args: unknown[]) => {
+		calls.push(args);
+		const options = args[2] as { signal?: AbortSignal } | undefined;
+		const stubSignal = options?.signal;
+		await new Promise<void>(resolve => {
+			if (!stubSignal) return;
+			if (stubSignal.aborted) return resolve();
+			stubSignal.addEventListener("abort", () => resolve(), { once: true });
+		});
+		return {
+			role: "assistant",
+			api: visionModel.api,
+			provider: visionModel.provider,
+			model: visionModel.id,
+			usage: {
+				input: 0,
+				output: 0,
+				cacheRead: 0,
+				cacheWrite: 0,
+				totalTokens: 0,
+				cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 },
+			},
+			stopReason: "aborted",
+			timestamp: Date.now(),
+			content: [],
+		};
+	}) as unknown as typeof completeSimple;
+
+	return { calls, fn };
+}
+
 describe("InspectImageTool", () => {
 	let testDir: string;

@@ -401,4 +434,52 @@ describe("InspectImageTool", () => {
 		);
 		expect(stub.calls).toHaveLength(0);
 	});
+
+	it("times out with a configured error when the vision-model call stalls", async () => {
+		const imagePath = path.join(testDir, "screen.png");
+		fs.writeFileSync(imagePath, Buffer.from(TINY_PNG_BASE64, "base64"));
+
+		const stub = createCompleteSimpleHangingStub();
+		const settings = Settings.isolated({ "inspect_image.timeoutMs": 50 });
+		const tool = new InspectImageTool(createSession(testDir, visionModel, "test-key", settings), stub.fn);
+
+		const start = Date.now();
+		await expect(tool.execute("call-timeout", { path: imagePath, question: "Anything?" })).rejects.toThrow(
+			/inspect_image request timed out.*inspect_image\.timeoutMs.*50ms/,
+		);
+		const elapsed = Date.now() - start;
+		expect(elapsed).toBeLessThan(5000);
+		expect(stub.calls).toHaveLength(1);
+	});
+
+	it("surfaces manual abort as aborted, not as timed out", async () => {
+		const imagePath = path.join(testDir, "screen.png");
+		fs.writeFileSync(imagePath, Buffer.from(TINY_PNG_BASE64, "base64"));
+
+		const stub = createCompleteSimpleHangingStub();
+		const settings = Settings.isolated({ "inspect_image.timeoutMs": 60_000 });
+		const tool = new InspectImageTool(createSession(testDir, visionModel, "test-key", settings), stub.fn);
+		const controller = new AbortController();
+
+		const pending = tool.execute("call-manual-abort", { path: imagePath, question: "Anything?" }, controller.signal);
+		setTimeout(() => controller.abort(), 25);
+		await expect(pending).rejects.toThrow(/inspect_image request aborted/);
+		await expect(pending).rejects.not.toThrow(/timed out/);
+		expect(stub.calls).toHaveLength(1);
+	});
+
+	it("skips the timeout guard when inspect_image.timeoutMs is zero", async () => {
+		const imagePath = path.join(testDir, "screen.png");
+		fs.writeFileSync(imagePath, Buffer.from(TINY_PNG_BASE64, "base64"));
+
+		const stub = createCompleteSimpleSuccessStub("Timeout disabled path");
+		const settings = Settings.isolated({ "inspect_image.timeoutMs": 0 });
+		const tool = new InspectImageTool(createSession(testDir, visionModel, "test-key", settings), stub.fn);
+
+		const result = await tool.execute("call-timeout-disabled", { path: imagePath, question: "Anything?" });
+		expect(result.content).toEqual([{ type: "text", text: "Timeout disabled path" }]);
+		expect(stub.calls).toHaveLength(1);
+		const passed = stub.calls[0]?.[2] as { signal?: AbortSignal } | undefined;
+		expect(passed?.signal).toBeUndefined();
+	});
 });