Skip to content

Throw EEXIST when ensureSymlink dest is a broken symlink#1076

Open
sarathfrancis90 wants to merge 1 commit into
jprichardson:masterfrom
sarathfrancis90:fix-ensuresymlink-broken-dest
Open

Throw EEXIST when ensureSymlink dest is a broken symlink#1076
sarathfrancis90 wants to merge 1 commit into
jprichardson:masterfrom
sarathfrancis90:fix-ensuresymlink-broken-dest

Conversation

@sarathfrancis90

Copy link
Copy Markdown
Contributor

Fixes #925.

ensureSymlink/ensureSymlinkSync stat the destination to check whether an existing symlink already points at srcpath. When that symlink is broken, following it fails and the raw ENOENT (stat '<dest>') bubbles up to the caller. That's surprising, since a symlink pointing at a different existing target already throws EEXIST — only the broken case leaked the stat error. @RyanZim noted the same direction in the issue.

I skip the idempotency check when the destination can't be followed, so the existing creation path reports EEXIST consistently whether the existing link is broken or just points elsewhere. Added async and sync regression tests; npm test passes locally.

ensureSymlink/ensureSymlinkSync stat() the destination to check whether
an existing symlink already points at srcpath. When that symlink is
broken, following it fails and the raw ENOENT (stat '<dest>') surfaced to
the caller, which is confusing: a symlink that points at a *different*
existing target already throws EEXIST. Skip the idempotency check when the
destination can't be followed so the existing creation path reports EEXIST
consistently, broken or not.

Closes jprichardson#925
Comment thread lib/ensure/symlink.js
let dstStat
try {
dstStat = await fs.stat(dstpath, { bigint: true })
} catch { }

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.

Seems like it might be wise to ensure the error is ENOENT before swallowing, and throw otherwise.

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.

Better error message for ensureSymlink if the current existing link is broken

2 participants