From f7681e6fe92a0fafcb9e1a263711f14dde1687c2 Mon Sep 17 00:00:00 2001 From: Jerry-Xin Date: Tue, 17 Mar 2026 13:02:45 +0800 Subject: [PATCH 1/3] fix(plugins): add timeout protection to before_agent_start hook Add configurable timeout (default 30s) to plugin lifecycle hooks to prevent a single misbehaving plugin from permanently blocking all agent runs. - Add hookTimeoutMs config option under plugins section - Wrap hook execution in Promise.race with timeout - Log warning with plugin name when hook exceeds timeout - Agent continues gracefully after timeout instead of hanging Fixes #48534 --- src/config/schema.help.ts | 2 + src/config/schema.labels.ts | 1 + src/config/types.plugins.ts | 2 + src/config/zod-schema.ts | 1 + src/plugins/hook-runner-global.ts | 6 +- src/plugins/hooks.timeout.test.ts | 307 ++++++++++++++++++++++++++++++ src/plugins/loader.ts | 16 +- 7 files changed, 330 insertions(+), 5 deletions(-) create mode 100644 src/plugins/hooks.timeout.test.ts diff --git a/src/config/schema.help.ts b/src/config/schema.help.ts index bb059bf5cad..19a5b842318 100644 --- a/src/config/schema.help.ts +++ b/src/config/schema.help.ts @@ -957,6 +957,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 62302e976af..afe402f8aab 100644 --- a/src/config/schema.labels.ts +++ b/src/config/schema.labels.ts @@ -852,6 +852,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 62d750b0470..037f0102117 100644 --- a/src/config/types.plugins.ts +++ b/src/config/types.plugins.ts @@ -29,6 +29,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 b32a86dc68f..671f9988b28 100644 --- a/src/config/zod-schema.ts +++ b/src/config/zod-schema.ts @@ -907,6 +907,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/loader.ts b/src/plugins/loader.ts index 8d064d477c3..0da477001b2 100644 --- a/src/plugins/loader.ts +++ b/src/plugins/loader.ts @@ -786,9 +786,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 { @@ -835,7 +839,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; } @@ -1372,7 +1378,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; } From 20615bbb13ac57c83594cfc770c9ddefcfcb1b86 Mon Sep 17 00:00:00 2001 From: Jerry-Xin Date: Tue, 17 Mar 2026 13:37:15 +0800 Subject: [PATCH 2/3] fix(plugins): correct JSDoc for callHandlerWithTimeout timeout behavior --- src/plugins/hooks.ts | 89 ++++++++++++++++++++++++++++++++------------ 1 file changed, 66 insertions(+), 23 deletions(-) diff --git a/src/plugins/hooks.ts b/src/plugins/hooks.ts index e8e1e2aa163..fb4faa0986d 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,27 @@ 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. + */ + 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 +280,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 +314,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 +350,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 +387,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 +440,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 }; } From 3d5b7a4a80756f942b281e8a68f036a73c1e4ae8 Mon Sep 17 00:00:00 2001 From: Jerry-Xin Date: Tue, 17 Mar 2026 14:08:32 +0800 Subject: [PATCH 3/3] fix(plugins): document leaked promise limitation on hook timeout Add a code comment explaining that timed-out handler promises continue running in the background since the current plugin hook API does not accept an AbortSignal for cooperative cancellation. Notes the path forward for when the plugin contract evolves. --- src/plugins/hooks.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/plugins/hooks.ts b/src/plugins/hooks.ts index fb4faa0986d..b80365951ef 100644 --- a/src/plugins/hooks.ts +++ b/src/plugins/hooks.ts @@ -175,6 +175,12 @@ export function createHookRunner(registry: PluginRegistry, options: HookRunnerOp /** * 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,