Refuse a Docker deployment before stopping anything

Cutover already refused a container -- recreating one from a new image is
not swapping a binary and rewriting a unit, and this tool does not automate
it -- but it refused from cutover.Run, which run.go calls at line 324. The
service is stopped at line 243. So the sequence on a container was: stop
Stalwart, convert the settings, then discover the deployment cannot be cut
over, return the error, and exit with mail still down. Reported as #1.

The deployment kind is known in preflight, before anything has been touched,
and that is now where it is acted on: docker is a blocking check. rehearse
keeps working -- it never stops the service or cuts over, and telling an
operator what the migration involves is most useful precisely when the tool
cannot do it for them -- so it sets DeploymentCheckAdvisory, alongside the
ToolCheckAdvisory it already set for the same reason.

The second half is not docker's alone. Every return between the stop and the
end of cutover returned with the service down; a failed settings conversion
would have done the same to a systemd host. run now registers a restart on
the way out, after the stop rather than before, so it only ever starts
something this tool stopped. It does not claim to have recovered the
migration -- a part-migrated store still needs --resume or the operator's
recovery point -- it removes the narrower failure of exiting on a
foreseeable error while the server it stopped stays stopped.

cmd/stalwart-migrate/rehearse.go

@@ -113,7 +113,7 @@ func runRehearse(args []string) (err error) {
 		BinaryPath: *binaryPath, ConfigPath: *configPath, DataDir: *dataDir, ContainerName: *containerName,
 		AdminURL: *adminURL, AdminUser: *adminUser, AdminPassword: *adminPassword,
 		TargetVersion: *targetVersion, MinFreeMultiple: *minFree, HTTPClient: httpClient,
-		CLIPath: *stalwartCLI, PythonPath: *pythonPath, ToolCheckAdvisory: true,
+		CLIPath: *stalwartCLI, PythonPath: *pythonPath, ToolCheckAdvisory: true, DeploymentCheckAdvisory: true,
 	})
 	pfReport, err := checker.Run(ctx, store, rs)
 	fmt.Print(pfReport.String())

cmd/stalwart-migrate/run.go

@@ -253,6 +253,41 @@ func runRun(args []string) (err error) {
 	}
 	fmt.Println(controller.Target(), "stopped")
 
+	// Mail is down from here, and every return below is a return with it
+	// still down. Registered after the stop rather than before, so it only
+	// ever restarts something this tool actually stopped.
+	//
+	// It does not pretend to have recovered the migration: a run that
+	// aborted midway is still part-migrated and still needs --resume or the
+	// operator's recovery point. What it prevents is the narrower and worse
+	// outcome of the tool exiting on an error it could see coming while the
+	// server it stopped stays stopped.
+	defer func() {
+		if err == nil {
+			return
+		}
+		// The run's context may already be cancelled - that can be why we
+		// are here - and a cancelled context cannot start anything.
+		restartCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 3*time.Minute)
+		defer cancel()
+		active, activeErr := controller.Active(restartCtx)
+		if activeErr == nil && active {
+			return
+		}
+		fmt.Println("\n--- this run failed with", controller.Target(), "stopped: starting it again ---")
+		if startErr := controller.Start(restartCtx); startErr != nil {
+			fmt.Printf("could not start %s: %v\n", controller.Target(), startErr)
+			fmt.Println("MAIL IS STILL DOWN - start it by hand before anything else.")
+			return
+		}
+		if waitErr := service.WaitFor(restartCtx, controller, true, 2*time.Minute); waitErr != nil {
+			fmt.Printf("%s was asked to start but did not come up: %v\n", controller.Target(), waitErr)
+			fmt.Println("MAIL IS STILL DOWN - check it by hand before anything else.")
+			return
+		}
+		fmt.Println(controller.Target(), "is running again. The migration itself did not complete - see the error below.")
+	}()
+
 	if p.CrossesMajorBoundary {
 		fmt.Println("\n--- convert ---")
 		if _, err := store.RunStep(rs, checkpoint.PhaseStage, "convert-settings", func() (checkpoint.StepOutcome, error) {

internal/preflight/checks.go

@@ -44,7 +44,15 @@ type Options struct {
 	// about to be used and a missing one means stopping a mail server to
 	// find out.
 	ToolCheckAdvisory bool
-	HTTPClient        *http.Client
+	// DeploymentCheckAdvisory downgrades the deployment-kind check from
+	// blocking to advisory, for the same reason as ToolCheckAdvisory.
+	// `rehearse` sets it: it never stops the service or cuts over, so a
+	// deployment this tool cannot cut over is still worth rehearsing
+	// against - the reconnaissance is exactly what tells an operator what
+	// the manual path involves. `run` leaves it false, because there the
+	// alternative is finding out after mail is already down.
+	DeploymentCheckAdvisory bool
+	HTTPClient              *http.Client
 }
 
 // Checker runs the preflight checks described in ARCHITECTURE.md §4.1.
@@ -186,6 +194,20 @@ func (c *Checker) Run(ctx context.Context, store *checkpoint.Store, rs *checkpoi
 
 	deploymentOutcome, err := runCheck("deployment-kind", func() (CheckResult, string) {
 		kind := DetectDeploymentKind(ctx, c.opts.ContainerName)
+		// Docker has to fail here rather than later. Cutover refuses this
+		// deployment - recreating a container from a new image is not
+		// swapping a binary and rewriting a unit, and this tool does not
+		// automate it - but cutover runs after the service has been
+		// stopped. Refusing there means refusing with mail already down,
+		// which is how a migration attempt turned into an outage.
+		if kind == DeploymentDocker && !c.opts.DeploymentCheckAdvisory {
+			return CheckResult{
+				Status: StatusFail,
+				Detail: "detected deployment kind: docker - this tool cannot cut over a container. " +
+					"Migrating one means pulling the new image and recreating the container, which has to be done by hand; " +
+					"`rehearse` still works and will tell you what the migration involves",
+			}, string(kind)
+		}
 		status := StatusOK
 		if kind == DeploymentUnknown {
 			status = StatusWarn

internal/preflight/checks_test.go

@@ -450,3 +450,95 @@ func TestAdminAccountKindWithoutASnapshot(t *testing.T) {
 		t.Fatalf("status = %q, want a warning when there was nothing to look in", res.Status)
 	}
 }
+
+// withFakeDocker puts a `docker` on PATH that answers `inspect` successfully,
+// so DetectDeploymentKind sees a container without one being involved.
+//
+// It skips rather than fails if this host has a real stalwart systemd unit:
+// detection checks those paths first and would win, and a test that depends
+// on the host *not* having Stalwart installed is a test that fails for a
+// reason unrelated to what it is checking.
+func withFakeDocker(t *testing.T) {
+	t.Helper()
+	for _, p := range systemdUnitPaths {
+		if _, err := os.Stat(p); err == nil {
+			t.Skipf("host has %s, which detection prefers over docker", p)
+		}
+	}
+	dir := t.TempDir()
+	script := "#!/bin/sh\ncase \"$1\" in inspect) exit 0 ;; esac\nexit 1\n"
+	if err := os.WriteFile(filepath.Join(dir, "docker"), []byte(script), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH"))
+}
+
+// dockerPreflight runs a minimal but real preflight against a fake 0.15.5
+// install with a fake docker on PATH.
+func dockerPreflight(t *testing.T, advisory bool) Report {
+	t.Helper()
+	withFakeDocker(t)
+
+	counterPath := filepath.Join(t.TempDir(), "invocations")
+	binaryPath := writeFakeBinary(t, "0.15.5", counterPath)
+	configPath := filepath.Join(t.TempDir(), "config.toml")
+	if err := os.WriteFile(configPath, []byte("[server]\nhostname = \"mail.example.com\"\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	withFakeGithub(t, func(w http.ResponseWriter, r *http.Request) {
+		json.NewEncoder(w).Encode(Release{TagName: "v0.16.14"})
+	})
+	store := checkpoint.NewStore(t.TempDir())
+	rs, err := store.Create("", "latest")
+	if err != nil {
+		t.Fatal(err)
+	}
+	// ToolCheckAdvisory is on in both cases so the deployment flag is the
+	// only thing that varies: whether stalwart-cli happens to be installed
+	// on the machine running the tests is not what this is testing.
+	report, err := New(Options{
+		BinaryPath: binaryPath, ConfigPath: configPath, DataDir: t.TempDir(),
+		TargetVersion: "latest", ToolCheckAdvisory: true, DeploymentCheckAdvisory: advisory,
+	}).Run(context.Background(), store, rs)
+	if err != nil {
+		t.Fatalf("Run: %v", err)
+	}
+	return report
+}
+
+// A container has to be refused here, in preflight, and not later. Cutover
+// already refuses it -- but cutover runs after the service has been stopped,
+// so refusing there refuses with mail down, which turned an attempted
+// migration into an outage.
+func TestPreflightBlocksADockerDeployment(t *testing.T) {
+	report := dockerPreflight(t, false)
+	if !report.Blocking() {
+		t.Fatalf("expected a blocking report for a docker deployment, got:\n%s", report.String())
+	}
+	var found bool
+	for _, res := range report.Results {
+		if res.Name == "deployment-kind" {
+			found = true
+			if res.Status != StatusFail {
+				t.Errorf("deployment-kind status = %q, want %q", res.Status, StatusFail)
+			}
+			if !strings.Contains(res.Detail, "docker") {
+				t.Errorf("deployment-kind detail does not mention docker: %q", res.Detail)
+			}
+		}
+	}
+	if !found {
+		t.Fatalf("no deployment-kind result in report:\n%s", report.String())
+	}
+}
+
+// rehearse is read-only: it never stops anything and never cuts over, so it
+// has to keep working against a container. Telling an operator what the
+// migration involves is most useful precisely when the tool cannot do it for
+// them.
+func TestRehearseStillRunsAgainstADockerDeployment(t *testing.T) {
+	report := dockerPreflight(t, true)
+	if report.Blocking() {
+		t.Fatalf("advisory mode should not block on docker, got:\n%s", report.String())
+	}
+}