Merge 3d5b7a4a80756f942b281e8a68f036a73c1e4ae8 into 598f1826d8b2bc969aace2c6459824737667218c

This commit is contained in:
Jerry-Xin 2026-03-21 12:03:56 +08:00 committed by GitHub
commit f978588845
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 402 additions and 28 deletions

View File

@ -936,6 +936,8 @@ export const FIELD_HELP: Record<string, string> = {
"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":

View File

@ -840,6 +840,7 @@ export const FIELD_LABELS: Record<string, string> = {
"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",

View File

@ -38,6 +38,8 @@ export type PluginInstallRecord = Omit<InstallRecordBase, "source"> & {
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). */

View File

@ -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

View File

@ -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;

View File

@ -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;
});
});

View File

@ -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<K extends PluginHookName>(
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<T>`) 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<T>(
fn: () => Promise<T>,
hookName: PluginHookName,
pluginId: string,
): Promise<T> {
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<void>)(event, ctx);
await callHandlerWithTimeout(
() => (hook.handler as (event: unknown, ctx: unknown) => Promise<void>)(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<TResult>
)(event, ctx);
const handlerResult = await callHandlerWithTimeout(
() => (hook.handler as (event: unknown, ctx: unknown) => Promise<TResult>)(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<TResult | void>)(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<PluginHookRegistration<K> & { pluginId: string }>,
hookName: K,
event: Parameters<NonNullable<PluginHookRegistration<K>["handler"]>>[0],
ctx: Parameters<NonNullable<PluginHookRegistration<K>["handler"]>>[1],
): Promise<TResult | undefined> {
for (const hook of hooks) {
try {
const handlerResult = await (
hook.handler as (event: unknown, ctx: unknown) => Promise<TResult | void>
)(event, ctx);
const handlerResult = await callHandlerWithTimeout(
() =>
(hook.handler as (event: unknown, ctx: unknown) => Promise<TResult | void>)(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<TResult | void>
)(event, ctx);
const handlerResult = await callHandlerWithTimeout(
() =>
(hook.handler as (event: unknown, ctx: unknown) => Promise<TResult | void>)(event, ctx),
hookName,
hook.pluginId,
);
if (handlerResult?.handled) {
return { status: "handled", result: handlerResult };
}

View File

@ -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;
}