From 9c0fa10162349c1f49fdb4c5a098c28843ed590c Mon Sep 17 00:00:00 2001 From: Robert Date: Sun, 30 Aug 2026 08:51:06 -0400 Subject: [PATCH 1/6] fix: surface connection failures and clean failed mounts --- lua/sshfs/lib/mount_point.lua | 20 +++++++++--- lua/sshfs/lib/ssh.lua | 15 +++++++-- lua/sshfs/lib/sshfs.lua | 61 +++++++++++++++++++++++++++++++---- lua/sshfs/session.lua | 13 ++++++-- 4 files changed, 94 insertions(+), 15 deletions(-) diff --git a/lua/sshfs/lib/mount_point.lua b/lua/sshfs/lib/mount_point.lua index 888acc4..cc23d74 100644 --- a/lua/sshfs/lib/mount_point.lua +++ b/lua/sshfs/lib/mount_point.lua @@ -126,14 +126,26 @@ end --- Get or create mount directory --- @param mount_dir string|nil Directory path (defaults to base mount dir from config) ---- @return boolean True if directory exists or was created successfully +--- @return boolean success True if directory exists or was created successfully +--- @return boolean created True only when this call created the directory function MountPoint.get_or_create(mount_dir) mount_dir = mount_dir or Config.get_base_dir() local stat = vim.uv.fs_stat(mount_dir) - if stat and stat.type == "directory" then return true end + if stat and stat.type == "directory" then return true, false end - local success = vim.fn.mkdir(mount_dir, "p") - return success == 1 + -- Ensure parent directories exist, then create the leaf without "p" so + -- ownership is only claimed when this call actually creates mount_dir. + local parent_dir = vim.fn.fnamemodify(mount_dir, ":h") + if parent_dir ~= mount_dir and vim.fn.mkdir(parent_dir, "p") == 0 and vim.fn.isdirectory(parent_dir) ~= 1 then + return false, false + end + + if vim.fn.mkdir(mount_dir) == 1 then return true, true end + + -- Another process may have won the creation race. Treat an existing + -- directory as usable, but do not claim ownership of it. + stat = vim.uv.fs_stat(mount_dir) + return stat ~= nil and stat.type == "directory", false end --- Release buffers associated with sshfs mount diff --git a/lua/sshfs/lib/ssh.lua b/lua/sshfs/lib/ssh.lua index f31a8b7..c469c0c 100644 --- a/lua/sshfs/lib/ssh.lua +++ b/lua/sshfs/lib/ssh.lua @@ -3,6 +3,17 @@ local Ssh = {} +--- Return the first non-empty process output after trimming whitespace. +--- @param ... string|nil Process output values in priority order +--- @return string|nil error_output First non-empty output, or nil +local function first_nonempty_output(...) + for i = 1, select("#", ...) do + local trimmed = vim.trim(select(i, ...) or "") + if trimmed ~= "" then return trimmed end + end + return nil +end + --- Get SSH socket directory, creating it if it doesn't exist --- @return string|nil socket_dir The socket directory path, or nil if creation failed --- @return string|nil error_msg Error message if creation failed @@ -161,7 +172,7 @@ function Ssh.get_remote_home(host, callback) callback(nil, "Remote $HOME output invalid: '" .. home_path .. "'") end else - local error_msg = vim.trim(obj.stderr or obj.stdout or "Unknown error") + local error_msg = first_nonempty_output(obj.stderr, obj.stdout) or "Unknown error" callback(nil, error_msg) end end) @@ -223,7 +234,7 @@ function Ssh.try_batch_connect(host, callback) vim.system(cmd, { text = true }, function(obj) vim.schedule(function() local success = obj.code == 0 - local error_msg = success and nil or (obj.stderr or obj.stdout or "Unknown error") + local error_msg = success and nil or (first_nonempty_output(obj.stderr, obj.stdout) or "Unknown error") callback(success, obj.code, error_msg) end) end) diff --git a/lua/sshfs/lib/sshfs.lua b/lua/sshfs/lib/sshfs.lua index 9e5131d..8eb0e54 100644 --- a/lua/sshfs/lib/sshfs.lua +++ b/lua/sshfs/lib/sshfs.lua @@ -3,6 +3,16 @@ local Sshfs = {} +--- Determine whether an SSH batch failure is an unresolved-host error. +--- @param error_message string|nil SSH error output +--- @return boolean True when the host could not be resolved +local function is_unresolved_host_error(error_message) + local error_lower = (error_message or ""):lower() + return error_lower:find("could not resolve hostname", 1, true) ~= nil + or error_lower:find("name or service not known", 1, true) ~= nil + or error_lower:find("nodename nor servname provided", 1, true) ~= nil +end + --- Convert sshfs_options table to array format for sshfs -o --- @param options_table table Table of options (e.g., {reconnect = true, ConnectTimeout = 5}) --- @return table Array of option strings (e.g., {"reconnect", "ConnectTimeout=5"}) @@ -77,10 +87,19 @@ local function mount_with_path(host, mount_point, remote_path_suffix, callback) resolved_path = remote_path_suffix, }) else - local error_msg = obj.stderr or obj.stdout or "Unknown error" + local stderr = vim.trim(obj.stderr or "") + local stdout = vim.trim(obj.stdout or "") + local error_output = stderr ~= "" and stderr or stdout + local message = string.format("Mount failed (exit code: %d)", obj.code) + if error_output ~= "" then message = message .. ": " .. error_output end + callback({ success = false, - message = "Mount failed: " .. error_msg, + stage = "mount", + exit_code = obj.code, + stdout = stdout ~= "" and stdout or nil, + stderr = stderr ~= "" and stderr or nil, + message = message, }) end end) @@ -136,16 +155,44 @@ function Sshfs.authenticate_and_mount(host, mount_point, remote_path_suffix, cal return end + -- An unresolved host cannot be fixed by interactive authentication. + if is_unresolved_host_error(error) then + local message = string.format("SSH connection failed for %s (exit code: %d)", host.name, exit_code) + if error and error ~= "" then message = message .. ": " .. vim.trim(error) end + + callback({ + success = false, + stage = "connection", + exit_code = exit_code, + output = error, + message = message, + }) + return + end + -- Batch failed, try interactive terminal Ssh.open_auth_terminal(host.name, function(term_success, term_exit_code) if term_success then mount_via_socket(host, mount_point, remote_path_suffix, callback) - else - callback({ - success = false, - message = string.format("SSH authentication failed for %s (exit code: %d)", host.name, term_exit_code), - }) + return end + + local message = string.format( + "SSH authentication failed for %s (batch exit code: %d, interactive exit code: %d)", + host.name, + exit_code, + term_exit_code + ) + if error and error ~= "" then message = message .. ": " .. vim.trim(error) end + + callback({ + success = false, + stage = "authentication", + exit_code = term_exit_code, + batch_exit_code = exit_code, + output = error, + message = message, + }) end) end) end diff --git a/lua/sshfs/session.lua b/lua/sshfs/session.lua index d805763..3de5e70 100644 --- a/lua/sshfs/session.lua +++ b/lua/sshfs/session.lua @@ -51,7 +51,9 @@ function Session.connect(host) PRE_MOUNT_DIRS[mount_dir] = vim.uv.cwd() -- Ensure the unique mount directory exists - if not MountPoint.get_or_create(mount_dir) then + local mount_ready, mount_created = MountPoint.get_or_create(mount_dir) + if not mount_ready then + PRE_MOUNT_DIRS[mount_dir] = nil vim.notify("Failed to create mount directory: " .. mount_dir, vim.log.levels.ERROR) return end @@ -61,8 +63,15 @@ function Session.connect(host) Sshfs.authenticate_and_mount(host, mount_dir, remote_path_suffix, function(result) -- Handle connection failure if not result.success then + PRE_MOUNT_DIRS[mount_dir] = nil + + -- Only remove an empty mount directory created by this connection attempt. + if mount_created and not MountPoint.is_active(mount_dir) then + local Directory = require("sshfs.lib.directory") + if Directory.is_empty(mount_dir) then pcall(vim.fn.delete, mount_dir, "d") end + end + vim.notify("Connection failed: " .. (result.message or "Unknown error"), vim.log.levels.ERROR) - MountPoint.cleanup() return end From 41170ac9cfd1d05a019e45e9920dd37f14caacd0 Mon Sep 17 00:00:00 2001 From: Robert Date: Sun, 30 Aug 2026 08:59:24 -0400 Subject: [PATCH 2/6] docs: clarify process output return annotation --- lua/sshfs/lib/ssh.lua | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lua/sshfs/lib/ssh.lua b/lua/sshfs/lib/ssh.lua index c469c0c..ba3ca0c 100644 --- a/lua/sshfs/lib/ssh.lua +++ b/lua/sshfs/lib/ssh.lua @@ -5,7 +5,7 @@ local Ssh = {} --- Return the first non-empty process output after trimming whitespace. --- @param ... string|nil Process output values in priority order ---- @return string|nil error_output First non-empty output, or nil +--- @return string|nil output First non-empty output, or nil local function first_nonempty_output(...) for i = 1, select("#", ...) do local trimmed = vim.trim(select(i, ...) or "") From f0460c8225878eb847195cbfcdcb6ed0b7b351a0 Mon Sep 17 00:00:00 2001 From: Robert Hill Date: Sun, 30 Aug 2026 23:51:02 -0400 Subject: [PATCH 3/6] fix: handle mkdir failures in MountPoint.get_or_create `vim.fn.mkdir` raises E739 rather than returning 0 on failure, so the non-"p" leaf creation threw out of Session.connect instead of reporting "Failed to create mount directory", and the creation-race fallback was unreachable. Wrap both mkdir calls in pcall so the function returns false as documented. --- lua/sshfs/lib/mount_point.lua | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lua/sshfs/lib/mount_point.lua b/lua/sshfs/lib/mount_point.lua index cc23d74..fb34a32 100644 --- a/lua/sshfs/lib/mount_point.lua +++ b/lua/sshfs/lib/mount_point.lua @@ -135,12 +135,15 @@ function MountPoint.get_or_create(mount_dir) -- Ensure parent directories exist, then create the leaf without "p" so -- ownership is only claimed when this call actually creates mount_dir. + -- vim.fn.mkdir raises E739 instead of returning 0, so every call is wrapped. local parent_dir = vim.fn.fnamemodify(mount_dir, ":h") - if parent_dir ~= mount_dir and vim.fn.mkdir(parent_dir, "p") == 0 and vim.fn.isdirectory(parent_dir) ~= 1 then - return false, false + if parent_dir ~= mount_dir then + local parent_ok, parent_created = pcall(vim.fn.mkdir, parent_dir, "p") + if (not parent_ok or parent_created == 0) and vim.fn.isdirectory(parent_dir) ~= 1 then return false, false end end - if vim.fn.mkdir(mount_dir) == 1 then return true, true end + local created_ok, created = pcall(vim.fn.mkdir, mount_dir) + if created_ok and created == 1 then return true, true end -- Another process may have won the creation race. Treat an existing -- directory as usable, but do not claim ownership of it. From eb24dff6affac5a2d891a2229177e62905150b0a Mon Sep 17 00:00:00 2001 From: Robert Hill Date: Mon, 31 Aug 2026 00:19:24 -0400 Subject: [PATCH 4/6] test: add connection failure and mount cleanup coverage Adds the regression suite this PR describes, on top of the shared harness. Covers unresolved-host diagnostics in both the Linux and macOS resolver wordings, including that they skip interactive authentication while other batch failures still reach it, and that authentication failures report both exit codes. Covers SSHFS failure reporting: the exit code is preserved, stderr wins over stdout, whitespace-only output is not treated as real output, and a silent failure still reports its code. Covers mount directory ownership from get_or_create, including the creation race and the non-raising failure path, and the cleanup rules after a failed attempt: only a directory this attempt created and left empty is removed, never one that pre-existed, holds contents, or turns out to be an active mount. Also covers that a cleanup error does not mask the connection error and that a failed attempt is never registered in the lockfile. Refs #24 --- tests/connection_failure_spec.lua | 227 ++++++++++++++++++++++++++++ tests/failed_mount_cleanup_spec.lua | 146 ++++++++++++++++++ 2 files changed, 373 insertions(+) create mode 100644 tests/connection_failure_spec.lua create mode 100644 tests/failed_mount_cleanup_spec.lua diff --git a/tests/connection_failure_spec.lua b/tests/connection_failure_spec.lua new file mode 100644 index 0000000..a5fae62 --- /dev/null +++ b/tests/connection_failure_spec.lua @@ -0,0 +1,227 @@ +-- tests/connection_failure_spec.lua +-- Connection failure diagnostics and mount directory ownership + +local RESOLVE_ERROR = "ssh: Could not resolve hostname nope: Name or service not known" + +--- Drive authenticate_and_mount with stubbed SSH and sshfs subprocesses +--- @param opts table {batch_success, batch_code, batch_error, interactive_success, +--- interactive_code, mount_code, mount_stdout, mount_stderr} +--- @return table result Callback result table +--- @return table events Ordered list of the SSH stages that ran +local function connect(opts) + stub.reload() + require("sshfs.config").setup({}) + + local events = {} + package.loaded["sshfs.lib.ssh"] = { + try_batch_connect = function(_, callback) + table.insert(events, "batch") + callback(opts.batch_success or false, opts.batch_code or 255, opts.batch_error) + end, + open_auth_terminal = function(_, callback) + table.insert(events, "interactive") + callback(opts.interactive_success or false, opts.interactive_code or 1) + end, + build_command_string = function() + return "ssh" + end, + get_remote_home = function(_, callback) + table.insert(events, "remote_home") + callback("/home/deploy", nil) + end, + } + + stub.notifications() + stub.set("schedule", function(fn) + fn() + end) + stub.set("system", function(_, _, callback) + table.insert(events, "sshfs") + local result = { + code = opts.mount_code or 0, + stdout = opts.mount_stdout or "", + stderr = opts.mount_stderr or "", + } + if callback then callback(result) end + return { + wait = function() + return result + end, + } + end) + + local result = nil + require("sshfs.lib.sshfs").authenticate_and_mount( + { name = "example.com" }, + "/home/tester/mnt/example", + "/srv/app", + function(callback_result) + result = callback_result + end + ) + stub.restore_all() + + return result, events +end + +describe("unresolved host handling", function() + it("fails fast instead of opening an interactive prompt", function() + local result, events = connect({ batch_error = RESOLVE_ERROR }) + + expect.eq(events, { "batch" }, "an unresolved host cannot be fixed by authenticating") + expect.falsy(result.success) + expect.eq(result.stage, "connection") + end) + + it("surfaces the underlying SSH message", function() + local result = connect({ batch_error = RESOLVE_ERROR }) + + expect.contains(result.message, "Could not resolve hostname") + expect.contains(result.message, "example.com") + expect.eq(result.exit_code, 255) + end) + + it("recognizes the macOS resolver wording", function() + local _, events = connect({ + batch_error = "ssh: Could not resolve hostname nope: nodename nor servname provided, or not known", + }) + + expect.eq(events, { "batch" }) + end) + + it("still offers interactive authentication for other batch failures", function() + local _, events = connect({ batch_error = "Permission denied (publickey,password)." }) + + expect.eq(events, { "batch", "interactive" }, "a credential failure is worth prompting for") + end) +end) + +describe("authentication failure reporting", function() + it("reports both the batch and interactive exit codes", function() + local result = connect({ + batch_error = "Permission denied (publickey).", + batch_code = 255, + interactive_code = 1, + }) + + expect.eq(result.stage, "authentication") + expect.contains(result.message, "batch exit code: 255") + expect.contains(result.message, "interactive exit code: 1") + expect.contains(result.message, "Permission denied") + end) +end) + +describe("mount failure reporting", function() + it("preserves the sshfs exit code and stderr", function() + local result = connect({ + batch_success = true, + mount_code = 1, + mount_stderr = "read: Connection reset by peer", + }) + + expect.falsy(result.success) + expect.eq(result.stage, "mount") + expect.eq(result.exit_code, 1) + expect.eq(result.stderr, "read: Connection reset by peer") + expect.contains(result.message, "exit code: 1") + expect.contains(result.message, "Connection reset by peer") + end) + + it("falls back to stdout when stderr is empty", function() + local result = connect({ + batch_success = true, + mount_code = 1, + mount_stdout = "remote host has disconnected", + mount_stderr = " ", + }) + + expect.contains(result.message, "remote host has disconnected") + expect.is_nil(result.stderr, "whitespace-only stderr is not real output") + end) + + it("still reports an exit code when the process says nothing", function() + local result = connect({ batch_success = true, mount_code = 32 }) + + expect.contains(result.message, "exit code: 32") + expect.is_nil(result.stdout) + expect.is_nil(result.stderr) + end) + + it("reports success for a clean mount", function() + local result = connect({ batch_success = true, mount_code = 0 }) + + expect.truthy(result.success) + expect.eq(result.resolved_path, "/srv/app") + end) +end) + +describe("MountPoint.get_or_create ownership", function() + local function temp_path() + return vim.fn.tempname() .. "/sshfs-test" + end + + it("claims ownership of a directory it created", function() + stub.reload() + local MountPoint = require("sshfs.lib.mount_point") + local path = temp_path() + + local success, created = MountPoint.get_or_create(path) + expect.truthy(success) + expect.truthy(created) + + vim.fn.delete(vim.fn.fnamemodify(path, ":h"), "rf") + end) + + it("does not claim ownership of a pre-existing directory", function() + stub.reload() + local MountPoint = require("sshfs.lib.mount_point") + local path = temp_path() + vim.fn.mkdir(path, "p") + + local success, created = MountPoint.get_or_create(path) + expect.truthy(success) + expect.falsy(created, "another process may still be using a directory we did not create") + + vim.fn.delete(vim.fn.fnamemodify(path, ":h"), "rf") + end) + + it("treats a directory that appears mid-call as usable but unowned", function() + stub.reload() + local MountPoint = require("sshfs.lib.mount_point") + local path = temp_path() + + -- Simulate losing the creation race: the directory really is on disk (so the + -- leaf mkdir raises E739 for real), but the initial stat reports it missing, + -- exactly as it would if another process created it a moment later. + vim.fn.mkdir(path, "p") + local real_fs_stat = vim.uv.fs_stat + local stats = 0 + stub.set("uv.fs_stat", function(target) + stats = stats + 1 + if stats == 1 and target == path then return nil end + return real_fs_stat(target) + end) + + local success, created = expect.no_error(function() + return MountPoint.get_or_create(path) + end) + stub.restore_all() + + expect.truthy(success, "the directory exists and is usable") + expect.falsy(created, "the race loser must not claim ownership") + + vim.fn.delete(vim.fn.fnamemodify(path, ":h"), "rf") + end) + + it("reports failure without raising when the directory cannot be created", function() + stub.reload() + local MountPoint = require("sshfs.lib.mount_point") + + local success, created = expect.no_error(function() + return MountPoint.get_or_create("/proc/definitely-not-writable/mount") + end) + + expect.falsy(success, "vim.fn.mkdir raises E739 rather than returning 0") + expect.falsy(created) + end) +end) diff --git a/tests/failed_mount_cleanup_spec.lua b/tests/failed_mount_cleanup_spec.lua new file mode 100644 index 0000000..3f81cf1 --- /dev/null +++ b/tests/failed_mount_cleanup_spec.lua @@ -0,0 +1,146 @@ +-- tests/failed_mount_cleanup_spec.lua +-- Mount directory cleanup after a failed connection +-- +-- A failed attempt must remove only a directory it created and left empty. +-- Anything else may belong to another Neovim process or hold real data. + +local BASE_DIR = "/home/tester/mnt" +local MOUNT_DIR = BASE_DIR .. "/example_srv_app" + +--- Drive Session.connect through a stubbed mount attempt +--- @param opts table {mount_created, mount_ready, is_active, is_empty, delete_fails, mount_succeeds} +--- @return table outcome {deleted, notifications, registered} +local function attempt_connection(opts) + stub.reload() + require("sshfs.config").setup({ mounts = { base_dir = BASE_DIR } }) + + package.loaded["sshfs.ui.ask"] = { + for_mount_path = function(_, _, callback) + callback("/srv/app") + end, + } + + package.loaded["sshfs.lib.mount_point"] = { + is_active = function() + return opts.is_active or false + end, + get_or_create = function() + if opts.mount_ready == false then return false, false end + return true, opts.mount_created ~= false + end, + cleanup = function() end, + } + + package.loaded["sshfs.lib.directory"] = { + is_empty = function() + return opts.is_empty ~= false + end, + } + + local registered = {} + package.loaded["sshfs.lib.lockfile"] = { + register = function(dir) + table.insert(registered, dir) + end, + } + + package.loaded["sshfs.ui.hooks"] = { + on_mount = function() end, + } + + package.loaded["sshfs.lib.sshfs"] = { + authenticate_and_mount = function(_, _, remote_path_suffix, callback) + if opts.mount_succeeds then + callback({ success = true, resolved_path = remote_path_suffix }) + else + callback({ success = false, message = "Mount failed (exit code: 1): permission denied" }) + end + end, + } + + local deleted = {} + stub.set("fn.delete", function(path, flags) + if opts.delete_fails then error("E739: could not delete " .. path) end + table.insert(deleted, { path = path, flags = flags }) + return 0 + end) + local notifications = stub.notifications() + + local ok, err = pcall(require("sshfs.session").connect, { name = "example" }) + stub.restore_all() + + return { deleted = deleted, notifications = notifications, registered = registered, ok = ok, err = err } +end + +--- Whether the mount directory was removed +local function removed_mount_dir(outcome) + for _, entry in ipairs(outcome.deleted) do + if entry.path == MOUNT_DIR then return true end + end + return false +end + +describe("failed connection cleanup", function() + it("removes an empty directory the attempt created", function() + local outcome = attempt_connection({ mount_created = true, is_empty = true }) + + expect.truthy(removed_mount_dir(outcome)) + expect.eq(outcome.deleted[1].flags, "d", "only an empty directory is removed, never a tree") + end) + + it("keeps a directory the attempt did not create", function() + local outcome = attempt_connection({ mount_created = false, is_empty = true }) + + expect.falsy(removed_mount_dir(outcome), "another process may own a directory that already existed") + end) + + it("keeps a directory that is not empty", function() + local outcome = attempt_connection({ mount_created = true, is_empty = false }) + + expect.falsy(removed_mount_dir(outcome), "unexpected contents must not be deleted") + end) + + it("keeps a directory that turns out to be an active mount", function() + local outcome = attempt_connection({ mount_created = true, is_empty = true, is_active = true }) + + expect.falsy(removed_mount_dir(outcome), "a live mount must never be removed by a failure path") + end) + + it("reports the underlying failure to the user", function() + local outcome = attempt_connection({ mount_created = true }) + + expect.eq(#outcome.notifications, 1) + expect.contains(outcome.notifications[1].message, "Connection failed") + expect.contains(outcome.notifications[1].message, "permission denied") + expect.eq(outcome.notifications[1].level, vim.log.levels.ERROR) + end) + + it("does not let a cleanup error mask the connection error", function() + local outcome = attempt_connection({ mount_created = true, is_empty = true, delete_fails = true }) + + expect.truthy(outcome.ok, "cleanup is best-effort and must not raise out of the callback") + expect.contains(outcome.notifications[1].message, "Connection failed") + end) + + it("does not register a failed attempt in the lockfile", function() + local outcome = attempt_connection({ mount_created = true }) + expect.eq(outcome.registered, {}) + end) + + it("reports a directory that could not be created without attempting a mount", function() + local outcome = attempt_connection({ mount_ready = false }) + + expect.eq(#outcome.notifications, 1) + expect.contains(outcome.notifications[1].message, "Failed to create mount directory") + expect.falsy(removed_mount_dir(outcome)) + end) +end) + +describe("successful connection", function() + it("keeps the mount directory and registers it", function() + local outcome = attempt_connection({ mount_created = true, mount_succeeds = true }) + + expect.falsy(removed_mount_dir(outcome)) + expect.eq(outcome.registered, { MOUNT_DIR }) + end) +end) From 41bdeda19d64c007b17f2e5a9fee984f6917705b Mon Sep 17 00:00:00 2001 From: Robert Hill Date: Mon, 31 Aug 2026 00:42:50 -0400 Subject: [PATCH 5/6] feat(health): warn when the socket path exceeds the Unix socket limit ControlMaster sockets are Unix domain sockets, so their paths are capped by sun_path: 108 bytes on Linux, 104 on macOS and the BSDs. SSH appends "/%C" plus a temporary suffix while creating one, leaving 49 characters for connections.socket_dir on Linux and 45 on macOS. Exceeding that makes every connection fail with "unix_listener: path ... too long for Unix domain socket", which names neither the setting at fault nor the limit involved. Report it from :checkhealth instead: an error past the budget, a warning within ten characters of it, and the measured budget either way. The check runs outside the ~/.ssh branch, since socket_dir is configurable and need not live there. --- lua/sshfs/health.lua | 51 ++++++++++ tests/health_socket_path_spec.lua | 150 ++++++++++++++++++++++++++++++ 2 files changed, 201 insertions(+) create mode 100644 tests/health_socket_path_spec.lua diff --git a/lua/sshfs/health.lua b/lua/sshfs/health.lua index ed44825..cac8332 100644 --- a/lua/sshfs/health.lua +++ b/lua/sshfs/health.lua @@ -113,6 +113,53 @@ local function check_system_dependencies() end end +--- Check that ControlMaster sockets fit inside the Unix socket path limit +--- ControlMaster sockets are Unix domain sockets, and the kernel caps their +--- path at sun_path: 108 bytes on Linux, 104 on macOS and the BSDs. SSH builds +--- one by appending "/%C" (a 40 character hash) plus a temporary suffix while +--- creating it, so a socket directory that is merely long makes every +--- connection fail with a cryptic "unix_listener: path ... too long for Unix +--- domain socket" instead of anything that points at the real cause. +local function check_socket_path_length() + local Config = require("sshfs.config") + local socket_dir = Config.get_socket_dir() + + local is_bsd = vim.fn.has("mac") == 1 or vim.fn.has("bsd") == 1 + local sun_path_max = is_bsd and 104 or 108 + -- "/" + 40 character %C hash + "." + 16 character temporary suffix. + local reserved = 58 + local budget = sun_path_max - 1 - reserved + local length = #socket_dir + + if length > budget then + health.error( + string.format( + "SSH socket directory path is too long: %d characters, but only %d fit (%s)", + length, + budget, + socket_dir + ), + "Every connection will fail with 'unix_listener: path ... too long for Unix domain socket'. " + .. "Set connections.socket_dir to a shorter path, for example ~/.ssh/sockets." + ) + elseif length > budget - 10 then + health.warn( + string.format( + "SSH socket directory path is close to the limit: %d of %d characters (%s)", + length, + budget, + socket_dir + ), + "Connections still work, but a longer hostname or a deeper path will break them. " + .. "Consider a shorter connections.socket_dir." + ) + else + health.ok( + string.format("SSH socket directory path fits the Unix socket limit (%d of %d characters)", length, budget) + ) + end +end + --- Check SSH configuration local function check_ssh_config() health.start("SSH Configuration") @@ -178,6 +225,10 @@ local function check_ssh_config() "Create SSH directory with: mkdir -p ~/.ssh && chmod 700 ~/.ssh" ) end + + -- Checked outside the ~/.ssh branch above: a custom socket_dir can live + -- anywhere, so this must run even when ~/.ssh is missing. + check_socket_path_length() end --- Check mount configuration diff --git a/tests/health_socket_path_spec.lua b/tests/health_socket_path_spec.lua new file mode 100644 index 0000000..2d5266c --- /dev/null +++ b/tests/health_socket_path_spec.lua @@ -0,0 +1,150 @@ +-- tests/health_socket_path_spec.lua +-- checkhealth warning for ControlMaster socket paths that exceed sun_path +-- +-- A long socket_dir makes every connection fail with a cryptic +-- "unix_listener: path ... too long for Unix domain socket", which is exactly +-- the kind of failure this PR exists to make legible. + +--- Run :checkhealth with a given socket directory and collect the reports +--- @param socket_dir string +--- @param platform string|nil "mac" to simulate the 104 byte BSD limit +--- @return table reports List of {level, message, advice} +local function health_reports(socket_dir, platform) + stub.reload() + + local reports = {} + local function record(level) + return function(message, advice) + table.insert(reports, { level = level, message = message, advice = advice }) + end + end + + stub.set("health", { + start = function() end, + ok = record("ok"), + info = record("info"), + warn = record("warn"), + error = record("error"), + }) + stub.set("fn.has", function(feature) + return (platform and feature == platform) and 1 or 0 + end) + + require("sshfs.config").setup({ connections = { socket_dir = socket_dir } }) + require("sshfs.health").check() + stub.restore_all() + + return reports +end + +--- The single report mentioning the socket directory path length +local function socket_path_report(reports) + for _, report in ipairs(reports) do + if type(report.message) == "string" and report.message:find("socket directory path", 1, true) then return report end + end + return nil +end + +--- Build a socket directory of an exact character length +local function dir_of_length(length) + local prefix = "/tmp/" + return prefix .. string.rep("s", length - #prefix) +end + +-- Linux: 108 byte sun_path, minus the trailing NUL and the 58 characters ssh +-- appends ("/" + 40 character %C hash + "." + 16 character suffix) = 49. +local LINUX_BUDGET = 49 +local MAC_BUDGET = 45 + +describe("checkhealth socket path length", function() + it("accepts a short socket directory", function() + local report = socket_path_report(health_reports("/home/me/.ssh/sockets")) + + expect.truthy(report) + expect.eq(report.level, "ok") + end) + + it("reports the budget it measured against", function() + local report = socket_path_report(health_reports("/home/me/.ssh/sockets")) + expect.contains(report.message, "of " .. LINUX_BUDGET .. " characters") + end) + + it("errors when the path cannot fit a control socket", function() + local report = socket_path_report(health_reports(dir_of_length(LINUX_BUDGET + 1))) + + expect.truthy(report) + expect.eq(report.level, "error", "every connection would fail, so this is not merely a warning") + expect.contains(report.message, "too long") + end) + + it("names the real failure the user would otherwise see", function() + local report = socket_path_report(health_reports(dir_of_length(LINUX_BUDGET + 1))) + expect.contains(report.advice, "unix_listener") + end) + + it("suggests a shorter socket directory", function() + local report = socket_path_report(health_reports(dir_of_length(LINUX_BUDGET + 1))) + expect.contains(report.advice, "connections.socket_dir") + end) + + it("accepts a path exactly at the limit", function() + local report = socket_path_report(health_reports(dir_of_length(LINUX_BUDGET))) + expect.falsy(report.level == "error", "a path that just fits still works") + end) + + it("warns before the limit is reached", function() + local report = socket_path_report(health_reports(dir_of_length(LINUX_BUDGET - 5))) + + expect.eq(report.level, "warn", "a nearly full budget breaks on the next deeper path") + expect.contains(report.message, "close to the limit") + end) + + it("applies the smaller BSD limit on macOS", function() + -- Long enough to overrun the 104 byte BSD sun_path, short enough to still + -- fit the 108 byte Linux one. + local length = MAC_BUDGET + 2 + + expect.falsy(socket_path_report(health_reports(dir_of_length(length))).level == "error", "still fits on Linux") + expect.eq(socket_path_report(health_reports(dir_of_length(length), "mac")).level, "error") + end) + + it("reports the smaller budget on macOS", function() + local report = socket_path_report(health_reports("/home/me/.ssh/sockets", "mac")) + expect.contains(report.message, "of " .. MAC_BUDGET .. " characters") + end) + + it("is reported even when ~/.ssh does not exist", function() + -- The socket directory is configurable and need not live under ~/.ssh. + stub.reload() + + local reports = {} + local function record(level) + return function(message, advice) + table.insert(reports, { level = level, message = message, advice = advice }) + end + end + stub.set("health", { + start = function() end, + ok = record("ok"), + info = record("info"), + warn = record("warn"), + error = record("error"), + }) + stub.set("fn.has", function() + return 0 + end) + local real_isdirectory = vim.fn.isdirectory + stub.set("fn.isdirectory", function(path) + if path == vim.fn.expand("~/.ssh") then return 0 end + return real_isdirectory(path) + end) + + require("sshfs.config").setup({ connections = { socket_dir = dir_of_length(LINUX_BUDGET + 1) } }) + require("sshfs.health").check() + stub.restore_all() + + local report = socket_path_report(reports) + expect.truthy(report, "a missing ~/.ssh must not hide a broken socket path") + expect.eq(report.level, "error") + end) +end) From 82241bf0d55fa6bddef8bc6e3b4158e3288606be Mon Sep 17 00:00:00 2001 From: Robert Hill Date: Thu, 10 Sep 2026 18:57:15 -0400 Subject: [PATCH 6/6] fix(health): derive the socket path budget from named parts The 58 character reserve was a bare magic number mirrored in the spec, so a wrong guess could not be caught. The %C hash half is now checked against real ssh -G output, and the temporary suffix is marked as an upper bound so the budget errs short rather than passing a path that fails at connect time. --- lua/sshfs/health.lua | 6 ++++-- tests/health_socket_path_spec.lua | 15 ++++++++++++++- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/lua/sshfs/health.lua b/lua/sshfs/health.lua index cac8332..6186543 100644 --- a/lua/sshfs/health.lua +++ b/lua/sshfs/health.lua @@ -126,8 +126,10 @@ local function check_socket_path_length() local is_bsd = vim.fn.has("mac") == 1 or vim.fn.has("bsd") == 1 local sun_path_max = is_bsd and 104 or 108 - -- "/" + 40 character %C hash + "." + 16 character temporary suffix. - local reserved = 58 + local control_hash_length = 40 + -- Upper bound: ssh renames a temporary socket into place, so budget errs short rather than long. + local temp_suffix_max = 16 + local reserved = 1 + control_hash_length + 1 + temp_suffix_max local budget = sun_path_max - 1 - reserved local length = #socket_dir diff --git a/tests/health_socket_path_spec.lua b/tests/health_socket_path_spec.lua index 2d5266c..990080b 100644 --- a/tests/health_socket_path_spec.lua +++ b/tests/health_socket_path_spec.lua @@ -52,7 +52,7 @@ local function dir_of_length(length) end -- Linux: 108 byte sun_path, minus the trailing NUL and the 58 characters ssh --- appends ("/" + 40 character %C hash + "." + 16 character suffix) = 49. +-- appends ("/" + 40 character %C hash + "." + 16 character suffix upper bound) = 49. local LINUX_BUDGET = 49 local MAC_BUDGET = 45 @@ -148,3 +148,16 @@ describe("checkhealth socket path length", function() expect.eq(report.level, "error") end) end) + +describe("ssh ControlPath hash", function() + it("expands %C to the length the budget reserves", function() + if vim.fn.executable("ssh") == 0 then return end + + local prefix = "/tmp/s/" + local output = vim.fn.system({ "ssh", "-G", "-o", "ControlPath=" .. prefix .. "%C", "sshfs-nvim-probe.invalid" }) + local path = output:match("controlpath (%S+)") + + expect.truthy(path, "ssh -G must report a resolved controlpath") + expect.eq(#path - #prefix, 40, "the %C hash length LINUX_BUDGET and MAC_BUDGET assume") + end) +end)