diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index a93dbd7e..15508de4 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -1,6 +1,7 @@ # Mops CLI Changelog ## Next +- Fix race conditions when two `mops` processes run on the same project (e.g. an editor watcher and `caffeine check --fix`, or back-to-back invocations). `mops check-stable` used a shared `.mops/.check-stable/` scratch dir and `mops check`/`build`/`check-stable` used a shared `/.migrations-/` staging dir; concurrent runs would clobber each other and surface as misleading errors like `.mops/.check-stable/new.most: No such file or directory` or `EEXIST: file already exists, symlink ...`. Both directories are now per-invocation (created via `mkdtemp` and removed when the command finishes). - Deprecate `skipIfMissing` in `[canisters..check-stable]`. Behavior is unchanged for now, but `mops check`/`check-stable` print a warning when it is set. For initial deployments, commit a `.most` file at the configured `path` containing an empty actor (`// Version: 1.0.0\nactor { };`) instead — the stable check then runs against an empty baseline. - Drop the "you may need a migration" hint after a failed stable compatibility check in `mops check`/`check-stable`. The hint guessed at whether the user needed a new migration or a fix to an existing one, and `moc`'s underlying compatibility error already links to the migration docs. - The missing-chain-directory error from `mops check`/`build`/`check-stable` now points at adding a `.mo` file to the `chain` directory instead of running the experimental `mops migrate new ` command. diff --git a/cli/commands/check-stable.ts b/cli/commands/check-stable.ts index 2cb28584..a071f59f 100644 --- a/cli/commands/check-stable.ts +++ b/cli/commands/check-stable.ts @@ -1,5 +1,5 @@ -import { basename, join } from "node:path"; -import { existsSync, mkdirSync } from "node:fs"; +import { join } from "node:path"; +import { existsSync, mkdirSync, mkdtempSync } from "node:fs"; import { rm } from "node:fs/promises"; import chalk from "chalk"; import { execa } from "execa"; @@ -17,7 +17,10 @@ import { import { sourcesArgs } from "./sources.js"; import { toolchain } from "./toolchain/index.js"; -const CHECK_STABLE_DIR = ".mops/.check-stable"; +// Per-invocation scratch dir lives under `.mops/`; `mkdtempSync` makes it unique so +// concurrent `mops` processes don't clobber each other's `old.most`/`new.most`. +const CHECK_STABLE_PARENT = ".mops"; +const CHECK_STABLE_PREFIX = ".check-stable-"; export interface CheckStableOptions { verbose: boolean; @@ -189,15 +192,17 @@ export async function runStableCheck( cliError(`File not found: ${oldFile}`); } - await rm(CHECK_STABLE_DIR, { recursive: true, force: true }); - mkdirSync(CHECK_STABLE_DIR, { recursive: true }); + mkdirSync(CHECK_STABLE_PARENT, { recursive: true }); + const scratchDir = mkdtempSync( + join(CHECK_STABLE_PARENT, CHECK_STABLE_PREFIX), + ); try { const oldMostPath = isOldMostFile ? oldFile : await generateStableTypes( mocPath, oldFile, - join(CHECK_STABLE_DIR, "old.most"), + join(scratchDir, "old.most"), sources, globalMocArgs, canisterArgs, @@ -207,7 +212,7 @@ export async function runStableCheck( const newMostPath = await generateStableTypes( mocPath, canisterMain, - join(CHECK_STABLE_DIR, "new.most"), + join(scratchDir, "new.most"), sources, globalMocArgs, canisterArgs, @@ -246,7 +251,7 @@ export async function runStableCheck( ), ); } finally { - await rm(CHECK_STABLE_DIR, { recursive: true, force: true }); + await rm(scratchDir, { recursive: true, force: true }); } } @@ -259,8 +264,7 @@ async function generateStableTypes( canisterArgs: string[], options: Partial, ): Promise { - const base = basename(outputPath, ".most"); - const wasmPath = join(CHECK_STABLE_DIR, base + ".wasm"); + const wasmPath = outputPath.replace(/\.most$/, ".wasm"); const args = [ "--stable-types", "-o", diff --git a/cli/helpers/migrations.ts b/cli/helpers/migrations.ts index 2943ab26..afb78aff 100644 --- a/cli/helpers/migrations.ts +++ b/cli/helpers/migrations.ts @@ -1,6 +1,7 @@ import { existsSync, mkdirSync, + mkdtempSync, readdirSync, symlinkSync, writeFileSync, @@ -193,9 +194,11 @@ export async function prepareMigrationArgs( return { migrationArgs, cleanup: async () => {} }; } - const tempDir = stagedMigrationsDir(chainDir, canisterName); - await rm(tempDir, { recursive: true, force: true }); - mkdirSync(tempDir, { recursive: true }); + // Per-invocation staging dir; `mkdtempSync` makes it unique so concurrent `mops` + // processes don't clobber each other's symlinks. Cleaned up below in `cleanup()`. + const baseDir = stagedMigrationsDir(chainDir, canisterName); + mkdirSync(dirname(baseDir), { recursive: true }); + const tempDir = mkdtempSync(`${baseDir}-`); writeFileSync(join(tempDir, ".gitignore"), "*\n"); for (const { file, dir } of included) { diff --git a/cli/tests/check-stable.test.ts b/cli/tests/check-stable.test.ts index 9d11f453..38e4203a 100644 --- a/cli/tests/check-stable.test.ts +++ b/cli/tests/check-stable.test.ts @@ -86,4 +86,24 @@ describe("check-stable", () => { expect(result.exitCode).toBe(1); expect(result.stderr).toMatch(/File not found/); }); + + // Regression: two concurrent `mops check-stable` runs on the same project used to clobber + // each other's `.mops/.check-stable/new.most` and the staged migration symlinks, surfacing + // as a misleading `new.most: No such file or directory` or an `EEXIST: symlink` crash. + test("concurrent runs do not clobber each other's scratch state", async () => { + const cwd = path.join(import.meta.dirname, "check-stable/migrations-chain"); + const results = await Promise.all( + Array.from({ length: 10 }, () => cli(["check-stable"], { cwd })), + ); + for (const result of results) { + expect({ + exitCode: result.exitCode, + stderr: result.stderr, + }).toEqual({ + exitCode: 0, + stderr: "", + }); + expect(result.stdout).toMatch(/Stable compatibility check passed/); + } + }, 60_000); }); diff --git a/cli/tests/check-stable/migrations-chain/deployed.most b/cli/tests/check-stable/migrations-chain/deployed.most new file mode 100644 index 00000000..634f4f96 --- /dev/null +++ b/cli/tests/check-stable/migrations-chain/deployed.most @@ -0,0 +1,14 @@ +// Version: 4.0.0 +{ + "20250101_000000_Init" : {} -> {a : Nat; b : Text}; + "20250201_000000_AddField" : (old : {a : Nat; b : Text}) -> {a : Nat; b : Text; c : Bool}; + "20250301_000000_AddD" : (old : {a : Nat; b : Text; c : Bool}) -> {a : Nat; b : Text; c : Bool; d : Int}; + "20250401_000000_AddE" : (old : {a : Nat; b : Text; c : Bool; d : Int}) -> {a : Nat; b : Text; c : Bool; d : Int; e : Text} +} +actor { + stable a : Nat; + stable b : Text; + stable c : Bool; + stable d : Int; + stable e : Text +}; diff --git a/cli/tests/check-stable/migrations-chain/migrations/20250101_000000_Init.mo b/cli/tests/check-stable/migrations-chain/migrations/20250101_000000_Init.mo new file mode 100644 index 00000000..d706b262 --- /dev/null +++ b/cli/tests/check-stable/migrations-chain/migrations/20250101_000000_Init.mo @@ -0,0 +1,8 @@ +module { + public func migration(_ : {}) : { a : Nat; b : Text } { + { + a = 42; + b = "hello"; + }; + }; +}; diff --git a/cli/tests/check-stable/migrations-chain/migrations/20250201_000000_AddField.mo b/cli/tests/check-stable/migrations-chain/migrations/20250201_000000_AddField.mo new file mode 100644 index 00000000..432414c9 --- /dev/null +++ b/cli/tests/check-stable/migrations-chain/migrations/20250201_000000_AddField.mo @@ -0,0 +1,9 @@ +module { + public func migration(old : { a : Nat; b : Text }) : { + a : Nat; + b : Text; + c : Bool; + } { + { old with c = true }; + }; +}; diff --git a/cli/tests/check-stable/migrations-chain/migrations/20250301_000000_AddD.mo b/cli/tests/check-stable/migrations-chain/migrations/20250301_000000_AddD.mo new file mode 100644 index 00000000..ef38ea1f --- /dev/null +++ b/cli/tests/check-stable/migrations-chain/migrations/20250301_000000_AddD.mo @@ -0,0 +1,10 @@ +module { + public func migration(old : { a : Nat; b : Text; c : Bool }) : { + a : Nat; + b : Text; + c : Bool; + d : Int; + } { + { old with d = 0 }; + }; +}; diff --git a/cli/tests/check-stable/migrations-chain/migrations/20250401_000000_AddE.mo b/cli/tests/check-stable/migrations-chain/migrations/20250401_000000_AddE.mo new file mode 100644 index 00000000..068c59a9 --- /dev/null +++ b/cli/tests/check-stable/migrations-chain/migrations/20250401_000000_AddE.mo @@ -0,0 +1,11 @@ +module { + public func migration(old : { a : Nat; b : Text; c : Bool; d : Int }) : { + a : Nat; + b : Text; + c : Bool; + d : Int; + e : Text; + } { + { old with e = "" }; + }; +}; diff --git a/cli/tests/check-stable/migrations-chain/mops.toml b/cli/tests/check-stable/migrations-chain/mops.toml new file mode 100644 index 00000000..5c43b5ca --- /dev/null +++ b/cli/tests/check-stable/migrations-chain/mops.toml @@ -0,0 +1,15 @@ +[toolchain] +moc = "1.5.0" + +[moc] +args = ["--default-persistent-actors"] + +[canisters.backend] +main = "src/main.mo" + +[canisters.backend.migrations] +chain = "migrations" +check-limit = 3 + +[canisters.backend.check-stable] +path = "deployed.most" diff --git a/cli/tests/check-stable/migrations-chain/src/main.mo b/cli/tests/check-stable/migrations-chain/src/main.mo new file mode 100644 index 00000000..53c1937f --- /dev/null +++ b/cli/tests/check-stable/migrations-chain/src/main.mo @@ -0,0 +1,13 @@ +import Prim "mo:prim"; + +actor { + let a : Nat; + let b : Text; + let c : Bool; + let d : Int; + let e : Text; + + public func check() : async () { + Prim.debugPrint(debug_show { a; b; c; d; e }); + }; +}; diff --git a/cli/tests/helpers.ts b/cli/tests/helpers.ts index 17e577ae..6385ace1 100644 --- a/cli/tests/helpers.ts +++ b/cli/tests/helpers.ts @@ -37,7 +37,11 @@ export const normalizePaths = (text: string): string => { .replace(/\/[^\s"]+\/\.cache\/mops/g, "") .replace(/\/[^\s"]+\/Library\/Caches\/mops/g, "") .replace(/\/[^\s"[\]]+\/moc(?:-wrapper)?(?=\s|$)/g, "moc-wrapper") - .replace(/\/[^\s"[\]]+\.motoko\/bin\/moc/g, "moc-wrapper"), + .replace(/\/[^\s"[\]]+\.motoko\/bin\/moc/g, "moc-wrapper") + // Per-invocation scratch / staging dirs use mkdtemp; redact the random suffix + // (Node's exact suffix format isn't a stable contract) so snapshots stay stable. + .replace(/\.mops\/\.check-stable-\w+/g, ".mops/.check-stable") + .replace(/(\.migrations-[\w.-]+?)-\w+(?=[/\s"]|$)/g, "$1"), ); };