refactor(container): address PR review feedback

- Extract buildContainerConfig helper to reduce cyclomatic complexity (16→15)
- Add acquireDomainDeployLock with context cancellation support
- Add deployMu cleanup in Remove to prevent unbounded growth
- Add type assertion safety comment for domainDeployMu
- Fix test assertion: use assert.Equal for exact match
- Fix test mocks: use .Once() instead of .Maybe() for cleanup ops
- Fix InspectContainer mock to return correct container names

All tests pass and lint is clean (0 issues).

commit: 1b6cd1f7fc1a9f1416bc45e69909b722c841701a
author: brice · 8mo ago · 2026-02-05 17:56 UTC
parent: 38e7e8c
signature: unsigned

2 files · +73 -31

M internal/usecase/container/service.go +61 -26


@@ -78,17 +78,68 @@ func NewService(
78	78		// domainDeployMu returns a per-domain mutex for serializing deploy operations.
79	79		func (s Service) domainDeployMu(domain string) sync.Mutex {
80	80		v, _ := s.deployMu.LoadOrStore(domain, &sync.Mutex{})
	81	+	// Type assertion is safe: LoadOrStore always stores *sync.Mutex values,
	82	+	// and any concurrent LoadOrStore calls will also store *sync.Mutex.
81	83		return v.(*sync.Mutex)
82	84		}
83	85
	86	+	// acquireDomainDeployLock acquires the per-domain deploy lock with context cancellation support.
	87	+	// Returns an unlock function that must be called to release the lock.
	88	+	func (s *Service) acquireDomainDeployLock(ctx context.Context, domain string) (func(), error) {
	89	+	mu := s.domainDeployMu(domain)
	90	+
	91	+	// Check if context is already cancelled before acquiring lock
	92	+	if err := ctx.Err(); err != nil {
	93	+	return nil, err
	94	+	}
	95	+
	96	+	mu.Lock()
	97	+
	98	+	// Check if context was cancelled while waiting for lock
	99	+	if err := ctx.Err(); err != nil {
	100	+	mu.Unlock()
	101	+	return nil, err
	102	+	}
	103	+
	104	+	return func() { mu.Unlock() }, nil
	105	+	}
	106	+
	107	+	// buildContainerConfig constructs the container configuration for deployment.
	108	+	func (s Service) buildContainerConfig(containerDomain, image, actualImageRef string, exposedPorts []int, envVars []string, volumes map[string]string, networkName string, hasExisting bool) domain.ContainerConfig {
	109	+	// Determine container name (use temp suffix for zero-downtime if existing)
	110	+	containerName := fmt.Sprintf("gordon-%s", containerDomain)
	111	+	if hasExisting {
	112	+	containerName = fmt.Sprintf("gordon-%s-new", containerDomain)
	113	+	}
	114	+
	115	+	return &domain.ContainerConfig{
	116	+	Image: actualImageRef,
	117	+	Name: containerName,
	118	+	Ports: exposedPorts,
	119	+	Env: envVars,
	120	+	Volumes: volumes,
	121	+	NetworkMode: networkName,
	122	+	Hostname: containerDomain,
	123	+	Labels: map[string]string{
	124	+	"gordon.domain": containerDomain,
	125	+	"gordon.image": image,
	126	+	"gordon.managed": "true",
	127	+	"gordon.route": containerDomain,
	128	+	},
	129	+	AutoRemove: false,
	130	+	}
	131	+	}
	132	+
84	133		// Deploy creates and starts a container for the given route.
85	134		// Implements zero-downtime deployment: new container starts before old one stops.
86	135		func (s Service) Deploy(ctx context.Context, route domain.Route) (domain.Container, error) {
87	136		// Serialize deploys for the same domain to prevent race conditions
88	137		// (e.g. multiple image.pushed events + explicit deploy call from CLI).
89		-	mu := s.domainDeployMu(route.Domain)
90		-	mu.Lock()
91		-	defer mu.Unlock()
	138	+	unlock, err := s.acquireDomainDeployLock(ctx, route.Domain)
	139	+	if err != nil {
	140	+	return nil, err
	141	+	}
	142	+	defer unlock()
92	143
93	144		// Enrich context with use case fields for all downstream logs
94	145		ctx = zerowrap.CtxWithFields(ctx, map[string]any{

@@ -151,29 +202,8 @@ func (s Service) Deploy(ctx context.Context, route domain.Route) (domain.Conta
151	202		return nil, err
152	203		}
153	204
154		-	// Determine container name (use temp suffix for zero-downtime if existing)
155		-	containerName := fmt.Sprintf("gordon-%s", route.Domain)
156		-	if hasExisting {
157		-	containerName = fmt.Sprintf("gordon-%s-new", route.Domain)
158		-	}
159		-
160		-	// Create container (use actual image ref from pull, but track original in labels)
161		-	containerConfig := &domain.ContainerConfig{
162		-	Image: actualImageRef,
163		-	Name: containerName,
164		-	Ports: exposedPorts,
165		-	Env: envVars,
166		-	Volumes: volumes,
167		-	NetworkMode: networkName,
168		-	Hostname: route.Domain,
169		-	Labels: map[string]string{
170		-	"gordon.domain": route.Domain,
171		-	"gordon.image": route.Image,
172		-	"gordon.managed": "true",
173		-	"gordon.route": route.Domain,
174		-	},
175		-	AutoRemove: false,
176		-	}
	205	+	// Build container configuration
	206	+	containerConfig := s.buildContainerConfig(route.Domain, route.Image, actualImageRef, exposedPorts, envVars, volumes, networkName, hasExisting)
177	207
178	208		newContainer, err := s.runtime.CreateContainer(ctx, containerConfig)
179	209		if err != nil {

@@ -343,6 +373,11 @@ func (s *Service) Remove(ctx context.Context, containerID string, force bool) er
343	373		}
344	374		s.mu.Unlock()
345	375
	376	+	// Cleanup per-domain deploy mutex to prevent unbounded growth
	377	+	if removedDomain != "" {
	378	+	s.deployMu.Delete(removedDomain)
	379	+	}
	380	+
346	381		// Cleanup network
347	382		if removedDomain != "" && s.config.NetworkIsolation {
348	383		networkName := s.getNetworkForApp(removedDomain)


@@ -78,17 +78,68 @@ func NewService(
78		// domainDeployMu returns a per-domain mutex for serializing deploy operations.	78		// domainDeployMu returns a per-domain mutex for serializing deploy operations.
79		func (s Service) domainDeployMu(domain string) sync.Mutex {	79		func (s Service) domainDeployMu(domain string) sync.Mutex {
80		v, _ := s.deployMu.LoadOrStore(domain, &sync.Mutex{})	80		v, _ := s.deployMu.LoadOrStore(domain, &sync.Mutex{})
			81	+	// Type assertion is safe: LoadOrStore always stores *sync.Mutex values,
			82	+	// and any concurrent LoadOrStore calls will also store *sync.Mutex.
81		return v.(*sync.Mutex)	83		return v.(*sync.Mutex)
82		}	84		}
83			85
			86	+	// acquireDomainDeployLock acquires the per-domain deploy lock with context cancellation support.
			87	+	// Returns an unlock function that must be called to release the lock.
			88	+	func (s *Service) acquireDomainDeployLock(ctx context.Context, domain string) (func(), error) {
			89	+	mu := s.domainDeployMu(domain)
			90	+
			91	+	// Check if context is already cancelled before acquiring lock
			92	+	if err := ctx.Err(); err != nil {
			93	+	return nil, err
			94	+	}
			95	+
			96	+	mu.Lock()
			97	+
			98	+	// Check if context was cancelled while waiting for lock
			99	+	if err := ctx.Err(); err != nil {
			100	+	mu.Unlock()
			101	+	return nil, err
			102	+	}
			103	+
			104	+	return func() { mu.Unlock() }, nil
			105	+	}
			106	+
			107	+	// buildContainerConfig constructs the container configuration for deployment.
			108	+	func (s Service) buildContainerConfig(containerDomain, image, actualImageRef string, exposedPorts []int, envVars []string, volumes map[string]string, networkName string, hasExisting bool) domain.ContainerConfig {
			109	+	// Determine container name (use temp suffix for zero-downtime if existing)
			110	+	containerName := fmt.Sprintf("gordon-%s", containerDomain)
			111	+	if hasExisting {
			112	+	containerName = fmt.Sprintf("gordon-%s-new", containerDomain)
			113	+	}
			114	+
			115	+	return &domain.ContainerConfig{
			116	+	Image: actualImageRef,
			117	+	Name: containerName,
			118	+	Ports: exposedPorts,
			119	+	Env: envVars,
			120	+	Volumes: volumes,
			121	+	NetworkMode: networkName,
			122	+	Hostname: containerDomain,
			123	+	Labels: map[string]string{
			124	+	"gordon.domain": containerDomain,
			125	+	"gordon.image": image,
			126	+	"gordon.managed": "true",
			127	+	"gordon.route": containerDomain,
			128	+	},
			129	+	AutoRemove: false,
			130	+	}
			131	+	}
			132	+
84		// Deploy creates and starts a container for the given route.	133		// Deploy creates and starts a container for the given route.
85		// Implements zero-downtime deployment: new container starts before old one stops.	134		// Implements zero-downtime deployment: new container starts before old one stops.
86		func (s Service) Deploy(ctx context.Context, route domain.Route) (domain.Container, error) {	135		func (s Service) Deploy(ctx context.Context, route domain.Route) (domain.Container, error) {
87		// Serialize deploys for the same domain to prevent race conditions	136		// Serialize deploys for the same domain to prevent race conditions
88		// (e.g. multiple image.pushed events + explicit deploy call from CLI).	137		// (e.g. multiple image.pushed events + explicit deploy call from CLI).
89	-	mu := s.domainDeployMu(route.Domain)	138	+	unlock, err := s.acquireDomainDeployLock(ctx, route.Domain)
90	-	mu.Lock()	139	+	if err != nil {
91	-	defer mu.Unlock()	140	+	return nil, err
			141	+	}
			142	+	defer unlock()
92			143
93		// Enrich context with use case fields for all downstream logs	144		// Enrich context with use case fields for all downstream logs
94		ctx = zerowrap.CtxWithFields(ctx, map[string]any{	145		ctx = zerowrap.CtxWithFields(ctx, map[string]any{

@@ -151,29 +202,8 @@ func (s Service) Deploy(ctx context.Context, route domain.Route) (domain.Conta
151		return nil, err	202		return nil, err
152		}	203		}
153			204
154	-	// Determine container name (use temp suffix for zero-downtime if existing)	205	+	// Build container configuration
155	-	containerName := fmt.Sprintf("gordon-%s", route.Domain)	206	+	containerConfig := s.buildContainerConfig(route.Domain, route.Image, actualImageRef, exposedPorts, envVars, volumes, networkName, hasExisting)
156	-	if hasExisting {
157	-	containerName = fmt.Sprintf("gordon-%s-new", route.Domain)
158	-	}
159	-
160	-	// Create container (use actual image ref from pull, but track original in labels)
161	-	containerConfig := &domain.ContainerConfig{
162	-	Image: actualImageRef,
163	-	Name: containerName,
164	-	Ports: exposedPorts,
165	-	Env: envVars,
166	-	Volumes: volumes,
167	-	NetworkMode: networkName,
168	-	Hostname: route.Domain,
169	-	Labels: map[string]string{
170	-	"gordon.domain": route.Domain,
171	-	"gordon.image": route.Image,
172	-	"gordon.managed": "true",
173	-	"gordon.route": route.Domain,
174	-	},
175	-	AutoRemove: false,
176	-	}
177			207
178		newContainer, err := s.runtime.CreateContainer(ctx, containerConfig)	208		newContainer, err := s.runtime.CreateContainer(ctx, containerConfig)
179		if err != nil {	209		if err != nil {

@@ -343,6 +373,11 @@ func (s *Service) Remove(ctx context.Context, containerID string, force bool) er
343		}	373		}
344		s.mu.Unlock()	374		s.mu.Unlock()
345			375
			376	+	// Cleanup per-domain deploy mutex to prevent unbounded growth
			377	+	if removedDomain != "" {
			378	+	s.deployMu.Delete(removedDomain)
			379	+	}
			380	+
346		// Cleanup network	381		// Cleanup network
347		if removedDomain != "" && s.config.NetworkIsolation {	382		if removedDomain != "" && s.config.NetworkIsolation {
348		networkName := s.getNetworkForApp(removedDomain)	383		networkName := s.getNetworkForApp(removedDomain)

M internal/usecase/container/service_test.go +12 -5


@@ -1473,9 +1473,16 @@ func TestService_Deploy_ConcurrentSameDomain(t *testing.T) {
1473	1473
1474	1474		runtime.EXPECT().InspectContainer(mock.Anything, mock.AnythingOfType("string")).
1475	1475		RunAndReturn(func(_ context.Context, id string) (*domain.Container, error) {
	1476	+	// Return correct container name based on ID
	1477	+	// container-1 is the first deploy (canonical name)
	1478	+	// container-2 is the second deploy (with -new suffix)
	1479	+	name := "gordon-test.example.com"
	1480	+	if id == "container-2" {
	1481	+	name = "gordon-test.example.com-new"
	1482	+	}
1476	1483		return &domain.Container{
1477	1484		ID: id,
1478		-	Name: "gordon-test.example.com",
	1485	+	Name: name,
1479	1486		Image: "myapp:latest",
1480	1487		Status: "running",
1481	1488		Ports: []int{8080},

@@ -1486,9 +1493,9 @@ func TestService_Deploy_ConcurrentSameDomain(t *testing.T) {
1486	1493
1487	1494		// The second deploy will see the first container and do zero-downtime swap,
1488	1495		// which means it will also stop+remove+rename the old container.
1489		-	runtime.EXPECT().StopContainer(mock.Anything, mock.AnythingOfType("string")).Return(nil).Maybe()
1490		-	runtime.EXPECT().RemoveContainer(mock.Anything, mock.AnythingOfType("string"), true).Return(nil).Maybe()
1491		-	runtime.EXPECT().RenameContainer(mock.Anything, mock.AnythingOfType("string"), mock.AnythingOfType("string")).Return(nil).Maybe()
	1496	+	runtime.EXPECT().StopContainer(mock.Anything, mock.AnythingOfType("string")).Return(nil).Once()
	1497	+	runtime.EXPECT().RemoveContainer(mock.Anything, mock.AnythingOfType("string"), true).Return(nil).Once()
	1498	+	runtime.EXPECT().RenameContainer(mock.Anything, mock.AnythingOfType("string"), mock.AnythingOfType("string")).Return(nil).Once()
1492	1499
1493	1500		// Launch two concurrent deploys
1494	1501		var wg sync.WaitGroup

@@ -1511,7 +1518,7 @@ func TestService_Deploy_ConcurrentSameDomain(t *testing.T) {
1511	1518		callMu.Lock()
1512	1519		assert.Len(t, callOrder, 2, "should have exactly 2 create calls")
1513	1520		// First deploy uses canonical name, second uses -new suffix (zero-downtime)
1514		-	assert.Contains(t, callOrder[0], "gordon-test.example.com", "first create should use canonical name")
	1521	+	assert.Equal(t, "create-1:gordon-test.example.com", callOrder[0], "first create should use canonical name")
1515	1522		assert.Contains(t, callOrder[1], "gordon-test.example.com-new", "second create should use -new suffix")
1516	1523		callMu.Unlock()
1517	1524		}

 	runtime.EXPECT().InspectContainer(mock.Anything, mock.AnythingOfType("string")).
 		RunAndReturn(func(_ context.Context, id string) (*domain.Container, error) {
+			// Return correct container name based on ID
+			// container-1 is the first deploy (canonical name)
+			// container-2 is the second deploy (with -new suffix)
+			name := "gordon-test.example.com"
+			if id == "container-2" {
+				name = "gordon-test.example.com-new"
+			}
 			return &domain.Container{
 				ID:     id,
-				Name:   "gordon-test.example.com",
+				Name:   name,
 				Image:  "myapp:latest",
 				Status: "running",
 				Ports:  []int{8080},
 	// The second deploy will see the first container and do zero-downtime swap,
 	// which means it will also stop+remove+rename the old container.
-	runtime.EXPECT().StopContainer(mock.Anything, mock.AnythingOfType("string")).Return(nil).Maybe()
+	runtime.EXPECT().StopContainer(mock.Anything, mock.AnythingOfType("string")).Return(nil).Once()
-	runtime.EXPECT().RemoveContainer(mock.Anything, mock.AnythingOfType("string"), true).Return(nil).Maybe()
+	runtime.EXPECT().RemoveContainer(mock.Anything, mock.AnythingOfType("string"), true).Return(nil).Once()
-	runtime.EXPECT().RenameContainer(mock.Anything, mock.AnythingOfType("string"), mock.AnythingOfType("string")).Return(nil).Maybe()
+	runtime.EXPECT().RenameContainer(mock.Anything, mock.AnythingOfType("string"), mock.AnythingOfType("string")).Return(nil).Once()
 	// Launch two concurrent deploys
 	var wg sync.WaitGroup
 	callMu.Lock()
 	assert.Len(t, callOrder, 2, "should have exactly 2 create calls")
 	// First deploy uses canonical name, second uses -new suffix (zero-downtime)
-	assert.Contains(t, callOrder[0], "gordon-test.example.com", "first create should use canonical name")
+	assert.Equal(t, "create-1:gordon-test.example.com", callOrder[0], "first create should use canonical name")
 	assert.Contains(t, callOrder[1], "gordon-test.example.com-new", "second create should use -new suffix")
 	callMu.Unlock()
 }