Skip to content

Fix rewatch lock races in watcher tests#8424

Open
cknitt wants to merge 2 commits into
masterfrom
lock-fix
Open

Fix rewatch lock races in watcher tests#8424
cknitt wants to merge 2 commits into
masterfrom
lock-fix

Conversation

@cknitt
Copy link
Copy Markdown
Member

@cknitt cknitt commented May 13, 2026

Summary

  • Make rewatch lock acquisition atomic so concurrent processes cannot both acquire the same lockfile.
  • Update watcher test helpers to wait for spawned watcher processes to exit after removing watch.lock.
  • Adapt the lock/watch tests so sequential test runs do not leave overlapping watchers in the same fixture workspace.

Why

CI was intermittently failing with corrupted compiled interface artifacts while running rewatch tests. The root causes were lock/test races:

  • lock acquisition used a check-then-create flow, allowing concurrent processes to both believe they acquired the same lock;
  • the shell test helper removed watch.lock but did not wait for the watcher process to terminate before the next test started.

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 13, 2026

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript@8424

@rescript/darwin-arm64

npm i https://pkg.pr.new/@rescript/darwin-arm64@8424

@rescript/darwin-x64

npm i https://pkg.pr.new/@rescript/darwin-x64@8424

@rescript/linux-arm64

npm i https://pkg.pr.new/@rescript/linux-arm64@8424

@rescript/linux-x64

npm i https://pkg.pr.new/@rescript/linux-x64@8424

@rescript/runtime

npm i https://pkg.pr.new/@rescript/runtime@8424

@rescript/win32-x64

npm i https://pkg.pr.new/@rescript/win32-x64@8424

commit: b68a935

@cknitt cknitt requested a review from jfrolich May 13, 2026 13:22
Comment thread rewatch/tests/utils.sh

exit_watcher() {
local watcher_pid="$1"
rm -f lib/watch.lock
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm why did this just remove the lockfile, that should be removed when killed

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah deleting the lock file would kill the watcher. Is that not the case anymore. Why do we need this complex setup. The pid is also in the lockfile, so we shouldn't need to pass it. But if it behaves correctly it should also exit so we shouldn't need to kill it. Maybe we need to await that it's really closed though? not sure though. At least killing shouldn't be needed, that's exactly what this test is testing. If it fails the behavior is wrong.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jfrolich Yeah there may have been some hallucinations here, sorry! Please have another look.
Now just waiting for the pid to disappear after removing the watcher lock file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants