From c7cd438b30da153bdedcc76e126a9bcd44041bef Mon Sep 17 00:00:00 2001 From: Sawyer Hood Date: Fri, 21 Aug 2026 05:45:08 +0000 Subject: [PATCH] Report a failed plugin reload instead of exit 0 `bb plugin reload` answered ok:true and exited 0 even when the reload left the plugin unusable: a previous service that ignored its abort put the plugin in degraded with nothing loaded (its CLI command gone, its closed database handles still ticking under the orphaned service), or the new sources failed to load and the previous instance was kept. `PluginService.reload` returned void, so neither POST /plugins/reload nor the CLI could tell. `loadOne` now resolves the load problem (null once the row's current sources are running or the plugin stays disabled). `reload` collects the problems per targeted plugin and returns a PluginReloadOutcome; the route answers 422 { ok: false, error, plugins } on failure, so the CLI prints the entries and the reason and exits 1, the dev loop logs "reload failed", and the app/mobile SDK callers surface the error. The builtin source watcher's reload throws the same problem. No daemon wire change. The committed-artifact rebuild in the same issue (#2029 defect 1) is the documented path-install cache policy and is tracked by #1863. Co-Authored-By: Claude --- apps/cli/src/commands/plugin.ts | 4 +- apps/server/src/routes/plugins.ts | 10 ++- .../src/services/plugins/plugin-activation.ts | 3 +- .../services/plugins/plugin-registration.ts | 3 +- .../src/services/plugins/plugin-runtime.ts | 63 ++++++++----- .../src/services/plugins/plugin-service.ts | 34 +++++-- .../skills/builtin-skills/bb-cli/SKILL.md | 4 +- .../plugins/plugin-background.test.ts | 56 +++++++++++- .../plugins/plugin-reload-route.test.ts | 88 +++++++++++++++++++ .../src/templates/bb-guide-plugins.md | 5 +- 10 files changed, 234 insertions(+), 36 deletions(-) create mode 100644 apps/server/test/services/plugins/plugin-reload-route.test.ts diff --git a/apps/cli/src/commands/plugin.ts b/apps/cli/src/commands/plugin.ts index b445c93356..08e0ebe535 100644 --- a/apps/cli/src/commands/plugin.ts +++ b/apps/cli/src/commands/plugin.ts @@ -1653,7 +1653,8 @@ export function registerPluginCommands( if (!result.ok) process.exit(1); return; } - if (!result.ok) exitWithError(result); + // A failed reload still carries the inventory: print the targeted + // entries (status and detail) before the error and the exit code. const reloaded = id === undefined ? (result.plugins ?? []) @@ -1661,6 +1662,7 @@ export function registerPluginCommands( for (const entry of reloaded) { printPlugin(entry); } + if (!result.ok) exitWithError(result); }), ); diff --git a/apps/server/src/routes/plugins.ts b/apps/server/src/routes/plugins.ts index 5d7897d6ef..e0fb957f26 100644 --- a/apps/server/src/routes/plugins.ts +++ b/apps/server/src/routes/plugins.ts @@ -450,8 +450,14 @@ export function registerPluginRoutes( app.post("/plugins/reload", async (context) => { const id = context.req.query("id") ?? undefined; - await plugins.reload(id); - return context.json({ ok: true, plugins: plugins.list() }); + const outcome = await plugins.reload(id); + // A reload that left a targeted plugin without its current sources + // running (degraded after a hung service, or the previous instance kept) + // is a failure the caller must see: `bb plugin reload` exits 1 on it and + // the dev loop logs it. The inventory rides along so the status detail + // is visible either way. + if (!outcome.ok) return context.json(outcome, 422); + return context.json(outcome); }); app.post("/plugins/:id/enable", async (context) => { diff --git a/apps/server/src/services/plugins/plugin-activation.ts b/apps/server/src/services/plugins/plugin-activation.ts index a57fb588fb..6fa7888fe5 100644 --- a/apps/server/src/services/plugins/plugin-activation.ts +++ b/apps/server/src/services/plugins/plugin-activation.ts @@ -55,7 +55,8 @@ interface PluginActivationContext { withArtifactLock: (key: string, fn: () => Promise) => Promise; withLifecycleLock: (id: string, fn: () => Promise) => Promise; disposeOne: (id: string) => Promise; - loadOne: (row: InstalledPluginRow) => Promise; + /** Resolves the load problem, or null once the row's sources are loaded. */ + loadOne: (row: InstalledPluginRow) => Promise; restoreRegistration: (row: InstalledPluginRow) => void; provenanceForRow: (row: InstalledPluginRow) => PluginProvenance; registrationMatchesForActivation: ( diff --git a/apps/server/src/services/plugins/plugin-registration.ts b/apps/server/src/services/plugins/plugin-registration.ts index ec5bdc28ee..2edd0f1823 100644 --- a/apps/server/src/services/plugins/plugin-registration.ts +++ b/apps/server/src/services/plugins/plugin-registration.ts @@ -86,7 +86,8 @@ interface PluginRegistrationContext { bundledPlugins: readonly BundledPluginRegistration[]; withLifecycleLock: (id: string, fn: () => Promise) => Promise; disposeOne: (id: string) => Promise; - loadOne: (row: InstalledPluginRow) => Promise; + /** Resolves the load problem, or null once the row's sources are loaded. */ + loadOne: (row: InstalledPluginRow) => Promise; statuses: ReadonlyMap< string, { status: PluginRuntimeStatus; detail: string | null } diff --git a/apps/server/src/services/plugins/plugin-runtime.ts b/apps/server/src/services/plugins/plugin-runtime.ts index aba937eb89..41bb2c3f2a 100644 --- a/apps/server/src/services/plugins/plugin-runtime.ts +++ b/apps/server/src/services/plugins/plugin-runtime.ts @@ -321,6 +321,12 @@ const DEV_BUILD_PROBLEM_LABELS: Record = { host: "host bundle build failed", }; +/** + * Suffix on a reload problem when the new sources did not load and the + * previous instance keeps serving (status stays "running"). + */ +const PREVIOUS_INSTANCE_KEPT = "the previous instance is still running"; + const DEFAULT_LOAD_TIMEOUT_MS = 30_000; const DEFAULT_SERVICE_STOP_TIMEOUT_MS = 5_000; const DEFAULT_SERVICE_RESTART_BASE_MS = 1_000; @@ -1354,19 +1360,32 @@ export function createPluginRuntime(context: PluginRuntimeContext) { } } - async function loadOne(row: InstalledPluginRow): Promise { + function hungServicesDetail(hung: ReadonlySet): string { + return `service ${[...hung].join(", ")} did not stop`; + } + + /** + * Load `row`'s current sources. Resolves null when they are now the running + * instance (or the plugin stays disabled by the user's switch), else the + * reason they are not: a failed first load, a failed reload that kept the + * previous instance serving, or a hung service that blocks the load. The + * status is recorded either way; the return value lets the caller that + * asked for this load (`bb plugin reload`) report the outcome instead of + * success (#2029). + */ + async function loadOne(row: InstalledPluginRow): Promise { // Refresh identity first so even a disabled/incompatible/errored plugin // keeps its name, icon, and logo in the list. await populateIdentity(row); if (!row.enabled) { setStatus(row.id, "disabled"); - return; + return null; } const previous = loaded.get(row.id); function failBeforeFactory( status: PluginRuntimeStatus, detail: string, - ): void { + ): string { // Every non-running outcome must leave a log line: without one, an // engines mismatch after a host upgrade leaves the plugin gone with // no trace outside the in-memory status (#1915). @@ -1375,49 +1394,46 @@ export function createPluginRuntime(context: PluginRuntimeContext) { logger.warn( `plugin ${row.id} reload failed (kept previous instance): ${detail}`, ); - } else { - setStatus(row.id, status, detail); - logger.warn(`plugin ${row.id} not loaded (${status}): ${detail}`); + return `${detail} (${PREVIOUS_INSTANCE_KEPT})`; } + setStatus(row.id, status, detail); + logger.warn(`plugin ${row.id} not loaded (${status}): ${detail}`); + return detail; } const hung = hungServices.get(row.id); if (hung !== undefined && hung.size > 0) { // A previous instance's service never stopped; loading now would // double-start it (design §3: degraded rather than double-starting). - const detail = `service ${[...hung].join(", ")} did not stop`; + const detail = hungServicesDetail(hung); setStatus(row.id, "degraded", detail); logger.warn(`plugin ${row.id} not loaded (degraded): ${detail}`); - return; + return detail; } try { await stat(row.rootDir); } catch { - failBeforeFactory( + return failBeforeFactory( "missing", `plugin directory not found: ${row.rootDir} (reinstall)`, ); - return; } let manifest: PluginManifest; try { manifest = await readPluginManifest(row.rootDir); } catch (error) { - failBeforeFactory( + return failBeforeFactory( "error", error instanceof Error ? error.message : String(error), ); - return; } const engineProblem = checkEngineRange(manifest) ?? checkPluginSdkRange(manifest); if (engineProblem) { - failBeforeFactory("incompatible", engineProblem); - return; + return failBeforeFactory("incompatible", engineProblem); } const artifactProblem = await packagedBuiltinArtifactProblem(row, manifest); if (artifactProblem !== null) { - failBeforeFactory("incompatible", artifactProblem); - return; + return failBeforeFactory("incompatible", artifactProblem); } // Build candidate assets without publishing them; a failed reload keeps // the previous backend and frontend registration sets together. @@ -1430,8 +1446,7 @@ export function createPluginRuntime(context: PluginRuntimeContext) { hostArtifactProblem = error instanceof Error ? error.message : String(error); if (previous !== undefined) { - failBeforeFactory("error", hostArtifactProblem); - return; + return failBeforeFactory("error", hostArtifactProblem); } } // Branding refresh rides every load too, so `bb plugin reload` picks up a @@ -1615,7 +1630,9 @@ export function createPluginRuntime(context: PluginRuntimeContext) { logger.warn( `plugin ${row.id} failed to load: ${statuses.get(row.id)?.detail}`, ); - return; + return previous !== undefined + ? `${message} (${PREVIOUS_INSTANCE_KEPT})` + : message; } if (hostArtifactProblem !== null) { rollbackGeneration?.(); @@ -1640,7 +1657,7 @@ export function createPluginRuntime(context: PluginRuntimeContext) { handle.invalidate(); setStatus(row.id, "error", hostArtifactProblem); logger.warn(`plugin ${row.id} failed to load: ${hostArtifactProblem}`); - return; + return hostArtifactProblem; } const plugin: LoadedPlugin = { manifest, @@ -1658,7 +1675,8 @@ export function createPluginRuntime(context: PluginRuntimeContext) { }; if (previous !== undefined) { await disposePluginInstance(row.id, previous); - if ((hungServices.get(row.id)?.size ?? 0) > 0) { + const hungAfterDispose = hungServices.get(row.id); + if (hungAfterDispose !== undefined && hungAfterDispose.size > 0) { loaded.delete(row.id); deps.sharedPorts?.clearDeclarationsForOwner(row.id); for (const database of handle.databaseHandles.splice(0)) { @@ -1669,7 +1687,7 @@ export function createPluginRuntime(context: PluginRuntimeContext) { } } handle.invalidate(); - return; + return hungServicesDetail(hungAfterDispose); } } // One map replacement is the registration commit point. Until this line, @@ -1720,6 +1738,7 @@ export function createPluginRuntime(context: PluginRuntimeContext) { ); } logger.info(`plugin ${row.id}@${manifest.version} loaded`); + return null; } async function disposePluginInstance( diff --git a/apps/server/src/services/plugins/plugin-service.ts b/apps/server/src/services/plugins/plugin-service.ts index d3159f0793..acd3301e09 100644 --- a/apps/server/src/services/plugins/plugin-service.ts +++ b/apps/server/src/services/plugins/plugin-service.ts @@ -145,6 +145,18 @@ export interface PluginSkillRootContribution { rootPath: string; } +/** + * Result of `reload`. `plugins` is the full inventory after the reload. A + * reload fails when any targeted plugin is not running its current sources + * afterwards: the new sources did not load (the previous instance keeps + * serving), or a service of the previous instance never stopped and the + * plugin is degraded with nothing loaded (#2029). A plugin the user disabled + * stays disabled and is not a failure. + */ +export type PluginReloadOutcome = + | { ok: true; plugins: PluginListEntry[] } + | { ok: false; error: string; plugins: PluginListEntry[] }; + /** * `fs.watch` is allowed to omit the changed filename. The dev loop still has * to reload in that case; `.` is a non-ignored synthetic path representing an @@ -256,7 +268,8 @@ export interface PluginService { id: string, enabled: boolean, ): Promise; - reload(id?: string): Promise; + /** Reload one plugin, or every plugin; see PluginReloadOutcome. */ + reload(id?: string): Promise; /** Live API handle for a running plugin (used by later phases and tests). */ getApi(id: string): BbPluginApi | undefined; /** @@ -1610,14 +1623,17 @@ export function createPluginService(deps: PluginServiceDeps): PluginService { } }, reloadPlugin: async () => { - await withLifecycleLock(row.id, async () => { + const problem = await withLifecycleLock(row.id, async () => { const current = getInstalledPlugin(deps.db, row.id); - if (current === undefined) return; + if (current === undefined) return null; await disposeOne(row.id); - await loadOne(current); + return loadOne(current); }); await syncCliSkill(); notifyPluginsChanged(); + // The dev loop logs a thrown reload as "reload failed: …" + // instead of "reloaded" while the plugin is not running. + if (problem !== null) throw new Error(problem); }, log: (message) => logger.info(`plugin ${row.id}: ${message}`), }); @@ -1824,11 +1840,19 @@ export function createPluginService(deps: PluginServiceDeps): PluginService { const rows = listInstalledPlugins(deps.db).filter( (row) => id === undefined || row.id === id, ); + const failures: string[] = []; for (const row of rows.sort((a, b) => a.id.localeCompare(b.id))) { - await withLifecycleLock(row.id, () => loadOne(row)); + const problem = await withLifecycleLock(row.id, () => loadOne(row)); + if (problem !== null) { + failures.push(`plugin "${row.id}" reload failed: ${problem}`); + } } await syncCliSkill(); notifyPluginsChanged(); + const plugins = list(); + return failures.length === 0 + ? { ok: true, plugins } + : { ok: false, error: failures.join("; "), plugins }; }, getApi(id) { diff --git a/apps/server/src/services/skills/builtin-skills/bb-cli/SKILL.md b/apps/server/src/services/skills/builtin-skills/bb-cli/SKILL.md index 45c87d9e28..05373fc9f6 100644 --- a/apps/server/src/services/skills/builtin-skills/bb-cli/SKILL.md +++ b/apps/server/src/services/skills/builtin-skills/bb-cli/SKILL.md @@ -858,7 +858,9 @@ them by mixing ink into canvas), the `--primary` accent, the secondary text tier repository subdirectory for a nested plugin, the semver range with its tag prefix and resolved tag for a Git range install, engine ranges, install time, integrity/registry details, and recent activation history. - - `bb plugin enable|disable `, `bb plugin reload [id]`, + - `bb plugin enable|disable `, `bb plugin reload [id]` (exits 1 when a + reloaded plugin does not come up on its current sources: the previous + instance was kept, or it is degraded because a service ignored its abort), `bb plugin remove ` (deletes the plugin's settings, secrets, and schedules; managed git/npm files are deleted, local path sources stay on disk, builtin removals are remembered). diff --git a/apps/server/test/services/plugins/plugin-background.test.ts b/apps/server/test/services/plugins/plugin-background.test.ts index 4346b92716..1938fcc5ef 100644 --- a/apps/server/test/services/plugins/plugin-background.test.ts +++ b/apps/server/test/services/plugins/plugin-background.test.ts @@ -273,18 +273,70 @@ describe("plugin background services", () => { `, }); await service.installPath(rootDir); - await service.reload("stubborn"); + const outcome = await service.reload("stubborn"); const entry = service.list().find((p) => p.id === "stubborn"); expect(entry?.status).toBe("degraded"); expect(entry?.statusDetail).toContain("service socket did not stop"); // Not re-loaded: that would double-start the hung service. expect(service.getApi("stubborn")).toBeUndefined(); + // The plugin is unusable after this reload (#2029): the outcome must say + // so instead of resolving as success while `bb stubborn` is gone. + expect(outcome).toEqual({ + ok: false, + error: 'plugin "stubborn" reload failed: service socket did not stop', + plugins: service.list(), + }); // Still degraded on a second reload attempt. - await service.reload("stubborn"); + const again = await service.reload("stubborn"); expect(service.list().find((p) => p.id === "stubborn")?.status).toBe( "degraded", ); + expect(again.ok).toBe(false); + }); + + it("reports a failed reload that kept the previous instance", async () => { + const rootDir = await writePlugin(workDir, { + name: "bb-plugin-keeper", + serverSource: ` + export default function plugin(bb: any) { + bb.cli.register({ name: "keeper", summary: "keeper", run() { return { exitCode: 0, stdout: "ok" }; } }); + } + `, + }); + const installed = await service.installPath(rootDir); + expect(installed.status).toBe("running"); + const healthy = await service.reload("keeper"); + expect(healthy.ok).toBe(true); + + // A broken edit: the new sources do not load, so the host keeps the + // previous instance serving. The reload still did not apply. + await writeFile( + join(rootDir, "server.ts"), + `export default function plugin() { throw new Error("boom on load"); }`, + ); + const outcome = await service.reload("keeper"); + const entry = service.list().find((p) => p.id === "keeper"); + expect(entry?.status).toBe("running"); + expect(entry?.statusDetail).toBe("reload failed: boom on load"); + expect(service.getApi("keeper")).toBeDefined(); + expect(outcome.ok).toBe(false); + if (outcome.ok) throw new Error("unreachable"); + expect(outcome.error).toBe( + 'plugin "keeper" reload failed: boom on load (the previous instance is still running)', + ); + + // Reloading every plugin reports the same failure; the fixed plugin + // reloads cleanly. + expect((await service.reload()).ok).toBe(false); + await writeFile( + join(rootDir, "server.ts"), + `export default function plugin(bb: any) { bb.cli.register({ name: "keeper", summary: "keeper", run() { return { exitCode: 0, stdout: "ok" }; } }); }`, + ); + expect((await service.reload("keeper")).ok).toBe(true); + expect( + service.list().find((p) => p.id === "keeper")?.statusDetail, + ).toBeNull(); }); it("restarts a crashed service with backoff", async () => { diff --git a/apps/server/test/services/plugins/plugin-reload-route.test.ts b/apps/server/test/services/plugins/plugin-reload-route.test.ts new file mode 100644 index 0000000000..fcf0b49f23 --- /dev/null +++ b/apps/server/test/services/plugins/plugin-reload-route.test.ts @@ -0,0 +1,88 @@ +import { mkdir, writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { + createTestAppHarness, + type TestAppHarness, +} from "../../helpers/test-app.js"; + +// The harness config uses serverPort 3334, so this host is on the local-app +// origin allowlist the "local" auth mode enforces. +const BASE = "http://127.0.0.1:3334"; + +const HEALTHY_SOURCE = `export default function plugin(bb: any) { + bb.cli.register({ name: "keeper", summary: "keeper", run() { return { exitCode: 0, stdout: "ok" }; } }); +} +`; +const BROKEN_SOURCE = `export default function plugin() { throw new Error("boom on load"); } +`; + +describe("POST /plugins/reload outcome", () => { + let harness: TestAppHarness; + let rootDir: string; + + beforeEach(async () => { + harness = await createTestAppHarness(); + rootDir = join(harness.config.dataDir, "fixtures", "bb-plugin-keeper"); + await mkdir(rootDir, { recursive: true }); + await writeFile( + join(rootDir, "package.json"), + JSON.stringify({ + name: "bb-plugin-keeper", + version: "0.1.0", + bb: { + name: "Keeper", + description: "Reload outcome fixture.", + branding: { icon: "Zap" }, + server: "./server.ts", + }, + }), + ); + await writeFile(join(rootDir, "server.ts"), HEALTHY_SOURCE); + const entry = await harness.pluginService.installPath(rootDir); + expect(entry.status).toBe("running"); + }); + + afterEach(async () => { + await harness.cleanup(); + }); + + it("answers ok with the plugin list when the reload applied", async () => { + const response = await harness.app.request( + `${BASE}/api/v1/plugins/reload?id=keeper`, + { method: "POST" }, + ); + expect(response.status).toBe(200); + const body: unknown = await response.json(); + expect(body).toMatchObject({ ok: true }); + expect(body).toMatchObject({ + plugins: expect.arrayContaining([ + expect.objectContaining({ id: "keeper", status: "running" }), + ]), + }); + }); + + it("answers ok:false with the load problem when the new sources did not load (#2029)", async () => { + await writeFile(join(rootDir, "server.ts"), BROKEN_SOURCE); + const response = await harness.app.request( + `${BASE}/api/v1/plugins/reload?id=keeper`, + { method: "POST" }, + ); + // `bb plugin reload` exits 1 through this structured failure; the + // previous instance keeps serving, which the entry list shows. + expect(response.status).toBe(422); + const body: unknown = await response.json(); + expect(body).toMatchObject({ + ok: false, + error: + 'plugin "keeper" reload failed: boom on load (the previous instance is still running)', + plugins: expect.arrayContaining([ + expect.objectContaining({ + id: "keeper", + status: "running", + statusDetail: "reload failed: boom on load", + }), + ]), + }); + }); +}); diff --git a/packages/templates/src/templates/bb-guide-plugins.md b/packages/templates/src/templates/bb-guide-plugins.md index 075a1ca4d1..f3f508c0b3 100644 --- a/packages/templates/src/templates/bb-guide-plugins.md +++ b/packages/templates/src/templates/bb-guide-plugins.md @@ -218,7 +218,10 @@ added/updated/unchanged counts. tag, engine ranges, install time, and recent activation history bb plugin enable|disable Load or unload an installed plugin - bb plugin reload [id] Re-run factories against current sources + bb plugin reload [id] Re-run factories against current sources. + Exits 1 when a plugin does not come up on + them (previous instance kept, or degraded + because a service ignored its abort) bb plugin config [set | unset ] Show or change a plugin's declared settings bb plugin logs [-n N] [-f] Print (or follow) a plugin's bb.log output