fix: kill Go server when Neovim terminates

README.md

@@ -178,6 +178,7 @@ These keymaps are available globally (i.e., in any buffer).
 | `glC`     | Create a new MR for currently checked-out feature branch                                          |
 | `glc`     | Chose MR for review                                                                               |
 | `glS`     | Start review for the currently checked-out branch                                                 |
+| `glQ`     | Close the reviewer, discussions, and shut down the Go server                                      |
 | `gl<C-R>` | Load new MR state from Gitlab and apply new diff refs to the diff view                            |
 | `gls`     | Show the editable summary of the MR                                                               |
 | `glu`     | Copy the URL of the MR to the system clipboard                                                    |

cmd/app/shutdown.go

@@ -4,10 +4,18 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"io"
 	"net/http"
 	"os"
+	"time"
 )
 
+/*
+How long to wait for in-flight requests to finish before shutting down anyway.
+Without a deadline a slow Gitlab call keeps the process alive after Neovim quits.
+*/
+const shutdownTimeout = 10 * time.Second
+
 type killer struct{}
 
 func (k killer) Signal() {}
@@ -22,18 +30,71 @@ type ShutdownHandler interface {
 
 type shutdownService struct {
 	sigCh chan os.Signal
+	/* How long to wait for in-flight requests. Zero means shutdownTimeout; tests
+	shrink it so they do not have to wait out the real deadline. */
+	timeout time.Duration
 }
 
 func (s shutdownService) WatchForShutdown(server *http.Server) {
 	/* Handles shutdown requests */
 	<-s.sigCh
-	err := server.Shutdown(context.Background())
-	if err != nil {
+
+	timeout := s.timeout
+	if timeout == 0 {
+		timeout = shutdownTimeout
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), timeout)
+	defer cancel()
+
+	/* Not fatal: exiting 0 keeps the Lua side from reporting the shutdown as a
+	   crash. */
+	if err := server.Shutdown(ctx); err != nil {
 		fmt.Fprintf(os.Stderr, "Server could not shut down gracefully: %s\n", err)
-		os.Exit(1)
 	}
 }
 
+/*
+WatchForParentExit exits the process once stdin reaches EOF, which happens when
+Neovim closes its end of the pipe, i.e. when it dies. This is the only cleanup
+that survives a SIGKILL or a crash, where neither /shutdown nor the VimLeavePre
+autocmd gets to run. The kernel closes the pipe no matter how the parent died.
+
+The callback exits immediately rather than waiting for running requests to finish,
+as /shutdown does. Neovim is already gone, so nobody would see those responses;
+waiting could keep the process alive for up to shutdownTimeout for no reason.
+*/
+func WatchForParentExit() {
+	if !isPipeLike(os.Stdin) {
+		return
+	}
+
+	watchForEOF(os.Stdin, func() { os.Exit(0) })
+}
+
+/*
+isPipeLike reports whether EOF on the file would really mean that Neovim is gone.
+Sockets count too: libuv connects the child's stdin with a socketpair, not a FIFO.
+When the server is started by hand, stdin is a terminal or /dev/null, where EOF
+means a Ctrl-D or nothing at all.
+*/
+func isPipeLike(f *os.File) bool {
+	info, err := f.Stat()
+	if err != nil {
+		return false
+	}
+
+	return info.Mode()&(os.ModeNamedPipe|os.ModeSocket) != 0
+}
+
+/* watchForEOF calls onEOF, in a goroutine, once r is exhausted or errors. */
+func watchForEOF(r io.Reader, onEOF func()) {
+	go func() {
+		_, _ = io.Copy(io.Discard, r)
+		onEOF()
+	}()
+}
+
 type ShutdownRequest struct {
 	Restart bool `json:"restart"`
 }

cmd/app/shutdown_test.go

@@ -0,0 +1,289 @@
+package app
+
+import (
+	"fmt"
+	"net"
+	"net/http"
+	"os"
+	"sync"
+	"syscall"
+	"testing"
+	"time"
+)
+
+/*
+WatchForShutdown must not wait for an in-flight request forever. Before the
+shutdown context had a deadline, a single request still waiting on Gitlab kept
+the process alive after Neovim had already quit.
+*/
+func TestWatchForShutdownGivesUpOnInFlightRequests(t *testing.T) {
+	/* Blocks until the test is over, standing in for a slow Gitlab call */
+	handlerDone := make(chan struct{})
+	defer close(handlerDone)
+
+	requestReceived := make(chan struct{})
+
+	server := &http.Server{
+		Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			close(requestReceived)
+			<-handlerDone
+		}),
+	}
+
+	listener, err := net.Listen("tcp", "localhost:0")
+	if err != nil {
+		t.Fatal(err)
+	}
+	go func() {
+		_ = server.Serve(listener)
+	}()
+	defer func() { _ = server.Close() }()
+
+	url := fmt.Sprintf("http://%s/", listener.Addr().String())
+	go func() {
+		resp, err := http.Get(url)
+		if err == nil {
+			_ = resp.Body.Close()
+		}
+	}()
+
+	/* Only shut down once the handler is actually running, otherwise the server
+	would be idle and would shut down immediately whether it is bounded or not. */
+	select {
+	case <-requestReceived:
+	case <-time.After(5 * time.Second):
+		t.Fatal("Handler never received the request")
+	}
+
+	s := shutdownService{sigCh: make(chan os.Signal, 1), timeout: 100 * time.Millisecond}
+	s.sigCh <- killer{}
+
+	returned := make(chan time.Duration, 1)
+	go func() {
+		start := time.Now()
+		s.WatchForShutdown(server)
+		returned <- time.Since(start)
+	}()
+
+	select {
+	case elapsed := <-returned:
+		/* A lower bound alone would also pass if the function blocked for 100ms for
+		some unrelated reason. The upper bound shows that it was the deadline that
+		released the wait, not some other delay. */
+		if elapsed < s.timeout {
+			t.Errorf("Returned after %v, before the %v deadline", elapsed, s.timeout)
+		}
+		if elapsed > time.Second {
+			t.Errorf("Returned after %v, long past the %v deadline: it did not give up on the request", elapsed, s.timeout)
+		}
+	case <-time.After(5 * time.Second):
+		t.Fatal("WatchForShutdown did not return: it is still waiting for the request")
+	}
+}
+
+/* An idle server should still shut down straight away, well inside the deadline. */
+func TestWatchForShutdownReturnsImmediatelyWhenIdle(t *testing.T) {
+	server := &http.Server{Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})}
+
+	listener, err := net.Listen("tcp", "localhost:0")
+	if err != nil {
+		t.Fatal(err)
+	}
+	go func() {
+		_ = server.Serve(listener)
+	}()
+
+	s := shutdownService{sigCh: make(chan os.Signal, 1), timeout: 5 * time.Second}
+	s.sigCh <- killer{}
+
+	returned := make(chan struct{})
+	go func() {
+		s.WatchForShutdown(server)
+		close(returned)
+	}()
+
+	select {
+	case <-returned:
+	case <-time.After(2 * time.Second):
+		t.Fatal("Idle server did not shut down promptly")
+	}
+}
+
+/*
+The default timeout applies when the service does not set one. If the fallback
+is dropped, the context expires immediately and Shutdown gives up before the
+in-flight request finishes.
+*/
+func TestWatchForShutdownDefaultsToShutdownTimeout(t *testing.T) {
+	handlerDone := make(chan struct{})
+	requestReceived := make(chan struct{})
+
+	server := &http.Server{
+		Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			close(requestReceived)
+			<-handlerDone
+		}),
+	}
+
+	listener, err := net.Listen("tcp", "localhost:0")
+	if err != nil {
+		t.Fatal(err)
+	}
+	go func() {
+		_ = server.Serve(listener)
+	}()
+	defer func() { _ = server.Close() }()
+
+	url := fmt.Sprintf("http://%s/", listener.Addr().String())
+	go func() {
+		resp, err := http.Get(url)
+		if err == nil {
+			_ = resp.Body.Close()
+		}
+	}()
+
+	select {
+	case <-requestReceived:
+	case <-time.After(5 * time.Second):
+		t.Fatal("Handler never received the request")
+	}
+
+	var once sync.Once
+	release := func() { once.Do(func() { close(handlerDone) }) }
+	/* Lets the handler finish shortly after the shutdown starts: the fallback
+	keeps the context alive long enough for Shutdown to wait it out. */
+	time.AfterFunc(200*time.Millisecond, release)
+	defer release()
+
+	s := shutdownService{sigCh: make(chan os.Signal, 1)}
+	assert(t, s.timeout, time.Duration(0))
+	assert(t, shutdownTimeout, 10*time.Second)
+	s.sigCh <- killer{}
+
+	start := time.Now()
+	s.WatchForShutdown(server)
+	elapsed := time.Since(start)
+
+	if elapsed < 100*time.Millisecond {
+		t.Errorf("Returned after %v, before the handler finished: the context expired immediately, so the default timeout was not applied", elapsed)
+	}
+}
+
+/* EOF on the pipe means the process that spawned the server is gone. */
+func TestWatchForEOFFiresWhenTheWriteEndCloses(t *testing.T) {
+	reader, writer, err := os.Pipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer func() { _ = reader.Close() }()
+
+	fired := make(chan struct{})
+	watchForEOF(reader, func() { close(fired) })
+
+	select {
+	case <-fired:
+		t.Fatal("Fired while the write end was still open")
+	case <-time.After(200 * time.Millisecond):
+	}
+
+	if err := writer.Close(); err != nil {
+		t.Fatal(err)
+	}
+
+	select {
+	case <-fired:
+	case <-time.After(5 * time.Second):
+		t.Fatal("Did not fire after the write end closed")
+	}
+}
+
+/*
+The parent may die between the process starting and the watch beginning: EOF
+that is already pending when watchForEOF starts must still fire.
+*/
+func TestWatchForEOFFiresOnAlreadyClosedPipe(t *testing.T) {
+	reader, writer, err := os.Pipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	/* Simulate that Neovim dies before the EOF watch even starts */
+	defer func() { _ = reader.Close() }()
+	if err := writer.Close(); err != nil {
+		t.Fatal(err)
+	}
+
+	fired := make(chan struct{})
+	watchForEOF(reader, func() { close(fired) })
+
+	select {
+	case <-fired:
+	case <-time.After(5 * time.Second):
+		t.Fatal("Did not fire on an already-exhausted pipe")
+	}
+}
+
+/* Data on stdin must not be mistaken for the parent going away. */
+func TestWatchForEOFIgnoresWrites(t *testing.T) {
+	reader, writer, err := os.Pipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer func() { _ = reader.Close() }()
+	defer func() { _ = writer.Close() }()
+
+	fired := make(chan struct{})
+	watchForEOF(reader, func() { close(fired) })
+
+	if _, err := writer.WriteString("some noise\n"); err != nil {
+		t.Fatal(err)
+	}
+
+	select {
+	case <-fired:
+		t.Fatal("Fired on a write rather than on EOF")
+	case <-time.After(200 * time.Millisecond):
+	}
+}
+
+/*
+Only a pipe means "spawned by the plugin". Started by hand, stdin is a terminal
+or /dev/null, and EOF there must not shut the server down.
+*/
+func TestIsPipeLike(t *testing.T) {
+	reader, writer, err := os.Pipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer func() { _ = reader.Close() }()
+	defer func() { _ = writer.Close() }()
+	assert(t, isPipeLike(reader), true)
+
+	devNull, err := os.Open(os.DevNull)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer func() { _ = devNull.Close() }()
+	assert(t, isPipeLike(devNull), false)
+
+	regular, err := os.CreateTemp(t.TempDir(), "stdin")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer func() { _ = regular.Close() }()
+	assert(t, isPipeLike(regular), false)
+}
+
+/* Neovim connects the child's stdin with a socketpair, not with a FIFO. */
+func TestIsPipeLikeAcceptsSocketpair(t *testing.T) {
+	fds, err := syscall.Socketpair(syscall.AF_UNIX, syscall.SOCK_STREAM, 0)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	local := os.NewFile(uintptr(fds[0]), "socketpair")
+	defer func() { _ = local.Close() }()
+	remote := os.NewFile(uintptr(fds[1]), "socketpair")
+	defer func() { _ = remote.Close() }()
+
+	assert(t, isPipeLike(local), true)
+}