fix(plugins): retry errored plugins at boot instead of leaving them dead (#12054)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Plugins extend the server with sandbox providers, tools, and jobs; a loader activates them at boot > - When activation fails, the loader marks the plugin `error` and skips it on every later boot > - Activation failures are often environmental — missing package dependencies, a stale build output, a module that moved under a pull — and the fix lands on disk without any write to the plugin row > - The plugin therefore stays dead forever, and every feature behind it (sandbox destroys, cleanup sweeps, probes) silently stops working until an operator flips the row by hand > - This pull request makes `loadAll` retry errored plugins once per boot: flip to `ready`, attempt activation, and re-record the error if the attempt fails > - The benefit is that a plugin recovers on the next boot after its environment is fixed, with no manual database or lifecycle intervention ## Linked Issues or Issue Description **What happened?** Several sandbox-provider plugins sat in `error` status for weeks after a transient activation failure (a module resolution error from an older checkout state). The boot loader only loads plugins in `ready` status, so it never retried them. Environments backed by those providers lost sandbox destroys, cleanup sweeps, and probes with no visible signal other than the stale `last_error`. **Expected behavior** A plugin whose activation failure has been fixed on disk recovers on the next server boot. A plugin that still fails stays in `error` with a fresh error message. **Steps to reproduce** 1. Install a plugin whose worker cannot start (for example, delete one of its dependencies), then boot the server. The plugin lands in `error` status. 2. Restore the dependency. 3. Restart the server. Before this change, the plugin stays in `error` forever. After this change, the boot retries it and the plugin activates. ## What Changed - `server/src/services/plugin-loader.ts`: `loadAll` also fetches plugins in `error` status, flips each to `ready`, and activates it with the normal batch. The flip runs before activation because the `error` status only legally transitions to `ready` or `uninstalled`; a retry that failed while still in `error` could not re-mark itself. A failed flip logs a warning and never aborts the boot load. The stale comment at the `markError` site now describes the retry. - `server/src/__tests__/plugin-loader-error-retry.test.ts`: covers the flip-then-retry flow, the failed-flip isolation, and the empty case. ## Verification - `npx vitest run server/src/__tests__/plugin-loader-error-retry.test.ts server/src/__tests__/bundled-plugins.test.ts server/src/__tests__/plugin-lifecycle-restart.test.ts server/src/__tests__/cloud-image-bundled-plugins.test.ts` - Manual: mark an installed plugin's status to `error`, restart the server, and observe the loader log line `retrying plugins that failed activation on a previous boot` followed by a successful activation (or a fresh `last_error` if the plugin is genuinely broken). ## Risks - A genuinely broken plugin now costs one bounded activation attempt per boot (the attempts run in parallel with the ready batch under `Promise.allSettled`). It cannot crash-loop within a running process, and it returns to `error` with a fresh message. - The flip clears `last_error` before the attempt. If the process dies between the flip and the activation, the row is `ready` with no error text; the next boot simply loads it as a ready plugin. - Operators who relied on `error` as a manual "keep this off" latch should use the `disabled` status, which this change does not touch. ## Model Used - Claude (Anthropic) — Fable 5, model id `claude-fable-5`, extended thinking, agentic tool use via Claude Code CLI. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [ ] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge
This commit is contained in:
Devin Foley
GitHub
parent
ae6761e2b0
commit
627eef7cbd
|
|
@ -0,0 +1,151 @@
|
|||
/**
|
||||
* loadAll boot retry for plugins stranded in error status.
|
||||
*
|
||||
* An activation failure marks a plugin `error`, and the loader used to skip
|
||||
* those rows on every later boot — the row stayed dead until an operator
|
||||
* flipped it back to `ready` by hand, even when the underlying cause (missing
|
||||
* package dependencies, a stale build output) had long been fixed on disk.
|
||||
* loadAll now queues errored plugins for one retry per boot: it flips each row
|
||||
* to `ready` first (the error status cannot legally re-enter `error`, so a
|
||||
* failed retry could not re-mark itself otherwise) and then activates it like
|
||||
* any ready plugin. A retry that fails re-records the error through markError.
|
||||
*/
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import type { Db } from "@paperclipai/db";
|
||||
|
||||
const mockRegistry = vi.hoisted(() => ({
|
||||
getById: vi.fn(),
|
||||
getByKey: vi.fn(),
|
||||
list: vi.fn(),
|
||||
listInstalled: vi.fn(),
|
||||
listByStatus: vi.fn(),
|
||||
update: vi.fn(),
|
||||
updateStatus: vi.fn(),
|
||||
upsertConfig: vi.fn(),
|
||||
getConfig: vi.fn(),
|
||||
delete: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock("../services/plugin-registry.js", () => ({
|
||||
pluginRegistryService: () => mockRegistry,
|
||||
}));
|
||||
|
||||
import { pluginLoader } from "../services/plugin-loader.js";
|
||||
import type { PluginRuntimeServices } from "../services/plugin-loader.js";
|
||||
|
||||
function createPluginRecord(overrides: Record<string, unknown> = {}) {
|
||||
return {
|
||||
id: "plugin-err-1",
|
||||
pluginKey: "example.broken-plugin",
|
||||
packageName: "@example/broken-plugin",
|
||||
packagePath: "/nonexistent/broken-plugin",
|
||||
version: "1.0.0",
|
||||
apiVersion: 1,
|
||||
categories: [],
|
||||
status: "error",
|
||||
lastError: "Activation failed: previous boot failure",
|
||||
installOrder: 1,
|
||||
manifestJson: {
|
||||
id: "example.broken-plugin",
|
||||
apiVersion: 1,
|
||||
version: "1.0.0",
|
||||
displayName: "Broken Plugin",
|
||||
description: "Fixture",
|
||||
author: "Test",
|
||||
categories: [],
|
||||
capabilities: [],
|
||||
entrypoints: { worker: "dist/worker.js" },
|
||||
},
|
||||
...overrides,
|
||||
};
|
||||
}
|
||||
|
||||
function createRuntimeServices() {
|
||||
return {
|
||||
lifecycleManager: {
|
||||
markError: vi.fn(async () => createPluginRecord()),
|
||||
},
|
||||
workerManager: {},
|
||||
eventBus: {},
|
||||
jobScheduler: {},
|
||||
jobStore: {},
|
||||
toolDispatcher: {},
|
||||
buildHostHandlers: vi.fn(() => ({})),
|
||||
instanceInfo: { hostVersion: "0.0.0-test" },
|
||||
} as unknown as PluginRuntimeServices;
|
||||
}
|
||||
|
||||
function createLoader(runtimeServices: PluginRuntimeServices) {
|
||||
return pluginLoader(
|
||||
{} as unknown as Db,
|
||||
{ localPluginDir: "/nonexistent/local-plugins", enableLocalFilesystem: false, enableNpmDiscovery: false },
|
||||
runtimeServices,
|
||||
);
|
||||
}
|
||||
|
||||
describe("pluginLoader.loadAll error retry", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
});
|
||||
|
||||
it("flips an errored plugin to ready and retries its activation at boot", async () => {
|
||||
const erroredPlugin = createPluginRecord();
|
||||
mockRegistry.listByStatus.mockImplementation(async (status: string) => {
|
||||
if (status === "error") return [erroredPlugin];
|
||||
return [];
|
||||
});
|
||||
mockRegistry.updateStatus.mockResolvedValue({ ...erroredPlugin, status: "ready", lastError: null });
|
||||
|
||||
const runtimeServices = createRuntimeServices();
|
||||
const loader = createLoader(runtimeServices);
|
||||
|
||||
const result = await loader.loadAll();
|
||||
|
||||
// The flip precedes activation, and clears the stale error.
|
||||
expect(mockRegistry.updateStatus).toHaveBeenCalledExactlyOnceWith(erroredPlugin.id, { status: "ready" });
|
||||
// The retried plugin joins the boot batch; its package is unresolvable, so
|
||||
// the attempt fails and re-records a fresh error via markError.
|
||||
expect(result.total).toBe(1);
|
||||
expect(result.succeeded).toBe(0);
|
||||
expect(result.failed).toBe(1);
|
||||
expect(runtimeServices.lifecycleManager.markError).toHaveBeenCalledWith(
|
||||
erroredPlugin.id,
|
||||
expect.stringContaining("Activation failed"),
|
||||
);
|
||||
});
|
||||
|
||||
it("keeps loading ready plugins when the errored flip fails", async () => {
|
||||
const readyPlugin = createPluginRecord({
|
||||
id: "plugin-ready-1",
|
||||
pluginKey: "example.ready-plugin",
|
||||
status: "ready",
|
||||
lastError: null,
|
||||
});
|
||||
const erroredPlugin = createPluginRecord();
|
||||
mockRegistry.listByStatus.mockImplementation(async (status: string) => {
|
||||
if (status === "ready") return [readyPlugin];
|
||||
if (status === "error") return [erroredPlugin];
|
||||
return [];
|
||||
});
|
||||
mockRegistry.updateStatus.mockRejectedValue(new Error("db write refused"));
|
||||
|
||||
const loader = createLoader(createRuntimeServices());
|
||||
|
||||
const result = await loader.loadAll();
|
||||
|
||||
// The failed flip skips the retry but never aborts the boot load.
|
||||
expect(mockRegistry.updateStatus).toHaveBeenCalledExactlyOnceWith(erroredPlugin.id, { status: "ready" });
|
||||
expect(result.total).toBe(1);
|
||||
expect(result.results[0]?.plugin.id).toBe(readyPlugin.id);
|
||||
});
|
||||
|
||||
it("returns the empty result when no plugin is ready or errored", async () => {
|
||||
mockRegistry.listByStatus.mockResolvedValue([]);
|
||||
|
||||
const loader = createLoader(createRuntimeServices());
|
||||
|
||||
const result = await loader.loadAll();
|
||||
|
||||
expect(result).toEqual({ total: 0, succeeded: 0, failed: 0, results: [] });
|
||||
});
|
||||
});
|
||||
|
|
@ -1945,25 +1945,61 @@ export function pluginLoader(
|
|||
// Fetch all plugins in ready status, ordered by installOrder
|
||||
const readyPlugins = (await registry.listByStatus("ready")) as PluginRecord[];
|
||||
|
||||
if (readyPlugins.length === 0) {
|
||||
// Retry plugins stranded in error status. An activation failure is often
|
||||
// environmental — missing package dependencies, a stale build output, a
|
||||
// module that moved under a pull — and the fix lands on disk without any
|
||||
// write to the plugin row, so the row would otherwise stay dead until an
|
||||
// operator flips it back by hand. One attempt per boot cannot crash-loop
|
||||
// within a running process, and a failed attempt re-records the error
|
||||
// through the normal markError path. The flip to ready must run BEFORE
|
||||
// activation: the error status only legally transitions to ready or
|
||||
// uninstalled, so a retry that failed while still in error status could
|
||||
// not re-mark itself as errored.
|
||||
const erroredPlugins = (await registry.listByStatus("error")) as PluginRecord[];
|
||||
const retriedPlugins: PluginRecord[] = [];
|
||||
for (const plugin of erroredPlugins) {
|
||||
try {
|
||||
const flipped = (await registry.updateStatus(plugin.id, { status: "ready" })) as PluginRecord | null;
|
||||
if (flipped) retriedPlugins.push(flipped);
|
||||
} catch (err) {
|
||||
log.warn(
|
||||
{
|
||||
pluginId: plugin.id,
|
||||
pluginKey: plugin.pluginKey,
|
||||
err: err instanceof Error ? err.message : String(err),
|
||||
},
|
||||
"plugin-loader: could not queue errored plugin for a boot retry",
|
||||
);
|
||||
}
|
||||
}
|
||||
if (retriedPlugins.length > 0) {
|
||||
log.info(
|
||||
{ count: retriedPlugins.length, pluginKeys: retriedPlugins.map((plugin) => plugin.pluginKey) },
|
||||
"plugin-loader: retrying plugins that failed activation on a previous boot",
|
||||
);
|
||||
}
|
||||
|
||||
const pluginsToLoad = [...readyPlugins, ...retriedPlugins];
|
||||
|
||||
if (pluginsToLoad.length === 0) {
|
||||
log.info("plugin-loader: no ready plugins to load");
|
||||
return { total: 0, succeeded: 0, failed: 0, results: [] };
|
||||
}
|
||||
|
||||
log.info(
|
||||
{ count: readyPlugins.length },
|
||||
{ count: pluginsToLoad.length },
|
||||
"plugin-loader: found ready plugins to load",
|
||||
);
|
||||
|
||||
// Load plugins in parallel
|
||||
const results = await Promise.allSettled(
|
||||
readyPlugins.map((plugin) => activatePlugin(plugin))
|
||||
pluginsToLoad.map((plugin) => activatePlugin(plugin))
|
||||
);
|
||||
|
||||
const loadResults = results.map((r, i) => {
|
||||
if (r.status === "fulfilled") return r.value;
|
||||
return {
|
||||
plugin: readyPlugins[i]!,
|
||||
plugin: pluginsToLoad[i]!,
|
||||
success: false,
|
||||
error: String(r.reason),
|
||||
registered: { worker: false, eventSubscriptions: 0, jobs: 0, webhooks: 0, tools: 0 },
|
||||
|
|
@ -1975,7 +2011,7 @@ export function pluginLoader(
|
|||
|
||||
log.info(
|
||||
{
|
||||
total: readyPlugins.length,
|
||||
total: pluginsToLoad.length,
|
||||
succeeded,
|
||||
failed,
|
||||
},
|
||||
|
|
@ -1983,7 +2019,7 @@ export function pluginLoader(
|
|||
);
|
||||
|
||||
return {
|
||||
total: readyPlugins.length,
|
||||
total: pluginsToLoad.length,
|
||||
succeeded,
|
||||
failed,
|
||||
results: loadResults,
|
||||
|
|
@ -2447,8 +2483,9 @@ export function pluginLoader(
|
|||
);
|
||||
|
||||
if (options.markErrorOnFailure) {
|
||||
// Mark the plugin as errored in the database so it is not retried
|
||||
// automatically on next startup without operator intervention.
|
||||
// Mark the plugin as errored in the database. The running process
|
||||
// leaves it inactive; the next boot's loadAll retries it once, and the
|
||||
// lifecycle enable() path can revive it sooner by hand.
|
||||
// markError also deactivates the plugin runtime in this process.
|
||||
try {
|
||||
await lifecycleManager.markError(pluginId, `Activation failed: ${errorMessage}`);
|
||||
|
|
|
|||
Loading…
Reference in New Issue