fix: improve error if lockfile exists but not a directory #6450
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6450 +/- ##
============================================
- Coverage 61.71% 61.65% -0.07%
============================================
Files 553 553
Lines 57900 57958 +58
Branches 1832 1833 +1
============================================
- Hits 35733 35732 -1
- Misses 22130 22189 +59
Partials 37 37 |
nflaig
force-pushed
the
nflaig/lockfile-errors
branch
from
525a557 to
4b1da59
Compare
Performance Report✔️ no performance regression detected Full benchmark results
|
ensi321
reviewed
Feb 19, 2024
| if (isLockfileError(e) && e.code === "ELOCKED") { | ||
| e.message = `${filepath} is already in use by another process`; | ||
| if (isLockfileError(e) && (e.code === "ELOCKED" || e.code === "ENOTDIR")) { | ||
| e.message = `'${filepath}' already in use by another process`; |
There was a problem hiding this comment.
I feel like this error message is not descriptive enough for the case of ENOTDIR.
Maybe have a separate error message like "Non-directory lockfile is deprecated. Most likely '${filepath}' is in use by another process running an old version, or orphaned lockfile during the upgrade"
There was a problem hiding this comment.
There a few cases where we would get error cde ENOTDIR
- A second Lodestar instance is running with version < 1.15.1
- Another client such as Teku is running
- Leftover files during upgrade
I do agree your suggested message is better for case 3. but this is very unlikely and the user will have to manually delete .lock files either way, even before 1.15.1 (but this issue is finally resolved from 1.15.1 onwards)
For cases 1. and 2. I think the current error is better and includes all details a user has to know and for case 3. the error message is better than what we had before.
ensi321
approved these changes
Feb 20, 2024
|
🎉 This PR is included in v1.16.0 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
We have upgraded the locking library in 1.15.1 (#6363) which now uses directories as .lock "files" this leads to a misleading error if users upgraded from previous versions and for some reason their previous .lock files are not cleaned up, see #6449.
There is also another case, where a user runs another client such as Teku which implements its own file locking but might also use files for locking instead of directories.
P.S. In case you wonder why proper-lockfile uses directories
Description
Improve error if lockfile exists but not a directory