fix: keep canary serving until roll completes to avoid downtime (#4745)

internal/command/deploy/machines_deploymachinesapp.go

@@ -288,9 +288,14 @@
 	return canaryGuest
 }
 
-// deployCanaryMachines creates canary machines for each process group.
-// The canary machines are destroyed before returning to the caller.
-func (md *machineDeployment) deployCanaryMachines(ctx context.Context) (err error) {
+// deployCanaryMachines creates and health-checks a canary machine for each
+// process group, returning the canaries to the caller. Unlike a smoke test,
+// the canaries are registered in DNS so they serve traffic, and they are kept
+// running until the caller rolls the existing machines. This keeps n+1 healthy
+// serving instances available throughout the roll, avoiding downtime for
+// single-machine apps. The caller is responsible for destroying the returned
+// canaries (see destroyCanaryMachines) once the roll is complete.
+func (md *machineDeployment) deployCanaryMachines(ctx context.Context) (canaries []machine.LeasableMachine, err error) {
 	ctx, span := tracing.GetTracer().Start(ctx, "deploy_canary")
 	defer span.End()
 
@@ -299,6 +304,8 @@
 	sl := statuslogger.Create(ctx, total, true)
 	defer sl.Destroy(false)
 
+	canaries = make([]machine.LeasableMachine, total)
+
 	errors, ctx := errgroup.WithContext(ctx)
 
 	for idx, name := range groupsInConfig {
@@ -313,11 +320,9 @@
 		name := name
 		idx := idx
 		errors.Go(func() error {
-			var err error
 			lm, err := md.spawnMachineInGroup(ctx, name, nil,
 				withMeta(metadata{key: "fly_canary", value: "true"}),
 				withGuest(md.inferCanaryGuest(name)),
-				withDns(&fly.DNSConfig{SkipRegistration: true}),
 			)
 			if err != nil {
 				tracing.RecordError(span, err, "failed to provision canary machine")
@@ -327,31 +332,44 @@
 				return err
 			}
 
-			defer func() {
-				if err == nil {
-					if destroyErr := machcmd.Destroy(ctx, md.app.Name, lm.Machine(), true); destroyErr != nil {
-						err = destroyErr
-					}
-				}
-			}()
+			// Safe to assign without a lock: each goroutine writes a distinct index.
+			canaries[idx] = lm
 
-			if err = md.runTestMachines(ctx, lm.Machine(), sl.Line(idx)); err != nil {
+			if err := md.runTestMachines(ctx, lm.Machine(), sl.Line(idx)); err != nil {
 				tracing.RecordError(span, err, "failed to run test machine for canary machine")
 				firstLine, _, _ := strings.Cut(err.Error(), "\n")
 				statuslogger.LogfStatus(ctx, statuslogger.StatusFailure, "Failed to run test machine for canary machine: %s", firstLine)
 
 				return err
 			}
 
-			return err
+			return nil
 		})
 	}
 
 	if err := errors.Wait(); err != nil {
-		return err
+		// Clean up any canaries that were created before the failure so we
+		// don't leak them when the deployment aborts.
+		md.destroyCanaryMachines(ctx, canaries)
+
+		return nil, err
 	}
 
-	return nil
+	return canaries, nil
+}
+
+// destroyCanaryMachines destroys the canary machines created by
+// deployCanaryMachines. nil entries (canaries that were never created) are
+// skipped, and destroy failures are logged but do not abort the deployment.
+func (md *machineDeployment) destroyCanaryMachines(ctx context.Context, canaries []machine.LeasableMachine) {
+	for _, lm := range canaries {
+		if lm == nil {
+			continue
+		}
+		if err := machcmd.Destroy(ctx, md.app.Name, lm.Machine(), true); err != nil {
+			terminal.Warnf("failed to destroy canary machine %s: %s", lm.FormattedMachineId(), err)
+		}
+	}
 }
 
 // Create machines for new process groups
@@ -479,9 +497,16 @@
 	md.warnAboutProcessGroupChanges(processGroupMachineDiff)
 
 	if md.strategy == "canary" && !md.isFirstDeploy {
-		if err := md.deployCanaryMachines(ctx); err != nil {
+		canaries, err := md.deployCanaryMachines(ctx)
+		if err != nil {
 			return err
 		}
+		// Keep the canaries serving traffic alongside the existing machines
+		// while they are rolled, then tear them down once the roll is complete
+		// (or has failed). Destroying them only after the roll guarantees an
+		// extra healthy instance is always available, avoiding downtime for
+		// single-machine apps (issue #4745).
+		defer md.destroyCanaryMachines(ctx, canaries)
 	}
 
 	// Destroy machines that don't fit the current process groups
@@ -1094,7 +1119,7 @@
 	}
 }
 
 func withDns(dns *fly.DNSConfig) spawnOptionsFn {
 	return func(o *spawnOptions) {
 		o.dns = dns
 	}

internal/command/deploy/machines_deploymachinesapp_test.go

@@ -6,6 +6,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"github.com/superfly/fly-go"
 	"github.com/superfly/fly-go/flaps"
 	"github.com/superfly/flyctl/internal/appconfig"
@@ -76,3 +77,64 @@ func TestDeployMachinesApp(t *testing.T) {
 	err := md.deployMachinesApp(ctx)
 	assert.NoError(t, err)
 }
+
+// TestDeployCanaryKeepsInstanceDuringRoll guards against issue #4745: a canary
+// deploy must not destroy the canary machine before the existing machines are
+// rolled. Destroying the canary first leaves a window with no extra serving
+// instance, which causes downtime for single-machine (n=1) apps. The canary
+// must outlive the roll so that n+1 instances exist throughout it.
+func TestDeployCanaryKeepsInstanceDuringRoll(t *testing.T) {
+	ios, _, _, _ := iostreams.Test()
+	client := &mockFlapsClient{}
+	webClient := &mock.Client{
+		GetAppLogsFunc: func(ctx context.Context, appName, token, region, instanceID string) (entries []fly.LogEntry, nextToken string, err error) {
+			return nil, "", nil
+		},
+	}
+	// The n=1 case from the issue: a single existing serving machine.
+	client.machines = []*fly.Machine{
+		{ID: "m1", LeaseNonce: "m1-lease", Config: &fly.MachineConfig{Metadata: map[string]string{fly.MachineConfigMetadataKeyFlyProcessGroup: "app"}}},
+	}
+	md := &machineDeployment{
+		app:             &flaps.App{},
+		io:              ios,
+		colorize:        ios.ColorScheme(),
+		flapsClient:     client,
+		apiClient:       webClient,
+		strategy:        "canary",
+		appConfig:       &appconfig.Config{},
+		machineSet:      machine.NewMachineSet(client, ios, "", client.machines, false),
+		skipSmokeChecks: true,
+		waitTimeout:     1 * time.Second,
+	}
+
+	// Shorten the NATS timeout since it's likely to require the fallback in CI
+	natsConnectTimeout = md.waitTimeout
+
+	ctx := context.Background()
+	ctx = iostreams.NewContext(ctx, ios)
+	ctx = flapsutil.NewContextWithClient(ctx, client)
+	err := md.deployMachinesApp(ctx)
+	assert.NoError(t, err)
+
+	client.mu.Lock()
+	defer client.mu.Unlock()
+
+	require.NotEmpty(t, client.canaryIDs, "expected a canary machine to be launched")
+	canaryID := client.canaryIDs[0]
+
+	canaryDestroyIdx := client.eventIndex("destroy:" + canaryID)
+	require.GreaterOrEqual(t, canaryDestroyIdx, 0,
+		"canary machine %s was never destroyed; events=%v", canaryID, client.events)
+
+	// The existing machine must be rolled (destroyed and replaced, or updated)
+	// before the canary is torn down, so that there is always an extra serving
+	// instance available during the roll.
+	rollIdx := client.eventIndex("destroy:m1")
+	require.GreaterOrEqual(t, rollIdx, 0,
+		"expected existing machine m1 to be rolled; events=%v", client.events)
+
+	assert.Greater(t, canaryDestroyIdx, rollIdx,
+		"canary %s must be destroyed AFTER the existing machine is rolled to avoid downtime; events=%v",
+		canaryID, client.events)
+}

internal/command/deploy/mock_client_test.go

@@ -35,6 +35,26 @@ type mockFlapsClient struct {
 	machines      []*fly.Machine
 	leases        map[string]struct{}
 	nextMachineID int
+
+	// events records an ordered log of notable flaps operations (e.g.
+	// "launch:<id>", "destroy:<id>") so tests can assert ordering, such as
+	// when a canary machine is destroyed relative to the rolling update.
+	events []string
+	// canaryIDs holds the IDs of machines launched with the fly_canary
+	// metadata, in launch order.
+	canaryIDs []string
+}
+
+// eventIndex returns the index of the first occurrence of the given event in
+// the recorded event log, or -1 if it was never recorded. Callers must hold m.mu.
+func (m *mockFlapsClient) eventIndex(event string) int {
+	for i, e := range m.events {
+		if e == event {
+			return i
+		}
+	}
+
+	return -1
 }
 
 func (m *mockFlapsClient) AcquireLease(ctx context.Context, appName, machineID string, ttl *int) (*fly.MachineLease, error) {
@@ -112,6 +132,10 @@ func (m *mockFlapsClient) DeleteVolume(ctx context.Context, appName, volumeId st
 }
 
 func (m *mockFlapsClient) Destroy(ctx context.Context, appName string, input fly.RemoveMachineInput, nonce string) (err error) {
+	m.mu.Lock()
+	m.events = append(m.events, "destroy:"+input.ID)
+	m.mu.Unlock()
+
 	if m.breakDestroy {
 		return fmt.Errorf("failed to destroy %s", input.ID)
 	}
@@ -200,9 +224,15 @@ func (m *mockFlapsClient) Launch(ctx context.Context, appName string, builder fl
 		return nil, fmt.Errorf("failed to launch %s", builder.ID)
 	}
 	m.nextMachineID += 1
+	id := fmt.Sprintf("%x", m.nextMachineID)
+
+	m.events = append(m.events, "launch:"+id)
+	if builder.Config != nil && builder.Config.Metadata["fly_canary"] == "true" {
+		m.canaryIDs = append(m.canaryIDs, id)
+	}
 
 	return &fly.Machine{
-		ID:         fmt.Sprintf("%x", m.nextMachineID),
+		ID:         id,
 		LeaseNonce: fmt.Sprintf("%x-launch-lease", m.nextMachineID),
 	}, nil
 }