From dfd52bf66895d2ccffa52d70eed8546b2bd9465c Mon Sep 17 00:00:00 2001 From: Cristiano Calcagno Date: Tue, 15 Sep 2026 03:29:55 +0200 Subject: [PATCH] Serialize test suites sharing build artifacts with a per-checkout lock Root `make test*` targets and `node scripts/test.js` now acquire a `flock` held for the whole command, so concurrent invocations in one checkout stop racing on shared build outputs. The conflicts are real. `make test` builds Belt through `lib` and then cleans and rebuilds it again in the mocha phase, while `tests/analysis_tests` cleans and rebuilds the same workspace as a workaround for #8539; either landing mid-run breaks the other. Two `scripts/test_syntax.sh` runs both `rm -rf tests/temp` and rewrite the same `expected` fixtures. Concurrent `dune build` invocations already fail outright on dune's own `_build` lock. The lock is taken before prerequisites are entered, so the build phase is covered rather than just the test recipe, and each suite's recursive Make runs with `-j1` so nested `clean test` goals stay ordered. This costs no real parallelism: the suites run as sequential recipe lines, and dune, cargo and rewatch parallelize internally regardless of Make. Nested commands reuse the active lock through an environment marker validated against the lock file contents and a live-PID check, so a stale marker cannot skip locking. The wrapper `exec`s its command, so the PID, signal disposition and exit status are preserved, and the kernel releases the lock once every process inheriting the descriptor exits, even after SIGKILL. Windows has no `flock` and CI runs the suites there, so commands on Windows print a notice and run unlocked; any other platform missing `flock` fails rather than racing silently. Signed-Off-By: Cristiano Calcagno Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Fz7ZXPYxfMT8B1sRgqqDoD --- .gitignore | 6 ++ AGENTS.md | 7 ++ CONTRIBUTING.md | 37 +++++++++ Makefile | 27 +++++-- lib_dev/test_lock.js | 71 +++++++++++++++++ scripts/test.js | 4 +- scripts/test_test_lock.py | 159 ++++++++++++++++++++++++++++++++++++++ scripts/with_test_lock.py | 84 ++++++++++++++++++++ 8 files changed, 386 insertions(+), 9 deletions(-) create mode 100644 lib_dev/test_lock.js create mode 100644 scripts/test_test_lock.py create mode 100644 scripts/with_test_lock.py diff --git a/.gitignore b/.gitignore index ba748abd224..6a136ffb863 100644 --- a/.gitignore +++ b/.gitignore @@ -108,3 +108,9 @@ package.tgz # AI Agents .claude/settings.local.json + +# Kernel lock inode for suites sharing build artifacts; do not delete while in use. +.rescript-test.lock + +# Bytecode from the Python build/test tooling in scripts/ +__pycache__/ diff --git a/AGENTS.md b/AGENTS.md index 9014b36aa1b..a64c63c9f3c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -160,6 +160,13 @@ also run `make test-syntax`; use `make test-syntax-roundtrip` when parsing or printing changes. Other focused suites are `make test-gentype`, `make test-analysis`, `make test-tools`, and `make test-rewatch`. +Root `make test*` suite targets and direct `node scripts/test.js` invocations +use a per-checkout artifact lock (currently a macOS/Linux prototype). Let a +waiting invocation wait; do not delete `.rescript-test.lock` or bypass the +`_locked-*` targets. See [test concurrency](CONTRIBUTING.md#test-concurrency) +for coverage and limitations. Separate worktrees need their own build outputs +and dependencies to run independently. + ### Testing Requirements #### When to Add Tests diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8f33ed0cd25..92dcfd0be59 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -247,6 +247,43 @@ To run all tests: make test ``` +#### Test concurrency + +The root test-suite targets and `node scripts/test.js` acquire a shared lock +before any build, cleanup, or test work. In particular, `make test` and +`make test-analysis` both rebuild or clean Belt's outputs: starting them in +parallel now makes one print `[test-lock] ... waiting for shared artifacts` +until the other finishes. `make test-all -j` serializes its suite targets too. +Nested runners reuse the active lock, and each suite's recursive Make invocation +runs with `-j1` so nested `clean test` goals remain ordered. + +This is a macOS/Linux prototype using Python 3 and `flock`. Windows has no +`flock`, and CI runs the suites there, so commands on Windows print +`[test-lock] ... unlocked` and run unprotected: concurrent suites in one +Windows checkout are not supported. Any other platform missing `flock` fails +rather than racing silently. The lock is per physical checkout. Separate +worktrees with independent build outputs and dependencies can run concurrently. +Direct low-level commands such as `make lib`, `make clean`, +`yarn workspace ... build`, and subdirectory Make invocations are not +automatically protected. To coordinate one with a suite: + +```sh +python3 scripts/with_test_lock.py --label manual-build -- make lib +``` + +The lock lives in `.rescript-test.lock`, outside directories cleaned by builds. +Do not delete it while commands are running: replacing the inode would let two +processes acquire different locks. The OS releases the lock when its command +(and any processes inheriting its descriptor) exits, even after a crash. Finish +child commands before leaving the protected command; do not detach test jobs. + +`make test` includes the process-level lock checks. To run just these checks +(contention, nesting, failure, killed owners, and independent checkouts): + +```sh +python3 scripts/test_test_lock.py +``` + **Run Mocha tests only (for our runtime code):** This will run our `mocha` unit test suite defined in `tests/tests`. diff --git a/Makefile b/Makefile index 1bc5e8cd7d6..8ab3d334973 100644 --- a/Makefile +++ b/Makefile @@ -154,36 +154,47 @@ artifacts: lib # Tests +# Lock before entering prerequisites: locking only the test recipe would leave +# lib builds and suite cleanup racing. test-all's leaf targets acquire separately, +# including under make -j. Recursive suites run serially because nested +# clean/test goals must also remain ordered. +LOCKED_TEST_TARGETS := test test-analysis test-reanalyze test-tools test-syntax test-syntax-roundtrip test-gentype test-rewatch +$(LOCKED_TEST_TARGETS): + +python3 scripts/with_test_lock.py --label $@ -- $(MAKE) -j1 _locked-$@ + +.PHONY: $(addprefix _locked-,$(LOCKED_TEST_TARGETS)) + bench: compiler $(DUNE_BIN_DIR)/syntax_benchmarks -test: lib +_locked-test: lib + python3 scripts/test_test_lock.py node scripts/test.js -all -test-analysis: lib +_locked-test-analysis: lib make -C tests/analysis_tests clean test -test-reanalyze: lib +_locked-test-reanalyze: lib make -C tests/analysis_tests/tests-reanalyze/deadcode test # Benchmark reanalyze on larger codebase (COPIES=N for more files) benchmark-reanalyze: lib make -C tests/analysis_tests/tests-reanalyze/deadcode-benchmark benchmark COPIES=$(or $(COPIES),50) -test-tools: lib +_locked-test-tools: lib make -C tests/tools_tests clean test -test-syntax: compiler +_locked-test-syntax: compiler ./scripts/test_syntax.sh -test-syntax-roundtrip: compiler +_locked-test-syntax-roundtrip: compiler ROUNDTRIP_TEST=1 ./scripts/test_syntax.sh -test-gentype: lib +_locked-test-gentype: lib make -C tests/gentype_tests/typescript-react-example clean test make -C tests/gentype_tests/stdlib-no-shims clean test -test-rewatch: lib +_locked-test-rewatch: lib ./rewatch/tests/suite.sh $(RESCRIPT_EXE) test-all: test test-gentype test-analysis test-tools test-rewatch diff --git a/lib_dev/test_lock.js b/lib_dev/test_lock.js new file mode 100644 index 00000000000..175a12667db --- /dev/null +++ b/lib_dev/test_lock.js @@ -0,0 +1,71 @@ +// @ts-check + +import { spawn } from "node:child_process"; +import { readFileSync, realpathSync } from "node:fs"; +import { constants } from "node:os"; +import { fileURLToPath } from "node:url"; + +const root = realpathSync(fileURLToPath(new URL("../", import.meta.url))); +const wrapper = fileURLToPath( + new URL("../scripts/with_test_lock.py", import.meta.url), +); + +/** Acquire the checkout lock before a directly invoked runner does any work. */ +export async function ensureTestLock() { + // Windows has no flock. CI runs this runner there, so proceed unlocked + // rather than failing; concurrent suites in one checkout stay unsupported. + if (process.platform === "win32") return; + try { + const owner = JSON.parse(process.env.RESCRIPT_TEST_LOCK ?? "null"); + const recorded = JSON.parse( + readFileSync(`${root}/.rescript-test.lock`, "utf8"), + ); + if ( + owner?.root === root && + owner.pid === recorded.pid && + owner.token === recorded.token + ) { + process.kill(owner.pid, 0); + return; + } + } catch { + // Missing/stale ownership: acquire through the OS lock, never skip it. + } + const child = spawn( + "python3", + [ + wrapper, + "--label", + "scripts/test.js", + "--", + process.execPath, + ...process.argv.slice(1), + ], + { stdio: "inherit" }, + ); + /** @type {NodeJS.Signals[]} */ + const signals = ["SIGINT", "SIGTERM", "SIGHUP"]; + const forwards = signals.map(signal => { + const forward = () => { + child.kill(signal); + }; + process.on(signal, forward); + return { signal, forward }; + }); + let status; + try { + status = await new Promise(resolve => { + child.once("error", error => { + console.error(`[test-lock] ${error.message}`); + resolve(1); + }); + child.once("exit", (code, signal) => + resolve(code ?? (signal ? 128 + constants.signals[signal] : 1)), + ); + }); + } finally { + for (const { signal, forward } of forwards) + process.removeListener(signal, forward); + } + process.exit(status); +} diff --git a/scripts/test.js b/scripts/test.js index a59676a0e48..122206c1716 100644 --- a/scripts/test.js +++ b/scripts/test.js @@ -10,7 +10,6 @@ import { ounitTestBin, projectDir, } from "#dev/paths"; - import { execBin, execBuild, @@ -20,6 +19,9 @@ import { rescript, shell, } from "#dev/process"; +import { ensureTestLock } from "#dev/test_lock"; + +await ensureTestLock(); let ounitTest = false; let mochaTest = false; diff --git a/scripts/test_test_lock.py b/scripts/test_test_lock.py new file mode 100644 index 00000000000..c96e1333bf1 --- /dev/null +++ b/scripts/test_test_lock.py @@ -0,0 +1,159 @@ +"""Process-level tests for the shared-artifact lock prototype.""" + +import json +import os +from pathlib import Path +import selectors +import signal +import subprocess +import sys +import tempfile +import unittest + +WRAPPER = Path(__file__).resolve().with_name("with_test_lock.py") +HOLD = """ +import pathlib, sys, time +print('START ' + sys.argv[1], flush=True) +while not pathlib.Path(sys.argv[2]).exists(): + time.sleep(0.02) +print('END ' + sys.argv[1], flush=True) +""" + + +@unittest.skipUnless(os.name == "posix", "prototype uses POSIX flock") +class TestArtifactLock(unittest.TestCase): + def setUp(self): + self.temp = tempfile.TemporaryDirectory(prefix="rescript-lock-test-") + self.addCleanup(self.temp.cleanup) + self.root = Path(self.temp.name) + self.children = [] + self.addCleanup(self.stop_children) + + def stop_children(self): + for child in self.children: + if child.poll() is None: + child.kill() + child.wait(timeout=5) + child.stdout.close() + child.stderr.close() + + def command(self, root, label, *command): + return [sys.executable, str(WRAPPER), "--root", str(root), + "--label", label, "--", *command] + + def start(self, root, label, *command, env=None): + child = subprocess.Popen(self.command(root, label, *command), + stdout=subprocess.PIPE, stderr=subprocess.PIPE, + text=True, env=env) + self.children.append(child) + return child + + def line(self, stream): + with selectors.DefaultSelector() as selector: + selector.register(stream, selectors.EVENT_READ) + self.assertTrue(selector.select(timeout=5), "timed out waiting for output") + return stream.readline().strip() + + def hold(self, root, label): + release = root / (label + ".release") + child = self.start(root, label, sys.executable, "-c", HOLD, label, str(release)) + return child, release + + def test_competing_suites_wait_for_entire_command(self): + first, release_first = self.hold(self.root, "test") + self.assertIn("acquired", self.line(first.stderr)) + self.assertEqual(self.line(first.stdout), "START test") + second, release_second = self.hold(self.root, "test-analysis") + self.assertIn("waiting for shared artifacts", self.line(second.stderr)) + self.assertIsNone(second.poll()) + release_first.touch() + self.assertEqual(self.line(first.stdout), "END test") + self.assertEqual(first.wait(timeout=5), 0) + self.assertIn("acquired", self.line(second.stderr)) + self.assertEqual(self.line(second.stdout), "START test-analysis") + release_second.touch() + self.assertEqual(self.line(second.stdout), "END test-analysis") + self.assertEqual(second.wait(timeout=5), 0) + + def test_node_command_retains_the_kernel_lock(self): + release = self.root / "node.release" + code = ("console.log('START node'); setInterval(() => {" + "if (require('node:fs').existsSync(process.argv[1])) process.exit(0);" + "}, 20)") + first = self.start(self.root, "node", "node", "--input-type=commonjs", + "-e", code, str(release)) + self.assertEqual(self.line(first.stdout), "START node") + second = self.start(self.root, "next", sys.executable, "-c", "pass") + self.assertIn("waiting", self.line(second.stderr)) + release.touch() + self.assertEqual(first.wait(timeout=5), 0) + _, err = second.communicate(timeout=5) + self.assertEqual(second.returncode, 0, err) + + def test_nested_command_does_not_deadlock(self): + nested = self.command(self.root, "nested", sys.executable, "-c", "print('nested ok')") + code = "import subprocess,sys; sys.exit(subprocess.call(" + repr(nested) + "))" + child = self.start(self.root, "outer", sys.executable, "-c", code) + out, err = child.communicate(timeout=5) + self.assertEqual(child.returncode, 0, err) + self.assertEqual(out.strip(), "nested ok") + self.assertEqual(err.count("acquired"), 1) + + def test_failure_releases_lock_and_preserves_exit_status(self): + child = self.start(self.root, "fails", sys.executable, "-c", "raise SystemExit(7)") + child.communicate(timeout=5) + self.assertEqual(child.returncode, 7) + successor = self.start(self.root, "next", sys.executable, "-c", "pass") + _, err = successor.communicate(timeout=5) + self.assertEqual(successor.returncode, 0, err) + self.assertNotIn("waiting", err) + + def test_killed_owner_releases_kernel_lock(self): + first, _ = self.hold(self.root, "killed") + self.assertIn("acquired", self.line(first.stderr)) + self.assertEqual(self.line(first.stdout), "START killed") + second = self.start(self.root, "next", sys.executable, "-c", "pass") + self.assertIn("waiting", self.line(second.stderr)) + first.kill() + self.assertEqual(first.wait(timeout=5), -signal.SIGKILL) + _, err = second.communicate(timeout=5) + self.assertEqual(second.returncode, 0, err) + self.assertIn("acquired", err) + + def test_separate_checkouts_do_not_block_each_other(self): + first, release_first = self.hold(self.root, "first") + self.assertEqual(self.line(first.stdout), "START first") + other_root = self.root / "other" + other_root.mkdir() + second, release_second = self.hold(other_root, "second") + self.assertIn("acquired", self.line(second.stderr)) + self.assertEqual(self.line(second.stdout), "START second") + self.assertIsNone(first.poll()) + release_first.touch() + release_second.touch() + + def test_missing_flock_fails_instead_of_running_unlocked(self): + shadow = Path(tempfile.mkdtemp(dir=self.root)) + (shadow / "fcntl.py").write_text('raise ImportError("no flock here")') + env = {**os.environ, "PYTHONPATH": str(shadow)} + child = self.start(self.root, "no-flock", sys.executable, "-c", "print('ran')", + env=env) + out, err = child.communicate(timeout=5) + self.assertEqual(child.returncode, 2, err) + self.assertNotIn("ran", out) + self.assertIn("requires POSIX flock", err) + + def test_stale_environment_marker_does_not_skip_lock(self): + first, release_first = self.hold(self.root, "owner") + self.assertEqual(self.line(first.stdout), "START owner") + stale = {"root": str(self.root), "pid": os.getpid(), "token": "old"} + env = {**os.environ, "RESCRIPT_TEST_LOCK": json.dumps(stale)} + second = self.start(self.root, "stale", sys.executable, "-c", "pass", env=env) + self.assertIn("waiting", self.line(second.stderr)) + release_first.touch() + _, err = second.communicate(timeout=5) + self.assertEqual(second.returncode, 0, err) + + +if __name__ == "__main__": + unittest.main(verbosity=2) diff --git a/scripts/with_test_lock.py b/scripts/with_test_lock.py new file mode 100644 index 00000000000..de130afff96 --- /dev/null +++ b/scripts/with_test_lock.py @@ -0,0 +1,84 @@ +#!/usr/bin/env python3 +"""Run a command under the checkout's shared-test-artifact lock (POSIX).""" + +import argparse +import json +import os +from pathlib import Path +import sys +import uuid + +LOCK_ENV = "RESCRIPT_TEST_LOCK" +LOCK_NAME = ".rescript-test.lock" + + +def inherited_lock(root): + """Recognize nested commands, without trusting a stale environment marker.""" + try: + owner = json.loads(os.environ.get(LOCK_ENV, "null")) + if not isinstance(owner, dict) or owner.get("root") != str(root): + return False + recorded = json.loads((root / LOCK_NAME).read_text()) + if recorded != owner: + return False + os.kill(owner["pid"], 0) + return True + except (OSError, ValueError, KeyError, TypeError): + return False + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--root", type=Path, default=Path(__file__).resolve().parent.parent) + parser.add_argument("--label", default="test command") + parser.add_argument("command", nargs=argparse.REMAINDER) + args = parser.parse_args() + command = args.command + if command[:1] == ["--"]: + command = command[1:] + if not command: + parser.error("expected a command after --") + root = args.root.resolve() + if inherited_lock(root): + os.execvpe(command[0], command, os.environ) + + try: + import fcntl + except ImportError: + # Windows has no flock, and CI runs these commands there. Degrade to an + # unlocked run instead of failing a supported platform. Anywhere else a + # missing flock is unexpected, so fail rather than silently race. + if sys.platform == "win32": + print(f"[test-lock] {args.label}: unlocked (no flock on this platform)", + file=sys.stderr, flush=True) + os.execvpe(command[0], command, os.environ) + parser.error("this lock prototype requires POSIX flock (macOS/Linux)") + + # Never unlink this file: waiters must all lock the same inode. Keeping it + # outside build directories also prevents ordinary test cleanup removing it. + fd = os.open(root / LOCK_NAME, os.O_CREAT | os.O_RDWR, 0o600) + try: + try: + fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) + except BlockingIOError: + print(f"[test-lock] {args.label}: waiting for shared artifacts in {root}", + file=sys.stderr, flush=True) + fcntl.flock(fd, fcntl.LOCK_EX) + + owner = {"root": str(root), "pid": os.getpid(), "token": uuid.uuid4().hex} + encoded = json.dumps(owner) + os.ftruncate(fd, 0) + os.write(fd, encoded.encode()) + os.set_inheritable(fd, True) + env = {**os.environ, LOCK_ENV: encoded} + print(f"[test-lock] {args.label}: acquired", file=sys.stderr, flush=True) + # exec preserves PID, signals and exit status. The inheritable descriptor + # holds the kernel lock for the command's lifetime; even SIGKILL releases + # it once processes inheriting that descriptor have exited. + os.execvpe(command[0], command, env) + finally: + os.close(fd) + + +if __name__ == "__main__": + main()