detached 6db35a4

fix(deploy): preserve tracked container during orphan cleanup

The orphan cleanup was removing containers by name without checking if
they were already tracked by the service. This caused the running
container to be stopped before the new one was ready, resulting in
~9 seconds of 503 errors during deployments.

Now cleanupOrphanedContainers accepts a skipContainerID parameter to
preserve the currently tracked container. The old container is only
removed after the new one is ready and traffic has been switched.
commit
6db35a418a12eb63112ef8931a7bf5b7cce3fdc4
author
brice · · 2026-01-16 05:26 UTC
parent
511b814
signature
GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email
2 files · +168 -3
Unified
UnifiedSplit
M internal/usecase/container/service.go +8 -3
@@ -84,7 +84,12 @@ func (s *Service) Deploy(ctx context.Context, route domain.Route) (*domain.Conta
8484 s.mu.RUnlock()
8585
8686 // Clean up orphaned containers (containers with same name but not tracked)
87- if err := s.cleanupOrphanedContainers(ctx, route.Domain); err != nil {
87+ // Skip the currently tracked container to preserve zero-downtime deployment
88+ existingID := ""
89+ if hasExisting {
90+ existingID = existing.ID
91+ }
92+ if err := s.cleanupOrphanedContainers(ctx, route.Domain, existingID); err != nil {
8893 log.WrapErr(err, "failed to cleanup orphaned containers")
8994 }
9095
@@ -738,7 +743,7 @@ func (s *Service) createNetworkIfNeeded(ctx context.Context, networkName string)
738743 return nil
739744 }
740745
741-func (s *Service) cleanupOrphanedContainers(ctx context.Context, domainName string) error {
746+func (s *Service) cleanupOrphanedContainers(ctx context.Context, domainName string, skipContainerID string) error {
742747 log := zerowrap.FromCtx(ctx)
743748 expectedName := fmt.Sprintf("gordon-%s", domainName)
744749
@@ -748,7 +753,7 @@ func (s *Service) cleanupOrphanedContainers(ctx context.Context, domainName stri
748753 }
749754
750755 for _, c := range allContainers {
751- if c.Name == expectedName {
756+ if c.Name == expectedName && c.ID != skipContainerID {
752757 log.Info().Str(zerowrap.FieldEntityID, c.ID).Str(zerowrap.FieldStatus, c.Status).Msg("found orphaned container, removing")
753758
754759 if err := s.runtime.StopContainer(ctx, c.ID); err != nil {
@@ -84,7 +84,12 @@ func (s *Service) Deploy(ctx context.Context, route domain.Route) (*domain.Conta
84 s.mu.RUnlock()84 s.mu.RUnlock()
85 85
86 // Clean up orphaned containers (containers with same name but not tracked)86 // Clean up orphaned containers (containers with same name but not tracked)
87- if err := s.cleanupOrphanedContainers(ctx, route.Domain); err != nil {87+ // Skip the currently tracked container to preserve zero-downtime deployment
88+ existingID := ""
89+ if hasExisting {
90+ existingID = existing.ID
91+ }
92+ if err := s.cleanupOrphanedContainers(ctx, route.Domain, existingID); err != nil {
88 log.WrapErr(err, "failed to cleanup orphaned containers")93 log.WrapErr(err, "failed to cleanup orphaned containers")
89 }94 }
90 95
@@ -738,7 +743,7 @@ func (s *Service) createNetworkIfNeeded(ctx context.Context, networkName string)
738 return nil743 return nil
739 }744 }
740 745
741-func (s *Service) cleanupOrphanedContainers(ctx context.Context, domainName string) error {746+func (s *Service) cleanupOrphanedContainers(ctx context.Context, domainName string, skipContainerID string) error {
742 log := zerowrap.FromCtx(ctx)747 log := zerowrap.FromCtx(ctx)
743 expectedName := fmt.Sprintf("gordon-%s", domainName)748 expectedName := fmt.Sprintf("gordon-%s", domainName)
744 749
@@ -748,7 +753,7 @@ func (s *Service) cleanupOrphanedContainers(ctx context.Context, domainName stri
748 }753 }
749 754
750 for _, c := range allContainers {755 for _, c := range allContainers {
751- if c.Name == expectedName {756+ if c.Name == expectedName && c.ID != skipContainerID {
752 log.Info().Str(zerowrap.FieldEntityID, c.ID).Str(zerowrap.FieldStatus, c.Status).Msg("found orphaned container, removing")757 log.Info().Str(zerowrap.FieldEntityID, c.ID).Str(zerowrap.FieldStatus, c.Status).Msg("found orphaned container, removing")
753 758
754 if err := s.runtime.StopContainer(ctx, c.ID); err != nil {759 if err := s.runtime.StopContainer(ctx, c.ID); err != nil {
M internal/usecase/container/service_test.go +160 -0
@@ -895,3 +895,163 @@ func TestService_AutoStart_HandlesDeployErrors(t *testing.T) {
895895 assert.Error(t, err)
896896 assert.Contains(t, err.Error(), "auto-start completed with 1 errors")
897897 }
898+
899+// TestService_Deploy_OrphanCleanupSkipsTrackedContainer verifies that the orphan cleanup
900+// does not remove the currently tracked container during zero-downtime deployment.
901+// This is critical for preventing downtime - the old container must stay running
902+// until the new container is ready and traffic is switched.
903+func TestService_Deploy_OrphanCleanupSkipsTrackedContainer(t *testing.T) {
904+ runtime := mocks.NewMockContainerRuntime(t)
905+ envLoader := mocks.NewMockEnvLoader(t)
906+ eventBus := mocks.NewMockEventPublisher(t)
907+
908+ config := Config{
909+ ReadinessDelay: 0, // No delay for tests
910+ }
911+ svc := NewService(runtime, envLoader, eventBus, nil, config)
912+ ctx := testContext()
913+
914+ // Pre-populate with existing tracked container
915+ existingContainer := &domain.Container{
916+ ID: "tracked-container-123",
917+ Name: "gordon-test.example.com",
918+ Status: "running",
919+ }
920+ svc.containers["test.example.com"] = existingContainer
921+
922+ route := domain.Route{
923+ Domain: "test.example.com",
924+ Image: "myapp:v2",
925+ }
926+
927+ // ListContainers returns the tracked container - this simulates the orphan check
928+ // finding a container with the same name. The bug was that it would remove this
929+ // container BEFORE the new one was ready, causing downtime.
930+ runtime.EXPECT().ListContainers(mock.Anything, true).Return([]*domain.Container{
931+ {
932+ ID: "tracked-container-123",
933+ Name: "gordon-test.example.com",
934+ Status: "running",
935+ },
936+ }, nil)
937+
938+ // Image operations
939+ runtime.EXPECT().ListImages(mock.Anything).Return([]string{}, nil)
940+ runtime.EXPECT().PullImage(mock.Anything, "myapp:v2").Return(nil)
941+ runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:v2").Return([]int{8080}, nil)
942+
943+ // Environment
944+ envLoader.EXPECT().LoadEnv(mock.Anything, "test.example.com").Return([]string{}, nil)
945+ runtime.EXPECT().InspectImageEnv(mock.Anything, "myapp:v2").Return([]string{}, nil)
946+
947+ // Create new container with -new suffix for zero-downtime
948+ newContainer := &domain.Container{ID: "new-container", Name: "gordon-test.example.com-new", Status: "created"}
949+ runtime.EXPECT().CreateContainer(mock.Anything, mock.MatchedBy(func(cfg *domain.ContainerConfig) bool {
950+ return cfg.Name == "gordon-test.example.com-new"
951+ })).Return(newContainer, nil)
952+ runtime.EXPECT().StartContainer(mock.Anything, "new-container").Return(nil)
953+
954+ // Wait for ready
955+ runtime.EXPECT().IsContainerRunning(mock.Anything, "new-container").Return(true, nil).Times(2)
956+
957+ // Inspect after ready
958+ runtime.EXPECT().InspectContainer(mock.Anything, "new-container").Return(&domain.Container{
959+ ID: "new-container",
960+ Status: "running",
961+ }, nil)
962+
963+ // Publish event
964+ eventBus.EXPECT().Publish(domain.EventContainerDeployed, mock.AnythingOfType("*domain.ContainerEventPayload")).Return(nil)
965+
966+ // NOW (after new container is ready) the old container should be stopped and removed
967+ // This is the correct zero-downtime sequence - not during orphan cleanup
968+ runtime.EXPECT().StopContainer(mock.Anything, "tracked-container-123").Return(nil)
969+ runtime.EXPECT().RemoveContainer(mock.Anything, "tracked-container-123", true).Return(nil)
970+
971+ // Rename new container to canonical name
972+ runtime.EXPECT().RenameContainer(mock.Anything, "new-container", "gordon-test.example.com").Return(nil)
973+
974+ result, err := svc.Deploy(ctx, route)
975+
976+ assert.NoError(t, err)
977+ assert.Equal(t, "new-container", result.ID)
978+
979+ // Verify new container is now tracked
980+ tracked, exists := svc.Get(ctx, "test.example.com")
981+ assert.True(t, exists)
982+ assert.Equal(t, "new-container", tracked.ID)
983+}
984+
985+// TestService_Deploy_OrphanCleanupRemovesTrueOrphans verifies that containers
986+// with the same name but NOT tracked are properly removed as orphans.
987+func TestService_Deploy_OrphanCleanupRemovesTrueOrphans(t *testing.T) {
988+ runtime := mocks.NewMockContainerRuntime(t)
989+ envLoader := mocks.NewMockEnvLoader(t)
990+ eventBus := mocks.NewMockEventPublisher(t)
991+
992+ config := Config{
993+ ReadinessDelay: 0, // No delay for tests
994+ }
995+ svc := NewService(runtime, envLoader, eventBus, nil, config)
996+ ctx := testContext()
997+
998+ // NO tracked container - service is empty for this domain
999+
1000+ route := domain.Route{
1001+ Domain: "test.example.com",
1002+ Image: "myapp:v1",
1003+ }
1004+
1005+ // ListContainers returns an orphaned container (same name, but not tracked)
1006+ // This could happen if Gordon crashed and restarted, or container was created manually
1007+ runtime.EXPECT().ListContainers(mock.Anything, true).Return([]*domain.Container{
1008+ {
1009+ ID: "orphan-container",
1010+ Name: "gordon-test.example.com",
1011+ Status: "running",
1012+ },
1013+ }, nil)
1014+
1015+ // Orphan should be stopped and removed BEFORE we proceed
1016+ runtime.EXPECT().StopContainer(mock.Anything, "orphan-container").Return(nil)
1017+ runtime.EXPECT().RemoveContainer(mock.Anything, "orphan-container", true).Return(nil)
1018+
1019+ // Image operations
1020+ runtime.EXPECT().ListImages(mock.Anything).Return([]string{}, nil)
1021+ runtime.EXPECT().PullImage(mock.Anything, "myapp:v1").Return(nil)
1022+ runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:v1").Return([]int{8080}, nil)
1023+
1024+ // Environment
1025+ envLoader.EXPECT().LoadEnv(mock.Anything, "test.example.com").Return([]string{}, nil)
1026+ runtime.EXPECT().InspectImageEnv(mock.Anything, "myapp:v1").Return([]string{}, nil)
1027+
1028+ // Create container (no -new suffix since no existing tracked container)
1029+ newContainer := &domain.Container{ID: "new-container", Name: "gordon-test.example.com", Status: "created"}
1030+ runtime.EXPECT().CreateContainer(mock.Anything, mock.MatchedBy(func(cfg *domain.ContainerConfig) bool {
1031+ return cfg.Name == "gordon-test.example.com"
1032+ })).Return(newContainer, nil)
1033+ runtime.EXPECT().StartContainer(mock.Anything, "new-container").Return(nil)
1034+
1035+ // Wait for ready
1036+ runtime.EXPECT().IsContainerRunning(mock.Anything, "new-container").Return(true, nil).Times(2)
1037+
1038+ // Inspect after ready
1039+ runtime.EXPECT().InspectContainer(mock.Anything, "new-container").Return(&domain.Container{
1040+ ID: "new-container",
1041+ Status: "running",
1042+ Ports: []int{8080},
1043+ }, nil)
1044+
1045+ // Publish event
1046+ eventBus.EXPECT().Publish(domain.EventContainerDeployed, mock.AnythingOfType("*domain.ContainerEventPayload")).Return(nil)
1047+
1048+ result, err := svc.Deploy(ctx, route)
1049+
1050+ assert.NoError(t, err)
1051+ assert.Equal(t, "new-container", result.ID)
1052+
1053+ // Verify new container is tracked
1054+ tracked, exists := svc.Get(ctx, "test.example.com")
1055+ assert.True(t, exists)
1056+ assert.Equal(t, "new-container", tracked.ID)
1057+}
@@ -895,3 +895,163 @@ func TestService_AutoStart_HandlesDeployErrors(t *testing.T) {
895 assert.Error(t, err)895 assert.Error(t, err)
896 assert.Contains(t, err.Error(), "auto-start completed with 1 errors")896 assert.Contains(t, err.Error(), "auto-start completed with 1 errors")
897 }897 }
898+
899+// TestService_Deploy_OrphanCleanupSkipsTrackedContainer verifies that the orphan cleanup
900+// does not remove the currently tracked container during zero-downtime deployment.
901+// This is critical for preventing downtime - the old container must stay running
902+// until the new container is ready and traffic is switched.
903+func TestService_Deploy_OrphanCleanupSkipsTrackedContainer(t *testing.T) {
904+ runtime := mocks.NewMockContainerRuntime(t)
905+ envLoader := mocks.NewMockEnvLoader(t)
906+ eventBus := mocks.NewMockEventPublisher(t)
907+
908+ config := Config{
909+ ReadinessDelay: 0, // No delay for tests
910+ }
911+ svc := NewService(runtime, envLoader, eventBus, nil, config)
912+ ctx := testContext()
913+
914+ // Pre-populate with existing tracked container
915+ existingContainer := &domain.Container{
916+ ID: "tracked-container-123",
917+ Name: "gordon-test.example.com",
918+ Status: "running",
919+ }
920+ svc.containers["test.example.com"] = existingContainer
921+
922+ route := domain.Route{
923+ Domain: "test.example.com",
924+ Image: "myapp:v2",
925+ }
926+
927+ // ListContainers returns the tracked container - this simulates the orphan check
928+ // finding a container with the same name. The bug was that it would remove this
929+ // container BEFORE the new one was ready, causing downtime.
930+ runtime.EXPECT().ListContainers(mock.Anything, true).Return([]*domain.Container{
931+ {
932+ ID: "tracked-container-123",
933+ Name: "gordon-test.example.com",
934+ Status: "running",
935+ },
936+ }, nil)
937+
938+ // Image operations
939+ runtime.EXPECT().ListImages(mock.Anything).Return([]string{}, nil)
940+ runtime.EXPECT().PullImage(mock.Anything, "myapp:v2").Return(nil)
941+ runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:v2").Return([]int{8080}, nil)
942+
943+ // Environment
944+ envLoader.EXPECT().LoadEnv(mock.Anything, "test.example.com").Return([]string{}, nil)
945+ runtime.EXPECT().InspectImageEnv(mock.Anything, "myapp:v2").Return([]string{}, nil)
946+
947+ // Create new container with -new suffix for zero-downtime
948+ newContainer := &domain.Container{ID: "new-container", Name: "gordon-test.example.com-new", Status: "created"}
949+ runtime.EXPECT().CreateContainer(mock.Anything, mock.MatchedBy(func(cfg *domain.ContainerConfig) bool {
950+ return cfg.Name == "gordon-test.example.com-new"
951+ })).Return(newContainer, nil)
952+ runtime.EXPECT().StartContainer(mock.Anything, "new-container").Return(nil)
953+
954+ // Wait for ready
955+ runtime.EXPECT().IsContainerRunning(mock.Anything, "new-container").Return(true, nil).Times(2)
956+
957+ // Inspect after ready
958+ runtime.EXPECT().InspectContainer(mock.Anything, "new-container").Return(&domain.Container{
959+ ID: "new-container",
960+ Status: "running",
961+ }, nil)
962+
963+ // Publish event
964+ eventBus.EXPECT().Publish(domain.EventContainerDeployed, mock.AnythingOfType("*domain.ContainerEventPayload")).Return(nil)
965+
966+ // NOW (after new container is ready) the old container should be stopped and removed
967+ // This is the correct zero-downtime sequence - not during orphan cleanup
968+ runtime.EXPECT().StopContainer(mock.Anything, "tracked-container-123").Return(nil)
969+ runtime.EXPECT().RemoveContainer(mock.Anything, "tracked-container-123", true).Return(nil)
970+
971+ // Rename new container to canonical name
972+ runtime.EXPECT().RenameContainer(mock.Anything, "new-container", "gordon-test.example.com").Return(nil)
973+
974+ result, err := svc.Deploy(ctx, route)
975+
976+ assert.NoError(t, err)
977+ assert.Equal(t, "new-container", result.ID)
978+
979+ // Verify new container is now tracked
980+ tracked, exists := svc.Get(ctx, "test.example.com")
981+ assert.True(t, exists)
982+ assert.Equal(t, "new-container", tracked.ID)
983+}
984+
985+// TestService_Deploy_OrphanCleanupRemovesTrueOrphans verifies that containers
986+// with the same name but NOT tracked are properly removed as orphans.
987+func TestService_Deploy_OrphanCleanupRemovesTrueOrphans(t *testing.T) {
988+ runtime := mocks.NewMockContainerRuntime(t)
989+ envLoader := mocks.NewMockEnvLoader(t)
990+ eventBus := mocks.NewMockEventPublisher(t)
991+
992+ config := Config{
993+ ReadinessDelay: 0, // No delay for tests
994+ }
995+ svc := NewService(runtime, envLoader, eventBus, nil, config)
996+ ctx := testContext()
997+
998+ // NO tracked container - service is empty for this domain
999+
1000+ route := domain.Route{
1001+ Domain: "test.example.com",
1002+ Image: "myapp:v1",
1003+ }
1004+
1005+ // ListContainers returns an orphaned container (same name, but not tracked)
1006+ // This could happen if Gordon crashed and restarted, or container was created manually
1007+ runtime.EXPECT().ListContainers(mock.Anything, true).Return([]*domain.Container{
1008+ {
1009+ ID: "orphan-container",
1010+ Name: "gordon-test.example.com",
1011+ Status: "running",
1012+ },
1013+ }, nil)
1014+
1015+ // Orphan should be stopped and removed BEFORE we proceed
1016+ runtime.EXPECT().StopContainer(mock.Anything, "orphan-container").Return(nil)
1017+ runtime.EXPECT().RemoveContainer(mock.Anything, "orphan-container", true).Return(nil)
1018+
1019+ // Image operations
1020+ runtime.EXPECT().ListImages(mock.Anything).Return([]string{}, nil)
1021+ runtime.EXPECT().PullImage(mock.Anything, "myapp:v1").Return(nil)
1022+ runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:v1").Return([]int{8080}, nil)
1023+
1024+ // Environment
1025+ envLoader.EXPECT().LoadEnv(mock.Anything, "test.example.com").Return([]string{}, nil)
1026+ runtime.EXPECT().InspectImageEnv(mock.Anything, "myapp:v1").Return([]string{}, nil)
1027+
1028+ // Create container (no -new suffix since no existing tracked container)
1029+ newContainer := &domain.Container{ID: "new-container", Name: "gordon-test.example.com", Status: "created"}
1030+ runtime.EXPECT().CreateContainer(mock.Anything, mock.MatchedBy(func(cfg *domain.ContainerConfig) bool {
1031+ return cfg.Name == "gordon-test.example.com"
1032+ })).Return(newContainer, nil)
1033+ runtime.EXPECT().StartContainer(mock.Anything, "new-container").Return(nil)
1034+
1035+ // Wait for ready
1036+ runtime.EXPECT().IsContainerRunning(mock.Anything, "new-container").Return(true, nil).Times(2)
1037+
1038+ // Inspect after ready
1039+ runtime.EXPECT().InspectContainer(mock.Anything, "new-container").Return(&domain.Container{
1040+ ID: "new-container",
1041+ Status: "running",
1042+ Ports: []int{8080},
1043+ }, nil)
1044+
1045+ // Publish event
1046+ eventBus.EXPECT().Publish(domain.EventContainerDeployed, mock.AnythingOfType("*domain.ContainerEventPayload")).Return(nil)
1047+
1048+ result, err := svc.Deploy(ctx, route)
1049+
1050+ assert.NoError(t, err)
1051+ assert.Equal(t, "new-container", result.ID)
1052+
1053+ // Verify new container is tracked
1054+ tracked, exists := svc.Get(ctx, "test.example.com")
1055+ assert.True(t, exists)
1056+ assert.Equal(t, "new-container", tracked.ID)
1057+}

Keyboard shortcuts

Single-key shortcuts are on. ctrl k and esc work whatever this says.

ctrl k :
go to a section, branch, tag, file or repository
?
this list
j k
move the row cursor down / up
enter
open the row under the cursor; on Files, a directory opens or closes in place
↑ ↓ ← → home end
in the file tree (focus on a row): previous / next row, close / open a directory or go to its parent / first entry, first / last row. They never open a file
l h
on Files: open a directory or a file / close a directory or go up to its parent, then to the parent page. Elsewhere: into the side panel (on a merge request, its files) / back to the content; there j k move between its controls
g g G
first / last row
/
filter the list; on Files, find a file anywhere in the repository (esc clears, enter opens the best match)
b
switch branch or tag
g s g f g c g b g m g i g r g a
tabs: summary, files, commits, branches & tags, merge requests, issues, releases, actions (when available)
g h
home
w
wide mode: use the whole width of the screen, or not (remembered)
y
copy a permalink pinned to the commit
d u x
diff: split / unified / collapse or expand all
e E
diff: the whole file in view / every file, with the lines the hunks hide
n
merge request: next note in the diff
esc
close a dialog, leave a field

Branches 7

Tags 106

↑↓ moveenter openesc close