Wire server discovery protocol into thv serve (#4319)

* Wire server discovery protocol into thv serve

The discovery package (pkg/server/discovery/) was already implemented
and tested but had zero imports in the codebase. This wires it into
the serve command so clients (CLI, Studio) can auto-discover a running
server without hardcoded ports or environment variables.

On startup, thv serve now generates a cryptographic nonce, writes a
discovery file to $XDG_STATE_HOME/toolhive/server/server.json with
the actual listen URL (supporting port 0 and Unix sockets), and
returns the nonce via the X-Toolhive-Nonce health check header. On
shutdown the file is removed.

The skills client now tries discovery before falling back to the
TOOLHIVE_API_URL env var or the default localhost:8080, with loopback
and socket-path validation on discovered URLs.

Additional fixes: SIGTERM handling in the serve command, a 30-second
shutdown timeout (was unbounded), symlink rejection on the discovery
file read path, directory permission tightening after MkdirAll, and
constant-time nonce comparison.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>

* Address review feedback on server discovery

- Wrap writeDiscoveryFile check-then-write in WithFileLock to prevent
  TOCTOU race when two servers start simultaneously
- Log FindProcess errors at Debug level instead of silently discarding
- Consolidate ListenURL tests into a table-driven test
- Rename healtcheck_test.go to healthcheck_test.go (fix typo)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>

* Create discovery directory before acquiring lock file

The discovery lock file is created in the same directory as
server.json, but the directory may not exist on a fresh system.
MkdirAll was called inside the lock callback (via WriteServerInfo),
but the lock acquisition itself needs the directory to already exist.
Create the directory before calling WithFileLock so the lock file
can be written.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Address review feedback on discovery wiring

- Extract shared HTTPClientForURL in the discovery package to
  deduplicate transport setup between health.go and the skills client
- Propagate context.Context through NewDefaultClient and
  resolveViaDiscovery instead of using context.Background()
- Add comment explaining intentional opts-shadowing order so
  caller-supplied options can override discovery defaults
- Use url.JoinPath in buildHealthClient instead of string concatenation

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: JAORMX

cmd/thv/app/skill_build.go

@@ -39,7 +39,7 @@ func skillBuildCmdFunc(cmd *cobra.Command, args []string) error {
 		return fmt.Errorf("failed to resolve path: %w", err)
 	}
 
-	c := newSkillClient()
+	c := newSkillClient(cmd.Context())
 
 	result, err := c.Build(cmd.Context(), skills.BuildOptions{
 		Path: absPath,

cmd/thv/app/skill_helpers.go

@@ -4,6 +4,7 @@
 package app
 
 import (
+	"context"
 	"errors"
 	"fmt"
 
@@ -14,8 +15,9 @@ import (
 )
 
 // newSkillClient creates a new Skills API HTTP client using default settings.
-func newSkillClient() *skillclient.Client {
-	return skillclient.NewDefaultClient()
+// The context is used for server discovery; it is not stored.
+func newSkillClient(ctx context.Context) *skillclient.Client {
+	return skillclient.NewDefaultClient(ctx)
 }
 
 // completeSkillNames provides shell completion for installed skill names.
@@ -24,7 +26,7 @@ func completeSkillNames(cmd *cobra.Command, args []string, _ string) ([]string,
 		return nil, cobra.ShellCompDirectiveNoFileComp
 	}
 
-	c := newSkillClient()
+	c := newSkillClient(cmd.Context())
 	installed, err := c.List(cmd.Context(), skills.ListOptions{})
 	if err != nil {
 		return nil, cobra.ShellCompDirectiveError

cmd/thv/app/skill_info.go

@@ -43,7 +43,7 @@ func init() {
 }
 
 func skillInfoCmdFunc(cmd *cobra.Command, args []string) error {
-	c := newSkillClient()
+	c := newSkillClient(cmd.Context())
 
 	info, err := c.Info(cmd.Context(), skills.InfoOptions{
 		Name:        args[0],

cmd/thv/app/skill_install.go

@@ -42,7 +42,7 @@ func init() {
 }
 
 func skillInstallCmdFunc(cmd *cobra.Command, args []string) error {
-	c := newSkillClient()
+	c := newSkillClient(cmd.Context())
 
 	_, err := c.Install(cmd.Context(), skills.InstallOptions{
 		Name:        args[0],

cmd/thv/app/skill_list.go

@@ -47,7 +47,7 @@ func init() {
 }
 
 func skillListCmdFunc(cmd *cobra.Command, _ []string) error {
-	c := newSkillClient()
+	c := newSkillClient(cmd.Context())
 
 	installed, err := c.List(cmd.Context(), skills.ListOptions{
 		Scope:       skills.Scope(skillListScope),

cmd/thv/app/skill_push.go

@@ -22,7 +22,7 @@ func init() {
 }
 
 func skillPushCmdFunc(cmd *cobra.Command, args []string) error {
-	c := newSkillClient()
+	c := newSkillClient(cmd.Context())
 
 	err := c.Push(cmd.Context(), skills.PushOptions{
 		Reference: args[0],

cmd/thv/app/skill_uninstall.go

@@ -39,7 +39,7 @@ func init() {
 }
 
 func skillUninstallCmdFunc(cmd *cobra.Command, args []string) error {
-	c := newSkillClient()
+	c := newSkillClient(cmd.Context())
 
 	err := c.Uninstall(cmd.Context(), skills.UninstallOptions{
 		Name:        args[0],

cmd/thv/app/skill_validate.go

@@ -37,7 +37,7 @@ func skillValidateCmdFunc(cmd *cobra.Command, args []string) error {
 		return fmt.Errorf("failed to resolve path: %w", err)
 	}
 
-	c := newSkillClient()
+	c := newSkillClient(cmd.Context())
 
 	result, err := c.Validate(cmd.Context(), absPath)
 	if err != nil {

pkg/api/server.go

@@ -17,6 +17,8 @@ package api
 
 import (
 	"context"
+	"crypto/rand"
+	"encoding/hex"
 	"errors"
 	"fmt"
 	"io"
@@ -39,9 +41,11 @@ import (
 	"github.com/stacklok/toolhive/pkg/config"
 	"github.com/stacklok/toolhive/pkg/container"
 	"github.com/stacklok/toolhive/pkg/container/runtime"
+	"github.com/stacklok/toolhive/pkg/fileutils"
 	"github.com/stacklok/toolhive/pkg/groups"
 	"github.com/stacklok/toolhive/pkg/recovery"
 	"github.com/stacklok/toolhive/pkg/registry"
+	"github.com/stacklok/toolhive/pkg/server/discovery"
 	"github.com/stacklok/toolhive/pkg/skills"
 	"github.com/stacklok/toolhive/pkg/skills/gitresolver"
 	"github.com/stacklok/toolhive/pkg/skills/skillsvc"
@@ -55,6 +59,7 @@ const (
 	middlewareTimeout  = 60 * time.Second
 	readHeaderTimeout  = 10 * time.Second
 	shutdownTimeout    = 30 * time.Second
+	nonceBytes         = 16
 	socketPermissions  = 0660    // Socket file permissions (owner/group read-write)
 	maxRequestBodySize = 1 << 20 // 1MB - Maximum request body size
 )
@@ -65,6 +70,7 @@ type ServerBuilder struct {
 	isUnixSocket     bool
 	debugMode        bool
 	enableDocs       bool
+	nonce            string
 	oidcConfig       *auth.TokenValidatorConfig
 	middlewares      []func(http.Handler) http.Handler
 	customRoutes     map[string]http.Handler
@@ -108,6 +114,14 @@ func (b *ServerBuilder) WithDocs(enableDocs bool) *ServerBuilder {
 	return b
 }
 
+// WithNonce sets the server instance nonce used for discovery verification.
+// When non-empty, the server writes a discovery file on startup and returns
+// the nonce in the X-Toolhive-Nonce health check header.
+func (b *ServerBuilder) WithNonce(nonce string) *ServerBuilder {
+	b.nonce = nonce
+	return b
+}
+
 // WithOIDCConfig sets the OIDC configuration
 func (b *ServerBuilder) WithOIDCConfig(oidcConfig *auth.TokenValidatorConfig) *ServerBuilder {
 	b.oidcConfig = oidcConfig
@@ -297,7 +311,7 @@ func (b *ServerBuilder) setupDefaultRoutes(r *chi.Mux) {
 
 	// All other routes get standard timeout
 	standardRouters := map[string]http.Handler{
-		"/health":               v1.HealthcheckRouter(b.containerRuntime),
+		"/health":               v1.HealthcheckRouter(b.containerRuntime, b.nonce),
 		"/api/v1beta/version":   v1.VersionRouter(),
 		"/api/v1beta/registry":  v1.RegistryRouter(true),
 		"/api/v1beta/discovery": v1.DiscoveryRouter(),
@@ -504,6 +518,7 @@ type Server struct {
 	address      string
 	isUnixSocket bool
 	addrType     string
+	nonce        string
 	storeCloser  io.Closer
 }
 
@@ -532,14 +547,29 @@ func NewServer(ctx context.Context, builder *ServerBuilder) (*Server, error) {
 		address:      builder.address,
 		isUnixSocket: builder.isUnixSocket,
 		addrType:     addrType,
+		nonce:        builder.nonce,
 		storeCloser:  builder.skillStoreCloser,
 	}, nil
 }
 
+// ListenURL returns the URL where the server is listening, using the actual
+// bound address from the listener (important when binding to port 0).
+func (s *Server) ListenURL() string {
+	if s.isUnixSocket {
+		return fmt.Sprintf("unix://%s", s.address)
+	}
+	return fmt.Sprintf("http://%s", s.listener.Addr().String())
+}
+
 // Start starts the server and blocks until the context is cancelled
 func (s *Server) Start(ctx context.Context) error {
 	slog.Info("starting server", "type", s.addrType, "address", s.address)
 
+	// Write server discovery file so clients can find this instance.
+	if err := s.writeDiscoveryFile(ctx); err != nil {
+		return err
+	}
+
 	// Start server in a goroutine
 	serverErr := make(chan error, 1)
 	go func() {
@@ -562,6 +592,61 @@ func (s *Server) Start(ctx context.Context) error {
 	}
 }
 
+// writeDiscoveryFile writes the server discovery file if a nonce is configured.
+// It checks for an existing healthy server first to prevent silent orphaning.
+// The entire check-then-write sequence is wrapped in a file lock to prevent
+// TOCTOU races when two servers start simultaneously.
+func (s *Server) writeDiscoveryFile(ctx context.Context) error {
+	if s.nonce == "" {
+		return nil
+	}
+
+	// Ensure the discovery directory exists before acquiring the lock,
+	// since the lock file is created in the same directory.
+	discoveryPath := discovery.FilePath()
+	if err := os.MkdirAll(filepath.Dir(discoveryPath), 0700); err != nil {
+		return fmt.Errorf("failed to create discovery directory: %w", err)
+	}
+
+	return fileutils.WithFileLock(discoveryPath, func() error {
+		// Guard against overwriting another server's discovery file.
+		result, err := discovery.Discover(ctx)
+		if err != nil {
+			slog.Debug("discovery check failed, proceeding with startup", "error", err)
+		} else {
+			switch result.State {
+			case discovery.StateRunning:
+				return fmt.Errorf("another ToolHive server is already running at %s (PID %d)", result.Info.URL, result.Info.PID)
+			case discovery.StateStale:
+				slog.Debug("cleaning up stale discovery file", "pid", result.Info.PID)
+				if err := discovery.CleanupStale(); err != nil {
+					slog.Warn("failed to clean up stale discovery file", "error", err)
+				}
+			case discovery.StateUnhealthy:
+				// The process is alive but not responding to health checks.
+				// This can happen after a crash-restart where the old process
+				// is hung. We intentionally overwrite the discovery file so
+				// this new server becomes discoverable.
+				slog.Warn("existing server is unhealthy, overwriting discovery file", "pid", result.Info.PID)
+			case discovery.StateNotFound:
+				// No existing server, proceed normally.
+			}
+		}
+
+		info := &discovery.ServerInfo{
+			URL:       s.ListenURL(),
+			PID:       os.Getpid(),
+			Nonce:     s.nonce,
+			StartedAt: time.Now().UTC(),
+		}
+		if err := discovery.WriteServerInfo(info); err != nil {
+			return fmt.Errorf("failed to write discovery file: %w", err)
+		}
+		slog.Debug("wrote discovery file", "url", info.URL, "pid", info.PID)
+		return nil
+	})
+}
+
 // shutdown gracefully shuts down the server
 func (s *Server) shutdown() error {
 	shutdownCtx, cancel := context.WithTimeout(context.Background(), shutdownTimeout)
@@ -579,6 +664,11 @@ func (s *Server) shutdown() error {
 
 // cleanup performs cleanup operations
 func (s *Server) cleanup() {
+	if s.nonce != "" {
+		if err := discovery.RemoveServerInfo(); err != nil {
+			slog.Warn("failed to remove discovery file", "error", err)
+		}
+	}
 	if s.storeCloser != nil {
 		if err := s.storeCloser.Close(); err != nil {
 			slog.Warn("failed to close skill store", "error", err)
@@ -653,6 +743,16 @@ func (a *clientPathAdapter) ListSkillSupportingClients() []string {
 	return result
 }
 
+// generateNonce creates a cryptographically random nonce for server instance
+// identification. It returns a 32-character hex string (16 random bytes).
+func generateNonce() (string, error) {
+	b := make([]byte, nonceBytes)
+	if _, err := rand.Read(b); err != nil {
+		return "", fmt.Errorf("failed to generate server nonce: %w", err)
+	}
+	return hex.EncodeToString(b), nil
+}
+
 // Serve starts the server on the given address and serves the API.
 // It is assumed that the caller sets up appropriate signal handling.
 // If isUnixSocket is true, address is treated as a UNIX socket path.
@@ -666,11 +766,17 @@ func Serve(
 	oidcConfig *auth.TokenValidatorConfig,
 	middlewares ...func(http.Handler) http.Handler,
 ) error {
+	nonce, err := generateNonce()
+	if err != nil {
+		return err
+	}
+
 	builder := NewServerBuilder().
 		WithAddress(address).
 		WithUnixSocket(isUnixSocket).
 		WithDebugMode(debugMode).
 		WithDocs(enableDocs).
+		WithNonce(nonce).
 		WithOIDCConfig(oidcConfig).
 		WithMiddleware(middlewares...)
 

pkg/api/server_test.go

@@ -0,0 +1,88 @@
+// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package api
+
+import (
+	"fmt"
+	"net"
+	"regexp"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestGenerateNonce(t *testing.T) {
+	t.Parallel()
+
+	t.Run("returns valid 32-char hex string", func(t *testing.T) {
+		t.Parallel()
+
+		nonce, err := generateNonce()
+		require.NoError(t, err)
+
+		assert.Len(t, nonce, 32)
+		assert.Regexp(t, regexp.MustCompile(`^[0-9a-f]{32}$`), nonce)
+	})
+
+	t.Run("returns unique values on successive calls", func(t *testing.T) {
+		t.Parallel()
+
+		nonce1, err := generateNonce()
+		require.NoError(t, err)
+
+		nonce2, err := generateNonce()
+		require.NoError(t, err)
+
+		assert.NotEqual(t, nonce1, nonce2)
+	})
+}
+
+func TestListenURL(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name     string
+		server   func(t *testing.T) *Server
+		expected func(s *Server) string
+	}{
+		{
+			name: "TCP returns http URL with actual port",
+			server: func(t *testing.T) *Server {
+				t.Helper()
+				listener, err := net.Listen("tcp", "127.0.0.1:0")
+				require.NoError(t, err)
+				t.Cleanup(func() { listener.Close() })
+				return &Server{
+					listener:     listener,
+					isUnixSocket: false,
+					address:      "127.0.0.1:0",
+				}
+			},
+			expected: func(s *Server) string {
+				return fmt.Sprintf("http://%s", s.listener.Addr().String())
+			},
+		},
+		{
+			name: "Unix socket returns unix URL",
+			server: func(_ *testing.T) *Server {
+				return &Server{
+					isUnixSocket: true,
+					address:      "/tmp/test.sock",
+				}
+			},
+			expected: func(_ *Server) string {
+				return "unix:///tmp/test.sock"
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			s := tt.server(t)
+			assert.Equal(t, tt.expected(s), s.ListenURL())
+		})
+	}
+}