From 7ed321de30ef8818c0a74a4388d02fbeb99e1f61 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 16 Aug 2026 06:03:39 -0700 Subject: [PATCH] fix(plugins): surface swallowed cleanup, setup, and CLI-load failures (#124570) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three silent-failure sinks in the plugin lifecycle, all violating 'every action ends in a visible outcome or a recorded reason': - cleanupReplacedPluginHostRegistry collects per-hook cleanup failures instead of throwing (it must finish every plugin), but both callers discarded the returned failures array entirely — broken session- extension/scheduler teardown vanished. The shared funnel now warns per failure with plugin and hook ids. - setup-registry dropped broken setup entries with catch{return null} and threw-away registration errors with catch{return false}, silently removing a plugin's providers/CLI backends/migrations from onboarding. Both now push typed diagnostics (setup-entry-load-failed, setup-registration-failed) — and since the diagnostics array had no operator surface at all, the registry build now warns once per uncached build for every diagnostic (including the previously invisible descriptor-drift ones). - loadPluginCliDescriptors swallowed total load failures behind a muted per-plugin logger; a failure removes every plugin command from help/dispatch and must not vanish with it. One warn on the catch. --- src/plugins/cli-registry-loader.ts | 9 ++++- src/plugins/runtime.ts | 10 +++++- src/plugins/setup-registry.test.ts | 23 ++++++++++++- src/plugins/setup-registry.ts | 53 +++++++++++++++++++++++++----- 4 files changed, 84 insertions(+), 11 deletions(-) diff --git a/src/plugins/cli-registry-loader.ts b/src/plugins/cli-registry-loader.ts index c29a49ea17b4..f76361c54b34 100644 --- a/src/plugins/cli-registry-loader.ts +++ b/src/plugins/cli-registry-loader.ts @@ -3,6 +3,7 @@ import { normalizeLowercaseStringOrEmpty } from "@openclaw/normalization-core/st import { uniqueStrings } from "@openclaw/normalization-core/string-normalization"; import { collectUniqueCommandDescriptors } from "../cli/program/command-descriptor-utils.js"; import type { OpenClawConfig } from "../config/types.openclaw.js"; +import { createSubsystemLogger } from "../logging/subsystem.js"; import { resolveManifestActivationPluginIds } from "./activation-planner.js"; import { createPluginCliGatewayNodesRuntime } from "./cli-gateway-nodes-runtime.js"; import type { PluginLoadOptions } from "./loader.js"; @@ -46,6 +47,8 @@ export type PluginCliCommandGroupEntry = { register: (program: OpenClawPluginCliContext["program"]) => Promise; }; +const log = createSubsystemLogger("plugins/cli-registry-loader"); + /** Creates the default plugin CLI logger shared with runtime loading. */ export function createPluginCliLogger(): PluginLogger { return createPluginRuntimeLoaderLogger(); @@ -236,7 +239,11 @@ export async function loadPluginCliDescriptors( .filter((entry) => (entry.parentPath ?? []).length === 0) .map((entry) => entry.descriptors), ); - } catch { + } catch (error) { + // Callers pass a muted per-plugin logger for descriptor scans; a total + // load failure still removes every plugin command from help/dispatch and + // must not vanish with it. + log.warn(`plugin CLI descriptor load failed: ${String(error)}`); return []; } } diff --git a/src/plugins/runtime.ts b/src/plugins/runtime.ts index f1fb246a34cc..c2a7025a8d7b 100644 --- a/src/plugins/runtime.ts +++ b/src/plugins/runtime.ts @@ -77,12 +77,20 @@ async function cleanupPreviousPluginHostRegistry(params: { // Async cleanup must not clear state for a registry that has been restored // active, but later swaps should not strand cleanup for the retiring registry. const shouldCleanup = () => state.activeRegistry !== params.previousRegistry; - await cleanupReplacedPluginHostRegistry({ + const { failures } = await cleanupReplacedPluginHostRegistry({ cfg: getRuntimeConfig(), previousRegistry: params.previousRegistry, nextRegistry, shouldCleanup, }); + // Per-hook cleanup errors are collected instead of thrown (host-hook-cleanup + // must finish every plugin); dropping them here would hide broken + // session-extension/scheduler teardown from operators entirely. + for (const failure of failures) { + log.warn( + `plugin host cleanup failed for ${failure.pluginId} hook ${failure.hookId}: ${String(failure.error)}`, + ); + } } function cleanupRetiredPluginHostRegistry(previousRegistry: PluginRegistry): void { diff --git a/src/plugins/setup-registry.test.ts b/src/plugins/setup-registry.test.ts index c222d39675d8..ebc0b3f21884 100644 --- a/src/plugins/setup-registry.test.ts +++ b/src/plugins/setup-registry.test.ts @@ -835,6 +835,24 @@ describe("setup-registry module loader", () => { }); }); + it("records a diagnostic when the setup entry fails to load", () => { + const brokenRoot = makeTempDir(); + writeSetupApiStub(brokenRoot); + mockSinglePlugin({ id: "broken-entry", rootDir: brokenRoot }); + mocks.createJiti.mockImplementation(() => () => { + throw new Error("module parse failed"); + }); + + const registry = resolvePluginSetupRegistry({ env: {} }); + + // A broken setup entry removes the plugin from onboarding; the reason must + // be recorded instead of vanishing. + expect(registry.providers).toStrictEqual([]); + expect(registry.diagnostics).toMatchObject([ + { pluginId: "broken-entry", code: "setup-entry-load-failed" }, + ]); + }); + it("publishes each plugin setup registration atomically on synchronous success", () => { const throwingRoot = makeTempDir(); const healthyRoot = makeTempDir(); @@ -905,7 +923,10 @@ describe("setup-registry module loader", () => { expect(registry.configMigrations[0]?.migrate({} as never)?.changes).toEqual(["healthy"]); expect(registry.autoEnableProbes).toHaveLength(1); expect(registry.autoEnableProbes[0]?.probe({ config: {}, env: {} } as never)).toBe("healthy"); - expect(registry.diagnostics).toStrictEqual([]); + // The throwing registration is recorded, not silently dropped. + expect(registry.diagnostics).toMatchObject([ + { pluginId: "shared-plugin", code: "setup-registration-failed" }, + ]); } expect(second).not.toBe(first); expect(mocks.loadPluginManifestRegistry).toHaveBeenCalledTimes(1); diff --git a/src/plugins/setup-registry.ts b/src/plugins/setup-registry.ts index 393c81ae2454..357eb106020b 100644 --- a/src/plugins/setup-registry.ts +++ b/src/plugins/setup-registry.ts @@ -8,6 +8,7 @@ import { normalizeUniqueStringEntries, } from "@openclaw/normalization-core/string-normalization"; import type { OpenClawConfig } from "../config/types.openclaw.js"; +import { createSubsystemLogger } from "../logging/subsystem.js"; import { buildPluginApi } from "./api-builder.js"; import { collectPluginConfigContractMatches } from "./config-contracts.js"; import { getCurrentPluginMetadataSnapshotState } from "./current-plugin-metadata-state.js"; @@ -30,6 +31,8 @@ import type { ProviderPlugin, } from "./types.js"; +const log = createSubsystemLogger("plugins/setup-registry"); + const SETUP_API_EXTENSIONS = [".js", ".mjs", ".cjs", ".ts", ".mts", ".cts"] as const; const CURRENT_MODULE_PATH = fileURLToPath(import.meta.url); const RUNNING_FROM_BUILT_ARTIFACT = @@ -61,7 +64,9 @@ type PluginSetupRegistryDiagnosticCode = | "setup-descriptor-provider-missing-runtime" | "setup-descriptor-provider-runtime-undeclared" | "setup-descriptor-cli-backend-missing-runtime" - | "setup-descriptor-cli-backend-runtime-undeclared"; + | "setup-descriptor-cli-backend-runtime-undeclared" + | "setup-entry-load-failed" + | "setup-registration-failed"; type PluginSetupRegistryDiagnostic = { pluginId: string; @@ -271,7 +276,10 @@ function resolveDeclaredSetupRuntimeSource(record: PluginManifestRecord): string ); } -function resolveSetupRegistration(record: PluginManifestRecord): { +function resolveSetupRegistration( + record: PluginManifestRecord, + diagnostics?: PluginSetupRegistryDiagnostic[], +): { setupSource: string; register: (api: ReturnType) => void | Promise; } | null { @@ -286,7 +294,14 @@ function resolveSetupRegistration(record: PluginManifestRecord): { let mod: OpenClawPluginModule; try { mod = getModuleLoader(setupSource)(setupSource) as OpenClawPluginModule; - } catch { + } catch (error) { + // A broken setup entry silently removes the plugin's providers/CLI + // backends/migrations from onboarding; record why instead of vanishing. + diagnostics?.push({ + pluginId: record.id, + code: "setup-entry-load-failed", + message: `setup entry failed to load from ${setupSource}: ${String(error)}`, + }); return null; } @@ -336,11 +351,13 @@ function ignoreAsyncSetupRegisterResult(result: void | Promise): void { function runSetupRegistration( register: (api: ReturnType) => void | Promise, api: ReturnType, + onError: (error: unknown) => void, ): boolean { try { ignoreAsyncSetupRegisterResult(register(api)); return true; - } catch { + } catch (error) { + onError(error); return false; } } @@ -644,7 +661,7 @@ export function resolvePluginSetupRegistry(params?: { }); continue; } - const setupRegistration = resolveSetupRegistration(record); + const setupRegistration = resolveSetupRegistration(record, diagnostics); if (!setupRegistration) { continue; } @@ -703,7 +720,13 @@ export function resolvePluginSetupRegistry(params?: { }, }); - const registered = runSetupRegistration(setupRegistration.register, api); + const registered = runSetupRegistration(setupRegistration.register, api, (error) => { + diagnostics.push({ + pluginId: record.id, + code: "setup-registration-failed", + message: `setup registration threw: ${String(error)}`, + }); + }); acceptingRegistrations = false; if (!registered) { continue; @@ -729,6 +752,12 @@ export function resolvePluginSetupRegistry(params?: { autoEnableProbes, diagnostics, } satisfies PluginSetupRegistry; + // The diagnostics array has no other operator surface; warn once per + // (uncached) build so broken setup entries and descriptor drift are + // visible instead of silently narrowing onboarding. + for (const diagnostic of diagnostics) { + log.warn(`plugin setup [${diagnostic.pluginId}] ${diagnostic.code}: ${diagnostic.message}`); + } if (resultCacheKey === null) { return registry; } @@ -786,7 +815,11 @@ export function resolvePluginSetupProviderCore(params: { }, }); - if (!runSetupRegistration(setupRegistration.register, api)) { + if ( + !runSetupRegistration(setupRegistration.register, api, (error) => { + log.warn(`plugin setup [${record.id}] setup-registration-failed: ${String(error)}`); + }) + ) { return undefined; } @@ -846,7 +879,11 @@ export function resolvePluginSetupCliBackend(params: { }, }); - if (!runSetupRegistration(setupRegistration.register, api)) { + if ( + !runSetupRegistration(setupRegistration.register, api, (error) => { + log.warn(`plugin setup [${record.id}] setup-registration-failed: ${String(error)}`); + }) + ) { return undefined; }