fix(deployment): record a rebuilt execution only after routing accepted it
redeployPinned recorded the rebuilt execution before the caller applied the graph. If the graph apply was then rejected, the next periodic pass saw a verified generation whose binds matched ACTIVE, skipped the publication, and left the service unrouted. The recording now happens only in the callers, after their own traffic application succeeded.
- commit
- 253c38ffbca462116c655a3a45edf71e71bb10c0
- author
- brice · · 2026-10-16 21:05 UTC
- parent
- 5f093c2
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email - refs
- feat/zero-downtime-stateless-deploy
2 files · +50 -3
M internal/usecase/deployment/lifecycle.go +4 -3
| @@ -782,9 +782,10 @@ func (s *Service) redeployPinned(ctx context.Context, app, opID, name string, ef | |||
| 782 | 782 | svcResult.Error = err.Error() | |
| 783 | 783 | return svcResult, err | |
| 784 | 784 | } | |
| 785 | - | // The rebuilt generation passed readiness and is published; the caller | |
| 786 | - | // applies the graph under the same app lock, before any periodic pass. | |
| 787 | - | s.recordVerifiedExecution(app, name, svcResult) | |
| 785 | + | // The execution is deliberately not recorded here: the graph is not | |
| 786 | + | // applied yet. Recording it before routing was accepted would let the next | |
| 787 | + | // periodic pass see a verified, unrouted generation and skip the | |
| 788 | + | // publication. Each caller records after its own refreshTraffic succeeded. | |
| 788 | 789 | return svcResult, nil | |
| 789 | 790 | } | |
| 790 | 791 | ||
| @@ -782,9 +782,10 @@ func (s *Service) redeployPinned(ctx context.Context, app, opID, name string, ef | |||||
| 782 | svcResult.Error = err.Error() | 782 | svcResult.Error = err.Error() | ||
| 783 | return svcResult, err | 783 | return svcResult, err | ||
| 784 | } | 784 | } | ||
| 785 | - | // The rebuilt generation passed readiness and is published; the caller | 785 | + | // The execution is deliberately not recorded here: the graph is not |
| 786 | - | // applies the graph under the same app lock, before any periodic pass. | 786 | + | // applied yet. Recording it before routing was accepted would let the next |
| 787 | - | s.recordVerifiedExecution(app, name, svcResult) | 787 | + | // periodic pass see a verified, unrouted generation and skip the |
| 788 | + | // publication. Each caller records after its own refreshTraffic succeeded. | |||
| 788 | return svcResult, nil | 789 | return svcResult, nil | ||
| 789 | } | 790 | } | ||
| 790 | 791 | ||||
M internal/usecase/deployment/overlap_replacement_test.go +46 -0
| @@ -968,3 +968,49 @@ func TestReconcileRunning_DoesNotWithdrawARestartedGeneration(t *testing.T) { | |||
| 968 | 968 | assert.Len(t, traffic.withdrawals(), withdrawnByRestart, "the periodic pass withdraws nothing more") | |
| 969 | 969 | assert.Equal(t, map[int]int{8080: 17002}, traffic.lastRoute("web")) | |
| 970 | 970 | } | |
| 971 | + | ||
| 972 | + | // TestReconcileRunning_RestoresARebuiltGenerationWhoseTrafficWasRejected is the | |
| 973 | + | // regression for a missing-container rebuild whose graph apply was rejected: | |
| 974 | + | // the rebuilt execution must not be recorded as verified, or the next periodic | |
| 975 | + | // pass sees an already verified generation whose binds match ACTIVE, skips the | |
| 976 | + | // publication, and leaves the service unrouted indefinitely. | |
| 977 | + | func TestReconcileRunning_RestoresARebuiltGenerationWhoseTrafficWasRejected(t *testing.T) { | |
| 978 | + | ctx := context.Background() | |
| 979 | + | store := newTestStore(t) | |
| 980 | + | runtime := outmocks.NewMockContainerRuntime(t) | |
| 981 | + | images := outmocks.NewMockImageResolver(t) | |
| 982 | + | secrets := outmocks.NewMockSecretProvider(t) | |
| 983 | + | seedReplaceableApp(t, ctx, store) | |
| 984 | + | traffic := &storeTraffic{store: store, failRebuild: 1} | |
| 985 | + | svc := deployment.NewService(deployment.Deps{ | |
| 986 | + | State: store, Runtime: runtime, Images: images, Secrets: secrets, Traffic: traffic, | |
| 987 | + | }, zerowrap.Default()).WithProbeDeps(deployment.NewTestProbeDeps(runtime, | |
| 988 | + | func(context.Context, string, string) (int, error) { return 200, nil }, | |
| 989 | + | func(context.Context, string) error { return nil }, | |
| 990 | + | )) | |
| 991 | + | ||
| 992 | + | // The recorded container is gone: restart rebuilds it from the pinned | |
| 993 | + | // ACTIVE digest, and the graph apply that follows is rejected. | |
| 994 | + | runtime.EXPECT().RestartContainer(mock.Anything, "c-old", mock.Anything).Return(domain.ErrContainerNotFound).Once() | |
| 995 | + | runtime.EXPECT().StopContainer(mock.Anything, "c-old", mock.Anything).Return(domain.ErrContainerNotFound).Once() | |
| 996 | + | runtime.EXPECT().RemoveContainer(mock.Anything, "c-old", false).Return(domain.ErrContainerNotFound).Once() | |
| 997 | + | expectNetworkProvision(runtime, "app-blog", 1) | |
| 998 | + | runtime.EXPECT().CreateContainer(mock.Anything, mock.Anything).Return(&domain.Container{ID: "c-rebuilt", Name: "web"}, nil).Once() | |
| 999 | + | runtime.EXPECT().StartContainer(mock.Anything, "c-rebuilt").Return(nil).Once() | |
| 1000 | + | runtime.EXPECT().InspectContainer(mock.Anything, "c-rebuilt").Return( | |
| 1001 | + | &domain.Container{ID: "c-rebuilt", Status: "running", StartedAt: executionStartedAt}, nil) | |
| 1002 | + | runtime.EXPECT().GetContainerBackendBinds(mock.Anything, "c-rebuilt", mock.Anything).Return( | |
| 1003 | + | []domain.ContainerBackendBind{{ContainerPort: 8080, HostPort: 18082, Protocol: domain.NetworkProtocolTCP}}, nil) | |
| 1004 | + | runtime.EXPECT().GetContainerHealthStatus(mock.Anything, "c-rebuilt").Return("", false, nil) | |
| 1005 | + | ||
| 1006 | + | _, err := svc.Restart(ctx, "blog", "web", "op-restart") | |
| 1007 | + | require.Error(t, err, "a rejected graph is a failed restart") | |
| 1008 | + | require.Zero(t, traffic.rebuildCount(), "no graph was accepted") | |
| 1009 | + | active, _, loadErr := store.LoadActive(ctx, "blog") | |
| 1010 | + | require.NoError(t, loadErr) | |
| 1011 | + | require.Equal(t, "c-rebuilt", active.Services["web"].Container) | |
| 1012 | + | ||
| 1013 | + | // The next periodic pass must verify and route the generation itself. | |
| 1014 | + | require.NoError(t, svc.ReconcileRunning(ctx)) | |
| 1015 | + | assert.Equal(t, map[int]int{8080: 18082}, traffic.lastRoute("web"), "the rebuilt generation is routed again") | |
| 1016 | + | } | |
| @@ -968,3 +968,49 @@ func TestReconcileRunning_DoesNotWithdrawARestartedGeneration(t *testing.T) { | |||||
| 968 | assert.Len(t, traffic.withdrawals(), withdrawnByRestart, "the periodic pass withdraws nothing more") | 968 | assert.Len(t, traffic.withdrawals(), withdrawnByRestart, "the periodic pass withdraws nothing more") | ||
| 969 | assert.Equal(t, map[int]int{8080: 17002}, traffic.lastRoute("web")) | 969 | assert.Equal(t, map[int]int{8080: 17002}, traffic.lastRoute("web")) | ||
| 970 | } | 970 | } | ||
| 971 | + | ||||
| 972 | + | // TestReconcileRunning_RestoresARebuiltGenerationWhoseTrafficWasRejected is the | |||
| 973 | + | // regression for a missing-container rebuild whose graph apply was rejected: | |||
| 974 | + | // the rebuilt execution must not be recorded as verified, or the next periodic | |||
| 975 | + | // pass sees an already verified generation whose binds match ACTIVE, skips the | |||
| 976 | + | // publication, and leaves the service unrouted indefinitely. | |||
| 977 | + | func TestReconcileRunning_RestoresARebuiltGenerationWhoseTrafficWasRejected(t *testing.T) { | |||
| 978 | + | ctx := context.Background() | |||
| 979 | + | store := newTestStore(t) | |||
| 980 | + | runtime := outmocks.NewMockContainerRuntime(t) | |||
| 981 | + | images := outmocks.NewMockImageResolver(t) | |||
| 982 | + | secrets := outmocks.NewMockSecretProvider(t) | |||
| 983 | + | seedReplaceableApp(t, ctx, store) | |||
| 984 | + | traffic := &storeTraffic{store: store, failRebuild: 1} | |||
| 985 | + | svc := deployment.NewService(deployment.Deps{ | |||
| 986 | + | State: store, Runtime: runtime, Images: images, Secrets: secrets, Traffic: traffic, | |||
| 987 | + | }, zerowrap.Default()).WithProbeDeps(deployment.NewTestProbeDeps(runtime, | |||
| 988 | + | func(context.Context, string, string) (int, error) { return 200, nil }, | |||
| 989 | + | func(context.Context, string) error { return nil }, | |||
| 990 | + | )) | |||
| 991 | + | ||||
| 992 | + | // The recorded container is gone: restart rebuilds it from the pinned | |||
| 993 | + | // ACTIVE digest, and the graph apply that follows is rejected. | |||
| 994 | + | runtime.EXPECT().RestartContainer(mock.Anything, "c-old", mock.Anything).Return(domain.ErrContainerNotFound).Once() | |||
| 995 | + | runtime.EXPECT().StopContainer(mock.Anything, "c-old", mock.Anything).Return(domain.ErrContainerNotFound).Once() | |||
| 996 | + | runtime.EXPECT().RemoveContainer(mock.Anything, "c-old", false).Return(domain.ErrContainerNotFound).Once() | |||
| 997 | + | expectNetworkProvision(runtime, "app-blog", 1) | |||
| 998 | + | runtime.EXPECT().CreateContainer(mock.Anything, mock.Anything).Return(&domain.Container{ID: "c-rebuilt", Name: "web"}, nil).Once() | |||
| 999 | + | runtime.EXPECT().StartContainer(mock.Anything, "c-rebuilt").Return(nil).Once() | |||
| 1000 | + | runtime.EXPECT().InspectContainer(mock.Anything, "c-rebuilt").Return( | |||
| 1001 | + | &domain.Container{ID: "c-rebuilt", Status: "running", StartedAt: executionStartedAt}, nil) | |||
| 1002 | + | runtime.EXPECT().GetContainerBackendBinds(mock.Anything, "c-rebuilt", mock.Anything).Return( | |||
| 1003 | + | []domain.ContainerBackendBind{{ContainerPort: 8080, HostPort: 18082, Protocol: domain.NetworkProtocolTCP}}, nil) | |||
| 1004 | + | runtime.EXPECT().GetContainerHealthStatus(mock.Anything, "c-rebuilt").Return("", false, nil) | |||
| 1005 | + | ||||
| 1006 | + | _, err := svc.Restart(ctx, "blog", "web", "op-restart") | |||
| 1007 | + | require.Error(t, err, "a rejected graph is a failed restart") | |||
| 1008 | + | require.Zero(t, traffic.rebuildCount(), "no graph was accepted") | |||
| 1009 | + | active, _, loadErr := store.LoadActive(ctx, "blog") | |||
| 1010 | + | require.NoError(t, loadErr) | |||
| 1011 | + | require.Equal(t, "c-rebuilt", active.Services["web"].Container) | |||
| 1012 | + | ||||
| 1013 | + | // The next periodic pass must verify and route the generation itself. | |||
| 1014 | + | require.NoError(t, svc.ReconcileRunning(ctx)) | |||
| 1015 | + | assert.Equal(t, map[int]int{8080: 18082}, traffic.lastRoute("web"), "the rebuilt generation is routed again") | |||
| 1016 | + | } | |||