From 00f5b40090753ca129676961456de1945a1f948d Mon Sep 17 00:00:00 2001 From: Kamil Listopad Date: Wed, 1 Apr 2026 09:34:05 +0200 Subject: [PATCH 1/3] feat(cli): add per-canister build lock to prevent parallel clobbering Parallel `mops build` invocations targeting the same canister race on writing .wasm, .did, and .most files, producing corrupted output. Add per-canister file locking via `proper-lockfile` so that concurrent builds of the same canister are serialized while different canisters can still build in parallel. Made-with: Cursor --- cli/commands/build.ts | 220 +++++++++++++++++++++++----------------- cli/package-lock.json | 45 ++++++++ cli/package.json | 2 + cli/tests/build.test.ts | 17 ++++ 4 files changed, 192 insertions(+), 92 deletions(-) diff --git a/cli/commands/build.ts b/cli/commands/build.ts index bc3f48e1..33ed6b9a 100644 --- a/cli/commands/build.ts +++ b/cli/commands/build.ts @@ -1,8 +1,9 @@ import chalk from "chalk"; import { execa } from "execa"; import { exists } from "fs-extra"; -import { mkdir, readFile, writeFile } from "node:fs/promises"; +import { mkdir, open, readFile, writeFile } from "node:fs/promises"; import { join } from "node:path"; +import { lock, unlockSync } from "proper-lockfile"; import { cliError } from "../error.js"; import { isCandidCompatible } from "../helpers/is-candid-compatible.js"; import { resolveCanisterConfigs } from "../helpers/resolve-canisters.js"; @@ -70,115 +71,150 @@ export async function build( motokoPath = resolveConfigPath(motokoPath); const wasmPath = join(outputDir, `${canisterName}.wasm`); const mostPath = join(outputDir, `${canisterName}.most`); - let args = [ - "-c", - "--idl", - "--stable-types", - "-o", - wasmPath, - motokoPath, - ...(await sourcesArgs()).flat(), - ...getGlobalMocArgs(config), - ]; - args.push( - ...collectExtraArgs(config, canister, canisterName, options.extraArgs), - ); - - const isPublicCandid = true; // always true for now to reduce corner cases - const candidVisibility = isPublicCandid ? "icp:public" : "icp:private"; - if (isPublicCandid) { - args.push("--public-metadata", "candid:service"); - args.push("--public-metadata", "candid:args"); - } + + // per-canister lock to prevent parallel builds of the same canister from clobbering output files + const sentinelPath = join(outputDir, `.${canisterName}.buildlock`); + const fd = await open(sentinelPath, "a"); + await fd.close(); + + let release: (() => Promise) | undefined; try { - if (options.verbose) { - console.log(chalk.gray(mocPath, JSON.stringify(args))); - } - const result = await execa(mocPath, args, { - stdio: options.verbose ? "inherit" : "pipe", - reject: false, + release = await lock(sentinelPath, { + stale: 300_000, + retries: { retries: 120, minTimeout: 500, maxTimeout: 5_000 }, }); - - if (result.exitCode !== 0) { - if (!options.verbose) { - if (result.stderr) { - console.error(chalk.red(result.stderr)); - } - if (result.stdout?.trim()) { - console.error(chalk.yellow("Build output:")); - console.error(result.stdout); - } - } + } catch (err: any) { + if (err.code === "ELOCKED") { cliError( - `Build failed for canister ${canisterName} (exit code: ${result.exitCode})`, + `Another build of canister ${canisterName} is already in progress`, ); } + throw err; + } - if (options.verbose && result.stdout && result.stdout.trim()) { - console.log(result.stdout); - } + // proper-lockfile's built-in signal-exit cleanup is unreliable with process.exit() — + // ensure the lock is released synchronously so subsequent builds don't stall + const exitCleanup = () => { + try { + unlockSync(sentinelPath); + } catch {} + }; + process.on("exit", exitCleanup); - options.verbose && - console.log(chalk.gray(`Stable types written to ${mostPath}`)); + try { + let args = [ + "-c", + "--idl", + "--stable-types", + "-o", + wasmPath, + motokoPath, + ...(await sourcesArgs()).flat(), + ...getGlobalMocArgs(config), + ]; + args.push( + ...collectExtraArgs(config, canister, canisterName, options.extraArgs), + ); - const generatedDidPath = join(outputDir, `${canisterName}.did`); - const resolvedCandidPath = canister.candid - ? resolveConfigPath(canister.candid) - : null; + const isPublicCandid = true; // always true for now to reduce corner cases + const candidVisibility = isPublicCandid ? "icp:public" : "icp:private"; + if (isPublicCandid) { + args.push("--public-metadata", "candid:service"); + args.push("--public-metadata", "candid:args"); + } + try { + if (options.verbose) { + console.log(chalk.gray(mocPath, JSON.stringify(args))); + } + const result = await execa(mocPath, args, { + stdio: options.verbose ? "inherit" : "pipe", + reject: false, + }); - if (resolvedCandidPath) { - try { - const compatible = await isCandidCompatible( - generatedDidPath, - resolvedCandidPath, + if (result.exitCode !== 0) { + if (!options.verbose) { + if (result.stderr) { + console.error(chalk.red(result.stderr)); + } + if (result.stdout?.trim()) { + console.error(chalk.yellow("Build output:")); + console.error(result.stdout); + } + } + cliError( + `Build failed for canister ${canisterName} (exit code: ${result.exitCode})`, ); + } - if (!compatible) { - cliError( - `Candid compatibility check failed for canister ${canisterName}`, + if (options.verbose && result.stdout && result.stdout.trim()) { + console.log(result.stdout); + } + + options.verbose && + console.log(chalk.gray(`Stable types written to ${mostPath}`)); + + const generatedDidPath = join(outputDir, `${canisterName}.did`); + const resolvedCandidPath = canister.candid + ? resolveConfigPath(canister.candid) + : null; + + if (resolvedCandidPath) { + try { + const compatible = await isCandidCompatible( + generatedDidPath, + resolvedCandidPath, ); - } - if (options.verbose) { - console.log( - chalk.gray( - `Candid compatibility check passed for canister ${canisterName}`, - ), + if (!compatible) { + cliError( + `Candid compatibility check failed for canister ${canisterName}`, + ); + } + + if (options.verbose) { + console.log( + chalk.gray( + `Candid compatibility check passed for canister ${canisterName}`, + ), + ); + } + } catch (err: any) { + cliError( + `Error during Candid compatibility check for canister ${canisterName}${err?.message ? `\n${err.message}` : ""}`, ); } - } catch (err: any) { - cliError( - `Error during Candid compatibility check for canister ${canisterName}${err?.message ? `\n${err.message}` : ""}`, - ); } - } - options.verbose && - console.log(chalk.gray(`Adding metadata to ${wasmPath}`)); - const candidPath = resolvedCandidPath ?? generatedDidPath; - const candidText = await readFile(candidPath, "utf-8"); - const customSections: CustomSection[] = [ - { name: `${candidVisibility} candid:service`, data: candidText }, - ]; - if (canister.initArg) { - customSections.push({ - name: `${candidVisibility} candid:args`, - data: canister.initArg, - }); - } - const wasmBytes = await readFile(wasmPath); - const newWasm = getWasmBindings().add_custom_sections( - wasmBytes, - customSections, - ); - await writeFile(wasmPath, newWasm); - } catch (err: any) { - if (err.message?.includes("Build failed for canister")) { - throw err; + options.verbose && + console.log(chalk.gray(`Adding metadata to ${wasmPath}`)); + const candidPath = resolvedCandidPath ?? generatedDidPath; + const candidText = await readFile(candidPath, "utf-8"); + const customSections: CustomSection[] = [ + { name: `${candidVisibility} candid:service`, data: candidText }, + ]; + if (canister.initArg) { + customSections.push({ + name: `${candidVisibility} candid:args`, + data: canister.initArg, + }); + } + const wasmBytes = await readFile(wasmPath); + const newWasm = getWasmBindings().add_custom_sections( + wasmBytes, + customSections, + ); + await writeFile(wasmPath, newWasm); + } catch (err: any) { + if (err.message?.includes("Build failed for canister")) { + throw err; + } + cliError( + `Error while compiling canister ${canisterName}${err?.message ? `\n${err.message}` : ""}`, + ); } - cliError( - `Error while compiling canister ${canisterName}${err?.message ? `\n${err.message}` : ""}`, - ); + } finally { + process.removeListener("exit", exitCleanup); + await release(); } } diff --git a/cli/package-lock.json b/cli/package-lock.json index 3f62f1dc..458fa84f 100644 --- a/cli/package-lock.json +++ b/cli/package-lock.json @@ -45,6 +45,7 @@ "prettier-plugin-motoko": "0.13.0", "promisify-child-process": "4.1.2", "prompts": "2.4.2", + "proper-lockfile": "4.1.2", "semver": "7.7.1", "stream-to-promise": "3.0.0", "string-width": "7.2.0", @@ -66,6 +67,7 @@ "@types/ncp": "2.0.8", "@types/node": "24.0.3", "@types/prompts": "2.4.9", + "@types/proper-lockfile": "4.1.4", "@types/semver": "7.5.8", "@types/stream-to-promise": "2.2.4", "@types/tar": "6.1.13", @@ -3966,6 +3968,23 @@ "kleur": "^3.0.3" } }, + "node_modules/@types/proper-lockfile": { + "version": "4.1.4", + "resolved": "https://registry.npmjs.org/@types/proper-lockfile/-/proper-lockfile-4.1.4.tgz", + "integrity": "sha512-uo2ABllncSqg9F1D4nugVl9v93RmjxF6LJzQLMLDdPaXCUIDPeOJ21Gbqi43xNKzBi/WQ0Q0dICqufzQbMjipQ==", + "dev": true, + "license": "MIT", + "dependencies": { + "@types/retry": "*" + } + }, + "node_modules/@types/retry": { + "version": "0.12.5", + "resolved": "https://registry.npmjs.org/@types/retry/-/retry-0.12.5.tgz", + "integrity": "sha512-3xSjTp3v03X/lSQLkczaN9UIEwJMoMCA1+Nb5HfbJEQWogdeQIyVtTvxPXDQjZ5zws8rFQfVfRdz03ARihPJgw==", + "dev": true, + "license": "MIT" + }, "node_modules/@types/semver": { "version": "7.5.8", "resolved": "https://registry.npmjs.org/@types/semver/-/semver-7.5.8.tgz", @@ -12442,6 +12461,23 @@ "node": ">= 6" } }, + "node_modules/proper-lockfile": { + "version": "4.1.2", + "resolved": "https://registry.npmjs.org/proper-lockfile/-/proper-lockfile-4.1.2.tgz", + "integrity": "sha512-TjNPblN4BwAWMXU8s9AEz4JmQxnD1NNL7bNOY/AKUzyamc379FWASUhc/K1pL2noVb+XmZKLL68cjzLsiOAMaA==", + "license": "MIT", + "dependencies": { + "graceful-fs": "^4.2.4", + "retry": "^0.12.0", + "signal-exit": "^3.0.2" + } + }, + "node_modules/proper-lockfile/node_modules/signal-exit": { + "version": "3.0.7", + "resolved": "https://registry.npmjs.org/signal-exit/-/signal-exit-3.0.7.tgz", + "integrity": "sha512-wnD2ZE+l+SPC/uoS0vXeE9L1+0wuaMqKlfz9AMUo38JsyLSBWSFcHR1Rri62LZc12vLr1gb3jl7iwQhgwpAbGQ==", + "license": "ISC" + }, "node_modules/punycode": { "version": "2.3.1", "resolved": "https://registry.npmjs.org/punycode/-/punycode-2.3.1.tgz", @@ -12772,6 +12808,15 @@ "url": "https://github.com/sponsors/sindresorhus" } }, + "node_modules/retry": { + "version": "0.12.0", + "resolved": "https://registry.npmjs.org/retry/-/retry-0.12.0.tgz", + "integrity": "sha512-9LkiTwjUh6rT555DtE9rTX+BKByPfrMzEAtnlEtdEwr3Nkffwiihqe2bWADg+OQRjt9gl6ICdmB/ZFDCGAtSow==", + "license": "MIT", + "engines": { + "node": ">= 4" + } + }, "node_modules/reusify": { "version": "1.1.0", "resolved": "https://registry.npmjs.org/reusify/-/reusify-1.1.0.tgz", diff --git a/cli/package.json b/cli/package.json index ab75c462..1813d422 100644 --- a/cli/package.json +++ b/cli/package.json @@ -86,6 +86,7 @@ "prettier-plugin-motoko": "0.13.0", "promisify-child-process": "4.1.2", "prompts": "2.4.2", + "proper-lockfile": "4.1.2", "semver": "7.7.1", "stream-to-promise": "3.0.0", "string-width": "7.2.0", @@ -102,6 +103,7 @@ "@types/ncp": "2.0.8", "@types/node": "24.0.3", "@types/prompts": "2.4.9", + "@types/proper-lockfile": "4.1.4", "@types/semver": "7.5.8", "@types/stream-to-promise": "2.2.4", "@types/tar": "6.1.13", diff --git a/cli/tests/build.test.ts b/cli/tests/build.test.ts index 777c44fd..cdc81059 100644 --- a/cli/tests/build.test.ts +++ b/cli/tests/build.test.ts @@ -100,6 +100,23 @@ describe("build", () => { } }); + test("parallel builds of the same canister are serialized by lock", async () => { + const cwd = path.join(import.meta.dirname, "build/success"); + try { + const [a, b] = await Promise.all([ + cli(["build", "foo"], { cwd }), + cli(["build", "foo"], { cwd }), + ]); + expect(a.exitCode).toBe(0); + expect(b.exitCode).toBe(0); + expect(existsSync(path.join(cwd, ".mops/.build/foo.wasm"))).toBe(true); + expect(existsSync(path.join(cwd, ".mops/.build/foo.did"))).toBe(true); + expect(existsSync(path.join(cwd, ".mops/.build/foo.most"))).toBe(true); + } finally { + cleanFixture(cwd); + } + }); + // Regression: bin/mops.js must route through environments/nodejs/cli.js // so that setWasmBindings() is called before any command runs. // The dev entry point (npm run mops) uses tsx and always worked; From 7fec91e5be52503b89170016f80f3cee753382a2 Mon Sep 17 00:00:00 2001 From: Kamil Listopad Date: Wed, 1 Apr 2026 09:43:20 +0200 Subject: [PATCH 2/3] fix: address review findings for build lock - Wrap release() in try-catch in finally to prevent masking build errors - Reduce retry count from 120 to 60 to align with 5-minute stale timeout - Update ELOCKED error message to describe timeout, not a live contention - Document intentional redundancy of exitCleanup with signal-exit - Rename test to match what it actually asserts Made-with: Cursor --- cli/commands/build.ts | 13 ++++++++----- cli/tests/build.test.ts | 2 +- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/cli/commands/build.ts b/cli/commands/build.ts index 33ed6b9a..c174bcd8 100644 --- a/cli/commands/build.ts +++ b/cli/commands/build.ts @@ -81,19 +81,20 @@ export async function build( try { release = await lock(sentinelPath, { stale: 300_000, - retries: { retries: 120, minTimeout: 500, maxTimeout: 5_000 }, + retries: { retries: 60, minTimeout: 500, maxTimeout: 5_000 }, }); } catch (err: any) { if (err.code === "ELOCKED") { cliError( - `Another build of canister ${canisterName} is already in progress`, + `Timed out waiting for build lock on canister ${canisterName} — another build may be stuck`, ); } throw err; } - // proper-lockfile's built-in signal-exit cleanup is unreliable with process.exit() — - // ensure the lock is released synchronously so subsequent builds don't stall + // proper-lockfile registers its own signal-exit handler, but it doesn't reliably + // fire on process.exit(). This manual handler covers that gap. Double-unlock is + // harmless (the second call throws and is caught). const exitCleanup = () => { try { unlockSync(sentinelPath); @@ -214,7 +215,9 @@ export async function build( } } finally { process.removeListener("exit", exitCleanup); - await release(); + try { + await release?.(); + } catch {} } } diff --git a/cli/tests/build.test.ts b/cli/tests/build.test.ts index cdc81059..f6bf25d3 100644 --- a/cli/tests/build.test.ts +++ b/cli/tests/build.test.ts @@ -100,7 +100,7 @@ describe("build", () => { } }); - test("parallel builds of the same canister are serialized by lock", async () => { + test("parallel builds of the same canister both succeed", async () => { const cwd = path.join(import.meta.dirname, "build/success"); try { const [a, b] = await Promise.all([ From 3f0ad00849d24e7b939dd66f6be80db4062c2c65 Mon Sep 17 00:00:00 2001 From: Kamil Listopad Date: Wed, 1 Apr 2026 10:02:51 +0200 Subject: [PATCH 3/3] refactor --- cli/commands/build.ts | 22 +++++++++------------- cli/tests/build/success/.gitignore | 1 + 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/cli/commands/build.ts b/cli/commands/build.ts index c174bcd8..2d552aea 100644 --- a/cli/commands/build.ts +++ b/cli/commands/build.ts @@ -1,7 +1,7 @@ import chalk from "chalk"; import { execa } from "execa"; import { exists } from "fs-extra"; -import { mkdir, open, readFile, writeFile } from "node:fs/promises"; +import { mkdir, readFile, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { lock, unlockSync } from "proper-lockfile"; import { cliError } from "../error.js"; @@ -73,23 +73,19 @@ export async function build( const mostPath = join(outputDir, `${canisterName}.most`); // per-canister lock to prevent parallel builds of the same canister from clobbering output files - const sentinelPath = join(outputDir, `.${canisterName}.buildlock`); - const fd = await open(sentinelPath, "a"); - await fd.close(); + const lockTarget = join(outputDir, `.${canisterName}.buildlock`); + await writeFile(lockTarget, "", { flag: "a" }); let release: (() => Promise) | undefined; try { - release = await lock(sentinelPath, { + release = await lock(lockTarget, { stale: 300_000, retries: { retries: 60, minTimeout: 500, maxTimeout: 5_000 }, }); - } catch (err: any) { - if (err.code === "ELOCKED") { - cliError( - `Timed out waiting for build lock on canister ${canisterName} — another build may be stuck`, - ); - } - throw err; + } catch { + cliError( + `Failed to acquire build lock for canister ${canisterName} — another build may be stuck`, + ); } // proper-lockfile registers its own signal-exit handler, but it doesn't reliably @@ -97,7 +93,7 @@ export async function build( // harmless (the second call throws and is caught). const exitCleanup = () => { try { - unlockSync(sentinelPath); + unlockSync(lockTarget); } catch {} }; process.on("exit", exitCleanup); diff --git a/cli/tests/build/success/.gitignore b/cli/tests/build/success/.gitignore index 38505c9e..1e04dc29 100644 --- a/cli/tests/build/success/.gitignore +++ b/cli/tests/build/success/.gitignore @@ -1,2 +1,3 @@ .mops/ cli-output-test/ +*.most