Serialize test suites sharing build artifacts with a per-checkout lock - #8658
cristianoc wants to merge 1 commit into
Conversation
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 <ccrisccris@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fz7ZXPYxfMT8B1sRgqqDoD
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Prob a few rough edges -- but gives the idea. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfd52bf668
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch { | ||
| // Missing/stale ownership: acquire through the OS lock, never skip it. | ||
| } | ||
| const child = spawn( |
There was a problem hiding this comment.
When a direct node scripts/test.js process is killed with SIGKILL (or another fatal signal not handled below), only this parent dies; the spawned wrapper/test process remains alive and continues holding the checkout lock. I reproduced this by killing the original Node PID, after which a second lock command waited until the inner PID was manually killed. This can leave tests running unexpectedly and block later suites, so the child must be tied to the parent's lifetime or the persistent intermediary should be avoided.
AGENTS.md reference: AGENTS.md:L163-L166
Useful? React with 👍 / 👎.
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-8658 |
|
Thanks! Will test next week when I am back from vacation. First Python script that we have in this project, but that's required to access Could you address the Codex review feedback and open questions 1 and 2? |
2 participants
Why
Two agent sessions — or an agent plus a terminal — sharing one checkout run test suites concurrently and corrupt each other's build artifacts. The cost is worse than a lost run:
make test-analysiscleans Belt partway through someone else'smake test, and the victim reports missing-module or stale-artifact errors that look like real compiler bugs. The agent then chases a phantom failure, re-running and "fixing" code that was never broken.The conflicts:
make testbuilds it vialib, then cleans and rebuilds it in the mocha phase (scripts/test.js:89);tests/analysis_tests/Makefile:11cleans and rebuilds the same workspace as a workaround for Rebuild source dependencies when package output settings change #8539.scripts/test_syntax.shruns bothrm -rf tests/tempand rewrite the sameexpected/*.txt._build. Concurrentdune buildalready hard-fails on dune's own lock; this makes it wait instead.Approach
scripts/with_test_lock.pytakes aflockon.rescript-test.lockandexecs the command, so PID, signals and exit status are preserved, and the kernel releases the lock once every process inheriting the descriptor exits — including after SIGKILL. Rootmake test*targets route through it via a_locked-*indirection;node scripts/test.jsself-locks so direct invocation is covered too.Two choices worth review:
libbuild and suite cleanup still race.-j1so nestedclean testgoals stay ordered. This costs no real parallelism: the suites are sequential recipe lines (tests/analysis_tests/Makefile:3is fivemake -Clines in one recipe),test_syntax.shfans out on its own, 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 from an exited owner cannot skip locking.
Windows has no
flockand CI runs the suites there, so Windows runs unlocked with a notice; any other platform missingflockfails rather than racing silently.Testing
scripts/test_test_lock.py— contention, nesting, exit status, killed owners, separate checkouts, stale markers, and POSIX-without-flock. 8/8.make test-syntax→ exit 0 through the locked path. Two concurrent invocations: the second waited, then acquired; both green. Unlocked, those two clobber each other's fixtures.make checkformatclean. Fullmake test/make test-allnot run.No CHANGELOG entry — contributor tooling, no user-facing surface.
Open questions
scripts/test_test_lock.pycurrently gatesmake test, adding ~1 s of process-handshake tests with 5 s deadlines that could flake on a loaded machine. Move it to its own target?make test-allis not atomic — the lock drops between leaf targets, so a concurrent session can clean Belt mid-sequence. Left alone since it is run rarely.scripts/test.js, where 39tests/build_tests/*directories are awaited one at a time (scripts/test.js:153). Unaffected by this change, since Make's-jnever reached that script.🤖 Generated with Claude Code
https://claude.ai/code/session_01Fz7ZXPYxfMT8B1sRgqqDoD