fix(server): guard Shutdown against a server that never started

internal/server.go

@@ -140,7 +140,14 @@ func (s *Server) Serve(ctx context.Context, ln net.Listener) error {
 	return s.server.Serve(ln)
 }
 
+// Shutdown gracefully stops the server. It is a no-op when the server was
+// never started: ListenAndServe returns before assigning s.server if the
+// listener cannot bind, and callers shut down unconditionally on that error.
 func (s *Server) Shutdown(ctx context.Context) error {
+	if s.server == nil {
+		return nil
+	}
+
 	return s.server.Shutdown(ctx)
 }
 

internal/server_test.go

@@ -1,15 +1,22 @@
 package internal_test
 
 import (
+	"bytes"
+	"context"
 	"errors"
+	"io"
 	"net"
 	"net/http"
 	"testing"
+	"time"
 
 	"github.com/kazz187/taskguild/internal"
 	"github.com/kazz187/taskguild/internal/config"
 )
 
+// testShutdownTimeout bounds the graceful shutdown performed during cleanup.
+const testShutdownTimeout = 5 * time.Second
+
 // TestServeProtocols verifies that a single listener accepts both HTTP/1.1 and
 // cleartext HTTP/2 (h2c) with prior knowledge, which is the behavior the
 // golang.org/x/net/http2/h2c handler wrapper used to provide.
@@ -63,23 +70,139 @@ func TestServeProtocols(t *testing.T) {
 	}
 }
 
-// startTestServer starts the production HTTP server on an ephemeral loopback
-// port and returns its "host:port" address.
-func startTestServer(t *testing.T) string {
-	t.Helper()
+// TestServeGRPCTrailersOverH2C verifies that the gRPC protocol still works over
+// cleartext HTTP/2, in particular that HTTP trailers reach the client. Trailers
+// are mandatory in the gRPC protocol (grpc-status is sent as one) and they are
+// the part most at risk from swapping the HTTP/2 implementation from
+// golang.org/x/net/http2 to the one built into net/http.
+func TestServeGRPCTrailersOverH2C(t *testing.T) {
+	t.Parallel()
+
+	baseURL := "http://" + startTestServer(t)
+
+	client := &http.Client{Transport: &http.Transport{Protocols: h2cProtocols()}}
+	t.Cleanup(client.CloseIdleConnections)
+
+	// A gRPC message is a 1-byte compression flag plus a 4-byte big-endian
+	// length. An empty HealthCheckRequest serializes to zero bytes, so this is
+	// a complete, uncompressed, zero-length request message.
+	msg := []byte{0x00, 0x00, 0x00, 0x00, 0x00}
+
+	// /grpc.health.v1.Health/Check bypasses apiKeyMiddleware, and the static
+	// checker needs no injected services, so no credentials are required.
+	req, err := http.NewRequestWithContext(t.Context(), http.MethodPost,
+		baseURL+"/grpc.health.v1.Health/Check", bytes.NewReader(msg))
+	if err != nil {
+		t.Fatalf("new request: %v", err)
+	}
+
+	req.Header.Set("Content-Type", "application/grpc+proto")
+	req.Header.Set("Te", "trailers")
+
+	resp, err := client.Do(req)
+	if err != nil {
+		t.Fatalf("POST Health/Check: %v", err)
+	}
+
+	defer func() {
+		closeErr := resp.Body.Close()
+		if closeErr != nil {
+			t.Errorf("close response body: %v", closeErr)
+		}
+	}()
+
+	// Trailers are only populated once the body has been drained.
+	body, err := io.ReadAll(resp.Body)
+	if err != nil {
+		t.Fatalf("read response body: %v", err)
+	}
+
+	if resp.Proto != "HTTP/2.0" {
+		t.Errorf("proto = %q, want %q", resp.Proto, "HTTP/2.0")
+	}
+
+	if resp.StatusCode != http.StatusOK {
+		t.Errorf("status = %d, want %d", resp.StatusCode, http.StatusOK)
+	}
+
+	if len(body) == 0 {
+		t.Error("response body is empty, want a gRPC message frame")
+	}
+
+	// grpc-status 0 is OK. Receiving it at all proves the trailer path works.
+	gotStatus := resp.Trailer.Get("Grpc-Status")
+	if gotStatus != "0" {
+		t.Errorf("grpc-status trailer = %q, want %q", gotStatus, "0")
+	}
+}
+
+// TestShutdownAfterListenFailure verifies that Shutdown is safe after
+// ListenAndServe failed to bind. cmd/taskguild-server cancels its context on a
+// server error and then calls Shutdown unconditionally, so a server that never
+// started must not panic there.
+func TestShutdownAfterListenFailure(t *testing.T) {
+	t.Parallel()
+
+	var lc net.ListenConfig
+
+	// Occupy a port so that the server's own bind is guaranteed to fail.
+	blocker, err := lc.Listen(t.Context(), "tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("listen: %v", err)
+	}
+
+	t.Cleanup(func() {
+		closeErr := blocker.Close()
+		if closeErr != nil && !errors.Is(closeErr, net.ErrClosed) {
+			t.Errorf("close blocker: %v", closeErr)
+		}
+	})
+
+	host, port, err := net.SplitHostPort(blocker.Addr().String())
+	if err != nil {
+		t.Fatalf("split host port: %v", err)
+	}
 
 	env := &config.Env{APIKey: "test-api-key"}
+	env.HTTPHost = host
+	env.HTTPPort = port
 
-	// The generated Connect handler constructors only take method values off
-	// the service pointers, so nil services are safe here: nothing is
-	// dereferenced until an RPC is dispatched, and this test only calls /health.
-	srv := internal.NewServer(
+	srv := newTestServer(env)
+
+	err = srv.ListenAndServe(t.Context())
+	if err == nil {
+		t.Fatal("ListenAndServe on an occupied port returned nil, want an error")
+	}
+
+	// Must not panic on the nil inner *http.Server.
+	err = srv.Shutdown(t.Context())
+	if err != nil {
+		t.Errorf("Shutdown after a failed listen: %v", err)
+	}
+}
+
+// newTestServer builds a Server with no backing services.
+//
+// The generated Connect handler constructors only take method values off the
+// service pointers, so nil services are safe: nothing is dereferenced until an
+// RPC is dispatched, and these tests only call /health and the gRPC health
+// endpoint, both of which are served without any injected service.
+func newTestServer(env *config.Env) *internal.Server {
+	return internal.NewServer(
 		env,
 		nil, nil, nil, nil,
 		nil, nil, nil, nil,
 		nil, nil, nil, nil,
 		nil, nil, nil, nil,
 	)
+}
+
+// startTestServer starts the production HTTP server on an ephemeral loopback
+// port and returns its "host:port" address.
+func startTestServer(t *testing.T) string {
+	t.Helper()
+
+	srv := newTestServer(&config.Env{APIKey: "test-api-key"})
 
 	ctx := t.Context()
 
@@ -97,9 +220,16 @@ func startTestServer(t *testing.T) string {
 	}()
 
 	t.Cleanup(func() {
-		closeErr := ln.Close()
-		if closeErr != nil && !errors.Is(closeErr, net.ErrClosed) {
-			t.Errorf("close listener: %v", closeErr)
+		// Shut down rather than just closing the listener: this is the path
+		// whose semantics changed when h2c stopped hijacking connections, and
+		// it also reaps the connection goroutines. ctx is already canceled by
+		// the time cleanups run, so detach from it.
+		shutdownCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), testShutdownTimeout)
+		defer cancel()
+
+		shutdownErr := srv.Shutdown(shutdownCtx)
+		if shutdownErr != nil {
+			t.Errorf("shutdown: %v", shutdownErr)
 		}
 
 		doneErr := <-serveErr