feat(lsp): polish — graceful shutdown, log level, upstream error-format canaries

Dockerfile

@@ -2,7 +2,7 @@
 # Builder
 # -----------------------------------------------------------------
 FROM golang:1.25-alpine3.22 as builder
-ARG VERSION
+ARG VERSION=dev
 
 RUN apk add --update git
 

Makefile

@@ -3,10 +3,10 @@
 all: build test lint
 
 build:
-	go build -ldflags="-s -w -X github.com/webrpc/ridl-lsp.VERSION=$$(git describe --tags)" -o ./bin/ridl-lsp ./cmd/ridl-lsp
+	go build -ldflags="-s -w -X github.com/webrpc/ridl-lsp.VERSION=$$(git describe --tags --always 2>/dev/null || echo dev)" -o ./bin/ridl-lsp ./cmd/ridl-lsp
 
 install:
-	go install -ldflags="-s -w -X github.com/webrpc/ridl-lsp.VERSION=$$(git describe --tags)" ./cmd/ridl-lsp
+	go install -ldflags="-s -w -X github.com/webrpc/ridl-lsp.VERSION=$$(git describe --tags --always 2>/dev/null || echo dev)" ./cmd/ridl-lsp
 
 lint:
 	go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint run ./... --fix -c .golangci.yml

cmd/ridl-lsp/exit_e2e_test.go

@@ -0,0 +1,55 @@
+package main
+
+import (
+	"errors"
+	"fmt"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// TestExitNotificationExitCode runs the real binary to pin the LSP exit-code
+// contract end-to-end: exit-without-shutdown is 1, shutdown-then-exit is 0. This
+// is the only level that exercises the read-loop ordering — a unit test on
+// Server.Exit can't see the EOF/async race that an in-process handler would hit.
+func TestExitNotificationExitCode(t *testing.T) {
+	bin := filepath.Join(t.TempDir(), "ridl-lsp-test")
+	if out, err := exec.Command("go", "build", "-o", bin, ".").CombinedOutput(); err != nil {
+		t.Fatalf("build binary: %v\n%s", err, out)
+	}
+
+	frame := func(body string) string {
+		return fmt.Sprintf("Content-Length: %d\r\n\r\n%s", len(body), body)
+	}
+	shutdown := frame(`{"jsonrpc":"2.0","id":1,"method":"shutdown","params":null}`)
+	exit := frame(`{"jsonrpc":"2.0","method":"exit","params":null}`)
+
+	cases := []struct {
+		name  string
+		input string
+		want  int
+	}{
+		{"exit without shutdown", exit, 1},
+		{"shutdown then exit", shutdown + exit, 0},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			cmd := exec.Command(bin)
+			cmd.Stdin = strings.NewReader(tc.input)
+
+			got := 0
+			if err := cmd.Run(); err != nil {
+				var exitErr *exec.ExitError
+				if !errors.As(err, &exitErr) {
+					t.Fatalf("run binary: %v", err)
+				}
+				got = exitErr.ExitCode()
+			}
+			if got != tc.want {
+				t.Fatalf("exit code: got %d, want %d", got, tc.want)
+			}
+		})
+	}
+}

cmd/ridl-lsp/main.go

@@ -5,10 +5,13 @@ import (
 	"fmt"
 	"log"
 	"os"
+	"os/signal"
+	"syscall"
 
 	"go.lsp.dev/jsonrpc2"
 	"go.lsp.dev/protocol"
 	"go.uber.org/zap"
+	"go.uber.org/zap/zapcore"
 
 	ridllsp "github.com/webrpc/ridl-lsp"
 	"github.com/webrpc/ridl-lsp/internal/lsp"
@@ -22,10 +25,13 @@ func main() {
 
 	log.Println("ridl-lsp starting on stdio")
 
-	ctx := context.Background()
+	// Cancel on SIGINT/SIGTERM so a supervised/containerized server (the Docker
+	// ENTRYPOINT) shuts the connection down cleanly instead of being killed.
+	ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
+	defer stop()
 	stream := jsonrpc2.NewStream(stdrwc{})
 
-	logger, err := zap.NewProduction()
+	logger, err := newLogger(os.Getenv("RIDL_LSP_LOG_LEVEL"))
 	if err != nil {
 		log.Fatalf("create logger: %v", err)
 	}
@@ -42,13 +48,51 @@ func main() {
 	ctx = protocol.WithClient(ctx, client)
 	server.SetClient(client)
 
-	handler := lsp.RecoverHandler(
+	serverHandler := lsp.RecoverHandler(
 		protocol.ServerHandler(server, jsonrpc2.MethodNotFoundHandler),
 		logger,
 	)
-	conn.Go(ctx, protocol.Handlers(handler))
+	asyncHandler := protocol.Handlers(serverHandler)
+	// Dispatch the lifecycle methods synchronously. protocol.Handlers' AsyncHandler
+	// runs each request in its own goroutine, which races process teardown two ways:
+	// `exit` could lose to conn.Done() reading the trailing EOF (→ wrong exit 0),
+	// and `exit` could read the shutdown flag before an async `shutdown` sets it.
+	// Running both in the read loop, in arrival order, makes the exit code reliable.
+	syncHandler := jsonrpc2.ReplyHandler(serverHandler)
+	handler := jsonrpc2.Handler(func(ctx context.Context, reply jsonrpc2.Replier, req jsonrpc2.Request) error {
+		switch req.Method() {
+		case protocol.MethodShutdown, protocol.MethodExit:
+			return syncHandler(ctx, reply, req)
+		}
+		return asyncHandler(ctx, reply, req)
+	})
+	conn.Go(ctx, handler)
 
-	<-conn.Done()
+	// The LSP `exit` notification drives the spec exit code from the Exit handler.
+	// Here we only handle the transport: a bare stream close (the common editor
+	// teardown) and OS signals both exit 0.
+	select {
+	case <-conn.Done():
+	case <-ctx.Done():
+		log.Println("ridl-lsp: signal received, shutting down")
+		_ = conn.Close()
+	}
+}
+
+// newLogger builds the production logger at the level named by level (debug,
+// info, warn, error). An empty string keeps the default, and an unparseable value
+// is reported and ignored rather than failing startup over a typo'd env var.
+func newLogger(level string) (*zap.Logger, error) {
+	cfg := zap.NewProductionConfig()
+	if level != "" {
+		var l zapcore.Level
+		if err := l.UnmarshalText([]byte(level)); err != nil {
+			log.Printf("ridl-lsp: ignoring invalid RIDL_LSP_LOG_LEVEL %q: %v", level, err)
+		} else {
+			cfg.Level = zap.NewAtomicLevelAt(l)
+		}
+	}
+	return cfg.Build()
 }
 
 type stdrwc struct{}

cmd/ridl-lsp/main_test.go

@@ -0,0 +1,34 @@
+package main
+
+import (
+	"testing"
+
+	"go.uber.org/zap/zapcore"
+)
+
+func TestNewLogger(t *testing.T) {
+	debug, err := newLogger("debug")
+	if err != nil {
+		t.Fatalf("newLogger(debug): %v", err)
+	}
+	if !debug.Core().Enabled(zapcore.DebugLevel) {
+		t.Fatal("debug level was not applied")
+	}
+
+	def, err := newLogger("")
+	if err != nil {
+		t.Fatalf("newLogger(empty): %v", err)
+	}
+	if def.Core().Enabled(zapcore.DebugLevel) {
+		t.Fatal("empty level must keep the default (info), not enable debug")
+	}
+
+	// An invalid value must not fail startup over a typo, and must keep the default.
+	bogus, err := newLogger("not-a-level")
+	if err != nil {
+		t.Fatalf("invalid level must not error: %v", err)
+	}
+	if bogus.Core().Enabled(zapcore.DebugLevel) {
+		t.Fatal("invalid level must fall back to the default")
+	}
+}

internal/lsp/diagnostics.go

@@ -79,8 +79,9 @@ func (s *Server) parseDocument(ctx context.Context, doc *documents.Document) []p
 }
 
 func (s *Server) overlayContents() map[string]string {
-	overlays := make(map[string]string, len(s.docs.All()))
-	for _, doc := range s.docs.All() {
+	docs := s.docs.All()
+	overlays := make(map[string]string, len(docs))
+	for _, doc := range docs {
 		overlays[doc.Path] = doc.Content
 	}
 	return overlays

internal/lsp/lifecycle_test.go

@@ -0,0 +1,41 @@
+package lsp
+
+import (
+	"context"
+	"testing"
+
+	"go.uber.org/zap"
+)
+
+// TestExitCodeFollowsShutdown pins the LSP contract: the process exits 0 only if
+// shutdown arrived before exit, otherwise 1. The exit func is injected so the
+// decision is observable without terminating the test binary.
+func TestExitCodeFollowsShutdown(t *testing.T) {
+	t.Run("exit without shutdown is non-zero", func(t *testing.T) {
+		srv := NewServer(zap.NewNop())
+		got, called := -1, false
+		srv.exitProcess = func(code int) { got, called = code, true }
+
+		_ = srv.Exit(context.Background())
+
+		if !called {
+			t.Fatal("Exit must terminate the process")
+		}
+		if got != 1 {
+			t.Fatalf("exit code without prior shutdown: got %d, want 1", got)
+		}
+	})
+
+	t.Run("shutdown then exit is zero", func(t *testing.T) {
+		srv := NewServer(zap.NewNop())
+		got := -1
+		srv.exitProcess = func(code int) { got = code }
+
+		_ = srv.Shutdown(context.Background())
+		_ = srv.Exit(context.Background())
+
+		if got != 0 {
+			t.Fatalf("exit code after shutdown: got %d, want 0", got)
+		}
+	})
+}

internal/lsp/server.go

@@ -2,6 +2,8 @@ package lsp
 
 import (
 	"context"
+	"os"
+	"sync/atomic"
 
 	"go.lsp.dev/protocol"
 	"go.uber.org/zap"
@@ -17,14 +19,20 @@ type Server struct {
 	parser    *ridlparser.Parser
 	client    protocol.Client
 	logger    *zap.Logger
+
+	shutdown atomic.Bool
+	// exitProcess is os.Exit in production; injectable so the exit-code contract
+	// can be tested without terminating the test binary.
+	exitProcess func(int)
 }
 
 func NewServer(logger *zap.Logger) *Server {
 	return &Server{
-		docs:      documents.NewStore(),
-		workspace: workspace.NewManager(),
-		parser:    ridlparser.NewParser(),
-		logger:    logger,
+		docs:        documents.NewStore(),
+		workspace:   workspace.NewManager(),
+		parser:      ridlparser.NewParser(),
+		logger:      logger,
+		exitProcess: os.Exit,
 	}
 }
 
@@ -104,13 +112,25 @@ func (s *Server) Initialized(ctx context.Context, params *protocol.InitializedPa
 }
 
 func (s *Server) Shutdown(ctx context.Context) error {
+	s.shutdown.Store(true)
 	return nil
 }
 
 func (s *Server) Exit(ctx context.Context) error {
+	// LSP: exit 0 only if shutdown was received first, otherwise 1.
+	_ = s.logger.Sync() //nolint:errcheck // best-effort flush before exit
+	s.exitProcess(exitCode(s.shutdown.Load()))
 	return nil
 }
 
+// exitCode maps the shutdown-before-exit state to the LSP-mandated process code.
+func exitCode(shutdownReceived bool) int {
+	if shutdownReceived {
+		return 0
+	}
+	return 1
+}
+
 func (s *Server) DidOpen(ctx context.Context, params *protocol.DidOpenTextDocumentParams) error {
 	doc := &documents.Document{
 		URI:     string(params.TextDocument.URI),

internal/lsp/upstream_error_format_test.go

@@ -0,0 +1,41 @@
+package lsp
+
+import (
+	"context"
+	"testing"
+
+	ridlparser "github.com/webrpc/ridl-lsp/internal/ridl"
+)
+
+// TestUpstreamErrorFormatCanary guards the string-shape coupling to the upstream
+// parser. errorToDiagnostic regex-extracts "line:col:" out of upstream error text
+// to position diagnostics; if upstream changes that format, the regex silently
+// stops matching and every diagnostic collapses to a full-line-1 smear. This pins
+// the current format so such a drift fails CI instead of degrading positions.
+func TestUpstreamErrorFormatCanary(t *testing.T) {
+	const positioned = `webrpc = v1
+
+name = test
+version = v0.1.0
+
+struct User
+  - id: uint64
+  - bad field here
+`
+	result, err := ridlparser.NewParser().Parse(context.Background(), t.TempDir(), "canary.ridl", map[string]string{"canary.ridl": positioned})
+	if err != nil {
+		t.Fatalf("Parse returned error: %v", err)
+	}
+	if len(result.Errors) == 0 {
+		t.Fatal("fixture should have produced a positioned parse error")
+	}
+
+	diag := errorToDiagnostic(result.Errors[0])
+
+	// The regex-matched path yields a narrow one-character range; the unmatched
+	// fallback yields a full-line smear (Character 0..1000). A narrow range proves
+	// the "line:col:" prefix was parsed out of the upstream message.
+	if diag.Range.End.Character-diag.Range.Start.Character != 1 {
+		t.Fatalf("expected a narrow position parsed from the upstream error format, got range %+v for message %q — upstream error format may have drifted", diag.Range, result.Errors[0].Error())
+	}
+}

internal/ridl/overlay_fs.go

@@ -57,10 +57,13 @@ type memFileInfo struct {
 	size int64
 }
 
-func (fi *memFileInfo) Name() string       { return fi.name }
-func (fi *memFileInfo) Size() int64        { return fi.size }
-func (fi *memFileInfo) Mode() fs.FileMode  { return 0444 }
-func (fi *memFileInfo) ModTime() time.Time { return time.Now() }
+func (fi *memFileInfo) Name() string      { return fi.name }
+func (fi *memFileInfo) Size() int64       { return fi.size }
+func (fi *memFileInfo) Mode() fs.FileMode { return 0444 }
+
+// In-memory overlays have no real mtime; a zero time keeps the FileInfo
+// deterministic (the parser only reads content, never the timestamp).
+func (fi *memFileInfo) ModTime() time.Time { return time.Time{} }
 func (fi *memFileInfo) IsDir() bool        { return false }
 func (fi *memFileInfo) Sys() any           { return nil }
 

internal/ridl/parser.go

@@ -226,16 +226,24 @@ func (p *Parser) parse(ctx context.Context, workspace, path string, overlays map
 	return result, nil
 }
 
+// These substrings couple us to upstream webrpc error wording — there is no typed
+// error to match on. TestVersionRequiredErrorFormat pins them so an upstream bump
+// that changes the wording fails CI instead of silently breaking import handling.
+const (
+	errVersionRequiredText = "schema error: version is required when services are defined"
+	errStackTraceText      = "stack trace:"
+)
+
 func isVersionOptionalSchemaError(err error, imported bool) bool {
 	if err == nil {
 		return false
 	}
 
-	if !strings.Contains(err.Error(), "schema error: version is required when services are defined") {
+	if !strings.Contains(err.Error(), errVersionRequiredText) {
 		return false
 	}
 
-	return imported || strings.Contains(err.Error(), "stack trace:")
+	return imported || strings.Contains(err.Error(), errStackTraceText)
 }
 
 func (p *Parser) buildPartialSchema(ctx context.Context, workspace, path string, root *RootNode, overlays map[string]string, visited map[string]struct{}) *schema.WebRPCSchema {