From 3adaee3bc6c4fa7c8e3390b4b20d0d035d6c29cd Mon Sep 17 00:00:00 2001 From: John Coffey Date: Mon, 24 Aug 2026 13:26:15 -0700 Subject: [PATCH] Stop a clean migration reporting domains it never lost The post-migration comparison had the two versions counting domains differently, and yesterday's wiring turned that into a gate: `run` would have failed a migration that lost nothing. The 0.15 side added every domain appearing in any account's address on top of the domain principals - the fallback's own comment says "if the instance has no explicit domain principals", but the loop ran unconditionally. The 0.16 side did the reverse, listing only domains some account calls its primary, discarding the full Domain list it had already fetched. An instance with three declared domains and accounts aliased across nine reported nine before and three after. INBUXA is exactly that shape, and this was the account/domain over-count noted as undiagnosed. Both sides now mean "the domains this server holds". A domain that still goes missing is reported as a warning rather than failing the run: what the two versions call a domain differs across this boundary in ways we have now been caught by once, and a missing account - which is compared with a local-part fallback and is what actually matters - still fails. Narrowing OK() also made String() return before printing the domain lines, so the new warning would have been silent. Caught by its own test. --- README.md | 7 +++- cmd/stalwart-migrate/report.go | 2 + internal/stalwartapi/management.go | 12 +++++- internal/stalwartapi/principal.go | 21 ++++++++--- internal/stalwartapi/principal_test.go | 52 ++++++++++++++++++++++++++ internal/validate/content_integrity.go | 27 ++++++++++--- internal/validate/live.go | 12 +++++- internal/validate/live_test.go | 44 ++++++++++++++++++---- internal/validate/report.go | 3 ++ 9 files changed, 156 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index 3a7e52f..cce5528 100644 --- a/README.md +++ b/README.md @@ -39,8 +39,11 @@ was down for **6 seconds** end to end. Plan the window around verification, not data volume. After cutover, `run` compares the migrated instance against the snapshot -preflight took, and fails the command if an account or a domain that existed -before is missing from it. The service is left running either way — by that +preflight took, and fails the command if an account that existed before is +missing from it. A domain that no longer appears is reported as a warning +rather than a failure: the two versions do not agree on what counts as a +domain — principals on one side, `Domain` objects on the other — and failing +a migration over that difference would abort runs that lost nothing. The service is left running either way — by that point the store has been migrated in place, so stopping it would not undo anything; your recovery point is the way back. `report ` prints the same finding again later. Where preflight had no admin URL to snapshot from, diff --git a/cmd/stalwart-migrate/report.go b/cmd/stalwart-migrate/report.go index 48d405b..be78594 100644 --- a/cmd/stalwart-migrate/report.go +++ b/cmd/stalwart-migrate/report.go @@ -76,6 +76,8 @@ func reportFromSteps(rs *checkpoint.RunState) validate.Report { status = validate.StatusFail case step.Verdict == string(validate.StatusSkip): status = validate.StatusSkip + case step.Verdict == string(validate.StatusWarn): + status = validate.StatusWarn case step.Status != checkpoint.StepDone: // Recorded but never completed: the run stopped partway. status = validate.StatusFail diff --git a/internal/stalwartapi/management.go b/internal/stalwartapi/management.go index 80165d2..f96c7b7 100644 --- a/internal/stalwartapi/management.go +++ b/internal/stalwartapi/management.go @@ -185,14 +185,22 @@ func (c *Client) accountSnapshotJMAP(ctx context.Context) (*Snapshot, error) { if err != nil { return nil, fmt.Errorf("stalwartapi: resolve domain names (an unresolved id would make every domain look missing): %w", err) } + // Every domain the server holds, not only those some account calls its + // primary. Deriving the list from accounts dropped alias domains and + // domains with no accounts, so comparing it against a "before" list + // that did include them reported losses that had not happened. domainSet := map[string]bool{} + for _, name := range domainNames { + if name != "" { + domainSet[name] = true + } + } for _, a := range accounts { if a.DomainID == "" { continue } if name, ok := domainNames[a.DomainID]; ok && name != "" { - domainSet[name] = true - continue + continue // already counted above } // Keep the raw id rather than dropping the domain entirely: a // domain that can't be named is still a domain that exists. diff --git a/internal/stalwartapi/principal.go b/internal/stalwartapi/principal.go index 18f07a1..24cbd94 100644 --- a/internal/stalwartapi/principal.go +++ b/internal/stalwartapi/principal.go @@ -180,12 +180,21 @@ func (c *Client) principalSnapshotREST(ctx context.Context) (*Snapshot, error) { domainSet[d.Name] = true } } - // Fall back to the domains implied by account addresses if the - // instance has no explicit domain principals. - for _, p := range individuals { - for _, email := range p.Emails { - if at := strings.LastIndex(email, "@"); at >= 0 && at+1 < len(email) { - domainSet[email[at+1:]] = true + // Fall back to the domains implied by account addresses only when the + // instance declares no domain principals at all. + // + // This loop used to run unconditionally, which quietly inflated the + // list with every alias domain. That matters because this snapshot is + // the "before" side of the post-migration comparison, and the 0.16 side + // reports the domains the server actually holds: an alias domain that + // was never its own principal would be present here, absent there, and + // reported as lost by a migration that lost nothing. + if len(domainSet) == 0 { + for _, p := range individuals { + for _, email := range p.Emails { + if at := strings.LastIndex(email, "@"); at >= 0 && at+1 < len(email) { + domainSet[email[at+1:]] = true + } } } } diff --git a/internal/stalwartapi/principal_test.go b/internal/stalwartapi/principal_test.go index 9357163..3e2f585 100644 --- a/internal/stalwartapi/principal_test.go +++ b/internal/stalwartapi/principal_test.go @@ -356,3 +356,55 @@ func TestTenantNamesReportsTenantPrincipals(t *testing.T) { t.Error("TenantNames returned nil without an error; want a (possibly empty) list") } } + +// The "before" side of the post-migration comparison. It used to add every +// domain appearing in any account's address on top of the domain principals, +// so an instance with three declared domains and accounts aliased across nine +// reported nine - and the 0.16 side, which lists the domains the server +// actually holds, then looked as though six had been lost by a migration that +// lost nothing. INBUXA is exactly that shape. +func TestRESTSnapshotDoesNotInflateDomainsWithAliases(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + typ := r.URL.Query().Get("types") + w.Header().Set("content-type", "application/json") + switch typ { + case "domain": + _, _ = w.Write([]byte(`{"data":{"items":[{"type":"domain","name":"example.org"}],"total":1}}`)) + default: + _, _ = w.Write([]byte(`{"data":{"items":[{"type":"individual","name":"ann@example.org",` + + `"emails":["ann@example.org","ann@alias.example","ann@other.example"],"usedQuota":1}],"total":1}}`)) + } + })) + defer srv.Close() + + snap, err := (&Client{BaseURL: srv.URL, Username: "admin", Password: "pw", HTTPClient: srv.Client()}).principalSnapshotREST(context.Background()) + if err != nil { + t.Fatalf("principalSnapshotREST: %v", err) + } + if len(snap.Domains) != 1 || snap.Domains[0] != "example.org" { + t.Fatalf("Domains = %v, want just the declared domain principal", snap.Domains) + } +} + +// The fallback still exists for an instance that declares none, where an +// address is the only evidence a domain is in use. +func TestRESTSnapshotFallsBackToAddressesWhenNoDomainPrincipals(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("content-type", "application/json") + if r.URL.Query().Get("types") == "domain" { + _, _ = w.Write([]byte(`{"data":{"items":[],"total":0}}`)) + return + } + _, _ = w.Write([]byte(`{"data":{"items":[{"type":"individual","name":"ann@example.org",` + + `"emails":["ann@example.org"],"usedQuota":1}],"total":1}}`)) + })) + defer srv.Close() + + snap, err := (&Client{BaseURL: srv.URL, Username: "admin", Password: "pw", HTTPClient: srv.Client()}).principalSnapshotREST(context.Background()) + if err != nil { + t.Fatalf("principalSnapshotREST: %v", err) + } + if len(snap.Domains) != 1 || snap.Domains[0] != "example.org" { + t.Fatalf("Domains = %v, want the domain implied by the address", snap.Domains) + } +} diff --git a/internal/validate/content_integrity.go b/internal/validate/content_integrity.go index 2787c17..719ad99 100644 --- a/internal/validate/content_integrity.go +++ b/internal/validate/content_integrity.go @@ -43,11 +43,25 @@ type ContentIntegrityResult struct { MessageCountsCompared bool // false when the source version could not report counts } -// OK reports whether everything this comparison was able to check matched. -// Read it together with MessageCountsCompared: OK with that false means -// "the directory survived", not "no mail was lost". +// OK reports whether everything that must match did: no account and no mail +// went missing. Read it together with MessageCountsCompared: OK with that +// false means "the directory survived", not "no mail was lost". +// +// Domains are deliberately not part of this. What the two versions call a +// domain differs across the 0.15/0.16 boundary — principals on one side, +// Domain objects on the other, with aliases and account-less domains +// counted differently — and we have already been caught once reporting a +// migration that lost nothing as having lost domains. A disagreement there +// is worth showing an operator; it is not worth failing a migration over, +// where a missing account is. func (r ContentIntegrityResult) OK() bool { - return len(r.MissingAccounts) == 0 && len(r.MessageCountMismatches) == 0 && len(r.MissingDomains) == 0 + return len(r.MissingAccounts) == 0 && len(r.MessageCountMismatches) == 0 +} + +// DomainsOK reports whether every domain seen before the migration is still +// listed after it. +func (r ContentIntegrityResult) DomainsOK() bool { + return len(r.MissingDomains) == 0 } func (r ContentIntegrityResult) String() string { @@ -59,7 +73,10 @@ func (r ContentIntegrityResult) String() string { "(this migration's source version reports no per-mailbox counts, so no-data-loss is NOT verified here - "+ "only that every account and domain survived)", r.AccountsChecked) } - if r.OK() { + // Everything below is a finding, so return early only when there is + // nothing at all to report - domains included, even though they no + // longer fail the run. A warning nobody can read is not a warning. + if r.OK() && r.DomainsOK() { if r.MessageCountsCompared { b.WriteString(", all message counts match") } diff --git a/internal/validate/live.go b/internal/validate/live.go index c347ca9..824bfa5 100644 --- a/internal/validate/live.go +++ b/internal/validate/live.go @@ -71,11 +71,16 @@ func RunLive(ctx context.Context, store *checkpoint.Store, rs *checkpoint.RunSta if err != nil { return checkpoint.StepOutcome{}, err } - if !r.OK() { + switch { + case !r.OK(): // Recorded as a completed step with a failing verdict rather // than an error: the comparison ran, and its answer is the // finding. An error here would read as "we could not look". return checkpoint.StepOutcome{Verdict: string(StatusFail), Detail: r.String()}, nil + case !r.DomainsOK(): + // The two versions disagree about what counts as a domain, so + // this is reported rather than treated as data loss. + return checkpoint.StepOutcome{Verdict: string(StatusWarn), Detail: r.String()}, nil } return checkpoint.StepOutcome{Detail: r.String()}, nil }) @@ -88,8 +93,11 @@ func RunLive(ctx context.Context, store *checkpoint.Store, rs *checkpoint.RunSta } status := StatusOK - if outcome.Verdict == string(StatusFail) { + switch outcome.Verdict { + case string(StatusFail): status = StatusFail + case string(StatusWarn): + status = StatusWarn } report.Results = append(report.Results, CheckResult{Name: "content-integrity", Status: status, Detail: outcome.Detail}) return report, nil diff --git a/internal/validate/live_test.go b/internal/validate/live_test.go index b632e6e..b965d7b 100644 --- a/internal/validate/live_test.go +++ b/internal/validate/live_test.go @@ -101,7 +101,40 @@ func TestRunLiveFailsWhenAnAccountIsMissing(t *testing.T) { } } -func TestRunLiveFailsWhenADomainIsMissing(t *testing.T) { +func TestRunLiveWarnsButDoesNotBlockWhenADomainIsMissing(t *testing.T) { + // What the two versions call a domain differs across the 0.15/0.16 + // boundary - principals on one side, Domain objects on the other, with + // aliases counted differently - and INBUXA's own before-list was + // inflated with alias domains that the after-list structurally cannot + // contain. Failing the migration on that would abort a run that lost + // nothing, so it is reported and not treated as data loss. + srv := fakeInstance(t, []string{"example.org"}, map[string]float64{"ann@example.org": 10}) + defer srv.Close() + + store, rs := newRun(t) + report, err := RunLive(context.Background(), store, rs, LiveOptions{ + AdminURL: srv.URL, AdminUser: "admin", AdminPassword: "pw", HTTPClient: srv.Client(), + Before: &checkpoint.PreflightSnapshot{ + Domains: []string{"example.org", "alias.example"}, + UsedQuota: map[string]int64{"ann@example.org": 1}, + }, + }) + if err != nil { + t.Fatalf("RunLive: %v", err) + } + if report.Blocking() { + t.Fatalf("a domain-only difference must not abort the migration, got: %s", report.String()) + } + if got := report.Results[0].Status; got != StatusWarn { + t.Fatalf("status = %q, want %q", got, StatusWarn) + } + if !strings.Contains(report.Results[0].Detail, "alias.example") { + t.Fatalf("the operator still needs to be told which domain, got %q", report.Results[0].Detail) + } +} + +func TestRunLiveStillBlocksWhenAnAccountAndADomainAreMissing(t *testing.T) { + // A lost account is a lost account, whatever the domain list says. srv := fakeInstance(t, []string{"example.org"}, map[string]float64{"ann@example.org": 10}) defer srv.Close() @@ -109,15 +142,12 @@ func TestRunLiveFailsWhenADomainIsMissing(t *testing.T) { report, _ := RunLive(context.Background(), store, rs, LiveOptions{ AdminURL: srv.URL, AdminUser: "admin", AdminPassword: "pw", HTTPClient: srv.Client(), Before: &checkpoint.PreflightSnapshot{ - Domains: []string{"example.org", "vanished.example"}, - UsedQuota: map[string]int64{"ann@example.org": 1}, + Domains: []string{"example.org", "alias.example"}, + UsedQuota: map[string]int64{"ann@example.org": 1, "bob@example.org": 2}, }, }) if !report.Blocking() { - t.Fatalf("a missing domain must block, got: %s", report.String()) - } - if !strings.Contains(report.Results[0].Detail, "vanished.example") { - t.Fatalf("the report should name the missing domain, got %q", report.Results[0].Detail) + t.Fatalf("a missing account must still block, got: %s", report.String()) } } diff --git a/internal/validate/report.go b/internal/validate/report.go index 7eda2e8..1523769 100644 --- a/internal/validate/report.go +++ b/internal/validate/report.go @@ -18,6 +18,9 @@ const ( // are different answers, and reporting the second as the first is the // failure mode ARCHITECTURE.md §4.7 warns about. StatusSkip Status = "skip" + // StatusWarn is a finding worth an operator's attention that is not + // worth failing a migration over. + StatusWarn Status = "warn" ) type CheckResult struct {