From 627eef7cbde92c673685ff8c275d9d079a5ff255 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Sun, 23 Aug 2026 16:52:42 -0700 Subject: [PATCH] fix(plugins): retry errored plugins at boot instead of leaving them dead (#12054) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- .../plugin-loader-error-retry.test.ts | 151 ++++++++++++++++++ server/src/services/plugin-loader.ts | 53 +++++- 2 files changed, 196 insertions(+), 8 deletions(-) create mode 100644 server/src/__tests__/plugin-loader-error-retry.test.ts diff --git a/server/src/__tests__/plugin-loader-error-retry.test.ts b/server/src/__tests__/plugin-loader-error-retry.test.ts new file mode 100644 index 0000000000..bcde5be425 --- /dev/null +++ b/server/src/__tests__/plugin-loader-error-retry.test.ts @@ -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 = {}) { + 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: [] }); + }); +}); diff --git a/server/src/services/plugin-loader.ts b/server/src/services/plugin-loader.ts index 7e24046e38..52d7843031 100644 --- a/server/src/services/plugin-loader.ts +++ b/server/src/services/plugin-loader.ts @@ -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}`);