9ef2c23 refactor: address code review findings for env-aware redeploy · gordon

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


@@ -273,7 +273,7 @@ func Run(ctx context.Context, configPath string) error {
273	273		registryHandler, proxyHandler := createHTTPHandlers(svc, cfg, log)
274	274
275	275		// Start servers, wait for listeners to bind, then sync/auto-start containers.
276		-	return runServers(ctx, v, cfg, registryHandler, proxyHandler, svc.containerSvc, svc.eventBus, svc, cleanupHandlers, log)
	276	+	return runServers(ctx, v, cfg, registryHandler, proxyHandler, svc, cleanupHandlers, log)
277	277		}
278	278
279	279		func ensureTLSConfig(cfg *Config, log zerowrap.Logger) error {

@@ -1329,7 +1329,7 @@ func registerEventHandlers(ctx context.Context, svc *services, cfg Config) (func
1329	1329		return nil, fmt.Errorf("failed to subscribe manual deploy handler: %w", err)
1330	1330		}
1331	1331
1332		-	secretsChangedHandler := container.NewSecretsChangedHandler(ctx, svc.containerSvc, svc.configSvc, 60*time.Second)
	1332	+	secretsChangedHandler := container.NewSecretsChangedHandler(ctx, svc.containerSvc, svc.configSvc, container.DefaultSecretsDebounce)
1333	1333		if err := svc.eventBus.Subscribe(secretsChangedHandler); err != nil {
1334	1334		return nil, fmt.Errorf("failed to subscribe secrets changed handler: %w", err)
1335	1335		}

@@ -1701,7 +1701,7 @@ func isLoopbackHost(host string) bool {
1701	1701		// - SIGUSR2: Triggers manual deploy for a specific route
1702	1702		// The deferred signal.Stop calls ensure signal handlers are properly
1703	1703		// cleaned up before program exit, preventing signal handler leaks.
1704		-	func runServers(ctx context.Context, v viper.Viper, cfg Config, registryHandler, proxyHandler http.Handler, containerSvc container.Service, eventBus out.EventBus, svc *services, cleanupHandlers func(), log zerowrap.Logger) error {
	1704	+	func runServers(ctx context.Context, v viper.Viper, cfg Config, registryHandler, proxyHandler http.Handler, svc services, cleanupHandlers func(), log zerowrap.Logger) error {
1705	1705		ctx, cancel := signal.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM)
1706	1706		defer cancel()
1707	1707

@@ -1771,9 +1771,9 @@ func runServers(ctx context.Context, v *viper.Viper, cfg Config, registryHandler
1771	1771		// Auto-start after servers are listening (registry port is now bound).
1772	1772		syncAndAutoStart(ctx, svc, log)
1773	1773
1774		-	waitForShutdown(ctx, errChan, reloadChan, deployChan, eventBus, log)
	1774	+	waitForShutdown(ctx, errChan, reloadChan, deployChan, svc.eventBus, log)
1775	1775		cleanupHandlers() // Stop debounce timers before draining containers
1776		-	gracefulShutdown(registrySrv, proxySrv, tlsSrv, containerSvc, svc.proxySvc, log)
	1776	+	gracefulShutdown(registrySrv, proxySrv, tlsSrv, svc.containerSvc, svc.proxySvc, log)
1777	1777		return nil
1778	1778		}
1779	1779


@@ -273,7 +273,7 @@ func Run(ctx context.Context, configPath string) error {
273		registryHandler, proxyHandler := createHTTPHandlers(svc, cfg, log)	273		registryHandler, proxyHandler := createHTTPHandlers(svc, cfg, log)
274			274
275		// Start servers, wait for listeners to bind, then sync/auto-start containers.	275		// Start servers, wait for listeners to bind, then sync/auto-start containers.
276	-	return runServers(ctx, v, cfg, registryHandler, proxyHandler, svc.containerSvc, svc.eventBus, svc, cleanupHandlers, log)	276	+	return runServers(ctx, v, cfg, registryHandler, proxyHandler, svc, cleanupHandlers, log)
277		}	277		}
278			278
279		func ensureTLSConfig(cfg *Config, log zerowrap.Logger) error {	279		func ensureTLSConfig(cfg *Config, log zerowrap.Logger) error {

@@ -1329,7 +1329,7 @@ func registerEventHandlers(ctx context.Context, svc *services, cfg Config) (func
1329		return nil, fmt.Errorf("failed to subscribe manual deploy handler: %w", err)	1329		return nil, fmt.Errorf("failed to subscribe manual deploy handler: %w", err)
1330		}	1330		}
1331			1331
1332	-	secretsChangedHandler := container.NewSecretsChangedHandler(ctx, svc.containerSvc, svc.configSvc, 60*time.Second)	1332	+	secretsChangedHandler := container.NewSecretsChangedHandler(ctx, svc.containerSvc, svc.configSvc, container.DefaultSecretsDebounce)
1333		if err := svc.eventBus.Subscribe(secretsChangedHandler); err != nil {	1333		if err := svc.eventBus.Subscribe(secretsChangedHandler); err != nil {
1334		return nil, fmt.Errorf("failed to subscribe secrets changed handler: %w", err)	1334		return nil, fmt.Errorf("failed to subscribe secrets changed handler: %w", err)
1335		}	1335		}

@@ -1701,7 +1701,7 @@ func isLoopbackHost(host string) bool {
1701		// - SIGUSR2: Triggers manual deploy for a specific route	1701		// - SIGUSR2: Triggers manual deploy for a specific route
1702		// The deferred signal.Stop calls ensure signal handlers are properly	1702		// The deferred signal.Stop calls ensure signal handlers are properly
1703		// cleaned up before program exit, preventing signal handler leaks.	1703		// cleaned up before program exit, preventing signal handler leaks.
1704	-	func runServers(ctx context.Context, v viper.Viper, cfg Config, registryHandler, proxyHandler http.Handler, containerSvc container.Service, eventBus out.EventBus, svc *services, cleanupHandlers func(), log zerowrap.Logger) error {	1704	+	func runServers(ctx context.Context, v viper.Viper, cfg Config, registryHandler, proxyHandler http.Handler, svc services, cleanupHandlers func(), log zerowrap.Logger) error {
1705		ctx, cancel := signal.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM)	1705		ctx, cancel := signal.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM)
1706		defer cancel()	1706		defer cancel()
1707			1707

@@ -1771,9 +1771,9 @@ func runServers(ctx context.Context, v *viper.Viper, cfg Config, registryHandler
1771		// Auto-start after servers are listening (registry port is now bound).	1771		// Auto-start after servers are listening (registry port is now bound).
1772		syncAndAutoStart(ctx, svc, log)	1772		syncAndAutoStart(ctx, svc, log)
1773			1773
1774	-	waitForShutdown(ctx, errChan, reloadChan, deployChan, eventBus, log)	1774	+	waitForShutdown(ctx, errChan, reloadChan, deployChan, svc.eventBus, log)
1775		cleanupHandlers() // Stop debounce timers before draining containers	1775		cleanupHandlers() // Stop debounce timers before draining containers
1776	-	gracefulShutdown(registrySrv, proxySrv, tlsSrv, containerSvc, svc.proxySvc, log)	1776	+	gracefulShutdown(registrySrv, proxySrv, tlsSrv, svc.containerSvc, svc.proxySvc, log)
1777		return nil	1777		return nil
1778		}	1778		}
1779			1779

M internal/usecase/container/secrets_handler.go +2 -0


@@ -11,6 +11,8 @@ import (
11	11		"github.com/bnema/gordon/internal/domain"
12	12		)
13	13
	14	+	const DefaultSecretsDebounce = 60 * time.Second
	15	+
14	16		// SecretsChangedHandler handles secrets.changed events with per-domain
15	17		// debouncing. When secrets are modified rapidly (e.g. multiple
16	18		// "gordon secrets set" calls), the handler resets a timer on each


@@ -11,6 +11,8 @@ import (
11	"github.com/bnema/gordon/internal/domain"	11		"github.com/bnema/gordon/internal/domain"
12	)	12		)
13		13
		14	+	const DefaultSecretsDebounce = 60 * time.Second
		15	+
14	// SecretsChangedHandler handles secrets.changed events with per-domain	16		// SecretsChangedHandler handles secrets.changed events with per-domain
15	// debouncing. When secrets are modified rapidly (e.g. multiple	17		// debouncing. When secrets are modified rapidly (e.g. multiple
16	// "gordon secrets set" calls), the handler resets a timer on each	18		// "gordon secrets set" calls), the handler resets a timer on each

M internal/usecase/container/secrets_handler_test.go +16 -5


@@ -8,6 +8,7 @@ import (
8	8
9	9		"github.com/stretchr/testify/assert"
10	10		"github.com/stretchr/testify/mock"
	11	+	"github.com/stretchr/testify/require"
11	12
12	13		inmocks "github.com/bnema/gordon/internal/boundaries/in/mocks"
13	14		"github.com/bnema/gordon/internal/domain"

@@ -55,8 +56,9 @@ func TestSecretsChangedHandler_DebouncesBurstEvents(t *testing.T) {
55	56		time.Sleep(10 * time.Millisecond)
56	57		}
57	58
58		-	time.Sleep(debounceDelay + 100*time.Millisecond)
59		-	assert.Equal(t, int32(1), deployCalls.Load())
	59	+	require.Eventually(t, func() bool {
	60	+	return deployCalls.Load() == 1
	61	+	}, debounceDelay+time.Second, 10*time.Millisecond, "expected exactly 1 deploy call after debounce")
60	62		}
61	63
62	64		func TestSecretsChangedHandler_IndependentDomains(t *testing.T) {

@@ -73,8 +75,15 @@ func TestSecretsChangedHandler_IndependentDomains(t *testing.T) {
73	75		configSvc.EXPECT().GetRoute(mock.Anything, routeOne.Domain).Return(routeOne, nil).Once()
74	76		configSvc.EXPECT().GetRoute(mock.Anything, routeTwo.Domain).Return(routeTwo, nil).Once()
75	77
76		-	containerSvc.EXPECT().Deploy(mock.Anything, *routeOne).Return(&domain.Container{ID: "container-1"}, nil).Once()
77		-	containerSvc.EXPECT().Deploy(mock.Anything, *routeTwo).Return(&domain.Container{ID: "container-2"}, nil).Once()
	78	+	var deployCalls atomic.Int32
	79	+	containerSvc.EXPECT().Deploy(mock.Anything, routeOne).RunAndReturn(func(_ context.Context, _ domain.Route) (domain.Container, error) {
	80	+	deployCalls.Add(1)
	81	+	return &domain.Container{ID: "container-1"}, nil
	82	+	}).Once()
	83	+	containerSvc.EXPECT().Deploy(mock.Anything, routeTwo).RunAndReturn(func(_ context.Context, _ domain.Route) (domain.Container, error) {
	84	+	deployCalls.Add(1)
	85	+	return &domain.Container{ID: "container-2"}, nil
	86	+	}).Once()
78	87
79	88		assert.NoError(t, h.Handle(context.Background(), domain.Event{
80	89		Type: domain.EventSecretsChanged,

@@ -85,7 +94,9 @@ func TestSecretsChangedHandler_IndependentDomains(t *testing.T) {
85	94		Data: domain.SecretsChangedPayload{Domain: routeTwo.Domain, Operation: "delete", Keys: []string{"B"}},
86	95		}))
87	96
88		-	time.Sleep(debounceDelay + 100*time.Millisecond)
	97	+	require.Eventually(t, func() bool {
	98	+	return deployCalls.Load() == 2
	99	+	}, debounceDelay+time.Second, 10*time.Millisecond, "expected 2 independent deploy calls")
89	100		}
90	101
91	102		func TestSecretsChangedHandler_NoRouteFound_NoOp(t *testing.T) {

 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
 	inmocks "github.com/bnema/gordon/internal/boundaries/in/mocks"
 	"github.com/bnema/gordon/internal/domain"
 		time.Sleep(10 * time.Millisecond)
 	}
-	time.Sleep(debounceDelay + 100*time.Millisecond)
+	require.Eventually(t, func() bool {
-	assert.Equal(t, int32(1), deployCalls.Load())
+		return deployCalls.Load() == 1
+	}, debounceDelay+time.Second, 10*time.Millisecond, "expected exactly 1 deploy call after debounce")
 }
 func TestSecretsChangedHandler_IndependentDomains(t *testing.T) {
 	configSvc.EXPECT().GetRoute(mock.Anything, routeOne.Domain).Return(routeOne, nil).Once()
 	configSvc.EXPECT().GetRoute(mock.Anything, routeTwo.Domain).Return(routeTwo, nil).Once()
-	containerSvc.EXPECT().Deploy(mock.Anything, *routeOne).Return(&domain.Container{ID: "container-1"}, nil).Once()
+	var deployCalls atomic.Int32
-	containerSvc.EXPECT().Deploy(mock.Anything, *routeTwo).Return(&domain.Container{ID: "container-2"}, nil).Once()
+	containerSvc.EXPECT().Deploy(mock.Anything, *routeOne).RunAndReturn(func(_ context.Context, _ domain.Route) (*domain.Container, error) {
+		deployCalls.Add(1)
+		return &domain.Container{ID: "container-1"}, nil
+	}).Once()
+	containerSvc.EXPECT().Deploy(mock.Anything, *routeTwo).RunAndReturn(func(_ context.Context, _ domain.Route) (*domain.Container, error) {
+		deployCalls.Add(1)
+		return &domain.Container{ID: "container-2"}, nil
+	}).Once()
 	assert.NoError(t, h.Handle(context.Background(), domain.Event{
 		Type: domain.EventSecretsChanged,
 		Data: domain.SecretsChangedPayload{Domain: routeTwo.Domain, Operation: "delete", Keys: []string{"B"}},
 	}))
-	time.Sleep(debounceDelay + 100*time.Millisecond)
+	require.Eventually(t, func() bool {
+		return deployCalls.Load() == 2
+	}, debounceDelay+time.Second, 10*time.Millisecond, "expected 2 independent deploy calls")
 }
 func TestSecretsChangedHandler_NoRouteFound_NoOp(t *testing.T) {

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


@@ -2164,22 +2164,35 @@ func (s Service) handleRunningAttachment(ctx context.Context, existing domain.
2164	2164		}
2165	2165
2166	2166		log := zerowrap.FromCtx(ctx)
	2167	+
	2168	+	// Check image drift
	2169	+	existingImage := existing.Labels[domain.LabelImage]
	2170	+	imageDrifted := existingImage != "" && existingImage != serviceImage
	2171	+
	2172	+	// Check env drift
2167	2173		envDrifted, err := s.attachmentEnvDrifted(ctx, existing, containerName, serviceImage)
2168	2174		if err != nil {
2169	2175		log.WrapErr(err, "failed to check attachment env drift")
2170	2176		return true, nil
2171	2177		}
2172		-	if !envDrifted {
2173		-	log.Debug().Str("container_name", containerName).Msg("attachment already running with current env, skipping")
	2178	+
	2179	+	if !envDrifted && !imageDrifted {
	2180	+	log.Debug().Str("container_name", containerName).Msg("attachment already running with current env and image, skipping")
2174	2181		return true, nil
2175	2182		}
2176	2183
2177		-	log.Info().Str("container_name", containerName).Msg("attachment env changed, recreating")
	2184	+	if imageDrifted {
	2185	+	log.Info().Str("container_name", containerName).Str("existing_image", existingImage).Str("desired_image", serviceImage).Msg("attachment image changed, recreating")
	2186	+	}
	2187	+	if envDrifted {
	2188	+	log.Info().Str("container_name", containerName).Msg("attachment env changed, recreating")
	2189	+	}
	2190	+
2178	2191		if err := s.runtime.StopContainer(ctx, existing.ID); err != nil {
2179		-	return false, log.WrapErr(err, "failed to stop attachment for env update")
	2192	+	return false, log.WrapErr(err, "failed to stop attachment for drift update")
2180	2193		}
2181	2194		if err := s.runtime.RemoveContainer(ctx, existing.ID, true); err != nil {
2182		-	return false, log.WrapErr(err, "failed to remove attachment for env update")
	2195	+	return false, log.WrapErr(err, "failed to remove attachment for drift update")
2183	2196		}
2184	2197
2185	2198		return false, nil


@@ -2164,22 +2164,35 @@ func (s Service) handleRunningAttachment(ctx context.Context, existing domain.
2164		}	2164		}
2165			2165
2166		log := zerowrap.FromCtx(ctx)	2166		log := zerowrap.FromCtx(ctx)
			2167	+
			2168	+	// Check image drift
			2169	+	existingImage := existing.Labels[domain.LabelImage]
			2170	+	imageDrifted := existingImage != "" && existingImage != serviceImage
			2171	+
			2172	+	// Check env drift
2167		envDrifted, err := s.attachmentEnvDrifted(ctx, existing, containerName, serviceImage)	2173		envDrifted, err := s.attachmentEnvDrifted(ctx, existing, containerName, serviceImage)
2168		if err != nil {	2174		if err != nil {
2169		log.WrapErr(err, "failed to check attachment env drift")	2175		log.WrapErr(err, "failed to check attachment env drift")
2170		return true, nil	2176		return true, nil
2171		}	2177		}
2172	-	if !envDrifted {	2178	+
2173	-	log.Debug().Str("container_name", containerName).Msg("attachment already running with current env, skipping")	2179	+	if !envDrifted && !imageDrifted {
			2180	+	log.Debug().Str("container_name", containerName).Msg("attachment already running with current env and image, skipping")
2174		return true, nil	2181		return true, nil
2175		}	2182		}
2176			2183
2177	-	log.Info().Str("container_name", containerName).Msg("attachment env changed, recreating")	2184	+	if imageDrifted {
			2185	+	log.Info().Str("container_name", containerName).Str("existing_image", existingImage).Str("desired_image", serviceImage).Msg("attachment image changed, recreating")
			2186	+	}
			2187	+	if envDrifted {
			2188	+	log.Info().Str("container_name", containerName).Msg("attachment env changed, recreating")
			2189	+	}
			2190	+
2178		if err := s.runtime.StopContainer(ctx, existing.ID); err != nil {	2191		if err := s.runtime.StopContainer(ctx, existing.ID); err != nil {
2179	-	return false, log.WrapErr(err, "failed to stop attachment for env update")	2192	+	return false, log.WrapErr(err, "failed to stop attachment for drift update")
2180		}	2193		}
2181		if err := s.runtime.RemoveContainer(ctx, existing.ID, true); err != nil {	2194		if err := s.runtime.RemoveContainer(ctx, existing.ID, true); err != nil {
2182	-	return false, log.WrapErr(err, "failed to remove attachment for env update")	2195	+	return false, log.WrapErr(err, "failed to remove attachment for drift update")
2183		}	2196		}
2184			2197
2185		return false, nil	2198		return false, nil

M internal/usecase/secrets/service.go +13 -10


@@ -4,6 +4,7 @@ package secrets
4	4		import (
5	5		"context"
6	6		"errors"
	7	+	"sort"
7	8		"strings"
8	9
9	10		"github.com/bnema/zerowrap"

@@ -56,6 +57,16 @@ func (s *Service) publishSecretsChanged(ctx context.Context, domainName, operati
56	57		}
57	58		}
58	59
	60	+	// sortedKeys returns the keys of a map in sorted order.
	61	+	func sortedKeys(m map[string]string) []string {
	62	+	keys := make([]string, 0, len(m))
	63	+	for k := range m {
	64	+	keys = append(keys, k)
	65	+	}
	66	+	sort.Strings(keys)
	67	+	return keys
	68	+	}
	69	+
59	70		// ListKeys returns the list of secret keys for a domain (not values).
60	71		func (s *Service) ListKeys(ctx context.Context, domain string) ([]string, error) {
61	72		ctx = zerowrap.CtxWithFields(ctx, map[string]any{

@@ -160,11 +171,7 @@ func (s *Service) Set(ctx context.Context, domain string, secrets map[string]str
160	171		return err
161	172		}
162	173
163		-	keys := make([]string, 0, len(secrets))
164		-	for k := range secrets {
165		-	keys = append(keys, k)
166		-	}
167		-	s.publishSecretsChanged(ctx, domain, "set", keys)
	174	+	s.publishSecretsChanged(ctx, domain, "set", sortedKeys(secrets))
168	175
169	176		log.Info().Int("count", len(secrets)).Msg("secrets set")
170	177		return nil

@@ -223,11 +230,7 @@ func (s *Service) SetAttachment(ctx context.Context, domainName, service string,
223	230		return err
224	231		}
225	232
226		-	keys := make([]string, 0, len(secrets))
227		-	for k := range secrets {
228		-	keys = append(keys, k)
229		-	}
230		-	s.publishSecretsChanged(ctx, domainName, "set", keys)
	233	+	s.publishSecretsChanged(ctx, domainName, "set", sortedKeys(secrets))
231	234
232	235		log.Info().Int("count", len(secrets)).Str("container", containerName).Msg("attachment secrets set")
233	236		return nil

 import (
 	"context"
 	"errors"
+	"sort"
 	"strings"
 	"github.com/bnema/zerowrap"
 	}
 }
+// sortedKeys returns the keys of a map in sorted order.
+func sortedKeys(m map[string]string) []string {
+	keys := make([]string, 0, len(m))
+	for k := range m {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)
+	return keys
+}
 // ListKeys returns the list of secret keys for a domain (not values).
 func (s *Service) ListKeys(ctx context.Context, domain string) ([]string, error) {
 	ctx = zerowrap.CtxWithFields(ctx, map[string]any{
 		return err
 	}
-	keys := make([]string, 0, len(secrets))
+	s.publishSecretsChanged(ctx, domain, "set", sortedKeys(secrets))
-	for k := range secrets {
-		keys = append(keys, k)
-	}
-	s.publishSecretsChanged(ctx, domain, "set", keys)
 	log.Info().Int("count", len(secrets)).Msg("secrets set")
 	return nil
 		return err
 	}
-	keys := make([]string, 0, len(secrets))
+	s.publishSecretsChanged(ctx, domainName, "set", sortedKeys(secrets))
-	for k := range secrets {
-		keys = append(keys, k)
-	}
-	s.publishSecretsChanged(ctx, domainName, "set", keys)
 	log.Info().Int("count", len(secrets)).Str("container", containerName).Msg("attachment secrets set")
 	return nil