diff --git a/flyctl/BLUEGREEN_HEALTHCHECK_BUG.md b/flyctl/BLUEGREEN_HEALTHCHECK_BUG.md new file mode 100644 index 0000000000..8216bbefbe --- /dev/null +++ b/flyctl/BLUEGREEN_HEALTHCHECK_BUG.md @@ -0,0 +1,231 @@ +# Bug: Blue-Green Deployment Bypasses Health Checks When Machines Are Stopped + +**Investigated:** flyctl `fly deploy --strategy bluegreen` +**Test app:** `hello-fly-private` (with `CRASH_NOW=1` env var causing immediate crash) +**Symptom:** Deployment reports success and shows "1/1 passing" health checks even though the app crashes on startup and all machines end up stopped. + +--- + +## The Short Answer + +When the existing **blue machines** are in a **`stopped`** state (e.g. because `auto_stop_machines = 'stop'` stopped them due to no traffic), their `launchInput.SkipLaunch = true` flag gets **silently inherited by the green machines**. Green machines are then created but never started, their health is never verified, and they are marked as `{Total: 1, Passing: 1}` — instantly and unconditionally — before the old machines are destroyed. + +--- + +## The Full Causal Chain + +### Step 1 — `skipLaunch()` returns `true` for stopped machines + +```go +// internal/command/deploy/machines_launchinput.go +func skipLaunch(origMachineRaw *fly.Machine, mConfig *fly.MachineConfig) bool { + state := "" + if origMachineRaw != nil { + state = origMachineRaw.State + } + + switch { + case slices.Contains([]string{fly.MachineStateStarted, "starting", "failed"}, state): + return false // ← only these three states skip the skip + case len(mConfig.Standbys) > 0: + return true + case origMachineRaw == nil: + return false + } + + return true // ← everything else (stopped, suspended, created, …) → SkipLaunch = true +} +``` + +With `auto_stop_machines = 'stop'` and `min_machines_running = 0`, Fly's platform automatically stops machines after they become idle. When a new deploy is triggered, the existing (blue) machines may already be in `stopped` state. `skipLaunch` sees `state = "stopped"` and falls through to `return true`. + +**This is correct behavior for rolling/immediate deployments** — you don't want to force-start a stopped machine just to update its config. + +### Step 2 — `CreateGreenMachines` inherits `SkipLaunch` without resetting it + +```go +// internal/command/deploy/strategy_bluegreen.go +func (bg *blueGreen) CreateGreenMachines(ctx context.Context) error { + for _, mach := range bg.blueMachines { + p.Go(func() error { + launchInput := mach.launchInput // ← copy of blue machine's input + launchInput.SkipServiceRegistration = true + // SkipLaunch is NOT reset here — it inherits from the blue machine! + + newMachineRaw, err := bg.flaps.Launch(ctx, bg.app.Name, *launchInput) + // ... + bg.greenMachines = append(bg.greenMachines, &machineUpdateEntry{greenMachine, launchInput}) + }) + } +} +``` + +The green machine's `launchInput` is a shallow copy of the blue machine's `launchInput`. Because `SkipLaunch` is never explicitly set to `false`, the green machine is created with `SkipLaunch = true` — meaning **the Fly API creates the machine but never starts it** (it stays in `created`/`stopped` state). + +### Step 3 — `WaitForGreenMachinesToBeStarted` trivially succeeds + +```go +// strategy_bluegreen.go +for _, gm := range bg.greenMachines { + if gm.launchInput.SkipLaunch { + machineIDToState[id] = "started" // ← instantly marked as started, no waiting + continue + } + go func(lm machine.LeasableMachine) { + err := machine.WaitForStartOrStop(...) // ← never reached + }(gm.leasableMachine) +} +``` + +Every green machine has `SkipLaunch = true`, so they all get immediately recorded as `"started"` in the state map. The output reads: + +``` +Waiting for all green machines to start + Machine d896005c5015e8 [app] - started ← machine was never actually started + Machine 6830971f5706e8 [app] - started +``` + +### Step 4 — `WaitForGreenMachinesToBeHealthy` trivially succeeds + +```go +// strategy_bluegreen.go +for _, gm := range bg.greenMachines { + if gm.launchInput.SkipLaunch { + // Unconditionally pre-mark as fully healthy + machineIDToHealthStatus[gm.leasableMachine.FormattedMachineId()] = + &fly.HealthCheckStatus{Total: 1, Passing: 1} + continue + } + // ... real health checking goroutine — never reached +} +``` + +All green machines are immediately slotted into `machineIDToHealthStatus` as `{Total: 1, Passing: 1}`. No goroutine is ever started. No API poll for actual health occurs. The first call to `allMachinesHealthy` sees every machine as passing and returns `true`. + +The output reads: + +``` +Waiting for all green machines to be healthy + Machine d896005c5015e8 [app] - 1/1 passing ← fabricated, machine is stopped + Machine 6830971f5706e8 [app] - 1/1 passing +``` + +### Step 5 — Green machines are "uncordoned" (made ready for traffic) while stopped + +`MarkGreenMachinesAsReadyForTraffic` calls `flaps.Uncordon()` on all green machines +with no `SkipLaunch` guard. The machines get uncordoned (marked as ready for traffic) +even though they are still in `stopped`/`created` state. + +The machine's event log confirms the whole sequence: + +``` + stopped │ update │ flyd │ ... ← auto-stopped shortly after + created │ uncordon │ user │ ... ← uncordoned while still in created state + created │ launch │ user │ ... ← created but NOT started (SkipLaunch=true in API call) + pending │ launch │ flyd │ ... +``` + +### Step 6 — Blue machines are destroyed + +Old machines are cordoned, stopped, and destroyed. The new "green" machines are now +the only ones — all stopped, all broken, none serving traffic. + +``` +fly machines list +ID │ STATE │ CHECKS +d896005c5015e8 │ stopped │ 0/2 ← both checks failing +6830971f5706e8 │ stopped │ 0/2 +``` + +--- + +## Why `auto_stop_machines` Is the Trigger (But Not the Only Way) + +With `auto_stop_machines = 'stop'` and `min_machines_running = 0`, any period of zero +incoming traffic will auto-stop all machines. A deploy that immediately follows such a +quiet period will find blue machines in `stopped` state and trigger this bug. + +The bug also fires whenever blue machines happen to be stopped for any other reason: +manual `fly machine stop`, a crash that wasn't restarted, a suspended machine, etc. + +--- + +## The Validation Check Does Not Save You + +`Deploy()` validates that blue machines have health checks configured before starting +the blue-green flow: + +```go +totalMachinesWithChecks := 0 +for _, entry := range bg.blueMachines { + machineChecks := len(entry.launchInput.Config.Checks) + for _, service := range entry.launchInput.Config.Services { + machineChecks += len(service.Checks) + } + if machineChecks == 0 { continue } + totalMachinesWithChecks++ +} +if totalMachinesWithChecks == 0 && len(bg.blueMachines) != 0 { + return ErrValidationError // ← fails correctly if no checks configured +} +``` + +This check validates that health checks are **configured** (in `Config.Checks` / +`Config.Services[*].Checks`) — which passes fine if you have checks in `fly.toml`. But +it does not validate that green machines will actually be **started and checked**. The +`SkipLaunch` bypass completely avoids the real health checking loop regardless. + +--- + +## The Fix + +In `CreateGreenMachines`, explicitly reset `SkipLaunch = false` before launching: + +```go +// strategy_bluegreen.go — CreateGreenMachines +launchInput := mach.launchInput +launchInput.SkipServiceRegistration = true +launchInput.SkipLaunch = false // ← green machines must ALWAYS be started +launchInput.Config.Metadata[fly.MachineConfigMetadataKeyFlyctlBGTag] = bg.timestamp +``` + +Green machines in a blue-green deployment are always brand-new machines that must be +started, passed health checks, and proven healthy before old machines are destroyed. +The `SkipLaunch` semantics from the blue machine's update context ("don't forcibly +restart a stopped machine") is irrelevant and harmful in this context. + +--- + +## Secondary Issue Also Worth Noting + +Even if `SkipLaunch` were always `false`, a second (independent) issue exists in +`WaitForGreenMachinesToBeHealthy`: + +```go +if len(gm.leasableMachine.Machine().Checks) == 0 { + continue // skip health checking for this machine +} +``` + +`Machine().Checks` is the **runtime health-check status** field (`[]*MachineCheckStatus`), +not the configuration. For a freshly launched machine it is often empty because health +checks haven't run yet. This check was designed to skip machines with no checks +**configured**, but it is reading the wrong field — it should be checking +`Machine().Config.Checks` (machine-level check config) and +`Machine().Config.Services[*].Checks` (service-level check config), exactly like +`GetMinIntervalAndMinGracePeriod()` does. + +In the `SkipLaunch` scenario, this secondary issue is never reached. But if `SkipLaunch` +is fixed, a machine that launches and briefly has an empty `Machine.Checks` runtime +status could still slip through this second guard. + +--- + +## Files Involved + +| File | Role | +|------|------| +| `internal/command/deploy/strategy_bluegreen.go` | `CreateGreenMachines` (bug origin), `WaitForGreenMachinesToBeStarted`, `WaitForGreenMachinesToBeHealthy`, `allMachinesHealthy` | +| `internal/command/deploy/machines_launchinput.go` | `skipLaunch()` — sets `SkipLaunch` for blue machines based on their current state | +| `internal/machine/leasable_machine.go` | `WaitForHealthchecksToPass` — same secondary issue (`Machine().Checks` vs `Config.Checks`) | +| `github.com/superfly/fly-go` `machine_types.go` | `Machine.Checks` (runtime status), `Machine.Config.Checks` (config), `AllHealthChecks()`, `RemoveCompatChecks()` | diff --git a/flyctl/BLUEGREEN_SKIPLAUNCHED_UX.md b/flyctl/BLUEGREEN_SKIPLAUNCHED_UX.md new file mode 100644 index 0000000000..7b0444531c --- /dev/null +++ b/flyctl/BLUEGREEN_SKIPLAUNCHED_UX.md @@ -0,0 +1,26 @@ +# Blue-Green Deploy: Silent Health Check Bypass on Stopped Machines + +## What happens + +When you run `fly deploy --strategy bluegreen` and your existing machines are **stopped** (e.g. auto-stopped due to no traffic), the deploy: + +1. Reports all green machines as **started** — they were never started +2. Reports all health checks as **1/1 passing** — no check was ever run +3. Destroys the old machines +4. Leaves you with a broken app and zero traffic served + +No error. No warning. Just "Deployment Complete." + +## When does it trigger + +Any time blue machines are not in `started`, `starting`, or `failed` state when a deploy kicks off. The most common scenario: + +- `auto_stop_machines = 'stop'` with `min_machines_running = 0` (a very common, recommended config for low-traffic apps) +- A period of zero traffic stops all machines +- Developer deploys — the whole point of blue-green was to catch regressions safely + +This is exactly the use case blue-green exists for, and it's exactly when the safety net silently disappears. + +## Why it's bad + +Blue-green's entire value proposition is **"don't cut over until the new version is proven healthy."** When this bug fires, users get the opposite: a false sense of safety. The deploy looks successful, CI goes green, and the app is down. The user has to go investigate `fly machines list` or check the monitoring dashboard to discover what happened — nothing in the deploy output hints that anything went wrong. diff --git a/internal/command/deploy/mock_client_test.go b/internal/command/deploy/mock_client_test.go index b7b30d84d3..7a60daa2bb 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,23 @@ 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..1c796cd0bd 100644 --- a/internal/command/deploy/strategy_bluegreen.go +++ b/internal/command/deploy/strategy_bluegreen.go @@ -90,6 +90,22 @@ 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 +190,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 +407,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 +422,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..004e645e74 --- /dev/null +++ b/internal/machine/leasable_machine_test.go @@ -0,0 +1,205 @@ +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{}