libutil: the retry after a failed `rename` in `replaceSymlink` is unreachable on POSIX, and would leak on Windows

[awsmadi]

Disclosure: the analysis below was produced with AI assistance (Claude Code,
claude-opus-5).

Summary

In replaceSymlink (src/libutil/file-system.cc:550-574) the two continue branches look symmetric
but are not, and the second is wrong under either reading of its reachability.

void replaceSymlink(const std::filesystem::path & target, const std::filesystem::path & link)
{
    for (unsigned int n = 0; true; n++) {
        auto tmp = link.parent_path() / std::filesystem::path{fmt(".%d_%s", n, link.filename().string())};
        tmp = tmp.lexically_normal();

        try {
            std::filesystem::create_symlink(target, tmp);
        } catch (std::filesystem::filesystem_error & e) {
            if (e.code() == std::errc::file_exists)
                continue;                                   // (A)
            throw SystemError(e.code(), "creating symlink %1% -> %2%", PathFmt(tmp), PathFmt(target));
        }

        try {
            std::filesystem::rename(tmp, link);
        } catch (std::filesystem::filesystem_error & e) {
            if (e.code() == std::errc::file_exists)
                continue;                                   // (B)
            throw SystemError(e.code(), "renaming %1% to %2%", PathFmt(tmp), PathFmt(link));
        }

        break;
    }
}

(A) is correct. create_symlink failed because the name was taken, nothing was created, and a
different n is exactly the right retry.

(B) is not. By the time it runs, the temporary symlink exists. Taking continue builds a new
temporary under n+1 and nothing ever removes the previous one.

Whether (B) is reachable, measured

I checked rather than assuming, and the answer differs by platform.

On Linux, EEXIST from this rename appears to be unreachable, so (B) is dead code. Renaming a
non-directory onto a directory gives EISDIR, not EEXIST, measured for both an empty and a
non-empty destination directory, and renaming onto an existing regular file or symlink simply
succeeds, which is the documented rename(2) behavior. EXDEV, EROFS and EACCES are likewise not
EEXIST. So on POSIX the existing code already throws and the retry never runs.

On Windows I could not test this. std::filesystem::rename is implemented over MoveFileExW there
rather than rename(2), and a directory at the destination is a case that implementation can fail
rather than replace, so file_exists looks reachable. I have no Windows host and Wine is not a sound
oracle for MoveFileEx semantics, so I am not claiming it.

Why it is worth fixing under either reading

If (B) is unreachable, it is dead code that reads as a deliberate retry and invites the reader to
believe a rename failure is recoverable here.

If (B) is reachable on any platform, it is worse than a leak. The condition that triggers it does not
depend on n, so every subsequent attempt fails identically: the loop does not terminate, and each
pass leaves one more .<n>_<name> symlink in the parent directory.

I have not observed either in the wild. This comes from reading the file while working on Windows
portability.

Possible remedy

Deleting (B) covers both readings, and removing the temporary on the failure path makes the function
leak-free regardless of which errors turn out to be reachable:

        /* From here the temporary symlink exists, so every path out other than a
           successful rename has to remove it. */
        try {
            std::filesystem::rename(tmp, link);
        } catch (std::filesystem::filesystem_error & e) {
            std::error_code ignored;
            std::filesystem::remove(tmp, ignored);
            throw SystemError(e.code(), "renaming %1% to %2%", PathFmt(tmp), PathFmt(link));
        }

I am filing this as an issue rather than a pull request because whether (B) should be deleted as dead
code or fixed as a live Windows bug depends on the platform question above, and that is yours to
answer rather than mine to assume.

Unrelated to the retry, but visible in the same few lines: createSymlink immediately above uses
SysError(ec.value(), ...) where this function uses SystemError(e.code(), ...).