The tests have quite a bit of duplicated code (both existing and my new tests). I can add a new block at the bottom of the file that adds the new tests for the sync, async and promise version without so much duplicated code if you want.

The tests have quite a bit of duplicated code (both existing and my new tests). I can add a new block at the bottom of the file that adds the new tests for the sync, async and promise version without so much duplicated code if you want.

For tests only, I generally prefer the repetition. (Ref: https://stackoverflow.com/a/11837973/436641)

Thanks for fixing the commit message, one question though: For something like addressing the review comments, should I always amend and force push or is another commit okay for this (I guess you can always squash at the end)?

Also I think the title of the PR is no longer 100% accurate, because deleting invalid links was also broken before (the stat call failed with ENOENT):

nathanael ~/tmp λ tree
.
└── b -> a

nathanael ~/tmp λ node -e 'fs.rmSync("b")'
node:internal/fs/utils:344
    throw err;
    ^

Error: ENOENT: no such file or directory, stat 'b'
    at Object.statSync (node:fs:1538:3)
    at __node_internal_ (node:internal/fs/utils:793:8)
    at Object.rmSync (node:fs:1213:13)
    at [eval]:1:4
    at Script.runInThisContext (node:vm:129:12)
    at Object.runInThisContext (node:vm:305:38)
    at node:internal/process/execution:76:19
    at [eval]-wrapper:6:22
    at evalScript (node:internal/process/execution:75:60)
    at node:internal/main/eval_string:27:3 {
  errno: -2,
  syscall: 'stat',
  code: 'ENOENT',
  path: 'b'
}

The loop thing was just the first issue I encountered.

/cc @nodejs/fs

Looks like existsSync (and therefore access) returns false on an invalid symlink (see my last commit). Is this also a bug or intended behaviour?

Looks like existsSync (and therefore access) returns false on an invalid symlink (see my last commit). Is this also a bug or intended behaviour?

I think this is intended. In general, symlinks are supposed to be transparent, so accessing invalid symlink should be equal to accessing its destination (e.g. nonexistent file).

Testing if broken symlink still exists can be performed explicitly with lstat instead.
Alternatively, improvements to access (like optional argument { verbatimSymlinks: false }) might also be considered.

CI: https://ci.nodejs.org/job/node-test-pull-request/48002/

Landed in 6f9175d

Thanks for the contribution!