detached b27b280

fix(container): clean up interrupted deploy candidates

commit
b27b280ba0dc2690c678442ae303f0c3484397bb
author
brice · · 2026-08-10 14:20 UTC
parent
8f4a170
signature
GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email
2 files · +64 -14
Unified
UnifiedSplit
M internal/usecase/container/service.go +15 -11
@@ -77,9 +77,10 @@ const (
7777 // readinessRecoveryWindow is an additional grace window used when a container
7878 // is briefly not running at the end of readiness delay. This avoids false
7979 // negatives during short startup flaps.
80- readinessRecoveryWindow = 30 * time.Second
81- internalPullMaxAttempts = 3
82- deployFailureLogLimit = 20
80+ readinessRecoveryWindow = 30 * time.Second
81+ failedContainerCleanupTimeout = 30 * time.Second
82+ internalPullMaxAttempts = 3
83+ deployFailureLogLimit = 20
8384 )
8485
8586 // Service implements the ContainerService interface.
@@ -1963,11 +1964,14 @@ func (s *Service) UpdateAttachments(attachments map[string][]string) {
19631964
19641965 // cleanupFailedContainer stops and removes a container that failed to start properly.
19651966 func (s *Service) cleanupFailedContainer(ctx context.Context, containerID string) {
1966- log := zerowrap.FromCtx(ctx)
1967- if err := s.runtime.StopContainer(ctx, containerID); err != nil {
1967+ cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), failedContainerCleanupTimeout)
1968+ defer cancel()
1969+
1970+ log := zerowrap.FromCtx(cleanupCtx)
1971+ if err := s.runtime.StopContainer(cleanupCtx, containerID); err != nil {
19681972 log.Warn().Err(err).Str(zerowrap.FieldEntityID, containerID).Msg("failed to stop container after failure")
19691973 }
1970- if err := s.runtime.RemoveContainer(ctx, containerID, true); err != nil {
1974+ if err := s.runtime.RemoveContainer(cleanupCtx, containerID, true); err != nil {
19711975 log.Warn().Err(err).Str(zerowrap.FieldEntityID, containerID).Msg("failed to remove container after failure")
19721976 }
19731977 }
@@ -2631,11 +2635,11 @@ func (s *Service) cleanupOrphanedContainers(ctx context.Context, domainName stri
26312635
26322636 for _, c := range allContainers {
26332637 if (c.Name == expectedName || c.Name == expectedNewName || c.Name == expectedNextName) && c.ID != skipContainerID {
2634- // Skip any container that is still active in the runtime, including
2635- // restart backoff, regardless of name — a temp container may be
2636- // serving traffic while the new container stabilizes, or may be from
2637- // a concurrent deploy.
2638- if c.Status == "running" || c.Status == "restarting" {
2638+ // Without a selected active container, preserve anything that may still
2639+ // be serving traffic. When an active container is selected, other active
2640+ // temp-name containers are stale candidates left by interrupted deploys.
2641+ isActive := c.Status == "running" || c.Status == "restarting"
2642+ if isActive && (skipContainerID == "" || c.Name == expectedName) {
26392643 log.Debug().
26402644 Str(zerowrap.FieldEntityID, c.ID).
26412645 Str("container_name", c.Name).
@@ -77,9 +77,10 @@ const (
77 // readinessRecoveryWindow is an additional grace window used when a container77 // readinessRecoveryWindow is an additional grace window used when a container
78 // is briefly not running at the end of readiness delay. This avoids false78 // is briefly not running at the end of readiness delay. This avoids false
79 // negatives during short startup flaps.79 // negatives during short startup flaps.
80- readinessRecoveryWindow = 30 * time.Second80+ readinessRecoveryWindow = 30 * time.Second
81- internalPullMaxAttempts = 381+ failedContainerCleanupTimeout = 30 * time.Second
82- deployFailureLogLimit = 2082+ internalPullMaxAttempts = 3
83+ deployFailureLogLimit = 20
83 )84 )
84 85
85 // Service implements the ContainerService interface.86 // Service implements the ContainerService interface.
@@ -1963,11 +1964,14 @@ func (s *Service) UpdateAttachments(attachments map[string][]string) {
1963 1964
1964 // cleanupFailedContainer stops and removes a container that failed to start properly.1965 // cleanupFailedContainer stops and removes a container that failed to start properly.
1965 func (s *Service) cleanupFailedContainer(ctx context.Context, containerID string) {1966 func (s *Service) cleanupFailedContainer(ctx context.Context, containerID string) {
1966- log := zerowrap.FromCtx(ctx)1967+ cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), failedContainerCleanupTimeout)
1967- if err := s.runtime.StopContainer(ctx, containerID); err != nil {1968+ defer cancel()
1969+
1970+ log := zerowrap.FromCtx(cleanupCtx)
1971+ if err := s.runtime.StopContainer(cleanupCtx, containerID); err != nil {
1968 log.Warn().Err(err).Str(zerowrap.FieldEntityID, containerID).Msg("failed to stop container after failure")1972 log.Warn().Err(err).Str(zerowrap.FieldEntityID, containerID).Msg("failed to stop container after failure")
1969 }1973 }
1970- if err := s.runtime.RemoveContainer(ctx, containerID, true); err != nil {1974+ if err := s.runtime.RemoveContainer(cleanupCtx, containerID, true); err != nil {
1971 log.Warn().Err(err).Str(zerowrap.FieldEntityID, containerID).Msg("failed to remove container after failure")1975 log.Warn().Err(err).Str(zerowrap.FieldEntityID, containerID).Msg("failed to remove container after failure")
1972 }1976 }
1973 }1977 }
@@ -2631,11 +2635,11 @@ func (s *Service) cleanupOrphanedContainers(ctx context.Context, domainName stri
2631 2635
2632 for _, c := range allContainers {2636 for _, c := range allContainers {
2633 if (c.Name == expectedName || c.Name == expectedNewName || c.Name == expectedNextName) && c.ID != skipContainerID {2637 if (c.Name == expectedName || c.Name == expectedNewName || c.Name == expectedNextName) && c.ID != skipContainerID {
2634- // Skip any container that is still active in the runtime, including2638+ // Without a selected active container, preserve anything that may still
2635- // restart backoff, regardless of name — a temp container may be2639+ // be serving traffic. When an active container is selected, other active
2636- // serving traffic while the new container stabilizes, or may be from2640+ // temp-name containers are stale candidates left by interrupted deploys.
2637- // a concurrent deploy.2641+ isActive := c.Status == "running" || c.Status == "restarting"
2638- if c.Status == "running" || c.Status == "restarting" {2642+ if isActive && (skipContainerID == "" || c.Name == expectedName) {
2639 log.Debug().2643 log.Debug().
2640 Str(zerowrap.FieldEntityID, c.ID).2644 Str(zerowrap.FieldEntityID, c.ID).
2641 Str("container_name", c.Name).2645 Str("container_name", c.Name).
M internal/usecase/container/service_test.go +49 -3
@@ -610,6 +610,51 @@ func TestService_Deploy_ReadinessFailureReturnsDeployFailureWithLogs(t *testing.
610610 assert.ErrorContains(t, deployErr.Err, "last status: starting")
611611 }
612612
613+func TestService_Deploy_RequestCancellationStillCleansUpFailedCandidate(t *testing.T) {
614+ runtime := mocks.NewMockContainerRuntime(t)
615+ envLoader := mocks.NewMockEnvLoader(t)
616+ eventBus := mocks.NewMockEventPublisher(t)
617+ svc := NewService(runtime, envLoader, eventBus, nil, testMinDelayConfig(), nil)
618+
619+ svc.containers["test.example.com"] = &domain.Container{
620+ ID: "old-container",
621+ Name: "gordon-test.example.com",
622+ Status: "running",
623+ }
624+ route := domain.Route{Domain: "test.example.com", Image: "myapp:v2"}
625+
626+ runtime.EXPECT().ListContainers(mock.Anything, true).Return([]*domain.Container{}, nil)
627+ runtime.EXPECT().ListImages(mock.Anything).Return([]string{"myapp:v2"}, nil)
628+ runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:v2").Return([]int{8080}, nil)
629+ runtime.EXPECT().GetImageLabels(mock.Anything, "myapp:v2").Return(nil, nil)
630+ envLoader.EXPECT().LoadEnv(mock.Anything, "test.example.com").Return([]string{}, nil)
631+ runtime.EXPECT().InspectImageEnv(mock.Anything, "myapp:v2").Return([]string{}, nil)
632+
633+ candidate := &domain.Container{ID: "candidate", Name: "gordon-test.example.com-new", Status: "created"}
634+ runtime.EXPECT().CreateContainer(mock.Anything, mock.Anything).Return(candidate, nil)
635+
636+ ctx, cancel := context.WithCancel(testContext())
637+ runtime.EXPECT().StartContainer(mock.Anything, candidate.ID).Run(func(context.Context, string) {
638+ cancel()
639+ }).Return(nil)
640+ runtime.EXPECT().IsContainerRunning(mock.Anything, candidate.ID).Return(true, nil).Once()
641+
642+ activeCleanupContext := mock.MatchedBy(func(ctx context.Context) bool {
643+ _, hasDeadline := ctx.Deadline()
644+ return ctx.Err() == nil && hasDeadline
645+ })
646+ runtime.EXPECT().StopContainer(activeCleanupContext, candidate.ID).Return(nil).Once()
647+ runtime.EXPECT().RemoveContainer(activeCleanupContext, candidate.ID, true).Return(nil).Once()
648+
649+ result, err := svc.Deploy(ctx, route)
650+
651+ assert.Nil(t, result)
652+ assert.ErrorIs(t, err, context.Canceled)
653+ tracked, exists := svc.Get(testContext(), route.Domain)
654+ require.True(t, exists)
655+ assert.Equal(t, "old-container", tracked.ID)
656+}
657+
613658 func TestService_Deploy_StrictHealthModeWithoutHealthcheckReturnsHint(t *testing.T) {
614659 runtime := mocks.NewMockContainerRuntime(t)
615660 envLoader := mocks.NewMockEnvLoader(t)
@@ -2492,7 +2537,7 @@ func TestService_Deploy_OrphanCleanupRemovesTrueOrphans(t *testing.T) {
24922537 assert.Equal(t, "new-container", tracked.ID)
24932538 }
24942539
2495-func TestService_Deploy_OrphanCleanupRemovesStaleNewContainer(t *testing.T) {
2540+func TestService_Deploy_OrphanCleanupRemovesRunningStaleCandidate(t *testing.T) {
24962541 runtime := mocks.NewMockContainerRuntime(t)
24972542 envLoader := mocks.NewMockEnvLoader(t)
24982543 eventBus := mocks.NewMockEventPublisher(t)
@@ -2503,7 +2548,8 @@ func TestService_Deploy_OrphanCleanupRemovesStaleNewContainer(t *testing.T) {
25032548 svc.SetProxyCacheInvalidator(cacheInvalidator)
25042549 ctx := testContext()
25052550
2506- // Tracked canonical container exists, but stale -new container should be removed.
2551+ // The tracked canonical container is active. A candidate left running by an
2552+ // interrupted request must be removed before its temporary name is reused.
25072553 svc.containers["test.example.com"] = &domain.Container{
25082554 ID: "tracked-container-123",
25092555 Name: "gordon-test.example.com",
@@ -2524,7 +2570,7 @@ func TestService_Deploy_OrphanCleanupRemovesStaleNewContainer(t *testing.T) {
25242570 {
25252571 ID: "stale-new-container",
25262572 Name: "gordon-test.example.com-new",
2527- Status: "exited",
2573+ Status: "running",
25282574 },
25292575 }, nil)
25302576 runtime.EXPECT().StopContainer(mock.Anything, "stale-new-container").Return(nil).Once()
@@ -610,6 +610,51 @@ func TestService_Deploy_ReadinessFailureReturnsDeployFailureWithLogs(t *testing.
610 assert.ErrorContains(t, deployErr.Err, "last status: starting")610 assert.ErrorContains(t, deployErr.Err, "last status: starting")
611 }611 }
612 612
613+func TestService_Deploy_RequestCancellationStillCleansUpFailedCandidate(t *testing.T) {
614+ runtime := mocks.NewMockContainerRuntime(t)
615+ envLoader := mocks.NewMockEnvLoader(t)
616+ eventBus := mocks.NewMockEventPublisher(t)
617+ svc := NewService(runtime, envLoader, eventBus, nil, testMinDelayConfig(), nil)
618+
619+ svc.containers["test.example.com"] = &domain.Container{
620+ ID: "old-container",
621+ Name: "gordon-test.example.com",
622+ Status: "running",
623+ }
624+ route := domain.Route{Domain: "test.example.com", Image: "myapp:v2"}
625+
626+ runtime.EXPECT().ListContainers(mock.Anything, true).Return([]*domain.Container{}, nil)
627+ runtime.EXPECT().ListImages(mock.Anything).Return([]string{"myapp:v2"}, nil)
628+ runtime.EXPECT().GetImageExposedPorts(mock.Anything, "myapp:v2").Return([]int{8080}, nil)
629+ runtime.EXPECT().GetImageLabels(mock.Anything, "myapp:v2").Return(nil, nil)
630+ envLoader.EXPECT().LoadEnv(mock.Anything, "test.example.com").Return([]string{}, nil)
631+ runtime.EXPECT().InspectImageEnv(mock.Anything, "myapp:v2").Return([]string{}, nil)
632+
633+ candidate := &domain.Container{ID: "candidate", Name: "gordon-test.example.com-new", Status: "created"}
634+ runtime.EXPECT().CreateContainer(mock.Anything, mock.Anything).Return(candidate, nil)
635+
636+ ctx, cancel := context.WithCancel(testContext())
637+ runtime.EXPECT().StartContainer(mock.Anything, candidate.ID).Run(func(context.Context, string) {
638+ cancel()
639+ }).Return(nil)
640+ runtime.EXPECT().IsContainerRunning(mock.Anything, candidate.ID).Return(true, nil).Once()
641+
642+ activeCleanupContext := mock.MatchedBy(func(ctx context.Context) bool {
643+ _, hasDeadline := ctx.Deadline()
644+ return ctx.Err() == nil && hasDeadline
645+ })
646+ runtime.EXPECT().StopContainer(activeCleanupContext, candidate.ID).Return(nil).Once()
647+ runtime.EXPECT().RemoveContainer(activeCleanupContext, candidate.ID, true).Return(nil).Once()
648+
649+ result, err := svc.Deploy(ctx, route)
650+
651+ assert.Nil(t, result)
652+ assert.ErrorIs(t, err, context.Canceled)
653+ tracked, exists := svc.Get(testContext(), route.Domain)
654+ require.True(t, exists)
655+ assert.Equal(t, "old-container", tracked.ID)
656+}
657+
613 func TestService_Deploy_StrictHealthModeWithoutHealthcheckReturnsHint(t *testing.T) {658 func TestService_Deploy_StrictHealthModeWithoutHealthcheckReturnsHint(t *testing.T) {
614 runtime := mocks.NewMockContainerRuntime(t)659 runtime := mocks.NewMockContainerRuntime(t)
615 envLoader := mocks.NewMockEnvLoader(t)660 envLoader := mocks.NewMockEnvLoader(t)
@@ -2492,7 +2537,7 @@ func TestService_Deploy_OrphanCleanupRemovesTrueOrphans(t *testing.T) {
2492 assert.Equal(t, "new-container", tracked.ID)2537 assert.Equal(t, "new-container", tracked.ID)
2493 }2538 }
2494 2539
2495-func TestService_Deploy_OrphanCleanupRemovesStaleNewContainer(t *testing.T) {2540+func TestService_Deploy_OrphanCleanupRemovesRunningStaleCandidate(t *testing.T) {
2496 runtime := mocks.NewMockContainerRuntime(t)2541 runtime := mocks.NewMockContainerRuntime(t)
2497 envLoader := mocks.NewMockEnvLoader(t)2542 envLoader := mocks.NewMockEnvLoader(t)
2498 eventBus := mocks.NewMockEventPublisher(t)2543 eventBus := mocks.NewMockEventPublisher(t)
@@ -2503,7 +2548,8 @@ func TestService_Deploy_OrphanCleanupRemovesStaleNewContainer(t *testing.T) {
2503 svc.SetProxyCacheInvalidator(cacheInvalidator)2548 svc.SetProxyCacheInvalidator(cacheInvalidator)
2504 ctx := testContext()2549 ctx := testContext()
2505 2550
2506- // Tracked canonical container exists, but stale -new container should be removed.2551+ // The tracked canonical container is active. A candidate left running by an
2552+ // interrupted request must be removed before its temporary name is reused.
2507 svc.containers["test.example.com"] = &domain.Container{2553 svc.containers["test.example.com"] = &domain.Container{
2508 ID: "tracked-container-123",2554 ID: "tracked-container-123",
2509 Name: "gordon-test.example.com",2555 Name: "gordon-test.example.com",
@@ -2524,7 +2570,7 @@ func TestService_Deploy_OrphanCleanupRemovesStaleNewContainer(t *testing.T) {
2524 {2570 {
2525 ID: "stale-new-container",2571 ID: "stale-new-container",
2526 Name: "gordon-test.example.com-new",2572 Name: "gordon-test.example.com-new",
2527- Status: "exited",2573+ Status: "running",
2528 },2574 },
2529 }, nil)2575 }, nil)
2530 runtime.EXPECT().StopContainer(mock.Anything, "stale-new-container").Return(nil).Once()2576 runtime.EXPECT().StopContainer(mock.Anything, "stale-new-container").Return(nil).Once()

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