diff --git a/.changeset/tool-deadlines.md b/.changeset/tool-deadlines.md new file mode 100644 index 000000000000..1624b139294c --- /dev/null +++ b/.changeset/tool-deadlines.md @@ -0,0 +1,8 @@ +--- +"@reddb-io/redcode": patch +"@reddb-io/redcode-core": patch +--- + +Stop a tool that never returns instead of letting it hold the turn open + +Most tools carry no bound of their own, so a read on a dead mount or an MCP call to a process that went away kept a turn running with no output and no error — and the turn's inactivity watchdog could not help, because a tool in flight is deliberately counted as work. Tool calls now have a ten minute backstop, reported to the model as an ordinary tool failure it can react to. Tools that legitimately take as long as they take are exempt (`shell`, `bash`, `question`, `task`), and time spent waiting on a permission prompt is not charged against the tool. Configurable via `experimental.tool_timeout`, `false` to disable. diff --git a/packages/core/src/v1/config/config.ts b/packages/core/src/v1/config/config.ts index 035cbc7df210..5c4ad95369d4 100644 --- a/packages/core/src/v1/config/config.ts +++ b/packages/core/src/v1/config/config.ts @@ -182,6 +182,10 @@ export const Info = Schema.Struct({ mcp_timeout: Schema.optional(PositiveInt).annotate({ description: "Timeout in milliseconds for model context protocol (MCP) requests", }), + tool_timeout: Schema.optional(Schema.Union([Schema.Literal(false), PositiveInt])).annotate({ + description: + "Milliseconds a tool may run before it is stopped and reported to the model as a failure (default: 600000). Tools that carry their own deadline, wait for a person, or run a whole child turn are not affected. Set to false to disable.", + }), turn_stall: Schema.optional( Schema.Union([ Schema.Literal(false), diff --git a/packages/redcode/src/session/prompt.ts b/packages/redcode/src/session/prompt.ts index 2710d92d2090..6b13dbe4fb53 100644 --- a/packages/redcode/src/session/prompt.ts +++ b/packages/redcode/src/session/prompt.ts @@ -1406,6 +1406,7 @@ const layer = Layer.effect( messages: msgs, promptOps, publishEvent: events.publish, + toolTimeout: (yield* config.get()).experimental?.tool_timeout, ...(lastUser.format?.type === "json_schema" ? { structuredOutputTool: createStructuredOutputTool({ diff --git a/packages/redcode/src/session/tool-deadline.ts b/packages/redcode/src/session/tool-deadline.ts new file mode 100644 index 000000000000..28014bf8699c --- /dev/null +++ b/packages/redcode/src/session/tool-deadline.ts @@ -0,0 +1,65 @@ +/** + * How long a tool may run before it is treated as wedged. + * + * Most tools have no bound at all today: a read on a dead network mount, an LSP request to a + * server that stopped answering, or an MCP call to a process that went away holds the whole turn + * with no output and no error. The turn's own inactivity watchdog cannot help, because a tool in + * flight is deliberately counted as work. + * + * A timeout here is an ordinary tool failure, not a crash: the model sees it, can say so, and can + * try something else. + */ + +import { Duration, Effect } from "effect" + +/** Generous on purpose. This is a backstop against wedging, not a performance budget. */ +export const TOOL_DEADLINE_DEFAULT_MS = 600_000 + +/** + * Tools that must not be bounded from here. + * + * `shell` carries its own deadline and lets the model choose it, so a deliberately long build is a + * legitimate call rather than a hang. `question` exists to wait for a person. `task` runs a whole + * child turn, which has its own watchdog — bounding it here would cut a subagent mid-thought and + * report it as a stuck tool. + */ +const UNBOUNDED = new Set(["shell", "bash", "question", "task"]) + +export function deadlineMs(input: { tool: string; configured?: number | false }): number | undefined { + if (UNBOUNDED.has(input.tool)) return undefined + if (input.configured === false) return undefined + const ms = input.configured ?? TOOL_DEADLINE_DEFAULT_MS + return ms > 0 ? ms : undefined +} + +export function message(input: { tool: string; ms: number }) { + const minutes = Math.round(input.ms / 60_000) + const how = minutes >= 1 ? `${minutes}m` : `${Math.round(input.ms / 1000)}s` + return `The ${input.tool} tool was still running after ${how} and was stopped. It may be waiting on something that will not answer; try a different approach, or narrow what you asked it to do.` +} + +/** How often the guard re-checks. Small enough for tests, coarse enough to cost nothing. */ +export const POLL_MS = 250 + +/** + * Bound a tool call without charging it for time a person spent deciding. + * + * A permission dialog left open all afternoon is not a hung tool, so the clock is checked against + * elapsed time minus whatever `waitedMs` reports as human deliberation. Written as a race rather + * than a plain timeout precisely so that subtraction can happen while the call is in flight. + */ +export const guard = ( + self: Effect.Effect, + input: { tool: string; ms: number; waitedMs: () => number }, +): Effect.Effect => + Effect.raceFirst( + self, + Effect.gen(function* () { + const start = Date.now() + const step = Duration.millis(Math.max(1, Math.min(input.ms, POLL_MS))) + while (Date.now() - start - input.waitedMs() < input.ms) yield* Effect.sleep(step) + return yield* Effect.die(new Error(message(input))) + }), + ) + +export * as ToolDeadline from "./tool-deadline" diff --git a/packages/redcode/src/session/tools.ts b/packages/redcode/src/session/tools.ts index bdd1928c14e6..f71097c4ad2b 100644 --- a/packages/redcode/src/session/tools.ts +++ b/packages/redcode/src/session/tools.ts @@ -26,6 +26,7 @@ import { ModelV2 } from "@reddb-io/redcode-core/model" import { isRecord } from "@/util/record" import { RuntimeFlags } from "@/effect/runtime-flags" import { OperationHook } from "@reddb-io/redcode-core/operation-hook" +import { ToolDeadline } from "./tool-deadline" import { OperationHookBridge } from "@/operation-hook-bridge" import { SessionMessage } from "@reddb-io/redcode-schema/session-message" @@ -53,6 +54,7 @@ export const resolve = Effect.fn("SessionTools.resolve")(function* (input: { promptOps: TaskPromptOps publishEvent: EventV2.Interface["publish"] structuredOutputTool?: AITool + toolTimeout?: number | false }) { const tools: Record = {} const run = yield* EffectBridge.make() @@ -64,6 +66,12 @@ export const resolve = Effect.fn("SessionTools.resolve")(function* (input: { const flags = yield* RuntimeFlags.Service const hooks = yield* OperationHookBridge.Service + // One global override rather than a knob per tool: the failure this guards against is a tool + // that never returns, and that is not a per-tool judgement. + const toolTimeout = input.toolTimeout + const permissionWaits = new Map() + const permissionWaitMs = (callID?: string) => permissionWaits.get(callID ?? "") ?? 0 + const withOperationHooks = (toolID: string, item: AITool): AITool => { const execute = item.execute if (!execute) return item @@ -101,9 +109,20 @@ export const resolve = Effect.fn("SessionTools.resolve")(function* (input: { yield* hooks.parallel(OperationHook.Operation.Tool.PostExecute, payload) yield* input.publishEvent(SessionEvent.Tool.PostExecute, payload).pipe(Effect.ignore) }) - const executed = yield* Effect.promise(() => Promise.resolve(execute(decided.args, options))).pipe( - Effect.exit, - ) + // Most tools carry no bound of their own, so one that never returns holds the whole + // turn with no output and no error — and the turn's watchdog cannot help, because a + // tool in flight is deliberately counted as work. A timeout here lands in the same + // failure branch as any other tool error, so the model reads it and can react. + const deadline = ToolDeadline.deadlineMs({ tool: toolID, configured: toolTimeout }) + const call = Effect.promise(() => Promise.resolve(execute(decided.args, options))) + const executed = yield* (deadline === undefined + ? call + : ToolDeadline.guard(call, { + tool: toolID, + ms: deadline, + waitedMs: () => permissionWaitMs(options.toolCallId), + }) + ).pipe(Effect.exit) if (Exit.isFailure(executed)) { yield* publishPost({ error: String(Cause.squash(executed.cause)) }, true).pipe(Effect.ignoreCause) return yield* Effect.failCause(executed.cause) @@ -142,14 +161,27 @@ export const resolve = Effect.fn("SessionTools.resolve")(function* (input: { } }), ask: (req) => - permission - .ask({ - ...req, - sessionID: input.session.id, - tool: { messageID: input.processor.message.id, callID: options.toolCallId }, - ruleset: Permission.merge(input.agent.permission, input.session.permission ?? []), - }) - .pipe(Effect.orDie), + // A tool blocked on a person is not a tool that hung, so the wait is deducted from its + // deadline rather than counted against it. + Effect.suspend(() => { + const started = Date.now() + return permission + .ask({ + ...req, + sessionID: input.session.id, + tool: { messageID: input.processor.message.id, callID: options.toolCallId }, + ruleset: Permission.merge(input.agent.permission, input.session.permission ?? []), + }) + .pipe( + Effect.ensuring( + Effect.sync(() => { + const key = options.toolCallId ?? "" + permissionWaits.set(key, (permissionWaits.get(key) ?? 0) + (Date.now() - started)) + }), + ), + Effect.orDie, + ) + }), }) for (const item of yield* registry.tools({ diff --git a/packages/redcode/test/session/prompt.test.ts b/packages/redcode/test/session/prompt.test.ts index 298bda9a022c..65ec21523df9 100644 --- a/packages/redcode/test/session/prompt.test.ts +++ b/packages/redcode/test/session/prompt.test.ts @@ -1417,6 +1417,50 @@ it.instance("leaves a turn alone while a tool is still running", () => }), ) +it.instance("stops a tool that never returns and hands the failure to the model", () => + Effect.gen(function* () { + // The gap the turn watchdog cannot close: a tool in flight counts as work, so a tool that + // never returns holds the turn open forever with no output and no error. + const { llm } = yield* useServerConfig((url) => ({ + ...providerCfg(url), + experimental: { tool_timeout: 500 }, + })) + const registry = yield* ToolRegistry.Service + const { read } = yield* registry.named() + const { ready, restore } = yield* hangUntilAborted(read) + yield* restore + + const prompt = yield* SessionPrompt.Service + const sessions = yield* Session.Service + const chat = yield* sessions.create({ title: "Pinned" }) + yield* seed(chat.id) + + yield* llm.tool("read", { filePath: "/tmp/whatever" }) + yield* llm.text("that path does not answer") + yield* user(chat.id, "more") + + const fiber = yield* prompt.loop({ sessionID: chat.id }).pipe(Effect.forkChild) + yield* awaitWithTimeout(Deferred.await(ready), "timed out waiting for the tool to start", "10 seconds") + + // The turn finishes on its own: no cancel, no interrupt, well inside a timeout that would + // catch the old wedge. + yield* awaitWithTimeout(Fiber.await(fiber), "the turn never finished", "20 seconds") + + const messages = yield* sessions.messages({ sessionID: chat.id }) + const assistant = messages.findLast( + (item): item is (typeof messages)[number] & { info: SessionV1.Assistant } => item.info.role === "assistant", + ) + // Not an aborted turn — an ordinary tool failure the model was free to answer. + expect(assistant?.info.error).toBeUndefined() + const failed = messages + .flatMap((item) => item.parts) + .find((part) => part.type === "tool" && part.state.status === "error") + expect(failed).toBeDefined() + expect((failed as { state: { error: string } }).state.error).toMatch(/read tool was still running/) + }), + 30_000, +) + it.instance("cancel records MessageAbortedError on interrupted process", () => Effect.gen(function* () { const { llm } = yield* useServerConfig(providerCfg) diff --git a/packages/redcode/test/session/tool-deadline.test.ts b/packages/redcode/test/session/tool-deadline.test.ts new file mode 100644 index 000000000000..0f8c51c2b5ca --- /dev/null +++ b/packages/redcode/test/session/tool-deadline.test.ts @@ -0,0 +1,53 @@ +import { describe, expect, test } from "bun:test" +import { Effect, Exit } from "effect" +import { deadlineMs, guard, message, TOOL_DEADLINE_DEFAULT_MS } from "@/session/tool-deadline" + +describe("tool deadlines", () => { + test("bounds a tool that has no bound of its own", () => { + expect(deadlineMs({ tool: "read" })).toBe(TOOL_DEADLINE_DEFAULT_MS) + expect(deadlineMs({ tool: "grep" })).toBe(TOOL_DEADLINE_DEFAULT_MS) + }) + + test("leaves alone the tools that legitimately take as long as they take", () => { + // shell carries its own deadline and the model chooses it; question waits for a person; task + // runs a whole child turn that has its own watchdog. + expect(deadlineMs({ tool: "shell" })).toBeUndefined() + expect(deadlineMs({ tool: "question" })).toBeUndefined() + expect(deadlineMs({ tool: "task" })).toBeUndefined() + }) + + test("configuration overrides the default, and false turns it off", () => { + expect(deadlineMs({ tool: "read", configured: 5_000 })).toBe(5_000) + expect(deadlineMs({ tool: "read", configured: false })).toBeUndefined() + expect(deadlineMs({ tool: "read", configured: 0 })).toBeUndefined() + }) + + test("the failure tells the model what happened and what to do", () => { + const text = message({ tool: "read", ms: 600_000 }) + expect(text).toContain("read") + expect(text).toContain("10m") + expect(text).toContain("try a different approach") + }) + + test("stops a call that outlives its deadline", async () => { + const exit = await Effect.runPromiseExit( + guard(Effect.never, { tool: "read", ms: 60, waitedMs: () => 0 }), + ) + expect(Exit.isFailure(exit)).toBe(true) + expect(String(exit)).toMatch(/read tool was still running/) + }) + + test("does not charge a tool for time a person spent deciding", async () => { + // The permission dialog left open is the case: elapsed time grows, but none of it is the + // tool's, so the deadline must not arrive. + const started = Date.now() + const exit = await Effect.runPromiseExit( + guard(Effect.sleep("300 millis").pipe(Effect.as("done")), { + tool: "edit", + ms: 60, + waitedMs: () => Date.now() - started, + }), + ) + expect(exit).toEqual(Exit.succeed("done")) + }) +})