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