From 1ead2d2345bebcbe14de30d68fbc82a5da0cae0a Mon Sep 17 00:00:00 2001 From: Kyle McLaren Date: Sat, 27 Jun 2026 08:12:24 +0000 Subject: [PATCH] fix: error instead of panic on max_unavailable=0 in fly.toml (#4262) A [deploy] max_unavailable = 0 value read from fly.toml bypassed the --max-unavailable flag's "> 0" validation, producing a pool size of 0 that reached acquireLeases and hit a hard panic ("pool size must be > 0"). Validate the config-file value where it is read in NewMachineDeployment, mirroring the flag check, so the user gets a clear error. As defense-in-depth, clamp getPoolSize to a minimum of 1 so a 0/negative value can never panic acquireLeases. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/command/deploy/machines.go | 7 ++++++ .../deploy/machines_deploymachinesapp.go | 4 +++- internal/command/deploy/plan_test.go | 24 +++++++++++++++++++ 3 files changed, 34 insertions(+), 1 deletion(-) diff --git a/internal/command/deploy/machines.go b/internal/command/deploy/machines.go index c509f66043..79dc5d6f6b 100644 --- a/internal/command/deploy/machines.go +++ b/internal/command/deploy/machines.go @@ -246,6 +246,13 @@ func NewMachineDeployment(ctx context.Context, args MachineDeploymentArgs) (_ Ma if appConfig.Deploy != nil && appConfig.Deploy.MaxUnavailable != nil { maxUnavailable = *appConfig.Deploy.MaxUnavailable } + // The --max-unavailable flag is validated in deploy.go, but a value read + // straight from fly.toml's [deploy] section bypasses that check. Guard it + // here so a max_unavailable of 0 yields a friendly error instead of an + // eventual "pool size must be > 0" panic. See #4262. + if maxUnavailable <= 0 { + return nil, fmt.Errorf("max_unavailable must be > 0 (got %v); set it to at least 1 or a fraction like 0.33", maxUnavailable) + } maxConcurrent := max(args.MaxConcurrent, 1) diff --git a/internal/command/deploy/machines_deploymachinesapp.go b/internal/command/deploy/machines_deploymachinesapp.go index e873fbbdc2..318739dc47 100644 --- a/internal/command/deploy/machines_deploymachinesapp.go +++ b/internal/command/deploy/machines_deploymachinesapp.go @@ -920,7 +920,9 @@ func (md *machineDeployment) getPoolSize(totalMachines int) int { case mu >= 1: return int(mu) default: - return int(math.Ceil(float64(totalMachines) * mu)) + // Never return a pool size below 1: a 0 (or negative) value would + // panic in acquireLeases ("pool size must be > 0"). See #4262. + return max(1, int(math.Ceil(float64(totalMachines)*mu))) } } diff --git a/internal/command/deploy/plan_test.go b/internal/command/deploy/plan_test.go index 0aa3ea9494..8aae9c5f84 100644 --- a/internal/command/deploy/plan_test.go +++ b/internal/command/deploy/plan_test.go @@ -47,6 +47,30 @@ func TestCompareConfig(t *testing.T) { assert.False(t, compareConfigs(ctx, config1, config2)) } +func TestGetPoolSize(t *testing.T) { + t.Parallel() + + // Regression test for #4262: a max_unavailable of 0 (e.g. from fly.toml + // [deploy] max_unavailable = 0) must never produce a pool size of 0, which + // previously flowed into acquireLeases and panicked with + // "pool size must be > 0". + cases := []struct { + maxUnavailable float64 + totalMachines int + want int + }{ + {maxUnavailable: 0, totalMachines: 4, want: 1}, + {maxUnavailable: 0.33, totalMachines: 4, want: 2}, + {maxUnavailable: 1, totalMachines: 4, want: 1}, + {maxUnavailable: 3, totalMachines: 4, want: 3}, + } + for _, tc := range cases { + md := &machineDeployment{maxUnavailable: tc.maxUnavailable} + assert.GreaterOrEqual(t, md.getPoolSize(tc.totalMachines), 1) + assert.Equal(t, tc.want, md.getPoolSize(tc.totalMachines)) + } +} + func TestAppState(t *testing.T) { t.Parallel()