From 29bce4a918859e705f5d261c8395485c41699396 Mon Sep 17 00:00:00 2001 From: HeikoGr <20295490+HeikoGr@users.noreply.github.com> Date: Fri, 23 Oct 2026 21:54:38 +0200 Subject: [PATCH 1/2] fix(updatenotification): apply all pending module updates Run pending updates one after another instead of stopping after the first one, release the update lock in a finally block, and restart once after all updates when updateAutorestart is enabled. Also show the done/error lines on the mirror (updates was an array with string keys), fix the MagicMirror check in applyCommand and drop the unhandled UPDATE_ERROR notification. Fixes #4311 Co-Authored-By: Claude Opus 5.5 --- .../updatenotification/node_helper.js | 2 +- .../updatenotification/update_helper.js | 52 +++-- .../updatenotification/updatenotification.js | 3 +- tests/unit/functions/update_helper_spec.js | 193 +++++++++++++++++- .../updatenotification/node_helper_spec.js | 26 +++ .../updatenotification_spec.js | 77 +++++++ 6 files changed, 333 insertions(+), 20 deletions(-) create mode 100644 tests/unit/modules/default/updatenotification/updatenotification_spec.js diff --git a/defaultmodules/updatenotification/node_helper.js b/defaultmodules/updatenotification/node_helper.js index bdca76987f..4728e884f4 100644 --- a/defaultmodules/updatenotification/node_helper.js +++ b/defaultmodules/updatenotification/node_helper.js @@ -92,7 +92,7 @@ export default class extends NodeHelper { if (updates.length && this.updateHelper) { const updateResult = await this.updateHelper.parse(updates); for (const update of updateResult) { - if (update.inProgress) { + if (update.updated || update.error) { this.sendSocketNotification("UPDATE_STATUS", update); } } diff --git a/defaultmodules/updatenotification/update_helper.js b/defaultmodules/updatenotification/update_helper.js index 0e6404170f..78bd1d1c8d 100644 --- a/defaultmodules/updatenotification/update_helper.js +++ b/defaultmodules/updatenotification/update_helper.js @@ -7,8 +7,9 @@ const execAsync = promisify(exec); /** * Updates third-party modules from commands defined in the configuration. * - * Only one module is updated at a time. Each update result reports whether the - * update succeeded and whether MagicMirror needs to be restarted. + * Pending updates run one after another. Each update result reports whether the + * update succeeded and whether MagicMirror needs to be restarted. A module is + * updated at most once per MagicMirror run, also when its update failed. */ class Updater { @@ -30,12 +31,33 @@ class Updater { } /** - * Parse modules and apply the first available update. + * Parse modules and apply all available updates one after another. * @param {Array<{module: string}>} modules Modules that may have updates. * @returns {Promise>} The current update state for each module. */ async parse (modules) { - await Promise.all(modules.map((module) => this.processModule(module))); + if (this.updating) { + Log.debug("Update run already in progress, skipping this check."); + return Object.values(this.moduleList); + } + + this.updating = true; + let updatedCount = 0; + try { + for (const module of modules) { + if (await this.processModule(module)) { + updatedCount++; + } + } + } finally { + this.updating = false; + } + + if (updatedCount && this.autoRestart) { + Log.info(`${updatedCount} module(s) updated, restarting MagicMirror...`); + setTimeout(() => this.nodeRestart(), 3000); + } + const updater = Object.values(this.moduleList); Log.debug("Update Result:", updater); return updater; @@ -44,17 +66,21 @@ class Updater { /** * Initialize and process one module update. * @param {{module: string}} module Module to process. - * @returns {Promise} Resolves when processing is complete. + * @returns {Promise} Whether the module was updated successfully. */ async processModule (module) { const moduleState = await this.getModuleState(module); - if (moduleState.inProgress || this.updating || !moduleState.updateCommand) { - return; + if (!moduleState.updateCommand || moduleState.updated || moduleState.error) { + return false; } - this.updating = true; moduleState.inProgress = true; - Object.assign(moduleState, await this.updateProcess(moduleState)); + try { + Object.assign(moduleState, await this.updateProcess(moduleState)); + } finally { + moduleState.inProgress = false; + } + return moduleState.updated; } /** @@ -112,7 +138,8 @@ class Updater { } /** - * Apply the successful update result and schedule an automatic restart when configured. + * Apply the successful update result. The automatic restart is scheduled by + * parse() once all pending updates are done. * @param {{name: string}} module Updated module. * @param {{updated: boolean, needRestart: boolean}} result Result to update. * @param {string} stdout Update command output. @@ -123,7 +150,6 @@ class Updater { result.updated = true; if (this.autoRestart) { Log.info("Update done"); - setTimeout(() => this.nodeRestart(), 3000); } else { Log.info("Update done, don't forget to restart MagicMirror!"); result.needRestart = true; @@ -177,11 +203,11 @@ class Updater { /** * Find the configured update command for a module. - * @param {{module: string}} module Module configuration. + * @param {string} module Module name. * @returns {string|null} The configured command, if one exists. */ applyCommand (module) { - if (Updater.isMagicMirror(module.module) || !this.updates.length) { + if (Updater.isMagicMirror(module) || !this.updates.length) { return null; } let command = null; diff --git a/defaultmodules/updatenotification/updatenotification.js b/defaultmodules/updatenotification/updatenotification.js index f3d661fc58..fbeb64eac4 100644 --- a/defaultmodules/updatenotification/updatenotification.js +++ b/defaultmodules/updatenotification/updatenotification.js @@ -13,7 +13,7 @@ Module.register("updatenotification", { suspended: false, moduleList: {}, needRestart: false, - updates: [], + updates: {}, start () { Log.info(`Starting module: ${this.name}`); @@ -109,7 +109,6 @@ Module.register("updatenotification", { }; if (payload.error) { - this.sendSocketNotification("UPDATE_ERROR", payload.name); this.updates[payload.name].done = false; } else { if (payload.updated) { diff --git a/tests/unit/functions/update_helper_spec.js b/tests/unit/functions/update_helper_spec.js index c58c821bdf..77ec93783f 100644 --- a/tests/unit/functions/update_helper_spec.js +++ b/tests/unit/functions/update_helper_spec.js @@ -1,5 +1,5 @@ import { spawn } from "node:child_process"; -import { mkdtempSync, mkdirSync, rmSync } from "node:fs"; +import { existsSync, mkdtempSync, mkdirSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; @@ -87,7 +87,7 @@ describe("UpdateHelper", () => { expect(result).toEqual({ error: true, updated: false, needRestart: false }); }); - it("schedules node restart when autoRestart is enabled", async () => { + it("does not restart from updateProcess when autoRestart is enabled", async () => { vi.useFakeTimers(); const moduleName = "MMM-Test"; @@ -105,10 +105,195 @@ describe("UpdateHelper", () => { expect(result.error).toBe(false); expect(result.updated).toBe(true); expect(result.needRestart).toBe(false); - expect(nodeRestartSpy).not.toHaveBeenCalled(); vi.advanceTimersByTime(3000); - expect(nodeRestartSpy).toHaveBeenCalledTimes(1); + expect(nodeRestartSpy).not.toHaveBeenCalled(); + }); + + describe("parse", () => { + + /** + * Builds an updates config with one command per module. + * @param {string[]} moduleNames - Module names. + * @param {string} command - Update command for each module. + * @returns {Array} Updates config. + */ + const updatesFor = (moduleNames, command = "git pull") => moduleNames.map((name) => ({ [name]: command })); + + /** + * Replaces updateProcess with a stub that records the call order and overlap. + * @param {object} updater - UpdateHelper instance. + * @param {object} results - Optional result per module name. + * @returns {{ calls: string[], maxRunning: () => number }} Recorded calls. + */ + const stubUpdateProcess = (updater, results = {}) => { + const calls = []; + let running = 0; + let maxRunning = 0; + vi.spyOn(updater, "updateProcess").mockImplementation(async (module) => { + calls.push(module.name); + running++; + maxRunning = Math.max(maxRunning, running); + await new Promise((resolve) => setTimeout(resolve, 5)); + running--; + return results[module.name] ?? { error: false, updated: true, needRestart: true }; + }); + return { calls, maxRunning: () => maxRunning }; + }; + + it("updates every module with a configured command, one after another", async () => { + const names = ["MMM-A", "MMM-B", "MMM-C"]; + const updater = await createUpdater({ updates: updatesFor(names) }); + const recorded = stubUpdateProcess(updater); + + const result = await updater.parse(names.map((module) => ({ module }))); + + expect(recorded.calls).toEqual(names); + expect(recorded.maxRunning()).toBe(1); + expect(result.map((state) => [state.name, state.updated, state.inProgress])).toEqual([ + ["MMM-A", true, false], + ["MMM-B", true, false], + ["MMM-C", true, false] + ]); + expect(updater.updating).toBe(false); + }); + + it("runs the real update commands of all modules", async () => { + const names = ["MMM-A", "MMM-B"]; + const root = mkdtempSync(join(tmpdir(), "mm-updater-")); + tempRoots.push(root); + for (const name of names) { + mkdirSync(join(root, "modules", name), { recursive: true }); + } + global.root_path = root; + + const command = `"${process.execPath}" -e "require('node:fs').writeFileSync('updated', '')"`; + const updater = await createUpdater({ updates: updatesFor(names, command) }); + + const result = await updater.parse(names.map((module) => ({ module }))); + + for (const name of names) { + expect(existsSync(join(root, "modules", name, "updated"))).toBe(true); + } + expect(result.every((state) => state.updated && state.needRestart)).toBe(true); + }); + + it("skips modules without an update command", async () => { + const updater = await createUpdater({ updates: updatesFor(["MMM-B"]) }); + const recorded = stubUpdateProcess(updater); + + const result = await updater.parse([{ module: "MMM-A" }, { module: "MMM-B" }]); + + expect(recorded.calls).toEqual(["MMM-B"]); + expect(result.find((state) => state.name === "MMM-A").updated).toBe(false); + }); + + it("continues with the next module when an update fails", async () => { + const names = ["MMM-A", "MMM-B"]; + const updater = await createUpdater({ updates: updatesFor(names) }); + const recorded = stubUpdateProcess(updater, { "MMM-A": { error: true, updated: false, needRestart: false } }); + + const result = await updater.parse(names.map((module) => ({ module }))); + + expect(recorded.calls).toEqual(names); + expect(result.map((state) => [state.name, state.error, state.updated])).toEqual([ + ["MMM-A", true, false], + ["MMM-B", false, true] + ]); + }); + + it("updates modules found in later checks", async () => { + const updater = await createUpdater({ updates: updatesFor(["MMM-A", "MMM-B"]) }); + const recorded = stubUpdateProcess(updater); + + await updater.parse([{ module: "MMM-A" }]); + await updater.parse([{ module: "MMM-B" }]); + + expect(recorded.calls).toEqual(["MMM-A", "MMM-B"]); + }); + + it("does not retry updated or failed modules in later checks", async () => { + const names = ["MMM-A", "MMM-B"]; + const updater = await createUpdater({ updates: updatesFor(names) }); + const recorded = stubUpdateProcess(updater, { "MMM-B": { error: true, updated: false, needRestart: false } }); + + await updater.parse(names.map((module) => ({ module }))); + await updater.parse(names.map((module) => ({ module }))); + + expect(recorded.calls).toEqual(names); + }); + + it("does not start a second run while one is in progress", async () => { + const names = ["MMM-A", "MMM-B"]; + const updater = await createUpdater({ updates: updatesFor(names) }); + const recorded = stubUpdateProcess(updater); + + const firstRun = updater.parse(names.map((module) => ({ module }))); + const secondResult = await updater.parse(names.map((module) => ({ module }))); + await firstRun; + + expect(Array.isArray(secondResult)).toBe(true); + expect(recorded.calls).toEqual(names); + }); + + it("releases the update lock when an update throws", async () => { + const updater = await createUpdater({ updates: updatesFor(["MMM-A"]) }); + vi.spyOn(updater, "updateProcess").mockRejectedValueOnce(new Error("boom")); + + await expect(updater.parse([{ module: "MMM-A" }])).rejects.toThrow("boom"); + + expect(updater.updating).toBe(false); + expect(updater.moduleList["MMM-A"].inProgress).toBe(false); + }); + + it("restarts once after all updates when autoRestart is enabled", async () => { + const names = ["MMM-A", "MMM-B"]; + const updater = await createUpdater({ updates: updatesFor(names), updateAutorestart: true }); + const nodeRestartSpy = vi.spyOn(updater, "nodeRestart").mockImplementation(() => {}); + const recorded = stubUpdateProcess(updater, { + "MMM-A": { error: false, updated: true, needRestart: false }, + "MMM-B": { error: false, updated: true, needRestart: false } + }); + + vi.useFakeTimers(); + const run = updater.parse(names.map((module) => ({ module }))); + await vi.advanceTimersByTimeAsync(5); + expect(recorded.calls).toEqual(["MMM-A", "MMM-B"]); + expect(nodeRestartSpy).not.toHaveBeenCalled(); + + await vi.advanceTimersByTimeAsync(5); + await run; + await vi.advanceTimersByTimeAsync(3000); + + expect(nodeRestartSpy).toHaveBeenCalledTimes(1); + }); + + it("does not restart when no update succeeded", async () => { + vi.useFakeTimers(); + const updater = await createUpdater({ updates: updatesFor(["MMM-A"]), updateAutorestart: true }); + const nodeRestartSpy = vi.spyOn(updater, "nodeRestart").mockImplementation(() => {}); + vi.spyOn(updater, "updateProcess").mockResolvedValue({ error: true, updated: false, needRestart: false }); + + await updater.parse([{ module: "MMM-A" }]); + await vi.advanceTimersByTimeAsync(3000); + + expect(nodeRestartSpy).not.toHaveBeenCalled(); + }); + }); + + describe("applyCommand", () => { + it("returns the configured command for a module", async () => { + const updater = await createUpdater({ updates: [{ "MMM-A": "git pull" }, { "MMM-B": "npm ci" }] }); + + expect(updater.applyCommand("MMM-B")).toBe("npm ci"); + expect(updater.applyCommand("MMM-C")).toBeNull(); + }); + + it("never returns a command for MagicMirror itself", async () => { + const updater = await createUpdater({ updates: [{ MagicMirror: "git pull" }] }); + + expect(updater.applyCommand("MagicMirror")).toBeNull(); + }); }); describe("nodeRestart", () => { diff --git a/tests/unit/modules/default/updatenotification/node_helper_spec.js b/tests/unit/modules/default/updatenotification/node_helper_spec.js index 58b582c4b1..216c24ec12 100644 --- a/tests/unit/modules/default/updatenotification/node_helper_spec.js +++ b/tests/unit/modules/default/updatenotification/node_helper_spec.js @@ -86,4 +86,30 @@ describe("updatenotification node helper", () => { expect(parse).toHaveBeenCalledWith(updates); }); + + it("sends the status of updated and failed modules to the frontend", async () => { + const updateResult = [ + { name: "MMM-Updated", updated: true, error: false, inProgress: false }, + { name: "MMM-Failed", updated: false, error: true, inProgress: false }, + { name: "MMM-NoCommand", updated: false, error: null, inProgress: false } + ]; + UpdateHelper.mockImplementation(function () { + this.parse = vi.fn().mockResolvedValue(updateResult); + }); + const { helper } = await loadNodeHelper({ + modules: [{ module: "updatenotification", config: { sendUpdatesNotifications: false } }] + }); + + await helper.socketNotificationReceived("CONFIG", { updateInterval: 1000 }); + helper.gitHelper.getRepos = vi.fn().mockResolvedValue([]); + helper.gitHelper.checkUpdates = vi.fn().mockResolvedValue([{ module: "MMM-Updated" }]); + helper.scheduleNextFetch = vi.fn(); + helper.sendSocketNotification = vi.fn(); + await helper.performFetch(); + + expect(helper.sendSocketNotification.mock.calls).toEqual([ + ["UPDATE_STATUS", updateResult[0]], + ["UPDATE_STATUS", updateResult[1]] + ]); + }); }); diff --git a/tests/unit/modules/default/updatenotification/updatenotification_spec.js b/tests/unit/modules/default/updatenotification/updatenotification_spec.js new file mode 100644 index 0000000000..81bbefa81d --- /dev/null +++ b/tests/unit/modules/default/updatenotification/updatenotification_spec.js @@ -0,0 +1,77 @@ +import { readFileSync } from "node:fs"; +import nunjucks from "nunjucks"; + +const moduleDir = new URL("../../../../../defaultmodules/updatenotification/", import.meta.url); + +describe("updatenotification module", () => { + let updatenotification; + let env; + + beforeEach(async () => { + vi.resetModules(); + global.Module = { + register: vi.fn((name, moduleDefinition) => { + updatenotification = moduleDefinition; + }) + }; + global.Log = { info: vi.fn() }; + + await import("../../../../../defaultmodules/updatenotification/updatenotification.js"); + + updatenotification.updateDom = vi.fn(); + updatenotification.sendSocketNotification = vi.fn(); + + env = new nunjucks.Environment(); + env.addFilter("translate", (key, vars = {}) => `${key}${vars.MODULE_NAME ? `:${vars.MODULE_NAME}` : ""}`); + env.addFilter("diffLink", (text) => text); + }); + + afterEach(() => { + delete global.Module; + delete global.Log; + }); + + const render = () => env.renderString(readFileSync(new URL("updatenotification.njk", moduleDir), "utf8"), updatenotification.getTemplateData()); + + it("renders a done line for every updated module", () => { + updatenotification.updatesNotifier({ name: "MMM-A", updated: true, needRestart: true }); + updatenotification.updatesNotifier({ name: "MMM-B", updated: true, needRestart: true }); + + const html = render(); + + expect(html).toContain("UPDATE_NOTIFICATION_DONE:MMM-A"); + expect(html).toContain("UPDATE_NOTIFICATION_DONE:MMM-B"); + expect(html).toContain("UPDATE_NOTIFICATION_NEED-RESTART"); + }); + + it("renders an error line for a failed module", () => { + updatenotification.updatesNotifier({ name: "MMM-A", error: true }); + + const html = render(); + + expect(html).toContain("UPDATE_NOTIFICATION_ERROR:MMM-A"); + expect(html).not.toContain("UPDATE_NOTIFICATION_NEED-RESTART"); + expect(updatenotification.sendSocketNotification).not.toHaveBeenCalled(); + }); + + it("removes an updated module from the list of available updates", () => { + updatenotification.updateUI({ module: "MMM-A", behind: 2, current: "master" }); + expect(render()).toContain("UPDATE_NOTIFICATION_MODULE:MMM-A"); + + updatenotification.updatesNotifier({ name: "MMM-A", updated: true, needRestart: true }); + + const html = render(); + expect(html).not.toContain("UPDATE_NOTIFICATION_MODULE:MMM-A"); + expect(html).toContain("UPDATE_NOTIFICATION_DONE:MMM-A"); + }); + + it("reports each module only once", () => { + updatenotification.updatesNotifier({ name: "MMM-A", updated: true, needRestart: true }); + updatenotification.updatesNotifier({ name: "MMM-A", error: true }); + + const html = render(); + + expect(html.match(/UPDATE_NOTIFICATION_(DONE|ERROR):MMM-A/g)).toEqual(["UPDATE_NOTIFICATION_DONE:MMM-A"]); + expect(updatenotification.updateDom).toHaveBeenCalledTimes(1); + }); +}); From 72d1437863c515e4f4153d3023c915bbd0fb295f Mon Sep 17 00:00:00 2001 From: HeikoGr <20295490+HeikoGr@users.noreply.github.com> Date: Fri, 23 Oct 2026 22:33:02 +0200 Subject: [PATCH 2/2] refactor(updatenotification): tidy log message and frontend test Shorten the log line before the automatic restart, since nodeRestart() already logs "Restarting MagicMirror...". Rename the frontend spec to frontend_spec.js to avoid clashing with the git helper spec, and derive the module import from moduleDir. Co-Authored-By: Claude Sonnet 5.5 --- defaultmodules/updatenotification/update_helper.js | 2 +- .../{updatenotification_spec.js => frontend_spec.js} | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename tests/unit/modules/default/updatenotification/{updatenotification_spec.js => frontend_spec.js} (96%) diff --git a/defaultmodules/updatenotification/update_helper.js b/defaultmodules/updatenotification/update_helper.js index 78bd1d1c8d..e10ef377bb 100644 --- a/defaultmodules/updatenotification/update_helper.js +++ b/defaultmodules/updatenotification/update_helper.js @@ -54,7 +54,7 @@ class Updater { } if (updatedCount && this.autoRestart) { - Log.info(`${updatedCount} module(s) updated, restarting MagicMirror...`); + Log.info(`${updatedCount} module(s) updated.`); setTimeout(() => this.nodeRestart(), 3000); } diff --git a/tests/unit/modules/default/updatenotification/updatenotification_spec.js b/tests/unit/modules/default/updatenotification/frontend_spec.js similarity index 96% rename from tests/unit/modules/default/updatenotification/updatenotification_spec.js rename to tests/unit/modules/default/updatenotification/frontend_spec.js index 81bbefa81d..88c26caa99 100644 --- a/tests/unit/modules/default/updatenotification/updatenotification_spec.js +++ b/tests/unit/modules/default/updatenotification/frontend_spec.js @@ -16,7 +16,7 @@ describe("updatenotification module", () => { }; global.Log = { info: vi.fn() }; - await import("../../../../../defaultmodules/updatenotification/updatenotification.js"); + await import(new URL("updatenotification.js", moduleDir).href); updatenotification.updateDom = vi.fn(); updatenotification.sendSocketNotification = vi.fn();