From 050aec763215d441f65adc4f1534431e879da941 Mon Sep 17 00:00:00 2001 From: roboomp Date: Tue, 21 Jul 2026 21:25:42 +0000 Subject: [PATCH] fix(coding-agent): make hindsight request timeouts per-op and configurable The single 30s hang-guard from #4290 (sized for metadata fetches per #4229) wrapped every Hindsight op including reflect, an agentic retrieve+synthesize call whose healthy latency routinely exceeds 30s, so ordinary successful reflects were aborted client-side. - Give HindsightApi per-op deadlines with sensible defaults (reflect 120s, retain 60s, recall/request 30s) plus a HindsightTimeouts option bag. - Report the effective seconds in the timeout error instead of a hard-coded "after 30s". - Add hindsight.requestTimeoutMs / reflectTimeoutMs / recallTimeoutMs / retainTimeoutMs settings and matching HINDSIGHT_*_TIMEOUT_MS env vars. - Preserve caller-abort merging via withTimeoutSignal. Fixes #6125 --- packages/coding-agent/CHANGELOG.md | 8 ++++ .../src/config/settings-schema.ts | 5 +++ .../coding-agent/src/hindsight/client.test.ts | 42 ++++++++++++++++++ packages/coding-agent/src/hindsight/client.ts | 43 +++++++++++++++++-- packages/coding-agent/src/hindsight/config.ts | 18 ++++++++ .../src/prompts/system/workflow-notice.md | 6 +-- .../coding-agent/test/hindsight-bank.test.ts | 4 ++ .../coding-agent/test/memory-tools.test.ts | 4 ++ 8 files changed, 122 insertions(+), 8 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 6f32dc885..1e93106af 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,14 @@ ## [Unreleased] +### Added + +- Added configurable Hindsight client request deadlines via `hindsight.requestTimeoutMs` / `reflectTimeoutMs` / `recallTimeoutMs` / `retainTimeoutMs` settings (and matching `HINDSIGHT_*_TIMEOUT_MS` env vars). + +### Fixed + +- Fixed the single 30s Hindsight client timeout aborting healthy `reflect` calls, which are agentic retrieve+synthesize ops that routinely exceed 30s; each op now has its own deadline (reflect defaults to 120s) and the timeout error reports the effective seconds ([#6125](https://github.com/can1357/oh-my-pi/issues/6125)). + ## [17.0.5] - 2026-07-18 ### Added diff --git a/packages/coding-agent/src/config/settings-schema.ts b/packages/coding-agent/src/config/settings-schema.ts index 23b0e6800..2fb6c85e4 100644 --- a/packages/coding-agent/src/config/settings-schema.ts +++ b/packages/coding-agent/src/config/settings-schema.ts @@ -2861,6 +2861,11 @@ export const SETTINGS_SCHEMA = { "hindsight.debug": { type: "boolean", default: false }, + "hindsight.requestTimeoutMs": { type: "number", default: 30_000 }, + "hindsight.reflectTimeoutMs": { type: "number", default: 120_000 }, + "hindsight.recallTimeoutMs": { type: "number", default: 30_000 }, + "hindsight.retainTimeoutMs": { type: "number", default: 60_000 }, + "hindsight.mentalModelsEnabled": { type: "boolean", default: true, diff --git a/packages/coding-agent/src/hindsight/client.test.ts b/packages/coding-agent/src/hindsight/client.test.ts index 746bbb018..05fcef89b 100644 --- a/packages/coding-agent/src/hindsight/client.test.ts +++ b/packages/coding-agent/src/hindsight/client.test.ts @@ -31,3 +31,45 @@ describe("HindsightApi fetch cancellation", () => { expect(requestSignal?.reason).toBe(caller.signal.reason); }); }); + +describe("HindsightApi per-op timeouts", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("reports the effective per-op deadline in the timeout error", async () => { + vi.spyOn(globalThis, "fetch").mockImplementation( + Object.assign( + async (): Promise => { + throw new DOMException("The operation timed out.", "TimeoutError"); + }, + { preconnect: globalThis.fetch.preconnect }, + ), + ); + + const client = new HindsightApi({ + baseUrl: "https://hindsight.example", + timeouts: { reflect: 90_000, recall: 15_000 }, + }); + + await expect(client.reflect("bank", "q")).rejects.toThrow("reflect request timed out after 90s"); + await expect(client.recall("bank", "q")).rejects.toThrow("recall request timed out after 15s"); + }); + + it("falls back to the client request default for ops without an override", async () => { + vi.spyOn(globalThis, "fetch").mockImplementation( + Object.assign( + async (): Promise => { + throw new DOMException("The operation timed out.", "TimeoutError"); + }, + { preconnect: globalThis.fetch.preconnect }, + ), + ); + const client = new HindsightApi({ + baseUrl: "https://hindsight.example", + timeouts: { request: 45_000 }, + }); + + await expect(client.createBank("bank")).rejects.toThrow("createBank request timed out after 45s"); + }); +}); diff --git a/packages/coding-agent/src/hindsight/client.ts b/packages/coding-agent/src/hindsight/client.ts index 697d2e2b8..b4e0feb72 100644 --- a/packages/coding-agent/src/hindsight/client.ts +++ b/packages/coding-agent/src/hindsight/client.ts @@ -13,17 +13,32 @@ 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; +/** Fallback deadlines (ms) applied when the caller supplies no override. */ +const DEFAULT_REQUEST_TIMEOUT_MS = 30_000; +const DEFAULT_REFLECT_TIMEOUT_MS = 120_000; +const DEFAULT_RECALL_TIMEOUT_MS = 30_000; +const DEFAULT_RETAIN_TIMEOUT_MS = 60_000; export type Budget = "low" | "mid" | "high" | string; export type TagsMatch = "any" | "all" | "any_strict" | "all_strict"; export type UpdateMode = "replace" | "append"; export type ConsolidationState = "failed" | "pending" | "done"; +/** Per-operation request deadlines (ms). Each falls back to a built-in default. */ +export interface HindsightTimeouts { + /** Default deadline for ops without a specific override. */ + request?: number; + reflect?: number; + recall?: number; + retain?: number; +} + export interface HindsightApiOptions { baseUrl: string; apiKey?: string; userAgent?: string; + /** Per-op deadlines; unset entries fall back to built-in defaults. */ + timeouts?: HindsightTimeouts; } /** Caller cancellation shared by Hindsight request option bags. */ @@ -216,11 +231,17 @@ interface RequestOptions { /** Return null instead of throwing on a 404 response. */ allow404?: boolean; signal?: AbortSignal; + /** Op deadline (ms); defaults to the client's request timeout. */ + timeoutMs?: number; } export class HindsightApi { #baseUrl: string; #headers: Record; + #reflectTimeoutMs: number; + #recallTimeoutMs: number; + #retainTimeoutMs: number; + #requestTimeoutMs: number; constructor(options: HindsightApiOptions) { this.#baseUrl = options.baseUrl.replace(/\/+$/, ""); @@ -231,6 +252,11 @@ export class HindsightApi { if (options.apiKey) { this.#headers.Authorization = `Bearer ${options.apiKey}`; } + const timeouts = options.timeouts; + this.#requestTimeoutMs = timeouts?.request ?? DEFAULT_REQUEST_TIMEOUT_MS; + this.#reflectTimeoutMs = timeouts?.reflect ?? DEFAULT_REFLECT_TIMEOUT_MS; + this.#recallTimeoutMs = timeouts?.recall ?? DEFAULT_RECALL_TIMEOUT_MS; + this.#retainTimeoutMs = timeouts?.retain ?? DEFAULT_RETAIN_TIMEOUT_MS; } async retain(bankId: string, content: string, options?: RetainOptions): Promise { @@ -251,6 +277,7 @@ export class HindsightApi { { body: { items: [item], async: options?.async }, signal: options?.signal, + timeoutMs: this.#retainTimeoutMs, }, ); } @@ -282,6 +309,7 @@ export class HindsightApi { async: options?.async, }, signal: options?.signal, + timeoutMs: this.#retainTimeoutMs, }, ); } @@ -301,6 +329,7 @@ export class HindsightApi { tags_match: options?.tagsMatch, }, signal: options?.signal, + timeoutMs: this.#recallTimeoutMs, }, ); } @@ -319,6 +348,7 @@ export class HindsightApi { tags_match: options?.tagsMatch, }, signal: options?.signal, + timeoutMs: this.#reflectTimeoutMs, }, ); } @@ -506,10 +536,11 @@ export class HindsightApi { if (qs) url += `?${qs}`; } + const timeoutMs = opts?.timeoutMs ?? this.#requestTimeoutMs; const init: RequestInit = { method, headers: this.#headers, - signal: withTimeoutSignal(HINDSIGHT_REQUEST_TIMEOUT_MS, opts?.signal), + signal: withTimeoutSignal(timeoutMs, opts?.signal), }; if (opts?.body !== undefined) { init.body = JSON.stringify(pruneUndefined(opts.body)); @@ -520,7 +551,7 @@ export class HindsightApi { response = await fetch(url, init); } catch (err) { const message = isTimeoutError(err) - ? `${operation} request timed out after 30s` + ? `${operation} request timed out after ${Math.round(timeoutMs / 1000)}s` : `${operation} request failed: ${err instanceof Error ? err.message : String(err)}`; throw new HindsightError(message, undefined, err); } @@ -639,5 +670,11 @@ export function createHindsightClient(config: HindsightConfig & { hindsightApiUr baseUrl: config.hindsightApiUrl, apiKey: config.hindsightApiToken ?? undefined, userAgent: USER_AGENT, + timeouts: { + request: config.requestTimeoutMs, + reflect: config.reflectTimeoutMs, + recall: config.recallTimeoutMs, + retain: config.retainTimeoutMs, + }, }); } diff --git a/packages/coding-agent/src/hindsight/config.ts b/packages/coding-agent/src/hindsight/config.ts index 05451443b..96ece2a2f 100644 --- a/packages/coding-agent/src/hindsight/config.ts +++ b/packages/coding-agent/src/hindsight/config.ts @@ -42,6 +42,15 @@ export interface HindsightConfig { debug: boolean; + /** Default per-request client deadline (ms) for ops without a specific override. */ + requestTimeoutMs: number; + /** Client deadline (ms) for reflect (agentic synthesis; costlier than a metadata fetch). */ + reflectTimeoutMs: number; + /** Client deadline (ms) for recall. */ + recallTimeoutMs: number; + /** Client deadline (ms) for retain / retainBatch. */ + retainTimeoutMs: number; + mentalModelsEnabled: boolean; mentalModelAutoSeed: boolean; mentalModelRefreshIntervalMs: number; @@ -115,6 +124,10 @@ export function loadHindsightConfig(settings: Settings, env: NodeJS.ProcessEnv = const recallContextTurnsEnv = envInt(env.HINDSIGHT_RECALL_CONTEXT_TURNS); const recallMaxQueryCharsEnv = envInt(env.HINDSIGHT_RECALL_MAX_QUERY_CHARS); const retainEveryNTurnsEnv = envInt(env.HINDSIGHT_RETAIN_EVERY_N_TURNS); + const requestTimeoutMsEnv = envInt(env.HINDSIGHT_REQUEST_TIMEOUT_MS); + const reflectTimeoutMsEnv = envInt(env.HINDSIGHT_REFLECT_TIMEOUT_MS); + const recallTimeoutMsEnv = envInt(env.HINDSIGHT_RECALL_TIMEOUT_MS); + const retainTimeoutMsEnv = envInt(env.HINDSIGHT_RETAIN_TIMEOUT_MS); // Read from settings (each falls back to its schema default). const settingsRetainMode = pickRetainMode(settings.get("hindsight.retainMode")); @@ -158,6 +171,11 @@ export function loadHindsightConfig(settings: Settings, env: NodeJS.ProcessEnv = debug: debugEnv ?? settings.get("hindsight.debug"), + requestTimeoutMs: requestTimeoutMsEnv ?? settings.get("hindsight.requestTimeoutMs"), + reflectTimeoutMs: reflectTimeoutMsEnv ?? settings.get("hindsight.reflectTimeoutMs"), + recallTimeoutMs: recallTimeoutMsEnv ?? settings.get("hindsight.recallTimeoutMs"), + retainTimeoutMs: retainTimeoutMsEnv ?? settings.get("hindsight.retainTimeoutMs"), + mentalModelsEnabled: settings.get("hindsight.mentalModelsEnabled"), mentalModelAutoSeed: settings.get("hindsight.mentalModelAutoSeed"), mentalModelRefreshIntervalMs: settings.get("hindsight.mentalModelRefreshIntervalMs"), diff --git a/packages/coding-agent/src/prompts/system/workflow-notice.md b/packages/coding-agent/src/prompts/system/workflow-notice.md index d15ad9127..09a849bef 100644 --- a/packages/coding-agent/src/prompts/system/workflow-notice.md +++ b/packages/coding-agent/src/prompts/system/workflow-notice.md @@ -47,7 +47,7 @@ For independent per-item chains (review → verify, fetch → extract → score) schema: FINDINGS_SCHEMA, }); return await parallel(found.findings.map((f) => async () => ({ - ...f, + …f, verdict: await agent( `Refute if you can (default refuted when unsure): ${f.title}`, { label: `verify:${f.file}`, schema: VERDICT_SCHEMA }, @@ -57,8 +57,6 @@ For independent per-item chains (review → verify, fetch → extract → score) phase("Review"); const results = await parallel(DIMENSIONS.map((d) => async () => reviewAndVerify(d))); const confirmed = results.flat().filter((f) => f.verdict.is_real); - - Reach for `pipeline()` only when a stage genuinely needs ALL of the previous stage first — dedup/merge across the whole set, early-exit on zero, or "compare against the other findings" — because its inter-stage barrier makes every item wait for the slowest peer: **Python (`eval`, Python backend):** @@ -80,8 +78,6 @@ Reach for `pipeline()` only when a stage genuinely needs ALL of the previous sta const verdicts = await parallel(findings.map((f) => async () => await agent(verifyPrompt(f), { schema: VERDICT_SCHEMA }), )); - - Use ordinary code between calls to flatten/map/filter; don't add a barrier just for that. Nested `parallel()` pools each cap independently, so keep total fan-out sane. diff --git a/packages/coding-agent/test/hindsight-bank.test.ts b/packages/coding-agent/test/hindsight-bank.test.ts index 952bbee3d..7ffdfc30c 100644 --- a/packages/coding-agent/test/hindsight-bank.test.ts +++ b/packages/coding-agent/test/hindsight-bank.test.ts @@ -60,6 +60,10 @@ const baseConfig = (overrides: Partial = {}): HindsightConfig = recallMaxQueryChars: 800, recallPromptPreamble: "preamble", debug: false, + requestTimeoutMs: 30_000, + reflectTimeoutMs: 120_000, + recallTimeoutMs: 30_000, + retainTimeoutMs: 60_000, mentalModelsEnabled: false, mentalModelAutoSeed: false, mentalModelRefreshIntervalMs: 5 * 60 * 1000, diff --git a/packages/coding-agent/test/memory-tools.test.ts b/packages/coding-agent/test/memory-tools.test.ts index a6911182d..8a8b39580 100644 --- a/packages/coding-agent/test/memory-tools.test.ts +++ b/packages/coding-agent/test/memory-tools.test.ts @@ -64,6 +64,10 @@ function makeConfig(overrides: Partial = {}): HindsightConfig { recallMaxQueryChars: 800, recallPromptPreamble: "preamble", debug: false, + requestTimeoutMs: 30_000, + reflectTimeoutMs: 120_000, + recallTimeoutMs: 30_000, + retainTimeoutMs: 60_000, mentalModelsEnabled: false, mentalModelAutoSeed: false, mentalModelRefreshIntervalMs: 5 * 60 * 1000,