fix(container): implement true context-aware deploy locking

Replace sync.Mutex with channel-based lock that respects context cancellation.
The previous ctx.Err() checks couldn't interrupt blocking Lock() calls, causing
deadlock potential when another deploy held the lock and caller's context expired.

Critical fixes:
- Implement domainDeployLock using buffered channel with select for ctx.Done()
- Remove deployMu.Delete() from Remove to fix serialization race condition
- Add test verifying context cancellation interrupts lock acquisition

The Delete() removal is necessary: deleting the entry while a concurrent Deploy
is acquiring the lock would cause LoadOrStore to create a fresh mutex, breaking
serialization and allowing concurrent deploys to the same domain. Memory growth
is acceptable (one small struct per domain ever deployed).

All tests pass, lint clean (0 issues).

commit: 3a7d01278185236522ed1b6767ad792691ed8413
author: brice · 8mo ago · 2026-02-05 18:09 UTC
parent: 1b6cd1f
signature: unsigned

2 files · +74 -25

M internal/usecase/container/service.go +49 -25


@@ -53,7 +53,41 @@ type Service struct {
53	53		containers map[string]*domain.Container
54	54		attachments map[string][]string // ownerDomain → []containerIDs
55	55		mu sync.RWMutex
56		-	deployMu sync.Map // per-domain deploy mutexes (domain → *sync.Mutex)
	56	+	deployMu sync.Map // per-domain deploy locks (domain → *domainDeployLock)
	57	+	}
	58	+
	59	+	// domainDeployLock is a context-aware mutex using a buffered channel.
	60	+	// It allows Lock() to be interrupted by context cancellation.
	61	+	type domainDeployLock struct {
	62	+	ch chan struct{}
	63	+	}
	64	+
	65	+	// newDomainDeployLock creates a new lock that is initially unlocked.
	66	+	func newDomainDeployLock() *domainDeployLock {
	67	+	l := &domainDeployLock{ch: make(chan struct{}, 1)}
	68	+	l.ch <- struct{}{}
	69	+	return l
	70	+	}
	71	+
	72	+	// Lock acquires the lock or returns an error if the context is cancelled.
	73	+	func (l *domainDeployLock) Lock(ctx context.Context) error {
	74	+	select {
	75	+	case <-ctx.Done():
	76	+	return ctx.Err()
	77	+	case <-l.ch:
	78	+	return nil
	79	+	}
	80	+	}
	81	+
	82	+	// Unlock releases the lock.
	83	+	func (l *domainDeployLock) Unlock() {
	84	+	select {
	85	+	case l.ch <- struct{}{}:
	86	+	default:
	87	+	// This should never happen if Lock/Unlock are used correctly.
	88	+	// Panic to detect double-unlocks or other misuse early.
	89	+	panic("domainDeployLock: double unlock detected")
	90	+	}
57	91		}
58	92
59	93		// NewService creates a new container service.

@@ -75,33 +109,22 @@ func NewService(
75	109		}
76	110		}
77	111
78		-	// domainDeployMu returns a per-domain mutex for serializing deploy operations.
79		-	func (s Service) domainDeployMu(domain string) 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.
83		-	return v.(*sync.Mutex)
	112	+	// domainDeployMu returns a per-domain lock for serializing deploy operations.
	113	+	func (s Service) domainDeployMu(domain string) domainDeployLock {
	114	+	v, _ := s.deployMu.LoadOrStore(domain, newDomainDeployLock())
	115	+	// Type assertion is safe: LoadOrStore always stores *domainDeployLock values,
	116	+	// and any concurrent LoadOrStore calls will also store *domainDeployLock.
	117	+	return v.(*domainDeployLock)
84	118		}
85	119
86	120		// acquireDomainDeployLock acquires the per-domain deploy lock with context cancellation support.
87	121		// Returns an unlock function that must be called to release the lock.
88	122		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 {
	123	+	lock := s.domainDeployMu(domain)
	124	+	if err := lock.Lock(ctx); err != nil {
93	125		return nil, err
94	126		}
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
	127	+	return func() { lock.Unlock() }, nil
105	128		}
106	129
107	130		// buildContainerConfig constructs the container configuration for deployment.

@@ -373,10 +396,11 @@ func (s *Service) Remove(ctx context.Context, containerID string, force bool) er
373	396		}
374	397		s.mu.Unlock()
375	398
376		-	// Cleanup per-domain deploy mutex to prevent unbounded growth
377		-	if removedDomain != "" {
378		-	s.deployMu.Delete(removedDomain)
379		-	}
	399	+	// Note: We intentionally do NOT clean up deployMu entries to avoid a race condition:
	400	+	// If Remove deletes the mutex entry while a concurrent Deploy is acquiring the lock,
	401	+	// the Deploy would create a fresh mutex, breaking serialization and allowing concurrent
	402	+	// deploys to the same domain. The memory footprint is acceptable (one small struct per
	403	+	// domain ever deployed), and this is the safer choice for correctness.
380	404
381	405		// Cleanup network
382	406		if removedDomain != "" && s.config.NetworkIsolation {


@@ -53,7 +53,41 @@ type Service struct {
53		containers map[string]*domain.Container	53		containers map[string]*domain.Container
54		attachments map[string][]string // ownerDomain → []containerIDs	54		attachments map[string][]string // ownerDomain → []containerIDs
55		mu sync.RWMutex	55		mu sync.RWMutex
56	-	deployMu sync.Map // per-domain deploy mutexes (domain → *sync.Mutex)	56	+	deployMu sync.Map // per-domain deploy locks (domain → *domainDeployLock)
			57	+	}
			58	+
			59	+	// domainDeployLock is a context-aware mutex using a buffered channel.
			60	+	// It allows Lock() to be interrupted by context cancellation.
			61	+	type domainDeployLock struct {
			62	+	ch chan struct{}
			63	+	}
			64	+
			65	+	// newDomainDeployLock creates a new lock that is initially unlocked.
			66	+	func newDomainDeployLock() *domainDeployLock {
			67	+	l := &domainDeployLock{ch: make(chan struct{}, 1)}
			68	+	l.ch <- struct{}{}
			69	+	return l
			70	+	}
			71	+
			72	+	// Lock acquires the lock or returns an error if the context is cancelled.
			73	+	func (l *domainDeployLock) Lock(ctx context.Context) error {
			74	+	select {
			75	+	case <-ctx.Done():
			76	+	return ctx.Err()
			77	+	case <-l.ch:
			78	+	return nil
			79	+	}
			80	+	}
			81	+
			82	+	// Unlock releases the lock.
			83	+	func (l *domainDeployLock) Unlock() {
			84	+	select {
			85	+	case l.ch <- struct{}{}:
			86	+	default:
			87	+	// This should never happen if Lock/Unlock are used correctly.
			88	+	// Panic to detect double-unlocks or other misuse early.
			89	+	panic("domainDeployLock: double unlock detected")
			90	+	}
57		}	91		}
58			92
59		// NewService creates a new container service.	93		// NewService creates a new container service.

@@ -75,33 +109,22 @@ func NewService(
75		}	109		}
76		}	110		}
77			111
78	-	// domainDeployMu returns a per-domain mutex for serializing deploy operations.	112	+	// domainDeployMu returns a per-domain lock for serializing deploy operations.
79	-	func (s Service) domainDeployMu(domain string) sync.Mutex {	113	+	func (s Service) domainDeployMu(domain string) domainDeployLock {
80	-	v, _ := s.deployMu.LoadOrStore(domain, &sync.Mutex{})	114	+	v, _ := s.deployMu.LoadOrStore(domain, newDomainDeployLock())
81	-	// Type assertion is safe: LoadOrStore always stores *sync.Mutex values,	115	+	// Type assertion is safe: LoadOrStore always stores *domainDeployLock values,
82	-	// and any concurrent LoadOrStore calls will also store *sync.Mutex.	116	+	// and any concurrent LoadOrStore calls will also store *domainDeployLock.
83	-	return v.(*sync.Mutex)	117	+	return v.(*domainDeployLock)
84		}	118		}
85			119
86		// acquireDomainDeployLock acquires the per-domain deploy lock with context cancellation support.	120		// acquireDomainDeployLock acquires the per-domain deploy lock with context cancellation support.
87		// Returns an unlock function that must be called to release the lock.	121		// Returns an unlock function that must be called to release the lock.
88		func (s *Service) acquireDomainDeployLock(ctx context.Context, domain string) (func(), error) {	122		func (s *Service) acquireDomainDeployLock(ctx context.Context, domain string) (func(), error) {
89	-	mu := s.domainDeployMu(domain)	123	+	lock := s.domainDeployMu(domain)
90	-		124	+	if err := lock.Lock(ctx); err != nil {
91	-	// Check if context is already cancelled before acquiring lock
92	-	if err := ctx.Err(); err != nil {
93		return nil, err	125		return nil, err
94		}	126		}
95	-		127	+	return func() { lock.Unlock() }, nil
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		}	128		}
106			129
107		// buildContainerConfig constructs the container configuration for deployment.	130		// buildContainerConfig constructs the container configuration for deployment.

@@ -373,10 +396,11 @@ func (s *Service) Remove(ctx context.Context, containerID string, force bool) er
373		}	396		}
374		s.mu.Unlock()	397		s.mu.Unlock()
375			398
376	-	// Cleanup per-domain deploy mutex to prevent unbounded growth	399	+	// Note: We intentionally do NOT clean up deployMu entries to avoid a race condition:
377	-	if removedDomain != "" {	400	+	// If Remove deletes the mutex entry while a concurrent Deploy is acquiring the lock,
378	-	s.deployMu.Delete(removedDomain)	401	+	// the Deploy would create a fresh mutex, breaking serialization and allowing concurrent
379	-	}	402	+	// deploys to the same domain. The memory footprint is acceptable (one small struct per
			403	+	// domain ever deployed), and this is the safer choice for correctness.
380			404
381		// Cleanup network	405		// Cleanup network
382		if removedDomain != "" && s.config.NetworkIsolation {	406		if removedDomain != "" && s.config.NetworkIsolation {

M internal/usecase/container/service_test.go +25 -0


@@ -1522,3 +1522,28 @@ func TestService_Deploy_ConcurrentSameDomain(t *testing.T) {
1522	1522		assert.Contains(t, callOrder[1], "gordon-test.example.com-new", "second create should use -new suffix")
1523	1523		callMu.Unlock()
1524	1524		}
	1525	+
	1526	+	func TestService_Deploy_ContextCancellation(t *testing.T) {
	1527	+	// Verify that deploy lock acquisition respects context cancellation.
	1528	+	runtime := mocks.NewMockContainerRuntime(t)
	1529	+	envLoader := mocks.NewMockEnvLoader(t)
	1530	+	eventBus := mocks.NewMockEventPublisher(t)
	1531	+
	1532	+	config := Config{
	1533	+	ReadinessDelay: time.Millisecond,
	1534	+	}
	1535	+	svc := NewService(runtime, envLoader, eventBus, nil, config)
	1536	+
	1537	+	route := domain.Route{
	1538	+	Domain: "test.example.com",
	1539	+	Image: "myapp:latest",
	1540	+	}
	1541	+
	1542	+	// Test context cancelled before lock acquisition.
	1543	+	cancelledCtx, cancel := context.WithCancel(testContext())
	1544	+	cancel() // Cancel immediately
	1545	+
	1546	+	_, err := svc.Deploy(cancelledCtx, route)
	1547	+	assert.Error(t, err, "deploy should fail with cancelled context")
	1548	+	assert.ErrorIs(t, err, context.Canceled, "error should be context.Canceled")
	1549	+	}


@@ -1522,3 +1522,28 @@ func TestService_Deploy_ConcurrentSameDomain(t *testing.T) {
1522	assert.Contains(t, callOrder[1], "gordon-test.example.com-new", "second create should use -new suffix")	1522		assert.Contains(t, callOrder[1], "gordon-test.example.com-new", "second create should use -new suffix")
1523	callMu.Unlock()	1523		callMu.Unlock()
1524	}	1524		}
		1525	+
		1526	+	func TestService_Deploy_ContextCancellation(t *testing.T) {
		1527	+	// Verify that deploy lock acquisition respects context cancellation.
		1528	+	runtime := mocks.NewMockContainerRuntime(t)
		1529	+	envLoader := mocks.NewMockEnvLoader(t)
		1530	+	eventBus := mocks.NewMockEventPublisher(t)
		1531	+
		1532	+	config := Config{
		1533	+	ReadinessDelay: time.Millisecond,
		1534	+	}
		1535	+	svc := NewService(runtime, envLoader, eventBus, nil, config)
		1536	+
		1537	+	route := domain.Route{
		1538	+	Domain: "test.example.com",
		1539	+	Image: "myapp:latest",
		1540	+	}
		1541	+
		1542	+	// Test context cancelled before lock acquisition.
		1543	+	cancelledCtx, cancel := context.WithCancel(testContext())
		1544	+	cancel() // Cancel immediately
		1545	+
		1546	+	_, err := svc.Deploy(cancelledCtx, route)
		1547	+	assert.Error(t, err, "deploy should fail with cancelled context")
		1548	+	assert.ErrorIs(t, err, context.Canceled, "error should be context.Canceled")
		1549	+	}