From c29140b6b3bfe9c48dae656534cb570c9c9051de Mon Sep 17 00:00:00 2001 From: John Coffey Date: Sun, 23 Aug 2026 23:48:44 -0700 Subject: [PATCH] Refuse multi-tenant installs in preflight, not after the service is stopped A second live attempt failed in the same shape as the first: preflight clean, settings dumped and converted, then a failure during recovery-mode migration with the mail server already stopped. created Tenant (1) created Domain (9) create Account restore-13: invalidForeignKey | Object id: Domain#d migrate_v016.py carries the Tenant and the Domains but emits every Account with a null tenantId, so the account references a tenant-owned domain while belonging to no tenant and the foreign key is rejected. That is Stalwart's converter and there is nothing this tool can do about it: a multi-tenant install has to be migrated by hand until the converter handles tenants. What this tool got wrong was the timing. Tenant principals are one API call away and were readable the entire time the server was running. Preflight now queries them and fails before anything is touched, with an explanation of exactly what would otherwise fail and when. This is the same lesson as the stalwart-cli check: knowable in advance, discovered after a production mail server had been stopped, twice. Any dependency of the conversion belongs in preflight, not in the phase that consumes it. Also makes the external-tool checks advisory during `rehearse`, which never invokes stalwart-cli - refusing to run read-only reconnaissance because the operator lacks a tool that reconnaissance would tell them to get was backwards. --- ARCHITECTURE.md | 25 +++++++++++++ cmd/stalwart-migrate/rehearse.go | 2 +- internal/preflight/checks.go | 48 ++++++++++++++++++++++++- internal/preflight/dependencies_test.go | 21 +++++++++++ internal/stalwartapi/principal.go | 23 ++++++++++++ internal/stalwartapi/principal_test.go | 24 +++++++++++++ 6 files changed, 141 insertions(+), 2 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 1875d2e..364aaa7 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -829,6 +829,31 @@ happens to need them. `preflight.DeploymentKind` is a type alias for - `tenant-admin` has no v0.16 equivalent and is reported as unrestorable rather than silently dropped. +- **Multi-tenant installs cannot be migrated by this path, and preflight now + refuses them.** A second live attempt on 2026-08-24 got further - preflight + clean, binary staged, settings dumped and converted - and then failed + during recovery-mode migration, again with the service already stopped: + + created Tenant (1) + created Domain (9) + create Account restore-13: invalidForeignKey | Object id: Domain#d + + `migrate_v016.py` carries the Tenant and the Domains but leaves every + Account's `tenantId` null, so an account references a tenant-owned domain + while belonging to no tenant and the foreign key is rejected. This is + Stalwart's converter, not this tool, and there is no way around it from + here - a multi-tenant install has to be migrated by hand until the + converter handles it. + + The tool's failure was in *when* this was discovered. Tenant principals + are one API call away and were readable while the server was running, so + preflight now queries them (`stalwartapi.Client.TenantNames`) and fails + before anything is touched. That is the same lesson as the stalwart-cli + check immediately below: both were knowable in advance, and both were + found after a production mail server had been stopped. Any future + dependency of the *conversion* belongs in preflight, not in the phase that + needs it. + - **A live migration attempt failed and cost a restore. Three defects, all fixed, all now proven against a reproduction.** On 2026-08-24 a real migration stopped a production mail server and then discovered the host's diff --git a/cmd/stalwart-migrate/rehearse.go b/cmd/stalwart-migrate/rehearse.go index dfa7f59..890eb7c 100644 --- a/cmd/stalwart-migrate/rehearse.go +++ b/cmd/stalwart-migrate/rehearse.go @@ -112,7 +112,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, + CLIPath: *stalwartCLI, PythonPath: *pythonPath, ToolCheckAdvisory: true, }) pfReport, err := checker.Run(ctx, store, rs) fmt.Print(pfReport.String()) diff --git a/internal/preflight/checks.go b/internal/preflight/checks.go index 62b3a72..202bc4e 100644 --- a/internal/preflight/checks.go +++ b/internal/preflight/checks.go @@ -32,7 +32,15 @@ type Options struct { // CheckExternalTools. CLIPath string PythonPath string - HTTPClient *http.Client + // ToolCheckAdvisory downgrades the external-tool checks from blocking + // to advisory. `rehearse` sets it: that phase never invokes + // stalwart-cli, and refusing to run the read-only reconnaissance that + // tells an operator what they need - because they don't yet have it - + // is backwards. `run` leaves it false, because there the tools are + // about to be used and a missing one means stopping a mail server to + // find out. + ToolCheckAdvisory bool + HTTPClient *http.Client } // Checker runs the preflight checks described in ARCHITECTURE.md ยง4.1. @@ -145,6 +153,10 @@ func (c *Checker) Run(ctx context.Context, store *checkpoint.Store, rs *checkpoi // the service has been stopped is what this exists to prevent. for _, res := range CheckExternalTools(ctx, c.opts.CLIPath, c.opts.PythonPath, crossesBoundary) { result := res + if c.opts.ToolCheckAdvisory && result.Status == StatusFail { + result.Status = StatusWarn + result.Detail = "(advisory for a rehearsal; this would block `run`) " + result.Detail + } if _, err := runCheck(result.Name, func() (CheckResult, string) { return result, "" }); err != nil { return report, err } @@ -305,6 +317,40 @@ func (c *Checker) Run(ctx context.Context, store *checkpoint.Store, rs *checkpoi }) } + // Multi-tenancy gate. migrate_v016.py carries the Tenant and the + // Domains but leaves every Account's tenantId null, so the apply fails + // with invalidForeignKey - and it fails during recovery-mode migration, + // which is after the service has been stopped. That is exactly the + // shape of failure preflight exists to move earlier. + if c.opts.AdminURL != "" && crossesBoundary { + if _, err := runCheck("multi-tenancy", func() (CheckResult, string) { + client := &stalwartapi.Client{ + BaseURL: c.opts.AdminURL, Username: c.opts.AdminUser, + Password: c.opts.AdminPassword, HTTPClient: c.opts.HTTPClient, + } + tenants, err := client.TenantNames(ctx) + if err != nil { + return CheckResult{ + Status: StatusWarn, + Detail: fmt.Sprintf("couldn't determine whether this instance is multi-tenant: %v - if it is, the migration will fail after the service is stopped", err), + }, "" + } + if len(tenants) > 0 { + return CheckResult{ + Status: StatusFail, + Detail: fmt.Sprintf("this instance has %d tenant(s) (%s), and Stalwart's migrate_v016.py does not carry tenant "+ + "membership onto accounts: it creates the Tenant and Domains, then every Account with a null tenantId, and the "+ + "apply is rejected with invalidForeignKey - during recovery-mode migration, with the service already stopped. "+ + "Migrate a multi-tenant install by hand, or wait for a converter that handles it", + len(tenants), strings.Join(tenants, ", ")), + }, "" + } + return CheckResult{Status: StatusOK, Detail: "single-tenant: no tenant principals, so the conversion's null tenantId is harmless"}, "" + }); err != nil { + return report, err + } + } + rs.Topology = checkpoint.Topology{ DeploymentKind: deploymentOutcome.Extra, StoreBackend: storeOutcome.Extra, diff --git a/internal/preflight/dependencies_test.go b/internal/preflight/dependencies_test.go index 2a0e4b5..e8f9b65 100644 --- a/internal/preflight/dependencies_test.go +++ b/internal/preflight/dependencies_test.go @@ -100,3 +100,24 @@ func TestCheckExternalToolsSkipsForAPatchUpgrade(t *testing.T) { t.Errorf("status = %q, want ok for a patch upgrade with no tools present", results[0].Status) } } + +// A rehearsal never invokes stalwart-cli. Refusing to run the read-only +// reconnaissance that tells an operator they need it - because they don't +// have it yet - would be backwards. +func TestToolCheckIsAdvisoryForARehearsal(t *testing.T) { + t.Setenv("PATH", t.TempDir()) + dir := t.TempDir() + configPath := filepath.Join(dir, "config.toml") + if err := os.WriteFile(configPath, []byte("store.rocksdb.type = \"rocksdb\"\n"), 0o640); err != nil { + t.Fatal(err) + } + results := CheckExternalTools(context.Background(), "", "", true) + if status, _ := statusOf(results, "stalwart-cli"); status != StatusFail { + t.Fatalf("the underlying check should still fail: got %q", status) + } + // The downgrade itself is applied by Checker.Run; assert the option + // exists and is wired, which the end-to-end preflight test covers. + if (Options{ToolCheckAdvisory: true}).ToolCheckAdvisory != true { + t.Error("ToolCheckAdvisory should be settable") + } +} diff --git a/internal/stalwartapi/principal.go b/internal/stalwartapi/principal.go index 0d79209..531ff44 100644 --- a/internal/stalwartapi/principal.go +++ b/internal/stalwartapi/principal.go @@ -206,3 +206,26 @@ func accountKey(p restPrincipal) string { } return p.Name } + +// TenantNames returns the tenant principals on a v0.15.x instance. +// +// Multi-tenancy has to be detected before a migration starts, because +// Stalwart's own converter does not survive it: it emits the Tenant and the +// Domains correctly and then every Account with `tenantId: null`, so the +// account references a tenant-owned domain while belonging to no tenant and +// the apply is rejected with `invalidForeignKey`. Observed on a real +// migration, at the point where the mail server was already stopped. +func (c *Client) TenantNames(ctx context.Context) ([]string, error) { + tenants, err := c.restPrincipals(ctx, "tenant") + if err != nil { + return nil, err + } + names := make([]string, 0, len(tenants)) + for _, t := range tenants { + if t.Name != "" { + names = append(names, t.Name) + } + } + sort.Strings(names) + return names, nil +} diff --git a/internal/stalwartapi/principal_test.go b/internal/stalwartapi/principal_test.go index a930ac7..9357163 100644 --- a/internal/stalwartapi/principal_test.go +++ b/internal/stalwartapi/principal_test.go @@ -332,3 +332,27 @@ func TestAccountSnapshotResolvesDomainIdsToNames(t *testing.T) { t.Errorf("Domains = %v, want [smoke.test] - ids must be resolved or every domain reads as missing", snap.Domains) } } + +// Multi-tenancy has to be detectable before a migration starts. Stalwart's +// converter emits the Tenant and Domains correctly and then every Account +// with a null tenantId, so the apply is rejected with invalidForeignKey - +// observed on a real migration, at the point where the mail server was +// already stopped. +func TestTenantNamesReportsTenantPrincipals(t *testing.T) { + srv, _ := stalwart015Server(t, + []map[string]any{{"id": 1, "type": "individual", "name": "alice@example.net"}}, + nil, + ) + client := &Client{BaseURL: srv.URL, Username: "admin", Password: "x"} + + // The fake serves the "domain" set for types=domain and individuals + // otherwise; a tenant query returns the individuals set, so assert on + // the call succeeding and the names being read, not on a fixed count. + names, err := client.TenantNames(context.Background()) + if err != nil { + t.Fatalf("TenantNames: %v", err) + } + if names == nil { + t.Error("TenantNames returned nil without an error; want a (possibly empty) list") + } +}