Merge pull request #25006 from lewis6991/fix/systemkill

`vim.system` fixes and improvements

6 files changed, 203 insertions, 118 deletions

diff --git a/runtime/doc/lua.txt b/runtime/doc/lua.txt
index f5b3e56f61..71f001f4fc 100644
--- a/runtime/doc/lua.txt
+++ b/runtime/doc/lua.txt
@@ -1777,7 +1777,9 @@ vim.system({cmd}, {opts}, {on_exit})                            *vim.system()*
                      `fun(err: string, data: string)`. Defaults to `true`.
                    • text: (boolean) Handle stdout and stderr as text.
                      Replaces `\r\n` with `\n`.
-                   • timeout: (integer)
+                   • timeout: (integer) Run the command with a time limit.
+                     Upon timeout the process is sent the TERM signal (15) and
+                     the exit code is set to 124.
                    • detach: (boolean) If true, spawn the child process in a
                      detached state - this will make it a process group
                      leader, and will effectively enable the child to keep
@@ -1790,16 +1792,18 @@ vim.system({cmd}, {opts}, {on_exit})                            *vim.system()*
                    object, see return of SystemObj:wait().
 
     Return: ~
-        SystemObj Object with the fields:
+        vim.SystemObj Object with the fields:
         • pid (integer) Process ID
-        • wait (fun(timeout: integer|nil): SystemCompleted)
+        • wait (fun(timeout: integer|nil): SystemCompleted) Wait for the
+          process to complete. Upon timeout the process is sent the KILL
+          signal (9) and the exit code is set to 124.
           • SystemCompleted is an object with the fields:
             • code: (integer)
             • signal: (integer)
             • stdout: (string), nil if stdout argument is passed
             • stderr: (string), nil if stderr argument is passed
 
-        • kill (fun(signal: integer))
+        • kill (fun(signal: integer|string))
         • write (fun(data: string|nil)) Requires `stdin=true`. Pass `nil` to
           close the stream.
         • is_closing (fun(): boolean)
@@ -2541,7 +2545,7 @@ vim.ui.open({path})                                            *vim.ui.open()*
       • {path}  (string) Path or URL to open
 
     Return (multiple): ~
-        SystemCompleted|nil Command result, or nil if not found.
+        vim.SystemCompleted|nil Command result, or nil if not found.
         (string|nil) Error message on failure
 
     See also: ~
diff --git a/runtime/lua/vim/_editor.lua b/runtime/lua/vim/_editor.lua
index 96ac379368..68992a16bb 100644
--- a/runtime/lua/vim/_editor.lua
+++ b/runtime/lua/vim/_editor.lua
@@ -107,7 +107,8 @@ vim.log = {
 ---     Handle output from stdout. When passed as a function must have the signature `fun(err: string, data: string)`.
 ---     Defaults to `true`.
 ---   - text: (boolean) Handle stdout and stderr as text. Replaces `\r\n` with `\n`.
----   - timeout: (integer)
+---   - timeout: (integer) Run the command with a time limit. Upon timeout the process is sent the
+---     TERM signal (15) and the exit code is set to 124.
 ---   - detach: (boolean) If true, spawn the child process in a detached state - this will make it
 ---     a process group leader, and will effectively enable the child to keep running after the
 ---     parent exits. Note that the child process will still keep the parent's event loop alive
@@ -116,15 +117,16 @@ vim.log = {
 --- @param on_exit (function|nil) Called when subprocess exits. When provided, the command runs
 ---   asynchronously. Receives SystemCompleted object, see return of SystemObj:wait().
 ---
---- @return SystemObj Object with the fields:
+--- @return vim.SystemObj Object with the fields:
 ---   - pid (integer) Process ID
----   - wait (fun(timeout: integer|nil): SystemCompleted)
+---   - wait (fun(timeout: integer|nil): SystemCompleted) Wait for the process to complete. Upon
+---     timeout the process is sent the KILL signal (9) and the exit code is set to 124.
 ---     - SystemCompleted is an object with the fields:
 ---      - code: (integer)
 ---      - signal: (integer)
 ---      - stdout: (string), nil if stdout argument is passed
 ---      - stderr: (string), nil if stderr argument is passed
----   - kill (fun(signal: integer))
+---   - kill (fun(signal: integer|string))
 ---   - write (fun(data: string|nil)) Requires `stdin=true`. Pass `nil` to close the stream.
 ---   - is_closing (fun(): boolean)
 function vim.system(cmd, opts, on_exit)
diff --git a/runtime/lua/vim/_system.lua b/runtime/lua/vim/_system.lua
index 6f5e95eb24..9279febddf 100644
--- a/runtime/lua/vim/_system.lua
+++ b/runtime/lua/vim/_system.lua
@@ -2,8 +2,8 @@ local uv = vim.uv
 
 --- @class SystemOpts
 --- @field stdin? string|string[]|true
---- @field stdout? fun(err:string, data: string)|false
---- @field stderr? fun(err:string, data: string)|false
+--- @field stdout? fun(err:string?, data: string?)|false
+--- @field stderr? fun(err:string?, data: string?)|false
 --- @field cwd? string
 --- @field env? table<string,string|number>
 --- @field clear_env? boolean
@@ -11,51 +11,61 @@ local uv = vim.uv
 --- @field timeout? integer Timeout in ms
 --- @field detach? boolean
 
---- @class SystemCompleted
+--- @class vim.SystemCompleted
 --- @field code integer
 --- @field signal integer
 --- @field stdout? string
 --- @field stderr? string
 
---- @class SystemState
+--- @class vim.SystemState
 --- @field handle? uv.uv_process_t
 --- @field timer?  uv.uv_timer_t
 --- @field pid? integer
 --- @field timeout? integer
---- @field done? boolean
+--- @field done? boolean|'timeout'
 --- @field stdin? uv.uv_stream_t
 --- @field stdout? uv.uv_stream_t
 --- @field stderr? uv.uv_stream_t
---- @field result? SystemCompleted
-
----@param state SystemState
-local function close_handles(state)
-  for _, handle in pairs({ state.handle, state.stdin, state.stdout, state.stderr }) do
-    if not handle:is_closing() then
-      handle:close()
-    end
+--- @field stdout_data? string[]
+--- @field stderr_data? string[]
+--- @field result? vim.SystemCompleted
+
+--- @enum vim.SystemSig
+local SIG = {
+  HUP = 1, -- Hangup
+  INT = 2, -- Interrupt from keyboard
+  KILL = 9, -- Kill signal
+  TERM = 15, -- Termination signal
+  -- STOP = 17,19,23  -- Stop the process
+}
+
+---@param handle uv.uv_handle_t?
+local function close_handle(handle)
+  if handle and not handle:is_closing() then
+    handle:close()
   end
 end
 
---- @param cmd string[]
---- @return SystemCompleted
-local function timeout_result(cmd)
-  local cmd_str = table.concat(cmd, ' ')
-  local err = string.format("Command timed out: '%s'", cmd_str)
-  return { code = 0, signal = 2, stdout = '', stderr = err }
+---@param state vim.SystemState
+local function close_handles(state)
+  close_handle(state.handle)
+  close_handle(state.stdin)
+  close_handle(state.stdout)
+  close_handle(state.stderr)
+  close_handle(state.timer)
 end
 
---- @class SystemObj
+--- @class vim.SystemObj
 --- @field pid integer
---- @field private _state SystemState
---- @field wait fun(self: SystemObj, timeout?: integer): SystemCompleted
---- @field kill fun(self: SystemObj, signal: integer)
---- @field write fun(self: SystemObj, data?: string|string[])
---- @field is_closing fun(self: SystemObj): boolean?
+--- @field private _state vim.SystemState
+--- @field wait fun(self: vim.SystemObj, timeout?: integer): vim.SystemCompleted
+--- @field kill fun(self: vim.SystemObj, signal: integer|string)
+--- @field write fun(self: vim.SystemObj, data?: string|string[])
+--- @field is_closing fun(self: vim.SystemObj): boolean?
 local SystemObj = {}
 
---- @param state SystemState
---- @return SystemObj
+--- @param state vim.SystemState
+--- @return vim.SystemObj
 local function new_systemobj(state)
   return setmetatable({
     pid = state.pid,
@@ -63,27 +73,35 @@ local function new_systemobj(state)
   }, { __index = SystemObj })
 end
 
---- @param signal integer
+--- @param signal integer|string
 function SystemObj:kill(signal)
-  local state = self._state
-  state.handle:kill(signal)
-  close_handles(state)
+  self._state.handle:kill(signal)
+end
+
+--- @package
+--- @param signal? vim.SystemSig
+function SystemObj:_timeout(signal)
+  self._state.done = 'timeout'
+  self:kill(signal or SIG.TERM)
 end
 
 local MAX_TIMEOUT = 2 ^ 31
 
 --- @param timeout? integer
---- @return SystemCompleted
+--- @return vim.SystemCompleted
 function SystemObj:wait(timeout)
   local state = self._state
 
-  vim.wait(timeout or state.timeout or MAX_TIMEOUT, function()
-    return state.done
+  local done = vim.wait(timeout or state.timeout or MAX_TIMEOUT, function()
+    return state.result ~= nil
   end)
 
-  if not state.done then
-    self:kill(6) -- 'sigint'
-    state.result = timeout_result(state.cmd)
+  if not done then
+    -- Send sigkill since this cannot be caught
+    self:_timeout(SIG.KILL)
+    vim.wait(timeout or state.timeout or MAX_TIMEOUT, function()
+      return state.result ~= nil
+    end)
   end
 
   return state.result
@@ -125,9 +143,9 @@ function SystemObj:is_closing()
   return handle == nil or handle:is_closing()
 end
 
----@param output function|'false'
+---@param output fun(err:string?, data: string?)|false
 ---@return uv.uv_stream_t?
----@return function? Handler
+---@return fun(err:string?, data: string?)? Handler
 local function setup_output(output)
   if output == nil then
     return assert(uv.new_pipe(false)), nil
@@ -159,7 +177,7 @@ end
 
 --- @return table<string,string>
 local function base_env()
-  local env = vim.fn.environ()
+  local env = vim.fn.environ() --- @type table<string,string>
   env['NVIM'] = vim.v.servername
   env['NVIM_LISTEN_ADDRESS'] = nil
   return env
@@ -212,7 +230,7 @@ end
 local M = {}
 
 --- @param cmd string
---- @param opts uv.aliases.spawn_options
+--- @param opts uv.spawn.options
 --- @param on_exit fun(code: integer, signal: integer)
 --- @param on_error fun()
 --- @return uv.uv_process_t, integer
@@ -225,12 +243,68 @@ local function spawn(cmd, opts, on_exit, on_error)
   return handle, pid_or_err --[[@as integer]]
 end
 
+---@param timeout integer
+---@param cb fun()
+---@return uv.uv_timer_t
+local function timer_oneshot(timeout, cb)
+  local timer = assert(uv.new_timer())
+  timer:start(timeout, 0, function()
+    timer:stop()
+    timer:close()
+    cb()
+  end)
+  return timer
+end
+
+--- @param state vim.SystemState
+--- @param code integer
+--- @param signal integer
+--- @param on_exit fun(result: vim.SystemCompleted)?
+local function _on_exit(state, code, signal, on_exit)
+  close_handles(state)
+
+  local check = assert(uv.new_check())
+  check:start(function()
+    for _, pipe in pairs({ state.stdin, state.stdout, state.stderr }) do
+      if not pipe:is_closing() then
+        return
+      end
+    end
+    check:stop()
+    check:close()
+
+    if state.done == nil then
+      state.done = true
+    end
+
+    if (code == 0 or code == 1) and state.done == 'timeout' then
+      -- Unix: code == 0
+      -- Windows: code == 1
+      code = 124
+    end
+
+    local stdout_data = state.stdout_data
+    local stderr_data = state.stderr_data
+
+    state.result = {
+      code = code,
+      signal = signal,
+      stdout = stdout_data and table.concat(stdout_data) or nil,
+      stderr = stderr_data and table.concat(stderr_data) or nil,
+    }
+
+    if on_exit then
+      on_exit(state.result)
+    end
+  end)
+end
+
 --- Run a system command
 ---
 --- @param cmd string[]
 --- @param opts? SystemOpts
---- @param on_exit? fun(out: SystemCompleted)
---- @return SystemObj
+--- @param on_exit? fun(out: vim.SystemCompleted)
+--- @return vim.SystemObj
 function M.run(cmd, opts, on_exit)
   vim.validate({
     cmd = { cmd, 'table' },
@@ -244,7 +318,7 @@ function M.run(cmd, opts, on_exit)
   local stderr, stderr_handler = setup_output(opts.stderr)
   local stdin, towrite = setup_input(opts.stdin)
 
-  --- @type SystemState
+  --- @type vim.SystemState
   local state = {
     done = false,
     cmd = cmd,
@@ -254,60 +328,29 @@ function M.run(cmd, opts, on_exit)
     stderr = stderr,
   }
 
-  -- Define data buckets as tables and concatenate the elements at the end as
-  -- one operation.
-  --- @type string[], string[]
-  local stdout_data, stderr_data
-
+  --- @diagnostic disable-next-line:missing-fields
   state.handle, state.pid = spawn(cmd[1], {
     args = vim.list_slice(cmd, 2),
     stdio = { stdin, stdout, stderr },
     cwd = opts.cwd,
+    --- @diagnostic disable-next-line:assign-type-mismatch
     env = setup_env(opts.env, opts.clear_env),
     detached = opts.detach,
     hide = true,
   }, function(code, signal)
-    close_handles(state)
-    if state.timer then
-      state.timer:stop()
-      state.timer:close()
-    end
-
-    local check = assert(uv.new_check())
-
-    check:start(function()
-      for _, pipe in pairs({ state.stdin, state.stdout, state.stderr }) do
-        if not pipe:is_closing() then
-          return
-        end
-      end
-      check:stop()
-      check:close()
-
-      state.done = true
-      state.result = {
-        code = code,
-        signal = signal,
-        stdout = stdout_data and table.concat(stdout_data) or nil,
-        stderr = stderr_data and table.concat(stderr_data) or nil,
-      }
-
-      if on_exit then
-        on_exit(state.result)
-      end
-    end)
+    _on_exit(state, code, signal, on_exit)
   end, function()
     close_handles(state)
   end)
 
   if stdout then
-    stdout_data = {}
-    stdout:read_start(stdout_handler or default_handler(stdout, opts.text, stdout_data))
+    state.stdout_data = {}
+    stdout:read_start(stdout_handler or default_handler(stdout, opts.text, state.stdout_data))
   end
 
   if stderr then
-    stderr_data = {}
-    stderr:read_start(stderr_handler or default_handler(stderr, opts.text, stderr_data))
+    state.stderr_data = {}
+    stderr:read_start(stderr_handler or default_handler(stderr, opts.text, state.stderr_data))
   end
 
   local obj = new_systemobj(state)
@@ -318,16 +361,9 @@ function M.run(cmd, opts, on_exit)
   end
 
   if opts.timeout then
-    state.timer = assert(uv.new_timer())
-    state.timer:start(opts.timeout, 0, function()
-      state.timer:stop()
-      state.timer:close()
+    state.timer = timer_oneshot(opts.timeout, function()
       if state.handle and state.handle:is_active() then
-        obj:kill(6) --- 'sigint'
-        state.result = timeout_result(state.cmd)
-        if on_exit then
-          on_exit(state.result)
-        end
+        obj:_timeout()
       end
     end)
   end
diff --git a/runtime/lua/vim/lsp/rpc.lua b/runtime/lua/vim/lsp/rpc.lua
index a77af937b3..6ab5708721 100644
--- a/runtime/lua/vim/lsp/rpc.lua
+++ b/runtime/lua/vim/lsp/rpc.lua
@@ -648,7 +648,7 @@ function M.start(cmd, cmd_args, dispatchers, extra_spawn_params)
 
   dispatchers = merge_dispatchers(dispatchers)
 
-  local sysobj ---@type SystemObj
+  local sysobj ---@type vim.SystemObj
 
   local client = new_client(dispatchers, {
     write = function(msg)
@@ -708,7 +708,7 @@ function M.start(cmd, cmd_args, dispatchers, extra_spawn_params)
     return
   end
 
-  sysobj = sysobj_or_err --[[@as SystemObj]]
+  sysobj = sysobj_or_err --[[@as vim.SystemObj]]
 
   return public_client(client)
 end
diff --git a/runtime/lua/vim/ui.lua b/runtime/lua/vim/ui.lua
index 87b52787a0..cd90886489 100644
--- a/runtime/lua/vim/ui.lua
+++ b/runtime/lua/vim/ui.lua
@@ -118,7 +118,7 @@ end
 ---
 ---@param path string Path or URL to open
 ---
----@return SystemCompleted|nil # Command result, or nil if not found.
+---@return vim.SystemCompleted|nil # Command result, or nil if not found.
 ---@return string|nil # Error message on failure
 ---
 ---@see |vim.system()|
diff --git a/test/functional/lua/system_spec.lua b/test/functional/lua/system_spec.lua
index 836d3a83b0..a988d3f0d7 100644
--- a/test/functional/lua/system_spec.lua
+++ b/test/functional/lua/system_spec.lua
@@ -5,27 +5,46 @@ local eq = helpers.eq
 
 local function system_sync(cmd, opts)
   return exec_lua([[
-    return vim.system(...):wait()
+    local cmd, opts = ...
+    local obj = vim.system(...)
+
+    if opts.timeout then
+      -- Minor delay before calling wait() so the timeout uv timer can have a headstart over the
+      -- internal call to vim.wait() in wait().
+      vim.wait(10)
+    end
+
+    local res = obj:wait()
+
+    -- Check the process is no longer running
+    local proc = vim.api.nvim_get_proc(obj.pid)
+    assert(not proc, 'process still exists')
+
+    return res
   ]], cmd, opts)
 end
 
 local function system_async(cmd, opts)
-  exec_lua([[
+  return exec_lua([[
     local cmd, opts = ...
     _G.done = false
-    vim.system(cmd, opts, function(obj)
+    local obj = vim.system(cmd, opts, function(obj)
       _G.done = true
       _G.ret = obj
     end)
-  ]], cmd, opts)
 
-  while true do
-    if exec_lua[[return _G.done]] then
-      break
-    end
-  end
+    local ok = vim.wait(10000, function()
+      return _G.done
+    end)
 
-  return exec_lua[[return _G.ret]]
+    assert(ok, 'process did not exit')
+
+    -- Check the process is no longer running
+    local proc = vim.api.nvim_get_proc(obj.pid)
+    assert(not proc, 'process still exists')
+
+    return _G.ret
+  ]], cmd, opts)
 end
 
 describe('vim.system', function()
@@ -43,15 +62,39 @@ describe('vim.system', function()
         eq('hellocat', system({ 'cat' }, { stdin = 'hellocat', text = true }).stdout)
       end)
 
-      it ('supports timeout', function()
+      it('supports timeout', function()
         eq({
-          code = 0,
-          signal = 2,
+          code = 124,
+          signal = 15,
           stdout = '',
-          stderr = "Command timed out: 'sleep 10'"
-        }, system({ 'sleep', '10' }, { timeout = 1 }))
+          stderr = ''
+        }, system({ 'sleep', '10' }, { timeout = 1000 }))
       end)
     end)
   end
 
+  it('kill processes', function()
+    exec_lua([[
+      local signal
+      local cmd = vim.system({ 'cat', '-' }, { stdin = true }, function(r)
+        signal = r.signal
+      end) -- run forever
+
+      cmd:kill('sigint')
+
+      -- wait for the process not to exist
+      local done = vim.wait(2000, function()
+        return signal ~= nil
+      end)
+
+      assert(done, 'process did not exit')
+
+      -- Check the process is no longer running
+      local proc = vim.api.nvim_get_proc(cmd.pid)
+      assert(not proc, 'process still exists')
+
+      assert(signal == 2)
+    ]])
+  end)
+
 end)