fix: address code review findings from CR

- registry: fix timer identity check in SuppressDeployEvent to prevent
  stale AfterFunc callbacks from evicting a newer suppression entry
- registry: add ExtractImageName helper for canonical image ref parsing
- admin/handler: add auth check, registrySvc nil-guard, and URL-decode
  for imageName in handleDeployIntent
- admin/handler: use ExtractImageName and registrySvc nil-guard when
  clearing suppression after explicit deploy
- container: assign stabilizeErr to named return err so deferred
  recordDeployMetrics and span see stabilization failures
- container: skip all running containers (not just canonical) in
  cleanupOrphanedContainers to protect active temp containers
- container: log StopContainer/RemoveContainer errors in rollback paths
  instead of silently discarding them

commit: 3bec053e2a173cd03b5b79931ffa82fc89343b8a
author: brice · 7mo ago · 2026-03-04 08:42 UTC
parent: 68b8dd1
signature: unsigned

3 files · +87 -34

M internal/adapters/in/http/admin/handler.go +25 -14


@@ -7,6 +7,7 @@ import (
7	7		"fmt"
8	8		"io"
9	9		"net/http"
	10	+	"net/url"
10	11		"strconv"
11	12		"strings"
12	13		"time"

@@ -237,13 +238,30 @@ func (h Handler) handleDeployIntent(w http.ResponseWriter, r http.Request, pat
237	238		return
238	239		}
239	240
240		-	imageName := strings.TrimPrefix(path, "/deploy-intent/")
241		-	if imageName == "" \|\| imageName == "/deploy-intent" {
	241	+	ctx := r.Context()
	242	+	if !HasAccess(ctx, domain.AdminResourceConfig, domain.AdminActionWrite) {
	243	+	h.sendError(w, http.StatusForbidden, "insufficient permissions for config:write")
	244	+	return
	245	+	}
	246	+
	247	+	if h.registrySvc == nil {
	248	+	h.sendError(w, http.StatusServiceUnavailable, "registry service unavailable")
	249	+	return
	250	+	}
	251	+
	252	+	rawName := strings.TrimPrefix(path, "/deploy-intent/")
	253	+	if rawName == "" \|\| rawName == "/deploy-intent" {
242	254		h.sendError(w, http.StatusBadRequest, "image name required")
243	255		return
244	256		}
245	257
246		-	log := zerowrap.FromCtx(r.Context())
	258	+	imageName, err := url.PathUnescape(rawName)
	259	+	if err != nil {
	260	+	h.sendError(w, http.StatusBadRequest, "invalid image name encoding")
	261	+	return
	262	+	}
	263	+
	264	+	log := zerowrap.FromCtx(ctx)
247	265		log.Info().Str("image", imageName).Msg("deploy intent registered, suppressing image.pushed events")
248	266
249	267		h.registrySvc.SuppressDeployEvent(imageName)

@@ -1106,17 +1124,10 @@ func (h Handler) handleDeploy(w http.ResponseWriter, r http.Request, path stri
1106	1124
1107	1125		// Clear deploy event suppression now that the explicit deploy has completed.
1108	1126		// This re-enables event-based deploys for future direct docker pushes.
1109		-	if route.Image != "" {
1110		-	// Extract image name (without registry prefix and tag) for suppression key
1111		-	imageName := route.Image
1112		-	// The suppression key is just the image name as stored in the registry
1113		-	// (e.g., "my-app" not "reg.example.com/my-app:latest")
1114		-	if parts := strings.SplitN(imageName, "/", 2); len(parts) == 2 {
1115		-	imageName = parts[1]
1116		-	}
1117		-	if idx := strings.LastIndex(imageName, ":"); idx != -1 {
1118		-	imageName = imageName[:idx]
1119		-	}
	1127	+	if route.Image != "" && h.registrySvc != nil {
	1128	+	// Use the registry package's image name normaliser so digest-form refs
	1129	+	// and multi-segment paths are handled correctly.
	1130	+	imageName := registry.ExtractImageName(route.Image)
1120	1131		h.registrySvc.ClearDeployEventSuppression(imageName)
1121	1132		}
1122	1133

 	"fmt"
 	"io"
 	"net/http"
+	"net/url"
 	"strconv"
 	"strings"
 	"time"
 		return
 	}
-	imageName := strings.TrimPrefix(path, "/deploy-intent/")
+	ctx := r.Context()
-	if imageName == "" || imageName == "/deploy-intent" {
+	if !HasAccess(ctx, domain.AdminResourceConfig, domain.AdminActionWrite) {
+		h.sendError(w, http.StatusForbidden, "insufficient permissions for config:write")
+		return
+	}
+	if h.registrySvc == nil {
+		h.sendError(w, http.StatusServiceUnavailable, "registry service unavailable")
+		return
+	}
+	rawName := strings.TrimPrefix(path, "/deploy-intent/")
+	if rawName == "" || rawName == "/deploy-intent" {
 		h.sendError(w, http.StatusBadRequest, "image name required")
 		return
 	}
-	log := zerowrap.FromCtx(r.Context())
+	imageName, err := url.PathUnescape(rawName)
+	if err != nil {
+		h.sendError(w, http.StatusBadRequest, "invalid image name encoding")
+		return
+	}
+	log := zerowrap.FromCtx(ctx)
 	log.Info().Str("image", imageName).Msg("deploy intent registered, suppressing image.pushed events")
 	h.registrySvc.SuppressDeployEvent(imageName)
 	// Clear deploy event suppression now that the explicit deploy has completed.
 	// This re-enables event-based deploys for future direct docker pushes.
-	if route.Image != "" {
+	if route.Image != "" && h.registrySvc != nil {
-		// Extract image name (without registry prefix and tag) for suppression key
+		// Use the registry package's image name normaliser so digest-form refs
-		imageName := route.Image
+		// and multi-segment paths are handled correctly.
-		// The suppression key is just the image name as stored in the registry
+		imageName := registry.ExtractImageName(route.Image)
-		// (e.g., "my-app" not "reg.example.com/my-app:latest")
-		if parts := strings.SplitN(imageName, "/", 2); len(parts) == 2 {
-			imageName = parts[1]
-		}
-		if idx := strings.LastIndex(imageName, ":"); idx != -1 {
-			imageName = imageName[:idx]
-		}
 		h.registrySvc.ClearDeployEventSuppression(imageName)
 	}

M internal/usecase/container/service.go +21 -18


@@ -295,8 +295,10 @@ func (s Service) Deploy(ctx context.Context, route domain.Route) (domain.Conta
295	295		if hasExisting {
296	296		stable, stabilizeErr := s.stabilizeNewContainer(ctx, route.Domain, newContainer, existing)
297	297		if stabilizeErr != nil {
298		-	// Both old and new containers are dead
299		-	return nil, stabilizeErr
	298	+	// Both old and new containers are dead; assign to named return so
	299	+	// the deferred recordDeployMetrics and span see the failure.
	300	+	err = stabilizeErr
	301	+	return nil, err
300	302		}
301	303		if !stable {
302	304		// Rollback performed — old container is restored

@@ -627,8 +629,12 @@ func (s *Service) stabilizeNewContainer(ctx context.Context, domainName string,
627	629		Msg("old container is also not running, cannot rollback")
628	630
629	631		// Cleanup failed new container
630		-	_ = s.runtime.StopContainer(ctx, newContainer.ID)
631		-	_ = s.runtime.RemoveContainer(ctx, newContainer.ID, true)
	632	+	if stopErr := s.runtime.StopContainer(ctx, newContainer.ID); stopErr != nil {
	633	+	log.WrapErrWithFields(stopErr, "failed to stop failed new container during rollback", map[string]any{zerowrap.FieldEntityID: newContainer.ID})
	634	+	}
	635	+	if removeErr := s.runtime.RemoveContainer(ctx, newContainer.ID, true); removeErr != nil {
	636	+	log.WrapErrWithFields(removeErr, "failed to remove failed new container during rollback", map[string]any{zerowrap.FieldEntityID: newContainer.ID})
	637	+	}
632	638
633	639		return false, fmt.Errorf("stabilization failed: new container crashed and old container %s is also not running", oldContainer.ID)
634	640		}

@@ -647,8 +653,12 @@ func (s *Service) stabilizeNewContainer(ctx context.Context, domainName string,
647	653		}
648	654
649	655		// Cleanup failed new container
650		-	_ = s.runtime.StopContainer(ctx, newContainer.ID)
651		-	_ = s.runtime.RemoveContainer(ctx, newContainer.ID, true)
	656	+	if stopErr := s.runtime.StopContainer(ctx, newContainer.ID); stopErr != nil {
	657	+	log.WrapErrWithFields(stopErr, "failed to stop failed new container during rollback", map[string]any{zerowrap.FieldEntityID: newContainer.ID})
	658	+	}
	659	+	if removeErr := s.runtime.RemoveContainer(ctx, newContainer.ID, true); removeErr != nil {
	660	+	log.WrapErrWithFields(removeErr, "failed to remove failed new container during rollback", map[string]any{zerowrap.FieldEntityID: newContainer.ID})
	661	+	}
652	662
653	663		return false, nil
654	664		}

@@ -1763,22 +1773,15 @@ func (s *Service) cleanupOrphanedContainers(ctx context.Context, domainName stri
1763	1773
1764	1774		for _, c := range allContainers {
1765	1775		if (c.Name == expectedName \|\| c.Name == expectedNewName \|\| c.Name == expectedNextName) && c.ID != skipContainerID {
1766		-	// The skipContainerID is the container we resolved as the active
1767		-	// one serving traffic. Any other container with a matching name is
1768		-	// an orphan from a previous (possibly interrupted) deploy.
1769		-	//
1770		-	// Running containers with the canonical name are protected — they
1771		-	// could be legitimate if resolveExistingContainer found a temp
1772		-	// container instead. Running temp containers (-new/-next) that are
1773		-	// NOT the skip container are safe to remove: they are leftovers
1774		-	// from failed deploys.
1775		-	isCanonical := c.Name == expectedName
1776		-	if isCanonical && c.Status == "running" {
	1776	+	// Skip any running container regardless of name — a running
	1777	+	// temp container may be actively serving traffic while the
	1778	+	// new container stabilizes, or may be from a concurrent deploy.
	1779	+	if c.Status == "running" {
1777	1780		log.Debug().
1778	1781		Str(zerowrap.FieldEntityID, c.ID).
1779	1782		Str("container_name", c.Name).
1780	1783		Str(zerowrap.FieldStatus, c.Status).
1781		-	Msg("skipping running canonical container during orphan cleanup")
	1784	+	Msg("skipping running container during orphan cleanup")
1782	1785		continue
1783	1786		}
1784	1787

 	if hasExisting {
 		stable, stabilizeErr := s.stabilizeNewContainer(ctx, route.Domain, newContainer, existing)
 		if stabilizeErr != nil {
-			// Both old and new containers are dead
+			// Both old and new containers are dead; assign to named return so
-			return nil, stabilizeErr
+			// the deferred recordDeployMetrics and span see the failure.
+			err = stabilizeErr
+			return nil, err
 		}
 		if !stable {
 			// Rollback performed — old container is restored
 				Msg("old container is also not running, cannot rollback")
 			// Cleanup failed new container
-			_ = s.runtime.StopContainer(ctx, newContainer.ID)
+			if stopErr := s.runtime.StopContainer(ctx, newContainer.ID); stopErr != nil {
-			_ = s.runtime.RemoveContainer(ctx, newContainer.ID, true)
+				log.WrapErrWithFields(stopErr, "failed to stop failed new container during rollback", map[string]any{zerowrap.FieldEntityID: newContainer.ID})
+			}
+			if removeErr := s.runtime.RemoveContainer(ctx, newContainer.ID, true); removeErr != nil {
+				log.WrapErrWithFields(removeErr, "failed to remove failed new container during rollback", map[string]any{zerowrap.FieldEntityID: newContainer.ID})
+			}
 			return false, fmt.Errorf("stabilization failed: new container crashed and old container %s is also not running", oldContainer.ID)
 		}
 		}
 		// Cleanup failed new container
-		_ = s.runtime.StopContainer(ctx, newContainer.ID)
+		if stopErr := s.runtime.StopContainer(ctx, newContainer.ID); stopErr != nil {
-		_ = s.runtime.RemoveContainer(ctx, newContainer.ID, true)
+			log.WrapErrWithFields(stopErr, "failed to stop failed new container during rollback", map[string]any{zerowrap.FieldEntityID: newContainer.ID})
+		}
+		if removeErr := s.runtime.RemoveContainer(ctx, newContainer.ID, true); removeErr != nil {
+			log.WrapErrWithFields(removeErr, "failed to remove failed new container during rollback", map[string]any{zerowrap.FieldEntityID: newContainer.ID})
+		}
 		return false, nil
 	}
 	for _, c := range allContainers {
 		if (c.Name == expectedName || c.Name == expectedNewName || c.Name == expectedNextName) && c.ID != skipContainerID {
-			// The skipContainerID is the container we resolved as the active
+			// Skip any running container regardless of name — a running
-			// one serving traffic. Any other container with a matching name is
+			// temp container may be actively serving traffic while the
-			// an orphan from a previous (possibly interrupted) deploy.
+			// new container stabilizes, or may be from a concurrent deploy.
-			//
+			if c.Status == "running" {
-			// Running containers with the canonical name are protected — they
-			// could be legitimate if resolveExistingContainer found a temp
-			// container instead. Running temp containers (-new/-next) that are
-			// NOT the skip container are safe to remove: they are leftovers
-			// from failed deploys.
-			isCanonical := c.Name == expectedName
-			if isCanonical && c.Status == "running" {
 				log.Debug().
 					Str(zerowrap.FieldEntityID, c.ID).
 					Str("container_name", c.Name).
 					Str(zerowrap.FieldStatus, c.Status).
-					Msg("skipping running canonical container during orphan cleanup")
+					Msg("skipping running container during orphan cleanup")
 				continue
 			}

M internal/usecase/registry/service.go +41 -2


@@ -54,8 +54,13 @@ func NewService(
54	54		// SuppressDeployEvent marks an image name to skip image.pushed events.
55	55		// The suppression auto-expires after 2 minutes to prevent leaks.
56	56		func (s *Service) SuppressDeployEvent(imageName string) {
57		-	timer := time.AfterFunc(2*time.Minute, func() {
58		-	s.suppressedImages.Delete(imageName)
	57	+	var timer *time.Timer
	58	+	timer = time.AfterFunc(2*time.Minute, func() {
	59	+	// Only delete if this timer is still the current one, preventing an
	60	+	// old timer's callback from removing a newer suppression entry.
	61	+	if v, ok := s.suppressedImages.Load(imageName); ok && v == timer {
	62	+	s.suppressedImages.Delete(imageName)
	63	+	}
59	64		})
60	65		if existing, loaded := s.suppressedImages.LoadOrStore(imageName, timer); loaded {
61	66		existing.(*time.Timer).Stop()

@@ -70,6 +75,40 @@ func (s *Service) ClearDeployEventSuppression(imageName string) {
70	75		}
71	76		}
72	77
	78	+	// ExtractImageName returns just the repository path of a container image
	79	+	// reference, stripping any registry host prefix, tag, and digest.
	80	+	// Examples:
	81	+	//
	82	+	// "reg.example.com/team/my-app:latest" -> "team/my-app"
	83	+	// "reg.example.com/my-app@sha256:abc" -> "my-app"
	84	+	// "my-app:v1.2" -> "my-app"
	85	+	func ExtractImageName(imageRef string) string {
	86	+	name := imageRef
	87	+	// Strip digest
	88	+	if idx := strings.Index(name, "@"); idx != -1 {
	89	+	name = name[:idx]
	90	+	}
	91	+	// Strip tag
	92	+	// Find the last colon, but only strip it if it comes after any slash
	93	+	// (to avoid treating a port number in the host as a tag).
	94	+	if idx := strings.LastIndex(name, ":"); idx != -1 {
	95	+	slashIdx := strings.LastIndex(name, "/")
	96	+	if idx > slashIdx {
	97	+	name = name[:idx]
	98	+	}
	99	+	}
	100	+	// Strip registry host: if the first segment contains a dot or colon it is
	101	+	// a registry hostname; remove it.
	102	+	parts := strings.SplitN(name, "/", 2)
	103	+	if len(parts) == 2 {
	104	+	host := parts[0]
	105	+	if strings.ContainsAny(host, ".:") \|\| host == "localhost" {
	106	+	name = parts[1]
	107	+	}
	108	+	}
	109	+	return name
	110	+	}
	111	+
73	112		// IsDeployEventSuppressed checks if deploy events are suppressed for an image.
74	113		func (s *Service) IsDeployEventSuppressed(imageName string) bool {
75	114		_, exists := s.suppressedImages.Load(imageName)


@@ -54,8 +54,13 @@ func NewService(
54		// SuppressDeployEvent marks an image name to skip image.pushed events.	54		// SuppressDeployEvent marks an image name to skip image.pushed events.
55		// The suppression auto-expires after 2 minutes to prevent leaks.	55		// The suppression auto-expires after 2 minutes to prevent leaks.
56		func (s *Service) SuppressDeployEvent(imageName string) {	56		func (s *Service) SuppressDeployEvent(imageName string) {
57	-	timer := time.AfterFunc(2*time.Minute, func() {	57	+	var timer *time.Timer
58	-	s.suppressedImages.Delete(imageName)	58	+	timer = time.AfterFunc(2*time.Minute, func() {
			59	+	// Only delete if this timer is still the current one, preventing an
			60	+	// old timer's callback from removing a newer suppression entry.
			61	+	if v, ok := s.suppressedImages.Load(imageName); ok && v == timer {
			62	+	s.suppressedImages.Delete(imageName)
			63	+	}
59		})	64		})
60		if existing, loaded := s.suppressedImages.LoadOrStore(imageName, timer); loaded {	65		if existing, loaded := s.suppressedImages.LoadOrStore(imageName, timer); loaded {
61		existing.(*time.Timer).Stop()	66		existing.(*time.Timer).Stop()

@@ -70,6 +75,40 @@ func (s *Service) ClearDeployEventSuppression(imageName string) {
70		}	75		}
71		}	76		}
72			77
			78	+	// ExtractImageName returns just the repository path of a container image
			79	+	// reference, stripping any registry host prefix, tag, and digest.
			80	+	// Examples:
			81	+	//
			82	+	// "reg.example.com/team/my-app:latest" -> "team/my-app"
			83	+	// "reg.example.com/my-app@sha256:abc" -> "my-app"
			84	+	// "my-app:v1.2" -> "my-app"
			85	+	func ExtractImageName(imageRef string) string {
			86	+	name := imageRef
			87	+	// Strip digest
			88	+	if idx := strings.Index(name, "@"); idx != -1 {
			89	+	name = name[:idx]
			90	+	}
			91	+	// Strip tag
			92	+	// Find the last colon, but only strip it if it comes after any slash
			93	+	// (to avoid treating a port number in the host as a tag).
			94	+	if idx := strings.LastIndex(name, ":"); idx != -1 {
			95	+	slashIdx := strings.LastIndex(name, "/")
			96	+	if idx > slashIdx {
			97	+	name = name[:idx]
			98	+	}
			99	+	}
			100	+	// Strip registry host: if the first segment contains a dot or colon it is
			101	+	// a registry hostname; remove it.
			102	+	parts := strings.SplitN(name, "/", 2)
			103	+	if len(parts) == 2 {
			104	+	host := parts[0]
			105	+	if strings.ContainsAny(host, ".:") \|\| host == "localhost" {
			106	+	name = parts[1]
			107	+	}
			108	+	}
			109	+	return name
			110	+	}
			111	+
73		// IsDeployEventSuppressed checks if deploy events are suppressed for an image.	112		// IsDeployEventSuppressed checks if deploy events are suppressed for an image.
74		func (s *Service) IsDeployEventSuppressed(imageName string) bool {	113		func (s *Service) IsDeployEventSuppressed(imageName string) bool {
75		_, exists := s.suppressedImages.Load(imageName)	114		_, exists := s.suppressedImages.Load(imageName)