diff --git a/src/config/schema.help.ts b/src/config/schema.help.ts index 947726bd7e8..a64187ec06b 100644 --- a/src/config/schema.help.ts +++ b/src/config/schema.help.ts @@ -936,6 +936,8 @@ export const FIELD_HELP: Record = { "Plugin system controls for enabling extensions, constraining load scope, configuring entries, and tracking installs. Keep plugin policy explicit and least-privilege in production environments.", "plugins.enabled": "Enable or disable plugin/extension loading globally during startup and config reload (default: true). Keep enabled only when extension capabilities are required by your deployment.", + "plugins.hookTimeoutMs": + "Maximum execution time in milliseconds for each async plugin hook handler (default: 10000). If a handler exceeds this limit it is skipped with a warning and the run proceeds. Set to 0 to disable the timeout.", "plugins.allow": "Optional allowlist of plugin IDs; when set, only listed plugins are eligible to load. Use this to enforce approved extension inventories in controlled environments.", "plugins.deny": diff --git a/src/config/schema.labels.ts b/src/config/schema.labels.ts index 53317e2fcd2..25b57ae2356 100644 --- a/src/config/schema.labels.ts +++ b/src/config/schema.labels.ts @@ -840,6 +840,7 @@ export const FIELD_LABELS: Record = { "discovery.mdns.mode": "mDNS Discovery Mode", plugins: "Plugins", "plugins.enabled": "Enable Plugins", + "plugins.hookTimeoutMs": "Plugin Hook Timeout (ms)", "plugins.allow": "Plugin Allowlist", "plugins.deny": "Plugin Denylist", "plugins.load": "Plugin Loader", diff --git a/src/config/types.plugins.ts b/src/config/types.plugins.ts index af37ba2020f..ed02ff0b042 100644 --- a/src/config/types.plugins.ts +++ b/src/config/types.plugins.ts @@ -38,6 +38,8 @@ export type PluginInstallRecord = Omit & { export type PluginsConfig = { /** Enable or disable plugin loading. */ enabled?: boolean; + /** Per-handler timeout for async plugin hooks (ms). Default 10 000. Set 0 to disable. */ + hookTimeoutMs?: number; /** Optional plugin allowlist (plugin ids). */ allow?: string[]; /** Optional plugin denylist (plugin ids). */ diff --git a/src/config/zod-schema.ts b/src/config/zod-schema.ts index f8ad6bfcbc9..92b700fd343 100644 --- a/src/config/zod-schema.ts +++ b/src/config/zod-schema.ts @@ -914,6 +914,7 @@ export const OpenClawSchema = z plugins: z .object({ enabled: z.boolean().optional(), + hookTimeoutMs: z.number().int().min(0).optional(), allow: z.array(z.string()).optional(), deny: z.array(z.string()).optional(), load: z diff --git a/src/plugins/hook-runner-global.ts b/src/plugins/hook-runner-global.ts index b2613f3467f..2714459635d 100644 --- a/src/plugins/hook-runner-global.ts +++ b/src/plugins/hook-runner-global.ts @@ -33,7 +33,10 @@ function getHookRunnerGlobalState(): HookRunnerGlobalState { * Initialize the global hook runner with a plugin registry. * Called once when plugins are loaded during gateway startup. */ -export function initializeGlobalHookRunner(registry: PluginRegistry): void { +export function initializeGlobalHookRunner( + registry: PluginRegistry, + options?: { hookTimeoutMs?: number }, +): void { const state = getHookRunnerGlobalState(); state.registry = registry; state.hookRunner = createHookRunner(registry, { @@ -43,6 +46,7 @@ export function initializeGlobalHookRunner(registry: PluginRegistry): void { error: (msg) => log.error(msg), }, catchErrors: true, + hookTimeoutMs: options?.hookTimeoutMs, }); const hookCount = registry.hooks.length; diff --git a/src/plugins/hooks.timeout.test.ts b/src/plugins/hooks.timeout.test.ts new file mode 100644 index 00000000000..289fe33a5e7 --- /dev/null +++ b/src/plugins/hooks.timeout.test.ts @@ -0,0 +1,307 @@ +/** + * Tests for per-handler hook timeout protection. + * + * Validates that a hanging plugin hook handler is terminated after the + * configured timeout so it cannot permanently block agent runs. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { createHookRunner } from "./hooks.js"; +import { addTestHook, TEST_PLUGIN_AGENT_CTX } from "./hooks.test-helpers.js"; +import { createEmptyPluginRegistry, type PluginRegistry } from "./registry.js"; +import type { PluginHookRegistration } from "./types.js"; + +const stubCtx = TEST_PLUGIN_AGENT_CTX; + +describe("hook handler timeout", () => { + let registry: PluginRegistry; + + beforeEach(() => { + registry = createEmptyPluginRegistry(); + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + // ----------------------------------------------------------------------- + // before_agent_start (modifying hook, sequential) + // ----------------------------------------------------------------------- + + it("completes normally when handler resolves within timeout", async () => { + addTestHook({ + registry, + pluginId: "fast-plugin", + hookName: "before_agent_start", + handler: (() => + Promise.resolve({ prependContext: "fast result" })) as PluginHookRegistration["handler"], + }); + + const runner = createHookRunner(registry, { hookTimeoutMs: 5000, catchErrors: true }); + const promise = runner.runBeforeAgentStart({ prompt: "hello" }, stubCtx); + await vi.advanceTimersByTimeAsync(0); + + const result = await promise; + expect(result?.prependContext).toBe("fast result"); + }); + + it("skips a hanging before_agent_start handler after timeout and proceeds", async () => { + const warnings: string[] = []; + + // This handler never resolves + addTestHook({ + registry, + pluginId: "hanging-plugin", + hookName: "before_agent_start", + handler: (() => new Promise(() => {})) as PluginHookRegistration["handler"], + }); + + const runner = createHookRunner(registry, { + hookTimeoutMs: 100, + catchErrors: true, + logger: { + debug: () => {}, + warn: (msg) => warnings.push(msg), + error: (msg) => warnings.push(msg), + }, + }); + + const promise = runner.runBeforeAgentStart({ prompt: "hello" }, stubCtx); + await vi.advanceTimersByTimeAsync(150); + + const result = await promise; + + // Handler timed out so no result is returned + expect(result).toBeUndefined(); + + // A warning/error should have been logged with the plugin name + expect(warnings.some((w) => w.includes("hanging-plugin"))).toBe(true); + expect(warnings.some((w) => w.includes("timed out"))).toBe(true); + }); + + it("other hooks still execute when one hangs", async () => { + const warnings: string[] = []; + + // First handler (higher priority) returns normally + addTestHook({ + registry, + pluginId: "good-plugin", + hookName: "before_agent_start", + handler: (() => + Promise.resolve({ + prependContext: "good context", + })) as PluginHookRegistration["handler"], + priority: 10, + }); + + // Second handler (lower priority) hangs + addTestHook({ + registry, + pluginId: "bad-plugin", + hookName: "before_agent_start", + handler: (() => new Promise(() => {})) as PluginHookRegistration["handler"], + priority: 1, + }); + + const runner = createHookRunner(registry, { + hookTimeoutMs: 100, + catchErrors: true, + logger: { + debug: () => {}, + warn: (msg) => warnings.push(msg), + error: (msg) => warnings.push(msg), + }, + }); + + const promise = runner.runBeforeAgentStart({ prompt: "hello" }, stubCtx); + await vi.advanceTimersByTimeAsync(150); + + const result = await promise; + + // The good plugin's result should be preserved + expect(result?.prependContext).toBe("good context"); + // The bad plugin should have been logged + expect(warnings.some((w) => w.includes("bad-plugin"))).toBe(true); + }); + + // ----------------------------------------------------------------------- + // Void hooks (parallel execution) + // ----------------------------------------------------------------------- + + it("skips a hanging void hook handler after timeout", async () => { + const warnings: string[] = []; + let fastHandlerRan = false; + + addTestHook({ + registry, + pluginId: "fast-void-plugin", + hookName: "agent_end", + handler: (() => { + fastHandlerRan = true; + return Promise.resolve(); + }) as PluginHookRegistration["handler"], + }); + + addTestHook({ + registry, + pluginId: "hanging-void-plugin", + hookName: "agent_end", + handler: (() => new Promise(() => {})) as PluginHookRegistration["handler"], + }); + + const runner = createHookRunner(registry, { + hookTimeoutMs: 100, + catchErrors: true, + logger: { + debug: () => {}, + warn: (msg) => warnings.push(msg), + error: (msg) => warnings.push(msg), + }, + }); + + const promise = runner.runAgentEnd({ messages: [], success: true }, stubCtx); + await vi.advanceTimersByTimeAsync(150); + await promise; + + expect(fastHandlerRan).toBe(true); + expect(warnings.some((w) => w.includes("hanging-void-plugin"))).toBe(true); + }); + + // ----------------------------------------------------------------------- + // Claiming hooks + // ----------------------------------------------------------------------- + + it("skips a hanging claiming hook handler after timeout", async () => { + const warnings: string[] = []; + + addTestHook({ + registry, + pluginId: "hanging-claim-plugin", + hookName: "inbound_claim", + handler: (() => new Promise(() => {})) as PluginHookRegistration["handler"], + }); + + const runner = createHookRunner(registry, { + hookTimeoutMs: 100, + catchErrors: true, + logger: { + debug: () => {}, + warn: (msg) => warnings.push(msg), + error: (msg) => warnings.push(msg), + }, + }); + + const claimCtx = { + channelId: "test", + }; + const promise = runner.runInboundClaim( + { content: "hello", channel: "test", isGroup: false }, + claimCtx, + ); + await vi.advanceTimersByTimeAsync(150); + + const result = await promise; + + expect(result).toBeUndefined(); + expect(warnings.some((w) => w.includes("hanging-claim-plugin"))).toBe(true); + }); + + // ----------------------------------------------------------------------- + // Timeout configuration + // ----------------------------------------------------------------------- + + it("uses the default timeout when hookTimeoutMs is not specified", async () => { + // Handler that resolves in 5 seconds (well under the 10s default) + addTestHook({ + registry, + pluginId: "slow-but-ok", + hookName: "before_agent_start", + handler: (() => + new Promise((resolve) => { + setTimeout(() => resolve({ prependContext: "slow result" }), 5000); + })) as PluginHookRegistration["handler"], + }); + + const runner = createHookRunner(registry, { catchErrors: true }); + const promise = runner.runBeforeAgentStart({ prompt: "hello" }, stubCtx); + await vi.advanceTimersByTimeAsync(5000); + + const result = await promise; + expect(result?.prependContext).toBe("slow result"); + }); + + it("respects hookTimeoutMs: 0 to disable timeout", async () => { + // Handler that takes a long time but should not be timed out + addTestHook({ + registry, + pluginId: "very-slow-plugin", + hookName: "before_agent_start", + handler: (() => + new Promise((resolve) => { + setTimeout(() => resolve({ prependContext: "very slow result" }), 30_000); + })) as PluginHookRegistration["handler"], + }); + + const runner = createHookRunner(registry, { hookTimeoutMs: 0, catchErrors: true }); + const promise = runner.runBeforeAgentStart({ prompt: "hello" }, stubCtx); + await vi.advanceTimersByTimeAsync(30_000); + + const result = await promise; + expect(result?.prependContext).toBe("very slow result"); + }); + + it("uses custom hookTimeoutMs when specified", async () => { + const errors: string[] = []; + + addTestHook({ + registry, + pluginId: "medium-slow-plugin", + hookName: "before_agent_start", + handler: (() => + new Promise((resolve) => { + setTimeout(() => resolve({ prependContext: "result" }), 3000); + })) as PluginHookRegistration["handler"], + }); + + const runner = createHookRunner(registry, { + hookTimeoutMs: 2000, + catchErrors: true, + logger: { + debug: () => {}, + warn: () => {}, + error: (msg) => errors.push(msg), + }, + }); + + const promise = runner.runBeforeAgentStart({ prompt: "hello" }, stubCtx); + await vi.advanceTimersByTimeAsync(2500); + + const result = await promise; + + // Should have timed out at 2000ms before the handler resolved at 3000ms + expect(result).toBeUndefined(); + expect(errors.some((e) => e.includes("medium-slow-plugin"))).toBe(true); + }); + + // ----------------------------------------------------------------------- + // Error propagation when catchErrors is false + // ----------------------------------------------------------------------- + + it("throws timeout error when catchErrors is false", async () => { + addTestHook({ + registry, + pluginId: "hanging-throw-plugin", + hookName: "before_agent_start", + handler: (() => new Promise(() => {})) as PluginHookRegistration["handler"], + }); + + const runner = createHookRunner(registry, { hookTimeoutMs: 50, catchErrors: false }); + const promise = runner.runBeforeAgentStart({ prompt: "hello" }, stubCtx); + + // Register the rejection handler before advancing timers to avoid + // unhandled rejection noise. + const assertion = expect(promise).rejects.toThrow(/timed out/); + await vi.advanceTimersByTimeAsync(100); + await assertion; + }); +}); diff --git a/src/plugins/hooks.ts b/src/plugins/hooks.ts index e8e1e2aa163..b80365951ef 100644 --- a/src/plugins/hooks.ts +++ b/src/plugins/hooks.ts @@ -5,6 +5,7 @@ * error handling, priority ordering, and async support. */ +import { withTimeout } from "../node-host/with-timeout.js"; import { concatOptionalTextSegments } from "../shared/text/join-segments.js"; import type { PluginRegistry } from "./registry.js"; import type { @@ -108,10 +109,15 @@ export type HookRunnerLogger = { error: (message: string) => void; }; +/** Default timeout for async plugin hook handlers (ms). */ +const DEFAULT_HOOK_TIMEOUT_MS = 10_000; + export type HookRunnerOptions = { logger?: HookRunnerLogger; /** If true, errors in hooks will be caught and logged instead of thrown */ catchErrors?: boolean; + /** Per-handler timeout for async hooks (ms). Defaults to 10 000. Set 0 to disable. */ + hookTimeoutMs?: number; }; export type PluginTargetedInboundClaimOutcome = @@ -159,6 +165,33 @@ function getHooksForNameAndPlugin( export function createHookRunner(registry: PluginRegistry, options: HookRunnerOptions = {}) { const logger = options.logger; const catchErrors = options.catchErrors ?? true; + const hookTimeoutMs = + typeof options.hookTimeoutMs === "number" && options.hookTimeoutMs > 0 + ? options.hookTimeoutMs + : options.hookTimeoutMs === 0 + ? undefined // explicitly disabled + : DEFAULT_HOOK_TIMEOUT_MS; + + /** + * Execute a single async handler with the configured timeout. + * Throws on timeout; callers catch via handleHookError. + * + * NOTE: On timeout, the underlying handler promise is NOT cancelled — it + * continues running in the background until it settles. The current plugin + * hook contract (`() => Promise`) does not accept an `AbortSignal`, so + * cooperative cancellation is not yet possible. If the plugin API evolves + * to accept a signal, wire it through here for proper cleanup. + */ + async function callHandlerWithTimeout( + fn: () => Promise, + hookName: PluginHookName, + pluginId: string, + ): Promise { + if (!hookTimeoutMs) { + return fn(); + } + return withTimeout(() => fn(), hookTimeoutMs, `${hookName} handler from ${pluginId}`); + } const mergeBeforeModelResolve = ( acc: PluginHookBeforeModelResolveResult | undefined, @@ -253,7 +286,11 @@ export function createHookRunner(registry: PluginRegistry, options: HookRunnerOp const promises = hooks.map(async (hook) => { try { - await (hook.handler as (event: unknown, ctx: unknown) => Promise)(event, ctx); + await callHandlerWithTimeout( + () => (hook.handler as (event: unknown, ctx: unknown) => Promise)(event, ctx), + hookName, + hook.pluginId, + ); } catch (err) { handleHookError({ hookName, pluginId: hook.pluginId, error: err }); } @@ -283,9 +320,11 @@ export function createHookRunner(registry: PluginRegistry, options: HookRunnerOp for (const hook of hooks) { try { - const handlerResult = await ( - hook.handler as (event: unknown, ctx: unknown) => Promise - )(event, ctx); + const handlerResult = await callHandlerWithTimeout( + () => (hook.handler as (event: unknown, ctx: unknown) => Promise)(event, ctx), + hookName, + hook.pluginId, + ); if (handlerResult !== undefined && handlerResult !== null) { if (mergeResults && result !== undefined) { @@ -317,7 +356,23 @@ export function createHookRunner(registry: PluginRegistry, options: HookRunnerOp logger?.debug?.(`[hooks] running ${hookName} (${hooks.length} handlers, first-claim wins)`); - return await runClaimingHooksList(hooks, hookName, event, ctx); + for (const hook of hooks) { + try { + const handlerResult = await callHandlerWithTimeout( + () => + (hook.handler as (event: unknown, ctx: unknown) => Promise)(event, ctx), + hookName, + hook.pluginId, + ); + if (handlerResult?.handled) { + return handlerResult; + } + } catch (err) { + handleHookError({ hookName, pluginId: hook.pluginId, error: err }); + } + } + + return undefined; } async function runClaimingHookForPlugin< @@ -338,23 +393,14 @@ export function createHookRunner(registry: PluginRegistry, options: HookRunnerOp `[hooks] running ${hookName} for ${pluginId} (${hooks.length} handlers, targeted)`, ); - return await runClaimingHooksList(hooks, hookName, event, ctx); - } - - async function runClaimingHooksList< - K extends PluginHookName, - TResult extends { handled: boolean }, - >( - hooks: Array & { pluginId: string }>, - hookName: K, - event: Parameters["handler"]>>[0], - ctx: Parameters["handler"]>>[1], - ): Promise { for (const hook of hooks) { try { - const handlerResult = await ( - hook.handler as (event: unknown, ctx: unknown) => Promise - )(event, ctx); + const handlerResult = await callHandlerWithTimeout( + () => + (hook.handler as (event: unknown, ctx: unknown) => Promise)(event, ctx), + hookName, + hook.pluginId, + ); if (handlerResult?.handled) { return handlerResult; } @@ -400,9 +446,12 @@ export function createHookRunner(registry: PluginRegistry, options: HookRunnerOp let firstError: string | null = null; for (const hook of hooks) { try { - const handlerResult = await ( - hook.handler as (event: unknown, ctx: unknown) => Promise - )(event, ctx); + const handlerResult = await callHandlerWithTimeout( + () => + (hook.handler as (event: unknown, ctx: unknown) => Promise)(event, ctx), + hookName, + hook.pluginId, + ); if (handlerResult?.handled) { return { status: "handled", result: handlerResult }; } diff --git a/src/plugins/loader.ts b/src/plugins/loader.ts index 6f5900f8334..a2a59bbb63f 100644 --- a/src/plugins/loader.ts +++ b/src/plugins/loader.ts @@ -715,9 +715,13 @@ function warnAboutUntrackedLoadedPlugins(params: { } } -function activatePluginRegistry(registry: PluginRegistry, cacheKey: string): void { +function activatePluginRegistry( + registry: PluginRegistry, + cacheKey: string, + options?: { hookTimeoutMs?: number }, +): void { setActivePluginRegistry(registry, cacheKey); - initializeGlobalHookRunner(registry); + initializeGlobalHookRunner(registry, { hookTimeoutMs: options?.hookTimeoutMs }); } export function loadOpenClawPlugins(options: PluginLoadOptions = {}): PluginRegistry { @@ -764,7 +768,9 @@ export function loadOpenClawPlugins(options: PluginLoadOptions = {}): PluginRegi const cached = getCachedPluginRegistry(cacheKey); if (cached) { if (shouldActivate) { - activatePluginRegistry(cached, cacheKey); + activatePluginRegistry(cached, cacheKey, { + hookTimeoutMs: options.config?.plugins?.hookTimeoutMs, + }); } return cached; } @@ -1320,7 +1326,9 @@ export function loadOpenClawPlugins(options: PluginLoadOptions = {}): PluginRegi setCachedPluginRegistry(cacheKey, registry); } if (shouldActivate) { - activatePluginRegistry(registry, cacheKey); + activatePluginRegistry(registry, cacheKey, { + hookTimeoutMs: options.config?.plugins?.hookTimeoutMs, + }); } return registry; }