fix(tui): wait for embedded server's exit code

Uses the undocumented "error_exit" UI event for a different purpose:
When :detach is used on the server, send an "error_exit" with 0 `status`
to indicate that the server shouldn't wait for client exit.

src/nvim/api/ui.c

@@ -71,12 +71,26 @@ static void remote_ui_destroy(RemoteUI *ui)
   xfree(ui);
 }
 
-void remote_ui_disconnect(uint64_t channel_id)
+/// Removes the client on the given channel from the list of UIs.
+///
+/// @param err  if non-NULL and there is no UI on the channel, set an error
+/// @param send_error_exit  send an "error_exit" event with 0 status first
+void remote_ui_disconnect(uint64_t channel_id, Error *err, bool send_error_exit)
 {
   RemoteUI *ui = pmap_get(uint64_t)(&connected_uis, channel_id);
   if (!ui) {
+    if (err != NULL) {
+      api_set_error(err, kErrorTypeException,
+                    "UI not attached to channel: %" PRId64, channel_id);
+    }
     return;
   }
+  if (send_error_exit) {
+    MAXSIZE_TEMP_ARRAY(args, 1);
+    ADD_C(args, INTEGER_OBJ(0));
+    push_call(ui, "error_exit", args);
+    ui_flush_buf(ui);
+  }
   pmap_del(uint64_t)(&connected_uis, channel_id, NULL);
   ui_detach_impl(ui, channel_id);
   remote_ui_destroy(ui);
@@ -231,12 +245,7 @@ void nvim_ui_set_focus(uint64_t channel_id, Boolean gained, Error *error)
 void nvim_ui_detach(uint64_t channel_id, Error *err)
   FUNC_API_SINCE(1) FUNC_API_REMOTE_ONLY
 {
-  if (!map_has(uint64_t, &connected_uis, channel_id)) {
+  remote_ui_disconnect(channel_id, err, false);
-    api_set_error(err, kErrorTypeException,
-                  "UI not attached to channel: %" PRId64, channel_id);
-    return;
-  }
-  remote_ui_disconnect(channel_id);
 }
 
 // TODO(bfredl): use me to detach a specific ui from the server

src/nvim/api/ui_events.in.h

@@ -173,5 +173,13 @@ void msg_history_show(Array entries)
 void msg_history_clear(void)
   FUNC_API_SINCE(10) FUNC_API_REMOTE_ONLY;
 
+// This UI event is currently undocumented.
+// - When the server needs to intentionally exit with an exit code, and there is no
+//   message in server stderr for the user, this event is sent with positive `status`
+//   argument, to indicate that the UI should exit normally with `status`.
+// - When the server has crashed or there is a message in server stderr for the user,
+//   this event is not sent, and the UI should make server stderr visible.
+// - When :detach is used on the server, this event is sent with a zero `status`
+//   argument, to indicate that the UI shouldn't wait for server exit.
 void error_exit(Integer status)
-  FUNC_API_SINCE(12);
+  FUNC_API_SINCE(12) FUNC_API_CLIENT_IMPL;

src/nvim/event/proc.c

@@ -5,6 +5,7 @@
 #include <uv.h>
 
 #include "klib/kvec.h"
+#include "nvim/channel.h"
 #include "nvim/event/libuv_proc.h"
 #include "nvim/event/loop.h"
 #include "nvim/event/multiqueue.h"
@@ -437,16 +438,20 @@ static void on_proc_exit(Proc *proc)
   Loop *loop = proc->loop;
   ILOG("child exited: pid=%d status=%d" PRIu64, proc->pid, proc->status);
 
-#ifdef MSWIN
+  // TODO(justinmk): figure out why rpc_close sometimes(??) isn't called.
-  // XXX: This assumes the TUI never spawns any other processes...?
-  // TODO(justinmk): figure out why rpc_close sometimes(??) isn't called, then remove this jank.
   // Theories:
   // - EOF not received in receive_msgpack, then doesn't call chan_close_on_err().
   // - proc_close_handles not tickled by ui_client.c's LOOP_PROCESS_EVENTS?
   if (ui_client_channel_id) {
+    uint64_t server_chan_id = ui_client_channel_id;
+    Channel *server_chan = find_channel(server_chan_id);
+    if (server_chan != NULL && server_chan->streamtype == kChannelStreamProc
+        && proc == &server_chan->stream.proc) {
+      // If the current embedded server has exited,
+      // the client should exit with the same status.
       exit_on_closed_chan(proc->status);
     }
-#endif
+  }
 
   // Process has terminated, but there could still be data to be read from the
   // OS. We are still in the libuv loop, so we cannot call code that polls for

src/nvim/ex_docmd.c

@@ -5553,8 +5553,8 @@ static void ex_detach(exarg_T *eap)
   if (eap && eap->forceit) {
     emsg("bang (!) not supported yet");
   } else {
-    // 1. (TODO) Send "detach" UI-event (notification only).
+    // 1. Send "error_exit" UI-event (notification only).
-    // 2. Perform server-side `nvim_ui_detach`.
+    // 2. Perform server-side UI detach.
     // 3. Close server-side channel without self-exit.
 
     if (!current_ui) {
@@ -5571,7 +5571,7 @@ static void ex_detach(exarg_T *eap)
 
     // Server-side UI detach. Doesn't close the channel.
     Error err2 = ERROR_INIT;
-    nvim_ui_detach(chan->id, &err2);
+    remote_ui_disconnect(chan->id, &err2, true);
     if (ERROR_SET(&err2)) {
       emsg(err2.msg);  // UI disappeared already?
       api_clear_error(&err2);

src/nvim/msgpack_rpc/channel.c

@@ -498,18 +498,22 @@ static void rpc_close_event(void **argv)
   if (is_ui_client || channel->streamtype == kChannelStreamStdio) {
     if (!is_ui_client) {
       // Avoid hanging when there are no other UIs and a prompt is triggered on exit.
-      remote_ui_disconnect(channel->id);
+      remote_ui_disconnect(channel->id, NULL, false);
     }
-
     if (!channel->detach) {
-      exit_on_closed_chan(channel->exit_status == -1 ? 0 : channel->exit_status);
+      if (channel->streamtype == kChannelStreamProc && ui_client_error_exit < 0) {
+        // Wait for the embedded server to exit instead of exiting immediately,
+        // as it's necessary to get the server's exit code in on_proc_exit().
+      } else {
+        exit_on_closed_chan(0);
+      }
     }
   }
 }
 
 void rpc_free(Channel *channel)
 {
-  remote_ui_disconnect(channel->id);
+  remote_ui_disconnect(channel->id, NULL, false);
   unpacker_teardown(channel->rpc.unpacker);
   xfree(channel->rpc.unpacker);
 

src/nvim/tui/tui.c

@@ -147,7 +147,6 @@ struct TUIData {
   char *space_buf;
   size_t space_buf_len;
   bool stopped;
-  int seen_error_exit;
   int width;
   int height;
   bool rgb;
@@ -170,7 +169,6 @@ void tui_start(TUIData **tui_p, int *width, int *height, char **term, bool *rgb)
   tui->is_starting = true;
   tui->screenshot = NULL;
   tui->stopped = false;
-  tui->seen_error_exit = 0;
   tui->loop = &main_loop;
   tui->url = -1;
 
@@ -558,14 +556,16 @@ static void terminfo_disable(TUIData *tui)
 /// Disable the alternate screen and prepare for the TUI to close.
 static void terminfo_stop(TUIData *tui)
 {
-  if (ui_client_exit_status == 0) {
+  if (ui_client_exit_status == 0 && ui_client_error_exit > 0) {
-    ui_client_exit_status = tui->seen_error_exit;
+    ui_client_exit_status = ui_client_error_exit;
   }
 
-  // if nvim exited with nonzero status, without indicated this was an
+  // If Nvim exited with nonzero status, without indicating this was an
   // intentional exit (like `:1cquit`), it likely was an internal failure.
-  // Don't clobber the stderr error message in this case.
+  // Don't clobber the stderr error message in this case. #21608
-  if (ui_client_exit_status == tui->seen_error_exit) {
+  if (ui_client_exit_status == MAX(ui_client_error_exit, 0)) {
+    // Position the cursor on the last screen line, below all the text
+    cursor_goto(tui, tui->height - 1, 0);
     // Exit alternate screen.
     unibi_out(tui, unibi_exit_ca_mode);
   }
@@ -604,12 +604,6 @@ static void tui_terminal_after_startup(TUIData *tui)
   flush_buf(tui);
 }
 
-void tui_error_exit(TUIData *tui, Integer status)
-  FUNC_ATTR_NONNULL_ALL
-{
-  tui->seen_error_exit = (int)status;
-}
-
 void tui_stop(TUIData *tui)
   FUNC_ATTR_NONNULL_ALL
 {
@@ -650,8 +644,6 @@ static void tui_terminal_stop(TUIData *tui)
   FUNC_ATTR_NONNULL_ALL
 {
   tinput_stop(&tui->input);
-  // Position the cursor on the last screen line, below all the text
-  cursor_goto(tui, tui->height - 1, 0);
   terminfo_stop(tui);
 }
 

src/nvim/ui_client.c

@@ -284,6 +284,16 @@ void ui_client_event_raw_line(GridLineEvent *g)
                (const schar_T *)grid_line_buf_char, grid_line_buf_attr);
 }
 
+void ui_client_event_error_exit(Array args)
+{
+  if (args.size < 1
+      || args.items[0].type != kObjectTypeInteger) {
+    ELOG("Error handling ui event 'error_exit'");
+    return;
+  }
+  ui_client_error_exit = (int)args.items[0].data.integer;
+}
+
 #ifdef EXITFREE
 void ui_client_free_all_mem(void)
 {

src/nvim/ui_client.h

@@ -17,11 +17,12 @@ EXTERN sattr_T *grid_line_buf_attr INIT( = NULL);
 // Client-side UI channel. Zero during early startup or if not a (--remote-ui) UI client.
 EXTERN uint64_t ui_client_channel_id INIT( = 0);
 
-// exit status from embedded nvim process
+/// `status` argument of the last "error_exit" UI event, or -1 if none has been seen.
-EXTERN int ui_client_exit_status INIT( = 0);
+/// NOTE: This assumes "error_exit" never has a negative `status` argument.
+EXTERN int ui_client_error_exit INIT( = -1);
 
-// TODO(bfredl): the current structure for how tui and ui_client.c communicate is a bit awkward.
+/// Server exit code.
-// This will be restructured as part of The UI Devirtualization Project.
+EXTERN int ui_client_exit_status INIT( = 0);
 
 /// Whether ui client has sent nvim_ui_attach yet
 EXTERN bool ui_client_attached INIT( = false);

test/functional/terminal/tui_spec.lua

@@ -33,6 +33,28 @@ local assert_log = t.assert_log
 
 local testlog = 'Xtest-tui-log'
 
+describe('TUI', function()
+  it('exit status 1 and error message with server --listen error #34365', function()
+    clear()
+    local addr_in_use = api.nvim_get_vvar('servername')
+    local screen = tt.setup_child_nvim(
+      { '--listen', addr_in_use, '-u', 'NONE', '-i', 'NONE' },
+      { extra_rows = 10, cols = 60 }
+    )
+    -- When the address is very long, the error message may be only partly visible.
+    if #addr_in_use <= 600 then
+      screen:expect({
+        any = vim.pesc(
+          ('%s: Failed to --listen: address already in use:'):format(
+            is_os('win') and 'nvim.exe' or 'nvim'
+          )
+        ),
+      })
+    end
+    screen:expect({ any = vim.pesc('[Process exited 1]') })
+  end)
+end)
+
 describe('TUI :detach', function()
   before_each(function()
     os.remove(testlog)
@@ -2119,15 +2141,20 @@ describe('TUI', function()
 
   it('no assert failure on deadly signal #21896', function()
     exec_lua([[vim.uv.kill(vim.fn.jobpid(vim.bo.channel), 'sigterm')]])
-    screen:expect {
+    screen:expect([[
-      grid = [[
       Nvim: Caught deadly signal 'SIGTERM'              |
                                                         |
       [Process exited 1]^                                |
                                                         |*3
       {3:-- TERMINAL --}                                    |
-    ]],
+    ]])
-    }
+  end)
+
+  it('exit status 1 and error message with deadly signal sent to server', function()
+    local _, server_pid = child_session:request('nvim_call_function', 'getpid', {})
+    exec_lua([[vim.uv.kill(..., 'sigterm')]], server_pid)
+    screen:expect({ any = vim.pesc([[Nvim: Caught deadly signal 'SIGTERM']]) })
+    screen:expect({ any = vim.pesc('[Process exited 1]') })
   end)
 
   it('no stack-use-after-scope with cursor color #22432', function()