Skip to content

Commit 140150f

Browse files
Kamiruscursoragent
andauthored
cli: fix concurrent mops invocations clobbering shared scratch dirs (#532)
## Problem Running two `mops` processes on the same project (e.g. an editor watcher and `caffeine check --fix`, or two back-to-back invocations from a script) intermittently failed with misleading errors: - `.mops/.check-stable/new.most: No such file or directory` from `mops check-stable` - `EEXIST: file already exists, symlink ...` from `mops check`/`build`/`check-stable` when `[canisters.<name>.migrations]` was configured Worse, `check-stable` would print a "Hint: You may need a migration" suggestion (now removed in #531) that pointed users at the wrong cause entirely. ## Root cause Two scratch directories were shared across all `mops` processes for the same project: 1. **`.mops/.check-stable/`** — `runStableCheck` did `rm -rf <dir>` → `mkdir <dir>` at the start and `rm -rf <dir>` at the end. Two concurrent runs would: A creates `new.most`, B's startup `rm -rf` deletes it, A's subsequent `--stable-compatible` read fails. 2. **`<parent>/.migrations-<canister>/`** — `prepareMigrationArgs` (used when migrations need a trimmed/`next`-merged staging dir) wiped the dir and re-symlinked. Two concurrent runs raced on `symlinkSync` (EEXIST) or on each other's wipe. Both directories existed mostly to give moc a stable path to read from during the invocation; they weren't actually shared state that needed to be coordinated. ## Fix Switch both directories to per-invocation unique paths created with `fs.mkdtempSync`: - `.mops/.check-stable/` → `.mops/.check-stable-XXXXXX/` - `<parent>/.migrations-<canister>/` → `<parent>/.migrations-<canister>-XXXXXX/` Each process owns its own dir for its lifetime and removes it on exit. No shared mutable state → no race possible by construction. Existing `.gitignore` rules already cover both patterns (`.mops/` and `.migrations-*/`). Caller behavior is unchanged: `prepareMigrationArgs` still returns a `cleanup()` callback that removes the staged dir in `finally`. ## Caveats - A crashed process leaks its scratch dir (small — symlinks + a couple of `.most`/`.wasm` files). Both locations are gitignored, so it's cosmetic. - Stale `.mops/.check-stable/` or `<parent>/.migrations-<canister>/` directories from older releases aren't auto-cleaned. They'll stay until the user removes them; harmless but visible in `git status` for projects that don't ignore them. ## Test plan - [x] Added regression test: 10 parallel `mops check-stable` invocations against a fixture with `check-limit = 3` and 4 chain migrations (forces both the check-stable scratch path and the migration staging `mkdtemp` branch). - [x] Existing migrate / check / build / check-stable suites pass (103/103, 64 snapshots). - [x] `npm run lint` + `npm run check` clean. - [x] Empirical reproduction of both error symptoms before the fix; both gone after. - [ ] Reviewer sanity: trigger the original failure with two parallel `mops check-stable` (or `caffeine check --fix` + an editor watcher) on a project with `[migrations]` configured; confirm clean output. Made with [Cursor](https://cursor.com) Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent dc66a27 commit 140150f

12 files changed

Lines changed: 126 additions & 14 deletions

File tree

cli/CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
# Mops CLI Changelog
22

33
## Next
4+
- 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 `<parent>/.migrations-<canister>/` 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).
45
- Deprecate `skipIfMissing` in `[canisters.<name>.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.
56
- 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.
67
- 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 <Name>` command.

cli/commands/check-stable.ts

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1-
import { basename, join } from "node:path";
2-
import { existsSync, mkdirSync } from "node:fs";
1+
import { join } from "node:path";
2+
import { existsSync, mkdirSync, mkdtempSync } from "node:fs";
33
import { rm } from "node:fs/promises";
44
import chalk from "chalk";
55
import { execa } from "execa";
@@ -17,7 +17,10 @@ import {
1717
import { sourcesArgs } from "./sources.js";
1818
import { toolchain } from "./toolchain/index.js";
1919

20-
const CHECK_STABLE_DIR = ".mops/.check-stable";
20+
// Per-invocation scratch dir lives under `.mops/`; `mkdtempSync` makes it unique so
21+
// concurrent `mops` processes don't clobber each other's `old.most`/`new.most`.
22+
const CHECK_STABLE_PARENT = ".mops";
23+
const CHECK_STABLE_PREFIX = ".check-stable-";
2124

2225
export interface CheckStableOptions {
2326
verbose: boolean;
@@ -189,15 +192,17 @@ export async function runStableCheck(
189192
cliError(`File not found: ${oldFile}`);
190193
}
191194

192-
await rm(CHECK_STABLE_DIR, { recursive: true, force: true });
193-
mkdirSync(CHECK_STABLE_DIR, { recursive: true });
195+
mkdirSync(CHECK_STABLE_PARENT, { recursive: true });
196+
const scratchDir = mkdtempSync(
197+
join(CHECK_STABLE_PARENT, CHECK_STABLE_PREFIX),
198+
);
194199
try {
195200
const oldMostPath = isOldMostFile
196201
? oldFile
197202
: await generateStableTypes(
198203
mocPath,
199204
oldFile,
200-
join(CHECK_STABLE_DIR, "old.most"),
205+
join(scratchDir, "old.most"),
201206
sources,
202207
globalMocArgs,
203208
canisterArgs,
@@ -207,7 +212,7 @@ export async function runStableCheck(
207212
const newMostPath = await generateStableTypes(
208213
mocPath,
209214
canisterMain,
210-
join(CHECK_STABLE_DIR, "new.most"),
215+
join(scratchDir, "new.most"),
211216
sources,
212217
globalMocArgs,
213218
canisterArgs,
@@ -246,7 +251,7 @@ export async function runStableCheck(
246251
),
247252
);
248253
} finally {
249-
await rm(CHECK_STABLE_DIR, { recursive: true, force: true });
254+
await rm(scratchDir, { recursive: true, force: true });
250255
}
251256
}
252257

@@ -259,8 +264,7 @@ async function generateStableTypes(
259264
canisterArgs: string[],
260265
options: Partial<CheckStableOptions>,
261266
): Promise<string> {
262-
const base = basename(outputPath, ".most");
263-
const wasmPath = join(CHECK_STABLE_DIR, base + ".wasm");
267+
const wasmPath = outputPath.replace(/\.most$/, ".wasm");
264268
const args = [
265269
"--stable-types",
266270
"-o",

cli/helpers/migrations.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import {
22
existsSync,
33
mkdirSync,
4+
mkdtempSync,
45
readdirSync,
56
symlinkSync,
67
writeFileSync,
@@ -193,9 +194,11 @@ export async function prepareMigrationArgs(
193194
return { migrationArgs, cleanup: async () => {} };
194195
}
195196

196-
const tempDir = stagedMigrationsDir(chainDir, canisterName);
197-
await rm(tempDir, { recursive: true, force: true });
198-
mkdirSync(tempDir, { recursive: true });
197+
// Per-invocation staging dir; `mkdtempSync` makes it unique so concurrent `mops`
198+
// processes don't clobber each other's symlinks. Cleaned up below in `cleanup()`.
199+
const baseDir = stagedMigrationsDir(chainDir, canisterName);
200+
mkdirSync(dirname(baseDir), { recursive: true });
201+
const tempDir = mkdtempSync(`${baseDir}-`);
199202
writeFileSync(join(tempDir, ".gitignore"), "*\n");
200203

201204
for (const { file, dir } of included) {

cli/tests/check-stable.test.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,4 +86,24 @@ describe("check-stable", () => {
8686
expect(result.exitCode).toBe(1);
8787
expect(result.stderr).toMatch(/File not found/);
8888
});
89+
90+
// Regression: two concurrent `mops check-stable` runs on the same project used to clobber
91+
// each other's `.mops/.check-stable/new.most` and the staged migration symlinks, surfacing
92+
// as a misleading `new.most: No such file or directory` or an `EEXIST: symlink` crash.
93+
test("concurrent runs do not clobber each other's scratch state", async () => {
94+
const cwd = path.join(import.meta.dirname, "check-stable/migrations-chain");
95+
const results = await Promise.all(
96+
Array.from({ length: 10 }, () => cli(["check-stable"], { cwd })),
97+
);
98+
for (const result of results) {
99+
expect({
100+
exitCode: result.exitCode,
101+
stderr: result.stderr,
102+
}).toEqual({
103+
exitCode: 0,
104+
stderr: "",
105+
});
106+
expect(result.stdout).toMatch(/Stable compatibility check passed/);
107+
}
108+
}, 60_000);
89109
});
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
// Version: 4.0.0
2+
{
3+
"20250101_000000_Init" : {} -> {a : Nat; b : Text};
4+
"20250201_000000_AddField" : (old : {a : Nat; b : Text}) -> {a : Nat; b : Text; c : Bool};
5+
"20250301_000000_AddD" : (old : {a : Nat; b : Text; c : Bool}) -> {a : Nat; b : Text; c : Bool; d : Int};
6+
"20250401_000000_AddE" : (old : {a : Nat; b : Text; c : Bool; d : Int}) -> {a : Nat; b : Text; c : Bool; d : Int; e : Text}
7+
}
8+
actor {
9+
stable a : Nat;
10+
stable b : Text;
11+
stable c : Bool;
12+
stable d : Int;
13+
stable e : Text
14+
};
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
module {
2+
public func migration(_ : {}) : { a : Nat; b : Text } {
3+
{
4+
a = 42;
5+
b = "hello";
6+
};
7+
};
8+
};
Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
module {
2+
public func migration(old : { a : Nat; b : Text }) : {
3+
a : Nat;
4+
b : Text;
5+
c : Bool;
6+
} {
7+
{ old with c = true };
8+
};
9+
};
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
module {
2+
public func migration(old : { a : Nat; b : Text; c : Bool }) : {
3+
a : Nat;
4+
b : Text;
5+
c : Bool;
6+
d : Int;
7+
} {
8+
{ old with d = 0 };
9+
};
10+
};
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
module {
2+
public func migration(old : { a : Nat; b : Text; c : Bool; d : Int }) : {
3+
a : Nat;
4+
b : Text;
5+
c : Bool;
6+
d : Int;
7+
e : Text;
8+
} {
9+
{ old with e = "" };
10+
};
11+
};
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
[toolchain]
2+
moc = "1.5.0"
3+
4+
[moc]
5+
args = ["--default-persistent-actors"]
6+
7+
[canisters.backend]
8+
main = "src/main.mo"
9+
10+
[canisters.backend.migrations]
11+
chain = "migrations"
12+
check-limit = 3
13+
14+
[canisters.backend.check-stable]
15+
path = "deployed.most"

0 commit comments

Comments
 (0)