Introduce new inter-process mutex implementation#7942
Introduce new inter-process mutex implementation#7942alcuadrado wants to merge 6 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 9d8bbc1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 21 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
This PR replaces the time-based stale lock detection mechanism in MultiProcessMutex with a PID-liveness-based approach. Instead of declaring locks stale after a timeout, the new implementation checks if the lock-owning process is still alive using process.kill(pid, 0). This change eliminates the risk of two processes running simultaneously in critical sections when long-running tasks exceed the previous timeout threshold.
Changes:
- Replaced file-based locking (
fs.writeFileSyncwithwx+flag) with directory-based locking (fs.mkdirSyncwithrecursive: false) - Implemented JSON metadata storage inside lock directories containing PID, hostname, platform, uid, and creation timestamp
- Added comprehensive error classes for incompatible locks (different hostname/platform/uid) and stale lock removal failures
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| v-next/hardhat-utils/src/synchronization.ts | Core implementation rewrite: directory-based locks with PID liveness checks, metadata storage, exit/signal handlers, and exponential backoff polling |
| v-next/hardhat-utils/src/errors/synchronization.ts | New error classes for mutex failures including timeout, stale locks, and incompatibility scenarios |
| v-next/hardhat-utils/test/synchronization.ts | Expanded test suite covering stale lock recovery, cross-process scenarios, error conditions, metadata handling, and exit/signal cleanup |
| v-next/hardhat-utils/test/fixture-projects/lock/lock-holder.ts | Test fixture for spawning child processes that acquire and hold locks |
| v-next/hardhat-utils/test/fixture-projects/lock/lock-and-exit.ts | Test fixture for verifying lock cleanup on process exit |
| v-next/hardhat-utils/test/helpers/synchronization.ts | Removed obsolete helper file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alcuadrado
added
no docs needed
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alcuadrado
force-pushed
the
new-lock-strategy
branch
from
8379f02 to
c7e0e5b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@ChristopherDedominici this is ready to be reviewed now |
alcuadrado
force-pushed
the
new-lock-strategy
branch
3 times, most recently
from
b1491ef to
38435c7
Compare
alcuadrado
force-pushed
the
new-lock-strategy
branch
from
844df79 to
5df73ff
Compare
alcuadrado
force-pushed
the
new-lock-strategy
branch
2 times, most recently
from
be6c846 to
755ae2e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alcuadrado
force-pushed
the
new-lock-strategy
branch
from
e8813bc to
7f38c86
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alcuadrado
force-pushed
the
new-lock-strategy
branch
from
04f2c5c to
9d8bbc1
Compare
|
Let's go for the lockfile version instead, because it's simpler to reason about, and the chances of people using hardhat in a network filesystem (AKA NFS) are really low. |
This PR introduces a new implementation of the
MultiProcessMutex, which inverts the behavior when compared to the previous one.Instead of considering a lock staled based on the time that the process has been waiting for it, it does it when the process that acquired it no longer exists. This can be really quick, without having to wait much. It's also less error prone.
Please read the jsdoc for more info.
This PR also fixes three related bugs:
downloadfunction in the request module was always using the same tmp path for every file with the same name, leading to more race-y code. This was only triggered because the locks weren't working (see 3)downloadfunction didn't properly wait for the file to be closed before moving it.