Fix two defects a production-clone dress rehearsal exposed

Streamed a clone of a production store into the smoke VM - 3.6 GB, 12,361
settings, 6 accounts across 9 domains - and migrated it 0.15.5 -> 0.16.14
with the tool's own phases. The migration succeeded. Two defects surfaced
that no smaller instance could have shown, plus one finding worth recording.

1. Account roles broke on production-shaped names. v0.16 stores an account
   as a local part plus a domain reference: a v0.15 account named
   "[email protected]" becomes name "john" with a domainId. The generator
   passed the full address and the server rejected it outright ("Invalid
   email local part"), failing the apply. The smoke instance used bare
   usernames - alice, bob - and never exercised this.

   Fixed to use the local part. And because local parts are unique only
   within a domain - [email protected] and [email protected] both become
   "postmaster" - an ambiguous one is now refused with a warning rather
   than risking an upsert that grants Admin to the wrong account. Verified
   on the clone: the one admin came out with roles {"@type": "Admin"} and
   the other five accounts untouched.

2. Cutover's health check conflated liveness with credentials. A config
   fallback-admin does not survive the migration - v0.16's config is a
   store pointer, so the old [authentication.fallback-admin] block simply
   ceases to exist - so the credentials supplied for the pre-migration
   instance came back 401 on the migrated one, and the check reported the
   service as never having answered. It had answered; it was up and serving
   on all ten ports. Liveness and credentials are now separate: any
   response proves the service is up, and credentials that stopped working
   are a warning that names this cause.

Also recorded: a failed apply leaves the store in bootstrap mode, where
only Bootstrap objects are accessible. A half-applied plan is not a
partially configured server but an unusable one.

Timing, which is the other reason to rehearse: the recovery-mode conversion
of that 3.6 GB store took 2 seconds. A migration window is dominated by
waiting and verification, not data volume.

No production data in this commit; fixtures use example.net and the shapes
involved.

internal/applyplan/roles.go

@@ -32,11 +32,27 @@ import (
 // own "@type" as well.
 func AccountRoleOperations(principals []backup.Principal) (ops []Operation, covered []string, warnings []string, err error) {
 	type change struct {
-		name    string
-		variant string
+		localPart string
+		domain    string
+		variant   string
+		source    string
 	}
 	var changes []change
 
+	// v0.16 stores an account as a local part plus a domain reference, not
+	// as the full address v0.15 uses for its principal name: a v0.15
+	// account named "[email protected]" becomes name "john" with a domainId.
+	// Passing the full address is rejected outright ("Invalid email local
+	// part"), which is how this was found - on a production clone, where
+	// accounts are named by address. The smoke instance used bare names and
+	// never exercised it.
+	localPartCount := map[string]int{}
+	for _, p := range principals {
+		if strings.EqualFold(p.Type, "individual") {
+			localPartCount[localPart(p)]++
+		}
+	}
+
 	for _, p := range principals {
 		if !strings.EqualFold(p.Type, "individual") {
 			continue // domains and groups don't carry these roles
@@ -50,30 +66,69 @@ func AccountRoleOperations(principals []backup.Principal) (ops []Operation, cove
 			// touch every account to no effect.
 			continue
 		}
-		changes = append(changes, change{name: p.Name, variant: variant})
+		local := localPart(p)
+		// Local parts are unique only within a domain, so an upsert matched
+		// on name alone could land on a different account that happens to
+		// share it - [email protected] and [email protected] both become
+		// "postmaster". Granting Admin to the wrong account is worse than
+		// granting it to none, so an ambiguous name is refused and
+		// reported rather than guessed at.
+		if localPartCount[local] > 1 {
+			warnings = append(warnings, fmt.Sprintf(
+				"account %q: %d accounts share the local part %q, so this role cannot be restored unambiguously - grant it by hand after migrating",
+				p.Name, localPartCount[local], local))
+			continue
+		}
+		changes = append(changes, change{localPart: local, domain: domainOf(p), variant: variant, source: p.Name})
 	}
-	sort.Slice(changes, func(i, j int) bool { return changes[i].name < changes[j].name })
+	sort.Slice(changes, func(i, j int) bool { return changes[i].localPart < changes[j].localPart })
 
 	for _, c := range changes {
+		value := map[string]any{
+			// Account is a multi-variant object: without its own @type the
+			// upsert is rejected outright.
+			"@type": "User",
+			"name":  c.localPart,
+			"roles": map[string]any{"@type": c.variant},
+		}
 		ops = append(ops, Operation{
 			Type:    "upsert",
 			Object:  "Account",
 			MatchOn: []string{"name"},
-			Value: map[string]map[string]any{
-				"role-" + c.name: {
-					// Account is a multi-variant object: without its own
-					// @type the upsert is rejected outright.
-					"@type": "User",
-					"name":  c.name,
-					"roles": map[string]any{"@type": c.variant},
-				},
-			},
+			Value:   map[string]map[string]any{"role-" + c.localPart: value},
 		})
-		covered = append(covered, "principal:"+c.name+":roles")
+		covered = append(covered, "principal:"+c.source+":roles")
 	}
 	return ops, covered, warnings, nil
 }
 
+// localPart is the account name v0.16 will hold: the part before "@" when
+// the v0.15 principal is named by address, otherwise the name as-is.
+func localPart(p backup.Principal) string {
+	name := p.Name
+	if name == "" && len(p.Emails) > 0 {
+		name = p.Emails[0]
+	}
+	if at := strings.Index(name, "@"); at > 0 {
+		return name[:at]
+	}
+	return name
+}
+
+// domainOf reports the domain an account belongs to, for messages.
+func domainOf(p backup.Principal) string {
+	name := p.Name
+	if at := strings.Index(name, "@"); at > 0 && at+1 < len(name) {
+		return name[at+1:]
+	}
+	if len(p.Emails) > 0 {
+		if at := strings.Index(p.Emails[0], "@"); at > 0 && at+1 < len(p.Emails[0]) {
+			return p.Emails[0][at+1:]
+		}
+	}
+	return ""
+}
+
 // roleVariant maps a v0.15 role list onto v0.16's x:UserRoles variant.
 //
 // v0.15 carries a list of role names; v0.16 carries one variant. Where an

internal/applyplan/roles_test.go

@@ -111,3 +111,53 @@ func TestAccountRolesReportsRolesItCannotMap(t *testing.T) {
 		t.Errorf("warnings = %v, want the unmappable role named", warnings)
 	}
 }
+
+// Found by a dress rehearsal against a clone of production, where accounts
+// are named by address. v0.16 stores an account as a local part plus a
+// domain reference, and rejects a full address outright ("Invalid email
+// local part"). The smoke instance used bare names and never exercised it.
+func TestAccountRolesUsesTheLocalPartOfAnEmailStyleName(t *testing.T) {
+	ops, _, warnings, err := AccountRoleOperations([]backup.Principal{
+		{Type: "individual", Name: "[email protected]", Emails: []string{"[email protected]"}, Roles: []string{"admin"}},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(ops) != 1 {
+		t.Fatalf("generated %d op(s), want 1", len(ops))
+	}
+	v := ops[0].Value["role-john"]
+	if v == nil {
+		t.Fatalf("operation not keyed by local part: %+v", ops[0].Value)
+	}
+	if v["name"] != "john" {
+		t.Errorf("name = %v, want the local part \"john\" - the full address is rejected by the server", v["name"])
+	}
+	if len(warnings) != 0 {
+		t.Errorf("unexpected warnings: %v", warnings)
+	}
+}
+
+// Local parts are unique only within a domain. Granting Admin to the wrong
+// account is worse than granting it to none.
+func TestAccountRolesRefusesAnAmbiguousLocalPart(t *testing.T) {
+	ops, covered, warnings, err := AccountRoleOperations([]backup.Principal{
+		{Type: "individual", Name: "[email protected]", Roles: []string{"admin"}},
+		{Type: "individual", Name: "[email protected]", Roles: []string{"user"}},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(ops) != 0 {
+		t.Errorf("generated %d op(s) for an ambiguous local part, want 0 - it could land on the wrong account", len(ops))
+	}
+	if len(covered) != 0 {
+		t.Errorf("covered = %v, want none", covered)
+	}
+	if len(warnings) != 1 || !strings.Contains(warnings[0], "share the local part") {
+		t.Errorf("warnings = %v, want one explaining the ambiguity", warnings)
+	}
+	if !strings.Contains(warnings[0], "by hand") {
+		t.Errorf("warning %q should tell the operator what to do instead", warnings[0])
+	}
+}

internal/cutover/cutover.go

@@ -340,9 +340,25 @@ func Run(ctx context.Context, store *checkpoint.Store, rs *checkpoint.RunState,
 			}, nil
 		}
 		client := newClient(opts)
-		if err := client.WaitForPing(ctx, healthTimeout); err != nil {
+		// Liveness first, and on its own. Any response - 401 included -
+		// proves the service is up and routing.
+		if err := client.WaitForResponse(ctx, healthTimeout); err != nil {
 			return checkpoint.StepOutcome{}, fmt.Errorf("the migrated service started but never answered at %s within %s: %w", opts.AdminURL, healthTimeout, err)
 		}
+		// Credentials are a separate question, and failing them is not a
+		// failed cutover. A config fallback-admin does not survive into
+		// v0.16 - its config is just a store pointer, so the old
+		// [authentication.fallback-admin] block is gone - so the
+		// credentials that worked before the migration routinely stop
+		// working after it, on an instance that is otherwise fine.
+		if err := client.Ping(ctx); err != nil {
+			return checkpoint.StepOutcome{
+				Verdict: string(StatusWarn),
+				Detail: fmt.Sprintf("migrated instance is up and answering at %s, but these admin credentials no longer work: %v. "+
+					"If they were a config fallback-admin, that does not survive the migration - v0.16 keeps its config in the store. "+
+					"Authenticate as an account that exists in the directory instead", opts.AdminURL, err),
+			}, nil
+		}
 		return checkpoint.StepOutcome{Detail: fmt.Sprintf("migrated instance answered an authenticated JMAP session request at %s", opts.AdminURL)}, nil
 	}); err != nil {
 		return report, err

internal/cutover/cutover_test.go

@@ -231,16 +231,13 @@ func TestRunResumesWithoutRedoingCompletedSteps(t *testing.T) {
 
 func TestRunFailsWhenTheMigratedServiceNeverAnswers(t *testing.T) {
 	store, rs, opts := migratedRun(t)
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(http.StatusBadGateway)
-	}))
-	defer srv.Close()
-	opts.AdminURL = srv.URL
+	// Nothing listening at all: a closed port, not an error response.
+	opts.AdminURL = "http://127.0.0.1:1"
 	opts.HealthTimeout = 300 * time.Millisecond
 
 	report, err := Run(context.Background(), store, rs, opts)
 	if err == nil {
-		t.Fatal("Run: want failure when the started service never answers, got nil")
+		t.Fatal("Run: want failure when nothing answers at all, got nil")
 	}
 	if !strings.Contains(err.Error(), "never answered") {
 		t.Errorf("error %q should distinguish 'started but not answering' from 'failed to start'", err)
@@ -250,6 +247,37 @@ func TestRunFailsWhenTheMigratedServiceNeverAnswers(t *testing.T) {
 	}
 }
 
+// A 401 proves the service is up and routing. Treating it as unhealthy
+// failed a cutover that had actually succeeded: the credentials supplied
+// for the pre-migration instance were a config fallback-admin, which does
+// not survive into v0.16.
+func TestRunWarnsRatherThanFailsWhenCredentialsStopWorking(t *testing.T) {
+	store, rs, opts := migratedRun(t)
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusUnauthorized)
+	}))
+	defer srv.Close()
+	opts.AdminURL = srv.URL
+	opts.HealthTimeout = 2 * time.Second
+
+	report, err := Run(context.Background(), store, rs, opts)
+	if err != nil {
+		t.Fatalf("a service that is up but rejects these credentials is not a failed cutover: %v\n%s", err, report)
+	}
+	var warned bool
+	for _, res := range report.Results {
+		if res.Name == "wait-healthy" {
+			warned = res.Status == StatusWarn
+			if !strings.Contains(res.Detail, "fallback-admin") {
+				t.Errorf("warning %q should name the likely cause", res.Detail)
+			}
+		}
+	}
+	if !warned {
+		t.Errorf("wait-healthy should warn, not fail or pass silently:\n%s", report)
+	}
+}
+
 // Rolling back a migration that completed successfully, because a counter
 // didn't get rebuilt, would be worse than a stale counter.
 func TestRunWarnsRatherThanFailsWhenQuotaRecalculationFails(t *testing.T) {

internal/stalwartapi/task.go

@@ -7,6 +7,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"net/http"
 	"sort"
 	"strings"
 	"time"
@@ -287,3 +288,42 @@ func (c *Client) WaitForPing(ctx context.Context, timeout time.Duration) error {
 		}
 	}
 }
+
+// WaitForResponse polls until the instance answers an HTTP request at all,
+// whatever the status, or timeout elapses.
+//
+// This is the liveness question, and it is deliberately separate from
+// WaitForPing's "and my credentials work". A 401 proves the server is up,
+// listening and routing - which is exactly what a caller waiting for a
+// restarted service needs to know. Conflating the two failed a cutover
+// that had in fact succeeded: the credentials supplied for the
+// pre-migration instance were a config fallback-admin, which does not
+// survive into v0.16 (its config is a store pointer, so the old
+// [authentication.fallback-admin] block is simply gone), so every poll came
+// back 401 and the phase reported the service as never having answered.
+func (c *Client) WaitForResponse(ctx context.Context, timeout time.Duration) error {
+	url := strings.TrimRight(c.BaseURL, "/") + "/.well-known/jmap"
+	deadline := time.Now().Add(timeout)
+	var lastErr error
+	for {
+		req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
+		if err != nil {
+			return err
+		}
+		req.SetBasicAuth(c.Username, c.Password)
+		resp, err := c.httpClient().Do(req)
+		if err == nil {
+			resp.Body.Close()
+			return nil
+		}
+		lastErr = err
+		if !time.Now().Before(deadline) {
+			return fmt.Errorf("stalwartapi: %s did not respond within %s: %w", url, timeout, lastErr)
+		}
+		select {
+		case <-ctx.Done():
+			return ctx.Err()
+		case <-time.After(500 * time.Millisecond):
+		}
+	}
+}