Fix concurrent isolated-startup races (network create + ingress DNS latch)

pkg/container/docker/client.go

@@ -115,6 +115,7 @@ type deployOps interface {
 		portBindings map[string][]runtime.PortBinding,
 		isolateNetwork bool,
 	) error
+	getContainerNetworkIP(ctx context.Context, containerName, networkName string) (string, error)
 }
 
 // Client implements the Deployer interface for Docker (and compatible runtimes)
@@ -323,9 +324,17 @@ func (c *Client) DeployWorkload(
 		return 0, err // extractFirstPort already wraps the error with context.
 	}
 	if effectiveIsolation {
-		// SetupIngress runs after createMcpContainer so the reverse-proxy upstream
-		// resolves on first probe (a squid ingress created earlier would cache the
-		// negative DNS lookup and never recover).
+		// Point the ingress reverse proxy at the MCP container's IP rather than
+		// its name. Ordering ingress after createMcpContainer is not enough: under
+		// concurrent startup the container's DNS record can still lag, and the
+		// squid cache_peer caches the failed lookup and never recovers within the
+		// readiness window (see #6063). An IP has no lookup, so a transient
+		// connection failure is retried instead of latching the peer dead.
+		upstreamIP, ipErr := c.ops.getContainerNetworkIP(ctx, name, networkName)
+		if ipErr != nil {
+			return 0, fmt.Errorf("failed to resolve MCP container IP for ingress: %w", ipErr)
+		}
+		pspec.UpstreamHost = upstreamIP
 		hostPort, err = c.proxy.SetupIngress(ctx, pspec, egress)
 		if err != nil {
 			return 0, fmt.Errorf("failed to set up ingress proxy: %w", err)
@@ -1184,11 +1193,38 @@ func (c *Client) createNetwork(
 
 	_, err = c.client.NetworkCreate(ctx, name, networkCreate)
 	if err != nil {
+		// The existence check above is not atomic with the create: another
+		// process (e.g. a second `thv run` sharing the "toolhive-external"
+		// network) can create the same network in between. Docker rejects a
+		// duplicate name with a conflict error, so treat that as success — the
+		// network is already in the state we wanted. Without this, workloads
+		// that start concurrently race here and all but one fail to launch.
+		if errdefs.IsConflict(err) {
+			return nil
+		}
 		return err
 	}
 	return nil
 }
 
+// getContainerNetworkIP returns a container's IP address on the named network.
+// Used to give the ingress proxy a DNS-free upstream address (see proxySpec.UpstreamHost).
+func (c *Client) getContainerNetworkIP(ctx context.Context, containerName, networkName string) (string, error) {
+	inspectResult, err := c.client.ContainerInspect(ctx, containerName, mobyclient.ContainerInspectOptions{})
+	if err != nil {
+		return "", fmt.Errorf("failed to inspect container %s: %w", containerName, err)
+	}
+	netSettings, ok := inspectResult.Container.NetworkSettings.Networks[networkName]
+	if !ok {
+		return "", fmt.Errorf("network %s not found in container %s network settings", networkName, containerName)
+	}
+	// EndpointSettings.IPAddress is a netip.Addr; render it to string form.
+	if !netSettings.IPAddress.IsValid() {
+		return "", fmt.Errorf("container %s has no IP address on network %s", containerName, networkName)
+	}
+	return netSettings.IPAddress.String(), nil
+}
+
 // getDockerBridgeGatewayIP returns the gateway IP of the Docker default bridge
 // network by inspecting it at runtime. This handles platform differences:
 // Linux Docker Engine uses 172.17.0.1 by default, while Docker Desktop on macOS
@@ -1636,8 +1672,9 @@ func mergeEnvVars(base, extra map[string]string) map[string]string {
 
 // setupIngressContainer creates the ingress Squid reverse-proxy container for
 // the workload and returns the host-side port it is bound on.
-func (c *Client) setupIngressContainer(ctx context.Context, containerName string, upstreamPort int, attachStdio bool,
-	externalEndpointsConfig map[string]*network.EndpointSettings, networkPermissions *permissions.NetworkPermissions) (int, error) {
+func (c *Client) setupIngressContainer(ctx context.Context, containerName, upstreamHost string, upstreamPort int,
+	attachStdio bool, externalEndpointsConfig map[string]*network.EndpointSettings,
+	networkPermissions *permissions.NetworkPermissions) (int, error) {
 	// A random port avoids every same-image workload racing to bind the same
 	// preferred port when starting concurrently (see #6063).
 	squidPort, err := networking.FindOrUsePort(0)
@@ -1660,6 +1697,7 @@ func (c *Client) setupIngressContainer(ctx context.Context, containerName string
 		ctx,
 		c,
 		containerName,
+		upstreamHost,
 		ingressContainerName,
 		attachStdio,
 		upstreamPort,

pkg/container/docker/client_deploy_test.go

@@ -32,6 +32,11 @@ type fakeDeployOps struct {
 	dnsID     string
 	dnsIP     string
 
+	// mcpIP is returned by getContainerNetworkIP as the MCP container's IP that
+	// the ingress proxy targets. errMcpIP injects a lookup failure.
+	mcpIP    string
+	errMcpIP error
+
 	mcpCalled        bool
 	mcpName          string
 	mcpNetworkName   string
@@ -75,6 +80,10 @@ func (f *fakeDeployOps) createDnsContainer(_ context.Context, _ string, _ bool,
 	return f.dnsID, f.dnsIP, f.errDNS
 }
 
+func (f *fakeDeployOps) getContainerNetworkIP(_ context.Context, _, _ string) (string, error) {
+	return f.mcpIP, f.errMcpIP
+}
+
 func (f *fakeDeployOps) createMcpContainer(
 	_ context.Context,
 	name string,
@@ -234,6 +243,7 @@ func TestDeployWorkload_SSE_IsolatedNetwork_ReturnsIngressPortAndPassesDNS(t *te
 
 	fops := &fakeDeployOps{
 		dnsIP: "172.18.0.20",
+		mcpIP: "172.18.0.21",
 	}
 	fproxy := &fakeNetworkProxy{
 		ingressPort: 18081,
@@ -269,6 +279,9 @@ func TestDeployWorkload_SSE_IsolatedNetwork_ReturnsIngressPortAndPassesDNS(t *te
 	assert.True(t, fproxy.setupCalled)
 	// The upstream port should be the first exposed port (8080).
 	assert.Equal(t, 8080, fproxy.capturedSpec.UpstreamPort)
+	// The ingress proxy must target the MCP container's IP (not its name) so it
+	// has no DNS dependency to latch on under concurrent startup (see #6063).
+	assert.Equal(t, "172.18.0.21", fproxy.capturedSpec.UpstreamHost)
 	require.True(t, fops.mcpCalled)
 	assert.Equal(t, "172.18.0.20", fops.mcpAdditionalDNS, "additionalDNS passed to MCP container should come from DNS container IP")
 }

pkg/container/docker/client_network_test.go

@@ -0,0 +1,96 @@
+// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package docker
+
+import (
+	"context"
+	"fmt"
+	"os"
+	"sync"
+	"testing"
+
+	mobyclient "github.com/moby/moby/client"
+	"github.com/stretchr/testify/require"
+
+	"github.com/stacklok/toolhive/pkg/container/docker/sdk"
+)
+
+// TestCreateNetwork_ConcurrentSameName reproduces the shared-network startup
+// race: multiple workloads (e.g. several `thv run` sharing "toolhive-external")
+// call createNetwork for the same name at once. The existence check and the
+// create are not atomic, so without idempotent conflict handling all but one
+// caller fails with "network ... already exists" and its workload never
+// launches. This test drives that exact contention against a real daemon and
+// asserts every caller succeeds with exactly one network created.
+//
+// Skips when Docker is unavailable, matching the real-daemon tests in this
+// package (see envoy_test.go).
+func TestCreateNetwork_ConcurrentSameName(t *testing.T) {
+	t.Parallel()
+
+	ctx := context.Background()
+	mc, _, _, err := sdk.NewDockerClient(ctx)
+	if err != nil {
+		t.Skipf("docker not available; skipping network create race test: %v", err)
+	}
+	c := &Client{client: mc}
+
+	// Unique per test process so parallel runs / leftovers never collide.
+	name := fmt.Sprintf("toolhive-test-netrace-%d", os.Getpid())
+	labels := map[string]string{"toolhive": "true"}
+	t.Cleanup(func() { removeNetworksByName(context.Background(), mc, name) })
+	removeNetworksByName(ctx, mc, name) // start clean
+
+	const workers = 8
+	var wg sync.WaitGroup
+	start := make(chan struct{})
+	errs := make([]error, workers)
+	for i := 0; i < workers; i++ {
+		wg.Add(1)
+		go func(i int) {
+			defer wg.Done()
+			<-start // release all goroutines together to maximize contention
+			errs[i] = c.createNetwork(ctx, name, labels, false)
+		}(i)
+	}
+	close(start)
+	wg.Wait()
+
+	for i, e := range errs {
+		require.NoErrorf(t, e, "worker %d: concurrent createNetwork must treat a lost race as success", i)
+	}
+
+	require.Equal(t, 1, countNetworksByName(ctx, mc, name),
+		"exactly one shared network should exist after the concurrent race")
+}
+
+func countNetworksByName(ctx context.Context, mc *mobyclient.Client, name string) int {
+	networks, err := mc.NetworkList(ctx, mobyclient.NetworkListOptions{
+		Filters: mobyclient.Filters{}.Add("name", name),
+	})
+	if err != nil {
+		return -1
+	}
+	n := 0
+	for _, item := range networks.Items {
+		if item.Name == name {
+			n++
+		}
+	}
+	return n
+}
+
+func removeNetworksByName(ctx context.Context, mc *mobyclient.Client, name string) {
+	networks, err := mc.NetworkList(ctx, mobyclient.NetworkListOptions{
+		Filters: mobyclient.Filters{}.Add("name", name),
+	})
+	if err != nil {
+		return
+	}
+	for _, item := range networks.Items {
+		if item.Name == name {
+			_, _ = mc.NetworkRemove(ctx, item.ID, mobyclient.NetworkRemoveOptions{})
+		}
+	}
+}

pkg/container/docker/envoy.go

@@ -776,6 +776,12 @@ func ingressDomains(_ proxySpec) []string {
 // buildIngressCluster returns the STRICT_DNS upstream cluster for the ingress
 // listener, pointing at spec.WorkloadName:spec.UpstreamPort.
 func buildIngressCluster(spec proxySpec) envoyCluster {
+	// Prefer the resolved upstream IP (no DNS dependency); fall back to the
+	// workload name. See proxySpec.UpstreamHost.
+	upstreamHost := spec.UpstreamHost
+	if upstreamHost == "" {
+		upstreamHost = spec.WorkloadName
+	}
 	return envoyCluster{
 		Name:            ingressClusterName,
 		ConnectTimeout:  "10s",
@@ -790,7 +796,7 @@ func buildIngressCluster(spec proxySpec) envoyCluster {
 							Endpoint: envoyEndpointAddress{
 								Address: envoyAddress{
 									SocketAddress: envoySocketAddress{
-										Address:   spec.WorkloadName,
+										Address:   upstreamHost,
 										PortValue: spec.UpstreamPort,
 									},
 								},

pkg/container/docker/networkproxy.go

@@ -57,6 +57,13 @@ type networkProxy interface {
 type proxySpec struct {
 	// WorkloadName is the base name of the MCP container (e.g. "myserver").
 	WorkloadName string
+	// UpstreamHost is the address the ingress reverse proxy connects to for the
+	// MCP container. It is the container's resolved IP on the internal network
+	// (not its name) so the ingress proxy has no DNS dependency: under
+	// concurrent startup the container's DNS record can lag, and a hostname-based
+	// Squid cache_peer caches the failed lookup and never recovers within the
+	// readiness window (see #6063). Empty falls back to WorkloadName.
+	UpstreamHost string
 	// Permissions holds the network permission profile from the workload's
 	// permission profile, governing what outbound traffic is allowed.
 	Permissions *permissions.NetworkPermissions

pkg/container/docker/squid.go

@@ -42,6 +42,7 @@ func createIngressSquidContainer(
 	ctx context.Context,
 	c *Client,
 	containerName string,
+	upstreamHost string,
 	squidContainerName string,
 	attachStdio bool,
 	upstreamPort int,
@@ -51,7 +52,7 @@ func createIngressSquidContainer(
 	portBindings map[string][]runtime.PortBinding,
 	networkPermissions *permissions.NetworkPermissions,
 ) (string, error) {
-	squidConfPath, err := createTempIngressSquidConf(containerName, upstreamPort, squidPort, networkPermissions)
+	squidConfPath, err := createTempIngressSquidConf(containerName, upstreamHost, upstreamPort, squidPort, networkPermissions)
 	if err != nil {
 		return "", fmt.Errorf("failed to create temporary squid.conf: %w", err)
 	}
@@ -385,8 +386,14 @@ func (s *squidProxy) SetupIngress(ctx context.Context, spec proxySpec, _ egressR
 	if spec.TransportType == "stdio" || spec.UpstreamPort == 0 {
 		return 0, nil
 	}
+	// Prefer the resolved upstream IP so the cache_peer has no DNS dependency;
+	// fall back to the workload name when unset (see proxySpec.UpstreamHost).
+	upstreamHost := spec.UpstreamHost
+	if upstreamHost == "" {
+		upstreamHost = spec.WorkloadName
+	}
 	ingressPort, err := s.client.setupIngressContainer(
-		ctx, spec.WorkloadName, spec.UpstreamPort, spec.AttachStdio, spec.Endpoints, spec.Permissions,
+		ctx, spec.WorkloadName, upstreamHost, spec.UpstreamPort, spec.AttachStdio, spec.Endpoints, spec.Permissions,
 	)
 	if err != nil {
 		return 0, err
@@ -396,6 +403,7 @@ func (s *squidProxy) SetupIngress(ctx context.Context, spec proxySpec, _ egressR
 
 func createTempIngressSquidConf(
 	serverHostname string,
+	upstreamHost string,
 	upstreamPort int,
 	squidPort int,
 	networkPermissions *permissions.NetworkPermissions,
@@ -404,7 +412,7 @@ func createTempIngressSquidConf(
 
 	writeCommonConfig(&sb, serverHostname, proxyIngress)
 
-	writeIngressProxyConfig(&sb, serverHostname, upstreamPort, squidPort, networkPermissions)
+	writeIngressProxyConfig(&sb, serverHostname, upstreamHost, upstreamPort, squidPort, networkPermissions)
 	sb.WriteString("http_access deny all\n")
 
 	tmpFile, err := os.CreateTemp("", "squid-*.conf")
@@ -433,21 +441,25 @@ func createTempIngressSquidConf(
 func writeIngressProxyConfig(
 	sb *strings.Builder,
 	serverHostname string,
+	upstreamHost string,
 	upstreamPort int,
 	squidPort int,
 	networkPermissions *permissions.NetworkPermissions,
 ) {
 	portNum := strconv.Itoa(upstreamPort)
 	squidPortNum := strconv.Itoa(squidPort)
-	// standby=2 keeps warm idle connections open to the upstream so the first
-	// request after a cold start (notably a long-lived GET SSE stream that a
-	// server-initiated request rides on) is forwarded without paying inline DNS
-	// + TCP connect latency. Without it, the cold first GET races behind a
-	// later POST that reuses a warmed path, reordering server->client streams.
+	// cache_peer targets upstreamHost (the MCP container's IP), not its name, so
+	// the peer has no DNS lookup to cache-and-latch on a cold-start miss (#6063).
+	// defaultsite keeps the name for the origin Host header. standby=2 keeps warm
+	// idle connections open to the upstream so the first request after a cold
+	// start (notably a long-lived GET SSE stream that a server-initiated request
+	// rides on) is forwarded without paying inline TCP connect latency. Without
+	// it, the cold first GET races behind a later POST that reuses a warmed path,
+	// reordering server->client streams.
 	sb.WriteString(
 		"\n# Reverse proxy setup for port " + portNum + "\n" +
 			"http_port 0.0.0.0:" + squidPortNum + " accel defaultsite=" + serverHostname + "\n" +
-			"cache_peer " + serverHostname + " parent " + portNum + " 0 no-query originserver name=origin_" +
+			"cache_peer " + upstreamHost + " parent " + portNum + " 0 no-query originserver name=origin_" +
 			portNum + " connect-timeout=5 connect-fail-limit=5 standby=2\n")
 
 	// Check if inbound network permissions are configured