fix(preview): address CodeRabbit review feedback

- Derive sweep interval from configured TTL (min 1m, max 1h)
- Reorder orphan teardown: cleanup resources before removing container
- Move ContainerLister interface to boundaries/out
- Don't treat zero Created timestamp as expired
- Use all=true in ListContainers to catch stopped orphans
- Log errors in CollectOrphans instead of silent nil return

commit: b32e7e8fb2f6793b16b0d5c6534883b1a5ad5714
author: brice · 7mo ago · 2026-03-25 06:02 UTC
parent: 470ef27
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

5 files · +28 -16

M internal/app/run.go +16 -5


@@ -587,7 +587,16 @@ func initPreviewService(ctx context.Context, cfg Config, svc *services, log zero
587	587		log.Warn().Err(err).Msg("failed to load previews")
588	588		}
589	589
590		-	svc.previewService.StartTicker(ctx, 1*time.Hour, func(ctx context.Context, p domain.PreviewRoute) {
	590	+	// Derive sweep interval from TTL: half the TTL, capped at 1 hour, minimum 1 minute.
	591	+	sweepInterval := previewConfig.TTL / 2
	592	+	if sweepInterval > 1*time.Hour {
	593	+	sweepInterval = 1 * time.Hour
	594	+	}
	595	+	if sweepInterval < 1*time.Minute {
	596	+	sweepInterval = 1 * time.Minute
	597	+	}
	598	+
	599	+	svc.previewService.StartTicker(ctx, sweepInterval, func(ctx context.Context, p domain.PreviewRoute) {
591	600		teardownTrackedPreview(ctx, svc, p)
592	601		}, func(ctx context.Context) {
593	602		gcOrphanedPreviews(ctx, svc, previewConfig)

@@ -636,18 +645,20 @@ func gcOrphanedPreviews(ctx context.Context, svc *services, previewConfig domain
636	645		log.Warn().Str("container", c.Name).Str("image", c.Image).Str("domain", orphanDomain).
637	646		Time("created", c.Created).Msg("orphaned preview container detected, cleaning up")
638	647
639		-	// Stop and remove container.
	648	+	// Stop container first, then clean up domain resources while container
	649	+	// still exists (so future scans can rediscover it on failure), then remove.
640	650		if err := svc.runtime.StopContainer(ctx, c.Name); err != nil {
641	651		log.Warn().Err(err).Str("container", c.Name).Msg("failed to stop orphan container")
642	652		}
643		-	if err := svc.runtime.RemoveContainer(ctx, c.Name, true); err != nil {
644		-	log.Warn().Err(err).Str("container", c.Name).Msg("failed to remove orphan container")
645		-	}
646	653
647	654		if orphanDomain != "" {
648	655		cleanupOrphanDomainResources(ctx, svc, orphanDomain)
649	656		}
650	657
	658	+	if err := svc.runtime.RemoveContainer(ctx, c.Name, true); err != nil {
	659	+	log.Warn().Err(err).Str("container", c.Name).Msg("failed to remove orphan container")
	660	+	}
	661	+
651	662		log.Info().Str("container", c.Name).Str("domain", orphanDomain).Msg("orphaned preview container cleaned up")
652	663		}
653	664		}

 		log.Warn().Err(err).Msg("failed to load previews")
 	}
-	svc.previewService.StartTicker(ctx, 1*time.Hour, func(ctx context.Context, p domain.PreviewRoute) {
+	// Derive sweep interval from TTL: half the TTL, capped at 1 hour, minimum 1 minute.
+	sweepInterval := previewConfig.TTL / 2
+	if sweepInterval > 1*time.Hour {
+		sweepInterval = 1 * time.Hour
+	}
+	if sweepInterval < 1*time.Minute {
+		sweepInterval = 1 * time.Minute
+	}
+	svc.previewService.StartTicker(ctx, sweepInterval, func(ctx context.Context, p domain.PreviewRoute) {
 		teardownTrackedPreview(ctx, svc, p)
 	}, func(ctx context.Context) {
 		gcOrphanedPreviews(ctx, svc, previewConfig)
 		log.Warn().Str("container", c.Name).Str("image", c.Image).Str("domain", orphanDomain).
 			Time("created", c.Created).Msg("orphaned preview container detected, cleaning up")
-		// Stop and remove container.
+		// Stop container first, then clean up domain resources while container
+		// still exists (so future scans can rediscover it on failure), then remove.
 		if err := svc.runtime.StopContainer(ctx, c.Name); err != nil {
 			log.Warn().Err(err).Str("container", c.Name).Msg("failed to stop orphan container")
 		}
-		if err := svc.runtime.RemoveContainer(ctx, c.Name, true); err != nil {
-			log.Warn().Err(err).Str("container", c.Name).Msg("failed to remove orphan container")
-		}
 		if orphanDomain != "" {
 			cleanupOrphanDomainResources(ctx, svc, orphanDomain)
 		}
+		if err := svc.runtime.RemoveContainer(ctx, c.Name, true); err != nil {
+			log.Warn().Err(err).Str("container", c.Name).Msg("failed to remove orphan container")
+		}
 		log.Info().Str("container", c.Name).Str("domain", orphanDomain).Msg("orphaned preview container cleaned up")
 	}
 }

M internal/boundaries/out/runtime.go +5 -0


@@ -78,6 +78,11 @@ type ContainerRuntime interface {
78	78		DisconnectContainerFromNetwork(ctx context.Context, containerName, networkName string) error
79	79		}
80	80
	81	+	// ContainerLister is the subset of ContainerRuntime needed by the orphan GC.
	82	+	type ContainerLister interface {
	83	+	ListContainers(ctx context.Context, all bool) ([]*domain.Container, error)
	84	+	}
	85	+
81	86		// ExecResult holds the result of executing a command in a container.
82	87		type ExecResult struct {
83	88		ExitCode int


@@ -78,6 +78,11 @@ type ContainerRuntime interface {
78	DisconnectContainerFromNetwork(ctx context.Context, containerName, networkName string) error	78		DisconnectContainerFromNetwork(ctx context.Context, containerName, networkName string) error
79	}	79		}
80		80
		81	+	// ContainerLister is the subset of ContainerRuntime needed by the orphan GC.
		82	+	type ContainerLister interface {
		83	+	ListContainers(ctx context.Context, all bool) ([]*domain.Container, error)
		84	+	}
		85	+
81	// ExecResult holds the result of executing a command in a container.	86		// ExecResult holds the result of executing a command in a container.
82	type ExecResult struct {	87		type ExecResult struct {
83	ExitCode int	88		ExitCode int

M internal/usecase/auto/preview/orphans.go +1 -7

@@ -1,7 +1,6 @@
1	1		package preview
2	2
3	3		import (
4		-	"context"
5	4		"path/filepath"
6	5		"strings"
7	6		"time"

@@ -11,11 +10,6 @@ import (
11	10
12	11		const orphanGracePeriod = 5 * time.Minute
13	12
14		-	// ContainerLister is the subset of ContainerRuntime needed by the orphan GC.
15		-	type ContainerLister interface {
16		-	ListContainers(ctx context.Context, all bool) ([]*domain.Container, error)
17		-	}
18		-
19	13		// IsOrphanPreview returns true if the container looks like a preview (by image
20	14		// tag pattern and domain separator) but is not in the tracked preview list.
21	15		// Containers younger than 5 minutes are skipped to avoid racing with

@@ -56,7 +50,7 @@ func IsOrphanPreview(c *domain.Container, tracked []domain.PreviewRoute, tagPatt
56	50		// its creation time.
57	51		func IsExpiredOrphan(c *domain.Container, ttl time.Duration) bool {
58	52		if c.Created.IsZero() {
59		-	return true // no creation time — treat as expired
	53	+	return false // unknown creation time — don't delete, play it safe
60	54		}
61	55		return time.Since(c.Created) > ttl
62	56		}

 package preview
 import (
-	"context"
 	"path/filepath"
 	"strings"
 	"time"
 const orphanGracePeriod = 5 * time.Minute
-// ContainerLister is the subset of ContainerRuntime needed by the orphan GC.
-type ContainerLister interface {
-	ListContainers(ctx context.Context, all bool) ([]*domain.Container, error)
-}
 // IsOrphanPreview returns true if the container looks like a preview (by image
 // tag pattern and domain separator) but is not in the tracked preview list.
 // Containers younger than 5 minutes are skipped to avoid racing with
 // its creation time.
 func IsExpiredOrphan(c *domain.Container, ttl time.Duration) bool {
 	if c.Created.IsZero() {
-		return true // no creation time — treat as expired
+		return false // unknown creation time — don't delete, play it safe
 	}
 	return time.Since(c.Created) > ttl
 }

M internal/usecase/auto/preview/orphans_test.go +2 -2


@@ -130,9 +130,9 @@ func TestIsExpiredOrphan(t *testing.T) {
130	130		want: false,
131	131		},
132	132		{
133		-	name: "zero time — treat as expired",
	133	+	name: "zero time — unknown, skip to be safe",
134	134		created: time.Time{},
135		-	want: true,
	135	+	want: false,
136	136		},
137	137		}
138	138


@@ -130,9 +130,9 @@ func TestIsExpiredOrphan(t *testing.T) {
130		want: false,	130		want: false,
131		},	131		},
132		{	132		{
133	-	name: "zero time — treat as expired",	133	+	name: "zero time — unknown, skip to be safe",
134		created: time.Time{},	134		created: time.Time{},
135	-	want: true,	135	+	want: false,
136		},	136		},
137		}	137		}
138			138

M internal/usecase/auto/preview/service.go +4 -2


@@ -228,9 +228,11 @@ func (s *Service) CleanupExpired(ctx context.Context) []domain.PreviewRoute {
228	228		// CollectOrphans lists Docker containers and returns those that look like
229	229		// expired preview orphans (not tracked, matching tag pattern + separator,
230	230		// past TTL based on container creation time).
231		-	func (s Service) CollectOrphans(ctx context.Context, lister ContainerLister, tagPatterns []string, separator string) []domain.Container {
232		-	containers, err := lister.ListContainers(ctx, false)
	231	+	func (s Service) CollectOrphans(ctx context.Context, lister out.ContainerLister, tagPatterns []string, separator string) []domain.Container {
	232	+	containers, err := lister.ListContainers(ctx, true)
233	233		if err != nil {
	234	+	log := zerowrap.FromCtx(ctx)
	235	+	log.Warn().Err(err).Msg("orphan GC: failed to list containers")
234	236		return nil
235	237		}
236	238

 // CollectOrphans lists Docker containers and returns those that look like
 // expired preview orphans (not tracked, matching tag pattern + separator,
 // past TTL based on container creation time).
-func (s *Service) CollectOrphans(ctx context.Context, lister ContainerLister, tagPatterns []string, separator string) []*domain.Container {
+func (s *Service) CollectOrphans(ctx context.Context, lister out.ContainerLister, tagPatterns []string, separator string) []*domain.Container {
-	containers, err := lister.ListContainers(ctx, false)
+	containers, err := lister.ListContainers(ctx, true)
 	if err != nil {
+		log := zerowrap.FromCtx(ctx)
+		log.Warn().Err(err).Msg("orphan GC: failed to list containers")
 		return nil
 	}