From efdd73c0962cf46889d2e043efb892cf3ec432b7 Mon Sep 17 00:00:00 2001 From: Lewis Russell Date: Mon, 7 Sep 2026 16:21:52 +0100 Subject: [PATCH] feat(watch): add on_error callback Problem: Watcher failures are reported inconsistently, and callers cannot detect when a watcher is no longer usable. Solution: Route startup and event failures and unexpected inotifywait exits through an optional on_error callback. Log to nvim-watch.log by default. Ignore disappearing child directories and exits caused by cancellation. Let LSP log failures and notify once per message, using INFO for missing roots and ERROR otherwise, without interrupting registration. AI-assisted --- runtime/lua/vim/_core/util.lua | 6 +- runtime/lua/vim/_watch.lua | 95 +++++++++++++++++--------- runtime/lua/vim/lsp/_watchfiles.lua | 15 +++++ test/functional/lua/watch_spec.lua | 101 ++++++++++++++++++++-------- test/functional/plugin/lsp_spec.lua | 45 +++++++++++-- 5 files changed, 196 insertions(+), 66 deletions(-) diff --git a/runtime/lua/vim/_core/util.lua b/runtime/lua/vim/_core/util.lua index 08cbf17f04..133e5a7eeb 100644 --- a/runtime/lua/vim/_core/util.lua +++ b/runtime/lua/vim/_core/util.lua @@ -209,9 +209,11 @@ end --- @param name string Plugin name, e.g. "zip". --- @param msg string --- @param level? integer Level from |vim.log.levels|. Defaults to ERROR. -function M.notify(name, msg, level) +--- @param once? boolean Only show the message once. +function M.notify(name, msg, level, once) vim.schedule(function() - vim.notify(('%s: %s'):format(name, msg), level or vim.log.levels.ERROR) + local notify = once and vim.notify_once or vim.notify + notify(('%s: %s'):format(name, msg), level or vim.log.levels.ERROR) end) end diff --git a/runtime/lua/vim/_watch.lua b/runtime/lua/vim/_watch.lua index 34490437b8..2cd61b36ab 100644 --- a/runtime/lua/vim/_watch.lua +++ b/runtime/lua/vim/_watch.lua @@ -1,4 +1,5 @@ local uv = vim.uv +local log = vim.log.new({ name = 'nvim-watch' }) local M = {} @@ -36,6 +37,9 @@ end --- --- @field debounce? integer ms --- +--- Handles watcher failures. Defaults to logging at ERROR in nvim-watch.log. +--- @field on_error? fun(err: string) +--- --- An |lpeg| pattern. Only changes to files whose full paths match the pattern --- will be reported. Only matches against non-directoriess, all directories will --- be watched for new potentially-matching files. exclude_pattern can be used to @@ -87,6 +91,7 @@ function M.watch(path, opts, callback) vim.validate('callback', callback, 'function') opts = opts or {} + local on_error = opts.on_error or log.error path = vim.fs.normalize(path) local uvflags = opts and opts.uvflags or {} @@ -94,8 +99,10 @@ function M.watch(path, opts, callback) local watching_dir = (uv.fs_stat(path) or {}).type == 'directory' - local _, start_err, start_errname = handle:start(path, uvflags, function(err, filename, events) - assert(not err, err) + local _, start_err = handle:start(path, uvflags, function(err, filename, events) + if err then + return on_error(err) + end local fullpath = path if filename and watching_dir then fullpath = vim.fs.normalize(vim.fs.joinpath(fullpath, filename)) @@ -112,7 +119,9 @@ function M.watch(path, opts, callback) if staterrname == 'ENOENT' then change_type = M.FileChangeType.Deleted else - assert(not staterr, staterr) + if staterr then + return on_error(staterr) + end change_type = M.FileChangeType.Created end elseif events.change then @@ -122,13 +131,8 @@ function M.watch(path, opts, callback) end) if start_err then - if start_errname == 'ENOENT' then - -- Server may send "workspace/didChangeWatchedFiles" with nonexistent `baseUri` path. - -- This is mostly a placeholder until we have `nvim_log` API. - vim.notify_once(('watch.watch: %s'):format(start_err), vim.log.levels.INFO) - end handle:close() - -- TODO(justinmk): log important errors once we have `nvim_log` API. + on_error(start_err) return function() end end @@ -156,6 +160,7 @@ function M.watchdirs(path, opts, callback) vim.validate('callback', callback, 'function') opts = opts or {} + local on_error = opts.on_error or log.error local debounce = opts.debounce or 500 ---@type table handle by fullpath @@ -174,7 +179,9 @@ function M.watchdirs(path, opts, callback) --- @return uv.fs_event_start.callback local function create_on_change(filepath) return function(err, filename, events) - assert(not err, err) + if err then + return on_error(err) + end local fullpath = vim.fs.joinpath(filepath, filename) if skip(fullpath, opts) then return @@ -204,7 +211,15 @@ function M.watchdirs(path, opts, callback) if not handle then handle = assert(uv.new_fs_event()) handles[fullpath] = handle - handle:start(fullpath, {}, create_on_change(fullpath)) + local _, err, errname = handle:start(fullpath, {}, create_on_change(fullpath)) + if err then + handle:close() + handles[fullpath] = nil + -- The directory may have disappeared since fs_stat(). + if errname ~= 'ENOENT' then + on_error(err) + end + end end end else @@ -225,17 +240,13 @@ function M.watchdirs(path, opts, callback) local root_handle = assert(uv.new_fs_event()) handles[path] = root_handle - local _, start_err, start_errname = root_handle:start(path, {}, create_on_change(path)) + local _, start_err = root_handle:start(path, {}, create_on_change(path)) if start_err then - if start_errname == 'ENOENT' then - -- Server may send "workspace/didChangeWatchedFiles" with nonexistent `baseUri` path. - -- This is mostly a placeholder until we have `nvim_log` API. - vim.notify_once(('watch.watchdirs: %s'):format(start_err), vim.log.levels.INFO) - end - -- TODO(justinmk): log important errors once we have `nvim_log` API. - - -- Continue. vim.fs.dir() will return nothing, so the code below is harmless. + root_handle:close() + timer:close() + on_error(start_err) + return function() end end --- "640K ought to be enough for anyone" @@ -256,7 +267,15 @@ function M.watchdirs(path, opts, callback) if not skip(filepath, opts) then local handle = assert(uv.new_fs_event()) handles[filepath] = handle - handle:start(filepath, {}, create_on_change(filepath)) + local _, err, errname = handle:start(filepath, {}, create_on_change(filepath)) + if err then + handle:close() + handles[filepath] = nil + -- The directory may have disappeared since it was listed. + if errname ~= 'ENOENT' then + on_error(err) + end + end end end end @@ -314,7 +333,10 @@ end --- @param callback vim._watch.Callback Callback for new events --- @return fun() cancel Stops the watcher function M.inotify(path, opts, callback) - local obj = vim.system({ + opts = opts or {} + local on_error = opts.on_error or log.error + local cancelled = false + local ok, obj = pcall(vim.system, { 'inotifywait', '--quiet', -- suppress startup messages '--no-dereference', -- don't follow symlinks @@ -333,22 +355,21 @@ function M.inotify(path, opts, callback) }, { stderr = function(err, data) if err then - error(err) + on_error(err) + return end if data and #vim.trim(data) > 0 then - vim.schedule(function() - if vim.fn.has('linux') == 1 and vim.startswith(data, 'Failed to watch') then - data = 'inotify(7) limit reached, see :h inotify-limitations for more info.' - end - - vim.notify('inotify: ' .. data, vim.log.levels.ERROR) - end) + if vim.fn.has('linux') == 1 and vim.startswith(data, 'Failed to watch') then + data = 'inotify(7) limit reached, see :h inotify-limitations for more info.' + end + on_error(data) end end, stdout = function(err, data) if err then - error(err) + on_error(err) + return end for line in vim.gsplit(data or '', '\n', { plain = true, trimempty = true }) do @@ -357,9 +378,19 @@ function M.inotify(path, opts, callback) end, -- --latency is locale dependent but tostring() isn't and will always have '.' as decimal point. env = { LC_NUMERIC = 'C' }, - }) + }, function(result) + if not cancelled then + on_error(('inotifywait exited with code %d'):format(result.code)) + end + end) + + if not ok then + on_error(obj) + return function() end + end return tracked_cancel('inotify', function() + cancelled = true obj:kill(2) end) end diff --git a/runtime/lua/vim/lsp/_watchfiles.lua b/runtime/lua/vim/lsp/_watchfiles.lua index 466a31d542..6e3fc825f4 100644 --- a/runtime/lua/vim/lsp/_watchfiles.lua +++ b/runtime/lua/vim/lsp/_watchfiles.lua @@ -2,6 +2,7 @@ local bit = require('bit') local glob = vim.glob local watch = vim._watch local log = require('vim.lsp.log') +local notify = require('vim._core.util').notify local protocol = require('vim.lsp.protocol') local lpeg = vim.lpeg @@ -185,6 +186,20 @@ function M.register(reg, client_id) -- match a *particular* pattern+kind pair. include_pattern = include_pattern, exclude_pattern = M._poll_exclude_pattern, + on_error = function(err) + local name = string.format('LSP[%s]', client.name) + local message = string.format('file watcher failed for %s', base_dir) + local level = vim.log.levels.ERROR + -- Servers may register a nonexistent baseUri. Keep this informational + -- and continue registering the other watchers. + if err:match('^ENOENT:') then + level = vim.log.levels.INFO + log.info(name, message, err) + else + log.error(name, message, err) + end + notify(name, message .. ': ' .. err, level, true) + end, }, callback(base_dir)) ) end diff --git a/test/functional/lua/watch_spec.lua b/test/functional/lua/watch_spec.lua index c3a5143db7..57797e5b3f 100644 --- a/test/functional/lua/watch_spec.lua +++ b/test/functional/lua/watch_spec.lua @@ -52,24 +52,49 @@ describe('vim._watch', function() end end) - local function run(watchfunc) - -- Monkey-patches vim.notify_once so we can "spy" on it. - local function spy_notify_once() - exec_lua [[ - _G.__notify_once_msgs = {} - vim.notify_once = (function(overridden) - return function(msg, level, opts) - table.insert(_G.__notify_once_msgs, msg) - return overridden(msg, level, opts) + it('watchdirs() tolerates directories deleted during setup', function() + local root_dir = t.tmpname(false) + t.finally(function() + n.rmdir(root_dir) + end) + n.mkdir_p(root_dir .. '/gone') + + exec_lua(function(root) + local dir = vim.fs.dir + vim.fs.dir = function(path, opts) + local iter = dir(path, opts) + return function() + local name, kind = iter() + if name == 'gone' then + -- Delete after enumeration, before the backend starts watching it. + assert(vim.uv.fs_rmdir(root .. '/gone')) end - end)(vim.notify_once) - ]] - end + return name, kind + end + end + local cancel = vim._watch.watchdirs(root, { on_error = error }, function() end) + vim.fs.dir = dir + cancel() + end, root_dir) + end) - local function last_notify_once_msg() - return exec_lua 'return _G.__notify_once_msgs[#_G.__notify_once_msgs]' - end + it('inotify() reports failure to start the process', function() + exec_lua(function() + vim.env.PATH = '' + local errors = {} + local cancel = vim._watch.inotify('.', { + on_error = function(err) + errors[#errors + 1] = err + end, + }, function() end) + cancel() + assert(#errors == 1, vim.inspect(errors)) + assert(errors[1]:find('ENOENT', 1, true), errors[1]) + assert(vim._watch.active.inotify == 0) + end) + end) + local function run(watchfunc) local function do_watch(root_dir, watchfunc_) exec_lua( [[ @@ -90,28 +115,48 @@ describe('vim._watch', function() ) end - it(watchfunc .. '() ignores nonexistent paths', function() + it(watchfunc .. '() reports nonexistent paths to on_error', function() if watchfunc == 'inotify' then skip(n.fn.executable('inotifywait') == 0, 'inotifywait not found') skip(is_os('bsd'), 'inotifywait on bsd CI seems to expect path to exist?') skip(t.is_arch('s390x'), 'inotifywait not available on s390x CI') end - local msg = ('watch.%s: ENOENT: no such file or directory'):format(watchfunc) - - spy_notify_once() - do_watch('/i am /very/funny.go', watchfunc) - - if watchfunc ~= 'inotify' then -- watch.inotify() doesn't (currently) call vim.notify_once. - t.retry(nil, 2000, function() - t.eq(msg, last_notify_once_msg()) + exec_lua(function(backend) + local errors = {} + local cancel = vim._watch[backend]('/i am /very/funny.go', { + on_error = function(err) + errors[#errors + 1] = err + end, + }, function() + error('Unexpected file change') end) - end - eq(0, exec_lua [[return #_G.events]]) - - exec_lua [[_G.stop_watch()]] + assert(vim.wait(2000, function() + return #errors > 0 + end)) + if backend ~= 'inotify' then + assert(errors[1]:match('^ENOENT:'), errors[1]) + end + cancel() + end, watchfunc) end) + if watchfunc ~= 'inotify' then + it(watchfunc .. '() logs startup failures without on_error', function() + local logfile = exec_lua(function(backend) + local logfile = vim.fs.joinpath(vim.fn.stdpath('log'), 'nvim-watch.log') + vim.fn.writefile({}, logfile) + local cancel = vim._watch[backend]('/i am /very/funny.go', {}, function() + error('Unexpected file change') + end) + cancel() + assert(vim._watch.active[backend] == 0) + return logfile + end, watchfunc) + t.assert_log('%[ERROR%].-ENOENT:', logfile) + end) + end + it(watchfunc .. '() detects file changes', function() if watchfunc == 'inotify' then skip(is_os('win'), 'N/A: inotify not supported on Windows') diff --git a/test/functional/plugin/lsp_spec.lua b/test/functional/plugin/lsp_spec.lua index b9cb675a91..cc50141ff2 100644 --- a/test/functional/plugin/lsp_spec.lua +++ b/test/functional/plugin/lsp_spec.lua @@ -4084,9 +4084,13 @@ describe('LSP', function() deleted = exec_lua([[return vim.lsp.protocol.FileChangeType.Deleted]]) end) - local function test_filechanges(watchfunc) + local function test_filechanges(watchfunc, missing_root) it( - string.format('sends notifications when files change (watchfunc=%s)', watchfunc), + string.format( + 'sends notifications when files change (watchfunc=%s)%s', + watchfunc, + missing_root and ' after root is created' or '' + ), function() if watchfunc == 'inotify' then skip(is_os('win'), 'not supported on windows') @@ -4112,12 +4116,19 @@ describe('LSP', function() end local root_dir = tmpname(false) - mkdir(root_dir) + if not missing_root then + mkdir(root_dir) + end exec_lua(create_server_definition) local result = exec_lua(function() local logfile = vim.lsp.log.get_filename() + vim.lsp.log.set_level('info') vim.fn.writefile({ '' }, logfile) + local notifications = {} + vim.notify = function(message, level) + notifications[#notifications + 1] = { message, level } + end local server = _G._create_server() local client_id = assert(vim.lsp.start({ name = 'watchfiles-test', @@ -4179,6 +4190,19 @@ describe('LSP', function() }, }, { client_id = client_id }) + if missing_root then + local client = assert(vim.lsp.get_client_by_id(client_id)) + local method = 'workspace/didChangeWatchedFiles' + local reg = vim.deepcopy(client.registrations[method][1]) + reg.id = 'watchfiles-test-missing' + client:_register({ reg }) + client:_unregister({ { id = reg.id, method = method } }) + vim.fn.mkdir(root_dir) + reg.id = 'watchfiles-test-1' + client:_register({ reg }) + client:_unregister({ { id = 'watchfiles-test-0', method = method } }) + end + if watchfunc ~= 'watch' then vim.wait(100) end @@ -4196,7 +4220,7 @@ describe('LSP', function() vim.lsp.get_client_by_id(client_id):stop() - return { logfile = logfile, messages = server.messages } + return { logfile = logfile, messages = server.messages, notifications = notifications } end) local uri = vim.uri_from_fname(root_dir .. '/watch') @@ -4238,6 +4262,18 @@ describe('LSP', function() .. pesc('{foo}'), result.logfile ) + if missing_root then + t.assert_log( + '%[INFO%].-file watcher failed for ' .. pesc(root_dir) .. '.-ENOENT', + result.logfile + ) + eq(1, #result.notifications) + eq(vim.log.levels.INFO, result.notifications[1][2]) + matches( + 'file watcher failed for ' .. pesc(root_dir) .. ': ENOENT', + result.notifications[1][1] + ) + end end ) end @@ -4245,6 +4281,7 @@ describe('LSP', function() test_filechanges('watch') test_filechanges('watchdirs') test_filechanges('inotify') + test_filechanges('watchdirs', true) it('correctly registers and unregisters', function() local root_dir = '/some_dir'