From a69f0bbff1f52a9f8e38607ee0c634b378e1b260 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Sun, 23 Aug 2026 21:58:53 -0700 Subject: [PATCH] Fix two preflight defects a live production rehearsal found Ran the rehearsal read-only against a live production instance. It completed, and two preflight checks were wrong in ways a test instance could not have shown. 1. Store-backend detection missed the config entirely. A config generated by `stalwart --init` declares `type = "rocksdb"` inside a `[store.rocksdb]` section, which is what every test fixture here used. The production config has no section headers at all and declares `store.rocksdb.type = "rocksdb"` flat. Detection only matched a bare `type` key, so it reported "no known store backend type found in config" and left topology.store_backend empty. That mattered more than a warning suggests: backup.Run treated an unrecognized backend as a *skip*, so a real run would have continued with no filesystem or database backup at all - the single artifact that phase exists to produce, quietly absent. Flat dotted keys are now detected, and an unrecognized backend is a hard failure rather than a skip. 2. The cluster warning didn't say where it matched. It fires on any occurrence of "cluster" anywhere in the config, which is the correct bias - a missed cluster corrupts a shared store - but on the production config the only match was inside the value of an unrelated setting, leaving a whole config to search to establish that. It now names the location and distinguishes a match in the setting name from one in its value. Both verified against the real config: store-backend now reports "rocksdb (store.rocksdb)", and the cluster warning names the setting, making a false positive dismissible at a glance. No production data is in this commit: the fixtures use example.com and the flat-key shape only. Coverage numbers from that run matched the earlier scrubbed-corpus measurement exactly. --- ARCHITECTURE.md | 27 ++++++++++++ internal/backup/backup.go | 17 ++++++-- internal/preflight/checks.go | 18 +++++--- internal/preflight/cluster.go | 55 ++++++++++++++++++++++++- internal/preflight/cluster_test.go | 26 ++++++++++++ internal/preflight/storebackend.go | 19 ++++++++- internal/preflight/storebackend_test.go | 50 ++++++++++++++++++++++ 7 files changed, 199 insertions(+), 13 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 46245a1..99dec3c 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -698,6 +698,33 @@ happens to need them. `preflight.DeploymentKind` is a type alias for it reachable, but the migrated instance refused the call - below), **systemd drop-in** handling, and anything on a **non-RocksDB backend** or a **Docker** deployment. +- **Rehearsal has been run against a live production instance**, read-only, + and found two preflight defects a test instance could not have: + + - **Store-backend detection missed a flat config entirely.** A config + generated by `stalwart --init` declares `type = "rocksdb"` inside a + `[store.rocksdb]` section; the production config has *no section + headers at all* and declares `store.rocksdb.type = "rocksdb"` flat. + Detection checked only for a bare `type` key, so it found nothing and + left `topology.store_backend` empty - and `backup.Run` treated an + unrecognized backend as a *skip*, meaning a real run would have + proceeded with no filesystem backup whatsoever. Both fixed: flat keys + are detected, and an unrecognized backend is now a hard failure, + because the one artifact this phase exists to produce must not be + quietly absent. + - **The cluster warning didn't say where it matched.** It fires on any + occurrence of "cluster" anywhere in the config, which is the right bias + - a missed cluster is the dangerous direction - but on the production + config the sole match was inside the *value* of an unrelated setting. + The warning now names the location and says whether the match was the + setting or only its value, which turns a config-wide search into a + glance. + + Also confirmed there: the HTTPS path works against a real certificate, + and the coverage numbers match the earlier scrubbed-corpus measurement + exactly (12,182 unmigrated). Account enumeration past one page is still + untested - that instance has six accounts, not the hundred-plus the + pagination loop exists for. - **A migrated instance has no working administrator - diagnosed and fixed.** `migrate_v016.py` assigns every migrated account the `User` role regardless of what it held before, so an account that was an diff --git a/internal/backup/backup.go b/internal/backup/backup.go index 8bf98ce..91bc19e 100644 --- a/internal/backup/backup.go +++ b/internal/backup/backup.go @@ -184,10 +184,19 @@ func Run(ctx context.Context, store *checkpoint.Store, rs *checkpoint.RunState, } default: - report.Results = append(report.Results, CheckResult{ - Name: "backend-backup", Status: StatusSkipped, - Detail: fmt.Sprintf("no known store backend recorded for this run (topology.store_backend=%q) - preflight must run first, or the backend wasn't recognized; no filesystem/DB backup was taken", rs.Topology.StoreBackend), - }) + // Not a skip. An unrecognized backend used to be reported as + // "skipped" and the run continued with no filesystem or database + // backup at all - the one artifact this phase exists to produce, + // quietly absent. It is reachable in practice: preflight failed to + // detect the backend of a real production instance because its + // config declares it with flat dotted keys, and the run would have + // proceeded backup-less. + err := fmt.Errorf("backup: no recognized store backend for this run (topology.store_backend=%q) - "+ + "refusing to continue without a filesystem or database backup. Run preflight first; if it also can't "+ + "identify the backend, the config layout isn't one this tool recognizes and the backup must be taken by hand", + rs.Topology.StoreBackend) + report.Results = append(report.Results, CheckResult{Name: "backend-backup", Status: StatusFail, Detail: err.Error()}) + return report, err } if _, err := step("settings-dump", func() (checkpoint.StepOutcome, error) { diff --git a/internal/preflight/checks.go b/internal/preflight/checks.go index 24ae2af..367c023 100644 --- a/internal/preflight/checks.go +++ b/internal/preflight/checks.go @@ -166,15 +166,21 @@ func (c *Checker) Run(ctx context.Context, store *checkpoint.Store, rs *checkpoi } if _, err := runCheck("cluster-config", func() (CheckResult, string) { - clustered, err := LooksClustered(c.opts.ConfigPath) + mentions, err := ClusterMentions(c.opts.ConfigPath) if err != nil { return CheckResult{Status: StatusFail, Detail: err.Error()}, "" } - if clustered { - return CheckResult{ - Status: StatusWarn, - Detail: "config mentions clustering - confirm every peer node is stopped before this run proceeds; the tool does not verify this for you", - }, "" + if len(mentions) > 0 { + shown := mentions + if len(shown) > 5 { + shown = shown[:5] + } + detail := fmt.Sprintf("config mentions clustering at %s", strings.Join(shown, ", ")) + if len(mentions) > len(shown) { + detail += fmt.Sprintf(" (and %d more)", len(mentions)-len(shown)) + } + detail += " - confirm every peer node is stopped before this run proceeds; the tool does not verify this for you" + return CheckResult{Status: StatusWarn, Detail: detail}, "" } return CheckResult{Status: StatusOK, Detail: "no cluster configuration detected"}, "" }); err != nil { diff --git a/internal/preflight/cluster.go b/internal/preflight/cluster.go index dd7cc35..8e5cfb5 100644 --- a/internal/preflight/cluster.go +++ b/internal/preflight/cluster.go @@ -17,9 +17,60 @@ import ( // false negative is the dangerous direction, so this errs toward matching // broadly rather than requiring an exact schema match. func LooksClustered(configPath string) (bool, error) { + locations, err := ClusterMentions(configPath) + if err != nil { + return false, err + } + return len(locations) > 0, nil +} + +// ClusterMentions returns the setting keys whose name or value mentions +// clustering, so a warning can say *where* it matched. +// +// The matching stays deliberately broad - a missed cluster is the dangerous +// direction - but a bare "config mentions clustering" leaves an operator +// with a whole config to search. Against a real instance the single match +// was inside the value of an unrelated key, which is obvious in one glance +// when the warning names it and needs investigation when it doesn't. +func ClusterMentions(configPath string) ([]string, error) { data, err := os.ReadFile(configPath) if err != nil { - return false, fmt.Errorf("preflight: read config %s: %w", configPath, err) + return nil, fmt.Errorf("preflight: read config %s: %w", configPath, err) } - return strings.Contains(strings.ToLower(string(data)), "cluster"), nil + var locations []string + section := "" + for _, raw := range strings.Split(string(data), "\n") { + line := strings.TrimSpace(raw) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + if strings.HasPrefix(line, "[") && strings.HasSuffix(line, "]") { + section = strings.Trim(line, "[]") + // A [cluster] header is itself the declaration; don't let + // recording the section swallow the match. + if strings.Contains(strings.ToLower(section), "cluster") { + locations = append(locations, "["+section+"]") + } + continue + } + if !strings.Contains(strings.ToLower(line), "cluster") { + continue + } + key := line + if eq := strings.Index(line, "="); eq >= 0 { + key = strings.TrimSpace(line[:eq]) + } + where := key + if section != "" { + where = section + "." + key + } + // Say whether it was the setting itself or only its value, since + // that is the difference between "this is clustered" and "this + // mentions the word". + if !strings.Contains(strings.ToLower(key), "cluster") { + where += " (in its value, not the setting name)" + } + locations = append(locations, where) + } + return locations, nil } diff --git a/internal/preflight/cluster_test.go b/internal/preflight/cluster_test.go index 6dc2b8e..5235f0c 100644 --- a/internal/preflight/cluster_test.go +++ b/internal/preflight/cluster_test.go @@ -6,6 +6,7 @@ package preflight import ( "os" "path/filepath" + "strings" "testing" ) @@ -35,3 +36,28 @@ func TestLooksClustered(t *testing.T) { }) } } + +// Against a real instance the only match was inside the value of an +// unrelated setting. A bare "config mentions clustering" left a whole +// config to search; naming the location makes it dismissible at a glance. +func TestClusterMentionsSaysWhereItMatched(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.toml") + body := "server.hostname = \"mail.example.com\"\nconfig.local-keys.08 = \"cluster.*\"\n" + if err := os.WriteFile(path, []byte(body), 0o640); err != nil { + t.Fatal(err) + } + mentions, err := ClusterMentions(path) + if err != nil { + t.Fatal(err) + } + if len(mentions) != 1 { + t.Fatalf("mentions = %v, want one", mentions) + } + if !strings.Contains(mentions[0], "config.local-keys.08") { + t.Errorf("mention = %q, should name the setting", mentions[0]) + } + if !strings.Contains(mentions[0], "in its value") { + t.Errorf("mention = %q, should distinguish a value match from a real cluster setting", mentions[0]) + } +} diff --git a/internal/preflight/storebackend.go b/internal/preflight/storebackend.go index 48cdb5f..5e53f5a 100644 --- a/internal/preflight/storebackend.go +++ b/internal/preflight/storebackend.go @@ -67,8 +67,25 @@ func scanTOMLBackends(data []byte) ([]BackendMatch, error) { } if m := tomlKVRe.FindStringSubmatch(line); m != nil { key, value := m[1], strings.ToLower(m[2]) - if key == "type" && knownBackends[value] { + if !knownBackends[value] { + continue + } + // Two spellings, both real. A config written with sections + // declares `type = "rocksdb"` under `[store.rocksdb]`; one + // written flat declares `store.rocksdb.type = "rocksdb"` with + // no sections at all. A real production instance uses the + // second form exclusively - checking only for a bare `type` + // key found nothing there, and an undetected backend makes the + // backup phase skip the filesystem snapshot entirely. + switch { + case key == "type": matches = append(matches, BackendMatch{Path: section, Backend: value}) + case strings.HasSuffix(key, ".type"): + path := strings.TrimSuffix(key, ".type") + if section != "" { + path = section + "." + path + } + matches = append(matches, BackendMatch{Path: path, Backend: value}) } } } diff --git a/internal/preflight/storebackend_test.go b/internal/preflight/storebackend_test.go index 891e950..79bba40 100644 --- a/internal/preflight/storebackend_test.go +++ b/internal/preflight/storebackend_test.go @@ -87,3 +87,53 @@ func TestDetectStoreBackendsNoMatch(t *testing.T) { t.Errorf("got %d matches, want 0: %+v", len(matches), matches) } } + +// A real production config declares its backend with flat dotted keys and +// has no section headers at all. Checking only for a bare `type` key found +// nothing there, and an undetected backend makes the backup phase skip the +// filesystem snapshot - the artifact it exists to produce. +func TestDetectStoreBackendsHandlesFlatDottedKeys(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.toml") + // Shape taken from a real 0.15.5 instance: no [sections] anywhere. + body := `storage.blob = "rocksdb" +storage.data = "rocksdb" +storage.directory = "internal" +store.rocksdb.compression = "lz4" +store.rocksdb.path = "/opt/stalwart/data" +store.rocksdb.type = "rocksdb" +directory.internal.type = "internal" +` + if err := os.WriteFile(path, []byte(body), 0o640); err != nil { + t.Fatal(err) + } + matches, err := DetectStoreBackends(path) + if err != nil { + t.Fatal(err) + } + if len(matches) != 1 { + t.Fatalf("found %d backend(s), want 1: %+v", len(matches), matches) + } + if matches[0].Backend != "rocksdb" { + t.Errorf("backend = %q, want rocksdb", matches[0].Backend) + } + if matches[0].Path != "store.rocksdb" { + t.Errorf("path = %q, want store.rocksdb", matches[0].Path) + } +} + +// The sectioned form must keep working; both spellings are real. +func TestDetectStoreBackendsStillHandlesSections(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.toml") + if err := os.WriteFile(path, []byte("[store.rocksdb]\ntype = \"rocksdb\"\npath = \"/var/lib/stalwart\"\n"), 0o640); err != nil { + t.Fatal(err) + } + matches, err := DetectStoreBackends(path) + if err != nil { + t.Fatal(err) + } + if len(matches) != 1 || matches[0].Backend != "rocksdb" || matches[0].Path != "store.rocksdb" { + t.Errorf("matches = %+v, want one rocksdb at store.rocksdb", matches) + } +}