detached 867c255

fix: orphan cleanup never kills running canonical container

Running containers with the canonical name are now preserved during
orphan cleanup even if not tracked in memory. Only stopped canonical
containers and temp (-new/-next) containers are cleaned up.
commit
867c2558fced85aa546f09e465274bc02e2c7415
author
brice · · 2026-03-02 16:20 UTC
parent
3efe7b9
signature
unsigned
2 files · +109 -14
Unified
UnifiedSplit
M internal/usecase/container/service.go +12 -0
@@ -1656,6 +1656,18 @@ func (s *Service) cleanupOrphanedContainers(ctx context.Context, domainName stri
16561656
16571657 for _, c := range allContainers {
16581658 if (c.Name == expectedName || c.Name == expectedNewName || c.Name == expectedNextName) && c.ID != skipContainerID {
1659+ // Never kill a running container with the canonical name — it may be
1660+ // the active container serving traffic. Only temp (-new/-next) containers
1661+ // and stopped canonical containers are safe to remove.
1662+ isCanonical := c.Name == expectedName
1663+ if isCanonical && c.Status == "running" {
1664+ log.Debug().
1665+ Str(zerowrap.FieldEntityID, c.ID).
1666+ Str(zerowrap.FieldStatus, c.Status).
1667+ Msg("skipping running canonical container during orphan cleanup")
1668+ continue
1669+ }
1670+
16591671 log.Info().Str(zerowrap.FieldEntityID, c.ID).Str(zerowrap.FieldStatus, c.Status).Msg("found orphaned container, removing")
16601672
16611673 if err := s.runtime.StopContainer(ctx, c.ID); err != nil {
@@ -1656,6 +1656,18 @@ func (s *Service) cleanupOrphanedContainers(ctx context.Context, domainName stri
1656 1656
1657 for _, c := range allContainers {1657 for _, c := range allContainers {
1658 if (c.Name == expectedName || c.Name == expectedNewName || c.Name == expectedNextName) && c.ID != skipContainerID {1658 if (c.Name == expectedName || c.Name == expectedNewName || c.Name == expectedNextName) && c.ID != skipContainerID {
1659+ // Never kill a running container with the canonical name — it may be
1660+ // the active container serving traffic. Only temp (-new/-next) containers
1661+ // and stopped canonical containers are safe to remove.
1662+ isCanonical := c.Name == expectedName
1663+ if isCanonical && c.Status == "running" {
1664+ log.Debug().
1665+ Str(zerowrap.FieldEntityID, c.ID).
1666+ Str(zerowrap.FieldStatus, c.Status).
1667+ Msg("skipping running canonical container during orphan cleanup")
1668+ continue
1669+ }
1670+
1659 log.Info().Str(zerowrap.FieldEntityID, c.ID).Str(zerowrap.FieldStatus, c.Status).Msg("found orphaned container, removing")1671 log.Info().Str(zerowrap.FieldEntityID, c.ID).Str(zerowrap.FieldStatus, c.Status).Msg("found orphaned container, removing")
1660 1672
1661 if err := s.runtime.StopContainer(ctx, c.ID); err != nil {1673 if err := s.runtime.StopContainer(ctx, c.ID); err != nil {
M internal/usecase/container/service_test.go +97 -14
@@ -1748,8 +1748,9 @@ func TestService_Deploy_OrphanCleanupSkipsTrackedContainer(t *testing.T) {
17481748 assert.Equal(t, "new-container", tracked.ID)
17491749 }
17501750
1751-// TestService_Deploy_OrphanCleanupRemovesTrueOrphans verifies that containers
1752-// with the same name but NOT tracked are properly removed as orphans.
1751+// TestService_Deploy_OrphanCleanupRemovesTrueOrphans verifies that stopped canonical
1752+// containers with the same name but NOT tracked are properly removed as orphans.
1753+// Running canonical containers are preserved (see NeverKillsRunningCanonical test).
17531754 func TestService_Deploy_OrphanCleanupRemovesTrueOrphans(t *testing.T) {
17541755 runtime := mocks.NewMockContainerRuntime(t)
17551756 envLoader := mocks.NewMockEnvLoader(t)
@@ -1770,26 +1771,20 @@ func TestService_Deploy_OrphanCleanupRemovesTrueOrphans(t *testing.T) {
17701771 }
17711772
17721773 // No container in memory — runtime resolution finds no managed container
1773- // (the orphan lacks LabelManaged so it won't match)
1774- runtime.EXPECT().ListContainers(mock.Anything, false).Return([]*domain.Container{
1775- {
1776- ID: "orphan-container",
1777- Name: "gordon-test.example.com",
1778- Status: "running",
1779- },
1780- }, nil)
1774+ // (the orphan is stopped so it won't appear in running-only query)
1775+ runtime.EXPECT().ListContainers(mock.Anything, false).Return([]*domain.Container{}, nil)
17811776
1782- // ListContainers returns an orphaned container (same name, but not tracked)
1783- // This could happen if Gordon crashed and restarted, or container was created manually
1777+ // ListContainers(true) returns a STOPPED orphaned container (same name, but not tracked)
1778+ // Stopped canonical containers are safe to remove during orphan cleanup
17841779 runtime.EXPECT().ListContainers(mock.Anything, true).Return([]*domain.Container{
17851780 {
17861781 ID: "orphan-container",
17871782 Name: "gordon-test.example.com",
1788- Status: "running",
1783+ Status: "exited",
17891784 },
17901785 }, nil)
17911786
1792- // Orphan should be stopped and removed BEFORE we proceed
1787+ // Stopped orphan should be stopped and removed BEFORE we proceed
17931788 runtime.EXPECT().StopContainer(mock.Anything, "orphan-container").Return(nil)
17941789 runtime.EXPECT().RemoveContainer(mock.Anything, "orphan-container", true).Return(nil)
17951790
@@ -2629,3 +2624,91 @@ func TestService_Deploy_SkipRedundantDeploy_ContainerNotRunning(t *testing.T) {
26292624 assert.NoError(t, err)
26302625 assert.Equal(t, "new-container", result.ID)
26312626 }
2627+
2628+// TestService_Deploy_OrphanCleanup_NeverKillsRunningCanonical verifies that orphan
2629+// cleanup never stops a running container with the canonical name, even if it is not
2630+// tracked in memory and was not resolved by resolveExistingContainer. Only stopped
2631+// canonical containers and temp containers (-new/-next) should be cleaned up.
2632+func TestService_Deploy_OrphanCleanup_NeverKillsRunningCanonical(t *testing.T) {
2633+ runtime := mocks.NewMockContainerRuntime(t)
2634+ envLoader := mocks.NewMockEnvLoader(t)
2635+ eventBus := mocks.NewMockEventPublisher(t)
2636+
2637+ config := Config{
2638+ ReadinessDelay: time.Millisecond,
2639+ DrainDelay: time.Millisecond,
2640+ }
2641+ svc := NewService(runtime, envLoader, eventBus, nil, config)
2642+ ctx := testContext()
2643+
2644+ // NO tracked container in memory. resolveExistingContainer will NOT find it either
2645+ // because the running container lacks the managed label in this edge case.
2646+
2647+ route := domain.Route{
2648+ Domain: "test.example.com",
2649+ Image: "myapp:v2",
2650+ }
2651+
2652+ // resolveExistingContainer: ListContainers(ctx, false) returns empty —
2653+ // the canonical container is running but not discovered (e.g., missing label)
2654+ runtime.EXPECT().ListContainers(mock.Anything, false).Return([]*domain.Container{}, nil)
2655+
2656+ // cleanupOrphanedContainers: ListContainers(ctx, true) finds both the running
2657+ // canonical container AND a stale -new leftover. The canonical container should
2658+ // NOT be killed because it's running and has the canonical name.
2659+ runtime.EXPECT().ListContainers(mock.Anything, true).Return([]*domain.Container{
2660+ {
2661+ ID: "active-canonical",
2662+ Name: "gordon-test.example.com",
2663+ Status: "running",
2664+ },
2665+ {
2666+ ID: "stale-leftover",
2667+ Name: "gordon-test.example.com-new",
2668+ Status: "exited",
2669+ },
2670+ }, nil)
2671+
2672+ // Only the stale -new container should be stopped and removed during orphan cleanup
2673+ runtime.EXPECT().StopContainer(mock.Anything, "stale-leftover").Return(nil).Once()
2674+ runtime.EXPECT().RemoveContainer(mock.Anything, "stale-leftover", true).Return(nil).Once()
2675+
2676+ // Image operations
2677+ runtime.EXPECT().ListImages(mock.Anything).Return([]string{}, nil)
2678+ runtime.EXPECT().PullImage(mock.Anything, "myapp:v2").Return(nil)
2679+ runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:v2").Return([]int{8080}, nil)
2680+
2681+ // Environment
2682+ envLoader.EXPECT().LoadEnv(mock.Anything, "test.example.com").Return([]string{}, nil)
2683+ runtime.EXPECT().InspectImageEnv(mock.Anything, "myapp:v2").Return([]string{}, nil)
2684+
2685+ // Create container — no existing tracked, so canonical name is used
2686+ newContainer := &domain.Container{ID: "new-container", Name: "gordon-test.example.com", Status: "created"}
2687+ runtime.EXPECT().CreateContainer(mock.Anything, mock.MatchedBy(func(cfg *domain.ContainerConfig) bool {
2688+ return cfg.Name == "gordon-test.example.com"
2689+ })).Return(newContainer, nil)
2690+ runtime.EXPECT().StartContainer(mock.Anything, "new-container").Return(nil)
2691+
2692+ // Wait for ready
2693+ runtime.EXPECT().IsContainerRunning(mock.Anything, "new-container").Return(true, nil).Times(2)
2694+
2695+ // Inspect after ready
2696+ runtime.EXPECT().InspectContainer(mock.Anything, "new-container").Return(&domain.Container{
2697+ ID: "new-container",
2698+ Status: "running",
2699+ Ports: []int{8080},
2700+ }, nil)
2701+
2702+ // Publish event
2703+ eventBus.EXPECT().Publish(domain.EventContainerDeployed, mock.AnythingOfType("*domain.ContainerEventPayload")).Return(nil)
2704+
2705+ result, err := svc.Deploy(ctx, route)
2706+
2707+ assert.NoError(t, err)
2708+ assert.Equal(t, "new-container", result.ID)
2709+
2710+ // Verify new container is tracked
2711+ tracked, exists := svc.Get(ctx, "test.example.com")
2712+ assert.True(t, exists)
2713+ assert.Equal(t, "new-container", tracked.ID)
2714+}
@@ -1748,8 +1748,9 @@ func TestService_Deploy_OrphanCleanupSkipsTrackedContainer(t *testing.T) {
1748 assert.Equal(t, "new-container", tracked.ID)1748 assert.Equal(t, "new-container", tracked.ID)
1749 }1749 }
1750 1750
1751-// TestService_Deploy_OrphanCleanupRemovesTrueOrphans verifies that containers1751+// TestService_Deploy_OrphanCleanupRemovesTrueOrphans verifies that stopped canonical
1752-// with the same name but NOT tracked are properly removed as orphans.1752+// containers with the same name but NOT tracked are properly removed as orphans.
1753+// Running canonical containers are preserved (see NeverKillsRunningCanonical test).
1753 func TestService_Deploy_OrphanCleanupRemovesTrueOrphans(t *testing.T) {1754 func TestService_Deploy_OrphanCleanupRemovesTrueOrphans(t *testing.T) {
1754 runtime := mocks.NewMockContainerRuntime(t)1755 runtime := mocks.NewMockContainerRuntime(t)
1755 envLoader := mocks.NewMockEnvLoader(t)1756 envLoader := mocks.NewMockEnvLoader(t)
@@ -1770,26 +1771,20 @@ func TestService_Deploy_OrphanCleanupRemovesTrueOrphans(t *testing.T) {
1770 }1771 }
1771 1772
1772 // No container in memory — runtime resolution finds no managed container1773 // No container in memory — runtime resolution finds no managed container
1773- // (the orphan lacks LabelManaged so it won't match)1774+ // (the orphan is stopped so it won't appear in running-only query)
1774- runtime.EXPECT().ListContainers(mock.Anything, false).Return([]*domain.Container{1775+ runtime.EXPECT().ListContainers(mock.Anything, false).Return([]*domain.Container{}, nil)
1775- {
1776- ID: "orphan-container",
1777- Name: "gordon-test.example.com",
1778- Status: "running",
1779- },
1780- }, nil)
1781 1776
1782- // ListContainers returns an orphaned container (same name, but not tracked)1777+ // ListContainers(true) returns a STOPPED orphaned container (same name, but not tracked)
1783- // This could happen if Gordon crashed and restarted, or container was created manually1778+ // Stopped canonical containers are safe to remove during orphan cleanup
1784 runtime.EXPECT().ListContainers(mock.Anything, true).Return([]*domain.Container{1779 runtime.EXPECT().ListContainers(mock.Anything, true).Return([]*domain.Container{
1785 {1780 {
1786 ID: "orphan-container",1781 ID: "orphan-container",
1787 Name: "gordon-test.example.com",1782 Name: "gordon-test.example.com",
1788- Status: "running",1783+ Status: "exited",
1789 },1784 },
1790 }, nil)1785 }, nil)
1791 1786
1792- // Orphan should be stopped and removed BEFORE we proceed1787+ // Stopped orphan should be stopped and removed BEFORE we proceed
1793 runtime.EXPECT().StopContainer(mock.Anything, "orphan-container").Return(nil)1788 runtime.EXPECT().StopContainer(mock.Anything, "orphan-container").Return(nil)
1794 runtime.EXPECT().RemoveContainer(mock.Anything, "orphan-container", true).Return(nil)1789 runtime.EXPECT().RemoveContainer(mock.Anything, "orphan-container", true).Return(nil)
1795 1790
@@ -2629,3 +2624,91 @@ func TestService_Deploy_SkipRedundantDeploy_ContainerNotRunning(t *testing.T) {
2629 assert.NoError(t, err)2624 assert.NoError(t, err)
2630 assert.Equal(t, "new-container", result.ID)2625 assert.Equal(t, "new-container", result.ID)
2631 }2626 }
2627+
2628+// TestService_Deploy_OrphanCleanup_NeverKillsRunningCanonical verifies that orphan
2629+// cleanup never stops a running container with the canonical name, even if it is not
2630+// tracked in memory and was not resolved by resolveExistingContainer. Only stopped
2631+// canonical containers and temp containers (-new/-next) should be cleaned up.
2632+func TestService_Deploy_OrphanCleanup_NeverKillsRunningCanonical(t *testing.T) {
2633+ runtime := mocks.NewMockContainerRuntime(t)
2634+ envLoader := mocks.NewMockEnvLoader(t)
2635+ eventBus := mocks.NewMockEventPublisher(t)
2636+
2637+ config := Config{
2638+ ReadinessDelay: time.Millisecond,
2639+ DrainDelay: time.Millisecond,
2640+ }
2641+ svc := NewService(runtime, envLoader, eventBus, nil, config)
2642+ ctx := testContext()
2643+
2644+ // NO tracked container in memory. resolveExistingContainer will NOT find it either
2645+ // because the running container lacks the managed label in this edge case.
2646+
2647+ route := domain.Route{
2648+ Domain: "test.example.com",
2649+ Image: "myapp:v2",
2650+ }
2651+
2652+ // resolveExistingContainer: ListContainers(ctx, false) returns empty —
2653+ // the canonical container is running but not discovered (e.g., missing label)
2654+ runtime.EXPECT().ListContainers(mock.Anything, false).Return([]*domain.Container{}, nil)
2655+
2656+ // cleanupOrphanedContainers: ListContainers(ctx, true) finds both the running
2657+ // canonical container AND a stale -new leftover. The canonical container should
2658+ // NOT be killed because it's running and has the canonical name.
2659+ runtime.EXPECT().ListContainers(mock.Anything, true).Return([]*domain.Container{
2660+ {
2661+ ID: "active-canonical",
2662+ Name: "gordon-test.example.com",
2663+ Status: "running",
2664+ },
2665+ {
2666+ ID: "stale-leftover",
2667+ Name: "gordon-test.example.com-new",
2668+ Status: "exited",
2669+ },
2670+ }, nil)
2671+
2672+ // Only the stale -new container should be stopped and removed during orphan cleanup
2673+ runtime.EXPECT().StopContainer(mock.Anything, "stale-leftover").Return(nil).Once()
2674+ runtime.EXPECT().RemoveContainer(mock.Anything, "stale-leftover", true).Return(nil).Once()
2675+
2676+ // Image operations
2677+ runtime.EXPECT().ListImages(mock.Anything).Return([]string{}, nil)
2678+ runtime.EXPECT().PullImage(mock.Anything, "myapp:v2").Return(nil)
2679+ runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:v2").Return([]int{8080}, nil)
2680+
2681+ // Environment
2682+ envLoader.EXPECT().LoadEnv(mock.Anything, "test.example.com").Return([]string{}, nil)
2683+ runtime.EXPECT().InspectImageEnv(mock.Anything, "myapp:v2").Return([]string{}, nil)
2684+
2685+ // Create container — no existing tracked, so canonical name is used
2686+ newContainer := &domain.Container{ID: "new-container", Name: "gordon-test.example.com", Status: "created"}
2687+ runtime.EXPECT().CreateContainer(mock.Anything, mock.MatchedBy(func(cfg *domain.ContainerConfig) bool {
2688+ return cfg.Name == "gordon-test.example.com"
2689+ })).Return(newContainer, nil)
2690+ runtime.EXPECT().StartContainer(mock.Anything, "new-container").Return(nil)
2691+
2692+ // Wait for ready
2693+ runtime.EXPECT().IsContainerRunning(mock.Anything, "new-container").Return(true, nil).Times(2)
2694+
2695+ // Inspect after ready
2696+ runtime.EXPECT().InspectContainer(mock.Anything, "new-container").Return(&domain.Container{
2697+ ID: "new-container",
2698+ Status: "running",
2699+ Ports: []int{8080},
2700+ }, nil)
2701+
2702+ // Publish event
2703+ eventBus.EXPECT().Publish(domain.EventContainerDeployed, mock.AnythingOfType("*domain.ContainerEventPayload")).Return(nil)
2704+
2705+ result, err := svc.Deploy(ctx, route)
2706+
2707+ assert.NoError(t, err)
2708+ assert.Equal(t, "new-container", result.ID)
2709+
2710+ // Verify new container is tracked
2711+ tracked, exists := svc.Get(ctx, "test.example.com")
2712+ assert.True(t, exists)
2713+ assert.Equal(t, "new-container", tracked.ID)
2714+}

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