From f7681e6fe92a0fafcb9e1a263711f14dde1687c2 Mon Sep 17 00:00:00 2001 From: Jerry-Xin Date: Tue, 17 Mar 2026 13:02:45 +0800 Subject: [PATCH] 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; }