Merge pull request #53 from bnema/fix/concurrent-deploy-race
fix(container): serialize per-domain deploys to avoid races
3 files · +236 -24
M internal/adapters/in/cli/remote/client.go +1 -1
| @@ -34,7 +34,7 @@ func NewClient(baseURL string, opts ...ClientOption) *Client { | |||
| 34 | 34 | c := &Client{ | |
| 35 | 35 | baseURL: baseURL, | |
| 36 | 36 | httpClient: &http.Client{ | |
| 37 | - | Timeout: 30 * time.Second, | |
| 37 | + | Timeout: 2 * time.Minute, | |
| 38 | 38 | }, | |
| 39 | 39 | } | |
| 40 | 40 | ||
| @@ -34,7 +34,7 @@ func NewClient(baseURL string, opts ...ClientOption) *Client { | |||||
| 34 | c := &Client{ | 34 | c := &Client{ | ||
| 35 | baseURL: baseURL, | 35 | baseURL: baseURL, | ||
| 36 | httpClient: &http.Client{ | 36 | httpClient: &http.Client{ | ||
| 37 | - | Timeout: 30 * time.Second, | 37 | + | Timeout: 2 * time.Minute, |
| 38 | }, | 38 | }, | ||
| 39 | } | 39 | } | ||
| 40 | 40 | ||||
M internal/usecase/container/service.go +105 -23
| @@ -53,6 +53,51 @@ 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 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 | + | // Fast path: honor already-canceled contexts before blocking. | |
| 75 | + | if err := ctx.Err(); err != nil { | |
| 76 | + | return err | |
| 77 | + | } | |
| 78 | + | select { | |
| 79 | + | case <-ctx.Done(): | |
| 80 | + | return ctx.Err() | |
| 81 | + | case <-l.ch: | |
| 82 | + | // Re-check context after acquisition: if canceled while we were | |
| 83 | + | // in the select, release the token so the lock isn't consumed. | |
| 84 | + | if err := ctx.Err(); err != nil { | |
| 85 | + | l.ch <- struct{}{} | |
| 86 | + | return err | |
| 87 | + | } | |
| 88 | + | return nil | |
| 89 | + | } | |
| 90 | + | } | |
| 91 | + | ||
| 92 | + | // Unlock releases the lock. | |
| 93 | + | func (l *domainDeployLock) Unlock() { | |
| 94 | + | select { | |
| 95 | + | case l.ch <- struct{}{}: | |
| 96 | + | default: | |
| 97 | + | // This should never happen if Lock/Unlock are used correctly. | |
| 98 | + | // Panic to detect double-unlocks or other misuse early. | |
| 99 | + | panic("domainDeployLock: double unlock detected") | |
| 100 | + | } | |
| 56 | 101 | } | |
| 57 | 102 | ||
| 58 | 103 | // NewService creates a new container service. | |
| @@ -74,9 +119,61 @@ func NewService( | |||
| 74 | 119 | } | |
| 75 | 120 | } | |
| 76 | 121 | ||
| 122 | + | // domainDeployMu returns a per-domain lock for serializing deploy operations. | |
| 123 | + | func (s *Service) domainDeployMu(domain string) *domainDeployLock { | |
| 124 | + | v, _ := s.deployMu.LoadOrStore(domain, newDomainDeployLock()) | |
| 125 | + | // Type assertion is safe: LoadOrStore always stores *domainDeployLock values, | |
| 126 | + | // and any concurrent LoadOrStore calls will also store *domainDeployLock. | |
| 127 | + | return v.(*domainDeployLock) | |
| 128 | + | } | |
| 129 | + | ||
| 130 | + | // acquireDomainDeployLock acquires the per-domain deploy lock with context cancellation support. | |
| 131 | + | // Returns an unlock function that must be called to release the lock. | |
| 132 | + | func (s *Service) acquireDomainDeployLock(ctx context.Context, domain string) (func(), error) { | |
| 133 | + | lock := s.domainDeployMu(domain) | |
| 134 | + | if err := lock.Lock(ctx); err != nil { | |
| 135 | + | return nil, err | |
| 136 | + | } | |
| 137 | + | return func() { lock.Unlock() }, nil | |
| 138 | + | } | |
| 139 | + | ||
| 140 | + | // buildContainerConfig constructs the container configuration for deployment. | |
| 141 | + | func (s *Service) buildContainerConfig(containerDomain, image, actualImageRef string, exposedPorts []int, envVars []string, volumes map[string]string, networkName string, hasExisting bool) *domain.ContainerConfig { | |
| 142 | + | // Determine container name (use temp suffix for zero-downtime if existing) | |
| 143 | + | containerName := fmt.Sprintf("gordon-%s", containerDomain) | |
| 144 | + | if hasExisting { | |
| 145 | + | containerName = fmt.Sprintf("gordon-%s-new", containerDomain) | |
| 146 | + | } | |
| 147 | + | ||
| 148 | + | return &domain.ContainerConfig{ | |
| 149 | + | Image: actualImageRef, | |
| 150 | + | Name: containerName, | |
| 151 | + | Ports: exposedPorts, | |
| 152 | + | Env: envVars, | |
| 153 | + | Volumes: volumes, | |
| 154 | + | NetworkMode: networkName, | |
| 155 | + | Hostname: containerDomain, | |
| 156 | + | Labels: map[string]string{ | |
| 157 | + | "gordon.domain": containerDomain, | |
| 158 | + | "gordon.image": image, | |
| 159 | + | "gordon.managed": "true", | |
| 160 | + | "gordon.route": containerDomain, | |
| 161 | + | }, | |
| 162 | + | AutoRemove: false, | |
| 163 | + | } | |
| 164 | + | } | |
| 165 | + | ||
| 77 | 166 | // Deploy creates and starts a container for the given route. | |
| 78 | 167 | // Implements zero-downtime deployment: new container starts before old one stops. | |
| 79 | 168 | func (s *Service) Deploy(ctx context.Context, route domain.Route) (*domain.Container, error) { | |
| 169 | + | // Serialize deploys for the same domain to prevent race conditions | |
| 170 | + | // (e.g. multiple image.pushed events + explicit deploy call from CLI). | |
| 171 | + | unlock, err := s.acquireDomainDeployLock(ctx, route.Domain) | |
| 172 | + | if err != nil { | |
| 173 | + | return nil, err | |
| 174 | + | } | |
| 175 | + | defer unlock() | |
| 176 | + | ||
| 80 | 177 | // Enrich context with use case fields for all downstream logs | |
| 81 | 178 | ctx = zerowrap.CtxWithFields(ctx, map[string]any{ | |
| 82 | 179 | zerowrap.FieldLayer: "usecase", | |
| @@ -138,29 +235,8 @@ func (s *Service) Deploy(ctx context.Context, route domain.Route) (*domain.Conta | |||
| 138 | 235 | return nil, err | |
| 139 | 236 | } | |
| 140 | 237 | ||
| 141 | - | // Determine container name (use temp suffix for zero-downtime if existing) | |
| 142 | - | containerName := fmt.Sprintf("gordon-%s", route.Domain) | |
| 143 | - | if hasExisting { | |
| 144 | - | containerName = fmt.Sprintf("gordon-%s-new", route.Domain) | |
| 145 | - | } | |
| 146 | - | ||
| 147 | - | // Create container (use actual image ref from pull, but track original in labels) | |
| 148 | - | containerConfig := &domain.ContainerConfig{ | |
| 149 | - | Image: actualImageRef, | |
| 150 | - | Name: containerName, | |
| 151 | - | Ports: exposedPorts, | |
| 152 | - | Env: envVars, | |
| 153 | - | Volumes: volumes, | |
| 154 | - | NetworkMode: networkName, | |
| 155 | - | Hostname: route.Domain, | |
| 156 | - | Labels: map[string]string{ | |
| 157 | - | "gordon.domain": route.Domain, | |
| 158 | - | "gordon.image": route.Image, | |
| 159 | - | "gordon.managed": "true", | |
| 160 | - | "gordon.route": route.Domain, | |
| 161 | - | }, | |
| 162 | - | AutoRemove: false, | |
| 163 | - | } | |
| 238 | + | // Build container configuration | |
| 239 | + | containerConfig := s.buildContainerConfig(route.Domain, route.Image, actualImageRef, exposedPorts, envVars, volumes, networkName, hasExisting) | |
| 164 | 240 | ||
| 165 | 241 | newContainer, err := s.runtime.CreateContainer(ctx, containerConfig) | |
| 166 | 242 | if err != nil { | |
| @@ -330,6 +406,12 @@ func (s *Service) Remove(ctx context.Context, containerID string, force bool) er | |||
| 330 | 406 | } | |
| 331 | 407 | s.mu.Unlock() | |
| 332 | 408 | ||
| 409 | + | // Note: We intentionally do NOT clean up deployMu entries to avoid a race condition: | |
| 410 | + | // If Remove deletes the mutex entry while a concurrent Deploy is acquiring the lock, | |
| 411 | + | // the Deploy would create a fresh mutex, breaking serialization and allowing concurrent | |
| 412 | + | // deploys to the same domain. The memory footprint is acceptable (one small struct per | |
| 413 | + | // domain ever deployed), and this is the safer choice for correctness. | |
| 414 | + | ||
| 333 | 415 | // Cleanup network | |
| 334 | 416 | if removedDomain != "" && s.config.NetworkIsolation { | |
| 335 | 417 | networkName := s.getNetworkForApp(removedDomain) | |
| @@ -53,6 +53,51 @@ 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 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 | + | // Fast path: honor already-canceled contexts before blocking. | |||
| 75 | + | if err := ctx.Err(); err != nil { | |||
| 76 | + | return err | |||
| 77 | + | } | |||
| 78 | + | select { | |||
| 79 | + | case <-ctx.Done(): | |||
| 80 | + | return ctx.Err() | |||
| 81 | + | case <-l.ch: | |||
| 82 | + | // Re-check context after acquisition: if canceled while we were | |||
| 83 | + | // in the select, release the token so the lock isn't consumed. | |||
| 84 | + | if err := ctx.Err(); err != nil { | |||
| 85 | + | l.ch <- struct{}{} | |||
| 86 | + | return err | |||
| 87 | + | } | |||
| 88 | + | return nil | |||
| 89 | + | } | |||
| 90 | + | } | |||
| 91 | + | ||||
| 92 | + | // Unlock releases the lock. | |||
| 93 | + | func (l *domainDeployLock) Unlock() { | |||
| 94 | + | select { | |||
| 95 | + | case l.ch <- struct{}{}: | |||
| 96 | + | default: | |||
| 97 | + | // This should never happen if Lock/Unlock are used correctly. | |||
| 98 | + | // Panic to detect double-unlocks or other misuse early. | |||
| 99 | + | panic("domainDeployLock: double unlock detected") | |||
| 100 | + | } | |||
| 56 | } | 101 | } | ||
| 57 | 102 | ||||
| 58 | // NewService creates a new container service. | 103 | // NewService creates a new container service. | ||
| @@ -74,9 +119,61 @@ func NewService( | |||||
| 74 | } | 119 | } | ||
| 75 | } | 120 | } | ||
| 76 | 121 | ||||
| 122 | + | // domainDeployMu returns a per-domain lock for serializing deploy operations. | |||
| 123 | + | func (s *Service) domainDeployMu(domain string) *domainDeployLock { | |||
| 124 | + | v, _ := s.deployMu.LoadOrStore(domain, newDomainDeployLock()) | |||
| 125 | + | // Type assertion is safe: LoadOrStore always stores *domainDeployLock values, | |||
| 126 | + | // and any concurrent LoadOrStore calls will also store *domainDeployLock. | |||
| 127 | + | return v.(*domainDeployLock) | |||
| 128 | + | } | |||
| 129 | + | ||||
| 130 | + | // acquireDomainDeployLock acquires the per-domain deploy lock with context cancellation support. | |||
| 131 | + | // Returns an unlock function that must be called to release the lock. | |||
| 132 | + | func (s *Service) acquireDomainDeployLock(ctx context.Context, domain string) (func(), error) { | |||
| 133 | + | lock := s.domainDeployMu(domain) | |||
| 134 | + | if err := lock.Lock(ctx); err != nil { | |||
| 135 | + | return nil, err | |||
| 136 | + | } | |||
| 137 | + | return func() { lock.Unlock() }, nil | |||
| 138 | + | } | |||
| 139 | + | ||||
| 140 | + | // buildContainerConfig constructs the container configuration for deployment. | |||
| 141 | + | func (s *Service) buildContainerConfig(containerDomain, image, actualImageRef string, exposedPorts []int, envVars []string, volumes map[string]string, networkName string, hasExisting bool) *domain.ContainerConfig { | |||
| 142 | + | // Determine container name (use temp suffix for zero-downtime if existing) | |||
| 143 | + | containerName := fmt.Sprintf("gordon-%s", containerDomain) | |||
| 144 | + | if hasExisting { | |||
| 145 | + | containerName = fmt.Sprintf("gordon-%s-new", containerDomain) | |||
| 146 | + | } | |||
| 147 | + | ||||
| 148 | + | return &domain.ContainerConfig{ | |||
| 149 | + | Image: actualImageRef, | |||
| 150 | + | Name: containerName, | |||
| 151 | + | Ports: exposedPorts, | |||
| 152 | + | Env: envVars, | |||
| 153 | + | Volumes: volumes, | |||
| 154 | + | NetworkMode: networkName, | |||
| 155 | + | Hostname: containerDomain, | |||
| 156 | + | Labels: map[string]string{ | |||
| 157 | + | "gordon.domain": containerDomain, | |||
| 158 | + | "gordon.image": image, | |||
| 159 | + | "gordon.managed": "true", | |||
| 160 | + | "gordon.route": containerDomain, | |||
| 161 | + | }, | |||
| 162 | + | AutoRemove: false, | |||
| 163 | + | } | |||
| 164 | + | } | |||
| 165 | + | ||||
| 77 | // Deploy creates and starts a container for the given route. | 166 | // Deploy creates and starts a container for the given route. | ||
| 78 | // Implements zero-downtime deployment: new container starts before old one stops. | 167 | // Implements zero-downtime deployment: new container starts before old one stops. | ||
| 79 | func (s *Service) Deploy(ctx context.Context, route domain.Route) (*domain.Container, error) { | 168 | func (s *Service) Deploy(ctx context.Context, route domain.Route) (*domain.Container, error) { | ||
| 169 | + | // Serialize deploys for the same domain to prevent race conditions | |||
| 170 | + | // (e.g. multiple image.pushed events + explicit deploy call from CLI). | |||
| 171 | + | unlock, err := s.acquireDomainDeployLock(ctx, route.Domain) | |||
| 172 | + | if err != nil { | |||
| 173 | + | return nil, err | |||
| 174 | + | } | |||
| 175 | + | defer unlock() | |||
| 176 | + | ||||
| 80 | // Enrich context with use case fields for all downstream logs | 177 | // Enrich context with use case fields for all downstream logs | ||
| 81 | ctx = zerowrap.CtxWithFields(ctx, map[string]any{ | 178 | ctx = zerowrap.CtxWithFields(ctx, map[string]any{ | ||
| 82 | zerowrap.FieldLayer: "usecase", | 179 | zerowrap.FieldLayer: "usecase", | ||
| @@ -138,29 +235,8 @@ func (s *Service) Deploy(ctx context.Context, route domain.Route) (*domain.Conta | |||||
| 138 | return nil, err | 235 | return nil, err | ||
| 139 | } | 236 | } | ||
| 140 | 237 | ||||
| 141 | - | // Determine container name (use temp suffix for zero-downtime if existing) | 238 | + | // Build container configuration |
| 142 | - | containerName := fmt.Sprintf("gordon-%s", route.Domain) | 239 | + | containerConfig := s.buildContainerConfig(route.Domain, route.Image, actualImageRef, exposedPorts, envVars, volumes, networkName, hasExisting) |
| 143 | - | if hasExisting { | |||
| 144 | - | containerName = fmt.Sprintf("gordon-%s-new", route.Domain) | |||
| 145 | - | } | |||
| 146 | - | ||||
| 147 | - | // Create container (use actual image ref from pull, but track original in labels) | |||
| 148 | - | containerConfig := &domain.ContainerConfig{ | |||
| 149 | - | Image: actualImageRef, | |||
| 150 | - | Name: containerName, | |||
| 151 | - | Ports: exposedPorts, | |||
| 152 | - | Env: envVars, | |||
| 153 | - | Volumes: volumes, | |||
| 154 | - | NetworkMode: networkName, | |||
| 155 | - | Hostname: route.Domain, | |||
| 156 | - | Labels: map[string]string{ | |||
| 157 | - | "gordon.domain": route.Domain, | |||
| 158 | - | "gordon.image": route.Image, | |||
| 159 | - | "gordon.managed": "true", | |||
| 160 | - | "gordon.route": route.Domain, | |||
| 161 | - | }, | |||
| 162 | - | AutoRemove: false, | |||
| 163 | - | } | |||
| 164 | 240 | ||||
| 165 | newContainer, err := s.runtime.CreateContainer(ctx, containerConfig) | 241 | newContainer, err := s.runtime.CreateContainer(ctx, containerConfig) | ||
| 166 | if err != nil { | 242 | if err != nil { | ||
| @@ -330,6 +406,12 @@ func (s *Service) Remove(ctx context.Context, containerID string, force bool) er | |||||
| 330 | } | 406 | } | ||
| 331 | s.mu.Unlock() | 407 | s.mu.Unlock() | ||
| 332 | 408 | ||||
| 409 | + | // Note: We intentionally do NOT clean up deployMu entries to avoid a race condition: | |||
| 410 | + | // If Remove deletes the mutex entry while a concurrent Deploy is acquiring the lock, | |||
| 411 | + | // the Deploy would create a fresh mutex, breaking serialization and allowing concurrent | |||
| 412 | + | // deploys to the same domain. The memory footprint is acceptable (one small struct per | |||
| 413 | + | // domain ever deployed), and this is the safer choice for correctness. | |||
| 414 | + | ||||
| 333 | // Cleanup network | 415 | // Cleanup network | ||
| 334 | if removedDomain != "" && s.config.NetworkIsolation { | 416 | if removedDomain != "" && s.config.NetworkIsolation { | ||
| 335 | networkName := s.getNetworkForApp(removedDomain) | 417 | networkName := s.getNetworkForApp(removedDomain) | ||
M internal/usecase/container/service_test.go +130 -0
| @@ -4,6 +4,7 @@ import ( | |||
| 4 | 4 | "context" | |
| 5 | 5 | "errors" | |
| 6 | 6 | "fmt" | |
| 7 | + | "sync" | |
| 7 | 8 | "testing" | |
| 8 | 9 | "time" | |
| 9 | 10 | ||
| @@ -1417,3 +1418,132 @@ func TestSanitizeName(t *testing.T) { | |||
| 1417 | 1418 | } | |
| 1418 | 1419 | }) | |
| 1419 | 1420 | } | |
| 1421 | + | ||
| 1422 | + | func TestService_Deploy_ConcurrentSameDomain(t *testing.T) { | |
| 1423 | + | // Verify that concurrent Deploy calls for the same domain are serialized | |
| 1424 | + | // and both succeed without container name conflicts. | |
| 1425 | + | runtime := mocks.NewMockContainerRuntime(t) | |
| 1426 | + | envLoader := mocks.NewMockEnvLoader(t) | |
| 1427 | + | eventBus := mocks.NewMockEventPublisher(t) | |
| 1428 | + | ||
| 1429 | + | config := Config{ | |
| 1430 | + | ReadinessDelay: time.Millisecond, | |
| 1431 | + | } | |
| 1432 | + | svc := NewService(runtime, envLoader, eventBus, nil, config) | |
| 1433 | + | ctx := testContext() | |
| 1434 | + | ||
| 1435 | + | route := domain.Route{ | |
| 1436 | + | Domain: "test.example.com", | |
| 1437 | + | Image: "myapp:latest", | |
| 1438 | + | } | |
| 1439 | + | ||
| 1440 | + | // Track call order to verify serialization | |
| 1441 | + | var callOrder []string | |
| 1442 | + | var callMu sync.Mutex | |
| 1443 | + | ||
| 1444 | + | // Setup mocks that will be called by both deploys sequentially. | |
| 1445 | + | // First deploy: no existing container → creates gordon-test.example.com | |
| 1446 | + | // Second deploy: sees first container → creates gordon-test.example.com-new | |
| 1447 | + | ||
| 1448 | + | runtime.EXPECT().ListContainers(mock.Anything, true).Return([]*domain.Container{}, nil).Times(2) | |
| 1449 | + | runtime.EXPECT().ListImages(mock.Anything).Return([]string{"myapp:latest"}, nil).Times(2) | |
| 1450 | + | runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:latest").Return([]int{8080}, nil).Times(2) | |
| 1451 | + | envLoader.EXPECT().LoadEnv(mock.Anything, "test.example.com").Return(nil, nil).Times(2) | |
| 1452 | + | runtime.EXPECT().InspectImageEnv(mock.Anything, "myapp:latest").Return(nil, nil).Times(2) | |
| 1453 | + | ||
| 1454 | + | createCall := 0 | |
| 1455 | + | runtime.EXPECT().CreateContainer(mock.Anything, mock.AnythingOfType("*domain.ContainerConfig")). | |
| 1456 | + | RunAndReturn(func(_ context.Context, cfg *domain.ContainerConfig) (*domain.Container, error) { | |
| 1457 | + | callMu.Lock() | |
| 1458 | + | createCall++ | |
| 1459 | + | n := createCall | |
| 1460 | + | callOrder = append(callOrder, fmt.Sprintf("create-%d:%s", n, cfg.Name)) | |
| 1461 | + | callMu.Unlock() | |
| 1462 | + | return &domain.Container{ | |
| 1463 | + | ID: fmt.Sprintf("container-%d", n), | |
| 1464 | + | Name: cfg.Name, | |
| 1465 | + | Image: cfg.Image, | |
| 1466 | + | Status: "created", | |
| 1467 | + | }, nil | |
| 1468 | + | }) | |
| 1469 | + | ||
| 1470 | + | runtime.EXPECT().StartContainer(mock.Anything, mock.AnythingOfType("string")).Return(nil).Times(2) | |
| 1471 | + | // IsContainerRunning is called 2x per deploy in waitForReady (initial check + after delay) | |
| 1472 | + | runtime.EXPECT().IsContainerRunning(mock.Anything, mock.AnythingOfType("string")).Return(true, nil).Times(4) | |
| 1473 | + | ||
| 1474 | + | runtime.EXPECT().InspectContainer(mock.Anything, mock.AnythingOfType("string")). | |
| 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 | + | } | |
| 1483 | + | return &domain.Container{ | |
| 1484 | + | ID: id, | |
| 1485 | + | Name: name, | |
| 1486 | + | Image: "myapp:latest", | |
| 1487 | + | Status: "running", | |
| 1488 | + | Ports: []int{8080}, | |
| 1489 | + | }, nil | |
| 1490 | + | }) | |
| 1491 | + | ||
| 1492 | + | eventBus.EXPECT().Publish(domain.EventContainerDeployed, mock.Anything).Return(nil).Times(2) | |
| 1493 | + | ||
| 1494 | + | // The second deploy will see the first container and do zero-downtime swap, | |
| 1495 | + | // which means it will also stop+remove+rename the old container. | |
| 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() | |
| 1499 | + | ||
| 1500 | + | // Launch two concurrent deploys | |
| 1501 | + | var wg sync.WaitGroup | |
| 1502 | + | errs := make([]error, 2) | |
| 1503 | + | ||
| 1504 | + | wg.Add(2) | |
| 1505 | + | for i := range 2 { | |
| 1506 | + | go func(idx int) { | |
| 1507 | + | defer wg.Done() | |
| 1508 | + | _, errs[idx] = svc.Deploy(ctx, route) | |
| 1509 | + | }(i) | |
| 1510 | + | } | |
| 1511 | + | wg.Wait() | |
| 1512 | + | ||
| 1513 | + | // Both should succeed (second deploy waits for the first to finish) | |
| 1514 | + | assert.NoError(t, errs[0], "first deploy should succeed") | |
| 1515 | + | assert.NoError(t, errs[1], "second deploy should succeed") | |
| 1516 | + | ||
| 1517 | + | // Verify creates were serialized (not interleaved) | |
| 1518 | + | callMu.Lock() | |
| 1519 | + | assert.Len(t, callOrder, 2, "should have exactly 2 create calls") | |
| 1520 | + | // First deploy uses canonical name, second uses -new suffix (zero-downtime) | |
| 1521 | + | assert.Equal(t, "create-1:gordon-test.example.com", callOrder[0], "first create should use canonical name") | |
| 1522 | + | assert.Contains(t, callOrder[1], "gordon-test.example.com-new", "second create should use -new suffix") | |
| 1523 | + | callMu.Unlock() | |
| 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 | + | } | |
| @@ -4,6 +4,7 @@ import ( | |||||
| 4 | "context" | 4 | "context" | ||
| 5 | "errors" | 5 | "errors" | ||
| 6 | "fmt" | 6 | "fmt" | ||
| 7 | + | "sync" | |||
| 7 | "testing" | 8 | "testing" | ||
| 8 | "time" | 9 | "time" | ||
| 9 | 10 | ||||
| @@ -1417,3 +1418,132 @@ func TestSanitizeName(t *testing.T) { | |||||
| 1417 | } | 1418 | } | ||
| 1418 | }) | 1419 | }) | ||
| 1419 | } | 1420 | } | ||
| 1421 | + | ||||
| 1422 | + | func TestService_Deploy_ConcurrentSameDomain(t *testing.T) { | |||
| 1423 | + | // Verify that concurrent Deploy calls for the same domain are serialized | |||
| 1424 | + | // and both succeed without container name conflicts. | |||
| 1425 | + | runtime := mocks.NewMockContainerRuntime(t) | |||
| 1426 | + | envLoader := mocks.NewMockEnvLoader(t) | |||
| 1427 | + | eventBus := mocks.NewMockEventPublisher(t) | |||
| 1428 | + | ||||
| 1429 | + | config := Config{ | |||
| 1430 | + | ReadinessDelay: time.Millisecond, | |||
| 1431 | + | } | |||
| 1432 | + | svc := NewService(runtime, envLoader, eventBus, nil, config) | |||
| 1433 | + | ctx := testContext() | |||
| 1434 | + | ||||
| 1435 | + | route := domain.Route{ | |||
| 1436 | + | Domain: "test.example.com", | |||
| 1437 | + | Image: "myapp:latest", | |||
| 1438 | + | } | |||
| 1439 | + | ||||
| 1440 | + | // Track call order to verify serialization | |||
| 1441 | + | var callOrder []string | |||
| 1442 | + | var callMu sync.Mutex | |||
| 1443 | + | ||||
| 1444 | + | // Setup mocks that will be called by both deploys sequentially. | |||
| 1445 | + | // First deploy: no existing container → creates gordon-test.example.com | |||
| 1446 | + | // Second deploy: sees first container → creates gordon-test.example.com-new | |||
| 1447 | + | ||||
| 1448 | + | runtime.EXPECT().ListContainers(mock.Anything, true).Return([]*domain.Container{}, nil).Times(2) | |||
| 1449 | + | runtime.EXPECT().ListImages(mock.Anything).Return([]string{"myapp:latest"}, nil).Times(2) | |||
| 1450 | + | runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:latest").Return([]int{8080}, nil).Times(2) | |||
| 1451 | + | envLoader.EXPECT().LoadEnv(mock.Anything, "test.example.com").Return(nil, nil).Times(2) | |||
| 1452 | + | runtime.EXPECT().InspectImageEnv(mock.Anything, "myapp:latest").Return(nil, nil).Times(2) | |||
| 1453 | + | ||||
| 1454 | + | createCall := 0 | |||
| 1455 | + | runtime.EXPECT().CreateContainer(mock.Anything, mock.AnythingOfType("*domain.ContainerConfig")). | |||
| 1456 | + | RunAndReturn(func(_ context.Context, cfg *domain.ContainerConfig) (*domain.Container, error) { | |||
| 1457 | + | callMu.Lock() | |||
| 1458 | + | createCall++ | |||
| 1459 | + | n := createCall | |||
| 1460 | + | callOrder = append(callOrder, fmt.Sprintf("create-%d:%s", n, cfg.Name)) | |||
| 1461 | + | callMu.Unlock() | |||
| 1462 | + | return &domain.Container{ | |||
| 1463 | + | ID: fmt.Sprintf("container-%d", n), | |||
| 1464 | + | Name: cfg.Name, | |||
| 1465 | + | Image: cfg.Image, | |||
| 1466 | + | Status: "created", | |||
| 1467 | + | }, nil | |||
| 1468 | + | }) | |||
| 1469 | + | ||||
| 1470 | + | runtime.EXPECT().StartContainer(mock.Anything, mock.AnythingOfType("string")).Return(nil).Times(2) | |||
| 1471 | + | // IsContainerRunning is called 2x per deploy in waitForReady (initial check + after delay) | |||
| 1472 | + | runtime.EXPECT().IsContainerRunning(mock.Anything, mock.AnythingOfType("string")).Return(true, nil).Times(4) | |||
| 1473 | + | ||||
| 1474 | + | runtime.EXPECT().InspectContainer(mock.Anything, mock.AnythingOfType("string")). | |||
| 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 | + | } | |||
| 1483 | + | return &domain.Container{ | |||
| 1484 | + | ID: id, | |||
| 1485 | + | Name: name, | |||
| 1486 | + | Image: "myapp:latest", | |||
| 1487 | + | Status: "running", | |||
| 1488 | + | Ports: []int{8080}, | |||
| 1489 | + | }, nil | |||
| 1490 | + | }) | |||
| 1491 | + | ||||
| 1492 | + | eventBus.EXPECT().Publish(domain.EventContainerDeployed, mock.Anything).Return(nil).Times(2) | |||
| 1493 | + | ||||
| 1494 | + | // The second deploy will see the first container and do zero-downtime swap, | |||
| 1495 | + | // which means it will also stop+remove+rename the old container. | |||
| 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() | |||
| 1499 | + | ||||
| 1500 | + | // Launch two concurrent deploys | |||
| 1501 | + | var wg sync.WaitGroup | |||
| 1502 | + | errs := make([]error, 2) | |||
| 1503 | + | ||||
| 1504 | + | wg.Add(2) | |||
| 1505 | + | for i := range 2 { | |||
| 1506 | + | go func(idx int) { | |||
| 1507 | + | defer wg.Done() | |||
| 1508 | + | _, errs[idx] = svc.Deploy(ctx, route) | |||
| 1509 | + | }(i) | |||
| 1510 | + | } | |||
| 1511 | + | wg.Wait() | |||
| 1512 | + | ||||
| 1513 | + | // Both should succeed (second deploy waits for the first to finish) | |||
| 1514 | + | assert.NoError(t, errs[0], "first deploy should succeed") | |||
| 1515 | + | assert.NoError(t, errs[1], "second deploy should succeed") | |||
| 1516 | + | ||||
| 1517 | + | // Verify creates were serialized (not interleaved) | |||
| 1518 | + | callMu.Lock() | |||
| 1519 | + | assert.Len(t, callOrder, 2, "should have exactly 2 create calls") | |||
| 1520 | + | // First deploy uses canonical name, second uses -new suffix (zero-downtime) | |||
| 1521 | + | assert.Equal(t, "create-1:gordon-test.example.com", callOrder[0], "first create should use canonical name") | |||
| 1522 | + | assert.Contains(t, callOrder[1], "gordon-test.example.com-new", "second create should use -new suffix") | |||
| 1523 | + | callMu.Unlock() | |||
| 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 | + | } | |||