Distinguish container restarts from exits in monitor (#3937)

The container monitor sends ErrContainerExited for both true exits
and Docker restarts (detected via StartedAt change). The transport
layer has no way to tell them apart, so it always tears down the
proxy — killing a healthy container that Docker already restarted.

Add ErrContainerRestarted sentinel error so the transport can handle
restarts differently: reconnect the monitor to track the new start
time while keeping the proxy running.

Fixes #3936

Signed-off-by: Greg Katz <gkatz@indeed.com>

Author: gkatz2

pkg/container/docker/errors.go

@@ -32,6 +32,9 @@ var (
 	// Deprecated: Use runtime.ErrContainerExited.
 	ErrContainerExited = runtime.ErrContainerExited
 
+	// Deprecated: Use runtime.ErrContainerRestarted.
+	ErrContainerRestarted = runtime.ErrContainerRestarted
+
 	// Deprecated: Use runtime.ErrContainerRemoved.
 	ErrContainerRemoved = runtime.ErrContainerRemoved
 

pkg/container/runtime/errors.go

@@ -22,6 +22,11 @@ var (
 	// ErrContainerExited is returned when a container has exited unexpectedly
 	ErrContainerExited = httperr.WithCode(fmt.Errorf("container exited unexpectedly"), http.StatusBadRequest)
 
+	// ErrContainerRestarted is returned when a container has been restarted
+	// (e.g., by Docker restart policy). The container is still running.
+	// This is distinct from ErrContainerExited.
+	ErrContainerRestarted = httperr.WithCode(fmt.Errorf("container restarted"), http.StatusBadRequest)
+
 	// ErrContainerRemoved is returned when a container has been removed
 	ErrContainerRemoved = httperr.WithCode(fmt.Errorf("container removed"), http.StatusBadRequest)
 )

pkg/container/runtime/monitor.go

@@ -148,7 +148,7 @@ func (m *WorkloadMonitor) monitor(ctx context.Context) {
 			if err == nil && !info.StartedAt.IsZero() && !info.StartedAt.Equal(m.initialStartTime) {
 				// Container was restarted (has a different start time)
 				restartErr := NewContainerError(
-					ErrContainerExited,
+					ErrContainerRestarted,
 					m.containerName,
 					fmt.Sprintf("Container %s was restarted (start time changed from %s to %s)",
 						m.containerName, m.initialStartTime.Format(time.RFC3339), info.StartedAt.Format(time.RFC3339)),

pkg/container/runtime/monitor_test.go

@@ -262,8 +262,7 @@ func TestWorkloadMonitor_ContainerRestarted(t *testing.T) {
 	select {
 	case exitErr := <-ch:
 		require.Error(t, exitErr)
-		assert.ErrorIs(t, exitErr, runtime.ErrContainerExited)
-		assert.Contains(t, exitErr.Error(), "restarted")
+		assert.ErrorIs(t, exitErr, runtime.ErrContainerRestarted)
 	case <-time.After(15 * time.Second):
 		t.Fatal("timed out waiting for container restart error")
 	}

pkg/transport/http.go

@@ -80,8 +80,9 @@ type HTTPTransport struct {
 	shutdownCh chan struct{}
 
 	// Container monitor
-	monitor rt.Monitor
-	errorCh <-chan error
+	monitor        rt.Monitor
+	monitorRuntime rt.Runtime // Stored for monitor reconnection on container restart
+	errorCh        <-chan error
 
 	// Container exit error (for determining if restart is needed)
 	containerExitErr error
@@ -329,6 +330,7 @@ func (t *HTTPTransport) Start(ctx context.Context) error {
 	if err != nil {
 		return fmt.Errorf("failed to create container monitor: %w", err)
 	}
+	t.monitorRuntime = monitorRuntime // Store for reconnection
 	t.monitor = container.NewMonitor(monitorRuntime, t.containerName)
 
 	// Start monitoring the container
@@ -348,8 +350,15 @@ func (t *HTTPTransport) Stop(ctx context.Context) error {
 	t.mutex.Lock()
 	defer t.mutex.Unlock()
 
-	// Signal shutdown
-	close(t.shutdownCh)
+	// Signal shutdown (guard against double-close if Stop is called
+	// both from handleContainerExit and externally by the runner)
+	select {
+	case <-t.shutdownCh:
+		// Already closed/stopping
+		return nil
+	default:
+		close(t.shutdownCh)
+	}
 
 	// For remote MCP servers, we don't need container monitoring
 	if t.remoteURL == "" {
@@ -378,39 +387,89 @@ func (t *HTTPTransport) Stop(ctx context.Context) error {
 }
 
 // handleContainerExit handles container exit events.
+// It loops to support reconnecting the monitor when a container is restarted
+// by Docker (e.g., via restart policy) rather than truly exiting.
 func (t *HTTPTransport) handleContainerExit(ctx context.Context) {
-	select {
-	case <-ctx.Done():
-		return
-	case err := <-t.errorCh:
-		// Store the exit error so runner can check if restart is needed
-		t.exitErrMutex.Lock()
-		t.containerExitErr = err
-		t.exitErrMutex.Unlock()
-
-		//nolint:gosec // G706: logging container name from config
-		slog.Warn("container exited", "container", t.containerName, "error", err)
+	for {
+		select {
+		case <-ctx.Done():
+			return
+		case <-t.shutdownCh:
+			return
+		case err := <-t.errorCh:
+			t.exitErrMutex.Lock()
+			t.containerExitErr = err
+			t.exitErrMutex.Unlock()
+
+			if errors.Is(err, rt.ErrContainerRestarted) {
+				//nolint:gosec // G706: logging container name from config
+				slog.Debug("container was restarted by Docker, reconnecting monitor",
+					"container", t.containerName)
+				if reconnectErr := t.reconnectMonitor(ctx); reconnectErr != nil {
+					//nolint:gosec // G706: logging container name from config
+					slog.Error("failed to reconnect monitor, stopping transport",
+						"container", t.containerName, "error", reconnectErr)
+				} else {
+					t.exitErrMutex.Lock()
+					t.containerExitErr = nil
+					t.exitErrMutex.Unlock()
+					continue
+				}
+			}
 
-		// Check if container was removed (not just exited) using typed error
-		if errors.Is(err, rt.ErrContainerRemoved) {
-			//nolint:gosec // G706: logging container name from config
-			slog.Debug("container was removed, stopping proxy and cleaning up",
-				"container", t.containerName)
-		} else {
 			//nolint:gosec // G706: logging container name from config
-			slog.Debug("container exited, will attempt automatic restart",
-				"container", t.containerName)
-		}
+			slog.Warn("container exited", "container", t.containerName, "error", err)
+
+			// Check if container was removed (not just exited) using typed error
+			if errors.Is(err, rt.ErrContainerRemoved) {
+				//nolint:gosec // G706: logging container name from config
+				slog.Debug("container was removed, stopping proxy and cleaning up",
+					"container", t.containerName)
+			} else {
+				//nolint:gosec // G706: logging container name from config
+				slog.Debug("container exited, will attempt automatic restart",
+					"container", t.containerName)
+			}
 
-		// Stop the transport when the container exits/removed
-		if stopErr := t.Stop(ctx); stopErr != nil {
-			slog.Error("error stopping transport after container exit", "error", stopErr)
+			// Stop the transport when the container exits/removed
+			if stopErr := t.Stop(ctx); stopErr != nil {
+				slog.Error("error stopping transport after container exit", "error", stopErr)
+			}
+			return
 		}
 	}
 }
 
+// reconnectMonitor stops the current monitor and starts a new one.
+// This is used when a container is restarted by Docker -- the proxy keeps running
+// but the monitor needs to track the new container start time.
+func (t *HTTPTransport) reconnectMonitor(ctx context.Context) error {
+	t.mutex.Lock()
+	defer t.mutex.Unlock()
+
+	// Stop the old monitor (safe even if goroutine already returned)
+	if t.monitor != nil {
+		t.monitor.StopMonitoring()
+	}
+
+	// Create a new monitor that records the current (post-restart) start time
+	t.monitor = container.NewMonitor(t.monitorRuntime, t.containerName)
+
+	// Start monitoring — errorCh is reassigned here, which is safe because
+	// handleContainerExit (the only reader) runs on the same goroutine and
+	// will see the new channel when it re-enters the select after continue.
+	var err error
+	t.errorCh, err = t.monitor.StartMonitoring(ctx)
+	if err != nil {
+		return fmt.Errorf("failed to restart container monitoring: %w", err)
+	}
+
+	return nil
+}
+
 // ShouldRestart returns true if the container exited and should be restarted.
-// Returns false if the container was removed (intentionally deleted).
+// Returns false if the container was removed (intentionally deleted) or
+// restarted by Docker (already running, no ToolHive restart needed).
 func (t *HTTPTransport) ShouldRestart() bool {
 	t.exitErrMutex.Lock()
 	defer t.exitErrMutex.Unlock()
@@ -419,8 +478,9 @@ func (t *HTTPTransport) ShouldRestart() bool {
 		return false // No exit error, normal shutdown
 	}
 
-	// Don't restart if container was removed (use typed error check)
-	return !errors.Is(t.containerExitErr, rt.ErrContainerRemoved)
+	// Don't restart if container was removed or restarted by Docker (use typed error check)
+	return !errors.Is(t.containerExitErr, rt.ErrContainerRemoved) &&
+		!errors.Is(t.containerExitErr, rt.ErrContainerRestarted)
 }
 
 // IsRunning checks if the transport is currently running.

pkg/transport/http_test.go

@@ -37,6 +37,11 @@ func TestHTTPTransport_ShouldRestart(t *testing.T) {
 			exitError:      rt.NewContainerError(rt.ErrContainerRemoved, "test", "Container removed"),
 			expectedResult: false,
 		},
+		{
+			name:           "container restarted by Docker - should not restart",
+			exitError:      rt.NewContainerError(rt.ErrContainerRestarted, "test", "Container restarted"),
+			expectedResult: false,
+		},
 		{
 			name:           "no error - should not restart",
 			exitError:      nil,

pkg/transport/stdio.go

@@ -705,7 +705,8 @@ func (t *StdioTransport) handleContainerExit(ctx context.Context) {
 }
 
 // ShouldRestart returns true if the container exited and should be restarted.
-// Returns false if the container was removed (intentionally deleted).
+// Returns false if the container was removed (intentionally deleted) or
+// restarted by Docker (already running, no ToolHive restart needed).
 func (t *StdioTransport) ShouldRestart() bool {
 	t.exitErrMutex.Lock()
 	defer t.exitErrMutex.Unlock()
@@ -714,6 +715,7 @@ func (t *StdioTransport) ShouldRestart() bool {
 		return false // No exit error, normal shutdown
 	}
 
-	// Don't restart if container was removed (use typed error check)
-	return !errors.Is(t.containerExitErr, rt.ErrContainerRemoved)
+	// Don't restart if container was removed or restarted by Docker (use typed error check)
+	return !errors.Is(t.containerExitErr, rt.ErrContainerRemoved) &&
+		!errors.Is(t.containerExitErr, rt.ErrContainerRestarted)
 }