From f0dd7eecaa227c57d0bc086002d3738e5de25495 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 18 Aug 2026 02:08:56 -0700 Subject: [PATCH] fix: preserve plugin update recovery after commit (#125739) --- src/cli/plugins-cli.update.test.ts | 73 +++++++++++++++++------------- src/cli/plugins-update-command.ts | 28 ++++++------ 2 files changed, 54 insertions(+), 47 deletions(-) diff --git a/src/cli/plugins-cli.update.test.ts b/src/cli/plugins-cli.update.test.ts index b31f82840edf..92f0f5cd50ba 100644 --- a/src/cli/plugins-cli.update.test.ts +++ b/src/cli/plugins-cli.update.test.ts @@ -895,6 +895,7 @@ describe("plugins cli update", () => { expect(notifyGatewayPluginMetadataChangedMock).not.toHaveBeenCalled(); expect(rollback).toHaveBeenCalledTimes(1); expect(commit).not.toHaveBeenCalled(); + expect(pluginsCliRuntimeLogs.join("\n")).not.toContain("Updated"); }); it("rolls back persisted install records when included config changes during a records-only update", async () => { @@ -1595,24 +1596,23 @@ describe("plugins cli update", () => { it("keeps durable state when transaction cleanup fails after the write", async () => { const cfg = { plugins: { - installs: { - alpha: { - source: "npm", - spec: "@openclaw/alpha@1.0.0", - }, + entries: { + alpha: { enabled: true }, }, }, } as OpenClawConfig; - const nextConfig = { - plugins: { - installs: { - alpha: { - source: "npm", - spec: "@openclaw/alpha@1.1.0", - }, - }, + const previousRecords = { + alpha: { + source: "npm" as const, + spec: "@openclaw/alpha@1.0.0", }, - } as OpenClawConfig; + }; + const nextRecords = { + alpha: { + source: "npm" as const, + spec: "@openclaw/alpha@1.1.0", + }, + }; const runtimeConfig = { ...cfg, messages: { @@ -1620,7 +1620,11 @@ describe("plugins cli update", () => { }, } as OpenClawConfig; const nextRuntimeConfig = { - ...nextConfig, + ...runtimeConfig, + plugins: { + ...runtimeConfig.plugins, + installs: nextRecords, + }, messages: runtimeConfig.messages, } as OpenClawConfig; primeUpdateConfigSnapshot({ @@ -1630,7 +1634,7 @@ describe("plugins cli update", () => { "/tmp/plugins.json5": "plugins-start-hash", }, }); - setInstalledPluginIndexInstallRecords(cfg.plugins?.installs ?? {}); + setInstalledPluginIndexInstallRecords(previousRecords); const rollback = vi.fn(async () => undefined); const failedCommit = vi.fn(async () => { throw new Error("cleanup failed"); @@ -1651,30 +1655,35 @@ describe("plugins cli update", () => { config: nextRuntimeConfig, }); - await expect(runPluginsCommand(["plugins", "update", "alpha"])).rejects.toThrow( - "Plugin install transaction commit failed", - ); + await runPluginsCommand(["plugins", "update", "alpha"]); const updateParams = expectSingleCallParams(updateNpmInstalledPluginsMock); - expect(updateParams.config).toEqual(runtimeConfig); + expect(updateParams.config).toEqual({ + ...runtimeConfig, + plugins: { + ...runtimeConfig.plugins, + installs: previousRecords, + }, + }); expect(updateParams.pluginIds).toEqual(["alpha"]); expect(updateParams.dryRun).toBe(false); - expectInstallRecordsWrittenWithLease(nextConfig.plugins?.installs, {}); + expectInstallRecordsWrittenWithLease(nextRecords, cfg); expect(updateNpmInstalledHookPacksMock).not.toHaveBeenCalled(); - expect(configWriteMock).toHaveBeenCalledWith({}); - expect(replaceConfigFileMock).toHaveBeenCalledWith({ - nextConfig: {}, - baseHash: "update-config", - writeOptions: expect.objectContaining({ - includeFileHashesForWrite: { - "/tmp/plugins.json5": "plugins-start-hash", - }, - }), - }); + expect(configWriteMock).not.toHaveBeenCalled(); + expect(replaceConfigFileMock).not.toHaveBeenCalled(); expect(failedCommit).toHaveBeenCalledOnce(); expect(remainingCommit).toHaveBeenCalledOnce(); expect(rollback).not.toHaveBeenCalled(); - expect(refreshPluginRegistryMock).not.toHaveBeenCalled(); + expect(refreshPluginRegistryMock).toHaveBeenCalledWith({ + config: cfg, + installRecords: nextRecords, + reason: "source-changed", + }); + expect(notifyGatewayPluginMetadataChangedMock).toHaveBeenCalledWith(runtimeConfig); + expect(pluginsCliRuntimeLogs.join("\n")).toContain("Plugin update committed"); + expect(pluginsCliRuntimeLogs).toContain("Updated alpha -> 1.1.0"); + expect(pluginsCliRuntimeLogs.join("\n")).toContain("Restart is required"); + expectRestartNoticeLogged(); }); it("exits non-zero when a plugin update reports an error after persisting successes", async () => { diff --git a/src/cli/plugins-update-command.ts b/src/cli/plugins-update-command.ts index 72be53980ab6..323bd679d027 100644 --- a/src/cli/plugins-update-command.ts +++ b/src/cli/plugins-update-command.ts @@ -445,10 +445,7 @@ async function runPluginUpdateCommandUnlocked(params: RunPluginUpdateCommandPara await settlePluginInstallTransactions(deferredPluginTransactions, "rollback"); throw error; } - const settlePluginTransactions = async (action: "commit" | "rollback") => { - await settlePluginInstallTransactions(deferredPluginTransactions, action); - }; - let packageCommitFinalized = false; + let packageUpdatePersisted = false; try { if (pluginSelection.pluginIds.length > 0 && pluginResult.changed && !params.opts.dryRun) { const nextInstallRecords = pluginResult.config.plugins?.installs ?? {}; @@ -464,7 +461,7 @@ async function runPluginUpdateCommandUnlocked(params: RunPluginUpdateCommandPara installOwnerMigrations: resolvePluginInstallOwnerMigrations(pluginResult), }); if (!reconciled.ok) { - await settlePluginTransactions("rollback"); + await settlePluginInstallTransactions(deferredPluginTransactions, "rollback"); defaultRuntime.error(reconciled.error); return defaultRuntime.exit(1); } @@ -498,11 +495,6 @@ async function runPluginUpdateCommandUnlocked(params: RunPluginUpdateCommandPara }) : { config: pluginResult.config, changed: false, outcomes: [] }; - const outcomeSummary = logPluginUpdateOutcomes({ - outcomes: [...pluginResult.outcomes, ...hookResult.outcomes], - log: (message) => defaultRuntime.log(message), - }); - if (!params.opts.dryRun && (pluginResult.changed || hookResult.changed)) { const sourceSnapshot = mutationSnapshot ?? (await sourceSnapshotPromise); if (pluginResult.changed) { @@ -516,7 +508,7 @@ async function runPluginUpdateCommandUnlocked(params: RunPluginUpdateCommandPara !currentSnapshot.ok || !isDeepStrictEqual([...currentSnapshot.value], [...packageUpdateSnapshot]) ) { - await settlePluginTransactions("rollback"); + await settlePluginInstallTransactions(deferredPluginTransactions, "rollback"); defaultRuntime.error( currentSnapshot.ok ? "Plugin package ownership changed during update; no config or index changes were committed. Refresh the plugin registry and retry." @@ -575,8 +567,10 @@ async function runPluginUpdateCommandUnlocked(params: RunPluginUpdateCommandPara writeOptions: sourceSnapshot?.writeOptions, }); } - packageCommitFinalized = true; - await settlePluginTransactions("commit"); + packageUpdatePersisted = true; + await settlePluginInstallTransactions(deferredPluginTransactions, "commit").catch(() => + logger.warn("Plugin update committed, but cleanup failed. Restart is required."), + ); if (pluginResult.changed) { await refreshPluginRegistryAfterConfigMutation({ config: nextConfig, @@ -592,12 +586,16 @@ async function runPluginUpdateCommandUnlocked(params: RunPluginUpdateCommandPara defaultRuntime.log("Restart the gateway to load plugins and hooks."); } + const outcomeSummary = logPluginUpdateOutcomes({ + outcomes: [...pluginResult.outcomes, ...hookResult.outcomes], + log: (message) => defaultRuntime.log(message), + }); if (outcomeSummary.hasErrors) { defaultRuntime.exit(1); } } catch (error) { - if (!packageCommitFinalized) { - await settlePluginTransactions("rollback"); + if (!packageUpdatePersisted) { + await settlePluginInstallTransactions(deferredPluginTransactions, "rollback"); } throw error; }