From 771cb82e1dba5666294abde9e345c3918d4a86e9 Mon Sep 17 00:00:00 2001 From: lubien Date: Wed, 5 Aug 2026 10:17:49 -0300 Subject: [PATCH 1/5] fix(deploy): bluegreen honors health checks when blue machines are stopped --- internal/command/deploy/mock_client_test.go | 37 +++- internal/command/deploy/strategy_bluegreen.go | 22 +- .../command/deploy/strategy_bluegreen_test.go | 169 +++++++++++++- internal/machine/leasable_machine.go | 26 ++- internal/machine/leasable_machine_test.go | 208 ++++++++++++++++++ 5 files changed, 445 insertions(+), 17 deletions(-) create mode 100644 internal/machine/leasable_machine_test.go diff --git a/internal/command/deploy/mock_client_test.go b/internal/command/deploy/mock_client_test.go index b7b30d84d3..30a5820ea8 100644 --- a/internal/command/deploy/mock_client_test.go +++ b/internal/command/deploy/mock_client_test.go @@ -5,6 +5,7 @@ import ( "fmt" "net/http" "sync" + "time" fly "github.com/superfly/fly-go" "github.com/superfly/fly-go/flaps" @@ -25,6 +26,8 @@ type mockFlapsClient struct { breakList bool breakDestroy bool breakLease bool + breakGet bool + launchInputs []fly.LaunchMachineInput // uncordonTransientFailures causes Uncordon to fail this many times before // succeeding, simulating transient API errors for retry tests. @@ -137,7 +140,25 @@ func (m *mockFlapsClient) GenerateSecretKey(ctx context.Context, appName, name, } func (m *mockFlapsClient) Get(ctx context.Context, appName, machineID string) (*fly.Machine, error) { - return nil, fmt.Errorf("failed to get %s", machineID) + m.mu.Lock() + defer m.mu.Unlock() + + if m.breakGet { + return nil, fmt.Errorf("failed to get %s", machineID) + } + // Return a machine with one passing check so that health-check loops + // can exit cleanly in tests that don't specifically test failure paths. + return &fly.Machine{ + ID: machineID, + Checks: []*fly.MachineCheckStatus{ + {Name: "check1", Status: fly.Passing}, + }, + Config: &fly.MachineConfig{ + Checks: map[string]fly.MachineCheck{ + "check1": {}, + }, + }, + }, nil } func (m *mockFlapsClient) GetApp(ctx context.Context, name string) (*flaps.App, error) { @@ -200,10 +221,24 @@ func (m *mockFlapsClient) Launch(ctx context.Context, appName string, builder fl return nil, fmt.Errorf("failed to launch %s", builder.ID) } m.nextMachineID += 1 + m.launchInputs = append(m.launchInputs, builder) + + shortDuration := fly.Duration{Duration: 10 * time.Millisecond} return &fly.Machine{ ID: fmt.Sprintf("%x", m.nextMachineID), LeaseNonce: fmt.Sprintf("%x-launch-lease", m.nextMachineID), + Config: &fly.MachineConfig{ + Metadata: map[string]string{}, + // Use a near-zero grace period and interval so that health-check + // goroutines poll almost immediately in tests without real timing. + Checks: map[string]fly.MachineCheck{ + "check1": { + GracePeriod: &shortDuration, + Interval: &shortDuration, + }, + }, + }, }, nil } diff --git a/internal/command/deploy/strategy_bluegreen.go b/internal/command/deploy/strategy_bluegreen.go index de57f5b7ee..5c318a7ef2 100644 --- a/internal/command/deploy/strategy_bluegreen.go +++ b/internal/command/deploy/strategy_bluegreen.go @@ -90,6 +90,23 @@ type blueGreen struct { uncordonRetryDelay time.Duration } +// machineHasConfiguredChecks returns true if the machine config has any health +// checks defined — either at the top-level or inside a service. This is +// intentionally based on the *configuration*, not on the runtime Machine.Checks +// status field, which is empty for freshly-launched machines. +func machineHasConfiguredChecks(cfg *fly.MachineConfig) bool { + if len(cfg.Checks) > 0 { + return true + } + for _, svc := range cfg.Services { + if len(svc.Checks) > 0 { + return true + } + } + + return false +} + func BlueGreenStrategy(md *machineDeployment, blueMachines []*machineUpdateEntry) *blueGreen { bg := &blueGreen{ greenMachines: machineUpdateEntries{}, @@ -174,6 +191,7 @@ func (bg *blueGreen) CreateGreenMachines(ctx context.Context) error { launchInput := mach.launchInput launchInput.SkipServiceRegistration = true + launchInput.SkipLaunch = false launchInput.Config.Metadata[fly.MachineConfigMetadataKeyFlyctlBGTag] = bg.timestamp newMachineRaw, err := bg.flaps.Launch(ctx, bg.app.Name, *launchInput) @@ -390,7 +408,7 @@ func (bg *blueGreen) WaitForGreenMachinesToBeHealthy(ctx context.Context) error // in some cases, not all processes have healthchecks setup // eg. processes that run background workers, etc. // there's no point checking for health, a started state is enough - if len(gm.leasableMachine.Machine().Checks) == 0 { + if !machineHasConfiguredChecks(gm.launchInput.Config) { continue } @@ -405,7 +423,7 @@ func (bg *blueGreen) WaitForGreenMachinesToBeHealthy(ctx context.Context) error // in some cases, not all processes have healthchecks setup // eg. processes that run background workers, etc. // there's no point checking for health, a started state is enough - if len(gm.leasableMachine.Machine().Checks) == 0 { + if !machineHasConfiguredChecks(gm.launchInput.Config) { continue } diff --git a/internal/command/deploy/strategy_bluegreen_test.go b/internal/command/deploy/strategy_bluegreen_test.go index cc08fe5449..bd0d1af105 100644 --- a/internal/command/deploy/strategy_bluegreen_test.go +++ b/internal/command/deploy/strategy_bluegreen_test.go @@ -39,15 +39,16 @@ func newBlueGreenStrategy(client flapsutil.FlapsClient, numberOfExistingMachines }) } strategy := &blueGreen{ - apiClient: &mockWebClient{}, - flaps: client, - maxConcurrent: 10, - appConfig: &appconfig.Config{}, - io: ios, - colorize: ios.ColorScheme(), - timeout: 1 * time.Second, - blueMachines: machines, - app: &flaps.App{Name: "test-app"}, + apiClient: &mockWebClient{}, + flaps: client, + maxConcurrent: 10, + appConfig: &appconfig.Config{}, + io: ios, + colorize: ios.ColorScheme(), + clearLinesAbove: func(int) {}, // no-op; avoids nil-panic in render loop + timeout: 5 * time.Second, + blueMachines: machines, + app: &flaps.App{Name: "test-app"}, } strategy.initialize() @@ -171,3 +172,153 @@ func FuzzDeploy(f *testing.F) { strategy.Deploy(ctx) }) } + +// --------------------------------------------------------------------------- +// Tests for the SkipLaunch / health-check fixes +// --------------------------------------------------------------------------- + +// TestMachineHasConfiguredChecks verifies the helper that decides whether a +// machine config carries any health-check definitions. +func TestMachineHasConfiguredChecks(t *testing.T) { + t.Run("no checks at all", func(t *testing.T) { + cfg := &fly.MachineConfig{} + assert.False(t, machineHasConfiguredChecks(cfg)) + }) + + t.Run("top-level check", func(t *testing.T) { + cfg := &fly.MachineConfig{ + Checks: map[string]fly.MachineCheck{"alive": {}}, + } + assert.True(t, machineHasConfiguredChecks(cfg)) + }) + + t.Run("service-level check only", func(t *testing.T) { + cfg := &fly.MachineConfig{ + Services: []fly.MachineService{ + {Checks: []fly.MachineServiceCheck{{}}}, + }, + } + assert.True(t, machineHasConfiguredChecks(cfg)) + }) + + t.Run("service with no checks", func(t *testing.T) { + cfg := &fly.MachineConfig{ + Services: []fly.MachineService{ + {Checks: nil}, + }, + } + assert.False(t, machineHasConfiguredChecks(cfg)) + }) +} + +// newBlueGreenStrategyWithState is like newBlueGreenStrategy but lets the +// caller specify the state of each blue machine and its SkipLaunch value. +// This is used to simulate machines that have been auto-stopped. +func newBlueGreenStrategyWithState(client flapsutil.FlapsClient, machineState string, skipLaunch bool) *blueGreen { + ios, _, _, _ := iostreams.Test() + + machines := []*machineUpdateEntry{ + { + leasableMachine: machine.NewLeasableMachine(client, ios, "", &fly.Machine{ + State: machineState, + Config: &fly.MachineConfig{ + Metadata: map[string]string{}, + Checks: map[string]fly.MachineCheck{ + "check1": {}, + }, + }, + }, false), + launchInput: &fly.LaunchMachineInput{ + SkipLaunch: skipLaunch, + Config: &fly.MachineConfig{ + Metadata: map[string]string{}, + Checks: map[string]fly.MachineCheck{ + "check1": {}, + }, + }, + MinSecretsVersion: nil, + }, + }, + } + + strategy := &blueGreen{ + apiClient: &mockWebClient{}, + flaps: client, + maxConcurrent: 10, + appConfig: &appconfig.Config{}, + io: ios, + colorize: ios.ColorScheme(), + clearLinesAbove: func(int) {}, + timeout: 5 * time.Second, + blueMachines: machines, + app: &flaps.App{Name: "test-app"}, + } + strategy.initialize() + strategy.waitBeforeStop = 0 + strategy.waitBeforeCordon = 0 + strategy.uncordonRetryDelay = 0 + + return strategy +} + +// TestCreateGreenMachinesAlwaysStartsGreenMachines verifies that green +// machines are always launched with SkipLaunch=false, even when the +// corresponding blue machine has SkipLaunch=true (e.g. because it was +// auto-stopped before the deploy). +func TestCreateGreenMachinesAlwaysStartsGreenMachines(t *testing.T) { + client := &mockFlapsClient{} + ctx := context.Background() + ctx = flapsutil.NewContextWithClient(ctx, client) + + // Simulate a stopped blue machine: SkipLaunch=true. + strategy := newBlueGreenStrategyWithState(client, fly.MachineStateStopped, true) + + err := strategy.CreateGreenMachines(ctx) + assert.NoError(t, err) + assert.Len(t, strategy.greenMachines, 1, "expected one green machine to be created") + + client.mu.Lock() + inputs := client.launchInputs + client.mu.Unlock() + + assert.Len(t, inputs, 1, "expected one Launch call") + assert.False(t, inputs[0].SkipLaunch, + "green machine must be launched with SkipLaunch=false regardless of blue machine state") +} + +// TestDeployWithStoppedBlueMachinesEnforcesHealthChecks verifies the full +// deploy pipeline when blue machines have SkipLaunch=true (auto-stopped). +// +// Before the fix, the deploy would silently succeed: green machines were +// never started and their health was faked as "1/1 passing". +// +// After the fix, the deploy must attempt real health checks and only succeed +// when they pass, or fail/roll back when they don't. +func TestDeployWithStoppedBlueMachinesEnforcesHealthChecks(t *testing.T) { + t.Run("fails when health checks cannot be verified", func(t *testing.T) { + // breakGet=true simulates the platform being unreachable for health polls. + client := &mockFlapsClient{breakGet: true} + ctx := context.Background() + ctx = flapsutil.NewContextWithClient(ctx, client) + + strategy := newBlueGreenStrategyWithState(client, fly.MachineStateStopped, true) + // Short timeout so the test doesn't hang. + strategy.timeout = 500 * time.Millisecond + + err := strategy.Deploy(ctx) + assert.Error(t, err, + "deploy must fail when health checks cannot be verified, not silently succeed") + }) + + t.Run("succeeds when health checks pass", func(t *testing.T) { + // Default mockFlapsClient.Get returns a passing machine. + client := &mockFlapsClient{} + ctx := context.Background() + ctx = flapsutil.NewContextWithClient(ctx, client) + + strategy := newBlueGreenStrategyWithState(client, fly.MachineStateStopped, true) + + err := strategy.Deploy(ctx) + assert.NoError(t, err, "deploy must succeed when health checks pass") + }) +} diff --git a/internal/machine/leasable_machine.go b/internal/machine/leasable_machine.go index ca3ec71f49..029de7fafe 100644 --- a/internal/machine/leasable_machine.go +++ b/internal/machine/leasable_machine.go @@ -365,9 +365,20 @@ func (lm *leasableMachine) WaitForSmokeChecksToPass(ctx context.Context) error { } func (lm *leasableMachine) WaitForHealthchecksToPass(ctx context.Context, timeout time.Duration) error { - ctx, span := tracing.GetTracer().Start(ctx, "wait_for_healthchecks", trace.WithAttributes(attribute.Int("num_checks", len(lm.Machine().Checks)), attribute.Int64("timeout_ms", timeout.Milliseconds()))) + // Count configured checks from the machine config, not the runtime status field. + // Machine.Checks (runtime status) is empty for freshly-launched machines and + // would cause us to skip health checking entirely if used as the gate. + // Use GetConfig() to safely handle a nil Config field. + configuredChecks := 0 + if cfg := lm.Machine().GetConfig(); cfg != nil { + configuredChecks = len(cfg.Checks) + for _, svc := range cfg.Services { + configuredChecks += len(svc.Checks) + } + } + ctx, span := tracing.GetTracer().Start(ctx, "wait_for_healthchecks", trace.WithAttributes(attribute.Int("num_checks", configuredChecks), attribute.Int64("timeout_ms", timeout.Milliseconds()))) defer span.End() - if len(lm.Machine().Checks) == 0 { + if configuredChecks == 0 { return nil } waitCtx, cancel := ctrlc.HookCancelableContext(context.WithTimeout(ctx, timeout)) @@ -395,9 +406,14 @@ func (lm *leasableMachine) WaitForHealthchecksToPass(ctx context.Context, timeou span.RecordError(err) return fmt.Errorf("error getting machine %s from api: %w", lm.Machine().ID, err) - case !updateMachine.AllHealthChecks().AllPassing(): + } + checkStatus := updateMachine.AllHealthChecks() + // Require at least one check result from the platform AND all checks passing. + // AllPassing() is vacuously true when Total == 0 (no results yet), so we + // must guard against that or we'd exit immediately on a freshly-started machine. + if checkStatus.Total == 0 || !checkStatus.AllPassing() { if lm.showLogs && (!printedFirst || lm.io.IsInteractive()) { - lm.logHealthCheckStatus(ctx, updateMachine.AllHealthChecks()) + lm.logHealthCheckStatus(ctx, checkStatus) printedFirst = true } select { @@ -408,7 +424,7 @@ func (lm *leasableMachine) WaitForHealthchecksToPass(ctx context.Context, timeou continue } if lm.showLogs { - lm.logHealthCheckStatus(ctx, updateMachine.AllHealthChecks()) + lm.logHealthCheckStatus(ctx, checkStatus) } return nil diff --git a/internal/machine/leasable_machine_test.go b/internal/machine/leasable_machine_test.go new file mode 100644 index 0000000000..4a204dfd4b --- /dev/null +++ b/internal/machine/leasable_machine_test.go @@ -0,0 +1,208 @@ +package machine + +import ( + "context" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + fly "github.com/superfly/fly-go" + "github.com/superfly/fly-go/flaps" + "github.com/superfly/flyctl/internal/mock" + "github.com/superfly/flyctl/iostreams" +) + +// newTestLeasableMachine creates a leasableMachine wired to the provided +// flaps mock and seeded with the given fly.Machine value. +func newTestLeasableMachine(client *mock.FlapsClient, m *fly.Machine) *leasableMachine { + ios, _, _, _ := iostreams.Test() + + return &leasableMachine{ + flapsClient: client, + io: ios, + colorize: ios.ColorScheme(), + appName: "test-app", + machine: m, + } +} + +// passingGetFunc returns a Get function that always responds with a machine +// whose named check is in the Passing state. +func passingGetFunc(checkName string) func(context.Context, string, string) (*fly.Machine, error) { + return func(_ context.Context, _ string, machineID string) (*fly.Machine, error) { + return &fly.Machine{ + ID: machineID, + Checks: []*fly.MachineCheckStatus{ + {Name: checkName, Status: fly.Passing}, + }, + }, nil + } +} + +// failingGetFunc returns a Get function that always responds with a machine +// whose named check is in the Critical (failing) state. +func failingGetFunc(checkName string) func(context.Context, string, string) (*fly.Machine, error) { + return func(_ context.Context, _ string, machineID string) (*fly.Machine, error) { + return &fly.Machine{ + ID: machineID, + Checks: []*fly.MachineCheckStatus{ + {Name: checkName, Status: fly.Critical}, + }, + }, nil + } +} + +// TestWaitForHealthchecksToPass_NilConfig verifies that a machine with a nil +// Config does not panic and returns immediately (no checks = nothing to wait for). +func TestWaitForHealthchecksToPass_NilConfig(t *testing.T) { + client := &mock.FlapsClient{} + lm := newTestLeasableMachine(client, &fly.Machine{ID: "m1", Config: nil}) + + err := lm.WaitForHealthchecksToPass(context.Background(), 5*time.Second) + assert.NoError(t, err) +} + +// TestWaitForHealthchecksToPass_NoConfiguredChecks verifies that a machine +// with an empty Config (no checks defined) returns immediately without +// calling the API at all. +func TestWaitForHealthchecksToPass_NoConfiguredChecks(t *testing.T) { + getCalls := atomic.Int32{} + client := &mock.FlapsClient{ + GetFunc: func(ctx context.Context, appName, machineID string) (*fly.Machine, error) { + getCalls.Add(1) + + return &fly.Machine{ID: machineID}, nil + }, + } + + lm := newTestLeasableMachine(client, &fly.Machine{ + ID: "m1", + Config: &fly.MachineConfig{}, // no checks + }) + + err := lm.WaitForHealthchecksToPass(context.Background(), 5*time.Second) + assert.NoError(t, err) + assert.Equal(t, int32(0), getCalls.Load(), "Get should not be called when no checks are configured") +} + +// TestWaitForHealthchecksToPass_TotalZeroDoesNotPass verifies that when the +// platform has not yet reported any check results (Total == 0), the function +// keeps waiting instead of exiting early. +// +// Prior to the fix, AllPassing() returned true vacuously when Total == 0 +// (because 0 == 0), causing the function to exit immediately on the very +// first poll before any real check result arrived. +func TestWaitForHealthchecksToPass_TotalZeroDoesNotPass(t *testing.T) { + calls := atomic.Int32{} + client := &mock.FlapsClient{ + GetFunc: func(ctx context.Context, appName, machineID string) (*fly.Machine, error) { + n := calls.Add(1) + if n == 1 { + // First poll: platform hasn't reported any results yet. + return &fly.Machine{ + ID: machineID, + Checks: []*fly.MachineCheckStatus{}, // Total == 0 + }, nil + } + // Second poll: checks are now passing. + return &fly.Machine{ + ID: machineID, + Checks: []*fly.MachineCheckStatus{ + {Name: "alive", Status: fly.Passing}, + }, + }, nil + }, + } + + lm := newTestLeasableMachine(client, &fly.Machine{ + ID: "m1", + Config: &fly.MachineConfig{ + Checks: map[string]fly.MachineCheck{"alive": {}}, + }, + }) + + err := lm.WaitForHealthchecksToPass(context.Background(), 10*time.Second) + assert.NoError(t, err) + assert.GreaterOrEqual(t, calls.Load(), int32(2), + "should poll at least twice: once with no results, once with passing results") +} + +// TestWaitForHealthchecksToPass_PassesWhenAllChecksPass verifies the happy +// path: configured checks, platform reports them as passing → returns nil. +func TestWaitForHealthchecksToPass_PassesWhenAllChecksPass(t *testing.T) { + client := &mock.FlapsClient{GetFunc: passingGetFunc("alive")} + + lm := newTestLeasableMachine(client, &fly.Machine{ + ID: "m1", + Config: &fly.MachineConfig{ + Checks: map[string]fly.MachineCheck{"alive": {}}, + }, + }) + + err := lm.WaitForHealthchecksToPass(context.Background(), 5*time.Second) + assert.NoError(t, err) +} + +// TestWaitForHealthchecksToPass_TimesOutWhenChecksFail verifies that when +// checks are configured but consistently failing, the function eventually +// returns a timeout error rather than hanging forever. +func TestWaitForHealthchecksToPass_TimesOutWhenChecksFail(t *testing.T) { + client := &mock.FlapsClient{GetFunc: failingGetFunc("alive")} + + lm := newTestLeasableMachine(client, &fly.Machine{ + ID: "m1", + Config: &fly.MachineConfig{ + Checks: map[string]fly.MachineCheck{"alive": {}}, + }, + }) + + err := lm.WaitForHealthchecksToPass(context.Background(), 300*time.Millisecond) + assert.Error(t, err, "should return an error when checks never pass within the timeout") +} + +// TestWaitForHealthchecksToPass_ServiceChecksAreIncluded verifies that +// service-level checks (Config.Services[*].Checks) are counted as configured +// checks, not just top-level Config.Checks. +func TestWaitForHealthchecksToPass_ServiceChecksAreIncluded(t *testing.T) { + getCalls := atomic.Int32{} + client := &mock.FlapsClient{ + GetFunc: func(ctx context.Context, appName, machineID string) (*fly.Machine, error) { + getCalls.Add(1) + + return &fly.Machine{ + ID: machineID, + Checks: []*fly.MachineCheckStatus{ + {Name: "servicecheck-00-http-8080", Status: fly.Passing}, + }, + }, nil + }, + } + + lm := newTestLeasableMachine(client, &fly.Machine{ + ID: "m1", + Config: &fly.MachineConfig{ + // No top-level checks; only a service-level check. + Services: []fly.MachineService{ + { + Checks: []fly.MachineServiceCheck{ + {Type: fly.StringPointer("http")}, + }, + }, + }, + }, + }) + + err := lm.WaitForHealthchecksToPass(context.Background(), 5*time.Second) + assert.NoError(t, err) + assert.Greater(t, getCalls.Load(), int32(0), "Get should be called to poll service checks") +} + +// Ensure the mock satisfies the interface at compile time. +var _ LeasableMachine = &leasableMachine{} + +// Compile-time check: mock.FlapsClient must satisfy flapsutil.FlapsClient. +// (Imported indirectly; checked via the flaps package type.) +var _ interface { + GetApp(context.Context, string) (*flaps.App, error) +} = &mock.FlapsClient{} From dc83e1cd7270d17a264d90e9375e5a3e97cf39f2 Mon Sep 17 00:00:00 2001 From: lubien Date: Wed, 5 Aug 2026 10:18:05 -0300 Subject: [PATCH 2/5] test(preflight): cover bluegreen deploy with stopped blue machines --- test/preflight/fly_deploy_test.go | 98 ++++++++++++++++++++++++++++++- 1 file changed, 96 insertions(+), 2 deletions(-) diff --git a/test/preflight/fly_deploy_test.go b/test/preflight/fly_deploy_test.go index 6df55babfb..44cd325472 100644 --- a/test/preflight/fly_deploy_test.go +++ b/test/preflight/fly_deploy_test.go @@ -18,8 +18,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - // fly "github.com/superfly/fly-go" - + fly "github.com/superfly/fly-go" "github.com/superfly/flyctl/test/preflight/testlib" ) @@ -448,3 +447,98 @@ func TestDeploy(t *testing.T) { testDeploy(t, filepath.Join(testlib.RepositoryRoot(), "test", "preflight", "fixtures", "example"), "--buildkit --remote-only") }) } + +// TestFlyDeploy_BlueGreen_StoppedMachines is a regression test for the bug where +// blue-green deployments silently bypassed health checks when blue machines were +// stopped (e.g. by auto_stop_machines = "stop" with min_machines_running = 0). +// +// Root cause: stopped blue machines had SkipLaunch=true in their launchInput. +// CreateGreenMachines copied that input without resetting the flag, so green +// machines were never started. WaitForGreenMachinesToBeHealthy then immediately +// marked them as "1/1 passing" without ever polling — a silent false success. +// +// The fix forces SkipLaunch=false for all green machines and guards health-check +// polling against vacuously-true AllPassing() on an empty result set. +func TestFlyDeploy_BlueGreen_StoppedMachines(t *testing.T) { + // setupBlueMachinesStopped launches a fresh single-machine app with an + // http service health check, does an initial deploy so machines reach + // "started", then manually stops every machine to reproduce the + // auto_stop_machines trigger condition. + setupBlueMachinesStopped := func(t *testing.T) (*testlib.FlyctlTestEnv, string) { + t.Helper() + f := testlib.NewTestEnvFromEnv(t) + appName := f.CreateRandomAppName() + + f.Fly("launch --org %s --name %s --region %s --image nginx --internal-port 80 --ha=false", + f.OrgSlug(), appName, f.PrimaryRegion()) + + // Add a service health check so bluegreen actually waits for check results. + // Without checks the strategy skips health polling, which would make the + // failing-app subtest pass vacuously. + appConfig := f.ReadFile("fly.toml") + appConfig += ` + [[http_service.checks]] + grace_period = "5s" + interval = "10s" + method = "GET" + timeout = "5s" + path = "/" +` + f.WriteFlyToml("%s", appConfig) + f.Fly("deploy --remote-only") + + // Stop every machine to simulate what auto_stop_machines does between deploys. + machines := f.MachinesList(appName) + require.NotEmpty(t, machines, "expected at least one machine after initial deploy") + for _, m := range machines { + f.Fly("machine stop -a %s %s", appName, m.ID) + } + require.Eventually(t, func() bool { + for _, m := range f.MachinesList(appName) { + if m.State != fly.MachineStateStopped { + return false + } + } + return true + }, 30*time.Second, 2*time.Second, "timed out waiting for all machines to reach stopped state") + + return f, appName + } + + // A bluegreen deploy of a crashing app must fail, never silently succeed. + // Before the fix this exited 0 with "Deployment Complete" while machines + // were still stopped and no traffic was served. + t.Run("fails when app crashes", func(t *testing.T) { + f, _ := setupBlueMachinesStopped(t) + + // Overwrite the entrypoint with /bin/false so the container exits immediately. + appConfig := f.ReadFile("fly.toml") + appConfig += ` +[experimental] + entrypoint = "/bin/false" +` + f.WriteFlyToml("%s", appConfig) + + deployRes := f.FlyAllowExitFailure("deploy --remote-only --strategy bluegreen") + require.NotEqual(t, 0, deployRes.ExitCode(), + "bluegreen deploy must fail when the app crashes, not silently report success;\nstdout:\n%s\nstderr:\n%s", + deployRes.StdOutString(), deployRes.StdErrString()) + }) + + // A bluegreen deploy of a healthy app must succeed and leave machines running. + // Before the fix this also appeared to succeed — but machines stayed stopped + // because green machines were never actually started. + t.Run("succeeds and machines are started", func(t *testing.T) { + f, appName := setupBlueMachinesStopped(t) + + // Re-deploy the same healthy nginx image; no changes, no builder needed. + f.Fly("deploy --remote-only --strategy bluegreen") + + // Every machine must be in "started" state — not stopped or created. + for _, m := range f.MachinesList(appName) { + require.Equal(t, fly.MachineStateStarted, m.State, + "machine %s should be 'started' after a successful bluegreen deploy, got '%s'", + m.ID, m.State) + } + }) +} From 7f7fbf1072df1ec3a86fcc861209e5159cb747d2 Mon Sep 17 00:00:00 2001 From: lubien Date: Wed, 5 Aug 2026 12:16:52 -0300 Subject: [PATCH 3/5] create bluegreen preflight test --- .github/workflows/preflight.yml | 1 + scripts/preflight.sh | 15 +++++++++++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/.github/workflows/preflight.yml b/.github/workflows/preflight.yml index 8e3f2080fc..60ea3b7682 100644 --- a/.github/workflows/preflight.yml +++ b/.github/workflows/preflight.yml @@ -21,6 +21,7 @@ jobs: group: - apps - deploy + - bluegreen - launch - scale - volume diff --git a/scripts/preflight.sh b/scripts/preflight.sh index dbf8ff7f0f..cd9ffefc8d 100755 --- a/scripts/preflight.sh +++ b/scripts/preflight.sh @@ -49,6 +49,7 @@ trap finish EXIT set +e # Define test groups based on logical groupings +test_skip="" if [[ -n "$group" ]]; then case "$group" in apps) @@ -56,6 +57,11 @@ if [[ -n "$group" ]]; then ;; deploy) test_pattern="^Test(FlyDeploy|Deploy)" + # Bluegreen tests live in their own matrix leg so this group stays under 15m. + test_skip="^TestFlyDeploy_BlueGreen" + ;; + bluegreen) + test_pattern="^TestFlyDeploy_BlueGreen" ;; launch) test_pattern="^Test(FlyLaunch|Launch)" @@ -89,12 +95,17 @@ if [[ -n "$group" ]]; then ;; *) echo "Unknown test group: $group" - echo "Available groups: apps, deploy, launch, scale, volume, console, logs, machine, postgres, tokens, wireguard, misc" + echo "Available groups: apps, deploy, bluegreen, launch, scale, volume, console, logs, machine, postgres, tokens, wireguard, misc" exit 1 ;; esac - go test -tags=integration -v -timeout=15m $test_opts -run "$test_pattern" github.com/superfly/flyctl/test/preflight/... | tee "$test_log" + skip_arg=() + if [[ -n "$test_skip" ]]; then + skip_arg=(-skip "$test_skip") + fi + + go test -tags=integration -v -timeout=15m $test_opts -run "$test_pattern" "${skip_arg[@]}" github.com/superfly/flyctl/test/preflight/... | tee "$test_log" test_status=$? # Legacy numeric sharding using gotesplit (deprecated) elif [[ -n "$total" && -n "$index" ]]; then From 1c9b1d864f304757f13fea3e6ddf669802518031 Mon Sep 17 00:00:00 2001 From: lubien Date: Thu, 6 Aug 2026 08:52:15 -0300 Subject: [PATCH 4/5] Ensure that each process group has one machine that starts --- internal/command/deploy/strategy_bluegreen.go | 58 ++++++- .../command/deploy/strategy_bluegreen_test.go | 164 +++++++++++++++++- 2 files changed, 213 insertions(+), 9 deletions(-) diff --git a/internal/command/deploy/strategy_bluegreen.go b/internal/command/deploy/strategy_bluegreen.go index 5c318a7ef2..d7fa522b52 100644 --- a/internal/command/deploy/strategy_bluegreen.go +++ b/internal/command/deploy/strategy_bluegreen.go @@ -166,6 +166,53 @@ func (bg *blueGreen) sleepAbortable(d time.Duration) bool { } } +// forceStartRepresentatives returns the set of blue-machine indices whose +// green replacements must be launched with SkipLaunch=false so that each +// process group with configured health checks has at least one machine that +// starts and can be health-verified. +// +// Rule: for each process group with any configured health check, ensure a +// representative will start. Machines whose blue counterpart is already going +// to start naturally (SkipLaunch=false — e.g. it was running when the deploy +// began) satisfy the invariant for free. If no machine in the group would +// naturally start (e.g. auto_stop_machines turned them all off), promote the +// first machine in the group. +// +// Machines outside the returned set inherit their blue's SkipLaunch value, +// so a stopped blue produces a stopped green — mirroring the app's +// pre-deploy state rather than unnecessarily waking up idle workers. +func (bg *blueGreen) forceStartRepresentatives() map[int]bool { + groupHasChecks := map[string]bool{} + groupWillStart := map[string]bool{} + groupFirstStopped := map[string]int{} // process group -> lowest index of a machine that would stay stopped + + for i, mach := range bg.blueMachines { + cfg := mach.launchInput.Config + pg := cfg.ProcessGroup() + + if machineHasConfiguredChecks(cfg) { + groupHasChecks[pg] = true + } + if !mach.launchInput.SkipLaunch { + groupWillStart[pg] = true + } else if _, seen := groupFirstStopped[pg]; !seen { + groupFirstStopped[pg] = i + } + } + + forceStart := map[int]bool{} + for pg := range groupHasChecks { + if groupWillStart[pg] { + continue + } + if idx, ok := groupFirstStopped[pg]; ok { + forceStart[idx] = true + } + } + + return forceStart +} + func (bg *blueGreen) CreateGreenMachines(ctx context.Context) error { ctx, span := tracing.GetTracer().Start(ctx, "green_machines_create") defer span.End() @@ -178,12 +225,17 @@ func (bg *blueGreen) CreateGreenMachines(ctx context.Context) error { float64(bg.maxConcurrent), ))) + // Decide upfront which green machines must be force-started so each + // process group with health checks has at least one machine to poll. + // Everything else inherits SkipLaunch from its blue counterpart. + forceStart := bg.forceStartRepresentatives() + var lock sync.Mutex p := pool.New(). WithErrors(). WithFirstError(). WithMaxGoroutines(createConcurrency) - for _, mach := range bg.blueMachines { + for i, mach := range bg.blueMachines { p.Go(func() error { if bg.isAborted() { return ErrAborted @@ -191,7 +243,9 @@ func (bg *blueGreen) CreateGreenMachines(ctx context.Context) error { launchInput := mach.launchInput launchInput.SkipServiceRegistration = true - launchInput.SkipLaunch = false + if forceStart[i] { + launchInput.SkipLaunch = false + } launchInput.Config.Metadata[fly.MachineConfigMetadataKeyFlyctlBGTag] = bg.timestamp newMachineRaw, err := bg.flaps.Launch(ctx, bg.app.Name, *launchInput) diff --git a/internal/command/deploy/strategy_bluegreen_test.go b/internal/command/deploy/strategy_bluegreen_test.go index bd0d1af105..dfe1eba18e 100644 --- a/internal/command/deploy/strategy_bluegreen_test.go +++ b/internal/command/deploy/strategy_bluegreen_test.go @@ -7,6 +7,7 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" fly "github.com/superfly/fly-go" "github.com/superfly/fly-go/flaps" "github.com/superfly/flyctl/internal/appconfig" @@ -261,16 +262,18 @@ func newBlueGreenStrategyWithState(client flapsutil.FlapsClient, machineState st return strategy } -// TestCreateGreenMachinesAlwaysStartsGreenMachines verifies that green -// machines are always launched with SkipLaunch=false, even when the -// corresponding blue machine has SkipLaunch=true (e.g. because it was -// auto-stopped before the deploy). -func TestCreateGreenMachinesAlwaysStartsGreenMachines(t *testing.T) { +// TestCreateGreenMachinesForceStartsRepresentative verifies that when the +// only blue machine in a process group with configured health checks would +// stay stopped (SkipLaunch=true), its green replacement is promoted to +// SkipLaunch=false so that at least one machine gets health-verified. +func TestCreateGreenMachinesForceStartsRepresentative(t *testing.T) { client := &mockFlapsClient{} ctx := context.Background() ctx = flapsutil.NewContextWithClient(ctx, client) - // Simulate a stopped blue machine: SkipLaunch=true. + // Simulate a stopped blue machine: SkipLaunch=true. It has configured + // checks, so its green replacement must be force-started as the group's + // health-check representative. strategy := newBlueGreenStrategyWithState(client, fly.MachineStateStopped, true) err := strategy.CreateGreenMachines(ctx) @@ -283,7 +286,154 @@ func TestCreateGreenMachinesAlwaysStartsGreenMachines(t *testing.T) { assert.Len(t, inputs, 1, "expected one Launch call") assert.False(t, inputs[0].SkipLaunch, - "green machine must be launched with SkipLaunch=false regardless of blue machine state") + "green machine must be launched with SkipLaunch=false as the process group's health-check representative") +} + +// makeMachineEntry constructs a machineUpdateEntry with just enough state to +// exercise forceStartRepresentatives: a process-group tag, a SkipLaunch value, +// and optional configured checks. +func makeMachineEntry(processGroup string, skipLaunch, hasChecks bool) *machineUpdateEntry { + checks := map[string]fly.MachineCheck{} + if hasChecks { + checks["check1"] = fly.MachineCheck{} + } + + return &machineUpdateEntry{ + launchInput: &fly.LaunchMachineInput{ + SkipLaunch: skipLaunch, + Config: &fly.MachineConfig{ + Metadata: map[string]string{ + fly.MachineConfigMetadataKeyFlyProcessGroup: processGroup, + }, + Checks: checks, + }, + }, + } +} + +// TestForceStartRepresentatives exercises the per-process-group selection +// logic that decides which green machines must be launched even when their +// blue counterparts were stopped. +func TestForceStartRepresentatives(t *testing.T) { + t.Run("stopped group with checks forces the first machine", func(t *testing.T) { + bg := &blueGreen{blueMachines: []*machineUpdateEntry{ + makeMachineEntry("app", true, true), + }} + + assert.Equal(t, map[int]bool{0: true}, bg.forceStartRepresentatives()) + }) + + t.Run("stopped group without checks is left alone", func(t *testing.T) { + bg := &blueGreen{blueMachines: []*machineUpdateEntry{ + makeMachineEntry("worker", true, false), + }} + + assert.Empty(t, bg.forceStartRepresentatives(), + "no checks means no representative needed; green should mirror blue's stopped state") + }) + + t.Run("group with a natural starter needs no forcing", func(t *testing.T) { + bg := &blueGreen{blueMachines: []*machineUpdateEntry{ + makeMachineEntry("app", true, true), // stopped + makeMachineEntry("app", false, true), // already going to start + }} + + assert.Empty(t, bg.forceStartRepresentatives(), + "a machine that already starts satisfies the invariant") + }) + + t.Run("multiple stopped in same group forces exactly one", func(t *testing.T) { + bg := &blueGreen{blueMachines: []*machineUpdateEntry{ + makeMachineEntry("app", true, true), + makeMachineEntry("app", true, true), + makeMachineEntry("app", true, true), + }} + + assert.Equal(t, map[int]bool{0: true}, bg.forceStartRepresentatives(), + "only the first stopped machine in the group should be promoted") + }) + + t.Run("multiple stopped groups get one representative each", func(t *testing.T) { + bg := &blueGreen{blueMachines: []*machineUpdateEntry{ + makeMachineEntry("app", true, true), + makeMachineEntry("worker", true, true), + makeMachineEntry("app", true, true), + makeMachineEntry("worker", true, true), + }} + + assert.Equal(t, map[int]bool{0: true, 1: true}, bg.forceStartRepresentatives(), + "each process group with checks needs its own representative") + }) + + t.Run("only groups with checks get a representative", func(t *testing.T) { + bg := &blueGreen{blueMachines: []*machineUpdateEntry{ + makeMachineEntry("app", true, true), // has checks -> force + makeMachineEntry("worker", true, false), // no checks -> mirror blue + }} + + assert.Equal(t, map[int]bool{0: true}, bg.forceStartRepresentatives()) + }) +} + +// TestCreateGreenMachinesMirrorsBlueSkipLaunchForNonRepresentatives verifies +// that, given multiple stopped blue machines in a process group with checks, +// exactly one green is force-started and the rest inherit SkipLaunch=true +// (so they mirror the pre-deploy stopped state). +func TestCreateGreenMachinesMirrorsBlueSkipLaunchForNonRepresentatives(t *testing.T) { + client := &mockFlapsClient{} + ctx := context.Background() + ctx = flapsutil.NewContextWithClient(ctx, client) + + ios, _, _, _ := iostreams.Test() + blues := []*machineUpdateEntry{ + { + leasableMachine: machine.NewLeasableMachine(client, ios, "", &fly.Machine{}, false), + launchInput: makeMachineEntry("app", true, true).launchInput, + }, + { + leasableMachine: machine.NewLeasableMachine(client, ios, "", &fly.Machine{}, false), + launchInput: makeMachineEntry("app", true, true).launchInput, + }, + { + leasableMachine: machine.NewLeasableMachine(client, ios, "", &fly.Machine{}, false), + launchInput: makeMachineEntry("app", true, true).launchInput, + }, + } + + strategy := &blueGreen{ + apiClient: &mockWebClient{}, + flaps: client, + maxConcurrent: 10, + appConfig: &appconfig.Config{}, + io: ios, + colorize: ios.ColorScheme(), + clearLinesAbove: func(int) {}, + timeout: 5 * time.Second, + blueMachines: blues, + app: &flaps.App{Name: "test-app"}, + } + strategy.initialize() + strategy.waitBeforeStop = 0 + strategy.waitBeforeCordon = 0 + strategy.uncordonRetryDelay = 0 + + err := strategy.CreateGreenMachines(ctx) + assert.NoError(t, err) + assert.Len(t, strategy.greenMachines, 3) + + client.mu.Lock() + inputs := client.launchInputs + client.mu.Unlock() + + require.Len(t, inputs, 3) + started := 0 + for _, in := range inputs { + if !in.SkipLaunch { + started++ + } + } + assert.Equal(t, 1, started, + "exactly one green machine in the group must be force-started; the rest mirror blue's stopped state") } // TestDeployWithStoppedBlueMachinesEnforcesHealthChecks verifies the full From 88a8c771fad229e6dc35c9105ae415e84f790131 Mon Sep 17 00:00:00 2001 From: lubien Date: Thu, 6 Aug 2026 09:08:45 -0300 Subject: [PATCH 5/5] Add unhealthyGet field to test config change regressions --- internal/command/deploy/mock_client_test.go | 18 +++++++-- .../command/deploy/strategy_bluegreen_test.go | 39 +++++++++++++++++++ 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/internal/command/deploy/mock_client_test.go b/internal/command/deploy/mock_client_test.go index 30a5820ea8..7e4ba5fbaf 100644 --- a/internal/command/deploy/mock_client_test.go +++ b/internal/command/deploy/mock_client_test.go @@ -27,7 +27,12 @@ type mockFlapsClient struct { breakDestroy bool breakLease bool breakGet bool - launchInputs []fly.LaunchMachineInput + // unhealthyGet, when true, makes Get return a Machine whose health check + // status is Critical. Used to simulate an app that starts but whose + // checks fail — e.g. because the new config changed the internal_port + // or introduced a Phoenix force_ssl redirect that traps the probe. + unhealthyGet bool + launchInputs []fly.LaunchMachineInput // uncordonTransientFailures causes Uncordon to fail this many times before // succeeding, simulating transient API errors for retry tests. @@ -146,12 +151,17 @@ func (m *mockFlapsClient) Get(ctx context.Context, appName, machineID string) (* if m.breakGet { return nil, fmt.Errorf("failed to get %s", machineID) } - // Return a machine with one passing check so that health-check loops - // can exit cleanly in tests that don't specifically test failure paths. + status := fly.Passing + if m.unhealthyGet { + status = fly.Critical + } + // Return a machine with a single check whose status is controlled by + // unhealthyGet. Health-check loops exit on all-passing; setting Critical + // keeps them polling until the strategy's timeout fires. return &fly.Machine{ ID: machineID, Checks: []*fly.MachineCheckStatus{ - {Name: "check1", Status: fly.Passing}, + {Name: "check1", Status: status}, }, Config: &fly.MachineConfig{ Checks: map[string]fly.MachineCheck{ diff --git a/internal/command/deploy/strategy_bluegreen_test.go b/internal/command/deploy/strategy_bluegreen_test.go index dfe1eba18e..cf52589072 100644 --- a/internal/command/deploy/strategy_bluegreen_test.go +++ b/internal/command/deploy/strategy_bluegreen_test.go @@ -472,3 +472,42 @@ func TestDeployWithStoppedBlueMachinesEnforcesHealthChecks(t *testing.T) { assert.NoError(t, err, "deploy must succeed when health checks pass") }) } + +// TestBlueGreenAbortsWhenGreenChecksFailAfterConfigChange exercises the +// primary value of the fix: catching an app-level misconfiguration that +// silently breaks health checks on the new deploy. +// +// Concrete real-world regressions this protects against: +// +// - The new config changes `internal_port` (or the app now listens on a +// different port), so the health probe can't reach the process at all. +// - The app adds `force_ssl` (e.g. Phoenix's default) or another redirect +// rule that traps the health probe with a 3xx, so `/` never returns 200. +// - Any other startup-time regression that keeps the machine "running" but +// makes the configured HTTP check fail. +// +// Pre-fix behaviour with auto-stopped blue machines: green machines were +// silently marked "1/1 passing" without ever running the check, blues were +// destroyed, and traffic hit a broken app. The representative-force-start +// fix ensures at least one green in each check-having group actually starts +// and gets polled — which is exactly what surfaces these regressions. +func TestBlueGreenAbortsWhenGreenChecksFailAfterConfigChange(t *testing.T) { + // unhealthyGet=true makes the mock return a Machine whose one configured + // check reports Critical, mimicking any of the misconfiguration modes + // listed above from the poller's point of view. + client := &mockFlapsClient{unhealthyGet: true} + ctx := context.Background() + ctx = flapsutil.NewContextWithClient(ctx, client) + + // Every blue is stopped (auto_stop_machines scenario). Without the fix, + // all greens would inherit SkipLaunch=true and skip health polling + // entirely — the misconfiguration would slip through undetected. + strategy := newBlueGreenStrategyWithState(client, fly.MachineStateStopped, true) + strategy.timeout = 500 * time.Millisecond // don't let the test hang + + err := strategy.Deploy(ctx) + assert.Error(t, err, + "deploy must abort when the representative green machine's health checks fail, "+ + "even though blue machines were all auto-stopped "+ + "(config regressions like a bad internal_port or force_ssl redirect must be caught)") +}