fix: address code review findings for zero-downtime deploy

- httpProbe: add CheckRedirect to prevent following redirects, treating
  3xx responses as successful readiness signals instead of chasing them
- stabilizeNewContainer: verify old container is still running before
  rollback; return error when both containers are dead instead of
  silently returning a dead container reference
- resolveExistingContainer: guard managedCount increment to prevent
  double-counting when domain is already tracked

commit: 6cba4200a4f0f014d8eb6ea20b35bb40361a29ab
author: brice · 7mo ago · 2026-03-03 03:35 UTC
parent: 815f4fe
signature: unsigned

4 files · +43 -10

M internal/usecase/container/readiness.go +7 -1


@@ -38,7 +38,13 @@ func tcpProbe(ctx context.Context, addr string, timeout time.Duration) error {
38	38		// httpProbe performs HTTP GET requests to url, retrying every 1s until a
39	39		// 2xx/3xx response or timeout. Used when gordon.health label is set.
40	40		func httpProbe(ctx context.Context, url string, timeout time.Duration) error {
41		-	client := &http.Client{Timeout: 2 * time.Second}
	41	+	client := &http.Client{
	42	+	Timeout: 2 * time.Second,
	43	+	CheckRedirect: func(req http.Request, via []http.Request) error {
	44	+	// Don't follow redirects — treat 3xx as a successful response.
	45	+	return http.ErrUseLastResponse
	46	+	},
	47	+	}
42	48		deadline := time.Now().Add(timeout)
43	49		var lastStatus int
44	50

 // httpProbe performs HTTP GET requests to url, retrying every 1s until a
 // 2xx/3xx response or timeout. Used when gordon.health label is set.
 func httpProbe(ctx context.Context, url string, timeout time.Duration) error {
-	client := &http.Client{Timeout: 2 * time.Second}
+	client := &http.Client{
+		Timeout: 2 * time.Second,
+		CheckRedirect: func(req *http.Request, via []*http.Request) error {
+			// Don't follow redirects — treat 3xx as a successful response.
+			return http.ErrUseLastResponse
+		},
+	}
 	deadline := time.Now().Add(timeout)
 	var lastStatus int

M internal/usecase/container/readiness_test.go +2 -2


@@ -141,12 +141,12 @@ func TestHTTPProbe_ContextCancelled(t *testing.T) {
141	141
142	142		func TestHTTPProbe_RedirectIsSuccess(t *testing.T) {
143	143		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
144		-	w.WriteHeader(302)
	144	+	http.Redirect(w, r, "/other", http.StatusFound)
145	145		}))
146	146		defer srv.Close()
147	147
148	148		ctx := testContext()
149		-	// Disable follow redirects for the test — the probe should treat 3xx as success
	149	+	// The probe should treat 3xx as success without following the redirect
150	150		err := httpProbe(ctx, srv.URL+"/healthz", 5*time.Second)
151	151		assert.NoError(t, err)
152	152		}


@@ -141,12 +141,12 @@ func TestHTTPProbe_ContextCancelled(t *testing.T) {
141			141
142		func TestHTTPProbe_RedirectIsSuccess(t *testing.T) {	142		func TestHTTPProbe_RedirectIsSuccess(t *testing.T) {
143		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {	143		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
144	-	w.WriteHeader(302)	144	+	http.Redirect(w, r, "/other", http.StatusFound)
145		}))	145		}))
146		defer srv.Close()	146		defer srv.Close()
147			147
148		ctx := testContext()	148		ctx := testContext()
149	-	// Disable follow redirects for the test — the probe should treat 3xx as success	149	+	// The probe should treat 3xx as success without following the redirect
150		err := httpProbe(ctx, srv.URL+"/healthz", 5*time.Second)	150		err := httpProbe(ctx, srv.URL+"/healthz", 5*time.Second)
151		assert.NoError(t, err)	151		assert.NoError(t, err)
152		}	152		}

M internal/usecase/container/service.go +31 -7


@@ -311,7 +311,12 @@ func (s Service) Deploy(ctx context.Context, route domain.Route) (domain.Conta
311	311
312	312		// Post-switch stabilization: verify new container stays running
313	313		if hasExisting {
314		-	if !s.stabilizeNewContainer(ctx, route.Domain, newContainer, existing) {
	314	+	stable, stabilizeErr := s.stabilizeNewContainer(ctx, route.Domain, newContainer, existing)
	315	+	if stabilizeErr != nil {
	316	+	// Both old and new containers are dead
	317	+	return nil, stabilizeErr
	318	+	}
	319	+	if !stable {
315	320		// Rollback performed — old container is restored
316	321		return existing, nil
317	322		}

@@ -432,8 +437,10 @@ func (s *Service) resolveExistingContainer(ctx context.Context, domainName strin
432	437
433	438		// Update in-memory state so subsequent lookups are fast
434	439		s.mu.Lock()
	440	+	if _, alreadyTracked := s.containers[domainName]; !alreadyTracked {
	441	+	s.managedCount++
	442	+	}
435	443		s.containers[domainName] = c
436		-	s.managedCount++
437	444		s.mu.Unlock()
438	445
439	446		return c, true

@@ -564,11 +571,12 @@ func (s *Service) activateDeployedContainer(ctx context.Context, domainName stri
564	571		// stabilizeNewContainer monitors the new container briefly after traffic switch.
565	572		// If it crashes during this window, rolls back to old container.
566	573		// Returns true if stabilization succeeded, false if rollback was performed.
567		-	func (s Service) stabilizeNewContainer(ctx context.Context, domainName string, newContainer, oldContainer domain.Container) bool {
	574	+	// Returns an error only when both new and old containers are dead.
	575	+	func (s Service) stabilizeNewContainer(ctx context.Context, domainName string, newContainer, oldContainer domain.Container) (bool, error) {
568	576		log := zerowrap.FromCtx(ctx)
569	577
570	578		if oldContainer == nil {
571		-	return true
	579	+	return true, nil
572	580		}
573	581
574	582		s.mu.RLock()

@@ -582,7 +590,7 @@ func (s *Service) stabilizeNewContainer(ctx context.Context, domainName string,
582	590		select {
583	591		case <-time.After(delay):
584	592		case <-ctx.Done():
585		-	return true
	593	+	return true, nil
586	594		}
587	595
588	596		running, err := s.runtime.IsContainerRunning(ctx, newContainer.ID)

@@ -592,6 +600,22 @@ func (s *Service) stabilizeNewContainer(ctx context.Context, domainName string,
592	600		Str("old_container", oldContainer.ID).
593	601		Msg("new container crashed during stabilization, rolling back to old")
594	602
	603	+	// Verify old container is still running before restoring it.
	604	+	// If both old and new are dead (e.g., OOM), returning a dead
	605	+	// container as "existing" would leave the domain in a broken state.
	606	+	oldRunning, oldErr := s.runtime.IsContainerRunning(ctx, oldContainer.ID)
	607	+	if oldErr != nil \|\| !oldRunning {
	608	+	log.Error().
	609	+	Str("old_container", oldContainer.ID).
	610	+	Msg("old container is also not running, cannot rollback")
	611	+
	612	+	// Cleanup failed new container
	613	+	_ = s.runtime.StopContainer(ctx, newContainer.ID)
	614	+	_ = s.runtime.RemoveContainer(ctx, newContainer.ID, true)
	615	+
	616	+	return false, fmt.Errorf("stabilization failed: new container crashed and old container %s is also not running", oldContainer.ID)
	617	+	}
	618	+
595	619		// Rollback: restore old container as tracked
596	620		s.mu.Lock()
597	621		s.containers[domainName] = oldContainer

@@ -609,10 +633,10 @@ func (s *Service) stabilizeNewContainer(ctx context.Context, domainName string,
609	633		_ = s.runtime.StopContainer(ctx, newContainer.ID)
610	634		_ = s.runtime.RemoveContainer(ctx, newContainer.ID, true)
611	635
612		-	return false
	636	+	return false, nil
613	637		}
614	638
615		-	return true
	639	+	return true, nil
616	640		}
617	641
618	642		func (s Service) finalizePreviousContainer(ctx context.Context, domainName string, existing domain.Container, hasExisting, invalidated bool, newContainerID string) {

 	// Post-switch stabilization: verify new container stays running
 	if hasExisting {
-		if !s.stabilizeNewContainer(ctx, route.Domain, newContainer, existing) {
+		stable, stabilizeErr := s.stabilizeNewContainer(ctx, route.Domain, newContainer, existing)
+		if stabilizeErr != nil {
+			// Both old and new containers are dead
+			return nil, stabilizeErr
+		}
+		if !stable {
 			// Rollback performed — old container is restored
 			return existing, nil
 		}
 			// Update in-memory state so subsequent lookups are fast
 			s.mu.Lock()
+			if _, alreadyTracked := s.containers[domainName]; !alreadyTracked {
+				s.managedCount++
+			}
 			s.containers[domainName] = c
-			s.managedCount++
 			s.mu.Unlock()
 			return c, true
 // stabilizeNewContainer monitors the new container briefly after traffic switch.
 // If it crashes during this window, rolls back to old container.
 // Returns true if stabilization succeeded, false if rollback was performed.
-func (s *Service) stabilizeNewContainer(ctx context.Context, domainName string, newContainer, oldContainer *domain.Container) bool {
+// Returns an error only when both new and old containers are dead.
+func (s *Service) stabilizeNewContainer(ctx context.Context, domainName string, newContainer, oldContainer *domain.Container) (bool, error) {
 	log := zerowrap.FromCtx(ctx)
 	if oldContainer == nil {
-		return true
+		return true, nil
 	}
 	s.mu.RLock()
 	select {
 	case <-time.After(delay):
 	case <-ctx.Done():
-		return true
+		return true, nil
 	}
 	running, err := s.runtime.IsContainerRunning(ctx, newContainer.ID)
 			Str("old_container", oldContainer.ID).
 			Msg("new container crashed during stabilization, rolling back to old")
+		// Verify old container is still running before restoring it.
+		// If both old and new are dead (e.g., OOM), returning a dead
+		// container as "existing" would leave the domain in a broken state.
+		oldRunning, oldErr := s.runtime.IsContainerRunning(ctx, oldContainer.ID)
+		if oldErr != nil || !oldRunning {
+			log.Error().
+				Str("old_container", oldContainer.ID).
+				Msg("old container is also not running, cannot rollback")
+			// Cleanup failed new container
+			_ = s.runtime.StopContainer(ctx, newContainer.ID)
+			_ = s.runtime.RemoveContainer(ctx, newContainer.ID, true)
+			return false, fmt.Errorf("stabilization failed: new container crashed and old container %s is also not running", oldContainer.ID)
+		}
 		// Rollback: restore old container as tracked
 		s.mu.Lock()
 		s.containers[domainName] = oldContainer
 		_ = s.runtime.StopContainer(ctx, newContainer.ID)
 		_ = s.runtime.RemoveContainer(ctx, newContainer.ID, true)
-		return false
+		return false, nil
 	}
-	return true
+	return true, nil
 }
 func (s *Service) finalizePreviousContainer(ctx context.Context, domainName string, existing *domain.Container, hasExisting, invalidated bool, newContainerID string) {

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


@@ -2788,6 +2788,9 @@ func TestService_Deploy_RollbackOnPostSwitchCrash(t *testing.T) {
2788	2788		runtime.EXPECT().IsContainerRunning(mock.Anything, "new-container").Return(true, nil).Times(2)
2789	2789		runtime.EXPECT().IsContainerRunning(mock.Anything, "new-container").Return(false, nil).Once()
2790	2790
	2791	+	// Rollback: verify old container is still running before restoring
	2792	+	runtime.EXPECT().IsContainerRunning(mock.Anything, "old-container").Return(true, nil).Once()
	2793	+
2791	2794		// Inspect after ready
2792	2795		runtime.EXPECT().InspectContainer(mock.Anything, "new-container").Return(&domain.Container{
2793	2796		ID: "new-container",


@@ -2788,6 +2788,9 @@ func TestService_Deploy_RollbackOnPostSwitchCrash(t *testing.T) {
2788	runtime.EXPECT().IsContainerRunning(mock.Anything, "new-container").Return(true, nil).Times(2)	2788		runtime.EXPECT().IsContainerRunning(mock.Anything, "new-container").Return(true, nil).Times(2)
2789	runtime.EXPECT().IsContainerRunning(mock.Anything, "new-container").Return(false, nil).Once()	2789		runtime.EXPECT().IsContainerRunning(mock.Anything, "new-container").Return(false, nil).Once()
2790		2790
		2791	+	// Rollback: verify old container is still running before restoring
		2792	+	runtime.EXPECT().IsContainerRunning(mock.Anything, "old-container").Return(true, nil).Once()
		2793	+
2791	// Inspect after ready	2794		// Inspect after ready
2792	runtime.EXPECT().InspectContainer(mock.Anything, "new-container").Return(&domain.Container{	2795		runtime.EXPECT().InspectContainer(mock.Anything, "new-container").Return(&domain.Container{
2793	ID: "new-container",	2796		ID: "new-container",