fix(updatenotification): apply all pending module updates

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);
 				}
 			}

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<Array<object>>} 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.`);
+			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<void>} Resolves when processing is complete.
+	 * @returns {Promise<boolean>} 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;

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) {

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<object>} 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", () => {