diff --git a/internal/command/deploy/plan.go b/internal/command/deploy/plan.go index 12f1266eb7..b905305f0b 100644 --- a/internal/command/deploy/plan.go +++ b/internal/command/deploy/plan.go @@ -559,7 +559,7 @@ func (md *machineDeployment) updateMachineWChecks(ctx context.Context, oldMachin } if !healthcheckResult.machineChecksPassed || !healthcheckResult.smokeChecksPassed { - sl.LogStatus(statuslogger.StatusRunning, fmt.Sprintf("Waiting for machine %s to reach a good state", oldMachine.ID)) + sl.LogStatus(statuslogger.StatusRunning, fmt.Sprintf("Waiting for machine %s to reach a good state", machine.ID)) _, err := waitForMachineState(ctx, lm, []string{"stopped", "started", "suspended"}, md.waitTimeout, sl) if err != nil { span.RecordError(err) diff --git a/internal/command/deploy/plan_test.go b/internal/command/deploy/plan_test.go index 6654f7d711..ee3510f625 100644 --- a/internal/command/deploy/plan_test.go +++ b/internal/command/deploy/plan_test.go @@ -368,6 +368,156 @@ func TestUpdateMachines(t *testing.T) { assert.ErrorAs(t, err, &unrecoverableErr) } +// TestUpdateMachinesWithNewMachine verifies that deploying with a new machine +// (oldMachine=nil) doesn't panic when health checks need to run. This +// reproduces the nil pointer dereference that occurred when oldMachine.ID was +// accessed in updateMachineWChecks for a newly created machine. +func TestUpdateMachinesWithNewMachine(t *testing.T) { + t.Parallel() + + ctx := withQuietIOStreams(context.Background()) + + // Old state has one machine; new state adds a second machine that doesn't + // exist in the old state, producing a pairing with oldMachine=nil. + oldMachines := []*fly.Machine{ + { + ID: "machine1", + State: "started", + HostStatus: fly.HostStatusOk, + Config: &fly.MachineConfig{ + Image: "image1", + }, + }, + } + + newMachines := []*fly.Machine{ + { + ID: "machine1", + State: "started", + HostStatus: fly.HostStatusOk, + Config: &fly.MachineConfig{ + Image: "image2", + }, + }, + { + ID: "new-machine", + State: "started", + Region: "iad", + HostStatus: fly.HostStatusOk, + Config: &fly.MachineConfig{ + Image: "image2", + }, + }, + } + + acquiredLeases := sync.Map{} + + flapsClient := &mock.FlapsClient{ + AcquireLeaseFunc: func(ctx context.Context, appName, machineID string, ttl *int) (*fly.MachineLease, error) { + if _, ok := acquiredLeases.Load(machineID); ok { + return nil, assert.AnError + } + acquiredLeases.Store(machineID, true) + return &fly.MachineLease{ + Data: &fly.MachineLeaseData{ + Nonce: machineID + "nonce", + }, + }, nil + }, + ReleaseLeaseFunc: func(ctx context.Context, appName, machineID, nonce string) error { + acquiredLeases.Delete(machineID) + return nil + }, + UpdateFunc: func(ctx context.Context, appName string, builder fly.LaunchMachineInput, nonce string) (out *fly.Machine, err error) { + return &fly.Machine{ + ID: builder.ID, + Config: builder.Config, + State: "started", + HostStatus: fly.HostStatusOk, + }, nil + }, + LaunchFunc: func(ctx context.Context, appName string, builder fly.LaunchMachineInput) (out *fly.Machine, err error) { + return &fly.Machine{ + ID: "created-machine", + Config: builder.Config, + State: "started", + HostStatus: fly.HostStatusOk, + }, nil + }, + DestroyFunc: func(ctx context.Context, appName string, input fly.RemoveMachineInput, nonce string) (err error) { + return nil + }, + WaitFunc: func(ctx context.Context, appName string, machine *fly.Machine, state string, timeout time.Duration) (err error) { + machine.State = state + return nil + }, + ListFunc: func(ctx context.Context, appName, state string) ([]*fly.Machine, error) { + return oldMachines, nil + }, + StartFunc: func(ctx context.Context, appName, machineID string, nonce string) (out *fly.MachineStartResponse, err error) { + return &fly.MachineStartResponse{}, nil + }, + GetFunc: func(ctx context.Context, appName, machineID string) (*fly.Machine, error) { + for _, m := range newMachines { + if m.ID == machineID { + return m, nil + } + } + // Return the created machine for the newly launched machine + return &fly.Machine{ + ID: machineID, + State: "started", + HostStatus: fly.HostStatusOk, + Config: &fly.MachineConfig{Image: "image2"}, + }, nil + }, + GetProcessesFunc: func(ctx context.Context, appName, machineID string) (fly.MachinePsResponse, error) { + return fly.MachinePsResponse{}, nil + }, + RefreshLeaseFunc: func(ctx context.Context, appName, machineID string, ttl *int, nonce string) (*fly.MachineLease, error) { + return &fly.MachineLease{ + Status: "success", + Data: &fly.MachineLeaseData{ + Nonce: nonce, + }, + }, nil + }, + } + + ctx = flapsutil.NewContextWithClient(ctx, flapsClient) + md := &machineDeployment{ + flapsClient: flapsClient, + io: iostreams.FromContext(ctx), + app: &flaps.App{ + Name: "myapp", + }, + appConfig: &appconfig.Config{AppName: "myapp"}, + waitTimeout: 10 * time.Second, + deployRetries: 5, + maxUnavailable: 3, + skipSmokeChecks: true, + } + + oldAppState := &AppState{ + Machines: oldMachines, + } + newAppState := &AppState{ + Machines: newMachines, + } + + // skipSmokeChecks=false ensures we enter the code path that previously + // dereferenced oldMachine.ID (which is nil for new machines). + settings := updateMachineSettings{ + pushForward: true, + skipHealthChecks: false, + skipSmokeChecks: false, + skipLeaseAcquisition: false, + } + + err := md.updateMachinesWRecovery(ctx, oldAppState, newAppState, nil, settings) + assert.NoError(t, err) +} + func TestUpdateOrCreateMachine(t *testing.T) { t.Parallel()