mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-27 21:07:01 -06:00
fix(plugins): surface swallowed cleanup, setup, and CLI-load failures (#124570)
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.
This commit is contained in:
committed by
GitHub
parent
509f947a20
commit
7ed321de30
@@ -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<void>;
|
||||
};
|
||||
|
||||
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 [];
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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<typeof buildPluginApi>) => void | Promise<void>;
|
||||
} | 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>): void {
|
||||
function runSetupRegistration(
|
||||
register: (api: ReturnType<typeof buildPluginApi>) => void | Promise<void>,
|
||||
api: ReturnType<typeof buildPluginApi>,
|
||||
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;
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user