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